| Message ID | 20240522121559.1024535-1-heiko@sntech.de |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [RESEND] clk: rockchip: rk3588: Set SPLL frequency during SPL stage | expand |
Hi Heiko, On 5/22/24 2:15 PM, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > All parts expect the SPLL to run at 702MHz. In U-Boot it's the SPLL_HZ > declaring this rate and in the kernel it's a fixed clock definition. > > While everything is expecting 702MHz, the SPLL is not running that > frequency when coming from the bootrom though, instead it's running > at 351MHz and the vendor-u-boot just sets it to the expected frequency. > > The SPLL itself is located inside the secure-BUSCRU and in theory > accessible as an SCMI clock, though this requires an unknown amount > of cooperation from trusted-firmware to set at a later stage, though > during the SPL stage we can still access the relevant CRU directly. > > The SPLL is for example necessary for the DSI controllers to produce > output. > > As the SPLL is "just" another rk3588 pll, just set the desired rate > directly during the SPL stage. > > Tested on rk3588-rock5b and rk3588-tiger by reading back the PLL rate > and also observing working DSI output with this change. > > Fixes: 6737771600d4 ("rockchip: rk3588: Add support for sdmmc clocks in SPL") > Suggested-by: Andy Yan <andy.yan@rock-chips.com> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > Cc: Jonas Karlman <jonas@kwiboo.se> > --- > Resend, because I forgot the u-boot list > > .../include/asm/arch-rockchip/cru_rk3588.h | 1 + > drivers/clk/rockchip/clk_rk3588.c | 26 ++++++++++++++++--- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > index a4507e5fdd7..85b4da0bc2c 100644 > --- a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > @@ -29,6 +29,7 @@ enum rk3588_pll_id { > V0PLL, > AUPLL, > PPLL, > + SPLL, > PLL_COUNT, > }; > > diff --git a/drivers/clk/rockchip/clk_rk3588.c b/drivers/clk/rockchip/clk_rk3588.c > index 4c611a39049..5384b3213f5 100644 > --- a/drivers/clk/rockchip/clk_rk3588.c > +++ b/drivers/clk/rockchip/clk_rk3588.c > @@ -37,6 +37,7 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = { > RK3588_PLL_RATE(786000000, 1, 131, 2, 0), > RK3588_PLL_RATE(742500000, 4, 495, 2, 0), > RK3588_PLL_RATE(722534400, 8, 963, 2, 24850), > + RK3588_PLL_RATE(702000000, 3, 351, 2, 0), > RK3588_PLL_RATE(600000000, 2, 200, 2, 0), > RK3588_PLL_RATE(594000000, 2, 198, 2, 0), > RK3588_PLL_RATE(200000000, 3, 400, 4, 0), > @@ -65,6 +66,11 @@ static struct rockchip_pll_clock rk3588_pll_clks[] = { > RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), > [PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3588_PMU_PLL_CON(128), > RK3588_MODE_CON0, 10, 15, 0, rk3588_pll_rates), > +#ifdef CONFIG_SPL_BUILD > + /* SBUSCRU MODE_CON has the same register offset as the main MODE_CON */ > + [SPLL] = PLL(pll_rk3588, 0, RK3588_PLL_CON(136), > + RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), FYI, it seems the rk3588 clock driver doesn't lock the PLL with the lock_shift member, so 15 here is useless. Maybe we should switch RK3588_PLLCON6_LOCK_STATUS to (1 << lock_shift) there for it to make sense. Anyway, nothing specific to your patch. RK3588_PLL_CON(136) is unreadable :/ Though I understand where it's coming from as we have the same issue from V0PLL to PPLL. Can't we have something a bit more proper here, e.g. #define RK3588_SBUSCRU_SPLL_CON(x) ((x) * 4 + 0x220) and then use RK3588_SBUSCRU_SPLL_CON(0) here? I'm also a bit wary of defining SPLL (and for that matter also V0PLL to PPLL) with offsets relative to a different base than CRU (SBUSCRU for SPLL for example) while all the others seem to have offsets relative to CRU, c.f. RK3588_B0_PLL_CON(x). Specifically, it seems we are calling rockchip_pll_set_rate with priv->cru which is the base of CRU. I am now not entirely sure anything from V0PLL to PPLL is actually working since we use offsets relative to some xCRU but call the function with the CRU_BASE. So... wondering if we shouldn't have: #define RK3588_SBUSCRU_BASE 0x18000 #define RK3588_SBUSCRU_SPLL_CON(x) ((x) * 4 + 0x220 + RK3588_SBUSCRU_BASE) and then in the probe of the scru driver, use CRU_BASE as the base, otherwise we're doing some mixing and I don't like that too much. Or.... What about making this handled the same way as other clocks in SCRU, without actually using the table? Or... Have another table just for SCRU in SPL and migrate existing clocks to use rockchip_pll_set_rate with that new table? > +#endif > }; > > #ifndef CONFIG_SPL_BUILD > @@ -2044,6 +2050,7 @@ U_BOOT_DRIVER(rockchip_rk3588_cru) = { > > #ifdef CONFIG_SPL_BUILD > #define SCRU_BASE 0xfd7d0000 > +#define BUSSCRU_BASE 0xfd7d8000 Can you please rename to SBUSCRU_BASE to match the TRM? Cheers, Quentin
Hi Quentin, Am Mittwoch, 22. Mai 2024, 15:59:24 CEST schrieb Quentin Schulz: > On 5/22/24 2:15 PM, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > > > All parts expect the SPLL to run at 702MHz. In U-Boot it's the SPLL_HZ > > declaring this rate and in the kernel it's a fixed clock definition. > > > > While everything is expecting 702MHz, the SPLL is not running that > > frequency when coming from the bootrom though, instead it's running > > at 351MHz and the vendor-u-boot just sets it to the expected frequency. > > > > The SPLL itself is located inside the secure-BUSCRU and in theory > > accessible as an SCMI clock, though this requires an unknown amount > > of cooperation from trusted-firmware to set at a later stage, though > > during the SPL stage we can still access the relevant CRU directly. > > > > The SPLL is for example necessary for the DSI controllers to produce > > output. > > > > As the SPLL is "just" another rk3588 pll, just set the desired rate > > directly during the SPL stage. > > > > Tested on rk3588-rock5b and rk3588-tiger by reading back the PLL rate > > and also observing working DSI output with this change. > > > > Fixes: 6737771600d4 ("rockchip: rk3588: Add support for sdmmc clocks in SPL") > > Suggested-by: Andy Yan <andy.yan@rock-chips.com> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > > Cc: Jonas Karlman <jonas@kwiboo.se> > > --- > > Resend, because I forgot the u-boot list > > > > .../include/asm/arch-rockchip/cru_rk3588.h | 1 + > > drivers/clk/rockchip/clk_rk3588.c | 26 ++++++++++++++++--- > > 2 files changed, 24 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > > index a4507e5fdd7..85b4da0bc2c 100644 > > --- a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > > +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h > > @@ -29,6 +29,7 @@ enum rk3588_pll_id { > > V0PLL, > > AUPLL, > > PPLL, > > + SPLL, > > PLL_COUNT, > > }; > > > > diff --git a/drivers/clk/rockchip/clk_rk3588.c b/drivers/clk/rockchip/clk_rk3588.c > > index 4c611a39049..5384b3213f5 100644 > > --- a/drivers/clk/rockchip/clk_rk3588.c > > +++ b/drivers/clk/rockchip/clk_rk3588.c > > @@ -37,6 +37,7 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = { > > RK3588_PLL_RATE(786000000, 1, 131, 2, 0), > > RK3588_PLL_RATE(742500000, 4, 495, 2, 0), > > RK3588_PLL_RATE(722534400, 8, 963, 2, 24850), > > + RK3588_PLL_RATE(702000000, 3, 351, 2, 0), > > RK3588_PLL_RATE(600000000, 2, 200, 2, 0), > > RK3588_PLL_RATE(594000000, 2, 198, 2, 0), > > RK3588_PLL_RATE(200000000, 3, 400, 4, 0), > > @@ -65,6 +66,11 @@ static struct rockchip_pll_clock rk3588_pll_clks[] = { > > RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), > > [PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3588_PMU_PLL_CON(128), > > RK3588_MODE_CON0, 10, 15, 0, rk3588_pll_rates), > > +#ifdef CONFIG_SPL_BUILD > > + /* SBUSCRU MODE_CON has the same register offset as the main MODE_CON */ > > + [SPLL] = PLL(pll_rk3588, 0, RK3588_PLL_CON(136), > > + RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), > > FYI, it seems the rk3588 clock driver doesn't lock the PLL with the > lock_shift member, so 15 here is useless. Maybe we should switch > RK3588_PLLCON6_LOCK_STATUS to (1 << lock_shift) there for it to make > sense. Anyway, nothing specific to your patch. > > RK3588_PLL_CON(136) is unreadable :/ Though I understand where it's > coming from as we have the same issue from V0PLL to PPLL. Can't we have > something a bit more proper here, e.g. > > #define RK3588_SBUSCRU_SPLL_CON(x) ((x) * 4 + 0x220) > > and then use RK3588_SBUSCRU_SPLL_CON(0) here? sounds doable ;-) > I'm also a bit wary of defining SPLL (and for that matter also V0PLL to > PPLL) with offsets relative to a different base than CRU (SBUSCRU for > SPLL for example) while all the others seem to have offsets relative to > CRU, c.f. RK3588_B0_PLL_CON(x). Specifically, it seems we are calling > rockchip_pll_set_rate with priv->cru which is the base of CRU. I am now > not entirely sure anything from V0PLL to PPLL is actually working since > we use offsets relative to some xCRU but call the function with the > CRU_BASE. > > So... wondering if we shouldn't have: > > #define RK3588_SBUSCRU_BASE 0x18000 > #define RK3588_SBUSCRU_SPLL_CON(x) ((x) * 4 + 0x220 + RK3588_SBUSCRU_BASE) > > and then in the probe of the scru driver, use CRU_BASE as the base, > otherwise we're doing some mixing and I don't like that too much. Or.... At least for the SPLL we're calling ret = rockchip_pll_set_rate(&rk3588_pll_clks[SPLL], (void *)BUSSCRU_BASE, SPLL, SPLL_HZ); so no mention of priv->cru there at all and the pll-function internally only hand down that iomem pointer. The scru-clock driver also is very specific to the SPL, as it the whole thing will be inaccessible after TF-A has run. Doing some janky maths on top of a different base definitly sounds a lot worse than just having a comment above the PLL definition stating that it belongs to the SBUSCRU ;-) . > What about making this handled the same way as other clocks in SCRU, > without actually using the table? Or... Have another table just for SCRU > in SPL and migrate existing clocks to use rockchip_pll_set_rate with > that new table? The rk3588-pll getter/setter relies on the pll id to do even more special- case handling. See all the pll_id == x checks in clk-pll.c, hence the PLL-id is sort of global over the whole set of PLLs > > > +#endif > > }; > > > > #ifndef CONFIG_SPL_BUILD > > @@ -2044,6 +2050,7 @@ U_BOOT_DRIVER(rockchip_rk3588_cru) = { > > > > #ifdef CONFIG_SPL_BUILD > > #define SCRU_BASE 0xfd7d0000 > > +#define BUSSCRU_BASE 0xfd7d8000 > > Can you please rename to SBUSCRU_BASE to match the TRM? ok Heiko
Hi Heiko, On 5/22/24 4:18 PM, Heiko Stübner wrote: > Hi Quentin, > > Am Mittwoch, 22. Mai 2024, 15:59:24 CEST schrieb Quentin Schulz: >> On 5/22/24 2:15 PM, Heiko Stuebner wrote: [...] >> I'm also a bit wary of defining SPLL (and for that matter also V0PLL to >> PPLL) with offsets relative to a different base than CRU (SBUSCRU for >> SPLL for example) while all the others seem to have offsets relative to >> CRU, c.f. RK3588_B0_PLL_CON(x). Specifically, it seems we are calling >> rockchip_pll_set_rate with priv->cru which is the base of CRU. I am now >> not entirely sure anything from V0PLL to PPLL is actually working since >> we use offsets relative to some xCRU but call the function with the >> CRU_BASE. >> I checked again and they are actually using proper offsets. All in CRU are using CRU_BASE, others all add the offset of their base, e.g. RK3588_PHP_CRU_BASE for PPLL, RK3588_BIGCORE0_CRU_BASE for B0PLL, RK3588_BIGCORE1_CRU_BASE for B1PLL, RK3588_DSU_CRU_BASE for LPLL. >> So... wondering if we shouldn't have: >> >> #define RK3588_SBUSCRU_BASE 0x18000 >> #define RK3588_SBUSCRU_SPLL_CON(x) ((x) * 4 + 0x220 + RK3588_SBUSCRU_BASE) >> >> and then in the probe of the scru driver, use CRU_BASE as the base, >> otherwise we're doing some mixing and I don't like that too much. Or.... > > At least for the SPLL we're calling > > ret = rockchip_pll_set_rate(&rk3588_pll_clks[SPLL], > (void *)BUSSCRU_BASE, SPLL, SPLL_HZ); > > so no mention of priv->cru there at all and the pll-function internally > only hand down that iomem pointer. The scru-clock driver also is > very specific to the SPL, as it the whole thing will be inaccessible > after TF-A has run. > > Doing some janky maths on top of a different base definitly sounds > a lot worse than just having a comment above the PLL definition > stating that it belongs to the SBUSCRU ;-) . > The thing is, everything in that array actually uses CRU_BASE as base, so for consistency-sake... doing the janky maths makes sense to me :) Up to you, at the very least please have the comment. > >> What about making this handled the same way as other clocks in SCRU, >> without actually using the table? Or... Have another table just for SCRU >> in SPL and migrate existing clocks to use rockchip_pll_set_rate with >> that new table? > > The rk3588-pll getter/setter relies on the pll id to do even more special- > case handling. See all the pll_id == x checks in clk-pll.c, hence the PLL-id > is sort of global over the whole set of PLLs > Yeah... an enum for that would have been nice as well. Still nothing for this patch :) Cheers, Quentin
diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h index a4507e5fdd7..85b4da0bc2c 100644 --- a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h @@ -29,6 +29,7 @@ enum rk3588_pll_id { V0PLL, AUPLL, PPLL, + SPLL, PLL_COUNT, }; diff --git a/drivers/clk/rockchip/clk_rk3588.c b/drivers/clk/rockchip/clk_rk3588.c index 4c611a39049..5384b3213f5 100644 --- a/drivers/clk/rockchip/clk_rk3588.c +++ b/drivers/clk/rockchip/clk_rk3588.c @@ -37,6 +37,7 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = { RK3588_PLL_RATE(786000000, 1, 131, 2, 0), RK3588_PLL_RATE(742500000, 4, 495, 2, 0), RK3588_PLL_RATE(722534400, 8, 963, 2, 24850), + RK3588_PLL_RATE(702000000, 3, 351, 2, 0), RK3588_PLL_RATE(600000000, 2, 200, 2, 0), RK3588_PLL_RATE(594000000, 2, 198, 2, 0), RK3588_PLL_RATE(200000000, 3, 400, 4, 0), @@ -65,6 +66,11 @@ static struct rockchip_pll_clock rk3588_pll_clks[] = { RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), [PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3588_PMU_PLL_CON(128), RK3588_MODE_CON0, 10, 15, 0, rk3588_pll_rates), +#ifdef CONFIG_SPL_BUILD + /* SBUSCRU MODE_CON has the same register offset as the main MODE_CON */ + [SPLL] = PLL(pll_rk3588, 0, RK3588_PLL_CON(136), + RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates), +#endif }; #ifndef CONFIG_SPL_BUILD @@ -2044,6 +2050,7 @@ U_BOOT_DRIVER(rockchip_rk3588_cru) = { #ifdef CONFIG_SPL_BUILD #define SCRU_BASE 0xfd7d0000 +#define BUSSCRU_BASE 0xfd7d8000 static ulong rk3588_scru_clk_get_rate(struct clk *clk) { @@ -2118,15 +2125,28 @@ static ulong rk3588_scru_clk_set_rate(struct clk *clk, ulong rate) return rk3588_scru_clk_get_rate(clk); } +static int rk3588_scru_clk_probe(struct udevice *dev) +{ + int ret; + + ret = rockchip_pll_set_rate(&rk3588_pll_clks[SPLL], + (void *)BUSSCRU_BASE, SPLL, SPLL_HZ); + if (ret) + debug("%s setting spll rate failed %d\n", __func__, ret); + + return 0; +} + static const struct clk_ops rk3588_scru_clk_ops = { .get_rate = rk3588_scru_clk_get_rate, .set_rate = rk3588_scru_clk_set_rate, }; U_BOOT_DRIVER(rockchip_rk3588_scru) = { - .name = "rockchip_rk3588_scru", - .id = UCLASS_CLK, - .ops = &rk3588_scru_clk_ops, + .name = "rockchip_rk3588_scru", + .id = UCLASS_CLK, + .ops = &rk3588_scru_clk_ops, + .probe = rk3588_scru_clk_probe, }; static int rk3588_scmi_spl_glue_bind(struct udevice *dev)