Re: [PATCH RESEND] clk: rockchip: rk3588: Set SPLL frequency during SPL stage

["Heiko Stübner" <heiko@sntech.de>]

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