Thanks Frieder
On Wed, 18 Dec 2024 at 11:17, Frieder Schrempf
<frieder.schre...@kontron.de> wrote:
>
> On 18.12.24 10:00 AM, Ilias Apalodimas wrote:
> > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G
> > boundary")
> > tried to adjust the usable memory limits on a 4GB boundary.
>
> I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix
> board_get_usable_ram_top()") which actually introduces the value of
> 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag.
Ah thanks, Tom can you amend it or want me to send a v2?
>
> >
> > ram_top is described as 'top address of RAM used by U-Boot' and we want
> > to preserve that. This is defined as a phys_addr_t and unfortunately
> > its size differs across architectures. This has lead us to a weird
> > state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as
> > 'SZ_4GB' unless it was otherwise defined.
> >
> > With some recent LMB changes and specifically
> > commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from
> > same bank")
> > the board fails to boot properly although the commit above is correct
> > since it's making sure that no memory above ram_top is usable -- but
> > added to our memory map so EFI can hand it over to the booted OS.
> >
> > The reason for that is that during the LMB init we add all usable memory
> > in lmb_add_memory(). In that function any memory above ram_top gets added
> > as 'reserved' for LMB. With the current values tha's set to 0xFFFF_FFFF
> > for this board. Later LMB is trying to protect the memory area U-Boot lives
> > in with lmb_reserve_common(). The latter fails though since it tries to
> > add U-Boot top (which is 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call
> > will fail since 1 byte of that memory range is already marked as 'reserved'.
> >
> > Since we are close to the release, LMB seems to assume that the address
> > is rounded up and is the 'next address' and so does parsing and adding
> > memory ranges from DT files, bump the ram_top of the board by 1byte.
> >
> > In the long run we should change all of the above and have 32b and 64b
> > platforms define ram_top identically.
>
> Some more ideas for cleaning up the situation in the long run:
>
> * Get rid of board_get_usable_ram_top() and fix the drivers that require
> address space below SZ_4G.
>
> * Parse the 'dma-ranges' property in the SoC node of the devicetree
> which already tells us what address space is usable (at least for
> i.MX8M).
>
> Anyway, the change looks good to me for now, so:
Yep both of these are valid and good ideas.
>
> Reviewed-by: Frieder Schrempf <frieder.schre...@kontron.de>
Thanks
/Ilias
>
> >
> > Add a Fixes tag although the commit is correct, so people can figure out
> > the broken scenarios in the future.
> >
> > Suggested-by: Sughosh Ganu <sughosh.g...@linaro.org>
> > Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even
> > from same bank")
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > ---
> > arch/arm/mach-imx/imx8m/soc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
> > index 9588b8b28bf2..85dc8b51a145 100644
> > --- a/arch/arm/mach-imx/imx8m/soc.c
> > +++ b/arch/arm/mach-imx/imx8m/soc.c
> > @@ -362,7 +362,7 @@ phys_addr_t board_get_usable_ram_top(phys_size_t
> > total_size)
> > * space below the 4G address boundary (which is 3GiB big),
> > * even when the effective available memory is bigger.
> > */
> > - top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, 0xffffffff);
> > + top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, SZ_4G);
> >
> > /*
> > * rom_pointer[0] stores the TEE memory start address.
>
