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

[PATCH 1/2] 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 | 266 +++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-alpha-pll.h |   9 ++
 2 files changed, 275 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 0ced4a5a9a17..bf36a929458b 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,15 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
 #define FABIA_PLL_OUT_MASK	0x7
 #define FABIA_PLL_RATE_MARGIN	500
 
+#define TRION_PLL_CAL_VAL	0x44
+#define TRION_PLL_STANDBY	0x0
+#define TRION_PLL_RUN		0x1
+#define TRION_PLL_OUT_MASK	0x7
+#define TRION_PCAL_DONE		BIT(26)
+#define TRION_PLL_RATE_MARGIN	500
+
+#define XO_RATE			19200000
+
 #define pll_alpha_width(p)					\
 		((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ?	\
 				 ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH)
@@ -392,6 +421,15 @@ alpha_pll_round_rate(unsigned long rate, unsigned long prate, u32 *l, u64 *a,
 	u64 remainder;
 	u64 quotient;
 
+	/*
+	 * The PLLs parent rate is zero probably since the parent hasn't
+	 * registered yet. Return early with the requested rate.
+	 */
+	if (!prate) {
+		pr_warn("PLLs parent rate hasn't been initialized.\n");
+		return rate;
+	}
+
 	quotient = rate;
 	remainder = do_div(quotient, prate);
 	*l = quotient;
@@ -730,6 +768,136 @@ 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_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 */
+	ret = regmap_update_bits(regmap, PLL_MODE(pll),
+				 PLL_OUTCTRL, PLL_OUTCTRL);
+	if (ret)
+		return ret;
+
+	/* Ensure that the write above goes through before returning. */
+	mb();
+	return ret;
+}
+
+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 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 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 +928,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)
 {
@@ -902,6 +1079,10 @@ static int alpha_pll_fabia_enable(struct clk_hw *hw)
 	ret = regmap_read(regmap, PLL_OPMODE(pll), &opmode_val);
 	if (ret)
 		return ret;
+	ret = regmap_update_bits(regmap, PLL_MODE(pll),
+				 PLL_BYPASSNL, PLL_BYPASSNL);
+	if (ret)
+		return ret;
 
 	/* Skip If PLL is already running */
 	if ((opmode_val & FABIA_OPMODE_RUN) && (val & PLL_OUTCTRL))
@@ -1058,6 +1239,91 @@ 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;
+
+	if (!pll->post_div_table) {
+		pr_err("Missing the post_div_table for the PLL\n");
+		return -EINVAL;
+	}
+
+	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);
+
+	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);
+};
+
+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;
+
+	if (!pll->post_div_table) {
+		pr_err("Missing the post_div_table for the PLL\n");
+		return -EINVAL;
+	}
+
+	div = DIV_ROUND_UP_ULL((u64)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..a2f844c66207 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
 };
 
@@ -43,6 +48,7 @@ struct pll_vco {
 /**
  * struct clk_alpha_pll - phase locked loop (PLL)
  * @offset: base address of registers
+ * @inited: flag that's set when the PLL is initialized
  * @vco_table: array of VCO settings
  * @regs: alpha pll register map (see @clk_alpha_pll_regs)
  * @clkr: regmap clock handle
@@ -50,6 +56,7 @@ struct pll_vco {
 struct clk_alpha_pll {
 	u32 offset;
 	const u8 *regs;
+	bool inited;
 
 	const struct pll_vco *vco_table;
 	size_t num_vco;
@@ -117,5 +124,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 2/2] clk: qcom: gcc: Add global clock controller driver for SM8150

[Stephen Boyd]

Quoting Vinod Koul (2019-06-07 03:12:34)
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> new file mode 100644
> index 000000000000..1cbc884444c9
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> +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 char * const gcc_parent_names_0[] = {

We have a new way of specifying clk parents now. Can you use that
instead of using strings everywhere?

> +       "bi_tcxo",
> +       "gpll0",
> +       "gpll0_out_even",
> +       "core_bi_pll_test_se",

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

[Stephen Boyd]

Quoting Vinod Koul (2019-06-07 03:12:33)
> 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]

This tag isn't very useful. Maybe you can list out what you actually did
instead of just "tidying"?

> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 0ced4a5a9a17..bf36a929458b 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -120,6 +140,15 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
>  #define FABIA_PLL_OUT_MASK     0x7
>  #define FABIA_PLL_RATE_MARGIN  500
>  
> +#define TRION_PLL_CAL_VAL      0x44
> +#define TRION_PLL_STANDBY      0x0
> +#define TRION_PLL_RUN          0x1
> +#define TRION_PLL_OUT_MASK     0x7
> +#define TRION_PCAL_DONE                BIT(26)
> +#define TRION_PLL_RATE_MARGIN  500

These last two aren't used. Do we need them?

> +
> +#define XO_RATE                        19200000

Please remove this define. It isn't used (thankfully!).

> +
>  #define pll_alpha_width(p)                                     \
>                 ((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ? \
>                                  ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH)
> @@ -392,6 +421,15 @@ alpha_pll_round_rate(unsigned long rate, unsigned long prate, u32 *l, u64 *a,
>         u64 remainder;
>         u64 quotient;
>  
> +       /*
> +        * The PLLs parent rate is zero probably since the parent hasn't
> +        * registered yet. Return early with the requested rate.
> +        */
> +       if (!prate) {

This hasn't been a problem before. Why is it a problem now?

> +               pr_warn("PLLs parent rate hasn't been initialized.\n");
> +               return rate;
> +       }
> +
>         quotient = rate;
>         remainder = do_div(quotient, prate);
>         *l = quotient;
> @@ -730,6 +768,136 @@ 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_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 */
> +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> +                                PLL_OUTCTRL, PLL_OUTCTRL);
> +       if (ret)
> +               return ret;

This if (ret) can be removed.

> +
> +       /* Ensure that the write above goes through before returning. */
> +       mb();

As far as I recall mb() does nothing to ensure the write above goes
through, just that writes after this one are ordered with the write
above.

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

Can you move this function right below trion_pll_is_enabled()?

> +
> +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,
> @@ -902,6 +1079,10 @@ static int alpha_pll_fabia_enable(struct clk_hw *hw)
>         ret = regmap_read(regmap, PLL_OPMODE(pll), &opmode_val);
>         if (ret)
>                 return ret;
> +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> +                                PLL_BYPASSNL, PLL_BYPASSNL);
> +       if (ret)
> +               return ret;

What is this?

>  
>         /* Skip If PLL is already running */
>         if ((opmode_val & FABIA_OPMODE_RUN) && (val & PLL_OUTCTRL))
> @@ -1058,6 +1239,91 @@ 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;
> +
> +       if (!pll->post_div_table) {
> +               pr_err("Missing the post_div_table for the PLL\n");

Ahhh!

> +               return -EINVAL;
> +       }
> +
> +       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);
> +
> +       if (!pll->post_div_table) {
> +               pr_err("Missing the post_div_table for the PLL\n");
> +               return -EINVAL;

Does this ever happen? I'd rather see this removed and the code blow up
if the driver author didn't test this.

> +       }
> +
> +       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;
> +
> +       if (!pll->post_div_table) {
> +               pr_err("Missing the post_div_table for the PLL\n");

Again!

> +               return -EINVAL;
> +       }
> +
> +       div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);

Is the cast necessary?

> +       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)
>  {

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

[Vinod Koul]

Thanks Stephen for a quick review

On 07-06-19, 10:55, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-07 03:12:33)
> > 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]
> 
> This tag isn't very useful. Maybe you can list out what you actually did
> instead of just "tidying"?

First was to port and in process remove bunch of code which cant be
upstreamed and has no user in current work. Had to rewrite bunch of
stuff while upstreaming. Then fix format and style issue

I will try to list..

> > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> > index 0ced4a5a9a17..bf36a929458b 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > @@ -120,6 +140,15 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
> >  #define FABIA_PLL_OUT_MASK     0x7
> >  #define FABIA_PLL_RATE_MARGIN  500
> >  
> > +#define TRION_PLL_CAL_VAL      0x44
> > +#define TRION_PLL_STANDBY      0x0
> > +#define TRION_PLL_RUN          0x1
> > +#define TRION_PLL_OUT_MASK     0x7
> > +#define TRION_PCAL_DONE                BIT(26)
> > +#define TRION_PLL_RATE_MARGIN  500
> 
> These last two aren't used. Do we need them?

Not anymore, I removed the user as that was required for this, I will
> 
> > +
> > +#define XO_RATE                        19200000
> 
> Please remove this define. It isn't used (thankfully!).

Yes will remove this and other is the series :)

> >  #define pll_alpha_width(p)                                     \
> >                 ((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ? \
> >                                  ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH)
> > @@ -392,6 +421,15 @@ alpha_pll_round_rate(unsigned long rate, unsigned long prate, u32 *l, u64 *a,
> >         u64 remainder;
> >         u64 quotient;
> >  
> > +       /*
> > +        * The PLLs parent rate is zero probably since the parent hasn't
> > +        * registered yet. Return early with the requested rate.
> > +        */
> > +       if (!prate) {
> 
> This hasn't been a problem before. Why is it a problem now?

Good point, some how downstream thinks so, I will remove these checks

> > +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 */
> > +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> > +                                PLL_OUTCTRL, PLL_OUTCTRL);
> > +       if (ret)
> > +               return ret;
> 
> This if (ret) can be removed.

Yes we should return!

> > +
> > +       /* Ensure that the write above goes through before returning. */
> > +       mb();
> 
> As far as I recall mb() does nothing to ensure the write above goes
> through, just that writes after this one are ordered with the write
> above.

Agreed someone wasn't convinced, will remove this.

> > +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);
> > +}
> 
> Can you move this function right below trion_pll_is_enabled()?

Sure

> >  const struct clk_ops clk_alpha_pll_ops = {
> >         .enable = clk_alpha_pll_enable,
> >         .disable = clk_alpha_pll_disable,
> > @@ -902,6 +1079,10 @@ static int alpha_pll_fabia_enable(struct clk_hw *hw)
> >         ret = regmap_read(regmap, PLL_OPMODE(pll), &opmode_val);
> >         if (ret)
> >                 return ret;
> > +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> > +                                PLL_BYPASSNL, PLL_BYPASSNL);
> > +       if (ret)
> > +               return ret;
> 
> What is this?

Sorry am not sure I understood the question. care to elaborate please?

> > +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);
> > +
> > +       if (!pll->post_div_table) {
> > +               pr_err("Missing the post_div_table for the PLL\n");
> > +               return -EINVAL;
> 
> Does this ever happen? I'd rather see this removed and the code blow up
> if the driver author didn't test this.

Sounds right! will remove this and others.

> > +               return -EINVAL;
> > +       }
> > +
> > +       div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
> 
> Is the cast necessary?

Dont think so, will remove
-- 
~Vinod

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

[Vinod Koul]

On 07-06-19, 10:43, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-07 03:12:34)
> > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > new file mode 100644
> > index 000000000000..1cbc884444c9
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > +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 char * const gcc_parent_names_0[] = {
> 
> We have a new way of specifying clk parents now. Can you use that
> instead of using strings everywhere?

Okay I will update, any pointers on new implementation I can look up?

Thanks
-- 
~Vinod

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

[Stephen Boyd]

Quoting Vinod Koul (2019-06-08 02:14:36)
> On 07-06-19, 10:55, Stephen Boyd wrote:
> > Quoting Vinod Koul (2019-06-07 03:12:33)
> 
> > >  const struct clk_ops clk_alpha_pll_ops = {
> > >         .enable = clk_alpha_pll_enable,
> > >         .disable = clk_alpha_pll_disable,
> > > @@ -902,6 +1079,10 @@ static int alpha_pll_fabia_enable(struct clk_hw *hw)
> > >         ret = regmap_read(regmap, PLL_OPMODE(pll), &opmode_val);
> > >         if (ret)
> > >                 return ret;
> > > +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> > > +                                PLL_BYPASSNL, PLL_BYPASSNL);
> > > +       if (ret)
> > > +               return ret;
> > 
> > What is this?
> 
> Sorry am not sure I understood the question. care to elaborate please?

The bypass bit of a PLL is very generic so I'm confused why the enable
function is only gaining this bit setting logic now. Plus, it's all
grouped together with the previous line so it looks like a possible
stray addition to the code? And after this there's an early exit from
the function if the PLL is already running, so we would put the PLL into
bypass and then return? What's going on here?

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

[Stephen Boyd]

Quoting Vinod Koul (2019-06-08 02:15:37)
> On 07-06-19, 10:43, Stephen Boyd wrote:
> > Quoting Vinod Koul (2019-06-07 03:12:34)
> > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > new file mode 100644
> > > index 000000000000..1cbc884444c9
> > > --- /dev/null
> > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > +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 char * const gcc_parent_names_0[] = {
> > 
> > We have a new way of specifying clk parents now. Can you use that
> > instead of using strings everywhere?
> 
> Okay I will update, any pointers on new implementation I can look up?
> 

Not really yet. Chen-Yu is working on the Allwinner driver
(drivers/clk/sunxi-ng/) and some work from Jeff for MSM8998 is on the
list that you can look at.

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

[Vinod Koul]

On 10-06-19, 08:08, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-08 02:15:37)
> > On 07-06-19, 10:43, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2019-06-07 03:12:34)
> > > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > > new file mode 100644
> > > > index 000000000000..1cbc884444c9
> > > > --- /dev/null
> > > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > > +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 char * const gcc_parent_names_0[] = {
> > > 
> > > We have a new way of specifying clk parents now. Can you use that
> > > instead of using strings everywhere?
> > 
> > Okay I will update, any pointers on new implementation I can look up?
> > 
> 
> Not really yet. Chen-Yu is working on the Allwinner driver
> (drivers/clk/sunxi-ng/) and some work from Jeff for MSM8998 is on the
> list that you can look at.

Thanks for the pointer, I have done the conversion and will post the
update in v2

-- 
~Vinod

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

[Vinod Koul]

On 10-06-19, 08:06, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-08 02:14:36)
> > On 07-06-19, 10:55, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2019-06-07 03:12:33)
> > 
> > > >  const struct clk_ops clk_alpha_pll_ops = {
> > > >         .enable = clk_alpha_pll_enable,
> > > >         .disable = clk_alpha_pll_disable,
> > > > @@ -902,6 +1079,10 @@ static int alpha_pll_fabia_enable(struct clk_hw *hw)
> > > >         ret = regmap_read(regmap, PLL_OPMODE(pll), &opmode_val);
> > > >         if (ret)
> > > >                 return ret;
> > > > +       ret = regmap_update_bits(regmap, PLL_MODE(pll),
> > > > +                                PLL_BYPASSNL, PLL_BYPASSNL);
> > > > +       if (ret)
> > > > +               return ret;
> > > 
> > > What is this?
> > 
> > Sorry am not sure I understood the question. care to elaborate please?
> 
> The bypass bit of a PLL is very generic so I'm confused why the enable
> function is only gaining this bit setting logic now. Plus, it's all
> grouped together with the previous line so it looks like a possible
> stray addition to the code? And after this there's an early exit from
> the function if the PLL is already running, so we would put the PLL into
> bypass and then return? What's going on here?

Thanks for spotting that is wrong. I am not sure why this crept in , I
am not supposed to change this, will fix it in v2

-- 
~Vinod

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

[Rob Herring]

On Fri, Jun 07, 2019 at 03:42:34PM +0530, Vinod Koul wrote:
> 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]
> 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                 | 3649 +++++++++++++++++

>  include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++

Next time, please split bindings to separate patch. 

For the DT parts,

Acked-by: Rob Herring <robh@kernel.org>

>  5 files changed, 3901 insertions(+)
>  create mode 100644 drivers/clk/qcom/gcc-sm8150.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h