On Wed, Nov 27, 2024 at 06:08:25AM -0700, Simon Glass wrote: > 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.
Simon, > 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. IPQ platforms use CONFIG_MULTI_DTB_FIT and CONFIG_MULTI_DTB_FIT_GZIP. The gzip decompressor allocates memory and faces this problem. Moving initf_malloc(), above helps resolve this. Would that be considered as a good justification to moving up initf_malloc()? Please advice. Thanks Varada > > >> 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
