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

Tue Aug 13 21:06:02 UTC 2019

Hi Simon,

On Tue, Aug 13, 2019 at 10:38:22PM +0200, Simon Goldschmidt wrote:
> Am 07.08.2019 um 23:23 schrieb Andreas Dannenberg:
> > 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.
> > > 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?
> > 
> > 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
> > 
> > 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?
> 
> Having searched more through that list, I'd say let's grab it and fix these.
> All of these have the potential to disturb more boards using that code in
> the future.

I've seen JJ has some patches already (not posted to the official ML yet
but I suppose it will happen soon) that will address at least two of my
concerns, amongst other things of what I think is a good cleanup effort...

"spl: mmc: do not use a static variable to prevent multiple initialization"
"spl: fat: remove usage of the fat_registered flag"

...so posted to the U-Boot ML that will help us here. Let's see what his plans
are to post those and go from there (JJ?).

> The very least we should do is delcare/access such BSS storage via some kind
> of guarding accessor/macro that checks if BSS is available (which by default
> would be checking gd->flags for GD_FLG_RELOC).
> 
> The boolean-type ints might probably become bits in gd->flags, but I'm not
> sure for the static pointer values...

We should see that we can avoid those.

Also I have since "re-discovered" two more uses of static that I
actually introduced into TI's "System Firmware" loader that gets executed from
SPL's board_init_f()...

https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-k3/sysfw-loader.c#L23

I use those statics to customize the SPL image loader load address (via
hooking into spl_get_load_buffer) depending on whether I load my own
file (system firmware) into a custom memory buffer or the next stage of
U-Boot (to CONFIG_SYS_TEXT_BASE). I'll need to see how to potentially do
that differently without leaning on gd...

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Regards,
> Simon