Re: [PATCH RFC 10/16] clk: qcom: clk-alpha-pll: Vote for the PLL calibration frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: dvfs: qcom_clk_dvfs_prepare_rate() unwinds to 0 on failure, dropping votes
- [High] clk: qcom: alpha-pll: disable failure during calibration drops DVFS votes while PLL runs
--

commit 27077f9ef026973ca3f8e723a45854a856554ed2
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Subject: clk: qcom: clk-alpha-pll: Vote for the PLL calibration frequency

This commit updates Lucid EVO PLL initialization to safely handle higher
calibration frequencies. It temporarily raises the DVFS vote to the
calibration rate during preparation and restores the nominal rate once
complete.

> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 5dc6b1adc9298..f6e73e2fbe768 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
[ ... ]
> @@ -2628,12 +2648,9 @@ static int _alpha_pll_lucid_evo_prepare(struct clk_hw *hw, bool reset)
>  		return -EINVAL;
>  
>  	ret = alpha_pll_lucid_evo_enable(hw);
> -	if (ret)
> -		return ret;
> -
> -	_alpha_pll_lucid_evo_disable(hw, reset);
> +	cleanup_ret = alpha_pll_lucid_evo_disable_checked(hw, reset);
>  
> -	return 0;
> +	return ret ?: cleanup_ret;
>  }

[Severity: High]
Is it safe to propagate the disable error here if it results in dropping the 
DVFS votes while the PLL is still running?

If alpha_pll_lucid_evo_enable() succeeds but alpha_pll_lucid_evo_disable_checked()
fails (e.g., due to a regmap write failure), the error is propagated up to
alpha_pll_lucid_evo_dvfs_prepare(). 

The error handler there unconditionally calls qcom_clk_dvfs_unprepare() via the
err_unprepare label, resetting all DVFS votes to 0. Because the hardware disable
failed, the PLL is left physically enabled and actively running without the
necessary voltage votes.

[ ... ]
> @@ -2641,21 +2658,69 @@ static void alpha_pll_lucid_evo_disable(struct clk_hw *hw)
>  	_alpha_pll_lucid_evo_disable(hw, false);
>  }
>  
> -static int alpha_pll_lucid_evo_prepare(struct clk_hw *hw)
> +/*
> + * Calibration runs at CAL_L * parent_rate, which may exceed the operating rate
> + * covered by the normal DVFS vote. Raise the vote temporarily to keep the
> + * hardware in-spec during that step.
> + */
> +static int alpha_pll_lucid_evo_dvfs_prepare(struct clk_hw *hw, bool reset)
>  {
[ ... ]
> +	ret = _alpha_pll_lucid_evo_prepare(hw, reset);
> +	if (ret)
> +		goto err_unprepare;
> +
> +	if (cal_rate > rate && qcom_clk_dvfs_prepare_rate(hw, rate))
> +		pr_warn("%s: failed to drop calibration DVFS vote\n",
> +			clk_hw_get_name(hw));
>  
> +	return 0;

[Severity: High]
Can ignoring the error from qcom_clk_dvfs_prepare_rate() here leave the unwound
power domains without voltage votes?

When down-voting from cal_rate to rate, if qcom_clk_dvfs_prepare_rate() fails
midway, its unwind path resets the successfully updated domains to 0:

drivers/clk/qcom/clk-dvfs.c:qcom_clk_dvfs_prepare_rate() {
	...
unwind:
	while (i--) {
		unsigned int pd_index = clock->data->pd_indices[i];

		qcom_clk_dvfs_replace_vote(&clock->rail_voters[pd_index],
					   &clock->votes[i], 0);
	}
	return ret;
}

Since the warning is caught but the error is deliberately ignored (returning 0),
the clock prepares and operates at the target rate but with missing DVFS votes
(vote = 0) on the unwound domains, which risks hardware brownouts or system
crashes.

> +
> +err_unprepare:
> +	qcom_clk_dvfs_unprepare(hw);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=10