diff mbox series

[v1,1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

Message ID 1572524473-19344-2-git-send-email-tdas@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Add GPU & Video Clock controller driver for SC7180 | expand

Commit Message

Taniya Das Oct. 31, 2019, 12:21 p.m. UTC 
In the cases where the PLL is not calibrated the PLL could fail to lock.
Add support for prepare ops which would take care of the same.

Fabia PLL user/test control registers might required to be configured, so
add support for configuring them.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 84 +++++++++++++++++++++++++++++++++++++---
 drivers/clk/qcom/clk-alpha-pll.h |  4 ++
 2 files changed, 83 insertions(+), 5 deletions(-)

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Comments

Stephen Boyd Nov. 6, 2019, 12:36 a.m. UTC | #1 
Quoting Taniya Das (2019-10-31 05:21:07)
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 055318f..8cb77ca 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>                                                 unsigned long prate)
>  {
>         struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> -       u32 val, l, alpha_width = pll_alpha_width(pll);
> +       u32 l, alpha_width = pll_alpha_width(pll);
>         u64 a;
>         unsigned long rrate;
>         int ret = 0;
> 
> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
> -       if (ret)
> -               return ret;
> -
>         rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);
> 
>         /*

How is this diff related? Looks like it should be split off into another
patch to remove a useless register read.

> @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>         return __clk_alpha_pll_update_latch(pll);
>  }
> 
> +static int alpha_pll_fabia_prepare(struct clk_hw *hw)
> +{

Why are we doing this in prepare vs. doing it at PLL configuration time?
Does it need to be recalibrated each time the PLL is enabled?

> +       struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> +       const struct pll_vco *vco;
> +       struct clk_hw *parent_hw;
> +       unsigned long cal_freq, rrate;
> +       u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
> +       u64 a;
> +       int ret;
> +
> +       /* Check if calibration needs to be done i.e. PLL is in reset */
> +       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);

Please use 'val' instead of 'regval' as regval almost never appears in
this file already.

> +       if (ret)
> +               return ret;
> +
> +       /* Return early if calibration is not needed. */
> +       if (regval & PLL_RESET_N)
> +               return 0;
> +
> +       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;
> +       }
> +
> +       cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
> +                               pll->vco_table[0].max_freq) * 54, 100);

Do we need to cast the first argument to a u64 to avoid overflow?

> +
> +       parent_hw = clk_hw_get_parent(hw);
> +       if (!parent_hw)
> +               return -EINVAL;
> +
> +       rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
> +                                       &cal_l, &a, alpha_width);
> +       /*
> +        * Due to a limited number of bits for fractional rate programming, the
> +        * rounded up rate could be marginally higher than the requested rate.
> +        */
> +       if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
> +               pr_err("Call set rate on the PLL with rounded rates!\n");

This message is weird. Drivers shouldn't need to call set rate with
rounded rates. What is going on?

> +               return -EINVAL;
> +       }
> +
> +       /* Setup PLL for calibration frequency */
> +       regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
> +
> +       /* Bringup the pll at calibration frequency */

capitalize PLL.

> +       ret = clk_alpha_pll_enable(hw);
> +       if (ret) {
> +               pr_err("alpha pll calibration failed\n");
> +               return ret;
> +       }
> +
> +       clk_alpha_pll_disable(hw);
> +
> +       return 0;
> +}
> +
Doug Anderson Nov. 7, 2019, 4:24 a.m. UTC | #2 
Hi,

On Thu, Oct 31, 2019 at 5:21 AM Taniya Das <tdas@codeaurora.org> wrote:
>
> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>                                                 unsigned long prate)
>  {
>         struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> -       u32 val, l, alpha_width = pll_alpha_width(pll);
> +       u32 l, alpha_width = pll_alpha_width(pll);
>         u64 a;
>         unsigned long rrate;
>         int ret = 0;
>
> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
> -       if (ret)
> -               return ret;

My compiler happened to notice that with the change to this function:

drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret'
[-Werror,-Wunused-variable]
        int ret = 0;

-Doug
Taniya Das Nov. 15, 2019, 8:11 a.m. UTC | #3 
Hi Stephen,

Thanks for your review.

On 11/6/2019 6:06 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2019-10-31 05:21:07)
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index 055318f..8cb77ca 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                                  unsigned long prate)
>>   {
>>          struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> -       u32 val, l, alpha_width = pll_alpha_width(pll);
>> +       u32 l, alpha_width = pll_alpha_width(pll);
>>          u64 a;
>>          unsigned long rrate;
>>          int ret = 0;
>>
>> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
>> -       if (ret)
>> -               return ret;
>> -
>>          rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);
>>
>>          /*
> 
> How is this diff related? Looks like it should be split off into another
> patch to remove a useless register read.
> 

I will split this in different patch.

>> @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>>          return __clk_alpha_pll_update_latch(pll);
>>   }
>>
>> +static int alpha_pll_fabia_prepare(struct clk_hw *hw)
>> +{
> 
> Why are we doing this in prepare vs. doing it at PLL configuration time?
> Does it need to be recalibrated each time the PLL is enabled?
> 

In the case if PLL looses the configuration then we would encounter PLL 
locking issues. Thus want to go ahead with prepare. In the case it is 
calibrated it would return.

>> +       struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> +       const struct pll_vco *vco;
>> +       struct clk_hw *parent_hw;
>> +       unsigned long cal_freq, rrate;
>> +       u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
>> +       u64 a;
>> +       int ret;
>> +
>> +       /* Check if calibration needs to be done i.e. PLL is in reset */
>> +       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);
> 
> Please use 'val' instead of 'regval' as regval almost never appears in
> this file already.
> 

Sure, will use 'val'.

>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Return early if calibration is not needed. */
>> +       if (regval & PLL_RESET_N)
>> +               return 0;
>> +
>> +       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;
>> +       }
>> +
>> +       cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
>> +                               pll->vco_table[0].max_freq) * 54, 100);
> 
> Do we need to cast the first argument to a u64 to avoid overflow?
> 

No we do not need.

>> +
>> +       parent_hw = clk_hw_get_parent(hw);
>> +       if (!parent_hw)
>> +               return -EINVAL;
>> +
>> +       rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
>> +                                       &cal_l, &a, alpha_width);
>> +       /*
>> +        * Due to a limited number of bits for fractional rate programming, the
>> +        * rounded up rate could be marginally higher than the requested rate.
>> +        */
>> +       if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
>> +               pr_err("Call set rate on the PLL with rounded rates!\n");
> 
> This message is weird. Drivers shouldn't need to call set rate with
> rounded rates. What is going on?
> 

:), my bad, copy paste from another function. I will remove this print.

>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Setup PLL for calibration frequency */
>> +       regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
>> +
>> +       /* Bringup the pll at calibration frequency */
> 
> capitalize PLL.
> 

Will take care of it.

>> +       ret = clk_alpha_pll_enable(hw);
>> +       if (ret) {
>> +               pr_err("alpha pll calibration failed\n");
>> +               return ret;
>> +       }
>> +
>> +       clk_alpha_pll_disable(hw);
>> +
>> +       return 0;
>> +}
>> +
Taniya Das Nov. 15, 2019, 8:11 a.m. UTC | #4 
Hi Doug,

On 11/7/2019 9:54 AM, Doug Anderson wrote:

>> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
>> -       if (ret)
>> -               return ret;
> 
> My compiler happened to notice that with the change to this function:
> 
> drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret'
> [-Werror,-Wunused-variable]
>          int ret = 0;
> 
> -Doug
> 

Thanks for the review, will remove the unused variable.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 055318f..8cb77ca 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -1024,6 +1024,25 @@  void clk_fabia_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 		regmap_write(regmap, PLL_CONFIG_CTL(pll),
 						config->config_ctl_val);

+	if (config->config_ctl_hi_val)
+		regmap_write(regmap, PLL_CONFIG_CTL_U(pll),
+						config->config_ctl_hi_val);
+
+	if (config->user_ctl_val)
+		regmap_write(regmap, PLL_USER_CTL(pll), config->user_ctl_val);
+
+	if (config->user_ctl_hi_val)
+		regmap_write(regmap, PLL_USER_CTL_U(pll),
+						config->user_ctl_hi_val);
+
+	if (config->test_ctl_val)
+		regmap_write(regmap, PLL_TEST_CTL(pll),
+						config->test_ctl_val);
+
+	if (config->test_ctl_hi_val)
+		regmap_write(regmap, PLL_TEST_CTL_U(pll),
+						config->test_ctl_hi_val);
+
 	if (config->post_div_mask) {
 		mask = config->post_div_mask;
 		val = config->post_div_val;
@@ -1141,15 +1160,11 @@  static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
 						unsigned long prate)
 {
 	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
-	u32 val, l, alpha_width = pll_alpha_width(pll);
+	u32 l, alpha_width = pll_alpha_width(pll);
 	u64 a;
 	unsigned long rrate;
 	int ret = 0;

-	ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
-	if (ret)
-		return ret;
-
 	rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);

 	/*
@@ -1167,7 +1182,66 @@  static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
 	return __clk_alpha_pll_update_latch(pll);
 }

+static int alpha_pll_fabia_prepare(struct clk_hw *hw)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	const struct pll_vco *vco;
+	struct clk_hw *parent_hw;
+	unsigned long cal_freq, rrate;
+	u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
+	u64 a;
+	int ret;
+
+	/* Check if calibration needs to be done i.e. PLL is in reset */
+	ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);
+	if (ret)
+		return ret;
+
+	/* Return early if calibration is not needed. */
+	if (regval & PLL_RESET_N)
+		return 0;
+
+	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;
+	}
+
+	cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
+				pll->vco_table[0].max_freq) * 54, 100);
+
+	parent_hw = clk_hw_get_parent(hw);
+	if (!parent_hw)
+		return -EINVAL;
+
+	rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
+					&cal_l, &a, alpha_width);
+	/*
+	 * Due to a limited number of bits for fractional rate programming, the
+	 * rounded up rate could be marginally higher than the requested rate.
+	 */
+	if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
+		pr_err("Call set rate on the PLL with rounded rates!\n");
+		return -EINVAL;
+	}
+
+	/* Setup PLL for calibration frequency */
+	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
+
+	/* Bringup the pll at calibration frequency */
+	ret = clk_alpha_pll_enable(hw);
+	if (ret) {
+		pr_err("alpha pll calibration failed\n");
+		return ret;
+	}
+
+	clk_alpha_pll_disable(hw);
+
+	return 0;
+}
+
 const struct clk_ops clk_alpha_pll_fabia_ops = {
+	.prepare = alpha_pll_fabia_prepare,
 	.enable = alpha_pll_fabia_enable,
 	.disable = alpha_pll_fabia_disable,
 	.is_enabled = clk_alpha_pll_is_enabled,
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 15f27f4..b03eea0 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -94,6 +94,10 @@  struct alpha_pll_config {
 	u32 alpha_hi;
 	u32 config_ctl_val;
 	u32 config_ctl_hi_val;
+	u32 user_ctl_val;
+	u32 user_ctl_hi_val;
+	u32 test_ctl_val;
+	u32 test_ctl_hi_val;
 	u32 main_output_mask;
 	u32 aux_output_mask;
 	u32 aux2_output_mask;