[PATCH] common/board_f: init malloc earlier

[Simon Glass]

Hi Caleb,

On Tue, 26 Nov 2024 at 05:22, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Simon,
>
> On 26/11/2024 01:32, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >> Currently the early malloc initialisation is done partially in
> >> board_init_f_init_reserve() (on arm64 at least), which configures
> >> gd->malloc_base. But it isn't actually usable until initf_malloc() is
> >> called which doesn't happen until after fdtdec_setup().
> >>
> >> This causes problems in a few scenarios:
> >>
> >> 1. when using MULTI_DTB_FIT as this needs a working malloc (especially
> >>    for compressed FIT).
> >
> > Hmmm, how does this work today?
>
> I honestly have no idea, I assume boards that make use of it do some
> custom board_f.

OK.

> >
> >> 2. Some platforms may need to allocate memory as part of memory map
> >>    initialisation (e.g. Qualcomm will need this to parse the memory map
> >>    from SMEM).
> >
> > That really needs to be tidied up. When does this fixup need to be
> > done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps
>
> It's necessary in order to figure out gd->ram_base/ram_size. We do it in
> board_fdt_blob_setup() so that we can support another use-case where
> U-Boot has an internal FDT (which it should use) but the memory layout
> is provided via an external FDT which is unavailable (although this
> usecase isn't enabled upstream yet).
>
> > we could introduce an event to do 'memory map' stuff?
>
> We could, but considering that on most platforms the pre-relocation
> malloc is fixed at build time and at a known location relative to U-Boot
> there is no reason for us to arbitrary decide that some codepaths at the
> start of board_f aren't allowed to use malloc().
>
> Just moving the malloc initialization earlier ensures malloc() is
> consistently available and greatly simplifies things.

I'd like to see a more generic solution to this problem...I think we
discussed this before?

To my eye it looks like if we called an event in setup_dest_addr() we
could allow ram_base to be set.

>
> fwiw, I had another variation of this patch which dropped initf_malloc()
> entirely and just set up gd->malloc_limit/malloc_ptr in
> board_init_f_init_reserve() since that's where we set malloc_base
> anyways. But after digging some more there seem to be quite a few other
> entrypoints into U-Boot that don't go through
> board_init_f_init_reserve() (e.g. sandbox, EFI app) and it seemed more
> error prone to duplicate the implementation there.

Fair enough.

One more thing I notice in your board_fdt_blob_setup() implementation
in arch/arm/mach-snapdragon/board.c :

It seems to be using either an built-in or external devicetree. It
seems that we should show this in dm_announce(), i.e. with the call to
fdtdec_get_srcname() ?

Regards,
Simon


>
> Kind regards
>
> >
> > Regards,
> > Simon
> >
> >
> >>
> >> Move the initf_malloc() call earlier so that malloc is available during
> >> fdtdec_setup().
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>  common/board_f.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/common/board_f.c b/common/board_f.c
> >> index 98dc2591e1d0..bddfa6b992b9 100644
> >> --- a/common/board_f.c
> >> +++ b/common/board_f.c
> >> @@ -867,15 +867,15 @@ static int initf_upl(void)
> >>  }
> >>
> >>  static const init_fnc_t init_sequence_f[] = {
> >>         setup_mon_len,
> >> +       initf_malloc,
> >>  #ifdef CONFIG_OF_CONTROL
> >>         fdtdec_setup,
> >>  #endif
> >>  #ifdef CONFIG_TRACE_EARLY
> >>         trace_early_init,
> >>  #endif
> >> -       initf_malloc,
> >>         initf_upl,
> >>         log_init,
> >>         initf_bootstage,        /* uses its own timer, so does not need DM */
> >>         event_init,
> >> --
> >> 2.47.0
> >>
>
> --
> // Caleb (they/them)
>