Re: [PATCH v4] clk: spacemit: k3: add CPU PLL rate tables

["Troy Mitchell" <troy.mitchell@linux.spacemit.com>]

[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]

Hi Yixun, Aurelien

On Sun Sep 6, 2026 at 6:07 AM +08, Yixun Lan wrote:
> Hi Aurelien, Troy,
>
> On 11:24 Fri 04 Sep     , Troy Mitchell wrote:
>> Hi Aurelien,
>> 
>> On Fri Sep 4, 2026 at 1:27 AM +08, Aurelien Jarno wrote:
>> > Hi Troy,
>> >
> ..
>> >>  drivers/clk/spacemit/ccu-k3.c | 96 +++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 96 insertions(+)
>> >
>> > Thanks for the patch, it makes sense to add all the values, to support 
>> > future firmware changes. The values match the ones in the vendor kernel.
>> I forgot that you had a similar patch as well..
>> 
>> >
>> >> diff --git a/drivers/clk/spacemit/ccu-k3.c b/drivers/clk/spacemit/ccu-k3.c
>> >> index 92b930d5ff30..950bf531c734 100644
>> >> --- a/drivers/clk/spacemit/ccu-k3.c
>> >> +++ b/drivers/clk/spacemit/ccu-k3.c
>> >> @@ -29,15 +29,91 @@ static const struct ccu_pll_rate_tbl pll2_rate_tbl[] = {
>> >>  	CCU_PLLA_RATE(3000000000UL, 0x0b3e2000, 0x00000000, 0xa0558c8c),
>> >>  };
>> >>  
>> >> +/* Keep the firmware default as the first entry. */
>> >>  static const struct ccu_pll_rate_tbl pll3_rate_tbl[] = {
>> >> +	CCU_PLLA_RATE(2000000000UL, 0x0b292aaa, 0x0000ab00, 0xa0558686),
>> >
>> > Is it really necessary to put this entry first? I guess it's the current 
>> > default frequency, but we have seen it can evolve. From my tests, the 
>> > driver seems to just keep the value configured by the firmware 
>> > independently from the table order, so I am not sure it's necessary.
>
> I agree with Aurelien here, the default frequency isn't a solid stable settings,
> which it may changes in the future
>
>> I think keeping it first is necessary. If the current register settings do
>> not match any entry in the table, ccu_plla_init() falls back to the first
>
> Isn't it a bug if all frequencies added still couldn't cover all settings?
> I'd simply suggest to bail out and yell loudly if there is a mismatch
>
> I just don't like putting specific frequency at first one as default,
> this logic is kind of fragile and tend to fail easily..
I checked the Rockchip PLL driver. The RK3036, RK3066 and RK3399
implementations calculate the current rate directly from the hardware
register parameters. If that rate is absent from the rate table, their PLL
init callbacks return without reprogramming the PLL. With
ROCKCHIP_PLL_SYNC_RATE enabled, they only synchronize the parameters when
the current rate matches a table entry but the register parameters differ
from those in that entry.

This approach seems preferable to me. Would it make sense to sort the rate
tables by frequency in the next revision, and follow up with a separate
series to calculate the current rate from the registers and preserve
configurations whose rates are absent from the table, as Rockchip does?
>
>> entry:
>> 
>> 
>> 	ccu_plla_update_param(pll, &pll->config.rate_tbl[0]);
>> 


-- 
Troy Mitchell


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]