Hi Kever, On Fri, 21 Jun 2024 at 21:49, Kever Yang <kever.y...@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 <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. > 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