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