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? -- Andreas Dannenberg Texas Instruments Inc _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
