[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Am 08.08.2019 um 21:43 schrieb Andreas Dannenberg:
> Hi Simon,
> 
> On Thu, Aug 08, 2019 at 09:01:03PM +0200, Simon Goldschmidt wrote:
>> Am 08.08.2019 um 20:29 schrieb Andreas Dannenberg:
> 
> <snip>
> 
>>> Ok back to my specific scenario, hopefully I'm adding at least some new
>>> aspects now rather than repeating what was discussed already in different
>>> threads...
>>>
>>>   From SPL I'm required to load (and start) our "System Firmware" which is
>>> a prerequisite for bringing up DDR. We know that DDR bringup itself
>>> should happen in SPL's board_init_f(), hence the need for loading stuff
>>> from board_init_f() when no DDR is yet available (only on-chip memory).
>>>
>>> I'm using the same loader framework to do the loading from
>>> board_init_f() that SPL later uses from a board_init_r() context to
>>> load U-Boot proper, ATF, and other files depending on the platform. >
>>> Now let's focus on two static variables that play a role in this
>>> context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
>>> static variables are essentially used to remember the initialization
>>> state of the FAT driver and the MMC loader, so that it doesn't get
>>> re-initialized the second time those get called (during SPL's main usage
>>> of loading U-Boot, etc.). So essentially the desire is to carry this
>>> initialization state from SPL's board_init_f() to board_init_r().
>>
>> OK, so essentially, you've added CONFIG_SPL_EARLY_BSS because FAT and MMC
>> contain variables in BSS? That would mean we could drop that config option
>> after fixing those two (given that they can be fixed)?
> 
> Yes I think I could drop it in this case, I'm not married to
> CONFIG_SPL_EARLY_BSS in any way. But we still have all the other
> platforms that use memset() to directly clear BSS from SPL's
> board_init_f()...  Ideally any improvements should be made across
> the board.
> 
> (And I just need to throw that out there) one could also imagine moving
> said platforms to also use CONFIG_SPL_EARLY_BSS (replacing their custom
> memset approach) to at least unify the approach...

That could well be a valid approach. However, ideally, we'd first check 
why these boards really need that memset. I fixed socfgpa_gen5 by fixing 
the sdram driver to not use BSS... (a rather simple fix that even ended 
up reducing code size by keeping the data on the stack).

> 
>>> it), but how could this play in here? Sure I can reserve some memory
>>> from board_init_f(), or the drivers under discussion, and store the
>>> initialization state there, but now I'd have the need to carry the
>>> pointer to that initialization data forward somehow. spl_fat.c is not a
>>> DM driver, it inherently doesn't have anything I can "tack on" additional
>>> data fields. I don't quite see how I can make this work more elegantly
>>> but I'm open to suggestions...
>>
>> No, sorry, what I wrote was probably a bit confusing. I wrote that as a
>> result of my work on socfpga_a10. There, we have the firmware loader
>> framework loading things e.g. from MMC. During my test of unifying the
>> socfpga config header files (both gen5 and a10 combined), I stumbled accross
>> the fact that you cannot use standard malloc in SPL when the devicetree is
>> initialized during board_init_f as dlmalloc.c makes heavy use of BSS. You
>> can only use simple malloc there, because its state is kept in 'gd'.
>>
>> But it seems that wasn't your problem?
>>
>> A next problem with simple malloc is that you can't free anything and I
>> think I remember code passages around file system loading that make heavy
>> use of malloc/free. But that again doesn't seem to be your problem here?
> 
> I had this exact problem initially, as I was loading two files from
> board_init_f (the "system firmware" and it's config data), which I since
> consolidated into a FIT image avoiding the issue altogether. The FAT
> driver specifically is very wasteful allocating 64KB chunks of memory
> (you can reduce the sector size to alleviate some of that) and with
> simple malloc it would eat through the little RAM I had too quickly...
> So for a while I was using full malloc from board_init_f() to make the
> FAT driver happy. And I had also found I need BSS for that after a
> little pain and suffering with the JTAG debugger. But this is no
> longer n issue when only loading one file.

Yeah, well, that issue remains even if we'd fix the already mentioned 
problems...

> 
>>> (Mr. Glass had suggested in one of the threads why I don't do the
>>> DDR initialization in board_init_r() then, which I experimented with,
>>> but the changes I had to make to common U-Boot files were rather drastic
>>> so I abandoned this attempt).
>>
>> Yes, I can understand that that's not an ideal way to move forward...
>>
>> To come back on the original issue, I'd still propose to add these static
>> variables to 'gd' or to some sub-struct referenced from 'gd'. I see a high
>> risk for others to run into these issues that you have hidden for your
>> platform by enabling CONFIG_SPL_EARLY_BSS.
> 
> I was always hesitant even thinking about adding stuff to gd due to the
> large impact so I haven't really considered this as a feasible path.
> But if we were to go down this road possibly we could add a bitfield
> variable of some type (to be considerate of memory use) containing a
> collection of "initialized" flags used by different drivers that really
> need it?

That might have a smaller impact on U-Boot/SPL code quality than 
allowing early bss...

However, making that change might only make sense of other boards don't 
end up with still needing early bss.

In the end, I'm not married with "no-early-BSS", too: I always have a 
hard time imagining why boards cannot provide early BSS but have RAM for 
'gd'. The only reason can be that BSS is too big for whatever little 
amount we have before setting up DDR. But if you have a board that uses 
DM before DDR is up, you'll end up with a fair amount of RAM (heap) 
usage anyway, so why is BSS bad here but heap (that is never freed) is good?

Moving global variables that are used early into a different section 
(".bss_early") might be a possible solution, too. That might solve 
things where an 'init' flag isn't enough. However, your suggestion of an 
'initialized' flag array in 'gd' is probably smaller...

Regards,
Simon