clk: spacemit: add K1 reset support

There are additional SpacemiT syscon CCUs whose registers control both clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined previously, these will initially support only resets. They do not incorporate power domain functionality.

Define the index values for resets associated with all SpacemiT K1 syscon nodes, including those with clocks already defined, as well as the new ones (without clocks).

Signed-off-by Alex Elder <[email protected]>

---
 .../soc/spacemit/spacemit,k1-syscon.yaml      |  13 +-
 include/dt-bindings/clock/spacemit,k1-ccu.h   | 134 ++++++++++++++++++
 2 files changed, 143 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +9 -4

index 07a6728e6f864..333c28e075b6c 100644
@@ -19,6 +19,9 @@ properties:
       - spacemit,k1-syscon-apbc
       - spacemit,k1-syscon-apmu
       - spacemit,k1-syscon-mpmu
+      - spacemit,k1-syscon-rcpu
+      - spacemit,k1-syscon-rcpu2
+      - spacemit,k1-syscon-apbc2
 
   reg:
     maxItems: 1

@@ -57,13 +60,15 @@ allOf:
       properties:
         compatible:
           contains:
-            const: spacemit,k1-syscon-apbc
+            enum:
+              - spacemit,k1-syscon-apmu
+              - spacemit,k1-syscon-mpmu
     then:
-      properties:
-        "#power-domain-cells": false
-    else:
       required:
         - "#power-domain-cells"
+    else:
+      properties:
+        "#power-domain-cells": false
 
 additionalProperties: false
 

include/dt-bindings/clock/spacemit,k1-ccu.h +3 -0

index 4a0c7163257e3..a1e1b1fe714ce 100644
@@ -78,6 +78,9 @@
 #define CLK_APB			31
 #define CLK_WDT_BUS		32
 
+/*	MPMU resets	*/
+#define RST_WDT			0
+
 /*	APBC clocks	*/
 #define CLK_UART0		0
 #define CLK_UART2		1

There are additional SpacemiT syscon CCUs whose registers control both clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined previously, these will initially support only resets. They do not incorporate power domain functionality.

Define the index values for resets associated with all SpacemiT K1 syscon nodes, including those with clocks already defined, as well as the new ones (without clocks).

Signed-off-by Alex Elder <[email protected]>

---
 .../soc/spacemit/spacemit,k1-syscon.yaml      |  13 +-
 include/dt-bindings/clock/spacemit,k1-ccu.h   | 134 ++++++++++++++++++
 2 files changed, 143 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +9 -4

index 07a6728e6f864..333c28e075b6c 100644
@@ -19,6 +19,9 @@ properties:
       - spacemit,k1-syscon-apbc
       - spacemit,k1-syscon-apmu
       - spacemit,k1-syscon-mpmu
+      - spacemit,k1-syscon-rcpu
+      - spacemit,k1-syscon-rcpu2
+      - spacemit,k1-syscon-apbc2
 
   reg:
     maxItems: 1

@@ -57,13 +60,15 @@ allOf:
       properties:
         compatible:
           contains:
-            const: spacemit,k1-syscon-apbc
+            enum:
+              - spacemit,k1-syscon-apmu
+              - spacemit,k1-syscon-mpmu
     then:
-      properties:
-        "#power-domain-cells": false
-    else:
       required:
         - "#power-domain-cells"
+    else:
+      properties:
+        "#power-domain-cells": false
 
 additionalProperties: false
 

include/dt-bindings/clock/spacemit,k1-ccu.h +3 -0

index 4a0c7163257e3..a1e1b1fe714ce 100644
@@ -78,6 +78,9 @@
 #define CLK_APB			31
 #define CLK_WDT_BUS		32
 
+/*	MPMU resets	*/
+#define RST_WDT			0
+
 /*	APBC clocks	*/
 #define CLK_UART0		0
 #define CLK_UART2		1

include/dt-bindings/clock/spacemit,k1-ccu.h +128 -0

+#define RST_UART0		0
+#define RST_UART2		1
+#define RST_UART3		2
+#define RST_UART4		3
+#define RST_UART5		4
+#define RST_UART6		5
+#define RST_UART7		6
+#define RST_UART8		7
+#define RST_UART9		8
+#define RST_GPIO		9
+#define RST_PWM0		10
+#define RST_PWM1		11
+#define RST_PWM2		12
+#define RST_PWM3		13
+#define RST_PWM4		14
+#define RST_PWM5		15
+#define RST_PWM6		16
+#define RST_PWM7		17
+#define RST_PWM8		18
+#define RST_PWM9		19
+#define RST_PWM10		20
+#define RST_PWM11		21
+#define RST_PWM12		22
+#define RST_PWM13		23
+#define RST_PWM14		24
+#define RST_PWM15		25
+#define RST_PWM16		26
+#define RST_PWM17		27
+#define RST_PWM18		28
+#define RST_PWM19		29
+#define RST_SSP3		30
+#define RST_RTC			31
+#define RST_TWSI0		32
+#define RST_TWSI1		33
+#define RST_TWSI2		34
+#define RST_TWSI4		35
+#define RST_TWSI5		36
+#define RST_TWSI6		37
+#define RST_TWSI7		38
+#define RST_TWSI8		39
+#define RST_TIMERS1		40
+#define RST_TIMERS2		41
+#define RST_AIB			42
+#define RST_ONEWIRE		43
+#define RST_SSPA0		44
+#define RST_SSPA1		45
+#define RST_DRO			46
+#define RST_IR			47
+#define RST_TSEN		48
+#define RST_IPC_AP2AUD		49
+#define RST_CAN0		50
+
 /*	APMU clocks	*/
 #define CLK_CCI550		0
 #define CLK_CPU_C0_HI		1

@@ -244,4 +302,80 @@
 #define CLK_V2D			60
 #define CLK_EMMC_BUS		61
 
+/*	APMU resets	*/
+
+#define RST_CCIC_4X		0
+#define RST_CCIC1_PHY		1
+#define RST_SDH_AXI		2
+#define RST_SDH0		3
+#define RST_SDH1		4
+#define RST_SDH2		5
+#define RST_USBP1_AXI		6
+#define RST_USB_AXI		7
+#define RST_USB3_0		8
+#define RST_QSPI		9
+#define RST_QSPI_BUS		10
+#define RST_DMA			11
+#define RST_AES			12
+#define RST_VPU			13
+#define RST_GPU			14
+#define RST_EMMC		15
+#define RST_EMMC_X		16
+#define RST_AUDIO		17
+#define RST_HDMI		18
+#define RST_PCIE0		19
+#define RST_PCIE1		20
+#define RST_PCIE2		21
+#define RST_EMAC0		22
+#define RST_EMAC1		23
+#define RST_JPG			24
+#define RST_CCIC2PHY		25
+#define RST_CCIC3PHY		26
+#define RST_CSI			27
+#define RST_ISP_CPP		28
+#define RST_ISP_BUS		29
+#define RST_ISP			30
+#define RST_ISP_CI		31
+#define RST_DPU_MCLK		32
+#define RST_DPU_ESC		33
+#define RST_DPU_HCLK		34
+#define RST_DPU_SPIBUS		35
+#define RST_DPU_SPI_HBUS	36
+#define RST_V2D			37
+#define RST_MIPI		38
+#define RST_MC			39
+
+/*	RCPU resets	*/
+
+#define RST_RCPU_SSP0		0
+#define RST_RCPU_I2C0		1
+#define RST_RCPU_UART1		2
+#define RST_RCPU_IR		3
+#define RST_RCPU_CAN		4
+#define RST_RCPU_UART0		5
+#define RST_RCPU_HDMI_AUDIO	6
+
+/*	RCPU2 resets	*/
+
+#define RST_RCPU2_PWM0		0
+#define RST_RCPU2_PWM1		1
+#define RST_RCPU2_PWM2		2
+#define RST_RCPU2_PWM3		3
+#define RST_RCPU2_PWM4		4
+#define RST_RCPU2_PWM5		5
+#define RST_RCPU2_PWM6		6
+#define RST_RCPU2_PWM7		7
+#define RST_RCPU2_PWM8		8
+#define RST_RCPU2_PWM9		9
+
+/*	APBC2 resets	*/
+
+#define RST_APBC2_UART1		0
+#define RST_APBC2_SSP2		1
+#define RST_APBC2_TWSI3		2
+#define RST_APBC2_RTC		3
+#define RST_APBC2_TIMERS0	4
+#define RST_APBC2_KPC		5
+#define RST_APBC2_GPIO		6
+
 #endif /* _DT_BINDINGS_SPACEMIT_CCU_H_ */

Signature · 1 line

2.43.0

hi Alex:

Quoted 27 lines · Alex Elder

There are additional SpacemiT syscon CCUs whose registers control both clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined previously, these will initially support only resets. They do not incorporate power domain functionality.

Define the index values for resets associated with all SpacemiT K1 syscon nodes, including those with clocks already defined, as well as the new ones (without clocks).

Signed-off-by Alex Elder <[email protected]>

---
  .../soc/spacemit/spacemit,k1-syscon.yaml      |  13 +-
  include/dt-bindings/clock/spacemit,k1-ccu.h   | 134 ++++++++++++++++++
  2 files changed, 143 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +3 -0

index 07a6728e6f864..333c28e075b6c 100644
@@ -19,6 +19,9 @@ properties:
        - spacemit,k1-syscon-apbc
        - spacemit,k1-syscon-apmu
        - spacemit,k1-syscon-mpmu
+      - spacemit,k1-syscon-rcpu
+      - spacemit,k1-syscon-rcpu2
+      - spacemit,k1-syscon-apbc2
 

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml properties: · line 27

Quoted 2 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +0 -0

    reg:
      maxItems: 1

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml line 188

Quoted 6 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +2 -0

@@ -180,6 +184,60 @@
  #define CLK_TSEN_BUS		98
  #define CLK_IPC_AP2AUD_BUS	99
 
+/*	APBC resets	*/
+

I'd also suggest to drop above blank line, keep style consistent with others in this file, some same below that I won't comment

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml line 193

Quoted 5 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +5 -0

+#define RST_UART0		0
+#define RST_UART2		1
+#define RST_UART3		2
+#define RST_UART4		3
+#define RST_UART5		4

Quoted 30 lines · Yixun Lan

hi Alex:

Quoted 27 lines · Alex Elder

There are additional SpacemiT syscon CCUs whose registers control both clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined previously, these will initially support only resets. They do not incorporate power domain functionality.

Define the index values for resets associated with all SpacemiT K1 syscon nodes, including those with clocks already defined, as well as the new ones (without clocks).

Signed-off-by Alex Elder <[email protected]>

---
  .../soc/spacemit/spacemit,k1-syscon.yaml      |  13 +-
  include/dt-bindings/clock/spacemit,k1-ccu.h   | 134 ++++++++++++++++++
  2 files changed, 143 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +3 -0

index 07a6728e6f864..333c28e075b6c 100644
@@ -19,6 +19,9 @@ properties:
        - spacemit,k1-syscon-apbc
        - spacemit,k1-syscon-apmu
        - spacemit,k1-syscon-mpmu
+      - spacemit,k1-syscon-rcpu
+      - spacemit,k1-syscon-rcpu2
+      - spacemit,k1-syscon-apbc2
 

Quoted 2 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml properties: · line 27

Quoted 2 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +0 -0

    reg:
      maxItems: 1

. . .

32

Quoted 8 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml line 188

Quoted 6 lines

Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml +2 -0

@@ -180,6 +184,60 @@
  #define CLK_TSEN_BUS		98
  #define CLK_IPC_AP2AUD_BUS	99
 
+/*	APBC resets	*/
+

I'd also suggest to drop above blank line, keep style consistent with others in this file, some same below that I won't comment

OK, I'll fix the weird extra line and will drop these blank lines as you suggest in v2. I'll post another version after Sunday. I recognize the merge window means I can't expect reviews during that time, but this code is waiting for the clock code to get accepted anyway.

Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead.

Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it.

Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/clk/spacemit/ccu-k1.c +28 -8

index 44db48ae71313..f7367271396a0 100644
@@ -129,6 +129,15 @@
 #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
 #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
 
+struct spacemit_ccu_clk {
+	int id;
+	struct clk_hw *hw;
+};
+
+struct k1_ccu_data {
+	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+};
+
 /*	APBS clocks start	*/
 
 /* Frequency of pll{1,2} should not be updated at runtime */

@@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk),
 		       0);
 /*	APMU clocks end		*/
 
-struct spacemit_ccu_clk {
-	int id;
-	struct clk_hw *hw;
-};
-
 static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
 	{ CLK_PLL1,		&pll1.common.hw },
 	{ CLK_PLL2,		&pll2.common.hw },

@@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
 	{ 0,			NULL },
 };
 
+static const struct k1_ccu_data k1_ccu_apbs_data = {
+	.clk		= k1_ccu_apbs_clks,
+};
+
 static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
 	{ CLK_PLL1_307P2,	&pll1_d8_307p2.common.hw },
 	{ CLK_PLL1_76P8,	&pll1_d32_76p8.common.hw },

@@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
 	{ 0,			NULL },
 };
 
+static const struct k1_ccu_data k1_ccu_mpmu_data = {
+	.clk		= k1_ccu_mpmu_clks,
+};
+
 static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
 	{ CLK_UART0,		&uart0_clk.common.hw },
 	{ CLK_UART2,		&uart2_clk.common.hw },

@@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
 	{ 0,			NULL },
 };
 
+static const struct k1_ccu_data k1_ccu_apbc_data = {
+	.clk		= k1_ccu_apbc_clks,
+};
+
 static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
 	{ CLK_CCI550,		&cci550_clk.common.hw },
 	{ CLK_CPU_C0_HI,	&cpu_c0_hi_clk.common.hw },

@@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
 	{ 0,			NULL },
 };
 
-static int spacemit_ccu_register(struct device *dev,
-				 struct regmap *regmap, struct regmap *lock_regmap,
-				 const struct spacemit_ccu_clk *clks)
+static const struct k1_ccu_data k1_ccu_apmu_data = {
+	.clk		= k1_ccu_apmu_clks,
+};
+
+static int k1_ccu_register(struct device *dev, struct regmap *regmap,
+			   struct regmap *lock_regmap,
+			   struct spacemit_ccu_clk *clks)
 {
 	const struct spacemit_ccu_clk *clk;
 	int i, ret, max_id = 0;

drivers/clk/spacemit/ccu-k1.c +8 -4

 }
 
 static int k1_ccu_probe(struct platform_device *pdev)
 {
 	struct regmap *base_regmap, *lock_regmap = NULL;
 	struct device *dev = &pdev->dev;
+	const struct k1_ccu_data *data;
 	int ret;
 
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
 	base_regmap = device_node_to_regmap(dev->of_node);
 	if (IS_ERR(base_regmap))
 		return dev_err_probe(dev, PTR_ERR(base_regmap),

@@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
 					     "failed to get lock regmap\n");
 	}
 
-	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
-				    of_device_get_match_data(dev));
+	ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to register clocks\n");
 

@@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev)
 static const struct of_device_id of_k1_ccu_match[] = {
 	{
 		.compatible	= "spacemit,k1-pll",
-		.data		= k1_ccu_apbs_clks,
+		.data		= &k1_ccu_apbs_data,
 	},
 	{
 		.compatible	= "spacemit,k1-syscon-mpmu",
-		.data		= k1_ccu_mpmu_clks,
+		.data		= &k1_ccu_mpmu_data,
 	},
 	{

drivers/clk/spacemit/ccu-k1.c +0 -0

 };

Signature · 1 line

2.43.0

Hi Alex:

this patch change relate to clock only, so how about let's fold it into clk patches (which now has not been merged), so we make the code right at first place? cause some moving around and renaming

Quoted 8 lines

Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead.

Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it.

Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

any good reason to change this? it make the code style inconsistent, do you just change it for shorten function, or want it to be more k1 specific, so next SoC - e.g maybe k2? will introduce another function?

Quoted 89 lines

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
  1 file changed, 43 insertions(+), 15 deletions(-)

drivers/clk/spacemit/ccu-k1.c +28 -8

index 44db48ae71313..f7367271396a0 100644
@@ -129,6 +129,15 @@
  #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
  #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
 
+struct spacemit_ccu_clk {
+	int id;
+	struct clk_hw *hw;
+};
+
+struct k1_ccu_data {
+	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+};
+
  /*	APBS clocks start	*/
 
  /* Frequency of pll{1,2} should not be updated at runtime */

@@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk),
  		       0);
  /*	APMU clocks end		*/
 
-struct spacemit_ccu_clk {
-	int id;
-	struct clk_hw *hw;
-};
-
  static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
  	{ CLK_PLL1,		&pll1.common.hw },
  	{ CLK_PLL2,		&pll2.common.hw },

@@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_apbs_data = {
+	.clk		= k1_ccu_apbs_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
  	{ CLK_PLL1_307P2,	&pll1_d8_307p2.common.hw },
  	{ CLK_PLL1_76P8,	&pll1_d32_76p8.common.hw },

@@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_mpmu_data = {
+	.clk		= k1_ccu_mpmu_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
  	{ CLK_UART0,		&uart0_clk.common.hw },
  	{ CLK_UART2,		&uart2_clk.common.hw },

@@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_apbc_data = {
+	.clk		= k1_ccu_apbc_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
  	{ CLK_CCI550,		&cci550_clk.common.hw },
  	{ CLK_CPU_C0_HI,	&cpu_c0_hi_clk.common.hw },

@@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
  	{ 0,			NULL },
  };
 
-static int spacemit_ccu_register(struct device *dev,
-				 struct regmap *regmap, struct regmap *lock_regmap,
-				 const struct spacemit_ccu_clk *clks)
+static const struct k1_ccu_data k1_ccu_apmu_data = {
+	.clk		= k1_ccu_apmu_clks,
+};
+
+static int k1_ccu_register(struct device *dev, struct regmap *regmap,
+			   struct regmap *lock_regmap,
+			   struct spacemit_ccu_clk *clks)
  {
  	const struct spacemit_ccu_clk *clk;
  	int i, ret, max_id = 0;

drivers/clk/spacemit/ccu-k1.c static int spacemit_ccu_register(struct device *dev, · line 1675

Quoted 9 lines

drivers/clk/spacemit/ccu-k1.c +5 -1

@@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev,
 
  	clk_data->num = max_id + 1;
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	if (ret)
+		dev_err(dev, "error %d adding clock hardware provider\n", ret);
+
+	return ret;

I'd use "return 0;", nothing different, just explicitly short

ok, I can understand this change ease debug procedure once there is problem. (but I'm fine with either way, failure should rarely happen & will identify early)

Quoted 39 lines

drivers/clk/spacemit/ccu-k1.c +8 -4

  }
 
  static int k1_ccu_probe(struct platform_device *pdev)
  {
  	struct regmap *base_regmap, *lock_regmap = NULL;
  	struct device *dev = &pdev->dev;
+	const struct k1_ccu_data *data;
  	int ret;
 
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
  	base_regmap = device_node_to_regmap(dev->of_node);
  	if (IS_ERR(base_regmap))
  		return dev_err_probe(dev, PTR_ERR(base_regmap),

@@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
  					     "failed to get lock regmap\n");
  	}
 
-	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
-				    of_device_get_match_data(dev));
+	ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
  	if (ret)
  		return dev_err_probe(dev, ret, "failed to register clocks\n");
 

@@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev)
  static const struct of_device_id of_k1_ccu_match[] = {
  	{
  		.compatible	= "spacemit,k1-pll",
-		.data		= k1_ccu_apbs_clks,
+		.data		= &k1_ccu_apbs_data,
  	},
  	{
  		.compatible	= "spacemit,k1-syscon-mpmu",
-		.data		= k1_ccu_mpmu_clks,
+		.data		= &k1_ccu_mpmu_data,
  	},
  	{

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_probe(struct platform_device *pdev) · line 1733

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +2 -2

  		.compatible	= "spacemit,k1-syscon-apbc",
-		.data		= k1_ccu_apbc_clks,
+		.data		= &k1_ccu_apbc_data,
  	},
  	{
  		.compatible	= "spacemit,k1-syscon-apmu",
-		.data		= k1_ccu_apmu_clks,
+		.data		= &k1_ccu_apmu_data,
  	},
  	{ }

	{ /* sentinel */ }

Quoted 4 lines

drivers/clk/spacemit/ccu-k1.c +0 -0

  };

Signature · 1 line

2.43.0

Quoted 5 lines · Yixun Lan

Hi Alex:

this patch change relate to clock only, so how about let's fold it into clk patches (which now has not been merged), so we make the code right at first place? cause some moving around and renaming

No I don't want to do that.

The clock patches are Haylen's and the are getting closer to acceptance. Let's not confuse things by adding a bunch of new functionality. Get those patches in, and mine can follow not too long after that.

Quoted 9 lines · Alex Elder

Quoted 5 lines · Yixun Lan

Hi Alex:

this patch change relate to clock only, so how about let's fold it into clk patches (which now has not been merged), so we make the code right at first place? cause some moving around and renaming

No I don't want to do that.

Quoted 5 lines

The clock patches are Haylen's and the are getting closer to acceptance. Let's not confuse things by adding a bunch of new functionality. Get those patches in, and mine can follow not too long after that.

I only mean patch [2/7], not all patches, as it's still clock related but, either way fine by me if you insist

Hi Alex:

this patch change relate to clock only, so how about let's fold it into clk patches (which now has not been merged), so we make the code right at first place? cause some moving around and renaming

Quoted 4 lines

Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead.

Quoted 4 lines

Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it.

Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

any good reason to change this? it make the code style inconsistent, do you just change it for shorten function, or want it to be more k1 specific, so next SoC - e.g maybe k2? will introduce another function?

Quoted 92 lines

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
  1 file changed, 43 insertions(+), 15 deletions(-)

drivers/clk/spacemit/ccu-k1.c +28 -8

index 44db48ae71313..f7367271396a0 100644
@@ -129,6 +129,15 @@
  #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
  #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
 
+struct spacemit_ccu_clk {
+	int id;
+	struct clk_hw *hw;
+};
+
+struct k1_ccu_data {
+	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+};
+
  /*	APBS clocks start	*/
 
  /* Frequency of pll{1,2} should not be updated at runtime */

@@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk),
  		       0);
  /*	APMU clocks end		*/
 
-struct spacemit_ccu_clk {
-	int id;
-	struct clk_hw *hw;
-};
-
  static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
  	{ CLK_PLL1,		&pll1.common.hw },
  	{ CLK_PLL2,		&pll2.common.hw },

@@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_apbs_data = {
+	.clk		= k1_ccu_apbs_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
  	{ CLK_PLL1_307P2,	&pll1_d8_307p2.common.hw },
  	{ CLK_PLL1_76P8,	&pll1_d32_76p8.common.hw },

@@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_mpmu_data = {
+	.clk		= k1_ccu_mpmu_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
  	{ CLK_UART0,		&uart0_clk.common.hw },
  	{ CLK_UART2,		&uart2_clk.common.hw },

@@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
  	{ 0,			NULL },
  };
 
+static const struct k1_ccu_data k1_ccu_apbc_data = {
+	.clk		= k1_ccu_apbc_clks,
+};
+
  static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
  	{ CLK_CCI550,		&cci550_clk.common.hw },
  	{ CLK_CPU_C0_HI,	&cpu_c0_hi_clk.common.hw },

@@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
  	{ 0,			NULL },
  };
 
-static int spacemit_ccu_register(struct device *dev,
-				 struct regmap *regmap, struct regmap *lock_regmap,
-				 const struct spacemit_ccu_clk *clks)
+static const struct k1_ccu_data k1_ccu_apmu_data = {
+	.clk		= k1_ccu_apmu_clks,
+};
+
+static int k1_ccu_register(struct device *dev, struct regmap *regmap,
+			   struct regmap *lock_regmap,
+			   struct spacemit_ccu_clk *clks)
  {
  	const struct spacemit_ccu_clk *clk;
  	int i, ret, max_id = 0;

@@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev,
 
  	clk_data->num = max_id + 1;
 

drivers/clk/spacemit/ccu-k1.c static int spacemit_ccu_register(struct device *dev, · line 1675

Quoted 6 lines

drivers/clk/spacemit/ccu-k1.c +5 -1

-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	if (ret)
+		dev_err(dev, "error %d adding clock hardware provider\n", ret);
+
+	return ret;

I'd use "return 0;", nothing different, just explicitly short

ok, I can understand this change ease debug procedure once there is problem. (but I'm fine with either way, failure should rarely happen & will identify early)

Quoted 42 lines

drivers/clk/spacemit/ccu-k1.c +9 -5

  }
 
  static int k1_ccu_probe(struct platform_device *pdev)
  {
  	struct regmap *base_regmap, *lock_regmap = NULL;
  	struct device *dev = &pdev->dev;
+	const struct k1_ccu_data *data;
  	int ret;
 
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
  	base_regmap = device_node_to_regmap(dev->of_node);
  	if (IS_ERR(base_regmap))
  		return dev_err_probe(dev, PTR_ERR(base_regmap),

@@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
  					     "failed to get lock regmap\n");
  	}
 
-	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
-				    of_device_get_match_data(dev));
+	ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
  	if (ret)
  		return dev_err_probe(dev, ret, "failed to register clocks\n");
 

@@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev)
  static const struct of_device_id of_k1_ccu_match[] = {
  	{
  		.compatible	= "spacemit,k1-pll",
-		.data		= k1_ccu_apbs_clks,
+		.data		= &k1_ccu_apbs_data,
  	},
  	{
  		.compatible	= "spacemit,k1-syscon-mpmu",
-		.data		= k1_ccu_mpmu_clks,
+		.data		= &k1_ccu_mpmu_data,
  	},
  	{
  		.compatible	= "spacemit,k1-syscon-apbc",
-		.data		= k1_ccu_apbc_clks,
+		.data		= &k1_ccu_apbc_data,

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_probe(struct platform_device *pdev) · line 1733

Quoted 7 lines

drivers/clk/spacemit/ccu-k1.c +1 -1

  	},
  	{
  		.compatible	= "spacemit,k1-syscon-apmu",
-		.data		= k1_ccu_apmu_clks,
+		.data		= &k1_ccu_apmu_data,
  	},
  	{ }

	{ /* sentinel */ }

Quoted 4 lines

drivers/clk/spacemit/ccu-k1.c +0 -0

  };

Signature · 1 line

2.43.0

Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead.

Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it.

Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 44db48ae71313..f7367271396a0 100644
@@ -129,6 +129,15 @@
 #define APMU_EMAC0_CLK_RES_CTRL		0x3e4

drivers/clk/spacemit/ccu-k1.c +3 -0

+static int k1_ccu_register(struct device *dev, struct regmap *regmap,
+			   struct regmap *lock_regmap,
+			   struct spacemit_ccu_clk *clks)
 {
 	const struct spacemit_ccu_clk *clk;
 	int i, ret, max_id = 0;

Quoted 8 lines · Alex Elder

Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead.

Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it.

Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

I've read your conversation about moving parts of the patch into the clock series, I'm of course willing to :)

Quoted 13 lines

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
  1 file changed, 43 insertions(+), 15 deletions(-)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 44db48ae71313..f7367271396a0 100644
@@ -129,6 +129,15 @@
  #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
  #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
 

drivers/clk/spacemit/ccu-k1.c line 139

Quoted 8 lines

drivers/clk/spacemit/ccu-k1.c +8 -0

+struct spacemit_ccu_clk {
+	int id;
+	struct clk_hw *hw;
+};
+
+struct k1_ccu_data {
+	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+};

This is something like what I've dropped in v5 of the clock series so I doubt whether it should be added back in clock series again, as at that point there's no reason for an extra structure: Alex, is it okay for you to keep the change in reset series?

...

Quoted 7 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+static int k1_ccu_register(struct device *dev, struct regmap *regmap,
+			   struct regmap *lock_regmap,
+			   struct spacemit_ccu_clk *clks)
  {
  	const struct spacemit_ccu_clk *clk;
  	int i, ret, max_id = 0;

drivers/clk/spacemit/ccu-k1.c static int spacemit_ccu_register(struct device *dev, · line 1673

Quoted 7 lines

drivers/clk/spacemit/ccu-k1.c +3 -1

@@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev,
 
  	clk_data->num = max_id + 1;
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	if (ret)
+		dev_err(dev, "error %d adding clock hardware provider\n", ret);

This error message definitely should go in the clock series.

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +2 -0

+	return ret;
  }
 
  static int k1_ccu_probe(struct platform_device *pdev)
  {
  	struct regmap *base_regmap, *lock_regmap = NULL;
  	struct device *dev = &pdev->dev;
+	const struct k1_ccu_data *data;
  	int ret;
 

drivers/clk/spacemit/ccu-k1.c static int spacemit_ccu_register(struct device *dev, · line 1687

Quoted 3 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;

Looking through the reset series, I don't see a reason that of_device_get_match_data() could return NULL. This is also something you've asked me to drop in v4 of the clock series, so I guess it isn't necessary.

Quoted 7 lines

drivers/clk/spacemit/ccu-k1.c +0 -0

  	base_regmap = device_node_to_regmap(dev->of_node);
  	if (IS_ERR(base_regmap))
  		return dev_err_probe(dev, PTR_ERR(base_regmap),

@@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
  					     "failed to get lock regmap\n");
  	}
 

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_probe(struct platform_device *pdev) · line 1711

Quoted 5 lines

drivers/clk/spacemit/ccu-k1.c +1 -2

-	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
-				    of_device_get_match_data(dev));
+	ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
  	if (ret)
  		return dev_err_probe(dev, ret, "failed to register clocks\n");

For using ARRAY_SIZE() to simplify runtime code, it's mostly okay since binding IDs are continuous 0-based integers. But I split the handling of TWSI8 into another patch, which creates a hole in the range and breaks the assumption. Do you think the TWSI8 commit should be merged back in the clock driver one?

Best regards,
Haylen Chu

Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array.

Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets.

Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure.

Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

drivers/clk/spacemit/ccu-k1.c +19 -0

index f7367271396a0..6d879411c6c05 100644
@@ -10,6 +10,7 @@
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 
 #include "ccu_common.h"
 #include "ccu_pll.h"

@@ -134,8 +135,26 @@ struct spacemit_ccu_clk {
 	struct clk_hw *hw;
 };
 
+struct ccu_reset_data {
+	u32 offset;
+	u32 assert_mask;
+	u32 deassert_mask;
+};
+
+struct ccu_reset_controller_data {
+	u32 count;
+	const struct ccu_reset_data *data;	/* array */
+};
+
 struct k1_ccu_data {
 	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+	const struct ccu_reset_controller_data *rst_data;
+};
+
+struct ccu_reset_controller {
+	struct regmap *regmap;
+	const struct ccu_reset_controller_data *data;
+	struct reset_controller_dev rcdev;
 };
 
 /*	APBS clocks start	*/

drivers/clk/spacemit/ccu-k1.c +10 -0

+{
+	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
+	struct regmap *regmap = controller->regmap;
+	const struct ccu_reset_data *data;
+	u32 val;
+	int ret;
+
+	data = &controller->data->data[id];
+
+	ret = regmap_read(regmap, data->offset, &val);

drivers/clk/spacemit/ccu-k1.c +33 -0

+{
+	return k1_rst_update(rcdev, id, true);
+}
+
+static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	return k1_rst_update(rcdev, id, false);
+}
+
+static const struct reset_control_ops k1_reset_control_ops = {
+	.assert		= k1_rst_assert,
+	.deassert	= k1_rst_deassert,
+};
+
 static int k1_ccu_register(struct device *dev, struct regmap *regmap,
 			   struct regmap *lock_regmap,
 			   struct spacemit_ccu_clk *clks)

@@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap,
 	return ret;
 }
 
+static int
+k1_reset_controller_register(struct device *dev, struct regmap *regmap,
+			     const struct ccu_reset_controller_data *data)
+{
+	struct ccu_reset_controller *controller;
+	struct reset_controller_dev *rcdev;
+
+	/* Resets are optional */
+	if (!data)
+		return 0;
+
+	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
+	if (!controller)
+		return -ENOMEM;
+
+	controller->regmap = regmap;
+	controller->data = data;
+
+	rcdev = &controller->rcdev;

drivers/clk/spacemit/ccu-k1.c +8 -0

+	rcdev->nr_resets = data->count;
+	rcdev->ops = &k1_reset_control_ops;
+	rcdev->of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, rcdev);
+}
+
 static int k1_ccu_probe(struct platform_device *pdev)
 {
 	struct regmap *base_regmap, *lock_regmap = NULL;

@@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to register clocks\n");
 
+	ret = k1_reset_controller_register(dev, base_regmap, data->rst_data);

drivers/clk/spacemit/ccu-k1.c +1 -0

+
 	return 0;
 }

Signature · 2 lines

-- 
2.43.0

Hi Alex:

Quoted 71 lines · Alex Elder

Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array.

Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets.

Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure.

Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
  1 file changed, 93 insertions(+)

drivers/clk/spacemit/ccu-k1.c +19 -0

index f7367271396a0..6d879411c6c05 100644
@@ -10,6 +10,7 @@
  #include <linux/minmax.h>
  #include <linux/module.h>
  #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 
  #include "ccu_common.h"
  #include "ccu_pll.h"

@@ -134,8 +135,26 @@ struct spacemit_ccu_clk {
  	struct clk_hw *hw;
  };
 
+struct ccu_reset_data {
+	u32 offset;
+	u32 assert_mask;
+	u32 deassert_mask;
+};
+
+struct ccu_reset_controller_data {
+	u32 count;
+	const struct ccu_reset_data *data;	/* array */
+};
+
  struct k1_ccu_data {
  	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+	const struct ccu_reset_controller_data *rst_data;
+};
+
+struct ccu_reset_controller {
+	struct regmap *regmap;
+	const struct ccu_reset_controller_data *data;
+	struct reset_controller_dev rcdev;
  };
 
  /*	APBS clocks start	*/

@@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
  	.clk		= k1_ccu_apmu_clks,
  };
 

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1653

Quoted 2 lines

drivers/clk/spacemit/ccu-k1.c +2 -0

+static struct ccu_reset_controller *
+rcdev_to_controller(struct reset_controller_dev *rcdev)

I'd suggest to avoid the line break to make it slightly more readable, intuitive as the 80 column limit isn't hard rule

there are maybe more place similar to this, I won't add more comments
https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1656

Quoted 3 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+{
+	return container_of(rcdev, struct ccu_reset_controller, rcdev);
+}

since this function is only used once, open-code it? but I'd fine with either way if you prefer to keep it

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1659

Quoted 4 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+
+static int
+k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)

s/k1_rst_update/k1_reset_update/g
this is a taste change, but I found more people follow this when grep driver/reset

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +10 -0

+{
+	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
+	struct regmap *regmap = controller->regmap;
+	const struct ccu_reset_data *data;
+	u32 val;
+	int ret;
+
+	data = &controller->data->data[id];
+
+	ret = regmap_read(regmap, data->offset, &val);

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1679

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +10 -0

+	if (ret)
+		return ret;
+
+	val &= ~(data->assert_mask | data->deassert_mask);
+	val |= assert ? data->assert_mask : data->deassert_mask;
+
+	return regmap_write(regmap, data->offset, val);
+}
+
+static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)

same reason, rst -> reset, more below

Quoted 40 lines

drivers/clk/spacemit/ccu-k1.c +33 -0

+{
+	return k1_rst_update(rcdev, id, true);
+}
+
+static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	return k1_rst_update(rcdev, id, false);
+}
+
+static const struct reset_control_ops k1_reset_control_ops = {
+	.assert		= k1_rst_assert,
+	.deassert	= k1_rst_deassert,
+};
+
  static int k1_ccu_register(struct device *dev, struct regmap *regmap,
  			   struct regmap *lock_regmap,
  			   struct spacemit_ccu_clk *clks)

@@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap,
  	return ret;
  }
 
+static int
+k1_reset_controller_register(struct device *dev, struct regmap *regmap,
+			     const struct ccu_reset_controller_data *data)
+{
+	struct ccu_reset_controller *controller;
+	struct reset_controller_dev *rcdev;
+
+	/* Resets are optional */
+	if (!data)
+		return 0;
+
+	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
+	if (!controller)
+		return -ENOMEM;
+
+	controller->regmap = regmap;
+	controller->data = data;
+
+	rcdev = &controller->rcdev;

..

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_register(struct device *dev, struct regmap *regmap, · line 1758

Quoted 1 line

drivers/clk/spacemit/ccu-k1.c +1 -0

+	rcdev->owner = THIS_MODULE;

move to last?

Quoted 14 lines

drivers/clk/spacemit/ccu-k1.c +7 -0

+	rcdev->nr_resets = data->count;
+	rcdev->ops = &k1_reset_control_ops;
+	rcdev->of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, rcdev);
+}
+
  static int k1_ccu_probe(struct platform_device *pdev)
  {
  	struct regmap *base_regmap, *lock_regmap = NULL;

@@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev)
  	if (ret)
  		return dev_err_probe(dev, ret, "failed to register clocks\n");
 

Quoted 1 line

drivers/clk/spacemit/ccu-k1.c +1 -0

+	ret = k1_reset_controller_register(dev, base_regmap, data->rst_data);

..

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_probe(struct platform_device *pdev) · line 1804

Quoted 3 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register reset controller\n");

same 100 column reason

Quoted 7 lines

+
return 0;
}

Signature · 1 line

2.43.0

Quoted 79 lines · Yixun Lan

Hi Alex:

Quoted 68 lines · Alex Elder

Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array.

Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets.

Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure.

Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
  1 file changed, 93 insertions(+)

drivers/clk/spacemit/ccu-k1.c +19 -0

index f7367271396a0..6d879411c6c05 100644
@@ -10,6 +10,7 @@
  #include <linux/minmax.h>
  #include <linux/module.h>
  #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 
  #include "ccu_common.h"
  #include "ccu_pll.h"

@@ -134,8 +135,26 @@ struct spacemit_ccu_clk {
  	struct clk_hw *hw;
  };
 
+struct ccu_reset_data {
+	u32 offset;
+	u32 assert_mask;
+	u32 deassert_mask;
+};
+
+struct ccu_reset_controller_data {
+	u32 count;
+	const struct ccu_reset_data *data;	/* array */
+};
+
  struct k1_ccu_data {
  	struct spacemit_ccu_clk *clk;		/* array with sentinel */
+	const struct ccu_reset_controller_data *rst_data;
+};
+
+struct ccu_reset_controller {
+	struct regmap *regmap;
+	const struct ccu_reset_controller_data *data;
+	struct reset_controller_dev rcdev;
  };
 
  /*	APBS clocks start	*/

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1653

Quoted 5 lines

drivers/clk/spacemit/ccu-k1.c +2 -0

@@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
  	.clk		= k1_ccu_apmu_clks,
  };
 
+static struct ccu_reset_controller *
+rcdev_to_controller(struct reset_controller_dev *rcdev)

I'd suggest to avoid the line break to make it slightly more readable, intuitive as the 80 column limit isn't hard rule

Quoted 2 lines

there are maybe more place similar to this, I won't add more comments
https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144

I disagree with this suggestion. I personally find this more readable. As the first line of the patch you link to, "80 columns is still preferred". And regardless, it is my (strong) preference to work within 80 columns in almost all cases.

Quoted 5 lines

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1656

Quoted 3 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+{
+	return container_of(rcdev, struct ccu_reset_controller, rcdev);
+}

since this function is only used once, open-code it? but I'd fine with either way if you prefer to keep it

The "to_<containing_type>()" function pattern is extremely common, but I like this suggestion, given it's used only once. I'll implement it in v2.

Quoted 6 lines

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1659

Quoted 4 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+
+static int
+k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)

s/k1_rst_update/k1_reset_update/g
this is a taste change, but I found more people follow this when grep driver/reset

I actually had reset (not rst) before, throughout. But it made a few lines too long, leading to line wraps, so I did this.

In addition, there was a sort of consistency with the use of "clk" instead of "clock", though I do recognize that abbreviation goes way back to when Mike implemented the common clock framework.

I'll switch back to "reset" (and "RESET") in names, but be warned I'll add some line breaks to fit within 80 columns.

Quoted 62 lines

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +10 -0

+{
+	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
+	struct regmap *regmap = controller->regmap;
+	const struct ccu_reset_data *data;
+	u32 val;
+	int ret;
+
+	data = &controller->data->data[id];
+
+	ret = regmap_read(regmap, data->offset, &val);

drivers/clk/spacemit/ccu-k1.c static const struct k1_ccu_data k1_ccu_apmu_data = { · line 1679

Quoted 10 lines

drivers/clk/spacemit/ccu-k1.c +10 -0

+	if (ret)
+		return ret;
+
+	val &= ~(data->assert_mask | data->deassert_mask);
+	val |= assert ? data->assert_mask : data->deassert_mask;
+
+	return regmap_write(regmap, data->offset, val);
+}
+
+static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)

same reason, rst -> reset, more below

Quoted 40 lines

drivers/clk/spacemit/ccu-k1.c +33 -0

+{
+	return k1_rst_update(rcdev, id, true);
+}
+
+static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	return k1_rst_update(rcdev, id, false);
+}
+
+static const struct reset_control_ops k1_reset_control_ops = {
+	.assert		= k1_rst_assert,
+	.deassert	= k1_rst_deassert,
+};
+
  static int k1_ccu_register(struct device *dev, struct regmap *regmap,
  			   struct regmap *lock_regmap,
  			   struct spacemit_ccu_clk *clks)

@@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap,
  	return ret;
  }
 
+static int
+k1_reset_controller_register(struct device *dev, struct regmap *regmap,
+			     const struct ccu_reset_controller_data *data)
+{
+	struct ccu_reset_controller *controller;
+	struct reset_controller_dev *rcdev;
+
+	/* Resets are optional */
+	if (!data)
+		return 0;
+
+	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
+	if (!controller)
+		return -ENOMEM;
+
+	controller->regmap = regmap;
+	controller->data = data;
+
+	rcdev = &controller->rcdev;

..

Quoted 2 lines

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_register(struct device *dev, struct regmap *regmap, · line 1758

Quoted 1 line

drivers/clk/spacemit/ccu-k1.c +1 -0

+	rcdev->owner = THIS_MODULE;

move to last?

You mean move nr_resets to last? I'll do that. I'll order the assignments in the order they're defined in "reset-controller.h".

Quoted 14 lines

Quoted 14 lines

drivers/clk/spacemit/ccu-k1.c +7 -0

+	rcdev->nr_resets = data->count;
+	rcdev->ops = &k1_reset_control_ops;
+	rcdev->of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, rcdev);
+}
+
  static int k1_ccu_probe(struct platform_device *pdev)
  {
  	struct regmap *base_regmap, *lock_regmap = NULL;

@@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev)
  	if (ret)
  		return dev_err_probe(dev, ret, "failed to register clocks\n");
 

Quoted 6 lines

Quoted 1 line

drivers/clk/spacemit/ccu-k1.c +1 -0

+	ret = k1_reset_controller_register(dev, base_regmap, data->rst_data);

..

drivers/clk/spacemit/ccu-k1.c static int k1_ccu_probe(struct platform_device *pdev) · line 1804

Quoted 3 lines

drivers/clk/spacemit/ccu-k1.c +3 -0

+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register reset controller\n");

same 100 column reason

This one I might go beyond columns, because it's only a few characters.

Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array.

Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets.

Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure.

Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index f7367271396a0..6d879411c6c05 100644

drivers/clk/spacemit/ccu-k1.c +10 -0

+static int
+k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)
+{
+	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
+	struct regmap *regmap = controller->regmap;
+	const struct ccu_reset_data *data;
+	u32 val;
+	int ret;
+
+	data = &controller->data->data[id];

Quoted 32 lines · Alex Elder

Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array.

Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets.

Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure.

Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
  1 file changed, 93 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index f7367271396a0..6d879411c6c05 100644

...

Quoted 19 lines

drivers/clk/spacemit/ccu-k1.c +19 -0

+static int
+k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)
+{
+	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
+	struct regmap *regmap = controller->regmap;
+	const struct ccu_reset_data *data;
+	u32 val;
+	int ret;
+
+	data = &controller->data->data[id];
+
+	ret = regmap_read(regmap, data->offset, &val);
+	if (ret)
+		return ret;
+
+	val &= ~(data->assert_mask | data->deassert_mask);
+	val |= assert ? data->assert_mask : data->deassert_mask;
+
+	return regmap_write(regmap, data->offset, val);

Quoted 1 line

+}

I don't think it's safe to write the regmap based on a value read earlier without the regmap's inner lock held: it's totally fine for the clock part to issue an update of the register at the same time. Without knowledge on it, reset code may rollback the clock bits written by clock code earlier to the original value. That's why I keep using ccu_update() everywhere and dropped ccu_write().

Thanks,
Haylen Chu

Define reset controls associated with the MPMU, APBC, and APMU SpacemiT K1 CCUs. These already have clocks associated with them.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 132 ++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 6d879411c6c05..be8abd27753cb 100644

drivers/clk/spacemit/ccu-k1.c +9 -0

+static const struct ccu_reset_data apmu_reset_data[] = {
+	[RST_CCIC_4X]	= RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_CCIC1_PHY] = RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(2)),
+	[RST_SDH_AXI]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_SDH0]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH1]	= RST_DATA(APMU_SDH1_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH2]	= RST_DATA(APMU_SDH2_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_USBP1_AXI] = RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_USB_AXI]	= RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(0)),

drivers/clk/spacemit/ccu-k1.c +43 -0

+	[RST_QSPI]	= RST_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_QSPI_BUS] = RST_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_DMA]	= RST_DATA(APMU_DMA_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_AES]	= RST_DATA(APMU_AES_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_VPU]	= RST_DATA(APMU_VPU_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_GPU]	= RST_DATA(APMU_GPU_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMMC]	= RST_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMMC_X]	= RST_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_AUDIO]	= RST_DATA(APMU_AUDIO_CLK_RES_CTRL,	0,
+				   BIT(0) | BIT(2) | BIT(3)),
+	[RST_HDMI]	= RST_DATA(APMU_HDMI_CLK_RES_CTRL,	0, BIT(9)),
+	[RST_PCIE0]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_0,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_PCIE1]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_1,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_PCIE2]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_2,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_EMAC0]	= RST_DATA(APMU_EMAC0_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMAC1]	= RST_DATA(APMU_EMAC1_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_JPG]	= RST_DATA(APMU_JPG_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_CCIC2PHY]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(2)),
+	[RST_CCIC3PHY]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(29)),
+	[RST_CSI]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_ISP]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_ISP_CPP]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(27)),
+	[RST_ISP_BUS]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(3)),
+	[RST_ISP_CI]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(16)),
+	[RST_DPU_MCLK]	= RST_DATA(APMU_LCD_CLK_RES_CTRL2,	0, BIT(9)),
+	[RST_DPU_ESC]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(3)),
+	[RST_DPU_HCLK]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(4)),
+	[RST_DPU_SPIBUS] = RST_DATA(APMU_LCD_SPI_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_DPU_SPI_HBUS] = RST_DATA(APMU_LCD_SPI_CLK_RES_CTRL, 0, BIT(2)),
+	[RST_V2D]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(27)),
+	[RST_MIPI]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(15)),
+	[RST_MC]	= RST_DATA(APMU_PMUA_MC_CTRL,		0, BIT(0)),
+};
+
+static const struct ccu_reset_controller_data apmu_reset_controller_data = {
+	.count		= ARRAY_SIZE(apmu_reset_data),
+	.data		= apmu_reset_data,
+};
+
 static const struct k1_ccu_data k1_ccu_apmu_data = {
 	.clk		= k1_ccu_apmu_clks,
+	.rst_data	= &apmu_reset_controller_data,
 };
 
 static struct ccu_reset_controller *

Signature · 1 line

2.43.0

Quoted 12 lines · Alex Elder

Define reset controls associated with the MPMU, APBC, and APMU SpacemiT K1 CCUs. These already have clocks associated with them.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 132 ++++++++++++++++++++++++++++++++++
  1 file changed, 132 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 6d879411c6c05..be8abd27753cb 100644

..

Quoted 9 lines

drivers/clk/spacemit/ccu-k1.c +9 -0

+static const struct ccu_reset_data apmu_reset_data[] = {
+	[RST_CCIC_4X]	= RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_CCIC1_PHY] = RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(2)),
+	[RST_SDH_AXI]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_SDH0]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH1]	= RST_DATA(APMU_SDH1_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH2]	= RST_DATA(APMU_SDH2_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_USBP1_AXI] = RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_USB_AXI]	= RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(0)),

..

drivers/clk/spacemit/ccu-k1.c static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { · line 1736

Quoted 2 lines

drivers/clk/spacemit/ccu-k1.c +2 -0

+	[RST_USB3_0]	= RST_DATA(APMU_USB_CLK_RES_CTRL,	0,
+				      BIT(9)|BIT(10)|BIT(11)),

100 column if possible, also add one space between "BIT(9) | BIT(10) .." continuous bits can just use GENMASK for short?

but may result slightly unreadable, anyway, either way is fine by me

Quoted 51 lines

drivers/clk/spacemit/ccu-k1.c +43 -0

+	[RST_QSPI]	= RST_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_QSPI_BUS] = RST_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_DMA]	= RST_DATA(APMU_DMA_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_AES]	= RST_DATA(APMU_AES_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_VPU]	= RST_DATA(APMU_VPU_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_GPU]	= RST_DATA(APMU_GPU_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMMC]	= RST_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMMC_X]	= RST_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_AUDIO]	= RST_DATA(APMU_AUDIO_CLK_RES_CTRL,	0,
+				   BIT(0) | BIT(2) | BIT(3)),
+	[RST_HDMI]	= RST_DATA(APMU_HDMI_CLK_RES_CTRL,	0, BIT(9)),
+	[RST_PCIE0]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_0,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_PCIE1]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_1,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_PCIE2]	= RST_DATA(APMU_PCIE_CLK_RES_CTRL_2,	BIT(8),
+				   BIT(3) | BIT(4) | BIT(5)),
+	[RST_EMAC0]	= RST_DATA(APMU_EMAC0_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_EMAC1]	= RST_DATA(APMU_EMAC1_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_JPG]	= RST_DATA(APMU_JPG_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_CCIC2PHY]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(2)),
+	[RST_CCIC3PHY]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(29)),
+	[RST_CSI]	= RST_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_ISP]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_ISP_CPP]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(27)),
+	[RST_ISP_BUS]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(3)),
+	[RST_ISP_CI]	= RST_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(16)),
+	[RST_DPU_MCLK]	= RST_DATA(APMU_LCD_CLK_RES_CTRL2,	0, BIT(9)),
+	[RST_DPU_ESC]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(3)),
+	[RST_DPU_HCLK]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(4)),
+	[RST_DPU_SPIBUS] = RST_DATA(APMU_LCD_SPI_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_DPU_SPI_HBUS] = RST_DATA(APMU_LCD_SPI_CLK_RES_CTRL, 0, BIT(2)),
+	[RST_V2D]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(27)),
+	[RST_MIPI]	= RST_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(15)),
+	[RST_MC]	= RST_DATA(APMU_PMUA_MC_CTRL,		0, BIT(0)),
+};
+
+static const struct ccu_reset_controller_data apmu_reset_controller_data = {
+	.count		= ARRAY_SIZE(apmu_reset_data),
+	.data		= apmu_reset_data,
+};
+
  static const struct k1_ccu_data k1_ccu_apmu_data = {
  	.clk		= k1_ccu_apmu_clks,
+	.rst_data	= &apmu_reset_controller_data,
  };
 
  static struct ccu_reset_controller *

Signature · 1 line

2.43.0

Quoted 24 lines · Yixun Lan

Quoted 12 lines · Alex Elder

Define reset controls associated with the MPMU, APBC, and APMU SpacemiT K1 CCUs. These already have clocks associated with them.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 132 ++++++++++++++++++++++++++++++++++
  1 file changed, 132 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 6d879411c6c05..be8abd27753cb 100644

..

Quoted 9 lines

drivers/clk/spacemit/ccu-k1.c +9 -0

+static const struct ccu_reset_data apmu_reset_data[] = {
+	[RST_CCIC_4X]	= RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_CCIC1_PHY] = RST_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(2)),
+	[RST_SDH_AXI]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(0)),
+	[RST_SDH0]	= RST_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH1]	= RST_DATA(APMU_SDH1_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_SDH2]	= RST_DATA(APMU_SDH2_CLK_RES_CTRL,	0, BIT(1)),
+	[RST_USBP1_AXI] = RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(4)),
+	[RST_USB_AXI]	= RST_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(0)),

..

Quoted 4 lines

drivers/clk/spacemit/ccu-k1.c static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { · line 1736

Quoted 2 lines

drivers/clk/spacemit/ccu-k1.c +2 -0

+	[RST_USB3_0]	= RST_DATA(APMU_USB_CLK_RES_CTRL,	0,
+				      BIT(9)|BIT(10)|BIT(11)),

100 column if possible, also add one space between "BIT(9) | BIT(10) .." continuous bits can just use GENMASK for short?

You'll notice that every place that has multiple bits in the mask also have a line break. I kind of liked that as a way to highlight that fact (i.e., it goes beyond my preference for 80 columns).

I will definitely add spaces, that was a mistake not to.

I will not define this with GENMASK(). In this case each bit represents a single reset, and so each one is significant on its own. It is *not* a mask of contiguous bits, it's a set of bits that happen to have consecutive positions.

Enable support for three additional syscon CCUs which support reset controls but no clocks: ARCPU, RCPU2, and APBC2.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/clk/spacemit/ccu-k1.c | 106 ++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 17e321c25959a..bf5a3e2048619 100644
@@ -130,6 +130,37 @@
 #define APMU_EMAC0_CLK_RES_CTRL		0x3e4

drivers/clk/spacemit/ccu-k1.c +98 -0

+#define AUDIO_HDMI_CLK_CTRL		0x2044
+
+/* RCPU2 register offsets */
+#define RCPU2_PWM0_CLK_RST		0x0000
+#define RCPU2_PWM1_CLK_RST		0x0004
+#define RCPU2_PWM2_CLK_RST		0x0008
+#define RCPU2_PWM3_CLK_RST		0x000c
+#define RCPU2_PWM4_CLK_RST		0x0010
+#define RCPU2_PWM5_CLK_RST		0x0014
+#define RCPU2_PWM6_CLK_RST		0x0018
+#define RCPU2_PWM7_CLK_RST		0x001c
+#define RCPU2_PWM8_CLK_RST		0x0020
+#define RCPU2_PWM9_CLK_RST		0x0024
+
+/* APBC2 register offsets */
+#define APBC2_UART1_CLK_RST		0x0000
+#define APBC2_SSP2_CLK_RST		0x0004
+#define APBC2_TWSI3_CLK_RST		0x0008
+#define APBC2_RTC_CLK_RST		0x000c
+#define APBC2_TIMERS0_CLK_RST		0x0010
+#define APBC2_KPC_CLK_RST		0x0014
+#define APBC2_GPIO_CLK_RST		0x001c
+
 struct spacemit_ccu_clk {
 	int id;
 	struct clk_hw *hw;

@@ -1781,6 +1812,69 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
 	.rst_data	= &apmu_reset_controller_data,
 };
 
+static const struct ccu_reset_data rcpu_reset_data[] = {
+	[RST_RCPU_SSP0]		= RST_DATA(RCPU_SSP0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_I2C0]		= RST_DATA(RCPU_I2C0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_UART1]	= RST_DATA(RCPU_UART1_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_IR]		= RST_DATA(RCPU_CAN_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_CAN]		= RST_DATA(RCPU_IR_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_UART0]	= RST_DATA(RCPU_UART0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_HDMI_AUDIO]	= RST_DATA(AUDIO_HDMI_CLK_CTRL,	0, BIT(0)),
+};
+
+static const struct ccu_reset_controller_data rcpu_reset_controller_data = {
+	.count		= ARRAY_SIZE(rcpu_reset_data),
+	.data		= rcpu_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_rcpu_data = {
+	/* No clocks in the RCPU CCU */
+	.rst_data	= &rcpu_reset_controller_data,
+};
+
+static const struct ccu_reset_data rcpu2_reset_data[] = {
+	[RST_RCPU2_PWM0]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM1]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM2]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM3]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM4]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM5]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM6]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM7]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM8]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM9]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+};
+
+static const struct ccu_reset_controller_data rcpu2_reset_controller_data = {
+	.count		= ARRAY_SIZE(rcpu2_reset_data),
+	.data		= rcpu2_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_rcpu2_data = {
+	/* No clocks in the RCPU2 CCU */
+	.rst_data	= &rcpu2_reset_controller_data,
+};
+
+static const struct ccu_reset_data apbc2_reset_data[] = {
+	[RST_APBC2_UART1]	= RST_DATA(APBC2_UART1_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_SSP2]	= RST_DATA(APBC2_SSP2_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_TWSI3]	= RST_DATA(APBC2_TWSI3_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_RTC]		= RST_DATA(APBC2_RTC_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_TIMERS0]	= RST_DATA(APBC2_TIMERS0_CLK_RST, BIT(2), (0)),
+	[RST_APBC2_KPC]		= RST_DATA(APBC2_KPC_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_GPIO]	= RST_DATA(APBC2_GPIO_CLK_RST,	BIT(2), (0)),
+};
+
+static const struct ccu_reset_controller_data apbc2_reset_controller_data = {
+	.count		= ARRAY_SIZE(apbc2_reset_data),
+	.data		= apbc2_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_apbc2_data = {
+	/* No clocks in the RCPU2 CCU */
+	.rst_data	= &apbc2_reset_controller_data,
+};
+
 static struct ccu_reset_controller *
 rcdev_to_controller(struct reset_controller_dev *rcdev)
 {

@@ -1959,6 +2053,18 @@ static const struct of_device_id of_k1_ccu_match[] = {
 		.compatible	= "spacemit,k1-syscon-apmu",
 		.data		= &k1_ccu_apmu_data,
 	},
+	{
+		.compatible	= "spacemit,k1-syscon-rcpu",
+		.data		= &k1_ccu_rcpu_data,
+	},
+	{
+		.compatible	= "spacemit,k1-syscon-rcpu2",
+		.data		= &k1_ccu_rcpu2_data,
+	},
+	{
+		.compatible	= "spacemit,k1-syscon-apbc2",
+		.data		= &k1_ccu_apbc2_data,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, of_k1_ccu_match);

Signature · 1 line

2.43.0

Hi Alex:

It occur to me it's a little odd to implemnt reset driver for RCPU block, but after check with vendor the RCPU region can be accessed both by ACPU and RCPU, then I'm fine with this.

ACPU - RISC-V Main CPU, with mmu, running Linux
RCPU - real time CPU, without mmu, running RT-OS

Quoted 16 lines

Enable support for three additional syscon CCUs which support reset controls but no clocks: ARCPU, RCPU2, and APBC2.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/clk/spacemit/ccu-k1.c | 106 ++++++++++++++++++++++++++++++++++
  1 file changed, 106 insertions(+)

drivers/clk/spacemit/ccu-k1.c +0 -0

index 17e321c25959a..bf5a3e2048619 100644
@@ -130,6 +130,37 @@
  #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
  #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
 

drivers/clk/spacemit/ccu-k1.c line 140

Quoted 8 lines

drivers/clk/spacemit/ccu-k1.c +8 -0

+/* RCPU register offsets */
+#define RCPU_SSP0_CLK_RST		0x0028
+#define RCPU_I2C0_CLK_RST		0x0030
+#define RCPU_UART1_CLK_RST		0x003c
+#define RCPU_CAN_CLK_RST		0x0048
+#define RCPU_IR_CLK_RST			0x004c
+#define RCPU_UART0_CLK_RST		0x00d8
+/* XXX Next one is part of the AUD_AUDCLOCK region @ 0xc0882000 */

this comment looks odd, XXX?

Quoted 118 lines

drivers/clk/spacemit/ccu-k1.c +98 -0

+#define AUDIO_HDMI_CLK_CTRL		0x2044
+
+/* RCPU2 register offsets */
+#define RCPU2_PWM0_CLK_RST		0x0000
+#define RCPU2_PWM1_CLK_RST		0x0004
+#define RCPU2_PWM2_CLK_RST		0x0008
+#define RCPU2_PWM3_CLK_RST		0x000c
+#define RCPU2_PWM4_CLK_RST		0x0010
+#define RCPU2_PWM5_CLK_RST		0x0014
+#define RCPU2_PWM6_CLK_RST		0x0018
+#define RCPU2_PWM7_CLK_RST		0x001c
+#define RCPU2_PWM8_CLK_RST		0x0020
+#define RCPU2_PWM9_CLK_RST		0x0024
+
+/* APBC2 register offsets */
+#define APBC2_UART1_CLK_RST		0x0000
+#define APBC2_SSP2_CLK_RST		0x0004
+#define APBC2_TWSI3_CLK_RST		0x0008
+#define APBC2_RTC_CLK_RST		0x000c
+#define APBC2_TIMERS0_CLK_RST		0x0010
+#define APBC2_KPC_CLK_RST		0x0014
+#define APBC2_GPIO_CLK_RST		0x001c
+
  struct spacemit_ccu_clk {
  	int id;
  	struct clk_hw *hw;

@@ -1781,6 +1812,69 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
  	.rst_data	= &apmu_reset_controller_data,
  };
 
+static const struct ccu_reset_data rcpu_reset_data[] = {
+	[RST_RCPU_SSP0]		= RST_DATA(RCPU_SSP0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_I2C0]		= RST_DATA(RCPU_I2C0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_UART1]	= RST_DATA(RCPU_UART1_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_IR]		= RST_DATA(RCPU_CAN_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_CAN]		= RST_DATA(RCPU_IR_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_UART0]	= RST_DATA(RCPU_UART0_CLK_RST,	0, BIT(0)),
+	[RST_RCPU_HDMI_AUDIO]	= RST_DATA(AUDIO_HDMI_CLK_CTRL,	0, BIT(0)),
+};
+
+static const struct ccu_reset_controller_data rcpu_reset_controller_data = {
+	.count		= ARRAY_SIZE(rcpu_reset_data),
+	.data		= rcpu_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_rcpu_data = {
+	/* No clocks in the RCPU CCU */
+	.rst_data	= &rcpu_reset_controller_data,
+};
+
+static const struct ccu_reset_data rcpu2_reset_data[] = {
+	[RST_RCPU2_PWM0]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM1]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM2]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM3]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM4]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM5]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM6]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM7]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM8]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+	[RST_RCPU2_PWM9]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
+};
+
+static const struct ccu_reset_controller_data rcpu2_reset_controller_data = {
+	.count		= ARRAY_SIZE(rcpu2_reset_data),
+	.data		= rcpu2_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_rcpu2_data = {
+	/* No clocks in the RCPU2 CCU */
+	.rst_data	= &rcpu2_reset_controller_data,
+};
+
+static const struct ccu_reset_data apbc2_reset_data[] = {
+	[RST_APBC2_UART1]	= RST_DATA(APBC2_UART1_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_SSP2]	= RST_DATA(APBC2_SSP2_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_TWSI3]	= RST_DATA(APBC2_TWSI3_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_RTC]		= RST_DATA(APBC2_RTC_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_TIMERS0]	= RST_DATA(APBC2_TIMERS0_CLK_RST, BIT(2), (0)),
+	[RST_APBC2_KPC]		= RST_DATA(APBC2_KPC_CLK_RST,	BIT(2), (0)),
+	[RST_APBC2_GPIO]	= RST_DATA(APBC2_GPIO_CLK_RST,	BIT(2), (0)),
+};
+
+static const struct ccu_reset_controller_data apbc2_reset_controller_data = {
+	.count		= ARRAY_SIZE(apbc2_reset_data),
+	.data		= apbc2_reset_data,
+};
+
+static struct k1_ccu_data k1_ccu_apbc2_data = {
+	/* No clocks in the RCPU2 CCU */
+	.rst_data	= &apbc2_reset_controller_data,
+};
+
  static struct ccu_reset_controller *
  rcdev_to_controller(struct reset_controller_dev *rcdev)
  {

@@ -1959,6 +2053,18 @@ static const struct of_device_id of_k1_ccu_match[] = {
  		.compatible	= "spacemit,k1-syscon-apmu",
  		.data		= &k1_ccu_apmu_data,
  	},
+	{
+		.compatible	= "spacemit,k1-syscon-rcpu",
+		.data		= &k1_ccu_rcpu_data,
+	},
+	{
+		.compatible	= "spacemit,k1-syscon-rcpu2",
+		.data		= &k1_ccu_rcpu2_data,
+	},
+	{
+		.compatible	= "spacemit,k1-syscon-apbc2",
+		.data		= &k1_ccu_apbc2_data,
+	},
  	{ }
  };
  MODULE_DEVICE_TABLE(of, of_k1_ccu_match);

Signature · 1 line

2.43.0