[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Vikas MANOCHA]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren <swarren at wwwdotorg.org>
> Sent: Tuesday, August 27, 2019 10:55 AM
> To: Tom Rini <trini at konsulko.com>
> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> <swarren at nvidia.com>; Vikas MANOCHA <vikas.manocha at st.com>
> Subject: [PATCH] board_f: fix noncached reservation calculation
> 
> From: Stephen Warren <swarren at nvidia.com>
> 
> The current code in reserve_noncached() has two issues:
> 
> 1) 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.

> 
> 2) 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 at st.com>
> Signed-off-by: Stephen Warren <swarren at 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 of
> malloc_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 in
> cache.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);
>  	debug("Reserving %dM for noncached_alloc() at: %08lx\n",
>  	      CONFIG_SYS_NONCACHED_MEMORY >> 20, gd->start_addr_sp);
> 
> 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 in
> board_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