[PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Simon Glass]

Hi Quentin,

On Tue, 11 Jun 2024 at 05:27, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 6/10/24 4:59 PM, Simon Glass wrote:
> > At present gd->ram_size is 0 in SPL, meaning that it is not possible to
> > enable the cache. Correct this by always populating the RAM size
> > correctly.
> >
> > Part of the confusion here comes from the large blocks of code which
> > are #ifdefed out. Add a function phase_sdram_init() which returns
> > whether SDRAM init should happen in the current phase, using that as
> > needed to control the code flow.
> >
> > This increases code size by about 500 bytes in SPL when the cache is on,
> > since it must call the rather large rockchip_sdram_size() function.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add new patch to correct memory size in SPL
> >
> >   drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
> >   1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> > index 02cc4a38cf0..2f37dd712e7 100644
> > --- a/drivers/ram/rockchip/sdram_rk3399.c
> > +++ b/drivers/ram/rockchip/sdram_rk3399.c
> > @@ -13,6 +13,7 @@
> >   #include <log.h>
> >   #include <ram.h>
> >   #include <regmap.h>
> > +#include <spl.h>
> >   #include <syscon.h>
> >   #include <asm/arch-rockchip/clock.h>
> >   #include <asm/arch-rockchip/cru.h>
> > @@ -63,8 +64,6 @@ struct chan_info {
> >   };
> >
> >   struct dram_info {
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> >       u32 pwrup_srefresh_exit[2];
> >       struct chan_info chan[2];
> >       struct clk ddr_clk;
> > @@ -75,7 +74,6 @@ struct dram_info {
> >       struct rk3399_pmusgrf_regs *pmusgrf;
> >       struct rk3399_ddr_cic_regs *cic;
> >       const struct sdram_rk3399_ops *ops;
> > -#endif
> >       struct ram_info info;
> >       struct rk3399_pmugrf_regs *pmugrf;
> >   };
> > @@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
> >                                       struct rk3399_sdram_params *params);
> >   };
> >
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> > -
> >   struct rockchip_dmc_plat {
> >   #if CONFIG_IS_ENABLED(OF_PLATDATA)
> >       struct dtd_rockchip_rk3399_dmc dtplat;
> > @@ -191,6 +186,17 @@ struct io_setting {
> >       },
> >   };
> >
> > +/**
> > + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
> > + *
> > + * Returns: true to do SDRAM init in this phase, false to not
> > + */
> > +static bool phase_sdram_init(void)
> > +{
> > +     return spl_phase() == PHASE_TPL ||
> > +         (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
> > +}
> > +
> >   static struct io_setting *
> >   lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
> >   {
> > @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
> >       struct rockchip_dmc_plat *plat = dev_get_plat(dev);
> >       int ret;
> >
> > -     if (!CONFIG_IS_ENABLED(OF_REAL))
> > +     if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
> >               return 0;
> >
> >       ret = dev_read_u32_array(dev, "rockchip,sdram-params",
> > @@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
> >
> >       return 0;
> >   }
> > -#endif
> >
> >   static int rk3399_dmc_probe(struct udevice *dev)
> >   {
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> > -     if (rk3399_dmc_init(dev))
> > -             return 0;
> > -#else
> >       struct dram_info *priv = dev_get_priv(dev);
> >
> > -     priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> > -     debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> > -     priv->info.base = CFG_SYS_SDRAM_BASE;
> > -     priv->info.size =
> > -             rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> > -#endif
> > +     if (phase_sdram_init()) {
> > +             if (rk3399_dmc_init(dev))
> > +                     return 0;
> > +     } else {
> > +             priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> > +             debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> > +     }
> > +
> > +     if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
> > +             priv->info.base = CFG_SYS_SDRAM_BASE;
> > +             priv->info.size =
> > +                     rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
> > +     }
> > +
>
> Isn't the whole change summarized to making sure that priv->info.base
> and priv->info.size are set when DCACHE is enabled AND we're in the
> first stage BL (TPL or SPL if no TPL)?
>
> i.e., shouldn't the following code be enough:
>
> """
> static int rk3399_dmc_probe(struct udevice *dev)
> {
> #if defined(CONFIG_TPL_BUILD) || \
>         (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>         if (rk3399_dmc_init(dev))
>                 return 0;
> #else
>         struct dram_info *priv = dev_get_priv(dev);
>
>         priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>         debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> #endif
>         priv->info.base = CFG_SYS_SDRAM_BASE;
>         priv->info.size =
>                 rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
>
>         return 0;
> }
> """
> ?

Yes, that's the nub of it. Of course, it doesn't actually fix the
problem. I presume the SRAM appears in the page tables?

>
> Then what's after the endif could be guarded by if
> (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { if we need to but it's not clear
> to me why that is needed?

Just to avoid increasing code size when the cache is off.

>
> Basically, I'm not sure the migration from ifdefs to the
> phase_sdram_init() function is necessary. I'm not against it, but it
> makes the whole thing much harder to read and hides the actual changes.

Yes well it would have been a lot better if I had put it in a separate
patch, which I forgot to do.

>
> Additionally, why was the cast to phys_addr_t changed to a ulong? The
> function actually expects a phys_addr_t.

Hmm that is something that we brought in for 32-bit machines. For
64-bit I don't really see why we are using it. It makes printf() hard.

But anyway, I should have left it alone :-)

>
> Finally, can you please explain why gd->ram_size being 0 is an issue for
> the caches, where is this checked? I'm not too familiar with the caches
> in general :)

It puts the MMU table just below the top of RAM which is currently
base + size (0 + 0) = somewhere like (4GB - 64KB).

Regards,
Simon