[PATCH v8 8/8] rockchip: Avoid #ifdefs in RK3399 SPL

[Simon Glass]

Hi Jonas,

On Wed, 31 Jul 2024 at 17:18, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2024-07-31 23:58, Simon Glass wrote:
> > The code here is confusing due to large blocks 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 v8:
> > - Include ROCKCHIP_EXTERNAL_TPL in the condition
> > - Put the non-dcache change back into patch 5
> >
> > Changes in v5:
> > - Move setting of pmugrf into the probe() function
> > - Drop the non-dcache optimisation, since the cache should normally be on
> > - Drop patches previously applied
> >
> > Changes in v4:
> > - Fix 'stating' typo
> > - Move Binman size feature to a separate series
> >
> > Changes in v3:
> > - Split out the refactoring into a separate patch
> >
> > Changes in v2:
> > - Drop patch "regulator: rk8xx: Fix incorrect parameter"
> > - Rewrite boneblack patch to onstead drop the target and update docs
> >
> >  drivers/ram/rockchip/sdram_rk3399.c | 35 +++++++++++++++--------------
> >  1 file changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> > index bc79c034808..611e30cbe52 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,19 @@ 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) &&
> > +              !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) &&
> > +              !spl_in_proper());
> > +}
> > +
> >  static struct io_setting *
> >  lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
> >  {
> > @@ -3024,7 +3032,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",
> > @@ -3093,7 +3101,6 @@ static int rk3399_dmc_init(struct udevice *dev)
> >       priv->cic = syscon_get_first_range(ROCKCHIP_SYSCON_CIC);
> >       priv->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
> >       priv->pmu = syscon_get_first_range(ROCKCHIP_SYSCON_PMU);
> > -     priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> >       priv->pmusgrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUSGRF);
> >       priv->pmucru = rockchip_get_pmucru();
> >       priv->cru = rockchip_get_cru();
> > @@ -3138,19 +3145,16 @@ static int rk3399_dmc_init(struct udevice *dev)
> >
> >       return 0;
> >  }
> > -#endif
> >
> >  static int rk3399_dmc_probe(struct udevice *dev)
> >  {
> >       struct dram_info *priv = dev_get_priv(dev);
> >
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> > -     if (rk3399_dmc_init(dev))
> > -             return 0;
> > -#endif
> >       priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> >       debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> > +     if (phase_sdram_init() && rk3399_dmc_init(dev))
> > +             return 0;
> > +
>
> As you pointed out in the other mail thread, this adds ~500 bytes to TPL
> because of the added (and unneeded) call to rockchip_sdram_size().
>
> The rockchip_sdram_size() call only make sense to be done in SPL and
> later phases, as was done before the changes in this patch.
>
> This RAM driver need to:
> - In TPL (or SPL on bob/kevin) initialize DRAM, i.e. rk3399_dmc_init().
> - In SPL and later phases report DRAM size, i.e. rockchip_sdram_size().
>
> Please restore the old behavior with something like:
>
>         if (IS_ENABLED(CONFIG_TPL_BUILD))
>                 return 0;
>
> Since we never are expected to need the struct ram_info in TPL, that
> should also reduce the unneeded size growth of TPL that you observed.

OK, I sent a v9 :-)

>
> Ideally we should really migrate bob and kevin to use both TPL and SPL
> same as all other Rockchip aarch64 boards. That should help simplify and
> reduce likelihood of future issues that are only observed on bob/kevin.
> I will try to send RFC patches for such migration, but wont be able to
> test them.

I can test them in my lab easily enough, although I am still waiting
for other bugfixes to land so that these boards actually boot.

Regards,
Simon