Re: [PATCH v8 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] zx29_pll_determine_rate ignores the req->min_rate and req->max_rate constraints provided by the clock framework, potentially violating bounds.
--

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

clk: zte: Add zx PLL support infrastructure

This commit introduces the ZX PLL support infrastructure for ZTE platforms,
adding rate calculation, prepare/enable operations, and registration logic.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index f8ddb4c7dff66..6e3a20cb5839a 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)
> +{
[ ... ]
> +			setting = fbdiv << ZX29_PLL_FBDIV_SHIFT;
> +			setting |= refdiv << ZX29_PLL_REFDIV_SHIFT;
> +			setting |= i << ZX29_PLL_POSTDIV1_SHIFT;
> +			setting |= j << ZX29_PLL_POSTDIV2_SHIFT;
> +			out = zx29_pll_get_rate(pll, parent_rate, setting);
> +
> +			if (abs_diff(out, rate) > best)
> +				continue;
> +
> +			if (abs_diff(out, rate) < best) {
> +				postdiv1 = i;
> +				postdiv2 = j;
> +				best_fbdiv = fbdiv;

[Severity: Medium]
Since this loop unconditionally picks the closest hardware frequency to the
requested rate, is it possible that it selects a frequency outside of the CCF
boundaries? 

If a driver sets strict constraints via clk_set_rate_range(), and the nearest
PLL approximation falls outside that range while a valid alternative exists
inside it, clk_calc_new_rates() might reject the out-of-bounds 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;
> +
> +	/* 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 practice it shouldn't matter though. Dpll is always configured to a fixed frequency
> +	 * and is the only clock with a switchable parent.
> +	 */
> +	if (!req->best_parent_rate) {
> +		dev_err(pll->dev, "Did not expect best_parent_rate=0\n");
> +		return -EINVAL;
> +	}
> +
> +	setting = zx29_pll_calc_values(pll, req->best_parent_rate, req->rate);

[Severity: Medium]
Does this code need to consider req->min_rate and req->max_rate before
calculating the new rate? The minimum and maximum boundaries passed by the
clock framework to restrict the search appear to be ignored here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7