Re: [PATCH v4 07/12] clk: qcom: ipq-cmn-pll: Add PON reference clock support

[Jie Luo <jie.luo@oss.qualcomm.com>]

On 9/6/2026 10:25 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] clk_pon_clk_determine_rate() incorrectly fails valid rate requests due to rounding at bound edges.
> --
> 
> commit 9a9a4823a8ebca7e3c30590799eb2ac41ada876e
> Author: Luo Jie <jie.luo@oss.qualcomm.com>
> 
> clk: qcom: ipq-cmn-pll: Add PON reference clock support
> 
> This commit adds support for the PON reference clock to the IPQ CMN PLL driver.
> It configures the clock to operate in either UNIPHY mode with a fixed 31.25 MHz
> rate, or PON mode using an 8-bit divider derived from the parent rate.
> 
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> index 44d035870a89b..f12352a707f7a 100644
>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> [ ... ]
>> +static int clk_pon_clk_determine_rate(struct clk_hw *hw,
>> +				      struct clk_rate_request *req)
>> +{
>> +	unsigned long div, pon_rate, uniphy_rate = 31250000UL;
>> +	bool uniphy_rate_valid, pon_rate_valid;
>> +
>> +	if (!req->rate || !req->best_parent_rate)
>> +		return -EINVAL;
>> +
>> +	div = DIV64_U64_ROUND_CLOSEST((u64)req->best_parent_rate, req->rate);
>> +
>> +	/* Clamp to valid range (1-255) */
>> +	div = clamp_t(unsigned long, div, 1, 255);
>> +
>> +	pon_rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, div);
>> +
>> +	uniphy_rate_valid = uniphy_rate >= req->min_rate &&
>> +			    uniphy_rate <= req->max_rate;
>> +	pon_rate_valid = pon_rate >= req->min_rate &&
>> +			 pon_rate <= req->max_rate;
>> +
>> +	if (!uniphy_rate_valid && !pon_rate_valid)
>> +		return -EINVAL;
> 
> [Severity: High]
> Will this logic incorrectly reject valid rate requests when the closest
> divider falls slightly outside the boundaries due to rounding?
> 
> If a client restricts the bounds (e.g., via clk_set_min_rate), the requested
> rate is clamped to the bounds. If calculating the closest divider for that
> rate yields a frequency that barely misses the min_rate or max_rate bound, 
> pon_rate_valid becomes false and this returns -EINVAL.
> 
> Should this code check adjacent dividers (such as div - 1 or div + 1) to see
> if they yield a rate strictly within the requested bounds before giving up?
> 

It doesn't hold up for this driver, a PON reference clock feeds the
UNIPHY/PON that needs one exact standard frequency (e.g. the 31.25 MHz
UNIPHY rate, or whatever exact PON-mode rate the line rate dictates),
not "anything within a window." A real consumer would call
clk_set_rate(clk, exact_rate) and expect either that exact rate back or
-EINVAL, never clk_set_rate_range().

>> +
>> +	/* Pick whichever mode gets closer to the requested rate */
>> +	if (uniphy_rate_valid && pon_rate_valid) {
> [ ... ]
>