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

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

On 10/29/2026 5:57 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] clk: meson: pll: CLK_SET_RATE_PARENT on ro_ops PLL DCOs breaks rate rounding
> - [High] clk: meson: pll: meson_clk_get_pll_table() fails on non-exact multipliers
> - [High] clk: meson: meson8b: missing CLK_SET_RATE_PARENT on hdmi_pll_dco_in
> - [High] clk: meson: pll: cached 0 Hz pre-divider rate on directly programmed PLLs
> --
>
> commit b0439d678a94dfead2cca909902c6363ae03f5d0
> Author: Jian Hu <jian.hu@amlogic.com>
>
> clk: meson: pll: Remove the dedicated n parameter
>
> This commit removes the dedicated pre-divider (n) field from
> struct meson_clk_pll_data and simplifies the Meson PLL rate calculation.
> Each PLL pre-divider is instead modeled as a standalone divider clock that
> serves as the parent of the PLL DCO clock.
>
>> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
>> index 280842e9cb371..e6e6b9a8030c0 100644
>> --- a/drivers/clk/meson/axg.c
>> +++ b/drivers/clk/meson/axg.c
> [ ... ]
>
>> @@ -202,10 +240,11 @@ static struct clk_regmap axg_sys_pll_dco = {
>>        .hw.init = &(struct clk_init_data){
>>                .name = "sys_pll_dco",
>>                .ops = &meson_clk_pll_ro_ops,
>> -             .parent_data = &(const struct clk_parent_data) {
>> -                     .fw_name = "xtal",
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &axg_sys_prediv.hw
>>                },
>>                .num_parents = 1,
>> +             .flags = CLK_SET_RATE_PARENT,
>>        },
>>   };
> [Severity: High]
> Does setting CLK_SET_RATE_PARENT on a clock using meson_clk_pll_ro_ops break
> rate rounding and rate calculation in the common clock framework?
>
> meson_clk_pll_ro_ops only provides .init, .recalc_rate, and .is_enabled, and
> does not implement .determine_rate or .round_rate, so clk_core_can_round() is
> false.
>
> In drivers/clk/clk.c, clk_core_round_rate_nolock() checks:
>
>          if (clk_core_can_round(core))
>                  return core->ops->determine_rate(core->hw, req);
>          else if (core->flags & CLK_SET_RATE_PARENT)
>                  return clk_core_round_rate_nolock(core->parent, req);
>
> When CLK_SET_RATE_PARENT is set on a clock that cannot round its own rate,
> clk_core_round_rate_nolock() and clk_calc_new_rates() treat the clock as a
> 1:1 pass-through of its parent and set the rate to the parent pre-divider
> rate (24 MHz or 48 MHz) instead of the multi-GHz DCO frequency.
>
> This affects child dividers with CLK_SET_RATE_PARENT when consumer drivers
> call clk_round_rate() or clk_set_rate(). The same pattern is also added on:
>
>    - sm1_gp1_pll_dco and g12a_hdmi_pll_dco in drivers/clk/meson/g12a.c
>    - gxbb_hdmi_pll_dco, gxl_hdmi_pll_dco, and gxbb_sys_pll_dco in
>      drivers/clk/meson/gxbb.c
>    - s4_fixed_pll_dco in drivers/clk/meson/s4-pll.c
>
> Should these meson_clk_pll_ro_ops DCO clocks omit CLK_SET_RATE_PARENT (and
> use clk_regmap_divider_ro_ops for their pre-dividers)?


Yes, the meson_clk_pll_ro_ops DCO's CLK_SET_RATE_PARENT should be 
removed. And the pre-dividers

use ro_ops. I will fix these in the next version.

>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index 1ea6579a760f3..a7e3f8c8eb783 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
> [ ... ]
>
>> +static int meson_clk_get_pll_table(unsigned long rate,
>> +                                unsigned long parent_rate,
>> +                                unsigned int *m,
>> +                                struct meson_clk_pll_data *pll)
>> +{
>> +     unsigned int i;
>> +     int ret;
>> +
>> +     for (i = 0, ret = 0; !ret; i++) {
>> +             if (!pll->table[i].m)
>> +                     return -EINVAL;
>> +
>> +             if (pll->table[i].m == rate / parent_rate) {
>> +                     *m = pll->table[i].m;
>> +                     return 0;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
> [Severity: High]
> What happens in meson_clk_get_pll_table() when rate / parent_rate does not
> exactly match an entry in pll->table?
>
> Previously, meson_clk_get_pll_settings() iterated over pll->table and used
> meson_clk_pll_is_better() to pick either the closest entry (when
> __pll_round_closest_mult(pll) was true) or the largest table entry with a
> rate less than or equal to the requested rate.
>
> With this change, meson_clk_get_pll_table() requires an exact match for
> rate / parent_rate and returns -EINVAL otherwise:
>
> 1. Sparse multiplier tables such as sys_pll_params_table (which has a gap
>     between m = 68 and m = 84 where Meson8 CPU OPPs at 1800 MHz and 1992 MHz
>     fall) and meson8b_hdmi_pll_params_table in drivers/clk/meson/meson8b.c
>     will fail with -EINVAL in meson_clk_pll_determine_rate() and
>     meson_clk_pll_set_rate() when rate / parent_rate lands in a gap.
>
> 2. Unlike meson_clk_get_pll_range(), which clamps to min and max and returns
>     -ENODATA, meson_clk_get_pll_table() returns -EINVAL when rate / parent_rate
>     is below the minimum or above the maximum multiplier in the table. When a
>     child post-divider runs clk_divider_bestdiv(), any candidate rate * i
>     outside the table range fails, and the fallback clk_hw_round_rate(parent, 1)
>     also fails with -EINVAL because 1 / parent_rate is 0, causing
>     clk_round_rate() to return 0 Hz.
>
> 3. meson_clk_get_pll_table() no longer checks __pll_round_closest_mult(pll)
>     (for example on axg_hifi_pll_dco when fractional mode is not used), and
>     leaves ret unmodified in the loop with an unreachable return 0 at the end.
>
> Should meson_clk_get_pll_table() retain the best-match and clamping behavior
> from meson_clk_get_pll_settings()?


Ok, I will retain the original best-match algorithm.

>> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
>> index bc029c6ab622d..0c9ba079bc51a 100644
>> --- a/drivers/clk/meson/meson8b.c
>> +++ b/drivers/clk/meson/meson8b.c
> [ ... ]
>
>> @@ -185,15 +209,38 @@ static struct clk_regmap meson8b_fixed_pll = {
>>        },
>>   };
>>
>> +static const struct clk_div_table meson8b_prediv_div_table[] = {
>> +     { .val = 1, .div = 1 },
>> +     { /* sentinel */ }
>> +};
>> +
>> +static struct clk_regmap meson8b_hdmi_prediv = {
>> +     .data = &(struct clk_regmap_div_data){
>> +             .offset = HHI_VID_PLL_CNTL,
>> +             .shift   = 10,
>> +             .width   = 5,
>> +             .table = meson8b_prediv_div_table,
>> +             .flags = CLK_DIVIDER_INVALID_ZERO,
>> +     },
>> +     .hw.init = &(struct clk_init_data){
>> +             .name = "hdmi_prediv",
>> +             .ops = &clk_regmap_divider_ops,
>> +             .parent_data = &(const struct clk_parent_data) {
>> +                     .fw_name = "xtal",
>> +                     .index = -1,
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
>> +
>>   static struct clk_fixed_factor hdmi_pll_dco_in = {
>>        .mult = 2,
>>        .div = 1,
>>        .hw.init = &(struct clk_init_data){
>>                .name = "hdmi_pll_dco_in",
>>                .ops = &clk_fixed_factor_ops,
>> -             .parent_data = &(const struct clk_parent_data) {
>> -                     .fw_name = "xtal",
>> -                     .index = -1,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &meson8b_hdmi_prediv.hw
>>                },
>>                .num_parents = 1,
>>        },
> [ ... ]
>
>> @@ -279,6 +321,7 @@ static struct clk_regmap meson8b_hdmi_pll_dco = {
>>                        &hdmi_pll_dco_in.hw
>>                },
>>                .num_parents = 1,
>> +             .flags = CLK_SET_RATE_PARENT,
>>        },
>>   };
> [Severity: High]
> Can rate requests from meson8b_hdmi_pll_dco reach meson8b_hdmi_prediv without
> CLK_SET_RATE_PARENT on hdmi_pll_dco_in?
>
> meson8b_hdmi_pll_dco has hdmi_pll_dco_in as its parent, and hdmi_pll_dco_in
> has meson8b_hdmi_prediv as its parent:
>
>    meson8b_hdmi_prediv -> hdmi_pll_dco_in -> meson8b_hdmi_pll_dco
>
> Because hdmi_pll_dco_in does not set CLK_SET_RATE_PARENT, when
> meson8b_hdmi_prediv resets to 0 and reports a 0 Hz rate via
> CLK_DIVIDER_INVALID_ZERO, meson_clk_pll_determine_rate() calls
> clk_hw_round_rate() on hdmi_pll_dco_in, and clk_factor_determine_rate()
> returns 0 without rounding meson8b_hdmi_prediv.
>
> This causes meson_clk_pll_determine_rate() to return -EINVAL and prevents
> clk_calc_new_rates() from propagating rate changes to program
> meson8b_hdmi_prediv. Should hdmi_pll_dco_in also have
> .flags = CLK_SET_RATE_PARENT?


Ok , I will add the missing CLK_SET_RATE_PARENT flag for hdmi_pll_dco_in.

>> diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
>> index 0a622f45fa368..28d563fcb1bd2 100644
>> --- a/drivers/clk/meson/t7-pll.c
>> +++ b/drivers/clk/meson/t7-pll.c
> [ ... ]
>
>> @@ -292,6 +346,24 @@ static struct clk_regmap t7_hifi_pll = {
>>        },
>>   };
>>
>> +static struct clk_regmap t7_pcie_prediv = {
>> +     .data = &(struct clk_regmap_div_data){
>> +             .offset = PCIEPLL_CTRL0,
>> +             .shift = 10,
>> +             .width = 5,
>> +             .table = t7_prediv_div_table,
>> +             .flags = CLK_DIVIDER_INVALID_ZERO,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "pcie_prediv",
>> +             .ops = &clk_regmap_divider_ops,
>> +             .parent_data = &(const struct clk_parent_data) {
>> +                     .fw_name = "in0",
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
> [ ... ]
>
>> @@ -345,10 +412,11 @@ static struct clk_regmap t7_pcie_pll_dco = {
>>        .hw.init = &(struct clk_init_data){
>>                .name = "pcie_pll_dco",
>>                .ops = &meson_clk_pcie_pll_ops,
>> -             .parent_data = &(const struct clk_parent_data) {
>> -                     .fw_name = "in0",
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &t7_pcie_prediv.hw
>>                },
>>                .num_parents = 1,
>> +             .flags = CLK_SET_RATE_PARENT,
>>        },
>>   };
> [Severity: High]
> Will t7_pcie_prediv (as well as g12a_hdmi_prediv, gxbb_hdmi_prediv, and
> gxl_hdmi_prediv) remain stuck at a cached rate of 0 Hz if the hardware
> pre-divider field is 0 at clock registration time?
>
> t7_pcie_pll_dco uses meson_clk_pcie_pll_ops and defines neither .table nor
> .range; it only programs PCIEPLL_CTRL0 (including N = 1) inside
> meson_clk_pcie_pll_enable(). If PCIEPLL_CTRL0 has N = 0 at registration,
> t7_pcie_prediv caches a rate of 0 Hz via CLK_DIVIDER_INVALID_ZERO. Because
> clk_set_rate() cannot be used on t7_pcie_pll_dco (meson_clk_get_pll_max_multiplier()
> returns 0 when neither .range nor .table is set) and t7_pcie_prediv does not
> have CLK_GET_RATE_NOCACHE, t7_pcie_prediv->rate is never recalculated after
> meson_clk_pcie_pll_enable() writes PCIEPLL_CTRL0.
>
> Similarly, in drivers/clk/meson/g12a.c (g12a_hdmi_prediv) and
> drivers/clk/meson/gxbb.c (gxbb_hdmi_prediv and gxl_hdmi_prediv), the HDMI PLL
> registers are written directly by the DRM driver in
> drivers/gpu/drm/meson/meson_vclk.c rather than through clk_set_rate(). While
> hdmi_pll_dco has CLK_GET_RATE_NOCACHE, clk_core_get_rate_recalc() only
> recalculates downward from the clock with CLK_GET_RATE_NOCACHE using
> core->parent->rate. Because the parent hdmi_prediv clocks lack
> CLK_GET_RATE_NOCACHE, their cached rate stays 0 Hz even after the display
> driver programs N = 1, causing meson_clk_pll_recalc_rate() on hdmi_pll_dco to
> return 0 Hz.
>
> Should pre-dividers whose registers are programmed outside clk_set_rate() be
> marked with CLK_GET_RATE_NOCACHE?


Ok, I will add CLK_GET_RATE_NOCACHE for 
t7_pcie_prediv/g12a_hdmi_prediv/gxbb_hdmi_prediv/gxl_hdmi_prediv.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261029-meson_refactor_n-v2-0-ff4ac74e527b@amlogic.com?part=3

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