[PATCH] common/board_f: init malloc earlier

[Caleb Connolly]

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

Kind regards,
> 
> 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)
>>

-- 
// Caleb (they/them)