[U-Boot] [PATCH v3 1/6] spl: add Kconfig option to clear bss early

Tue May 7 03:51:56 UTC 2019

Hi Andreas,

On Fri, 3 May 2019 at 14:25, Andreas Dannenberg <dannenberg at ti.com> wrote:
>
> Simon,
>
> On Sat, Mar 30, 2019 at 09:18:08PM +0100, Simon Goldschmidt wrote:
> > Simon Glass <sjg at 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.goldschmidt at 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.

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

You can use global_data to store state, or malloc() to allocate memory
and put things there. But using BSS seems wrong to me. 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()?

>
> 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.goldschmidt at 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 at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot

Regards,
SImon