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

Andreas Dannenberg dannenberg at ti.com
Thu Aug 8 18:29:06 UTC 2019


Simon,

On Thu, Aug 08, 2019 at 09:29:03AM +0200, Simon Goldschmidt wrote:
> Hi Andreas,
> 
> On Wed, Aug 7, 2019 at 11:24 PM Andreas Dannenberg <dannenberg at ti.com> wrote:
> >
> > Hi Simon,
> > thanks for your patience waiting for a response. Please see comments inlined...
> >
> > On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
> > > On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla at ti.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > > > > Hi Lokesh,
> > > > >
> > > > > thanks for following up on this.
> > > > >
> > > > > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla at ti.com> wrote:
> > > > >>
> > > > >> Hi Tom,
> > > > >>
> > > > >> On 20/07/19 9:21 PM, Tom Rini wrote:
> > > > >>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> > > > >>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini at konsulko.com> wrote:
> > > > >>>>>
> > > > >>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> > > > >>>>>
> > > > >>>>>> In order to be able to use more advanced driver functionality which often
> > > > >>>>>> relies on having BSS initialized during early boot prior to relocation
> > > > >>>>>> several things need to be in place:
> > > > >>>>>>
> > > > >>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > > > >>>>>>    at the top of the MCU SRAM area, with the stack starting right below
> > > > >>>>>>    it,
> > > > >>>>>> 2) We need to move the initialization of BSS prior to entering
> > > > >>>>>>    board_init_f(). We will do this with a separate commit by turning on
> > > > >>>>>>    the respective CONFIG option.
> > > > >>>>>>
> > > > >>>>>> In this commit we also clean up the assignment of the initial SP address
> > > > >>>>>> as part of the refactoring, taking into account the pre-decrement post-
> > > > >>>>>> increment nature in which the SP is used on ARM.
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Andreas Dannenberg <dannenberg at ti.com>
> > > > >>>>>
> > > > >>>>> Applied to u-boot/master, thanks!
> > > > >>>>
> > > > >>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> > > > >>>> but in a discussion about a similar patch I sent [1], using BSS from
> > > > >>>> board_init_f
> > > > >>>> was turned down. And Simon Glass rather convinced me that this is the current
> > > > >>>> API U-Boot has (and is documented in README).
> > > > >>>>
> > > > >>>> So either we must change this API and its documentation (and I would expect the
> > > > >>>> author of this patch to combine the README change with the code change), or this
> > > > >>>> patch would have to be rejected.
> > > > >>>>
> > > > >>>> Again, I'm sorry I only see this now. In thought to remember a
> > > > >>>> discussion in this
> > > > >>>> thread, but I clearly remember that wrong...
> > > > >>>>
> > > > >>>> [1] https://patchwork.ozlabs.org/patch/1057237/
> > > > >>>
> > > > >>> And I had missed that other thread.  Lokesh, since I think Andreas is
> > > > >>> out currently can you expand a little on what we can/can't do on this
> > > > >>> platform?  Thanks!
> > > > >>
> > > > >> The reason why BSS is needed very early in this platform is for the following
> > > > >> reasons:
> > > > >> - System co-processor is the central resource manager in SoC and should be
> > > > >> loaded and started very early in the boot process. Without that no peripheral or
> > > > >> memory can be initialized. So for loading system co-processor image, we only
> > > > >> have limited SRAM and a peripheral initialized by ROM.
> > > > >> - System co-processor(DMSC) is being represented as remote-core in
> > > > >> Device-tree(We are strictly following DM and DT model for the entire SoC).
> > > > >> - Since DM is also followed by each peripheral device and remote core, DM should
> > > > >> be enabled very early and many peripheral drivers are dependent on BSS usage.
> > > > >> So, BSS has been made available very early.
> > > > >>
> > > > >> Hope this is clear. Let me know if more details are required, I will be happy to
> > > > >> explain.
> > > > >
> > > > > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > > > > stays consistent throught U-Boot.
> > > >
> > > > I understand and agree that it should be consistent. Just discussed this with
> > > > Andreas, who is courteous enough to update the details in his vacation.
> > >
> > > We don't have to rush here, I don't have a problem waiting for Andreas to
> > > answer when he's back.
> > >
> > > >
> > > > >
> > > > > The reasons you stated still don't make it clear to me *why* bss is needed
> > > > > early. There are other boards using DM early that don't need this. In my
> > > > > opinion, DM drivers normally don't rely on BSS but keep all their state in
> > > >
> > > > This statement doesn't hold true for all the drviers. At least the mmc driver
> > > > uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> > > > times[0]. In the past we en counted other drivers using it. I guess the idea
> > > > here is to enable the BSS support generically instead of fixing each of every
> > > > driver.
> > >
> > > So this driver is generally not usable in pre-relocation phase? The README
> > > document is pretty clear about BSS not being available in board_init_f. I know
> > > this text is old, but it seems still valid.
> > >
> > > And if this is really a workaround because it's easier to use this workaround
> > > instead of fixing drivers that invalidly use BSS, is this what we want?
> > >
> > > >
> > > > > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > > > > have to fix those drivers?
> > > >
> > > > So, correct me here, why should driver be restricted to not use BSS?
> > >
> > > Post-relocation drivers might be free to use BSS (although you lose the
> > > per-instance storage when using BSS instead of the driver's priv data),
> > > but pre-relocation drivers are not.
> > > That's the current definition in U-Boot. This patch changes it by
> > > adding the option
> > > to use BSS early. This bears the danger of code being changed in a way that
> > > it requires BSS to be available early and might not work on other boards that
> > > actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
> > >
> > > >
> > > > Also doing a grep for bss usage very early in board_init_f produced many results:
> > > > ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> > > > arch/arm/mach-socfpga/spl_gen5.c
> > >
> > > Right, that's my responsibility, and there's a patch in Marek's queue
> > > to fix this:
> > > the DDR driver used BSS and I simply moved it's BSS variables to its driver.
> > > Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
> > >
> > > > arch/arm/mach-zynqmp/spl.c
> > > > arch/mips/mach-jz47xx/jz4780/jz4780.c
> > > > board/barco/platinum/spl_picon.c
> > > > board/barco/platinum/spl_titanium.c
> > > > board/compulab/cl-som-imx7/spl.c
> > > > board/congatec/cgtqmx6eval/cgtqmx6eval.c
> > > > board/dhelectronics/dh_imx6/dh_imx6_spl.c
> > > > board/el/el6x/el6x.c
> > > > board/freescale/imx8mq_evk/spl.c
> > > > board/freescale/imx8qm_mek/spl.c
> > > > board/freescale/imx8qxp_mek/spl.c
> > > > board/freescale/ls1021aiot/ls1021aiot.c
> > > > board/freescale/ls1021aqds/ls1021aqds.c
> > > > board/freescale/ls1021atwr/ls1021atwr.c
> > > > board/freescale/mx6sabreauto/mx6sabreauto.c
> > > > board/freescale/mx6sabresd/mx6sabresd.c
> > > > board/freescale/mx6slevk/mx6slevk.c
> > > > board/freescale/mx6sxsabresd/mx6sxsabresd.c
> > > > board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> > > > board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> > > > board/liebherr/display5/spl.c
> > > > board/logicpd/imx6/imx6logic.c
> > > > board/phytec/pcm058/pcm058.c
> > > > board/phytec/pfla02/pfla02.c
> > > > board/sks-kinkel/sksimx6/sksimx6.c
> > > > board/solidrun/mx6cuboxi/mx6cuboxi.c
> > > > board/technexion/pico-imx6ul/spl.c
> > > > board/technexion/pico-imx7d/spl.c
> > > > board/toradex/apalis_imx6/apalis_imx6.c
> > > > board/toradex/colibri_imx6/colibri_imx6.c
> > > > board/udoo/neo/neo.c
> > > > board/variscite/dart_6ul/spl.c
> > > > board/woodburn/woodburn.c
> > > >
> > > > There might be some false positive cases but most of the above files are
> > > > utilizing bss in board_init_f.
> > >
> > > So all these boards include a hack that's against what's the currently
> > > documented status.
> >
> > Yes implementation and doc need to stay consistent.
> >
> > > >
> > > > >
> > > > > Further, allowing BSS early is still against what the main README says, so I
> > > > > want to raise the question again: shouldn't this main README be changed if we
> > > > > suddenly allow BSS to be used early (because that main README says we can'that
> > > > > do that)?
> > > >
> > > > I do agree on this part. We should fix README in this case.
> >
> > I can prepare a PATCH to propose an update to the README, it definitely
> > should stay in sync with the implementation, independent of the path we
> > are choosing to potentially make any improvements moving forward.
> 
> That would be very welcome!

Proposal submitted.
 
> > > My point (Simon Glass has convinced me in the previous discussion I
> > > mentioned) is that there *are* boards that can't use BSS early. You can't just
> > > allow all code to use BSS early as you risk breaking such boards.
> >
> > That's why the early BSS option has to be turned on explicitly. Nobody
> > requires you to use early BSS. It should be considered an option for
> > certain limited use cases (and described as such in an update to README,
> > like there are many other very "special" options in U-Boot that have no
> > wide use). But I guess one of your concerns as you alluded to earlier
> > is that it may result in incompatibilities moving forward as this
> > essentially lowers the "barrier of entry" to using this feature,
> > potentially spilling into drivers or other common files?
> 
> Well, yes, that's my main concern. I'm not against boards using BSS early.
> In fact, I started an attempt for socfpga_a10 to use BSS early some months
> ago. However, I am concerned that this gets the default for some boards and
> people just don't notice using BSS. Given the lack of discussion on this
> patch before it was merged, I can easly imagine patches making use of BSS
> slipping in.


Understood.
 
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
> >
> > > If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
> > > and changing the README.
> > >
> > > >
> > > > [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
> > >
> > > Right, that one looks strange. It seems to me that's some kind of leftover
> > > code from pre-DM? I would have expected the UCLASS for mmc drivers
> > > to include this 'initialized' flag instead of this ugly "static in
> > > function" thing.
> >
> > I did a quick search, some other users of BSS that could potentially have an
> > impact in the context of "early FW loading from SPL board_init_f()" include...
> >
> > 'static int fat_registered' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
> >
> > 'static struct mmc *mmc' in ...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
> > (actually I'm responsible for the 'static' there)
> >
> > 'static int usb_stor_curr_dev' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
> >
> > Then, there are also quite a few instances in drivers/, many of them not
> > relevant to operating from SPL's board_init_f() context, with some of
> > them however possibly being affected like these:
> >
> > 'static struct device_node *of_aliases' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
> >
> > 'static int reloc_done' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
> >
> > 'static bool sf_mtd_registered' in....
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
> >
> > or
> >
> > 'static ulong next_reset' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76
> 
> Great to have that list! What I'm still missing is a more detailed reason
> *why* you need BSS early If it's because of one of the invalid usages above,
> it might be better to fix them. If it's because of memory shortage: why
> wouldn't the heap be enough to solve that problem? If it's because of heap
> shortage when using simple malloc (I had that on socfpga_a10): maybe it can
> be fixed in a different way?

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

You suggested simple malloc. We have that available and use it (DM uses
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...

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


> > Now, I have not validated each and every one of those (beyond 'fat_registered'
> > which I know is problematic), and there are more, for having an impact or not.
> > Whether all of those need any fixes or improvements set aside for a moment, at
> > a minimum doesn't it make you concerned about stability of code execution
> > without initialized BSS, no?
> 
> Of course it does! I know there are files using BSS when they shouldn't;
> socfpga_gen5 SPL was one of them. The problem is that either people don't know
> they shouldn't be using BSS or they don't care (because it just works).
> 
> What I'm afraid of with this new config option is that using BSS early might
> become more or less the standard (as there *are* many boards having it
> available from the start), which might easily result in boards that cannot use
> BSS ealry being broken.
> 
> Sadly, I failed to come up with a way of how to detect such invalid usage of
> BSS at compile time or runtime.

I used the JTAG debugger to look at the BSS region during SPL execution
to see which piece of code would touch it when.... But yeah that is not
something that is automatic and easily scalable across platforms...

--
Andreas Dannenberg
Texas Instruments Inc


More information about the U-Boot mailing list