Re: [PATCH v6 07/12] clk: zte: Add zx PLL support infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] zx29_pll_set_rate can fail with -EINVAL for valid configurations due to a strict equality check and integer truncation during recalculation.
- [High] Modifying the PLL rate while it is active without the CLK_SET_RATE_GATE flag can crash downstream hardware.
- [Medium] Repeated error logging inside a nested loop when parent_rate is 0 or calc_values fails floods the kernel logs.
- [Medium] dev_warn is used unconditionally in .determine_rate when a requested rate cannot be matched exactly.
- [Low] Uninitialized stack variable `val` is printed in an error message if `regmap_read_poll_timeout` fails.
--

commit 65cd736a246afa41560a934d5f06e151e30761f6
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Add zx PLL support infrastructure

This patch adds the infrastructure to support ZTE zx platforms' PLL
configuration. It includes rate calculation, prepare/enable handling,
and registration functions for the CCF.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index 1a8cb37ca7e20..f527c15be01e7 100644
> --- a/drivers/clk/zte/pll-zx.c
> +++ b/drivers/clk/zte/pll-zx.c
[ ... ]
> @@ -3,15 +3,493 @@
> +static int zx29_pll_prepare(struct clk_hw *hw)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	u32 val;
> +	int res;
> +
> +	res = regmap_clear_bits(pll->map, pll->reg, ZX29_PLL_DISABLE);
> +	if (res < 0)
> +		return res;
> +
> +	/* Lock duration is usually between 300us to 500us */
> +	res = regmap_read_poll_timeout(pll->map, pll->reg, val, val & ZX29_PLL_LOCKED, 50, 2000);
> +	if (res) {
> +		regmap_set_bits(pll->map, pll->reg, ZX29_PLL_DISABLE);
> +		dev_err(pll->dev, "%s: PLL enable failed: %d\n", clk_hw_get_name(&pll->hw), val);

[Severity: Low]
Is there a chance this could print uninitialized stack memory?

If regmap_read_poll_timeout() fails due to a hardware or regmap fault before
populating 'val', it looks like we might log the uninitialized contents of
'val' here.

> +	}
> +	return res;
> +}
[ ... ]
> +static unsigned long zx29_pll_get_rate(const struct zx29_clk_pll *pll, unsigned long parent_rate,
> +				       u32 setting)
> +{
> +	unsigned long refdiv, fbdiv, postdiv1, postdiv2, freq;
> +	const char *name = clk_hw_get_name(&pll->hw);
> +	u64 vco;
> +
> +	refdiv = (setting & ZX29_PLL_REFDIV_MASK) >> ZX29_PLL_REFDIV_SHIFT;
> +	fbdiv = (setting & ZX29_PLL_FBDIV_MASK) >> ZX29_PLL_FBDIV_SHIFT;
> +	postdiv1 = (setting & ZX29_PLL_POSTDIV1_MASK) >> ZX29_PLL_POSTDIV1_SHIFT;
> +	postdiv2 = (setting & ZX29_PLL_POSTDIV2_MASK) >> ZX29_PLL_POSTDIV2_SHIFT;
> +	dev_dbg(pll->dev, "%s: reference clock %lu HZ, PLL setting 0x%08x\n",
> +		name, parent_rate, setting);
> +
> +	if (!refdiv || !postdiv1 || !postdiv2) {
> +		dev_err(pll->dev, "%s: divide by zero (%lu, %lu, %lu)\n", name, refdiv, postdiv1,
> +			postdiv2);

[Severity: Medium]
Could this dev_err() flood the kernel logs during frequency scaling or probe?

If a driver queries a clock rate while the parent clock is disabled
(parent_rate == 0), rational_best_approximation() can exit with refdiv = 0.

Since this get_rate() function is called repeatedly inside the 7x7 nested loop
in zx29_pll_calc_values():

zx29_pll_calc_values() {
...
			setting |= j << ZX29_PLL_POSTDIV2_SHIFT;
			out = zx29_pll_get_rate(pll, parent_rate, setting);
...
}

It seems this might print 49 error messages for a single rate calculation
attempt.

> +		return 0;
> +	}
[ ... ]
> +static int zx29_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	unsigned long new_rate;
> +	u32 setting;
> +
> +	/* TODO: DPLL can switch between two parents, one of which is another PLL. Take this into
> +	 * account when searching the config space and set req->best_parent_rate.
> +	 *
> +	 * In practise it shouldn't matter though. DPLL is always configured to a fixed frequency.
> +	 */
> +	setting = zx29_pll_calc_values(pll, req->best_parent_rate, req->rate);
> +	if (!setting)
> +		return -EINVAL;
> +
> +	new_rate = zx29_pll_get_rate(pll, req->best_parent_rate, setting);
> +	if (new_rate != req->rate) {
> +		dev_warn(pll->dev, "Did not find an exact match. Want %lu, got %lu\n",
> +			 req->rate, new_rate);

[Severity: Medium]
Is dev_warn() the appropriate log level here?

When a driver queries clk_round_rate() or clk_set_rate() with a target
frequency that cannot be perfectly matched by the dividers, returning a
rounded rate is the expected framework behavior.

Because .determine_rate() is queried heavily by the CCF, could this
unconditional warning cause significant log spam during normal operation?

> +		req->rate = new_rate;
> +	}
> +
> +	return 0;
> +}
> +
> +static int zx29_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +		      unsigned long parent_rate)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	int res = -EINVAL;
> +	u32 setting;
> +
> +	setting = zx29_pll_calc_values(pll, parent_rate, rate);
> +	if (zx29_pll_get_rate(pll, parent_rate, setting) == rate) {

[Severity: High]
Will this strict equality check reject valid configuration requests?

When CCF's determine_rate computes a new rate, the value can be slightly
truncated. If CCF passes this truncated rate to zx29_pll_set_rate(),
recalculating the VCO target inside zx29_pll_calc_values() with:

	u64 vco = (u64)rate * i * j;

might cause it to fall below ZX29_PLL_VCO_MIN_FREQ or yield a slightly
different fractional setting. Since the condition here demands strict equality,
is it possible that legitimate clock rate changes will fail with -EINVAL?

> +		res = regmap_update_bits(pll->map, pll->reg, 0x00ffffff, setting);

[Severity: High]
Is it safe to modify these hardware divisors while the PLL is actively feeding
downstream clocks?

This regmap_update_bits() modifies the reference and feedback dividers directly
without engaging a bypass mode or polling for the lock bit.

Additionally, the initialization structure in zx_clk_register_plls() doesn't
set the CLK_SET_RATE_GATE flag:

zx_clk_register_plls() {
...
		init.name = desc[i].name;
		init.ops = &zx29_pll_ops;
...
}

Could changing the dividers on an active PLL cause the VCO to slew
unpredictably and crash downstream hardware?

> +		dev_info(pll->dev, "%s: Setting rate: 0x%08x\n", clk_hw_get_name(hw), setting);
> +	}
> +
> +	return res;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-zx29clk-v6-0-377b704f80c4@gmail.com?part=7