| Message ID | 20240626155945.278640-15-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Bug-fixes for a few boards | expand |
Hi Simon, On 2024-06-26 17:59, 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. > > - Drop the non-dcache optimisation, since the cache should normally be on > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v5: > - Move setting of pmugrf into the probe() function > > Changes in v3: > - Split out the refactoring into a separate patch > > drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++--------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c > index 3c4e20f4e80..b259e8e3dd6 100644 > --- a/drivers/ram/rockchip/sdram_rk3399.c > +++ b/drivers/ram/rockchip/sdram_rk3399.c [snip] > +/** > + * 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()); This still need to check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) to be correct, else you must build with both CONFIG_TPL and CONFIG_ROCKCHIP_EXTERNAL_TPL or the generated SPL will incorrectly try to initialize DRAM. DRAM should only be initialized in TPL or ROCKCHIP_EXTERNAL_TPL, or if none of them are defined in SPL. All other phases should just read and report dram size. Regards, Jonas > +} > + [snip]
Hi Jonas, On Wed, 26 Jun 2024 at 17:16, Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi Simon, > > On 2024-06-26 17:59, 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. > > > > - Drop the non-dcache optimisation, since the cache should normally be on > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Changes in v5: > > - Move setting of pmugrf into the probe() function > > > > Changes in v3: > > - Split out the refactoring into a separate patch > > > > drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++--------------- > > 1 file changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c > > index 3c4e20f4e80..b259e8e3dd6 100644 > > --- a/drivers/ram/rockchip/sdram_rk3399.c > > +++ b/drivers/ram/rockchip/sdram_rk3399.c > > [snip] > > > +/** > > + * 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()); > > This still need to check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) > to be correct, else you must build with both CONFIG_TPL and > CONFIG_ROCKCHIP_EXTERNAL_TPL or the generated SPL will incorrectly try > to initialize DRAM. > > DRAM should only be initialized in TPL or ROCKCHIP_EXTERNAL_TPL, > or if none of them are defined in SPL. All other phases should > just read and report dram size. Thanks for the info. The reason I am finding this hard to understand is that ROCKCHIP_EXTERNAL_TPL does not appear in the code as it is. So I cannot figure out why it needs to be added. Is there a bug in the current code, or does my patch introduce a bug, or...? Regards, Simon
Hi Simon, On 2024-06-27 10:02, Simon Glass wrote: > Hi Jonas, > > On Wed, 26 Jun 2024 at 17:16, Jonas Karlman <jonas@kwiboo.se> wrote: >> >> Hi Simon, >> >> On 2024-06-26 17:59, 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. >>> >>> - Drop the non-dcache optimisation, since the cache should normally be on >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> Changes in v5: >>> - Move setting of pmugrf into the probe() function >>> >>> Changes in v3: >>> - Split out the refactoring into a separate patch >>> >>> drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++--------------- >>> 1 file changed, 16 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c >>> index 3c4e20f4e80..b259e8e3dd6 100644 >>> --- a/drivers/ram/rockchip/sdram_rk3399.c >>> +++ b/drivers/ram/rockchip/sdram_rk3399.c >> >> [snip] >> >>> +/** >>> + * 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()); >> >> This still need to check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) >> to be correct, else you must build with both CONFIG_TPL and >> CONFIG_ROCKCHIP_EXTERNAL_TPL or the generated SPL will incorrectly try >> to initialize DRAM. >> >> DRAM should only be initialized in TPL or ROCKCHIP_EXTERNAL_TPL, >> or if none of them are defined in SPL. All other phases should >> just read and report dram size. > > Thanks for the info. > > The reason I am finding this hard to understand is that > ROCKCHIP_EXTERNAL_TPL does not appear in the code as it is. So I > cannot figure out why it needs to be added. Is there a bug in the > current code, or does my patch introduce a bug, or...? The current code does not handle ROCKCHIP_EXTERNAL_TPL and TPL must be built to later be replaced with the external TPL blob. This adds a new helper that states "Check if this is the phase where SDRAM init happens", and for this statement to be true it should also consider ROCKCHIP_EXTERNAL_TPL or a bugfix patch is later needed for this newly added helper. Adding !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) should make this statement be valid for the foreseeable combinations. return spl_phase() == PHASE_TPL || (!IS_ENABLED(CONFIG_TPL) && !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) && !spl_in_proper()); With that it should be possible to have TPL=n and ROCKCHIP_EXTERNAL_TPL=y and produce a working SPL. Regards, Jonas > > Regards, > Simon
Hi Jonas, On Thu, 27 Jun 2024 at 10:00, Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi Simon, > > On 2024-06-27 10:02, Simon Glass wrote: > > Hi Jonas, > > > > On Wed, 26 Jun 2024 at 17:16, Jonas Karlman <jonas@kwiboo.se> wrote: > >> > >> Hi Simon, > >> > >> On 2024-06-26 17:59, 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. > >>> > >>> - Drop the non-dcache optimisation, since the cache should normally be on > >>> > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>> --- > >>> > >>> Changes in v5: > >>> - Move setting of pmugrf into the probe() function > >>> > >>> Changes in v3: > >>> - Split out the refactoring into a separate patch > >>> > >>> drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++--------------- > >>> 1 file changed, 16 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c > >>> index 3c4e20f4e80..b259e8e3dd6 100644 > >>> --- a/drivers/ram/rockchip/sdram_rk3399.c > >>> +++ b/drivers/ram/rockchip/sdram_rk3399.c > >> > >> [snip] > >> > >>> +/** > >>> + * 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()); > >> > >> This still need to check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) > >> to be correct, else you must build with both CONFIG_TPL and > >> CONFIG_ROCKCHIP_EXTERNAL_TPL or the generated SPL will incorrectly try > >> to initialize DRAM. > >> > >> DRAM should only be initialized in TPL or ROCKCHIP_EXTERNAL_TPL, > >> or if none of them are defined in SPL. All other phases should > >> just read and report dram size. > > > > Thanks for the info. > > > > The reason I am finding this hard to understand is that > > ROCKCHIP_EXTERNAL_TPL does not appear in the code as it is. So I > > cannot figure out why it needs to be added. Is there a bug in the > > current code, or does my patch introduce a bug, or...? > > The current code does not handle ROCKCHIP_EXTERNAL_TPL and TPL must be > built to later be replaced with the external TPL blob. > > This adds a new helper that states "Check if this is the phase where > SDRAM init happens", and for this statement to be true it should also > consider ROCKCHIP_EXTERNAL_TPL or a bugfix patch is later needed for > this newly added helper. > > Adding !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) should make this > statement be valid for the foreseeable combinations. > > return spl_phase() == PHASE_TPL || > (!IS_ENABLED(CONFIG_TPL) && > !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) && > !spl_in_proper()); > > With that it should be possible to have TPL=n and ROCKCHIP_EXTERNAL_TPL=y > and produce a working SPL. Thanks very much, I understand it now. Regards, Simon
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..b259e8e3dd6 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", @@ -3093,7 +3099,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 +3143,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; + priv->info.base = CFG_SYS_SDRAM_BASE; priv->info.size = rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2); @@ -3181,10 +3183,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) || \
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. - Drop the non-dcache optimisation, since the cache should normally be on Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v5: - Move setting of pmugrf into the probe() function Changes in v3: - Split out the refactoring into a separate patch drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 17 deletions(-)