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

Hi Quentin,

On Tue, 11 Jun 2024 at 05:27, Quentin Schulz quentin.schulz@cherry.de 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@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;

} """ ?

Yes, that's the nub of it. Of course, it doesn't actually fix the problem. I presume the SRAM appears in the page tables?

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?

Just to avoid increasing code size when the cache is off.

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.

Yes well it would have been a lot better if I had put it in a separate patch, which I forgot to do.

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

Hmm that is something that we brought in for 32-bit machines. For 64-bit I don't really see why we are using it. It makes printf() hard.

But anyway, I should have left it alone :-)

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

It puts the MMU table just below the top of RAM which is currently base + size (0 + 0) = somewhere like (4GB - 64KB).

Regards, Simon