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

[Yixun Lan]

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..

> entry:
> 
> 
> 	ccu_plla_update_param(pll, &pll->config.rate_tbl[0]);
> 

-- 
Yixun Lan (dlan)