Message-ID: <fce2fe9e-5c1b-4fa9-b6ca-0a4ab5133e5a@quicinc.com>
Date: Mon, 17 Mar 2025 12:10:13 +0530
From: Taniya Das <quic_tdas@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd
	<sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Catalin Marinas
	<catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Ajit Pandey
	<quic_ajipan@...cinc.com>,
        Imran Shaik <quic_imrashai@...cinc.com>,
        "Jagadeesh Kona" <quic_jkona@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v6 01/10] clk: qcom: clk-alpha-pll: Add support for
 dynamic update for slewing PLLs



On 3/14/2025 5:09 AM, Bjorn Andersson wrote:
> On Thu, Mar 13, 2025 at 12:29:38PM +0530, Taniya Das wrote:
>> The alpha PLLs which slew to a new frequency at runtime would require
>> the PLL to calibrate at the mid point of the VCO. Add the new PLL ops
>> which can support the slewing of the PLL to a new frequency.
>>
>> Reviewed-by: Imran Shaik <quic_imrashai@...cinc.com>
>> Signed-off-by: Taniya Das <quic_tdas@...cinc.com>
>> ---
>>  drivers/clk/qcom/clk-alpha-pll.c | 170 +++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/qcom/clk-alpha-pll.h |   1 +
>>  2 files changed, 171 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index cec0afea8e446010f0d4140d4ef63121706dde47..7d784db8b7441e886d94ded1d3e3258dda46674c 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -2960,3 +2960,173 @@ const struct clk_ops clk_alpha_pll_regera_ops = {
>>  	.set_rate = clk_zonda_pll_set_rate,
>>  };
>>  EXPORT_SYMBOL_GPL(clk_alpha_pll_regera_ops);
>> +
>> +static int clk_alpha_pll_slew_update(struct clk_alpha_pll *pll)
>> +{
>> +	int ret;
>> +	u32 val;
>> +
>> +	regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_UPDATE, PLL_UPDATE);
>> +	regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
>> +
>> +	ret = wait_for_pll_update(pll);
>> +	if (ret)
>> +		return ret;
>> +	/*
>> +	 * Hardware programming mandates a wait of at least 570ns before polling the LOCK
>> +	 * detect bit. Have a delay of 1us just to be safe.
>> +	 */
>> +	mb();
>> +	udelay(1);
>> +
>> +	return wait_for_pll_enable_lock(pll);
>> +}
>> +
>> +static int clk_alpha_pll_slew_set_rate(struct clk_hw *hw, unsigned long rate,
>> +					unsigned long parent_rate)
>> +{
>> +	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> +	unsigned long freq_hz;
>> +	const struct pll_vco *curr_vco, *vco;
>> +	u32 l, alpha_width = pll_alpha_width(pll);
>> +	u64 a;
>> +
>> +	freq_hz =  alpha_pll_round_rate(rate, parent_rate, &l, &a, alpha_width);
> 
> Double space here.

Sure, needs a fix.

> 
>> +	if (freq_hz != rate) {
>> +		pr_err("alpha_pll: Call clk_set_rate with rounded rates!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	curr_vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw));
>> +	if (!curr_vco) {
>> +		pr_err("alpha pll: not in a valid vco range\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	vco = alpha_pll_find_vco(pll, freq_hz);
>> +	if (!vco) {
>> +		pr_err("alpha pll: not in a valid vco range\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Dynamic pll update will not support switching frequencies across
>> +	 * vco ranges. In those cases fall back to normal alpha set rate.
>> +	 */
>> +	if (curr_vco->val != vco->val)
>> +		return clk_alpha_pll_set_rate(hw, rate, parent_rate);
>> +
>> +	a = a << (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
> 
> Above this function is written to deal with both alpha bitwidths, but
> here it's assumed to only be one of the cases.
> 
> It would be nice to get this cleaned up somehow, because we now have
> this shift 6 times in slightly different forms.
> 

I will check if I can clean up.

>> +
>> +	regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), a >> 32);
> 
> In a number of places in the driver alpha_width is compared to 32 bits
> to see if this should be written or not. Perhaps that's not applicable
> here, but again, if so then why is it dynamic above?
> 
> 
> Also, how about upper_32_bits() and lower_32_bits() to make it clear
> what's going on here?
> 

Sure.


>> +
>> +	/* Ensure that the write above goes through before proceeding. */
> 
> That's not what mb() does.
> 
> Regards,
> Bjorn
> 
>> +	mb();
>> +
>> +	if (clk_hw_is_enabled(hw))
>> +		return clk_alpha_pll_slew_update(pll);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Slewing plls should be bought up at frequency which is in the middle of the
>> + * desired VCO range. So after bringing up the pll at calibration freq, set it
>> + * back to desired frequency(that was set by previous clk_set_rate).
>> + */
>> +static int clk_alpha_pll_calibrate(struct clk_hw *hw)
>> +{
>> +	unsigned long calibration_freq, freq_hz;
>> +	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> +	struct clk_hw *parent;
>> +	const struct pll_vco *vco;
>> +	u32 l, alpha_width = pll_alpha_width(pll);
>> +	int rc;
>> +	u64 a;
>> +
>> +	parent = clk_hw_get_parent(hw);
>> +	if (!parent) {
>> +		pr_err("alpha pll: no valid parent found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw));
>> +	if (!vco) {
>> +		pr_err("alpha pll: not in a valid vco range\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * As during slewing plls vco_sel won't be allowed to change, vco table
>> +	 * should have only one entry table, i.e. index = 0, find the
>> +	 * calibration frequency.
>> +	 */
>> +	calibration_freq = (pll->vco_table[0].min_freq + pll->vco_table[0].max_freq) / 2;
>> +
>> +	freq_hz = alpha_pll_round_rate(calibration_freq, clk_hw_get_rate(parent),
>> +					&l, &a, alpha_width);
>> +	if (freq_hz != calibration_freq) {
>> +		pr_err("alpha_pll: call clk_set_rate with rounded rates!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Setup PLL for calibration frequency */
>> +	a <<= (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
>> +
>> +	regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), a >> 32);
>> +
>> +	regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_VCO_MASK << PLL_VCO_SHIFT,
>> +				vco->val << PLL_VCO_SHIFT);
>> +
>> +	/* Bringup the pll at calibration frequency */
>> +	rc = clk_alpha_pll_enable(hw);
>> +	if (rc) {
>> +		pr_err("alpha pll calibration failed\n");
>> +		return rc;
>> +	}
>> +
>> +	/*
>> +	 * PLL is already running at calibration frequency.
>> +	 * So slew pll to the previously set frequency.
>> +	 */
>> +	freq_hz = alpha_pll_round_rate(clk_hw_get_rate(hw),
>> +			clk_hw_get_rate(parent), &l, &a, alpha_width);
>> +
>> +	pr_debug("pll %s: setting back to required rate %lu, freq_hz %ld\n",
>> +		clk_hw_get_name(hw), clk_hw_get_rate(hw), freq_hz);
>> +
>> +	/* Setup the PLL for the new frequency */
>> +	a <<= (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
>> +
>> +	regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a);
>> +	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), a >> 32);
>> +
>> +	regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_ALPHA_EN, PLL_ALPHA_EN);
>> +
>> +	return clk_alpha_pll_slew_update(pll);
>> +}
>> +
>> +static int clk_alpha_pll_slew_enable(struct clk_hw *hw)
>> +{
>> +	int rc;
>> +
>> +	rc = clk_alpha_pll_calibrate(hw);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return clk_alpha_pll_enable(hw);
>> +}
>> +
>> +const struct clk_ops clk_alpha_pll_slew_ops = {
>> +	.enable = clk_alpha_pll_slew_enable,
>> +	.disable = clk_alpha_pll_disable,
>> +	.recalc_rate = clk_alpha_pll_recalc_rate,
>> +	.round_rate = clk_alpha_pll_round_rate,
>> +	.set_rate = clk_alpha_pll_slew_set_rate,
>> +};
>> +EXPORT_SYMBOL(clk_alpha_pll_slew_ops);
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
>> index 79aca8525262211ae5295245427d4540abf1e09a..1d19001605eb10fd8ae8041c56d951e928cbbe9f 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.h
>> +++ b/drivers/clk/qcom/clk-alpha-pll.h
>> @@ -204,6 +204,7 @@ extern const struct clk_ops clk_alpha_pll_rivian_evo_ops;
>>  #define clk_alpha_pll_postdiv_rivian_evo_ops clk_alpha_pll_postdiv_fabia_ops
>>  
>>  extern const struct clk_ops clk_alpha_pll_regera_ops;
>> +extern const struct clk_ops clk_alpha_pll_slew_ops;
>>  
>>  void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>  			     const struct alpha_pll_config *config);
>>
>> -- 
>> 2.48.1
>>

Powered by blists - more mailing lists