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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
