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

[Simon Glass]

Hi Jonas,

On Sun, 9 Feb 2025 at 08:17, Jonas Karlman <jonas at 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 <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?

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.

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 <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.

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?

>
> > 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
>