linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* clk: Ingenic: Add support for the X1830 and add USB clk for X1000.
@ 2019-11-27  3:32 Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

1.Adjust existing code to make it compatible with Ingenic X1830 SoC.
2.Add support for the clocks provided by the CGU in the Ingenic X1830
  SoC, making use of the cgu code to do the heavy lifting.
3.Add USB related clock for the Ingenic X1000 SoC, and use the
  "CLK_OF_DECLARE_DRIVER" instead "CLK_OF_DECLARE" like the
  other CGU drivers.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
  2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
@ 2019-11-27  3:32 ` Zhou Yanjie
  2019-11-27 17:37   ` Paul Cercueil
  2019-11-27  3:32 ` [PATCH 2/5] dt-bindings: clock: Add X1830 bindings Zhou Yanjie
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

1.Adjust the PLL related code in "cgu.c" and "cgu.h" to make it
  compatible with the X1830 Soc from Ingenic.
2.Adjust the code in "jz4740-cgu.c" to be compatible with the
  new cgu code.
3.Adjust the code in "jz4725b-cgu.c" to be compatible with the
  new cgu code.
4.Adjust the code in "jz4770-cgu.c" to be compatible with the
  new cgu code.
5.Adjust the code in "jz4780-cgu.c" to be compatible with the
  new cgu code.
6.Adjust the code in "x1000-cgu.c" to be compatible with the
  new cgu code.

Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
---
 drivers/clk/ingenic/cgu.c         | 55 +++++++++++++++++++++++++++++----------
 drivers/clk/ingenic/cgu.h         | 12 ++++++++-
 drivers/clk/ingenic/jz4725b-cgu.c |  3 ++-
 drivers/clk/ingenic/jz4740-cgu.c  |  3 ++-
 drivers/clk/ingenic/jz4770-cgu.c  |  6 +++--
 drivers/clk/ingenic/jz4780-cgu.c  |  3 ++-
 drivers/clk/ingenic/x1000-cgu.c   |  6 +++--
 7 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 6e96303..c3c69a8 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -84,7 +84,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	pll_info = &clk_info->pll;
 
 	spin_lock_irqsave(&cgu->lock, flags);
-	ctl = readl(cgu->base + pll_info->reg);
+	ctl = readl(cgu->base + pll_info->reg[1]);
 	spin_unlock_irqrestore(&cgu->lock, flags);
 
 	m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
@@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	n += pll_info->n_offset;
 	od_enc = ctl >> pll_info->od_shift;
 	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
-	bypass = !pll_info->no_bypass_bit &&
-		 !!(ctl & BIT(pll_info->bypass_bit));
+
+	if (pll_info->version >= CGU_X1830) {
+		spin_lock_irqsave(&cgu->lock, flags);
+		ctl = readl(cgu->base + pll_info->reg[0]);
+		spin_unlock_irqrestore(&cgu->lock, flags);
+
+		bypass = !pll_info->no_bypass_bit &&
+			 !!(ctl & BIT(pll_info->bypass_bit));
+	} else
+		bypass = !pll_info->no_bypass_bit &&
+			 !!(ctl & BIT(pll_info->bypass_bit));
 
 	if (bypass)
 		return parent_rate;
@@ -106,7 +115,10 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	BUG_ON(od == pll_info->od_max);
 	od++;
 
-	return div_u64((u64)parent_rate * m, n * od);
+	if (pll_info->version >= CGU_X1830)
+		return div_u64((u64)parent_rate * m * 2, n * od);
+	else
+		return div_u64((u64)parent_rate * m, n * od);
 }
 
 static unsigned long
@@ -139,7 +151,10 @@ ingenic_pll_calc(const struct ingenic_cgu_clk_info *clk_info,
 	if (pod)
 		*pod = od;
 
-	return div_u64((u64)parent_rate * m, n * od);
+	if (pll_info->version >= CGU_X1830)
+		return div_u64((u64)parent_rate * m * 2, n * od);
+	else
+		return div_u64((u64)parent_rate * m, n * od);
 }
 
 static inline const struct ingenic_cgu_clk_info *to_clk_info(
@@ -183,7 +198,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
 			clk_info->name, req_rate, rate);
 
 	spin_lock_irqsave(&cgu->lock, flags);
-	ctl = readl(cgu->base + pll_info->reg);
+	ctl = readl(cgu->base + pll_info->reg[1]);
 
 	ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << pll_info->m_shift);
 	ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
@@ -194,7 +209,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
 	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);
+	writel(ctl, cgu->base + pll_info->reg[1]);
 	spin_unlock_irqrestore(&cgu->lock, flags);
 
 	return 0;
@@ -212,16 +227,28 @@ static int ingenic_pll_enable(struct clk_hw *hw)
 	u32 ctl;
 
 	spin_lock_irqsave(&cgu->lock, flags);
-	ctl = readl(cgu->base + pll_info->reg);
 
-	ctl &= ~BIT(pll_info->bypass_bit);
+	if (pll_info->version >= CGU_X1830) {
+		ctl = readl(cgu->base + pll_info->reg[0]);
+
+		ctl &= ~BIT(pll_info->bypass_bit);
+
+		writel(ctl, cgu->base + pll_info->reg[0]);
+
+		ctl = readl(cgu->base + pll_info->reg[1]);
+	} else {
+		ctl = readl(cgu->base + pll_info->reg[1]);
+
+		ctl &= ~BIT(pll_info->bypass_bit);
+	}
+
 	ctl |= BIT(pll_info->enable_bit);
 
-	writel(ctl, cgu->base + pll_info->reg);
+	writel(ctl, cgu->base + pll_info->reg[1]);
 
 	/* wait for the PLL to stabilise */
 	for (i = 0; i < timeout; i++) {
-		ctl = readl(cgu->base + pll_info->reg);
+		ctl = readl(cgu->base + pll_info->reg[1]);
 		if (ctl & BIT(pll_info->stable_bit))
 			break;
 		mdelay(1);
@@ -245,11 +272,11 @@ static void ingenic_pll_disable(struct clk_hw *hw)
 	u32 ctl;
 
 	spin_lock_irqsave(&cgu->lock, flags);
-	ctl = readl(cgu->base + pll_info->reg);
+	ctl = readl(cgu->base + pll_info->reg[1]);
 
 	ctl &= ~BIT(pll_info->enable_bit);
 
-	writel(ctl, cgu->base + pll_info->reg);
+	writel(ctl, cgu->base + pll_info->reg[1]);
 	spin_unlock_irqrestore(&cgu->lock, flags);
 }
 
@@ -263,7 +290,7 @@ static int ingenic_pll_is_enabled(struct clk_hw *hw)
 	u32 ctl;
 
 	spin_lock_irqsave(&cgu->lock, flags);
-	ctl = readl(cgu->base + pll_info->reg);
+	ctl = readl(cgu->base + pll_info->reg[1]);
 	spin_unlock_irqrestore(&cgu->lock, flags);
 
 	return !!(ctl & BIT(pll_info->enable_bit));
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index 0dc8004..5f87be4 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -42,8 +42,18 @@
  * @stable_bit: the index of the stable bit in the PLL control register
  * @no_bypass_bit: if set, the PLL has no bypass functionality
  */
+enum ingenic_cgu_version {
+	CGU_JZ4740,
+	CGU_JZ4725B,
+	CGU_JZ4770,
+	CGU_JZ4780,
+	CGU_X1000,
+	CGU_X1830,
+};
+
 struct ingenic_cgu_pll_info {
-	unsigned reg;
+	enum ingenic_cgu_version version;
+	unsigned reg[2];
 	const s8 *od_encoding;
 	u8 m_shift, m_bits, m_offset;
 	u8 n_shift, n_bits, n_offset;
diff --git a/drivers/clk/ingenic/jz4725b-cgu.c b/drivers/clk/ingenic/jz4725b-cgu.c
index a3b4635..6da7b41 100644
--- a/drivers/clk/ingenic/jz4725b-cgu.c
+++ b/drivers/clk/ingenic/jz4725b-cgu.c
@@ -53,7 +53,8 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = {
 		"pll", CGU_CLK_PLL,
 		.parents = { JZ4725B_CLK_EXT, -1, -1, -1 },
 		.pll = {
-			.reg = CGU_REG_CPPCR,
+			.version = CGU_JZ4725B,
+			.reg = { -1, CGU_REG_CPPCR },
 			.m_shift = 23,
 			.m_bits = 9,
 			.m_offset = 2,
diff --git a/drivers/clk/ingenic/jz4740-cgu.c b/drivers/clk/ingenic/jz4740-cgu.c
index 4f0e92c..3cf800d 100644
--- a/drivers/clk/ingenic/jz4740-cgu.c
+++ b/drivers/clk/ingenic/jz4740-cgu.c
@@ -68,7 +68,8 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
 		"pll", CGU_CLK_PLL,
 		.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
 		.pll = {
-			.reg = CGU_REG_CPPCR,
+			.version = CGU_JZ4740,
+			.reg = { -1, CGU_REG_CPPCR },
 			.m_shift = 23,
 			.m_bits = 9,
 			.m_offset = 2,
diff --git a/drivers/clk/ingenic/jz4770-cgu.c b/drivers/clk/ingenic/jz4770-cgu.c
index 956dd65..a62dfb1 100644
--- a/drivers/clk/ingenic/jz4770-cgu.c
+++ b/drivers/clk/ingenic/jz4770-cgu.c
@@ -101,7 +101,8 @@ static const struct ingenic_cgu_clk_info jz4770_cgu_clocks[] = {
 		"pll0", CGU_CLK_PLL,
 		.parents = { JZ4770_CLK_EXT },
 		.pll = {
-			.reg = CGU_REG_CPPCR0,
+			.version = CGU_JZ4770,
+			.reg = { -1, CGU_REG_CPPCR0 },
 			.m_shift = 24,
 			.m_bits = 7,
 			.m_offset = 1,
@@ -123,7 +124,8 @@ static const struct ingenic_cgu_clk_info jz4770_cgu_clocks[] = {
 		"pll1", CGU_CLK_PLL,
 		.parents = { JZ4770_CLK_EXT },
 		.pll = {
-			.reg = CGU_REG_CPPCR1,
+			.version = CGU_JZ4770,
+			.reg = { -1, CGU_REG_CPPCR1 },
 			.m_shift = 24,
 			.m_bits = 7,
 			.m_offset = 1,
diff --git a/drivers/clk/ingenic/jz4780-cgu.c b/drivers/clk/ingenic/jz4780-cgu.c
index ea905ff..59356d1b 100644
--- a/drivers/clk/ingenic/jz4780-cgu.c
+++ b/drivers/clk/ingenic/jz4780-cgu.c
@@ -220,7 +220,8 @@ static const struct ingenic_cgu_clk_info jz4780_cgu_clocks[] = {
 	/* PLLs */
 
 #define DEF_PLL(name) { \
-	.reg = CGU_REG_ ## name, \
+	.version = CGU_JZ4780, \
+	.reg = { -1, CGU_REG_ ## name }, \
 	.m_shift = 19, \
 	.m_bits = 13, \
 	.m_offset = 1, \
diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index b22d87b..7179b9f 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -57,7 +57,8 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		"apll", CGU_CLK_PLL,
 		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
 		.pll = {
-			.reg = CGU_REG_APLL,
+			.version = CGU_X1000,
+			.reg = { -1, CGU_REG_APLL },
 			.m_shift = 24,
 			.m_bits = 7,
 			.m_offset = 1,
@@ -78,7 +79,8 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		"mpll", CGU_CLK_PLL,
 		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
 		.pll = {
-			.reg = CGU_REG_MPLL,
+			.version = CGU_X1000,
+			.reg = { -1, CGU_REG_MPLL },
 			.m_shift = 24,
 			.m_bits = 7,
 			.m_offset = 1,
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] dt-bindings: clock: Add X1830 bindings.
  2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
@ 2019-11-27  3:32 ` Zhou Yanjie
  2019-12-05 20:38   ` Rob Herring
  2019-11-27  3:32 ` [PATCH 3/5] clk: Ingenic: Add CGU driver for X1830 Zhou Yanjie
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

Add the clock bindings for the X1830 Soc from Ingenic.

Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
---
 .../devicetree/bindings/clock/ingenic,cgu.txt      |  1 +
 include/dt-bindings/clock/x1830-cgu.h              | 46 ++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 include/dt-bindings/clock/x1830-cgu.h

diff --git a/Documentation/devicetree/bindings/clock/ingenic,cgu.txt b/Documentation/devicetree/bindings/clock/ingenic,cgu.txt
index 75598e6..74bfc57 100644
--- a/Documentation/devicetree/bindings/clock/ingenic,cgu.txt
+++ b/Documentation/devicetree/bindings/clock/ingenic,cgu.txt
@@ -12,6 +12,7 @@ Required properties:
   * ingenic,jz4770-cgu
   * ingenic,jz4780-cgu
   * ingenic,x1000-cgu
+  * ingenic,x1830-cgu
 - reg : The address & length of the CGU registers.
 - clocks : List of phandle & clock specifiers for clocks external to the CGU.
   Two such external clocks should be specified - first the external crystal
diff --git a/include/dt-bindings/clock/x1830-cgu.h b/include/dt-bindings/clock/x1830-cgu.h
new file mode 100644
index 00000000..6499170
--- /dev/null
+++ b/include/dt-bindings/clock/x1830-cgu.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides clock numbers for the ingenic,x1830-cgu DT binding.
+ *
+ * They are roughly ordered as:
+ *   - external clocks
+ *   - PLLs
+ *   - muxes/dividers in the order they appear in the x1830 programmers manual
+ *   - gates in order of their bit in the CLKGR* registers
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_X1830_CGU_H__
+#define __DT_BINDINGS_CLOCK_X1830_CGU_H__
+
+#define X1830_CLK_EXCLK		0
+#define X1830_CLK_RTCLK		1
+#define X1830_CLK_APLL		2
+#define X1830_CLK_MPLL		3
+#define X1830_CLK_EPLL		4
+#define X1830_CLK_VPLL		5
+#define X1830_CLK_SCLKA		6
+#define X1830_CLK_CPUMUX	7
+#define X1830_CLK_CPU		8
+#define X1830_CLK_L2CACHE	9
+#define X1830_CLK_AHB0		10
+#define X1830_CLK_AHB2PMUX	11
+#define X1830_CLK_AHB2		12
+#define X1830_CLK_PCLK		13
+#define X1830_CLK_DDR		14
+#define X1830_CLK_MAC		15
+#define X1830_CLK_MSCMUX	16
+#define X1830_CLK_MSC0		17
+#define X1830_CLK_MSC1		18
+#define X1830_CLK_SSIPLL	19
+#define X1830_CLK_SSIMUX	20
+#define X1830_CLK_SSI0		21
+#define X1830_CLK_SMB0		22
+#define X1830_CLK_SMB1		23
+#define X1830_CLK_SMB2		24
+#define X1830_CLK_UART0		25
+#define X1830_CLK_UART1		26
+#define X1830_CLK_SSI1		27
+#define X1830_CLK_SFC		28
+#define X1830_CLK_PDMA		29
+
+#endif /* __DT_BINDINGS_CLOCK_X1830_CGU_H__ */
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] clk: Ingenic: Add CGU driver for X1830.
  2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 2/5] dt-bindings: clock: Add X1830 bindings Zhou Yanjie
@ 2019-11-27  3:32 ` Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000 Zhou Yanjie
  2019-11-27  3:32 ` [PATCH 5/5] clk: Ingenic: " Zhou Yanjie
  4 siblings, 0 replies; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

Add support for the clocks provided by the CGU in the Ingenic X1830
SoC, making use of the cgu code to do the heavy lifting.

Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
---
 drivers/clk/ingenic/Kconfig     |  10 ++
 drivers/clk/ingenic/Makefile    |   1 +
 drivers/clk/ingenic/x1830-cgu.c | 336 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/clk/ingenic/x1830-cgu.c

diff --git a/drivers/clk/ingenic/Kconfig b/drivers/clk/ingenic/Kconfig
index fb7b399..59c6c2c 100644
--- a/drivers/clk/ingenic/Kconfig
+++ b/drivers/clk/ingenic/Kconfig
@@ -55,6 +55,16 @@ config INGENIC_CGU_X1000
 
 	  If building for a X1000 SoC, you want to say Y here.
 
+config INGENIC_CGU_X1830
+	bool "Ingenic X1830 CGU driver"
+	default MACH_X1830
+	select INGENIC_CGU_COMMON
+	help
+	  Support the clocks provided by the CGU hardware on Ingenic X1830
+	  and compatible SoCs.
+
+	  If building for a X1830 SoC, you want to say Y here.
+
 config INGENIC_TCU_CLK
 	bool "Ingenic JZ47xx TCU clocks driver"
 	default MACH_INGENIC
diff --git a/drivers/clk/ingenic/Makefile b/drivers/clk/ingenic/Makefile
index 8b1dad9..aaa4bff 100644
--- a/drivers/clk/ingenic/Makefile
+++ b/drivers/clk/ingenic/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_INGENIC_CGU_JZ4725B)	+= jz4725b-cgu.o
 obj-$(CONFIG_INGENIC_CGU_JZ4770)	+= jz4770-cgu.o
 obj-$(CONFIG_INGENIC_CGU_JZ4780)	+= jz4780-cgu.o
 obj-$(CONFIG_INGENIC_CGU_X1000)		+= x1000-cgu.o
+obj-$(CONFIG_INGENIC_CGU_X1830)		+= x1830-cgu.o
 obj-$(CONFIG_INGENIC_TCU_CLK)		+= tcu.o
diff --git a/drivers/clk/ingenic/x1830-cgu.c b/drivers/clk/ingenic/x1830-cgu.c
new file mode 100644
index 00000000..946af6f
--- /dev/null
+++ b/drivers/clk/ingenic/x1830-cgu.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * X1830 SoC CGU driver
+ * Copyright (c) 2019 Zhou Yanjie <zhouyanjie@zoho.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <dt-bindings/clock/x1830-cgu.h>
+#include "cgu.h"
+#include "pm.h"
+
+/* CGU register offsets */
+#define CGU_REG_CPCCR		0x00
+#define CGU_REG_CPPCR		0x0c
+#define CGU_REG_APLL		0x10
+#define CGU_REG_MPLL		0x14
+#define CGU_REG_EPLL		0x58
+#define CGU_REG_VPLL		0xe0
+#define CGU_REG_CLKGR0		0x20
+#define CGU_REG_OPCR		0x24
+#define CGU_REG_CLKGR1		0x28
+#define CGU_REG_DDRCDR		0x2c
+#define CGU_REG_USBPCR		0x3c
+#define CGU_REG_USBRDT		0x40
+#define CGU_REG_USBVBFIL	0x44
+#define CGU_REG_USBPCR1		0x48
+#define CGU_REG_MACCDR		0x54
+#define CGU_REG_I2SCDR		0x60
+#define CGU_REG_LPCDR		0x64
+#define CGU_REG_MSC0CDR		0x68
+#define CGU_REG_I2SCDR1		0x70
+#define CGU_REG_SSICDR		0x74
+#define CGU_REG_CIMCDR		0x7c
+#define CGU_REG_MSC1CDR		0xa4
+#define CGU_REG_CMP_INTR	0xb0
+#define CGU_REG_CMP_INTRE	0xb4
+#define CGU_REG_DRCG		0xd0
+#define CGU_REG_CPCSR		0xd4
+#define CGU_REG_MACPHYC		0xe8
+
+/* bits within the OPCR register */
+#define OPCR_SPENDN0		BIT(7)
+#define OPCR_SPENDN1		BIT(6)
+
+static struct ingenic_cgu *cgu;
+
+static const s8 pll_od_encoding[64] = {
+	0x0, 0x1,  -1, 0x2,  -1,  -1,  -1, 0x3,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1, 0x4,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1,  -1,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1, 0x5,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1,  -1,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1,  -1,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1,  -1,
+	 -1,  -1,  -1,  -1,  -1,  -1,  -1, 0x6,
+};
+
+static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
+
+	/* External clocks */
+
+	[X1830_CLK_EXCLK] = { "ext", CGU_CLK_EXT },
+	[X1830_CLK_RTCLK] = { "rtc", CGU_CLK_EXT },
+
+	/* PLLs */
+
+	[X1830_CLK_APLL] = {
+		"apll", CGU_CLK_PLL,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.pll = {
+			.version = CGU_X1830,
+			.reg = { CGU_REG_CPPCR, CGU_REG_APLL },
+			.m_shift = 20,
+			.m_bits = 9,
+			.m_offset = 1,
+			.n_shift = 14,
+			.n_bits = 6,
+			.n_offset = 1,
+			.od_shift = 11,
+			.od_bits = 3,
+			.od_max = 64,
+			.od_encoding = pll_od_encoding,
+			.bypass_bit = 30,
+			.enable_bit = 0,
+			.stable_bit = 3,
+		},
+	},
+
+	[X1830_CLK_MPLL] = {
+		"mpll", CGU_CLK_PLL,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.pll = {
+			.version = CGU_X1830,
+			.reg = { CGU_REG_CPPCR, CGU_REG_MPLL },
+			.m_shift = 20,
+			.m_bits = 9,
+			.m_offset = 1,
+			.n_shift = 14,
+			.n_bits = 6,
+			.n_offset = 1,
+			.od_shift = 11,
+			.od_bits = 3,
+			.od_max = 64,
+			.od_encoding = pll_od_encoding,
+			.bypass_bit = 28,
+			.enable_bit = 0,
+			.stable_bit = 3,
+		},
+	},
+
+	[X1830_CLK_EPLL] = {
+		"epll", CGU_CLK_PLL,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.pll = {
+			.version = CGU_X1830,
+			.reg = { CGU_REG_CPPCR, CGU_REG_EPLL },
+			.m_shift = 20,
+			.m_bits = 9,
+			.m_offset = 1,
+			.n_shift = 14,
+			.n_bits = 6,
+			.n_offset = 1,
+			.od_shift = 11,
+			.od_bits = 3,
+			.od_max = 64,
+			.od_encoding = pll_od_encoding,
+			.bypass_bit = 24,
+			.enable_bit = 0,
+			.stable_bit = 3,
+		},
+	},
+
+	[X1830_CLK_VPLL] = {
+		"vpll", CGU_CLK_PLL,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.pll = {
+			.version = CGU_X1830,
+			.reg = { CGU_REG_CPPCR, CGU_REG_VPLL },
+			.m_shift = 20,
+			.m_bits = 9,
+			.m_offset = 1,
+			.n_shift = 14,
+			.n_bits = 6,
+			.n_offset = 1,
+			.od_shift = 11,
+			.od_bits = 3,
+			.od_max = 64,
+			.od_encoding = pll_od_encoding,
+			.bypass_bit = 26,
+			.enable_bit = 0,
+			.stable_bit = 3,
+		},
+	},
+
+	/* Muxes & dividers */
+
+	[X1830_CLK_SCLKA] = {
+		"sclk_a", CGU_CLK_MUX,
+		.parents = { -1, X1830_CLK_EXCLK, X1830_CLK_APLL, -1 },
+		.mux = { CGU_REG_CPCCR, 30, 2 },
+	},
+
+	[X1830_CLK_CPUMUX] = {
+		"cpu_mux", CGU_CLK_MUX,
+		.parents = { -1, X1830_CLK_SCLKA, X1830_CLK_MPLL, -1 },
+		.mux = { CGU_REG_CPCCR, 28, 2 },
+	},
+
+	[X1830_CLK_CPU] = {
+		"cpu", CGU_CLK_DIV,
+		.parents = { X1830_CLK_CPUMUX, -1, -1, -1 },
+		.div = { CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1 },
+	},
+
+	[X1830_CLK_L2CACHE] = {
+		"l2cache", CGU_CLK_DIV,
+		.parents = { X1830_CLK_CPUMUX, -1, -1, -1 },
+		.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
+	},
+
+	[X1830_CLK_AHB0] = {
+		"ahb0", CGU_CLK_MUX | CGU_CLK_DIV,
+		.parents = { -1, X1830_CLK_SCLKA, X1830_CLK_MPLL, -1 },
+		.mux = { CGU_REG_CPCCR, 26, 2 },
+		.div = { CGU_REG_CPCCR, 8, 1, 4, 21, -1, -1 },
+	},
+
+	[X1830_CLK_AHB2PMUX] = {
+		"ahb2_apb_mux", CGU_CLK_MUX,
+		.parents = { -1, X1830_CLK_SCLKA, X1830_CLK_MPLL, -1 },
+		.mux = { CGU_REG_CPCCR, 24, 2 },
+	},
+
+	[X1830_CLK_AHB2] = {
+		"ahb2", CGU_CLK_DIV,
+		.parents = { X1830_CLK_AHB2PMUX, -1, -1, -1 },
+		.div = { CGU_REG_CPCCR, 12, 1, 4, 20, -1, -1 },
+	},
+
+	[X1830_CLK_PCLK] = {
+		"pclk", CGU_CLK_DIV,
+		.parents = { X1830_CLK_AHB2PMUX, -1, -1, -1 },
+		.div = { CGU_REG_CPCCR, 16, 1, 4, 20, -1, -1 },
+	},
+
+	[X1830_CLK_DDR] = {
+		"ddr", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+		.parents = { -1, X1830_CLK_SCLKA, X1830_CLK_MPLL, -1 },
+		.mux = { CGU_REG_DDRCDR, 30, 2 },
+		.div = { CGU_REG_DDRCDR, 0, 1, 4, 29, 28, 27 },
+		.gate = { CGU_REG_CLKGR0, 31 },
+	},
+
+	[X1830_CLK_MAC] = {
+		"mac", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+		.parents = { X1830_CLK_SCLKA, X1830_CLK_MPLL,
+					 X1830_CLK_VPLL, X1830_CLK_EPLL },
+		.mux = { CGU_REG_MACCDR, 30, 2 },
+		.div = { CGU_REG_MACCDR, 0, 1, 8, 29, 28, 27 },
+		.gate = { CGU_REG_CLKGR1, 4 },
+	},
+
+	[X1830_CLK_MSCMUX] = {
+		"msc_mux", CGU_CLK_MUX,
+		.parents = { X1830_CLK_SCLKA, X1830_CLK_MPLL,
+					 X1830_CLK_VPLL, X1830_CLK_EPLL },
+		.mux = { CGU_REG_MSC0CDR, 30, 2 },
+	},
+
+	[X1830_CLK_MSC0] = {
+		"msc0", CGU_CLK_DIV | CGU_CLK_GATE,
+		.parents = { X1830_CLK_MSCMUX, -1, -1, -1 },
+		.div = { CGU_REG_MSC0CDR, 0, 2, 8, 29, 28, 27 },
+		.gate = { CGU_REG_CLKGR0, 4 },
+	},
+
+	[X1830_CLK_MSC1] = {
+		"msc1", CGU_CLK_DIV | CGU_CLK_GATE,
+		.parents = { X1830_CLK_MSCMUX, -1, -1, -1 },
+		.div = { CGU_REG_MSC1CDR, 0, 2, 8, 29, 28, 27 },
+		.gate = { CGU_REG_CLKGR0, 5 },
+	},
+
+	[X1830_CLK_SSIPLL] = {
+		"ssi_pll", CGU_CLK_MUX | CGU_CLK_DIV,
+		.parents = { X1830_CLK_SCLKA, X1830_CLK_MPLL,
+					 X1830_CLK_VPLL, X1830_CLK_EPLL },
+		.mux = { CGU_REG_SSICDR, 30, 2 },
+		.div = { CGU_REG_SSICDR, 0, 1, 8, 28, 27, 26 },
+	},
+
+	[X1830_CLK_SSIMUX] = {
+		"ssi_mux", CGU_CLK_MUX,
+		.parents = { X1830_CLK_EXCLK, X1830_CLK_SSIPLL, -1, -1 },
+		.mux = { CGU_REG_SSICDR, 29, 1 },
+	},
+
+	/* Gate-only clocks */
+
+	[X1830_CLK_SSI0] = {
+		"ssi0", CGU_CLK_GATE,
+		.parents = { X1830_CLK_SSIMUX, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 6 },
+	},
+
+	[X1830_CLK_SMB0] = {
+		"smb0", CGU_CLK_GATE,
+		.parents = { X1830_CLK_PCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 7 },
+	},
+
+	[X1830_CLK_SMB1] = {
+		"smb1", CGU_CLK_GATE,
+		.parents = { X1830_CLK_PCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 8 },
+	},
+
+	[X1830_CLK_SMB2] = {
+		"smb2", CGU_CLK_GATE,
+		.parents = { X1830_CLK_PCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 9 },
+	},
+
+	[X1830_CLK_UART0] = {
+		"uart0", CGU_CLK_GATE,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 14 },
+	},
+
+	[X1830_CLK_UART1] = {
+		"uart1", CGU_CLK_GATE,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 15 },
+	},
+
+	[X1830_CLK_SSI1] = {
+		"ssi1", CGU_CLK_GATE,
+		.parents = { X1830_CLK_SSIMUX, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 19 },
+	},
+
+	[X1830_CLK_SFC] = {
+		"sfc", CGU_CLK_GATE,
+		.parents = { X1830_CLK_SSIPLL, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 20 },
+	},
+
+	[X1830_CLK_PDMA] = {
+		"pdma", CGU_CLK_GATE,
+		.parents = { X1830_CLK_EXCLK, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR0, 21 },
+	},
+};
+
+static void __init x1830_cgu_init(struct device_node *np)
+{
+	int retval;
+
+	cgu = ingenic_cgu_new(x1830_cgu_clocks,
+			      ARRAY_SIZE(x1830_cgu_clocks), np);
+	if (!cgu) {
+		pr_err("%s: failed to initialise CGU\n", __func__);
+		return;
+	}
+
+	retval = ingenic_cgu_register_clocks(cgu);
+	if (retval) {
+		pr_err("%s: failed to register CGU Clocks\n", __func__);
+		return;
+	}
+
+	ingenic_cgu_register_syscore_ops(cgu);
+}
+CLK_OF_DECLARE_DRIVER(x1830_cgu, "ingenic,x1830-cgu", x1830_cgu_init);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000.
  2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
                   ` (2 preceding siblings ...)
  2019-11-27  3:32 ` [PATCH 3/5] clk: Ingenic: Add CGU driver for X1830 Zhou Yanjie
@ 2019-11-27  3:32 ` Zhou Yanjie
  2019-11-27 17:19   ` Paul Cercueil
  2019-11-27  3:32 ` [PATCH 5/5] clk: Ingenic: " Zhou Yanjie
  4 siblings, 1 reply; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

Add the USB OTC clock bindings for the X1000 Soc from Ingenic.

Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
---
 include/dt-bindings/clock/x1000-cgu.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/dt-bindings/clock/x1000-cgu.h b/include/dt-bindings/clock/x1000-cgu.h
index bbaebaf..c401fce 100644
--- a/include/dt-bindings/clock/x1000-cgu.h
+++ b/include/dt-bindings/clock/x1000-cgu.h
@@ -29,16 +29,17 @@
 #define X1000_CLK_MSCMUX	14
 #define X1000_CLK_MSC0		15
 #define X1000_CLK_MSC1		16
-#define X1000_CLK_SSIPLL	17
-#define X1000_CLK_SSIMUX	18
-#define X1000_CLK_SFC		19
-#define X1000_CLK_I2C0		20
-#define X1000_CLK_I2C1		21
-#define X1000_CLK_I2C2		22
-#define X1000_CLK_UART0		23
-#define X1000_CLK_UART1		24
-#define X1000_CLK_UART2		25
-#define X1000_CLK_SSI		26
-#define X1000_CLK_PDMA		27
+#define X1000_CLK_OTG		17
+#define X1000_CLK_SSIPLL	18
+#define X1000_CLK_SSIMUX	19
+#define X1000_CLK_SFC		20
+#define X1000_CLK_I2C0		21
+#define X1000_CLK_I2C1		22
+#define X1000_CLK_I2C2		23
+#define X1000_CLK_UART0		24
+#define X1000_CLK_UART1		25
+#define X1000_CLK_UART2		26
+#define X1000_CLK_SSI		27
+#define X1000_CLK_PDMA		28
 
 #endif /* __DT_BINDINGS_CLOCK_X1000_CGU_H__ */
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] clk: Ingenic: Add USB OTG clock for X1000.
  2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
                   ` (3 preceding siblings ...)
  2019-11-27  3:32 ` [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000 Zhou Yanjie
@ 2019-11-27  3:32 ` Zhou Yanjie
  4 siblings, 0 replies; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-27  3:32 UTC (permalink / raw)
  To: linux-mips
  Cc: linux-kernel, linux-clk, devicetree, robh+dt, paul.burton,
	paulburton, mturquette, sboyd, mark.rutland, syq, paul,
	sernia.zhou, zhenwenjin

1.Add the USB OTC clock driver for the X1000 Soc from Ingenic.
2.Use the "CLK_OF_DECLARE_DRIVER" instead "CLK_OF_DECLARE" like
  the other CGU drivers.

Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
---
 drivers/clk/ingenic/x1000-cgu.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index 7179b9f..7da7c69 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -18,6 +18,11 @@
 #define CGU_REG_CLKGR		0x20
 #define CGU_REG_OPCR		0x24
 #define CGU_REG_DDRCDR		0x2c
+#define CGU_REG_USBPCR		0x3c
+#define CGU_REG_USBRDT		0x40
+#define CGU_REG_USBVBFIL	0x44
+#define CGU_REG_USBPCR1		0x48
+#define CGU_REG_USBCDR		0x50
 #define CGU_REG_MACCDR		0x54
 #define CGU_REG_I2SCDR		0x60
 #define CGU_REG_LPCDR		0x64
@@ -184,6 +189,15 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
 		.gate = { CGU_REG_CLKGR, 5 },
 	},
 
+	[X1000_CLK_OTG] = {
+		"otg", CGU_CLK_DIV | CGU_CLK_GATE | CGU_CLK_MUX,
+		.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 },
+	},
+
 	[X1000_CLK_SSIPLL] = {
 		"ssi_pll", CGU_CLK_MUX | CGU_CLK_DIV,
 		.parents = { X1000_CLK_SCLKA, X1000_CLK_MPLL, -1, -1 },
@@ -273,4 +287,4 @@ static void __init x1000_cgu_init(struct device_node *np)
 
 	ingenic_cgu_register_syscore_ops(cgu);
 }
-CLK_OF_DECLARE(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
+CLK_OF_DECLARE_DRIVER(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000.
  2019-11-27  3:32 ` [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000 Zhou Yanjie
@ 2019-11-27 17:19   ` Paul Cercueil
  2019-11-28  5:44     ` Zhou Yanjie
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Cercueil @ 2019-11-27 17:19 UTC (permalink / raw)
  To: Zhou Yanjie
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	sernia.zhou, zhenwenjin

Hi Zhou,


Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
écrit :
> Add the USB OTC clock bindings for the X1000 Soc from Ingenic.
> 
> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
> ---
>  include/dt-bindings/clock/x1000-cgu.h | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/include/dt-bindings/clock/x1000-cgu.h 
> b/include/dt-bindings/clock/x1000-cgu.h
> index bbaebaf..c401fce 100644
> --- a/include/dt-bindings/clock/x1000-cgu.h
> +++ b/include/dt-bindings/clock/x1000-cgu.h
> @@ -29,16 +29,17 @@
>  #define X1000_CLK_MSCMUX	14
>  #define X1000_CLK_MSC0		15
>  #define X1000_CLK_MSC1		16
> -#define X1000_CLK_SSIPLL	17
> -#define X1000_CLK_SSIMUX	18
> -#define X1000_CLK_SFC		19
> -#define X1000_CLK_I2C0		20
> -#define X1000_CLK_I2C1		21
> -#define X1000_CLK_I2C2		22
> -#define X1000_CLK_UART0		23
> -#define X1000_CLK_UART1		24
> -#define X1000_CLK_UART2		25
> -#define X1000_CLK_SSI		26
> -#define X1000_CLK_PDMA		27

You can't do that. These macros are ABI now, since they are used in the 
devicetree. Just use the next valid number for your OTG clock.

Cheers,
-Paul

> +#define X1000_CLK_OTG		17
> +#define X1000_CLK_SSIPLL	18
> +#define X1000_CLK_SSIMUX	19
> +#define X1000_CLK_SFC		20
> +#define X1000_CLK_I2C0		21
> +#define X1000_CLK_I2C1		22
> +#define X1000_CLK_I2C2		23
> +#define X1000_CLK_UART0		24
> +#define X1000_CLK_UART1		25
> +#define X1000_CLK_UART2		26
> +#define X1000_CLK_SSI		27
> +#define X1000_CLK_PDMA		28
> 
>  #endif /* __DT_BINDINGS_CLOCK_X1000_CGU_H__ */
> --
> 2.7.4
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
  2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
@ 2019-11-27 17:37   ` Paul Cercueil
  2019-11-28  6:29     ` Zhou Yanjie
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Cercueil @ 2019-11-27 17:37 UTC (permalink / raw)
  To: Zhou Yanjie
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	sernia.zhou, zhenwenjin

Hi Zhou,


Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
écrit :
> 1.Adjust the PLL related code in "cgu.c" and "cgu.h" to make it
>   compatible with the X1830 Soc from Ingenic.
> 2.Adjust the code in "jz4740-cgu.c" to be compatible with the
>   new cgu code.
> 3.Adjust the code in "jz4725b-cgu.c" to be compatible with the
>   new cgu code.
> 4.Adjust the code in "jz4770-cgu.c" to be compatible with the
>   new cgu code.
> 5.Adjust the code in "jz4780-cgu.c" to be compatible with the
>   new cgu code.
> 6.Adjust the code in "x1000-cgu.c" to be compatible with the
>   new cgu code.
> 
> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
> ---
>  drivers/clk/ingenic/cgu.c         | 55 
> +++++++++++++++++++++++++++++----------
>  drivers/clk/ingenic/cgu.h         | 12 ++++++++-
>  drivers/clk/ingenic/jz4725b-cgu.c |  3 ++-
>  drivers/clk/ingenic/jz4740-cgu.c  |  3 ++-
>  drivers/clk/ingenic/jz4770-cgu.c  |  6 +++--
>  drivers/clk/ingenic/jz4780-cgu.c  |  3 ++-
>  drivers/clk/ingenic/x1000-cgu.c   |  6 +++--
>  7 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 6e96303..c3c69a8 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -84,7 +84,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned 
> long parent_rate)
>  	pll_info = &clk_info->pll;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);

I really don't like this patch. There is no info on what reg[1] and 
reg[0] are. First, don't use hardcoded numbers, use macros with 
meaningful names. Second, why not just have two fields instead of one 
2-values array? That would remove a lot of the noise.


>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
> @@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  	n += pll_info->n_offset;
>  	od_enc = ctl >> pll_info->od_shift;
>  	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> -	bypass = !pll_info->no_bypass_bit &&
> -		 !!(ctl & BIT(pll_info->bypass_bit));
> +
> +	if (pll_info->version >= CGU_X1830) {
> +		spin_lock_irqsave(&cgu->lock, flags);
> +		ctl = readl(cgu->base + pll_info->reg[0]);
> +		spin_unlock_irqrestore(&cgu->lock, flags);

Why the spinlock?


> +
> +		bypass = !pll_info->no_bypass_bit &&
> +			 !!(ctl & BIT(pll_info->bypass_bit));
> +	} else

Please comply to the kernel coding style - use brackets after the else.

> +		bypass = !pll_info->no_bypass_bit &&
> +			 !!(ctl & BIT(pll_info->bypass_bit));
> 
>  	if (bypass)
>  		return parent_rate;
> @@ -106,7 +115,10 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  	BUG_ON(od == pll_info->od_max);
>  	od++;
> 
> -	return div_u64((u64)parent_rate * m, n * od);
> +	if (pll_info->version >= CGU_X1830)
> +		return div_u64((u64)parent_rate * m * 2, n * od);

Where does that *2 come from?

> +	else
> +		return div_u64((u64)parent_rate * m, n * od);
>  }
> 
>  static unsigned long
> @@ -139,7 +151,10 @@ ingenic_pll_calc(const struct 
> ingenic_cgu_clk_info *clk_info,
>  	if (pod)
>  		*pod = od;
> 
> -	return div_u64((u64)parent_rate * m, n * od);
> +	if (pll_info->version >= CGU_X1830)
> +		return div_u64((u64)parent_rate * m * 2, n * od);
> +	else
> +		return div_u64((u64)parent_rate * m, n * od);
>  }
> 
>  static inline const struct ingenic_cgu_clk_info *to_clk_info(
> @@ -183,7 +198,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
> long req_rate,
>  			clk_info->name, req_rate, rate);
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
> 
>  	ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << pll_info->m_shift);
>  	ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
> @@ -194,7 +209,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
> long req_rate,
>  	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);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	return 0;
> @@ -212,16 +227,28 @@ static int ingenic_pll_enable(struct clk_hw *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> 
> -	ctl &= ~BIT(pll_info->bypass_bit);
> +	if (pll_info->version >= CGU_X1830) {
> +		ctl = readl(cgu->base + pll_info->reg[0]);
> +
> +		ctl &= ~BIT(pll_info->bypass_bit);
> +
> +		writel(ctl, cgu->base + pll_info->reg[0]);
> +
> +		ctl = readl(cgu->base + pll_info->reg[1]);
> +	} else {
> +		ctl = readl(cgu->base + pll_info->reg[1]);
> +
> +		ctl &= ~BIT(pll_info->bypass_bit);
> +	}
> +
>  	ctl |= BIT(pll_info->enable_bit);
> 
> -	writel(ctl, cgu->base + pll_info->reg);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
> 
>  	/* wait for the PLL to stabilise */
>  	for (i = 0; i < timeout; i++) {
> -		ctl = readl(cgu->base + pll_info->reg);
> +		ctl = readl(cgu->base + pll_info->reg[1]);
>  		if (ctl & BIT(pll_info->stable_bit))
>  			break;
>  		mdelay(1);
> @@ -245,11 +272,11 @@ static void ingenic_pll_disable(struct clk_hw 
> *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
> 
>  	ctl &= ~BIT(pll_info->enable_bit);
> 
> -	writel(ctl, cgu->base + pll_info->reg);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
>  }
> 
> @@ -263,7 +290,7 @@ static int ingenic_pll_is_enabled(struct clk_hw 
> *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	return !!(ctl & BIT(pll_info->enable_bit));
> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
> index 0dc8004..5f87be4 100644
> --- a/drivers/clk/ingenic/cgu.h
> +++ b/drivers/clk/ingenic/cgu.h
> @@ -42,8 +42,18 @@
>   * @stable_bit: the index of the stable bit in the PLL control 
> register
>   * @no_bypass_bit: if set, the PLL has no bypass functionality
>   */
> +enum ingenic_cgu_version {
> +	CGU_JZ4740,
> +	CGU_JZ4725B,
> +	CGU_JZ4770,
> +	CGU_JZ4780,
> +	CGU_X1000,
> +	CGU_X1830,
> +};
> +
>  struct ingenic_cgu_pll_info {
> -	unsigned reg;
> +	enum ingenic_cgu_version version;
> +	unsigned reg[2];
>  	const s8 *od_encoding;
>  	u8 m_shift, m_bits, m_offset;
>  	u8 n_shift, n_bits, n_offset;
> diff --git a/drivers/clk/ingenic/jz4725b-cgu.c 
> b/drivers/clk/ingenic/jz4725b-cgu.c
> index a3b4635..6da7b41 100644
> --- a/drivers/clk/ingenic/jz4725b-cgu.c
> +++ b/drivers/clk/ingenic/jz4725b-cgu.c
> @@ -53,7 +53,8 @@ static const struct ingenic_cgu_clk_info 
> jz4725b_cgu_clocks[] = {
>  		"pll", CGU_CLK_PLL,
>  		.parents = { JZ4725B_CLK_EXT, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR,
> +			.version = CGU_JZ4725B,
> +			.reg = { -1, CGU_REG_CPPCR },
>  			.m_shift = 23,
>  			.m_bits = 9,
>  			.m_offset = 2,
> diff --git a/drivers/clk/ingenic/jz4740-cgu.c 
> b/drivers/clk/ingenic/jz4740-cgu.c
> index 4f0e92c..3cf800d 100644
> --- a/drivers/clk/ingenic/jz4740-cgu.c
> +++ b/drivers/clk/ingenic/jz4740-cgu.c
> @@ -68,7 +68,8 @@ static const struct ingenic_cgu_clk_info 
> jz4740_cgu_clocks[] = {
>  		"pll", CGU_CLK_PLL,
>  		.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR,
> +			.version = CGU_JZ4740,
> +			.reg = { -1, CGU_REG_CPPCR },
>  			.m_shift = 23,
>  			.m_bits = 9,
>  			.m_offset = 2,
> diff --git a/drivers/clk/ingenic/jz4770-cgu.c 
> b/drivers/clk/ingenic/jz4770-cgu.c
> index 956dd65..a62dfb1 100644
> --- a/drivers/clk/ingenic/jz4770-cgu.c
> +++ b/drivers/clk/ingenic/jz4770-cgu.c
> @@ -101,7 +101,8 @@ static const struct ingenic_cgu_clk_info 
> jz4770_cgu_clocks[] = {
>  		"pll0", CGU_CLK_PLL,
>  		.parents = { JZ4770_CLK_EXT },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR0,
> +			.version = CGU_JZ4770,
> +			.reg = { -1, CGU_REG_CPPCR0 },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> @@ -123,7 +124,8 @@ static const struct ingenic_cgu_clk_info 
> jz4770_cgu_clocks[] = {
>  		"pll1", CGU_CLK_PLL,
>  		.parents = { JZ4770_CLK_EXT },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR1,
> +			.version = CGU_JZ4770,
> +			.reg = { -1, CGU_REG_CPPCR1 },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> diff --git a/drivers/clk/ingenic/jz4780-cgu.c 
> b/drivers/clk/ingenic/jz4780-cgu.c
> index ea905ff..59356d1b 100644
> --- a/drivers/clk/ingenic/jz4780-cgu.c
> +++ b/drivers/clk/ingenic/jz4780-cgu.c
> @@ -220,7 +220,8 @@ static const struct ingenic_cgu_clk_info 
> jz4780_cgu_clocks[] = {
>  	/* PLLs */
> 
>  #define DEF_PLL(name) { \
> -	.reg = CGU_REG_ ## name, \
> +	.version = CGU_JZ4780, \
> +	.reg = { -1, CGU_REG_ ## name }, \
>  	.m_shift = 19, \
>  	.m_bits = 13, \
>  	.m_offset = 1, \
> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> b/drivers/clk/ingenic/x1000-cgu.c
> index b22d87b..7179b9f 100644
> --- a/drivers/clk/ingenic/x1000-cgu.c
> +++ b/drivers/clk/ingenic/x1000-cgu.c
> @@ -57,7 +57,8 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		"apll", CGU_CLK_PLL,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_APLL,
> +			.version = CGU_X1000,
> +			.reg = { -1, CGU_REG_APLL },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> @@ -78,7 +79,8 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		"mpll", CGU_CLK_PLL,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_MPLL,
> +			.version = CGU_X1000,
> +			.reg = { -1, CGU_REG_MPLL },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> --
> 2.7.4
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000.
  2019-11-27 17:19   ` Paul Cercueil
@ 2019-11-28  5:44     ` Zhou Yanjie
  0 siblings, 0 replies; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-28  5:44 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	sernia.zhou, zhenwenjin

Hi Paul,

On 2019年11月28日 01:19, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
> écrit :
>> Add the USB OTC clock bindings for the X1000 Soc from Ingenic.
>>
>> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
>> ---
>>  include/dt-bindings/clock/x1000-cgu.h | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/dt-bindings/clock/x1000-cgu.h 
>> b/include/dt-bindings/clock/x1000-cgu.h
>> index bbaebaf..c401fce 100644
>> --- a/include/dt-bindings/clock/x1000-cgu.h
>> +++ b/include/dt-bindings/clock/x1000-cgu.h
>> @@ -29,16 +29,17 @@
>>  #define X1000_CLK_MSCMUX    14
>>  #define X1000_CLK_MSC0        15
>>  #define X1000_CLK_MSC1        16
>> -#define X1000_CLK_SSIPLL    17
>> -#define X1000_CLK_SSIMUX    18
>> -#define X1000_CLK_SFC        19
>> -#define X1000_CLK_I2C0        20
>> -#define X1000_CLK_I2C1        21
>> -#define X1000_CLK_I2C2        22
>> -#define X1000_CLK_UART0        23
>> -#define X1000_CLK_UART1        24
>> -#define X1000_CLK_UART2        25
>> -#define X1000_CLK_SSI        26
>> -#define X1000_CLK_PDMA        27
>
> You can't do that. These macros are ABI now, since they are used in 
> the devicetree. Just use the next valid number for your OTG clock.
>

My fault, I will fix this in v2.

> Cheers,
> -Paul
>
>> +#define X1000_CLK_OTG        17
>> +#define X1000_CLK_SSIPLL    18
>> +#define X1000_CLK_SSIMUX    19
>> +#define X1000_CLK_SFC        20
>> +#define X1000_CLK_I2C0        21
>> +#define X1000_CLK_I2C1        22
>> +#define X1000_CLK_I2C2        23
>> +#define X1000_CLK_UART0        24
>> +#define X1000_CLK_UART1        25
>> +#define X1000_CLK_UART2        26
>> +#define X1000_CLK_SSI        27
>> +#define X1000_CLK_PDMA        28
>>
>>  #endif /* __DT_BINDINGS_CLOCK_X1000_CGU_H__ */
>> -- 
>> 2.7.4
>>
>>
>
>




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
  2019-11-27 17:37   ` Paul Cercueil
@ 2019-11-28  6:29     ` Zhou Yanjie
  2019-11-29 11:23       ` Paul Cercueil
  0 siblings, 1 reply; 13+ messages in thread
From: Zhou Yanjie @ 2019-11-28  6:29 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	sernia.zhou, zhenwenjin

Hi Paul,

On 2019年11月28日 01:37, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
> écrit :
>> 1.Adjust the PLL related code in "cgu.c" and "cgu.h" to make it
>>   compatible with the X1830 Soc from Ingenic.
>> 2.Adjust the code in "jz4740-cgu.c" to be compatible with the
>>   new cgu code.
>> 3.Adjust the code in "jz4725b-cgu.c" to be compatible with the
>>   new cgu code.
>> 4.Adjust the code in "jz4770-cgu.c" to be compatible with the
>>   new cgu code.
>> 5.Adjust the code in "jz4780-cgu.c" to be compatible with the
>>   new cgu code.
>> 6.Adjust the code in "x1000-cgu.c" to be compatible with the
>>   new cgu code.
>>
>> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
>> ---
>>  drivers/clk/ingenic/cgu.c         | 55 
>> +++++++++++++++++++++++++++++----------
>>  drivers/clk/ingenic/cgu.h         | 12 ++++++++-
>>  drivers/clk/ingenic/jz4725b-cgu.c |  3 ++-
>>  drivers/clk/ingenic/jz4740-cgu.c  |  3 ++-
>>  drivers/clk/ingenic/jz4770-cgu.c  |  6 +++--
>>  drivers/clk/ingenic/jz4780-cgu.c  |  3 ++-
>>  drivers/clk/ingenic/x1000-cgu.c   |  6 +++--
>>  7 files changed, 66 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index 6e96303..c3c69a8 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -84,7 +84,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned 
>> long parent_rate)
>>      pll_info = &clk_info->pll;
>>
>>      spin_lock_irqsave(&cgu->lock, flags);
>> -    ctl = readl(cgu->base + pll_info->reg);
>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>
> I really don't like this patch. There is no info on what reg[1] and 
> reg[0] are. First, don't use hardcoded numbers, use macros with 
> meaningful names. Second, why not just have two fields instead of one 
> 2-values array? That would remove a lot of the noise.
>

Because the X1830's PLL has been greatly changed, the bypass control is
placed in another register, so now two registers may needed to control the
PLL. If two fields are used, all the PLL related code of jz47xx-cgu.c / 
x1000-cgu.c
will be more verbose. Using array methods such as:
.reg = { -1, CGU_REG_CPPCR0 },
in jz4770-cgu.c,

or :
.reg = { CGU_REG_CPPCR, CGU_REG_APLL },
in x1830-cgu.c,

can let us intuitively see that how many control registers the PLL has.
Maybe adding a corresponding comment to reg[0] and reg[1] is a better way?

>
>> spin_unlock_irqrestore(&cgu->lock, flags);
>>
>>      m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
>> @@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
>> unsigned long parent_rate)
>>      n += pll_info->n_offset;
>>      od_enc = ctl >> pll_info->od_shift;
>>      od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>> -    bypass = !pll_info->no_bypass_bit &&
>> -         !!(ctl & BIT(pll_info->bypass_bit));
>> +
>> +    if (pll_info->version >= CGU_X1830) {
>> +        spin_lock_irqsave(&cgu->lock, flags);
>> +        ctl = readl(cgu->base + pll_info->reg[0]);
>> +        spin_unlock_irqrestore(&cgu->lock, flags);
>
> Why the spinlock?
>

The original code used spinlock when reading the control register,
so when reading this new control register, I think it should also
be added with spinlock.

>
>> +
>> +        bypass = !pll_info->no_bypass_bit &&
>> +             !!(ctl & BIT(pll_info->bypass_bit));
>> +    } else
>
> Please comply to the kernel coding style - use brackets after the else.
>

My fault, I will fix this in v2.

>> +        bypass = !pll_info->no_bypass_bit &&
>> +             !!(ctl & BIT(pll_info->bypass_bit));
>>
>>      if (bypass)
>>          return parent_rate;
>> @@ -106,7 +115,10 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
>> unsigned long parent_rate)
>>      BUG_ON(od == pll_info->od_max);
>>      od++;
>>
>> -    return div_u64((u64)parent_rate * m, n * od);
>> +    if (pll_info->version >= CGU_X1830)
>> +        return div_u64((u64)parent_rate * m * 2, n * od);
>
> Where does that *2 come from?

This is because the calculation method of the PLL frequency multiplication
of X1830 has changed, and a factor of 2 is added compared to the original
method (on page 381 of the X1830's PM manual).

Thanks and best regards!

>
>> +    else
>> +        return div_u64((u64)parent_rate * m, n * od);
>>  }
>>
>>  static unsigned long
>> @@ -139,7 +151,10 @@ ingenic_pll_calc(const struct 
>> ingenic_cgu_clk_info *clk_info,
>>      if (pod)
>>          *pod = od;
>>
>> -    return div_u64((u64)parent_rate * m, n * od);
>> +    if (pll_info->version >= CGU_X1830)
>> +        return div_u64((u64)parent_rate * m * 2, n * od);
>> +    else
>> +        return div_u64((u64)parent_rate * m, n * od);
>>  }
>>
>>  static inline const struct ingenic_cgu_clk_info *to_clk_info(
>> @@ -183,7 +198,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
>> long req_rate,
>>              clk_info->name, req_rate, rate);
>>
>>      spin_lock_irqsave(&cgu->lock, flags);
>> -    ctl = readl(cgu->base + pll_info->reg);
>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>
>>      ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << pll_info->m_shift);
>>      ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
>> @@ -194,7 +209,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
>> long req_rate,
>>      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);
>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>
>>      return 0;
>> @@ -212,16 +227,28 @@ static int ingenic_pll_enable(struct clk_hw *hw)
>>      u32 ctl;
>>
>>      spin_lock_irqsave(&cgu->lock, flags);
>> -    ctl = readl(cgu->base + pll_info->reg);
>>
>> -    ctl &= ~BIT(pll_info->bypass_bit);
>> +    if (pll_info->version >= CGU_X1830) {
>> +        ctl = readl(cgu->base + pll_info->reg[0]);
>> +
>> +        ctl &= ~BIT(pll_info->bypass_bit);
>> +
>> +        writel(ctl, cgu->base + pll_info->reg[0]);
>> +
>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>> +    } else {
>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>> +
>> +        ctl &= ~BIT(pll_info->bypass_bit);
>> +    }
>> +
>>      ctl |= BIT(pll_info->enable_bit);
>>
>> -    writel(ctl, cgu->base + pll_info->reg);
>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>
>>      /* wait for the PLL to stabilise */
>>      for (i = 0; i < timeout; i++) {
>> -        ctl = readl(cgu->base + pll_info->reg);
>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>>          if (ctl & BIT(pll_info->stable_bit))
>>              break;
>>          mdelay(1);
>> @@ -245,11 +272,11 @@ static void ingenic_pll_disable(struct clk_hw *hw)
>>      u32 ctl;
>>
>>      spin_lock_irqsave(&cgu->lock, flags);
>> -    ctl = readl(cgu->base + pll_info->reg);
>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>
>>      ctl &= ~BIT(pll_info->enable_bit);
>>
>> -    writel(ctl, cgu->base + pll_info->reg);
>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>  }
>>
>> @@ -263,7 +290,7 @@ static int ingenic_pll_is_enabled(struct clk_hw *hw)
>>      u32 ctl;
>>
>>      spin_lock_irqsave(&cgu->lock, flags);
>> -    ctl = readl(cgu->base + pll_info->reg);
>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>
>>      return !!(ctl & BIT(pll_info->enable_bit));
>> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
>> index 0dc8004..5f87be4 100644
>> --- a/drivers/clk/ingenic/cgu.h
>> +++ b/drivers/clk/ingenic/cgu.h
>> @@ -42,8 +42,18 @@
>>   * @stable_bit: the index of the stable bit in the PLL control register
>>   * @no_bypass_bit: if set, the PLL has no bypass functionality
>>   */
>> +enum ingenic_cgu_version {
>> +    CGU_JZ4740,
>> +    CGU_JZ4725B,
>> +    CGU_JZ4770,
>> +    CGU_JZ4780,
>> +    CGU_X1000,
>> +    CGU_X1830,
>> +};
>> +
>>  struct ingenic_cgu_pll_info {
>> -    unsigned reg;
>> +    enum ingenic_cgu_version version;
>> +    unsigned reg[2];
>>      const s8 *od_encoding;
>>      u8 m_shift, m_bits, m_offset;
>>      u8 n_shift, n_bits, n_offset;
>> diff --git a/drivers/clk/ingenic/jz4725b-cgu.c 
>> b/drivers/clk/ingenic/jz4725b-cgu.c
>> index a3b4635..6da7b41 100644
>> --- a/drivers/clk/ingenic/jz4725b-cgu.c
>> +++ b/drivers/clk/ingenic/jz4725b-cgu.c
>> @@ -53,7 +53,8 @@ static const struct ingenic_cgu_clk_info 
>> jz4725b_cgu_clocks[] = {
>>          "pll", CGU_CLK_PLL,
>>          .parents = { JZ4725B_CLK_EXT, -1, -1, -1 },
>>          .pll = {
>> -            .reg = CGU_REG_CPPCR,
>> +            .version = CGU_JZ4725B,
>> +            .reg = { -1, CGU_REG_CPPCR },
>>              .m_shift = 23,
>>              .m_bits = 9,
>>              .m_offset = 2,
>> diff --git a/drivers/clk/ingenic/jz4740-cgu.c 
>> b/drivers/clk/ingenic/jz4740-cgu.c
>> index 4f0e92c..3cf800d 100644
>> --- a/drivers/clk/ingenic/jz4740-cgu.c
>> +++ b/drivers/clk/ingenic/jz4740-cgu.c
>> @@ -68,7 +68,8 @@ static const struct ingenic_cgu_clk_info 
>> jz4740_cgu_clocks[] = {
>>          "pll", CGU_CLK_PLL,
>>          .parents = { JZ4740_CLK_EXT, -1, -1, -1 },
>>          .pll = {
>> -            .reg = CGU_REG_CPPCR,
>> +            .version = CGU_JZ4740,
>> +            .reg = { -1, CGU_REG_CPPCR },
>>              .m_shift = 23,
>>              .m_bits = 9,
>>              .m_offset = 2,
>> diff --git a/drivers/clk/ingenic/jz4770-cgu.c 
>> b/drivers/clk/ingenic/jz4770-cgu.c
>> index 956dd65..a62dfb1 100644
>> --- a/drivers/clk/ingenic/jz4770-cgu.c
>> +++ b/drivers/clk/ingenic/jz4770-cgu.c
>> @@ -101,7 +101,8 @@ static const struct ingenic_cgu_clk_info 
>> jz4770_cgu_clocks[] = {
>>          "pll0", CGU_CLK_PLL,
>>          .parents = { JZ4770_CLK_EXT },
>>          .pll = {
>> -            .reg = CGU_REG_CPPCR0,
>> +            .version = CGU_JZ4770,
>> +            .reg = { -1, CGU_REG_CPPCR0 },
>>              .m_shift = 24,
>>              .m_bits = 7,
>>              .m_offset = 1,
>> @@ -123,7 +124,8 @@ static const struct ingenic_cgu_clk_info 
>> jz4770_cgu_clocks[] = {
>>          "pll1", CGU_CLK_PLL,
>>          .parents = { JZ4770_CLK_EXT },
>>          .pll = {
>> -            .reg = CGU_REG_CPPCR1,
>> +            .version = CGU_JZ4770,
>> +            .reg = { -1, CGU_REG_CPPCR1 },
>>              .m_shift = 24,
>>              .m_bits = 7,
>>              .m_offset = 1,
>> diff --git a/drivers/clk/ingenic/jz4780-cgu.c 
>> b/drivers/clk/ingenic/jz4780-cgu.c
>> index ea905ff..59356d1b 100644
>> --- a/drivers/clk/ingenic/jz4780-cgu.c
>> +++ b/drivers/clk/ingenic/jz4780-cgu.c
>> @@ -220,7 +220,8 @@ static const struct ingenic_cgu_clk_info 
>> jz4780_cgu_clocks[] = {
>>      /* PLLs */
>>
>>  #define DEF_PLL(name) { \
>> -    .reg = CGU_REG_ ## name, \
>> +    .version = CGU_JZ4780, \
>> +    .reg = { -1, CGU_REG_ ## name }, \
>>      .m_shift = 19, \
>>      .m_bits = 13, \
>>      .m_offset = 1, \
>> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
>> b/drivers/clk/ingenic/x1000-cgu.c
>> index b22d87b..7179b9f 100644
>> --- a/drivers/clk/ingenic/x1000-cgu.c
>> +++ b/drivers/clk/ingenic/x1000-cgu.c
>> @@ -57,7 +57,8 @@ static const struct ingenic_cgu_clk_info 
>> x1000_cgu_clocks[] = {
>>          "apll", CGU_CLK_PLL,
>>          .parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>>          .pll = {
>> -            .reg = CGU_REG_APLL,
>> +            .version = CGU_X1000,
>> +            .reg = { -1, CGU_REG_APLL },
>>              .m_shift = 24,
>>              .m_bits = 7,
>>              .m_offset = 1,
>> @@ -78,7 +79,8 @@ static const struct ingenic_cgu_clk_info 
>> x1000_cgu_clocks[] = {
>>          "mpll", CGU_CLK_PLL,
>>          .parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>>          .pll = {
>> -            .reg = CGU_REG_MPLL,
>> +            .version = CGU_X1000,
>> +            .reg = { -1, CGU_REG_MPLL },
>>              .m_shift = 24,
>>              .m_bits = 7,
>>              .m_offset = 1,
>> -- 
>> 2.7.4
>>
>>
>
>




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
  2019-11-28  6:29     ` Zhou Yanjie
@ 2019-11-29 11:23       ` Paul Cercueil
  2019-12-10 22:55         ` Paul Burton
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Cercueil @ 2019-11-29 11:23 UTC (permalink / raw)
  To: Zhou Yanjie
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	sernia.zhou, zhenwenjin

Hi Zhou,


>> Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
>> \x7fécrit :
>>> 1.Adjust the PLL related code in "cgu.c" and "cgu.h" to make it
>>>   compatible with the X1830 Soc from Ingenic.
>>> 2.Adjust the code in "jz4740-cgu.c" to be compatible with the
>>>   new cgu code.
>>> 3.Adjust the code in "jz4725b-cgu.c" to be compatible with the
>>>   new cgu code.
>>> 4.Adjust the code in "jz4770-cgu.c" to be compatible with the
>>>   new cgu code.
>>> 5.Adjust the code in "jz4780-cgu.c" to be compatible with the
>>>   new cgu code.
>>> 6.Adjust the code in "x1000-cgu.c" to be compatible with the
>>>   new cgu code.
>>> 
>>> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
>>> ---
>>>  drivers/clk/ingenic/cgu.c         | 55 
>>> \x7f\x7f+++++++++++++++++++++++++++++----------
>>>  drivers/clk/ingenic/cgu.h         | 12 ++++++++-
>>>  drivers/clk/ingenic/jz4725b-cgu.c |  3 ++-
>>>  drivers/clk/ingenic/jz4740-cgu.c  |  3 ++-
>>>  drivers/clk/ingenic/jz4770-cgu.c  |  6 +++--
>>>  drivers/clk/ingenic/jz4780-cgu.c  |  3 ++-
>>>  drivers/clk/ingenic/x1000-cgu.c   |  6 +++--
>>>  7 files changed, 66 insertions(+), 22 deletions(-)
>>> 
>>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>>> index 6e96303..c3c69a8 100644
>>> --- a/drivers/clk/ingenic/cgu.c
>>> +++ b/drivers/clk/ingenic/cgu.c
>>> @@ -84,7 +84,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
>>> unsigned \x7f\x7flong parent_rate)
>>>      pll_info = &clk_info->pll;
>>> 
>>>      spin_lock_irqsave(&cgu->lock, flags);
>>> -    ctl = readl(cgu->base + pll_info->reg);
>>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>> 
>> I really don't like this patch. There is no info on what reg[1] and 
>> \x7freg[0] are. First, don't use hardcoded numbers, use macros with 
>> \x7fmeaningful names. Second, why not just have two fields instead of 
>> one \x7f2-values array? That would remove a lot of the noise.
>> 
> 
> Because the X1830's PLL has been greatly changed, the bypass control 
> is
> placed in another register, so now two registers may needed to 
> control the
> PLL.

That kind of info belongs in the commit message.


> If two fields are used, all the PLL related code of jz47xx-cgu.c / 
> x1000-cgu.c
> will be more verbose. Using array methods such as:
> .reg = { -1, CGU_REG_CPPCR0 },
> in jz4770-cgu.c,
> 
> or :
> .reg = { CGU_REG_CPPCR, CGU_REG_APLL },
> in x1830-cgu.c,
> 
> can let us intuitively see that how many control registers the PLL 
> has.
> Maybe adding a corresponding comment to reg[0] and reg[1] is a better 
> way?

I don't see how having two fields would make the code more verbose. 
 From what I can see it would make this patch much smaller...

>> 
>>> spin_unlock_irqrestore(&cgu->lock, flags);
>>> 
>>>      m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 
>>> 0);
>>> @@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
>>> \x7f\x7funsigned long parent_rate)
>>>      n += pll_info->n_offset;
>>>      od_enc = ctl >> pll_info->od_shift;
>>>      od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>>> -    bypass = !pll_info->no_bypass_bit &&
>>> -         !!(ctl & BIT(pll_info->bypass_bit));
>>> +
>>> +    if (pll_info->version >= CGU_X1830) {
>>> +        spin_lock_irqsave(&cgu->lock, flags);
>>> +        ctl = readl(cgu->base + pll_info->reg[0]);
>>> +        spin_unlock_irqrestore(&cgu->lock, flags);
>> 
>> Why the spinlock?
>> 
> 
> The original code used spinlock when reading the control register,
> so when reading this new control register, I think it should also
> be added with spinlock.

Well, the original code looks wrong to me. There's nothing to protect 
here.

Maybe @Paul Burton can shed some light?

>>> +
>>> +        bypass = !pll_info->no_bypass_bit &&
>>> +             !!(ctl & BIT(pll_info->bypass_bit));
>>> +    } else
>> 
>> Please comply to the kernel coding style - use brackets after the 
>> else.
>> 
> 
> My fault, I will fix this in v2.
> 
>>> +        bypass = !pll_info->no_bypass_bit &&
>>> +             !!(ctl & BIT(pll_info->bypass_bit));
>>> 
>>>      if (bypass)
>>>          return parent_rate;
>>> @@ -106,7 +115,10 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
>>> \x7f\x7funsigned long parent_rate)
>>>      BUG_ON(od == pll_info->od_max);
>>>      od++;
>>> 
>>> -    return div_u64((u64)parent_rate * m, n * od);
>>> +    if (pll_info->version >= CGU_X1830)
>>> +        return div_u64((u64)parent_rate * m * 2, n * od);
>> 
>> Where does that *2 come from?
> 
> This is because the calculation method of the PLL frequency 
> multiplication
> of X1830 has changed, and a factor of 2 is added compared to the 
> original
> method (on page 381 of the X1830's PM manual).

Instead of adding a pll_info->version, add a pll_info->rate_multiplier, 
set it to 1 everywhere and 2 on the X1830.

Cheers,
-Paul

>> 
>>> +    else
>>> +        return div_u64((u64)parent_rate * m, n * od);
>>>  }
>>> 
>>>  static unsigned long
>>> @@ -139,7 +151,10 @@ ingenic_pll_calc(const struct 
>>> \x7f\x7fingenic_cgu_clk_info *clk_info,
>>>      if (pod)
>>>          *pod = od;
>>> 
>>> -    return div_u64((u64)parent_rate * m, n * od);
>>> +    if (pll_info->version >= CGU_X1830)
>>> +        return div_u64((u64)parent_rate * m * 2, n * od);
>>> +    else
>>> +        return div_u64((u64)parent_rate * m, n * od);
>>>  }
>>> 
>>>  static inline const struct ingenic_cgu_clk_info *to_clk_info(
>>> @@ -183,7 +198,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, 
>>> unsigned \x7f\x7flong req_rate,
>>>              clk_info->name, req_rate, rate);
>>> 
>>>      spin_lock_irqsave(&cgu->lock, flags);
>>> -    ctl = readl(cgu->base + pll_info->reg);
>>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>> 
>>>      ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << 
>>> pll_info->m_shift);
>>>      ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
>>> @@ -194,7 +209,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, 
>>> unsigned \x7f\x7flong req_rate,
>>>      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);
>>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>> 
>>>      return 0;
>>> @@ -212,16 +227,28 @@ static int ingenic_pll_enable(struct clk_hw 
>>> *hw)
>>>      u32 ctl;
>>> 
>>>      spin_lock_irqsave(&cgu->lock, flags);
>>> -    ctl = readl(cgu->base + pll_info->reg);
>>> 
>>> -    ctl &= ~BIT(pll_info->bypass_bit);
>>> +    if (pll_info->version >= CGU_X1830) {
>>> +        ctl = readl(cgu->base + pll_info->reg[0]);
>>> +
>>> +        ctl &= ~BIT(pll_info->bypass_bit);
>>> +
>>> +        writel(ctl, cgu->base + pll_info->reg[0]);
>>> +
>>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>>> +    } else {
>>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>>> +
>>> +        ctl &= ~BIT(pll_info->bypass_bit);
>>> +    }
>>> +
>>>      ctl |= BIT(pll_info->enable_bit);
>>> 
>>> -    writel(ctl, cgu->base + pll_info->reg);
>>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>> 
>>>      /* wait for the PLL to stabilise */
>>>      for (i = 0; i < timeout; i++) {
>>> -        ctl = readl(cgu->base + pll_info->reg);
>>> +        ctl = readl(cgu->base + pll_info->reg[1]);
>>>          if (ctl & BIT(pll_info->stable_bit))
>>>              break;
>>>          mdelay(1);
>>> @@ -245,11 +272,11 @@ static void ingenic_pll_disable(struct clk_hw 
>>> *hw)
>>>      u32 ctl;
>>> 
>>>      spin_lock_irqsave(&cgu->lock, flags);
>>> -    ctl = readl(cgu->base + pll_info->reg);
>>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>> 
>>>      ctl &= ~BIT(pll_info->enable_bit);
>>> 
>>> -    writel(ctl, cgu->base + pll_info->reg);
>>> +    writel(ctl, cgu->base + pll_info->reg[1]);
>>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>>  }
>>> 
>>> @@ -263,7 +290,7 @@ static int ingenic_pll_is_enabled(struct clk_hw 
>>> *hw)
>>>      u32 ctl;
>>> 
>>>      spin_lock_irqsave(&cgu->lock, flags);
>>> -    ctl = readl(cgu->base + pll_info->reg);
>>> +    ctl = readl(cgu->base + pll_info->reg[1]);
>>>      spin_unlock_irqrestore(&cgu->lock, flags);
>>> 
>>>      return !!(ctl & BIT(pll_info->enable_bit));
>>> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
>>> index 0dc8004..5f87be4 100644
>>> --- a/drivers/clk/ingenic/cgu.h
>>> +++ b/drivers/clk/ingenic/cgu.h
>>> @@ -42,8 +42,18 @@
>>>   * @stable_bit: the index of the stable bit in the PLL control 
>>> register
>>>   * @no_bypass_bit: if set, the PLL has no bypass functionality
>>>   */
>>> +enum ingenic_cgu_version {
>>> +    CGU_JZ4740,
>>> +    CGU_JZ4725B,
>>> +    CGU_JZ4770,
>>> +    CGU_JZ4780,
>>> +    CGU_X1000,
>>> +    CGU_X1830,
>>> +};
>>> +
>>>  struct ingenic_cgu_pll_info {
>>> -    unsigned reg;
>>> +    enum ingenic_cgu_version version;
>>> +    unsigned reg[2];
>>>      const s8 *od_encoding;
>>>      u8 m_shift, m_bits, m_offset;
>>>      u8 n_shift, n_bits, n_offset;
>>> diff --git a/drivers/clk/ingenic/jz4725b-cgu.c 
>>> \x7f\x7fb/drivers/clk/ingenic/jz4725b-cgu.c
>>> index a3b4635..6da7b41 100644
>>> --- a/drivers/clk/ingenic/jz4725b-cgu.c
>>> +++ b/drivers/clk/ingenic/jz4725b-cgu.c
>>> @@ -53,7 +53,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fjz4725b_cgu_clocks[] = {
>>>          "pll", CGU_CLK_PLL,
>>>          .parents = { JZ4725B_CLK_EXT, -1, -1, -1 },
>>>          .pll = {
>>> -            .reg = CGU_REG_CPPCR,
>>> +            .version = CGU_JZ4725B,
>>> +            .reg = { -1, CGU_REG_CPPCR },
>>>              .m_shift = 23,
>>>              .m_bits = 9,
>>>              .m_offset = 2,
>>> diff --git a/drivers/clk/ingenic/jz4740-cgu.c 
>>> \x7f\x7fb/drivers/clk/ingenic/jz4740-cgu.c
>>> index 4f0e92c..3cf800d 100644
>>> --- a/drivers/clk/ingenic/jz4740-cgu.c
>>> +++ b/drivers/clk/ingenic/jz4740-cgu.c
>>> @@ -68,7 +68,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fjz4740_cgu_clocks[] = {
>>>          "pll", CGU_CLK_PLL,
>>>          .parents = { JZ4740_CLK_EXT, -1, -1, -1 },
>>>          .pll = {
>>> -            .reg = CGU_REG_CPPCR,
>>> +            .version = CGU_JZ4740,
>>> +            .reg = { -1, CGU_REG_CPPCR },
>>>              .m_shift = 23,
>>>              .m_bits = 9,
>>>              .m_offset = 2,
>>> diff --git a/drivers/clk/ingenic/jz4770-cgu.c 
>>> \x7f\x7fb/drivers/clk/ingenic/jz4770-cgu.c
>>> index 956dd65..a62dfb1 100644
>>> --- a/drivers/clk/ingenic/jz4770-cgu.c
>>> +++ b/drivers/clk/ingenic/jz4770-cgu.c
>>> @@ -101,7 +101,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fjz4770_cgu_clocks[] = {
>>>          "pll0", CGU_CLK_PLL,
>>>          .parents = { JZ4770_CLK_EXT },
>>>          .pll = {
>>> -            .reg = CGU_REG_CPPCR0,
>>> +            .version = CGU_JZ4770,
>>> +            .reg = { -1, CGU_REG_CPPCR0 },
>>>              .m_shift = 24,
>>>              .m_bits = 7,
>>>              .m_offset = 1,
>>> @@ -123,7 +124,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fjz4770_cgu_clocks[] = {
>>>          "pll1", CGU_CLK_PLL,
>>>          .parents = { JZ4770_CLK_EXT },
>>>          .pll = {
>>> -            .reg = CGU_REG_CPPCR1,
>>> +            .version = CGU_JZ4770,
>>> +            .reg = { -1, CGU_REG_CPPCR1 },
>>>              .m_shift = 24,
>>>              .m_bits = 7,
>>>              .m_offset = 1,
>>> diff --git a/drivers/clk/ingenic/jz4780-cgu.c 
>>> \x7f\x7fb/drivers/clk/ingenic/jz4780-cgu.c
>>> index ea905ff..59356d1b 100644
>>> --- a/drivers/clk/ingenic/jz4780-cgu.c
>>> +++ b/drivers/clk/ingenic/jz4780-cgu.c
>>> @@ -220,7 +220,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fjz4780_cgu_clocks[] = {
>>>      /* PLLs */
>>> 
>>>  #define DEF_PLL(name) { \
>>> -    .reg = CGU_REG_ ## name, \
>>> +    .version = CGU_JZ4780, \
>>> +    .reg = { -1, CGU_REG_ ## name }, \
>>>      .m_shift = 19, \
>>>      .m_bits = 13, \
>>>      .m_offset = 1, \
>>> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
>>> \x7f\x7fb/drivers/clk/ingenic/x1000-cgu.c
>>> index b22d87b..7179b9f 100644
>>> --- a/drivers/clk/ingenic/x1000-cgu.c
>>> +++ b/drivers/clk/ingenic/x1000-cgu.c
>>> @@ -57,7 +57,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fx1000_cgu_clocks[] = {
>>>          "apll", CGU_CLK_PLL,
>>>          .parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>>>          .pll = {
>>> -            .reg = CGU_REG_APLL,
>>> +            .version = CGU_X1000,
>>> +            .reg = { -1, CGU_REG_APLL },
>>>              .m_shift = 24,
>>>              .m_bits = 7,
>>>              .m_offset = 1,
>>> @@ -78,7 +79,8 @@ static const struct ingenic_cgu_clk_info 
>>> \x7f\x7fx1000_cgu_clocks[] = {
>>>          "mpll", CGU_CLK_PLL,
>>>          .parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>>>          .pll = {
>>> -            .reg = CGU_REG_MPLL,
>>> +            .version = CGU_X1000,
>>> +            .reg = { -1, CGU_REG_MPLL },
>>>              .m_shift = 24,
>>>              .m_bits = 7,
>>>              .m_offset = 1,
>>> --
>>> 2.7.4
>>> 
>>> 
>> 
>> 
> 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] dt-bindings: clock: Add X1830 bindings.
  2019-11-27  3:32 ` [PATCH 2/5] dt-bindings: clock: Add X1830 bindings Zhou Yanjie
@ 2019-12-05 20:38   ` Rob Herring
  0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2019-12-05 20:38 UTC (permalink / raw)
  To: Zhou Yanjie
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, paulburton, mturquette, sboyd, mark.rutland, syq,
	paul, sernia.zhou, zhenwenjin

On Wed, 27 Nov 2019 11:32:53 +0800, Zhou Yanjie wrote:
> Add the clock bindings for the X1830 Soc from Ingenic.
> 
> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
> ---
>  .../devicetree/bindings/clock/ingenic,cgu.txt      |  1 +
>  include/dt-bindings/clock/x1830-cgu.h              | 46 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 include/dt-bindings/clock/x1830-cgu.h
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
  2019-11-29 11:23       ` Paul Cercueil
@ 2019-12-10 22:55         ` Paul Burton
  0 siblings, 0 replies; 13+ messages in thread
From: Paul Burton @ 2019-12-10 22:55 UTC (permalink / raw)
  To: Paul Cercueil, Zhou Yanjie
  Cc: linux-mips, linux-kernel, linux-clk, devicetree, robh+dt,
	paul.burton, mturquette, sboyd, mark.rutland, syq, sernia.zhou,
	zhenwenjin

Hi Paul, Zhou,

On Fri, Nov 29, 2019 at 12:23:42PM +0100, Paul Cercueil wrote:
> > > > @@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw,
> > > > \x7f\x7funsigned long parent_rate)
> > > >      n += pll_info->n_offset;
> > > >      od_enc = ctl >> pll_info->od_shift;
> > > >      od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> > > > -    bypass = !pll_info->no_bypass_bit &&
> > > > -         !!(ctl & BIT(pll_info->bypass_bit));
> > > > +
> > > > +    if (pll_info->version >= CGU_X1830) {
> > > > +        spin_lock_irqsave(&cgu->lock, flags);
> > > > +        ctl = readl(cgu->base + pll_info->reg[0]);
> > > > +        spin_unlock_irqrestore(&cgu->lock, flags);
> > > 
> > > Why the spinlock?
> > > 
> > 
> > The original code used spinlock when reading the control register,
> > so when reading this new control register, I think it should also
> > be added with spinlock.
> 
> Well, the original code looks wrong to me. There's nothing to protect here.
> 
> Maybe @Paul Burton can shed some light?

I wish I could remember, but I agree it seems pointless here. The only
way I can think it could be of any use is if writes to the CGU register
we're accessing aren't atomic (ie. if we could observe a partially
completed write), but I don't believe that's the case.

So Zhou, if you want to drop the spinlock here from your X1830 path &
ideally also add a patch to remove it in the non-X1830 path that would
be great.

Thanks,
    Paul

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-12-10 22:54 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
2019-11-27 17:37   ` Paul Cercueil
2019-11-28  6:29     ` Zhou Yanjie
2019-11-29 11:23       ` Paul Cercueil
2019-12-10 22:55         ` Paul Burton
2019-11-27  3:32 ` [PATCH 2/5] dt-bindings: clock: Add X1830 bindings Zhou Yanjie
2019-12-05 20:38   ` Rob Herring
2019-11-27  3:32 ` [PATCH 3/5] clk: Ingenic: Add CGU driver for X1830 Zhou Yanjie
2019-11-27  3:32 ` [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000 Zhou Yanjie
2019-11-27 17:19   ` Paul Cercueil
2019-11-28  5:44     ` Zhou Yanjie
2019-11-27  3:32 ` [PATCH 5/5] clk: Ingenic: " Zhou Yanjie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).