| Message ID | 1371200101-11510-1-git-send-email-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Stefan, On Fri, 14 Jun 2013 10:54:59 +0200, Stefan Roese <sr@denx.de> wrote: > SPL already has GD set to the correct location (in s_init), we mustn't > move it around now since some data (clocks etc) is already present. > > This error was detected on the SPL port for the Compulab CM-T35 board > (OMAP3530). > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Tom Rini <trini@ti.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > arch/arm/lib/crt0.S | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S > index a9657d1..b05f66a 100644 > --- a/arch/arm/lib/crt0.S > +++ b/arch/arm/lib/crt0.S > @@ -85,7 +85,13 @@ ENTRY(_main) > bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ > sub sp, #GD_SIZE /* allocate one GD above SP */ > bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ > +#if !defined(CONFIG_SPL_BUILD) > +/* > + * SPL already has GD set to the correct location (in s_init), we mustn't > + * move it around now since some data (clocks etc) is already present. > + */ > mov r8, sp /* GD is above SP */ > +#endif > mov r0, #0 > bl board_init_f > NAK in this form. I don't want gd to be set "somewhere in the code" depending on the actual target; I want it set in crt0.S, period. I see there are several locations in ARM architecture or board code which set up GD themselves in the same manner as OMAP does. Luckily all these locations set it to the same value, the address of gdata. The correct fix (read: the one I won't NAK) is thus to add a #else clause in the code above, in which r8 will be set to =gdata, and to remove the corresponding assignments in the various places where they reside. (also, maybe not all SPLs want GD in gdata rather than on the stack; for instance, those SPLs loaded in DDR by some ROM code. Therefore, the whole gdata thing could possibly be placed under a specific condition such as CONFIG_SPL_GD_GLOBAL) Amicalement,
Hi Albert, On 20.06.2013 18:42, Albert ARIBAUD wrote: >> diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S >> index a9657d1..b05f66a 100644 >> --- a/arch/arm/lib/crt0.S >> +++ b/arch/arm/lib/crt0.S >> @@ -85,7 +85,13 @@ ENTRY(_main) >> bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ >> sub sp, #GD_SIZE /* allocate one GD above SP */ >> bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ >> +#if !defined(CONFIG_SPL_BUILD) >> +/* >> + * SPL already has GD set to the correct location (in s_init), we mustn't >> + * move it around now since some data (clocks etc) is already present. >> + */ >> mov r8, sp /* GD is above SP */ >> +#endif >> mov r0, #0 >> bl board_init_f >> > > NAK in this form. I don't want gd to be set "somewhere in the code" > depending on the actual target; I want it set in crt0.S, period. > > I see there are several locations in ARM architecture or board code > which set up GD themselves in the same manner as OMAP does. Luckily all > these locations set it to the same value, the address of gdata. > > The correct fix (read: the one I won't NAK) is thus to add a #else > clause in the code above, in which r8 will be set to =gdata, and to > remove the corresponding assignments in the various places where they > reside. Here's the problem. Setting r8 in _main is too late. As it has already been used (in the current implementation) to store some data (e.g. clocks for baudrate generation etc). Here the code from arch/arm/cpu/armv7/omap3/board.c:s_init(): #ifdef CONFIG_SPL_BUILD gd = &gdata; preloader_console_init(); timer_init(); #endif Note that this is done *before* _main() is called (we are talking about SPL for OMAP here). And it did cost me quite some time to find this problem, that r8 was re-configured in _main() and all the already set values disappeared again (no serial output etc). Yes, this needs some cleanup/fixup. Unfortunately I won't find the time to look into such a cleanup in the next days. Perhaps somebody else might jump in... Thanks, Stefan
Hi Stefan, On Thu, 20 Jun 2013 19:01:22 +0200, Stefan Roese <sr@denx.de> wrote: > Hi Albert, > > On 20.06.2013 18:42, Albert ARIBAUD wrote: > >> diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S > >> index a9657d1..b05f66a 100644 > >> --- a/arch/arm/lib/crt0.S > >> +++ b/arch/arm/lib/crt0.S > >> @@ -85,7 +85,13 @@ ENTRY(_main) > >> bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ > >> sub sp, #GD_SIZE /* allocate one GD above SP */ > >> bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ > >> +#if !defined(CONFIG_SPL_BUILD) > >> +/* > >> + * SPL already has GD set to the correct location (in s_init), we mustn't > >> + * move it around now since some data (clocks etc) is already present. > >> + */ > >> mov r8, sp /* GD is above SP */ > >> +#endif > >> mov r0, #0 > >> bl board_init_f > >> > > > > NAK in this form. I don't want gd to be set "somewhere in the code" > > depending on the actual target; I want it set in crt0.S, period. > > > > I see there are several locations in ARM architecture or board code > > which set up GD themselves in the same manner as OMAP does. Luckily all > > these locations set it to the same value, the address of gdata. > > > > The correct fix (read: the one I won't NAK) is thus to add a #else > > clause in the code above, in which r8 will be set to =gdata, and to > > remove the corresponding assignments in the various places where they > > reside. > > Here's the problem. Setting r8 in _main is too late. As it has already > been used (in the current implementation) to store some data (e.g. > clocks for baudrate generation etc). Here the code from > arch/arm/cpu/armv7/omap3/board.c:s_init(): > > #ifdef CONFIG_SPL_BUILD > gd = &gdata; > > preloader_console_init(); > > timer_init(); > #endif > > Note that this is done *before* _main() is called (we are talking about > SPL for OMAP here). Yes, it is done before _main... right before it. Like, really *right* *before* *it*. Like, s_init is called at the very end of lowlevel_init, which was branched straight into from cpu_init_crit which itself is called just before _main. In other words, after s_init(), _main immediately kicks in, sets up sp and r8 (which was done also in lowlevel_init and will thus be a no-op once gdata is handled in crt0.S too) and calls board_init_f(). So, calling s_init() last in lowlevel_init() would be the same as calling it first in board_init_f(). The difference with the current situation is, s_init() is C code, and C runtime is supposed not to be available until just before entering board_init_f(). This is the only reason why sp and r8 are set up in lowlevel_init: because s_init() is called at a time when C runtime is not officially set up yet. Note that as far as setting the C runtime environment is concerned, crt0.S does *exactly* the same thing as lowlevel_init -- or will, once the gdata stuff is added in crt0.S as I suggest. --- 8< --- So you just need to add the gdata stuff in crt0.S as I previously suggested, move the s_init() call in crt0.S (conditioned on building SPL) just before the call to board_init_f(), and then make lowlevel_init empty since it would then be useless. --- 8< --- > And it did cost me quite some time to find this > problem, that r8 was re-configured in _main() and all the already set > values disappeared again (no serial output etc). Actually your trouble precisely shows why gd/r8 should only be touched in a single file and nowhere else: because it is set in many places, its value was hard to control. > Yes, this needs some cleanup/fixup. Unfortunately I won't find the time > to look into such a cleanup in the next days. Perhaps somebody else > might jump in... There'll have to be a V2 for this patch anyway. Here's my offer: you submit V2 of this patch as described above between the '--- 8< ---' markers, and I handle the scrubbing of all spurious assignments to gd myself. Deal? > Thanks, > Stefan Amicalement,
Hi Albert, On 20.06.2013 19:51, Albert ARIBAUD wrote: >>> The correct fix (read: the one I won't NAK) is thus to add a #else >>> clause in the code above, in which r8 will be set to =gdata, and to >>> remove the corresponding assignments in the various places where they >>> reside. >> >> Here's the problem. Setting r8 in _main is too late. As it has already >> been used (in the current implementation) to store some data (e.g. >> clocks for baudrate generation etc). Here the code from >> arch/arm/cpu/armv7/omap3/board.c:s_init(): >> >> #ifdef CONFIG_SPL_BUILD >> gd = &gdata; >> >> preloader_console_init(); >> >> timer_init(); >> #endif >> >> Note that this is done *before* _main() is called (we are talking about >> SPL for OMAP here). > > Yes, it is done before _main... right before it. Like, really *right* > *before* *it*. Like, s_init is called at the very end of lowlevel_init, > which was branched straight into from cpu_init_crit which itself is > called just before _main. > > In other words, after s_init(), _main immediately kicks in, sets up sp > and r8 (which was done also in lowlevel_init and will thus be a no-op > once gdata is handled in crt0.S too) and calls board_init_f(). > > So, calling s_init() last in lowlevel_init() would be the same as > calling it first in board_init_f(). > > The difference with the current situation is, s_init() is C code, and C > runtime is supposed not to be available until just before entering > board_init_f(). This is the only reason why sp and r8 are set up in > lowlevel_init: because s_init() is called at a time when C runtime is > not officially set up yet. > > Note that as far as setting the C runtime environment is concerned, > crt0.S does *exactly* the same thing as lowlevel_init -- or will, once > the gdata stuff is added in crt0.S as I suggest. > > --- 8< --- > So you just need to add the gdata stuff in crt0.S as I previously > suggested, move the s_init() call in crt0.S (conditioned on building > SPL) just before the call to board_init_f(), and then make > lowlevel_init empty since it would then be useless. > --- 8< --- Useless? Its still calling cpy_clk_code(). Isn't this needed anymore? >> And it did cost me quite some time to find this >> problem, that r8 was re-configured in _main() and all the already set >> values disappeared again (no serial output etc). > > Actually your trouble precisely shows why gd/r8 should only be touched > in a single file and nowhere else: because it is set in many places, > its value was hard to control. Full ack on this. >> Yes, this needs some cleanup/fixup. Unfortunately I won't find the time >> to look into such a cleanup in the next days. Perhaps somebody else >> might jump in... > > There'll have to be a V2 for this patch anyway. > > Here's my offer: you submit V2 of this patch as described above between > the '--- 8< ---' markers, and I handle the scrubbing of all spurious > assignments to gd myself. Deal? Yes, I'll try to squeeze a few cycles from the other projects to get this done hopefully tomorrow. Thanks, Stefan
Hi Stefan, On Thu, 20 Jun 2013 20:28:01 +0200, Stefan Roese <sr@denx.de> wrote: > Hi Albert, > > On 20.06.2013 19:51, Albert ARIBAUD wrote: > >>> The correct fix (read: the one I won't NAK) is thus to add a #else > >>> clause in the code above, in which r8 will be set to =gdata, and to > >>> remove the corresponding assignments in the various places where they > >>> reside. > >> > >> Here's the problem. Setting r8 in _main is too late. As it has already > >> been used (in the current implementation) to store some data (e.g. > >> clocks for baudrate generation etc). Here the code from > >> arch/arm/cpu/armv7/omap3/board.c:s_init(): > >> > >> #ifdef CONFIG_SPL_BUILD > >> gd = &gdata; > >> > >> preloader_console_init(); > >> > >> timer_init(); > >> #endif > >> > >> Note that this is done *before* _main() is called (we are talking about > >> SPL for OMAP here). > > > > Yes, it is done before _main... right before it. Like, really *right* > > *before* *it*. Like, s_init is called at the very end of lowlevel_init, > > which was branched straight into from cpu_init_crit which itself is > > called just before _main. > > > > In other words, after s_init(), _main immediately kicks in, sets up sp > > and r8 (which was done also in lowlevel_init and will thus be a no-op > > once gdata is handled in crt0.S too) and calls board_init_f(). > > > > So, calling s_init() last in lowlevel_init() would be the same as > > calling it first in board_init_f(). > > > > The difference with the current situation is, s_init() is C code, and C > > runtime is supposed not to be available until just before entering > > board_init_f(). This is the only reason why sp and r8 are set up in > > lowlevel_init: because s_init() is called at a time when C runtime is > > not officially set up yet. > > > > Note that as far as setting the C runtime environment is concerned, > > crt0.S does *exactly* the same thing as lowlevel_init -- or will, once > > the gdata stuff is added in crt0.S as I suggest. > > > > --- 8< --- > > So you just need to add the gdata stuff in crt0.S as I previously > > suggested, move the s_init() call in crt0.S (conditioned on building > > SPL) just before the call to board_init_f(), and then make > > lowlevel_init empty since it would then be useless. > > --- 8< --- > > Useless? Its still calling cpy_clk_code(). Isn't this needed anymore? My bad -- I was looking at arch/arm/cpu/armv7/lowlevel_init.S, not arch/arm/cpu/armv7/omap3/lowlevel_init.S. > >> And it did cost me quite some time to find this > >> problem, that r8 was re-configured in _main() and all the already set > >> values disappeared again (no serial output etc). > > > > Actually your trouble precisely shows why gd/r8 should only be touched > > in a single file and nowhere else: because it is set in many places, > > its value was hard to control. > > Full ack on this. > > >> Yes, this needs some cleanup/fixup. Unfortunately I won't find the time > >> to look into such a cleanup in the next days. Perhaps somebody else > >> might jump in... > > > > There'll have to be a V2 for this patch anyway. > > > > Here's my offer: you submit V2 of this patch as described above between > > the '--- 8< ---' markers, and I handle the scrubbing of all spurious > > assignments to gd myself. Deal? > > Yes, I'll try to squeeze a few cycles from the other projects to get > this done hopefully tomorrow. Thank you. > Thanks, > Stefan Amicalement,
On Fri, Jun 14, 2013 at 10:54:59AM +0200, Stefan Roese wrote: > SPL already has GD set to the correct location (in s_init), we mustn't > move it around now since some data (clocks etc) is already present. > > This error was detected on the SPL port for the Compulab CM-T35 board > (OMAP3530). > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Tom Rini <trini@ti.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> While we have a problem here, it's not a problem that's visible on current SPL using platforms, just the CM-T35, yes? I just gave my beagleboard (classic and xM) a spin and they're OK. I've got a few more platforms I can dig out if needed, but I'm inclined to hold this until after v2013.07 and we can take one of the paths Albert outlined (change s_init to system_init, add to the function table, call that way). Does that work or have I underestimated the impact of this issue? Thanks!
Hi Tom, On 07/15/2013 04:33 PM, Tom Rini wrote: >> SPL already has GD set to the correct location (in s_init), we mustn't >> move it around now since some data (clocks etc) is already present. >> >> This error was detected on the SPL port for the Compulab CM-T35 board >> (OMAP3530). >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Tom Rini <trini@ti.com> >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > > While we have a problem here, it's not a problem that's visible on > current SPL using platforms, just the CM-T35, yes? Thats what I noticed as well. I was a bit astonished that I didn't see it on beagle. It should hit there too. > I just gave my > beagleboard (classic and xM) a spin and they're OK. Did you also power-cycle the board (cold boot)? > I've got a few more > platforms I can dig out if needed, but I'm inclined to hold this until > after v2013.07 and we can take one of the paths Albert outlined (change > s_init to system_init, add to the function table, call that way). Does > that work or have I underestimated the impact of this issue? Thanks! I can definitely live with postponing this solution/fix to after this release. Since your tests on the beagle boards are also working fine, then lets just hold this patch and release v2013.07 now. Thanks, Stefan
On Tue, Jul 16, 2013 at 08:24:48AM +0200, Stefan Roese wrote: > Hi Tom, > > On 07/15/2013 04:33 PM, Tom Rini wrote: > >> SPL already has GD set to the correct location (in s_init), we mustn't > >> move it around now since some data (clocks etc) is already present. > >> > >> This error was detected on the SPL port for the Compulab CM-T35 board > >> (OMAP3530). > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > >> Cc: Tom Rini <trini@ti.com> > >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > > > > While we have a problem here, it's not a problem that's visible on > > current SPL using platforms, just the CM-T35, yes? > > Thats what I noticed as well. I was a bit astonished that I didn't see > it on beagle. It should hit there too. > > > I just gave my > > beagleboard (classic and xM) a spin and they're OK. > > Did you also power-cycle the board (cold boot)? Yes, both SD and NAND boot on classic and SD boot on xM were cold boots. > > I've got a few more > > platforms I can dig out if needed, but I'm inclined to hold this until > > after v2013.07 and we can take one of the paths Albert outlined (change > > s_init to system_init, add to the function table, call that way). Does > > that work or have I underestimated the impact of this issue? Thanks! > > I can definitely live with postponing this solution/fix to after this > release. Since your tests on the beagle boards are also working fine, > then lets just hold this patch and release v2013.07 now. OK, thanks.
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index a9657d1..b05f66a 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -85,7 +85,13 @@ ENTRY(_main) bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ sub sp, #GD_SIZE /* allocate one GD above SP */ bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ +#if !defined(CONFIG_SPL_BUILD) +/* + * SPL already has GD set to the correct location (in s_init), we mustn't + * move it around now since some data (clocks etc) is already present. + */ mov r8, sp /* GD is above SP */ +#endif mov r0, #0 bl board_init_f
SPL already has GD set to the correct location (in s_init), we mustn't move it around now since some data (clocks etc) is already present. This error was detected on the SPL port for the Compulab CM-T35 board (OMAP3530). Signed-off-by: Stefan Roese <sr@denx.de> Cc: Tom Rini <trini@ti.com> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> --- arch/arm/lib/crt0.S | 6 ++++++ 1 file changed, 6 insertions(+)