Re: [PATCH v7 08/13] clk: zte: Add zx PLL support infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] zx29_pll_calc_values fails to clamp out-of-bounds frequencies, causing rate determination to abort instead of returning the closest achievable rate.
- [High] .determine_rate does not enforce req->min_rate and req->max_rate boundaries.
- [Medium] Using `dev_info` inside `zx29_pll_set_rate` will spam the kernel log.
--

--- Patch [8]: [PATCH v7 08/13] clk: zte: Add zx PLL support infrastructure ---
commit c3e5de791970b113941346866ebf54af4b186d7d
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Add zx PLL support infrastructure

This commit adds PLL support infrastructure for ZTE zx chips, specifically 
tested with the zx297520v3. It provides the standard clock operations 
including rate calculation, parent selection, and enabling/disabling 
of the PLLs.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index f8ddb4c7dff66..701e580609f2b 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)
> +{

[ ... ]

> +	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: High]
Does this logic fail to clamp out-of-bounds frequencies?

If a consumer requests a clock rate where the maximum evaluated VCO 
(e.g., 10 MHz * 7 * 7 = 490 MHz) is less than ZX29_PLL_VCO_MIN_FREQ, 
this loop will just continue and return 0.

Should the requested rate be clamped to the physical minimum and maximum 
achievable bounds before computing the VCO to ensure it returns the closest 
supported rate?

[ ... ]

> +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;

[ ... ]

> +	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);
> +		req->rate = new_rate;

[Severity: High]
Does this assignment violate the caller's min/max rate constraints?

If the closest hardware rate falls outside the req->min_rate and req->max_rate 
boundaries, assigning it back to req->rate could cause the clock framework to 
reject the rate change entirely, even if another slightly less optimal but 
in-bounds rate was available.

> +	}
> +
> +	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;
> +
> +	/* PLLs can be changed while they are running and in testing the downstream hardware
> +	 * handles it gracefully - as long as the new rate is a rate the hardware can operate at.
> +	 */
> +	setting = zx29_pll_calc_values(pll, parent_rate, rate);
> +	if (!setting)
> +		return -EINVAL;
> +
> +	if (zx29_pll_get_rate(pll, parent_rate, setting) == rate) {
> +		res = regmap_update_bits(pll->map, pll->reg, 0x00ffffff, setting);
> +		dev_info(pll->dev, "%s: Setting rate: 0x%08x\n", clk_hw_get_name(hw), setting);

[Severity: Medium]
Could this dev_info call cause log spam during dynamic frequency scaling?

If this PLL is used for scaling operations where .set_rate is called 
hundreds of times per second, this will rapidly flood the kernel log.
Would dev_dbg be more appropriate for this hot path?

> +	}
> +
> +	return res;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-zx29clk-v7-0-408411cfcf36@gmail.com?part=8