Patchwork [U-Boot,1/3] arm: spl: Fix SPL booting for OMAP3

login
register
mail settings
Submitter Stefan Roese
Date June 14, 2013, 8:54 a.m.
Message ID <1371200101-11510-1-git-send-email-sr@denx.de>
Download mbox | patch
Permalink /patch/251293/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Stefan Roese - June 14, 2013, 8:54 a.m.
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(+)
Albert ARIBAUD - June 20, 2013, 4:42 p.m.
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,
Stefan Roese - June 20, 2013, 5:01 p.m.
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
Albert ARIBAUD - June 20, 2013, 5:51 p.m.
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,
Stefan Roese - June 20, 2013, 6:28 p.m.
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
Albert ARIBAUD - June 20, 2013, 7:18 p.m.
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,
Tom Rini - July 15, 2013, 2:33 p.m.
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!
Stefan Roese - July 16, 2013, 6:24 a.m.
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
Tom Rini - July 16, 2013, 2:36 p.m.
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.

Patch

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