[U-Boot] [PATCH 1/2] rockchip: allow DRAM init in SPL and fix ordering
b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") removed SoC-specific code for RK3399's SPL and in the process introduced two regressions:
1. It caused the previously-unconditional DRAM initialization in board_init_f() to only happen when compiling a configuration that did not support TPL, meaning DRAM would never get initialized if TPL was supported but disabled. 2. It reordered the DRAM initialization before rockchip_stimer_init(), which as far as I can tell causes RK3399 to lock up completely.
Fix both of these issues in the common code so that we can again boot RK3399 without a TPL. This fixes custom configurations that have disabled TPL, and it should also unbreak the "ficus-rk3399", "rock960-rk3399", and "chromebook_bob" defconfigs, although since I don't have any of those devices I can't confirm they're broken now.
Signed-off-by: Thomas Hebb tommyhebb@gmail.com --- arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 92102b39e7..089f0a5258 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) void board_init_f(ulong dummy) { int ret; -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) struct udevice *dev; #endif
@@ -128,20 +128,20 @@ void board_init_f(ulong dummy) hang(); } arch_cpu_init(); -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) - debug("\nspl:init dram\n"); - ret = uclass_get_device(UCLASS_RAM, 0, &dev); - if (ret) { - printf("DRAM init failed: %d\n", ret); - return; - } -#endif #if !defined(CONFIG_ROCKCHIP_RK3188) rockchip_stimer_init(); #endif #ifdef CONFIG_SYS_ARCH_TIMER /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ timer_init(); +#endif +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) + debug("\nspl:init dram\n"); + ret = uclass_get_device(UCLASS_RAM, 0, &dev); + if (ret) { + printf("DRAM init failed: %d\n", ret); + return; + } #endif preloader_console_init(); }
We shouldn't force which allocator the SPL uses, since there's no platform requirement for one over the other: in fact, we currently allow selection of the TPL allocator but not the SPL one!
Signed-off-by: Thomas Hebb tommyhebb@gmail.com --- arch/arm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 7b80630aa1..f96841c777 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1604,7 +1604,6 @@ config ARCH_ROCKCHIP select OF_CONTROL select SPI select SPL_DM if SPL - select SPL_SYS_MALLOC_SIMPLE if SPL select SYS_MALLOC_F select SYS_THUMB_BUILD if !ARM64 imply ADC @@ -1614,6 +1613,7 @@ config ARCH_ROCKCHIP imply FAT_WRITE imply SARADC_ROCKCHIP imply SPL_SYSRESET + imply SPL_SYS_MALLOC_SIMPLE imply SYS_NS16550 imply TPL_SYSRESET imply USB_FUNCTION_FASTBOOT
On 2019/11/11 上午12:25, Thomas Hebb wrote:
We shouldn't force which allocator the SPL uses, since there's no platform requirement for one over the other: in fact, we currently allow selection of the TPL allocator but not the SPL one!
Signed-off-by: Thomas Hebb tommyhebb@gmail.com
Reviewed-by: Kever Yang kever.yang@rock-chips.com
Thanks,
- Kever
arch/arm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 7b80630aa1..f96841c777 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1604,7 +1604,6 @@ config ARCH_ROCKCHIP select OF_CONTROL select SPI select SPL_DM if SPL
- select SPL_SYS_MALLOC_SIMPLE if SPL select SYS_MALLOC_F select SYS_THUMB_BUILD if !ARM64 imply ADC
@@ -1614,6 +1613,7 @@ config ARCH_ROCKCHIP imply FAT_WRITE imply SARADC_ROCKCHIP imply SPL_SYSRESET
- imply SPL_SYS_MALLOC_SIMPLE imply SYS_NS16550 imply TPL_SYSRESET imply USB_FUNCTION_FASTBOOT
On Sun, Nov 10, 2019 at 6:12 PM Thomas Hebb tommyhebb@gmail.com wrote:
b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") removed SoC-specific code for RK3399's SPL and in the process introduced two regressions:
- It caused the previously-unconditional DRAM initialization in board_init_f() to only happen when compiling a configuration that did not support TPL, meaning DRAM would never get initialized if TPL was supported but disabled.
- It reordered the DRAM initialization before rockchip_stimer_init(), which as far as I can tell causes RK3399 to lock up completely.
Fix both of these issues in the common code so that we can again boot RK3399 without a TPL. This fixes custom configurations that have disabled TPL, and it should also unbreak the "ficus-rk3399", "rock960-rk3399", and "chromebook_bob" defconfigs, although since I don't have any of those devices I can't confirm they're broken now.
Signed-off-by: Thomas Hebb tommyhebb@gmail.com
arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 92102b39e7..089f0a5258 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) void board_init_f(ulong dummy) { int ret; -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) struct udevice *dev; #endif
@@ -128,20 +128,20 @@ void board_init_f(ulong dummy) hang(); } arch_cpu_init(); -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}-#endif #if !defined(CONFIG_ROCKCHIP_RK3188) rockchip_stimer_init(); #endif #ifdef CONFIG_SYS_ARCH_TIMER /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ timer_init(); +#endif +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}#endif preloader_console_init(); }
Reviewed-by: Michael Trimarchi michael@amarulasolutions.com
-- 2.23.0
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Thomas,
On 2019/11/11 上午12:25, Thomas Hebb wrote:
b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") removed SoC-specific code for RK3399's SPL and in the process introduced two regressions:
- It caused the previously-unconditional DRAM initialization in board_init_f() to only happen when compiling a configuration that did not support TPL, meaning DRAM would never get initialized if TPL was supported but disabled.
If the board support TPL, that means the dram init should be done in TPL, no matter the really
implement is the U-Boot TPL or use rockchip rkbin instead. In this case, enable DRAM driver in SPL
make no sense, for dram driver itself would only get dram size which do not have any use other
than the case CONFIG_SPL_OS_BOOT.
- It reordered the DRAM initialization before rockchip_stimer_init(), which as far as I can tell causes RK3399 to lock up completely.
The stimer_init should goes before dram_init.
So please only update the order.
Thanks,
- Kever
Fix both of these issues in the common code so that we can again boot RK3399 without a TPL. This fixes custom configurations that have disabled TPL, and it should also unbreak the "ficus-rk3399", "rock960-rk3399", and "chromebook_bob" defconfigs, although since I don't have any of those devices I can't confirm they're broken now.
Signed-off-by: Thomas Hebb tommyhebb@gmail.com
arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 92102b39e7..089f0a5258 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) void board_init_f(ulong dummy) { int ret; -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) struct udevice *dev; #endif
@@ -128,20 +128,20 @@ void board_init_f(ulong dummy) hang(); } arch_cpu_init(); -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT)
- debug("\nspl:init dram\n");
- ret = uclass_get_device(UCLASS_RAM, 0, &dev);
- if (ret) {
printf("DRAM init failed: %d\n", ret);return;- }
-#endif #if !defined(CONFIG_ROCKCHIP_RK3188) rockchip_stimer_init(); #endif #ifdef CONFIG_SYS_ARCH_TIMER /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ timer_init(); +#endif +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT)
- debug("\nspl:init dram\n");
- ret = uclass_get_device(UCLASS_RAM, 0, &dev);
- if (ret) {
printf("DRAM init failed: %d\n", ret);return;- } #endif preloader_console_init(); }
Hi Kever,
On Thu, Nov 14, 2019 at 1:25 AM Kever Yang kever.yang@rock-chips.com wrote:
Hi Thomas,
On 2019/11/11 上午12:25, Thomas Hebb wrote:
b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") removed SoC-specific code for RK3399's SPL and in the process introduced two regressions:
- It caused the previously-unconditional DRAM initialization in board_init_f() to only happen when compiling a configuration that did not support TPL, meaning DRAM would never get initialized if TPL was supported but disabled.
If the board support TPL, that means the dram init should be done in TPL, no matter the really
implement is the U-Boot TPL or use rockchip rkbin instead. In this case, enable DRAM driver in SPL
make no sense, for dram driver itself would only get dram size which do not have any use other
than the case CONFIG_SPL_OS_BOOT.
I would like to boot my RK3399 board with only an SPL, loaded directly by the. BootROM into IRAM. I have no need for a three-stage bootloader, and adding only increases boot time and adds complexity. Is there a reason not to support this use case?
I do realize that this will add an unnecessary call if someone uses rkbin for DDR init and SPL for the second stage. Do any boards actually do that, though?
- It reordered the DRAM initialization before rockchip_stimer_init(),
which as far as I can tell causes RK3399 to lock up completely.The stimer_init should goes before dram_init.
So please only update the order.
Should timer_init() also go before dram_init(), like I have it now? Or should I put dram_init() between rockchip_stimer_init() and timer_init()?
Thanks,
- Kever
Fix both of these issues in the common code so that we can again boot RK3399 without a TPL. This fixes custom configurations that have disabled TPL, and it should also unbreak the "ficus-rk3399", "rock960-rk3399", and "chromebook_bob" defconfigs, although since I don't have any of those devices I can't confirm they're broken now.
Signed-off-by: Thomas Hebb tommyhebb@gmail.com
arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 92102b39e7..089f0a5258 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) void board_init_f(ulong dummy) { int ret; -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) struct udevice *dev; #endif
@@ -128,20 +128,20 @@ void board_init_f(ulong dummy) hang(); } arch_cpu_init(); -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}-#endif #if !defined(CONFIG_ROCKCHIP_RK3188) rockchip_stimer_init(); #endif #ifdef CONFIG_SYS_ARCH_TIMER /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ timer_init(); +#endif +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}
Hi Thomas,
On 2019/11/15 上午12:09, Tom Hebb wrote:
Hi Kever,
On Thu, Nov 14, 2019 at 1:25 AM Kever Yang <kever.yang@rock-chips.com mailto:kever.yang@rock-chips.com> wrote:
Hi Thomas, On 2019/11/11 上午12:25, Thomas Hebb wrote: > b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") > removed SoC-specific code for RK3399's SPL and in the process introduced > two regressions: > > 1. It caused the previously-unconditional DRAM initialization in > board_init_f() to only happen when compiling a configuration that > did not support TPL, meaning DRAM would never get initialized if TPL > was supported but disabled. If the board support TPL, that means the dram init should be done in TPL, no matter the really implement is the U-Boot TPL or use rockchip rkbin instead. In this case, enable DRAM driver in SPL make no sense, for dram driver itself would only get dram size which do not have any use other than the case CONFIG_SPL_OS_BOOT.I would like to boot my RK3399 board with only an SPL, loaded directly by the. BootROM into IRAM. I have no need for a three-stage bootloader, and adding only increases boot time and adds complexity. Is there a reason not to support this use case?
OK, I understand it now, you update is necessary, thanks.
Well, it will be better to make this patch into two patches.
I do realize that this will add an unnecessary call if someone uses rkbin for DDR init and SPL for the second stage. Do any boards actually do that, though?
> 2. It reordered the DRAM initialization before rockchip_stimer_init(), > which as far as I can tell causes RK3399 to lock up completely. The stimer_init should goes before dram_init. So please only update the order.Should timer_init() also go before dram_init(), like I have it now? Or should I put dram_init() between rockchip_stimer_init() and timer_init()?
timer_init() goes before dram_init, because the dram_init() will use udelay().
Thanks,
- Kever
Thanks, - Kever > > Fix both of these issues in the common code so that we can again boot > RK3399 without a TPL. This fixes custom configurations that have > disabled TPL, and it should also unbreak the "ficus-rk3399", > "rock960-rk3399", and "chromebook_bob" defconfigs, although since I > don't have any of those devices I can't confirm they're broken now. > > Signed-off-by: Thomas Hebb <tommyhebb@gmail.com <mailto:tommyhebb@gmail.com>> > --- > arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c > index 92102b39e7..089f0a5258 100644 > --- a/arch/arm/mach-rockchip/spl.c > +++ b/arch/arm/mach-rockchip/spl.c > @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) > void board_init_f(ulong dummy) > { > int ret; > -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) > +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) > struct udevice *dev; > #endif > > @@ -128,20 +128,20 @@ void board_init_f(ulong dummy) > hang(); > } > arch_cpu_init(); > -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) > - debug("\nspl:init dram\n"); > - ret = uclass_get_device(UCLASS_RAM, 0, &dev); > - if (ret) { > - printf("DRAM init failed: %d\n", ret); > - return; > - } > -#endif > #if !defined(CONFIG_ROCKCHIP_RK3188) > rockchip_stimer_init(); > #endif > #ifdef CONFIG_SYS_ARCH_TIMER > /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ > timer_init(); > +#endif > +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) > + debug("\nspl:init dram\n"); > + ret = uclass_get_device(UCLASS_RAM, 0, &dev); > + if (ret) { > + printf("DRAM init failed: %d\n", ret); > + return; > + } > #endif > preloader_console_init(); > }
Thank you for the review! I will split the patch and send a v2.
On Thu, Nov 14, 2019, 17:17 Kever Yang kever.yang@rock-chips.com wrote:
Hi Thomas, On 2019/11/15 上午12:09, Tom Hebb wrote:
Hi Kever,
On Thu, Nov 14, 2019 at 1:25 AM Kever Yang kever.yang@rock-chips.com wrote:
Hi Thomas,
On 2019/11/11 上午12:25, Thomas Hebb wrote:
b7abef2ecbcc ("rockchip: rk3399: Migrate to use common spl board file") removed SoC-specific code for RK3399's SPL and in the process introduced two regressions:
- It caused the previously-unconditional DRAM initialization in board_init_f() to only happen when compiling a configuration that did not support TPL, meaning DRAM would never get initialized if
TPL
was supported but disabled.If the board support TPL, that means the dram init should be done in TPL, no matter the really
implement is the U-Boot TPL or use rockchip rkbin instead. In this case, enable DRAM driver in SPL
make no sense, for dram driver itself would only get dram size which do not have any use other
than the case CONFIG_SPL_OS_BOOT.
I would like to boot my RK3399 board with only an SPL, loaded directly by the. BootROM into IRAM. I have no need for a three-stage bootloader, and adding only increases boot time and adds complexity. Is there a reason not to support this use case?
OK, I understand it now, you update is necessary, thanks.
Well, it will be better to make this patch into two patches.
I do realize that this will add an unnecessary call if someone uses rkbin for DDR init and SPL for the second stage. Do any boards actually do that, though?
- It reordered the DRAM initialization before rockchip_stimer_init(),
which as far as I can tell causes RK3399 to lock up completely.The stimer_init should goes before dram_init.
So please only update the order.
Should timer_init() also go before dram_init(), like I have it now? Or should I put dram_init() between rockchip_stimer_init() and timer_init()?
timer_init() goes before dram_init, because the dram_init() will use udelay().
Thanks,
- Kever
Thanks,
- Kever
Fix both of these issues in the common code so that we can again boot RK3399 without a TPL. This fixes custom configurations that have disabled TPL, and it should also unbreak the "ficus-rk3399", "rock960-rk3399", and "chromebook_bob" defconfigs, although since I don't have any of those devices I can't confirm they're broken now.
Signed-off-by: Thomas Hebb tommyhebb@gmail.com
arch/arm/mach-rockchip/spl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 92102b39e7..089f0a5258 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -103,7 +103,7 @@ __weak int arch_cpu_init(void) void board_init_f(ulong dummy) { int ret; -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT) +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT) struct udevice *dev; #endif
@@ -128,20 +128,20 @@ void board_init_f(ulong dummy) hang(); } arch_cpu_init(); -#if !defined(CONFIG_SUPPORT_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}-#endif #if !defined(CONFIG_ROCKCHIP_RK3188) rockchip_stimer_init(); #endif #ifdef CONFIG_SYS_ARCH_TIMER /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */ timer_init(); +#endif +#if !defined(CONFIG_TPL) || defined(CONFIG_SPL_OS_BOOT)
debug("\nspl:init dram\n");ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {printf("DRAM init failed: %d\n", ret);return;}
participants (4)
-
Kever Yang -
Michael Nazzareno Trimarchi -
Thomas Hebb -
Tom Hebb