[PATCH v1 0/5] Add support for X1000 audio clocks

[PATCH v1 0/5] Add support for X1000 audio clocks

[Aidan MacDonald]

The first three patches of this series modify the Ingenic CGU driver to
allow the X1000's I2S divider to be modeled as a PLL clock. This is not
really true -- it's just a fractional divider -- but doing it this way
maximizes code reuse and avoids the need for a custom clock. (Thanks to
Zhou Yanjie & Paul Cercueil for the idea.)

The last two patches actually add the X1000 SoC's audio clocks.

Aidan MacDonald (5):
  clk: ingenic: Make PLL clock "od" field optional
  clk: ingenic: Make PLL clock enable_bit and stable_bit optional
  clk: ingenic: Add .set_rate_hook() for PLL clocks
  dt-bindings: ingenic,x1000-cgu: Add audio clocks
  clk: ingenic: Add X1000 audio clocks

 drivers/clk/ingenic/cgu.c                     | 45 +++++++++---
 drivers/clk/ingenic/cgu.h                     | 17 +++--
 drivers/clk/ingenic/x1000-cgu.c               | 69 +++++++++++++++++++
 include/dt-bindings/clock/ingenic,x1000-cgu.h |  4 ++
 4 files changed, 120 insertions(+), 15 deletions(-)

-- 
2.38.1

[PATCH v1 1/5] clk: ingenic: Make PLL clock "od" field optional

[Aidan MacDonald]

Add support for defining PLL clocks with od_bits = 0, meaning that
OD is fixed to 1 and there is no OD field in the register.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/clk/ingenic/cgu.c | 28 +++++++++++++++++++---------
 drivers/clk/ingenic/cgu.h |  3 ++-
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 861c50d6cb24..7dc2e2567d53 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	m += pll_info->m_offset;
 	n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
 	n += pll_info->n_offset;
-	od_enc = ctl >> pll_info->od_shift;
-	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
+
+	if (pll_info->od_bits > 0) {
+		od_enc = ctl >> pll_info->od_shift;
+		od_enc &= GENMASK(pll_info->od_bits - 1, 0);
+	}
 
 	if (pll_info->bypass_bit >= 0) {
 		ctl = readl(cgu->base + pll_info->bypass_reg);
@@ -108,12 +111,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 			return parent_rate;
 	}
 
-	for (od = 0; od < pll_info->od_max; od++) {
-		if (pll_info->od_encoding[od] == od_enc)
-			break;
+	if (pll_info->od_bits > 0) {
+		for (od = 0; od < pll_info->od_max; od++) {
+			if (pll_info->od_encoding[od] == od_enc)
+				break;
+		}
+		BUG_ON(od == pll_info->od_max);
+		od++;
+	} else {
+		/* OD is fixed to 1 if no OD field is present. */
+		od = 1;
 	}
-	BUG_ON(od == pll_info->od_max);
-	od++;
 
 	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
 		n * od);
@@ -215,8 +223,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
 	ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
 	ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
 
-	ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
-	ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
+	if (pll_info->od_bits > 0) {
+		ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
+		ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
+	}
 
 	writel(ctl, cgu->base + pll_info->reg);
 
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index 147b7df0d657..567142b584bb 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -33,7 +33,8 @@
  * @od_shift: the number of bits to shift the post-VCO divider value by (ie.
  *            the index of the lowest bit of the post-VCO divider value in
  *            the PLL's control register)
- * @od_bits: the size of the post-VCO divider field in bits
+ * @od_bits: the size of the post-VCO divider field in bits, or 0 if no
+ *	     OD field exists (then the OD is fixed to 1)
  * @od_max: the maximum post-VCO divider value
  * @od_encoding: a pointer to an array mapping post-VCO divider values to
  *               their encoded values in the PLL control register, or -1 for
-- 
2.38.1

[PATCH v1 2/5] clk: ingenic: Make PLL clock enable_bit and stable_bit optional

[Aidan MacDonald]

When the enable bit is undefined, the clock is assumed to be always
on and enable/disable is a no-op. When the stable bit is undefined,
the PLL stable check is a no-op.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/clk/ingenic/cgu.c | 14 +++++++++++++-
 drivers/clk/ingenic/cgu.h | 10 ++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 7dc2e2567d53..bbb55e8d8b55 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -190,6 +190,9 @@ static inline int ingenic_pll_check_stable(struct ingenic_cgu *cgu,
 {
 	u32 ctl;
 
+	if (pll_info->stable_bit < 0)
+		return 0;
+
 	return readl_poll_timeout(cgu->base + pll_info->reg, ctl,
 				  ctl & BIT(pll_info->stable_bit),
 				  0, 100 * USEC_PER_MSEC);
@@ -231,7 +234,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
 	writel(ctl, cgu->base + pll_info->reg);
 
 	/* If the PLL is enabled, verify that it's stable */
-	if (ctl & BIT(pll_info->enable_bit))
+	if (pll_info->enable_bit >= 0 && (ctl & BIT(pll_info->enable_bit)))
 		ret = ingenic_pll_check_stable(cgu, pll_info);
 
 	spin_unlock_irqrestore(&cgu->lock, flags);
@@ -249,6 +252,9 @@ static int ingenic_pll_enable(struct clk_hw *hw)
 	int ret;
 	u32 ctl;
 
+	if (pll_info->enable_bit < 0)
+		return 0;
+
 	spin_lock_irqsave(&cgu->lock, flags);
 	if (pll_info->bypass_bit >= 0) {
 		ctl = readl(cgu->base + pll_info->bypass_reg);
@@ -279,6 +285,9 @@ static void ingenic_pll_disable(struct clk_hw *hw)
 	unsigned long flags;
 	u32 ctl;
 
+	if (pll_info->enable_bit < 0)
+		return;
+
 	spin_lock_irqsave(&cgu->lock, flags);
 	ctl = readl(cgu->base + pll_info->reg);
 
@@ -296,6 +305,9 @@ static int ingenic_pll_is_enabled(struct clk_hw *hw)
 	const struct ingenic_cgu_pll_info *pll_info = &clk_info->pll;
 	u32 ctl;
 
+	if (pll_info->enable_bit < 0)
+		return true;
+
 	ctl = readl(cgu->base + pll_info->reg);
 
 	return !!(ctl & BIT(pll_info->enable_bit));
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index 567142b584bb..a5e44ca7f969 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -42,8 +42,10 @@
  * @bypass_reg: the offset of the bypass control register within the CGU
  * @bypass_bit: the index of the bypass bit in the PLL control register, or
  *              -1 if there is no bypass bit
- * @enable_bit: the index of the enable bit in the PLL control register
- * @stable_bit: the index of the stable bit in the PLL control register
+ * @enable_bit: the index of the enable bit in the PLL control register, or
+ *		-1 if there is no enable bit (ie, the PLL is always on)
+ * @stable_bit: the index of the stable bit in the PLL control register, or
+ *		-1 if there is no stable bit
  */
 struct ingenic_cgu_pll_info {
 	unsigned reg;
@@ -54,8 +56,8 @@ struct ingenic_cgu_pll_info {
 	u8 od_shift, od_bits, od_max;
 	unsigned bypass_reg;
 	s8 bypass_bit;
-	u8 enable_bit;
-	u8 stable_bit;
+	s8 enable_bit;
+	s8 stable_bit;
 	void (*calc_m_n_od)(const struct ingenic_cgu_pll_info *pll_info,
 			    unsigned long rate, unsigned long parent_rate,
 			    unsigned int *m, unsigned int *n, unsigned int *od);
-- 
2.38.1

[PATCH v1 3/5] clk: ingenic: Add .set_rate_hook() for PLL clocks

[Aidan MacDonald]

The set rate hook is called immediately after updating the clock
register but before the spinlock is released. This allows another
register to be updated alongside the main one, which is needed to
handle the I2S divider on some SoCs.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/clk/ingenic/cgu.c | 3 +++
 drivers/clk/ingenic/cgu.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index bbb55e8d8b55..574d5fe10fdf 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -233,6 +233,9 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
 
 	writel(ctl, cgu->base + pll_info->reg);
 
+	if (pll_info->set_rate_hook)
+		pll_info->set_rate_hook(pll_info, rate, parent_rate);
+
 	/* If the PLL is enabled, verify that it's stable */
 	if (pll_info->enable_bit >= 0 && (ctl & BIT(pll_info->enable_bit)))
 		ret = ingenic_pll_check_stable(cgu, pll_info);
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index a5e44ca7f969..99da9bd86e63 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -46,6 +46,8 @@
  *		-1 if there is no enable bit (ie, the PLL is always on)
  * @stable_bit: the index of the stable bit in the PLL control register, or
  *		-1 if there is no stable bit
+ * @set_rate_hook: hook called immediately after updating the CGU register,
+ *		   before releasing the spinlock
  */
 struct ingenic_cgu_pll_info {
 	unsigned reg;
@@ -61,6 +63,8 @@ struct ingenic_cgu_pll_info {
 	void (*calc_m_n_od)(const struct ingenic_cgu_pll_info *pll_info,
 			    unsigned long rate, unsigned long parent_rate,
 			    unsigned int *m, unsigned int *n, unsigned int *od);
+	void (*set_rate_hook)(const struct ingenic_cgu_pll_info *pll_info,
+			      unsigned long rate, unsigned long parent_rate);
 };
 
 /**
-- 
2.38.1

[PATCH v1 4/5] dt-bindings: ingenic,x1000-cgu: Add audio clocks

[Aidan MacDonald]

Add bindings for audio-related clocks on the Ingenic X1000 SoC.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 include/dt-bindings/clock/ingenic,x1000-cgu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/ingenic,x1000-cgu.h b/include/dt-bindings/clock/ingenic,x1000-cgu.h
index f187e0719fd3..78daf44b3514 100644
--- a/include/dt-bindings/clock/ingenic,x1000-cgu.h
+++ b/include/dt-bindings/clock/ingenic,x1000-cgu.h
@@ -50,5 +50,9 @@
 #define X1000_CLK_PDMA			35
 #define X1000_CLK_EXCLK_DIV512	36
 #define X1000_CLK_RTC			37
+#define X1000_CLK_AIC			38
+#define X1000_CLK_I2SPLLMUX		39
+#define X1000_CLK_I2SPLL		40
+#define X1000_CLK_I2S			41
 
 #endif /* __DT_BINDINGS_CLOCK_X1000_CGU_H__ */
-- 
2.38.1

[PATCH v1 5/5] clk: ingenic: Add X1000 audio clocks

[Aidan MacDonald]

The X1000's CGU supplies the I2S system clock to the AIC module
and ultimately the audio codec, represented by the "i2s" clock.
It is a simple mux which can either pass through EXCLK or a PLL
multiplied by a fractional divider (the "i2s_pll" clock).

The AIC contains a separate 1/N divider controlled by the I2S
driver, which generates the bit clock from the system clock.
The frame clock is always fixed to 1/64th of the bit clock.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/clk/ingenic/x1000-cgu.c | 69 +++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index b2ce3fb83f54..341276e5e1ef 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/rational.h>
 
 #include <dt-bindings/clock/ingenic,x1000-cgu.h>
 
@@ -168,6 +169,37 @@ static const struct clk_ops x1000_otg_phy_ops = {
 	.is_enabled	= x1000_usb_phy_is_enabled,
 };
 
+static void
+x1000_i2spll_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
+			 unsigned long rate, unsigned long parent_rate,
+			 unsigned int *pm, unsigned int *pn, unsigned int *pod)
+{
+	const unsigned long m_max = GENMASK(pll_info->m_bits - 1, 0);
+	const unsigned long n_max = GENMASK(pll_info->n_bits - 1, 0);
+	unsigned long m, n;
+
+	rational_best_approximation(rate, parent_rate, m_max, n_max, &m, &n);
+
+	/* n should not be less than 2*m */
+	if (n < 2 * m)
+		n = 2 * m;
+
+	*pm = m;
+	*pn = n;
+	*pod = 1;
+}
+
+static void
+x1000_i2spll_set_rate_hook(const struct ingenic_cgu_pll_info *pll_info,
+			   unsigned long rate, unsigned long parent_rate)
+{
+	/*
+	 * For some reason, the I2S divider doesn't work properly after
+	 * updating I2SCDR unless I2SCDR1 is read & written back.
+	 */
+	writel(readl(cgu->base + CGU_REG_I2SCDR1), cgu->base + CGU_REG_I2SCDR1);
+}
+
 static const s8 pll_od_encoding[8] = {
 	0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
 };
@@ -319,6 +351,37 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		.gate = { CGU_REG_CLKGR, 25 },
 	},
 
+	[X1000_CLK_I2SPLLMUX] = {
+		"i2s_pll_mux", CGU_CLK_MUX,
+		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },
+		.mux = { CGU_REG_I2SCDR, 31, 1 },
+	},
+
+	[X1000_CLK_I2SPLL] = {
+		"i2s_pll", CGU_CLK_PLL,
+		.parents = { X1000_CLK_I2SPLLMUX, -1, -1, -1 },
+		.pll = {
+			.reg = CGU_REG_I2SCDR,
+			.rate_multiplier = 1,
+			.m_shift = 13,
+			.m_bits = 9,
+			.n_shift = 0,
+			.n_bits = 13,
+			.calc_m_n_od = x1000_i2spll_calc_m_n_od,
+			.set_rate_hook = x1000_i2spll_set_rate_hook,
+		},
+	},
+
+	[X1000_CLK_I2S] = {
+		"i2s", CGU_CLK_MUX,
+		.parents = { X1000_CLK_EXCLK, -1, -1, X1000_CLK_I2SPLL },
+		/*
+		 * NOTE: the mux is at bit 30; bit 29 enables the M/N divider.
+		 * Therefore, the divider is disabled when EXCLK is selected.
+		 */
+		.mux = { CGU_REG_I2SCDR, 29, 2 },
+	},
+
 	[X1000_CLK_LCD] = {
 		"lcd", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
 		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
@@ -426,6 +489,12 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		.gate = { CGU_REG_CLKGR, 9 },
 	},
 
+	[X1000_CLK_AIC] = {
+		"aic", CGU_CLK_GATE,
+		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR, 11 },
+	},
+
 	[X1000_CLK_UART0] = {
 		"uart0", CGU_CLK_GATE,
 		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
-- 
2.38.1

Re: [PATCH v1 1/5] clk: ingenic: Make PLL clock "od" field optional

[Paul Cercueil]

Hi Aidan,

Le dim. 23 oct. 2022 à 15:56:49 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Add support for defining PLL clocks with od_bits = 0, meaning that
> OD is fixed to 1 and there is no OD field in the register.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/clk/ingenic/cgu.c | 28 +++++++++++++++++++---------
>  drivers/clk/ingenic/cgu.h |  3 ++-
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 861c50d6cb24..7dc2e2567d53 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  	m += pll_info->m_offset;
>  	n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
>  	n += pll_info->n_offset;
> -	od_enc = ctl >> pll_info->od_shift;
> -	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> +
> +	if (pll_info->od_bits > 0) {
> +		od_enc = ctl >> pll_info->od_shift;
> +		od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> +	}
> 
>  	if (pll_info->bypass_bit >= 0) {
>  		ctl = readl(cgu->base + pll_info->bypass_reg);
> @@ -108,12 +111,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  			return parent_rate;
>  	}
> 
> -	for (od = 0; od < pll_info->od_max; od++) {
> -		if (pll_info->od_encoding[od] == od_enc)
> -			break;
> +	if (pll_info->od_bits > 0) {
> +		for (od = 0; od < pll_info->od_max; od++) {
> +			if (pll_info->od_encoding[od] == od_enc)
> +				break;
> +		}
> +		BUG_ON(od == pll_info->od_max);
> +		od++;
> +	} else {
> +		/* OD is fixed to 1 if no OD field is present. */
> +		od = 1;
>  	}
> -	BUG_ON(od == pll_info->od_max);
> -	od++;

I think if pll_info->od_max is 0 you get the same result without 
modifying this code. You just need to modify the BUG_ON() to only 
trigger if pll_info->od_max > 0.

Cheers,
-Paul

> 
>  	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
>  		n * od);
> @@ -215,8 +223,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
> long req_rate,
>  	ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
>  	ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
> 
> -	ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
> -	ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
> +	if (pll_info->od_bits > 0) {
> +		ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
> +		ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
> +	}
> 
>  	writel(ctl, cgu->base + pll_info->reg);
> 
> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
> index 147b7df0d657..567142b584bb 100644
> --- a/drivers/clk/ingenic/cgu.h
> +++ b/drivers/clk/ingenic/cgu.h
> @@ -33,7 +33,8 @@
>   * @od_shift: the number of bits to shift the post-VCO divider value 
> by (ie.
>   *            the index of the lowest bit of the post-VCO divider 
> value in
>   *            the PLL's control register)
> - * @od_bits: the size of the post-VCO divider field in bits
> + * @od_bits: the size of the post-VCO divider field in bits, or 0 if 
> no
> + *	     OD field exists (then the OD is fixed to 1)
>   * @od_max: the maximum post-VCO divider value
>   * @od_encoding: a pointer to an array mapping post-VCO divider 
> values to
>   *               their encoded values in the PLL control register, 
> or -1 for
> --
> 2.38.1
> 

Re: [PATCH v1 4/5] dt-bindings: ingenic,x1000-cgu: Add audio clocks

[Krzysztof Kozlowski]

On 23/10/2022 10:56, Aidan MacDonald wrote:
> Add bindings for audio-related clocks on the Ingenic X1000 SoC.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v1 5/5] clk: ingenic: Add X1000 audio clocks

[Paul Cercueil]

Hi Aidan,

Le dim. 23 oct. 2022 à 15:56:53 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> The X1000's CGU supplies the I2S system clock to the AIC module
> and ultimately the audio codec, represented by the "i2s" clock.
> It is a simple mux which can either pass through EXCLK or a PLL
> multiplied by a fractional divider (the "i2s_pll" clock).
> 
> The AIC contains a separate 1/N divider controlled by the I2S
> driver, which generates the bit clock from the system clock.
> The frame clock is always fixed to 1/64th of the bit clock.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/clk/ingenic/x1000-cgu.c | 69 
> +++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> b/drivers/clk/ingenic/x1000-cgu.c
> index b2ce3fb83f54..341276e5e1ef 100644
> --- a/drivers/clk/ingenic/x1000-cgu.c
> +++ b/drivers/clk/ingenic/x1000-cgu.c
> @@ -8,6 +8,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> +#include <linux/rational.h>
> 
>  #include <dt-bindings/clock/ingenic,x1000-cgu.h>
> 
> @@ -168,6 +169,37 @@ static const struct clk_ops x1000_otg_phy_ops = {
>  	.is_enabled	= x1000_usb_phy_is_enabled,
>  };
> 
> +static void
> +x1000_i2spll_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
> +			 unsigned long rate, unsigned long parent_rate,
> +			 unsigned int *pm, unsigned int *pn, unsigned int *pod)
> +{
> +	const unsigned long m_max = GENMASK(pll_info->m_bits - 1, 0);
> +	const unsigned long n_max = GENMASK(pll_info->n_bits - 1, 0);
> +	unsigned long m, n;
> +
> +	rational_best_approximation(rate, parent_rate, m_max, n_max, &m, 
> &n);
> +
> +	/* n should not be less than 2*m */
> +	if (n < 2 * m)
> +		n = 2 * m;
> +
> +	*pm = m;
> +	*pn = n;
> +	*pod = 1;
> +}
> +
> +static void
> +x1000_i2spll_set_rate_hook(const struct ingenic_cgu_pll_info 
> *pll_info,
> +			   unsigned long rate, unsigned long parent_rate)
> +{
> +	/*
> +	 * For some reason, the I2S divider doesn't work properly after
> +	 * updating I2SCDR unless I2SCDR1 is read & written back.
> +	 */
> +	writel(readl(cgu->base + CGU_REG_I2SCDR1), cgu->base + 
> CGU_REG_I2SCDR1);

Not fond of the nesting here, just use a variable.

Besides... According to the documentation, bits 31 and 30 of this 
register are misconnected: writing to bit 31 will be reflected in bit 
30, and vice-versa. So this would work only if the bits 30 and 31 have 
the same value.

And worse than that, where do you actually set the register's value? 
Because bits 30/31, if cleared, will automatically compute the M/N 
values to the I2SCDR fields, overriding what the driver's .set_rate() 
callback is doing.

Either we want that, and in that case the I2S clock should be a custom 
clock (since it wouldn't need to compute or write M/N), or we don't, 
and in this case bits 30/31 of this register should be set.

> +}
> +
>  static const s8 pll_od_encoding[8] = {
>  	0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>  };
> @@ -319,6 +351,37 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		.gate = { CGU_REG_CLKGR, 25 },
>  	},
> 
> +	[X1000_CLK_I2SPLLMUX] = {
> +		"i2s_pll_mux", CGU_CLK_MUX,
> +		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },

If you have only 1 bit you can only have two parents, so you can remove 
the -1s.

> +		.mux = { CGU_REG_I2SCDR, 31, 1 },
> +	},
> +
> +	[X1000_CLK_I2SPLL] = {
> +		"i2s_pll", CGU_CLK_PLL,
> +		.parents = { X1000_CLK_I2SPLLMUX, -1, -1, -1 },

.parents = { X1000_CLK_I2SPLLMUX, },

> +		.pll = {
> +			.reg = CGU_REG_I2SCDR,
> +			.rate_multiplier = 1,
> +			.m_shift = 13,
> +			.m_bits = 9,
> +			.n_shift = 0,
> +			.n_bits = 13,
> +			.calc_m_n_od = x1000_i2spll_calc_m_n_od,
> +			.set_rate_hook = x1000_i2spll_set_rate_hook,
> +		},
> +	},
> +
> +	[X1000_CLK_I2S] = {
> +		"i2s", CGU_CLK_MUX,
> +		.parents = { X1000_CLK_EXCLK, -1, -1, X1000_CLK_I2SPLL },
> +		/*
> +		 * NOTE: the mux is at bit 30; bit 29 enables the M/N divider.
> +		 * Therefore, the divider is disabled when EXCLK is selected.
> +		 */
> +		.mux = { CGU_REG_I2SCDR, 29, 2 },
> +	},
> +
>  	[X1000_CLK_LCD] = {
>  		"lcd", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
>  		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
> @@ -426,6 +489,12 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		.gate = { CGU_REG_CLKGR, 9 },
>  	},
> 
> +	[X1000_CLK_AIC] = {
> +		"aic", CGU_CLK_GATE,
> +		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },

.parents = { X1000_CLK_EXCLK, },

Cheers,
-Paul

> +		.gate = { CGU_REG_CLKGR, 11 },
> +	},
> +
>  	[X1000_CLK_UART0] = {
>  		"uart0", CGU_CLK_GATE,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> --
> 2.38.1
> 

Re: [PATCH v1 1/5] clk: ingenic: Make PLL clock "od" field optional

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le dim. 23 oct. 2022 à 15:56:49 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> Add support for defining PLL clocks with od_bits = 0, meaning that
>> OD is fixed to 1 and there is no OD field in the register.
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  drivers/clk/ingenic/cgu.c | 28 +++++++++++++++++++---------
>>  drivers/clk/ingenic/cgu.h |  3 ++-
>>  2 files changed, 21 insertions(+), 10 deletions(-)
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index 861c50d6cb24..7dc2e2567d53 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>>  	m += pll_info->m_offset;
>>  	n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
>>  	n += pll_info->n_offset;
>> -	od_enc = ctl >> pll_info->od_shift;
>> -	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>> +
>> +	if (pll_info->od_bits > 0) {
>> +		od_enc = ctl >> pll_info->od_shift;
>> +		od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>> +	}
>>  	if (pll_info->bypass_bit >= 0) {
>>  		ctl = readl(cgu->base + pll_info->bypass_reg);
>> @@ -108,12 +111,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned
>> long parent_rate)
>>  			return parent_rate;
>>  	}
>> -	for (od = 0; od < pll_info->od_max; od++) {
>> -		if (pll_info->od_encoding[od] == od_enc)
>> -			break;
>> +	if (pll_info->od_bits > 0) {
>> +		for (od = 0; od < pll_info->od_max; od++) {
>> +			if (pll_info->od_encoding[od] == od_enc)
>> +				break;
>> +		}
>> +		BUG_ON(od == pll_info->od_max);
>> +		od++;
>> +	} else {
>> +		/* OD is fixed to 1 if no OD field is present. */
>> +		od = 1;
>>  	}
>> -	BUG_ON(od == pll_info->od_max);
>> -	od++;
>
> I think if pll_info->od_max is 0 you get the same result without modifying this
> code. You just need to modify the BUG_ON() to only trigger if pll_info->od_max > 0.
>
> Cheers,
> -Paul
>

Yeah, you're right, that's simpler. Thanks,
Aidan

>>  	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
>>  		n * od);
>> @@ -215,8 +223,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long
>> req_rate,
>>  	ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
>>  	ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
>> -	ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> -	ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
>> +	if (pll_info->od_bits > 0) {
>> +		ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> +		ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
>> +	}
>>  	writel(ctl, cgu->base + pll_info->reg);
>> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
>> index 147b7df0d657..567142b584bb 100644
>> --- a/drivers/clk/ingenic/cgu.h
>> +++ b/drivers/clk/ingenic/cgu.h
>> @@ -33,7 +33,8 @@
>>   * @od_shift: the number of bits to shift the post-VCO divider value by (ie.
>>   *            the index of the lowest bit of the post-VCO divider value in
>>   *            the PLL's control register)
>> - * @od_bits: the size of the post-VCO divider field in bits
>> + * @od_bits: the size of the post-VCO divider field in bits, or 0 if no
>> + *	     OD field exists (then the OD is fixed to 1)
>>   * @od_max: the maximum post-VCO divider value
>>   * @od_encoding: a pointer to an array mapping post-VCO divider values to
>>   *               their encoded values in the PLL control register, or -1 for
>> --
>> 2.38.1
>>

Re: [PATCH v1 5/5] clk: ingenic: Add X1000 audio clocks

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le dim. 23 oct. 2022 à 15:56:53 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> The X1000's CGU supplies the I2S system clock to the AIC module
>> and ultimately the audio codec, represented by the "i2s" clock.
>> It is a simple mux which can either pass through EXCLK or a PLL
>> multiplied by a fractional divider (the "i2s_pll" clock).
>> The AIC contains a separate 1/N divider controlled by the I2S
>> driver, which generates the bit clock from the system clock.
>> The frame clock is always fixed to 1/64th of the bit clock.
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  drivers/clk/ingenic/x1000-cgu.c | 69 +++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>> diff --git a/drivers/clk/ingenic/x1000-cgu.c
>> b/drivers/clk/ingenic/x1000-cgu.c
>> index b2ce3fb83f54..341276e5e1ef 100644
>> --- a/drivers/clk/ingenic/x1000-cgu.c
>> +++ b/drivers/clk/ingenic/x1000-cgu.c
>> @@ -8,6 +8,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/io.h>
>>  #include <linux/of.h>
>> +#include <linux/rational.h>
>>  #include <dt-bindings/clock/ingenic,x1000-cgu.h>
>> @@ -168,6 +169,37 @@ static const struct clk_ops x1000_otg_phy_ops = {
>>  	.is_enabled	= x1000_usb_phy_is_enabled,
>>  };
>> +static void
>> +x1000_i2spll_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
>> +			 unsigned long rate, unsigned long parent_rate,
>> +			 unsigned int *pm, unsigned int *pn, unsigned int *pod)
>> +{
>> +	const unsigned long m_max = GENMASK(pll_info->m_bits - 1, 0);
>> +	const unsigned long n_max = GENMASK(pll_info->n_bits - 1, 0);
>> +	unsigned long m, n;
>> +
>> +	rational_best_approximation(rate, parent_rate, m_max, n_max, &m, &n);
>> +
>> +	/* n should not be less than 2*m */
>> +	if (n < 2 * m)
>> +		n = 2 * m;
>> +
>> +	*pm = m;
>> +	*pn = n;
>> +	*pod = 1;
>> +}
>> +
>> +static void
>> +x1000_i2spll_set_rate_hook(const struct ingenic_cgu_pll_info *pll_info,
>> +			   unsigned long rate, unsigned long parent_rate)
>> +{
>> +	/*
>> +	 * For some reason, the I2S divider doesn't work properly after
>> +	 * updating I2SCDR unless I2SCDR1 is read & written back.
>> +	 */
>> +	writel(readl(cgu->base + CGU_REG_I2SCDR1), cgu->base + CGU_REG_I2SCDR1);
>
> Not fond of the nesting here, just use a variable.
>
> Besides... According to the documentation, bits 31 and 30 of this register are
> misconnected: writing to bit 31 will be reflected in bit 30, and vice-versa. So
> this would work only if the bits 30 and 31 have the same value.

From my tests it seems that reads are swapped w.r.t. the documented
bit positions, but not writes. I'm assuming this is the case because
when I write to the register with bit 30 (I2S_DEN) set, I can change
I2SDIV_D freely, but if bit 30 is 0, then I2SDIV_D is replaced by
I2SDIV_N/2 like the manual suggests.

> And worse than that, where do you actually set the register's value? Because
> bits 30/31, if cleared, will automatically compute the M/N values to the I2SCDR
> fields, overriding what the driver's .set_rate() callback is doing.

I don't initialize the register, but I2S_NEN (bit 31 of I2SCDR1) doesn't
cause I2SCDR to be automatically updated like I see with I2SCDR1 when
I2S_DEN = 0. It seems setting I2S_NEN to 1 has some other effect on the
clock output, but I don't have an oscilloscope so I'm not sure exactly
what it is. I'm limited to testing by ear.

For example, with f_in = 1008 MHz, M = 7, N = 2500, the output should
nominally be 2.8224 MHz (= 64 * 44.1 KHz). Now that works just fine
for playing 44.1 KHz audio if I set I2S_NEN to 0, but setting it to 1,
I need to increase M to 16 to get things to sound right. I've tested
a couple other frequencies but there isn't an obvious pattern to what
increase in M is needed (it always seems to need an increase).

I don't really care to play around with it because everything works
fine if I keep I2S_NEN = I2S_DEN = 0. I've tested that configuration
from 8 KHz to 192 KHz at typical audio frequencies and randomly chosen
non-standard frequencies, and haven't experienced any issues.

> Either we want that, and in that case the I2S clock should be a custom clock
> (since it wouldn't need to compute or write M/N), or we don't, and in this case
> bits 30/31 of this register should be set.

My experience suggests we should set I2S_NEN and I2S_DEN to 0 and
calculate output frequency as F_out = F_in * I2SDIV_M / I2SDIV_N.

Reading I2SCDR1 and writing the same value back is something I took
from the Ingenic BSP kernels. (The manual also suggests doing this.)
It worked fine for me so I didn't bother to invesigate it further.

The important part actually seems to be refreshing I2SDIV_D to a
"reasonable" value, which happens automatically when I2S_DEN = 0.
But the automatic calculation only happens when writing the register.

Writing 0 to I2SCDR1 solves both problems: the register is properly
initialized and I2SDIV_D gets updated automatically. That seems to
be the best solution.

>> +}
>> +
>>  static const s8 pll_od_encoding[8] = {itt
>>  	0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>>  };
>> @@ -319,6 +351,37 @@ static const struct ingenic_cgu_clk_info
>> x1000_cgu_clocks[] = {
>>  		.gate = { CGU_REG_CLKGR, 25 },
>>  	},
>> +	[X1000_CLK_I2SPLLMUX] = {
>> +		"i2s_pll_mux", CGU_CLK_MUX,
>> +		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },
>
> If you have only 1 bit you can only have two parents, so you can remove the
> -1s.
>

Thanks, will do.

Regards,
Aidan

>> +		.mux = { CGU_REG_I2SCDR, 31, 1 },
>> +	},
>> +
>> +	[X1000_CLK_I2SPLL] = {
>> +		"i2s_pll", CGU_CLK_PLL,
>> +		.parents = { X1000_CLK_I2SPLLMUX, -1, -1, -1 },
>
> .parents = { X1000_CLK_I2SPLLMUX, },
>
>> +		.pll = {
>> +			.reg = CGU_REG_I2SCDR,
>> +			.rate_multiplier = 1,
>> +			.m_shift = 13,
>> +			.m_bits = 9,
>> +			.n_shift = 0,
>> +			.n_bits = 13,
>> +			.calc_m_n_od = x1000_i2spll_calc_m_n_od,
>> +			.set_rate_hook = x1000_i2spll_set_rate_hook,
>> +		},
>> +	},
>> +
>> +	[X1000_CLK_I2S] = {
>> +		"i2s", CGU_CLK_MUX,
>> +		.parents = { X1000_CLK_EXCLK, -1, -1, X1000_CLK_I2SPLL },
>> +		/*
>> +		 * NOTE: the mux is at bit 30; bit 29 enables the M/N divider.
>> +		 * Therefore, the divider is disabled when EXCLK is selected.
>> +		 */
>> +		.mux = { CGU_REG_I2SCDR, 29, 2 },
>> +	},
>> +
>>  	[X1000_CLK_LCD] = {
>>  		"lcd", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
>>  		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
>> @@ -426,6 +489,12 @@ static const struct ingenic_cgu_clk_info
>> x1000_cgu_clocks[] = {
>>  		.gate = { CGU_REG_CLKGR, 9 },
>>  	},
>> +	[X1000_CLK_AIC] = {
>> +		"aic", CGU_CLK_GATE,
>> +		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>
> .parents = { X1000_CLK_EXCLK, },
>
> Cheers,
> -Paul
>
>> +		.gate = { CGU_REG_CLKGR, 11 },
>> +	},
>> +
>>  	[X1000_CLK_UART0] = {
>>  		"uart0", CGU_CLK_GATE,
>>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>> --
>> 2.38.1
>>