Hi Caleb, On Tue, 26 Nov 2024 at 09:15, Caleb Connolly <caleb.conno...@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.conno...@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.conno...@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
