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

[Ilias Apalodimas]

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.

>
> > 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.

>
> > 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

Thanks
/Ilias
>
> Regards,
> Simon