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

login
register
mail settings
Submitter Stefan Roese
Date June 25, 2013, 7:14 a.m.
Message ID <1372144452-14108-1-git-send-email-sr@denx.de>
Download mbox | patch
Permalink /patch/254044/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Stefan Roese - June 25, 2013, 7:14 a.m.
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(-)
Albert ARIBAUD - June 27, 2013, 8:27 a.m.
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,
Tom Rini - July 3, 2013, 7:47 p.m.
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!
Albert ARIBAUD - July 4, 2013, 11:58 a.m.
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,

Patch

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