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

login
register
mail settings
Submitter Stefan Roese
Date June 21, 2013, 2:13 a.m.
Message ID <1371780797-17248-1-git-send-email-sr@denx.de>
Download mbox | patch
Permalink /patch/253118/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Stefan Roese - June 21, 2013, 2:13 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>
---
v2:
- Change handling/initializing of r8 as suggested by Albert.
  It should only be written in crt0.S.
  
Tom, while working on this version one question came up:
Is lowlevel_init() (file arch/arm/cpu/armv7/omap3/lowlevel_init.S)
needed any more? It calls cpy_clk_code() to copy some clk init
code into SRAM. But I fail to see if and where this code is really
executed from SRAM. Maybe I missed something. Perhaps you could
shed some light into this.

Thanks, Stefan

 arch/arm/cpu/armv7/omap3/board.c         | 2 --
 arch/arm/cpu/armv7/omap3/lowlevel_init.S | 3 +--
 arch/arm/lib/crt0.S                      | 7 ++++++-
 3 files changed, 7 insertions(+), 5 deletions(-)
Albert ARIBAUD - June 21, 2013, 8:57 a.m.
Hi Stefan,

On Fri, 21 Jun 2013 04:13:17 +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>
> ---
> v2:
> - Change handling/initializing of r8 as suggested by Albert.
>   It should only be written in crt0.S.
>   
> Tom, while working on this version one question came up:
> Is lowlevel_init() (file arch/arm/cpu/armv7/omap3/lowlevel_init.S)
> needed any more? It calls cpy_clk_code() to copy some clk init
> code into SRAM. But I fail to see if and where this code is really
> executed from SRAM. Maybe I missed something. Perhaps you could
> shed some light into this.
> 
> Thanks, Stefan
> 
>  arch/arm/cpu/armv7/omap3/board.c         | 2 --
>  arch/arm/cpu/armv7/omap3/lowlevel_init.S | 3 +--
>  arch/arm/lib/crt0.S                      | 7 ++++++-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> 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..0f8d9f5 100644
> --- a/arch/arm/lib/crt0.S
> +++ b/arch/arm/lib/crt0.S
> @@ -85,8 +85,13 @@ ENTRY(_main)
>  	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */

Actually, this...

>  	sub	sp, #GD_SIZE	/* allocate one GD above SP */
>  	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */

... should also be moved under the #else clause below -- no need to
eat up valuable stack space when GD is in gdata.

> -	mov	r8, sp		/* GD is above SP */

Also, this...

>  	mov	r0, #0

... should remain just before the call to board_init_f which needs it.
Otherwise you run the (non-null) risk that r0 be non-zero on return
from s_init and then this non-zero value be passed to board_init_f.

> +#if defined(CONFIG_SPL_BUILD)
> +	ldr	r8, =gdata	/* SPL assigns r8 directly to &gdata */
> +	bl	s_init		/* s_init() needs GD to be setup */
> +#else
> +	mov	r8, sp		/* GD is above SP */
> +#endif
>  	bl	board_init_f
>  
>  #if ! defined(CONFIG_SPL_BUILD)

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..0f8d9f5 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -85,8 +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 */
-	mov	r8, sp		/* GD is above SP */
 	mov	r0, #0
+#if defined(CONFIG_SPL_BUILD)
+	ldr	r8, =gdata	/* SPL assigns r8 directly to &gdata */
+	bl	s_init		/* s_init() needs GD to be setup */
+#else
+	mov	r8, sp		/* GD is above SP */
+#endif
 	bl	board_init_f
 
 #if ! defined(CONFIG_SPL_BUILD)