[PATCH v2 0/8] clk: i.MX: PLL14xx: Support dynamic rates

[PATCH v2 0/8] clk: i.MX: PLL14xx: Support dynamic rates

[Sascha Hauer]

Changes since v1:
- reword commit message for 2/8
- Add missing includes (kernel test robot)
- Fix printf format (kernel test robot)
- Add Reviewed-by: tags

Sascha Hauer (8):
  clk: imx: pll14xx: Use register defines consistently
  clk: imx: pll14xx: Drop wrong shifting
  clk: imx: pll14xx: Use FIELD_GET/FIELD_PREP
  clk: imx: pll14xx: consolidate rate calculation
  clk: imx: pll14xx: name variables after usage
  clk: imx: pll14xx: explicitly return lowest rate
  clk: imx: pll14xx: Add pr_fmt
  clk: imx: pll14xx: Support dynamic rates

 drivers/clk/imx/clk-pll14xx.c | 287 +++++++++++++++++++++++-----------
 1 file changed, 194 insertions(+), 93 deletions(-)

-- 
2.30.2

[PATCH v2 1/8] clk: imx: pll14xx: Use register defines consistently

[Sascha Hauer]

The driver has defines for the registers, but they are mostly unused.
Use the defines consistently throughout the driver. While at it rename
DIV_CTL to DIV_CTL0 because that's the name in the reference manual.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
---
 drivers/clk/imx/clk-pll14xx.c | 49 ++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 2b5ed86b9dbbb..cae64d750672e 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -15,7 +15,8 @@
 #include "clk.h"
 
 #define GNRL_CTL	0x0
-#define DIV_CTL		0x4
+#define DIV_CTL0	0x4
+#define DIV_CTL1	0x8
 #define LOCK_STATUS	BIT(31)
 #define LOCK_SEL_MASK	BIT(29)
 #define CLKE_MASK	BIT(11)
@@ -122,7 +123,7 @@ static unsigned long clk_pll1416x_recalc_rate(struct clk_hw *hw,
 	u32 mdiv, pdiv, sdiv, pll_div;
 	u64 fvco = parent_rate;
 
-	pll_div = readl_relaxed(pll->base + 4);
+	pll_div = readl_relaxed(pll->base + DIV_CTL0);
 	mdiv = (pll_div & MDIV_MASK) >> MDIV_SHIFT;
 	pdiv = (pll_div & PDIV_MASK) >> PDIV_SHIFT;
 	sdiv = (pll_div & SDIV_MASK) >> SDIV_SHIFT;
@@ -141,8 +142,8 @@ static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw,
 	short int kdiv;
 	u64 fvco = parent_rate;
 
-	pll_div_ctl0 = readl_relaxed(pll->base + 4);
-	pll_div_ctl1 = readl_relaxed(pll->base + 8);
+	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
+	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
 	mdiv = (pll_div_ctl0 & MDIV_MASK) >> MDIV_SHIFT;
 	pdiv = (pll_div_ctl0 & PDIV_MASK) >> PDIV_SHIFT;
 	sdiv = (pll_div_ctl0 & SDIV_MASK) >> SDIV_SHIFT;
@@ -172,7 +173,7 @@ static int clk_pll14xx_wait_lock(struct clk_pll14xx *pll)
 {
 	u32 val;
 
-	return readl_poll_timeout(pll->base, val, val & LOCK_STATUS, 0,
+	return readl_poll_timeout(pll->base + GNRL_CTL, val, val & LOCK_STATUS, 0,
 			LOCK_TIMEOUT_US);
 }
 
@@ -191,32 +192,32 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	tmp = readl_relaxed(pll->base + 4);
+	tmp = readl_relaxed(pll->base + DIV_CTL0);
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
 		tmp &= ~(SDIV_MASK) << SDIV_SHIFT;
 		tmp |= rate->sdiv << SDIV_SHIFT;
-		writel_relaxed(tmp, pll->base + 4);
+		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
 		return 0;
 	}
 
 	/* Bypass clock and set lock to pll output lock */
-	tmp = readl_relaxed(pll->base);
+	tmp = readl_relaxed(pll->base + GNRL_CTL);
 	tmp |= LOCK_SEL_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	/* Enable RST */
 	tmp &= ~RST_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	/* Enable BYPASS */
 	tmp |= BYPASS_MASK;
-	writel(tmp, pll->base);
+	writel(tmp, pll->base + GNRL_CTL);
 
 	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
 		(rate->sdiv << SDIV_SHIFT);
-	writel_relaxed(div_val, pll->base + 0x4);
+	writel_relaxed(div_val, pll->base + DIV_CTL0);
 
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
@@ -228,7 +229,7 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	/* Disable RST */
 	tmp |= RST_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	/* Wait Lock */
 	ret = clk_pll14xx_wait_lock(pll);
@@ -237,7 +238,7 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	/* Bypass */
 	tmp &= ~BYPASS_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	return 0;
 }
@@ -257,32 +258,32 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	tmp = readl_relaxed(pll->base + 4);
+	tmp = readl_relaxed(pll->base + DIV_CTL0);
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
 		tmp &= ~(SDIV_MASK) << SDIV_SHIFT;
 		tmp |= rate->sdiv << SDIV_SHIFT;
-		writel_relaxed(tmp, pll->base + 4);
+		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
 		tmp = rate->kdiv << KDIV_SHIFT;
-		writel_relaxed(tmp, pll->base + 8);
+		writel_relaxed(tmp, pll->base + DIV_CTL1);
 
 		return 0;
 	}
 
 	/* Enable RST */
-	tmp = readl_relaxed(pll->base);
+	tmp = readl_relaxed(pll->base + GNRL_CTL);
 	tmp &= ~RST_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	/* Enable BYPASS */
 	tmp |= BYPASS_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
 		(rate->sdiv << SDIV_SHIFT);
-	writel_relaxed(div_val, pll->base + 0x4);
-	writel_relaxed(rate->kdiv << KDIV_SHIFT, pll->base + 0x8);
+	writel_relaxed(div_val, pll->base + DIV_CTL0);
+	writel_relaxed(rate->kdiv << KDIV_SHIFT, pll->base + DIV_CTL1);
 
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
@@ -294,7 +295,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	/* Disable RST */
 	tmp |= RST_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	/* Wait Lock*/
 	ret = clk_pll14xx_wait_lock(pll);
@@ -303,7 +304,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	/* Bypass */
 	tmp &= ~BYPASS_MASK;
-	writel_relaxed(tmp, pll->base);
+	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
 	return 0;
 }
-- 
2.30.2

[PATCH v2 2/8] clk: imx: pll14xx: Drop wrong shifting

[Sascha Hauer]

The code tries to mask the bits in SDIV_MASK from 'tmp'. SDIV_MASK
already contains the shifted value, so shifting it again is wrong.
No functional change though as SDIV_SHIFT is zero.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
---

Notes:
    Changes since v1:
    - Avoid the word 'fix' in the subject to prevent stable guys from picking this patch
    - Add note to the commit message that there's no functional change

 drivers/clk/imx/clk-pll14xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index cae64d750672e..b295d8a049009 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -195,7 +195,7 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp = readl_relaxed(pll->base + DIV_CTL0);
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
-		tmp &= ~(SDIV_MASK) << SDIV_SHIFT;
+		tmp &= ~SDIV_MASK;
 		tmp |= rate->sdiv << SDIV_SHIFT;
 		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
@@ -261,7 +261,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp = readl_relaxed(pll->base + DIV_CTL0);
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
-		tmp &= ~(SDIV_MASK) << SDIV_SHIFT;
+		tmp &= ~SDIV_MASK;
 		tmp |= rate->sdiv << SDIV_SHIFT;
 		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
-- 
2.30.2

[PATCH v2 3/8] clk: imx: pll14xx: Use FIELD_GET/FIELD_PREP

[Sascha Hauer]

Linux has these marvelous FIELD_GET/FIELD_PREP macros for easy access
to bitfields in registers. Use them and remove the now unused *_SHIFT
defines.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
---

Notes:
    Changes since v1:
    - Explicitly include linux/bitfield.h for FIELD_PREP/FIELD_GET

 drivers/clk/imx/clk-pll14xx.c | 40 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index b295d8a049009..fabb380b87305 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -3,6 +3,7 @@
  * Copyright 2017-2018 NXP.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
@@ -22,13 +23,9 @@
 #define CLKE_MASK	BIT(11)
 #define RST_MASK	BIT(9)
 #define BYPASS_MASK	BIT(4)
-#define MDIV_SHIFT	12
 #define MDIV_MASK	GENMASK(21, 12)
-#define PDIV_SHIFT	4
 #define PDIV_MASK	GENMASK(9, 4)
-#define SDIV_SHIFT	0
 #define SDIV_MASK	GENMASK(2, 0)
-#define KDIV_SHIFT	0
 #define KDIV_MASK	GENMASK(15, 0)
 
 #define LOCK_TIMEOUT_US		10000
@@ -124,9 +121,9 @@ static unsigned long clk_pll1416x_recalc_rate(struct clk_hw *hw,
 	u64 fvco = parent_rate;
 
 	pll_div = readl_relaxed(pll->base + DIV_CTL0);
-	mdiv = (pll_div & MDIV_MASK) >> MDIV_SHIFT;
-	pdiv = (pll_div & PDIV_MASK) >> PDIV_SHIFT;
-	sdiv = (pll_div & SDIV_MASK) >> SDIV_SHIFT;
+	mdiv = FIELD_GET(MDIV_MASK, pll_div);
+	pdiv = FIELD_GET(PDIV_MASK, pll_div);
+	sdiv = FIELD_GET(SDIV_MASK, pll_div);
 
 	fvco *= mdiv;
 	do_div(fvco, pdiv << sdiv);
@@ -144,10 +141,10 @@ static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw,
 
 	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
 	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
-	mdiv = (pll_div_ctl0 & MDIV_MASK) >> MDIV_SHIFT;
-	pdiv = (pll_div_ctl0 & PDIV_MASK) >> PDIV_SHIFT;
-	sdiv = (pll_div_ctl0 & SDIV_MASK) >> SDIV_SHIFT;
-	kdiv = pll_div_ctl1 & KDIV_MASK;
+	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
+	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
+	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
+	kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1);
 
 	/* fvco = (m * 65536 + k) * Fin / (p * 65536) */
 	fvco *= (mdiv * 65536 + kdiv);
@@ -163,8 +160,8 @@ static inline bool clk_pll14xx_mp_change(const struct imx_pll14xx_rate_table *ra
 {
 	u32 old_mdiv, old_pdiv;
 
-	old_mdiv = (pll_div & MDIV_MASK) >> MDIV_SHIFT;
-	old_pdiv = (pll_div & PDIV_MASK) >> PDIV_SHIFT;
+	old_mdiv = FIELD_GET(MDIV_MASK, pll_div);
+	old_pdiv = FIELD_GET(PDIV_MASK, pll_div);
 
 	return rate->mdiv != old_mdiv || rate->pdiv != old_pdiv;
 }
@@ -196,7 +193,7 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
 		tmp &= ~SDIV_MASK;
-		tmp |= rate->sdiv << SDIV_SHIFT;
+		tmp |= FIELD_PREP(SDIV_MASK, rate->sdiv);
 		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
 		return 0;
@@ -215,8 +212,8 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp |= BYPASS_MASK;
 	writel(tmp, pll->base + GNRL_CTL);
 
-	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
-		(rate->sdiv << SDIV_SHIFT);
+	div_val = FIELD_PREP(MDIV_MASK, rate->mdiv) | FIELD_PREP(PDIV_MASK, rate->pdiv) |
+		FIELD_PREP(SDIV_MASK, rate->sdiv);
 	writel_relaxed(div_val, pll->base + DIV_CTL0);
 
 	/*
@@ -262,10 +259,10 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	if (!clk_pll14xx_mp_change(rate, tmp)) {
 		tmp &= ~SDIV_MASK;
-		tmp |= rate->sdiv << SDIV_SHIFT;
+		tmp |= FIELD_PREP(SDIV_MASK, rate->sdiv);
 		writel_relaxed(tmp, pll->base + DIV_CTL0);
 
-		tmp = rate->kdiv << KDIV_SHIFT;
+		tmp = FIELD_PREP(KDIV_MASK, rate->kdiv);
 		writel_relaxed(tmp, pll->base + DIV_CTL1);
 
 		return 0;
@@ -280,10 +277,11 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp |= BYPASS_MASK;
 	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
-	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
-		(rate->sdiv << SDIV_SHIFT);
+	div_val = FIELD_PREP(MDIV_MASK, rate->mdiv) |
+		  FIELD_PREP(PDIV_MASK, rate->pdiv) |
+		  FIELD_PREP(SDIV_MASK, rate->sdiv);
 	writel_relaxed(div_val, pll->base + DIV_CTL0);
-	writel_relaxed(rate->kdiv << KDIV_SHIFT, pll->base + DIV_CTL1);
+	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
 
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
-- 
2.30.2

[PATCH v2 4/8] clk: imx: pll14xx: consolidate rate calculation

[Sascha Hauer]

The PLL driver has support for two different PLLs: The pll1416x and
the pll1443x. The latter has support for an additional kdiv value.
recalc_rate can be the same calculation when kdiv is assumed to be zero
for the PLL which doesn't support that value.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/imx/clk-pll14xx.c | 59 +++++++++++++++--------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index fabb380b87305..ebd5d888fea6d 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -97,6 +97,20 @@ static const struct imx_pll14xx_rate_table *imx_get_pll_settings(
 	return NULL;
 }
 
+static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv,
+			      int sdiv, int kdiv, unsigned long prate)
+{
+	u64 fvco = prate;
+
+	/* fvco = (m * 65536 + k) * Fin / (p * 65536) */
+	fvco *= (mdiv * 65536 + kdiv);
+	pdiv *= 65536;
+
+	do_div(fvco, pdiv << sdiv);
+
+	return fvco;
+}
+
 static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
 			unsigned long *prate)
 {
@@ -113,46 +127,25 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
 	return rate_table[i - 1].rate;
 }
 
-static unsigned long clk_pll1416x_recalc_rate(struct clk_hw *hw,
-						  unsigned long parent_rate)
-{
-	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
-	u32 mdiv, pdiv, sdiv, pll_div;
-	u64 fvco = parent_rate;
-
-	pll_div = readl_relaxed(pll->base + DIV_CTL0);
-	mdiv = FIELD_GET(MDIV_MASK, pll_div);
-	pdiv = FIELD_GET(PDIV_MASK, pll_div);
-	sdiv = FIELD_GET(SDIV_MASK, pll_div);
-
-	fvco *= mdiv;
-	do_div(fvco, pdiv << sdiv);
-
-	return fvco;
-}
-
-static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw,
+static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
 						  unsigned long parent_rate)
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
-	u32 mdiv, pdiv, sdiv, pll_div_ctl0, pll_div_ctl1;
-	short int kdiv;
-	u64 fvco = parent_rate;
+	u32 mdiv, pdiv, sdiv, kdiv, pll_div_ctl0, pll_div_ctl1;
 
 	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
-	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
 	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
 	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
 	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
-	kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1);
 
-	/* fvco = (m * 65536 + k) * Fin / (p * 65536) */
-	fvco *= (mdiv * 65536 + kdiv);
-	pdiv *= 65536;
-
-	do_div(fvco, pdiv << sdiv);
+	if (pll->type == PLL_1443X) {
+		pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
+		kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1);
+	} else {
+		kdiv = 0;
+	}
 
-	return fvco;
+	return pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, parent_rate);
 }
 
 static inline bool clk_pll14xx_mp_change(const struct imx_pll14xx_rate_table *rate,
@@ -363,20 +356,20 @@ static const struct clk_ops clk_pll1416x_ops = {
 	.prepare	= clk_pll14xx_prepare,
 	.unprepare	= clk_pll14xx_unprepare,
 	.is_prepared	= clk_pll14xx_is_prepared,
-	.recalc_rate	= clk_pll1416x_recalc_rate,
+	.recalc_rate	= clk_pll14xx_recalc_rate,
 	.round_rate	= clk_pll14xx_round_rate,
 	.set_rate	= clk_pll1416x_set_rate,
 };
 
 static const struct clk_ops clk_pll1416x_min_ops = {
-	.recalc_rate	= clk_pll1416x_recalc_rate,
+	.recalc_rate	= clk_pll14xx_recalc_rate,
 };
 
 static const struct clk_ops clk_pll1443x_ops = {
 	.prepare	= clk_pll14xx_prepare,
 	.unprepare	= clk_pll14xx_unprepare,
 	.is_prepared	= clk_pll14xx_is_prepared,
-	.recalc_rate	= clk_pll1443x_recalc_rate,
+	.recalc_rate	= clk_pll14xx_recalc_rate,
 	.round_rate	= clk_pll14xx_round_rate,
 	.set_rate	= clk_pll1443x_set_rate,
 };
-- 
2.30.2

Re: [PATCH v2 4/8] clk: imx: pll14xx: consolidate rate calculation

[Abel Vesa]

On 22-02-25 09:29:33, Sascha Hauer wrote:
> The PLL driver has support for two different PLLs: The pll1416x and
> the pll1443x. The latter has support for an additional kdiv value.
> recalc_rate can be the same calculation when kdiv is assumed to be zero
> for the PLL which doesn't support that value.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
>  drivers/clk/imx/clk-pll14xx.c | 59 +++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index fabb380b87305..ebd5d888fea6d 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -97,6 +97,20 @@ static const struct imx_pll14xx_rate_table *imx_get_pll_settings(
>  	return NULL;
>  }
>  
> +static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv,
> +			      int sdiv, int kdiv, unsigned long prate)
> +{
> +	u64 fvco = prate;
> +
> +	/* fvco = (m * 65536 + k) * Fin / (p * 65536) */
> +	fvco *= (mdiv * 65536 + kdiv);
> +	pdiv *= 65536;
> +
> +	do_div(fvco, pdiv << sdiv);
> +
> +	return fvco;
> +}
> +
>  static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
>  			unsigned long *prate)
>  {
> @@ -113,46 +127,25 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
>  	return rate_table[i - 1].rate;
>  }
>  
> -static unsigned long clk_pll1416x_recalc_rate(struct clk_hw *hw,
> -						  unsigned long parent_rate)
> -{
> -	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> -	u32 mdiv, pdiv, sdiv, pll_div;
> -	u64 fvco = parent_rate;
> -
> -	pll_div = readl_relaxed(pll->base + DIV_CTL0);
> -	mdiv = FIELD_GET(MDIV_MASK, pll_div);
> -	pdiv = FIELD_GET(PDIV_MASK, pll_div);
> -	sdiv = FIELD_GET(SDIV_MASK, pll_div);
> -
> -	fvco *= mdiv;
> -	do_div(fvco, pdiv << sdiv);
> -
> -	return fvco;
> -}
> -
> -static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw,
> +static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
>  						  unsigned long parent_rate)
>  {
>  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> -	u32 mdiv, pdiv, sdiv, pll_div_ctl0, pll_div_ctl1;
> -	short int kdiv;
> -	u64 fvco = parent_rate;
> +	u32 mdiv, pdiv, sdiv, kdiv, pll_div_ctl0, pll_div_ctl1;
>  
>  	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> -	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
>  	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
>  	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
>  	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
> -	kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1);
>  
> -	/* fvco = (m * 65536 + k) * Fin / (p * 65536) */
> -	fvco *= (mdiv * 65536 + kdiv);
> -	pdiv *= 65536;
> -
> -	do_div(fvco, pdiv << sdiv);
> +	if (pll->type == PLL_1443X) {
> +		pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> +		kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1);
> +	} else {
> +		kdiv = 0;
> +	}
>  
> -	return fvco;
> +	return pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, parent_rate);
>  }
>  
>  static inline bool clk_pll14xx_mp_change(const struct imx_pll14xx_rate_table *rate,
> @@ -363,20 +356,20 @@ static const struct clk_ops clk_pll1416x_ops = {
>  	.prepare	= clk_pll14xx_prepare,
>  	.unprepare	= clk_pll14xx_unprepare,
>  	.is_prepared	= clk_pll14xx_is_prepared,
> -	.recalc_rate	= clk_pll1416x_recalc_rate,
> +	.recalc_rate	= clk_pll14xx_recalc_rate,
>  	.round_rate	= clk_pll14xx_round_rate,
>  	.set_rate	= clk_pll1416x_set_rate,
>  };
>  
>  static const struct clk_ops clk_pll1416x_min_ops = {
> -	.recalc_rate	= clk_pll1416x_recalc_rate,
> +	.recalc_rate	= clk_pll14xx_recalc_rate,
>  };
>  
>  static const struct clk_ops clk_pll1443x_ops = {
>  	.prepare	= clk_pll14xx_prepare,
>  	.unprepare	= clk_pll14xx_unprepare,
>  	.is_prepared	= clk_pll14xx_is_prepared,
> -	.recalc_rate	= clk_pll1443x_recalc_rate,
> +	.recalc_rate	= clk_pll14xx_recalc_rate,
>  	.round_rate	= clk_pll14xx_round_rate,
>  	.set_rate	= clk_pll1443x_set_rate,
>  };
> -- 
> 2.30.2
>

[PATCH v2 5/8] clk: imx: pll14xx: name variables after usage

[Sascha Hauer]

In clk_pll1443x_set_rate() 'tmp' is used for the content of different
registers which makes it a bit hard to follow. Use different variables
named after the registers to make it clearer. No functional change
intended.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/imx/clk-pll14xx.c | 42 +++++++++++++++++------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index ebd5d888fea6d..b464e1155e25b 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -238,7 +238,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
 	const struct imx_pll14xx_rate_table *rate;
-	u32 tmp, div_val;
+	u32 gnrl_ctl, div_ctl0;
 	int ret;
 
 	rate = imx_get_pll_settings(pll, drate);
@@ -248,32 +248,32 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	tmp = readl_relaxed(pll->base + DIV_CTL0);
+	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
 
-	if (!clk_pll14xx_mp_change(rate, tmp)) {
-		tmp &= ~SDIV_MASK;
-		tmp |= FIELD_PREP(SDIV_MASK, rate->sdiv);
-		writel_relaxed(tmp, pll->base + DIV_CTL0);
+	if (!clk_pll14xx_mp_change(rate, div_ctl0)) {
+		div_ctl0 &= ~SDIV_MASK;
+		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate->sdiv);
+		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
 
-		tmp = FIELD_PREP(KDIV_MASK, rate->kdiv);
-		writel_relaxed(tmp, pll->base + DIV_CTL1);
+		writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv),
+			       pll->base + DIV_CTL1);
 
 		return 0;
 	}
 
 	/* Enable RST */
-	tmp = readl_relaxed(pll->base + GNRL_CTL);
-	tmp &= ~RST_MASK;
-	writel_relaxed(tmp, pll->base + GNRL_CTL);
+	gnrl_ctl = readl_relaxed(pll->base + GNRL_CTL);
+	gnrl_ctl &= ~RST_MASK;
+	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
 	/* Enable BYPASS */
-	tmp |= BYPASS_MASK;
-	writel_relaxed(tmp, pll->base + GNRL_CTL);
+	gnrl_ctl |= BYPASS_MASK;
+	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
-	div_val = FIELD_PREP(MDIV_MASK, rate->mdiv) |
-		  FIELD_PREP(PDIV_MASK, rate->pdiv) |
-		  FIELD_PREP(SDIV_MASK, rate->sdiv);
-	writel_relaxed(div_val, pll->base + DIV_CTL0);
+	div_ctl0 = FIELD_PREP(MDIV_MASK, rate->mdiv) |
+		   FIELD_PREP(PDIV_MASK, rate->pdiv) |
+		   FIELD_PREP(SDIV_MASK, rate->sdiv);
+	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
 	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
 
 	/*
@@ -285,8 +285,8 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	udelay(3);
 
 	/* Disable RST */
-	tmp |= RST_MASK;
-	writel_relaxed(tmp, pll->base + GNRL_CTL);
+	gnrl_ctl |= RST_MASK;
+	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
 	/* Wait Lock*/
 	ret = clk_pll14xx_wait_lock(pll);
@@ -294,8 +294,8 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		return ret;
 
 	/* Bypass */
-	tmp &= ~BYPASS_MASK;
-	writel_relaxed(tmp, pll->base + GNRL_CTL);
+	gnrl_ctl &= ~BYPASS_MASK;
+	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH v2 5/8] clk: imx: pll14xx: name variables after usage

[Abel Vesa]

On 22-02-25 09:29:34, Sascha Hauer wrote:
> In clk_pll1443x_set_rate() 'tmp' is used for the content of different
> registers which makes it a bit hard to follow. Use different variables
> named after the registers to make it clearer. No functional change
> intended.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
>  drivers/clk/imx/clk-pll14xx.c | 42 +++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index ebd5d888fea6d..b464e1155e25b 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -238,7 +238,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  {
>  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
>  	const struct imx_pll14xx_rate_table *rate;
> -	u32 tmp, div_val;
> +	u32 gnrl_ctl, div_ctl0;
>  	int ret;
>  
>  	rate = imx_get_pll_settings(pll, drate);
> @@ -248,32 +248,32 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  		return -EINVAL;
>  	}
>  
> -	tmp = readl_relaxed(pll->base + DIV_CTL0);
> +	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
>  
> -	if (!clk_pll14xx_mp_change(rate, tmp)) {
> -		tmp &= ~SDIV_MASK;
> -		tmp |= FIELD_PREP(SDIV_MASK, rate->sdiv);
> -		writel_relaxed(tmp, pll->base + DIV_CTL0);
> +	if (!clk_pll14xx_mp_change(rate, div_ctl0)) {
> +		div_ctl0 &= ~SDIV_MASK;
> +		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate->sdiv);
> +		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
>  
> -		tmp = FIELD_PREP(KDIV_MASK, rate->kdiv);
> -		writel_relaxed(tmp, pll->base + DIV_CTL1);
> +		writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv),
> +			       pll->base + DIV_CTL1);
>  
>  		return 0;
>  	}
>  
>  	/* Enable RST */
> -	tmp = readl_relaxed(pll->base + GNRL_CTL);
> -	tmp &= ~RST_MASK;
> -	writel_relaxed(tmp, pll->base + GNRL_CTL);
> +	gnrl_ctl = readl_relaxed(pll->base + GNRL_CTL);
> +	gnrl_ctl &= ~RST_MASK;
> +	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>  
>  	/* Enable BYPASS */
> -	tmp |= BYPASS_MASK;
> -	writel_relaxed(tmp, pll->base + GNRL_CTL);
> +	gnrl_ctl |= BYPASS_MASK;
> +	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>  
> -	div_val = FIELD_PREP(MDIV_MASK, rate->mdiv) |
> -		  FIELD_PREP(PDIV_MASK, rate->pdiv) |
> -		  FIELD_PREP(SDIV_MASK, rate->sdiv);
> -	writel_relaxed(div_val, pll->base + DIV_CTL0);
> +	div_ctl0 = FIELD_PREP(MDIV_MASK, rate->mdiv) |
> +		   FIELD_PREP(PDIV_MASK, rate->pdiv) |
> +		   FIELD_PREP(SDIV_MASK, rate->sdiv);
> +	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
>  	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
>  
>  	/*
> @@ -285,8 +285,8 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	udelay(3);
>  
>  	/* Disable RST */
> -	tmp |= RST_MASK;
> -	writel_relaxed(tmp, pll->base + GNRL_CTL);
> +	gnrl_ctl |= RST_MASK;
> +	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>  
>  	/* Wait Lock*/
>  	ret = clk_pll14xx_wait_lock(pll);
> @@ -294,8 +294,8 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  		return ret;
>  
>  	/* Bypass */
> -	tmp &= ~BYPASS_MASK;
> -	writel_relaxed(tmp, pll->base + GNRL_CTL);
> +	gnrl_ctl &= ~BYPASS_MASK;
> +	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>  
>  	return 0;
>  }
> -- 
> 2.30.2
>

[PATCH v2 6/8] clk: imx: pll14xx: explicitly return lowest rate

[Sascha Hauer]

clk_pll14xx_round_rate() returns the lowest rate by indexing into
the rate table with the variable i. i is actually pll->rate_count
as this is the value we come out of the loop with. Use pll->rate_count
explicitly to make it a bit more clear what is being done. While at
it fix a typo in the comment. No functional change.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/imx/clk-pll14xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index b464e1155e25b..af4c979e70a64 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -118,13 +118,13 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
 	const struct imx_pll14xx_rate_table *rate_table = pll->rate_table;
 	int i;
 
-	/* Assumming rate_table is in descending order */
+	/* 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[i - 1].rate;
+	return rate_table[pll->rate_count - 1].rate;
 }
 
 static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
-- 
2.30.2

Re: [PATCH v2 6/8] clk: imx: pll14xx: explicitly return lowest rate

[Abel Vesa]

On 22-02-25 09:29:35, Sascha Hauer wrote:
> clk_pll14xx_round_rate() returns the lowest rate by indexing into
> the rate table with the variable i. i is actually pll->rate_count
> as this is the value we come out of the loop with. Use pll->rate_count
> explicitly to make it a bit more clear what is being done. While at
> it fix a typo in the comment. No functional change.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
>  drivers/clk/imx/clk-pll14xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b464e1155e25b..af4c979e70a64 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -118,13 +118,13 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
>  	const struct imx_pll14xx_rate_table *rate_table = pll->rate_table;
>  	int i;
>  
> -	/* Assumming rate_table is in descending order */
> +	/* 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[i - 1].rate;
> +	return rate_table[pll->rate_count - 1].rate;
>  }
>  
>  static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
> -- 
> 2.30.2
>

[PATCH v2 7/8] clk: imx: pll14xx: Add pr_fmt

[Sascha Hauer]

Print all messages from within the pll14xx driver with a common
prefix using pr_fmt. No need to print function names anymore, so
drop them from the messages.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Notes:
    Changes since v1:
    - Drop __func__ argument for which the %s was removed

 drivers/clk/imx/clk-pll14xx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index af4c979e70a64..646e0ce7d6242 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -3,6 +3,8 @@
  * Copyright 2017-2018 NXP.
  */
 
+#define pr_fmt(fmt) "pll14xx: " fmt
+
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/clk-provider.h>
@@ -177,8 +179,8 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	rate = imx_get_pll_settings(pll, drate);
 	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
-		       drate, clk_hw_get_name(hw));
+		pr_err("Invalid rate %lu for pll clk %s\n", drate,
+		       clk_hw_get_name(hw));
 		return -EINVAL;
 	}
 
@@ -404,8 +406,7 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
 		init.ops = &clk_pll1443x_ops;
 		break;
 	default:
-		pr_err("%s: Unknown pll type for pll clk %s\n",
-		       __func__, name);
+		pr_err("Unknown pll type for pll clk %s\n", name);
 		kfree(pll);
 		return ERR_PTR(-EINVAL);
 	}
@@ -424,8 +425,7 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
 
 	ret = clk_hw_register(dev, hw);
 	if (ret) {
-		pr_err("%s: failed to register pll %s %d\n",
-			__func__, name, ret);
+		pr_err("failed to register pll %s %d\n", name, ret);
 		kfree(pll);
 		return ERR_PTR(ret);
 	}
-- 
2.30.2

Re: [PATCH v2 7/8] clk: imx: pll14xx: Add pr_fmt

[Abel Vesa]

On 22-02-25 09:29:36, Sascha Hauer wrote:
> Print all messages from within the pll14xx driver with a common
> prefix using pr_fmt. No need to print function names anymore, so
> drop them from the messages.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
> 
> Notes:
>     Changes since v1:
>     - Drop __func__ argument for which the %s was removed
> 
>  drivers/clk/imx/clk-pll14xx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index af4c979e70a64..646e0ce7d6242 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -3,6 +3,8 @@
>   * Copyright 2017-2018 NXP.
>   */
>  
> +#define pr_fmt(fmt) "pll14xx: " fmt
> +
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/clk-provider.h>
> @@ -177,8 +179,8 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
>  
>  	rate = imx_get_pll_settings(pll, drate);
>  	if (!rate) {
> -		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
> -		       drate, clk_hw_get_name(hw));
> +		pr_err("Invalid rate %lu for pll clk %s\n", drate,
> +		       clk_hw_get_name(hw));
>  		return -EINVAL;
>  	}
>  
> @@ -404,8 +406,7 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
>  		init.ops = &clk_pll1443x_ops;
>  		break;
>  	default:
> -		pr_err("%s: Unknown pll type for pll clk %s\n",
> -		       __func__, name);
> +		pr_err("Unknown pll type for pll clk %s\n", name);
>  		kfree(pll);
>  		return ERR_PTR(-EINVAL);
>  	}
> @@ -424,8 +425,7 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
>  
>  	ret = clk_hw_register(dev, hw);
>  	if (ret) {
> -		pr_err("%s: failed to register pll %s %d\n",
> -			__func__, name, ret);
> +		pr_err("failed to register pll %s %d\n", name, ret);
>  		kfree(pll);
>  		return ERR_PTR(ret);
>  	}
> -- 
> 2.30.2
>

[PATCH v2 8/8] clk: imx: pll14xx: Support dynamic rates

[Sascha Hauer]

The pll1443x 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:

- First try to only adjust kdiv which specifies the fractional part of the PLL.
  This setting can be changed without glitches on the output and is therefore
  preferred
- When that isn't possible then the rate table is searched for suitable rates,
  so for standard rates the same settings are used as without this patch
- As a last resort the best settings are calculated dynamically

The code in this patch is based on patches from Adrian Alonso <adrian.alonso@nxp.com>
and Mads Bligaard Nielsen <bli@bang-olufsen.dk>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/imx/clk-pll14xx.c | 143 ++++++++++++++++++++++++++++++----
 1 file changed, 126 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 646e0ce7d6242..eef0f3b693ed9 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -29,6 +29,8 @@
 #define PDIV_MASK	GENMASK(9, 4)
 #define SDIV_MASK	GENMASK(2, 0)
 #define KDIV_MASK	GENMASK(15, 0)
+#define KDIV_MIN	SHRT_MIN
+#define KDIV_MAX	SHRT_MAX
 
 #define LOCK_TIMEOUT_US		10000
 
@@ -113,7 +115,106 @@ static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv,
 	return fvco;
 }
 
-static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
+static long pll1443x_calc_kdiv(int mdiv, int pdiv, int sdiv,
+		unsigned long rate, unsigned long prate)
+{
+	long kdiv;
+
+	/* calc kdiv = round(rate * pdiv * 65536 * 2^sdiv / prate) - (mdiv * 65536) */
+	kdiv = ((rate * ((pdiv * 65536) << sdiv) + prate / 2) / prate) - (mdiv * 65536);
+
+	return clamp_t(short, kdiv, KDIV_MIN, KDIV_MAX);
+}
+
+static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rate,
+				      unsigned long prate, struct imx_pll14xx_rate_table *t)
+{
+	u32 pll_div_ctl0, pll_div_ctl1;
+	int mdiv, pdiv, sdiv, kdiv;
+	long fvco, rate_min, rate_max, dist, best = LONG_MAX;
+	const struct imx_pll14xx_rate_table *tt;
+
+	/*
+	 * Fractional PLL constrains:
+	 *
+	 * a) 6MHz <= prate <= 25MHz
+	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
+	 * c) 64 <= m <= 1023
+	 * d) 0 <= s <= 6
+	 * e) -32768 <= k <= 32767
+	 *
+	 * fvco = (m * 65536 + k) * prate / (p * 65536)
+	 */
+
+	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
+	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
+	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
+	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
+	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
+
+	/* First see if we can get the desired rate by only adjusting kdiv (glitch free) */
+	rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate);
+	rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate);
+
+	if (rate >= rate_min && rate <= rate_max) {
+		kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
+		pr_debug("%s: in=%ld, want=%ld Only adjust kdiv %ld -> %d\n",
+			 clk_hw_get_name(&pll->hw), prate, rate,
+			 FIELD_GET(KDIV_MASK, pll_div_ctl1), kdiv);
+		fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
+		t->rate = (unsigned int)fvco;
+		t->mdiv = mdiv;
+		t->pdiv = pdiv;
+		t->sdiv = sdiv;
+		t->kdiv = kdiv;
+		return;
+	}
+
+	/* Then 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->mdiv = tt->mdiv;
+		t->pdiv = tt->pdiv;
+		t->sdiv = tt->sdiv;
+		t->kdiv = tt->kdiv;
+		return;
+	}
+
+	/* Finally calculate best values */
+	for (pdiv = 1; pdiv <= 7; pdiv++) {
+		for (sdiv = 0; sdiv <= 6; sdiv++) {
+			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
+			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
+			mdiv = clamp(mdiv, 64, 1023);
+
+			kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
+			fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
+
+			/* best match */
+			dist = abs((long)rate - (long)fvco);
+			if (dist < best) {
+				best = dist;
+				t->rate = (unsigned int)fvco;
+				t->mdiv = mdiv;
+				t->pdiv = pdiv;
+				t->sdiv = sdiv;
+				t->kdiv = kdiv;
+
+				if (!dist)
+					goto found;
+			}
+		}
+	}
+found:
+	pr_debug("%s: in=%ld, want=%ld got=%d (pdiv=%d sdiv=%d mdiv=%d kdiv=%d)\n",
+		 clk_hw_get_name(&pll->hw), prate, rate, t->rate, t->pdiv, t->sdiv,
+		 t->mdiv, t->kdiv);
+}
+
+static long clk_pll1416x_round_rate(struct clk_hw *hw, unsigned long rate,
 			unsigned long *prate)
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
@@ -129,6 +230,17 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
 	return rate_table[pll->rate_count - 1].rate;
 }
 
+static long clk_pll1443x_round_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long *prate)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+	struct imx_pll14xx_rate_table t;
+
+	imx_pll14xx_calc_settings(pll, rate, *prate, &t);
+
+	return t.rate;
+}
+
 static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
 						  unsigned long parent_rate)
 {
@@ -239,25 +351,21 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 				 unsigned long prate)
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
-	const struct imx_pll14xx_rate_table *rate;
+	struct imx_pll14xx_rate_table rate;
 	u32 gnrl_ctl, div_ctl0;
 	int ret;
 
-	rate = imx_get_pll_settings(pll, drate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
-			drate, clk_hw_get_name(hw));
-		return -EINVAL;
-	}
+	imx_pll14xx_calc_settings(pll, drate, prate, &rate);
 
 	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
 
-	if (!clk_pll14xx_mp_change(rate, div_ctl0)) {
+	if (!clk_pll14xx_mp_change(&rate, div_ctl0)) {
+		/* only sdiv and/or kdiv changed - no need to RESET PLL */
 		div_ctl0 &= ~SDIV_MASK;
-		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate->sdiv);
+		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
 		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
 
-		writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv),
+		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
 			       pll->base + DIV_CTL1);
 
 		return 0;
@@ -272,11 +380,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	gnrl_ctl |= BYPASS_MASK;
 	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
-	div_ctl0 = FIELD_PREP(MDIV_MASK, rate->mdiv) |
-		   FIELD_PREP(PDIV_MASK, rate->pdiv) |
-		   FIELD_PREP(SDIV_MASK, rate->sdiv);
+	div_ctl0 = FIELD_PREP(MDIV_MASK, rate.mdiv) |
+		   FIELD_PREP(PDIV_MASK, rate.pdiv) |
+		   FIELD_PREP(SDIV_MASK, rate.sdiv);
 	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
-	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
+
+	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
 
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
@@ -359,7 +468,7 @@ static const struct clk_ops clk_pll1416x_ops = {
 	.unprepare	= clk_pll14xx_unprepare,
 	.is_prepared	= clk_pll14xx_is_prepared,
 	.recalc_rate	= clk_pll14xx_recalc_rate,
-	.round_rate	= clk_pll14xx_round_rate,
+	.round_rate	= clk_pll1416x_round_rate,
 	.set_rate	= clk_pll1416x_set_rate,
 };
 
@@ -372,7 +481,7 @@ static const struct clk_ops clk_pll1443x_ops = {
 	.unprepare	= clk_pll14xx_unprepare,
 	.is_prepared	= clk_pll14xx_is_prepared,
 	.recalc_rate	= clk_pll14xx_recalc_rate,
-	.round_rate	= clk_pll14xx_round_rate,
+	.round_rate	= clk_pll1443x_round_rate,
 	.set_rate	= clk_pll1443x_set_rate,
 };
 
-- 
2.30.2

Re: [PATCH v2 8/8] clk: imx: pll14xx: Support dynamic rates

[Abel Vesa]

On 22-02-25 09:29:37, Sascha Hauer wrote:
> The pll1443x 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:
> 
> - First try to only adjust kdiv which specifies the fractional part of the PLL.
>   This setting can be changed without glitches on the output and is therefore
>   preferred
> - When that isn't possible then the rate table is searched for suitable rates,
>   so for standard rates the same settings are used as without this patch
> - As a last resort the best settings are calculated dynamically
> 
> The code in this patch is based on patches from Adrian Alonso <adrian.alonso@nxp.com>
> and Mads Bligaard Nielsen <bli@bang-olufsen.dk>

Hmm, I wish this was also possible for SSCG plls.

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/clk/imx/clk-pll14xx.c | 143 ++++++++++++++++++++++++++++++----
>  1 file changed, 126 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 646e0ce7d6242..eef0f3b693ed9 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -29,6 +29,8 @@
>  #define PDIV_MASK	GENMASK(9, 4)
>  #define SDIV_MASK	GENMASK(2, 0)
>  #define KDIV_MASK	GENMASK(15, 0)
> +#define KDIV_MIN	SHRT_MIN
> +#define KDIV_MAX	SHRT_MAX
>  
>  #define LOCK_TIMEOUT_US		10000
>  
> @@ -113,7 +115,106 @@ static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv,
>  	return fvco;
>  }
>  
> -static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
> +static long pll1443x_calc_kdiv(int mdiv, int pdiv, int sdiv,
> +		unsigned long rate, unsigned long prate)
> +{
> +	long kdiv;
> +
> +	/* calc kdiv = round(rate * pdiv * 65536 * 2^sdiv / prate) - (mdiv * 65536) */
> +	kdiv = ((rate * ((pdiv * 65536) << sdiv) + prate / 2) / prate) - (mdiv * 65536);
> +
> +	return clamp_t(short, kdiv, KDIV_MIN, KDIV_MAX);
> +}
> +
> +static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rate,
> +				      unsigned long prate, struct imx_pll14xx_rate_table *t)
> +{
> +	u32 pll_div_ctl0, pll_div_ctl1;
> +	int mdiv, pdiv, sdiv, kdiv;
> +	long fvco, rate_min, rate_max, dist, best = LONG_MAX;
> +	const struct imx_pll14xx_rate_table *tt;
> +
> +	/*
> +	 * Fractional PLL constrains:
> +	 *
> +	 * a) 6MHz <= prate <= 25MHz
> +	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> +	 * c) 64 <= m <= 1023
> +	 * d) 0 <= s <= 6
> +	 * e) -32768 <= k <= 32767
> +	 *
> +	 * fvco = (m * 65536 + k) * prate / (p * 65536)
> +	 */
> +
> +	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> +	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
> +	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
> +	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
> +	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> +
> +	/* First see if we can get the desired rate by only adjusting kdiv (glitch free) */
> +	rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate);
> +	rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate);
> +
> +	if (rate >= rate_min && rate <= rate_max) {
> +		kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> +		pr_debug("%s: in=%ld, want=%ld Only adjust kdiv %ld -> %d\n",
> +			 clk_hw_get_name(&pll->hw), prate, rate,
> +			 FIELD_GET(KDIV_MASK, pll_div_ctl1), kdiv);
> +		fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> +		t->rate = (unsigned int)fvco;
> +		t->mdiv = mdiv;
> +		t->pdiv = pdiv;
> +		t->sdiv = sdiv;
> +		t->kdiv = kdiv;
> +		return;
> +	}
> +
> +	/* Then try if we can get the desired rate from one of the static entries */
> +	tt = imx_get_pll_settings(pll, rate);

Shouldn't we try this one first? Maybe we don't need to compute kdiv at
all.

> +	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->mdiv = tt->mdiv;
> +		t->pdiv = tt->pdiv;
> +		t->sdiv = tt->sdiv;
> +		t->kdiv = tt->kdiv;
> +		return;
> +	}
> +
> +	/* Finally calculate best values */
> +	for (pdiv = 1; pdiv <= 7; pdiv++) {
> +		for (sdiv = 0; sdiv <= 6; sdiv++) {
> +			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
> +			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> +			mdiv = clamp(mdiv, 64, 1023);
> +
> +			kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> +			fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> +
> +			/* best match */
> +			dist = abs((long)rate - (long)fvco);
> +			if (dist < best) {
> +				best = dist;
> +				t->rate = (unsigned int)fvco;
> +				t->mdiv = mdiv;
> +				t->pdiv = pdiv;
> +				t->sdiv = sdiv;
> +				t->kdiv = kdiv;
> +
> +				if (!dist)
> +					goto found;
> +			}
> +		}
> +	}
> +found:
> +	pr_debug("%s: in=%ld, want=%ld got=%d (pdiv=%d sdiv=%d mdiv=%d kdiv=%d)\n",
> +		 clk_hw_get_name(&pll->hw), prate, rate, t->rate, t->pdiv, t->sdiv,
> +		 t->mdiv, t->kdiv);
> +}
> +
> +static long clk_pll1416x_round_rate(struct clk_hw *hw, unsigned long rate,
>  			unsigned long *prate)
>  {
>  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> @@ -129,6 +230,17 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
>  	return rate_table[pll->rate_count - 1].rate;
>  }
>  
> +static long clk_pll1443x_round_rate(struct clk_hw *hw, unsigned long rate,
> +			unsigned long *prate)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +	struct imx_pll14xx_rate_table t;
> +
> +	imx_pll14xx_calc_settings(pll, rate, *prate, &t);
> +
> +	return t.rate;
> +}
> +
>  static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
>  						  unsigned long parent_rate)
>  {
> @@ -239,25 +351,21 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  				 unsigned long prate)
>  {
>  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> -	const struct imx_pll14xx_rate_table *rate;
> +	struct imx_pll14xx_rate_table rate;
>  	u32 gnrl_ctl, div_ctl0;
>  	int ret;
>  
> -	rate = imx_get_pll_settings(pll, drate);
> -	if (!rate) {
> -		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
> -			drate, clk_hw_get_name(hw));
> -		return -EINVAL;
> -	}
> +	imx_pll14xx_calc_settings(pll, drate, prate, &rate);
>  
>  	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
>  
> -	if (!clk_pll14xx_mp_change(rate, div_ctl0)) {
> +	if (!clk_pll14xx_mp_change(&rate, div_ctl0)) {
> +		/* only sdiv and/or kdiv changed - no need to RESET PLL */
>  		div_ctl0 &= ~SDIV_MASK;
> -		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate->sdiv);
> +		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
>  		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
>  
> -		writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv),
> +		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
>  			       pll->base + DIV_CTL1);
>  
>  		return 0;
> @@ -272,11 +380,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	gnrl_ctl |= BYPASS_MASK;
>  	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>  
> -	div_ctl0 = FIELD_PREP(MDIV_MASK, rate->mdiv) |
> -		   FIELD_PREP(PDIV_MASK, rate->pdiv) |
> -		   FIELD_PREP(SDIV_MASK, rate->sdiv);
> +	div_ctl0 = FIELD_PREP(MDIV_MASK, rate.mdiv) |
> +		   FIELD_PREP(PDIV_MASK, rate.pdiv) |
> +		   FIELD_PREP(SDIV_MASK, rate.sdiv);
>  	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> -	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
> +
> +	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
>  
>  	/*
>  	 * According to SPEC, t3 - t2 need to be greater than
> @@ -359,7 +468,7 @@ static const struct clk_ops clk_pll1416x_ops = {
>  	.unprepare	= clk_pll14xx_unprepare,
>  	.is_prepared	= clk_pll14xx_is_prepared,
>  	.recalc_rate	= clk_pll14xx_recalc_rate,
> -	.round_rate	= clk_pll14xx_round_rate,
> +	.round_rate	= clk_pll1416x_round_rate,
>  	.set_rate	= clk_pll1416x_set_rate,
>  };
>  
> @@ -372,7 +481,7 @@ static const struct clk_ops clk_pll1443x_ops = {
>  	.unprepare	= clk_pll14xx_unprepare,
>  	.is_prepared	= clk_pll14xx_is_prepared,
>  	.recalc_rate	= clk_pll14xx_recalc_rate,
> -	.round_rate	= clk_pll14xx_round_rate,
> +	.round_rate	= clk_pll1443x_round_rate,
>  	.set_rate	= clk_pll1443x_set_rate,
>  };
>  
> -- 
> 2.30.2
>

Re: [PATCH v2 8/8] clk: imx: pll14xx: Support dynamic rates

[Abel Vesa]

On 22-02-25 16:13:06, Abel Vesa wrote:
> On 22-02-25 09:29:37, Sascha Hauer wrote:
> > The pll1443x 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:
> > 
> > - First try to only adjust kdiv which specifies the fractional part of the PLL.
> >   This setting can be changed without glitches on the output and is therefore
> >   preferred
> > - When that isn't possible then the rate table is searched for suitable rates,
> >   so for standard rates the same settings are used as without this patch
> > - As a last resort the best settings are calculated dynamically
> > 
> > The code in this patch is based on patches from Adrian Alonso <adrian.alonso@nxp.com>
> > and Mads Bligaard Nielsen <bli@bang-olufsen.dk>
> 
> Hmm, I wish this was also possible for SSCG plls.
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/clk/imx/clk-pll14xx.c | 143 ++++++++++++++++++++++++++++++----
> >  1 file changed, 126 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> > index 646e0ce7d6242..eef0f3b693ed9 100644
> > --- a/drivers/clk/imx/clk-pll14xx.c
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -29,6 +29,8 @@
> >  #define PDIV_MASK	GENMASK(9, 4)
> >  #define SDIV_MASK	GENMASK(2, 0)
> >  #define KDIV_MASK	GENMASK(15, 0)
> > +#define KDIV_MIN	SHRT_MIN
> > +#define KDIV_MAX	SHRT_MAX
> >  
> >  #define LOCK_TIMEOUT_US		10000
> >  
> > @@ -113,7 +115,106 @@ static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv,
> >  	return fvco;
> >  }
> >  
> > -static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
> > +static long pll1443x_calc_kdiv(int mdiv, int pdiv, int sdiv,
> > +		unsigned long rate, unsigned long prate)
> > +{
> > +	long kdiv;
> > +
> > +	/* calc kdiv = round(rate * pdiv * 65536 * 2^sdiv / prate) - (mdiv * 65536) */
> > +	kdiv = ((rate * ((pdiv * 65536) << sdiv) + prate / 2) / prate) - (mdiv * 65536);
> > +
> > +	return clamp_t(short, kdiv, KDIV_MIN, KDIV_MAX);
> > +}
> > +
> > +static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rate,
> > +				      unsigned long prate, struct imx_pll14xx_rate_table *t)
> > +{
> > +	u32 pll_div_ctl0, pll_div_ctl1;
> > +	int mdiv, pdiv, sdiv, kdiv;
> > +	long fvco, rate_min, rate_max, dist, best = LONG_MAX;
> > +	const struct imx_pll14xx_rate_table *tt;
> > +
> > +	/*
> > +	 * Fractional PLL constrains:
> > +	 *
> > +	 * a) 6MHz <= prate <= 25MHz
> > +	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> > +	 * c) 64 <= m <= 1023
> > +	 * d) 0 <= s <= 6
> > +	 * e) -32768 <= k <= 32767
> > +	 *
> > +	 * fvco = (m * 65536 + k) * prate / (p * 65536)
> > +	 */
> > +
> > +	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> > +	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
> > +	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
> > +	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
> > +	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> > +
> > +	/* First see if we can get the desired rate by only adjusting kdiv (glitch free) */
> > +	rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate);
> > +	rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate);
> > +
> > +	if (rate >= rate_min && rate <= rate_max) {
> > +		kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> > +		pr_debug("%s: in=%ld, want=%ld Only adjust kdiv %ld -> %d\n",
> > +			 clk_hw_get_name(&pll->hw), prate, rate,
> > +			 FIELD_GET(KDIV_MASK, pll_div_ctl1), kdiv);
> > +		fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> > +		t->rate = (unsigned int)fvco;
> > +		t->mdiv = mdiv;
> > +		t->pdiv = pdiv;
> > +		t->sdiv = sdiv;
> > +		t->kdiv = kdiv;
> > +		return;
> > +	}
> > +
> > +	/* Then try if we can get the desired rate from one of the static entries */
> > +	tt = imx_get_pll_settings(pll, rate);
> 
> Shouldn't we try this one first? Maybe we don't need to compute kdiv at
> all.
> 

Ping.

> > +	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->mdiv = tt->mdiv;
> > +		t->pdiv = tt->pdiv;
> > +		t->sdiv = tt->sdiv;
> > +		t->kdiv = tt->kdiv;
> > +		return;
> > +	}
> > +
> > +	/* Finally calculate best values */
> > +	for (pdiv = 1; pdiv <= 7; pdiv++) {
> > +		for (sdiv = 0; sdiv <= 6; sdiv++) {
> > +			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
> > +			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> > +			mdiv = clamp(mdiv, 64, 1023);
> > +
> > +			kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> > +			fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> > +
> > +			/* best match */
> > +			dist = abs((long)rate - (long)fvco);
> > +			if (dist < best) {
> > +				best = dist;
> > +				t->rate = (unsigned int)fvco;
> > +				t->mdiv = mdiv;
> > +				t->pdiv = pdiv;
> > +				t->sdiv = sdiv;
> > +				t->kdiv = kdiv;
> > +
> > +				if (!dist)
> > +					goto found;
> > +			}
> > +		}
> > +	}
> > +found:
> > +	pr_debug("%s: in=%ld, want=%ld got=%d (pdiv=%d sdiv=%d mdiv=%d kdiv=%d)\n",
> > +		 clk_hw_get_name(&pll->hw), prate, rate, t->rate, t->pdiv, t->sdiv,
> > +		 t->mdiv, t->kdiv);
> > +}
> > +
> > +static long clk_pll1416x_round_rate(struct clk_hw *hw, unsigned long rate,
> >  			unsigned long *prate)
> >  {
> >  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > @@ -129,6 +230,17 @@ static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
> >  	return rate_table[pll->rate_count - 1].rate;
> >  }
> >  
> > +static long clk_pll1443x_round_rate(struct clk_hw *hw, unsigned long rate,
> > +			unsigned long *prate)
> > +{
> > +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > +	struct imx_pll14xx_rate_table t;
> > +
> > +	imx_pll14xx_calc_settings(pll, rate, *prate, &t);
> > +
> > +	return t.rate;
> > +}
> > +
> >  static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw,
> >  						  unsigned long parent_rate)
> >  {
> > @@ -239,25 +351,21 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
> >  				 unsigned long prate)
> >  {
> >  	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > -	const struct imx_pll14xx_rate_table *rate;
> > +	struct imx_pll14xx_rate_table rate;
> >  	u32 gnrl_ctl, div_ctl0;
> >  	int ret;
> >  
> > -	rate = imx_get_pll_settings(pll, drate);
> > -	if (!rate) {
> > -		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
> > -			drate, clk_hw_get_name(hw));
> > -		return -EINVAL;
> > -	}
> > +	imx_pll14xx_calc_settings(pll, drate, prate, &rate);
> >  
> >  	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> >  
> > -	if (!clk_pll14xx_mp_change(rate, div_ctl0)) {
> > +	if (!clk_pll14xx_mp_change(&rate, div_ctl0)) {
> > +		/* only sdiv and/or kdiv changed - no need to RESET PLL */
> >  		div_ctl0 &= ~SDIV_MASK;
> > -		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate->sdiv);
> > +		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
> >  		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> >  
> > -		writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv),
> > +		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
> >  			       pll->base + DIV_CTL1);
> >  
> >  		return 0;
> > @@ -272,11 +380,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
> >  	gnrl_ctl |= BYPASS_MASK;
> >  	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
> >  
> > -	div_ctl0 = FIELD_PREP(MDIV_MASK, rate->mdiv) |
> > -		   FIELD_PREP(PDIV_MASK, rate->pdiv) |
> > -		   FIELD_PREP(SDIV_MASK, rate->sdiv);
> > +	div_ctl0 = FIELD_PREP(MDIV_MASK, rate.mdiv) |
> > +		   FIELD_PREP(PDIV_MASK, rate.pdiv) |
> > +		   FIELD_PREP(SDIV_MASK, rate.sdiv);
> >  	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> > -	writel_relaxed(FIELD_PREP(KDIV_MASK, rate->kdiv), pll->base + DIV_CTL1);
> > +
> > +	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
> >  
> >  	/*
> >  	 * According to SPEC, t3 - t2 need to be greater than
> > @@ -359,7 +468,7 @@ static const struct clk_ops clk_pll1416x_ops = {
> >  	.unprepare	= clk_pll14xx_unprepare,
> >  	.is_prepared	= clk_pll14xx_is_prepared,
> >  	.recalc_rate	= clk_pll14xx_recalc_rate,
> > -	.round_rate	= clk_pll14xx_round_rate,
> > +	.round_rate	= clk_pll1416x_round_rate,
> >  	.set_rate	= clk_pll1416x_set_rate,
> >  };
> >  
> > @@ -372,7 +481,7 @@ static const struct clk_ops clk_pll1443x_ops = {
> >  	.unprepare	= clk_pll14xx_unprepare,
> >  	.is_prepared	= clk_pll14xx_is_prepared,
> >  	.recalc_rate	= clk_pll14xx_recalc_rate,
> > -	.round_rate	= clk_pll14xx_round_rate,
> > +	.round_rate	= clk_pll1443x_round_rate,
> >  	.set_rate	= clk_pll1443x_set_rate,
> >  };
> >  
> > -- 
> > 2.30.2
> >

Re: [PATCH v2 8/8] clk: imx: pll14xx: Support dynamic rates

[Sascha Hauer]

On Fri, Feb 25, 2022 at 04:13:06PM +0200, Abel Vesa wrote:
> On 22-02-25 09:29:37, Sascha Hauer wrote:
> > The pll1443x 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.
> > +	const struct imx_pll14xx_rate_table *tt;
> > +
> > +	/*
> > +	 * Fractional PLL constrains:
> > +	 *
> > +	 * a) 6MHz <= prate <= 25MHz
> > +	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> > +	 * c) 64 <= m <= 1023
> > +	 * d) 0 <= s <= 6
> > +	 * e) -32768 <= k <= 32767
> > +	 *
> > +	 * fvco = (m * 65536 + k) * prate / (p * 65536)
> > +	 */
> > +
> > +	pll_div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> > +	mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0);
> > +	pdiv = FIELD_GET(PDIV_MASK, pll_div_ctl0);
> > +	sdiv = FIELD_GET(SDIV_MASK, pll_div_ctl0);
> > +	pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> > +
> > +	/* First see if we can get the desired rate by only adjusting kdiv (glitch free) */
> > +	rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate);
> > +	rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate);
> > +
> > +	if (rate >= rate_min && rate <= rate_max) {
> > +		kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> > +		pr_debug("%s: in=%ld, want=%ld Only adjust kdiv %ld -> %d\n",
> > +			 clk_hw_get_name(&pll->hw), prate, rate,
> > +			 FIELD_GET(KDIV_MASK, pll_div_ctl1), kdiv);
> > +		fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> > +		t->rate = (unsigned int)fvco;
> > +		t->mdiv = mdiv;
> > +		t->pdiv = pdiv;
> > +		t->sdiv = sdiv;
> > +		t->kdiv = kdiv;
> > +		return;
> > +	}
> > +
> > +	/* Then try if we can get the desired rate from one of the static entries */
> > +	tt = imx_get_pll_settings(pll, rate);
> 
> Shouldn't we try this one first? Maybe we don't need to compute kdiv at
> all.

Sorry, missed that part of your mail.

The intention was to try the glitchfree path first so that we switch
glitchfree when possible. For most cases the order doesn't make a
difference, I think we can change it if you like.

There's one case in which we end up with a glitch where we could do a
glitchfree switch: We come from some rate to a rate which is nearly but
not exactly the rate from one of the table entries. From there we then
switch to the exact rate from the table entry. The algorithm below will
find other values than the ones in the table entries, so the PLL
switches to the same output frequency with completely different values.

I don't know if anyone ever hits this and then if he has a problem with
the PLL being restartet, so yes, we can change the order.

Sascha

> 
> > +	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->mdiv = tt->mdiv;
> > +		t->pdiv = tt->pdiv;
> > +		t->sdiv = tt->sdiv;
> > +		t->kdiv = tt->kdiv;
> > +		return;
> > +	}
> > +
> > +	/* Finally calculate best values */
> > +	for (pdiv = 1; pdiv <= 7; pdiv++) {
> > +		for (sdiv = 0; sdiv <= 6; sdiv++) {
> > +			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
> > +			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> > +			mdiv = clamp(mdiv, 64, 1023);
> > +
> > +			kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate);
> > +			fvco = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate);
> > +
> > +			/* best match */
> > +			dist = abs((long)rate - (long)fvco);
> > +			if (dist < best) {
> > +				best = dist;
> > +				t->rate = (unsigned int)fvco;
> > +				t->mdiv = mdiv;
> > +				t->pdiv = pdiv;
> > +				t->sdiv = sdiv;
> > +				t->kdiv = kdiv;
> > +
> > +				if (!dist)
> > +					goto found;
> > +			}
> > +		}
> > +	}

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