[PATCH v5 5/7] common: Add an option to relocate on ram top

[Simon Glass]

Hi Ilias,

On Mon, 8 Jun 2026 at 01:44, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> >
> > I'm just a bit concerned about what this changes...see below.
> >
> > > diff --git a/common/board_f.c b/common/board_f.c
> > > @@ -339,7 +344,24 @@ static int setup_ram_base(void)
> > >  static int setup_ram_config(void)
> > >  {
> > >       debug("Monitor len: %08x\n", gd->mon_len);
> > > -#if CONFIG_VAL(SYS_MEM_TOP_HIDE)
> > > +
> > > +     if (CONFIG_IS_ENABLED(RELOC_ADDR_TOP)) {
> > > +             int i;
> > > +             phys_addr_t top;
> > > +
> > > +             gd->ram_size = 0;
> > > +             for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> > > +                     top = get_mem_top(gd->dram[i].start, gd->dram[i].size,
> > > +                                       ALIGN(gd->mon_len, SZ_1M),
> > > +                                       (void *)gd->fdt_blob);
> > > +                     gd->ram_top = max(top, gd->ram_top);
> > > +                     gd->ram_size += gd->dram[i].size;
> > > +             }
> > > +             gd->ram_top = board_get_usable_ram_top(gd->ram_size);
> > > +     } else {
> > > +             gd->ram_top = gd->ram_base + get_effective_memsize();
> > > +             gd->ram_top = board_get_usable_ram_top(gd->mon_len);
> > > +     }
> > >       /*
> > >        * Subtract specified amount of memory to hide so that it won't
> > >        * get 'touched' at all by U-Boot. By fixing up gd->ram_size
> > > @@ -350,10 +372,9 @@ static int setup_ram_config(void)
> > >        * memory size from the SDRAM controller setup will have to
> > >        * get fixed.
> > >        */
> > > +#if CONFIG_VAL(SYS_MEM_TOP_HIDE)
> > >       gd->ram_size -= CONFIG_SYS_MEM_TOP_HIDE;
> > >  #endif
> > > -     gd->ram_top = gd->ram_base + get_effective_memsize();
> > > -     gd->ram_top = board_get_usable_ram_top(gd->mon_len);
> >
> > This reorders SYS_MEM_TOP_HIDE relative to the ram_top calculation in
> > the non-RELOC_ADDR_TOP path. Previously gd->ram_size was reduced first
> > and get_effective_memsize() (which reads gd->ram_size) then yielded a
> > ram_top below the hidden region.
> >
> > After this patch ram_top is computed from the full size and the
> > subtraction happens afterwards, so ram_top (and hence relocaddr) now
> > lands inside the region SYS_MEM_TOP_HIDE is meant to keep U-Boot out
> > of
> >
> > Quite a few defconfigs still set CONFIG_SYS_MEM_TOP_HIDE (e.g. odroid)
> > and look like they would regress here. Please keep the old ordering
> > for the else branch, or do the subtraction up front and feed the
> > reduced size to both branches.
>
> Yea that's true, however how that's used is weird because
> get_effective_memsize() is board specific and it doesn't always use
> ram_size.
> So perhaps we should deduct the same value from ram_top and keep the
> call last ? That way we can keep the functionality for both cases.

That makes sense to me, yes. It seems more in keeping with
board-specific functions. This is likely another area to clean up one
day...

>
> >
> > > diff --git a/common/board_f.c b/common/board_f.c
> > > @@ -339,7 +344,24 @@ static int setup_ram_base(void)
> > > +             gd->ram_top = board_get_usable_ram_top(gd->ram_size);
> > > +     } else {
> > > +             gd->ram_top = gd->ram_base + get_effective_memsize();
> > > +             gd->ram_top = board_get_usable_ram_top(gd->mon_len);
> > > +     }
> >
> > The total_size argument to board_get_usable_ram_top() has always been
> > gd->mon_len in tree — that is what the ~40 board overrides expect
> > (they typically use it as the minimum headroom needed for the
> > relocated monitor). Passing gd->ram_size in the RELOC_ADDR_TOP path
> > silently changes the contract for any board that both overrides
> > board_get_usable_ram_top() and turns the new option on. I'd keep
> > mon_len here, or rename the parameter and check callers if this is
> > intended.
>
> That's another unclear option to me, since this function is also board
> specific. What I can do though since this is largely untested, is move
> the board_get_usable_ram_top() out of the if statements and call it
> with mon_len. In the future we can look at all the callsites and
> decide what's needed eventually.

Yes that seem safer.

>
> >
> > > diff --git a/common/board_f.c b/common/board_f.c
> > > @@ -339,7 +344,24 @@ static int setup_ram_base(void)
> > > +                     top = get_mem_top(gd->dram[i].start, gd->dram[i].size,
> > > +                                       ALIGN(gd->mon_len, SZ_1M),
> > > +                                       (void *)gd->fdt_blob);
> >
> > Perhaps a follow-up but we should be able to avoid the cast by making
> > the fdt argument const.
>
> Yea, but let's focus on the adding the functionality for now. There's
> a lot of cleanups that we can do once this is applied

Indeed!

Regards,
Simon