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,