On Tue, Apr 08, 2014 at 03:02:44PM +0200, Wolfgang Denk wrote: > Dear Tom, > > In message <20140407195731.GL23803@bill-the-cat> you wrote: > > > > Maybe, maybe not. But it's beside the point. The point is we have SoC > > init code that needs to be run once. Sometimes this is true when > > CONFIG_SPL_BUILD=y, sometimes this is true when other options are set > > instead. What should we call the option to denote one-time init? > > Should we extend CONFIG_SKIP_LOWLEVEL_INIT to cover this case? > > Um... I think, if we build (and use) the SPL, then the low level init > code would always be run (if at all) in the SPL; without SPL, it would > be run in the normal U-Boot code. CONFIG_SKIP_LOWLEVEL_INIT is > intended to skip running this low level init code. To me this looks > independent of where this init code is located - it is yet another > orthogonal decision. > > Which sort of extension do you have in mind here?
Maybe talking about this in patches will be clearer. If instead of
adding a new CONFIG option, I use CONFIG_SKIP_LOWLEVEL_INIT like this!
diff --git a/arch/arm/cpu/armv7/am33xx/board.c
b/arch/arm/cpu/armv7/am33xx/board.c
index fb44cc8..28c16f8 100644
--- a/arch/arm/cpu/armv7/am33xx/board.c
+++ b/arch/arm/cpu/armv7/am33xx/board.c
@@ -142,7 +142,7 @@ int arch_misc_init(void)
return 0;
}
-#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_NOR_BOOT)
+#ifndef CONFIG_SKIP_LOWLEVEL_INIT
/*
* This function is the place to do per-board things such as ramp up the
* MPU clock frequency.
@@ -200,9 +200,7 @@ static void watchdog_disable(void)
while (readl(&wdtimer->wdtwwps) != 0x0)
;
}
-#endif
-#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_NOR_BOOT)
void s_init(void)
{
/*
diff --git a/include/configs/ti_am335x_common.h
b/include/configs/ti_am335x_common.h
index 50c3203..89d14c8 100644
--- a/include/configs/ti_am335x_common.h
+++ b/include/configs/ti_am335x_common.h
@@ -69,7 +69,8 @@
* Since SPL did pll and ddr initialization for us,
* we don't need to do it twice.
*/
-#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_NOR_BOOT)
+#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_NOR_BOOT) && \
+ !defined(CONFIG_QSPI_BOOT)
#define CONFIG_SKIP_LOWLEVEL_INIT
#endif
The following is true:
a) am335x_evm build, SPL calls s_init, does DDR init, u-boot.img
continues to not do so, just like today.
b) am335x_evm_norboot, _no_ SPL, u-boot.bin calls s_init, does DDR init,
just like today.
c) am43xx_evm build, SPL and u-boot.img work just like am335x_evm
d) am43xx_evm_qspiboot, _no_ SPL, u-boot.bin calls s_init, does DDR
init, just like today with the patch series that adds the target.
If instead we add a new option (for the moment, CONFIG_SYS_XIP_BOOT):
diff --git a/arch/arm/cpu/armv7/am33xx/board.c
b/arch/arm/cpu/armv7/am33xx/board.c
index fb44cc8..133ca1c 100644
--- a/arch/arm/cpu/armv7/am33xx/board.c
+++ b/arch/arm/cpu/armv7/am33xx/board.c
@@ -142,7 +142,7 @@ int arch_misc_init(void)
return 0;
}
-#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_NOR_BOOT)
+#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_SYS_XIP_BOOT)
/*
* This function is the place to do per-board things such as ramp up the
* MPU clock frequency.
@@ -200,9 +200,7 @@ static void watchdog_disable(void)
while (readl(&wdtimer->wdtwwps) != 0x0)
;
}
-#endif
-#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_NOR_BOOT)
void s_init(void)
{
/*
diff --git a/boards.cfg b/boards.cfg
index b4203f1..b5a79d0 100644
--- a/boards.cfg
+++ b/boards.cfg
@@ -267,7 +267,7 @@ Active arm armv7 am33xx silica
pengwyn
Active arm armv7 am33xx ti am335x
am335x_boneblack
am335x_evm:SERIAL1,CONS_INDEX=1,EMMC_BOOT
Tom Rini <tr...@ti.com>
Active arm armv7 am33xx ti am335x
am335x_evm am335x_evm:SERIAL1,CONS_INDEX=1,NAND
Tom Rini <tr...@ti.com>
Active arm armv7 am33xx ti am335x
am335x_evm_nor
am335x_evm:SERIAL1,CONS_INDEX=1,NAND,NOR
Tom Rini <tr...@ti.com>
-Active arm armv7 am33xx ti am335x
am335x_evm_norboot
am335x_evm:SERIAL1,CONS_INDEX=1,NOR,NOR_BOOT
Tom Rini <tr...@ti.com>
+Active arm armv7 am33xx ti am335x
am335x_evm_norboot
am335x_evm:SERIAL1,CONS_INDEX=1,NOR,NOR_BOOT,SYS_XIP_BOOT
Tom Rini <tr...@ti.com>
Active arm armv7 am33xx ti am335x
am335x_evm_spiboot
am335x_evm:SERIAL1,CONS_INDEX=1,SPI_BOOT
Tom Rini <tr...@ti.com>
Active arm armv7 am33xx ti am335x
am335x_evm_uart1 am335x_evm:SERIAL2,CONS_INDEX=2,NAND
Tom Rini <tr...@ti.com>
Active arm armv7 am33xx ti am335x
am335x_evm_uart2 am335x_evm:SERIAL3,CONS_INDEX=3,NAND
Tom Rini <tr...@ti.com>
diff --git a/include/configs/ti_am335x_common.h
b/include/configs/ti_am335x_common.h
index 50c3203..1092c25 100644
--- a/include/configs/ti_am335x_common.h
+++ b/include/configs/ti_am335x_common.h
@@ -69,7 +69,8 @@
* Since SPL did pll and ddr initialization for us,
* we don't need to do it twice.
*/
-#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_NOR_BOOT)
+#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_NOR_BOOT) && \
+ !defined(CONFIG_QSPI_BOOT)
#define CONFIG_SKIP_LOWLEVEL_INIT
#endif
Pasting it out, I like using CONFIG_SKIP_LOWLEVEL_INIT more to cover
this code. Does that work for you?
> > > If you use the memory mapped mode, then it looks just like any
> > > other ROM, and we should not need special code either.
> >
> > Well, for CONFIG_NOR_BOOT we must have it on am335x (haven't checked
> > am43xx) to finish doing pinmux as only 12KiB is mapped, in addition to
> > env related options. On am43xx and QSPI it's just used to denote when
> > we are doing one-time init in U-Boot proper rather than SPL.
>
> Sorry, I lost you. what is the "it" in both sentences referring to?
First it is the running binary (in this case, U-Boot). The second it is
CONFIG_QSPI_BOOT.
> Also, I'm not sure what exactly you mean by "one-time"init - all
> initialization steps in init_sequence[] are executed only once.
Right, but we don't initalize DDR twice, especially if we're running
from DDR.
> These settings you are referring to - can these me made here, or is
> this stuff that is running before board_init_f() ?
>
>
> Um... I just ran over this in "arch/arm/lib/spl.c":
>
> 21 * In the context of SPL, board_init_f must ensure that any clocks/etc for
> 22 * DDR are enabled, ensure that the stack pointer is valid, clear the BSS
> 23 * and call board_init_f. We provide this version by default but mark it
> ^^^^^^^^^^^^^^^^^
> 24 * as __weak to allow for platforms to do this in their own way if needed.
> 25 */
> 26 void __weak board_init_f(ulong dummy)
> 27 {
> 28 /* Clear the BSS. */
> 29 memset(__bss_start, 0, __bss_end - __bss_start);
> 30
> 31 /* Set global data pointer. */
> 32 gd = &gdata;
> 33
> 34 board_init_r(NULL, 0);
> ^^^^^^^^^^^^
>
> The comment says _f, the code has _r - which is right?
Oops, the comment should say "_r" as board_init_r is common SPL code.
--
Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
