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

[Jonas Karlman]

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.

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.

Regards,
Jonas

>  	priv->info.base = CFG_SYS_SDRAM_BASE;
>  	priv->info.size =
>  		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> @@ -3181,10 +3185,7 @@ U_BOOT_DRIVER(dmc_rk3399) = {
>  	.id = UCLASS_RAM,
>  	.of_match = rk3399_dmc_ids,
>  	.ops = &rk3399_dmc_ops,
> -#if defined(CONFIG_TPL_BUILD) || \
> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>  	.of_to_plat = rk3399_dmc_of_to_plat,
> -#endif
>  	.probe = rk3399_dmc_probe,
>  	.priv_auto	= sizeof(struct dram_info),
>  #if defined(CONFIG_TPL_BUILD) || \