[PATCH] common/board_f: init malloc earlier

[Simon Glass]

Hi Caleb,

On Tue, 26 Nov 2024 at 09:15, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Simon,
>
> On 26/11/2024 16:38, Simon Glass wrote:
> > 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?
>
> I think so. I'm not sure I follow with "more generic solution"?

Using an event to set the RAM location, rather than repurposing the
FDT-setup function.

> >
> > To my eye it looks like if we called an event in setup_dest_addr() we
> > could allow ram_base to be set.
>
> Right, it's possible/likely that we could clean up how mach-snapdragon
> currently does memory parsing.
>
> I do think this patch would still be justified even if we ignore the
> Qualcomm case.

It does put malloc() outside the purview of trace. There is always
going to be a race between which subsystem wants to be first and we
have certainly changed it several times. But until we have an actual
need, I think it is better to wait.

> >
> >>
> >> 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() ?
>
> Yes, the way it's implemented currently assumes that if CONFIG_OF_BOARD
> is set that we'd never use the built-in DT and mach-snapdragon doesn't
> fit this assumption. It's not a high priority but for sure something I'd
> like to see fixed.

I'm not even sure that assumption is right, actually...

[..]

REgards,
Simon