[PATCH v2 00/10] clk: qcom: PLL updates

[PATCH v2 00/10] clk: qcom: PLL updates

[Rajendra Nayak]

Hi,

This series adds some additional support to the clk-alpha-pll and the
clk-pll drivers in preperation to add the CPU clock driver support
on msm8996

Changes in v2:
* Patch 1 to 6 are same as v1 post, added patches 7 to 10

Rajendra Nayak (8):
  clk: Fix inconsistencies in usage of data types
  clk: qcom: Add support for alpha pll hwfsm ops
  clk: qcom: Add support to initialize alpha plls
  clk: qcom: Add support for PLLs with alpha mode
  clk: qcom: Add support for PLLs with early output
  clk: qcom: Add support for PLLs supporting dynamic reprogramming
  clk: qcom: Add support to enable FSM mode for votable alpha PLLs
  clk: qcom: Add .is_enabled ops for clk-alpha-pll

Taniya Das (2):
  clk: qcom: Cleanup some macro defs
  clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

 drivers/clk/clk.c                |   4 +-
 drivers/clk/qcom/clk-alpha-pll.c | 235 ++++++++++++++++++++++++++++++++++-----
 drivers/clk/qcom/clk-alpha-pll.h |  17 +++
 drivers/clk/qcom/clk-pll.c       | 123 +++++++++++++++++---
 drivers/clk/qcom/clk-pll.h       |  12 +-
 drivers/clk/qcom/common.c        |  29 +++++
 drivers/clk/qcom/common.h        |   2 +
 include/linux/clk-provider.h     |   5 +-
 8 files changed, 378 insertions(+), 49 deletions(-)

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

[PATCH v2 01/10] clk: Fix inconsistencies in usage of data types

[Rajendra Nayak]

index is of type u8 in all places except in clk_hw_get_parent_by_index()
and return value of all round_rate functions is long except for
clk_hw_round_rate(). Make them consistent with the rest of the places

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/clk.c            | 4 ++--
 include/linux/clk-provider.h | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 820a939..e768071 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -261,7 +261,7 @@ static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
 }
 
 struct clk_hw *
-clk_hw_get_parent_by_index(const struct clk_hw *hw, unsigned int index)
+clk_hw_get_parent_by_index(const struct clk_hw *hw, u8 index)
 {
 	struct clk_core *parent;
 
@@ -889,7 +889,7 @@ int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 }
 EXPORT_SYMBOL_GPL(__clk_determine_rate);
 
-unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
+long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 {
 	int ret;
 	struct clk_rate_request req;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a39c0c5..230a249 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -732,8 +732,7 @@ const char *clk_hw_get_name(const struct clk_hw *hw);
 struct clk_hw *__clk_get_hw(struct clk *clk);
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
-struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
-					  unsigned int index);
+struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw, u8 index);
 unsigned int __clk_get_enable_count(struct clk *clk);
 unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 unsigned long __clk_get_flags(struct clk *clk);
@@ -760,7 +759,7 @@ static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 /*
  * FIXME clock api without lock protection
  */
-unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate);
+long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate);
 
 struct of_device_id;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 01/10] clk: Fix inconsistencies in usage of data types

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> index is of type u8 in all places except in clk_hw_get_parent_by_index()
> and return value of all round_rate functions is long except for
> clk_hw_round_rate(). Make them consistent with the rest of the places
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/clk.c            | 4 ++--
>  include/linux/clk-provider.h | 5 ++---
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 820a939..e768071 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -261,7 +261,7 @@ static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
>  }
>  
>  struct clk_hw *
> -clk_hw_get_parent_by_index(const struct clk_hw *hw, unsigned int index)
> +clk_hw_get_parent_by_index(const struct clk_hw *hw, u8 index)

I guess this is ok, but u8 seems limiting for no reason and we
may want to expand the parent array at some point in the future
(although it would be a huge change). So I'd like to leave this
alone. Inconsistent as it is.

>  {
>  	struct clk_core *parent;
>  
> @@ -889,7 +889,7 @@ int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>  }
>  EXPORT_SYMBOL_GPL(__clk_determine_rate);
>  
> -unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
> +long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
>  {
>  	int ret;
>  	struct clk_rate_request req;

This function should never return a negative value, so the return
type is unsigned.

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

[PATCH v2 02/10] clk: qcom: Add support for alpha pll hwfsm ops

[Rajendra Nayak]

Add support to enable/disable the alpha pll using hwfsm

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 109 ++++++++++++++++++++++++++++++++++-----
 drivers/clk/qcom/clk-alpha-pll.h |   1 +
 2 files changed, 98 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index e6a03ea..bae31f9 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -62,9 +62,10 @@
 #define to_clk_alpha_pll_postdiv(_hw) container_of(to_clk_regmap(_hw), \
 					   struct clk_alpha_pll_postdiv, clkr)
 
-static int wait_for_pll(struct clk_alpha_pll *pll)
+static int wait_for_pll(struct clk_alpha_pll *pll, u32 mask, bool inverse,
+			const char *action)
 {
-	u32 val, mask, off;
+	u32 val, off;
 	int count;
 	int ret;
 	const char *name = clk_hw_get_name(&pll->clkr.hw);
@@ -74,26 +75,101 @@ static int wait_for_pll(struct clk_alpha_pll *pll)
 	if (ret)
 		return ret;
 
-	if (val & PLL_VOTE_FSM_ENA)
-		mask = PLL_ACTIVE_FLAG;
-	else
-		mask = PLL_LOCK_DET;
-
-	/* Wait for pll to enable. */
 	for (count = 100; count > 0; count--) {
 		ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
 		if (ret)
 			return ret;
-		if ((val & mask) == mask)
+		if (inverse && (val & mask))
+			return 0;
+		else if ((val & mask) == mask)
 			return 0;
 
 		udelay(1);
 	}
 
-	WARN(1, "%s didn't enable after voting for it!\n", name);
+	WARN(1, "%s failed to %s!\n", name, action);
 	return -ETIMEDOUT;
 }
 
+static int wait_for_pll_enable(struct clk_alpha_pll *pll, u32 mask)
+{
+	return wait_for_pll(pll, mask, 0, "enable");
+}
+
+static int wait_for_pll_disable(struct clk_alpha_pll *pll, u32 mask)
+{
+	return wait_for_pll(pll, mask, 1, "disable");
+}
+
+static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
+{
+	return wait_for_pll(pll, mask, 0, "offline");
+}
+
+/* alpha pll with hwfsm support */
+#define PLL_OFFLINE_REQ		BIT(7)
+#define PLL_FSM_ENA		BIT(20)
+#define PLL_OFFLINE_ACK		BIT(28)
+#define PLL_ACTIVE_FLAG		BIT(30)
+
+static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
+{
+	int ret;
+	u32 val, off;
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+
+	off = pll->offset;
+	ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
+	if (ret)
+		return ret;
+
+	/* Enable HW FSM mode, clear OFFLINE request */
+	val |= PLL_FSM_ENA;
+	val &= ~PLL_OFFLINE_REQ;
+	ret = regmap_write(pll->clkr.regmap, off + PLL_MODE, val);
+	if (ret)
+		return ret;
+
+	/* Make sure enable request goes through before waiting for update */
+	mb();
+
+	ret = wait_for_pll_enable(pll, PLL_ACTIVE_FLAG);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void clk_alpha_pll_hwfsm_disable(struct clk_hw *hw)
+{
+	int ret;
+	u32 val, off;
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+
+	off = pll->offset;
+	ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
+	if (ret)
+		return;
+
+	/* Request PLL_OFFLINE and wait for ack */
+	ret = regmap_update_bits(pll->clkr.regmap, off + PLL_MODE,
+				 PLL_OFFLINE_REQ, PLL_OFFLINE_REQ);
+	if (ret)
+		return;
+
+	ret = wait_for_pll_offline(pll, PLL_OFFLINE_ACK);
+	if (ret)
+		return;
+
+	/* Disable hwfsm */
+	ret = regmap_update_bits(pll->clkr.regmap, off + PLL_MODE,
+				 PLL_FSM_ENA, 0);
+	if (ret)
+		return;
+
+	wait_for_pll_disable(pll, PLL_ACTIVE_FLAG);
+}
+
 static int clk_alpha_pll_enable(struct clk_hw *hw)
 {
 	int ret;
@@ -112,7 +188,7 @@ static int clk_alpha_pll_enable(struct clk_hw *hw)
 		ret = clk_enable_regmap(hw);
 		if (ret)
 			return ret;
-		return wait_for_pll(pll);
+		return wait_for_pll_enable(pll, PLL_ACTIVE_FLAG);
 	}
 
 	/* Skip if already enabled */
@@ -136,7 +212,7 @@ static int clk_alpha_pll_enable(struct clk_hw *hw)
 	if (ret)
 		return ret;
 
-	ret = wait_for_pll(pll);
+	ret = wait_for_pll_enable(pll, PLL_LOCK_DET);
 	if (ret)
 		return ret;
 
@@ -300,6 +376,15 @@ const struct clk_ops clk_alpha_pll_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_alpha_pll_ops);
 
+const struct clk_ops clk_alpha_pll_hwfsm_ops = {
+	.enable = clk_alpha_pll_hwfsm_enable,
+	.disable = clk_alpha_pll_hwfsm_disable,
+	.recalc_rate = clk_alpha_pll_recalc_rate,
+	.round_rate = clk_alpha_pll_round_rate,
+	.set_rate = clk_alpha_pll_set_rate,
+};
+EXPORT_SYMBOL_GPL(clk_alpha_pll_hwfsm_ops);
+
 static unsigned long
 clk_alpha_pll_postdiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 90ce201..f78bf4c 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -52,6 +52,7 @@ struct clk_alpha_pll_postdiv {
 };
 
 extern const struct clk_ops clk_alpha_pll_ops;
+extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
 extern const struct clk_ops clk_alpha_pll_postdiv_ops;
 
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 02/10] clk: qcom: Add support for alpha pll hwfsm ops

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> Add support to enable/disable the alpha pll using hwfsm

Care to add some more description here about what's going on?

> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-alpha-pll.c | 109 ++++++++++++++++++++++++++++++++++-----
>  drivers/clk/qcom/clk-alpha-pll.h |   1 +
>  2 files changed, 98 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index e6a03ea..bae31f9 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -62,9 +62,10 @@
>  #define to_clk_alpha_pll_postdiv(_hw) container_of(to_clk_regmap(_hw), \
>  					   struct clk_alpha_pll_postdiv, clkr)
>  
> -static int wait_for_pll(struct clk_alpha_pll *pll)
> +static int wait_for_pll(struct clk_alpha_pll *pll, u32 mask, bool inverse,
> +			const char *action)
>  {
> -	u32 val, mask, off;
> +	u32 val, off;
>  	int count;
>  	int ret;
>  	const char *name = clk_hw_get_name(&pll->clkr.hw);
> @@ -74,26 +75,101 @@ static int wait_for_pll(struct clk_alpha_pll *pll)
>  	if (ret)
>  		return ret;
>  
> -	if (val & PLL_VOTE_FSM_ENA)
> -		mask = PLL_ACTIVE_FLAG;
> -	else
> -		mask = PLL_LOCK_DET;
> -
> -	/* Wait for pll to enable. */

Perhaps commit text could state why we shouldn't keep extending
this model of figuring out what to poll?

>  	for (count = 100; count > 0; count--) {
>  		ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
>  		if (ret)
>  			return ret;
> -		if ((val & mask) == mask)
> +		if (inverse && (val & mask))
> +			return 0;
> +		else if ((val & mask) == mask)
>  			return 0;
>  
>  		udelay(1);
>  	}
>  
> -	WARN(1, "%s didn't enable after voting for it!\n", name);
> +	WARN(1, "%s failed to %s!\n", name, action);
>  	return -ETIMEDOUT;
>  }
>  
> +static int wait_for_pll_enable(struct clk_alpha_pll *pll, u32 mask)
> +{
> +	return wait_for_pll(pll, mask, 0, "enable");
> +}

This is only called with two masks, so maybe we can have two
functions for it or a simple macro to avoid making clients know
about the mask?

> +
> +static int wait_for_pll_disable(struct clk_alpha_pll *pll, u32 mask)
> +{
> +	return wait_for_pll(pll, mask, 1, "disable");
> +}
> +
> +static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
> +{
> +	return wait_for_pll(pll, mask, 0, "offline");
> +}

These two are only called with one mask, why have that as a parameter?

> +
> +/* alpha pll with hwfsm support */
> +#define PLL_OFFLINE_REQ		BIT(7)
> +#define PLL_FSM_ENA		BIT(20)
> +#define PLL_OFFLINE_ACK		BIT(28)
> +#define PLL_ACTIVE_FLAG		BIT(30)

Please put these up top next to the register that they're for.

> +
> +static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
> +{
> +	int ret;
> +	u32 val, off;
> +	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> +
> +	off = pll->offset;
> +	ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable HW FSM mode, clear OFFLINE request */

That's pretty obvious.

> +	val |= PLL_FSM_ENA;
> +	val &= ~PLL_OFFLINE_REQ;
> +	ret = regmap_write(pll->clkr.regmap, off + PLL_MODE, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Make sure enable request goes through before waiting for update */
> +	mb();
> +
> +	ret = wait_for_pll_enable(pll, PLL_ACTIVE_FLAG);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Simplify to return wait_for_pll_enable()?


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

Re: [PATCH v2 02/10] clk: qcom: Add support for alpha pll hwfsm ops

[Rajendra Nayak]

On 08/24/2016 11:43 AM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> Add support to enable/disable the alpha pll using hwfsm
> 
> Care to add some more description here about what's going on?

Sure, I will add some details on what the additional hwfsm mode
brings in to the alpha plls.

> 
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/clk/qcom/clk-alpha-pll.c | 109 ++++++++++++++++++++++++++++++++++-----
>>  drivers/clk/qcom/clk-alpha-pll.h |   1 +
>>  2 files changed, 98 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index e6a03ea..bae31f9 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -62,9 +62,10 @@
>>  #define to_clk_alpha_pll_postdiv(_hw) container_of(to_clk_regmap(_hw), \
>>  					   struct clk_alpha_pll_postdiv, clkr)
>>  
>> -static int wait_for_pll(struct clk_alpha_pll *pll)
>> +static int wait_for_pll(struct clk_alpha_pll *pll, u32 mask, bool inverse,
>> +			const char *action)
>>  {
>> -	u32 val, mask, off;
>> +	u32 val, off;
>>  	int count;
>>  	int ret;
>>  	const char *name = clk_hw_get_name(&pll->clkr.hw);
>> @@ -74,26 +75,101 @@ static int wait_for_pll(struct clk_alpha_pll *pll)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (val & PLL_VOTE_FSM_ENA)
>> -		mask = PLL_ACTIVE_FLAG;
>> -	else
>> -		mask = PLL_LOCK_DET;
>> -
>> -	/* Wait for pll to enable. */
> 
> Perhaps commit text could state why we shouldn't keep extending
> this model of figuring out what to poll?

sure, the intention was to make this a more generic poll function, without
it having to know if its polling for enable/disable/offline or some future
case that we might end up with.

> 
>>  	for (count = 100; count > 0; count--) {
>>  		ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
>>  		if (ret)
>>  			return ret;
>> -		if ((val & mask) == mask)
>> +		if (inverse && (val & mask))
>> +			return 0;
>> +		else if ((val & mask) == mask)
>>  			return 0;
>>  
>>  		udelay(1);
>>  	}
>>  
>> -	WARN(1, "%s didn't enable after voting for it!\n", name);
>> +	WARN(1, "%s failed to %s!\n", name, action);
>>  	return -ETIMEDOUT;
>>  }
>>  
>> +static int wait_for_pll_enable(struct clk_alpha_pll *pll, u32 mask)
>> +{
>> +	return wait_for_pll(pll, mask, 0, "enable");
>> +}
> 
> This is only called with two masks, so maybe we can have two
> functions for it or a simple macro to avoid making clients know
> about the mask?

The idea was to basically make it more future proof by letting the
caller pass what they want to poll on (there is no reason why we won't
possibly end up with more masks in future)
I am fine moving these to 2 functions/macros instead.

> 
>> +
>> +static int wait_for_pll_disable(struct clk_alpha_pll *pll, u32 mask)
>> +{
>> +	return wait_for_pll(pll, mask, 1, "disable");
>> +}
>> +
>> +static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
>> +{
>> +	return wait_for_pll(pll, mask, 0, "offline");
>> +}
> 
> These two are only called with one mask, why have that as a parameter?

Again this was assuming we would end up with more masks perhaps as we go
along. For now I can get rid of it.

> 
>> +
>> +/* alpha pll with hwfsm support */
>> +#define PLL_OFFLINE_REQ		BIT(7)
>> +#define PLL_FSM_ENA		BIT(20)
>> +#define PLL_OFFLINE_ACK		BIT(28)
>> +#define PLL_ACTIVE_FLAG		BIT(30)
> 
> Please put these up top next to the register that they're for.

will do.

> 
>> +
>> +static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
>> +{
>> +	int ret;
>> +	u32 val, off;
>> +	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> +
>> +	off = pll->offset;
>> +	ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable HW FSM mode, clear OFFLINE request */
> 
> That's pretty obvious.

I can remove it

> 
>> +	val |= PLL_FSM_ENA;
>> +	val &= ~PLL_OFFLINE_REQ;
>> +	ret = regmap_write(pll->clkr.regmap, off + PLL_MODE, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Make sure enable request goes through before waiting for update */
>> +	mb();
>> +
>> +	ret = wait_for_pll_enable(pll, PLL_ACTIVE_FLAG);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Simplify to return wait_for_pll_enable()?

will do, thanks.
> 
> 

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

[PATCH v2 03/10] clk: qcom: Add support to initialize alpha plls

[Rajendra Nayak]

Add a function to do initial configuration of the alpha plls

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 23 +++++++++++++++++++++++
 drivers/clk/qcom/clk-alpha-pll.h | 13 +++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index bae31f9..8b8710f 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -112,6 +112,29 @@ static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
 #define PLL_OFFLINE_ACK		BIT(28)
 #define PLL_ACTIVE_FLAG		BIT(30)
 
+void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
+			     const struct alpha_pll_config *config)
+{
+	u32 val, mask;
+
+	regmap_write(regmap, pll->offset + PLL_CONFIG_CTL,
+		     config->config_ctl_val);
+
+	val = config->main_output_mask;
+	val |= config->aux_output_mask;
+	val |= config->aux2_output_mask;
+	val |= config->early_output_mask;
+	val |= config->post_div_val;
+
+	mask = config->main_output_mask;
+	mask |= config->aux_output_mask;
+	mask |= config->aux2_output_mask;
+	mask |= config->early_output_mask;
+	mask |= config->post_div_mask;
+
+	regmap_update_bits(regmap, pll->offset + PLL_USER_CTL, mask, val);
+}
+
 static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
 {
 	int ret;
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index f78bf4c..12a349e 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -51,8 +51,21 @@ struct clk_alpha_pll_postdiv {
 	struct clk_regmap clkr;
 };
 
+struct alpha_pll_config {
+	u32 config_ctl_val;
+	u32 main_output_mask;
+	u32 aux_output_mask;
+	u32 aux2_output_mask;
+	u32 early_output_mask;
+	u32 post_div_val;
+	u32 post_div_mask;
+};
+
 extern const struct clk_ops clk_alpha_pll_ops;
 extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
 extern const struct clk_ops clk_alpha_pll_postdiv_ops;
 
+void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
+			     const struct alpha_pll_config *config);
+
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 04/10] clk: qcom: Add support for PLLs with alpha mode

[Rajendra Nayak]

Some PLLs can support an alpha mode, and a single alpha
register (instead of registers to program the M/N values),
the contents of which depend on the alpha mode selected.
(They are either treated as two's complement or M/N value)
Add support for this in the clk PLL driver.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-pll.c | 8 ++++++--
 drivers/clk/qcom/clk-pll.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
index 5b940d6..08d2fa2 100644
--- a/drivers/clk/qcom/clk-pll.c
+++ b/drivers/clk/qcom/clk-pll.c
@@ -255,8 +255,12 @@ static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
 	u32 mask;
 
 	regmap_write(regmap, pll->l_reg, config->l);
-	regmap_write(regmap, pll->m_reg, config->m);
-	regmap_write(regmap, pll->n_reg, config->n);
+	if (pll->alpha_reg) {
+		regmap_write(regmap, pll->alpha_reg, config->alpha);
+	} else {
+		regmap_write(regmap, pll->m_reg, config->m);
+		regmap_write(regmap, pll->n_reg, config->n);
+	}
 
 	val = config->vco_val;
 	val |= config->pre_div_val;
diff --git a/drivers/clk/qcom/clk-pll.h b/drivers/clk/qcom/clk-pll.h
index ffd0c63..083727e 100644
--- a/drivers/clk/qcom/clk-pll.h
+++ b/drivers/clk/qcom/clk-pll.h
@@ -48,6 +48,7 @@ struct clk_pll {
 	u32	l_reg;
 	u32	m_reg;
 	u32	n_reg;
+	u32	alpha_reg;
 	u32	config_reg;
 	u32	mode_reg;
 	u32	status_reg;
@@ -70,6 +71,7 @@ struct pll_config {
 	u16 l;
 	u32 m;
 	u32 n;
+	u32 alpha;
 	u32 vco_val;
 	u32 vco_mask;
 	u32 pre_div_val;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 04/10] clk: qcom: Add support for PLLs with alpha mode

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> Some PLLs can support an alpha mode, and a single alpha
> register (instead of registers to program the M/N values),
> the contents of which depend on the alpha mode selected.
> (They are either treated as two's complement or M/N value)

That's just a sentence, so please drop the parentheses.

> Add support for this in the clk PLL driver.
> 

I'm confused, don't we already have clk-alpha-pll.c to handle
alpha type plls? What are we doing adding support to the "legacy"
pll code?

> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-pll.c | 8 ++++++--
>  drivers/clk/qcom/clk-pll.h | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
> index 5b940d6..08d2fa2 100644
> --- a/drivers/clk/qcom/clk-pll.c
> +++ b/drivers/clk/qcom/clk-pll.c
> @@ -255,8 +255,12 @@ static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
>  	u32 mask;
>  
>  	regmap_write(regmap, pll->l_reg, config->l);
> -	regmap_write(regmap, pll->m_reg, config->m);
> -	regmap_write(regmap, pll->n_reg, config->n);
> +	if (pll->alpha_reg) {

This assumes that alpha_reg is not 0 offset from base, which
seems like a bad assumption to make.

> +		regmap_write(regmap, pll->alpha_reg, config->alpha);
> +	} else {
> +		regmap_write(regmap, pll->m_reg, config->m);
> +		regmap_write(regmap, pll->n_reg, config->n);
> +	}
>  
>  	val = config->vco_val;
>  	val |= config->pre_div_val;
> 

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

Re: [PATCH v2 04/10] clk: qcom: Add support for PLLs with alpha mode

[Rajendra Nayak]

On 08/24/2016 11:45 AM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> Some PLLs can support an alpha mode, and a single alpha
>> register (instead of registers to program the M/N values),
>> the contents of which depend on the alpha mode selected.
>> (They are either treated as two's complement or M/N value)
> 
> That's just a sentence, so please drop the parentheses.

OK

> 
>> Add support for this in the clk PLL driver.
>>
> 
> I'm confused, don't we already have clk-alpha-pll.c to handle
> alpha type plls? What are we doing adding support to the "legacy"
> pll code?

Yes, this does look confusing now that I took a relook at it all.
I will redo this whole thing so it fits into the alpha PLL support that
we already have.

> 
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/clk/qcom/clk-pll.c | 8 ++++++--
>>  drivers/clk/qcom/clk-pll.h | 2 ++
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
>> index 5b940d6..08d2fa2 100644
>> --- a/drivers/clk/qcom/clk-pll.c
>> +++ b/drivers/clk/qcom/clk-pll.c
>> @@ -255,8 +255,12 @@ static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
>>  	u32 mask;
>>  
>>  	regmap_write(regmap, pll->l_reg, config->l);
>> -	regmap_write(regmap, pll->m_reg, config->m);
>> -	regmap_write(regmap, pll->n_reg, config->n);
>> +	if (pll->alpha_reg) {
> 
> This assumes that alpha_reg is not 0 offset from base, which
> seems like a bad assumption to make.

sure, I need to handle this in a better way

> 
>> +		regmap_write(regmap, pll->alpha_reg, config->alpha);
>> +	} else {
>> +		regmap_write(regmap, pll->m_reg, config->m);
>> +		regmap_write(regmap, pll->n_reg, config->n);
>> +	}
>>  
>>  	val = config->vco_val;
>>  	val |= config->pre_div_val;
>>
> 

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

[PATCH v2 05/10] clk: qcom: Add support for PLLs with early output

[Rajendra Nayak]

Some PLLs can have an additional early output (apart from
the main and aux outputs). Add support for the PLL driver
so it can be used to initialize/configure the early output

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-pll.c | 2 ++
 drivers/clk/qcom/clk-pll.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
index 08d2fa2..b463432 100644
--- a/drivers/clk/qcom/clk-pll.c
+++ b/drivers/clk/qcom/clk-pll.c
@@ -268,6 +268,7 @@ static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
 	val |= config->mn_ena_mask;
 	val |= config->main_output_mask;
 	val |= config->aux_output_mask;
+	val |= config->early_output_mask;
 
 	mask = config->vco_mask;
 	mask |= config->pre_div_mask;
@@ -275,6 +276,7 @@ static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
 	mask |= config->mn_ena_mask;
 	mask |= config->main_output_mask;
 	mask |= config->aux_output_mask;
+	mask |= config->early_output_mask;
 
 	regmap_update_bits(regmap, pll->config_reg, mask, val);
 }
diff --git a/drivers/clk/qcom/clk-pll.h b/drivers/clk/qcom/clk-pll.h
index 083727e..dbe22a9 100644
--- a/drivers/clk/qcom/clk-pll.h
+++ b/drivers/clk/qcom/clk-pll.h
@@ -81,6 +81,7 @@ struct pll_config {
 	u32 mn_ena_mask;
 	u32 main_output_mask;
 	u32 aux_output_mask;
+	u32 early_output_mask;
 };
 
 void clk_pll_configure_sr(struct clk_pll *pll, struct regmap *regmap,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 06/10] clk: qcom: Add support for PLLs supporting dynamic reprogramming

[Rajendra Nayak]

Some PLLs can support dynamic reprogramming, which means just a L value
change is whats needed to change the PLL frequency without having to
explicitly enable/disable or bypass/re-lock the PLL.
Add support for such PLLs' initial configuration and the ops needed to
support the dynamic reprogramming thereafter.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-pll.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-pll.h |   9 +++-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
index b463432..13d3f64 100644
--- a/drivers/clk/qcom/clk-pll.c
+++ b/drivers/clk/qcom/clk-pll.c
@@ -32,6 +32,7 @@
 #define PLL_BIAS_COUNT_SHIFT	14
 #define PLL_BIAS_COUNT_MASK	0x3f
 #define PLL_VOTE_FSM_ENA	BIT(20)
+#define PLL_DYN_FSM_ENA		BIT(20)
 #define PLL_VOTE_FSM_RESET	BIT(21)
 
 static int clk_pll_enable(struct clk_hw *hw)
@@ -248,6 +249,19 @@ clk_pll_set_fsm_mode(struct clk_pll *pll, struct regmap *regmap, u8 lock_count)
 		PLL_VOTE_FSM_ENA);
 }
 
+static void
+clk_pll_set_dynamic_fsm_mode(struct clk_pll *pll, struct regmap *regmap)
+{
+	u32 val;
+	u32 mask;
+
+	mask = PLL_BIAS_COUNT_MASK | PLL_DYN_FSM_ENA;
+	val = 6 << PLL_BIAS_COUNT_SHIFT;
+	val |= PLL_DYN_FSM_ENA;
+
+	regmap_update_bits(regmap, pll->mode_reg, mask, val);
+}
+
 static void clk_pll_configure(struct clk_pll *pll, struct regmap *regmap,
 	const struct pll_config *config)
 {
@@ -299,6 +313,21 @@ void clk_pll_configure_sr_hpm_lp(struct clk_pll *pll, struct regmap *regmap,
 }
 EXPORT_SYMBOL_GPL(clk_pll_configure_sr_hpm_lp);
 
+void clk_pll_configure_dynamic(struct clk_pll *pll, struct regmap *regmap,
+			       const struct pll_config *config)
+{
+	u32 config_ctl_reg = pll->config_ctl_reg;
+	u32 config_ctl_hi_reg = pll->config_ctl_reg + 4;
+
+	clk_pll_configure(pll, regmap, config);
+
+	regmap_write(regmap, config_ctl_reg, config->config_ctl_val);
+	regmap_write(regmap, config_ctl_hi_reg, config->config_ctl_hi_val);
+
+	clk_pll_set_dynamic_fsm_mode(pll, regmap);
+}
+EXPORT_SYMBOL_GPL(clk_pll_configure_dynamic);
+
 static int clk_pll_sr2_enable(struct clk_hw *hw)
 {
 	struct clk_pll *pll = to_clk_pll(hw);
@@ -373,3 +402,80 @@ const struct clk_ops clk_pll_sr2_ops = {
 	.determine_rate = clk_pll_determine_rate,
 };
 EXPORT_SYMBOL_GPL(clk_pll_sr2_ops);
+
+static int clk_pll_dynamic_enable(struct clk_hw *hw)
+{
+	struct clk_pll *pll = to_clk_pll(hw);
+
+	/* Wait for 50us explicitly to avoid transient locks */
+	udelay(50);
+
+	return wait_for_pll(pll);
+};
+
+static void clk_pll_dynamic_disable(struct clk_hw *hw)
+{
+	/* 8 reference clock cycle delay mandated by the HPG */
+	udelay(1);
+};
+
+static unsigned long
+clk_pll_dynamic_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	u32 l_val;
+	int ret;
+
+	struct clk_pll *pll = to_clk_pll(hw);
+
+	ret = regmap_read(pll->clkr.regmap, pll->l_reg, &l_val);
+	if (ret)
+		return ret;
+
+	return l_val * parent_rate;
+};
+
+static int
+clk_pll_dynamic_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	struct clk_pll *pll = to_clk_pll(hw);
+	const struct pll_freq_tbl *f;
+
+	f = find_freq(pll->freq_tbl, req->rate);
+	if (!f)
+		req->rate = DIV_ROUND_UP(req->rate, req->best_parent_rate)
+					* req->best_parent_rate;
+	else
+		req->rate = f->freq;
+
+	if (req->rate < pll->min_rate)
+		req->rate = pll->min_rate;
+	else if (req->rate > pll->max_rate)
+		req->rate = pll->max_rate;
+
+	return 0;
+}
+
+static int
+clk_pll_dynamic_set_rate(struct clk_hw *hw, unsigned long rate,
+			 unsigned long prate)
+{
+	u32 l_val;
+	struct clk_pll *pll = to_clk_pll(hw);
+
+	if ((rate < pll->min_rate) || (rate > pll->max_rate) || !prate)
+		return -EINVAL;
+
+	l_val = rate / prate;
+	regmap_write(pll->clkr.regmap, pll->l_reg, l_val);
+
+	return 0;
+}
+
+const struct clk_ops clk_pll_dynamic_ops = {
+	.enable = clk_pll_dynamic_enable,
+	.disable = clk_pll_dynamic_disable,
+	.set_rate = clk_pll_dynamic_set_rate,
+	.recalc_rate = clk_pll_dynamic_recalc_rate,
+	.determine_rate = clk_pll_dynamic_determine_rate,
+};
+EXPORT_SYMBOL_GPL(clk_pll_dynamic_ops);
diff --git a/drivers/clk/qcom/clk-pll.h b/drivers/clk/qcom/clk-pll.h
index dbe22a9..627588f 100644
--- a/drivers/clk/qcom/clk-pll.h
+++ b/drivers/clk/qcom/clk-pll.h
@@ -52,9 +52,12 @@ struct clk_pll {
 	u32	config_reg;
 	u32	mode_reg;
 	u32	status_reg;
+	u32	config_ctl_reg;
 	u8	status_bit;
 	u8	post_div_width;
 	u8	post_div_shift;
+	unsigned long min_rate;
+	unsigned long max_rate;
 
 	const struct pll_freq_tbl *freq_tbl;
 
@@ -64,6 +67,7 @@ struct clk_pll {
 extern const struct clk_ops clk_pll_ops;
 extern const struct clk_ops clk_pll_vote_ops;
 extern const struct clk_ops clk_pll_sr2_ops;
+extern const struct clk_ops clk_pll_dynamic_ops;
 
 #define to_clk_pll(_hw) container_of(to_clk_regmap(_hw), struct clk_pll, clkr)
 
@@ -78,6 +82,8 @@ struct pll_config {
 	u32 pre_div_mask;
 	u32 post_div_val;
 	u32 post_div_mask;
+	u32 config_ctl_val;
+	u32 config_ctl_hi_val;
 	u32 mn_ena_mask;
 	u32 main_output_mask;
 	u32 aux_output_mask;
@@ -88,5 +94,6 @@ void clk_pll_configure_sr(struct clk_pll *pll, struct regmap *regmap,
 		const struct pll_config *config, bool fsm_mode);
 void clk_pll_configure_sr_hpm_lp(struct clk_pll *pll, struct regmap *regmap,
 		const struct pll_config *config, bool fsm_mode);
-
+void clk_pll_configure_dynamic(struct clk_pll *pll, struct regmap *regmap,
+		const struct pll_config *config);
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 07/10] clk: qcom: Add support to enable FSM mode for votable alpha PLLs

[Rajendra Nayak]

The votable alpha PLLs need to have the fsm mode enabled as part
of the initialization. The sequence seems to be the same as used
by clk-pll, so move the function which does this into a common
place and reuse it for the clk-alpha-pll

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c |  5 +++++
 drivers/clk/qcom/clk-alpha-pll.h |  2 ++
 drivers/clk/qcom/clk-pll.c       | 25 +++----------------------
 drivers/clk/qcom/common.c        | 29 +++++++++++++++++++++++++++++
 drivers/clk/qcom/common.h        |  2 ++
 5 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 8b8710f..e8f3505 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 
 #include "clk-alpha-pll.h"
+#include "common.h"
 
 #define PLL_MODE		0x00
 # define PLL_OUTCTRL		BIT(0)
@@ -133,6 +134,10 @@ void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 	mask |= config->post_div_mask;
 
 	regmap_update_bits(regmap, pll->offset + PLL_USER_CTL, mask, val);
+
+	if (pll->flags & SUPPORTS_VOTE_FSM)
+		qcom_pll_set_fsm_mode(regmap, pll->offset + PLL_MODE, 6, 0);
+
 }
 
 static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 12a349e..4bd42fd 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -35,6 +35,8 @@ struct clk_alpha_pll {
 	const struct pll_vco *vco_table;
 	size_t num_vco;
 
+#define SUPPORTS_VOTE_FSM	BIT(0)
+	u8 flags;
 	struct clk_regmap clkr;
 };
 
diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
index 13d3f64..776278b 100644
--- a/drivers/clk/qcom/clk-pll.c
+++ b/drivers/clk/qcom/clk-pll.c
@@ -23,6 +23,7 @@
 #include <asm/div64.h>
 
 #include "clk-pll.h"
+#include "common.h"
 
 #define PLL_OUTCTRL		BIT(0)
 #define PLL_BYPASSNL		BIT(1)
@@ -230,26 +231,6 @@ const struct clk_ops clk_pll_vote_ops = {
 EXPORT_SYMBOL_GPL(clk_pll_vote_ops);
 
 static void
-clk_pll_set_fsm_mode(struct clk_pll *pll, struct regmap *regmap, u8 lock_count)
-{
-	u32 val;
-	u32 mask;
-
-	/* De-assert reset to FSM */
-	regmap_update_bits(regmap, pll->mode_reg, PLL_VOTE_FSM_RESET, 0);
-
-	/* Program bias count and lock count */
-	val = 1 << PLL_BIAS_COUNT_SHIFT | lock_count << PLL_LOCK_COUNT_SHIFT;
-	mask = PLL_BIAS_COUNT_MASK << PLL_BIAS_COUNT_SHIFT;
-	mask |= PLL_LOCK_COUNT_MASK << PLL_LOCK_COUNT_SHIFT;
-	regmap_update_bits(regmap, pll->mode_reg, mask, val);
-
-	/* Enable PLL FSM voting */
-	regmap_update_bits(regmap, pll->mode_reg, PLL_VOTE_FSM_ENA,
-		PLL_VOTE_FSM_ENA);
-}
-
-static void
 clk_pll_set_dynamic_fsm_mode(struct clk_pll *pll, struct regmap *regmap)
 {
 	u32 val;
@@ -300,7 +281,7 @@ void clk_pll_configure_sr(struct clk_pll *pll, struct regmap *regmap,
 {
 	clk_pll_configure(pll, regmap, config);
 	if (fsm_mode)
-		clk_pll_set_fsm_mode(pll, regmap, 8);
+		qcom_pll_set_fsm_mode(regmap, pll->mode_reg, 1, 8);
 }
 EXPORT_SYMBOL_GPL(clk_pll_configure_sr);
 
@@ -309,7 +290,7 @@ void clk_pll_configure_sr_hpm_lp(struct clk_pll *pll, struct regmap *regmap,
 {
 	clk_pll_configure(pll, regmap, config);
 	if (fsm_mode)
-		clk_pll_set_fsm_mode(pll, regmap, 0);
+		qcom_pll_set_fsm_mode(regmap, pll->mode_reg, 1, 0);
 }
 EXPORT_SYMBOL_GPL(clk_pll_configure_sr_hpm_lp);
 
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index f7c226a..6bf5abd 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -25,6 +25,14 @@
 #include "reset.h"
 #include "gdsc.h"
 
+#define PLL_LOCK_COUNT_SHIFT	8
+#define PLL_LOCK_COUNT_MASK	0x3f
+#define PLL_BIAS_COUNT_SHIFT	14
+#define PLL_BIAS_COUNT_MASK	0x3f
+#define PLL_VOTE_FSM_ENA	BIT(20)
+#define PLL_DYN_FSM_ENA		BIT(20)
+#define PLL_VOTE_FSM_RESET	BIT(21)
+
 struct qcom_cc {
 	struct qcom_reset_controller reset;
 	struct clk_onecell_data data;
@@ -74,6 +82,27 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 }
 EXPORT_SYMBOL_GPL(qcom_cc_map);
 
+void
+qcom_pll_set_fsm_mode(struct regmap *map, u32 reg, u8 bias_count, u8 lock_count)
+{
+	u32 val;
+	u32 mask;
+
+	/* De-assert reset to FSM */
+	regmap_update_bits(map, reg, PLL_VOTE_FSM_RESET, 0);
+
+	/* Program bias count and lock count */
+	val = bias_count << PLL_BIAS_COUNT_SHIFT |
+		lock_count << PLL_LOCK_COUNT_SHIFT;
+	mask = PLL_BIAS_COUNT_MASK << PLL_BIAS_COUNT_SHIFT;
+	mask |= PLL_LOCK_COUNT_MASK << PLL_LOCK_COUNT_SHIFT;
+	regmap_update_bits(map, reg, mask, val);
+
+	/* Enable PLL FSM voting */
+	regmap_update_bits(map, reg, PLL_VOTE_FSM_ENA, PLL_VOTE_FSM_ENA);
+}
+EXPORT_SYMBOL_GPL(qcom_pll_set_fsm_mode);
+
 static void qcom_cc_del_clk_provider(void *data)
 {
 	of_clk_del_provider(data);
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index ae9bdeb..5b12ac0 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -34,6 +34,8 @@ struct qcom_cc_desc {
 
 extern const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f,
 					     unsigned long rate);
+extern void
+qcom_pll_set_fsm_mode(struct regmap *m, u32 reg, u8 bias_count, u8 lock_count);
 extern int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map,
 			       u8 src);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 07/10] clk: qcom: Add support to enable FSM mode for votable alpha PLLs

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index f7c226a..6bf5abd 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -25,6 +25,14 @@
>  #include "reset.h"
>  #include "gdsc.h"
>  
> +#define PLL_LOCK_COUNT_SHIFT	8
> +#define PLL_LOCK_COUNT_MASK	0x3f
> +#define PLL_BIAS_COUNT_SHIFT	14
> +#define PLL_BIAS_COUNT_MASK	0x3f
> +#define PLL_VOTE_FSM_ENA	BIT(20)
> +#define PLL_DYN_FSM_ENA		BIT(20)
> +#define PLL_VOTE_FSM_RESET	BIT(21)

These appear but aren't deleted from anywhere else so I suspect
now we have two copies of these defines somewhere? Perhaps this
should go into common.h as well?

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

Re: [PATCH v2 07/10] clk: qcom: Add support to enable FSM mode for votable alpha PLLs

[Rajendra Nayak]

On 08/24/2016 12:01 PM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index f7c226a..6bf5abd 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -25,6 +25,14 @@
>>  #include "reset.h"
>>  #include "gdsc.h"
>>  
>> +#define PLL_LOCK_COUNT_SHIFT	8
>> +#define PLL_LOCK_COUNT_MASK	0x3f
>> +#define PLL_BIAS_COUNT_SHIFT	14
>> +#define PLL_BIAS_COUNT_MASK	0x3f
>> +#define PLL_VOTE_FSM_ENA	BIT(20)
>> +#define PLL_DYN_FSM_ENA		BIT(20)
>> +#define PLL_VOTE_FSM_RESET	BIT(21)
> 
> These appear but aren't deleted from anywhere else so I suspect
> now we have two copies of these defines somewhere? Perhaps this
> should go into common.h as well?

yes, these are used in clk-pll.c as well, so could as well move them
over to common.h instead of having two copies

> 

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

[PATCH v2 08/10] clk: qcom: Cleanup some macro defs

[Rajendra Nayak]

From: Taniya Das <tdas@codeaurora.org>

Move all
'# define XYZ'
to
'#define XYZ'

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index e8f3505..854487e 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -21,28 +21,28 @@
 #include "common.h"
 
 #define PLL_MODE		0x00
-# define PLL_OUTCTRL		BIT(0)
-# define PLL_BYPASSNL		BIT(1)
-# define PLL_RESET_N		BIT(2)
-# define PLL_LOCK_COUNT_SHIFT	8
-# define PLL_LOCK_COUNT_MASK	0x3f
-# define PLL_BIAS_COUNT_SHIFT	14
-# define PLL_BIAS_COUNT_MASK	0x3f
-# define PLL_VOTE_FSM_ENA	BIT(20)
-# define PLL_VOTE_FSM_RESET	BIT(21)
-# define PLL_ACTIVE_FLAG	BIT(30)
-# define PLL_LOCK_DET		BIT(31)
+#define PLL_OUTCTRL		BIT(0)
+#define PLL_BYPASSNL		BIT(1)
+#define PLL_RESET_N		BIT(2)
+#define PLL_LOCK_COUNT_SHIFT	8
+#define PLL_LOCK_COUNT_MASK	0x3f
+#define PLL_BIAS_COUNT_SHIFT	14
+#define PLL_BIAS_COUNT_MASK	0x3f
+#define PLL_VOTE_FSM_ENA	BIT(20)
+#define PLL_VOTE_FSM_RESET	BIT(21)
+#define PLL_ACTIVE_FLAG	BIT(30)
+#define PLL_LOCK_DET		BIT(31)
 
 #define PLL_L_VAL		0x04
 #define PLL_ALPHA_VAL		0x08
 #define PLL_ALPHA_VAL_U		0x0c
 
 #define PLL_USER_CTL		0x10
-# define PLL_POST_DIV_SHIFT	8
-# define PLL_POST_DIV_MASK	0xf
-# define PLL_ALPHA_EN		BIT(24)
-# define PLL_VCO_SHIFT		20
-# define PLL_VCO_MASK		0x3
+#define PLL_POST_DIV_SHIFT	8
+#define PLL_POST_DIV_MASK	0xf
+#define PLL_ALPHA_EN		BIT(24)
+#define PLL_VCO_SHIFT		20
+#define PLL_VCO_MASK		0x3
 
 #define PLL_USER_CTL_U		0x14
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 08/10] clk: qcom: Cleanup some macro defs

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> From: Taniya Das <tdas@codeaurora.org>
> 
> Move all
> '# define XYZ'
> to
> '#define XYZ'
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

This was done on purpose. Please drop this patch

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

[PATCH v2 09/10] clk: qcom: Add .is_enabled ops for clk-alpha-pll

[Rajendra Nayak]

This would be useful in subsequent patches when the .set_rate operation
would need to identify if the PLL is actually enabled

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 854487e..2184dc1 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -198,6 +198,23 @@ static void clk_alpha_pll_hwfsm_disable(struct clk_hw *hw)
 	wait_for_pll_disable(pll, PLL_ACTIVE_FLAG);
 }
 
+static int clk_alpha_pll_is_enabled(struct clk_hw *hw)
+{
+	int ret;
+	u32 val, off;
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+
+	off = pll->offset;
+	ret = regmap_read(pll->clkr.regmap, off + PLL_MODE, &val);
+	if (ret)
+		return ret;
+
+	if (val & PLL_LOCK_DET)
+		return 1;
+	else
+		return 0;
+}
+
 static int clk_alpha_pll_enable(struct clk_hw *hw)
 {
 	int ret;
@@ -398,6 +415,7 @@ static long clk_alpha_pll_round_rate(struct clk_hw *hw, unsigned long rate,
 const struct clk_ops clk_alpha_pll_ops = {
 	.enable = clk_alpha_pll_enable,
 	.disable = clk_alpha_pll_disable,
+	.is_enabled = clk_alpha_pll_is_enabled,
 	.recalc_rate = clk_alpha_pll_recalc_rate,
 	.round_rate = clk_alpha_pll_round_rate,
 	.set_rate = clk_alpha_pll_set_rate,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 09/10] clk: qcom: Add .is_enabled ops for clk-alpha-pll

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> This would be useful in subsequent patches when the .set_rate operation
> would need to identify if the PLL is actually enabled
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Hmmm I suspect I never implemented the is_enabled op because that
will happen to turn off clks during late init that shouldn't
otherwise be disabled because the framework now can see that some
PLL is enabled out of the bootloader. Is that happening now? We
really should fix the framework to make this not be a problem,
mostly by finishing off the clk handoff patches that Mike posted
a while back. But either way, I'm worried with these patches that
implement is_enabled ops.

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

Re: [PATCH v2 09/10] clk: qcom: Add .is_enabled ops for clk-alpha-pll

[Rajendra Nayak]

On 08/24/2016 11:58 AM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> This would be useful in subsequent patches when the .set_rate operation
>> would need to identify if the PLL is actually enabled
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
> 
> Hmmm I suspect I never implemented the is_enabled op because that
> will happen to turn off clks during late init that shouldn't
> otherwise be disabled because the framework now can see that some
> PLL is enabled out of the bootloader. Is that happening now? We
> really should fix the framework to make this not be a problem,
> mostly by finishing off the clk handoff patches that Mike posted
> a while back. But either way, I'm worried with these patches that
> implement is_enabled ops.

I did not see any, but will try and test some more to see if we indeed
run into such issues

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

[PATCH v2 10/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

[Rajendra Nayak]

From: Taniya Das <tdas@codeaurora.org>

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 don't have to be disabled but need to follow a update
sequence (as implemented by clk_alpha_pll_dynamic_update() in the patch).
They also need the PLL_HW_LOGIC_BYPASS bit set at init.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 48 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-alpha-pll.h |  1 +
 2 files changed, 49 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 2184dc1..68c90f3 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -113,6 +113,11 @@ static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
 #define PLL_OFFLINE_ACK		BIT(28)
 #define PLL_ACTIVE_FLAG		BIT(30)
 
+/* alpha pll with dynamic update support */
+#define PLL_UPDATE		BIT(22)
+#define PLL_HW_LOGIC_BYPASS	BIT(23)
+#define PLL_ACK_LATCH		BIT(29)
+
 void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 			     const struct alpha_pll_config *config)
 {
@@ -138,6 +143,37 @@ void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 	if (pll->flags & SUPPORTS_VOTE_FSM)
 		qcom_pll_set_fsm_mode(regmap, pll->offset + PLL_MODE, 6, 0);
 
+	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
+		regmap_update_bits(regmap, pll->offset + PLL_MODE,
+				   PLL_HW_LOGIC_BYPASS,
+				   PLL_HW_LOGIC_BYPASS);
+}
+
+static int clk_alpha_pll_dynamic_update(struct clk_alpha_pll *pll)
+{
+	u32 val;
+
+	/* Latch the input to the PLL */
+	regmap_update_bits(pll->clkr.regmap, pll->offset + PLL_MODE,
+			   PLL_UPDATE, PLL_UPDATE);
+
+	/* Wait for 2 reference cycle before checking ACK bit */
+	udelay(1);
+
+	regmap_read(pll->clkr.regmap, pll->offset + PLL_MODE, &val);
+	if (!(val & PLL_ACK_LATCH)) {
+		WARN(1, "PLL latch failed. Output may be unstable!\n");
+		return -EINVAL;
+	}
+
+	/* Return latch input to 0 */
+	regmap_update_bits(pll->clkr.regmap, pll->offset + PLL_MODE,
+			   PLL_UPDATE, 0);
+
+	/* Wait for PLL output to stabilize */
+	udelay(100);
+
+	return 0;
 }
 
 static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw)
@@ -366,6 +402,7 @@ 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)
 {
+	int enabled;
 	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
 	const struct pll_vco *vco;
 	u32 l, off = pll->offset;
@@ -378,6 +415,11 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 		return -EINVAL;
 	}
 
+	enabled = hw->init->ops->is_enabled(hw);
+
+	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
+		hw->init->ops->disable(hw);
+
 	a <<= (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
 
 	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);
@@ -391,6 +433,12 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL, PLL_ALPHA_EN,
 			   PLL_ALPHA_EN);
 
+	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
+		hw->init->ops->enable(hw);
+
+	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
+		clk_alpha_pll_dynamic_update(pll);
+
 	return 0;
 }
 
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 4bd42fd..23e32db 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -36,6 +36,7 @@ struct clk_alpha_pll {
 	size_t num_vco;
 
 #define SUPPORTS_VOTE_FSM	BIT(0)
+#define SUPPORTS_DYNAMIC_UPDATE	BIT(1)
 	u8 flags;
 	struct clk_regmap clkr;
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 10/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 2184dc1..68c90f3 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -113,6 +113,11 @@ static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
>  #define PLL_OFFLINE_ACK		BIT(28)
>  #define PLL_ACTIVE_FLAG		BIT(30)
>  
> +/* alpha pll with dynamic update support */
> +#define PLL_UPDATE		BIT(22)
> +#define PLL_HW_LOGIC_BYPASS	BIT(23)
> +#define PLL_ACK_LATCH		BIT(29)

These need to move next to associated registers.

> +
>  void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>  			     const struct alpha_pll_config *config)
>  {
> @@ -366,6 +402,7 @@ 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)
>  {
> +	int enabled;

bool

>  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>  	const struct pll_vco *vco;
>  	u32 l, off = pll->offset;
> @@ -378,6 +415,11 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  		return -EINVAL;
>  	}
>  
> +	enabled = hw->init->ops->is_enabled(hw);

We have clk_hw_is_enabled() for this.

> +
> +	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
> +		hw->init->ops->disable(hw);

Please call the function directly instead of going through the
init structure to get the clk ops.

> +
>  	a <<= (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
>  
>  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);
> @@ -391,6 +433,12 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  	regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL, PLL_ALPHA_EN,
>  			   PLL_ALPHA_EN);
>  
> +	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
> +		hw->init->ops->enable(hw);
> +
> +	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
> +		clk_alpha_pll_dynamic_update(pll);
> +

Perhaps write it as

	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
		clk_alpha_pll_dynamic_update()
	else if (enabled)
		toggle the enable bit...

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

Re: [PATCH v2 10/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

[Rajendra Nayak]

On 08/24/2016 11:56 AM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index 2184dc1..68c90f3 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -113,6 +113,11 @@ static int wait_for_pll_offline(struct clk_alpha_pll *pll, u32 mask)
>>  #define PLL_OFFLINE_ACK		BIT(28)
>>  #define PLL_ACTIVE_FLAG		BIT(30)
>>  
>> +/* alpha pll with dynamic update support */
>> +#define PLL_UPDATE		BIT(22)
>> +#define PLL_HW_LOGIC_BYPASS	BIT(23)
>> +#define PLL_ACK_LATCH		BIT(29)
> 
> These need to move next to associated registers.

will do

> 
>> +
>>  void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>  			     const struct alpha_pll_config *config)
>>  {
>> @@ -366,6 +402,7 @@ 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)
>>  {
>> +	int enabled;
> 
> bool

sure

> 
>>  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>>  	const struct pll_vco *vco;
>>  	u32 l, off = pll->offset;
>> @@ -378,6 +415,11 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>  		return -EINVAL;
>>  	}
>>  
>> +	enabled = hw->init->ops->is_enabled(hw);
> 
> We have clk_hw_is_enabled() for this.

sure, will change it to use clk_hw_is_enabled

> 
>> +
>> +	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
>> +		hw->init->ops->disable(hw);
> 
> Please call the function directly instead of going through the
> init structure to get the clk ops.

okay

> 
>> +
>>  	a <<= (ALPHA_REG_BITWIDTH - ALPHA_BITWIDTH);
>>  
>>  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);
>> @@ -391,6 +433,12 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>  	regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL, PLL_ALPHA_EN,
>>  			   PLL_ALPHA_EN);
>>  
>> +	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
>> +		hw->init->ops->enable(hw);
>> +
>> +	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
>> +		clk_alpha_pll_dynamic_update(pll);
>> +
> 
> Perhaps write it as
> 
> 	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE)
> 		clk_alpha_pll_dynamic_update()
> 	else if (enabled)
> 		toggle the enable bit...

okay will update, thanks
> 

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

Re: [PATCH v2 00/10] clk: qcom: PLL updates

[Stephen Boyd]

On 08/11, Rajendra Nayak wrote:
> Hi,
> 
> This series adds some additional support to the clk-alpha-pll and the
> clk-pll drivers in preperation to add the CPU clock driver support
> on msm8996

It would be nice to see the users of this new code. Can that also
be posted, even if it's just RFC and not for merging at this
time?

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

Re: [PATCH v2 00/10] clk: qcom: PLL updates

[Rajendra Nayak]

On 08/24/2016 11:47 AM, Stephen Boyd wrote:
> On 08/11, Rajendra Nayak wrote:
>> Hi,
>>
>> This series adds some additional support to the clk-alpha-pll and the
>> clk-pll drivers in preperation to add the CPU clock driver support
>> on msm8996
> 
> It would be nice to see the users of this new code. Can that also
> be posted, even if it's just RFC and not for merging at this
> time?

Sure, will do that when I do a respin of this series.
Thanks for the review.

> 

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