Hi Jonas, On Wed, 31 Jul 2024 at 17:18, Jonas Karlman <[email protected]> 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 <[email protected]> > > --- > > > > 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
