Hi Stefan, On Thu, 20 Jun 2013 19:01:22 +0200, Stefan Roese <s...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
