[RFC PATCH 1/3] clk: imx: clk-fracn-gppll: Do not access num/denom register for integer PLL

[RFC PATCH 1/3] clk: imx: clk-fracn-gppll: Do not access num/denom register for integer PLL

[Alexander Stein]

Similar to clk_fracn_gppll_set_rate(), do not access the numerator and
denominator register for integer PLL. Set MFD/MFN to 0 instead, so the
table lookup will match.
See i.MX93 RM section 74.5.2.1 (PLL memory map) for ARMPLL, addresses
0x40 and 0x50 are not listed/reserved.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/clk/imx/clk-fracn-gppll.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/imx/clk-fracn-gppll.c b/drivers/clk/imx/clk-fracn-gppll.c
index 85771afd4698a..3aef548110e25 100644
--- a/drivers/clk/imx/clk-fracn-gppll.c
+++ b/drivers/clk/imx/clk-fracn-gppll.c
@@ -154,17 +154,24 @@ static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned lon
 {
 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
 	const struct imx_fracn_gppll_rate_table *rate_table = pll->rate_table;
-	u32 pll_numerator, pll_denominator, pll_div;
+	u32 pll_div;
 	u32 mfi, mfn, mfd, rdiv, odiv;
 	u64 fvco = parent_rate;
 	long rate = 0;
 	int i;
 
-	pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
-	mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
+	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
+		u32 pll_numerator, pll_denominator;
+
+		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
+		mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
 
-	pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
-	mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
+		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
+		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
+	} else {
+		mfd = 0;
+		mfn = 0;
+	}
 
 	pll_div = readl_relaxed(pll->base + PLL_DIV);
 	mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
-- 
2.34.1

[RFC PATCH 2/3] clk: imx: clk-fracn-gppll: Refactor clk_fracn_gppll_calc_rate

[Alexander Stein]

Move the frequency calculation into its dedicated function for multiple
usage. No functional change.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/clk/imx/clk-fracn-gppll.c | 54 ++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/imx/clk-fracn-gppll.c b/drivers/clk/imx/clk-fracn-gppll.c
index 3aef548110e25..a7d57fbe93196 100644
--- a/drivers/clk/imx/clk-fracn-gppll.c
+++ b/drivers/clk/imx/clk-fracn-gppll.c
@@ -150,35 +150,15 @@ static long clk_fracn_gppll_round_rate(struct clk_hw *hw, unsigned long rate,
 	return rate_table[pll->rate_count - 1].rate;
 }
 
-static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+static long clk_fracn_gppll_calc_rate(struct clk_fracn_gppll *pll, u32 mfn,
+				      u32 mfd, u32 mfi, u32 rdiv, u32 odiv,
+				      unsigned long parent_rate)
 {
-	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
 	const struct imx_fracn_gppll_rate_table *rate_table = pll->rate_table;
-	u32 pll_div;
-	u32 mfi, mfn, mfd, rdiv, odiv;
 	u64 fvco = parent_rate;
 	long rate = 0;
 	int i;
 
-	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
-		u32 pll_numerator, pll_denominator;
-
-		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
-		mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
-
-		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
-		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
-	} else {
-		mfd = 0;
-		mfn = 0;
-	}
-
-	pll_div = readl_relaxed(pll->base + PLL_DIV);
-	mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
-
-	rdiv = FIELD_GET(PLL_RDIV_MASK, pll_div);
-	odiv = FIELD_GET(PLL_ODIV_MASK, pll_div);
-
 	/*
 	 * Sometimes, the recalculated rate has deviation due to
 	 * the frac part. So find the accurate pll rate from the table
@@ -222,6 +202,34 @@ static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned lon
 	return (unsigned long)fvco;
 }
 
+static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
+	u32 pll_div;
+	u32 mfi, mfn, mfd, rdiv, odiv;
+
+	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
+		u32 pll_numerator, pll_denominator;
+
+		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
+		mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
+
+		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
+		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
+	} else {
+		mfd = 0;
+		mfn = 0;
+	}
+
+	pll_div = readl_relaxed(pll->base + PLL_DIV);
+	mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
+
+	rdiv = FIELD_GET(PLL_RDIV_MASK, pll_div);
+	odiv = FIELD_GET(PLL_ODIV_MASK, pll_div);
+
+	return clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, parent_rate);
+}
+
 static int clk_fracn_gppll_wait_lock(struct clk_fracn_gppll *pll)
 {
 	u32 val;
-- 
2.34.1

[RFC PATCH 3/3] clk: imx: clk-fracn-gppll: Support dynamic rates

[Alexander Stein]

The fracn gppll PLL so far only supports rates from a rate table passed
during initialization. Calculating PLL settings dynamically helps audio
applications to get their desired rates, so support for this is added
in this patch.

The strategy to get to the PLL setting for a rate is:

- The rate table is searched for suitable rates, so for standard rates the
  same settings are used as without this patch
- Then try to only adjust mfn, on fractional PLLs only, which specifies
  the fractional part of the PLL. This setting can be changed without
  glitches on the output and is therefore preferred
- As a last resort the best settings are calculated dynamically

Implementation is inspired by commit b09c68dc57c9d ("clk: imx: pll14xx:
Support dynamic rates")

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
This is the first time I'm touching PLL code, I might be missing things
or not being aware of important aspects when it comes to PLL.
Thus this is a RFC

 drivers/clk/imx/clk-fracn-gppll.c | 203 ++++++++++++++++++++++++++----
 1 file changed, 181 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/imx/clk-fracn-gppll.c b/drivers/clk/imx/clk-fracn-gppll.c
index a7d57fbe93196..68c5b4a95efbe 100644
--- a/drivers/clk/imx/clk-fracn-gppll.c
+++ b/drivers/clk/imx/clk-fracn-gppll.c
@@ -25,6 +25,12 @@
 
 #define PLL_NUMERATOR		0x40
 #define PLL_MFN_MASK		GENMASK(31, 2)
+#define PLL_MFI_MIN		0x2
+#define PLL_MFI_MAX		0x1ff
+#define PLL_MFN_MIN		0x0
+#define PLL_MFN_MAX		0x3fffffff
+#define PLL_MFD_MIN		0x1
+#define PLL_MFD_MAX		0x3fffffff
 
 #define PLL_DENOMINATOR		0x50
 #define PLL_MFD_MASK		GENMASK(29, 0)
@@ -134,21 +140,6 @@ imx_get_pll_settings(struct clk_fracn_gppll *pll, unsigned long rate)
 	return NULL;
 }
 
-static long clk_fracn_gppll_round_rate(struct clk_hw *hw, unsigned long rate,
-				       unsigned long *prate)
-{
-	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
-	const struct imx_fracn_gppll_rate_table *rate_table = pll->rate_table;
-	int i;
-
-	/* Assuming rate_table is in descending order */
-	for (i = 0; i < pll->rate_count; i++)
-		if (rate >= rate_table[i].rate)
-			return rate_table[i].rate;
-
-	/* return minimum supported value */
-	return rate_table[pll->rate_count - 1].rate;
-}
 
 static long clk_fracn_gppll_calc_rate(struct clk_fracn_gppll *pll, u32 mfn,
 				      u32 mfd, u32 mfi, u32 rdiv, u32 odiv,
@@ -202,6 +193,174 @@ static long clk_fracn_gppll_calc_rate(struct clk_fracn_gppll *pll, u32 mfn,
 	return (unsigned long)fvco;
 }
 
+static u32 clk_fracn_gppll_calc_mfi(int rdiv, unsigned long fvco,
+				    unsigned long fref)
+{
+	u32 mfi;
+
+	/* fvco = fref / rdiv * mfi */
+	mfi = DIV_ROUND_CLOSEST(fvco * rdiv, fref);
+	return clamp_t(u32, mfi, PLL_MFI_MIN, PLL_MFI_MAX);
+}
+
+static long clk_fracn_gppll_calc_mfn(int mfd, int mfi, int odiv, int rdiv,
+				     unsigned long rate, unsigned long prate)
+{
+	unsigned long tmp;
+	long mfn;
+
+	/* calc mfn = ((rate * odiv * rdiv / prate) - mfi) * mfd */
+	tmp = rate * odiv * rdiv - (mfi * prate);
+	mfn = DIV_ROUND_CLOSEST(tmp * mfd, prate);
+	return clamp_t(long, mfn, PLL_MFN_MIN, PLL_MFN_MAX);
+}
+
+static void clk_fracn_gppll_calc_settings(struct clk_fracn_gppll *pll, unsigned long rate,
+					  unsigned long prate, struct imx_fracn_gppll_rate_table *t)
+{
+	u32 pll_denominator, pll_numerator, pll_div;
+	u32 mfi, mfn, mfd, rdiv, odiv;
+	long fout, rate_min, rate_max, dist, best = LONG_MAX;
+	const struct imx_fracn_gppll_rate_table *tt;
+
+	/*
+	 * PLL constrains:
+	 *
+	 * a) 1 <= MFN <= 0x3fffffff (fractional only)
+	 * b) 1 <= MFD <= 0x3fffffff (fractional only)
+	 * c) 2 <= MFI <= 0x1ff
+	 * d) 1 <= RDIV <= 7
+	 * e) 2 <= ODIV <= 255
+	 * f) -2 <= MFN/MFD <= 2
+	 * g) 20MHz <= (Fref / rdiv) <= 40MHz
+	 * h) 2.5GHz <= Fvco <= 5Ghz
+	 *
+	 * Fvco = (Fref / rdiv) * (MFI + MFN / MFD)
+	 * Fout = Fvco / odiv
+	 */
+
+	/* First try if we can get the desired rate from one of the static entries */
+	tt = imx_get_pll_settings(pll, rate);
+	if (tt) {
+		pr_debug("%s: in=%ld, want=%ld, Using PLL setting from table\n",
+			 clk_hw_get_name(&pll->hw), prate, rate);
+		t->rate = tt->rate;
+		t->mfi = tt->mfi;
+		t->mfn = tt->mfn;
+		t->mfd = tt->mfd;
+		t->rdiv = tt->rdiv;
+		t->odiv = tt->odiv;
+		return;
+	}
+
+	/* glitch free MFN adjustment only for fractional PLL */
+	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
+		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
+
+		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
+		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
+
+		pll_div = readl_relaxed(pll->base + PLL_DIV);
+		mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
+		rdiv = FIELD_GET(PLL_RDIV_MASK, pll_div);
+		odiv = FIELD_GET(PLL_ODIV_MASK, pll_div);
+
+		/* Then see if we can get the desired rate by only adjusting MFN (glitch free) */
+		rate_min = clk_fracn_gppll_calc_rate(pll, PLL_MFN_MIN, mfd, mfi, rdiv, odiv, prate);
+		rate_max = clk_fracn_gppll_calc_rate(pll, PLL_MFN_MAX, mfd, mfi, rdiv, odiv, prate);
+
+		if (rate >= rate_min && rate <= rate_max) {
+			mfn = clk_fracn_gppll_calc_mfn(mfd, mfi, odiv, rdiv, rate, prate);
+			pr_debug("%s: in=%ld, want=%ld Only adjust mfn %ld -> %d\n",
+				 clk_hw_get_name(&pll->hw), prate, rate,
+				 FIELD_GET(PLL_MFN_MASK, pll_numerator), mfn);
+			fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
+			t->rate = (unsigned int)fout;
+			t->mfi = mfi;
+			t->mfn = mfn;
+			t->mfd = mfd;
+			t->rdiv = rdiv;
+			t->odiv = odiv;
+			return;
+		}
+	}
+
+	/* Finally calculate best values */
+	for (rdiv = 1; rdiv <= 7; rdiv++) {
+		if ((prate / rdiv) < 20000000)
+			continue;
+		if ((prate / rdiv) > 40000000)
+			continue;
+
+		for (odiv = 2; odiv <= 255; odiv++) {
+			mfi = clk_fracn_gppll_calc_mfi(rdiv, rate * odiv, prate);
+			mfd = 1;
+			mfn = 0;
+
+			/* Try integer PLL part only first */
+			fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
+			if (fout * odiv < 2500000000UL)
+				continue;
+			if (fout * odiv > 5000000000UL)
+				continue;
+
+			if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
+				if (!dist) {
+					/* Disable fractional part upon exact match */
+					mfd = 1;
+					mfn = 0;
+				} else {
+					mfd = 100;
+					mfd = clamp(mfd, PLL_MFD_MIN, PLL_MFN_MAX);
+
+					mfn = clk_fracn_gppll_calc_mfn(mfd, mfi, odiv, rdiv, rate, prate);
+					if ((mfn / mfd) > 2)
+						continue;
+
+					fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
+					if (fout * odiv < 2500000000)
+						continue;
+					if (fout * odiv > 5000000000)
+						continue;
+				}
+			} else {
+				mfd = 0;
+				mfn = 0;
+			}
+
+			/* best match */
+			dist = abs((long)rate - (long)fout);
+			if (dist < best) {
+				best = dist;
+				t->rate = (unsigned int)fout;
+				t->mfi = mfi;
+				t->mfn = mfn;
+				t->mfd = mfd;
+				t->rdiv = rdiv;
+				t->odiv = odiv;
+
+				if (!dist)
+					goto found;
+			}
+		}
+	}
+found:
+	pr_debug("%s: in=%lu, want=%lu got=%u (mfi=%u mfn=%u mfd=%u rdiv=%u odiv=%u)\n",
+		 clk_hw_get_name(&pll->hw), prate, rate, t->rate, t->mfi, t->mfn, t->mfd,
+		 t->rdiv, t->odiv);
+}
+
+static long clk_fracn_gppll_round_rate(struct clk_hw *hw, unsigned long rate,
+				       unsigned long *prate)
+{
+	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
+	struct imx_fracn_gppll_rate_table t;
+
+	clk_fracn_gppll_calc_settings(pll, rate, *prate, &t);
+
+	return t.rate;
+}
+
 static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
@@ -242,11 +401,11 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
 				    unsigned long prate)
 {
 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
-	const struct imx_fracn_gppll_rate_table *rate;
+	struct imx_fracn_gppll_rate_table rate;
 	u32 tmp, pll_div, ana_mfn;
 	int ret;
 
-	rate = imx_get_pll_settings(pll, drate);
+	clk_fracn_gppll_calc_settings(pll, drate, prate, &rate);
 
 	/* Hardware control select disable. PLL is control by register */
 	tmp = readl_relaxed(pll->base + PLL_CTRL);
@@ -266,13 +425,13 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp &= ~CLKMUX_BYPASS;
 	writel_relaxed(tmp, pll->base + PLL_CTRL);
 
-	pll_div = FIELD_PREP(PLL_RDIV_MASK, rate->rdiv) | rate->odiv |
-		FIELD_PREP(PLL_MFI_MASK, rate->mfi);
+	pll_div = FIELD_PREP(PLL_RDIV_MASK, rate.rdiv) | rate.odiv |
+		FIELD_PREP(PLL_MFI_MASK, rate.mfi);
 	writel_relaxed(pll_div, pll->base + PLL_DIV);
 	readl(pll->base + PLL_DIV);
 	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
-		writel_relaxed(rate->mfd, pll->base + PLL_DENOMINATOR);
-		writel_relaxed(FIELD_PREP(PLL_MFN_MASK, rate->mfn), pll->base + PLL_NUMERATOR);
+		writel_relaxed(rate.mfd, pll->base + PLL_DENOMINATOR);
+		writel_relaxed(FIELD_PREP(PLL_MFN_MASK, rate.mfn), pll->base + PLL_NUMERATOR);
 		readl(pll->base + PLL_NUMERATOR);
 	}
 
@@ -296,7 +455,7 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
 	ana_mfn = readl_relaxed(pll->base + PLL_STATUS);
 	ana_mfn = FIELD_GET(PLL_MFN_MASK, ana_mfn);
 
-	WARN(ana_mfn != rate->mfn, "ana_mfn != rate->mfn\n");
+	WARN(ana_mfn != rate.mfn, "ana_mfn != rate->mfn\n");
 
 	return 0;
 }
-- 
2.34.1

Re: [RFC PATCH 3/3] clk: imx: clk-fracn-gppll: Support dynamic rates

[Sascha Hauer]

Hi Alexander,

On Mon, Feb 10, 2025 at 05:00:11PM +0100, Alexander Stein wrote:
> The fracn gppll PLL so far only supports rates from a rate table passed
> during initialization. Calculating PLL settings dynamically helps audio
> applications to get their desired rates, so support for this is added
> in this patch.
> 
> The strategy to get to the PLL setting for a rate is:
> 
> - The rate table is searched for suitable rates, so for standard rates the
>   same settings are used as without this patch
> - Then try to only adjust mfn, on fractional PLLs only, which specifies
>   the fractional part of the PLL. This setting can be changed without
>   glitches on the output and is therefore preferred

I wonder if this part is worth it. There might be cases in which a
glitch free switch is required, but without being able to enforce
a glitch free switch we can't rely on it.

Also this makes the result depend on the current PLL settings, so the
result is no longer reproducible. I.e. switching from a fari away
frequency to the desired frequency might yield in different settings
than switching from a nearby frequency to the desired frequency.

Finally I think the glitch free switch doesn't work currently, because
the PLL is fully disabled and re-enabled unconditionally in
clk_fracn_gppll_set_rate().

That said, glitch free switching would be great to have sometimes.

> - As a last resort the best settings are calculated dynamically
> 
> Implementation is inspired by commit b09c68dc57c9d ("clk: imx: pll14xx:
> Support dynamic rates")
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---

...

> +			if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
> +				if (!dist) {
> +					/* Disable fractional part upon exact match */
> +					mfd = 1;
> +					mfn = 0;
> +				} else {
> +					mfd = 100;
> +					mfd = clamp(mfd, PLL_MFD_MIN, PLL_MFN_MAX);

With mfd = 100 this clamp looks like a no-op. Do we need this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH 3/3] clk: imx: clk-fracn-gppll: Support dynamic rates

[Peng Fan]

On Mon, Feb 10, 2025 at 05:00:11PM +0100, Alexander Stein wrote:
>The fracn gppll PLL so far only supports rates from a rate table passed
>during initialization. Calculating PLL settings dynamically helps audio
>applications to get their desired rates, so support for this is added
>in this patch.
>
>The strategy to get to the PLL setting for a rate is:
>
>- The rate table is searched for suitable rates, so for standard rates the
>  same settings are used as without this patch
>- Then try to only adjust mfn, on fractional PLLs only, which specifies
>  the fractional part of the PLL. This setting can be changed without
>  glitches on the output and is therefore preferred
>- As a last resort the best settings are calculated dynamically
>
>Implementation is inspired by commit b09c68dc57c9d ("clk: imx: pll14xx:
>Support dynamic rates")

Not a fan of this, we have seen issues that not able to calculate
out exact audio frequency with dynamic rates. But anyway
if your setup needs this feature, it is ok to add.

>
>Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>---
>This is the first time I'm touching PLL code, I might be missing things
>or not being aware of important aspects when it comes to PLL.
>Thus this is a RFC
>
> drivers/clk/imx/clk-fracn-gppll.c | 203 ++++++++++++++++++++++++++----
> 1 file changed, 181 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/clk/imx/clk-fracn-gppll.c b/drivers/clk/imx/clk-fracn-gppll.c
>index a7d57fbe93196..68c5b4a95efbe 100644
>--- a/drivers/clk/imx/clk-fracn-gppll.c
>+++ b/drivers/clk/imx/clk-fracn-gppll.c
>@@ -25,6 +25,12 @@
> 
> #define PLL_NUMERATOR		0x40
> #define PLL_MFN_MASK		GENMASK(31, 2)
>+#define PLL_MFI_MIN		0x2
>+#define PLL_MFI_MAX		0x1ff
>+#define PLL_MFN_MIN		0x0
>+#define PLL_MFN_MAX		0x3fffffff
>+#define PLL_MFD_MIN		0x1
>+#define PLL_MFD_MAX		0x3fffffff
> 
> #define PLL_DENOMINATOR		0x50
> #define PLL_MFD_MASK		GENMASK(29, 0)
>@@ -134,21 +140,6 @@ imx_get_pll_settings(struct clk_fracn_gppll *pll, unsigned long rate)
> 	return NULL;
> }
> 
>-static long clk_fracn_gppll_round_rate(struct clk_hw *hw, unsigned long rate,
>-				       unsigned long *prate)
>-{
>-	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
>-	const struct imx_fracn_gppll_rate_table *rate_table = pll->rate_table;
>-	int i;
>-
>-	/* Assuming rate_table is in descending order */
>-	for (i = 0; i < pll->rate_count; i++)
>-		if (rate >= rate_table[i].rate)
>-			return rate_table[i].rate;
>-
>-	/* return minimum supported value */
>-	return rate_table[pll->rate_count - 1].rate;
>-}
> 
> static long clk_fracn_gppll_calc_rate(struct clk_fracn_gppll *pll, u32 mfn,
> 				      u32 mfd, u32 mfi, u32 rdiv, u32 odiv,
>@@ -202,6 +193,174 @@ static long clk_fracn_gppll_calc_rate(struct clk_fracn_gppll *pll, u32 mfn,
> 	return (unsigned long)fvco;
> }
> 
>+static u32 clk_fracn_gppll_calc_mfi(int rdiv, unsigned long fvco,
>+				    unsigned long fref)
>+{
>+	u32 mfi;
>+
>+	/* fvco = fref / rdiv * mfi */
>+	mfi = DIV_ROUND_CLOSEST(fvco * rdiv, fref);
>+	return clamp_t(u32, mfi, PLL_MFI_MIN, PLL_MFI_MAX);
>+}
>+
>+static long clk_fracn_gppll_calc_mfn(int mfd, int mfi, int odiv, int rdiv,
>+				     unsigned long rate, unsigned long prate)
>+{
>+	unsigned long tmp;
>+	long mfn;
>+
>+	/* calc mfn = ((rate * odiv * rdiv / prate) - mfi) * mfd */
>+	tmp = rate * odiv * rdiv - (mfi * prate);
>+	mfn = DIV_ROUND_CLOSEST(tmp * mfd, prate);
>+	return clamp_t(long, mfn, PLL_MFN_MIN, PLL_MFN_MAX);
>+}
>+
>+static void clk_fracn_gppll_calc_settings(struct clk_fracn_gppll *pll, unsigned long rate,
>+					  unsigned long prate, struct imx_fracn_gppll_rate_table *t)
>+{
>+	u32 pll_denominator, pll_numerator, pll_div;
>+	u32 mfi, mfn, mfd, rdiv, odiv;
>+	long fout, rate_min, rate_max, dist, best = LONG_MAX;
>+	const struct imx_fracn_gppll_rate_table *tt;
>+
>+	/*
>+	 * PLL constrains:
>+	 *
>+	 * a) 1 <= MFN <= 0x3fffffff (fractional only)
>+	 * b) 1 <= MFD <= 0x3fffffff (fractional only)
>+	 * c) 2 <= MFI <= 0x1ff
>+	 * d) 1 <= RDIV <= 7
>+	 * e) 2 <= ODIV <= 255
>+	 * f) -2 <= MFN/MFD <= 2
>+	 * g) 20MHz <= (Fref / rdiv) <= 40MHz
>+	 * h) 2.5GHz <= Fvco <= 5Ghz
>+	 *
>+	 * Fvco = (Fref / rdiv) * (MFI + MFN / MFD)
>+	 * Fout = Fvco / odiv
>+	 */
>+
>+	/* First try if we can get the desired rate from one of the static entries */
>+	tt = imx_get_pll_settings(pll, rate);
>+	if (tt) {
>+		pr_debug("%s: in=%ld, want=%ld, Using PLL setting from table\n",
>+			 clk_hw_get_name(&pll->hw), prate, rate);
>+		t->rate = tt->rate;
>+		t->mfi = tt->mfi;
>+		t->mfn = tt->mfn;
>+		t->mfd = tt->mfd;
>+		t->rdiv = tt->rdiv;
>+		t->odiv = tt->odiv;
>+		return;
>+	}
>+
>+	/* glitch free MFN adjustment only for fractional PLL */
>+	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
>+		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
>+
>+		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
>+		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
>+
>+		pll_div = readl_relaxed(pll->base + PLL_DIV);
>+		mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
>+		rdiv = FIELD_GET(PLL_RDIV_MASK, pll_div);
>+		odiv = FIELD_GET(PLL_ODIV_MASK, pll_div);
>+
>+		/* Then see if we can get the desired rate by only adjusting MFN (glitch free) */
>+		rate_min = clk_fracn_gppll_calc_rate(pll, PLL_MFN_MIN, mfd, mfi, rdiv, odiv, prate);
>+		rate_max = clk_fracn_gppll_calc_rate(pll, PLL_MFN_MAX, mfd, mfi, rdiv, odiv, prate);
>+
>+		if (rate >= rate_min && rate <= rate_max) {
>+			mfn = clk_fracn_gppll_calc_mfn(mfd, mfi, odiv, rdiv, rate, prate);
>+			pr_debug("%s: in=%ld, want=%ld Only adjust mfn %ld -> %d\n",
>+				 clk_hw_get_name(&pll->hw), prate, rate,
>+				 FIELD_GET(PLL_MFN_MASK, pll_numerator), mfn);
>+			fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
>+			t->rate = (unsigned int)fout;
>+			t->mfi = mfi;
>+			t->mfn = mfn;
>+			t->mfd = mfd;
>+			t->rdiv = rdiv;
>+			t->odiv = odiv;
>+			return;
>+		}
>+	}
>+
>+	/* Finally calculate best values */
>+	for (rdiv = 1; rdiv <= 7; rdiv++) {
>+		if ((prate / rdiv) < 20000000)
>+			continue;
>+		if ((prate / rdiv) > 40000000)
>+			continue;
>+
>+		for (odiv = 2; odiv <= 255; odiv++) {
>+			mfi = clk_fracn_gppll_calc_mfi(rdiv, rate * odiv, prate);
>+			mfd = 1;
>+			mfn = 0;
>+
>+			/* Try integer PLL part only first */
>+			fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
>+			if (fout * odiv < 2500000000UL)
>+				continue;
>+			if (fout * odiv > 5000000000UL)
>+				continue;
>+
>+			if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
>+				if (!dist) {
>+					/* Disable fractional part upon exact match */
>+					mfd = 1;
>+					mfn = 0;
>+				} else {
>+					mfd = 100;

why hardcode mfd to 100?

>+					mfd = clamp(mfd, PLL_MFD_MIN, PLL_MFN_MAX);
>+
>+					mfn = clk_fracn_gppll_calc_mfn(mfd, mfi, odiv, rdiv, rate, prate);
>+					if ((mfn / mfd) > 2)
>+						continue;
>+
>+					fout = clk_fracn_gppll_calc_rate(pll, mfn, mfd, mfi, rdiv, odiv, prate);
>+					if (fout * odiv < 2500000000)
>+						continue;
>+					if (fout * odiv > 5000000000)
>+						continue;
>+				}
>+			} else {
>+				mfd = 0;
>+				mfn = 0;
>+			}
>+
>+			/* best match */
>+			dist = abs((long)rate - (long)fout);
>+			if (dist < best) {
>+				best = dist;
>+				t->rate = (unsigned int)fout;
>+				t->mfi = mfi;
>+				t->mfn = mfn;
>+				t->mfd = mfd;
>+				t->rdiv = rdiv;
>+				t->odiv = odiv;
>+
>+				if (!dist)
>+					goto found;
>+			}
>+		}
>+	}
>+found:
>+	pr_debug("%s: in=%lu, want=%lu got=%u (mfi=%u mfn=%u mfd=%u rdiv=%u odiv=%u)\n",
>+		 clk_hw_get_name(&pll->hw), prate, rate, t->rate, t->mfi, t->mfn, t->mfd,
>+		 t->rdiv, t->odiv);
>+}
>+
>+static long clk_fracn_gppll_round_rate(struct clk_hw *hw, unsigned long rate,
>+				       unsigned long *prate)
>+{
>+	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
>+	struct imx_fracn_gppll_rate_table t;
>+
>+	clk_fracn_gppll_calc_settings(pll, rate, *prate, &t);
>+
>+	return t.rate;
>+}
>+
> static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> {
> 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
>@@ -242,11 +401,11 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
> 				    unsigned long prate)
> {
> 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
>-	const struct imx_fracn_gppll_rate_table *rate;
>+	struct imx_fracn_gppll_rate_table rate;
> 	u32 tmp, pll_div, ana_mfn;
> 	int ret;
> 
>-	rate = imx_get_pll_settings(pll, drate);
>+	clk_fracn_gppll_calc_settings(pll, drate, prate, &rate);
> 
> 	/* Hardware control select disable. PLL is control by register */
> 	tmp = readl_relaxed(pll->base + PLL_CTRL);
>@@ -266,13 +425,13 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
> 	tmp &= ~CLKMUX_BYPASS;
> 	writel_relaxed(tmp, pll->base + PLL_CTRL);

As Sascha mentioned, set rate will first disable output, power down pll based
on current implementation. So this patch might not get tested.

Regards
Peng

> 
>-	pll_div = FIELD_PREP(PLL_RDIV_MASK, rate->rdiv) | rate->odiv |
>-		FIELD_PREP(PLL_MFI_MASK, rate->mfi);
>+	pll_div = FIELD_PREP(PLL_RDIV_MASK, rate.rdiv) | rate.odiv |
>+		FIELD_PREP(PLL_MFI_MASK, rate.mfi);
> 	writel_relaxed(pll_div, pll->base + PLL_DIV);
> 	readl(pll->base + PLL_DIV);
> 	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
>-		writel_relaxed(rate->mfd, pll->base + PLL_DENOMINATOR);
>-		writel_relaxed(FIELD_PREP(PLL_MFN_MASK, rate->mfn), pll->base + PLL_NUMERATOR);
>+		writel_relaxed(rate.mfd, pll->base + PLL_DENOMINATOR);
>+		writel_relaxed(FIELD_PREP(PLL_MFN_MASK, rate.mfn), pll->base + PLL_NUMERATOR);
> 		readl(pll->base + PLL_NUMERATOR);
> 	}
> 
>@@ -296,7 +455,7 @@ static int clk_fracn_gppll_set_rate(struct clk_hw *hw, unsigned long drate,
> 	ana_mfn = readl_relaxed(pll->base + PLL_STATUS);
> 	ana_mfn = FIELD_GET(PLL_MFN_MASK, ana_mfn);
> 
>-	WARN(ana_mfn != rate->mfn, "ana_mfn != rate->mfn\n");
>+	WARN(ana_mfn != rate.mfn, "ana_mfn != rate->mfn\n");
> 
> 	return 0;
> }
>-- 
>2.34.1
>

Re: [RFC PATCH 1/3] clk: imx: clk-fracn-gppll: Do not access num/denom register for integer PLL

[Peng Fan]

On Mon, Feb 10, 2025 at 05:00:09PM +0100, Alexander Stein wrote:
>Similar to clk_fracn_gppll_set_rate(), do not access the numerator and
>denominator register for integer PLL. Set MFD/MFN to 0 instead, so the
>table lookup will match.

For integer, the calculation will not take mfn/mfi into consideration.

Do you see this in test or just code inspection?

>See i.MX93 RM section 74.5.2.1 (PLL memory map) for ARMPLL, addresses
>0x40 and 0x50 are not listed/reserved.
>
>Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>---
> drivers/clk/imx/clk-fracn-gppll.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/clk/imx/clk-fracn-gppll.c b/drivers/clk/imx/clk-fracn-gppll.c
>index 85771afd4698a..3aef548110e25 100644
>--- a/drivers/clk/imx/clk-fracn-gppll.c
>+++ b/drivers/clk/imx/clk-fracn-gppll.c
>@@ -154,17 +154,24 @@ static unsigned long clk_fracn_gppll_recalc_rate(struct clk_hw *hw, unsigned lon
> {
> 	struct clk_fracn_gppll *pll = to_clk_fracn_gppll(hw);
> 	const struct imx_fracn_gppll_rate_table *rate_table = pll->rate_table;
>-	u32 pll_numerator, pll_denominator, pll_div;
>+	u32 pll_div;
> 	u32 mfi, mfn, mfd, rdiv, odiv;
> 	u64 fvco = parent_rate;
> 	long rate = 0;
> 	int i;
> 
>-	pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
>-	mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
>+	if (pll->flags & CLK_FRACN_GPPLL_FRACN) {
>+		u32 pll_numerator, pll_denominator;
>+
>+		pll_numerator = readl_relaxed(pll->base + PLL_NUMERATOR);
>+		mfn = FIELD_GET(PLL_MFN_MASK, pll_numerator);
> 
>-	pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
>-	mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
>+		pll_denominator = readl_relaxed(pll->base + PLL_DENOMINATOR);
>+		mfd = FIELD_GET(PLL_MFD_MASK, pll_denominator);
>+	} else {
>+		mfd = 0;
>+		mfn = 0;
>+	}

Reading the registers for ARM PLL, should be 0 even RM not list.
But it is good that using a check here. So I am ok with this change.

Regards,
Peng

> 
> 	pll_div = readl_relaxed(pll->base + PLL_DIV);
> 	mfi = FIELD_GET(PLL_MFI_MASK, pll_div);
>-- 
>2.34.1
>