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

[Quentin Schulz <quentin.schulz@cherry.de>]

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@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;
}
"""
?

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?

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.

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

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 :)

Cheers,
Quentin