[U-Boot] [RFC 0/3] ARM: cleanup gd init

[U-Boot] [RFC 0/3] ARM: cleanup gd init

[Jeroen Hofstee]

Hello,

As I noticed at [1] gd is setup multiple times. As pointed
Tom pointed out out me this actually leads to problems as
well, see [2]. These patches attempt to cleanup gd usage
(a bit, board_f.c is not included since that one is rather
trivial), some questions I have about it:

1) The major one, do these init changes brick any board?
2) Is there any board which needs gdata as a global?
3) What to do with lowlevel_init on armv7. Hide it under
a CONFIG_*, make it weak.. or just remove it completely?
4) Keep the s_init in crt0.S or move it to the board_init_f?
The disadvantage of the later is that all the different
board_init_f's need to call system_init.
5) Where to put the __weak s_init (or hide this under a define)

Regards,
Jeroen

[1] http://lists.denx.de/pipermail/u-boot/2013-August/160933.html
[2] http://lists.denx.de/pipermail/u-boot/2013-July/158144.html

Jeroen Hofstee (3):
  ARM,crt0.S: call s_init instead from ctr0.S
  ARM,crt0.S: optional init gd to gdata for spl
  ARM: do not assign gd outside of crt0.S

 arch/arm/cpu/arm926ejs/davinci/spl.c           |  3 +--
 arch/arm/cpu/armv7/exynos/spl_boot.c           |  6 ++----
 arch/arm/cpu/armv7/lowlevel_init.S             | 23 +----------------------
 arch/arm/cpu/armv7/omap-common/hwinit-common.c |  2 --
 arch/arm/cpu/armv7/omap3/board.c               |  2 --
 arch/arm/cpu/armv7/omap3/lowlevel_init.S       |  8 ++++----
 arch/arm/cpu/armv7/rmobile/lowlevel_init.S     |  6 ------
 arch/arm/lib/crt0.S                            |  8 ++++++++
 arch/arm/lib/reset.c                           |  5 +++++
 arch/arm/lib/spl.c                             |  5 ++---
 board/isee/igep0033/board.c                    |  1 -
 board/phytec/pcm051/board.c                    |  2 --
 board/ti/am335x/board.c                        |  2 --
 board/ti/omap5912osk/lowlevel_init.S           | 11 -----------
 board/ti/ti814x/evm.c                          |  2 --
 board/woodburn/woodburn.c                      |  3 ---
 16 files changed, 23 insertions(+), 66 deletions(-)

-- 
1.8.1.2

[U-Boot] [RFC 1/3] ARM,crt0.S: call s_init instead from ctr0.S

[Jeroen Hofstee]

The only reason gd is setup before _main seems to be
the call to s_init. Therefore call s_init in crt0.S
after gd has been setup. (It might become system_init
one day, but that has the disadvantage that all versions
of board_init_f must call system_init).
---
 arch/arm/cpu/armv7/lowlevel_init.S         | 23 +----------------------
 arch/arm/cpu/armv7/omap3/lowlevel_init.S   |  8 ++++----
 arch/arm/cpu/armv7/rmobile/lowlevel_init.S |  6 ------
 arch/arm/lib/crt0.S                        |  3 +++
 arch/arm/lib/reset.c                       |  5 +++++
 board/ti/omap5912osk/lowlevel_init.S       | 11 -----------
 6 files changed, 13 insertions(+), 43 deletions(-)

diff --git a/arch/arm/cpu/armv7/lowlevel_init.S b/arch/arm/cpu/armv7/lowlevel_init.S
index 82b2b86..1ec7481 100644
--- a/arch/arm/cpu/armv7/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/lowlevel_init.S
@@ -16,26 +16,5 @@
 #include <linux/linkage.h>
 
 ENTRY(lowlevel_init)
-	/*
-	 * Setup a temporary stack
-	 */
-	ldr	sp, =CONFIG_SYS_INIT_SP_ADDR
-	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
-#ifdef CONFIG_SPL_BUILD
-	ldr	r8, =gdata
-#else
-	sub	sp, #GD_SIZE
-	bic	sp, sp, #7
-	mov	r8, sp
-#endif
-	/*
-	 * Save the old lr(passed in ip) and the current lr to stack
-	 */
-	push	{ip, lr}
-
-	/*
-	 * go setup pll, mux, memory
-	 */
-	bl	s_init
-	pop	{ip, pc}
+	mov	pc, lr
 ENDPROC(lowlevel_init)
diff --git a/arch/arm/cpu/armv7/omap3/lowlevel_init.S b/arch/arm/cpu/armv7/omap3/lowlevel_init.S
index bdf74ea..db1d4dc 100644
--- a/arch/arm/cpu/armv7/omap3/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/omap3/lowlevel_init.S
@@ -197,22 +197,22 @@ pll_div_val5:
 #endif
 
 ENTRY(lowlevel_init)
+#if !defined(CONFIG_SYS_NAND_BOOT) && !defined(CONFIG_SYS_ONENAND_BOOT)
 	ldr	sp, SRAM_STACK
 	str	ip, [sp]	/* stash ip register */
 	mov	ip, lr		/* save link reg across call */
-#if !defined(CONFIG_SYS_NAND_BOOT) && !defined(CONFIG_SYS_ONENAND_BOOT)
 /*
  * No need to copy/exec the clock code - DPLL adjust already done
  * in NAND/oneNAND Boot.
  */
 	ldr	r1, =SRAM_CLK_CODE
 	bl	cpy_clk_code
-#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
+#endif /* NAND Boot */
 
+	mov	pc, lr
 ENDPROC(lowlevel_init)
 
 	/* the literal pools origin */
diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
index ff18d96..70ec22d 100644
--- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
@@ -59,14 +59,8 @@ loop0:
 	subs r0, r0, #1
 	bne  loop0
 
-	ldr sp, MERAM_STACK
-	b s_init
-
 	.pool
 	.align 4
 
 ENDPROC(lowlevel_init)
 	.ltorg
-
-MERAM_STACK:
-	.word LOW_LEVEL_MERAM_STACK
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 960d12e..98d7881 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -70,6 +70,9 @@ ENTRY(_main)
 	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 */
+
+	bl	s_init
+
 	mov	r0, #0
 	bl	board_init_f
 
diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
index 7a03580..d14eac9 100644
--- a/arch/arm/lib/reset.c
+++ b/arch/arm/lib/reset.c
@@ -35,3 +35,8 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	/*NOTREACHED*/
 	return 0;
 }
+
+/* HACK */
+__weak void s_init(void)
+{
+}
diff --git a/board/ti/omap5912osk/lowlevel_init.S b/board/ti/omap5912osk/lowlevel_init.S
index cad0a5a..61c8605 100644
--- a/board/ti/omap5912osk/lowlevel_init.S
+++ b/board/ti/omap5912osk/lowlevel_init.S
@@ -296,17 +296,6 @@ common_tc:
 	ldr	sp,	SRAM_STACK
 	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 
-	/*
-	 * Save the old lr(passed in ip) and the current lr to stack
-	 */
-	push	{ip, lr}
-
-	/*
-	 * go setup pll, mux, memory
-	 */
-	bl	s_init
-	pop	{ip, pc}
-
 	/* back to arch calling code */
 	mov	pc,	lr
 
-- 
1.8.1.2

[U-Boot] [RFC 2/3] ARM,crt0.S: optional init gd to gdata for spl

[Jeroen Hofstee]

The common/spl changes gd to gdata in board_init_f.
I fail to see why. The only reason I can think of to
use the gdata is the case where is won't fit into the
region where the stack lives.

Move the init to crt0.S and make it a CONFIG.
---
 arch/arm/lib/crt0.S | 5 +++++
 arch/arm/lib/spl.c  | 5 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 98d7881..b8fa10e 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -67,9 +67,14 @@ ENTRY(_main)
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
 #endif
 	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */
+
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_GD_GLOBAL)
+	ldr r8, =gdata	/* SPL assigns GD 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
 
 	bl	s_init
 
diff --git a/arch/arm/lib/spl.c b/arch/arm/lib/spl.c
index 583bdb3..52dba2f 100644
--- a/arch/arm/lib/spl.c
+++ b/arch/arm/lib/spl.c
@@ -15,7 +15,9 @@
 
 /* Pointer to as well as the global data structure for SPL */
 DECLARE_GLOBAL_DATA_PTR;
+#ifdef CONFIG_SPL_GD_GLOBAL
 gd_t gdata __attribute__ ((section(".data")));
+#endif
 
 /*
  * In the context of SPL, board_init_f must ensure that any clocks/etc for
@@ -31,9 +33,6 @@ void __weak board_init_f(ulong dummy)
 	/* Clear the BSS. */
 	memset(__bss_start, 0, __bss_end - __bss_start);
 
-	/* Set global data pointer. */
-	gd = &gdata;
-
 	board_init_r(NULL, 0);
 }
 
-- 
1.8.1.2

[U-Boot] [RFC 3/3] ARM: do not assign gd outside of crt0.S

[Jeroen Hofstee]

---
 arch/arm/cpu/arm926ejs/davinci/spl.c           | 3 +--
 arch/arm/cpu/armv7/exynos/spl_boot.c           | 6 ++----
 arch/arm/cpu/armv7/omap-common/hwinit-common.c | 2 --
 arch/arm/cpu/armv7/omap3/board.c               | 2 --
 board/isee/igep0033/board.c                    | 1 -
 board/phytec/pcm051/board.c                    | 2 --
 board/ti/am335x/board.c                        | 2 --
 board/ti/ti814x/evm.c                          | 2 --
 board/woodburn/woodburn.c                      | 3 ---
 9 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
index 59b304e..0af3646 100644
--- a/arch/arm/cpu/arm926ejs/davinci/spl.c
+++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
@@ -50,8 +50,7 @@ void board_init_f(ulong dummy)
 	/* Third, we clear the BSS. */
 	memset(__bss_start, 0, __bss_end - __bss_start);
 
-	/* Finally, setup gd and move to the next step. */
-	gd = &gdata;
+	/* Finally move to the next step. */
 	board_init_r(NULL, 0);
 }
 
diff --git a/arch/arm/cpu/armv7/exynos/spl_boot.c b/arch/arm/cpu/armv7/exynos/spl_boot.c
index 3651c00..c0287aa 100644
--- a/arch/arm/cpu/armv7/exynos/spl_boot.c
+++ b/arch/arm/cpu/armv7/exynos/spl_boot.c
@@ -149,9 +149,8 @@ void memzero(void *s, size_t n)
  *
  * @param gdp   Value to give to gd
  */
-static void setup_global_data(gd_t *gdp)
+static void setup_global_data(void)
 {
-	gd = gdp;
 	memzero((void *)gd, sizeof(gd_t));
 	gd->flags |= GD_FLG_RELOC;
 	gd->baudrate = CONFIG_BAUDRATE;
@@ -160,10 +159,9 @@ static void setup_global_data(gd_t *gdp)
 
 void board_init_f(unsigned long bootflag)
 {
-	__aligned(8) gd_t local_gd;
 	__attribute__((noreturn)) void (*uboot)(void);
 
-	setup_global_data(&local_gd);
+	setup_global_data();
 
 	if (do_lowlevel_init())
 		power_exit_wakeup();
diff --git a/arch/arm/cpu/armv7/omap-common/hwinit-common.c b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
index 85d3754..69cbfd6 100644
--- a/arch/arm/cpu/armv7/omap-common/hwinit-common.c
+++ b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
@@ -143,8 +143,6 @@ void s_init(void)
 	srcomp_enable();
 	setup_clocks_for_console();
 
-	gd = &gdata;
-
 	preloader_console_init();
 	do_io_settings();
 #endif
diff --git a/arch/arm/cpu/armv7/omap3/board.c b/arch/arm/cpu/armv7/omap3/board.c
index 7d1f8d9..3c0042d 100644
--- a/arch/arm/cpu/armv7/omap3/board.c
+++ b/arch/arm/cpu/armv7/omap3/board.c
@@ -240,8 +240,6 @@ void s_init(void)
 #endif
 
 #ifdef CONFIG_SPL_BUILD
-	gd = &gdata;
-
 	preloader_console_init();
 
 	timer_init();
diff --git a/board/isee/igep0033/board.c b/board/isee/igep0033/board.c
index c0f0c0d..8cb9e4c 100644
--- a/board/isee/igep0033/board.c
+++ b/board/isee/igep0033/board.c
@@ -102,7 +102,6 @@ void s_init(void)
 	enable_uart0_pin_mux();
 
 	uart_soft_reset();
-	gd = &gdata;
 
 	preloader_console_init();
 
diff --git a/board/phytec/pcm051/board.c b/board/phytec/pcm051/board.c
index 6291d03..3bf45ac 100644
--- a/board/phytec/pcm051/board.c
+++ b/board/phytec/pcm051/board.c
@@ -113,8 +113,6 @@ void s_init(void)
 	enable_uart0_pin_mux();
 	uart_soft_reset();
 
-	gd = &gdata;
-
 	preloader_console_init();
 
 	/* Initalize the board header */
diff --git a/board/ti/am335x/board.c b/board/ti/am335x/board.c
index 7138d73..f11eb93 100644
--- a/board/ti/am335x/board.c
+++ b/board/ti/am335x/board.c
@@ -328,8 +328,6 @@ void s_init(void)
 
 	uart_soft_reset();
 
-	gd = &gdata;
-
 	preloader_console_init();
 
 	/* Initalize the board header */
diff --git a/board/ti/ti814x/evm.c b/board/ti/ti814x/evm.c
index 17fba5a..7800a4d 100644
--- a/board/ti/ti814x/evm.c
+++ b/board/ti/ti814x/evm.c
@@ -143,8 +143,6 @@ void s_init(void)
 	/* Enable UART */
 	uart_enable();
 
-	gd = &gdata;
-
 	preloader_console_init();
 
 	config_dmm(&evm_lisa_map_regs);
diff --git a/board/woodburn/woodburn.c b/board/woodburn/woodburn.c
index 2744514..3da61a4 100644
--- a/board/woodburn/woodburn.c
+++ b/board/woodburn/woodburn.c
@@ -137,9 +137,6 @@ void board_init_f(ulong dummy)
 	/* Clear the BSS. */
 	memset(__bss_start, 0, __bss_end - __bss_start);
 
-	/* Set global data pointer. */
-	gd = &gdata;
-
 	preloader_console_init();
 	timer_init();
 
-- 
1.8.1.2

[U-Boot] [RFC 1/3] ARM,crt0.S: call s_init instead from ctr0.S

[Jeroen Hofstee]

On 08/24/2013 06:32 PM, Jeroen Hofstee wrote:
>   	/* the literal pools origin */
> diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
> index ff18d96..70ec22d 100644
> --- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
> +++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S
> @@ -59,14 +59,8 @@ loop0:
>   	subs r0, r0, #1
>   	bne  loop0
>   
> -	ldr sp, MERAM_STACK
> -	b s_init
> -
right, this ^^^ will brick things nicely, how about returning to the 
caller...
>   	.pool
>   	.align 4
>   
>   ENDPROC(lowlevel_init)
>   	.ltorg
> -
> -MERAM_STACK:
> -	.word LOW_LEVEL_MERAM_STACK

[U-Boot] [RFC 0/3] ARM: cleanup gd init

[Jeroen Hofstee]

Hello,

On 08/24/2013 06:32 PM, Jeroen Hofstee wrote:
> 4) Keep the s_init in crt0.S or move it to the board_init_f?
> The disadvantage of the later is that all the different
> board_init_f's need to call system_init.
Since this has been on the mailing-list for a month without a reply,
let's push this a bit.

Moving s_init to board_init_f is not smart since there are many
board_init_f already and likely there will be more in the future
since SPL uses it as well. It also provides a chance to save bootrom
registers to gd. After boad_init_f r1 is clobbered at least..

Regards,
Jeroen

[U-Boot] [RFC 0/3] ARM: cleanup gd init

[Albert ARIBAUD]

Hi Jeroen,

On Sun, 22 Sep 2013 20:26:07 +0200, Jeroen Hofstee
<jeroen@myspectrum.nl> wrote:

> Hello,
> 
> On 08/24/2013 06:32 PM, Jeroen Hofstee wrote:
> > 4) Keep the s_init in crt0.S or move it to the board_init_f?
> > The disadvantage of the later is that all the different
> > board_init_f's need to call system_init.
> Since this has been on the mailing-list for a month without a reply,
> let's push this a bit.
> 
> Moving s_init to board_init_f is not smart since there are many
> board_init_f already and likely there will be more in the future
> since SPL uses it as well. It also provides a chance to save bootrom
> registers to gd. After boad_init_f r1 is clobbered at least..

Not sure what you mean by there being "many" board_init_f()s -- I
count five occurrences, one of which is the standard board_init_f(),
one is a weak default in SPL, and three are custom versions; the last
four may or may not be merged into 'the' board_init_f() one.

Regarding saving bootrom (or other) registers, that is a feature for
start.S, more precisely from _start, which is the only place in the
whole of U-Boot that has not clobbered any register yet.

The right process is thus to split s_init(): any part that deals with
saving registers at boot should go in start.S; any part that deals
with anything else and is not absolutely required before entering
crt0.S should move in a function called from board_init_f().

> Regards,
> Jeroen

Amicalement,
-- 
Albert.