Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1514176 > unrolled thread

Re: [PATCH v3 06/11] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 06/11] clk: qcom: Fix .set_rate to handle alpha PLLs  w/wo dynamic update Stephen Boyd <sboyd@codeaurora.org> - 2016-11-02 23:00 +0100
    Re: [PATCH v3 06/11] clk: qcom: Fix .set_rate to handle alpha PLLs  w/wo dynamic update Rajendra Nayak <rnayak@codeaurora.org> - 2016-11-03 09:30 +0100
      Re: [PATCH v3 06/11] clk: qcom: Fix .set_rate to handle alpha PLLs  w/wo dynamic update Stephen Boyd <sboyd@codeaurora.org> - 2016-11-03 20:50 +0100

#1514176 — Re: [PATCH v3 06/11] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

On 09/29, Rajendra Nayak wrote:
> Alpha PLLs which do not support dynamic update feature
> need to be explicitly disabled before a rate change.
> The ones which do support dynamic update do so within a
> single vco range, so add a min/max freq check for such
> PLLs so they fall in the vco range.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>

Is Taniya the author?

> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-alpha-pll.c | 49 +++++++++++++++++++++++++++++++++-------
>  drivers/clk/qcom/clk-alpha-pll.h |  3 +++
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 89c7fdb..6f90a86 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -382,16 +382,41 @@ clk_alpha_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>  static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  				  unsigned long prate)
>  {
> +	bool enabled;
>  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>  	const struct pll_vco *vco;
>  	u32 l, off = pll->offset;
>  	u64 a;
>  
>  	rate = alpha_pll_round_rate(rate, prate, &l, &a);
> -	vco = alpha_pll_find_vco(pll, rate);
> -	if (!vco) {
> -		pr_err("alpha pll not in a valid vco range\n");
> -		return -EINVAL;
> +	enabled = clk_hw_is_enabled(hw);
> +
> +	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
> +		/*
> +		 * PLLs which support dynamic updates support one single
> +		 * vco range, between min_rate and max_rate supported
> +		 */
> +		if (rate < pll->min_rate || rate > pll->max_rate) {
> +			pr_err("alpha pll rate outside supported min/max range\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		/*
> +		 * All alpha PLLs which do not support dynamic update,
> +		 * should be disabled before a vco update.
> +		 */
> +		if (enabled)
> +			hw->init->ops->disable(hw);

Please just call the function directly instead of going through
the init structure.

> +
> +		vco = alpha_pll_find_vco(pll, rate);
> +		if (!vco) {
> +			pr_err("alpha pll not in a valid vco range\n");
> +			return -EINVAL;
> +		}
> +
> +		regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
> +				   PLL_VCO_MASK << PLL_VCO_SHIFT,
> +				   vco->val << PLL_VCO_SHIFT);
>  	}
>  
>  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);
> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
> index d6e1ee2..e43a9c0 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.h
> +++ b/drivers/clk/qcom/clk-alpha-pll.h
> @@ -37,8 +37,11 @@ struct clk_alpha_pll {
>  #define SUPPORTS_OFFLINE_REQ	BIT(0)
>  #define SUPPORTS_16BIT_ALPHA	BIT(1)
>  #define SUPPORTS_FSM_MODE	BIT(2)
> +#define SUPPORTS_DYNAMIC_UPDATE	BIT(3)
>  	u8 flags;
>  
> +	unsigned long min_rate;
> +	unsigned long max_rate;

Document these?

>  	struct clk_regmap clkr;
>  };
>  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1514380

On 11/03/2016 03:24 AM, Stephen Boyd wrote:
> On 09/29, Rajendra Nayak wrote:
>> Alpha PLLs which do not support dynamic update feature
>> need to be explicitly disabled before a rate change.
>> The ones which do support dynamic update do so within a
>> single vco range, so add a min/max freq check for such
>> PLLs so they fall in the vco range.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> 
> Is Taniya the author?

ah, yes, I seem to have messed up the authorship in v3, will fix.

> 
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/clk/qcom/clk-alpha-pll.c | 49 +++++++++++++++++++++++++++++++++-------
>>  drivers/clk/qcom/clk-alpha-pll.h |  3 +++
>>  2 files changed, 44 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index 89c7fdb..6f90a86 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -382,16 +382,41 @@ clk_alpha_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>>  static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>  				  unsigned long prate)
>>  {
>> +	bool enabled;
>>  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>>  	const struct pll_vco *vco;
>>  	u32 l, off = pll->offset;
>>  	u64 a;
>>  
>>  	rate = alpha_pll_round_rate(rate, prate, &l, &a);
>> -	vco = alpha_pll_find_vco(pll, rate);
>> -	if (!vco) {
>> -		pr_err("alpha pll not in a valid vco range\n");
>> -		return -EINVAL;
>> +	enabled = clk_hw_is_enabled(hw);
>> +
>> +	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
>> +		/*
>> +		 * PLLs which support dynamic updates support one single
>> +		 * vco range, between min_rate and max_rate supported
>> +		 */
>> +		if (rate < pll->min_rate || rate > pll->max_rate) {
>> +			pr_err("alpha pll rate outside supported min/max range\n");
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		/*
>> +		 * All alpha PLLs which do not support dynamic update,
>> +		 * should be disabled before a vco update.
>> +		 */
>> +		if (enabled)
>> +			hw->init->ops->disable(hw);
> 
> Please just call the function directly instead of going through
> the init structure.

But we now have 2 different versions of disable based on clk_ops,
clk_alpha_pll_disable and clk_alpha_pll_hwfsm_disable.

> 
>> +
>> +		vco = alpha_pll_find_vco(pll, rate);
>> +		if (!vco) {
>> +			pr_err("alpha pll not in a valid vco range\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
>> +				   PLL_VCO_MASK << PLL_VCO_SHIFT,
>> +				   vco->val << PLL_VCO_SHIFT);
>>  	}
>>  
>>  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
>> index d6e1ee2..e43a9c0 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.h
>> +++ b/drivers/clk/qcom/clk-alpha-pll.h
>> @@ -37,8 +37,11 @@ struct clk_alpha_pll {
>>  #define SUPPORTS_OFFLINE_REQ	BIT(0)
>>  #define SUPPORTS_16BIT_ALPHA	BIT(1)
>>  #define SUPPORTS_FSM_MODE	BIT(2)
>> +#define SUPPORTS_DYNAMIC_UPDATE	BIT(3)
>>  	u8 flags;
>>  
>> +	unsigned long min_rate;
>> +	unsigned long max_rate;
> 
> Document these?

will do, thanks.

> 
>>  	struct clk_regmap clkr;
>>  };
>>  
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

#1514804

On 11/03, Rajendra Nayak wrote:
> 
> 
> On 11/03/2016 03:24 AM, Stephen Boyd wrote:
> > On 09/29, Rajendra Nayak wrote:
> >> Alpha PLLs which do not support dynamic update feature
> >> need to be explicitly disabled before a rate change.
> >> The ones which do support dynamic update do so within a
> >> single vco range, so add a min/max freq check for such
> >> PLLs so they fall in the vco range.
> >>
> >> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > 
> > Is Taniya the author?
> 
> ah, yes, I seem to have messed up the authorship in v3, will fix.
> 
> > 
> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> >> ---
> >>  drivers/clk/qcom/clk-alpha-pll.c | 49 +++++++++++++++++++++++++++++++++-------
> >>  drivers/clk/qcom/clk-alpha-pll.h |  3 +++
> >>  2 files changed, 44 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> >> index 89c7fdb..6f90a86 100644
> >> --- a/drivers/clk/qcom/clk-alpha-pll.c
> >> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> >> @@ -382,16 +382,41 @@ clk_alpha_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> >>  static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> >>  				  unsigned long prate)
> >>  {
> >> +	bool enabled;
> >>  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> >>  	const struct pll_vco *vco;
> >>  	u32 l, off = pll->offset;
> >>  	u64 a;
> >>  
> >>  	rate = alpha_pll_round_rate(rate, prate, &l, &a);
> >> -	vco = alpha_pll_find_vco(pll, rate);
> >> -	if (!vco) {
> >> -		pr_err("alpha pll not in a valid vco range\n");
> >> -		return -EINVAL;
> >> +	enabled = clk_hw_is_enabled(hw);
> >> +
> >> +	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
> >> +		/*
> >> +		 * PLLs which support dynamic updates support one single
> >> +		 * vco range, between min_rate and max_rate supported
> >> +		 */
> >> +		if (rate < pll->min_rate || rate > pll->max_rate) {
> >> +			pr_err("alpha pll rate outside supported min/max range\n");
> >> +			return -EINVAL;
> >> +		}
> >> +	} else {
> >> +		/*
> >> +		 * All alpha PLLs which do not support dynamic update,
> >> +		 * should be disabled before a vco update.
> >> +		 */
> >> +		if (enabled)
> >> +			hw->init->ops->disable(hw);
> > 
> > Please just call the function directly instead of going through
> > the init structure.
> 
> But we now have 2 different versions of disable based on clk_ops,
> clk_alpha_pll_disable and clk_alpha_pll_hwfsm_disable.
> 

So have two set_rate ops that call a common function that takes a
function pointer pair of enable/disable ops to call? Or have a
flag to know which one to call? Either way, don't use the init
structure after registration time please.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web