[U-Boot] [RFC 0/3] ARM: cleanup gd init
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(-)
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
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
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); }
--- 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();
Hello,
On 08/24/2013 06:32 PM, Jeroen Hofstee wrote:
- 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
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:
- 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,
participants (2)
-
Albert ARIBAUD -
Jeroen Hofstee