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

[Albert ARIBAUD]

Hi Stefan,

On Thu, 20 Jun 2013 19:01:22 +0200, Stefan Roese <sr at 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,
-- 
Albert.