[U-Boot] [PATCH] ARM: crt0: Pass malloc base address

[Simon Glass]

Hi Albert,

On 12 November 2015 at 09:13, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
> Hello Simon,
>
> On Thu, 12 Nov 2015 08:59:54 -0700, Simon Glass <sjg at chromium.org>
> wrote:
>> Hi Albert,
>>
>> On 11 November 2015 at 23:57, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
>> > Hello Simon,
>> >
>> > On Wed, 11 Nov 2015 14:49:05 -0700, Simon Glass <sjg at chromium.org>
>> > wrote:
>> >> Hi Fabio,
>> >>
>> >> On 11 November 2015 at 14:24, Fabio Estevam <festevam at gmail.com>
>> >> wrote:
>> >> > Hi Simon,
>> >> >
>> >> > On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass <sjg at chromium.org>
>> >> > wrote:
>> >> >
>> >> >> That test is intended to avoid setting up simple malloc() if we
>> >> >> plan to use full malloc() in SPL. Of course, full malloc() is set
>> >> >> up a little later (in spl_init()). But we should not need both -
>> >> >> either we use simple malloc() or full malloc().
>> >> >>
>> >> >> But for your board I see:
>> >> >>
>> >> >> $ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg
>> >> >> #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000
>> >> >>
>> >> >> So you will not be able to use simple malloc(). I'd suggest calling
>> >> >> spl_init() from board_init_f() if you need malloc() there. But it
>> >> >
>> >> > Yes, I do call spl_init() from board_init_f() prior to calling
>> >> > malloc() and this has been working fine prior to 5ba534d247d418.
>> >> >
>> >> >> presumably needs to be done after you have SDRAM up. So perhaps
>> >> >
>> >> > The trick here is that I need malloc to work prior to have SDRAM
>> >> > configured.
>> >> >
>> >> > On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR
>> >> > we will need to configure.
>> >> >
>> >> > Otavio has sent the SPL support for cgtqmx6eval, but it has not
>> >> > reached U-boot mainline yet.
>> >> >
>> >> > I am reproducing the problem on a mx6sabresd_spl target with the
>> >> > simple example code I sent previously.
>> >>
>> >> I see. That's not a use case I have seen before.
>> >>
>> >> I'd suggest changing the #ifdef to always set up early malloc() if
>> >> CONFIG_SYS_MALLOC_F is set. There will of course be a new malloc()
>> >> once spi_init() is called, but that does not matter.
>> >>
>> >> In your patch, please be careful to add some docs to the README on
>> >> this point (the early malloc() is only there to permit DRAM init,
>> >> etc.). It could get quite confusing...
>> >
>> > I'm not sure all details are clear to me so let me chime in on this.
>> >
>> > IIUC, what Fabio needs is a working C runtime including malloc, based on
>> > some IRAM rather than SDRAM.
>> >
>> > This means there will be two phases where malloc can be used, the usual
>> > one after SDRAM init, and a new one before SDRAM init.
>> >
>> > Of course, the amount of memory reserved for the malloc arena cannot
>> > be the same in IRAM as it will be in SDRAM, meaning that the routine
>> > which reserves the arena must handle both cases, by detecting whether
>> > it is running before or after SDRAM init and choosing the right malloc
>> > size macro, or by just bein passed that info from the code responsible
>> > for maintaining the C runtime environment (arch/arm/lib/crt0.C in the
>> > ARM case) -- via the 'chunk_id' method I described earlier in
>> > https://www.mail-archive.com/u-boot@lists.denx.de/msg191898.html
>> >
>> > There is a *VERY* *BIG* *PROBLEM* to the whole idea of
>> > malloc-before-SDRAM:
>> >
>> > We could manage to transfer the pre-SDRAM-init maloc arena content to
>> > the post-SDRAM-init malloc arena, thus preserving all pre- SDRAM-init
>> > mallocs() across the stack switch.
>> >
>> > *BUT*: all (variable-stored) pointers to pre-SDRAM-malloc'ed space will
>> > become dangling pointers, because we have no easy way of telling where
>> > these pointers are and relocating them.
>> >
>> > This means that the mallocs() done before SDRAM init are lost and that
>> > al pointers to them /will/ become dangling, including any copies and
>> > derivatives of these; all code which depend these pointers and their
>> > will have to handle the situation, which will prove *quite* complex or,
>> > to be blunter, will prove not actually done, and will prove so through
>> > weird bugs creeping up that we'll take some time to relate to malloc
>> > issues, and to fix.
>> >
>> > Of course, we could just decide that any malloc done before SDRAM init
>> > is lost and that user code should deal with it. But I fear it will do so
>> > just as badly as it would handle the "maloc transfer" alternative I just
>> > described. Plus, if the malloc'ed data contains hardware module state
>> > or any non-recoverable-again information, then we *cannot* just drop
>> > it.
>> >
>> > Now, there is a systemic solution: that all code which stores a
>> > malloc-arena-based pointer value in memory call a system-provided
>> > function which, if running pre-SDRAM-init, would record the location of
>> > the pointer in a list; upon C environment switch from pre- to
>> > post-SDRAM contexts, the list would be run through and each pointer
>> > would be 'relocated' from the old to the new maloc arena. Post-init,
>> > the functon would not do any recording. That causes a slight
>> > performance hit on mallocs, but I don't think it'll affect boot time
>> > that much.
>> >
>> > Only code which has to run before SDRAM init would have to use the
>> > function.
>> >
>> > The benefit of this approach is that maloc'ed space would remain
>> > malloc'ed after SDRAM init and declared pointers to it would remain
>> > valid. Code which has to malloc before SDRAM init would not have to
>> > re-malloc or fix anything, or even realize the stack and malloc arena
>> > have moved, as long as it has declared its malloc-related pointer(s).
>> >
>> > Of course the list would be limited. But seeing as we'd be running in a
>> > tight context and only to get SDRAM running, the list would not extend
>> > much.
>> >
>> > I imagine a scheme where the list is kept in the malloc arena. The
>> > malloc space itself would still grow 'down' from the stack top while
>> > the pointer list would grow 'up' from the stack 'bottom' limit.
>> >
>> > I could whip up an RFC patch (with ARM support, to be extended to other
>> > platforms as my recent experience showed I'm not that good at NIOS2 for
>> > instance) if people are interested.
>>
>> I'm really not sure that this problem is worth solving.
>>
>> We have quite a hard boundary between board_init_f() and
>> board_init_r() in U-Boot proper. For some platforms the memory
>> available in the former is not available in the latter. People
>> understand (I think) that pre-relocation stuff goes away and exists
>> only to assist with bringing up the new environment.
>
> (I fear that half the question is what people understand, and then the
> other half is what they think they understand and have not -- here,
> since malloc and DM is available before and after SDRAM init, people
> might think it is just the *same* DM across the transition -- or,
> eventually, want it to be.)

Possibly, but we can add documentation in the code to correct this view.

>
>> Driver model is started up in board_init_f() but then the whole thing
>> is thrown away and started again in board_init_r(). So if someone kept
>> a device pointer around for later (e.g. in global_data) it would not
>> work (Don't Do That!). This approach has not caused any reported
>> problems yet.
>
> Isn't it a potential problem that the driver model is started (and
> therefore some devices intialized) and then thrown away? Some devices
> might not like (or even permit) being initialized twice, or that may
> cause glitches, for instance.

Yes. This is called out in the driver model README for example. It
hasn't caused any problems yet though.

>
>> While you can copy memory and fix up pointers returned by malloc(),
>> pointers to rodata and other things may still exist. It would be
>> tedious or error-prone to relocate these.
>
> rodata does not move during SDRAM init (and the stack swap we're
> considering here). It moves during U-Boot relocation though; and yes,
> pointers to ro data pre-relocation will become stale after relocation,
> but this has been the case even way before DM existed.

Conceptually I am trying to make board_init_f() and board_init_r()
mean the same thing in SPL and U-Boot proper. That is, the SDRAM init
happens in board_init_f() and there is an early malloc() available
then. Once you get to board_init_r() you have a new malloc() which
sits in SDRAM. For both SPL and U-Boot proper I don't think the
boundary should be when SDRAM is inited, but when board_init_r() is
called.

>
>> My preference would be to keep it simple, and just make it clear that
>> board_init_f() may have an early malloc(), but even if it does it goes
>> away in board_init_r(). That rule can apply to SPL just as easily as
>> U-Boot proper.
>
> IMO, keeping it simple does not play well with making DM and malloc
> available pre-SDRAM init :) --- but don't miscontrue my meaning: I'm
> fine with DM and malloc pre-SDRAM on platforms which have enough IRAM /
> SRAM / lockable cache / whatever for it. My point is just that sooner
> or later, someone will start wanting not to do again after SDRAM init
> what they've already done before it, because it avoids double HW init
> issues, becaue it saves time, etc. And that's going to be way sooner
> than later, IMO.

Yes that is quite possibility true. I'm happy to review any patches if
it helps. My comment about it possibly not being worthwhile solving is
just that. Things have a way of changing, so what looks unnecessary
now might look essential tomorrow. In a way, the simple malloc() fits
into that category.

Regards,
Simon