Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC
@ 2019-06-25  6:31 Vinod Koul
  2019-06-25  6:31 ` [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks Vinod Koul
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Vinod Koul @ 2019-06-25  6:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Deepak Katragadda,
	Andy Gross, David Brown, Michael Turquette, linux-clk,
	Taniya Das

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks
  2019-06-25  6:31 [PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC Vinod Koul
@ 2019-06-25  6:31 ` Vinod Koul
  2019-06-27 21:37   ` Stephen Boyd
  2019-07-16  0:08   ` Bjorn Andersson
  2019-06-25  6:31 ` [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs Vinod Koul
       [not found] ` <20190625063140.17106-4-vkoul@kernel.org>
  2 siblings, 2 replies; 13+ messages in thread
From: Vinod Koul @ 2019-06-25  6:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Deepak Katragadda,
	Andy Gross, David Brown, Michael Turquette, linux-clk,
	Taniya Das

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs
  2019-06-25  6:31 [PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC Vinod Koul
  2019-06-25  6:31 ` [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks Vinod Koul
@ 2019-06-25  6:31 ` Vinod Koul
  2019-06-27 21:50   ` Stephen Boyd
       [not found] ` <20190625063140.17106-4-vkoul@kernel.org>
  2 siblings, 1 reply; 13+ messages in thread
From: Vinod Koul @ 2019-06-25  6:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das,
	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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150
       [not found] ` <20190625063140.17106-4-vkoul@kernel.org>
@ 2019-06-27 21:31   ` Stephen Boyd
  2019-06-29 13:51     ` Vinod Koul
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2019-06-27 21:31 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das,
	Vinod Koul

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,
> +               },
> +       },
> +};
> +

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks
  2019-06-25  6:31 ` [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks Vinod Koul
@ 2019-06-27 21:37   ` Stephen Boyd
  2019-06-29 11:14     ` Vinod Koul
  2019-07-16  0:08   ` Bjorn Andersson
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2019-06-27 21:37 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Deepak Katragadda,
	Andy Gross, David Brown, Michael Turquette, linux-clk,
	Taniya Das

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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs
  2019-06-25  6:31 ` [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs Vinod Koul
@ 2019-06-27 21:50   ` Stephen Boyd
  2019-06-29 12:28     ` Vinod Koul
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2019-06-27 21:50 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das,
	Vinod Koul

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);
> +}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks
  2019-06-27 21:37   ` Stephen Boyd
@ 2019-06-29 11:14     ` Vinod Koul
  0 siblings, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2019-06-29 11:14 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs
  2019-06-27 21:50   ` Stephen Boyd
@ 2019-06-29 12:28     ` Vinod Koul
  0 siblings, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2019-06-29 12:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150
  2019-06-27 21:31   ` [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150 Stephen Boyd
@ 2019-06-29 13:51     ` Vinod Koul
  2019-07-15 23:01       ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Vinod Koul @ 2019-06-29 13:51 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150
  2019-06-29 13:51     ` Vinod Koul
@ 2019-07-15 23:01       ` Stephen Boyd
  2019-07-16  4:07         ` Vinod Koul
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2019-07-15 23:01 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks
  2019-06-25  6:31 ` [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks Vinod Koul
  2019-06-27 21:37   ` Stephen Boyd
@ 2019-07-16  0:08   ` Bjorn Andersson
  2019-07-16  4:08     ` Vinod Koul
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2019-07-16  0:08 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Stephen Boyd, linux-arm-msm, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150
  2019-07-15 23:01       ` Stephen Boyd
@ 2019-07-16  4:07         ` Vinod Koul
  0 siblings, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2019-07-16  4:07 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-msm, Bjorn Andersson, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks
  2019-07-16  0:08   ` Bjorn Andersson
@ 2019-07-16  4:08     ` Vinod Koul
  0 siblings, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2019-07-16  4:08 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stephen Boyd, linux-arm-msm, Deepak Katragadda, Andy Gross,
	David Brown, Michael Turquette, linux-clk, Taniya Das

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-25  6:31 [PATCH v3 0/3] clk: qcom: Add support for SM8150 GCC Vinod Koul
2019-06-25  6:31 ` [PATCH v3 1/3] clk: qcom: clk-alpha-pll: Remove post_div_table checks Vinod Koul
2019-06-27 21:37   ` Stephen Boyd
2019-06-29 11:14     ` Vinod Koul
2019-07-16  0:08   ` Bjorn Andersson
2019-07-16  4:08     ` Vinod Koul
2019-06-25  6:31 ` [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs Vinod Koul
2019-06-27 21:50   ` Stephen Boyd
2019-06-29 12:28     ` Vinod Koul
     [not found] ` <20190625063140.17106-4-vkoul@kernel.org>
2019-06-27 21:31   ` [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150 Stephen Boyd
2019-06-29 13:51     ` Vinod Koul
2019-07-15 23:01       ` Stephen Boyd
2019-07-16  4:07         ` Vinod Koul

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox