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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Due to integer division truncation, valid PLL configurations are falsely rejected during clk_set_rate(), causing frequency changes to fail when the calculated VCO falls slightly below the minimum limit.
--

--- Patch [7]: [PATCH v10 07/12] clk: zte: Add zx PLL support infrastructure ---
commit 02ad18f21bbd9b41b5dbd910425b934e48d8ecd9
Author: Stefan Dösinger <[email protected]>

clk: zte: Add zx PLL support infrastructure

This commit adds PLL support infrastructure for ZTE ZX-series platforms.
It implements the standard clock API operations including rate calculation,
parent selection, and configuration of fractional and post dividers.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index fc76c6524a163..e8d57dd1386f1 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]
Can this strict lower bound check reject valid configurations? 

If a frequency is requested where the chosen dividers produce a VCO at or
just above the 900 MHz minimum, integer division truncation might cause the
recalculated vco to fall slightly below ZX29_PLL_VCO_MIN_FREQ, falsely
rejecting the configuration.

[ ... ]

> +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);
> +	if (!setting)
> +		return -EINVAL;

[Severity: Medium]
Will zx29_pll_set_rate() unexpectedly fail here for valid rates it previously
advertised via zx29_pll_determine_rate()?

If the requested rate was slightly truncated due to integer division, passing
it back into zx29_pll_calc_values() with strict exact-match bounds where
min_rate and max_rate equal the truncated rate might trigger the minimum VCO
limit check shown above. This would result in returning -EINVAL and breaking
the frequency change.

> +
> +	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/[email protected]?part=7

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy