Hi Stephen,
-----Original Message----- From: Stephen Warren swarren@wwwdotorg.org Sent: Tuesday, August 27, 2019 10:55 AM To: Tom Rini trini@konsulko.com Cc: twarren@wwwdotorg.org; u-boot@lists.denx.de; Stephen Warren swarren@nvidia.com; Vikas MANOCHA vikas.manocha@st.com Subject: [PATCH] board_f: fix noncached reservation calculation
From: Stephen Warren swarren@nvidia.com
The current code in reserve_noncached() has two issues:
- The first update of gd->start_addr_sp always rounds down to a section
start. However, the equivalent calculation in cache.c:noncached_init() always first rounds up to a section start, then subtracts a section size. These two calculations differ if the initial value is already rounded to section alignment.
It shouldn't cause any issue, first one round down to section size. Second one(cache.c: noncached_init()) rounds up, so needs section size subtraction.
- The second update of gd->start_addr_sp subtracts exactly
CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent calculation in cache.c:noncached_init() rounds the noncached size up to section alignment before subtracting it. The two calculations differ if the noncached region size is not a multiple of the MMU section size.
Never thought CONFIG_SYS_NON_CACACHED_MEMORY could be non-multiple of MMU section size for basic MMU setup in u-boot. It has granularity of section size. Is it the case with Jetson TX1 ?
In practice, one/both of those issues causes a practical problem on Jetson TX1; U-Boot triggers a synchronous abort during initialization, likely due to overlapping use of some memory region.
This change fixes both these issues by duplicating the exact calculations from noncached_init() into reserve_noncached().
However, this fix assumes that gd->start_addr_sp on entry to reserve_noncached() exactly matches mem_malloc_start on entry to noncached_init(). I haven't traced the code to see whether it absolutely guarantees this in all (or indeed any!) cases. Consequently, I added some comments in the hope that this condition will continue to be true.
It is enforced it in the code, reserve_noncached is called from reserve_malloc() after malloc area reservation.
Fixes: 5f7adb5b1c02 ("board_f: reserve noncached space below malloc area") Cc: Vikas Manocha vikas.manocha@st.com Signed-off-by: Stephen Warren swarren@nvidia.com
arch/arm/lib/cache.c | 1 + common/board_f.c | 15 ++++++++++++--- common/board_r.c | 4 ++++ 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c index 449544d11cff..463d283cb768 100644 --- a/arch/arm/lib/cache.c +++ b/arch/arm/lib/cache.c @@ -77,6 +77,7 @@ void noncached_init(void) phys_addr_t start, end; size_t size;
- /* If this calculation changes, update board_f.c:reserve_noncached()
+*/ end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) - MMU_SECTION_SIZE; size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE); start = end - size; diff --git a/common/board_f.c b/common/board_f.c index 6867abc8e679..591f18f391e2 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -470,9 +470,18 @@ static int reserve_uboot(void) #ifdef CONFIG_SYS_NONCACHED_MEMORY static int reserve_noncached(void) {
- /* round down to SECTION SIZE (typicaly 1MB) limit */
- gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
- gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;
- /*
* The value of gd->start_addr_sp must match the value ofmalloc_start
* calculated in boatrd_f.c:initr_malloc(), which is passed to* board_r.c:mem_malloc_init() and then used by* cache.c:noncached_init()** These calculations must match the code incache.c:noncached_init()
*/- gd->start_addr_sp = ALIGN(gd->start_addr_sp, MMU_SECTION_SIZE)
MMU_SECTION_SIZE;- gd->start_addr_sp -= ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
MMU_SECTION_SIZE);diff --git a/common/board_r.c b/common/board_r.c index b7f68bba4a7e..d6fb5047a265 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -247,6 +247,10 @@ static int initr_malloc(void) gd->malloc_ptr / 1024); #endif /* The malloc area is immediately below the monitor copy in DRAM */
- /*
* This value MUST match the value of gd->start_addr_sp inboard_f.c:
* reserve_noncached().*/
minor cosmetic suggestion: gd->start_addr_sp is moving pointer, difficult to comprehend sometimes here, same is true for malloc area also, how about merging two comments like:
/* The malloc area is immediately below the monitor copy in DRAM followed by noncached */
Cheers, Vikas
malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN; mem_malloc_init((ulong)map_sysmem(malloc_start, TOTAL_MALLOC_LEN), TOTAL_MALLOC_LEN); -- 2.22.1