[PATCH v2 0/6] Add support for X1000 audio clocks

[PATCH v2 0/6] 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.)

Patches 04-05 actually add the X1000 SoC's audio clocks. The last patch
is just a cosmetic cleanup, feel free to take it or leave it.

Aidan MacDonald (6):
  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
  clk: ingenic: Minor cosmetic fixups for X1000

 drivers/clk/ingenic/cgu.c                     |  38 ++++--
 drivers/clk/ingenic/cgu.h                     |  17 ++-
 drivers/clk/ingenic/x1000-cgu.c               | 119 ++++++++++++++----
 include/dt-bindings/clock/ingenic,x1000-cgu.h |   4 +
 4 files changed, 141 insertions(+), 37 deletions(-)

-- 
2.38.1

[PATCH v2 1/6] 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. In this
case od_max must also be 0, which is enforced with BUG_ON().

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
          enforce od_max == 0 when od_bits is zero.

 drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
 drivers/clk/ingenic/cgu.h |  3 ++-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 861c50d6cb24..3481129114b1 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
 	struct ingenic_cgu *cgu = ingenic_clk->cgu;
 	const struct ingenic_cgu_pll_info *pll_info;
-	unsigned m, n, od_enc, od;
+	unsigned m, n, od, od_enc = 0;
 	bool bypass;
 	u32 ctl;
 
@@ -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);
@@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 		if (pll_info->od_encoding[od] == od_enc)
 			break;
 	}
-	BUG_ON(od == pll_info->od_max);
+	/* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
+	if (pll_info->od_max == 0)
+		BUG_ON(pll_info->od_bits != 0);
+	else
+		BUG_ON(od == pll_info->od_max);
 	od++;
 
 	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
@@ -215,8 +222,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 v2 2/6] 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 3481129114b1..aea01b6b2764 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -189,6 +189,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);
@@ -230,7 +233,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);
@@ -248,6 +251,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);
@@ -278,6 +284,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);
 
@@ -295,6 +304,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 v2 3/6] 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 aea01b6b2764..b6a4d4236c16 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -232,6 +232,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 v2 4/6] dt-bindings: ingenic,x1000-cgu: Add audio clocks

[Aidan MacDonald]

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

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
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 v2 5/6] 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>
---
v1 -> v2:
* Fix I2SCDR1 refresh to ensure the register is properly initialized
  and we don't rely on the reset value. Since the I2SDIV_D field can
  be automatically calculated by the hardware we don't need to provide
  it, writing 0 triggers the auto calculation.
* Remove redundant -1 entries from parent clocks.

 drivers/clk/ingenic/x1000-cgu.c | 70 +++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index b2ce3fb83f54..95d5e3a44cee 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,38 @@ 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)
+{
+	/*
+	 * Writing 0 causes I2SCDR1.I2SDIV_D to be automatically recalculated
+	 * based on the current value of I2SCDR.I2SDIV_N, which is needed for
+	 * the divider to function correctly.
+	 */
+	writel(0, cgu->base + CGU_REG_I2SCDR1);
+}
+
 static const s8 pll_od_encoding[8] = {
 	0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
 };
@@ -319,6 +352,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 },
+		.mux = { CGU_REG_I2SCDR, 31, 1 },
+	},
+
+	[X1000_CLK_I2SPLL] = {
+		"i2s_pll", CGU_CLK_PLL,
+		.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 +490,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 },
+		.gate = { CGU_REG_CLKGR, 11 },
+	},
+
 	[X1000_CLK_UART0] = {
 		"uart0", CGU_CLK_GATE,
 		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
-- 
2.38.1

[PATCH v2 6/6] clk: ingenic: Minor cosmetic fixups for X1000

[Aidan MacDonald]

Remove redundant -1 entries from the parents array and fix
a couple indentation / whitespace issues.

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

diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index 95d5e3a44cee..feb03eed4fe8 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -216,7 +216,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_APLL] = {
 		"apll", CGU_CLK_PLL,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.pll = {
 			.reg = CGU_REG_APLL,
 			.rate_multiplier = 1,
@@ -239,7 +239,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_MPLL] = {
 		"mpll", CGU_CLK_PLL,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.pll = {
 			.reg = CGU_REG_MPLL,
 			.rate_multiplier = 1,
@@ -289,7 +289,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		 * system; mark it critical.
 		 */
 		.flags = CLK_IS_CRITICAL,
-		.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_CPUMUX },
 		.div = { CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1 },
 		.gate = { CGU_REG_CLKGR, 30 },
 	},
@@ -301,7 +301,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		 * disabling it or any parent clocks will hang the system.
 		 */
 		.flags = CLK_IS_CRITICAL,
-		.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_CPUMUX },
 		.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
 	},
 
@@ -320,13 +320,13 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_AHB2] = {
 		"ahb2", CGU_CLK_DIV,
-		.parents = { X1000_CLK_AHB2PMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_AHB2PMUX },
 		.div = { CGU_REG_CPCCR, 12, 1, 4, 20, -1, -1 },
 	},
 
 	[X1000_CLK_PCLK] = {
 		"pclk", CGU_CLK_DIV | CGU_CLK_GATE,
-		.parents = { X1000_CLK_AHB2PMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_AHB2PMUX },
 		.div = { CGU_REG_CPCCR, 16, 1, 4, 20, -1, -1 },
 		.gate = { CGU_REG_CLKGR, 28 },
 	},
@@ -393,13 +393,13 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_MSCMUX] = {
 		"msc_mux", CGU_CLK_MUX,
-		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL},
+		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
 		.mux = { CGU_REG_MSC0CDR, 31, 1 },
 	},
 
 	[X1000_CLK_MSC0] = {
 		"msc0", CGU_CLK_DIV | CGU_CLK_GATE,
-		.parents = { X1000_CLK_MSCMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_MSCMUX },
 		.div = { CGU_REG_MSC0CDR, 0, 2, 8, 29, 28, 27 },
 		.gate = { CGU_REG_CLKGR, 4 },
 	},
@@ -413,8 +413,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_OTG] = {
 		"otg", CGU_CLK_DIV | CGU_CLK_GATE | CGU_CLK_MUX,
-		.parents = { X1000_CLK_EXCLK, -1,
-					 X1000_CLK_APLL, X1000_CLK_MPLL },
+		.parents = { X1000_CLK_EXCLK, -1, X1000_CLK_APLL, X1000_CLK_MPLL },
 		.mux = { CGU_REG_USBCDR, 30, 2 },
 		.div = { CGU_REG_USBCDR, 0, 1, 8, 29, 28, 27 },
 		.gate = { CGU_REG_CLKGR, 3 },
@@ -422,7 +421,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_SSIPLL] = {
 		"ssi_pll", CGU_CLK_MUX | CGU_CLK_DIV,
-		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },
+		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
 		.mux = { CGU_REG_SSICDR, 31, 1 },
 		.div = { CGU_REG_SSICDR, 0, 1, 8, 29, 28, 27 },
 	},
@@ -435,7 +434,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_SSIMUX] = {
 		"ssi_mux", CGU_CLK_MUX,
-		.parents = { X1000_CLK_EXCLK, X1000_CLK_SSIPLL_DIV2, -1, -1 },
+		.parents = { X1000_CLK_EXCLK, X1000_CLK_SSIPLL_DIV2 },
 		.mux = { CGU_REG_SSICDR, 30, 1 },
 	},
 
@@ -456,37 +455,37 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_EMC] = {
 		"emc", CGU_CLK_GATE,
-		.parents = { X1000_CLK_AHB2, -1, -1, -1 },
+		.parents = { X1000_CLK_AHB2 },
 		.gate = { CGU_REG_CLKGR, 0 },
 	},
 
 	[X1000_CLK_EFUSE] = {
 		"efuse", CGU_CLK_GATE,
-		.parents = { X1000_CLK_AHB2, -1, -1, -1 },
+		.parents = { X1000_CLK_AHB2 },
 		.gate = { CGU_REG_CLKGR, 1 },
 	},
 
 	[X1000_CLK_SFC] = {
 		"sfc", CGU_CLK_GATE,
-		.parents = { X1000_CLK_SSIPLL, -1, -1, -1 },
+		.parents = { X1000_CLK_SSIPLL },
 		.gate = { CGU_REG_CLKGR, 2 },
 	},
 
 	[X1000_CLK_I2C0] = {
 		"i2c0", CGU_CLK_GATE,
-		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_PCLK },
 		.gate = { CGU_REG_CLKGR, 7 },
 	},
 
 	[X1000_CLK_I2C1] = {
 		"i2c1", CGU_CLK_GATE,
-		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_PCLK },
 		.gate = { CGU_REG_CLKGR, 8 },
 	},
 
 	[X1000_CLK_I2C2] = {
 		"i2c2", CGU_CLK_GATE,
-		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_PCLK },
 		.gate = { CGU_REG_CLKGR, 9 },
 	},
 
@@ -498,43 +497,43 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 
 	[X1000_CLK_UART0] = {
 		"uart0", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.gate = { CGU_REG_CLKGR, 14 },
 	},
 
 	[X1000_CLK_UART1] = {
 		"uart1", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK},
 		.gate = { CGU_REG_CLKGR, 15 },
 	},
 
 	[X1000_CLK_UART2] = {
 		"uart2", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.gate = { CGU_REG_CLKGR, 16 },
 	},
 
 	[X1000_CLK_TCU] = {
 		"tcu", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.gate = { CGU_REG_CLKGR, 18 },
 	},
 
 	[X1000_CLK_SSI] = {
 		"ssi", CGU_CLK_GATE,
-		.parents = { X1000_CLK_SSIMUX, -1, -1, -1 },
+		.parents = { X1000_CLK_SSIMUX },
 		.gate = { CGU_REG_CLKGR, 19 },
 	},
 
 	[X1000_CLK_OST] = {
 		"ost", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.gate = { CGU_REG_CLKGR, 20 },
 	},
 
 	[X1000_CLK_PDMA] = {
 		"pdma", CGU_CLK_GATE,
-		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
+		.parents = { X1000_CLK_EXCLK },
 		.gate = { CGU_REG_CLKGR, 21 },
 	},
 };
-- 
2.38.1

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

[Paul Cercueil]

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:40 +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. In this
> case od_max must also be 0, which is enforced with BUG_ON().
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
>           enforce od_max == 0 when od_bits is zero.
> 
>  drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
>  drivers/clk/ingenic/cgu.h |  3 ++-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 861c50d6cb24..3481129114b1 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned 
> long parent_rate)
>  	const struct ingenic_cgu_clk_info *clk_info = 
> to_clk_info(ingenic_clk);
>  	struct ingenic_cgu *cgu = ingenic_clk->cgu;
>  	const struct ingenic_cgu_pll_info *pll_info;
> -	unsigned m, n, od_enc, od;
> +	unsigned m, n, od, od_enc = 0;
>  	bool bypass;
>  	u32 ctl;
> 
> @@ -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);
> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  		if (pll_info->od_encoding[od] == od_enc)
>  			break;
>  	}

I'd add a space there.

With that:
Reviewed-by: Paul Cercueil <paul@crapouillou.net>

> -	BUG_ON(od == pll_info->od_max);
> +	/* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
> +	if (pll_info->od_max == 0)
> +		BUG_ON(pll_info->od_bits != 0);

I don't think this first BUG_ON() is needed, if we do a good job 
reviewing patches. But I don't care enough to ask you to remove it.

Cheers,
-Paul

> +	else
> +		BUG_ON(od == pll_info->od_max);
>  	od++;
> 
>  	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
> @@ -215,8 +222,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 v2 2/6] clk: ingenic: Make PLL clock enable_bit and stable_bit optional

[Paul Cercueil]

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:41 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  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 3481129114b1..aea01b6b2764 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -189,6 +189,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);
> @@ -230,7 +233,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);
> @@ -248,6 +251,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);
> @@ -278,6 +284,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);
> 
> @@ -295,6 +304,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
> 

Re: [PATCH v2 3/6] clk: ingenic: Add .set_rate_hook() for PLL clocks

[Paul Cercueil]

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:42 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  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 aea01b6b2764..b6a4d4236c16 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -232,6 +232,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
> 

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

[Paul Cercueil]

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:44 +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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
> v1 -> v2:
> * Fix I2SCDR1 refresh to ensure the register is properly initialized
>   and we don't rely on the reset value. Since the I2SDIV_D field can
>   be automatically calculated by the hardware we don't need to provide
>   it, writing 0 triggers the auto calculation.
> * Remove redundant -1 entries from parent clocks.
> 
>  drivers/clk/ingenic/x1000-cgu.c | 70 
> +++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> b/drivers/clk/ingenic/x1000-cgu.c
> index b2ce3fb83f54..95d5e3a44cee 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,38 @@ 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)
> +{
> +	/*
> +	 * Writing 0 causes I2SCDR1.I2SDIV_D to be automatically 
> recalculated
> +	 * based on the current value of I2SCDR.I2SDIV_N, which is needed 
> for
> +	 * the divider to function correctly.
> +	 */
> +	writel(0, cgu->base + CGU_REG_I2SCDR1);
> +}
> +
>  static const s8 pll_od_encoding[8] = {
>  	0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>  };
> @@ -319,6 +352,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 },
> +		.mux = { CGU_REG_I2SCDR, 31, 1 },
> +	},
> +
> +	[X1000_CLK_I2SPLL] = {
> +		"i2s_pll", CGU_CLK_PLL,
> +		.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 +490,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 },
> +		.gate = { CGU_REG_CLKGR, 11 },
> +	},
> +
>  	[X1000_CLK_UART0] = {
>  		"uart0", CGU_CLK_GATE,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> --
> 2.38.1
> 

Re: [PATCH v2 6/6] clk: ingenic: Minor cosmetic fixups for X1000

[Paul Cercueil]

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:45 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Remove redundant -1 entries from the parents array and fix
> a couple indentation / whitespace issues.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  drivers/clk/ingenic/x1000-cgu.c | 49 
> ++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> b/drivers/clk/ingenic/x1000-cgu.c
> index 95d5e3a44cee..feb03eed4fe8 100644
> --- a/drivers/clk/ingenic/x1000-cgu.c
> +++ b/drivers/clk/ingenic/x1000-cgu.c
> @@ -216,7 +216,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_APLL] = {
>  		"apll", CGU_CLK_PLL,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.pll = {
>  			.reg = CGU_REG_APLL,
>  			.rate_multiplier = 1,
> @@ -239,7 +239,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_MPLL] = {
>  		"mpll", CGU_CLK_PLL,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.pll = {
>  			.reg = CGU_REG_MPLL,
>  			.rate_multiplier = 1,
> @@ -289,7 +289,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		 * system; mark it critical.
>  		 */
>  		.flags = CLK_IS_CRITICAL,
> -		.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_CPUMUX },
>  		.div = { CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1 },
>  		.gate = { CGU_REG_CLKGR, 30 },
>  	},
> @@ -301,7 +301,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		 * disabling it or any parent clocks will hang the system.
>  		 */
>  		.flags = CLK_IS_CRITICAL,
> -		.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_CPUMUX },
>  		.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
>  	},
> 
> @@ -320,13 +320,13 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_AHB2] = {
>  		"ahb2", CGU_CLK_DIV,
> -		.parents = { X1000_CLK_AHB2PMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_AHB2PMUX },
>  		.div = { CGU_REG_CPCCR, 12, 1, 4, 20, -1, -1 },
>  	},
> 
>  	[X1000_CLK_PCLK] = {
>  		"pclk", CGU_CLK_DIV | CGU_CLK_GATE,
> -		.parents = { X1000_CLK_AHB2PMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_AHB2PMUX },
>  		.div = { CGU_REG_CPCCR, 16, 1, 4, 20, -1, -1 },
>  		.gate = { CGU_REG_CLKGR, 28 },
>  	},
> @@ -393,13 +393,13 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_MSCMUX] = {
>  		"msc_mux", CGU_CLK_MUX,
> -		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL},
> +		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
>  		.mux = { CGU_REG_MSC0CDR, 31, 1 },
>  	},
> 
>  	[X1000_CLK_MSC0] = {
>  		"msc0", CGU_CLK_DIV | CGU_CLK_GATE,
> -		.parents = { X1000_CLK_MSCMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_MSCMUX },
>  		.div = { CGU_REG_MSC0CDR, 0, 2, 8, 29, 28, 27 },
>  		.gate = { CGU_REG_CLKGR, 4 },
>  	},
> @@ -413,8 +413,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_OTG] = {
>  		"otg", CGU_CLK_DIV | CGU_CLK_GATE | CGU_CLK_MUX,
> -		.parents = { X1000_CLK_EXCLK, -1,
> -					 X1000_CLK_APLL, X1000_CLK_MPLL },
> +		.parents = { X1000_CLK_EXCLK, -1, X1000_CLK_APLL, X1000_CLK_MPLL },
>  		.mux = { CGU_REG_USBCDR, 30, 2 },
>  		.div = { CGU_REG_USBCDR, 0, 1, 8, 29, 28, 27 },
>  		.gate = { CGU_REG_CLKGR, 3 },
> @@ -422,7 +421,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_SSIPLL] = {
>  		"ssi_pll", CGU_CLK_MUX | CGU_CLK_DIV,
> -		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },
> +		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL },
>  		.mux = { CGU_REG_SSICDR, 31, 1 },
>  		.div = { CGU_REG_SSICDR, 0, 1, 8, 29, 28, 27 },
>  	},
> @@ -435,7 +434,7 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_SSIMUX] = {
>  		"ssi_mux", CGU_CLK_MUX,
> -		.parents = { X1000_CLK_EXCLK, X1000_CLK_SSIPLL_DIV2, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK, X1000_CLK_SSIPLL_DIV2 },
>  		.mux = { CGU_REG_SSICDR, 30, 1 },
>  	},
> 
> @@ -456,37 +455,37 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_EMC] = {
>  		"emc", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_AHB2, -1, -1, -1 },
> +		.parents = { X1000_CLK_AHB2 },
>  		.gate = { CGU_REG_CLKGR, 0 },
>  	},
> 
>  	[X1000_CLK_EFUSE] = {
>  		"efuse", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_AHB2, -1, -1, -1 },
> +		.parents = { X1000_CLK_AHB2 },
>  		.gate = { CGU_REG_CLKGR, 1 },
>  	},
> 
>  	[X1000_CLK_SFC] = {
>  		"sfc", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_SSIPLL, -1, -1, -1 },
> +		.parents = { X1000_CLK_SSIPLL },
>  		.gate = { CGU_REG_CLKGR, 2 },
>  	},
> 
>  	[X1000_CLK_I2C0] = {
>  		"i2c0", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_PCLK },
>  		.gate = { CGU_REG_CLKGR, 7 },
>  	},
> 
>  	[X1000_CLK_I2C1] = {
>  		"i2c1", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_PCLK },
>  		.gate = { CGU_REG_CLKGR, 8 },
>  	},
> 
>  	[X1000_CLK_I2C2] = {
>  		"i2c2", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_PCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_PCLK },
>  		.gate = { CGU_REG_CLKGR, 9 },
>  	},
> 
> @@ -498,43 +497,43 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
> 
>  	[X1000_CLK_UART0] = {
>  		"uart0", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.gate = { CGU_REG_CLKGR, 14 },
>  	},
> 
>  	[X1000_CLK_UART1] = {
>  		"uart1", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK},
>  		.gate = { CGU_REG_CLKGR, 15 },
>  	},
> 
>  	[X1000_CLK_UART2] = {
>  		"uart2", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.gate = { CGU_REG_CLKGR, 16 },
>  	},
> 
>  	[X1000_CLK_TCU] = {
>  		"tcu", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.gate = { CGU_REG_CLKGR, 18 },
>  	},
> 
>  	[X1000_CLK_SSI] = {
>  		"ssi", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_SSIMUX, -1, -1, -1 },
> +		.parents = { X1000_CLK_SSIMUX },
>  		.gate = { CGU_REG_CLKGR, 19 },
>  	},
> 
>  	[X1000_CLK_OST] = {
>  		"ost", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.gate = { CGU_REG_CLKGR, 20 },
>  	},
> 
>  	[X1000_CLK_PDMA] = {
>  		"pdma", CGU_CLK_GATE,
> -		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
> +		.parents = { X1000_CLK_EXCLK },
>  		.gate = { CGU_REG_CLKGR, 21 },
>  	},
>  };
> --
> 2.38.1
> 

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

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:40)
> 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. In this
> case od_max must also be 0, which is enforced with BUG_ON().
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Applied to clk-next

Re: [PATCH v2 2/6] clk: ingenic: Make PLL clock enable_bit and stable_bit optional

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:41)
> 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>
> ---

Applied to clk-next

Re: [PATCH v2 3/6] clk: ingenic: Add .set_rate_hook() for PLL clocks

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:42)
> 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>
> ---

Applied to clk-next

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

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:43)
> Add bindings for audio-related clocks on the Ingenic X1000 SoC.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Applied to clk-next

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

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:44)
> 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>
> ---

Applied to clk-next

Re: [PATCH v2 6/6] clk: ingenic: Minor cosmetic fixups for X1000

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-26 12:43:45)
> Remove redundant -1 entries from the parents array and fix
> a couple indentation / whitespace issues.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Applied to clk-next

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

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le mer. 26 oct. 2022 à 20:43:40 +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. In this
>> case od_max must also be 0, which is enforced with BUG_ON().
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
>>           enforce od_max == 0 when od_bits is zero.
>>  drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
>>  drivers/clk/ingenic/cgu.h |  3 ++-
>>  2 files changed, 17 insertions(+), 7 deletions(-)
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index 861c50d6cb24..3481129114b1 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>>  	const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
>>  	struct ingenic_cgu *cgu = ingenic_clk->cgu;
>>  	const struct ingenic_cgu_pll_info *pll_info;
>> -	unsigned m, n, od_enc, od;
>> +	unsigned m, n, od, od_enc = 0;
>>  	bool bypass;
>>  	u32 ctl;
>> @@ -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);
>> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>>  		if (pll_info->od_encoding[od] == od_enc)
>>  			break;
>>  	}
>
> I'd add a space there.
>
> With that:
> Reviewed-by: Paul Cercueil <paul@crapouillou.net>
>

Already done; the space is there in my outbox and on lore.kernel.org.
I think you might've accidentally removed it. Stephen's already
applied the series anyway, so...

>> -	BUG_ON(od == pll_info->od_max);
>> +	/* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
>> +	if (pll_info->od_max == 0)
>> +		BUG_ON(pll_info->od_bits != 0);
>
> I don't think this first BUG_ON() is needed, if we do a good job reviewing
> patches. But I don't care enough to ask you to remove it.
>
> Cheers,
> -Paul
>
>> +	else
>> +		BUG_ON(od == pll_info->od_max);
>>  	od++;
>>  	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
>> @@ -215,8 +222,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 v2 1/6] clk: ingenic: Make PLL clock "od" field optional

[Stephen Boyd]

Quoting Aidan MacDonald (2022-10-27 14:40:02)
> 
> Paul Cercueil <paul@crapouillou.net> writes:
> 
> > Hi Aidan,
> >
> > Le mer. 26 oct. 2022 à 20:43:40 +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. In this
> >> case od_max must also be 0, which is enforced with BUG_ON().
> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> >> ---
> >> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
> >>           enforce od_max == 0 when od_bits is zero.
> >>  drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
> >>  drivers/clk/ingenic/cgu.h |  3 ++-
> >>  2 files changed, 17 insertions(+), 7 deletions(-)
> >> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> >> index 861c50d6cb24..3481129114b1 100644
> >> --- a/drivers/clk/ingenic/cgu.c
> >> +++ b/drivers/clk/ingenic/cgu.c
> >> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
> >> parent_rate)
> >>      const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
> >>      struct ingenic_cgu *cgu = ingenic_clk->cgu;
> >>      const struct ingenic_cgu_pll_info *pll_info;
> >> -    unsigned m, n, od_enc, od;
> >> +    unsigned m, n, od, od_enc = 0;
> >>      bool bypass;
> >>      u32 ctl;
> >> @@ -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);
> >> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
> >> parent_rate)
> >>              if (pll_info->od_encoding[od] == od_enc)
> >>                      break;
> >>      }
> >
> > I'd add a space there.
> >
> > With that:
> > Reviewed-by: Paul Cercueil <paul@crapouillou.net>
> >
> 
> Already done; the space is there in my outbox and on lore.kernel.org.
> I think you might've accidentally removed it. Stephen's already
> applied the series anyway, so...
> 

I fixed the whitespace.