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

[sashiko-bot@kernel.org]

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)?

> 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()?

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

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

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