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

[Yixun Lan <dlan@kernel.org>]

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


Hi Troy,

On 19:20 Sun 06 Sep     , Troy Mitchell wrote:
> 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?

Yes, I generally agree with this approach, so let's push current patch first 
ith sorted frequencies, then improve further later in another series.. thanks

-- 
Yixun Lan (dlan)

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