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

On 7/24/2026 11:15 PM, [email protected] wrote:
> 
>> +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?
> 

"-EINVAL" should be returned in this case to properly reject the invalid
rate request.

>> +
>> +	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?
> 
>> +}

I will remove clk_cmn_pll_ana_soft_reset() from all output clock paths,
as it is unnecessary and has been confirmed by the HW team to adversely
affect other derived clocks.

>