[PATCH 05/29] rockchip: Allow RAM init to happen in SPL on rk3399

[Jonas Karlman]

Hi Simon,

On 2025-02-09 15:26, Simon Glass wrote:
> Hi Jonas,
> 
> On Wed, 5 Feb 2025 at 15:18, Jonas Karlman <jonas at kwiboo.se> wrote:
>>
>> Hi Simon,
>>
>> On 2025-02-05 02:54, Simon Glass wrote:
>>> TPL runs before VPL. The earliest updatable phase with VBE is SPL. We
>>> want to be able to update the RAM-init code in the field.
>>
>> If I understand your description here, you mean that both TPL and VPL
>> are meant to be "burned" onto the board and not intended to be updatable?
> 
> Yes, that's right. If you look at [1] and click on 'vbe-intro' there
> is a diagram that shows the flow.

Thanks for the reference, I can see that both are listed as read-only.
Unfortunately that do not explain why we need to have both TPL and VPL
aside from it being two separate conceptual parts.

We can as easily just have BootROM load VPL and start from there, it
does not look like TPL provides anything of real value for VBE on
Rockchip, only one more part that can potentially fail?

> 
>>
>> Is this a typo or do I misunderstand something? Else I do not understand
>> why you would need TPL->VPL->SPL, please explain.
>>
>>>
>>> So when VPL is being used, init the RAM later, in SPL.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>  drivers/ram/rockchip/sdram_rk3399.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
>>> index 6fa8f268770..9ac16dfdc71 100644
>>> --- a/drivers/ram/rockchip/sdram_rk3399.c
>>> +++ b/drivers/ram/rockchip/sdram_rk3399.c
>>> @@ -194,6 +194,7 @@ struct io_setting {
>>>  static bool phase_sdram_init(void)
>>>  {
>>>       return xpl_phase() == PHASE_TPL ||
>>> +             (IS_ENABLED(CONFIG_VPL) && xpl_phase() == PHASE_SPL) ||
>>>               (!IS_ENABLED(CONFIG_TPL) &&
>>>                !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) &&
>>>                !not_xpl());
>>> @@ -3195,8 +3196,9 @@ U_BOOT_DRIVER(dmc_rk3399) = {
>>>       .of_to_plat = rk3399_dmc_of_to_plat,
>>>       .probe = rk3399_dmc_probe,
>>>       .priv_auto      = sizeof(struct dram_info),
>>> -#if defined(CONFIG_TPL_BUILD) || \
>>> -     (!defined(CONFIG_TPL) && defined(CONFIG_XPL_BUILD))
>>> +#if defined(CONFIG_VPL) && defined(CONFIG_SPL_BUILD) || \
>>> +     !defined(CONFIG_VPL) && defined(CONFIG_TPL_BUILD) || \
>>> +     !defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)
>>>       .plat_auto      = sizeof(struct rockchip_dmc_plat),
>>>  #endif
>>>  };
>>
>> These two conditions are starting to get ridiculous.
>>
>> With "[PATCH 24/29] rockchip: Allow SPL to set up SDRAM" you are
>> depending on TPL_RAM to decide if RAM should be initialized. Maybe that
>> is something we can use here to simplify?
> 
> The problem is that TPL_RAM means that the ram driver is present,
> which isn't the same thing as actually doing the RAM init in that
> phase. If we had a Kconfig value for 'phase' then we could perhaps
> have CONFIG_PHASE_RAM=tpl or something like that.

Yes, we may need to add a new Kconfig symbol to more easily control
where DRAM init happens.

I have been slowly working on unifying all Rockchip SoCs to use TPL for
DRAM init and SPL for loading a FIT. All AArch64 should now follow this,
and currently looking at migrating the older ARMv7 SoCs.

> I plan to get back
> to my rejected XPL series soon and that is something I could
> incorporate when that goes in.
> 
>>
>> Basically we will have 3 possible DRAM init scenarios to support:
>> - ROCKCHIP_EXTERNA_TPL=y -> DRAM init happen in external TPL and SPL
>>   should only read DRAM size
>> - VPL=y or TPL=n -> both DRAM init and read DRAM size happen in SPL
>> - TPL=y -> DRAM init in TPL and SPL should only read DRAM size
> 
> Yes that's my understanding too. For the last one, for clarity:
> 
>  - TPL=y -> DRAM init in TPL; SPL should only read DRAM size

Correct.

Regards,
Jonas

> 
> Regards,
> Simon
> 
> [1] https://docs.u-boot.org/en/latest/develop/vbe.html#verified-boot-for-embedded-vbe