[PATCH 0/4] Allwinner H6 RSB support

[PATCH 0/4] Allwinner H6 RSB support

[Samuel Holland]

The Allwinner H6 SoC contains an RSB controller. It is almost completely
undocumented, so it was missed when doing the initial SoC bringup.

This series adds the clock/reset, pin configuration, and device tree
node needed to use the RSB controller. Since RSB is faster, simpler, and
generally more reliable than the I2C controller IP in the SoC, switch to
using it where possible.

This was tested on an Orange Pi 3 and a Pine H64 model B. This series
does not switch the Pine H64 to use RSB, as doing so would prevent
accessing the external RTC that shares the I2C bus.

Samuel Holland (4):
  clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset
  pinctrl: sunxi: h6-r: Add s_rsb pin functions
  arm64: dts: allwinner: h6: Add RSB controller node
  arm64: dts: allwinner: h6: Use RSB for AXP805 PMIC connection

 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   | 38 +++++++++----------
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 14 +++----
 .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 22 +++++------
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 19 ++++++++++
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c        |  5 +++
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h        |  2 +-
 drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c   |  2 +
 include/dt-bindings/clock/sun50i-h6-r-ccu.h   |  1 +
 include/dt-bindings/reset/sun50i-h6-r-ccu.h   |  1 +
 9 files changed, 66 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH 1/4] clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset

[Samuel Holland]

While no information about the H6 RSB controller is included in the
datasheet or manual, the vendor BSP and power management blob both
reference the RSB clock parent and register address. These values were
verified by experimentation.

Since this clock/reset are added late, the specifier is added at the end
to maintain the existing DT binding. The code is kept in register order.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 5 +++++
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
 include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
 include/dt-bindings/reset/sun50i-h6-r-ccu.h | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
index 50f8d1bc7046..56e351b513f3 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
@@ -91,6 +91,8 @@ static SUNXI_CCU_GATE(r_apb2_uart_clk,	"r-apb2-uart",	"r-apb2",
 		      0x18c, BIT(0), 0);
 static SUNXI_CCU_GATE(r_apb2_i2c_clk,	"r-apb2-i2c",	"r-apb2",
 		      0x19c, BIT(0), 0);
+static SUNXI_CCU_GATE(r_apb2_rsb_clk,	"r-apb2-rsb",	"r-apb2",
+		      0x1bc, BIT(0), 0);
 static SUNXI_CCU_GATE(r_apb1_ir_clk,	"r-apb1-ir",	"r-apb1",
 		      0x1cc, BIT(0), 0);
 static SUNXI_CCU_GATE(r_apb1_w1_clk,	"r-apb1-w1",	"r-apb1",
@@ -130,6 +132,7 @@ static struct ccu_common *sun50i_h6_r_ccu_clks[] = {
 	&r_apb1_pwm_clk.common,
 	&r_apb2_uart_clk.common,
 	&r_apb2_i2c_clk.common,
+	&r_apb2_rsb_clk.common,
 	&r_apb1_ir_clk.common,
 	&r_apb1_w1_clk.common,
 	&ir_clk.common,
@@ -147,6 +150,7 @@ static struct clk_hw_onecell_data sun50i_h6_r_hw_clks = {
 		[CLK_R_APB1_PWM]	= &r_apb1_pwm_clk.common.hw,
 		[CLK_R_APB2_UART]	= &r_apb2_uart_clk.common.hw,
 		[CLK_R_APB2_I2C]	= &r_apb2_i2c_clk.common.hw,
+		[CLK_R_APB2_RSB]	= &r_apb2_rsb_clk.common.hw,
 		[CLK_R_APB1_IR]		= &r_apb1_ir_clk.common.hw,
 		[CLK_R_APB1_W1]		= &r_apb1_w1_clk.common.hw,
 		[CLK_IR]		= &ir_clk.common.hw,
@@ -161,6 +165,7 @@ static struct ccu_reset_map sun50i_h6_r_ccu_resets[] = {
 	[RST_R_APB1_PWM]	=  { 0x13c, BIT(16) },
 	[RST_R_APB2_UART]	=  { 0x18c, BIT(16) },
 	[RST_R_APB2_I2C]	=  { 0x19c, BIT(16) },
+	[RST_R_APB2_RSB]	=  { 0x1bc, BIT(16) },
 	[RST_R_APB1_IR]		=  { 0x1cc, BIT(16) },
 	[RST_R_APB1_W1]		=  { 0x1ec, BIT(16) },
 };
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
index 782117dc0b28..7e290b840803 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
@@ -14,6 +14,6 @@
 
 #define CLK_R_APB2	3
 
-#define CLK_NUMBER	(CLK_W1 + 1)
+#define CLK_NUMBER	(CLK_R_APB2_RSB + 1)
 
 #endif /* _CCU_SUN50I_H6_R_H */
diff --git a/include/dt-bindings/clock/sun50i-h6-r-ccu.h b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
index 76136132a13e..f46ec03848ca 100644
--- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
+++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
@@ -15,6 +15,7 @@
 #define CLK_R_APB1_PWM		6
 #define CLK_R_APB2_UART		7
 #define CLK_R_APB2_I2C		8
+#define CLK_R_APB2_RSB		13
 #define CLK_R_APB1_IR		9
 #define CLK_R_APB1_W1		10
 
diff --git a/include/dt-bindings/reset/sun50i-h6-r-ccu.h b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
index 01c84dba49a4..6fe199a7969d 100644
--- a/include/dt-bindings/reset/sun50i-h6-r-ccu.h
+++ b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
@@ -11,6 +11,7 @@
 #define RST_R_APB1_PWM		2
 #define RST_R_APB2_UART		3
 #define RST_R_APB2_I2C		4
+#define RST_R_APB2_RSB		7
 #define RST_R_APB1_IR		5
 #define RST_R_APB1_W1		6
 
-- 
2.26.2

[PATCH 2/4] pinctrl: sunxi: h6-r: Add s_rsb pin functions

[Samuel Holland]

As there is an RSB controller in the H6 SoC, there should be some pin
configuration for it. While no such configuration is documented, the
"s_i2c" pins are suspiciously on the "alternate" function 3, with no
primary function 2 given. This suggests the primary function for these
pins is actually RSB, and that is indeed the case.

Add the "s_rsb" pin functions so the RSB controller can be used.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c b/drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c
index 4557e18d5989..c7d90c44e87a 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sun50i-h6-r.c
@@ -24,11 +24,13 @@ static const struct sunxi_desc_pin sun50i_h6_r_pins[] = {
 	SUNXI_PIN(SUNXI_PINCTRL_PIN(L, 0),
 		  SUNXI_FUNCTION(0x0, "gpio_in"),
 		  SUNXI_FUNCTION(0x1, "gpio_out"),
+		  SUNXI_FUNCTION(0x2, "s_rsb"),		/* SCK */
 		  SUNXI_FUNCTION(0x3, "s_i2c"),		/* SCK */
 		  SUNXI_FUNCTION_IRQ_BANK(0x6, 0, 0)),	/* PL_EINT0 */
 	SUNXI_PIN(SUNXI_PINCTRL_PIN(L, 1),
 		  SUNXI_FUNCTION(0x0, "gpio_in"),
 		  SUNXI_FUNCTION(0x1, "gpio_out"),
+		  SUNXI_FUNCTION(0x2, "s_rsb"),		/* SDA */
 		  SUNXI_FUNCTION(0x3, "s_i2c"),		/* SDA */
 		  SUNXI_FUNCTION_IRQ_BANK(0x6, 0, 1)),	/* PL_EINT1 */
 	SUNXI_PIN(SUNXI_PINCTRL_PIN(L, 2),
-- 
2.26.2

[PATCH 3/4] arm64: dts: allwinner: h6: Add RSB controller node

[Samuel Holland]

The H6 SoC contains an undocumented but fully functional RSB controller.
Add support for it. The MMIO register address matches other SoCs of the
same generation, and the IRQ matches a hole in the documented IRQ list.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 34cfd68ca523..dd47176e432d 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -1014,6 +1014,11 @@ r_ir_rx_pin: r-ir-rx-pin {
 				pins = "PL9";
 				function = "s_cir_rx";
 			};
+
+			r_rsb_pins: r-rsb-pins {
+				pins = "PL0", "PL1";
+				function = "s_rsb";
+			};
 		};
 
 		r_ir: ir@7040000 {
@@ -1045,6 +1050,20 @@ r_i2c: i2c@7081400 {
 			#size-cells = <0>;
 		};
 
+		r_rsb: rsb@7083000 {
+			compatible = "allwinner,sun8i-a23-rsb";
+			reg = <0x07083000 0x400>;
+			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&r_ccu CLK_R_APB2_RSB>;
+			clock-frequency = <3000000>;
+			resets = <&r_ccu RST_R_APB2_RSB>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&r_rsb_pins>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		ths: thermal-sensor@5070400 {
 			compatible = "allwinner,sun50i-h6-ths";
 			reg = <0x05070400 0x100>;
-- 
2.26.2

[PATCH 4/4] arm64: dts: allwinner: h6: Use RSB for AXP805 PMIC connection

[Samuel Holland]

On boards where the only peripheral connected to PL0/PL1 is an X-Powers
PMIC, configure the connection to use the RSB bus rather than the I2C
bus. Compared to the I2C controller that shares the pins, the RSB
controller allows a higher bus frequency, and it is more CPU-efficient.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   | 38 +++++++++----------
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 14 +++----
 .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 22 +++++------
 3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 416e0fa76ba7..8a95abfc2ebb 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -158,12 +158,28 @@ &pio {
 	vcc-pg-supply = <&reg_aldo1>;
 };
 
-&r_i2c {
+&r_ir {
+	linux,rc-map-name = "rc-beelink-gs1";
+	status = "okay";
+};
+
+&r_pio {
+	/*
+	 * FIXME: We can't add that supply for now since it would
+	 * create a circular dependency between pinctrl, the regulator
+	 * and the RSB Bus.
+	 *
+	 * vcc-pl-supply = <&reg_aldo1>;
+	 */
+	vcc-pm-supply = <&reg_aldo1>;
+};
+
+&r_rsb {
 	status = "okay";
 
-	axp805: pmic@36 {
+	axp805: pmic@745 {
 		compatible = "x-powers,axp805", "x-powers,axp806";
-		reg = <0x36>;
+		reg = <0x745>;
 		interrupt-parent = <&r_intc>;
 		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
 		interrupt-controller;
@@ -281,22 +297,6 @@ sw {
 	};
 };
 
-&r_ir {
-	linux,rc-map-name = "rc-beelink-gs1";
-	status = "okay";
-};
-
-&r_pio {
-	/*
-	 * PL0 and PL1 are used for PMIC I2C
-	 * don't enable the pl-supply else
-	 * it will fail at boot
-	 *
-	 * vcc-pl-supply = <&reg_aldo1>;
-	 */
-	vcc-pm-supply = <&reg_aldo1>;
-};
-
 &rtc {
 	clocks = <&ext_osc32k>;
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index 8a8d1a608e30..e86360ea022e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -222,12 +222,16 @@ &pio {
 	vcc-pg-supply = <&reg_vcc_wifi_io>;
 };
 
-&r_i2c {
+&r_ir {
+	status = "okay";
+};
+
+&r_rsb {
 	status = "okay";
 
-	axp805: pmic@36 {
+	axp805: pmic@745 {
 		compatible = "x-powers,axp805", "x-powers,axp806";
-		reg = <0x36>;
+		reg = <0x745>;
 		interrupt-parent = <&r_intc>;
 		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
 		interrupt-controller;
@@ -339,10 +343,6 @@ sw {
 	};
 };
 
-&r_ir {
-	status = "okay";
-};
-
 &rtc {
 	clocks = <&ext_osc32k>;
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
index 0d5f9aeb96d0..96635588e9a6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
@@ -128,12 +128,20 @@ &pio {
 	vcc-pg-supply = <&reg_aldo1>;
 };
 
-&r_i2c {
+&r_ir {
+	status = "okay";
+};
+
+&r_pio {
+	vcc-pm-supply = <&reg_bldo3>;
+};
+
+&r_rsb {
 	status = "okay";
 
-	axp805: pmic@36 {
+	axp805: pmic@745 {
 		compatible = "x-powers,axp805", "x-powers,axp806";
-		reg = <0x36>;
+		reg = <0x745>;
 		interrupt-parent = <&r_intc>;
 		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
 		interrupt-controller;
@@ -248,14 +256,6 @@ sw {
 	};
 };
 
-&r_ir {
-	status = "okay";
-};
-
-&r_pio {
-	vcc-pm-supply = <&reg_bldo3>;
-};
-
 &rtc {
 	clocks = <&ext_osc32k>;
 };
-- 
2.26.2

Re: [PATCH 1/4] clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4141 bytes --]

Hi Samuel,

On Sun, Dec 13, 2020 at 05:55:03PM -0600, Samuel Holland wrote:
> While no information about the H6 RSB controller is included in the
> datasheet or manual, the vendor BSP and power management blob both
> reference the RSB clock parent and register address. These values were
> verified by experimentation.
> 
> Since this clock/reset are added late, the specifier is added at the end
> to maintain the existing DT binding. The code is kept in register order.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 5 +++++
>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
>  include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
>  include/dt-bindings/reset/sun50i-h6-r-ccu.h | 1 +
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> index 50f8d1bc7046..56e351b513f3 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> @@ -91,6 +91,8 @@ static SUNXI_CCU_GATE(r_apb2_uart_clk,	"r-apb2-uart",	"r-apb2",
>  		      0x18c, BIT(0), 0);
>  static SUNXI_CCU_GATE(r_apb2_i2c_clk,	"r-apb2-i2c",	"r-apb2",
>  		      0x19c, BIT(0), 0);
> +static SUNXI_CCU_GATE(r_apb2_rsb_clk,	"r-apb2-rsb",	"r-apb2",
> +		      0x1bc, BIT(0), 0);
>  static SUNXI_CCU_GATE(r_apb1_ir_clk,	"r-apb1-ir",	"r-apb1",
>  		      0x1cc, BIT(0), 0);
>  static SUNXI_CCU_GATE(r_apb1_w1_clk,	"r-apb1-w1",	"r-apb1",
> @@ -130,6 +132,7 @@ static struct ccu_common *sun50i_h6_r_ccu_clks[] = {
>  	&r_apb1_pwm_clk.common,
>  	&r_apb2_uart_clk.common,
>  	&r_apb2_i2c_clk.common,
> +	&r_apb2_rsb_clk.common,
>  	&r_apb1_ir_clk.common,
>  	&r_apb1_w1_clk.common,
>  	&ir_clk.common,
> @@ -147,6 +150,7 @@ static struct clk_hw_onecell_data sun50i_h6_r_hw_clks = {
>  		[CLK_R_APB1_PWM]	= &r_apb1_pwm_clk.common.hw,
>  		[CLK_R_APB2_UART]	= &r_apb2_uart_clk.common.hw,
>  		[CLK_R_APB2_I2C]	= &r_apb2_i2c_clk.common.hw,
> +		[CLK_R_APB2_RSB]	= &r_apb2_rsb_clk.common.hw,
>  		[CLK_R_APB1_IR]		= &r_apb1_ir_clk.common.hw,
>  		[CLK_R_APB1_W1]		= &r_apb1_w1_clk.common.hw,
>  		[CLK_IR]		= &ir_clk.common.hw,
> @@ -161,6 +165,7 @@ static struct ccu_reset_map sun50i_h6_r_ccu_resets[] = {
>  	[RST_R_APB1_PWM]	=  { 0x13c, BIT(16) },
>  	[RST_R_APB2_UART]	=  { 0x18c, BIT(16) },
>  	[RST_R_APB2_I2C]	=  { 0x19c, BIT(16) },
> +	[RST_R_APB2_RSB]	=  { 0x1bc, BIT(16) },
>  	[RST_R_APB1_IR]		=  { 0x1cc, BIT(16) },
>  	[RST_R_APB1_W1]		=  { 0x1ec, BIT(16) },
>  };
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
> index 782117dc0b28..7e290b840803 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
> @@ -14,6 +14,6 @@
>  
>  #define CLK_R_APB2	3
>  
> -#define CLK_NUMBER	(CLK_W1 + 1)
> +#define CLK_NUMBER	(CLK_R_APB2_RSB + 1)
>  
>  #endif /* _CCU_SUN50I_H6_R_H */
> diff --git a/include/dt-bindings/clock/sun50i-h6-r-ccu.h b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
> index 76136132a13e..f46ec03848ca 100644
> --- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
> +++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
> @@ -15,6 +15,7 @@
>  #define CLK_R_APB1_PWM		6
>  #define CLK_R_APB2_UART		7
>  #define CLK_R_APB2_I2C		8
> +#define CLK_R_APB2_RSB		13
>  #define CLK_R_APB1_IR		9
>  #define CLK_R_APB1_W1		10
>  
> diff --git a/include/dt-bindings/reset/sun50i-h6-r-ccu.h b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
> index 01c84dba49a4..6fe199a7969d 100644
> --- a/include/dt-bindings/reset/sun50i-h6-r-ccu.h
> +++ b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
> @@ -11,6 +11,7 @@
>  #define RST_R_APB1_PWM		2
>  #define RST_R_APB2_UART		3
>  #define RST_R_APB2_I2C		4
> +#define RST_R_APB2_RSB		7
>  #define RST_R_APB1_IR		5
>  #define RST_R_APB1_W1		6

I think for the clock and reset binding, we'll want to sort by number.
It's fairly easy to miss otherwise and if we end up adding another one
it wouldn't be far fetched to assume the same indices would be used

maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset

[Samuel Holland]

On 12/14/20 8:57 AM, Maxime Ripard wrote:
> Hi Samuel,
> 
> On Sun, Dec 13, 2020 at 05:55:03PM -0600, Samuel Holland wrote:
>> While no information about the H6 RSB controller is included in the
>> datasheet or manual, the vendor BSP and power management blob both
>> reference the RSB clock parent and register address. These values were
>> verified by experimentation.
>>
>> Since this clock/reset are added late, the specifier is added at the end
>> to maintain the existing DT binding. The code is kept in register order.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 5 +++++
>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
>>  include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
>>  include/dt-bindings/reset/sun50i-h6-r-ccu.h | 1 +
>>  4 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>> index 50f8d1bc7046..56e351b513f3 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>> @@ -91,6 +91,8 @@ static SUNXI_CCU_GATE(r_apb2_uart_clk,	"r-apb2-uart",	"r-apb2",
>>  		      0x18c, BIT(0), 0);
>>  static SUNXI_CCU_GATE(r_apb2_i2c_clk,	"r-apb2-i2c",	"r-apb2",
>>  		      0x19c, BIT(0), 0);
>> +static SUNXI_CCU_GATE(r_apb2_rsb_clk,	"r-apb2-rsb",	"r-apb2",
>> +		      0x1bc, BIT(0), 0);
>>  static SUNXI_CCU_GATE(r_apb1_ir_clk,	"r-apb1-ir",	"r-apb1",
>>  		      0x1cc, BIT(0), 0);
>>  static SUNXI_CCU_GATE(r_apb1_w1_clk,	"r-apb1-w1",	"r-apb1",
>> @@ -130,6 +132,7 @@ static struct ccu_common *sun50i_h6_r_ccu_clks[] = {
>>  	&r_apb1_pwm_clk.common,
>>  	&r_apb2_uart_clk.common,
>>  	&r_apb2_i2c_clk.common,
>> +	&r_apb2_rsb_clk.common,
>>  	&r_apb1_ir_clk.common,
>>  	&r_apb1_w1_clk.common,
>>  	&ir_clk.common,
>> @@ -147,6 +150,7 @@ static struct clk_hw_onecell_data sun50i_h6_r_hw_clks = {
>>  		[CLK_R_APB1_PWM]	= &r_apb1_pwm_clk.common.hw,
>>  		[CLK_R_APB2_UART]	= &r_apb2_uart_clk.common.hw,
>>  		[CLK_R_APB2_I2C]	= &r_apb2_i2c_clk.common.hw,
>> +		[CLK_R_APB2_RSB]	= &r_apb2_rsb_clk.common.hw,
>>  		[CLK_R_APB1_IR]		= &r_apb1_ir_clk.common.hw,
>>  		[CLK_R_APB1_W1]		= &r_apb1_w1_clk.common.hw,
>>  		[CLK_IR]		= &ir_clk.common.hw,
>> @@ -161,6 +165,7 @@ static struct ccu_reset_map sun50i_h6_r_ccu_resets[] = {
>>  	[RST_R_APB1_PWM]	=  { 0x13c, BIT(16) },
>>  	[RST_R_APB2_UART]	=  { 0x18c, BIT(16) },
>>  	[RST_R_APB2_I2C]	=  { 0x19c, BIT(16) },
>> +	[RST_R_APB2_RSB]	=  { 0x1bc, BIT(16) },
>>  	[RST_R_APB1_IR]		=  { 0x1cc, BIT(16) },
>>  	[RST_R_APB1_W1]		=  { 0x1ec, BIT(16) },
>>  };
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>> index 782117dc0b28..7e290b840803 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>> @@ -14,6 +14,6 @@
>>  
>>  #define CLK_R_APB2	3
>>  
>> -#define CLK_NUMBER	(CLK_W1 + 1)
>> +#define CLK_NUMBER	(CLK_R_APB2_RSB + 1)
>>  
>>  #endif /* _CCU_SUN50I_H6_R_H */
>> diff --git a/include/dt-bindings/clock/sun50i-h6-r-ccu.h b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>> index 76136132a13e..f46ec03848ca 100644
>> --- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>> +++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>> @@ -15,6 +15,7 @@
>>  #define CLK_R_APB1_PWM		6
>>  #define CLK_R_APB2_UART		7
>>  #define CLK_R_APB2_I2C		8
>> +#define CLK_R_APB2_RSB		13
>>  #define CLK_R_APB1_IR		9
>>  #define CLK_R_APB1_W1		10
>>  
>> diff --git a/include/dt-bindings/reset/sun50i-h6-r-ccu.h b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>> index 01c84dba49a4..6fe199a7969d 100644
>> --- a/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>> +++ b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>> @@ -11,6 +11,7 @@
>>  #define RST_R_APB1_PWM		2
>>  #define RST_R_APB2_UART		3
>>  #define RST_R_APB2_I2C		4
>> +#define RST_R_APB2_RSB		7
>>  #define RST_R_APB1_IR		5
>>  #define RST_R_APB1_W1		6
> 
> I think for the clock and reset binding, we'll want to sort by number.
> It's fairly easy to miss otherwise and if we end up adding another one
> it wouldn't be far fetched to assume the same indices would be used

I think GCC would complain about the duplicate array initialization in
the driver, but I can move them for v2.

Cheers,
Samuel

Re: [PATCH 1/4] clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset

[André Przywara]

On 15/12/2020 03:25, Samuel Holland wrote:
> On 12/14/20 8:57 AM, Maxime Ripard wrote:
>> Hi Samuel,
>>
>> On Sun, Dec 13, 2020 at 05:55:03PM -0600, Samuel Holland wrote:
>>> While no information about the H6 RSB controller is included in the
>>> datasheet or manual, the vendor BSP and power management blob both
>>> reference the RSB clock parent and register address. These values were
>>> verified by experimentation.
>>>
>>> Since this clock/reset are added late, the specifier is added at the end
>>> to maintain the existing DT binding. The code is kept in register order.
>>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>> ---
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 5 +++++
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
>>>  include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
>>>  include/dt-bindings/reset/sun50i-h6-r-ccu.h | 1 +
>>>  4 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> index 50f8d1bc7046..56e351b513f3 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> @@ -91,6 +91,8 @@ static SUNXI_CCU_GATE(r_apb2_uart_clk,	"r-apb2-uart",	"r-apb2",
>>>  		      0x18c, BIT(0), 0);
>>>  static SUNXI_CCU_GATE(r_apb2_i2c_clk,	"r-apb2-i2c",	"r-apb2",
>>>  		      0x19c, BIT(0), 0);
>>> +static SUNXI_CCU_GATE(r_apb2_rsb_clk,	"r-apb2-rsb",	"r-apb2",
>>> +		      0x1bc, BIT(0), 0);
>>>  static SUNXI_CCU_GATE(r_apb1_ir_clk,	"r-apb1-ir",	"r-apb1",
>>>  		      0x1cc, BIT(0), 0);
>>>  static SUNXI_CCU_GATE(r_apb1_w1_clk,	"r-apb1-w1",	"r-apb1",
>>> @@ -130,6 +132,7 @@ static struct ccu_common *sun50i_h6_r_ccu_clks[] = {
>>>  	&r_apb1_pwm_clk.common,
>>>  	&r_apb2_uart_clk.common,
>>>  	&r_apb2_i2c_clk.common,
>>> +	&r_apb2_rsb_clk.common,
>>>  	&r_apb1_ir_clk.common,
>>>  	&r_apb1_w1_clk.common,
>>>  	&ir_clk.common,
>>> @@ -147,6 +150,7 @@ static struct clk_hw_onecell_data sun50i_h6_r_hw_clks = {
>>>  		[CLK_R_APB1_PWM]	= &r_apb1_pwm_clk.common.hw,
>>>  		[CLK_R_APB2_UART]	= &r_apb2_uart_clk.common.hw,
>>>  		[CLK_R_APB2_I2C]	= &r_apb2_i2c_clk.common.hw,
>>> +		[CLK_R_APB2_RSB]	= &r_apb2_rsb_clk.common.hw,
>>>  		[CLK_R_APB1_IR]		= &r_apb1_ir_clk.common.hw,
>>>  		[CLK_R_APB1_W1]		= &r_apb1_w1_clk.common.hw,
>>>  		[CLK_IR]		= &ir_clk.common.hw,
>>> @@ -161,6 +165,7 @@ static struct ccu_reset_map sun50i_h6_r_ccu_resets[] = {
>>>  	[RST_R_APB1_PWM]	=  { 0x13c, BIT(16) },
>>>  	[RST_R_APB2_UART]	=  { 0x18c, BIT(16) },
>>>  	[RST_R_APB2_I2C]	=  { 0x19c, BIT(16) },
>>> +	[RST_R_APB2_RSB]	=  { 0x1bc, BIT(16) },
>>>  	[RST_R_APB1_IR]		=  { 0x1cc, BIT(16) },
>>>  	[RST_R_APB1_W1]		=  { 0x1ec, BIT(16) },
>>>  };
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> index 782117dc0b28..7e290b840803 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> @@ -14,6 +14,6 @@
>>>  
>>>  #define CLK_R_APB2	3
>>>  
>>> -#define CLK_NUMBER	(CLK_W1 + 1)
>>> +#define CLK_NUMBER	(CLK_R_APB2_RSB + 1)
>>>  
>>>  #endif /* _CCU_SUN50I_H6_R_H */
>>> diff --git a/include/dt-bindings/clock/sun50i-h6-r-ccu.h b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> index 76136132a13e..f46ec03848ca 100644
>>> --- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> +++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> @@ -15,6 +15,7 @@
>>>  #define CLK_R_APB1_PWM		6
>>>  #define CLK_R_APB2_UART		7
>>>  #define CLK_R_APB2_I2C		8
>>> +#define CLK_R_APB2_RSB		13
>>>  #define CLK_R_APB1_IR		9
>>>  #define CLK_R_APB1_W1		10
>>>  
>>> diff --git a/include/dt-bindings/reset/sun50i-h6-r-ccu.h b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> index 01c84dba49a4..6fe199a7969d 100644
>>> --- a/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> +++ b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> @@ -11,6 +11,7 @@
>>>  #define RST_R_APB1_PWM		2
>>>  #define RST_R_APB2_UART		3
>>>  #define RST_R_APB2_I2C		4
>>> +#define RST_R_APB2_RSB		7
>>>  #define RST_R_APB1_IR		5
>>>  #define RST_R_APB1_W1		6
>>
>> I think for the clock and reset binding, we'll want to sort by number.
>> It's fairly easy to miss otherwise and if we end up adding another one
>> it wouldn't be far fetched to assume the same indices would be used

I agree here. Admittedly this whole approach is really fragile. I ended
up with some tiny tool to check for consecutive numbers, reporting any
outliers (some gaps are legit). Of course the "-r" versions of the CCU
are not really a big issue, but with the 100+ clocks in the main CCU
this is a problem. Also it becomes ABI, so is hard to fix.

I guess there is no nice kernel CPP hack to re-use enums as preprocessor
symbols?

> I think GCC would complain about the duplicate array initialization in
> the driver, but I can move them for v2.

It doesn't with -Wall, you need -Wextra for that. I actually had a
subtle bug in my H616 CCU patch due to a double numbering.

Cheers,
Andre