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

[Jonas Karlman]

Hi Simon and Quentin,

On 2024-06-11 13:27, Quentin Schulz 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;
> }
> """
> ?
> 
> 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?

Agree, this look strange to me too and your diff much cleaner :-)

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

My best guess is that enabling of caches in SPL cause issue for
bob/kevin because they only use SPL not TPL+SPL like (if I am not
mistaken) all other Rockchip arm64 targets.

Using SPL-only was not something I tested when caches was enabled in SPL.

Maybe bob/kevin can be changed to also use TPL+SPL similar to all other
RK3399 boards?

How U-Boot works on these chromebooks is still a mystery to me. If I
understand correctly SPL is only involved for bare metal boot, if this
is the case then using TPL + back-to-brom to load SPL should probably
work fine?, and would align all RK3399 boards to work in a similar way.

Regards,
Jonas

> 
> Cheers,
> Quentin