Hi Simon,
On 2025-02-09 16:36, Simon Glass wrote:
> Hi Jonas,
>
> On Sun, 9 Feb 2025 at 08:17, Jonas Karlman <jo...@kwiboo.se> wrote:
>>
>> Hi Simon,
>>
>> On 2025-02-09 15:26, Simon Glass wrote:
>>> Hi Jonas,
>>>
>>> On Wed, 5 Feb 2025 at 15:18, Jonas Karlman <jo...@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?
>
> It is partly conceptual, partly due to space.
>
> TPL is about init setup and it is board-specific. VPL is a generic
> 'root-of-trust' thing which contains the ABrec logic. It is supposed
> to be generic and run on any supported board.
>
> Some boards have very limited space for TPL, but can load a larger
> amount of code after that. There are quite a lot of x86 boards like
> that. Also, for rk3399 I was unable to fit all the TPL init and VPL
> logic into a single phase and still have room to load SPL.
For Rockchip RK3399 I do not really understand why it would not fit.
BootROM already does most init that is typically needed, and TPL
probably only need to init iomux for debug uart and pinctrl for the
storage media next phase is loaded from. Will be easier to understand
once you have a series that can be applied :-)
>
> As to combining them, I am sure it could be done, if there is enough
> SRAM. Perhaps that could be a build option?
>
> If the MMC fails, we are not going to boot, so loading an extra phase
> is not a big problem, IMO.
>
>>
>>>
>>>>
>>>> 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 <s...@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.
>
> OK, that's fine, but note that SPL is where this is supposed to
> happen. What is the benefit of doing it earlier? From my understanding
> it was just to accommodate Rockchip's secret blobs?
It has nothing to do with accommodating Rockchip's secret blobs to my
knowledge.
Most Rockchip SoCs have limited SRAM and initializing full DRAM is
what needs to happen early, i.e. RK3036 have 8 KiB SRAM. Thanks to
this SPL has full DRAM access and does not really have a size limit.
Theoretically you can have BootROM load full U-Boot proper as the next
phase after the initial DRAM init phase. Not something that is really
used, instead SPL is loaded to have more control over what is loaded
next, i.e. falcon boot, EDK2, TF-A/OP-TEE and/or U-Boot proper.
Rockchip does provide DRAM init blobs that typically can be used as a
stand in replacement for U-Boot TPL (DRAM init) in case there is no
open-source implementation for the DRAM used on a board or SoC.
Regards,
Jonas
>
>>
>>> 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,
> SImon
>
>>>
>>> [1]
>>> https://docs.u-boot.org/en/latest/develop/vbe.html#verified-boot-for-embedded-vbe
>>
