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

On 7/24/2026 11:11 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] 64-bit divisor passed to DIV_ROUND_CLOSEST_ULL causes 32-bit truncation and potential divide-by-zero panics.
> - [Medium] Non-atomic register updates when switching to PON mode can cause transient invalid clock states.
> - [Medium] `clk_pon_refclk_determine_rate()` returns 0 on invalid rate requests instead of an error code.
> - [Medium] Missing PLL analog soft reset when entering UNIPHY fixed mode.
> - [Medium] UNIPHY mode is incorrectly excluded from rate rounding for near-match requests.
> - [Medium] Missing error check on regmap_read in clk_pon_refclk_recalc_rate leads to use of uninitialized variable.
> --
> 
> --- Patch [6]: [PATCH 06/12] clk: qcom: ipq-cmn-pll: Add PON reference clock support ---
> commit 66c5c5bce6266424eec1b5f6855bb022b40a0322
> Author: Luo Jie <[email protected]>
> 
> 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.
> The clock supports UNIPHY and PON modes, with configurable dividers.
> 
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> index 020ad12ca1f2b..1ae32052b5634 100644
>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> 
> [ ... ]
> 
>> @@ -540,6 +545,155 @@ static struct clk_hw *ipq_cmn_pll_ppe_register(struct platform_device *pdev,
>>  	return &ppe_clk->hw;
>>  }
>>  
>> +/*
>> + * PON (Passive Optical Network) reference clock operations.
>> + * The PON refclk is derived from CMN PLL rate / 2, then divided by
>> + * a configurable 8-bit divider (1-255).
>> + */
> 
> [ ... ]
> 
>> +static unsigned long clk_pon_refclk_recalc_rate(struct clk_hw *hw,
>> +						unsigned long parent_rate)
>> +{
>> +	struct clk_cmn_pll *pon_clk = to_clk_cmn_pll(hw);
>> +	u32 val, div;
>> +
>> +	regmap_read(pon_clk->regmap, CMN_PLL_PON_CONFIG, &val);
>> +
>> +	/* Check if in UNIPHY mode (bit 9 = 0) - fixed 31.25 MHz */
>> +	if (!(val & CMN_PLL_PON_MODE_SEL))
> 
> [Severity: Medium]
> Could this use uninitialized memory if the underlying MMIO read fails?
> 
> The return value of regmap_read() is a standard negative error code on failure.
> Since it is not checked, val may be left uninitialized if the read fails,
> causing garbage clock rates to be returned to the framework.
> 

Good catch. Will check ret in the next version.

>> +		return 31250000UL;
>> +
>> +	/* PON mode: calculate from divider */
>> +	div = FIELD_GET(CMN_PLL_PON_DIV_CTRL, val);
>> +	if (WARN_ON_ONCE(!div))
>> +		return 0;
>> +
>> +	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2ULL * div);
>> +}
>> +
>> +static int clk_pon_refclk_determine_rate(struct clk_hw *hw,
>> +					 struct clk_rate_request *req)
>> +{
>> +	unsigned long div;
>> +
>> +	if (WARN_ON_ONCE(!req->rate))
>> +		return 0;
> 
> [Severity: Medium]
> Should this return a negative error code (like -EINVAL) instead of 0?
> 
> Returning 0 tells the Common Clock Framework that the rate request was
> successfully determined and adapted, which will cause it to proceed to call
> set_rate() with rate = 0.
> 

Will change this to return -EINVAL.

>> +
>> +	/* UNIPHY fixed mode */
>> +	if (req->rate == 31250000UL)
>> +		return 0;
> 
> [Severity: Medium]
> Does this exact match prevent near-match rounding to UNIPHY mode?
> 
> If a consumer requests 31249999 Hz, it seems it would bypass this exact match
> and fall back to calculating a PON mode divider. This might yield a worse
> rate, even though the hardware inherently supports exactly 31.25 MHz.

Good catch — Will update clk_pon_refclk_determine_rate() to compute both
the fixed UNIPHY-mode rate (31.25 MHz) and the closest achievable PON-
mode rate for the requested divider, then selects whichever one is
closer to the requested rate.


> 
>> +
>> +	div = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2ULL * req->rate);
> 
> [Severity: High]
> Can this cause a divide-by-zero panic on 32-bit platforms?
> 
> DIV_ROUND_CLOSEST_ULL() internally uses do_div(), which fundamentally
> requires a 32-bit divisor. When the requested rate is large enough,
> 2ULL * req->rate can exceed 32 bits. On 32-bit platforms, do_div() silently
> truncates the 64-bit divisor to 32 bits, which could result in a divisor of 0.
> 

Will switch to use DIV64_U64_ROUND_CLOSEST() instead.

>> +
>> +	/* Clamp to valid range (1-255) */
>> +	div = clamp_t(unsigned long, div, 1, 255);
>> +
>> +	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2ULL * div);
>> +	return 0;
>> +}
>> +
>> +static int clk_pon_refclk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				   unsigned long parent_rate)
>> +{
>> +	struct clk_cmn_pll *pon_clk = to_clk_cmn_pll(hw);
>> +	unsigned long div;
>> +	int ret;
>> +
>> +	if (rate == 0)
>> +		return -EINVAL;
>> +
>> +	/* UNIPHY fixed mode (31.25 MHz) */
>> +	if (rate == 31250000UL)
>> +		return regmap_clear_bits(pon_clk->regmap, CMN_PLL_PON_CONFIG,
>> +					 CMN_PLL_PON_MODE_SEL);
> 
> [Severity: Medium]
> Is it intentional to bypass the analog soft reset here?
> 
> The soft reset is performed at the end of the function for PON mode. If
> entering UNIPHY mode also requires a soft reset to reliably latch or
> stabilize the new clock output, the hardware might be left in an
> inconsistent state.
> 

This is intentional — the analog soft reset will be removed from all
derived clock output paths entirely, not just for UNIPHY mode. Keeping
it risks disturbing other already-stable clocks that share the same PLL.

>> +
>> +	div = DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2ULL * rate);
> 
> [Severity: High]
> Can this cause a divide-by-zero panic on 32-bit platforms?
> 
> Similar to the determine_rate callback, the 64-bit expression 2ULL * rate
> is passed to a macro that expects a 32-bit divisor.
>

>> +	if (div == 0 || div > 255)
>> +		return -EINVAL;
>> +
>> +	/* Switch to PON mode */
>> +	ret = regmap_set_bits(pon_clk->regmap, CMN_PLL_PON_CONFIG,
>> +			      CMN_PLL_PON_MODE_SEL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(pon_clk->regmap, CMN_PLL_PON_CONFIG,
>> +				 CMN_PLL_PON_DIV_CTRL,
>> +				 FIELD_PREP(CMN_PLL_PON_DIV_CTRL, div));
> 
> [Severity: Medium]
> Can this sequential update cause transient invalid clock states?
> 
> There is a brief window where the mode bit is set to PON mode, but the
> divider has not yet been updated. If the previous divider was 0 or
> inappropriate, it might expose downstream hardware to an invalid
> frequency or glitch.
> 

This is intentional and acceptable by design — the clock rate is
configured before the connected PON/UNIPHY is brought out of reset and
reconfigured.

>> +	if (ret)
>> +		return ret;
>> +
>> +	return clk_cmn_pll_ana_soft_reset(pon_clk->regmap);
>> +}
>