Hi Caleb,
On Tue, 26 Nov 2024 at 05:22, Caleb Connolly caleb.connolly@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@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:
- 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.
- 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@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)