[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>



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic