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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: zte: set_rate rejects valid rates due to strict VCO bounds check
--

commit 03d27f14b3d2dbba9c34e624628df45495020947
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Add zx PLL support infrastructure

This patch adds code to decode and program PLL settings on ZTE's
zx297520v3 SoC.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index fc76c6524a163..d3687495d06ce 100644
> --- a/drivers/clk/zte/pll-zx.c
> +++ b/drivers/clk/zte/pll-zx.c
[ ... ]
> +static u32 zx29_pll_calc_values(const struct zx29_clk_pll *pll, unsigned long parent_rate,
> +				unsigned long rate, unsigned long min_rate, unsigned long max_rate)
> +{
[ ... ]
> +	for (j = 1; j <= postdiv2_max; j++) {
> +		for (i = 1; i <= postdiv1_max; i++) {
> +			u64 vco = (u64)rate * i * j;
> +			unsigned long out;
> +
> +			if (vco > ZX29_PLL_VCO_MAX_FREQ || vco < ZX29_PLL_VCO_MIN_FREQ)
> +				continue;

[Severity: Medium]
Will this strict bounds check reject valid rates during zx29_pll_set_rate()?

If the requested target rate maps to a VCO frequency at the very bottom edge of
the valid bounds (e.g. exactly 900 MHz), the rational approximation for the
dividers could produce an actual VCO frequency just below 900 MHz.

When zx29_pll_determine_rate() returns this slightly truncated output rate, it
is then passed to zx29_pll_set_rate() which passes it back to this function.

At that point, the recalculated VCO will fall slightly below
ZX29_PLL_VCO_MIN_FREQ and be strictly rejected.

[ ... ]
> +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);
> +	u32 setting;
> +
> +	/*
> +	 * TODO: Implement gradual PLL rate change. PLLs can be changed while they are running and
> +	 * downstream hardware is generally fine with that. The exception is DRAM, which reads
> +	 * incorrect values if changed too fast.
> +	 *
> +	 * Changing the mpll rate is potentially useful for over/underclocking. Gating mpll is
> +	 * unrealistic because too many devices depend on it.
> +	 */
> +	setting = zx29_pll_calc_values(pll, parent_rate, rate, rate, rate);

[Severity: Medium]
Since zx29_pll_calc_values() can fail due to the strict VCO bounds check, does
this cause zx29_pll_set_rate() to unexpectedly return -EINVAL for a rate that
was just validated by zx29_pll_determine_rate()?

This breaks the Common Clock Framework (CCF) contract, which expects set_rate()
to succeed when passed a rate validated by determine_rate(). Could this cause
regressions through unexpected driver probe failures?

> +	if (!setting)
> +		return -EINVAL;
> +
> +	dev_dbg(pll->dev, "%s: Setting new configuration: 0x%08x\n", clk_hw_get_name(hw), setting);
> +
> +	return regmap_update_bits(pll->map, pll->reg, 0x00ffffff, setting);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-zx29clk-v12-0-9c243796b6d2@gmail.com?part=7