Am 08.08.2019 um 20:29 schrieb Andreas Dannenberg:
Simon,
On Thu, Aug 08, 2019 at 09:29:03AM +0200, Simon Goldschmidt wrote:
Hi Andreas,
On Wed, Aug 7, 2019 at 11:24 PM Andreas Dannenberg <dannenb...@ti.com> wrote:
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.
That would be very welcome!
Proposal submitted.
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?
Well, yes, that's my main concern. I'm not against boards using BSS early.
In fact, I started an attempt for socfpga_a10 to use BSS early some months
ago. However, I am concerned that this gets the default for some boards and
people just don't notice using BSS. Given the lack of discussion on this
patch before it was merged, I can easly imagine patches making use of BSS
slipping in.
Understood.
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
Great to have that list! What I'm still missing is a more detailed reason
*why* you need BSS early If it's because of one of the invalid usages above,
it might be better to fix them. If it's because of memory shortage: why
wouldn't the heap be enough to solve that problem? If it's because of heap
shortage when using simple malloc (I had that on socfpga_a10): maybe it can
be fixed in a different way?
Ok back to my specific scenario, hopefully I'm adding at least some new
aspects now rather than repeating what was discussed already in different
threads...
From SPL I'm required to load (and start) our "System Firmware" which is
a prerequisite for bringing up DDR. We know that DDR bringup itself
should happen in SPL's board_init_f(), hence the need for loading stuff
from board_init_f() when no DDR is yet available (only on-chip memory).
I'm using the same loader framework to do the loading from
board_init_f() that SPL later uses from a board_init_r() context to
load U-Boot proper, ATF, and other files depending on the platform. >
Now let's focus on two static variables that play a role in this
context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
static variables are essentially used to remember the initialization
state of the FAT driver and the MMC loader, so that it doesn't get
re-initialized the second time those get called (during SPL's main usage
of loading U-Boot, etc.). So essentially the desire is to carry this
initialization state from SPL's board_init_f() to board_init_r().
OK, so essentially, you've added CONFIG_SPL_EARLY_BSS because FAT and
MMC contain variables in BSS? That would mean we could drop that config
option after fixing those two (given that they can be fixed)?
You suggested simple malloc. We have that available and use it (DM uses
it), but how could this play in here? Sure I can reserve some memory
from board_init_f(), or the drivers under discussion, and store the
initialization state there, but now I'd have the need to carry the
pointer to that initialization data forward somehow. spl_fat.c is not a
DM driver, it inherently doesn't have anything I can "tack on" additional
data fields. I don't quite see how I can make this work more elegantly
but I'm open to suggestions...
No, sorry, what I wrote was probably a bit confusing. I wrote that as a
result of my work on socfpga_a10. There, we have the firmware loader
framework loading things e.g. from MMC. During my test of unifying the
socfpga config header files (both gen5 and a10 combined), I stumbled
accross the fact that you cannot use standard malloc in SPL when the
devicetree is initialized during board_init_f as dlmalloc.c makes heavy
use of BSS. You can only use simple malloc there, because its state is
kept in 'gd'.
But it seems that wasn't your problem?
A next problem with simple malloc is that you can't free anything and I
think I remember code passages around file system loading that make
heavy use of malloc/free. But that again doesn't seem to be your problem
here?
(Mr. Glass had suggested in one of the threads why I don't do the
DDR initialization in board_init_r() then, which I experimented with,
but the changes I had to make to common U-Boot files were rather drastic
so I abandoned this attempt).
Yes, I can understand that that's not an ideal way to move forward...
To come back on the original issue, I'd still propose to add these
static variables to 'gd' or to some sub-struct referenced from 'gd'. I
see a high risk for others to run into these issues that you have hidden
for your platform by enabling CONFIG_SPL_EARLY_BSS.
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?
Of course it does! I know there are files using BSS when they shouldn't;
socfpga_gen5 SPL was one of them. The problem is that either people don't know
they shouldn't be using BSS or they don't care (because it just works).
What I'm afraid of with this new config option is that using BSS early might
become more or less the standard (as there *are* many boards having it
available from the start), which might easily result in boards that cannot use
BSS ealry being broken.
Sadly, I failed to come up with a way of how to detect such invalid usage of
BSS at compile time or runtime.
I used the JTAG debugger to look at the BSS region during SPL execution
to see which piece of code would touch it when.... But yeah that is not
something that is automatic and easily scalable across platforms...
No, that's not what I meant :-) I do have access to a JTAG debugger for
my platform, too, but I don't want to run this manual step with every
update to check BSS...
Regards,
Simon
--
Andreas Dannenberg
Texas Instruments Inc
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
