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

[Aurelien Jarno]

Hi Troy,

On 2026-09-04 11:24, Troy Mitchell wrote:
> Hi Aurelien,
> 
> On Fri Sep 4, 2026 at 1:27 AM +08, Aurelien Jarno wrote:
> > Hi Troy,
> >
> > On 2026-09-03 11:06, Troy Mitchell wrote:
> >> The K3 CPU PLL rate tables currently describe only one rate per PLL,
> >> although the hardware supports a wider range.
> >> 
> >> PLL3 and PLL4 support rates from 1.05 to 2.4 GHz, while PLL5 and PLL8
> >> support rates from 1.05 to 2 GHz. Populate the tables with every
> >> supported rate in 50 MHz steps.
> >> 
> >> Cc: stable@xxxxxxxxxxxxxxx # 7.0+
> >> Signed-off-by: Troy Mitchell <troy.mitchell@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> The corresponding U-Boot changes are available at:
> >> 
> >> - Add the CPU PLL rate tables:
> >>   https://github.com/spacemit-com/uboot-2022.10/commit/787b5cf44c8dbc99db886d5522c0c9cd5a96ffd6
> >> - Correct the 1.10 GHz and 2.15 GHz table entries:
> >>   https://github.com/spacemit-com/uboot-2022.10/commit/16a775c20e9efb54ca55352215e76eb3f12e6cf0
> >> ---
> >> Changes in v4:
> >> - Expand the update to every supported PLL3, PLL4, PLL5 and PLL8 rate.
> >> - Drop Aurelien Jarno's Reviewed-by and Tested-by trailers because the
> >>   patch scope has expanded.
> >> - Add U-Boot references for the complete tables and corrected entries.
> >> - Link to v3: https://patch.msgid.link/20260831-k3-pll5-pll8-1800mhz-v3-1-530ed0978615@xxxxxxxxxxxxxxxxxx
> >> 
> >> Changes in v3:
> >> - Mark the change for stable kernels starting with v7.0.
> >> - Add Aurelien Jarno's Reviewed-by and Tested-by trailers.
> >> - Rebase onto v7.3-rc1.
> >> - Link to v2: https://patch.msgid.link/20260810-k3-pll5-pll8-1800mhz-v2-1-aabff510a797@xxxxxxxxxxxxxxxxxx
> >> 
> >> Changes in v2:
> >> - Drop "clk: spacemit: re-enable PLLs after init"; the PLL init issue
> >>   will be handled in a separate series.
> >> - Send the K3 PLL5 and PLL8 1.8 GHz rate-table update as a standalone
> >>   patch.
> >> - Clarify that 1.8 GHz is nominal and 2 GHz is an overclocking rate.
> >> - Link to v1: https://patch.msgid.link/20260804-k3-pll5-pll8-1800mhz-v1-0-a3a75866ce32@xxxxxxxxxxxxxxxxxx
> >> 
> >> To: Stephen Boyd <sboyd@xxxxxxxxxx>
> >> To: Brian Masney <bmasney+clk@xxxxxxxxxx>
> >> To: Jerome Brunet <jbrunet+clk@xxxxxxxxxxxx>
> >> To: Yixun Lan <dlan@xxxxxxxxxx>
> >> To: Paul Walmsley <pjw@xxxxxxxxxx>
> >> To: Palmer Dabbelt <palmer@xxxxxxxxxxx>
> >> To: Albert Ou <aou@xxxxxxxxxxxxxxxxx>
> >> To: Alexandre Ghiti <alex@xxxxxxxx>
> >> Cc: linux-clk@xxxxxxxxxxxxxxx
> >> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx
> >> Cc: spacemit@xxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> ---
> >>  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 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
> entry:
> 
> 
> 	ccu_plla_update_param(pll, &pll->config.rate_tbl[0]);
> 
> [...]

For the A100 cores, that indeed makes sense. For the X100 cores this 
line is never reached, as the line above just stops the PLL, basically 
stopping the cores:

        if (ccu_plla_lookup_matched_entry(pll))
                return 0;

        ccu_plla_disable(hw);
        ccu_plla_update_param(pll, &pll->config.rate_tbl[0]);

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@xxxxxxxxxxx                     http://aurel32.net

Attachment: signature.asc
Description: PGP signature