[PATCH] arm: stm32mp: Fix board_get_usable_ram_top() again
patrick.delaunay at foss.st.com
Fri Jan 6 13:16:31 CET 2023
On 1/5/23 20:16, Pali Rohar wrote:
> On Thursday 05 January 2023 11:03:07 Patrick DELAUNAY wrote:
>> 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_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 ?
> Well, I think that it is quite confusing when it is phys_addr_t and when
> phys_size_t. Because U-Boot on more places is doing addition:
> phys_addr_t + phys_size_t.
> And I was confused by it too, because get_effective_memsize() function
> works only with variables of phys_size_t type.
> So I would agree that it should be mentioned as phys_addr_t.
> But is there any platform on which phys_addr_t and phys_size_t types
> have different sizes?
I don't know, it is just more clear
=> addr overflow based on address type
> Why it is not ram_base? Because there is no such type. Why it is not
> phys_addr_t? Because some 32-bit platforms support more than 4GB of
> physical memory and unsigned long type is small for them. Also there are
> 64-bit platforms with just 32-bit data bus (but I'm not sure if these
> platforms are handled somehow specially with just 32-bit phys_addr_t).
>> why align on 4kB and not on MMU_SECTION_SIZE ?
> Because of my mistake and seems that nobody spotted or reported this
> issue yet. I agree it should be some platform specific macro (I have not
> checked now if MMU_SECTION_SIZE is the correct one).
I propose MMU_SECTION_SIZE to allow MMU configuration up to end
of effective ram top.
>> 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.size = get_effective_memsize();
> Yes, this really looks strange to reduce total size of DDR banks to the
> effective memory size which U-Boot can use or map. There is also config
> option CONFIG_MAX_MEM_MAPPED to allow user to reduce mappable memory in
> U-Boot to more strict value.
> For sure DDR bank size should be set to the real size.
> For example 32-bit U-Boot can map maximally 4GB of RAM, but on some
> (32-bit) platforms there can be more than 4GB of DDR and operating
> system may support to use more than 4GB of it. So DDR bank size passed
> from bootloader to OS should be precise and not reduced to size
> supported by bootloader.
>> 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 ?)
> You are missing there +1. That is restriction in the U-Boot as it
> requires that one byte after the RAM can be stored in both phys_addr_t
> and phys_size_t variables.
> Again, I wrote code comment slightly wrong. It should say that
> ram_base + get_effective_memsize() must be representable by phys_addr_t
For me it is not a problem, 0x0 for RAM TOP should be (max of
phys_addr_t + 1)
=> relocation address of U-Boot is (ram_top - u-boot size) is correctly
but perhaps I miss something....
and I dig deeper.
Anyway on STM32MP15X platform with 1GB DDR, for basic boot
ram_base = 0xC0000000
ram_size = 0x40000000
=> and it is working (with ram_top = 0x0)
>> + /*
>> + * 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
> For example U-Boot code in common/board_f.c sets gd->ram_top to value:
> gd->ram_top = gd->ram_base + get_effective_memsize();
> So your above change with -1 just break this code.
For me ram_top = 0x0 is functional ( for u32 equivalant with overflow to
0x1 0000 0000)
> I think that the best would be to fix U-Boot code so that it never use
> last byte +1 in phys_addr_t.
I see that over platform don't change this common part of U-Boot
but reduce the usable RAM by U-Boot by using the weak function:
common/board_f.c:376: gd->ram_top =
and if you see the default implementation of this function :
/* Get the top of usable RAM */
__weak phys_size_t gd->ram_base(phys_size_t total_size)
#if defined(CONFIG_SYS_SDRAM_BASE) && CONFIG_SYS_SDRAM_BASE > 0
* Detect whether we have so much RAM that it goes past the end of our
* 32-bit address space. If so, clip the usable RAM so it doesn't.
if (gd->ram_top < CONFIG_SYS_SDRAM_BASE)
* Will wrap back to top of 32-bit space when reservations
* are made.
the 32-bis address space overflow is already detected here when
gd->ram_top = gd->ram_base + gd->ram_size < CONFIG_SYS_SDRAM_BASE
=> ram_top = board_get_usable_ram_top()
So I think you can revert your patch....
and perhaps check your implementation of this weak function if you
More information about the U-Boot