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

Tue Aug 13 20:38:22 UTC 2019

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.

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

Regards,
Simon