[PATCH 0/2] Add Anbernic RG35XX-SP

[PATCH 0/2] Add Anbernic RG35XX-SP

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add support for the Anbernic RG35XX-SP handheld gaming device. The
Anbernic RG35XX-SP is a clamshell device, but hardware wise is very
similar to the RG35XX-Plus. The RG35XX-SP has a lid-switch and an
external RTC that necessitate a distinct device tree.

Chris Morgan (2):
  dt-bindings: arm: sunxi: Add Anbernic RG35XXSP
  arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

 .../devicetree/bindings/arm/sunxi.yaml        |  24 ++-
 arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
 .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
 3 files changed, 156 insertions(+), 16 deletions(-)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts

-- 
2.34.1

[PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
H700 devices.

The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
clamshell form-factor.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 .../devicetree/bindings/arm/sunxi.yaml        | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
index c2a158b75e49..1ae77e5edf9a 100644
--- a/Documentation/devicetree/bindings/arm/sunxi.yaml
+++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
@@ -51,25 +51,19 @@ properties:
           - const: allwinner,parrot
           - const: allwinner,sun8i-a33
 
-      - description: Anbernic RG-Nano
-        items:
-          - const: anbernic,rg-nano
-          - const: allwinner,sun8i-v3s
-
-      - description: Anbernic RG35XX (2024)
-        items:
-          - const: anbernic,rg35xx-2024
-          - const: allwinner,sun50i-h700
-
-      - description: Anbernic RG35XX Plus
+      - description: Anbernic H700 Handheld Gaming Console
         items:
-          - const: anbernic,rg35xx-plus
+          - enum:
+              - anbernic,rg35xx-2024
+              - anbernic,rg35xx-h
+              - anbernic,rg35xx-plus
+              - anbernic,rg35xx-sp
           - const: allwinner,sun50i-h700
 
-      - description: Anbernic RG35XX H
+      - description: Anbernic RG-Nano
         items:
-          - const: anbernic,rg35xx-h
-          - const: allwinner,sun50i-h700
+          - const: anbernic,rg-nano
+          - const: allwinner,sun8i-v3s
 
       - description: Amarula A64 Relic
         items:
-- 
2.34.1

[PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
clamshell form-factor. The key differences between the SP and the Plus
is a lid switch and an RTC on the same i2c bus as the PMIC.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
 .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
 2 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts

diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 0db7b60b49a1..00bed412ee31 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
 dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
 dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
 dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
-dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
 dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
+dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
+dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
new file mode 100644
index 000000000000..a1d16b65ef5d
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
+ * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
+ */
+
+#include <dt-bindings/input/gpio-keys.h>
+#include "sun50i-h700-anbernic-rg35xx-plus.dts"
+
+/ {
+	model = "Anbernic RG35XX SP";
+	compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
+
+	gpio-keys-lid {
+		compatible = "gpio-keys";
+
+		lid-switch {
+			label = "Lid Switch";
+			gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
+			linux,can-disable;
+			linux,code = <SW_LID>;
+			linux,input-type = <EV_SW>;
+			wakeup-event-action = <EV_ACT_DEASSERTED>;
+			wakeup-source;
+		};
+	};
+};
+
+/delete-node/ &r_rsb;
+
+&r_i2c {
+	pinctrl-0 = <&r_i2c_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+
+	axp717: pmic@34 {
+		compatible = "x-powers,axp717";
+		reg = <0x34>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupt-parent = <&nmi_intc>;
+		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+
+		vin1-supply = <&reg_vcc5v>;
+		vin2-supply = <&reg_vcc5v>;
+		vin3-supply = <&reg_vcc5v>;
+		vin4-supply = <&reg_vcc5v>;
+
+		regulators {
+			reg_dcdc1: dcdc1 {
+				regulator-always-on;
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-cpu";
+			};
+
+			reg_dcdc2: dcdc2 {
+				regulator-always-on;
+				regulator-min-microvolt = <940000>;
+				regulator-max-microvolt = <940000>;
+				regulator-name = "vdd-gpu-sys";
+			};
+
+			reg_dcdc3: dcdc3 {
+				regulator-always-on;
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-dram";
+			};
+
+			reg_aldo1: aldo1 {
+				/* 1.8v - unused */
+			};
+
+			reg_aldo2: aldo2 {
+				/* 1.8v - unused */
+			};
+
+			reg_aldo3: aldo3 {
+				/* 1.8v - unused */
+			};
+
+			reg_aldo4: aldo4 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc-pg";
+			};
+
+			reg_bldo1: bldo1 {
+				/* 1.8v - unused */
+			};
+
+			reg_bldo2: bldo2 {
+				regulator-always-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc-pll";
+			};
+
+			reg_bldo3: bldo3 {
+				/* 2.8v - unused */
+			};
+
+			reg_bldo4: bldo4 {
+				/* 1.2v - unused */
+			};
+
+			reg_cldo1: cldo1 {
+				/* 3.3v - audio codec - not yet implemented */
+			};
+
+			reg_cldo2: cldo2 {
+				/* 3.3v - unused */
+			};
+
+			reg_cldo3: cldo3 {
+				regulator-always-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-io";
+			};
+
+			reg_cldo4: cldo4 {
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-wifi";
+			};
+
+			reg_boost: boost {
+				regulator-min-microvolt = <5000000>;
+				regulator-max-microvolt = <5200000>;
+				regulator-name = "boost";
+			};
+
+			reg_cpusldo: cpusldo {
+				/* unused */
+			};
+		};
+	};
+
+	rtc_ext: rtc@51 {
+		compatible = "nxp,pcf8563";
+		reg = <0x51>;
+	};
+};
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Krzysztof Kozlowski]

On 05/06/2024 20:53, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
> H700 devices.
> 
> The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> clamshell form-factor.

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

Best regards,
Krzysztof

Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Andre Przywara]

On Wed,  5 Jun 2024 13:53:39 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> clamshell form-factor. The key differences between the SP and the Plus
> is a lid switch and an RTC on the same i2c bus as the PMIC.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
>  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
>  2 files changed, 147 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 0db7b60b49a1..00bed412ee31 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> new file mode 100644
> index 000000000000..a1d16b65ef5d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> + */
> +
> +#include <dt-bindings/input/gpio-keys.h>
> +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> +
> +/ {
> +	model = "Anbernic RG35XX SP";
> +	compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> +
> +	gpio-keys-lid {
> +		compatible = "gpio-keys";
> +
> +		lid-switch {
> +			label = "Lid Switch";
> +			gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> +			linux,can-disable;
> +			linux,code = <SW_LID>;
> +			linux,input-type = <EV_SW>;
> +			wakeup-event-action = <EV_ACT_DEASSERTED>;
> +			wakeup-source;
> +		};
> +	};
> +};
> +
> +/delete-node/ &r_rsb;

I don't think deleting the *RSB* node is right here, if at all, I'd expect
status="disabled", and then maybe deleting the axp717 node within?
Or maybe factor the AXP node out into a separate file, and include it from
the -2024.dts and from this one?
Or move every board to I2C?
What do people think about this?

Cheers,
Andre

> +
> +&r_i2c {
> +	pinctrl-0 = <&r_i2c_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +
> +	axp717: pmic@34 {
> +		compatible = "x-powers,axp717";
> +		reg = <0x34>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupt-parent = <&nmi_intc>;
> +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +
> +		vin1-supply = <&reg_vcc5v>;
> +		vin2-supply = <&reg_vcc5v>;
> +		vin3-supply = <&reg_vcc5v>;
> +		vin4-supply = <&reg_vcc5v>;
> +
> +		regulators {
> +			reg_dcdc1: dcdc1 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd-cpu";
> +			};
> +
> +			reg_dcdc2: dcdc2 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <940000>;
> +				regulator-max-microvolt = <940000>;
> +				regulator-name = "vdd-gpu-sys";
> +			};
> +
> +			reg_dcdc3: dcdc3 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <1100000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd-dram";
> +			};
> +
> +			reg_aldo1: aldo1 {
> +				/* 1.8v - unused */
> +			};
> +
> +			reg_aldo2: aldo2 {
> +				/* 1.8v - unused */
> +			};
> +
> +			reg_aldo3: aldo3 {
> +				/* 1.8v - unused */
> +			};
> +
> +			reg_aldo4: aldo4 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-name = "vcc-pg";
> +			};
> +
> +			reg_bldo1: bldo1 {
> +				/* 1.8v - unused */
> +			};
> +
> +			reg_bldo2: bldo2 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-name = "vcc-pll";
> +			};
> +
> +			reg_bldo3: bldo3 {
> +				/* 2.8v - unused */
> +			};
> +
> +			reg_bldo4: bldo4 {
> +				/* 1.2v - unused */
> +			};
> +
> +			reg_cldo1: cldo1 {
> +				/* 3.3v - audio codec - not yet implemented */
> +			};
> +
> +			reg_cldo2: cldo2 {
> +				/* 3.3v - unused */
> +			};
> +
> +			reg_cldo3: cldo3 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-io";
> +			};
> +
> +			reg_cldo4: cldo4 {
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-wifi";
> +			};
> +
> +			reg_boost: boost {
> +				regulator-min-microvolt = <5000000>;
> +				regulator-max-microvolt = <5200000>;
> +				regulator-name = "boost";
> +			};
> +
> +			reg_cpusldo: cpusldo {
> +				/* unused */
> +			};
> +		};
> +	};
> +
> +	rtc_ext: rtc@51 {
> +		compatible = "nxp,pcf8563";
> +		reg = <0x51>;
> +	};
> +};

Re: [PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Andre Przywara]

On Wed,  5 Jun 2024 13:53:38 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

Hi,

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
> H700 devices.
> 
> The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> clamshell form-factor.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  .../devicetree/bindings/arm/sunxi.yaml        | 24 +++++++------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> index c2a158b75e49..1ae77e5edf9a 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> @@ -51,25 +51,19 @@ properties:
>            - const: allwinner,parrot
>            - const: allwinner,sun8i-a33
>  
> -      - description: Anbernic RG-Nano
> -        items:
> -          - const: anbernic,rg-nano
> -          - const: allwinner,sun8i-v3s
> -
> -      - description: Anbernic RG35XX (2024)
> -        items:
> -          - const: anbernic,rg35xx-2024
> -          - const: allwinner,sun50i-h700
> -
> -      - description: Anbernic RG35XX Plus
> +      - description: Anbernic H700 Handheld Gaming Console

So that's certainly an interesting optimisation, but so far we were using
one entry per device, it seems.
I am not entirely sure what the purpose of this file is, exactly: just to
document the compatible names, to reserve them and avoid clashes in the
future? Or also to put some official names to each device? That seems to
somewhat overlap with the root node's model property in the respective
device .dts, though.

It would be good to clarify this, and establish how to group those devices.
I mean technically we could for instance put *all* H6 devices into one
entry, using the same scheme as below.
Not sure that's desired, though.

Cheers,
Andre

>          items:
> -          - const: anbernic,rg35xx-plus
> +          - enum:
> +              - anbernic,rg35xx-2024
> +              - anbernic,rg35xx-h
> +              - anbernic,rg35xx-plus
> +              - anbernic,rg35xx-sp
>            - const: allwinner,sun50i-h700
>  
> -      - description: Anbernic RG35XX H
> +      - description: Anbernic RG-Nano
>          items:
> -          - const: anbernic,rg35xx-h
> -          - const: allwinner,sun50i-h700
> +          - const: anbernic,rg-nano
> +          - const: allwinner,sun8i-v3s
>  
>        - description: Amarula A64 Relic
>          items:

Re: [PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Maxime Ripard]

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

Hi,

On Thu, Jun 06, 2024 at 11:26:14AM GMT, Andre Przywara wrote:
> On Wed,  5 Jun 2024 13:53:38 -0500
> Chris Morgan <macroalpha82@gmail.com> wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
> > H700 devices.
> > 
> > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > clamshell form-factor.
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >  .../devicetree/bindings/arm/sunxi.yaml        | 24 +++++++------------
> >  1 file changed, 9 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > index c2a158b75e49..1ae77e5edf9a 100644
> > --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> > +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > @@ -51,25 +51,19 @@ properties:
> >            - const: allwinner,parrot
> >            - const: allwinner,sun8i-a33
> >  
> > -      - description: Anbernic RG-Nano
> > -        items:
> > -          - const: anbernic,rg-nano
> > -          - const: allwinner,sun8i-v3s
> > -
> > -      - description: Anbernic RG35XX (2024)
> > -        items:
> > -          - const: anbernic,rg35xx-2024
> > -          - const: allwinner,sun50i-h700
> > -
> > -      - description: Anbernic RG35XX Plus
> > +      - description: Anbernic H700 Handheld Gaming Console
> 
> So that's certainly an interesting optimisation, but so far we were using
> one entry per device, it seems.
> I am not entirely sure what the purpose of this file is, exactly: just to
> document the compatible names

That was the initial intent, yes.

> to reserve them and avoid clashes in the future?

And I guess it helps with that too :)

> Or also to put some official names to each device? That seems to
> somewhat overlap with the root node's model property in the respective
> device .dts, though.

I guess it's a fair criticism. It would be hard to collect all the
compatibles without describing which device they belong too though. So
yeah, there's some redundancy, but removing the descriptions entirely
would be worse imo.

> It would be good to clarify this, and establish how to group those devices.
> I mean technically we could for instance put *all* H6 devices into one
> entry, using the same scheme as below.
> Not sure that's desired, though.

I don't really have a say there anymore, but I always tend to prefer
consistency in documentation as a user. Even more so since that kind of
categorization tends to be very subjective and thus super inconsistent.

Maxime

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

Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Chen-Yu Tsai]

On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Wed,  5 Jun 2024 13:53:39 -0500
> Chris Morgan <macroalpha82@gmail.com> wrote:
>
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > clamshell form-factor. The key differences between the SP and the Plus
> > is a lid switch and an RTC on the same i2c bus as the PMIC.
> >
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> >  2 files changed, 147 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > index 0db7b60b49a1..00bed412ee31 100644
> > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > new file mode 100644
> > index 000000000000..a1d16b65ef5d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > @@ -0,0 +1,145 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +/*
> > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> > + */
> > +
> > +#include <dt-bindings/input/gpio-keys.h>
> > +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> > +
> > +/ {
> > +     model = "Anbernic RG35XX SP";
> > +     compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> > +
> > +     gpio-keys-lid {
> > +             compatible = "gpio-keys";
> > +
> > +             lid-switch {
> > +                     label = "Lid Switch";
> > +                     gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> > +                     linux,can-disable;
> > +                     linux,code = <SW_LID>;
> > +                     linux,input-type = <EV_SW>;
> > +                     wakeup-event-action = <EV_ACT_DEASSERTED>;
> > +                     wakeup-source;
> > +             };
> > +     };
> > +};
> > +
> > +/delete-node/ &r_rsb;
>
> I don't think deleting the *RSB* node is right here, if at all, I'd expect
> status="disabled", and then maybe deleting the axp717 node within?
> Or maybe factor the AXP node out into a separate file, and include it from
> the -2024.dts and from this one?

Doesn't quite work because the unit address is different.

> Or move every board to I2C?

Doesn't this depend on the bootloader also running in RSB mode? My memory
is a bit foggy on this, but IIRC we did a conversion on some other boards
before?

> What do people think about this?

"disabled" in RSB node and deleting the axp717 node is probably the right
thing to do.


> Cheers,
> Andre
>
> > +
> > +&r_i2c {
> > +     pinctrl-0 = <&r_i2c_pins>;
> > +     pinctrl-names = "default";
> > +     status = "okay";
> > +
> > +     axp717: pmic@34 {
> > +             compatible = "x-powers,axp717";
> > +             reg = <0x34>;
> > +             interrupt-controller;
> > +             #interrupt-cells = <1>;
> > +             interrupt-parent = <&nmi_intc>;
> > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +             vin1-supply = <&reg_vcc5v>;
> > +             vin2-supply = <&reg_vcc5v>;
> > +             vin3-supply = <&reg_vcc5v>;
> > +             vin4-supply = <&reg_vcc5v>;
> > +
> > +             regulators {
> > +                     reg_dcdc1: dcdc1 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <900000>;
> > +                             regulator-max-microvolt = <1100000>;
> > +                             regulator-name = "vdd-cpu";
> > +                     };
> > +
> > +                     reg_dcdc2: dcdc2 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <940000>;
> > +                             regulator-max-microvolt = <940000>;
> > +                             regulator-name = "vdd-gpu-sys";
> > +                     };
> > +
> > +                     reg_dcdc3: dcdc3 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <1100000>;
> > +                             regulator-max-microvolt = <1100000>;
> > +                             regulator-name = "vdd-dram";
> > +                     };
> > +
> > +                     reg_aldo1: aldo1 {
> > +                             /* 1.8v - unused */
> > +                     };

You can drop all the unused ones, unless you plan to include *this*
file from another one and use those nodes/labels.

ChenYu

> > +
> > +                     reg_aldo2: aldo2 {
> > +                             /* 1.8v - unused */
> > +                     };
> > +
> > +                     reg_aldo3: aldo3 {
> > +                             /* 1.8v - unused */
> > +                     };
> > +
> > +                     reg_aldo4: aldo4 {
> > +                             regulator-min-microvolt = <1800000>;
> > +                             regulator-max-microvolt = <1800000>;
> > +                             regulator-name = "vcc-pg";
> > +                     };
> > +
> > +                     reg_bldo1: bldo1 {
> > +                             /* 1.8v - unused */
> > +                     };
> > +
> > +                     reg_bldo2: bldo2 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <1800000>;
> > +                             regulator-max-microvolt = <1800000>;
> > +                             regulator-name = "vcc-pll";
> > +                     };
> > +
> > +                     reg_bldo3: bldo3 {
> > +                             /* 2.8v - unused */
> > +                     };
> > +
> > +                     reg_bldo4: bldo4 {
> > +                             /* 1.2v - unused */
> > +                     };
> > +
> > +                     reg_cldo1: cldo1 {
> > +                             /* 3.3v - audio codec - not yet implemented */
> > +                     };
> > +
> > +                     reg_cldo2: cldo2 {
> > +                             /* 3.3v - unused */
> > +                     };
> > +
> > +                     reg_cldo3: cldo3 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <3300000>;
> > +                             regulator-max-microvolt = <3300000>;
> > +                             regulator-name = "vcc-io";
> > +                     };
> > +
> > +                     reg_cldo4: cldo4 {
> > +                             regulator-min-microvolt = <3300000>;
> > +                             regulator-max-microvolt = <3300000>;
> > +                             regulator-name = "vcc-wifi";
> > +                     };
> > +
> > +                     reg_boost: boost {
> > +                             regulator-min-microvolt = <5000000>;
> > +                             regulator-max-microvolt = <5200000>;
> > +                             regulator-name = "boost";
> > +                     };
> > +
> > +                     reg_cpusldo: cpusldo {
> > +                             /* unused */
> > +                     };
> > +             };
> > +     };
> > +
> > +     rtc_ext: rtc@51 {
> > +             compatible = "nxp,pcf8563";
> > +             reg = <0x51>;
> > +     };
> > +};
>
>

Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Andre Przywara]

On Thu, 6 Jun 2024 19:58:07 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi Chen-Yu,

> On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Wed,  5 Jun 2024 13:53:39 -0500
> > Chris Morgan <macroalpha82@gmail.com> wrote:
> >  
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > >
> > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > clamshell form-factor. The key differences between the SP and the Plus
> > > is a lid switch and an RTC on the same i2c bus as the PMIC.
> > >
> > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > ---
> > >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> > >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> > >  2 files changed, 147 insertions(+), 1 deletion(-)
> > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > index 0db7b60b49a1..00bed412ee31 100644
> > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > > -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > new file mode 100644
> > > index 000000000000..a1d16b65ef5d
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > @@ -0,0 +1,145 @@
> > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +/*
> > > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > > + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> > > + */
> > > +
> > > +#include <dt-bindings/input/gpio-keys.h>
> > > +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> > > +
> > > +/ {
> > > +     model = "Anbernic RG35XX SP";
> > > +     compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> > > +
> > > +     gpio-keys-lid {
> > > +             compatible = "gpio-keys";
> > > +
> > > +             lid-switch {
> > > +                     label = "Lid Switch";
> > > +                     gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> > > +                     linux,can-disable;
> > > +                     linux,code = <SW_LID>;
> > > +                     linux,input-type = <EV_SW>;
> > > +                     wakeup-event-action = <EV_ACT_DEASSERTED>;
> > > +                     wakeup-source;
> > > +             };
> > > +     };
> > > +};
> > > +
> > > +/delete-node/ &r_rsb;  
> >
> > I don't think deleting the *RSB* node is right here, if at all, I'd expect
> > status="disabled", and then maybe deleting the axp717 node within?
> > Or maybe factor the AXP node out into a separate file, and include it from
> > the -2024.dts and from this one?  
> 
> Doesn't quite work because the unit address is different.

Ah, right, good point. It's a bit annoying because the node name itself is
mostly irrelevant, but I see that this wouldn't pass validation.

> > Or move every board to I2C?  
> 
> Doesn't this depend on the bootloader also running in RSB mode? My memory
> is a bit foggy on this, but IIRC we did a conversion on some other boards
> before?

In the SPL we use I2C only, mostly because we have an SPL capable I2C
driver already.
In U-Boot proper we use whatever the DT says, that should work like in the
kernel.
In TF-A we used to have one hardcoded transport per SoC, and that happens
to be RSB everywhere at the moment, but I have a patch series [1] to
determine this dynamically, via the DT. As it stands, that chooses the
transport by looking at the PMIC (I2C for the AXP313, RSB for the AXP717
or AXP305), but I think it's fairly easy to check for the status property
of both RSB and I2C, or look at the parent node of the AXP node to find
the transport protocol to use.
I will sketch something up.

Chris has plans to auto-detect the exact Anbernic model in U-Boot, and
switch to the right DT then automatically. IIUC I2C devices play a role in
this, so switching to I2C for all Anbernic models would make this more
viable.
It just leaves a bit of a bitter taste that we now model the DT after this
particular requirement, and not after what the hardware looks like.

Cheers,
Andre.

[1] https://review.trustedfirmware.org/q/topic:%22h616_pmics%22

> > What do people think about this?  
> 
> "disabled" in RSB node and deleting the axp717 node is probably the right
> thing to do.
> 
> 
> > Cheers,
> > Andre
> >  
> > > +
> > > +&r_i2c {
> > > +     pinctrl-0 = <&r_i2c_pins>;
> > > +     pinctrl-names = "default";
> > > +     status = "okay";
> > > +
> > > +     axp717: pmic@34 {
> > > +             compatible = "x-powers,axp717";
> > > +             reg = <0x34>;
> > > +             interrupt-controller;
> > > +             #interrupt-cells = <1>;
> > > +             interrupt-parent = <&nmi_intc>;
> > > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > +
> > > +             vin1-supply = <&reg_vcc5v>;
> > > +             vin2-supply = <&reg_vcc5v>;
> > > +             vin3-supply = <&reg_vcc5v>;
> > > +             vin4-supply = <&reg_vcc5v>;
> > > +
> > > +             regulators {
> > > +                     reg_dcdc1: dcdc1 {
> > > +                             regulator-always-on;
> > > +                             regulator-min-microvolt = <900000>;
> > > +                             regulator-max-microvolt = <1100000>;
> > > +                             regulator-name = "vdd-cpu";
> > > +                     };
> > > +
> > > +                     reg_dcdc2: dcdc2 {
> > > +                             regulator-always-on;
> > > +                             regulator-min-microvolt = <940000>;
> > > +                             regulator-max-microvolt = <940000>;
> > > +                             regulator-name = "vdd-gpu-sys";
> > > +                     };
> > > +
> > > +                     reg_dcdc3: dcdc3 {
> > > +                             regulator-always-on;
> > > +                             regulator-min-microvolt = <1100000>;
> > > +                             regulator-max-microvolt = <1100000>;
> > > +                             regulator-name = "vdd-dram";
> > > +                     };
> > > +
> > > +                     reg_aldo1: aldo1 {
> > > +                             /* 1.8v - unused */
> > > +                     };  
> 
> You can drop all the unused ones, unless you plan to include *this*
> file from another one and use those nodes/labels.
> 
> ChenYu
> 
> > > +
> > > +                     reg_aldo2: aldo2 {
> > > +                             /* 1.8v - unused */
> > > +                     };
> > > +
> > > +                     reg_aldo3: aldo3 {
> > > +                             /* 1.8v - unused */
> > > +                     };
> > > +
> > > +                     reg_aldo4: aldo4 {
> > > +                             regulator-min-microvolt = <1800000>;
> > > +                             regulator-max-microvolt = <1800000>;
> > > +                             regulator-name = "vcc-pg";
> > > +                     };
> > > +
> > > +                     reg_bldo1: bldo1 {
> > > +                             /* 1.8v - unused */
> > > +                     };
> > > +
> > > +                     reg_bldo2: bldo2 {
> > > +                             regulator-always-on;
> > > +                             regulator-min-microvolt = <1800000>;
> > > +                             regulator-max-microvolt = <1800000>;
> > > +                             regulator-name = "vcc-pll";
> > > +                     };
> > > +
> > > +                     reg_bldo3: bldo3 {
> > > +                             /* 2.8v - unused */
> > > +                     };
> > > +
> > > +                     reg_bldo4: bldo4 {
> > > +                             /* 1.2v - unused */
> > > +                     };
> > > +
> > > +                     reg_cldo1: cldo1 {
> > > +                             /* 3.3v - audio codec - not yet implemented */
> > > +                     };
> > > +
> > > +                     reg_cldo2: cldo2 {
> > > +                             /* 3.3v - unused */
> > > +                     };
> > > +
> > > +                     reg_cldo3: cldo3 {
> > > +                             regulator-always-on;
> > > +                             regulator-min-microvolt = <3300000>;
> > > +                             regulator-max-microvolt = <3300000>;
> > > +                             regulator-name = "vcc-io";
> > > +                     };
> > > +
> > > +                     reg_cldo4: cldo4 {
> > > +                             regulator-min-microvolt = <3300000>;
> > > +                             regulator-max-microvolt = <3300000>;
> > > +                             regulator-name = "vcc-wifi";
> > > +                     };
> > > +
> > > +                     reg_boost: boost {
> > > +                             regulator-min-microvolt = <5000000>;
> > > +                             regulator-max-microvolt = <5200000>;
> > > +                             regulator-name = "boost";
> > > +                     };
> > > +
> > > +                     reg_cpusldo: cpusldo {
> > > +                             /* unused */
> > > +                     };
> > > +             };
> > > +     };
> > > +
> > > +     rtc_ext: rtc@51 {
> > > +             compatible = "nxp,pcf8563";
> > > +             reg = <0x51>;
> > > +     };
> > > +};  
> >
> >  

Re: [PATCH 0/2] Add Anbernic RG35XX-SP

[Rob Herring (Arm)]

On Wed, 05 Jun 2024 13:53:37 -0500, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support for the Anbernic RG35XX-SP handheld gaming device. The
> Anbernic RG35XX-SP is a clamshell device, but hardware wise is very
> similar to the RG35XX-Plus. The RG35XX-SP has a lid-switch and an
> external RTC that necessitate a distinct device tree.
> 
> Chris Morgan (2):
>   dt-bindings: arm: sunxi: Add Anbernic RG35XXSP
>   arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP
> 
>  .../devicetree/bindings/arm/sunxi.yaml        |  24 ++-
>  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
>  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
>  3 files changed, 156 insertions(+), 16 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> 
> --
> 2.34.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y allwinner/sun50i-h700-anbernic-rg35xx-sp.dtb' for 20240605185339.266833-1-macroalpha82@gmail.com:

arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dtb: pmic@34: regulators: 'boost' does not match any of the regexes: '^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/mfd/x-powers,axp152.yaml#

Re: [PATCH 0/2] Add Anbernic RG35XX-SP

[Chris Morgan]

On Thu, Jun 06, 2024 at 09:56:13AM -0600, Rob Herring (Arm) wrote:
> 
> On Wed, 05 Jun 2024 13:53:37 -0500, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add support for the Anbernic RG35XX-SP handheld gaming device. The
> > Anbernic RG35XX-SP is a clamshell device, but hardware wise is very
> > similar to the RG35XX-Plus. The RG35XX-SP has a lid-switch and an
> > external RTC that necessitate a distinct device tree.
> > 
> > Chris Morgan (2):
> >   dt-bindings: arm: sunxi: Add Anbernic RG35XXSP
> >   arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP
> > 
> >  .../devicetree/bindings/arm/sunxi.yaml        |  24 ++-
> >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> >  3 files changed, 156 insertions(+), 16 deletions(-)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > 
> > --
> > 2.34.1
> > 
> > 
> > 
> 
> 
> My bot found new DTB warnings on the .dts files added or changed in this
> series.
> 
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
> 
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
> 
>   pip3 install dtschema --upgrade

Sorry, while this error probably occurs on other upstream trees for the
moment, this commit should fix the error [1].

[1] https://lore.kernel.org/linux-sunxi/20240418000736.24338-4-andre.przywara@arm.com/

Thank you,
Chris.

> 
> 
> New warnings running 'make CHECK_DTBS=y allwinner/sun50i-h700-anbernic-rg35xx-sp.dtb' for 20240605185339.266833-1-macroalpha82@gmail.com:
> 
> arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dtb: pmic@34: regulators: 'boost' does not match any of the regexes: '^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$', 'pinctrl-[0-9]+'
> 	from schema $id: http://devicetree.org/schemas/mfd/x-powers,axp152.yaml#
> 
> 
> 
> 
> 

Re: [PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Chris Morgan]

On Thu, Jun 06, 2024 at 01:30:45PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Jun 06, 2024 at 11:26:14AM GMT, Andre Przywara wrote:
> > On Wed,  5 Jun 2024 13:53:38 -0500
> > Chris Morgan <macroalpha82@gmail.com> wrote:
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > > 
> > > Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
> > > H700 devices.
> > > 
> > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > clamshell form-factor.
> > > 
> > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > ---
> > >  .../devicetree/bindings/arm/sunxi.yaml        | 24 +++++++------------
> > >  1 file changed, 9 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > index c2a158b75e49..1ae77e5edf9a 100644
> > > --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > @@ -51,25 +51,19 @@ properties:
> > >            - const: allwinner,parrot
> > >            - const: allwinner,sun8i-a33
> > >  
> > > -      - description: Anbernic RG-Nano
> > > -        items:
> > > -          - const: anbernic,rg-nano
> > > -          - const: allwinner,sun8i-v3s
> > > -
> > > -      - description: Anbernic RG35XX (2024)
> > > -        items:
> > > -          - const: anbernic,rg35xx-2024
> > > -          - const: allwinner,sun50i-h700
> > > -
> > > -      - description: Anbernic RG35XX Plus
> > > +      - description: Anbernic H700 Handheld Gaming Console
> > 
> > So that's certainly an interesting optimisation, but so far we were using
> > one entry per device, it seems.
> > I am not entirely sure what the purpose of this file is, exactly: just to
> > document the compatible names
> 
> That was the initial intent, yes.
> 
> > to reserve them and avoid clashes in the future?
> 
> And I guess it helps with that too :)
> 
> > Or also to put some official names to each device? That seems to
> > somewhat overlap with the root node's model property in the respective
> > device .dts, though.
> 
> I guess it's a fair criticism. It would be hard to collect all the
> compatibles without describing which device they belong too though. So
> yeah, there's some redundancy, but removing the descriptions entirely
> would be worse imo.
> 
> > It would be good to clarify this, and establish how to group those devices.
> > I mean technically we could for instance put *all* H6 devices into one
> > entry, using the same scheme as below.
> > Not sure that's desired, though.
> 
> I don't really have a say there anymore, but I always tend to prefer
> consistency in documentation as a user. Even more so since that kind of
> categorization tends to be very subjective and thus super inconsistent.
> 
> Maxime

This is how I was asked to do it in the rockchip.yaml file [1], but I
know different teams have different style requests. Just let me know
what you prefer and I'll get it done that way.

[1] https://lore.kernel.org/all/20240123212111.202146-3-macroalpha82@gmail.com/

Thank you,
Chris.

Re: [PATCH 1/2] dt-bindings: arm: sunxi: Add Anbernic RG35XXSP

[Chen-Yu Tsai]

On Fri, Jun 7, 2024 at 12:55 AM Chris Morgan <macromorgan@hotmail.com> wrote:
>
> On Thu, Jun 06, 2024 at 01:30:45PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Thu, Jun 06, 2024 at 11:26:14AM GMT, Andre Przywara wrote:
> > > On Wed,  5 Jun 2024 13:53:38 -0500
> > > Chris Morgan <macroalpha82@gmail.com> wrote:
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > >
> > > > Add the Anbernic RG35XXSP variant device and consolidate the Anbernic
> > > > H700 devices.
> > > >
> > > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > > clamshell form-factor.
> > > >
> > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > > ---
> > > >  .../devicetree/bindings/arm/sunxi.yaml        | 24 +++++++------------
> > > >  1 file changed, 9 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > > index c2a158b75e49..1ae77e5edf9a 100644
> > > > --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> > > > @@ -51,25 +51,19 @@ properties:
> > > >            - const: allwinner,parrot
> > > >            - const: allwinner,sun8i-a33
> > > >
> > > > -      - description: Anbernic RG-Nano
> > > > -        items:
> > > > -          - const: anbernic,rg-nano
> > > > -          - const: allwinner,sun8i-v3s
> > > > -
> > > > -      - description: Anbernic RG35XX (2024)
> > > > -        items:
> > > > -          - const: anbernic,rg35xx-2024
> > > > -          - const: allwinner,sun50i-h700
> > > > -
> > > > -      - description: Anbernic RG35XX Plus
> > > > +      - description: Anbernic H700 Handheld Gaming Console
> > >
> > > So that's certainly an interesting optimisation, but so far we were using
> > > one entry per device, it seems.
> > > I am not entirely sure what the purpose of this file is, exactly: just to
> > > document the compatible names
> >
> > That was the initial intent, yes.
> >
> > > to reserve them and avoid clashes in the future?
> >
> > And I guess it helps with that too :)
> >
> > > Or also to put some official names to each device? That seems to
> > > somewhat overlap with the root node's model property in the respective
> > > device .dts, though.
> >
> > I guess it's a fair criticism. It would be hard to collect all the
> > compatibles without describing which device they belong too though. So
> > yeah, there's some redundancy, but removing the descriptions entirely
> > would be worse imo.
> >
> > > It would be good to clarify this, and establish how to group those devices.
> > > I mean technically we could for instance put *all* H6 devices into one
> > > entry, using the same scheme as below.
> > > Not sure that's desired, though.
> >
> > I don't really have a say there anymore, but I always tend to prefer
> > consistency in documentation as a user. Even more so since that kind of
> > categorization tends to be very subjective and thus super inconsistent.
> >
> > Maxime
>
> This is how I was asked to do it in the rockchip.yaml file [1], but I
> know different teams have different style requests. Just let me know
> what you prefer and I'll get it done that way.

For now, please just keep each board entry separate to keep the same
style as the rest of the file.

ChenYu

> [1] https://lore.kernel.org/all/20240123212111.202146-3-macroalpha82@gmail.com/
>
> Thank you,
> Chris.

Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Chris Morgan]

On Thu, Jun 06, 2024 at 01:35:15PM +0100, Andre Przywara wrote:
> On Thu, 6 Jun 2024 19:58:07 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
> 
> Hi Chen-Yu,
> 
> > On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > On Wed,  5 Jun 2024 13:53:39 -0500
> > > Chris Morgan <macroalpha82@gmail.com> wrote:
> > >  
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > >
> > > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > > clamshell form-factor. The key differences between the SP and the Plus
> > > > is a lid switch and an RTC on the same i2c bus as the PMIC.
> > > >
> > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > > ---
> > > >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> > > >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> > > >  2 files changed, 147 insertions(+), 1 deletion(-)
> > > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > index 0db7b60b49a1..00bed412ee31 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
> > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
> > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > > > -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > new file mode 100644
> > > > index 000000000000..a1d16b65ef5d
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > @@ -0,0 +1,145 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +/*
> > > > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > > > + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> > > > + */
> > > > +
> > > > +#include <dt-bindings/input/gpio-keys.h>
> > > > +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> > > > +
> > > > +/ {
> > > > +     model = "Anbernic RG35XX SP";
> > > > +     compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> > > > +
> > > > +     gpio-keys-lid {
> > > > +             compatible = "gpio-keys";
> > > > +
> > > > +             lid-switch {
> > > > +                     label = "Lid Switch";
> > > > +                     gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> > > > +                     linux,can-disable;
> > > > +                     linux,code = <SW_LID>;
> > > > +                     linux,input-type = <EV_SW>;
> > > > +                     wakeup-event-action = <EV_ACT_DEASSERTED>;
> > > > +                     wakeup-source;
> > > > +             };
> > > > +     };
> > > > +};
> > > > +
> > > > +/delete-node/ &r_rsb;  
> > >
> > > I don't think deleting the *RSB* node is right here, if at all, I'd expect
> > > status="disabled", and then maybe deleting the axp717 node within?
> > > Or maybe factor the AXP node out into a separate file, and include it from
> > > the -2024.dts and from this one?  
> > 
> > Doesn't quite work because the unit address is different.
> 
> Ah, right, good point. It's a bit annoying because the node name itself is
> mostly irrelevant, but I see that this wouldn't pass validation.
> 
> > > Or move every board to I2C?  
> > 
> > Doesn't this depend on the bootloader also running in RSB mode? My memory
> > is a bit foggy on this, but IIRC we did a conversion on some other boards
> > before?
> 
> In the SPL we use I2C only, mostly because we have an SPL capable I2C
> driver already.
> In U-Boot proper we use whatever the DT says, that should work like in the
> kernel.
> In TF-A we used to have one hardcoded transport per SoC, and that happens
> to be RSB everywhere at the moment, but I have a patch series [1] to
> determine this dynamically, via the DT. As it stands, that chooses the
> transport by looking at the PMIC (I2C for the AXP313, RSB for the AXP717
> or AXP305), but I think it's fairly easy to check for the status property
> of both RSB and I2C, or look at the parent node of the AXP node to find
> the transport protocol to use.
> I will sketch something up.
> 
> Chris has plans to auto-detect the exact Anbernic model in U-Boot, and
> switch to the right DT then automatically. IIUC I2C devices play a role in
> this, so switching to I2C for all Anbernic models would make this more
> viable.
> It just leaves a bit of a bitter taste that we now model the DT after this
> particular requirement, and not after what the hardware looks like.

I honestly would rather simply have the Linux tree use i2c for all
devices, to be honest. For at least the Anbernic RG35XXSP device
as well as a device I haven't started yet (the RG28XX) there is an
external rtc on the same i2c bus as the PMIC at 0x51. This device
does not appear to be present on the other devices.

I'm thinking that with U-Boot I can use a single bootloader and load
one of five device trees based on what happens when I check for this
RTC at 0x51, when I attempt to communicate with mmc1 (wifi), and when
I check GPIO PE11. I don't need to actually use the wifi in U-Boot,
just find out if it's there.

This means basically:

GPIO PE11 - RG35XX-H
RTC present, wifi present - RG35XX-SP
RTC present, wifi missing - RG28XX
RTC missing, wifi missing - RG35XX-2024
RTC missing, wifi present - RG35XX-Plus

Regardless of this though, I will find a way to manage no matter what
we decide here. But for the RG35XX-SP and RG28XX (not yet submitted) I
want to run the PMIC off i2c so I can use the external RTC too.

Thank you,
Chris.

> 
> Cheers,
> Andre.
> 
> [1] https://review.trustedfirmware.org/q/topic:%22h616_pmics%22
> 
> > > What do people think about this?  
> > 
> > "disabled" in RSB node and deleting the axp717 node is probably the right
> > thing to do.
> > 
> > 
> > > Cheers,
> > > Andre
> > >  
> > > > +
> > > > +&r_i2c {
> > > > +     pinctrl-0 = <&r_i2c_pins>;
> > > > +     pinctrl-names = "default";
> > > > +     status = "okay";
> > > > +
> > > > +     axp717: pmic@34 {
> > > > +             compatible = "x-powers,axp717";
> > > > +             reg = <0x34>;
> > > > +             interrupt-controller;
> > > > +             #interrupt-cells = <1>;
> > > > +             interrupt-parent = <&nmi_intc>;
> > > > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > +
> > > > +             vin1-supply = <&reg_vcc5v>;
> > > > +             vin2-supply = <&reg_vcc5v>;
> > > > +             vin3-supply = <&reg_vcc5v>;
> > > > +             vin4-supply = <&reg_vcc5v>;
> > > > +
> > > > +             regulators {
> > > > +                     reg_dcdc1: dcdc1 {
> > > > +                             regulator-always-on;
> > > > +                             regulator-min-microvolt = <900000>;
> > > > +                             regulator-max-microvolt = <1100000>;
> > > > +                             regulator-name = "vdd-cpu";
> > > > +                     };
> > > > +
> > > > +                     reg_dcdc2: dcdc2 {
> > > > +                             regulator-always-on;
> > > > +                             regulator-min-microvolt = <940000>;
> > > > +                             regulator-max-microvolt = <940000>;
> > > > +                             regulator-name = "vdd-gpu-sys";
> > > > +                     };
> > > > +
> > > > +                     reg_dcdc3: dcdc3 {
> > > > +                             regulator-always-on;
> > > > +                             regulator-min-microvolt = <1100000>;
> > > > +                             regulator-max-microvolt = <1100000>;
> > > > +                             regulator-name = "vdd-dram";
> > > > +                     };
> > > > +
> > > > +                     reg_aldo1: aldo1 {
> > > > +                             /* 1.8v - unused */
> > > > +                     };  
> > 
> > You can drop all the unused ones, unless you plan to include *this*
> > file from another one and use those nodes/labels.
> > 
> > ChenYu
> > 
> > > > +
> > > > +                     reg_aldo2: aldo2 {
> > > > +                             /* 1.8v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_aldo3: aldo3 {
> > > > +                             /* 1.8v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_aldo4: aldo4 {
> > > > +                             regulator-min-microvolt = <1800000>;
> > > > +                             regulator-max-microvolt = <1800000>;
> > > > +                             regulator-name = "vcc-pg";
> > > > +                     };
> > > > +
> > > > +                     reg_bldo1: bldo1 {
> > > > +                             /* 1.8v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_bldo2: bldo2 {
> > > > +                             regulator-always-on;
> > > > +                             regulator-min-microvolt = <1800000>;
> > > > +                             regulator-max-microvolt = <1800000>;
> > > > +                             regulator-name = "vcc-pll";
> > > > +                     };
> > > > +
> > > > +                     reg_bldo3: bldo3 {
> > > > +                             /* 2.8v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_bldo4: bldo4 {
> > > > +                             /* 1.2v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_cldo1: cldo1 {
> > > > +                             /* 3.3v - audio codec - not yet implemented */
> > > > +                     };
> > > > +
> > > > +                     reg_cldo2: cldo2 {
> > > > +                             /* 3.3v - unused */
> > > > +                     };
> > > > +
> > > > +                     reg_cldo3: cldo3 {
> > > > +                             regulator-always-on;
> > > > +                             regulator-min-microvolt = <3300000>;
> > > > +                             regulator-max-microvolt = <3300000>;
> > > > +                             regulator-name = "vcc-io";
> > > > +                     };
> > > > +
> > > > +                     reg_cldo4: cldo4 {
> > > > +                             regulator-min-microvolt = <3300000>;
> > > > +                             regulator-max-microvolt = <3300000>;
> > > > +                             regulator-name = "vcc-wifi";
> > > > +                     };
> > > > +
> > > > +                     reg_boost: boost {
> > > > +                             regulator-min-microvolt = <5000000>;
> > > > +                             regulator-max-microvolt = <5200000>;
> > > > +                             regulator-name = "boost";
> > > > +                     };
> > > > +
> > > > +                     reg_cpusldo: cpusldo {
> > > > +                             /* unused */
> > > > +                     };
> > > > +             };
> > > > +     };
> > > > +
> > > > +     rtc_ext: rtc@51 {
> > > > +             compatible = "nxp,pcf8563";
> > > > +             reg = <0x51>;
> > > > +     };
> > > > +};  
> > >
> > >  
> 

Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Andre Przywara]

On Thu, 6 Jun 2024 12:06:54 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

Hi,

> On Thu, Jun 06, 2024 at 01:35:15PM +0100, Andre Przywara wrote:
> > On Thu, 6 Jun 2024 19:58:07 +0800
> > Chen-Yu Tsai <wens@csie.org> wrote:
> > 
> > Hi Chen-Yu,
> >   
> > > On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@arm.com> wrote:  
> > > >
> > > > On Wed,  5 Jun 2024 13:53:39 -0500
> > > > Chris Morgan <macroalpha82@gmail.com> wrote:
> > > >    
> > > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > >
> > > > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > > > clamshell form-factor. The key differences between the SP and the Plus
> > > > > is a lid switch and an RTC on the same i2c bus as the PMIC.
> > > > >
> > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> > > > >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> > > > >  2 files changed, 147 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > index 0db7b60b49a1..00bed412ee31 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > > > > -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > > new file mode 100644
> > > > > index 000000000000..a1d16b65ef5d
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > > @@ -0,0 +1,145 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +/*
> > > > > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > > > > + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> > > > > + */
> > > > > +
> > > > > +#include <dt-bindings/input/gpio-keys.h>
> > > > > +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> > > > > +
> > > > > +/ {
> > > > > +     model = "Anbernic RG35XX SP";
> > > > > +     compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> > > > > +
> > > > > +     gpio-keys-lid {
> > > > > +             compatible = "gpio-keys";
> > > > > +
> > > > > +             lid-switch {
> > > > > +                     label = "Lid Switch";
> > > > > +                     gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> > > > > +                     linux,can-disable;
> > > > > +                     linux,code = <SW_LID>;
> > > > > +                     linux,input-type = <EV_SW>;
> > > > > +                     wakeup-event-action = <EV_ACT_DEASSERTED>;
> > > > > +                     wakeup-source;
> > > > > +             };
> > > > > +     };
> > > > > +};
> > > > > +
> > > > > +/delete-node/ &r_rsb;    
> > > >
> > > > I don't think deleting the *RSB* node is right here, if at all, I'd expect
> > > > status="disabled", and then maybe deleting the axp717 node within?
> > > > Or maybe factor the AXP node out into a separate file, and include it from
> > > > the -2024.dts and from this one?    
> > > 
> > > Doesn't quite work because the unit address is different.  
> > 
> > Ah, right, good point. It's a bit annoying because the node name itself is
> > mostly irrelevant, but I see that this wouldn't pass validation.
> >   
> > > > Or move every board to I2C?    
> > > 
> > > Doesn't this depend on the bootloader also running in RSB mode? My memory
> > > is a bit foggy on this, but IIRC we did a conversion on some other boards
> > > before?  
> > 
> > In the SPL we use I2C only, mostly because we have an SPL capable I2C
> > driver already.
> > In U-Boot proper we use whatever the DT says, that should work like in the
> > kernel.
> > In TF-A we used to have one hardcoded transport per SoC, and that happens
> > to be RSB everywhere at the moment, but I have a patch series [1] to
> > determine this dynamically, via the DT. As it stands, that chooses the
> > transport by looking at the PMIC (I2C for the AXP313, RSB for the AXP717
> > or AXP305), but I think it's fairly easy to check for the status property
> > of both RSB and I2C, or look at the parent node of the AXP node to find
> > the transport protocol to use.
> > I will sketch something up.
> > 
> > Chris has plans to auto-detect the exact Anbernic model in U-Boot, and
> > switch to the right DT then automatically. IIUC I2C devices play a role in
> > this, so switching to I2C for all Anbernic models would make this more
> > viable.
> > It just leaves a bit of a bitter taste that we now model the DT after this
> > particular requirement, and not after what the hardware looks like.  
> 
> I honestly would rather simply have the Linux tree use i2c for all
> devices, to be honest. For at least the Anbernic RG35XXSP device
> as well as a device I haven't started yet (the RG28XX) there is an
> external rtc on the same i2c bus as the PMIC at 0x51. This device
> does not appear to be present on the other devices.
> 
> I'm thinking that with U-Boot I can use a single bootloader and load
> one of five device trees based on what happens when I check for this
> RTC at 0x51, when I attempt to communicate with mmc1 (wifi), and when
> I check GPIO PE11. I don't need to actually use the wifi in U-Boot,
> just find out if it's there.
> 
> This means basically:
> 
> GPIO PE11 - RG35XX-H
> RTC present, wifi present - RG35XX-SP
> RTC present, wifi missing - RG28XX
> RTC missing, wifi missing - RG35XX-2024
> RTC missing, wifi present - RG35XX-Plus
> 
> Regardless of this though, I will find a way to manage no matter what
> we decide here. But for the RG35XX-SP and RG28XX (not yet submitted) I
> want to run the PMIC off i2c so I can use the external RTC too.

Yes, if you have I2C devices connected to those pins, then we *need* to
use I2C for the PMIC. We have the same situation for the Pine H64 board,
check sun50i-h6-pine-h64.dts.
That's why the idea to switch the others, too.

Cheers,
Andre

> > 
> > [1] https://review.trustedfirmware.org/q/topic:%22h616_pmics%22
> >   
> > > > What do people think about this?    
> > > 
> > > "disabled" in RSB node and deleting the axp717 node is probably the right
> > > thing to do.
> > > 
> > >   
> > > > Cheers,
> > > > Andre
> > > >    
> > > > > +
> > > > > +&r_i2c {
> > > > > +     pinctrl-0 = <&r_i2c_pins>;
> > > > > +     pinctrl-names = "default";
> > > > > +     status = "okay";
> > > > > +
> > > > > +     axp717: pmic@34 {
> > > > > +             compatible = "x-powers,axp717";
> > > > > +             reg = <0x34>;
> > > > > +             interrupt-controller;
> > > > > +             #interrupt-cells = <1>;
> > > > > +             interrupt-parent = <&nmi_intc>;
> > > > > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > +
> > > > > +             vin1-supply = <&reg_vcc5v>;
> > > > > +             vin2-supply = <&reg_vcc5v>;
> > > > > +             vin3-supply = <&reg_vcc5v>;
> > > > > +             vin4-supply = <&reg_vcc5v>;
> > > > > +
> > > > > +             regulators {
> > > > > +                     reg_dcdc1: dcdc1 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <900000>;
> > > > > +                             regulator-max-microvolt = <1100000>;
> > > > > +                             regulator-name = "vdd-cpu";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_dcdc2: dcdc2 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <940000>;
> > > > > +                             regulator-max-microvolt = <940000>;
> > > > > +                             regulator-name = "vdd-gpu-sys";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_dcdc3: dcdc3 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <1100000>;
> > > > > +                             regulator-max-microvolt = <1100000>;
> > > > > +                             regulator-name = "vdd-dram";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo1: aldo1 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };    
> > > 
> > > You can drop all the unused ones, unless you plan to include *this*
> > > file from another one and use those nodes/labels.
> > > 
> > > ChenYu
> > >   
> > > > > +
> > > > > +                     reg_aldo2: aldo2 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo3: aldo3 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo4: aldo4 {
> > > > > +                             regulator-min-microvolt = <1800000>;
> > > > > +                             regulator-max-microvolt = <1800000>;
> > > > > +                             regulator-name = "vcc-pg";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo1: bldo1 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo2: bldo2 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <1800000>;
> > > > > +                             regulator-max-microvolt = <1800000>;
> > > > > +                             regulator-name = "vcc-pll";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo3: bldo3 {
> > > > > +                             /* 2.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo4: bldo4 {
> > > > > +                             /* 1.2v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo1: cldo1 {
> > > > > +                             /* 3.3v - audio codec - not yet implemented */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo2: cldo2 {
> > > > > +                             /* 3.3v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo3: cldo3 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <3300000>;
> > > > > +                             regulator-max-microvolt = <3300000>;
> > > > > +                             regulator-name = "vcc-io";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo4: cldo4 {
> > > > > +                             regulator-min-microvolt = <3300000>;
> > > > > +                             regulator-max-microvolt = <3300000>;
> > > > > +                             regulator-name = "vcc-wifi";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_boost: boost {
> > > > > +                             regulator-min-microvolt = <5000000>;
> > > > > +                             regulator-max-microvolt = <5200000>;
> > > > > +                             regulator-name = "boost";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cpusldo: cpusldo {
> > > > > +                             /* unused */
> > > > > +                     };
> > > > > +             };
> > > > > +     };
> > > > > +
> > > > > +     rtc_ext: rtc@51 {
> > > > > +             compatible = "nxp,pcf8563";
> > > > > +             reg = <0x51>;
> > > > > +     };
> > > > > +};    
> > > >
> > > >    
> >