[PATCH] arm: stm32mp: Fix board_get_usable_ram_top() again

[Patrick DELAUNAY]

Hi,

On 1/5/23 02:22, Marek Vasut wrote:
> Do not access gd->ram_size and assume this is actual valid RAM size. Since commit
> 777aaaa706b ("common/memsize.c: Fix get_effective_memsize() to check for overflow")
> the RAM size may be less than gd->ram_size , call get_effective_memsize() to get
> the limited value instead.
>
> The aforementioned commit makes STM32MP15xx boards with 1 GiB of DRAM
> at 0xc0000000 hang on boot, which is a grave defect.
>
> Signed-off-by: Marek Vasut <marex at denx.de>
> ---
> Cc: Pali Rohar <pali at kernel.org>
> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> Cc: Tom Rini <trini at konsulko.com>
> ---
>   arch/arm/mach-stm32mp/dram_init.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-stm32mp/dram_init.c b/arch/arm/mach-stm32mp/dram_init.c
> index 9346fa8546d..80ba5c27741 100644
> --- a/arch/arm/mach-stm32mp/dram_init.c
> +++ b/arch/arm/mach-stm32mp/dram_init.c
> @@ -51,7 +51,7 @@ phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>   
>   	/* found enough not-reserved memory to relocated U-Boot */
>   	lmb_init(&lmb);
> -	lmb_add(&lmb, gd->ram_base, gd->ram_size);
> +	lmb_add(&lmb, gd->ram_base, get_effective_memsize());
>   	boot_fdt_add_mem_rsv_regions(&lmb, (void *)gd->fdt_blob);
>   	/* add 8M for reserved memory for display, fdt, gd,... */
>   	size = ALIGN(SZ_8M + CONFIG_SYS_MALLOC_LEN + total_size, MMU_SECTION_SIZE),


Thanks for the patch

Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>


Even if I don't uderstood where the code blocked here...

I need to cros-check it.




But I think it is more a regression introduced by

777aaaa706b ("common/memsize.c: Fix get_effective_memsize() to check for overflow")


in this patch I don't understood the assumption:

* It is required that ram_base + ram_size must be representable by
* phys_size_t type and must be aligned by direct access


why add now this limitation ?

why phys_size_t here and not phys_addr_t or unsigned long, the ram_base type ?


why align on 4kB and not on MMU_SECTION_SIZE ?



until now on ARM 32bits U-Boot is working with

   ram_base = 0xC0000000 (unsigned long)
   ram_size = 0x40000000 (phys_size_t)

overflow for ram_top (phys_addr_t) = ram_base + get_effective_memsize() was previously managed in the code
as we work only on base address + size (in board_f.c, in lmb library)

I correct in the code overflow issues, for example in commit 54be09cd8f6e ("arm: caches: manage phys
addr_t overflow in mmu_set_region_dcache_behaviour").


moreover for me it is strange to reduce the size of the DDR banks by default
(see resutl of bdinfo command) only just to avoid overflow:

common/board_f.c:267:	gd->bd->bi_dram[0].size = get_effective_memsize();


For me the real assumption is on the end of RAM:

"ram_base + ram_size - 1" must be representable by 'ram_base' type
(phys_addr_t or unsigned long ?)



+
+	/*
+	 * Check for overflow and limit ram size.
+	 * It is required that ram_base + ram_size - 1 must be representable
+	 * by ram_base type, unsigned long.
+	 */
+	if (gd->ram_base - 1 + ram_size < gd->ram_base)
+		ram_size = ((unsigned long)~0x0ULL) - gd->ram_base;



=> no need to remove 4KB is here... base + size can reach the MAX value for the type + 1


Patrick