[PATCH v4 14/16] rockchip: Avoid #ifdefs in RK3399 SPL
Quentin Schulz
quentin.schulz at cherry.de
Mon Jun 24 10:24:02 CEST 2024
Hi Simon,
On 6/23/24 7:53 PM, 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.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v4:
> - Drop the non-dcache optimisation, since the cache should normally be on
>
> Changes in v3:
> - Split out the refactoring into a separate patch
>
> drivers/ram/rockchip/sdram_rk3399.c | 42 +++++++++++++++--------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> index 3c4e20f4e80..949a082d00c 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,22 +3144,21 @@ 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()) {
> + 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);
> + }
> +
I'm not sure we need to put the debug message into an else as this
pmugrf is also used when dmc_init passes.
Additionally, we could also just set priv->pmugrf before the if
condition and remove it from dmc_init if we really wanted to.
> priv->info.base = CFG_SYS_SDRAM_BASE;
> - priv->info.size =
> - rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> + priv->info.size = rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
>
Can you please say a few words about this change in the commit log? Why
phys_addr_t->ulong here?
Otherwise looks ok to me,
Cheers,
Quentin
More information about the U-Boot
mailing list