[PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

[PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

[Jian Hu via B4 Relay]

This series refactors the Meson PLL framework to remove the dedicated
PLL pre-divider (N) parameter from the PLL implementation and model it
as a separate divider clock.

Currently, the Meson PLL framework models the PLL pre-divider using a
dedicated n field in struct meson_clk_pll_data. This makes the
pre-divider part of the PLL-specific implementation, although the
Common Clock Framework already provides a generic divider clock.

This series separates the pre-divider from the PLL and makes the PLL
DCO take the pre-divider clock as its parent. This allows the
pre-divider to be modeled using the standard CCF divider implementation
and simplifies the PLL framework.

The series currently covers T7 as an RFC to get feedback on the
framework design before applying the same approach to other SoCs.

Series:
clk: meson: pll: Remove the dedicated n parameter
dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
clk: meson: t7: Model PLL pre-divider as a divider clock

The other Meson SoCs will be converted separately after the T7 PLL
framework refactoring has been reviewed and the overall approach is
agreed upon.

Any feedback on the proposed clock hierarchy and the separation of the
PLL pre-divider from the PLL itself would be appreciated.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
Jian Hu (3):
      clk: meson: pll: Remove the dedicated n parameter
      dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
      clk: meson: t7: Model PLL pre-divider as a divider clock

 drivers/clk/meson/clk-pll.c                     | 178 +++++------------------
 drivers/clk/meson/clk-pll.h                     |  13 --
 drivers/clk/meson/t7-pll.c                      | 183 ++++++++++++++++++------
 include/dt-bindings/clock/amlogic,t7-pll-clkc.h |   6 +
 4 files changed, 181 insertions(+), 199 deletions(-)
---
base-commit: 43e1705ecab981c66baee89041e6f728c0436f19
change-id: 20260923-meson_refactor_n-e7f25904e536

Best regards,
-- 
Jian Hu <jian.hu@amlogic.com>

[PATCH RFC 1/3] clk: meson: pll: Remove the dedicated n parameter

[Jian Hu via B4 Relay]

From: Jian Hu <jian.hu@amlogic.com>

The Meson PLL framework models the PLL pre-divider (N) using a dedicated
field in struct meson_clk_pll_data. Since the common clock framework
already provides a generic divider implementation, there is no need to
keep PLL-specific support for it.

Remove the dedicated n parameter from the framework and simplify the PLL
rate calculation accordingly. The pre-divider will be represented as a
separate divider clock by platform drivers where needed.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
 drivers/clk/meson/clk-pll.c | 178 +++++++++-----------------------------------
 drivers/clk/meson/clk-pll.h |  13 ----
 2 files changed, 34 insertions(+), 157 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 1ea6579a760f..de56d55e3b79 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -14,7 +14,7 @@
  *        +--------------------------------+
  *        |                                |
  *        |             +--+               |
- *  in >>-----[ /N ]--->|  |      +-----+  |
+ *  in >>----------->|  |      +-----+  |
  *        |             |  |------| DCO |---->> out
  *        |  +--------->|  |      +--v--+  |
  *        |  |          +--+         |     |
@@ -23,7 +23,7 @@
  *        |                                |
  *        +--------------------------------+
  *
- * out = in * (m + frac / frac_max) / n
+ * out = in * (m + frac / frac_max)
  */
 
 #include <linux/clk-provider.h>
@@ -52,8 +52,7 @@ static int __pll_round_closest_mult(struct meson_clk_pll_data *pll)
 }
 
 static unsigned long __pll_params_to_rate(unsigned long parent_rate,
-					  unsigned int m, unsigned int n,
-					  unsigned int frac,
+					  unsigned int m, unsigned int frac,
 					  struct meson_clk_pll_data *pll)
 {
 	u64 rate = (u64)parent_rate * m;
@@ -66,7 +65,7 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate,
 		rate += DIV_ROUND_UP_ULL(frac_rate, frac_max);
 	}
 
-	return DIV_ROUND_UP_ULL(rate, n);
+	return rate;
 }
 
 static unsigned long meson_clk_pll_recalc_rate(struct clk_hw *hw,
@@ -74,17 +73,7 @@ static unsigned long meson_clk_pll_recalc_rate(struct clk_hw *hw,
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-	unsigned int m, n, frac;
-
-	n = meson_parm_read(clk->map, &pll->n);
-
-	/*
-	 * On some HW, N is set to zero on init. This value is invalid as
-	 * it would result in a division by zero. The rate can't be
-	 * calculated in this case
-	 */
-	if (n == 0)
-		return 0;
+	unsigned int m, frac;
 
 	m = meson_parm_read(clk->map, &pll->m);
 
@@ -92,21 +81,20 @@ static unsigned long meson_clk_pll_recalc_rate(struct clk_hw *hw,
 		meson_parm_read(clk->map, &pll->frac) :
 		0;
 
-	return __pll_params_to_rate(parent_rate, m, n, frac, pll);
+	return __pll_params_to_rate(parent_rate, m, frac, pll);
 }
 
 static unsigned int __pll_params_with_frac(unsigned long rate,
 					   unsigned long parent_rate,
 					   unsigned int m,
-					   unsigned int n,
 					   struct meson_clk_pll_data *pll)
 {
 	unsigned int frac_max = pll->frac_max ? pll->frac_max :
 						(1 << pll->frac.width);
-	u64 val = (u64)rate * n;
+	u64 val = (u64)rate;
 
 	/* Bail out if we are already over the requested rate */
-	if (rate < parent_rate * m / n)
+	if (rate < parent_rate * m)
 		return 0;
 
 	if (pll->flags & CLK_MESON_PLL_ROUND_CLOSEST)
@@ -119,145 +107,48 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
 	return min((unsigned int)val, (frac_max - 1));
 }
 
-static bool meson_clk_pll_is_better(unsigned long rate,
-				    unsigned long best,
-				    unsigned long now,
-				    struct meson_clk_pll_data *pll)
-{
-	if (__pll_round_closest_mult(pll)) {
-		/* Round Closest */
-		if (abs(now - rate) < abs(best - rate))
-			return true;
-	} else {
-		/* Round down */
-		if (now <= rate && best < now)
-			return true;
-	}
-
-	return false;
-}
-
-static int meson_clk_get_pll_table_index(unsigned int index,
-					 unsigned int *m,
-					 unsigned int *n,
-					 struct meson_clk_pll_data *pll)
+static int meson_clk_get_pll_multiplier(unsigned long rate,
+					unsigned long parent_rate,
+					unsigned int *m,
+					struct meson_clk_pll_data *pll)
 {
-	if (!pll->table[index].n)
+	if (!pll->range)
 		return -EINVAL;
 
-	*m = pll->table[index].m;
-	*n = pll->table[index].n;
-
-	return 0;
-}
-
-static unsigned int meson_clk_get_pll_range_m(unsigned long rate,
-					      unsigned long parent_rate,
-					      unsigned int n,
-					      struct meson_clk_pll_data *pll)
-{
-	u64 val = (u64)rate * n;
-
-	if (__pll_round_closest_mult(pll))
-		return DIV_ROUND_CLOSEST_ULL(val, parent_rate);
-
-	return div_u64(val,  parent_rate);
-}
-
-static int meson_clk_get_pll_range_index(unsigned long rate,
-					 unsigned long parent_rate,
-					 unsigned int index,
-					 unsigned int *m,
-					 unsigned int *n,
-					 struct meson_clk_pll_data *pll)
-{
-	*n = index + 1;
-
-	/* Check the predivider range */
-	if (*n >= (1 << pll->n.width))
-		return -EINVAL;
-
-	if (*n == 1) {
-		/* Get the boundaries out the way */
-		if (rate <= pll->range->min * parent_rate) {
-			*m = pll->range->min;
-			return -ENODATA;
-		} else if (rate >= pll->range->max * parent_rate) {
-			*m = pll->range->max;
-			return -ENODATA;
-		}
+	if (rate <= pll->range->min * parent_rate) {
+		*m = pll->range->min;
+		return -ENODATA;
+	} else if (rate >= pll->range->max * parent_rate) {
+		*m = pll->range->max;
+		return -ENODATA;
 	}
 
-	*m = meson_clk_get_pll_range_m(rate, parent_rate, *n, pll);
+	if (__pll_round_closest_mult(pll))
+		*m = DIV_ROUND_CLOSEST_ULL(rate, parent_rate);
+	else
+		*m = div_u64(rate, parent_rate);
 
-	/* the pre-divider gives a multiplier too big - stop */
+	/* Multiplier exceeds hardware range */
 	if (*m >= (1 << pll->m.width))
 		return -EINVAL;
 
 	return 0;
 }
 
-static int meson_clk_get_pll_get_index(unsigned long rate,
-				       unsigned long parent_rate,
-				       unsigned int index,
-				       unsigned int *m,
-				       unsigned int *n,
-				       struct meson_clk_pll_data *pll)
-{
-	if (pll->range)
-		return meson_clk_get_pll_range_index(rate, parent_rate,
-						     index, m, n, pll);
-	else if (pll->table)
-		return meson_clk_get_pll_table_index(index, m, n, pll);
-
-	return -EINVAL;
-}
-
-static int meson_clk_get_pll_settings(unsigned long rate,
-				      unsigned long parent_rate,
-				      unsigned int *best_m,
-				      unsigned int *best_n,
-				      struct meson_clk_pll_data *pll)
-{
-	unsigned long best = 0, now = 0;
-	unsigned int i, m, n;
-	int ret;
-
-	for (i = 0, ret = 0; !ret; i++) {
-		ret = meson_clk_get_pll_get_index(rate, parent_rate,
-						  i, &m, &n, pll);
-		if (ret == -EINVAL)
-			break;
-
-		now = __pll_params_to_rate(parent_rate, m, n, 0, pll);
-		if (meson_clk_pll_is_better(rate, best, now, pll)) {
-			best = now;
-			*best_m = m;
-			*best_n = n;
-
-			if (now == rate)
-				break;
-		}
-	}
-
-	return best ? 0 : -EINVAL;
-}
-
 static int meson_clk_pll_determine_rate(struct clk_hw *hw,
 					struct clk_rate_request *req)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-	unsigned int m, n, frac;
+	unsigned int m, frac;
 	unsigned long round;
 	int ret;
 
-	ret = meson_clk_get_pll_settings(req->rate, req->best_parent_rate,
-					 &m, &n, pll);
-	if (ret)
+	ret = meson_clk_get_pll_multiplier(req->rate, req->best_parent_rate, &m, pll);
+	if (ret && ret != -ENODATA)
 		return ret;
 
-	round = __pll_params_to_rate(req->best_parent_rate, m, n, 0, pll);
+	round = __pll_params_to_rate(req->best_parent_rate, m, 0, pll);
 
 	if (!MESON_PARM_APPLICABLE(&pll->frac) || req->rate == round) {
 		req->rate = round;
@@ -268,8 +159,8 @@ static int meson_clk_pll_determine_rate(struct clk_hw *hw,
 	 * The rate provided by the setting is not an exact match, let's
 	 * try to improve the result using the fractional parameter
 	 */
-	frac = __pll_params_with_frac(req->rate, req->best_parent_rate, m, n, pll);
-	req->rate = __pll_params_to_rate(req->best_parent_rate, m, n, frac, pll);
+	frac = __pll_params_with_frac(req->rate, req->best_parent_rate, m, pll);
+	req->rate = __pll_params_to_rate(req->best_parent_rate, m, frac, pll);
 
 	return 0;
 }
@@ -420,7 +311,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-	unsigned int enabled, m, n, frac = 0;
+	unsigned int enabled, m, frac = 0;
 	unsigned long old_rate;
 	int ret;
 
@@ -429,19 +320,18 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	old_rate = clk_hw_get_rate(hw);
 
-	ret = meson_clk_get_pll_settings(rate, parent_rate, &m, &n, pll);
-	if (ret)
+	ret = meson_clk_get_pll_multiplier(rate, parent_rate, &m, pll);
+	if (ret && ret != -ENODATA)
 		return ret;
 
 	enabled = meson_parm_read(clk->map, &pll->en);
 	if (enabled)
 		meson_clk_pll_disable(hw);
 
-	meson_parm_write(clk->map, &pll->n, n);
 	meson_parm_write(clk->map, &pll->m, m);
 
 	if (MESON_PARM_APPLICABLE(&pll->frac)) {
-		frac = __pll_params_with_frac(rate, parent_rate, m, n, pll);
+		frac = __pll_params_with_frac(rate, parent_rate, m, pll);
 		meson_parm_write(clk->map, &pll->frac, frac);
 	}
 
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index 949157fb7bf5..46feee72830f 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -11,29 +11,17 @@
 #include <linux/regmap.h>
 #include "parm.h"
 
-struct pll_params_table {
-	unsigned int	m;
-	unsigned int	n;
-};
-
 struct pll_mult_range {
 	unsigned int	min;
 	unsigned int	max;
 };
 
-#define PLL_PARAMS(_m, _n)						\
-	{								\
-		.m		= (_m),					\
-		.n		= (_n),					\
-	}
-
 #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
 #define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
 
 struct meson_clk_pll_data {
 	struct parm en;
 	struct parm m;
-	struct parm n;
 	struct parm frac;
 	struct parm l;
 	struct parm rst;
@@ -41,7 +29,6 @@ struct meson_clk_pll_data {
 	struct parm l_detect;
 	const struct reg_sequence *init_regs;
 	unsigned int init_count;
-	const struct pll_params_table *table;
 	const struct pll_mult_range *range;
 	unsigned int frac_max;
 	u8 flags;

-- 
2.47.1

[PATCH RFC 2/3] dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs

[Jian Hu via B4 Relay]

From: Jian Hu <jian.hu@amlogic.com>

Add clock IDs for the T7 PLL pre-divider clocks. These IDs are
required for the subsequent conversion of the PLL pre-divider
into a standalone divider clock following the PLL framework refactoring.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
 include/dt-bindings/clock/amlogic,t7-pll-clkc.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/dt-bindings/clock/amlogic,t7-pll-clkc.h b/include/dt-bindings/clock/amlogic,t7-pll-clkc.h
index e2481f2f1163..69c90bc0be0a 100644
--- a/include/dt-bindings/clock/amlogic,t7-pll-clkc.h
+++ b/include/dt-bindings/clock/amlogic,t7-pll-clkc.h
@@ -9,20 +9,24 @@
 /* GP0 */
 #define CLKID_GP0_PLL_DCO	0
 #define CLKID_GP0_PLL		1
+#define CLKID_GP0_PREDIV	2
 
 /* GP1 */
 #define CLKID_GP1_PLL_DCO	0
 #define CLKID_GP1_PLL		1
+#define CLKID_GP1_PREDIV	2
 
 /* HIFI */
 #define CLKID_HIFI_PLL_DCO	0
 #define CLKID_HIFI_PLL		1
+#define CLKID_HIFI_PREDIV	2
 
 /* PCIE */
 #define CLKID_PCIE_PLL_DCO	0
 #define CLKID_PCIE_PLL_DCO_DIV2	1
 #define CLKID_PCIE_PLL_OD	2
 #define CLKID_PCIE_PLL		3
+#define CLKID_PCIE_PREDIV	4
 
 /* MPLL */
 #define CLKID_MPLL_PREDIV	0
@@ -39,6 +43,7 @@
 #define CLKID_HDMI_PLL_DCO	0
 #define CLKID_HDMI_PLL_OD	1
 #define CLKID_HDMI_PLL		2
+#define CLKID_HDMI_PREDIV	3
 
 /* MCLK */
 #define CLKID_MCLK_PLL_DCO	0
@@ -52,5 +57,6 @@
 #define CLKID_MCLK_1_DIV2	8
 #define CLKID_MCLK_1_PRE	9
 #define CLKID_MCLK_1		10
+#define CLKID_MCLK_PREDIV	11
 
 #endif /* __T7_PLL_CLKC_H */

-- 
2.47.1

[PATCH RFC 3/3] clk: meson: t7: Model PLL pre-divider as a divider clock

[Jian Hu via B4 Relay]

From: Jian Hu <jian.hu@amlogic.com>

Replace the dedicated PLL pre-divider with a standalone
divider clock. The PLL DCO now takes the pre-divider clock
as its parent instead of the input clock directly.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
 drivers/clk/meson/t7-pll.c | 183 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 141 insertions(+), 42 deletions(-)

diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
index 0a622f45fa36..049f0c879c45 100644
--- a/drivers/clk/meson/t7-pll.c
+++ b/drivers/clk/meson/t7-pll.c
@@ -71,6 +71,34 @@
 #define MCLK_PLL_CNTL4		0x10
 #define MCLK_PLL_STS		0x14
 
+static const struct clk_div_table t7_prediv_div_table[] = {
+	{ .val = 1, .div = 1 },
+	{ /* sentinel */ }
+};
+
+static struct clk_regmap t7_gp0_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = GP0PLL_CTRL0,
+		.shift = 10,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/*
+		 * The hardware reset value is 0. Allow it during clock registration
+		 * to avoid a warning from the common divider code.
+		 * set_rate() will program the valid divider value (1).
+		 */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "gp0_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static const struct pll_mult_range t7_media_pll_mult_range = {
 	.min = 125,
 	.max = 250,
@@ -97,11 +125,6 @@ static struct clk_regmap t7_gp0_pll_dco = {
 			.shift   = 0,
 			.width   = 8,
 		},
-		.n = {
-			.reg_off = GP0PLL_CTRL0,
-			.shift   = 10,
-			.width   = 5,
-		},
 		.l = {
 			.reg_off = GP0PLL_STS,
 			.shift   = 31,
@@ -119,8 +142,8 @@ static struct clk_regmap t7_gp0_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "gp0_pll_dco",
 		.ops = &meson_clk_pll_ops,
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "in0",
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_gp0_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -159,6 +182,25 @@ static const struct reg_sequence t7_gp1_init_regs[] = {
 	{ .reg = GP1PLL_CTRL3,  .def = 0x00000000 },
 };
 
+static struct clk_regmap t7_gp1_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = GP1PLL_CTRL0,
+		.shift = 16,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/* Same rationale as gp0_prediv. */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "gp1_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_regmap t7_gp1_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -171,11 +213,6 @@ static struct clk_regmap t7_gp1_pll_dco = {
 			.shift   = 0,
 			.width   = 8,
 		},
-		.n = {
-			.reg_off = GP1PLL_CTRL0,
-			.shift   = 16,
-			.width   = 5,
-		},
 		.l = {
 			.reg_off = GP1PLL_STS,
 			.shift   = 31,
@@ -193,8 +230,8 @@ static struct clk_regmap t7_gp1_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "gp1_pll_dco",
 		.ops = &meson_clk_pll_ops,
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "in0",
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_gp1_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -227,6 +264,25 @@ static const struct reg_sequence t7_hifi_init_regs[] = {
 	{ .reg = HIFIPLL_CTRL6, .def = 0x56540000 }
 };
 
+static struct clk_regmap t7_hifi_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HIFIPLL_CTRL0,
+		.shift = 10,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/* Same rationale as gp0_prediv. */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "hifi_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_regmap t7_hifi_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -239,11 +295,6 @@ static struct clk_regmap t7_hifi_pll_dco = {
 			.shift   = 0,
 			.width   = 8,
 		},
-		.n = {
-			.reg_off = HIFIPLL_CTRL0,
-			.shift   = 10,
-			.width   = 5,
-		},
 		.frac = {
 			.reg_off = HIFIPLL_CTRL1,
 			.shift   = 0,
@@ -267,8 +318,8 @@ static struct clk_regmap t7_hifi_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "hifi_pll_dco",
 		.ops = &meson_clk_pll_ops,
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "in0",
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_hifi_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -312,6 +363,25 @@ static const struct reg_sequence t7_pcie_pll_init_regs[] = {
 	{ .reg = PCIEPLL_CTRL2,	.def = 0x00001000 }
 };
 
+static struct clk_regmap t7_pcie_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = PCIEPLL_CTRL0,
+		.shift = 10,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/* Same rationale as gp0_prediv. */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "pcie_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_regmap t7_pcie_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -324,11 +394,6 @@ static struct clk_regmap t7_pcie_pll_dco = {
 			.shift   = 0,
 			.width   = 8,
 		},
-		.n = {
-			.reg_off = PCIEPLL_CTRL0,
-			.shift   = 10,
-			.width   = 5,
-		},
 		.l = {
 			.reg_off = PCIEPLL_CTRL0,
 			.shift   = 31,
@@ -345,8 +410,8 @@ static struct clk_regmap t7_pcie_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "pcie_pll_dco",
 		.ops = &meson_clk_pcie_pll_ops,
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "in0",
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_pcie_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -633,6 +698,25 @@ static const struct reg_sequence t7_hdmi_init_regs[] = {
 	{ .reg = HDMIPLL_CTRL6, .def = 0x56540000 }
 };
 
+static struct clk_regmap t7_hdmi_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HDMIPLL_CTRL0,
+		.shift = 10,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/* Same rationale as gp0_prediv. */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "hdmi_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_regmap t7_hdmi_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -645,11 +729,6 @@ static struct clk_regmap t7_hdmi_pll_dco = {
 			.shift   = 0,
 			.width   = 9,
 		},
-		.n = {
-			.reg_off = HDMIPLL_CTRL0,
-			.shift   = 10,
-			.width   = 5,
-		},
 		.l = {
 			.reg_off = HDMIPLL_CTRL0,
 			.shift   = 31,
@@ -667,8 +746,8 @@ static struct clk_regmap t7_hdmi_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "hdmi_pll_dco",
 		.ops = &meson_clk_pll_ops,
-		.parent_data = (const struct clk_parent_data []) {
-			{ .fw_name = "in0", }
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_hdmi_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -722,6 +801,25 @@ static const struct reg_sequence t7_mclk_init_regs[] = {
 	{ .reg = MCLK_PLL_CNTL4, .def = 0x00180303 },
 };
 
+static struct clk_regmap t7_mclk_prediv = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = MCLK_PLL_CNTL0,
+		.shift = 16,
+		.width = 5,
+		.table = t7_prediv_div_table,
+		/* Same rationale as gp0_prediv. */
+		.flags = CLK_DIVIDER_ALLOW_ZERO,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "mclk_prediv",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "in0",
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_regmap t7_mclk_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -734,11 +832,6 @@ static struct clk_regmap t7_mclk_pll_dco = {
 			.shift   = 0,
 			.width   = 8,
 		},
-		.n = {
-			.reg_off = MCLK_PLL_CNTL0,
-			.shift   = 16,
-			.width   = 5,
-		},
 		.l = {
 			.reg_off = MCLK_PLL_CNTL0,
 			.shift   = 31,
@@ -761,8 +854,8 @@ static struct clk_regmap t7_mclk_pll_dco = {
 	.hw.init = &(struct clk_init_data){
 		.name = "mclk_pll_dco",
 		.ops = &meson_clk_pll_ops,
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "in0",
+		.parent_hws = (const struct clk_hw *[]) {
+			&t7_mclk_prediv.hw
 		},
 		.num_parents = 1,
 	},
@@ -939,21 +1032,25 @@ static struct clk_regmap t7_mclk_1 = {
 };
 
 static struct clk_hw *t7_gp0_hw_clks[] = {
+	[CLKID_GP0_PREDIV]		= &t7_gp0_prediv.hw,
 	[CLKID_GP0_PLL_DCO]		= &t7_gp0_pll_dco.hw,
 	[CLKID_GP0_PLL]			= &t7_gp0_pll.hw,
 };
 
 static struct clk_hw *t7_gp1_hw_clks[] = {
+	[CLKID_GP1_PREDIV]		= &t7_gp1_prediv.hw,
 	[CLKID_GP1_PLL_DCO]		= &t7_gp1_pll_dco.hw,
 	[CLKID_GP1_PLL]			= &t7_gp1_pll.hw,
 };
 
 static struct clk_hw *t7_hifi_hw_clks[] = {
+	[CLKID_HIFI_PREDIV]		= &t7_hifi_prediv.hw,
 	[CLKID_HIFI_PLL_DCO]		= &t7_hifi_pll_dco.hw,
 	[CLKID_HIFI_PLL]		= &t7_hifi_pll.hw,
 };
 
 static struct clk_hw *t7_pcie_hw_clks[] = {
+	[CLKID_PCIE_PREDIV]		= &t7_pcie_prediv.hw,
 	[CLKID_PCIE_PLL_DCO]		= &t7_pcie_pll_dco.hw,
 	[CLKID_PCIE_PLL_DCO_DIV2]	= &t7_pcie_pll_dco_div2.hw,
 	[CLKID_PCIE_PLL_OD]		= &t7_pcie_pll_od.hw,
@@ -973,12 +1070,14 @@ static struct clk_hw *t7_mpll_hw_clks[] = {
 };
 
 static struct clk_hw *t7_hdmi_hw_clks[] = {
+	[CLKID_HDMI_PREDIV]		= &t7_hdmi_prediv.hw,
 	[CLKID_HDMI_PLL_DCO]		= &t7_hdmi_pll_dco.hw,
 	[CLKID_HDMI_PLL_OD]		= &t7_hdmi_pll_od.hw,
 	[CLKID_HDMI_PLL]		= &t7_hdmi_pll.hw,
 };
 
 static struct clk_hw *t7_mclk_hw_clks[] = {
+	[CLKID_MCLK_PREDIV]		= &t7_mclk_prediv.hw,
 	[CLKID_MCLK_PLL_DCO]		= &t7_mclk_pll_dco.hw,
 	[CLKID_MCLK_PRE]		= &t7_mclk_pre_od.hw,
 	[CLKID_MCLK_PLL]		= &t7_mclk_pll.hw,

-- 
2.47.1

Re: [PATCH RFC 1/3] clk: meson: pll: Remove the dedicated n parameter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] clk: meson: struct meson_clk_pll_data removal of 'n' breaks compilation
- [High] clk: meson: pll unconditionally requires range, breaking table-based PLLs
- [High] clk: meson: external pre-divider violates PLL disable/reset sequencing
--

commit 9a0d9eb075576d244c161f62134109ad2353a8b7
Author: Jian Hu <jian.hu@amlogic.com>

clk: meson: pll: Remove the dedicated n parameter

The Meson PLL framework models the PLL pre-divider (N) using a dedicated
field in struct meson_clk_pll_data. Since the common clock framework already
provides a generic divider implementation, this patch removes the dedicated
n parameter from the framework and simplifies the PLL rate calculation.

> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
[ ... ]
> @@ -119,145 +107,48 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
[ ... ]
> -static int meson_clk_get_pll_table_index(unsigned int index,
> -					 unsigned int *m,
> -					 unsigned int *n,
> -					 struct meson_clk_pll_data *pll)
> +static int meson_clk_get_pll_multiplier(unsigned long rate,
> +					unsigned long parent_rate,
> +					unsigned int *m,
> +					struct meson_clk_pll_data *pll)
>  {
> -	if (!pll->table[index].n)
> +	if (!pll->range)
>  		return -EINVAL;

[Severity: High]
Does this unconditionally reject any PLL that lacks a .range definition?

Many table-based PLLs in Meson SoC drivers (like axg_gp0_pll_dco) leave
.range as NULL and rely strictly on .table. Without .range, rate calculations
for these PLLs will deterministically fail at runtime.

[ ... ]
> @@ -420,7 +311,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
[ ... ]
>  	enabled = meson_parm_read(clk->map, &pll->en);
>  	if (enabled)
>  		meson_clk_pll_disable(hw);
>  
> -	meson_parm_write(clk->map, &pll->n, n);

[Severity: High]
Does stripping the write to the N pre-divider from this protected atomic
section violate the hardware requirement to place the PLL in a disabled and
reset state before its input frequency changes?

By modelling the pre-divider as an external generic CCF clk_divider,
its .set_rate will modify the hardware register immediately without
notifying the running downstream PLL. Can this cause the active PLL to
lose lock and produce an unstable clock, potentially locking up the system?

>  	meson_parm_write(clk->map, &pll->m, m);
>  
>  	if (MESON_PARM_APPLICABLE(&pll->frac)) {
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -11,29 +11,17 @@
>  #include <linux/regmap.h>
>  #include "parm.h"
>  
> -struct pll_params_table {
> -	unsigned int	m;
> -	unsigned int	n;
> -};
> -

[Severity: High]
Does removing pll_params_table also remove the framework's capability to
enforce non-contiguous PLL multiplier rules?

>  struct pll_mult_range {
>  	unsigned int	min;
>  	unsigned int	max;
>  };
[ ... ]
>  struct meson_clk_pll_data {
>  	struct parm en;
>  	struct parm m;
> -	struct parm n;

[Severity: Critical]
Does removing the n and table fields from struct meson_clk_pll_data break
the build for untouched SoC platform drivers?

Drivers such as axg.c continue to initialize these fields in their static PLL
definitions (like axg_sys_pll_dco):

static struct clk_regmap axg_sys_pll_dco = {
	.data = &(struct meson_clk_pll_data){
		...
		.n = {
			.reg_off = HHI_SYS_PLL_CNTL,
			.shift   = 9,
			.width   = 5,
		},

This will cause an immediate compiler error because the types have been
removed. Will this breakage persist and break git bisect?

>  	struct parm frac;
>  	struct parm l;
>  	struct parm rst;
[ ... ]
> @@ -41,7 +29,6 @@ struct meson_clk_pll_data {
>  	struct parm l_detect;
>  	const struct reg_sequence *init_regs;
>  	unsigned int init_count;
> -	const struct pll_params_table *table;
>  	const struct pll_mult_range *range;
>  	unsigned int frac_max;
>  	u8 flags;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=1

Re: [PATCH RFC 3/3] clk: meson: t7: Model PLL pre-divider as a divider clock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: meson: t7: PLL pre-dividers permanently stuck at 0 due to missing CLK_SET_RATE_PARENT
- [Low] clk: meson: build failure in drivers after struct meson_clk_pll_data change
--

commit 03f0a62ef49427b4fecb0de15b51e80cb45be623
Author: Jian Hu <jian.hu@amlogic.com>

clk: meson: t7: Model PLL pre-divider as a divider clock

This commit replaces the dedicated PLL pre-divider with a standalone
divider clock in the Meson T7 clock driver. The PLL DCO clocks are 
updated to take the new pre-divider clock as their parent instead of 
the input clock directly.

> diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
> index 0a622f45fa368..049f0c879c458 100644
> --- a/drivers/clk/meson/t7-pll.c
> +++ b/drivers/clk/meson/t7-pll.c
> @@ -71,6 +71,34 @@
>  #define MCLK_PLL_CNTL4		0x10
>  #define MCLK_PLL_STS		0x14
>  
> +static const struct clk_div_table t7_prediv_div_table[] = {
> +	{ .val = 1, .div = 1 },
> +	{ /* sentinel */ }
> +};
> +
> +static struct clk_regmap t7_gp0_prediv = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = GP0PLL_CTRL0,
> +		.shift = 10,
> +		.width = 5,
> +		.table = t7_prediv_div_table,
> +		/*
> +		 * The hardware reset value is 0. Allow it during clock registration
> +		 * to avoid a warning from the common divider code.
> +		 * set_rate() will program the valid divider value (1).
> +		 */
> +		.flags = CLK_DIVIDER_ALLOW_ZERO,

[ ... ]

> @@ -97,11 +125,6 @@ static struct clk_regmap t7_gp0_pll_dco = {
>  			.shift   = 0,
>  			.width   = 8,
>  		},
> -		.n = {
> -			.reg_off = GP0PLL_CTRL0,
> -			.shift   = 10,
> -			.width   = 5,
> -		},

[Severity: Low]
When removing the .n parameter initialization here (and presumably from
struct meson_clk_pll_data in a prerequisite commit), does this patch series
also update other Meson drivers like drivers/clk/meson/gxbb.c and
drivers/clk/meson/axg.c? 

Looking at other drivers in the tree, they appear to still statically
initialize .n in their declarations:

drivers/clk/meson/gxbb.c:gxbb_fixed_pll_dco() {
        ...
        .n = {
            .reg_off = HHI_MPLL_CNTL,
            .shift   = 9,
            .width   = 5,
        },
        ...
}

Will leaving these initializations in place cause a build failure with a
'has no member' error for those drivers?

>  		.l = {
>  			.reg_off = GP0PLL_STS,
>  			.shift   = 31,
> @@ -119,8 +142,8 @@ static struct clk_regmap t7_gp0_pll_dco = {
>  	.hw.init = &(struct clk_init_data){
>  		.name = "gp0_pll_dco",
>  		.ops = &meson_clk_pll_ops,
> -		.parent_data = &(const struct clk_parent_data) {
> -			.fw_name = "in0",
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_gp0_prediv.hw
>  		},
>  		.num_parents = 1,
>  	},

[Severity: High]
Since the pre-divider relies on .set_rate() to correct the invalid
hardware reset value of 0, will the common clock framework ever actually
call .set_rate() on the pre-divider?

Without adding the CLK_SET_RATE_PARENT flag here on the DCO clock
(t7_gp0_pll_dco), rate requests to the DCO won't propagate up to the
pre-divider. Could this leave the pre-divider stuck at the invalid 0 state
and permanently prevent the PLL from functioning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=3

Re: [PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

[Jerome Brunet]

On mer. 23 sept. 2026 at 19:14, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:

> This series refactors the Meson PLL framework to remove the dedicated
> PLL pre-divider (N) parameter from the PLL implementation and model it
> as a separate divider clock.
>
> Currently, the Meson PLL framework models the PLL pre-divider using a
> dedicated n field in struct meson_clk_pll_data. This makes the
> pre-divider part of the PLL-specific implementation, although the
> Common Clock Framework already provides a generic divider clock.
>
> This series separates the pre-divider from the PLL and makes the PLL
> DCO take the pre-divider clock as its parent. This allows the
> pre-divider to be modeled using the standard CCF divider implementation
> and simplifies the PLL framework.
>
> The series currently covers T7 as an RFC to get feedback on the
> framework design before applying the same approach to other SoCs.
>
> Series:
> clk: meson: pll: Remove the dedicated n parameter
> dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
> clk: meson: t7: Model PLL pre-divider as a divider clock
>
> The other Meson SoCs will be converted separately after the T7 PLL
> framework refactoring has been reviewed and the overall approach is
> agreed upon.
>
> Any feedback on the proposed clock hierarchy and the separation of the
> PLL pre-divider from the PLL itself would be appreciated.

So if I summarize this RFC, you have simply taken the divider out of the
PLL, no futher addaptation. right ?

I'm happy with it on the general principle and fine with the change as
long as you test it on as much platform as you can, clearly flagging
those you have just compiled tested.

A change like this would likely need to land early in the cycle give as
much time as possible for testing.

However there a couple of thing I'm concerned about:

* You've drop the table support: are you sure this is not needed anymore
  ? don't you want to be able to restrict mutlipliers to specific values
  sometimes ? If not, then OK.

* the determine_rate() make no call to round the parent rate: Since the
  parent will be the divier, how do you progate the rate change so N
  moves and the best parent rate is found ? For sure this fractional
  multiplier clock will need CLK_SET_RATE_PARENT to adjust the
  pre-divider.

* Goes with the point above, but I'm not seeing anything that favors
  lower N for lower jitter, Or mention of a minimum input rate (which
  could be a property) ?
  Those are constraints I think I have understood from your explanation
  here [1] but maybe you've got new information to share ?

This is overall going in the right direction but determine_rate() and
constraints need work.

Note: you are more likely to get test feedback if you add g12 (sm1) as an
example. Those are still the most widely used amlogic platforms with
mainline.

[1]: https://lore.kernel.org/linux-clk/c9c4945f-cdfc-4382-b8ca-71b69d91deb4@amlogic.com/

>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
> Jian Hu (3):
>       clk: meson: pll: Remove the dedicated n parameter
>       dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>       clk: meson: t7: Model PLL pre-divider as a divider clock
>
>  drivers/clk/meson/clk-pll.c                     | 178 +++++------------------
>  drivers/clk/meson/clk-pll.h                     |  13 --
>  drivers/clk/meson/t7-pll.c                      | 183 ++++++++++++++++++------
>  include/dt-bindings/clock/amlogic,t7-pll-clkc.h |   6 +
>  4 files changed, 181 insertions(+), 199 deletions(-)
> ---
> base-commit: 43e1705ecab981c66baee89041e6f728c0436f19
> change-id: 20260923-meson_refactor_n-e7f25904e536
>
> Best regards,
> -- 
> Jian Hu <jian.hu@amlogic.com>
>
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Jerome

Re: [PATCH RFC 1/3] clk: meson: pll: Remove the dedicated n parameter

[Jian Hu]

On 9/23/2026 7:24 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] clk: meson: struct meson_clk_pll_data removal of 'n' breaks compilation
> - [High] clk: meson: pll unconditionally requires range, breaking table-based PLLs
> - [High] clk: meson: external pre-divider violates PLL disable/reset sequencing
> --
>
> commit 9a0d9eb075576d244c161f62134109ad2353a8b7
> Author: Jian Hu <jian.hu@amlogic.com>
>
> clk: meson: pll: Remove the dedicated n parameter
>
> The Meson PLL framework models the PLL pre-divider (N) using a dedicated
> field in struct meson_clk_pll_data. Since the common clock framework already
> provides a generic divider implementation, this patch removes the dedicated
> n parameter from the framework and simplifies the PLL rate calculation.
>
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
> [ ... ]
>> @@ -119,145 +107,48 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
> [ ... ]
>> -static int meson_clk_get_pll_table_index(unsigned int index,
>> -                                      unsigned int *m,
>> -                                      unsigned int *n,
>> -                                      struct meson_clk_pll_data *pll)
>> +static int meson_clk_get_pll_multiplier(unsigned long rate,
>> +                                     unsigned long parent_rate,
>> +                                     unsigned int *m,
>> +                                     struct meson_clk_pll_data *pll)
>>   {
>> -     if (!pll->table[index].n)
>> +     if (!pll->range)
>>                return -EINVAL;
> [Severity: High]
> Does this unconditionally reject any PLL that lacks a .range definition?
>
> Many table-based PLLs in Meson SoC drivers (like axg_gp0_pll_dco) leave
> .range as NULL and rely strictly on .table. Without .range, rate calculations
> for these PLLs will deterministically fail at runtime.


Yes, the table consists of m and n, and n has now been deleted.

Therefore, the table method will be converted to the range method.

> [ ... ]
>> @@ -420,7 +311,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> [ ... ]
>>        enabled = meson_parm_read(clk->map, &pll->en);
>>        if (enabled)
>>                meson_clk_pll_disable(hw);
>>
>> -     meson_parm_write(clk->map, &pll->n, n);
> [Severity: High]
> Does stripping the write to the N pre-divider from this protected atomic
> section violate the hardware requirement to place the PLL in a disabled and
> reset state before its input frequency changes?
>
> By modelling the pre-divider as an external generic CCF clk_divider,
> its .set_rate will modify the hardware register immediately without
> notifying the running downstream PLL. Can this cause the active PLL to
> lose lock and produce an unstable clock, potentially locking up the system?


Separating N from the PLL will not affect PLL locking.

The PLL locking operation is initiated by the enable signal.

>>        meson_parm_write(clk->map, &pll->m, m);
>>
>>        if (MESON_PARM_APPLICABLE(&pll->frac)) {
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -11,29 +11,17 @@
>>   #include <linux/regmap.h>
>>   #include "parm.h"
>>
>> -struct pll_params_table {
>> -     unsigned int    m;
>> -     unsigned int    n;
>> -};
>> -
> [Severity: High]
> Does removing pll_params_table also remove the framework's capability to
> enforce non-contiguous PLL multiplier rules?


Yes, it losts  non-contiguous PLL multiplier rules

The "range" method can cover the frequency points of the "table" method.

In the pre-divider, N is typically fixed at 1.

>>   struct pll_mult_range {
>>        unsigned int    min;
>>        unsigned int    max;
>>   };
> [ ... ]
>>   struct meson_clk_pll_data {
>>        struct parm en;
>>        struct parm m;
>> -     struct parm n;
> [Severity: Critical]
> Does removing the n and table fields from struct meson_clk_pll_data break
> the build for untouched SoC platform drivers?
>
> Drivers such as axg.c continue to initialize these fields in their static PLL
> definitions (like axg_sys_pll_dco):
>
> static struct clk_regmap axg_sys_pll_dco = {
>          .data = &(struct meson_clk_pll_data){
>                  ...
>                  .n = {
>                          .reg_off = HHI_SYS_PLL_CNTL,
>                          .shift   = 9,
>                          .width   = 5,
>                  },
>
> This will cause an immediate compiler error because the types have been
> removed. Will this breakage persist and break git bisect?


That is indeed the case,  I will include clk-pll.c and all the PLLs in a 
single patch.

>>        struct parm frac;
>>        struct parm l;
>>        struct parm rst;
> [ ... ]
>> @@ -41,7 +29,6 @@ struct meson_clk_pll_data {
>>        struct parm l_detect;
>>        const struct reg_sequence *init_regs;
>>        unsigned int init_count;
>> -     const struct pll_params_table *table;
>>        const struct pll_mult_range *range;
>>        unsigned int frac_max;
>>        u8 flags;
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=1

Re: [PATCH RFC 3/3] clk: meson: t7: Model PLL pre-divider as a divider clock

[Jian Hu]

On 9/23/2026 7:26 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] clk: meson: t7: PLL pre-dividers permanently stuck at 0 due to missing CLK_SET_RATE_PARENT
> - [Low] clk: meson: build failure in drivers after struct meson_clk_pll_data change
> --
>
> commit 03f0a62ef49427b4fecb0de15b51e80cb45be623
> Author: Jian Hu <jian.hu@amlogic.com>
>
> clk: meson: t7: Model PLL pre-divider as a divider clock
>
> This commit replaces the dedicated PLL pre-divider with a standalone
> divider clock in the Meson T7 clock driver. The PLL DCO clocks are
> updated to take the new pre-divider clock as their parent instead of
> the input clock directly.
>
>> diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
>> index 0a622f45fa368..049f0c879c458 100644
>> --- a/drivers/clk/meson/t7-pll.c
>> +++ b/drivers/clk/meson/t7-pll.c
>> @@ -71,6 +71,34 @@
>>   #define MCLK_PLL_CNTL4               0x10
>>   #define MCLK_PLL_STS         0x14
>>
>> +static const struct clk_div_table t7_prediv_div_table[] = {
>> +     { .val = 1, .div = 1 },
>> +     { /* sentinel */ }
>> +};
>> +
>> +static struct clk_regmap t7_gp0_prediv = {
>> +     .data = &(struct clk_regmap_div_data){
>> +             .offset = GP0PLL_CTRL0,
>> +             .shift = 10,
>> +             .width = 5,
>> +             .table = t7_prediv_div_table,
>> +             /*
>> +              * The hardware reset value is 0. Allow it during clock registration
>> +              * to avoid a warning from the common divider code.
>> +              * set_rate() will program the valid divider value (1).
>> +              */
>> +             .flags = CLK_DIVIDER_ALLOW_ZERO,
> [ ... ]
>
>> @@ -97,11 +125,6 @@ static struct clk_regmap t7_gp0_pll_dco = {
>>                        .shift   = 0,
>>                        .width   = 8,
>>                },
>> -             .n = {
>> -                     .reg_off = GP0PLL_CTRL0,
>> -                     .shift   = 10,
>> -                     .width   = 5,
>> -             },
> [Severity: Low]
> When removing the .n parameter initialization here (and presumably from
> struct meson_clk_pll_data in a prerequisite commit), does this patch series
> also update other Meson drivers like drivers/clk/meson/gxbb.c and
> drivers/clk/meson/axg.c?
>
> Looking at other drivers in the tree, they appear to still statically
> initialize .n in their declarations:
>
> drivers/clk/meson/gxbb.c:gxbb_fixed_pll_dco() {
>          ...
>          .n = {
>              .reg_off = HHI_MPLL_CNTL,
>              .shift   = 9,
>              .width   = 5,
>          },
>          ...
> }
>
> Will leaving these initializations in place cause a build failure with a
> 'has no member' error for those drivers?


  I will include clk-pll.c and all the PLLs in a single patch.

>>                .l = {
>>                        .reg_off = GP0PLL_STS,
>>                        .shift   = 31,
>> @@ -119,8 +142,8 @@ static struct clk_regmap t7_gp0_pll_dco = {
>>        .hw.init = &(struct clk_init_data){
>>                .name = "gp0_pll_dco",
>>                .ops = &meson_clk_pll_ops,
>> -             .parent_data = &(const struct clk_parent_data) {
>> -                     .fw_name = "in0",
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &t7_gp0_prediv.hw
>>                },
>>                .num_parents = 1,
>>        },
> [Severity: High]
> Since the pre-divider relies on .set_rate() to correct the invalid
> hardware reset value of 0, will the common clock framework ever actually
> call .set_rate() on the pre-divider?
>
> Without adding the CLK_SET_RATE_PARENT flag here on the DCO clock
> (t7_gp0_pll_dco), rate requests to the DCO won't propagate up to the
> pre-divider. Could this leave the pre-divider stuck at the invalid 0 state
> and permanently prevent the PLL from functioning?


Ok, I will add CLK_SET_RATE_PARENT flag for DCO clock in the next version.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=3

Re: [PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

[Jian Hu]

Hi Jerome,


Thanks for your review.


On 9/24/2026 5:35 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On mer. 23 sept. 2026 at 19:14, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>
>> This series refactors the Meson PLL framework to remove the dedicated
>> PLL pre-divider (N) parameter from the PLL implementation and model it
>> as a separate divider clock.
>>
>> Currently, the Meson PLL framework models the PLL pre-divider using a
>> dedicated n field in struct meson_clk_pll_data. This makes the
>> pre-divider part of the PLL-specific implementation, although the
>> Common Clock Framework already provides a generic divider clock.
>>
>> This series separates the pre-divider from the PLL and makes the PLL
>> DCO take the pre-divider clock as its parent. This allows the
>> pre-divider to be modeled using the standard CCF divider implementation
>> and simplifies the PLL framework.
>>
>> The series currently covers T7 as an RFC to get feedback on the
>> framework design before applying the same approach to other SoCs.
>>
>> Series:
>> clk: meson: pll: Remove the dedicated n parameter
>> dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>> clk: meson: t7: Model PLL pre-divider as a divider clock
>>
>> The other Meson SoCs will be converted separately after the T7 PLL
>> framework refactoring has been reviewed and the overall approach is
>> agreed upon.
>>
>> Any feedback on the proposed clock hierarchy and the separation of the
>> PLL pre-divider from the PLL itself would be appreciated.
> So if I summarize this RFC, you have simply taken the divider out of the
> PLL, no futher addaptation. right ?
>
> I'm happy with it on the general principle and fine with the change as
> long as you test it on as much platform as you can, clearly flagging
> those you have just compiled tested.
>
> A change like this would likely need to land early in the cycle give as
> much time as possible for testing.
>
> However there a couple of thing I'm concerned about:
>
> * You've drop the table support: are you sure this is not needed anymore
>    ? don't you want to be able to restrict mutlipliers to specific values
>    sometimes ? If not, then OK.
>
> * the determine_rate() make no call to round the parent rate: Since the
>    parent will be the divier, how do you progate the rate change so N
>    moves and the best parent rate is found ? For sure this fractional
>    multiplier clock will need CLK_SET_RATE_PARENT to adjust the
>    pre-divider.
>
> * Goes with the point above, but I'm not seeing anything that favors
>    lower N for lower jitter, Or mention of a minimum input rate (which
>    could be a property) ?
>    Those are constraints I think I have understood from your explanation
>    here [1] but maybe you've got new information to share ?
>
> This is overall going in the right direction but determine_rate() and
> constraints need work.
>
> Note: you are more likely to get test feedback if you add g12 (sm1) as an
> example. Those are still the most widely used amlogic platforms with
> mainline.
>
> [1]: https://lore.kernel.org/linux-clk/c9c4945f-cdfc-4382-b8ca-71b69d91deb4@amlogic.com/


Yes, your summary is correct: the RFC simply takes the pre-divider out 
of the PLL.


1) Keeping the table support

Agreed, removing it was premature. Some tables cannot be expressed as
a multiplier range:

- pinned (m, n) pairs, e.g. axg PCIe GP0 (m=200, n=3) and meson8m2
   GP0 (m=182, n=3)
- a pinned m, e.g. g12a PCIe PLL (m=150)
- sparse tables, e.g. meson8b HDMI PLL

The per-platform conversions will turn the tables with contiguous m
and n = 1 into range, and keep the tables for the rest, so
the framework will support both.


2) N is fixed per PLL

The main new information: the pre-divider is not meant to be selected
dynamically. Per the hardware design, each PLL has a single fixed N
value, defined together with the rest of the PLL: N = 1 for most PLLs,
and N = 3 for a few special cases on older SoCs (the axg PCIe PLL and
the meson8m2 GP0 PLL). The PLL input frequency constraints are
respected by this fixed value.

Keeping N at 1 minimizes PLL jitter and yields the best performance.

So this is less about dropping the constraints than about the fact
that there is nothing to choose at runtime: no N search in
determine_rate(), and no PFD input frequency constraint to enforce,
since the PLL input frequency is a constant for each PLL.

3) No CLK_SET_RATE_PARENT on the DCO

With N fixed, the pre-divider rate never changes, so the DCO does not
set CLK_SET_RATE_PARENT on purpose: the flag would claim that the
parent rate may change to satisfy the child, which is not the case
here. determine_rate() never touches best_parent_rate so the flag
would be a no operation today.


4) Implementation methods for pre-divider clock

While testing the RFC I found that with the single-entry pre-divider
table ({val = 1, div = 1}), the pre-divider register can never actually
be programmed.

The pre-divider field resets to 0, which is not a valid setting.

During registration, recalc_rate() reports the parent rate for the
zero register value. With N = 1 the reported rate is the parent rate,
and the single-entry table also rounds every rate request back to the
parent rate, so clk_set_rate() always bails out early (rounded rate ==
current rate) and clk_regmap_div_set_rate() is never called. The
register keeps its invalid reset value.

v2 programs the fixed N at registration time, with a new
init_val field in clk_regmap_div_data, applied from .init() once the
regmap is available.

Or do you have any other good ideas?

The patch is available[1], Please help to review it.


5) Testing and rollout

So far this has been boot tested on T7. For v2 I will
convert the SoCs one by one, starting with g12a and sm1 which
have the most mainline users, then the remaining platforms.


[1]

--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -163,10 +163,28 @@ static int clk_regmap_div_set_rate(struct clk_hw 
*hw, unsigned long rate,
                                   clk_div_mask(div->width) << 
div->shift, val);
  };

+static int clk_regmap_div_init(struct clk_hw *hw)
+{
+       int ret;
+       struct clk_regmap *clk = to_clk_regmap(hw);
+       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+
+       ret = clk_regmap_init(hw);
+       if (ret)
+               return ret;
+
+       if (div->init_val)
+               regmap_update_bits(clk->map, div->offset,
+                                  clk_div_mask(div->width) << div->shift,
+                                  div->init_val << div->shift);
+
+       return 0;
+}
+
  /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */

  const struct clk_ops clk_regmap_divider_ops = {
-       .init = clk_regmap_init,
+       .init = clk_regmap_div_init,

>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>> Jian Hu (3):
>>        clk: meson: pll: Remove the dedicated n parameter
>>        dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>>        clk: meson: t7: Model PLL pre-divider as a divider clock
>>
>>   drivers/clk/meson/clk-pll.c                     | 178 +++++------------------
>>   drivers/clk/meson/clk-pll.h                     |  13 --
>>   drivers/clk/meson/t7-pll.c                      | 183 ++++++++++++++++++------
>>   include/dt-bindings/clock/amlogic,t7-pll-clkc.h |   6 +
>>   4 files changed, 181 insertions(+), 199 deletions(-)
>> ---
>> base-commit: 43e1705ecab981c66baee89041e6f728c0436f19
>> change-id: 20260923-meson_refactor_n-e7f25904e536
>>
>> Best regards,
>> --
>> Jian Hu <jian.hu@amlogic.com>
>>
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> --
> Jerome

--

Jian

Re: [PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

[Jerome Brunet]

On ven. 09 oct. 2026 at 14:35, Jian Hu <jian.hu@amlogic.com> wrote:

> Hi Jerome,
>
>
> Thanks for your review.
>
>
> On 9/24/2026 5:35 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On mer. 23 sept. 2026 at 19:14, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>>
>>> This series refactors the Meson PLL framework to remove the dedicated
>>> PLL pre-divider (N) parameter from the PLL implementation and model it
>>> as a separate divider clock.
>>>
>>> Currently, the Meson PLL framework models the PLL pre-divider using a
>>> dedicated n field in struct meson_clk_pll_data. This makes the
>>> pre-divider part of the PLL-specific implementation, although the
>>> Common Clock Framework already provides a generic divider clock.
>>>
>>> This series separates the pre-divider from the PLL and makes the PLL
>>> DCO take the pre-divider clock as its parent. This allows the
>>> pre-divider to be modeled using the standard CCF divider implementation
>>> and simplifies the PLL framework.
>>>
>>> The series currently covers T7 as an RFC to get feedback on the
>>> framework design before applying the same approach to other SoCs.
>>>
>>> Series:
>>> clk: meson: pll: Remove the dedicated n parameter
>>> dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>>> clk: meson: t7: Model PLL pre-divider as a divider clock
>>>
>>> The other Meson SoCs will be converted separately after the T7 PLL
>>> framework refactoring has been reviewed and the overall approach is
>>> agreed upon.
>>>
>>> Any feedback on the proposed clock hierarchy and the separation of the
>>> PLL pre-divider from the PLL itself would be appreciated.
>> So if I summarize this RFC, you have simply taken the divider out of the
>> PLL, no futher addaptation. right ?
>>
>> I'm happy with it on the general principle and fine with the change as
>> long as you test it on as much platform as you can, clearly flagging
>> those you have just compiled tested.
>>
>> A change like this would likely need to land early in the cycle give as
>> much time as possible for testing.
>>
>> However there a couple of thing I'm concerned about:
>>
>> * You've drop the table support: are you sure this is not needed anymore
>>    ? don't you want to be able to restrict mutlipliers to specific values
>>    sometimes ? If not, then OK.
>>
>> * the determine_rate() make no call to round the parent rate: Since the
>>    parent will be the divier, how do you progate the rate change so N
>>    moves and the best parent rate is found ? For sure this fractional
>>    multiplier clock will need CLK_SET_RATE_PARENT to adjust the
>>    pre-divider.
>>
>> * Goes with the point above, but I'm not seeing anything that favors
>>    lower N for lower jitter, Or mention of a minimum input rate (which
>>    could be a property) ?
>>    Those are constraints I think I have understood from your explanation
>>    here [1] but maybe you've got new information to share ?
>>
>> This is overall going in the right direction but determine_rate() and
>> constraints need work.
>>
>> Note: you are more likely to get test feedback if you add g12 (sm1) as an
>> example. Those are still the most widely used amlogic platforms with
>> mainline.
>>
>> [1]: https://lore.kernel.org/linux-clk/c9c4945f-cdfc-4382-b8ca-71b69d91deb4@amlogic.com/
>
>
> Yes, your summary is correct: the RFC simply takes the pre-divider out 
> of the PLL.
>
>
> 1) Keeping the table support
>
> Agreed, removing it was premature. Some tables cannot be expressed as
> a multiplier range:
>
> - pinned (m, n) pairs, e.g. axg PCIe GP0 (m=200, n=3) and meson8m2
>    GP0 (m=182, n=3)
> - a pinned m, e.g. g12a PCIe PLL (m=150)
> - sparse tables, e.g. meson8b HDMI PLL
>
> The per-platform conversions will turn the tables with contiguous m
> and n = 1 into range, and keep the tables for the rest, so
> the framework will support both.
>
>
> 2) N is fixed per PLL
>
> The main new information: the pre-divider is not meant to be selected
> dynamically. Per the hardware design, each PLL has a single fixed N
> value, defined together with the rest of the PLL: N = 1 for most PLLs,
> and N = 3 for a few special cases on older SoCs (the axg PCIe PLL and
> the meson8m2 GP0 PLL). The PLL input frequency constraints are
> respected by this fixed value.

Ok, but this is not we have done so far. I don't think fixing the
predivier would break any use case, but if it does we may have to
re-visit this.

>
> Keeping N at 1 minimizes PLL jitter and yields the best performance.
>
> So this is less about dropping the constraints than about the fact
> that there is nothing to choose at runtime: no N search in
> determine_rate(), and no PFD input frequency constraint to enforce,
> since the PLL input frequency is a constant for each PLL.
>
> 3) No CLK_SET_RATE_PARENT on the DCO
>
> With N fixed, the pre-divider rate never changes, so the DCO does not
> set CLK_SET_RATE_PARENT on purpose: the flag would claim that the
> parent rate may change to satisfy the child, which is not the case
> here. determine_rate() never touches best_parent_rate so the flag
> would be a no operation today.
>
>
> 4) Implementation methods for pre-divider clock
>
> While testing the RFC I found that with the single-entry pre-divider
> table ({val = 1, div = 1}), the pre-divider register can never actually
> be programmed.
>
> The pre-divider field resets to 0, which is not a valid setting.
>
> During registration, recalc_rate() reports the parent rate for the
> zero register value. With N = 1 the reported rate is the parent rate,
> and the single-entry table also rounds every rate request back to the
> parent rate, so clk_set_rate() always bails out early (rounded rate ==
> current rate) and clk_regmap_div_set_rate() is never called. The
> register keeps its invalid reset value.

I see, you should have had a warning like that then ?

''' Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set  '''

But from what I understand, your divider may have zero written in its
register but it will not output the parent rate unmodified, correct ?
In such case CLK_DIVIDER_ALLOW_ZERO is not correct.

Can you clarify the meaning of invalid ? If the just no oscillation I
guess we could add a 'CLK_DIVIDER_INVALID_ZERO' that returns 0 from
recalc_rate().

If your multiplier you'll probably have to handle this is
determine_rate() with something like

if (req->best_parend_rate == 0 ||
    (req->rate / req->best_parent_rate) > YOUR_MAX_MULT) {
    clk_hw_round_rate(parent, req->rate / YOUR_MAX_MULT)
}

Then you update req->best_parent_rate with the value you get and CCF
will update the divider when the rate is applied.

If you then add your single entry table and CLK_SET_RATE_PARENT, it
should work as you expect AFAIU.

>
> v2 programs the fixed N at registration time, with a new
> init_val field in clk_regmap_div_data, applied from .init() once the
> regmap is available.

I don't like driver setting regs without a clear instruction from the
consumer or the framework, it is a splipery slope where people just tend
shove their use cases..

>
> Or do you have any other good ideas?
>
> The patch is available[1], Please help to review it.
>
>
> 5) Testing and rollout
>
> So far this has been boot tested on T7. For v2 I will
> convert the SoCs one by one, starting with g12a and sm1 which
> have the most mainline users, then the remaining platforms.
>

Please add the RFT tag. Please test GXL as well, you should have no
problem getting your hands on one, those are still easily available.

For Meson8, maybe Martin will be able to help us out ?

>
> [1]
>
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -163,10 +163,28 @@ static int clk_regmap_div_set_rate(struct clk_hw 
> *hw, unsigned long rate,
>                                    clk_div_mask(div->width) << 
> div->shift, val);
>   };
>
> +static int clk_regmap_div_init(struct clk_hw *hw)
> +{
> +       int ret;
> +       struct clk_regmap *clk = to_clk_regmap(hw);
> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> +
> +       ret = clk_regmap_init(hw);
> +       if (ret)
> +               return ret;
> +
> +       if (div->init_val)
> +               regmap_update_bits(clk->map, div->offset,
> +                                  clk_div_mask(div->width) << div->shift,
> +                                  div->init_val << div->shift);
> +
> +       return 0;
> +}
> +
>   /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>
>   const struct clk_ops clk_regmap_divider_ops = {
> -       .init = clk_regmap_init,
> +       .init = clk_regmap_div_init,
>
>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>> ---
>>> Jian Hu (3):
>>>        clk: meson: pll: Remove the dedicated n parameter
>>>        dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>>>        clk: meson: t7: Model PLL pre-divider as a divider clock
>>>
>>>   drivers/clk/meson/clk-pll.c                     | 178 +++++------------------
>>>   drivers/clk/meson/clk-pll.h                     |  13 --
>>>   drivers/clk/meson/t7-pll.c                      | 183 ++++++++++++++++++------
>>>   include/dt-bindings/clock/amlogic,t7-pll-clkc.h |   6 +
>>>   4 files changed, 181 insertions(+), 199 deletions(-)
>>> ---
>>> base-commit: 43e1705ecab981c66baee89041e6f728c0436f19
>>> change-id: 20260923-meson_refactor_n-e7f25904e536
>>>
>>> Best regards,
>>> --
>>> Jian Hu <jian.hu@amlogic.com>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-amlogic mailing list
>>> linux-amlogic@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>> --
>> Jerome
>
> --
>
> Jian
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Jerome