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

Thu Jul 25 09:52:55 UTC 2019

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.

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

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.

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.

Regards,
Simon

>
> Thanks and regards,
> Lokesh
>