[PATCH v3 16/18] rockchip: Avoid #ifdefs in RK3399 SPL
Simon Glass
sjg at chromium.org
Sat Jun 22 15:35:59 CEST 2024
Hi Kever,
On Fri, 21 Jun 2024 at 21:49, Kever Yang <kever.yang at rock-chips.com> wrote:
>
> Hi Simon,
>
> On 2024/6/21 22:57, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Fri, 21 Jun 2024 at 00:45, Jonas Karlman <jonas at 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 <sjg at 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.
> The logic of BootRom is very simple and very clear:
> - Read image from boot medium and check the availability(format of
> rockchip IDB,
> which is packed by the mkimage rksd/rkspi);
> - Load TPL/dram init blob to the internal SRAM(size limit by the sram),
> and jump to the entry;
> - Back to bootRom with '0' return means the dram init success, bootRom
> will get next image for dram;
> - Read SPL image from boot medium to DDR start address(no size limit),
> and jump to the ddr entry;
> - The signature verify will be done for all the firmware load by bootRom
> if the secure boot feature is enabled;
> This model works for all Rockchip SoCs, although some SoCs have very
> limit SRAM size(only available
> for DRAM init) and some other SoCs like rk3399 have bigger SRAM size.
Thanks for the info. Is there a document that describes this exactly?
Some questions it should cover:
- What is the signature verification?
- What offsets are used?
- What memory addresses are things loaded to?
- What other values of 'back to bootRom' are supported?
- Is there a jump table listing available functions for U-Boot to call?
- How to call the ROM functions to read blocks from mmc, SPI, etc.
>
> Thanks,
> - Kever
> > 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
Regards,
Simon
More information about the U-Boot
mailing list