| Message ID | 1372144452-14108-1-git-send-email-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Stefan, On Tue, 25 Jun 2013 09:14:12 +0200, Stefan Roese <sr@denx.de> wrote: > Fix a problem with a re-assignment of r8 in the SPL version. > > This patch now moves the call to s_init() to a later stage, right before > calling board_init_f(). And makes sure that r8 is correctly initialized > before s_init() is called. r8 now is only written in crt0.S. > > This error was detected on the SPL port for the Compulab CM-T35 board > (OMAP3530). > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Tom Rini <trini@ti.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > Albert, I'm not really happy with this patch as it evolves now. As you > will see, I had to make some further additions to crt0.S to fix a > problem for non-SPL builds and to fix compilation errors for non-OMAP > platforms. This gets quite ugly now. Looking back at my patch v1, this > looks much less intrusive. > > What do you think? I said the first patch was NAK, and the reasons I NAKed it remain. However, there might be another solution: instead of squeezing the call to s_init() in crt0.S right between the initial environment setting and the call to board_init_f(), we could simply move the s_init() call inside board_init_f(). From a running conditions perspective, the only change would be that s_init() is going to run from a non-empty stack, but we know that there is free stack enough during board_init_f() to call functions. Moving the call to s_init() into board_init_f() removes any changes to crt0.S, which were my essential NAK reason and saves you some ugliness. I would even hazard that you could place s_init in init_sequence[], for instance as a first entry to be called (before arch_cpu_init). After all, the only difference in execution is that gdata is going to be initialized properly before s_init() kicks in. Also, a name change would be in order, because s_init() as a private OMAP function is ok, but as an init function invoked from board_init_f() it needs a more meaningful name. Amicalement,
On Thu, Jun 27, 2013 at 10:27:26AM +0200, Albert ARIBAUD wrote: > Hi Stefan, > > On Tue, 25 Jun 2013 09:14:12 +0200, Stefan Roese <sr@denx.de> wrote: > > > Fix a problem with a re-assignment of r8 in the SPL version. > > > > This patch now moves the call to s_init() to a later stage, right before > > calling board_init_f(). And makes sure that r8 is correctly initialized > > before s_init() is called. r8 now is only written in crt0.S. > > > > This error was detected on the SPL port for the Compulab CM-T35 board > > (OMAP3530). > > > > Signed-off-by: Stefan Roese <sr@denx.de> > > Cc: Tom Rini <trini@ti.com> > > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > > --- > > Albert, I'm not really happy with this patch as it evolves now. As you > > will see, I had to make some further additions to crt0.S to fix a > > problem for non-SPL builds and to fix compilation errors for non-OMAP > > platforms. This gets quite ugly now. Looking back at my patch v1, this > > looks much less intrusive. > > > > What do you think? > > I said the first patch was NAK, and the reasons I NAKed it remain. > > However, there might be another solution: instead of squeezing the > call to s_init() in crt0.S right between the initial environment > setting and the call to board_init_f(), we could simply move the > s_init() call inside board_init_f(). > > From a running conditions perspective, the only change would be that > s_init() is going to run from a non-empty stack, but we know that there > is free stack enough during board_init_f() to call functions. > > Moving the call to s_init() into board_init_f() removes any changes to > crt0.S, which were my essential NAK reason and saves you some ugliness. > > I would even hazard that you could place s_init in init_sequence[], for > instance as a first entry to be called (before arch_cpu_init). After > all, the only difference in execution is that gdata is going to be > initialized properly before s_init() kicks in. > > Also, a name change would be in order, because s_init() as a private > OMAP function is ok, but as an init function invoked from board_init_f() > it needs a more meaningful name. So, this is one of the things that needs resolving for v2013.07. What do you want to call the renamed s_init function? I think we need to go the init_sequence route, and keep common/board_f.c in sync (I did a trivial test the other week about moving am335x into the generic board framework and it went fine, so I'll want to move all the TI boards I can over soon). Thanks!
Hi Tom, On Wed, 3 Jul 2013 15:47:27 -0400, Tom Rini <trini@ti.com> wrote: > On Thu, Jun 27, 2013 at 10:27:26AM +0200, Albert ARIBAUD wrote: > > Hi Stefan, > > > > On Tue, 25 Jun 2013 09:14:12 +0200, Stefan Roese <sr@denx.de> wrote: > > > > > Fix a problem with a re-assignment of r8 in the SPL version. > > > > > > This patch now moves the call to s_init() to a later stage, right before > > > calling board_init_f(). And makes sure that r8 is correctly initialized > > > before s_init() is called. r8 now is only written in crt0.S. > > > > > > This error was detected on the SPL port for the Compulab CM-T35 board > > > (OMAP3530). > > > > > > Signed-off-by: Stefan Roese <sr@denx.de> > > > Cc: Tom Rini <trini@ti.com> > > > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > > > --- > > > Albert, I'm not really happy with this patch as it evolves now. As you > > > will see, I had to make some further additions to crt0.S to fix a > > > problem for non-SPL builds and to fix compilation errors for non-OMAP > > > platforms. This gets quite ugly now. Looking back at my patch v1, this > > > looks much less intrusive. > > > > > > What do you think? > > > > I said the first patch was NAK, and the reasons I NAKed it remain. > > > > However, there might be another solution: instead of squeezing the > > call to s_init() in crt0.S right between the initial environment > > setting and the call to board_init_f(), we could simply move the > > s_init() call inside board_init_f(). > > > > From a running conditions perspective, the only change would be that > > s_init() is going to run from a non-empty stack, but we know that there > > is free stack enough during board_init_f() to call functions. > > > > Moving the call to s_init() into board_init_f() removes any changes to > > crt0.S, which were my essential NAK reason and saves you some ugliness. > > > > I would even hazard that you could place s_init in init_sequence[], for > > instance as a first entry to be called (before arch_cpu_init). After > > all, the only difference in execution is that gdata is going to be > > initialized properly before s_init() kicks in. > > > > Also, a name change would be in order, because s_init() as a private > > OMAP function is ok, but as an init function invoked from board_init_f() > > it needs a more meaningful name. > > So, this is one of the things that needs resolving for v2013.07. What > do you want to call the renamed s_init function? I think we need to go > the init_sequence route, and keep common/board_f.c in sync (I did a > trivial test the other week about moving am335x into the generic board > framework and it went fine, so I'll want to move all the TI boards I can > over soon). Thanks! I have no strong opinion on the name... As is does mux and clock inits needed by the 'system' (which for all I know may well be where the "s" of s_init comes from), we could simply name it system_init(). Amicalement,
diff --git a/arch/arm/cpu/armv7/omap3/board.c b/arch/arm/cpu/armv7/omap3/board.c index b72fadc..8f41dcd 100644 --- a/arch/arm/cpu/armv7/omap3/board.c +++ b/arch/arm/cpu/armv7/omap3/board.c @@ -256,8 +256,6 @@ void s_init(void) #endif #ifdef CONFIG_SPL_BUILD - gd = &gdata; - preloader_console_init(); timer_init(); diff --git a/arch/arm/cpu/armv7/omap3/lowlevel_init.S b/arch/arm/cpu/armv7/omap3/lowlevel_init.S index eacfef8..8539093 100644 --- a/arch/arm/cpu/armv7/omap3/lowlevel_init.S +++ b/arch/arm/cpu/armv7/omap3/lowlevel_init.S @@ -226,8 +226,7 @@ ENTRY(lowlevel_init) #endif /* NAND Boot */ mov lr, ip /* restore link reg */ ldr ip, [sp] /* restore save ip */ - /* tail-call s_init to setup pll, mux, memory */ - b s_init + mov pc, lr ENDPROC(lowlevel_init) diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index a5bffb8..78eb951 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -83,9 +83,17 @@ ENTRY(_main) ldr sp, =(CONFIG_SYS_INIT_SP_ADDR) #endif bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ +#if defined(CONFIG_OMAP34XX) && defined(CONFIG_SPL_BUILD) + ldr r8, =gdata /* SPL assigns r8 directly to &gdata */ +#else sub sp, #GD_SIZE /* allocate one GD above SP */ bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ mov r8, sp /* GD is above SP */ +#endif +#if defined(CONFIG_OMAP34XX) && \ + (defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SKIP_LOWLEVEL_INIT)) + bl s_init /* s_init() needs GD to be setup */ +#endif mov r0, #0 bl board_init_f
Fix a problem with a re-assignment of r8 in the SPL version. This patch now moves the call to s_init() to a later stage, right before calling board_init_f(). And makes sure that r8 is correctly initialized before s_init() is called. r8 now is only written in crt0.S. This error was detected on the SPL port for the Compulab CM-T35 board (OMAP3530). Signed-off-by: Stefan Roese <sr@denx.de> Cc: Tom Rini <trini@ti.com> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> --- Albert, I'm not really happy with this patch as it evolves now. As you will see, I had to make some further additions to crt0.S to fix a problem for non-SPL builds and to fix compilation errors for non-OMAP platforms. This gets quite ugly now. Looking back at my patch v1, this looks much less intrusive. What do you think? v5: - Fix a problem with non-SPL build on OMAP3 where s_init() needs to get called (if CONFIG_SKIP_LOWLEVEL_INIT is not defined). With the "code shuffling" in v3, s_init() was only called in the SPL U-Boot version. - Fix compilation problem for non-OMAP platforms with assigning gdata to r8 (e.g. tx25). v4: - Corrected commit text to reflect changed patch v3: - Some code shuffling in crt0.S as requested by Albert v2: - Change handling/initializing of r8 as suggested by Albert. It should only be written in crt0.S. arch/arm/cpu/armv7/omap3/board.c | 2 -- arch/arm/cpu/armv7/omap3/lowlevel_init.S | 3 +-- arch/arm/lib/crt0.S | 8 ++++++++ 3 files changed, 9 insertions(+), 4 deletions(-)