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

[Albert ARIBAUD]

Hi Stefan,

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