Hi Jonas,
On Fri, 21 Jun 2024 at 00:45, Jonas Karlman <jo...@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2024-06-21 01:06, 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 <s...@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Split out the refactoring into a separate patch
> > - Drop the non-dcache optimisation, since the cache should normally be on
> >
> > drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++-------------
> > 1 file changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/ram/rockchip/sdram_rk3399.c
> > b/drivers/ram/rockchip/sdram_rk3399.c
> > index 3c4e20f4e80..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,22 +3144,24 @@ 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);
> > - priv->info.base = CFG_SYS_SDRAM_BASE;
> > - priv->info.size =
> > - rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> > + 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)) {
>
> This check should be dropped, this driver intent is to initialize dram
> in first phase (normally TPL), and report the size to any other phase.
This whole patch can be dropped :-) Here I am just trying to avoid
code-size growth when the cache is off. But yes, it is not needed.
>
> The main need for phase_sdram_init() and disable of DCACHE can probably
> be avoided if bob/kevin can move to use separate TPL and SPL instead of
> doing both dram init and everything else in SPL.
>
> My best guess is that enabling of caches in SPL cause issue for bob and
> 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.
>
> When coreboot is involved it would only load U-Boot proper and SPL or
> TPL+SPL would never be involved at all?
>
> 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 and reduce the need for special care/handling in the code.
These boards were the first rk3399 support we had in mainline, I
believe. Everything else came later but conversions were not done for
these existing boards.
I actually intensely dislike the back-to-brom stuff. If the ROM had an
actual API (mmc_read(), etc.) then that would be fine. But just
jumping back makes it hard to control what is going on.
It is interesting to hear that SPL-only is causing a problem. Do you
have any inkling of what that might be? I could take a look at
converting the boards to use TPL, but it does seem a bit sideways to
the issue here. Is the bootrom doing some magic, perhaps? Is there
decent documentation for the boot ROM these days?
Regards,
Simon
