[PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC

[PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC

[Vinod Koul]

This series adds suport for SM8150 GCC which requires Trion PLLs and adds
that as well.

Also fixed some minor nits seen in clk-alpha-pll code and module alias for
qcs404 gcc driver.

Changes in v3:
	- Drop the cast for DIV_ROUND_UP_ULL as that need s afix in macro,
	  so patch it up once core change in merged
	- Drop module alias patch and drop it from sm8150 driver
	- Add review tag by Bjorn

Deepak Katragadda (2):
  clk: qcom: clk-alpha-pll: Add support for Trion PLLs
  clk: qcom: gcc: Add global clock controller driver for SM8150

Vinod Koul (1):
  clk: qcom: clk-alpha-pll: Remove post_div_table checks

 .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
 drivers/clk/qcom/Kconfig                      |    7 +
 drivers/clk/qcom/Makefile                     |    1 +
 drivers/clk/qcom/clk-alpha-pll.c              |  243 +-
 drivers/clk/qcom/clk-alpha-pll.h              |    7 +
 drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
 7 files changed, 4206 insertions(+), 15 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sm8150.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h

-- 
2.20.1

[PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks

[Vinod Koul]

We want users to code properly and fix the post_div_table missing and
not reply on core to check. So remove the post_div_table check.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index b48707693ffd..2c6773188761 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -1036,11 +1036,6 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
 	u32 i, div = 1, val;
 	int ret;
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val);
 	if (ret)
 		return ret;
@@ -1063,11 +1058,6 @@ static long clk_alpha_pll_postdiv_fabia_round_rate(struct clk_hw *hw,
 {
 	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	return divider_round_rate(hw, rate, prate, pll->post_div_table,
 				pll->width, CLK_DIVIDER_ROUND_CLOSEST);
 }
@@ -1089,11 +1079,6 @@ static int clk_alpha_pll_postdiv_fabia_set_rate(struct clk_hw *hw,
 	if (val & PLL_VOTE_FSM_ENA)
 		return 0;
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	div = DIV_ROUND_UP_ULL(parent_rate, rate);
 	for (i = 0; i < pll->num_post_div; i++) {
 		if (pll->post_div_table[i].div == div) {
-- 
2.20.1

Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks

[Stephen Boyd]

Quoting Vinod Koul (2019-06-24 23:31:38)
> We want users to code properly and fix the post_div_table missing and
> not reply on core to check. So remove the post_div_table check.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

This doesn't apply. Not sure why. Can you please format-patch with
--base= so I can know what baseline commit you've based your patches on?
Helps me avoid needing to ask, like right now.

Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks

[Vinod Koul]

On 27-06-19, 14:37, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-24 23:31:38)
> > We want users to code properly and fix the post_div_table missing and
> > not reply on core to check. So remove the post_div_table check.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> 
> This doesn't apply. Not sure why. Can you please format-patch with

I had rebased it on clk-next, but that was v2, let me rebase

> --base= so I can know what baseline commit you've based your patches on?
> Helps me avoid needing to ask, like right now.

Sure will do that and repost after rebase. Also the patch fix for
DIV_ROUND_DOWN_ULL has been merged to mm tree, will add that as well. 

Thanks
-- 
~Vinod

Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks

[Bjorn Andersson]

On Mon 24 Jun 23:31 PDT 2019, Vinod Koul wrote:

> We want users to code properly and fix the post_div_table missing and
> not reply on core to check. So remove the post_div_table check.

s/reply/rely/

> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/clk/qcom/clk-alpha-pll.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index b48707693ffd..2c6773188761 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -1036,11 +1036,6 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
>  	u32 i, div = 1, val;
>  	int ret;
>  
> -	if (!pll->post_div_table) {
> -		pr_err("Missing the post_div_table for the PLL\n");
> -		return -EINVAL;
> -	}
> -
>  	ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val);
>  	if (ret)
>  		return ret;
> @@ -1063,11 +1058,6 @@ static long clk_alpha_pll_postdiv_fabia_round_rate(struct clk_hw *hw,
>  {
>  	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
>  
> -	if (!pll->post_div_table) {
> -		pr_err("Missing the post_div_table for the PLL\n");
> -		return -EINVAL;
> -	}
> -
>  	return divider_round_rate(hw, rate, prate, pll->post_div_table,
>  				pll->width, CLK_DIVIDER_ROUND_CLOSEST);
>  }
> @@ -1089,11 +1079,6 @@ static int clk_alpha_pll_postdiv_fabia_set_rate(struct clk_hw *hw,
>  	if (val & PLL_VOTE_FSM_ENA)
>  		return 0;
>  
> -	if (!pll->post_div_table) {
> -		pr_err("Missing the post_div_table for the PLL\n");
> -		return -EINVAL;
> -	}
> -
>  	div = DIV_ROUND_UP_ULL(parent_rate, rate);
>  	for (i = 0; i < pll->num_post_div; i++) {
>  		if (pll->post_div_table[i].div == div) {
> -- 
> 2.20.1
> 

Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks

[Vinod Koul]

On 15-07-19, 17:08, Bjorn Andersson wrote:
> On Mon 24 Jun 23:31 PDT 2019, Vinod Koul wrote:
> 
> > We want users to code properly and fix the post_div_table missing and
> > not reply on core to check. So remove the post_div_table check.
> 
> s/reply/rely/

Thanks for pointing, will update

-- 
~Vinod

[PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs

[Vinod Koul]

From: Deepak Katragadda <dkatraga@codeaurora.org>

Add programming sequence support for managing the Trion
PLLs.

Signed-off-by: Deepak Katragadda <dkatraga@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
[vkoul: port to upstream and tidy-up]
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 228 +++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-alpha-pll.h |   7 +
 2 files changed, 235 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 2c6773188761..30210f5c6726 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -32,6 +32,7 @@
 # define PLL_LOCK_DET		BIT(31)
 
 #define PLL_L_VAL(p)		((p)->offset + (p)->regs[PLL_OFF_L_VAL])
+#define PLL_CAL_L_VAL(p)	((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
 #define PLL_ALPHA_VAL(p)	((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
 #define PLL_ALPHA_VAL_U(p)	((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
 
@@ -44,14 +45,17 @@
 # define PLL_VCO_MASK		0x3
 
 #define PLL_USER_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U])
+#define PLL_USER_CTL_U1(p)	((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U1])
 
 #define PLL_CONFIG_CTL(p)	((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL])
 #define PLL_CONFIG_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U])
+#define PLL_CONFIG_CTL_U1(p)	((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U11])
 #define PLL_TEST_CTL(p)		((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
 #define PLL_TEST_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
 #define PLL_STATUS(p)		((p)->offset + (p)->regs[PLL_OFF_STATUS])
 #define PLL_OPMODE(p)		((p)->offset + (p)->regs[PLL_OFF_OPMODE])
 #define PLL_FRAC(p)		((p)->offset + (p)->regs[PLL_OFF_FRAC])
+#define PLL_CAL_VAL(p)		((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
 
 const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
 	[CLK_ALPHA_PLL_TYPE_DEFAULT] =  {
@@ -96,6 +100,22 @@ const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
 		[PLL_OFF_OPMODE] = 0x2c,
 		[PLL_OFF_FRAC] = 0x38,
 	},
+	[CLK_ALPHA_PLL_TYPE_TRION] = {
+		[PLL_OFF_L_VAL] = 0x04,
+		[PLL_OFF_CAL_L_VAL] = 0x08,
+		[PLL_OFF_USER_CTL] = 0x0c,
+		[PLL_OFF_USER_CTL_U] = 0x10,
+		[PLL_OFF_USER_CTL_U1] = 0x14,
+		[PLL_OFF_CONFIG_CTL] = 0x18,
+		[PLL_OFF_CONFIG_CTL_U] = 0x1c,
+		[PLL_OFF_CONFIG_CTL_U1] = 0x20,
+		[PLL_OFF_TEST_CTL] = 0x24,
+		[PLL_OFF_TEST_CTL_U] = 0x28,
+		[PLL_OFF_STATUS] = 0x30,
+		[PLL_OFF_OPMODE] = 0x38,
+		[PLL_OFF_ALPHA_VAL] = 0x40,
+		[PLL_OFF_CAL_VAL] = 0x44,
+	},
 };
 EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
 
@@ -120,6 +140,10 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
 #define FABIA_PLL_OUT_MASK	0x7
 #define FABIA_PLL_RATE_MARGIN	500
 
+#define TRION_PLL_STANDBY	0x0
+#define TRION_PLL_RUN		0x1
+#define TRION_PLL_OUT_MASK	0x7
+
 #define pll_alpha_width(p)					\
 		((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ?	\
 				 ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH)
@@ -730,6 +754,130 @@ static long alpha_pll_huayra_round_rate(struct clk_hw *hw, unsigned long rate,
 	return alpha_huayra_pll_round_rate(rate, *prate, &l, &a);
 }
 
+static int trion_pll_is_enabled(struct clk_alpha_pll *pll,
+				struct regmap *regmap)
+{
+	u32 mode_regval, opmode_regval;
+	int ret;
+
+	ret = regmap_read(regmap, PLL_MODE(pll), &mode_regval);
+	ret |= regmap_read(regmap, PLL_OPMODE(pll), &opmode_regval);
+	if (ret)
+		return 0;
+
+	return ((opmode_regval & TRION_PLL_RUN) && (mode_regval & PLL_OUTCTRL));
+}
+
+static int clk_trion_pll_is_enabled(struct clk_hw *hw)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+
+	return trion_pll_is_enabled(pll, pll->clkr.regmap);
+}
+
+static int clk_trion_pll_enable(struct clk_hw *hw)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	struct regmap *regmap = pll->clkr.regmap;
+	u32 val;
+	int ret;
+
+	ret = regmap_read(regmap, PLL_MODE(pll), &val);
+	if (ret)
+		return ret;
+
+	/* If in FSM mode, just vote for it */
+	if (val & PLL_VOTE_FSM_ENA) {
+		ret = clk_enable_regmap(hw);
+		if (ret)
+			return ret;
+		return wait_for_pll_enable_active(pll);
+	}
+
+	/* Set operation mode to RUN */
+	regmap_write(regmap, PLL_OPMODE(pll), TRION_PLL_RUN);
+
+	ret = wait_for_pll_enable_lock(pll);
+	if (ret)
+		return ret;
+
+	/* Enable the PLL outputs */
+	ret = regmap_update_bits(regmap, PLL_USER_CTL(pll),
+				 TRION_PLL_OUT_MASK, TRION_PLL_OUT_MASK);
+	if (ret)
+		return ret;
+
+	/* Enable the global PLL outputs */
+	return regmap_update_bits(regmap, PLL_MODE(pll),
+				 PLL_OUTCTRL, PLL_OUTCTRL);
+}
+
+static void clk_trion_pll_disable(struct clk_hw *hw)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	struct regmap *regmap = pll->clkr.regmap;
+	u32 val;
+	int ret;
+
+	ret = regmap_read(regmap, PLL_MODE(pll), &val);
+	if (ret)
+		return;
+
+	/* If in FSM mode, just unvote it */
+	if (val & PLL_VOTE_FSM_ENA) {
+		clk_disable_regmap(hw);
+		return;
+	}
+
+	/* Disable the global PLL output */
+	ret = regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
+	if (ret)
+		return;
+
+	/* Disable the PLL outputs */
+	ret = regmap_update_bits(regmap, PLL_USER_CTL(pll),
+				 TRION_PLL_OUT_MASK, 0);
+	if (ret)
+		return;
+
+	/* Place the PLL mode in STANDBY */
+	regmap_write(regmap, PLL_OPMODE(pll), TRION_PLL_STANDBY);
+	regmap_update_bits(regmap, PLL_MODE(pll), PLL_RESET_N, PLL_RESET_N);
+}
+
+static unsigned long
+clk_trion_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	struct regmap *regmap = pll->clkr.regmap;
+	u32 l, frac;
+	u64 prate = parent_rate;
+
+	regmap_read(regmap, PLL_L_VAL(pll), &l);
+	regmap_read(regmap, PLL_ALPHA_VAL(pll), &frac);
+
+	return alpha_pll_calc_rate(prate, l, frac, ALPHA_REG_16BIT_WIDTH);
+}
+
+static long clk_trion_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *prate)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	unsigned long min_freq, max_freq;
+	u32 l;
+	u64 a;
+
+	rate = alpha_pll_round_rate(rate, *prate,
+				    &l, &a, ALPHA_REG_16BIT_WIDTH);
+	if (!pll->vco_table || alpha_pll_find_vco(pll, rate))
+		return rate;
+
+	min_freq = pll->vco_table[0].min_freq;
+	max_freq = pll->vco_table[pll->num_vco - 1].max_freq;
+
+	return clamp(rate, min_freq, max_freq);
+}
+
 const struct clk_ops clk_alpha_pll_ops = {
 	.enable = clk_alpha_pll_enable,
 	.disable = clk_alpha_pll_disable,
@@ -760,6 +908,15 @@ const struct clk_ops clk_alpha_pll_hwfsm_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_alpha_pll_hwfsm_ops);
 
+const struct clk_ops clk_trion_fixed_pll_ops = {
+	.enable = clk_trion_pll_enable,
+	.disable = clk_trion_pll_disable,
+	.is_enabled = clk_trion_pll_is_enabled,
+	.recalc_rate = clk_trion_pll_recalc_rate,
+	.round_rate = clk_trion_pll_round_rate,
+};
+EXPORT_SYMBOL_GPL(clk_trion_fixed_pll_ops);
+
 static unsigned long
 clk_alpha_pll_postdiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
@@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
 	return (parent_rate / div);
 }
 
+static unsigned long
+clk_trion_pll_postdiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
+	struct regmap *regmap = pll->clkr.regmap;
+	u32 i, div = 1, val;
+
+	regmap_read(regmap, PLL_USER_CTL(pll), &val);
+
+	val >>= pll->post_div_shift;
+	val &= PLL_POST_DIV_MASK(pll);
+
+	for (i = 0; i < pll->num_post_div; i++) {
+		if (pll->post_div_table[i].val == val) {
+			div = pll->post_div_table[i].div;
+			break;
+		}
+	}
+
+	return (parent_rate / div);
+}
+
+static long
+clk_trion_pll_postdiv_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *prate)
+{
+	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
+
+	return divider_round_rate(hw, rate, prate, pll->post_div_table,
+				  pll->width, CLK_DIVIDER_ROUND_CLOSEST);
+};
+
+static int
+clk_trion_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long parent_rate)
+{
+	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
+	struct regmap *regmap = pll->clkr.regmap;
+	int i, val = 0, div, ret;
+
+	/*
+	 * If the PLL is in FSM mode, then treat the set_rate callback
+	 * as a no-operation.
+	 */
+	ret = regmap_read(regmap, PLL_MODE(pll), &val);
+	if (ret)
+		return ret;
+
+	if (val & PLL_VOTE_FSM_ENA)
+		return 0;
+
+	div = DIV_ROUND_UP_ULL(parent_rate, rate);
+	for (i = 0; i < pll->num_post_div; i++) {
+		if (pll->post_div_table[i].div == div) {
+			val = pll->post_div_table[i].val;
+			break;
+		}
+	}
+
+	return regmap_update_bits(regmap, PLL_USER_CTL(pll),
+				  PLL_POST_DIV_MASK(pll) << PLL_POST_DIV_SHIFT,
+				  val << PLL_POST_DIV_SHIFT);
+}
+
+const struct clk_ops clk_trion_pll_postdiv_ops = {
+	.recalc_rate = clk_trion_pll_postdiv_recalc_rate,
+	.round_rate = clk_trion_pll_postdiv_round_rate,
+	.set_rate = clk_trion_pll_postdiv_set_rate,
+};
+EXPORT_SYMBOL_GPL(clk_trion_pll_postdiv_ops);
+
 static long clk_alpha_pll_postdiv_fabia_round_rate(struct clk_hw *hw,
 				unsigned long rate, unsigned long *prate)
 {
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 66755f0f84fc..15f27f4b06df 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -13,22 +13,27 @@ enum {
 	CLK_ALPHA_PLL_TYPE_HUAYRA,
 	CLK_ALPHA_PLL_TYPE_BRAMMO,
 	CLK_ALPHA_PLL_TYPE_FABIA,
+	CLK_ALPHA_PLL_TYPE_TRION,
 	CLK_ALPHA_PLL_TYPE_MAX,
 };
 
 enum {
 	PLL_OFF_L_VAL,
+	PLL_OFF_CAL_L_VAL,
 	PLL_OFF_ALPHA_VAL,
 	PLL_OFF_ALPHA_VAL_U,
 	PLL_OFF_USER_CTL,
 	PLL_OFF_USER_CTL_U,
+	PLL_OFF_USER_CTL_U1,
 	PLL_OFF_CONFIG_CTL,
 	PLL_OFF_CONFIG_CTL_U,
+	PLL_OFF_CONFIG_CTL_U1,
 	PLL_OFF_TEST_CTL,
 	PLL_OFF_TEST_CTL_U,
 	PLL_OFF_STATUS,
 	PLL_OFF_OPMODE,
 	PLL_OFF_FRAC,
+	PLL_OFF_CAL_VAL,
 	PLL_OFF_MAX_REGS
 };
 
@@ -117,5 +122,7 @@ void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 			     const struct alpha_pll_config *config);
 void clk_fabia_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 				const struct alpha_pll_config *config);
+extern const struct clk_ops clk_trion_fixed_pll_ops;
+extern const struct clk_ops clk_trion_pll_postdiv_ops;
 
 #endif
-- 
2.20.1

Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs

[Stephen Boyd]

Quoting Vinod Koul (2019-06-24 23:31:39)
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 2c6773188761..30210f5c6726 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -32,6 +32,7 @@
>  # define PLL_LOCK_DET          BIT(31)
>  
>  #define PLL_L_VAL(p)           ((p)->offset + (p)->regs[PLL_OFF_L_VAL])
> +#define PLL_CAL_L_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
>  #define PLL_ALPHA_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
>  #define PLL_ALPHA_VAL_U(p)     ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
>  
> @@ -44,14 +45,17 @@
>  # define PLL_VCO_MASK          0x3
>  
>  #define PLL_USER_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U])
> +#define PLL_USER_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U1])
>  
>  #define PLL_CONFIG_CTL(p)      ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL])
>  #define PLL_CONFIG_CTL_U(p)    ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U])
> +#define PLL_CONFIG_CTL_U1(p)   ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U11])

This looks like a typo, U11 vs U1. So I don't think this has been
compile tested....

>  #define PLL_TEST_CTL(p)                ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
>  #define PLL_TEST_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
>  #define PLL_STATUS(p)          ((p)->offset + (p)->regs[PLL_OFF_STATUS])
>  #define PLL_OPMODE(p)          ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
>  #define PLL_FRAC(p)            ((p)->offset + (p)->regs[PLL_OFF_FRAC])
> +#define PLL_CAL_VAL(p)         ((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
>  
>  const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
>         [CLK_ALPHA_PLL_TYPE_DEFAULT] =  {
>  const struct clk_ops clk_alpha_pll_ops = {
>         .enable = clk_alpha_pll_enable,
>         .disable = clk_alpha_pll_disable,
> @@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
[...]
> +
> +static int
> +clk_trion_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +                              unsigned long parent_rate)
> +{
> +       struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
> +       struct regmap *regmap = pll->clkr.regmap;
> +       int i, val = 0, div, ret;
> +
> +       /*
> +        * If the PLL is in FSM mode, then treat the set_rate callback
> +        * as a no-operation.

And this is OK? Shouldn't we fail because we can't change to the rate
that's desired?

> +        */
> +       ret = regmap_read(regmap, PLL_MODE(pll), &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val & PLL_VOTE_FSM_ENA)
> +               return 0;
> +
> +       div = DIV_ROUND_UP_ULL(parent_rate, rate);
> +       for (i = 0; i < pll->num_post_div; i++) {
> +               if (pll->post_div_table[i].div == div) {
> +                       val = pll->post_div_table[i].val;
> +                       break;
> +               }
> +       }
> +
> +       return regmap_update_bits(regmap, PLL_USER_CTL(pll),
> +                                 PLL_POST_DIV_MASK(pll) << PLL_POST_DIV_SHIFT,
> +                                 val << PLL_POST_DIV_SHIFT);
> +}

Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs

[Vinod Koul]

On 27-06-19, 14:50, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-24 23:31:39)
> > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> > index 2c6773188761..30210f5c6726 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > @@ -32,6 +32,7 @@
> >  # define PLL_LOCK_DET          BIT(31)
> >  
> >  #define PLL_L_VAL(p)           ((p)->offset + (p)->regs[PLL_OFF_L_VAL])
> > +#define PLL_CAL_L_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
> >  #define PLL_ALPHA_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
> >  #define PLL_ALPHA_VAL_U(p)     ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
> >  
> > @@ -44,14 +45,17 @@
> >  # define PLL_VCO_MASK          0x3
> >  
> >  #define PLL_USER_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U])
> > +#define PLL_USER_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U1])
> >  
> >  #define PLL_CONFIG_CTL(p)      ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL])
> >  #define PLL_CONFIG_CTL_U(p)    ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U])
> > +#define PLL_CONFIG_CTL_U1(p)   ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U11])
> 
> This looks like a typo, U11 vs U1. So I don't think this has been
> compile tested....

Not sure how this has happened, I have this in my test br too. Will fix
this

> >  #define PLL_TEST_CTL(p)                ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
> >  #define PLL_TEST_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
> >  #define PLL_STATUS(p)          ((p)->offset + (p)->regs[PLL_OFF_STATUS])
> >  #define PLL_OPMODE(p)          ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
> >  #define PLL_FRAC(p)            ((p)->offset + (p)->regs[PLL_OFF_FRAC])
> > +#define PLL_CAL_VAL(p)         ((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
> >  
> >  const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
> >         [CLK_ALPHA_PLL_TYPE_DEFAULT] =  {
> >  const struct clk_ops clk_alpha_pll_ops = {
> >         .enable = clk_alpha_pll_enable,
> >         .disable = clk_alpha_pll_disable,
> > @@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
> [...]
> > +
> > +static int
> > +clk_trion_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long parent_rate)
> > +{
> > +       struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
> > +       struct regmap *regmap = pll->clkr.regmap;
> > +       int i, val = 0, div, ret;
> > +
> > +       /*
> > +        * If the PLL is in FSM mode, then treat the set_rate callback
> > +        * as a no-operation.
> 
> And this is OK? Shouldn't we fail because we can't change to the rate
> that's desired?

Agreed, we should error out.
Also looking at other PLLs I see we check for FSM in clk enabled
and I guess we should do it there for this as well, will move

-- 
~Vinod

Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150

[Stephen Boyd]

Quoting Vinod Koul (2019-06-24 23:31:40)
> From: Deepak Katragadda <dkatraga@codeaurora.org>
> 
> Add the clocks supported in global clock controller which clock the
> peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks
> to the clock framework for the clients to be able to request for them.
> 
> Signed-off-by: Deepak Katragadda <dkatraga@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> [vkoul: port to upstream and tidy-up]

This can say "changed to parent_data scheme"?

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
>  drivers/clk/qcom/Kconfig                      |    7 +
>  drivers/clk/qcom/Makefile                     |    1 +
>  drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
>  include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
>  5 files changed, 3971 insertions(+)
>  create mode 100644 drivers/clk/qcom/gcc-sm8150.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> index 8661c3cd3ccf..1b39d949e63d 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> @@ -23,6 +23,7 @@ Required properties :
>                         "qcom,gcc-sdm630"
>                         "qcom,gcc-sdm660"
>                         "qcom,gcc-sdm845"
> +                       "qcom,gcc-sm8150"
>  
>  - reg : shall contain base register location and length
>  - #clock-cells : shall contain 1

I'd expect the binding to get a larger update indicating which clks come
from outside the clk controller and need to be specified as 'clocks' and
'clock-names'.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 18bdf34d5e64..076872d195fd 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
>           Say Y if you want to use the LPASS branch clocks of the LPASS clock
>           controller to reset the LPASS subsystem.
>  
> +config SM_GCC_8150
> +       tristate "SM8150 Global Clock Controller"
> +       help
> +         Support for the global clock controller on SM8150 devices.
> +         Say Y if you want to use peripheral devices such as UART,
> +         SPI, I2C, USB, SD/eMMC, PCIe etc.

Is there eMMC support?

> +
>  config SPMI_PMIC_CLKDIV
>         tristate "SPMI PMIC clkdiv Support"
>         depends on SPMI || COMPILE_TEST
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index f0768fb1f037..4a813b4055d0 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
>  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
>  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
>  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
>  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
>  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
>  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> new file mode 100644
> index 000000000000..11cd9e19f18d
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -0,0 +1,3719 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-19, The Linux Foundation. All rights reserved.

Is it really 2017-19 and not 2017-2019?

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-sm8150.h>
> +
> +#include "common.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "reset.h"
> +
> +#define GCC_NPU_MISC                           0x4d110
> +#define GCC_GPU_MISC                           0x71028

Drop these defines please and inline the values.

> +
> +enum {
> +       P_BI_TCXO,
> +       P_AUD_REF_CLK,
> +       P_CORE_BI_PLL_TEST_SE,
> +       P_GPLL0_OUT_EVEN,
> +       P_GPLL0_OUT_MAIN,
> +       P_GPLL4_OUT_MAIN,
> +       P_GPLL7_OUT_MAIN,
> +       P_GPLL9_OUT_MAIN,
> +       P_SLEEP_CLK,
> +};
> +
> +static const struct parent_map gcc_parent_map_0[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_0[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct clk_parent_data gcc_parents_0_ao[] = {
> +       { .fw_name = "bi_tcxo_ao", .name = "bi_tcxo_ao" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_1[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_SLEEP_CLK, 5 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_1[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_2[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_SLEEP_CLK, 5 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_2[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_3[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_3[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_4[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_4[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_5[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_GPLL7_OUT_MAIN, 3 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_5[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll7", .name = "gpll7" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },

Aren't these gplls all created in this file? They shouldn't be listed in
DT so I'm confused why we have .fw_name for them.

> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },

If there isn't a legacy string name for core_bi_pll_test_se we should
just leave it out and not have a .name field, just the .fw_name one.

> +};
> +
> +static const struct parent_map gcc_parent_map_6[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_GPLL9_OUT_MAIN, 2 },
> +       { P_GPLL4_OUT_MAIN, 5 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_6[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll9", .name = "gpll9" },
> +       { .fw_name = "gpll4", .name = "gpll4" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_7[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_AUD_REF_CLK, 2 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_7[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static struct pll_vco trion_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
> +static struct clk_alpha_pll gpll0 = {
> +       .offset = 0x0,
> +       .vco_table = trion_vco,
> +       .num_vco = ARRAY_SIZE(trion_vco),
> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> +       .clkr = {
> +               .enable_reg = 0x52000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gpll0",
> +                       .parent_data = &(const struct clk_parent_data){
> +                               .fw_name = "bi_tcxo",
> +                               .name = "bi_tcxo",
> +                       },
> +                       .num_parents = 1,
> +                       .ops = &clk_trion_fixed_pll_ops,
> +               },
> +       },
> +};
> +
> +static const struct clk_div_table post_div_table_trion_even[] = {
> +       { 0x0, 1 },
> +       { 0x1, 2 },
> +       { 0x3, 4 },
> +       { 0x7, 8 },
> +       { }
> +};
> +
> +static struct clk_alpha_pll_postdiv gpll0_out_even = {
> +       .offset = 0x0,
> +       .post_div_shift = 8,
> +       .post_div_table = post_div_table_trion_even,
> +       .num_post_div = ARRAY_SIZE(post_div_table_trion_even),
> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> +       .width = 4,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "gpll0_out_even",
> +               .parent_data = &(const struct clk_parent_data){
> +                       .fw_name = "gpll0",
> +                       .name = "gpll0",

Use a direct pointer? This applies in many places in the code.

> +               },
> +               .num_parents = 1,
> +               .ops = &clk_trion_pll_postdiv_ops,
> +       },
> +};
> +
[...]
> +
> +static struct clk_branch gcc_video_ahb_clk = {
> +       .halt_reg = 0xb004,
> +       .halt_check = BRANCH_HALT_DELAY,
> +       .hwcg_reg = 0xb004,
> +       .hwcg_bit = 1,
> +       .clkr = {
> +               .enable_reg = 0xb004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_ahb_clk",
> +                       .flags = CLK_IS_CRITICAL,

Can you please add a comment around all CLK_IS_CRITICAL flags indicating
why these clks can't be turned off?

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axi0_clk = {
> +       .halt_reg = 0xb024,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0xb024,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_axi0_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axi1_clk = {
> +       .halt_reg = 0xb028,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0xb028,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_axi1_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axic_clk = {
> +       .halt_reg = 0xb02c,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0xb02c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_axic_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_xo_clk = {
> +       .halt_reg = 0xb040,
> +       .halt_check = BRANCH_HALT_DELAY,

Why doesn't the halt bit work?

> +       .clkr = {
> +               .enable_reg = 0xb040,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_xo_clk",
> +                       .flags = CLK_IS_CRITICAL,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +

Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150

[Vinod Koul]

On 27-06-19, 14:31, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-24 23:31:40)
> > From: Deepak Katragadda <dkatraga@codeaurora.org>
> > 
> > Add the clocks supported in global clock controller which clock the
> > peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks
> > to the clock framework for the clients to be able to request for them.
> > 
> > Signed-off-by: Deepak Katragadda <dkatraga@codeaurora.org>
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > [vkoul: port to upstream and tidy-up]
> 
> This can say "changed to parent_data scheme"?
> 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
> >  drivers/clk/qcom/Kconfig                      |    7 +
> >  drivers/clk/qcom/Makefile                     |    1 +
> >  drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
> >  include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
> >  5 files changed, 3971 insertions(+)
> >  create mode 100644 drivers/clk/qcom/gcc-sm8150.c
> >  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > index 8661c3cd3ccf..1b39d949e63d 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > @@ -23,6 +23,7 @@ Required properties :
> >                         "qcom,gcc-sdm630"
> >                         "qcom,gcc-sdm660"
> >                         "qcom,gcc-sdm845"
> > +                       "qcom,gcc-sm8150"
> >  
> >  - reg : shall contain base register location and length
> >  - #clock-cells : shall contain 1
> 
> I'd expect the binding to get a larger update indicating which clks come
> from outside the clk controller and need to be specified as 'clocks' and
> 'clock-names'.
> 
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index 18bdf34d5e64..076872d195fd 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> >           controller to reset the LPASS subsystem.
> >  
> > +config SM_GCC_8150
> > +       tristate "SM8150 Global Clock Controller"
> > +       help
> > +         Support for the global clock controller on SM8150 devices.
> > +         Say Y if you want to use peripheral devices such as UART,
> > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> 
> Is there eMMC support?
> 
> > +
> >  config SPMI_PMIC_CLKDIV
> >         tristate "SPMI PMIC clkdiv Support"
> >         depends on SPMI || COMPILE_TEST
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index f0768fb1f037..4a813b4055d0 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > new file mode 100644
> > index 000000000000..11cd9e19f18d
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > @@ -0,0 +1,3719 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2017-19, The Linux Foundation. All rights reserved.
> 
> Is it really 2017-19 and not 2017-2019?

will update

> 
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/clk.h>
> 
> Is this include used?

will check and update

> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include <dt-bindings/clock/qcom,gcc-sm8150.h>
> > +
> > +#include "common.h"
> > +#include "clk-alpha-pll.h"
> > +#include "clk-branch.h"
> > +#include "clk-pll.h"
> > +#include "clk-rcg.h"
> > +#include "clk-regmap.h"
> > +#include "reset.h"
> > +
> > +#define GCC_NPU_MISC                           0x4d110
> > +#define GCC_GPU_MISC                           0x71028
> 
> Drop these defines please and inline the values.

Sure

> > +static const struct clk_parent_data gcc_parents_5[] = {
> > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > +       { .fw_name = "gpll0", .name = "gpll0" },
> > +       { .fw_name = "gpll7", .name = "gpll7" },
> > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> 
> Aren't these gplls all created in this file? They shouldn't be listed in
> DT so I'm confused why we have .fw_name for them.

Yes they are and the DT doesnt provide these clock. From what I
understood from the name conversion to new schema was we should add it
like above, let me know if I missed something

> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> 
> If there isn't a legacy string name for core_bi_pll_test_se we should
> just leave it out and not have a .name field, just the .fw_name one.

Okay

> > +static const struct clk_parent_data gcc_parents_7[] = {
> > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > +       { .fw_name = "gpll0", .name = "gpll0" },
> > +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },
> > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +};
> > +
> > +static struct pll_vco trion_vco[] = {
> 
> const?

will add

> > +static struct clk_alpha_pll_postdiv gpll0_out_even = {
> > +       .offset = 0x0,
> > +       .post_div_shift = 8,
> > +       .post_div_table = post_div_table_trion_even,
> > +       .num_post_div = ARRAY_SIZE(post_div_table_trion_even),
> > +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> > +       .width = 4,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "gpll0_out_even",
> > +               .parent_data = &(const struct clk_parent_data){
> > +                       .fw_name = "gpll0",
> > +                       .name = "gpll0",
> 
> Use a direct pointer? This applies in many places in the code.

okay will check here and other places

> > +static struct clk_branch gcc_video_ahb_clk = {
> > +       .halt_reg = 0xb004,
> > +       .halt_check = BRANCH_HALT_DELAY,
> > +       .hwcg_reg = 0xb004,
> > +       .hwcg_bit = 1,
> > +       .clkr = {
> > +               .enable_reg = 0xb004,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_video_ahb_clk",
> > +                       .flags = CLK_IS_CRITICAL,
> 
> Can you please add a comment around all CLK_IS_CRITICAL flags indicating
> why these clks can't be turned off?

okay will do

> > +static struct clk_branch gcc_video_axic_clk = {
> > +       .halt_reg = 0xb02c,
> > +       .halt_check = BRANCH_HALT,
> > +       .clkr = {
> > +               .enable_reg = 0xb02c,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_video_axic_clk",
> > +                       .ops = &clk_branch2_ops,
> > +               },
> > +       },
> > +};
> > +
> > +static struct clk_branch gcc_video_xo_clk = {
> > +       .halt_reg = 0xb040,
> > +       .halt_check = BRANCH_HALT_DELAY,
> 
> Why doesn't the halt bit work?

Not sure, will check and update

-- 
~Vinod

Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150

[Stephen Boyd]

Quoting Vinod Koul (2019-06-29 06:51:19)
> On 27-06-19, 14:31, Stephen Boyd wrote:
> > Quoting Vinod Koul (2019-06-24 23:31:40)
> > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > index 18bdf34d5e64..076872d195fd 100644
> > > --- a/drivers/clk/qcom/Kconfig
> > > +++ b/drivers/clk/qcom/Kconfig
> > > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> > >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> > >           controller to reset the LPASS subsystem.
> > >  
> > > +config SM_GCC_8150
> > > +       tristate "SM8150 Global Clock Controller"
> > > +       help
> > > +         Support for the global clock controller on SM8150 devices.
> > > +         Say Y if you want to use peripheral devices such as UART,
> > > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> > 
> > Is there eMMC support?
> > 

I guess no?

> > > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > > index f0768fb1f037..4a813b4055d0 100644
> > > --- a/drivers/clk/qcom/Makefile
> > > +++ b/drivers/clk/qcom/Makefile
> > > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> > >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> > >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> > >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> > >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> > >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> > >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > new file mode 100644
> > > index 000000000000..11cd9e19f18d
> > > --- /dev/null
> > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > +static const struct clk_parent_data gcc_parents_5[] = {
> > > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > > +       { .fw_name = "gpll0", .name = "gpll0" },
> > > +       { .fw_name = "gpll7", .name = "gpll7" },
> > > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > 
> > Aren't these gplls all created in this file? They shouldn't be listed in
> > DT so I'm confused why we have .fw_name for them.
> 
> Yes they are and the DT doesnt provide these clock. From what I
> understood from the name conversion to new schema was we should add it
> like above, let me know if I missed something
> 

Yes, you should use the direct clk_hw pointer part of the
clk_parent_data structure instead of having a .fw_name or .name member
in these rows. Any clk inside of the clk controller shouldn't be exposed
into DT just so that we can find it again through string comparisons.

Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150

[Vinod Koul]

On 15-07-19, 16:01, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-29 06:51:19)
> > On 27-06-19, 14:31, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2019-06-24 23:31:40)
> > > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > > index 18bdf34d5e64..076872d195fd 100644
> > > > --- a/drivers/clk/qcom/Kconfig
> > > > +++ b/drivers/clk/qcom/Kconfig
> > > > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> > > >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> > > >           controller to reset the LPASS subsystem.
> > > >  
> > > > +config SM_GCC_8150
> > > > +       tristate "SM8150 Global Clock Controller"
> > > > +       help
> > > > +         Support for the global clock controller on SM8150 devices.
> > > > +         Say Y if you want to use peripheral devices such as UART,
> > > > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> > > 
> > > Is there eMMC support?
> 
> I guess no?

Nope, copy paste error, have fixed it up :)

> 
> > > > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > > > index f0768fb1f037..4a813b4055d0 100644
> > > > --- a/drivers/clk/qcom/Makefile
> > > > +++ b/drivers/clk/qcom/Makefile
> > > > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> > > >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> > > >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> > > >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > > > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> > > >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> > > >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> > > >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > > new file mode 100644
> > > > index 000000000000..11cd9e19f18d
> > > > --- /dev/null
> > > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > > +static const struct clk_parent_data gcc_parents_5[] = {
> > > > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > > > +       { .fw_name = "gpll0", .name = "gpll0" },
> > > > +       { .fw_name = "gpll7", .name = "gpll7" },
> > > > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > > 
> > > Aren't these gplls all created in this file? They shouldn't be listed in
> > > DT so I'm confused why we have .fw_name for them.
> > 
> > Yes they are and the DT doesnt provide these clock. From what I
> > understood from the name conversion to new schema was we should add it
> > like above, let me know if I missed something
> > 
> 
> Yes, you should use the direct clk_hw pointer part of the
> clk_parent_data structure instead of having a .fw_name or .name member
> in these rows. Any clk inside of the clk controller shouldn't be exposed
> into DT just so that we can find it again through string comparisons.

Thanks, I think I have fixed it up. The parent clk in DT would be cxo
and sleep_clk and creating bi_tcxo,  gpll0, gpll7 etc in driver

I have updated the patchset and will post shortly

Thanks
-- 
~Vinod