Re: [PATCH 04/12] clk: qcom: ipq-cmn-pll: Add NSS clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk_nss_ppe_determine_rate violates the clock framework API contract when req->rate is 0.
- [Medium] Modifying the NSS clock divider resets the entire CMN PLL block, potentially disrupting other shared fixed-rate clocks.
--

--- Patch [4]: [PATCH 04/12] clk: qcom: ipq-cmn-pll: Add NSS clock support ---
commit b809b8bd5941993b064f7e68f395a77c3ea61452
Author: Luo Jie <jie.luo@oss.qualcomm.com>

clk: qcom: ipq-cmn-pll: Add NSS clock support

The NSS (network subsystem) clock is derived from the CMN PLL output
divided by 2 and then further divided by a configurable 6-bit divider
with a valid range of 8-63.

> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> index aa319d577871a..3df326c22fbac 100644
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c

[ ... ]

> @@ -364,11 +369,105 @@ static struct clk_hw *ipq_cmn_pll_clk_hw_register(struct platform_device *pdev)
>  	return &cmn_pll->hw;
>  }
>  
> +/*
> + * NSS (Network Subsystem) clock operations.
> + * The NSS clock is derived from CMN PLL rate / 2, then divided by
> + * a configurable 6-bit divider (8-63).
> + */

[ ... ]

> +static int clk_nss_ppe_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	unsigned long div;
> +
> +	if (WARN_ON_ONCE(!req->rate))
> +		return 0;

[Severity: Medium]
When req->rate is 0, this returns 0 (success) without updating req->rate to a
valid boundary or returning an error code like -EINVAL.

Can this regression violate the clock framework API contract by falsely
indicating to consumers that 0 Hz is a supported configuration?

> +
> +	div = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2ULL * req->rate);
> +	div = clamp_t(unsigned long, div, CMN_PLL_NSS_PPE_DIV_MIN,
> +		      CMN_PLL_NSS_PPE_DIV_MAX);
> +
> +	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2ULL * div);
> +	return 0;
> +}
> +
> +static int clk_nss_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{

[ ... ]

> +	ret = regmap_update_bits(nss_clk->regmap, CMN_PLL_NSS_PPE_FREQ_CTRL,
> +				 CMN_PLL_NSS_CLK_SEL,
> +				 FIELD_PREP(CMN_PLL_NSS_CLK_SEL, div));
> +	if (ret)
> +		return ret;
> +
> +	return clk_cmn_pll_ana_soft_reset(nss_clk->regmap);

[Severity: Medium]
Does updating the leaf NSS clock divider strictly require a hardware-level
analog reset of the parent PLL block?

Calling clk_cmn_pll_ana_soft_reset() drops the global PLL lock by toggling
CMN_ANA_EN_SW_RSTN:

clk_cmn_pll_ana_soft_reset() {
	...
	ret = regmap_clear_bits(regmap, CMN_PLL_POWER_ON_AND_RESET,
				CMN_ANA_EN_SW_RSTN);
	...
}

Will this regression interrupt other derived clocks (e.g., PCIe, WLAN,
SYS_CLK) and cause link drops or system instability whenever the NSS divider
is dynamically updated?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-qcom_ipq5210_cmnpll-v1-0-a8b8679d215f@oss.qualcomm.com?part=4