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