Hi Simon, On Mon, May 06, 2019 at 09:51:56PM -0600, Simon Glass wrote: > Hi Andreas, > > On Fri, 3 May 2019 at 14:25, Andreas Dannenberg <dannenb...@ti.com> wrote: > > > > Simon, > > > > On Sat, Mar 30, 2019 at 09:18:08PM +0100, Simon Goldschmidt wrote: > > > Simon Glass <s...@chromium.org> schrieb am Sa., 30. März 2019, 21:06: > > > > > > > Hi Simon, > > > > > > > > On Wed, 27 Mar 2019 at 13:40, Simon Goldschmidt > > > > <simon.k.r.goldschm...@gmail.com> wrote: > > > > > > > > > > This introduces a new Kconfig option SPL_CLEAR_BSS_F. If enabled, it > > > > clears > > > > > the bss before calling board_init_f() instead of clearing it before > > > > calling > > > > > board_init_r(). > > > > > > > > > > This also ensures that variables placed in BSS can be shared between > > > > > board_init_f() and board_init_r() in SPL. > > > > > > > > > > Such global variables are used, for example, when loading things from > > > > > FAT > > > > > before SDRAM is available: the full heap required for FAT uses global > > > > > variables and clearing BSS after board_init_f() would reset the heap > > > > state. > > > > > An example for such a usage is socfpa_arria10 where an FPGA > > > > > configuration > > > > > is required before SDRAM can be used. > > > > > > > > > > Make the new option depend on ARM for now until more implementations > > > > follow. > > > > > > > > > > > > > I still have objections to this series and I think we should discuss > > > > other ways of solving this problem. > > > > > > > > Does socfgpa have SRAM that could be used before SDRAM is available? > > > > If so, can we not use that for the configuration? What various are > > > > actually in BSS that are needed before board_init_r() is called? Can > > > > they not be in a struct created from malloc()? > > > > > > > > > > The problem is the board needs to load an FPGA configuration from FAT > > > before SDRAM is available. Yes, this is loaded into SRAM of course, but > > > the > > > whole code until that is done uses so many malloc/free iterations that The > > > simple mall of implementation would require too much memory. > > > > > > And it's the full malloc state variables only that use BSS, not the FAT > > > code. > > > > I've actually faced very similar issues working on our TI AM654x "System > > Firmware Loader" implementation (will post upstream soon), where I need > > to load this firmware and other files from media such as MMC/FAT in a very > > memory-constrained SPL pre-relocation environment *before* I can bring up > > DDR. > > > > Initially, I modified the fat.c driver to re-use memory so it is not as > > wasteful during SYS_MALLOC_SIMPLE. While I'm not proud of this solution [1] > > this allowed us to get going, allowing to load multiple files without > > issues in pre-relocation SPL. > > That seems to point the way to a useful solution I think. We could > have a struct containing allocated pointers which is private to FAT, > and just allocate them the first time.
The board_init_f()-based loader solution we use extends beyond MMC/FAT, but also for OSPI, X/Y-Modem, and (later) USB, network, etc. Background: On our "TI K3" devices we need to do a whole bunch of stuff before DDR is up with limited memory, namely loading and installing a firmware that controls the entire SoC called "System Firmware". It is only after this FW is loaded from boot media and successfully started that I can bring up DDR. So all this is done in SPL board_init_f(), which as the last step brings up DDR. Not having BSS available to carry over certain state to the board_init_r() world would lead to a bunch of hacky changes across the board I'm afraid, more below. > I wonder if that would be enough for > > > > > In the quest of creating something more upstream-friendly I had then > > switched to using full malloc in pre-relocation SPL so that I didn't > > have to hack the FAT driver, encountering similar issues like you > > brought up and got this working, but ultimately abandoned this > > approach after bundling all files needed to get loaded into a single > > image tree blob which no longer required any of those solutions. > > > > What remained till today however is a need to preserve specific BSS > > state from pre-relocation SPL over to post-relocation SPL environment, > > namely flags set to avoid the (expensive) re-probing of peripheral > > drivers by the SPL loader. For that I introduced a Kconfig option that > > allows skipping the automatic clearing of BSS during relocation [2]. > > > > Seeing this very related discussion here got me thinking about how else > > I can carry over this "state" from pre- to post relocation but that's > > probably a discussion to be had once I post my "System Firmware Loader > > Series", probably next week. > > Since this is SPL I don't you mean 'relocation' here. I think you mean > board_init_f() to board_init_r()? Yes that's what I mean. AFAIK relocation in SPL is still called relocation from what I have seen working on U-boot, it just relocates gd and stack but not the actual code (personally I find it misleading calling what SPL does "relocation", but I got used to it). > You can use global_data to store state, I thought the idea was to stay away from gd, so we can eventually get rid of it altogether? > or malloc() to allocate memory and put things there. The challenge with potentially having to incorporate such a custom solution for state preservation into several drivers as explained earlier (SPI, USB, network, etc.) is that it does not appear to scale well. I think using BSS instead would make all those additions cleaner and simpler. > But using BSS seems wrong to me. I've seen you saying this a few times :) Why? > If you are doing something in board_init_f() in SPL that needs BSS, > can you not just move that code to board_init_r()? I need to access media drivers in board_init_f(), for which currently I'm using BSS so I can preserve some limited state, such as that the peripheral init was done, so that it doesn't get re-done by the actual SPL loader later. board_init_r() requires DDR to be available which I can't use without doing all that work in board_init_f() first to load/start the system controller firmware, so it's a bit of a chicken and egg issue here. My system firmware loader patch series is about ready and I was planning on posting it tomorrow. How about with the entire approach being in the open we use this as an opportunity to re-look at potential alternative solutions... Thanks, -- Andreas Dannenberg Texas Instruments Inc > > > > PS: If you want to save a ton of memory during FAT loading you can > > try something like CONFIG_FS_FAT_MAX_CLUSTSIZE=16384, I argue the > > default is overkill for all practical scenarios. > > > > -- > > Andreas Dannenberg > > Texas Instruments Inc > > > > [1] > > http://git.ti.com/gitweb/?p=ti-u-boot/ti-u-boot.git;a=commitdiff;h=3de26c27d232425e6a3b337dc402d73fe22ea88c > > [2] > > http://git.ti.com/gitweb/?p=ti-u-boot/ti-u-boot.git;a=commitdiff;h=9ab4a405c98e966a59c7c3005c08cb95ed87eea8 > > > > > > > > One way out could be to move the full mall of state variables into 'gd'... > > > > > > Another way would be to continue into board_init_f without SDRAM enabled > > > and in it it later... > > > > > > Regards, > > > Simon > > > > > > > > > > If this is a limitation of FAT, then I think we should fix that, > > > > instead. > > > > > > > > Regards, > > > > Simon > > > > > > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> > > > > > --- > > > > > > > > > > Changes in v3: > > > > > - improve commit message to show why CONFIG_CLEAR_BSS_F is needed > > > > > > > > > > Changes in v2: > > > > > - make CONFIG_SPL_CLEAR_BSS_F depend on ARM for now > > > > > > > > > > common/spl/Kconfig | 12 ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig > > > > > index 206c24076d..6a4270516a 100644 > > > > > --- a/common/spl/Kconfig > > > > > +++ b/common/spl/Kconfig > > > > > @@ -156,6 +156,18 @@ config SPL_STACK_R_MALLOC_SIMPLE_LEN > > > > > to give board_init_r() a larger heap then the initial heap > > > > > in > > > > > SRAM which is limited to SYS_MALLOC_F_LEN bytes. > > > > > > > > > > +config SPL_CLEAR_BSS_F > > > > > + bool "Clear BSS section before calling board_init_f" > > > > > + depends on ARM > > > > > + help > > > > > + The BSS section is initialized to zero. In SPL, this is > > > > normally done > > > > > + before calling board_init_r(). > > > > > + For platforms using BSS in board_init_f() already, enable > > > > > this > > > > to > > > > > + clear the BSS section before calling board_init_f() instead > > > > > of > > > > > + clearing it before calling board_init_r(). This also ensures > > > > that > > > > > + variables placed in BSS can be shared between board_init_f() > > > > and > > > > > + board_init_r(). > > > > > + > > > > > config SPL_SEPARATE_BSS > > > > > bool "BSS section is in a different memory region from text" > > > > > help > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > _______________________________________________ > > > U-Boot mailing list > > > U-Boot@lists.denx.de > > > https://lists.denx.de/listinfo/u-boot > > Regards, > SImon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
