[PATCH v3 17/27] lmb: do away with arch_lmb_reserve()

[Sughosh Ganu]

On Sat, 24 Aug 2024 at 02:17, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 21 Aug 2024 at 05:00, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > All of the current definitions of arch_lmb_reserve() are doing the
> > same thing -- reserve the region of memory occupied by U-Boot,
> > starting from the current stack address to the ram_top. Introduce a
> > function lmb_reserve_uboot_region() which does this, and do away with
> > the arch_lmb_reserve() function.
> >
> > Instead of using the current value of stack pointer for starting the
> > reserved region, have a fixed value, considering the stack size config
> > value.nit
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V2:
> > * Remove the definitions of arch_lmb_reserve() and
> >   arch_lmb_reserve_generic() functions.
> > * Introduce a new function lmb_reserve_uboot_region() and call it from
> >   lmb_reserve_common().
> >
> >  arch/arc/lib/cache.c        | 14 -------
> >  arch/arm/lib/stack.c        | 14 -------
> >  arch/m68k/lib/bootm.c       | 17 --------
> >  arch/microblaze/lib/bootm.c | 14 -------
> >  arch/mips/lib/bootm.c       | 15 -------
> >  arch/nios2/lib/bootm.c      | 13 ------
> >  arch/powerpc/lib/bootm.c    |  9 ----
> >  arch/riscv/lib/bootm.c      | 13 ------
> >  arch/sh/lib/bootm.c         | 13 ------
> >  arch/x86/lib/bootm.c        | 18 --------
> >  arch/xtensa/lib/bootm.c     | 13 ------
> >  include/lmb.h               |  2 -
> >  lib/lmb.c                   | 83 ++++++++++++++++++-------------------
> >  13 files changed, 41 insertions(+), 197 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> question below
>
> [..]
>
> > diff --git a/include/lmb.h b/include/lmb.h
> > index e6c6f1cdcd..6e435bd35c 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -108,8 +108,6 @@ void lmb_dump_all(void);
> >  void lmb_dump_all_force(void);
> >
> >  void board_lmb_reserve(void);
> > -void arch_lmb_reserve(void);
> > -void arch_lmb_reserve_generic(ulong sp, ulong end, ulong align);
> >
> >  #if CONFIG_IS_ENABLED(UNIT_TEST)
>
> There is no need to do this in the header file, BTW. It will cause a
> link error if someone uses it by mistake.

Okay

>
> >  struct lmb *lmb_get(void);
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index e26e329248..1116b2b868 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> [..]
> > @@ -215,10 +179,50 @@ static __maybe_unused int efi_lmb_reserve(void)
> >         return 0;
> >  }
> >
> > +static void lmb_reserve_uboot_region(void)
> > +{
> > +       int bank;
> > +       ulong end, bank_end;
> > +       phys_addr_t rsv_start;
> > +
> > +       rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
> > +       end = gd->ram_top;
> > +
> > +       /*
> > +        * Reserve memory from aligned address below the bottom of U-Boot stack
> > +        * until end of RAM area to prevent LMB from overwriting that memory.
> > +        */
> > +       debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
> > +
> > +       /* adjust sp by 4K to be safe */
> > +       rsv_start -= 16384;
>
> Should that be SZ_4K ?

It should be SZ_16K. This is using the highest value that was being
used by the different architectures. I will fix the comment. Thanks.

-sughosh

>
> > +       for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > +               if (!gd->bd->bi_dram[bank].size ||
> > +                   rsv_start < gd->bd->bi_dram[bank].start)
> > +                       continue;
> > +               /* Watch out for RAM at end of address space! */
> > +               bank_end = gd->bd->bi_dram[bank].start +
> > +                       gd->bd->bi_dram[bank].size - 1;
> > +               if (rsv_start > bank_end)
> > +                       continue;
> > +               if (bank_end > end)
> > +                       bank_end = end - 1;
> > +
> > +               lmb_reserve_flags(rsv_start, bank_end - rsv_start + 1,
> > +                                 LMB_NOOVERWRITE);
> > +
> > +               if (gd->flags & GD_FLG_SKIP_RELOC)
> > +                       lmb_reserve_flags((phys_addr_t)(uintptr_t)_start,
> > +                                   gd->mon_len, LMB_NOOVERWRITE);
> > +
> > +               break;
> > +       }
> > +}
> > +
> >  static void lmb_reserve_common(void *fdt_blob)
> >  {
> > -       arch_lmb_reserve();
> >         board_lmb_reserve();
> > +       lmb_reserve_uboot_region();
> >
> >         if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
> >                 boot_fdt_add_mem_rsv_regions(fdt_blob);
> > @@ -689,11 +693,6 @@ __weak void board_lmb_reserve(void)
> >         /* please define platform specific board_lmb_reserve() */
> >  }
> >
> > -__weak void arch_lmb_reserve(void)
> > -{
> > -       /* please define platform specific arch_lmb_reserve() */
> > -}
> > -
> >  static int lmb_setup(void)
> >  {
> >         bool ret;
> > --
> > 2.34.1
> >
>
> Regards,
> Simon