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

From: Yixun Lan <dlan@kernel.org>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>,
	Michael Turquette <mturquette@baylibre.com>,
	Inochi Amaoto <inochiama@outlook.com>,
	Alex Elder <elder@riscstar.com>, Haylen Chu <heylenay@4d2.org>,
	stable@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	Brian Masney <bmasney+clk@redhat.com>,
	Jerome Brunet <jbrunet+clk@baylibre.com>,
	Paul Walmsley <pjw@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Alexandre Ghiti <alex@ghiti.fr>,
	linux-clk@vger.kernel.org, linux-riscv@lists.infradead.org,
	spacemit@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] clk: spacemit: k3: add CPU PLL rate tables
Date: Sat, 5 Sep 2026 22:07:46 +0000	[thread overview]
Message-ID: <20260905220746-GKC53903@kernel.org> (raw)
In-Reply-To: <DL67LEVCXFA5.39M5FO7JGKWZS@linux.spacemit.com>

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)