Re: [PATCH RFC 1/3] clk: meson: pll: Remove the dedicated n parameter

[Jian Hu <jian.hu@amlogic.com>]

On 9/23/2026 7:24 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] clk: meson: struct meson_clk_pll_data removal of 'n' breaks compilation
> - [High] clk: meson: pll unconditionally requires range, breaking table-based PLLs
> - [High] clk: meson: external pre-divider violates PLL disable/reset sequencing
> --
>
> commit 9a0d9eb075576d244c161f62134109ad2353a8b7
> Author: Jian Hu <jian.hu@amlogic.com>
>
> clk: meson: pll: Remove the dedicated n parameter
>
> The Meson PLL framework models the PLL pre-divider (N) using a dedicated
> field in struct meson_clk_pll_data. Since the common clock framework already
> provides a generic divider implementation, this patch removes the dedicated
> n parameter from the framework and simplifies the PLL rate calculation.
>
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
> [ ... ]
>> @@ -119,145 +107,48 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
> [ ... ]
>> -static int meson_clk_get_pll_table_index(unsigned int index,
>> -                                      unsigned int *m,
>> -                                      unsigned int *n,
>> -                                      struct meson_clk_pll_data *pll)
>> +static int meson_clk_get_pll_multiplier(unsigned long rate,
>> +                                     unsigned long parent_rate,
>> +                                     unsigned int *m,
>> +                                     struct meson_clk_pll_data *pll)
>>   {
>> -     if (!pll->table[index].n)
>> +     if (!pll->range)
>>                return -EINVAL;
> [Severity: High]
> Does this unconditionally reject any PLL that lacks a .range definition?
>
> Many table-based PLLs in Meson SoC drivers (like axg_gp0_pll_dco) leave
> .range as NULL and rely strictly on .table. Without .range, rate calculations
> for these PLLs will deterministically fail at runtime.


Yes, the table consists of m and n, and n has now been deleted.

Therefore, the table method will be converted to the range method.

> [ ... ]
>> @@ -420,7 +311,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> [ ... ]
>>        enabled = meson_parm_read(clk->map, &pll->en);
>>        if (enabled)
>>                meson_clk_pll_disable(hw);
>>
>> -     meson_parm_write(clk->map, &pll->n, n);
> [Severity: High]
> Does stripping the write to the N pre-divider from this protected atomic
> section violate the hardware requirement to place the PLL in a disabled and
> reset state before its input frequency changes?
>
> By modelling the pre-divider as an external generic CCF clk_divider,
> its .set_rate will modify the hardware register immediately without
> notifying the running downstream PLL. Can this cause the active PLL to
> lose lock and produce an unstable clock, potentially locking up the system?


Separating N from the PLL will not affect PLL locking.

The PLL locking operation is initiated by the enable signal.

>>        meson_parm_write(clk->map, &pll->m, m);
>>
>>        if (MESON_PARM_APPLICABLE(&pll->frac)) {
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -11,29 +11,17 @@
>>   #include <linux/regmap.h>
>>   #include "parm.h"
>>
>> -struct pll_params_table {
>> -     unsigned int    m;
>> -     unsigned int    n;
>> -};
>> -
> [Severity: High]
> Does removing pll_params_table also remove the framework's capability to
> enforce non-contiguous PLL multiplier rules?


Yes, it losts  non-contiguous PLL multiplier rules

The "range" method can cover the frequency points of the "table" method.

In the pre-divider, N is typically fixed at 1.

>>   struct pll_mult_range {
>>        unsigned int    min;
>>        unsigned int    max;
>>   };
> [ ... ]
>>   struct meson_clk_pll_data {
>>        struct parm en;
>>        struct parm m;
>> -     struct parm n;
> [Severity: Critical]
> Does removing the n and table fields from struct meson_clk_pll_data break
> the build for untouched SoC platform drivers?
>
> Drivers such as axg.c continue to initialize these fields in their static PLL
> definitions (like axg_sys_pll_dco):
>
> static struct clk_regmap axg_sys_pll_dco = {
>          .data = &(struct meson_clk_pll_data){
>                  ...
>                  .n = {
>                          .reg_off = HHI_SYS_PLL_CNTL,
>                          .shift   = 9,
>                          .width   = 5,
>                  },
>
> This will cause an immediate compiler error because the types have been
> removed. Will this breakage persist and break git bisect?


That is indeed the case,  I will include clk-pll.c and all the PLLs in a 
single patch.

>>        struct parm frac;
>>        struct parm l;
>>        struct parm rst;
> [ ... ]
>> @@ -41,7 +29,6 @@ struct meson_clk_pll_data {
>>        struct parm l_detect;
>>        const struct reg_sequence *init_regs;
>>        unsigned int init_count;
>> -     const struct pll_params_table *table;
>>        const struct pll_mult_range *range;
>>        unsigned int frac_max;
>>        u8 flags;
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic