| Message ID | 8F4057F9-7EDF-494C-BF77-530475D06E7C@prograde.net |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Michael, On Wed, 3 Apr 2013 10:59:23 -0400, Michael Cashwell <mboards@prograde.net> wrote: > >>>> ...Making that: > >>>> > >>>> u32 *boot_params_ptr __attribute__ ((section(".data"))); > >> > >> Yes, that was my thinking too. Surely clearing data after code has set > >> it can't be right. > > > > With all due respect, the documentation can with greater legitimately > > turn your admonition around and ask that you please refrain from > > setting BSS or data variables when the C runtime environment has not > > been set. :) > > > > IOW, what is wrong here is writing to a BSS variable before you're > > allowed to as per the rules under which your code is running. > > I think we're in agreement, but it's not my code doing it. The code, > as it exists in mainline is writing early to space in bss. My change > avoids that by moving the variable from the default bss to data: ... except, as I said above, at this point your code should not write at all, be int in BSS or data, until the C environment is set up. So... > diff --git a/common/spl/spl.c b/common/spl/spl.c > index 6715e0d..1d84535 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -42,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR; > #define CONFIG_SYS_MONITOR_LEN (200 * 1024) > #endif > > -u32 *boot_params_ptr = NULL; > +u32 *boot_params_ptr __attribute__ ((section(".data"))); > struct spl_image_info spl_image; ... NAK. Place this in a fixed section that you'll map somewhere else then in BSS or data. Also: in the future, avoid pasting a diff directly in a mail to the u-boot list if it is not a real patch submission, as our patchwork instance at (http://patchwork.ozlabs.org/project/uboot/list/) will get confused and record your mail as a legitimate patch. > /* Define board data structure */ > > >> OK, here we have an unfortunate name overloading. The boot_params here > >> is specifically an OMAP handoff from the CPU's internal boot ROM to SPL > >> and then from SPL to u-boot. (The same code paths are involved.) It's > >> totally unrelated to the the boot_params passed to the Linux kernel. > >> > >> Since it's confusing maybe a renaming is called for as well. > > > > Indeed. Plus, if it is shared data, it should definitely be mapped at a > > fixed memory location or copied from stage to stage (the latter only if > > the former is impossible) > > Yes, I'm exploring that now. The differences between SPL and U-boot are > subtle. Actually not that subtle once you get the hang of it: SPL and U-Boot are built on the same code base; SPL is the minimal, non-interactive, early boot stage which can be loaded and run by ROM code, while U-Boot is the full-featured, interactive, too big to boot directly, stage, which SPL can chain into. > Best regards, > -Mike Amicalement,
On Wed, Apr 03, 2013 at 05:34:18PM +0200, Albert ARIBAUD wrote: > Hi Michael, > > On Wed, 3 Apr 2013 10:59:23 -0400, Michael Cashwell > <mboards@prograde.net> wrote: > > > >>>> ...Making that: > > >>>> > > >>>> u32 *boot_params_ptr __attribute__ ((section(".data"))); > > >> > > >> Yes, that was my thinking too. Surely clearing data after code has set > > >> it can't be right. > > > > > > With all due respect, the documentation can with greater legitimately > > > turn your admonition around and ask that you please refrain from > > > setting BSS or data variables when the C runtime environment has not > > > been set. :) > > > > > > IOW, what is wrong here is writing to a BSS variable before you're > > > allowed to as per the rules under which your code is running. > > > > I think we're in agreement, but it's not my code doing it. The code, > > as it exists in mainline is writing early to space in bss. My change > > avoids that by moving the variable from the default bss to data: > > ... except, as I said above, at this point your code should not write at > all, be int in BSS or data, until the C environment is set up. So... But we have to save this ROM-passed information before we overwrite it ourselves (by accident or purpose). > > > diff --git a/common/spl/spl.c b/common/spl/spl.c > > index 6715e0d..1d84535 > > --- a/common/spl/spl.c > > +++ b/common/spl/spl.c > > @@ -42,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR; > > #define CONFIG_SYS_MONITOR_LEN (200 * 1024) > > #endif > > > > -u32 *boot_params_ptr = NULL; > > +u32 *boot_params_ptr __attribute__ ((section(".data"))); > > struct spl_image_info spl_image; > > ... NAK. Place this in a fixed section that you'll map somewhere else > then in BSS or data. > > Also: in the future, avoid pasting a diff directly in a mail to the > u-boot list if it is not a real patch submission, as our patchwork > instance at (http://patchwork.ozlabs.org/project/uboot/list/) will get > confused and record your mail as a legitimate patch. > > > /* Define board data structure */ > > > > >> OK, here we have an unfortunate name overloading. The boot_params here > > >> is specifically an OMAP handoff from the CPU's internal boot ROM to SPL > > >> and then from SPL to u-boot. (The same code paths are involved.) It's > > >> totally unrelated to the the boot_params passed to the Linux kernel. > > >> > > >> Since it's confusing maybe a renaming is called for as well. > > > > > > Indeed. Plus, if it is shared data, it should definitely be mapped at a > > > fixed memory location or copied from stage to stage (the latter only if > > > the former is impossible) > > > > Yes, I'm exploring that now. The differences between SPL and U-boot are > > subtle. > > Actually not that subtle once you get the hang of it: SPL and U-Boot > are built on the same code base; SPL is the minimal, non-interactive, > early boot stage which can be loaded and run by ROM code, while U-Boot > is the full-featured, interactive, too big to boot directly, stage, > which SPL can chain into. Part of the confusion here is that I think some TI-isms didn't get removed from the general code. jump_to_image_no_args() does not in fact jump to an image without passing any arguments. It jumps to an image passing an argument of where we may have saved some previously passed paramters (in this case, the format the TI's ROM has defined for a while). We also _may_ be in U-Boot without SPL having been run because U-Boot was given a config header instead. But I think, and need to re-read this thread a bit more, part of the solution is to rename jump_to_image_no_args as jump_to_image_uboot, keep it __weak and provide one that deals with this (and perhaps more cleanly deals with VIRTIO/ZEBU image_entry). And after that we can talk about moving things that can't be in the BSS out of the data section and into another section.
Dear Tom, In message <20130403164215.GK7035@bill-the-cat> you wrote: > > > ... except, as I said above, at this point your code should not write at > > all, be int in BSS or data, until the C environment is set up. So... > > But we have to save this ROM-passed information before we overwrite it > ourselves (by accident or purpose). Thete are two official places for data storage before the full C runtime environment is available: the stack, and the "global data" structure. > But I think, and need to re-read this thread a bit more, part of the > solution is to rename jump_to_image_no_args as jump_to_image_uboot, keep > it __weak and provide one that deals with this (and perhaps more cleanly > deals with VIRTIO/ZEBU image_entry). And after that we can talk about > moving things that can't be in the BSS out of the data section and into > another section. Adding another section makes things more complicated, but not really better. If you can provide writable storage, then you could also use it in a more regular way, say for a writable data segment, or bigger stack, or malloc space, or ... so it is generally useful instead of only this special case here. Best regards, Wolfgang Denk
On Apr 4, 2013, at 1:52 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom, > > On Apr 3, 2013, at 11:34 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: >>> ... except, as I said above, at this point your code should not write at >>> all, be it in BSS or data, until the C environment is set up. So... >> >> But we have to save this ROM-passed information before we overwrite it >> ourselves (by accident or purpose). > > Thete are two official places for data storage before the full C > runtime environment is available: the stack, and the "global data" > structure. I thought there were more levels than just pre and post CRT. Specifically, the global_data struct's comment says it is intended to be used "until we have set up the memory controller so that we can use RAM." To me that suggests once we have RAM any further data storage should go there instead of bloating global_data. I thought this distinction was embodied in the bss/data section difference with the former being zeroed during CRT init and the latter not. And I'm clearly not the only one who thought this. The change I proposed in common/spl/spl.c that Albert doesn't like: > -u32 *boot_params_ptr = NULL; > +u32 *boot_params_ptr __attribute__ ((section(".data"))); is already immediately followed by exactly the same pattern: > static bd_t bdata __attribute__ ((section(".data"))); The only reason I can think of to put bdata explicitly in .data instead of the default .bss is so it can avoid the CRT zeroing of .bss. If that's wrong then why have both sections? How are they different? >> ... after that we can talk about moving things that can't be in the BSS >> out of the data section and into another section. > > Adding another section makes things more complicated, but not really > better. My proposal does not add another section. The needed section already exists and seemingly for precisely the purpose under discussion. > If you can provide writable storage, then you could also use > it in a more regular way, say for a writable data segment, or bigger > stack, or malloc space, or ... so it is generally useful instead of > only this special case here. This is exactly my concern. I see no justification for a special case. If never writing to any linker-defined section (.data or .bss) before CRT init really is the design rule then there are quite a few other violations that need to be fixed. Rolling an ad hoc solution for each can't be the right approach. Best regards, -Mike Cashwell
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/04/2013 01:52 AM, Wolfgang Denk wrote: > Dear Tom, > > In message <20130403164215.GK7035@bill-the-cat> you wrote: >> >>> ... except, as I said above, at this point your code should not >>> write at all, be int in BSS or data, until the C environment is >>> set up. So... >> >> But we have to save this ROM-passed information before we >> overwrite it ourselves (by accident or purpose). > > Thete are two official places for data storage before the full C > runtime environment is available: the stack, and the "global data" > structure. Well, there's a 3rd "official" way, that crept in. There's a certain amount of that has to get run before we can have the BSS ready (which resides in DDR) and we're still in some other form of RAM and need a few variables that would otherwise live in the BSS available now. Generally this is the i2c driver (so that we can see what platform this is and what our DDR is then). The other case is the pointer to whatever might have come in from ROM. We do not have stack at this point in time yet, even, when we call save_boot_params. > >> But I think, and need to re-read this thread a bit more, part of >> the solution is to rename jump_to_image_no_args as >> jump_to_image_uboot, keep it __weak and provide one that deals >> with this (and perhaps more cleanly deals with VIRTIO/ZEBU >> image_entry). And after that we can talk about moving things >> that can't be in the BSS out of the data section and into another >> section. > > Adding another section makes things more complicated, but not > really better. If you can provide writable storage, then you could > also use it in a more regular way, say for a writable data segment, > or bigger stack, or malloc space, or ... so it is generally useful > instead of only this special case here. I don't think we have much choice here. This is really the very first thing we do. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRXdqLAAoJENk4IS6UOR1Wf+AQAJLKAKrxpl3RcmXpQ83ecOTY k02SCgww94WkAeQUSdhA2zHXkf1lXyF7T7C5qzB2GYwyuoSxCrK98fgauZfynZCL 0CXmoCh3rG7ZJudKMI105dQ9e5/GyF7LGKiCrpUyynNCzdAi3D30JU5oXkBVa7I8 NdmzhYz7DfmOg047oX9PlK+xM2vu9SUP+QkkFuKGJAFCD0F8IbjskMWaij/A9zuK 67eVX3Uv8/FccLriDh8eHVbPSIzku50wBM/QFEy5uB2jV3DZ/NwcOM7+DYWgx0yT 7atsjsNF5WkFzKY50u5CDNIWgMqDBKbL/a3EOZWhelIwHP1/hzMHpq1GFuvyCLxt Rm3RwylJEBCBWD8RJ0j64vcUQxPhsuicIrKyZ0sQlw9P3iPfeXtDXoA0nw6KaoJk zh/kohfXqczYmi5OBRFKa1/BIDCRVBVEMzfkZxBb2b2JbQLcTo/1UboTKdOPIEQr 08Il3HRawlb7/VFXWLNRQSJqpi5vSkBoMcZ1yeymucZw7WNi+pwLRwD7VnQRUpt8 kcZk/9n1y8iC26bcIZS26v6UaqXG4XjiiSUXs1Ht3fXsPIeSOZgtEbnao5b3CMyQ isaXtVWK07hDODkcX0M8/M/3Wya+w+6AAP17F2X1C9H9wmtkb+IKe7idAL+s9zW6 4zHOQxrnH9syDBxOY5Uo =pG9U -----END PGP SIGNATURE-----
Hi Mike Cashwell, On Thursday 04 April 2013 07:48 PM, Michael Cashwell wrote: > On Apr 4, 2013, at 1:52 AM, Wolfgang Denk <wd@denx.de> wrote: > >> Dear Tom, >> >> On Apr 3, 2013, at 11:34 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: >>>> ... except, as I said above, at this point your code should not write at >>>> all, be it in BSS or data, until the C environment is set up. So... >>> >>> But we have to save this ROM-passed information before we overwrite it >>> ourselves (by accident or purpose). >> >> Thete are two official places for data storage before the full C >> runtime environment is available: the stack, and the "global data" >> structure. > > I thought there were more levels than just pre and post CRT. Specifically, > the global_data struct's comment says it is intended to be used "until we > have set up the memory controller so that we can use RAM." > > To me that suggests once we have RAM any further data storage should go there > instead of bloating global_data. I thought this distinction was embodied in > the bss/data section difference with the former being zeroed during CRT init > and the latter not. > > And I'm clearly not the only one who thought this. The change I proposed in > common/spl/spl.c that Albert doesn't like: > >> -u32 *boot_params_ptr = NULL; >> +u32 *boot_params_ptr __attribute__ ((section(".data"))); > > is already immediately followed by exactly the same pattern: > >> static bd_t bdata __attribute__ ((section(".data"))); > > The only reason I can think of to put bdata explicitly in .data instead of > the default .bss is so it can avoid the CRT zeroing of .bss. > > If that's wrong then why have both sections? How are they different? > >>> ... after that we can talk about moving things that can't be in the BSS >>> out of the data section and into another section. >> >> Adding another section makes things more complicated, but not really >> better. > > My proposal does not add another section. The needed section already exists > and seemingly for precisely the purpose under discussion. > >> If you can provide writable storage, then you could also use >> it in a more regular way, say for a writable data segment, or bigger >> stack, or malloc space, or ... so it is generally useful instead of >> only this special case here. > > This is exactly my concern. I see no justification for a special case. > If never writing to any linker-defined section (.data or .bss) before CRT > init really is the design rule then there are quite a few other violations > that need to be fixed. Rolling an ad hoc solution for each can't be the > right approach. > Sorry for the late feedback. The **only** reason for passing the boot_params from SPL to U-BOOT was when somebody uses a CONFIGURATION HEADER + SPL + U-BOOT, which was never a case. But the broken code that you pointed was trying to help such a scenario. I guess nobody would have used this combination. save_boot_params ideally should not write in to either .data or .bss. Because this would break a XIP kind of a boot. The only place where it can write is the GD or some reserved SRAM area that is always 'writable'. We did not have a XIP in OMAP4/5 and thus this went unnoticed. I will post a patch today to address this. Regards, Sricharan
On Apr 8, 2013, at 5:43 AM, Sricharan R <r.sricharan@ti.com> wrote: > The **only** reason for passing the boot_params from SPL to U-BOOT was > when somebody uses a CONFIGURATION HEADER + SPL + U-BOOT, which > was never a case. But the broken code that you pointed was trying to > help such a scenario. I guess nobody would have used this combination. I think there is a much more common case that needs this information. Consider a normal memory-boot (e.g.: not UART or USB). It goes like: ROM -> SPL -> U-Boot -> Linux kernel+initrd When there are multiple possible bootable busses/memories a decision must be made at each step as to which to read from. The current behavior seems broken because SPL and u-boot can come from one source while u-boot will load linux from a different source. I think, by default, the selected source should be consistent. My approach for this is to decode boot_params.omap_bootdevice in board_mmc_init() and call mmc_init() so the correct default bus is selected before any "mmc read" commands (that don't specify a bus) execute. I found that boot_params.omap_bootdevice (actually all of boot_params) was always zero no matter what boot device had actually been used. This was because of the .bss clearing. > save_boot_params ideally should not write in to either .data or .bss. > Because this would break a XIP kind of a boot. The only place where it can > write is the GD or some reserved SRAM area that is always 'writable'. > We did not have a XIP in OMAP4/5 and thus this went unnoticed. > > I will post a patch today to address this. Great! I will look for this and track it. Perhaps we need to add any missing fields to struct omap_boot_parameters, add that whole struct added to an OMAP4/5 section in: ./arch/arm/include/asm/global_data.h:struct arch_global_data since that's in struct global_data already. The only hard part I see is that C structs are not directly accessible from assembly code like save_boot_params and tracking the needed assembly offsets manually is error prone. And of course, save_boot_params runs so early we don't even have a stack setup yet. One idea I was thinking about was to just save the r0 pointer somewhere but defer the processing of it until after we're done with CRT setup. That would get all this out of assembly code and into C code. Not only would the bss clearing then already be done it's much cleaner to access structs from C. Let me know if I can assist in any way. Best regards, -Michael Cashwell
diff --git a/common/spl/spl.c b/common/spl/spl.c index 6715e0d..1d84535 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -42,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_SYS_MONITOR_LEN (200 * 1024) #endif -u32 *boot_params_ptr = NULL; +u32 *boot_params_ptr __attribute__ ((section(".data"))); struct spl_image_info spl_image; /* Define board data structure */