Hi Quentin, On Tue, 11 Jun 2024 at 05:27, Quentin Schulz <quentin.sch...@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 <s...@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