Thank you for the review! I will split the patch and send a v2.
On Thu, Nov 14, 2019, 17:17 Kever Yang <kever.y...@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.y...@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 <tommyh...@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();
>> > }
>>
>>
>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
