[U-Boot] OMAP (4) boot_params
Greetings,
I've been fighting with SPL passing not boot_params properly to u-boot on OMAP4. There are many layers to this onion but I've tracked the bulk of the problem down to the following issues.
--- SPL ---
arch/arm/cpu/armv7/omap-common/hwinit-common.c sets a pointer to the SPL's &boot_params correctly (cpu_init_crit->lowlevel_init->s_init) but the definition of that pointer in common/spl/spl.c:
u32 *boot_params_ptr = NULL;
puts it into the spl bss section (in SDRAM) which is cleared long after cpu_init_crit(). Making that:
u32 *boot_params_ptr __attribute__ ((section(".data")));
allows the pointer to be in SPL data section (SRAM) and still have its value by the time image_entry() is called. But common/spl/spl.c is not omap-specific so changes there are a concern.
Next, image_entry() is called with the argument being indirected an extra time: u32 boot_params_ptr_addr = (u32)&boot_params_ptr; image_entry((u32 *)boot_params_ptr_addr);
That extra level of indirection is never dealt with (on ARM anyway) and it ends up passing junk to u-boot. I've tested replacing those lines with: image_entry((u32 *)boot_params_ptr);
and that passes a correct address in r0 to lowlevel_init.S in u-boot.
--- u-boot ---
lowlevel_init.S only deals with pointers for boot_params. It does not *copy* the content of the boot_params struct. With the fixes above we get to u-boot with *the address in SRAM* of the SPL's boot_params struct stored in the first word of u-boot's boot_params struct. Here's logging showing what I have working to this point:
U-Boot SPL 2013.04-rc1-00386-g1d3dea1-dirty (Apr 02 2013 - 17:21:36) OMAP4460 ES1.1 boot_params_ptr @40309a5c = 40309918 OMAP SD/MMC: 0 image entry point: 0xBF800000
U-Boot 2013.04-rc1-00386-g1d3dea1-dirty (Apr 02 2013 - 17:21:36)
<<< a debug %p print of & boot_params in board_mmc_init said bffdbf10 >>>
# md 0xbffdbf10 4 bffdbf10: 40309918 00000000 00000000 bffd297c
<<< 40309918 is the expected SPL &boot_params in SRAM as noted above >>>
# md 40309918 5 40309918: 4030d204 00000000 00000005 00000001 40309928: 00000001
That maps to the expected (omap_bootdevice == 5 being salient for me):
struct omap_boot_parameters { char *boot_message; unsigned int mem_boot_descriptor; unsigned char omap_bootdevice; unsigned char reset_reason; unsigned char ch_flags; };
That leaves me at an impasse. If we expect to have a struct in both contexts then something must copy its contents. That's not currently done.
Or we could have a struct in SPL but a *struct in u-boot. But that mixes the . and -> access syntax and there may be source that's agnostic to being in the SPL or u-boot.
The last idea I had was to make the SPL struct hidden and only use a pointer for access. That means both SPL and u-boot source could use the -> syntax.
The only places where I see this are: ./arch/arm/cpu/armv7/omap-common/boot-common.c: return (u32) (boot_params.omap_bootdevice); ./arch/arm/include/asm/arch-omap4/sys_proto.h:extern struct omap_boot_parameters boot_params; ./arch/arm/include/asm/arch-omap4/sys_proto.h: if ((boot_params.ch_flags) & (CH_FLAGS_CHSETTINGS)) ./arch/arm/include/asm/arch-omap5/sys_proto.h:extern struct omap_boot_parameters boot_params; ./arch/arm/include/asm/arch-omap5/sys_proto.h: if ((boot_params.ch_flags) & (CH_FLAGS_CHSETTINGS))
but I have no OMAP5 to test against. And I'm a little surprised that OMAP3 isn't in evidence here.
This has me wrapped around the axle so many times I need guidance regarding what's "the right way" to fix this.
Pointers welcome. (pun intended.)
-Mike Cashwell
Hi Michael,
(please wrap your line around 70 chars max)
I won't comment on the 'u-boot' part of your message as it is quite SPL-vs-U-Boot- and OMAP-specific, and I prefer to leave this to people better suited for this. However, on the low level init front:
On Tue, 2 Apr 2013 18:39:17 -0400, Michael Cashwell mboards@prograde.net wrote:
Greetings,
I've been fighting with SPL passing not boot_params properly to u-boot on OMAP4. There are many layers to this onion but I've tracked the bulk of the problem down to the following issues.
--- SPL ---
arch/arm/cpu/armv7/omap-common/hwinit-common.c sets a pointer to the SPL's &boot_params correctly (cpu_init_crit->lowlevel_init->s_init) but the definition of that pointer in common/spl/spl.c:
u32 *boot_params_ptr = NULL;
puts it into the spl bss section (in SDRAM) which is cleared long after cpu_init_crit(). Making that:
u32 *boot_params_ptr __attribute__ ((section(".data")));
allows the pointer to be in SPL data section (SRAM) and still have its value by the time image_entry() is called. But common/spl/spl.c is not omap-specific so changes there are a concern.
If I'm not mistaken, lowlevel_init() is not supposed to use BSS at all, as the C runtime has not been initialized yet -- precisely, the BSS clearing loop long after the cpu_init_crit() call belongs to the code that sets up this environment.
Besides, it seems like SPL does not jump directly to Linux but to U-Boot, so U-Boot itself should set up the boot params, not SPL, which can at best prepare and store values in static RAM not mapped as data or BSS in either SPL or U-Boot (this is normally done through GD).
Next, image_entry() is called with the argument being indirected an extra time: u32 boot_params_ptr_addr = (u32)&boot_params_ptr; image_entry((u32 *)boot_params_ptr_addr);
That extra level of indirection is never dealt with (on ARM anyway) and it ends up passing junk to u-boot. I've tested replacing those lines with: image_entry((u32 *)boot_params_ptr);
and that passes a correct address in r0 to lowlevel_init.S in u-boot.
This has to be investigated indeed.
Amicalement,
On Apr 3, 2013, at 1:56 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
(please wrap your line around 70 chars max)
I've never understood why this is useful. It's poor on both large computer screens (wastes space and forces extra vertical scrolling) AND on small screens like handheld devices (because the arbitrary width is limit still too wide).
Your MUA seems to have handled the quoted-printable content transfer encoding I sent (since your quoted text had no tell tail = characters at the end of each line). Why can't it wrap the text to whatever width *you* want? Mine does (provided the message ISN'T hard-wrapped) and I don't much like senders forcing the rendering on my devices to be done in ways that are counter to my preferences.
Wouldn't it be better for readers to do what's best for each device? Imagine someone on a tablet viewing email first in portrait mode and then rotating to landscape. Why advocate forcing one or the other to have a demonstrably poor user experience?
The MUA controls many other elements of the presentation. HTLM aside, does the sender control what font face, size or color all recipients must use to view the message? Of course not, and for good reason. I fail to see why line width should be some magical special case.
So with all due respect, I can with greater legitimately turn your admonition around and ask that you please update or configure your MUA to handle your display preferences on your side.
On Tue, 2 Apr 2013 18:39:17 -0400, Michael Cashwell mboards@prograde.net wrote:
I've been fighting with SPL passing not boot_params properly to u-boot on OMAP4. There are many layers to this onion but I've tracked the bulk of the problem down to the following issues.
...Making that:
u32 *boot_params_ptr __attribute__ ((section(".data")));
allows the pointer to be in SPL data section (SRAM) and still have its value by the time image_entry() is called. But common/spl/spl.c is not omap-specific so changes there are a concern.
If I'm not mistaken, lowlevel_init() is not supposed to use BSS at all, as the C runtime has not been initialized yet -- precisely, the BSS clearing loop long after the cpu_init_crit() call belongs to the code that sets up this environment.
Yes, that was my thinking too. Surely clearing data after code has set it can't be right.
Besides, it seems like SPL does not jump directly to Linux but to U-Boot, so U-Boot itself should set up the boot params, not SPL, which can at best prepare and store values in static RAM not mapped as data or BSS in either SPL or U-Boot (this is normally done through GD).
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.
Best regards, -Mike
Hi Michael,
On Wed, 3 Apr 2013 09:45:19 -0400, Michael Cashwell mboards@prograde.net wrote:
On Apr 3, 2013, at 1:56 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
(please wrap your line around 70 chars max)
I've never understood why this is useful. [...]
... but apparently you managed to do it, thanks.
On Tue, 2 Apr 2013 18:39:17 -0400, Michael Cashwell mboards@prograde.net wrote:
I've been fighting with SPL passing not boot_params properly to u-boot on OMAP4. There are many layers to this onion but I've tracked the bulk of the problem down to the following issues.
...Making that:
u32 *boot_params_ptr __attribute__ ((section(".data")));
allows the pointer to be in SPL data section (SRAM) and still have its value by the time image_entry() is called. But common/spl/spl.c is not omap-specific so changes there are a concern.
If I'm not mistaken, lowlevel_init() is not supposed to use BSS at all, as the C runtime has not been initialized yet -- precisely, the BSS clearing loop long after the cpu_init_crit() call belongs to the code that sets up this environment.
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.
Besides, it seems like SPL does not jump directly to Linux but to U-Boot, so U-Boot itself should set up the boot params, not SPL, which can at best prepare and store values in static RAM not mapped as data or BSS in either SPL or U-Boot (this is normally done through GD).
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)
Best regards, -Mike
Amicalement,
On Apr 3, 2013, at 10:36 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Michael,
On Wed, 3 Apr 2013 09:45:19 -0400, Michael Cashwell mboards@prograde.net wrote:
I've never understood why this is useful. [...]
... but apparently you managed to do it, thanks.
With extra effort that could be better applied to other work, but yes. :-)
...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:
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 */
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.
Best regards, -Mike
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
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
-----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
participants (5)
-
Albert ARIBAUD -
Michael Cashwell -
Sricharan R -
Tom Rini -
Wolfgang Denk