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

[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...

> > 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.

> > (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?

--
Andreas Dannenberg
Texas Instruments Inc

<snip>