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

[Quentin Schulz]

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