Hi Tom,
On Mon, 10 Feb 2025 at 10:06, Tom Rini <tr...@konsulko.com> wrote:
>
> On Sun, Feb 09, 2025 at 07:26:38AM -0700, 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.
> >
> > >
> > > 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. I plan to get back
> > to my rejected XPL series soon and that is something I could
> > incorporate when that goes in.
>
> Please note that if you're going to revisit the "XPL" series I rejected
> you need to do what I asked instead or there's just going to be a larger
> gap between mainline and your downstream fork.
Yes I recall you wanted to completely separate the phases at the
defconfig level, which I am not keen on. I will send a proposal and
let's see if we can figure something out there.
Regards,
Simon
