Re: [PATCH v1 2/2] board: verdin-am62: fix check for minimum memory size

[Francesco Dolcini <francesco@dolcini.it>]

On Thu, Aug 24, 2023 at 09:58:56AM -0500, Nishanth Menon wrote:
> On 16:37-20230824, Francesco Dolcini wrote:
> > Hello Nishanth,
> > 
> > On Thu, Aug 24, 2023 at 07:14:12AM -0500, Nishanth Menon wrote:
> > > On 10:08-20230824, Emanuele Ghidoli wrote:
> > > > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> > > > 
> > > > verdin am62 SKUs comes in multiple memory configuration, check that
> > > > the detected memory is at least 512MB since we have some
> > > > reserved memory just before this threshold and therefore
> > > > the module cannot work with less memory.
> > > > 
> > > > Fixes: 7d1a10659f5b ("board: toradex: add verdin am62 support")
> > > > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> > > > ---
> > > >  board/toradex/verdin-am62/verdin-am62.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/board/toradex/verdin-am62/verdin-am62.c b/board/toradex/verdin-am62/verdin-am62.c
> > > > index a3d1d07a0cbf..d09dda5bccc9 100644
> > > > --- a/board/toradex/verdin-am62/verdin-am62.c
> > > > +++ b/board/toradex/verdin-am62/verdin-am62.c
> > > > @@ -28,8 +28,8 @@ int dram_init(void)
> > > >  {
> > > >  	gd->ram_size = get_ram_size((long *)CFG_SYS_SDRAM_BASE, CFG_SYS_SDRAM_SIZE);
> > > >  
> > > > -	if (gd->ram_size < SZ_64M)
> > > > -		puts("## WARNING: Less than 64MB RAM detected\n");
> > > > +	if (gd->ram_size < SZ_512M)
> > > > +		puts("## WARNING: Less than 512MB RAM detected\n");
> > > 
> > > Have you considered fdtdec_setup_mem_size_base
> > > fdtdec_setup_memory_banksize  - in which case the reserved memory
> > > regions can be changed in dt (will need corresponding custom binaries
> > > ofcourse)..
> > 
> > I would say that custom binaries is not something we want. Our goal is
> > to rely on whatever TI is providing and to not deviate from it unless
> > required. Luckily enough for the moment it works since our
> > minimum memory size is 512MB.
> 
> Sure - you will still benefit using fdtdec_setup_mem_size_base and
> fdtdec_setup_memory_banksize, but really, your call.

What would be the actual benefit? Never used it and looking at the code
I do not think it works for us.

We have a range of SKUs with different memory sizes (512MB to 2GB), we
do have a single device tree and a single U-Boot binary for all of the
variants, the memory node in the source dts file is just a default.

The DT memory node is updated dynamically by U-Boot, and the actual memory
size is detected by U-Boot using get_ram_size().

(and all of that just works fine).

Francesco