[PATCH v2 0/5] arm64: dts: allwinner: Add Anbernic RG35XX (Plus/H/2024) support

[PATCH v2 0/5] arm64: dts: allwinner: Add Anbernic RG35XX (Plus/H/2024) support

[Ryan Walklin]

Revised patchset based on review with many changes including improved regulators and formatting.
Changelog inline, original cover below.

--

The Anbernic RG35XX is a family of handheld gaming devices. There are 4 
variants, of which 3 using the Allwinner H700 chip are covered by this patchset.
The fourth (released first and named simply RG35XX) uses an Actions 
Semiconductor ATM7039s which is a 32-bit Cortex-A9 chip with no mainline support 
and is not covered.

Common features (RG35XX-2024):
- Allwinner H700 @ 1.5GHz (H616 variant exposing RGB LCD pins, with 4x 
  Cortex-A53 Cores and a Mali G31 GPU)
- 1 GB LPDDR4 DRAM
- AXP717 PMIC (patches accepted in mfd-next - 
  https://kernel.googlesource.com/pub/scm/linux/kernel/git/lee/mfd/+/d2ac3df75c3a995064cfac0171e082a30d8c4c66)
- 3.5" 640x480 RGB LCD
- Mini-HDMI, 3.5mm audio jack, mono speaker, two microSD slots and USB-C 
  (USB 2.0) for power.

RG35XX-Plus adds:
- RTL8821CS SDIO Wifi/BT chip

RG35XX-H (Horizontal form-factor) adds:
- RTL8821CS SDIO Wifi/BT chip
- Two analog thumbsticks
- Second USB-C port
- Stereo speaker

Patch 1 adds the DT bindings for the board names, Patch 2 adds the -2024 device
as a common base, Patch 3 adds Wifi/BT support for the -Plus (and -H), and Patch 
3 adds the second USB and thumbsticks for the -H. The -H is a strict superset of
the -Plus, which is in turn a strict superset of the -2024, so this translates 
quite neatly. Alternatively a single DTS for the three devices could be 
considered.

LCD, HDMI, audio and GPU support are not yet ready and relying on out-of-tree 
patches currently, so will be added once these drivers are mainlined.

Ryan

Signed-off-by: Ryan Walklin <ryan@testtoast.com>

Ryan Walklin (5):
  dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming
    device variants
  arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
  arm64: dts: allwinner: h700: Add RG35XX-Plus DTS
  arm64: dts: allwinner: h700: Add RG35XX-H DTS
  arm64: dts: allwinner: h700: Add RG35XX DTBs to Makefile

 .../devicetree/bindings/arm/sunxi.yaml        |  15 +
 arch/arm64/boot/dts/allwinner/Makefile        |   3 +
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
 .../sun50i-h700-anbernic-rg35xx-h.dts         | 126 ++++++
 .../sun50i-h700-anbernic-rg35xx-plus.dts      |  53 +++
 5 files changed, 572 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts

-- 
2.44.0

[PATCH v2 1/5] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants

[Ryan Walklin]

RG35XX 2024: Base version with Allwinner H700
RG35XX Plus: Adds Wifi/BT
RG35XX H: Adds second USB port and analog sticks to -Plus in horizontal form factor

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/arm/sunxi.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
index 09d835db6db5..fc10f54561c9 100644
--- a/Documentation/devicetree/bindings/arm/sunxi.yaml
+++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
@@ -56,6 +56,21 @@ properties:
           - 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
+      - items:
+          - const: anbernic,rg35xx-plus
+          - const: allwinner,sun50i-h700
+
+      - description: Anbernic RG35XX H
+      - items:
+          - const: anbernic,rg35xx-h
+          - const: allwinner,sun50i-h700
+
       - description: Amarula A64 Relic
         items:
           - const: amarula,a64-relic
-- 
2.44.0

[PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Ryan Walklin]

The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.

The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.

Device features:
- Allwinner H700 @ 1.5GHz
- 1GB LPDDR4 DRAM
- X-Powers AXP717 PMIC
- 3.5" 640x480 RGB LCD
- Two microSD slots
- Mini-HDMI out
- GPIO keypad
- 3.5mm headphone jack

Enabled in this DTS:
- AXP717 PMIC with regulators (interrupt controller TBC/TBD)
- Power LED (charge LED on device controlled directly by PMIC)
- Serial UART (accessible from PIN headers on the board)
- MMC slots

Changelog v1..v2:
- Update copyright
- Spaces -> Tabs
- Add cpufreq support [1]
- Remove MMC aliases
- Fix GPIO button and regulator node names
- Note unused AXP717 regulators
- Update regulator for SD slots
- Remove unused I2C3 device
- Update NMI interrupt controller for AXP717 [2]
- Add chassis-type
- Address USB EHCI/OHCI0 correctly and add usb vbus supply
- Add PIO vcc-pg-supply
- Correct boost regulator voltage and name

Signed-off-by: Ryan Walklin <ryan@testtoast.com>

[1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
[2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
 1 file changed, 375 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
new file mode 100644
index 000000000000..d8081273677d
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
+ */
+
+/dts-v1/;
+
+#include "sun50i-h616.dtsi"
+#include "sun50i-h616-cpu-opp.dtsi"
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/linux-event-codes.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/leds/common.h>
+
+/ {
+	model = "Anbernic RG35XX 2024";
+	chassis-type = "handset";
+	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
+
+	aliases {
+		serial0 = &uart0;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		led-0 {
+			function = LED_FUNCTION_POWER;
+			color = <LED_COLOR_ID_GREEN>;
+			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
+			default-state = "on";
+		};
+	};
+
+	gpio-keys {
+	   compatible = "gpio-keys";
+
+	   button-up {
+		   label = "D-Pad Up";
+		   gpios = <&pio 0 6 GPIO_ACTIVE_LOW>; /* PA6 */
+		   linux,input-type = <EV_KEY>;
+		   linux,code = <BTN_DPAD_UP>;
+		};
+
+		button-down {
+			label = "D-Pad Down";
+			gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_DPAD_DOWN>;
+		};
+
+		button-left {
+			label = "D-Pad left";
+			gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_DPAD_LEFT>;
+		};
+
+		button-right {
+			label = "D-Pad Right";
+			gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_DPAD_RIGHT>;
+		};
+
+		button-a {
+			label = "Action-Pad A";
+			gpios = <&pio 0 0 GPIO_ACTIVE_LOW>; /* PA0 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_EAST>;
+		};
+
+		button-b {
+			label = "Action-Pad B";
+			gpios = <&pio 0 1 GPIO_ACTIVE_LOW>; /* PA1 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_SOUTH>;
+		};
+
+		button-x {
+			label = "Action-Pad X";
+			gpios = <&pio 0 3 GPIO_ACTIVE_LOW>; /* PA3 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_NORTH>;
+		};
+
+		button-y {
+			label = "Action Pad Y";
+			gpios = <&pio 0 2 GPIO_ACTIVE_LOW>; /* PA2 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_WEST>;
+		};
+
+		button-start {
+			label = "Key Start";
+			gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_START>;
+		};
+
+		button-select {
+			label = "Key Select";
+			gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_SELECT>;
+		};
+
+		button-l1 {
+			label = "Key L1";
+			gpios = <&pio 0 10 GPIO_ACTIVE_LOW>; /* PA10 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_TL>;
+		};
+
+		button-l2 {
+			label = "Key L2";
+			gpios = <&pio 0 11 GPIO_ACTIVE_LOW>; /* PA11 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_TL2>;
+		};
+
+		button-r1 {
+			label = "Key R1";
+			gpios = <&pio 0 12 GPIO_ACTIVE_LOW>; /* PA12 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_TR>;
+		};
+
+		button-r2 {
+			label = "Key R2";
+			gpios = <&pio 0 7 GPIO_ACTIVE_LOW>; /* PA7 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_TR2>;
+		};
+
+		button-menu {
+			label = "Key Menu";
+			gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_MODE>;
+		};
+
+		button-vol-up {
+			label = "Key Volume Up";
+			gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; /* PE1 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <KEY_VOLUMEUP>;
+		};
+
+		button-vol-down {
+			label = "Key Volume Down";
+			gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; /* PE2 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <KEY_VOLUMEDOWN>;
+		};
+	};
+
+	reg_vcc_sd2: regulator-vcc3v3 {
+		compatible = "regulator-fixed";
+		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
+		regulator-name = "vcc_3v3_sd2";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
+	reg_vcc_usb: regulator-vcc-5v0-usb {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
+		regulator-name = "vcc_5v0_usb";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&vcc_3v3_usb>;
+	};
+
+	vcc_3v3_usb: vcc-3v3-usb {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-name = "vcc_3v3_usb";
+		regulator-max-microvolt = <3300000>;
+		regulator-min-microvolt = <3300000>;
+	};
+
+	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
+		compatible = "regulator-fixed";
+		regulator-name = "vcc-5v";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+	};
+
+	reg_pll_dcx0: regulator-pll-dcx0 {
+		compatible = "regulator-fixed";
+		regulator-always-on;
+		regulator-name = "vcc-pll";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+	};
+};
+
+&cpu0 {
+	cpu-supply = <&reg_dcdc1>;
+};
+
+&mmc0 {
+	vmmc-supply = <&reg_vcc_sd2>;
+	disable-wp;
+	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
+	bus-width = <4>;
+	status = "okay";
+};
+
+&mmc2 {
+	vmmc-supply = <&reg_vcc_sd2>;
+	vqmmc-supply = <&reg_aldo1>;
+	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
+	bus-width = <4>;
+	status = "okay";
+};
+
+&ohci0 {
+	status = "okay";
+};
+
+&ehci0 {
+	status = "okay";
+};
+
+&r_rsb {
+   status = "okay";
+
+   axp717: pmic@3a3 {
+		compatible = "x-powers,axp717";
+		reg = <0x3a3>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupt-parent = <&nmi_intc>;
+		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+		x-powers,drive-vbus-en;
+
+		vin1-supply = <&reg_vcc5v>;
+		vin2-supply = <&reg_vcc5v>;
+		vin3-supply = <&reg_vcc5v>;
+		vin4-supply = <&reg_vcc5v>;
+
+		regulators {
+			reg_dcdc1: dcdc1 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <810000>;
+				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-sys";
+			};
+
+			reg_dcdc3: dcdc3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-dram";
+			};
+
+			reg_aldo1: aldo1 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-sd2";
+			};
+
+			reg_aldo2: aldo2 {
+				/* unused */
+			};
+
+			reg_aldo3: aldo3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3500000>;
+				regulator-name = "axp717-aldo3";
+			};
+
+			reg_aldo4: aldo4 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3500000>;
+				regulator-name = "axp717-aldo4";
+			};
+
+			reg_bldo1: bldo1 {
+				/* unused */
+			};
+
+			reg_bldo2: bldo2 {
+				regulator-always-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc-pll";
+			};
+
+			reg_bldo3: bldo3 {
+				/* unused */
+			};
+
+			reg_bldo4: bldo4 {
+				/* unused */
+			};
+
+			reg_cldo1: cldo1 {
+				/* unused */
+			};
+
+			reg_cldo2: cldo2 {
+				/* unused */
+			};
+
+			reg_cldo3: cldo3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3500000>;
+				regulator-name = "axp717-cldo3";
+			};
+
+			reg_cldo4: cldo4 {
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-wifi";
+			};
+
+			reg_boost: boost {
+				regulator-min-microvolt = <5126000>;
+				regulator-max-microvolt = <5126000>;
+				regulator-name = "boost";
+			};
+
+			reg_cpusldo: cpusldo {
+				/* unused */
+			};
+		};
+	};
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_ph_pins>;
+	status = "okay";
+};
+
+&pio {
+	vcc-pg-supply = <&reg_pll_dcx0>;
+};
+
+/* the AXP717 has USB type-C role switch functionality, to be implemented */
+&usbotg {
+	dr_mode = "host";   /* USB type-C receptable */
+	status = "okay";
+};
+
+&usbphy {
+	usb0_vbus-supply = <&reg_vcc_usb>;
+	status = "okay";
+};
-- 
2.44.0

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Krzysztof Kozlowski]

On 20/04/2024 12:43, Ryan Walklin wrote:
> The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.
> 
> The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.
> 
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-0 {
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_GREEN>;
> +			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
> +			default-state = "on";
> +		};
> +	};
> +
> +	gpio-keys {
> +	   compatible = "gpio-keys";
> +
> +	   button-up {
> +		   label = "D-Pad Up";

Please fix your indentation to match kernel/DTS coding style.


Best regards,
Krzysztof

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Ryan Walklin]

>> +	   button-up {
>> +		   label = "D-Pad Up";
>
> Please fix your indentation to match kernel/DTS coding style.
>
Good spot, thanks!

>
> Best regards,
> Krzysztof

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Andre Przywara]

On Sat, 20 Apr 2024 22:43:56 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi Ryan,

many thanks for the respin and the changes! We are getting there ...

> The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.
> 
> The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.
> 
> Device features:
> - Allwinner H700 @ 1.5GHz
> - 1GB LPDDR4 DRAM
> - X-Powers AXP717 PMIC
> - 3.5" 640x480 RGB LCD
> - Two microSD slots
> - Mini-HDMI out
> - GPIO keypad
> - 3.5mm headphone jack
> 
> Enabled in this DTS:
> - AXP717 PMIC with regulators (interrupt controller TBC/TBD)
> - Power LED (charge LED on device controlled directly by PMIC)
> - Serial UART (accessible from PIN headers on the board)
> - MMC slots
> 
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Add cpufreq support [1]
> - Remove MMC aliases
> - Fix GPIO button and regulator node names
> - Note unused AXP717 regulators
> - Update regulator for SD slots
> - Remove unused I2C3 device
> - Update NMI interrupt controller for AXP717 [2]
> - Add chassis-type
> - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> - Add PIO vcc-pg-supply
> - Correct boost regulator voltage and name
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> 
> [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
> [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t

As those are dependencies, but WIP, this gets a bit tricky:
- cpufreq is pretty far, but comes through a different tree. I suggest
  you drop the cpu-opp.dtsi include, to not complicate things. We can
  add this later as a fix, once this OPP file has reached the master
  tree.
- The NMI binding and DT node seem important, but have not been merged
  yet. I suggest to mention them as a requirement. The patches (binding
  plus H616 .dtsi change) should go through the sunxi tree as well, so
  the maintainers can order this appropriately when merging.
- The GPADC (in the later patch) is similar, but it is not as critical
  as the NMI. Not sure how the maintainers want to handle this, but we
  might add those bits as an (optional) patch on top, so this can be
  handled more independently from the GPADC series.

> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
>  1 file changed, 375 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> new file mode 100644
> index 000000000000..d8081273677d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> + */
> +
> +/dts-v1/;
> +
> +#include "sun50i-h616.dtsi"
> +#include "sun50i-h616-cpu-opp.dtsi"

As mentioned, please drop this line for now.

> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/linux-event-codes.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/leds/common.h>
> +
> +/ {
> +	model = "Anbernic RG35XX 2024";
> +	chassis-type = "handset";
> +	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
> +
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-0 {
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_GREEN>;
> +			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
> +			default-state = "on";
> +		};
> +	};
> +
> +	gpio-keys {
> +	   compatible = "gpio-keys";
> +
> +	   button-up {

indentation

> +		   label = "D-Pad Up";
> +		   gpios = <&pio 0 6 GPIO_ACTIVE_LOW>; /* PA6 */
> +		   linux,input-type = <EV_KEY>;
> +		   linux,code = <BTN_DPAD_UP>;
> +		};
> +
> +		button-down {
> +			label = "D-Pad Down";
> +			gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_DOWN>;
> +		};
> +
> +		button-left {
> +			label = "D-Pad left";
> +			gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_LEFT>;
> +		};
> +
> +		button-right {
> +			label = "D-Pad Right";
> +			gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_RIGHT>;
> +		};
> +
> +		button-a {
> +			label = "Action-Pad A";
> +			gpios = <&pio 0 0 GPIO_ACTIVE_LOW>; /* PA0 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_EAST>;
> +		};
> +
> +		button-b {
> +			label = "Action-Pad B";
> +			gpios = <&pio 0 1 GPIO_ACTIVE_LOW>; /* PA1 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_SOUTH>;
> +		};
> +
> +		button-x {
> +			label = "Action-Pad X";
> +			gpios = <&pio 0 3 GPIO_ACTIVE_LOW>; /* PA3 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_NORTH>;
> +		};
> +
> +		button-y {
> +			label = "Action Pad Y";
> +			gpios = <&pio 0 2 GPIO_ACTIVE_LOW>; /* PA2 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_WEST>;
> +		};
> +
> +		button-start {
> +			label = "Key Start";
> +			gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_START>;
> +		};
> +
> +		button-select {
> +			label = "Key Select";
> +			gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_SELECT>;
> +		};
> +
> +		button-l1 {
> +			label = "Key L1";
> +			gpios = <&pio 0 10 GPIO_ACTIVE_LOW>; /* PA10 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TL>;
> +		};
> +
> +		button-l2 {
> +			label = "Key L2";
> +			gpios = <&pio 0 11 GPIO_ACTIVE_LOW>; /* PA11 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TL2>;
> +		};
> +
> +		button-r1 {
> +			label = "Key R1";
> +			gpios = <&pio 0 12 GPIO_ACTIVE_LOW>; /* PA12 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TR>;
> +		};
> +
> +		button-r2 {
> +			label = "Key R2";
> +			gpios = <&pio 0 7 GPIO_ACTIVE_LOW>; /* PA7 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TR2>;
> +		};
> +
> +		button-menu {
> +			label = "Key Menu";
> +			gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_MODE>;
> +		};
> +
> +		button-vol-up {
> +			label = "Key Volume Up";
> +			gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; /* PE1 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <KEY_VOLUMEUP>;
> +		};
> +
> +		button-vol-down {
> +			label = "Key Volume Down";
> +			gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; /* PE2 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <KEY_VOLUMEDOWN>;
> +		};
> +	};
> +
> +	reg_vcc_sd2: regulator-vcc3v3 {
> +		compatible = "regulator-fixed";
> +		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> +		regulator-name = "vcc_3v3_sd2";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	reg_vcc_usb: regulator-vcc-5v0-usb {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
> +		regulator-name = "vcc_5v0_usb";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc_3v3_usb>;

This looks wrong, see below.

> +	};
> +
> +	vcc_3v3_usb: vcc-3v3-usb {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
> +		regulator-always-on;
> +		regulator-boot-on;

There seems to be something off with this one. First, it seems odd that
an always-on regulator is GPIO controlled, as this surely means it's
not enabled at reset time (because the GPIO is initially HighZ and thus
not enabled). So who turns this on? Most likely it's the kernel? What
happens if we turn this off (or leave it off)?

Secondly, why is this 3.3V (both by name and voltage), but then
supplies the 5.0V USB VBUS?
And given that this is chained with reg_vcc_usb above, are those really
two regulators, controlled by two GPIOs?

> +		regulator-name = "vcc_3v3_usb";
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <3300000>;
> +	};
> +
> +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc-5v";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +
> +	reg_pll_dcx0: regulator-pll-dcx0 {

It's DCXO (letter "oh", not zero): digitally controlled/compensated
crystal *o*scillator.

> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-name = "vcc-pll";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};

That one looks odd as well. While there might be a discrete regulator
(what this node is describing), I doubt it, since the AXP717 has plenty
of rails. In particular I am not sure if that fixed one would supply
PortG (mainly WiFi), which seems unneeded for the boot process, and
surely we want that switchable to save power if the WiFi is not needed.

You also have a VCC-PLL regulator below (BLDO2).
So please try to drop this regulator, I am pretty sure there is an AXP
rail that powers the WiFi.

See below for more on this.

> +};
> +
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc1>;
> +};
> +
> +&mmc0 {
> +	vmmc-supply = <&reg_vcc_sd2>;

I don't think this GPIO controlled regulator provides the supply voltage
for the first SD card, since it would be disabled on reset, and the
BROM couldn't boot from the SD card. So it must be some other 3.3V
source, either a discrete regulator (fixed regulator), or some
default-on 3.3V AXP rail.

> +	disable-wp;
> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> +	bus-width = <4>;
> +	status = "okay";
> +};
> +
> +&mmc2 {
> +	vmmc-supply = <&reg_vcc_sd2>;
> +	vqmmc-supply = <&reg_aldo1>;
> +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> +	bus-width = <4>;
> +	status = "okay";
> +};

This one seems more plausible. To confirm this, you could not use
reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
should break operation on the second SD card. Then just swap
reg_vcc_sd2 back in, and if it starts working again, we have confirmation.

> +
> +&ohci0 {
> +	status = "okay";
> +};
> +
> +&ehci0 {
> +	status = "okay";
> +};
> +
> +&r_rsb {
> +   status = "okay";
> +
> +   axp717: pmic@3a3 {
> +		compatible = "x-powers,axp717";
> +		reg = <0x3a3>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupt-parent = <&nmi_intc>;
> +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +		x-powers,drive-vbus-en;

Remove this last line, the AXP717 binding does not support this, and the
Linux driver doesn't implement this, as the AXP717 does not seem to
have this functionality.

> +
> +		vin1-supply = <&reg_vcc5v>;
> +		vin2-supply = <&reg_vcc5v>;
> +		vin3-supply = <&reg_vcc5v>;
> +		vin4-supply = <&reg_vcc5v>;
> +
> +		regulators {
> +			reg_dcdc1: dcdc1 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <810000>;
> +				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-sys";
> +			};
> +
> +			reg_dcdc3: dcdc3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1100000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd-dram";
> +			};
> +
> +			reg_aldo1: aldo1 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-sd2";
> +			};
> +
> +			reg_aldo2: aldo2 {
> +				/* unused */
> +			};
> +
> +			reg_aldo3: aldo3 {
> +				regulator-always-on;
> +				regulator-boot-on;

Please remove this last line, it doesn't make sense in this context. Cf.
Documentation//devicetree/bindings/regulator/regulator.yaml.
I think the same applies to the other uses throughout this file.

> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;

This is not right. What is the voltage of this rail? The kernel should
tell you what was set in the register, through regulator_summary, or you
check what's the voltage on a BSP system.

> +				regulator-name = "axp717-aldo3";

If the system dies when you remove always-on, you might have found some
essential supply. Candidates for consumers are listed in the
H616 or T5 series datasheet. Matching the voltage might give you an
idea. Then use this consumer as the name.

> +			};
> +
> +			reg_aldo4: aldo4 {

Same for this one: Please remove regulator-boot-on, fix the voltage,
and provide some rationale why this needs to be on.

> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;
> +				regulator-name = "axp717-aldo4";
> +			};
> +
> +			reg_bldo1: bldo1 {
> +				/* unused */
> +			};
> +
> +			reg_bldo2: bldo2 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-name = "vcc-pll";
> +			};

This one is a good example: fixed voltage, no boot-on, and regulator
name provides rationale why it must be always-on. The others should
look similar.

> +
> +			reg_bldo3: bldo3 {
> +				/* unused */
> +			};
> +
> +			reg_bldo4: bldo4 {
> +				/* unused */
> +			};
> +
> +			reg_cldo1: cldo1 {
> +				/* unused */
> +			};
> +
> +			reg_cldo2: cldo2 {
> +				/* unused */
> +			};
> +
> +			reg_cldo3: cldo3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;
> +				regulator-name = "axp717-cldo3";

Same here as ALDO3/4: what voltage is it, and what does it probably
supply?

> +			};
> +
> +			reg_cldo4: cldo4 {
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-wifi";
> +			};
> +
> +			reg_boost: boost {
> +				regulator-min-microvolt = <5126000>;
> +				regulator-max-microvolt = <5126000>;

It might be better to use a range, say 5.0V till 5.2V. The
kernel will then just continue using the default, which seems to be this
5.126V (5440mV + 9 * 64mV).

> +				regulator-name = "boost";
> +			};
> +
> +			reg_cpusldo: cpusldo {
> +				/* unused */

Is that so? I thought you mentioned on IRC this is required as well?

> +			};
> +		};
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_ph_pins>;
> +	status = "okay";
> +};
> +
> +&pio {
> +	vcc-pg-supply = <&reg_pll_dcx0>;

As mentioned above, it seems unlikely to be a fixed regulator. If in
doubt, leave them out, they are not essential for the operation.

> +};
> +
> +/* the AXP717 has USB type-C role switch functionality, to be implemented */

Replace "to be implemented" with "not yet described by the binding".
This is DT land, so we don't care about implementations or the Linux
kernel, it's all about DTs and DT bindings.

> +&usbotg {
> +	dr_mode = "host";   /* USB type-C receptable */

So does this really work? It seems wrong to make this unconditional,
given this is the only way to charge the device. When power is supplied
through the USB-C port, surely driving VBUS from the board sounds
wrong. Unless you have a killer feature for a host port, I think
without working role switching, "peripheral" would be the safer
choice.

> +	status = "okay";
> +};
> +
> +&usbphy {
> +	usb0_vbus-supply = <&reg_vcc_usb>;

When you stick to "peripheral" above, you should drop this line for
now, especially since this regulator chain looks quite suspicious still.

Cheers,
Andre

> +	status = "okay";
> +};

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Ryan Walklin]

On Sun, 21 Apr 2024, at 12:49 PM, Andre Przywara wrote:
> On Sat, 20 Apr 2024 22:43:56 +1200
> Ryan Walklin <ryan@testtoast.com> wrote:
>
> Hi Ryan,
>
> many thanks for the respin and the changes! We are getting there ...

Thanks for the review!

>> 
>> [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
>> [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t
>
> As those are dependencies, but WIP, this gets a bit tricky:
> - cpufreq is pretty far, but comes through a different tree. I suggest
>   you drop the cpu-opp.dtsi include, to not complicate things. We can
>   add this later as a fix, once this OPP file has reached the master
>   tree.

Done, thanks. Have also not increased the DCDC1 voltage to 1.16v for 1.5GHz as it's not yet working (and technically out of spec for the SoC) but will relook at the vendor BSP once this set is merged.

> - The NMI binding and DT node seem important, but have not been merged
>   yet. I suggest to mention them as a requirement. 

Done, thanks.

> - The GPADC (in the later patch) is similar, but it is not as critical
>   as the NMI. 

Will pull this out separately, as you say its only required for the joysticks on the -H.

>> +	reg_vcc_usb: regulator-vcc-5v0-usb {
>> +		compatible = "regulator-fixed";
>> +		enable-active-high;
>> +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
>> +		regulator-name = "vcc_5v0_usb";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		vin-supply = <&vcc_3v3_usb>;
>
> This looks wrong, see below.
>
>> +	};
>> +
>> +	vcc_3v3_usb: vcc-3v3-usb {
>> +		compatible = "regulator-fixed";
>> +		enable-active-high;
>> +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
>> +		regulator-always-on;
>> +		regulator-boot-on;
>
> There seems to be something off with this one. First, it seems odd that
> an always-on regulator is GPIO controlled, as this surely means it's
> not enabled at reset time (because the GPIO is initially HighZ and thus
> not enabled). So who turns this on? Most likely it's the kernel? What
> happens if we turn this off (or leave it off)?

This makes more sense with that explanation, thanks. Taking a while to get my head round how the power distribution is done in the embedded space.

>
> Secondly, why is this 3.3V (both by name and voltage), but then
> supplies the 5.0V USB VBUS?
> And given that this is chained with reg_vcc_usb above, are those really
> two regulators, controlled by two GPIOs?
>
>> +		regulator-name = "vcc_3v3_usb";
>> +		regulator-max-microvolt = <3300000>;
>> +		regulator-min-microvolt = <3300000>;
>> +	};
>> +
>> +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc-5v";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +	};

Will do some more work on this and try and figure it out. The USB and second SD are not working well currently, probably this is why.
>> +
>> +	reg_pll_dcx0: regulator-pll-dcx0 {
>
> It's DCXO (letter "oh", not zero): digitally controlled/compensated
> crystal *o*scillator.

D'oh! Had to google PLL, should have googled this too.

>
>> +		compatible = "regulator-fixed";
>> +		regulator-always-on;
>> +		regulator-name = "vcc-pll";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +	};
>
> That one looks odd as well. While there might be a discrete regulator
> (what this node is describing), I doubt it, since the AXP717 has plenty
> of rails. In particular I am not sure if that fixed one would supply
> PortG (mainly WiFi), which seems unneeded for the boot process, and
> surely we want that switchable to save power if the WiFi is not needed.
>
> You also have a VCC-PLL regulator below (BLDO2).
> So please try to drop this regulator, I am pretty sure there is an AXP
> rail that powers the WiFi.

Makes sense, will dig into the vendor BSP.
>
> See below for more on this.
>

>> +
>> +&mmc2 {
>> +	vmmc-supply = <&reg_vcc_sd2>;
>> +	vqmmc-supply = <&reg_aldo1>;
>> +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
>> +	bus-width = <4>;
>> +	status = "okay";
>> +};
>
> This one seems more plausible. To confirm this, you could not use
> reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
> should break operation on the second SD card. Then just swap
> reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
>
In hindsight this is taken from the vendor BSP, so I think is correct but will do some testing.

>> +		x-powers,drive-vbus-en;
>
> Remove this last line, the AXP717 binding does not support this, and the
> Linux driver doesn't implement this, as the AXP717 does not seem to
> have this functionality.

Thanks, fixed.

>> +
>> +			reg_aldo3: aldo3 {
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> Please remove this last line, it doesn't make sense in this context. Cf.
> Documentation//devicetree/bindings/regulator/regulator.yaml.
> I think the same applies to the other uses throughout this file.
>
>> +				regulator-min-microvolt = <500000>;
>> +				regulator-max-microvolt = <3500000>;
>
> This is not right. What is the voltage of this rail? The kernel should
> tell you what was set in the register, through regulator_summary, or you
> check what's the voltage on a BSP system.
>
>> +				regulator-name = "axp717-aldo3";
>
> If the system dies when you remove always-on, you might have found some
> essential supply. Candidates for consumers are listed in the
> H616 or T5 series datasheet. Matching the voltage might give you an
> idea. Then use this consumer as the name.
>
>> +			};
>> +
>> +			reg_aldo4: aldo4 {
>
> Same for this one: Please remove regulator-boot-on, fix the voltage,
> and provide some rationale why this needs to be on.

Thanks, will get the correct voltages. It turns out ALDO3/4 are both needed, but CPUSLDO is not. Will try and figure out what they are supplying.

>> +
>> +			reg_boost: boost {
>> +				regulator-min-microvolt = <5126000>;
>> +				regulator-max-microvolt = <5126000>;
>
> It might be better to use a range, say 5.0V till 5.2V. The
> kernel will then just continue using the default, which seems to be this
> 5.126V (5440mV + 9 * 64mV).

Thanks, fixed.

>> +
>> +&pio {
>> +	vcc-pg-supply = <&reg_pll_dcx0>;
>
> As mentioned above, it seems unlikely to be a fixed regulator. If in
> doubt, leave them out, they are not essential for the operation.
>
Will do for now, thanks.
>> +};
>> +
>> +/* the AXP717 has USB type-C role switch functionality, to be implemented */
>
> Replace "to be implemented" with "not yet described by the binding".
> This is DT land, so we don't care about implementations or the Linux
> kernel, it's all about DTs and DT bindings.
>
>> +&usbotg {
>> +	dr_mode = "host";   /* USB type-C receptable */
>
> So does this really work? It seems wrong to make this unconditional,
> given this is the only way to charge the device. When power is supplied
> through the USB-C port, surely driving VBUS from the board sounds
> wrong. Unless you have a killer feature for a host port, I think
> without working role switching, "peripheral" would be the safer
> choice.
>
>> +	status = "okay";
>> +};
>> +
>> +&usbphy {
>> +	usb0_vbus-supply = <&reg_vcc_usb>;
>
> When you stick to "peripheral" above, you should drop this line for
> now, especially since this regulator chain looks quite suspicious still.
>
Thanks, fixed.
> Cheers,
> Andre

Thanks again for the review! Will post up a v3 after some more thought about the regulators.

Ryan

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Andre Przywara]

On Sun, 21 Apr 2024 14:28:22 +1200
"Ryan Walklin" <ryan@testtoast.com> wrote:

Hi Ryan,

(please also see my reply to Chris' email, it was easier to reply there.)

> On Sun, 21 Apr 2024, at 12:49 PM, Andre Przywara wrote:
> > On Sat, 20 Apr 2024 22:43:56 +1200
> > Ryan Walklin <ryan@testtoast.com> wrote:
> >
> > Hi Ryan,
> >
> > many thanks for the respin and the changes! We are getting there ...  
> 
> Thanks for the review!
> 
> >> 
> >> [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
> >> [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t  
> >
> > As those are dependencies, but WIP, this gets a bit tricky:
> > - cpufreq is pretty far, but comes through a different tree. I suggest
> >   you drop the cpu-opp.dtsi include, to not complicate things. We can
> >   add this later as a fix, once this OPP file has reached the master
> >   tree.  
> 
> Done, thanks. Have also not increased the DCDC1 voltage to 1.16v for 1.5GHz as it's not yet working (and technically out of spec for the SoC) but will relook at the vendor BSP once this set is merged.

Yes, that sounds good to me, thanks!

> > - The NMI binding and DT node seem important, but have not been merged
> >   yet. I suggest to mention them as a requirement.   
> 
> Done, thanks.
> 
> > - The GPADC (in the later patch) is similar, but it is not as critical
> >   as the NMI.   
> 
> Will pull this out separately, as you say its only required for the joysticks on the -H.
> 
> >> +	reg_vcc_usb: regulator-vcc-5v0-usb {
> >> +		compatible = "regulator-fixed";
> >> +		enable-active-high;
> >> +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
> >> +		regulator-name = "vcc_5v0_usb";
> >> +		regulator-min-microvolt = <5000000>;
> >> +		regulator-max-microvolt = <5000000>;
> >> +		vin-supply = <&vcc_3v3_usb>;  
> >
> > This looks wrong, see below.
> >  
> >> +	};
> >> +
> >> +	vcc_3v3_usb: vcc-3v3-usb {
> >> +		compatible = "regulator-fixed";
> >> +		enable-active-high;
> >> +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
> >> +		regulator-always-on;
> >> +		regulator-boot-on;  
> >
> > There seems to be something off with this one. First, it seems odd that
> > an always-on regulator is GPIO controlled, as this surely means it's
> > not enabled at reset time (because the GPIO is initially HighZ and thus
> > not enabled). So who turns this on? Most likely it's the kernel? What
> > happens if we turn this off (or leave it off)?  
> 
> This makes more sense with that explanation, thanks. Taking a while to get my head round how the power distribution is done in the embedded space.
> 
> >
> > Secondly, why is this 3.3V (both by name and voltage), but then
> > supplies the 5.0V USB VBUS?
> > And given that this is chained with reg_vcc_usb above, are those really
> > two regulators, controlled by two GPIOs?
> >  
> >> +		regulator-name = "vcc_3v3_usb";
> >> +		regulator-max-microvolt = <3300000>;
> >> +		regulator-min-microvolt = <3300000>;
> >> +	};
> >> +
> >> +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "vcc-5v";
> >> +		regulator-min-microvolt = <5000000>;
> >> +		regulator-max-microvolt = <5000000>;
> >> +	};  
> 
> Will do some more work on this and try and figure it out. The USB and second SD are not working well currently, probably this is why.
> >> +
> >> +	reg_pll_dcx0: regulator-pll-dcx0 {  
> >
> > It's DCXO (letter "oh", not zero): digitally controlled/compensated
> > crystal *o*scillator.  
> 
> D'oh! Had to google PLL, should have googled this too.
> 
> >  
> >> +		compatible = "regulator-fixed";
> >> +		regulator-always-on;
> >> +		regulator-name = "vcc-pll";
> >> +		regulator-min-microvolt = <1800000>;
> >> +		regulator-max-microvolt = <1800000>;
> >> +	};  
> >
> > That one looks odd as well. While there might be a discrete regulator
> > (what this node is describing), I doubt it, since the AXP717 has plenty
> > of rails. In particular I am not sure if that fixed one would supply
> > PortG (mainly WiFi), which seems unneeded for the boot process, and
> > surely we want that switchable to save power if the WiFi is not needed.
> >
> > You also have a VCC-PLL regulator below (BLDO2).
> > So please try to drop this regulator, I am pretty sure there is an AXP
> > rail that powers the WiFi.  
> 
> Makes sense, will dig into the vendor BSP.
> >
> > See below for more on this.
> >  
> 
> >> +
> >> +&mmc2 {
> >> +	vmmc-supply = <&reg_vcc_sd2>;
> >> +	vqmmc-supply = <&reg_aldo1>;
> >> +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> >> +	bus-width = <4>;
> >> +	status = "okay";
> >> +};  
> >
> > This one seems more plausible. To confirm this, you could not use
> > reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
> > should break operation on the second SD card. Then just swap
> > reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
> >  
> In hindsight this is taken from the vendor BSP, so I think is correct but will do some testing.

Don't sweat it, if that's working, it's fine. If it's not entirely
accurate, we can fix that later.

> 
> >> +		x-powers,drive-vbus-en;  
> >
> > Remove this last line, the AXP717 binding does not support this, and the
> > Linux driver doesn't implement this, as the AXP717 does not seem to
> > have this functionality.  
> 
> Thanks, fixed.
> 
> >> +
> >> +			reg_aldo3: aldo3 {
> >> +				regulator-always-on;
> >> +				regulator-boot-on;  
> >
> > Please remove this last line, it doesn't make sense in this context. Cf.
> > Documentation//devicetree/bindings/regulator/regulator.yaml.
> > I think the same applies to the other uses throughout this file.
> >  
> >> +				regulator-min-microvolt = <500000>;
> >> +				regulator-max-microvolt = <3500000>;  
> >
> > This is not right. What is the voltage of this rail? The kernel should
> > tell you what was set in the register, through regulator_summary, or you
> > check what's the voltage on a BSP system.
> >  
> >> +				regulator-name = "axp717-aldo3";  
> >
> > If the system dies when you remove always-on, you might have found some
> > essential supply. Candidates for consumers are listed in the
> > H616 or T5 series datasheet. Matching the voltage might give you an
> > idea. Then use this consumer as the name.
> >  
> >> +			};
> >> +
> >> +			reg_aldo4: aldo4 {  
> >
> > Same for this one: Please remove regulator-boot-on, fix the voltage,
> > and provide some rationale why this needs to be on.  
> 
> Thanks, will get the correct voltages. It turns out ALDO3/4 are both needed, but CPUSLDO is not. Will try and figure out what they are supplying.
> 
> >> +
> >> +			reg_boost: boost {
> >> +				regulator-min-microvolt = <5126000>;
> >> +				regulator-max-microvolt = <5126000>;  
> >
> > It might be better to use a range, say 5.0V till 5.2V. The
> > kernel will then just continue using the default, which seems to be this
> > 5.126V (5440mV + 9 * 64mV).  
> 
> Thanks, fixed.

I saw in your git tree you kept the max at 5.126V. I think it would make
more sense to put 5.2V there, since this is the typical accepted high
voltage for VBUS, as issued by many power supplies, for instance. Also it
somewhat detaches that entry from the limitations of the AXP, and more
states what we *want*.
It wouldn't immediately make a difference anyways, since the kernel will
see that the voltage already programmed (5.126V) is within range, and will
just keep it.

Cheers,
Andre

> >> +
> >> +&pio {
> >> +	vcc-pg-supply = <&reg_pll_dcx0>;  
> >
> > As mentioned above, it seems unlikely to be a fixed regulator. If in
> > doubt, leave them out, they are not essential for the operation.
> >  
> Will do for now, thanks.
> >> +};
> >> +
> >> +/* the AXP717 has USB type-C role switch functionality, to be implemented */  
> >
> > Replace "to be implemented" with "not yet described by the binding".
> > This is DT land, so we don't care about implementations or the Linux
> > kernel, it's all about DTs and DT bindings.
> >  
> >> +&usbotg {
> >> +	dr_mode = "host";   /* USB type-C receptable */  
> >
> > So does this really work? It seems wrong to make this unconditional,
> > given this is the only way to charge the device. When power is supplied
> > through the USB-C port, surely driving VBUS from the board sounds
> > wrong. Unless you have a killer feature for a host port, I think
> > without working role switching, "peripheral" would be the safer
> > choice.
> >  
> >> +	status = "okay";
> >> +};
> >> +
> >> +&usbphy {
> >> +	usb0_vbus-supply = <&reg_vcc_usb>;  
> >
> > When you stick to "peripheral" above, you should drop this line for
> > now, especially since this regulator chain looks quite suspicious still.
> >  
> Thanks, fixed.
> > Cheers,
> > Andre  
> 
> Thanks again for the review! Will post up a v3 after some more thought about the regulators.
> 
> Ryan

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Chris Morgan]

On Sun, Apr 21, 2024 at 01:49:20AM +0100, Andre Przywara wrote:
> On Sat, 20 Apr 2024 22:43:56 +1200
> Ryan Walklin <ryan@testtoast.com> wrote:
> 
> Hi Ryan,
> 
> many thanks for the respin and the changes! We are getting there ...
> 
> > The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.
> > 
> > The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.
> > 
> > Device features:
> > - Allwinner H700 @ 1.5GHz
> > - 1GB LPDDR4 DRAM
> > - X-Powers AXP717 PMIC
> > - 3.5" 640x480 RGB LCD
> > - Two microSD slots
> > - Mini-HDMI out
> > - GPIO keypad
> > - 3.5mm headphone jack
> > 
> > Enabled in this DTS:
> > - AXP717 PMIC with regulators (interrupt controller TBC/TBD)
> > - Power LED (charge LED on device controlled directly by PMIC)
> > - Serial UART (accessible from PIN headers on the board)
> > - MMC slots
> > 
> > Changelog v1..v2:
> > - Update copyright
> > - Spaces -> Tabs
> > - Add cpufreq support [1]
> > - Remove MMC aliases
> > - Fix GPIO button and regulator node names
> > - Note unused AXP717 regulators
> > - Update regulator for SD slots
> > - Remove unused I2C3 device
> > - Update NMI interrupt controller for AXP717 [2]
> > - Add chassis-type
> > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > - Add PIO vcc-pg-supply
> > - Correct boost regulator voltage and name
> > 
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > 
> > [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
> > [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t
> 
> As those are dependencies, but WIP, this gets a bit tricky:
> - cpufreq is pretty far, but comes through a different tree. I suggest
>   you drop the cpu-opp.dtsi include, to not complicate things. We can
>   add this later as a fix, once this OPP file has reached the master
>   tree.
> - The NMI binding and DT node seem important, but have not been merged
>   yet. I suggest to mention them as a requirement. The patches (binding
>   plus H616 .dtsi change) should go through the sunxi tree as well, so
>   the maintainers can order this appropriately when merging.
> - The GPADC (in the later patch) is similar, but it is not as critical
>   as the NMI. Not sure how the maintainers want to handle this, but we
>   might add those bits as an (optional) patch on top, so this can be
>   handled more independently from the GPADC series.
> 
> > 
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > ---
> >  .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
> >  1 file changed, 375 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > new file mode 100644
> > index 000000000000..d8081273677d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > @@ -0,0 +1,375 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +/*
> > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sun50i-h616.dtsi"
> > +#include "sun50i-h616-cpu-opp.dtsi"
> 
> As mentioned, please drop this line for now.
> 
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/linux-event-codes.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/leds/common.h>
> > +
> > +/ {
> > +	model = "Anbernic RG35XX 2024";
> > +	chassis-type = "handset";

I've done my extensive testing on the 35XXH only so far, but based
on past experience I strongly doubt Anbernic did much different
between boards. Superficially the main differences besides form
factor are gpadc joysticks and an extra USB host port on the 35XXH,
so anything I have to say about USB look at with skepticsm.

> > +	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		led-0 {
> > +			function = LED_FUNCTION_POWER;
> > +			color = <LED_COLOR_ID_GREEN>;
> > +			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
> > +			default-state = "on";
> > +		};

FYI - PI12 can run as a PWM so when we get PWM output I intend to
request this be run as a PWM led (so we can adjust the brightness).
I'll handle that when it's ready though so don't worry for now.

> > +	};
> > +
> > +	gpio-keys {
> > +	   compatible = "gpio-keys";
> > +
> > +	   button-up {
> 
> indentation
> 
> > +		   label = "D-Pad Up";
> > +		   gpios = <&pio 0 6 GPIO_ACTIVE_LOW>; /* PA6 */
> > +		   linux,input-type = <EV_KEY>;
> > +		   linux,code = <BTN_DPAD_UP>;
> > +		};
> > +
> > +		button-down {
> > +			label = "D-Pad Down";
> > +			gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_DPAD_DOWN>;
> > +		};
> > +
> > +		button-left {
> > +			label = "D-Pad left";
> > +			gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_DPAD_LEFT>;
> > +		};
> > +
> > +		button-right {
> > +			label = "D-Pad Right";
> > +			gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_DPAD_RIGHT>;
> > +		};
> > +
> > +		button-a {
> > +			label = "Action-Pad A";
> > +			gpios = <&pio 0 0 GPIO_ACTIVE_LOW>; /* PA0 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_EAST>;
> > +		};
> > +
> > +		button-b {
> > +			label = "Action-Pad B";
> > +			gpios = <&pio 0 1 GPIO_ACTIVE_LOW>; /* PA1 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_SOUTH>;
> > +		};
> > +
> > +		button-x {
> > +			label = "Action-Pad X";
> > +			gpios = <&pio 0 3 GPIO_ACTIVE_LOW>; /* PA3 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_NORTH>;
> > +		};
> > +
> > +		button-y {
> > +			label = "Action Pad Y";
> > +			gpios = <&pio 0 2 GPIO_ACTIVE_LOW>; /* PA2 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_WEST>;
> > +		};
> > +
> > +		button-start {
> > +			label = "Key Start";
> > +			gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_START>;
> > +		};
> > +
> > +		button-select {
> > +			label = "Key Select";
> > +			gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_SELECT>;
> > +		};
> > +
> > +		button-l1 {
> > +			label = "Key L1";
> > +			gpios = <&pio 0 10 GPIO_ACTIVE_LOW>; /* PA10 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_TL>;
> > +		};
> > +
> > +		button-l2 {
> > +			label = "Key L2";
> > +			gpios = <&pio 0 11 GPIO_ACTIVE_LOW>; /* PA11 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_TL2>;
> > +		};
> > +
> > +		button-r1 {
> > +			label = "Key R1";
> > +			gpios = <&pio 0 12 GPIO_ACTIVE_LOW>; /* PA12 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_TR>;
> > +		};
> > +
> > +		button-r2 {
> > +			label = "Key R2";
> > +			gpios = <&pio 0 7 GPIO_ACTIVE_LOW>; /* PA7 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_TR2>;
> > +		};
> > +
> > +		button-menu {
> > +			label = "Key Menu";
> > +			gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_MODE>;
> > +		};
> > +

Your preference, but in the past I've had the volume up/down done as a
seperate node so we can enable key repeat. That way holding volume up
or down has the effect of continuously raising or lowering the volume.
It's desirable for volume buttons, but not for gamepads, hence the
separation.  Also I usually alphabetize node names, I don't remember
if I'm just that anal or if I was told to at one point though.

> > +		button-vol-up {
> > +			label = "Key Volume Up";
> > +			gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; /* PE1 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <KEY_VOLUMEUP>;
> > +		};
> > +
> > +		button-vol-down {
> > +			label = "Key Volume Down";
> > +			gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; /* PE2 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <KEY_VOLUMEDOWN>;
> > +		};
> > +	};
> > +
> > +	reg_vcc_sd2: regulator-vcc3v3 {
> > +		compatible = "regulator-fixed";
> > +		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> > +		regulator-name = "vcc_3v3_sd2";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +
> > +	reg_vcc_usb: regulator-vcc-5v0-usb {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
> > +		regulator-name = "vcc_5v0_usb";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc_3v3_usb>;
> 
> This looks wrong, see below.
> 
> > +	};
> > +
> > +	vcc_3v3_usb: vcc-3v3-usb {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
> > +		regulator-always-on;
> > +		regulator-boot-on;
> 
> There seems to be something off with this one. First, it seems odd that
> an always-on regulator is GPIO controlled, as this surely means it's
> not enabled at reset time (because the GPIO is initially HighZ and thus
> not enabled). So who turns this on? Most likely it's the kernel? What
> happens if we turn this off (or leave it off)?
> 
> Secondly, why is this 3.3V (both by name and voltage), but then
> supplies the 5.0V USB VBUS?
> And given that this is chained with reg_vcc_usb above, are those really
> two regulators, controlled by two GPIOs?
> 

It's described wrong, but based on the behaviour I've seen. The specific
seems to be 2 GPIO controlled regulators; one of them enables the logic
voltage for the USB (the 3.3v regulator) and the other enables the VBUS
for the USB (the 5v regulator). If you only enable the 5v regulator the
bus otherwise lays dormant. If you only enable the 3v3 regulator the
USB bus powers on and intermittently enumerates devices at 3.3v.
Further enabling the 5v regulator drives the USB at 4.6v (I'm guessing
something is still wrong, we have more to poke at).

To my knowledge this only applies for the USB host port which this
device lacks.


> > +		regulator-name = "vcc_3v3_usb";
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-min-microvolt = <3300000>;
> > +	};
> > +
> > +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc-5v";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +	};
> > +
> > +	reg_pll_dcx0: regulator-pll-dcx0 {
> 
> It's DCXO (letter "oh", not zero): digitally controlled/compensated
> crystal *o*scillator.
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-always-on;
> > +		regulator-name = "vcc-pll";
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +	};
> 
> That one looks odd as well. While there might be a discrete regulator
> (what this node is describing), I doubt it, since the AXP717 has plenty
> of rails. In particular I am not sure if that fixed one would supply
> PortG (mainly WiFi), which seems unneeded for the boot process, and
> surely we want that switchable to save power if the WiFi is not needed.
> 
> You also have a VCC-PLL regulator below (BLDO2).
> So please try to drop this regulator, I am pretty sure there is an AXP
> rail that powers the WiFi.
> 
> See below for more on this.

After much digging I'm certain the rails powering the wifi are
reg_cldo4 for the 3.3v and reg_aldo4 for the IO bits at 1.8v.
The curious thing about reg_cldo4 is it appears to feed into
another regulator that regulates at precisely 3.3v. If I under or
overvolt cldo4 I still read exactly 3.3v at the wifi module, but as
soon as I turn off reg_cldo4 the wifi reads 0v. So while I can't
directly control the power of the wifi's 3.3v rail, as long as I
get cldo4 close to 3.3v it powers correctly.

> 
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&reg_dcdc1>;
> > +};
> > +
> > +&mmc0 {
> > +	vmmc-supply = <&reg_vcc_sd2>;
> 
> I don't think this GPIO controlled regulator provides the supply voltage
> for the first SD card, since it would be disabled on reset, and the
> BROM couldn't boot from the SD card. So it must be some other 3.3V
> source, either a discrete regulator (fixed regulator), or some
> default-on 3.3V AXP rail.

Both the vcc and the logic for the mmc0 appear to be handled by the
cldo3 regulator at 3.3v.

> 
> > +	disable-wp;
> > +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> > +	bus-width = <4>;
> > +	status = "okay";
> > +};
> > +
> > +&mmc2 {
> > +	vmmc-supply = <&reg_vcc_sd2>;
> > +	vqmmc-supply = <&reg_aldo1>;
> > +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > +	bus-width = <4>;
> > +	status = "okay";
> > +};
> 
> This one seems more plausible. To confirm this, you could not use
> reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
> should break operation on the second SD card. Then just swap
> reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
> 

cldo3 and the gpio controlled regulator are the 2 regulators used for
mmc2 on my board. My notes have the vmmc supply as cldo3 and the
vqmmc supply as the GPIO controlled one, but that feels wrong. That
said the IO pins measured 1.1v when the GPIO regulator was off and
3.3v when the GPIO regulator was on. The 1.1v didn't seem to come
from the PMIC, as the only rail I had running 1.1v at the time was
the RAM and I tested and confirmed that wasn't it.

> > +
> > +&ohci0 {
> > +	status = "okay";
> > +};
> > +
> > +&ehci0 {
> > +	status = "okay";
> > +};
> > +

I haven't confirmed on my board we need ohci0 and ehci0 for the OTG
port.

> > +&r_rsb {
> > +   status = "okay";
> > +

Any advantage to doing this on the rsb over i2c? That's how I have mine
wired. Both are fine with me, I just didn't know what was better.

> > +   axp717: pmic@3a3 {
> > +		compatible = "x-powers,axp717";
> > +		reg = <0x3a3>;
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		interrupt-parent = <&nmi_intc>;
> > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +		x-powers,drive-vbus-en;
> 
> Remove this last line, the AXP717 binding does not support this, and the
> Linux driver doesn't implement this, as the AXP717 does not seem to
> have this functionality.
> 
> > +
> > +		vin1-supply = <&reg_vcc5v>;
> > +		vin2-supply = <&reg_vcc5v>;
> > +		vin3-supply = <&reg_vcc5v>;
> > +		vin4-supply = <&reg_vcc5v>;

FYI - I never actually confirmed the vin supply of the PMIC, I just put
these in here to shut up some errors or warnings. If these are based
on something I did it couldn't hurt to recheck.

> > +
> > +		regulators {
> > +			reg_dcdc1: dcdc1 {
> > +				regulator-always-on;
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <810000>;
> > +				regulator-max-microvolt = <1100000>;

The CPU opp table in the BSP device tree had this between 900000 and
1120000. Though like most things, take anything in the BSP device
tree with a grain of salt (lime and tequila help too).

> > +				regulator-name = "vdd-cpu";
> > +			};
> > +

I'm just speculating, but I strongly *guess* that this dcdc2 is used
for the GPU. SoC datasheet says max should be 900000 for the GPU, but
I don't have an opp table to go off of sadly.

> > +			reg_dcdc2: dcdc2 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <940000>;
> > +				regulator-max-microvolt = <940000>;
> > +				regulator-name = "vdd-sys";
> > +			};
> > +
> > +			reg_dcdc3: dcdc3 {
> > +				regulator-always-on;
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <1100000>;
> > +				regulator-max-microvolt = <1100000>;
> > +				regulator-name = "vdd-dram";
> > +			};
> > +
> > +			reg_aldo1: aldo1 {
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-name = "vcc-sd2";
> > +			};
> > +
> > +			reg_aldo2: aldo2 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_aldo3: aldo3 {
> > +				regulator-always-on;
> > +				regulator-boot-on;
> 
> Please remove this last line, it doesn't make sense in this context. Cf.
> Documentation//devicetree/bindings/regulator/regulator.yaml.
> I think the same applies to the other uses throughout this file.
> 
> > +				regulator-min-microvolt = <500000>;
> > +				regulator-max-microvolt = <3500000>;
> 
> This is not right. What is the voltage of this rail? The kernel should
> tell you what was set in the register, through regulator_summary, or you
> check what's the voltage on a BSP system.
> 
> > +				regulator-name = "axp717-aldo3";
> 
> If the system dies when you remove always-on, you might have found some
> essential supply. Candidates for consumers are listed in the
> H616 or T5 series datasheet. Matching the voltage might give you an
> idea. Then use this consumer as the name.
> 
> > +			};
> > +
> > +			reg_aldo4: aldo4 {
> 
> Same for this one: Please remove regulator-boot-on, fix the voltage,
> and provide some rationale why this needs to be on.

aldo4 is a critical regulator that should run at 1.8v based on my
testing.

> 
> > +				regulator-always-on;
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <500000>;
> > +				regulator-max-microvolt = <3500000>;
> > +				regulator-name = "axp717-aldo4";
> > +			};
> > +
> > +			reg_bldo1: bldo1 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_bldo2: bldo2 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <1800000>;
> > +				regulator-name = "vcc-pll";
> > +			};
> 
> This one is a good example: fixed voltage, no boot-on, and regulator
> name provides rationale why it must be always-on. The others should
> look similar.
> 
> > +
> > +			reg_bldo3: bldo3 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_bldo4: bldo4 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_cldo1: cldo1 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_cldo2: cldo2 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_cldo3: cldo3 {
> > +				regulator-always-on;
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <500000>;
> > +				regulator-max-microvolt = <3500000>;
> > +				regulator-name = "axp717-cldo3";
> 
> Same here as ALDO3/4: what voltage is it, and what does it probably
> supply?

cldo3 is a critical regulator run at 3.3v based on my testing. It
supplies power to the majority of the system's pins.

> 
> > +			};
> > +

cldo4 feeds the wifi, but through another fixed voltage regulator that
seems to pull up or down to 3.3v if we happen to raise or lower cldo4
a bit above or below 3.3v.

> > +			reg_cldo4: cldo4 {
> > +				regulator-min-microvolt = <3300000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-name = "vcc-wifi";
> > +			};
> > +
> > +			reg_boost: boost {
> > +				regulator-min-microvolt = <5126000>;
> > +				regulator-max-microvolt = <5126000>;
> 
> It might be better to use a range, say 5.0V till 5.2V. The
> kernel will then just continue using the default, which seems to be this
> 5.126V (5440mV + 9 * 64mV).
> 
> > +				regulator-name = "boost";
> > +			};
> > +
> > +			reg_cpusldo: cpusldo {
> > +				/* unused */
> 
> Is that so? I thought you mentioned on IRC this is required as well?
> 

I still haven't completely finished the regulator analysis, but I
actually think it is the case that this one isn't used.

> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_ph_pins>;
> > +	status = "okay";
> > +};
> > +
> > +&pio {
> > +	vcc-pg-supply = <&reg_pll_dcx0>;
> 
> As mentioned above, it seems unlikely to be a fixed regulator. If in
> doubt, leave them out, they are not essential for the operation.
> 

On my 35XXH I can confirm PG is supplied by reg_aldo4 at 1.8v and the
rest are supplied by reg_cldo3 at 3.3v.

        vcc-pa-supply = <&reg_cldo3>;
        vcc-pc-supply = <&reg_cldo3>;
        vcc-pe-supply = <&reg_cldo3>;
        vcc-pf-supply = <&reg_cldo3>;
        vcc-pg-supply = <&reg_aldo4>;
        vcc-ph-supply = <&reg_cldo3>;
        vcc-pi-supply = <&reg_cldo3>;

This is what my 35XXH looks like after manually raising or lowering the
PMIC voltage values a tad, observing the GPIO out voltages, adjusting
again, measuring the voltages again, etc etc. By checking at least 1
pin from each bank and confirming voltage changes via PMIC changes
I get this mapping.

> > +};
> > +
> > +/* the AXP717 has USB type-C role switch functionality, to be implemented */
> 
> Replace "to be implemented" with "not yet described by the binding".
> This is DT land, so we don't care about implementations or the Linux
> kernel, it's all about DTs and DT bindings.
> 
> > +&usbotg {
> > +	dr_mode = "host";   /* USB type-C receptable */
> 
> So does this really work? It seems wrong to make this unconditional,
> given this is the only way to charge the device. When power is supplied
> through the USB-C port, surely driving VBUS from the board sounds
> wrong. Unless you have a killer feature for a host port, I think
> without working role switching, "peripheral" would be the safer
> choice.

Again making me wish I'd have used a different device for my mainline
efforts, but on the 35XXH the OTG port I know does work for me as a
peripherial port. As for the Type-C stuff, I don't expect any future
AXP717 modifications to matter for this device, as manually fiddling
the USB-C bits in the PMIC didn't register my otg port as anything
other than a standard USB port (none of the USB-C specific
registers seemed to register anything).

> 
> > +	status = "okay";
> > +};
> > +
> > +&usbphy {
> > +	usb0_vbus-supply = <&reg_vcc_usb>;
> 
> When you stick to "peripheral" above, you should drop this line for
> now, especially since this regulator chain looks quite suspicious still.
> 

Concur, especially since now when I look at my 35XX+ 2024 model I only
see an OTG port and not a host port.

> Cheers,
> Andre
> 
> > +	status = "okay";
> > +};
> 

Thank you for all your hard work on this series. I'm going to continue
to try and identify the remaining regulators on my board and what they
do/how they're used. I expect at least one or two of the ones we've
flagged for removal will need to be added back once we get the panel
code working.

Chris

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Ryan Walklin]

On Sun, 21 Apr 2024, at 4:00 PM, Chris Morgan wrote:

> FYI - PI12 can run as a PWM so when we get PWM output I intend to
> request this be run as a PWM led (so we can adjust the brightness).
> I'll handle that when it's ready though so don't worry for now.

Sounds good.

> Your preference, but in the past I've had the volume up/down done as a
> seperate node so we can enable key repeat. 

Done as mentioned.

> separation.  Also I usually alphabetize node names, I don't remember
> if I'm just that anal or if I was told to at one point though.

I'm easy, the order there seemed logical based on the physical layout on the -Plus but I don't have a real preference.

>
> It's described wrong, but based on the behaviour I've seen. The specific
> seems to be 2 GPIO controlled regulators; one of them enables the logic
> voltage for the USB (the 3.3v regulator) and the other enables the VBUS
> for the USB (the 5v regulator). If you only enable the 5v regulator the
> bus otherwise lays dormant. If you only enable the 3v3 regulator the
> USB bus powers on and intermittently enumerates devices at 3.3v.
> Further enabling the 5v regulator drives the USB at 4.6v (I'm guessing
> something is still wrong, we have more to poke at).
>
> To my knowledge this only applies for the USB host port which this
> device lacks.
>
Will await your work on this, I'm not at all familiar with USB to this depth, conceptually makes sense though. This will also rely on the boost working well, which is at least now being enabled with the WIP AXP717 driver.


>
> After much digging I'm certain the rails powering the wifi are
> reg_cldo4 for the 3.3v and reg_aldo4 for the IO bits at 1.8v.
> The curious thing about reg_cldo4 is it appears to feed into
> another regulator that regulates at precisely 3.3v. If I under or
> overvolt cldo4 I still read exactly 3.3v at the wifi module, but as
> soon as I turn off reg_cldo4 the wifi reads 0v. So while I can't
> directly control the power of the wifi's 3.3v rail, as long as I
> get cldo4 close to 3.3v it powers correctly.

Nice, have reworked the SD and Wifi regulators as described, and power management for the Wifi in particular seems to be working much better after moving it off BLDO2 (which seems to be VCC-PLL) to CLDO4. 

>> > +&mmc0 {
>> > +	vmmc-supply = <&reg_vcc_sd2>;
>> 
>> I don't think this GPIO controlled regulator provides the supply voltage
>> for the first SD card, since it would be disabled on reset, and the
>> BROM couldn't boot from the SD card. So it must be some other 3.3V
>> source, either a discrete regulator (fixed regulator), or some
>> default-on 3.3V AXP rail.
>
> Both the vcc and the logic for the mmc0 appear to be handled by the
> cldo3 regulator at 3.3v.
>
Thanks, makes sense. I've tentatively renamed CLDO3 to VCC-IO, looking at the H616 datasheet.
>> 
>> > +	disable-wp;
>> > +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
>> > +	bus-width = <4>;
>> > +	status = "okay";
>> > +};
>> > +
>> > +&mmc2 {
>> > +	vmmc-supply = <&reg_vcc_sd2>;
>> > +	vqmmc-supply = <&reg_aldo1>;
>> > +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
>> > +	bus-width = <4>;
>> > +	status = "okay";
>> > +};
>> 
>> This one seems more plausible. To confirm this, you could not use
>> reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
>> should break operation on the second SD card. Then just swap
>> reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
>> 
Swapped in CLDO4 for MMC2 and the card read fine, will swap it back and see what happens.
>
> cldo3 and the gpio controlled regulator are the 2 regulators used for
> mmc2 on my board. My notes have the vmmc supply as cldo3 and the
> vqmmc supply as the GPIO controlled one, but that feels wrong. That
> said the IO pins measured 1.1v when the GPIO regulator was off and
> 3.3v when the GPIO regulator was on. The 1.1v didn't seem to come
> from the PMIC, as the only rail I had running 1.1v at the time was
> the RAM and I tested and confirmed that wasn't it.
>
>> > +
>> > +&ohci0 {
>> > +	status = "okay";
>> > +};
>> > +
>> > +&ehci0 {
>> > +	status = "okay";
>> > +};
>> > +
>
> I haven't confirmed on my board we need ohci0 and ehci0 for the OTG
> port.
>
OK, shall I remove them or will it do no harm?

>> > +&r_rsb {
>> > +   status = "okay";
>> > +
>
> Any advantage to doing this on the rsb over i2c? That's how I have mine
> wired. Both are fine with me, I just didn't know what was better.
>
I don't think so, this was Andre's first suggestion when he sent the AXP717's kernel driver so have stuck with it, but either should be fine.


>> 
>> > +
>> > +		vin1-supply = <&reg_vcc5v>;
>> > +		vin2-supply = <&reg_vcc5v>;
>> > +		vin3-supply = <&reg_vcc5v>;
>> > +		vin4-supply = <&reg_vcc5v>;
>
> FYI - I never actually confirmed the vin supply of the PMIC, I just put
> these in here to shut up some errors or warnings. If these are based
> on something I did it couldn't hurt to recheck.

Ta, will see if I can find anything out.

>
>> > +
>> > +		regulators {
>> > +			reg_dcdc1: dcdc1 {
>> > +				regulator-always-on;
>> > +				regulator-boot-on;
>> > +				regulator-min-microvolt = <810000>;
>> > +				regulator-max-microvolt = <1100000>;
>
> The CPU opp table in the BSP device tree had this between 900000 and
> 1120000. Though like most things, take anything in the BSP device
> tree with a grain of salt (lime and tequila help too).
>

Quite! That's the spec from the H616 datasheet, I may need to push it up to 1.16v max to hit 1.5Ghz, but will relook at it once the cpufreq patches land.

> I'm just speculating, but I strongly *guess* that this dcdc2 is used
> for the GPU. SoC datasheet says max should be 900000 for the GPU, but
> I don't have an opp table to go off of sadly.
>
>> > +			reg_dcdc2: dcdc2 {
>> > +				regulator-always-on;
>> > +				regulator-min-microvolt = <940000>;
>> > +				regulator-max-microvolt = <940000>;
>> > +				regulator-name = "vdd-sys";
>> > +			};

Thanks, I think I did know that from somewhere. My reading of the datasheet is 0.81-0.99v though, so will put those in.

>
> aldo4 is a critical regulator that should run at 1.8v based on my
> testing.
>
Yup, noted.

>
> cldo3 is a critical regulator run at 3.3v based on my testing. It
> supplies power to the majority of the system's pins.
>
Have called this VCC-IO for now.


> On my 35XXH I can confirm PG is supplied by reg_aldo4 at 1.8v and the
> rest are supplied by reg_cldo3 at 3.3v.
>
>         vcc-pa-supply = <&reg_cldo3>;
>         vcc-pc-supply = <&reg_cldo3>;
>         vcc-pe-supply = <&reg_cldo3>;
>         vcc-pf-supply = <&reg_cldo3>;
>         vcc-pg-supply = <&reg_aldo4>;
>         vcc-ph-supply = <&reg_cldo3>;
>         vcc-pi-supply = <&reg_cldo3>;
>
> This is what my 35XXH looks like after manually raising or lowering the
> PMIC voltage values a tad, observing the GPIO out voltages, adjusting
> again, measuring the voltages again, etc etc. By checking at least 1
> pin from each bank and confirming voltage changes via PMIC changes
> I get this mapping.
>

Great, have added these in. From the vendor DT looks like the audio codec is powered from the G pin bank.

> Thank you for all your hard work on this series. I'm going to continue
> to try and identify the remaining regulators on my board and what they
> do/how they're used. I expect at least one or two of the ones we've
> flagged for removal will need to be added back once we get the panel
> code working.
>
> Chris

No worries, thanks for the feedback! Much improved now, I think the last big issue here is the USB ports. Have been testing on my -H for now, but will have to look at the -Plus too given the single port.

Ryan

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Andre Przywara]

On Sun, 21 Apr 2024 20:43:26 +1200
"Ryan Walklin" <ryan@testtoast.com> wrote:

Hi,

> On Sun, 21 Apr 2024, at 4:00 PM, Chris Morgan wrote:
> 
> > FYI - PI12 can run as a PWM so when we get PWM output I intend to
> > request this be run as a PWM led (so we can adjust the brightness).
> > I'll handle that when it's ready though so don't worry for now.  
> 
> Sounds good.
> 
> > Your preference, but in the past I've had the volume up/down done as a
> > seperate node so we can enable key repeat.   
> 
> Done as mentioned.
> 
> > separation.  Also I usually alphabetize node names, I don't remember
> > if I'm just that anal or if I was told to at one point though.  
> 
> I'm easy, the order there seemed logical based on the physical layout on the -Plus but I don't have a real preference.
> 
> >
> > It's described wrong, but based on the behaviour I've seen. The specific
> > seems to be 2 GPIO controlled regulators; one of them enables the logic
> > voltage for the USB (the 3.3v regulator) and the other enables the VBUS
> > for the USB (the 5v regulator). If you only enable the 5v regulator the
> > bus otherwise lays dormant. If you only enable the 3v3 regulator the
> > USB bus powers on and intermittently enumerates devices at 3.3v.
> > Further enabling the 5v regulator drives the USB at 4.6v (I'm guessing
> > something is still wrong, we have more to poke at).
> >
> > To my knowledge this only applies for the USB host port which this
> > device lacks.
> >  
> Will await your work on this, I'm not at all familiar with USB to this depth, conceptually makes sense though. This will also rely on the boost working well, which is at least now being enabled with the WIP AXP717 driver.
> 
> 
> >
> > After much digging I'm certain the rails powering the wifi are
> > reg_cldo4 for the 3.3v and reg_aldo4 for the IO bits at 1.8v.
> > The curious thing about reg_cldo4 is it appears to feed into
> > another regulator that regulates at precisely 3.3v. If I under or
> > overvolt cldo4 I still read exactly 3.3v at the wifi module, but as
> > soon as I turn off reg_cldo4 the wifi reads 0v. So while I can't
> > directly control the power of the wifi's 3.3v rail, as long as I
> > get cldo4 close to 3.3v it powers correctly.  
> 
> Nice, have reworked the SD and Wifi regulators as described, and power management for the Wifi in particular seems to be working much better after moving it off BLDO2 (which seems to be VCC-PLL) to CLDO4. 
> 
> >> > +&mmc0 {
> >> > +	vmmc-supply = <&reg_vcc_sd2>;  
> >> 
> >> I don't think this GPIO controlled regulator provides the supply voltage
> >> for the first SD card, since it would be disabled on reset, and the
> >> BROM couldn't boot from the SD card. So it must be some other 3.3V
> >> source, either a discrete regulator (fixed regulator), or some
> >> default-on 3.3V AXP rail.  
> >
> > Both the vcc and the logic for the mmc0 appear to be handled by the
> > cldo3 regulator at 3.3v.
> >  
> Thanks, makes sense. I've tentatively renamed CLDO3 to VCC-IO, looking at the H616 datasheet.
> >>   
> >> > +	disable-wp;
> >> > +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> >> > +	bus-width = <4>;
> >> > +	status = "okay";
> >> > +};
> >> > +
> >> > +&mmc2 {
> >> > +	vmmc-supply = <&reg_vcc_sd2>;
> >> > +	vqmmc-supply = <&reg_aldo1>;
> >> > +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> >> > +	bus-width = <4>;
> >> > +	status = "okay";
> >> > +};  
> >> 
> >> This one seems more plausible. To confirm this, you could not use
> >> reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
> >> should break operation on the second SD card. Then just swap
> >> reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
> >>   
> Swapped in CLDO4 for MMC2 and the card read fine, will swap it back and see what happens.
> >
> > cldo3 and the gpio controlled regulator are the 2 regulators used for
> > mmc2 on my board. My notes have the vmmc supply as cldo3 and the
> > vqmmc supply as the GPIO controlled one, but that feels wrong. That
> > said the IO pins measured 1.1v when the GPIO regulator was off and
> > 3.3v when the GPIO regulator was on. The 1.1v didn't seem to come
> > from the PMIC, as the only rail I had running 1.1v at the time was
> > the RAM and I tested and confirmed that wasn't it.
> >  
> >> > +
> >> > +&ohci0 {
> >> > +	status = "okay";
> >> > +};
> >> > +
> >> > +&ehci0 {
> >> > +	status = "okay";
> >> > +};
> >> > +  
> >
> > I haven't confirmed on my board we need ohci0 and ehci0 for the OTG
> > port.
> >  
> OK, shall I remove them or will it do no harm?

For pure peripheral mode we don't need them, but it wouldn't harm to have
them anyway, and later on we will want to have them.

> >> > +&r_rsb {
> >> > +   status = "okay";
> >> > +  
> >
> > Any advantage to doing this on the rsb over i2c? That's how I have mine
> > wired. Both are fine with me, I just didn't know what was better.
> >  
> I don't think so, this was Andre's first suggestion when he sent the AXP717's kernel driver so have stuck with it, but either should be fine.

As mentioned in the other mail, please stick to RSB.

> >> > +
> >> > +		vin1-supply = <&reg_vcc5v>;
> >> > +		vin2-supply = <&reg_vcc5v>;
> >> > +		vin3-supply = <&reg_vcc5v>;
> >> > +		vin4-supply = <&reg_vcc5v>;  
> >
> > FYI - I never actually confirmed the vin supply of the PMIC, I just put
> > these in here to shut up some errors or warnings. If these are based
> > on something I did it couldn't hurt to recheck.  
> 
> Ta, will see if I can find anything out.
> 
> >  
> >> > +
> >> > +		regulators {
> >> > +			reg_dcdc1: dcdc1 {
> >> > +				regulator-always-on;
> >> > +				regulator-boot-on;
> >> > +				regulator-min-microvolt = <810000>;
> >> > +				regulator-max-microvolt = <1100000>;  
> >
> > The CPU opp table in the BSP device tree had this between 900000 and
> > 1120000. Though like most things, take anything in the BSP device
> > tree with a grain of salt (lime and tequila help too).
> >  
> 
> Quite! That's the spec from the H616 datasheet, I may need to push it up to 1.16v max to hit 1.5Ghz, but will relook at it once the cpufreq patches land.
> 
> > I'm just speculating, but I strongly *guess* that this dcdc2 is used
> > for the GPU. SoC datasheet says max should be 900000 for the GPU, but
> > I don't have an opp table to go off of sadly.
> >  
> >> > +			reg_dcdc2: dcdc2 {
> >> > +				regulator-always-on;
> >> > +				regulator-min-microvolt = <940000>;
> >> > +				regulator-max-microvolt = <940000>;
> >> > +				regulator-name = "vdd-sys";
> >> > +			};  
> 
> Thanks, I think I did know that from somewhere. My reading of the datasheet is 0.81-0.99v though, so will put those in.

Please try to be as precise as possible. The BSP used 0.94V, if I remember
the dumps correctly, so it's better to stick with that single value.
*Changing* such a critical voltage during runtime sounds like asking for
trouble, and given the recent experience with GPU DVFS on the A64, I'd say
we stick with a single voltage.
Also a range only makes sense if the consumer knows about that and
supports a change, examples are the SD card I/O voltage, or the CPU
supply, for DVFS. But most devices just *enable* their regulator. So this
would leave the voltage at whatever was set before, which does not sound
helpful if this was wrong or slightly off for whatever reason.

> > aldo4 is a critical regulator that should run at 1.8v based on my
> > testing.
> >  
> Yup, noted.
> 
> >
> > cldo3 is a critical regulator run at 3.3v based on my testing. It
> > supplies power to the majority of the system's pins.
> >  
> Have called this VCC-IO for now.

Yeah, that's a good name.

Cheers,
Andre

> > On my 35XXH I can confirm PG is supplied by reg_aldo4 at 1.8v and the
> > rest are supplied by reg_cldo3 at 3.3v.
> >
> >         vcc-pa-supply = <&reg_cldo3>;
> >         vcc-pc-supply = <&reg_cldo3>;
> >         vcc-pe-supply = <&reg_cldo3>;
> >         vcc-pf-supply = <&reg_cldo3>;
> >         vcc-pg-supply = <&reg_aldo4>;
> >         vcc-ph-supply = <&reg_cldo3>;
> >         vcc-pi-supply = <&reg_cldo3>;
> >
> > This is what my 35XXH looks like after manually raising or lowering the
> > PMIC voltage values a tad, observing the GPIO out voltages, adjusting
> > again, measuring the voltages again, etc etc. By checking at least 1
> > pin from each bank and confirming voltage changes via PMIC changes
> > I get this mapping.
> >  
> 
> Great, have added these in. From the vendor DT looks like the audio codec is powered from the G pin bank.
> 
> > Thank you for all your hard work on this series. I'm going to continue
> > to try and identify the remaining regulators on my board and what they
> > do/how they're used. I expect at least one or two of the ones we've
> > flagged for removal will need to be added back once we get the panel
> > code working.
> >
> > Chris  
> 
> No worries, thanks for the feedback! Much improved now, I think the last big issue here is the USB ports. Have been testing on my -H for now, but will have to look at the -Plus too given the single port.
> 
> Ryan

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Andre Przywara]

On Sat, 20 Apr 2024 23:00:15 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

Hi,

> On Sun, Apr 21, 2024 at 01:49:20AM +0100, Andre Przywara wrote:
> > On Sat, 20 Apr 2024 22:43:56 +1200
> > Ryan Walklin <ryan@testtoast.com> wrote:
> > 
> > Hi Ryan,
> > 
> > many thanks for the respin and the changes! We are getting there ...
> >   
> > > The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.
> > > 
> > > The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.
> > > 
> > > Device features:
> > > - Allwinner H700 @ 1.5GHz
> > > - 1GB LPDDR4 DRAM
> > > - X-Powers AXP717 PMIC
> > > - 3.5" 640x480 RGB LCD
> > > - Two microSD slots
> > > - Mini-HDMI out
> > > - GPIO keypad
> > > - 3.5mm headphone jack
> > > 
> > > Enabled in this DTS:
> > > - AXP717 PMIC with regulators (interrupt controller TBC/TBD)
> > > - Power LED (charge LED on device controlled directly by PMIC)
> > > - Serial UART (accessible from PIN headers on the board)
> > > - MMC slots
> > > 
> > > Changelog v1..v2:
> > > - Update copyright
> > > - Spaces -> Tabs
> > > - Add cpufreq support [1]
> > > - Remove MMC aliases
> > > - Fix GPIO button and regulator node names
> > > - Note unused AXP717 regulators
> > > - Update regulator for SD slots
> > > - Remove unused I2C3 device
> > > - Update NMI interrupt controller for AXP717 [2]
> > > - Add chassis-type
> > > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > > - Add PIO vcc-pg-supply
> > > - Correct boost regulator voltage and name
> > > 
> > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > 
> > > [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
> > > [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t  
> > 
> > As those are dependencies, but WIP, this gets a bit tricky:
> > - cpufreq is pretty far, but comes through a different tree. I suggest
> >   you drop the cpu-opp.dtsi include, to not complicate things. We can
> >   add this later as a fix, once this OPP file has reached the master
> >   tree.
> > - The NMI binding and DT node seem important, but have not been merged
> >   yet. I suggest to mention them as a requirement. The patches (binding
> >   plus H616 .dtsi change) should go through the sunxi tree as well, so
> >   the maintainers can order this appropriately when merging.
> > - The GPADC (in the later patch) is similar, but it is not as critical
> >   as the NMI. Not sure how the maintainers want to handle this, but we
> >   might add those bits as an (optional) patch on top, so this can be
> >   handled more independently from the GPADC series.
> >   
> > > 
> > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > ---
> > >  .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
> > >  1 file changed, 375 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > new file mode 100644
> > > index 000000000000..d8081273677d
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > @@ -0,0 +1,375 @@
> > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +/*
> > > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "sun50i-h616.dtsi"
> > > +#include "sun50i-h616-cpu-opp.dtsi"  
> > 
> > As mentioned, please drop this line for now.
> >   
> > > +#include <dt-bindings/gpio/gpio.h>
> > > +#include <dt-bindings/input/linux-event-codes.h>
> > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +#include <dt-bindings/leds/common.h>
> > > +
> > > +/ {
> > > +	model = "Anbernic RG35XX 2024";
> > > +	chassis-type = "handset";  
> 
> I've done my extensive testing on the 35XXH only so far, but based
> on past experience I strongly doubt Anbernic did much different
> between boards. Superficially the main differences besides form
> factor are gpadc joysticks and an extra USB host port on the 35XXH,
> so anything I have to say about USB look at with skepticsm.
> 
> > > +	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
> > > +
> > > +	aliases {
> > > +		serial0 = &uart0;
> > > +	};
> > > +
> > > +	chosen {
> > > +		stdout-path = "serial0:115200n8";
> > > +	};
> > > +
> > > +	leds {
> > > +		compatible = "gpio-leds";
> > > +
> > > +		led-0 {
> > > +			function = LED_FUNCTION_POWER;
> > > +			color = <LED_COLOR_ID_GREEN>;
> > > +			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
> > > +			default-state = "on";
> > > +		};  
> 
> FYI - PI12 can run as a PWM so when we get PWM output I intend to
> request this be run as a PWM led (so we can adjust the brightness).
> I'll handle that when it's ready though so don't worry for now.

Well, this has the bitter taste of requiring the PWM driver, so it wouldn't
work with current kernels. As the DTs are independent from the kernel,
this is some kind of regression: with the current DT it works on every
kernel, but using an updated DT (using PWM) the LED would stay off.
So it needs to be seen whether it's worth it. Maybe it works when U-Boot
powers this on, and Linux wouldn't touch it, when there is no PWM support.
Anyway that's indeed something to consider later, just wanted to mention
it.

> > > +	};
> > > +
> > > +	gpio-keys {
> > > +	   compatible = "gpio-keys";
> > > +
> > > +	   button-up {  
> > 
> > indentation
> >   
> > > +		   label = "D-Pad Up";
> > > +		   gpios = <&pio 0 6 GPIO_ACTIVE_LOW>; /* PA6 */
> > > +		   linux,input-type = <EV_KEY>;
> > > +		   linux,code = <BTN_DPAD_UP>;
> > > +		};
> > > +
> > > +		button-down {
> > > +			label = "D-Pad Down";
> > > +			gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_DPAD_DOWN>;
> > > +		};
> > > +
> > > +		button-left {
> > > +			label = "D-Pad left";
> > > +			gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_DPAD_LEFT>;
> > > +		};
> > > +
> > > +		button-right {
> > > +			label = "D-Pad Right";
> > > +			gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_DPAD_RIGHT>;
> > > +		};
> > > +
> > > +		button-a {
> > > +			label = "Action-Pad A";
> > > +			gpios = <&pio 0 0 GPIO_ACTIVE_LOW>; /* PA0 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_EAST>;
> > > +		};
> > > +
> > > +		button-b {
> > > +			label = "Action-Pad B";
> > > +			gpios = <&pio 0 1 GPIO_ACTIVE_LOW>; /* PA1 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_SOUTH>;
> > > +		};
> > > +
> > > +		button-x {
> > > +			label = "Action-Pad X";
> > > +			gpios = <&pio 0 3 GPIO_ACTIVE_LOW>; /* PA3 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_NORTH>;
> > > +		};
> > > +
> > > +		button-y {
> > > +			label = "Action Pad Y";
> > > +			gpios = <&pio 0 2 GPIO_ACTIVE_LOW>; /* PA2 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_WEST>;
> > > +		};
> > > +
> > > +		button-start {
> > > +			label = "Key Start";
> > > +			gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_START>;
> > > +		};
> > > +
> > > +		button-select {
> > > +			label = "Key Select";
> > > +			gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_SELECT>;
> > > +		};
> > > +
> > > +		button-l1 {
> > > +			label = "Key L1";
> > > +			gpios = <&pio 0 10 GPIO_ACTIVE_LOW>; /* PA10 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_TL>;
> > > +		};
> > > +
> > > +		button-l2 {
> > > +			label = "Key L2";
> > > +			gpios = <&pio 0 11 GPIO_ACTIVE_LOW>; /* PA11 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_TL2>;
> > > +		};
> > > +
> > > +		button-r1 {
> > > +			label = "Key R1";
> > > +			gpios = <&pio 0 12 GPIO_ACTIVE_LOW>; /* PA12 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_TR>;
> > > +		};
> > > +
> > > +		button-r2 {
> > > +			label = "Key R2";
> > > +			gpios = <&pio 0 7 GPIO_ACTIVE_LOW>; /* PA7 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_TR2>;
> > > +		};
> > > +
> > > +		button-menu {
> > > +			label = "Key Menu";
> > > +			gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_MODE>;
> > > +		};
> > > +  
> 
> Your preference, but in the past I've had the volume up/down done as a
> seperate node so we can enable key repeat. That way holding volume up
> or down has the effect of continuously raising or lowering the volume.
> It's desirable for volume buttons, but not for gamepads, hence the
> separation.  Also I usually alphabetize node names, I don't remember
> if I'm just that anal or if I was told to at one point though.

Typically some kind of ordering is preferred, to avoid confusion as to
where put new nodes. And if in doubt, this is indeed alphabetical.

> 
> > > +		button-vol-up {
> > > +			label = "Key Volume Up";
> > > +			gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; /* PE1 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <KEY_VOLUMEUP>;
> > > +		};
> > > +
> > > +		button-vol-down {
> > > +			label = "Key Volume Down";
> > > +			gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; /* PE2 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <KEY_VOLUMEDOWN>;
> > > +		};
> > > +	};
> > > +
> > > +	reg_vcc_sd2: regulator-vcc3v3 {
> > > +		compatible = "regulator-fixed";
> > > +		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> > > +		regulator-name = "vcc_3v3_sd2";
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +	};
> > > +
> > > +	reg_vcc_usb: regulator-vcc-5v0-usb {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
> > > +		regulator-name = "vcc_5v0_usb";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		vin-supply = <&vcc_3v3_usb>;  
> > 
> > This looks wrong, see below.
> >   
> > > +	};
> > > +
> > > +	vcc_3v3_usb: vcc-3v3-usb {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
> > > +		regulator-always-on;
> > > +		regulator-boot-on;  
> > 
> > There seems to be something off with this one. First, it seems odd that
> > an always-on regulator is GPIO controlled, as this surely means it's
> > not enabled at reset time (because the GPIO is initially HighZ and thus
> > not enabled). So who turns this on? Most likely it's the kernel? What
> > happens if we turn this off (or leave it off)?
> > 
> > Secondly, why is this 3.3V (both by name and voltage), but then
> > supplies the 5.0V USB VBUS?
> > And given that this is chained with reg_vcc_usb above, are those really
> > two regulators, controlled by two GPIOs?
> >   
> 
> It's described wrong, but based on the behaviour I've seen. The specific
> seems to be 2 GPIO controlled regulators; one of them enables the logic
> voltage for the USB (the 3.3v regulator)

As mentioned in the other mail, there are VCC-USB and VCC-USB2 pins on the
SoC, requiring 3.3V for the internal USB logic/PHY.
But it would be odd to see that on a GPIO controlled regulator, since IIUC
this is essential for USB operation, so would affect the FEL mode as well,
which is driven early by the BROM, knowing nothing about the board
specifics.
So I would expect any AXP rails (except those being on at reset) or GPIO
regulators only being responsible for *host mode* related power - FEL mode
does not need that, it's just peripheral mode. But since the AXP has reset
defaults, it might be an AXP rails that drives VCC-USB.

> and the other enables the VBUS
> for the USB (the 5v regulator). If you only enable the 5v regulator the
> bus otherwise lays dormant. If you only enable the 3v3 regulator the
> USB bus powers on and intermittently enumerates devices at 3.3v.
> Further enabling the 5v regulator drives the USB at 4.6v (I'm guessing
> something is still wrong, we have more to poke at).

As mentioned, 4.6V would make sense if that's the pass-through from the
input power. It's probably already not 5.0V anymore when it comes in, and
might drop further under load.

> To my knowledge this only applies for the USB host port which this
> device lacks.
> 
> 
> > > +		regulator-name = "vcc_3v3_usb";
> > > +		regulator-max-microvolt = <3300000>;
> > > +		regulator-min-microvolt = <3300000>;
> > > +	};
> > > +
> > > +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc-5v";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +	};
> > > +
> > > +	reg_pll_dcx0: regulator-pll-dcx0 {  
> > 
> > It's DCXO (letter "oh", not zero): digitally controlled/compensated
> > crystal *o*scillator.
> >   
> > > +		compatible = "regulator-fixed";
> > > +		regulator-always-on;
> > > +		regulator-name = "vcc-pll";
> > > +		regulator-min-microvolt = <1800000>;
> > > +		regulator-max-microvolt = <1800000>;
> > > +	};  
> > 
> > That one looks odd as well. While there might be a discrete regulator
> > (what this node is describing), I doubt it, since the AXP717 has plenty
> > of rails. In particular I am not sure if that fixed one would supply
> > PortG (mainly WiFi), which seems unneeded for the boot process, and
> > surely we want that switchable to save power if the WiFi is not needed.
> > 
> > You also have a VCC-PLL regulator below (BLDO2).
> > So please try to drop this regulator, I am pretty sure there is an AXP
> > rail that powers the WiFi.
> > 
> > See below for more on this.  
> 
> After much digging I'm certain the rails powering the wifi are
> reg_cldo4 for the 3.3v and reg_aldo4 for the IO bits at 1.8v.
> The curious thing about reg_cldo4 is it appears to feed into
> another regulator that regulates at precisely 3.3v. If I under or
> overvolt cldo4 I still read exactly 3.3v at the wifi module, but as
> soon as I turn off reg_cldo4 the wifi reads 0v. So while I can't
> directly control the power of the wifi's 3.3v rail, as long as I
> get cldo4 close to 3.3v it powers correctly.

That's interesting, but I highly doubt that Anbernic would slap an
expensive buck/boost converter to something uncritical as the WiFi supply.
So there might be something different at play here? Are you able to get
any hints from looking at the PCB and the traces? Those regulators are
typically easy to spot.

> > > +};
> > > +
> > > +&cpu0 {
> > > +	cpu-supply = <&reg_dcdc1>;
> > > +};
> > > +
> > > +&mmc0 {
> > > +	vmmc-supply = <&reg_vcc_sd2>;  
> > 
> > I don't think this GPIO controlled regulator provides the supply voltage
> > for the first SD card, since it would be disabled on reset, and the
> > BROM couldn't boot from the SD card. So it must be some other 3.3V
> > source, either a discrete regulator (fixed regulator), or some
> > default-on 3.3V AXP rail.  
> 
> Both the vcc and the logic for the mmc0 appear to be handled by the
> cldo3 regulator at 3.3v.

That makes sense, and would mean that CLDO3 is enabled on AXP reset and
set to 3.3V. The AXP datasheet is not helpful, and typically just refers
to "EFUSE" when it comes to the reset values.

Would you be able to poke the AXP early? Maybe drop the AXP driver from
U-Boot proper, and read the registers from U-Boot via the i2c command? To
see what the default values and enabled state is for all rails?

> > > +	disable-wp;
> > > +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> > > +	bus-width = <4>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&mmc2 {
> > > +	vmmc-supply = <&reg_vcc_sd2>;
> > > +	vqmmc-supply = <&reg_aldo1>;
> > > +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > > +	bus-width = <4>;
> > > +	status = "okay";
> > > +};  
> > 
> > This one seems more plausible. To confirm this, you could not use
> > reg_vcc_sd2 anywhere, and use some other 3.3V supply for vmmc, which
> > should break operation on the second SD card. Then just swap
> > reg_vcc_sd2 back in, and if it starts working again, we have confirmation.
> >   
> 
> cldo3 and the gpio controlled regulator are the 2 regulators used for
> mmc2 on my board. My notes have the vmmc supply as cldo3 and the
> vqmmc supply as the GPIO controlled one, but that feels wrong. That
> said the IO pins measured 1.1v when the GPIO regulator was off and
> 3.3v when the GPIO regulator was on. The 1.1v didn't seem to come
> from the PMIC, as the only rail I had running 1.1v at the time was
> the RAM and I tested and confirmed that wasn't it.

The easiest solution would just wire the PortC pins directly to the SD
card socket, and whatever VCC-PC is, would be used.
There is the possibility to have switchable level shifters between the SoC
and the SD card slot, to be able to switch between 1.8V and 3.3V, for
UHS-1 operation. But I doubt that Anbernic would go this way, since this
sounds more expensive, for little benefit.

> > > +
> > > +&ohci0 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&ehci0 {
> > > +	status = "okay";
> > > +};
> > > +  
> 
> I haven't confirmed on my board we need ohci0 and ehci0 for the OTG
> port.

So we don't need them for pure peripheral mode, but for host mode they are
needed. Technically the MUSB controller also supports host mode, but IIUC
Allwinner doesn't use this setup, and recommends the proper HCIs for host
mode.

> > > +&r_rsb {
> > > +   status = "okay";
> > > +  
> 
> Any advantage to doing this on the rsb over i2c? That's how I have mine
> wired. Both are fine with me, I just didn't know what was better.

In general we prefer RSB. If nothing else: it's faster (3MHz instead of
100/400 KHz), which gives a slight advantage for fast DVFS switching. Also
it's easier to program - each register write is a single fire-and-forget
setup in the RSB registers, as opposed to much more driver hand-holding
for I2C. That doesn't really matter for Linux (which supports both), but
helps TF-A, for instance.

> > > +   axp717: pmic@3a3 {
> > > +		compatible = "x-powers,axp717";
> > > +		reg = <0x3a3>;
> > > +		interrupt-controller;
> > > +		#interrupt-cells = <1>;
> > > +		interrupt-parent = <&nmi_intc>;
> > > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > +		x-powers,drive-vbus-en;  
> > 
> > Remove this last line, the AXP717 binding does not support this, and the
> > Linux driver doesn't implement this, as the AXP717 does not seem to
> > have this functionality.
> >   
> > > +
> > > +		vin1-supply = <&reg_vcc5v>;
> > > +		vin2-supply = <&reg_vcc5v>;
> > > +		vin3-supply = <&reg_vcc5v>;
> > > +		vin4-supply = <&reg_vcc5v>;  
> 
> FYI - I never actually confirmed the vin supply of the PMIC, I just put
> these in here to shut up some errors or warnings. If these are based
> on something I did it couldn't hurt to recheck.

I would assume the buck converters use the input power, since this is the
least current limited of all available, and the outputs promise quite some
current capability.
For the LDOs it might make sense to use a lower, closer matching input
voltage, so it only has to drop from say 3.3V to 1.8V, but for the DCDCs
using reg_vcc5v would make sense.

Though on second thought I wonder how this works when the device runs on
battery. Need to check the manual on how it handles the battery switch,
but I think for now it's safe to go with that above.
 
> > > +
> > > +		regulators {
> > > +			reg_dcdc1: dcdc1 {
> > > +				regulator-always-on;
> > > +				regulator-boot-on;
> > > +				regulator-min-microvolt = <810000>;
> > > +				regulator-max-microvolt = <1100000>;  
> 
> The CPU opp table in the BSP device tree had this between 900000 and
> 1120000. Though like most things, take anything in the BSP device
> tree with a grain of salt (lime and tequila help too).

You need lots of booze for anything BSP related ;-)

But indeed the lowest OPP voltage is 900mV, so it would make sense to go
with that.

> > > +				regulator-name = "vdd-cpu";
> > > +			};
> > > +  
> 
> I'm just speculating, but I strongly *guess* that this dcdc2 is used
> for the GPU. SoC datasheet says max should be 900000 for the GPU, but
> I don't have an opp table to go off of sadly.

It's probably used for both: on other H616 boards we call this regulator
"vdd-gpu-sys". The datasheet describes both voltages with the same
constraints, and since you need quite some oomph for both, and it's an odd
voltage, it makes sense to use one DCDC for this.

> > > +			reg_dcdc2: dcdc2 {
> > > +				regulator-always-on;
> > > +				regulator-min-microvolt = <940000>;
> > > +				regulator-max-microvolt = <940000>;
> > > +				regulator-name = "vdd-sys";
> > > +			};
> > > +
> > > +			reg_dcdc3: dcdc3 {
> > > +				regulator-always-on;
> > > +				regulator-boot-on;
> > > +				regulator-min-microvolt = <1100000>;
> > > +				regulator-max-microvolt = <1100000>;
> > > +				regulator-name = "vdd-dram";
> > > +			};
> > > +
> > > +			reg_aldo1: aldo1 {
> > > +				regulator-min-microvolt = <1800000>;
> > > +				regulator-max-microvolt = <3300000>;
> > > +				regulator-name = "vcc-sd2";
> > > +			};
> > > +
> > > +			reg_aldo2: aldo2 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_aldo3: aldo3 {
> > > +				regulator-always-on;
> > > +				regulator-boot-on;  
> > 
> > Please remove this last line, it doesn't make sense in this context. Cf.
> > Documentation//devicetree/bindings/regulator/regulator.yaml.
> > I think the same applies to the other uses throughout this file.
> >   
> > > +				regulator-min-microvolt = <500000>;
> > > +				regulator-max-microvolt = <3500000>;  
> > 
> > This is not right. What is the voltage of this rail? The kernel should
> > tell you what was set in the register, through regulator_summary, or you
> > check what's the voltage on a BSP system.
> >   
> > > +				regulator-name = "axp717-aldo3";  
> > 
> > If the system dies when you remove always-on, you might have found some
> > essential supply. Candidates for consumers are listed in the
> > H616 or T5 series datasheet. Matching the voltage might give you an
> > idea. Then use this consumer as the name.
> >   
> > > +			};
> > > +
> > > +			reg_aldo4: aldo4 {  
> > 
> > Same for this one: Please remove regulator-boot-on, fix the voltage,
> > and provide some rationale why this needs to be on.  
> 
> aldo4 is a critical regulator that should run at 1.8v based on my
> testing.

Thanks, that makes sense, since 1.8V is needed quite a lot.

> > > +				regulator-always-on;
> > > +				regulator-boot-on;
> > > +				regulator-min-microvolt = <500000>;
> > > +				regulator-max-microvolt = <3500000>;
> > > +				regulator-name = "axp717-aldo4";
> > > +			};
> > > +
> > > +			reg_bldo1: bldo1 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_bldo2: bldo2 {
> > > +				regulator-always-on;
> > > +				regulator-min-microvolt = <1800000>;
> > > +				regulator-max-microvolt = <1800000>;
> > > +				regulator-name = "vcc-pll";
> > > +			};  
> > 
> > This one is a good example: fixed voltage, no boot-on, and regulator
> > name provides rationale why it must be always-on. The others should
> > look similar.
> >   
> > > +
> > > +			reg_bldo3: bldo3 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_bldo4: bldo4 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_cldo1: cldo1 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_cldo2: cldo2 {
> > > +				/* unused */
> > > +			};
> > > +
> > > +			reg_cldo3: cldo3 {
> > > +				regulator-always-on;
> > > +				regulator-boot-on;
> > > +				regulator-min-microvolt = <500000>;
> > > +				regulator-max-microvolt = <3500000>;
> > > +				regulator-name = "axp717-cldo3";  
> > 
> > Same here as ALDO3/4: what voltage is it, and what does it probably
> > supply?  
> 
> cldo3 is a critical regulator run at 3.3v based on my testing. It
> supplies power to the majority of the system's pins.
> 
> >   
> > > +			};
> > > +  
> 
> cldo4 feeds the wifi, but through another fixed voltage regulator that
> seems to pull up or down to 3.3v if we happen to raise or lower cldo4
> a bit above or below 3.3v.
> 
> > > +			reg_cldo4: cldo4 {
> > > +				regulator-min-microvolt = <3300000>;
> > > +				regulator-max-microvolt = <3300000>;
> > > +				regulator-name = "vcc-wifi";
> > > +			};
> > > +
> > > +			reg_boost: boost {
> > > +				regulator-min-microvolt = <5126000>;
> > > +				regulator-max-microvolt = <5126000>;  
> > 
> > It might be better to use a range, say 5.0V till 5.2V. The
> > kernel will then just continue using the default, which seems to be this
> > 5.126V (5440mV + 9 * 64mV).
> >   
> > > +				regulator-name = "boost";
> > > +			};
> > > +
> > > +			reg_cpusldo: cpusldo {
> > > +				/* unused */  
> > 
> > Is that so? I thought you mentioned on IRC this is required as well?
> >   
> 
> I still haven't completely finished the regulator analysis, but I
> actually think it is the case that this one isn't used.

Right, fair enough: the H616/H700 does not have a management processor, so
the SoC probably does not need some kind of standby voltage.

> 
> > > +			};
> > > +		};
> > > +	};
> > > +};
> > > +
> > > +&uart0 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&uart0_ph_pins>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&pio {
> > > +	vcc-pg-supply = <&reg_pll_dcx0>;  
> > 
> > As mentioned above, it seems unlikely to be a fixed regulator. If in
> > doubt, leave them out, they are not essential for the operation.
> >   
> 
> On my 35XXH I can confirm PG is supplied by reg_aldo4 at 1.8v and the
> rest are supplied by reg_cldo3 at 3.3v.
> 
>         vcc-pa-supply = <&reg_cldo3>;
>         vcc-pc-supply = <&reg_cldo3>;
>         vcc-pe-supply = <&reg_cldo3>;
>         vcc-pf-supply = <&reg_cldo3>;
>         vcc-pg-supply = <&reg_aldo4>;
>         vcc-ph-supply = <&reg_cldo3>;
>         vcc-pi-supply = <&reg_cldo3>;
> 
> This is what my 35XXH looks like after manually raising or lowering the
> PMIC voltage values a tad, observing the GPIO out voltages, adjusting
> again, measuring the voltages again, etc etc. By checking at least 1
> pin from each bank and confirming voltage changes via PMIC changes
> I get this mapping.

Ah, many thanks for doing this - and sounds also fun, actually ;-)

> > > +};
> > > +
> > > +/* the AXP717 has USB type-C role switch functionality, to be implemented */  
> > 
> > Replace "to be implemented" with "not yet described by the binding".
> > This is DT land, so we don't care about implementations or the Linux
> > kernel, it's all about DTs and DT bindings.
> >   
> > > +&usbotg {
> > > +	dr_mode = "host";   /* USB type-C receptable */  
> > 
> > So does this really work? It seems wrong to make this unconditional,
> > given this is the only way to charge the device. When power is supplied
> > through the USB-C port, surely driving VBUS from the board sounds
> > wrong. Unless you have a killer feature for a host port, I think
> > without working role switching, "peripheral" would be the safer
> > choice.  
> 
> Again making me wish I'd have used a different device for my mainline
> efforts, but on the 35XXH the OTG port I know does work for me as a
> peripherial port. As for the Type-C stuff, I don't expect any future
> AXP717 modifications to matter for this device, as manually fiddling
> the USB-C bits in the PMIC didn't register my otg port as anything
> other than a standard USB port (none of the USB-C specific
> registers seemed to register anything).

Mmmh, interesting, though this might mean we don't understand this very
well yet. Because it looks like easy on the board side: just connect the
CC pins on the AXP to the CC pins on the connector, and the rest falls in
place?

Cheers,
Andre


> > > +	status = "okay";
> > > +};
> > > +
> > > +&usbphy {
> > > +	usb0_vbus-supply = <&reg_vcc_usb>;  
> > 
> > When you stick to "peripheral" above, you should drop this line for
> > now, especially since this regulator chain looks quite suspicious still.
> >   
> 
> Concur, especially since now when I look at my 35XX+ 2024 model I only
> see an OTG port and not a host port.
> 
> > Cheers,
> > Andre
> >   
> > > +	status = "okay";
> > > +};  
> >   
> 
> Thank you for all your hard work on this series. I'm going to continue
> to try and identify the remaining regulators on my board and what they
> do/how they're used. I expect at least one or two of the ones we've
> flagged for removal will need to be added back once we get the panel
> code working.
> 
> Chris

Re: [PATCH v2 2/5] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

[Jernej Škrabec]

Dne sobota, 20. april 2024 ob 12:43:56 GMT +2 je Ryan Walklin napisal(a):
> The base model RG35XX (2024) is a handheld gaming device based on an Allwinner H700 chip.
> 
> The H700 is a H616 variant (4x ARM Cortex-A53 cores @ 1.5Ghz with Mali G31 GPU) which exposes RGB LCD and NMI pins.
> 
> Device features:
> - Allwinner H700 @ 1.5GHz
> - 1GB LPDDR4 DRAM
> - X-Powers AXP717 PMIC
> - 3.5" 640x480 RGB LCD
> - Two microSD slots
> - Mini-HDMI out
> - GPIO keypad
> - 3.5mm headphone jack
> 
> Enabled in this DTS:
> - AXP717 PMIC with regulators (interrupt controller TBC/TBD)
> - Power LED (charge LED on device controlled directly by PMIC)
> - Serial UART (accessible from PIN headers on the board)
> - MMC slots
> 
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Add cpufreq support [1]
> - Remove MMC aliases
> - Fix GPIO button and regulator node names
> - Note unused AXP717 regulators
> - Update regulator for SD slots
> - Remove unused I2C3 device
> - Update NMI interrupt controller for AXP717 [2]
> - Add chassis-type
> - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> - Add PIO vcc-pg-supply
> - Correct boost regulator voltage and name
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> 
> [1]: https://lore.kernel.org/linux-sunxi/20240419071450.7aiheeoyq35yjoo7@vireshk-i7/T/#t
> [2]: https://lore.kernel.org/linux-sunxi/20240418181615.1370179-1-macroalpha82@gmail.com/T/#t

Put changelog and links in this case below --- line, so it won't be part
of the commit.

Best regards,
Jernej

> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../sun50i-h700-anbernic-rg35xx-2024.dts      | 375 ++++++++++++++++++
>  1 file changed, 375 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> new file mode 100644
> index 000000000000..d8081273677d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> + */
> +
> +/dts-v1/;
> +
> +#include "sun50i-h616.dtsi"
> +#include "sun50i-h616-cpu-opp.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/linux-event-codes.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/leds/common.h>
> +
> +/ {
> +	model = "Anbernic RG35XX 2024";
> +	chassis-type = "handset";
> +	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
> +
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-0 {
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_GREEN>;
> +			gpios = <&pio 8 12 GPIO_ACTIVE_HIGH>; /* PI12 */
> +			default-state = "on";
> +		};
> +	};
> +
> +	gpio-keys {
> +	   compatible = "gpio-keys";
> +
> +	   button-up {
> +		   label = "D-Pad Up";
> +		   gpios = <&pio 0 6 GPIO_ACTIVE_LOW>; /* PA6 */
> +		   linux,input-type = <EV_KEY>;
> +		   linux,code = <BTN_DPAD_UP>;
> +		};
> +
> +		button-down {
> +			label = "D-Pad Down";
> +			gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_DOWN>;
> +		};
> +
> +		button-left {
> +			label = "D-Pad left";
> +			gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_LEFT>;
> +		};
> +
> +		button-right {
> +			label = "D-Pad Right";
> +			gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_DPAD_RIGHT>;
> +		};
> +
> +		button-a {
> +			label = "Action-Pad A";
> +			gpios = <&pio 0 0 GPIO_ACTIVE_LOW>; /* PA0 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_EAST>;
> +		};
> +
> +		button-b {
> +			label = "Action-Pad B";
> +			gpios = <&pio 0 1 GPIO_ACTIVE_LOW>; /* PA1 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_SOUTH>;
> +		};
> +
> +		button-x {
> +			label = "Action-Pad X";
> +			gpios = <&pio 0 3 GPIO_ACTIVE_LOW>; /* PA3 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_NORTH>;
> +		};
> +
> +		button-y {
> +			label = "Action Pad Y";
> +			gpios = <&pio 0 2 GPIO_ACTIVE_LOW>; /* PA2 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_WEST>;
> +		};
> +
> +		button-start {
> +			label = "Key Start";
> +			gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_START>;
> +		};
> +
> +		button-select {
> +			label = "Key Select";
> +			gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_SELECT>;
> +		};
> +
> +		button-l1 {
> +			label = "Key L1";
> +			gpios = <&pio 0 10 GPIO_ACTIVE_LOW>; /* PA10 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TL>;
> +		};
> +
> +		button-l2 {
> +			label = "Key L2";
> +			gpios = <&pio 0 11 GPIO_ACTIVE_LOW>; /* PA11 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TL2>;
> +		};
> +
> +		button-r1 {
> +			label = "Key R1";
> +			gpios = <&pio 0 12 GPIO_ACTIVE_LOW>; /* PA12 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TR>;
> +		};
> +
> +		button-r2 {
> +			label = "Key R2";
> +			gpios = <&pio 0 7 GPIO_ACTIVE_LOW>; /* PA7 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_TR2>;
> +		};
> +
> +		button-menu {
> +			label = "Key Menu";
> +			gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_MODE>;
> +		};
> +
> +		button-vol-up {
> +			label = "Key Volume Up";
> +			gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; /* PE1 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <KEY_VOLUMEUP>;
> +		};
> +
> +		button-vol-down {
> +			label = "Key Volume Down";
> +			gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; /* PE2 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <KEY_VOLUMEDOWN>;
> +		};
> +	};
> +
> +	reg_vcc_sd2: regulator-vcc3v3 {
> +		compatible = "regulator-fixed";
> +		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> +		regulator-name = "vcc_3v3_sd2";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	reg_vcc_usb: regulator-vcc-5v0-usb {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&pio 8 7 GPIO_ACTIVE_HIGH>; /* PI7 */
> +		regulator-name = "vcc_5v0_usb";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc_3v3_usb>;
> +	};
> +
> +	vcc_3v3_usb: vcc-3v3-usb {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&pio 4 5 GPIO_ACTIVE_HIGH>; /* PE5 */
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-name = "vcc_3v3_usb";
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <3300000>;
> +	};
> +
> +	reg_vcc5v: regulator-vcc5v { /* USB-C power input */
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc-5v";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +
> +	reg_pll_dcx0: regulator-pll-dcx0 {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-name = "vcc-pll";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc1>;
> +};
> +
> +&mmc0 {
> +	vmmc-supply = <&reg_vcc_sd2>;
> +	disable-wp;
> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> +	bus-width = <4>;
> +	status = "okay";
> +};
> +
> +&mmc2 {
> +	vmmc-supply = <&reg_vcc_sd2>;
> +	vqmmc-supply = <&reg_aldo1>;
> +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> +	bus-width = <4>;
> +	status = "okay";
> +};
> +
> +&ohci0 {
> +	status = "okay";
> +};
> +
> +&ehci0 {
> +	status = "okay";
> +};
> +
> +&r_rsb {
> +   status = "okay";
> +
> +   axp717: pmic@3a3 {
> +		compatible = "x-powers,axp717";
> +		reg = <0x3a3>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupt-parent = <&nmi_intc>;
> +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +		x-powers,drive-vbus-en;
> +
> +		vin1-supply = <&reg_vcc5v>;
> +		vin2-supply = <&reg_vcc5v>;
> +		vin3-supply = <&reg_vcc5v>;
> +		vin4-supply = <&reg_vcc5v>;
> +
> +		regulators {
> +			reg_dcdc1: dcdc1 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <810000>;
> +				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-sys";
> +			};
> +
> +			reg_dcdc3: dcdc3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1100000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd-dram";
> +			};
> +
> +			reg_aldo1: aldo1 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-sd2";
> +			};
> +
> +			reg_aldo2: aldo2 {
> +				/* unused */
> +			};
> +
> +			reg_aldo3: aldo3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;
> +				regulator-name = "axp717-aldo3";
> +			};
> +
> +			reg_aldo4: aldo4 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;
> +				regulator-name = "axp717-aldo4";
> +			};
> +
> +			reg_bldo1: bldo1 {
> +				/* unused */
> +			};
> +
> +			reg_bldo2: bldo2 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-name = "vcc-pll";
> +			};
> +
> +			reg_bldo3: bldo3 {
> +				/* unused */
> +			};
> +
> +			reg_bldo4: bldo4 {
> +				/* unused */
> +			};
> +
> +			reg_cldo1: cldo1 {
> +				/* unused */
> +			};
> +
> +			reg_cldo2: cldo2 {
> +				/* unused */
> +			};
> +
> +			reg_cldo3: cldo3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <3500000>;
> +				regulator-name = "axp717-cldo3";
> +			};
> +
> +			reg_cldo4: cldo4 {
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-wifi";
> +			};
> +
> +			reg_boost: boost {
> +				regulator-min-microvolt = <5126000>;
> +				regulator-max-microvolt = <5126000>;
> +				regulator-name = "boost";
> +			};
> +
> +			reg_cpusldo: cpusldo {
> +				/* unused */
> +			};
> +		};
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_ph_pins>;
> +	status = "okay";
> +};
> +
> +&pio {
> +	vcc-pg-supply = <&reg_pll_dcx0>;
> +};
> +
> +/* the AXP717 has USB type-C role switch functionality, to be implemented */
> +&usbotg {
> +	dr_mode = "host";   /* USB type-C receptable */
> +	status = "okay";
> +};
> +
> +&usbphy {
> +	usb0_vbus-supply = <&reg_vcc_usb>;
> +	status = "okay";
> +};
> 

[PATCH v2 3/5] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS

[Ryan Walklin]

The RG35XX-Plus adds a RTL8221CS SDIO Wifi/BT chip to the RG35XX (2024).

Enabled in this DTS:
- WiFi
- Bluetooth
- Supporting power sequence and GPIOs

Changelog v1..v2:
- Update copyright
- Spaces -> Tabs
- Remove redundant &uart0 node and DTS version tag
- Add GPIO bank comments
- Use generic pwrseq name

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 .../sun50i-h700-anbernic-rg35xx-plus.dts      | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
new file mode 100644
index 000000000000..67ba1c7bb3ca
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
+ */
+
+#include "sun50i-h700-anbernic-rg35xx-2024.dts"
+
+/ {
+	model = "Anbernic RG35XX Plus";
+	compatible = "anbernic,rg35xx-plus", "allwinner,sun50i-h700";
+
+	wifi_pwrseq: pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		clocks = <&rtc CLK_OSC32K_FANOUT>;
+		clock-names = "ext_clock";
+		pinctrl-0 = <&x32clk_fanout_pin>;
+		pinctrl-names = "default";
+		post-power-on-delay-ms = <200>;
+		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
+	};
+};
+
+/* SDIO WiFi RTL8821CS */
+&mmc1 {
+	vmmc-supply = <&reg_cldo4>;
+	vqmmc-supply = <&reg_pll_dcx0>;
+	mmc-pwrseq = <&wifi_pwrseq>;
+	bus-width = <4>;
+	non-removable;
+	status = "okay";
+
+	sdio_wifi: wifi@1 {
+	   reg = <1>;
+	   interrupt-parent = <&pio>;
+	   interrupts = <6 15 IRQ_TYPE_LEVEL_LOW>; /* PG15 */
+	   interrupt-names = "host-wake";
+	};
+};
+
+/* Bluetooth RTL8821CS */
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+	uart-has-rtscts;
+	status = "okay";
+
+	bluetooth {
+		compatible = "realtek,rtl8821cs-bt", "realtek,rtl8723bs-bt";
+		device-wake-gpios = <&pio 6 17 GPIO_ACTIVE_HIGH>; /* PG17 */
+		enable-gpios = <&pio 6 19 GPIO_ACTIVE_HIGH>; /* PG19 */
+		host-wake-gpios = <&pio 6 16 GPIO_ACTIVE_HIGH>; /* PG16 */
+	};
+};
-- 
2.44.0

Re: [PATCH v2 3/5] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS

[Andre Przywara]

On Sat, 20 Apr 2024 22:43:57 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi,

> The RG35XX-Plus adds a RTL8221CS SDIO Wifi/BT chip to the RG35XX (2024).
> 
> Enabled in this DTS:
> - WiFi
> - Bluetooth
> - Supporting power sequence and GPIOs
> 
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Remove redundant &uart0 node and DTS version tag
> - Add GPIO bank comments
> - Use generic pwrseq name
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../sun50i-h700-anbernic-rg35xx-plus.dts      | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> new file mode 100644
> index 000000000000..67ba1c7bb3ca
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> + */
> +
> +#include "sun50i-h700-anbernic-rg35xx-2024.dts"
> +
> +/ {
> +	model = "Anbernic RG35XX Plus";
> +	compatible = "anbernic,rg35xx-plus", "allwinner,sun50i-h700";
> +
> +	wifi_pwrseq: pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		clocks = <&rtc CLK_OSC32K_FANOUT>;
> +		clock-names = "ext_clock";
> +		pinctrl-0 = <&x32clk_fanout_pin>;
> +		pinctrl-names = "default";
> +		post-power-on-delay-ms = <200>;
> +		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
> +	};
> +};
> +
> +/* SDIO WiFi RTL8821CS */
> +&mmc1 {
> +	vmmc-supply = <&reg_cldo4>;
> +	vqmmc-supply = <&reg_pll_dcx0>;

It would be great to confirm what the I/O voltage for MMC1/WiFi really
is, 1.8V or 3.3V? Is someone with a board able to measure this?

The rest looks good to me, thanks for the changes.

Cheers,
Andre

> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	bus-width = <4>;
> +	non-removable;
> +	status = "okay";
> +
> +	sdio_wifi: wifi@1 {
> +	   reg = <1>;
> +	   interrupt-parent = <&pio>;
> +	   interrupts = <6 15 IRQ_TYPE_LEVEL_LOW>; /* PG15 */
> +	   interrupt-names = "host-wake";
> +	};
> +};
> +
> +/* Bluetooth RTL8821CS */
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
> +	uart-has-rtscts;
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "realtek,rtl8821cs-bt", "realtek,rtl8723bs-bt";
> +		device-wake-gpios = <&pio 6 17 GPIO_ACTIVE_HIGH>; /* PG17 */
> +		enable-gpios = <&pio 6 19 GPIO_ACTIVE_HIGH>; /* PG19 */
> +		host-wake-gpios = <&pio 6 16 GPIO_ACTIVE_HIGH>; /* PG16 */
> +	};
> +};

Re: [PATCH v2 3/5] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS

[Chris Morgan]

On Sun, Apr 21, 2024 at 01:53:17AM +0100, Andre Przywara wrote:
> On Sat, 20 Apr 2024 22:43:57 +1200
> Ryan Walklin <ryan@testtoast.com> wrote:
> 
> Hi,
> 
> > The RG35XX-Plus adds a RTL8221CS SDIO Wifi/BT chip to the RG35XX (2024).
> > 
> > Enabled in this DTS:
> > - WiFi
> > - Bluetooth
> > - Supporting power sequence and GPIOs
> > 
> > Changelog v1..v2:
> > - Update copyright
> > - Spaces -> Tabs
> > - Remove redundant &uart0 node and DTS version tag
> > - Add GPIO bank comments
> > - Use generic pwrseq name
> > 
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > ---
> >  .../sun50i-h700-anbernic-rg35xx-plus.dts      | 53 +++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> > new file mode 100644
> > index 000000000000..67ba1c7bb3ca
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +/*
> > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > + */
> > +
> > +#include "sun50i-h700-anbernic-rg35xx-2024.dts"
> > +
> > +/ {
> > +	model = "Anbernic RG35XX Plus";
> > +	compatible = "anbernic,rg35xx-plus", "allwinner,sun50i-h700";
> > +
> > +	wifi_pwrseq: pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		clocks = <&rtc CLK_OSC32K_FANOUT>;
> > +		clock-names = "ext_clock";
> > +		pinctrl-0 = <&x32clk_fanout_pin>;
> > +		pinctrl-names = "default";
> > +		post-power-on-delay-ms = <200>;
> > +		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
> > +	};
> > +};
> > +
> > +/* SDIO WiFi RTL8821CS */
> > +&mmc1 {
> > +	vmmc-supply = <&reg_cldo4>;
> > +	vqmmc-supply = <&reg_pll_dcx0>;
> 
> It would be great to confirm what the I/O voltage for MMC1/WiFi really
> is, 1.8V or 3.3V? Is someone with a board able to measure this?
> 
> The rest looks good to me, thanks for the changes.

I have measured this on my 35XXH and can confirm both the mmc1 IO
voltage and the uart1 IO voltage is 1.8v. It's supplied by aldo4.

Thank you.
Chris

> 
> Cheers,
> Andre
> 
> > +	mmc-pwrseq = <&wifi_pwrseq>;
> > +	bus-width = <4>;
> > +	non-removable;
> > +	status = "okay";
> > +
> > +	sdio_wifi: wifi@1 {
> > +	   reg = <1>;
> > +	   interrupt-parent = <&pio>;
> > +	   interrupts = <6 15 IRQ_TYPE_LEVEL_LOW>; /* PG15 */
> > +	   interrupt-names = "host-wake";
> > +	};
> > +};
> > +
> > +/* Bluetooth RTL8821CS */
> > +&uart1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
> > +	uart-has-rtscts;
> > +	status = "okay";
> > +
> > +	bluetooth {
> > +		compatible = "realtek,rtl8821cs-bt", "realtek,rtl8723bs-bt";
> > +		device-wake-gpios = <&pio 6 17 GPIO_ACTIVE_HIGH>; /* PG17 */
> > +		enable-gpios = <&pio 6 19 GPIO_ACTIVE_HIGH>; /* PG19 */
> > +		host-wake-gpios = <&pio 6 16 GPIO_ACTIVE_HIGH>; /* PG16 */
> > +	};
> > +};
> 

[PATCH v2 4/5] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Ryan Walklin]

The RG35XX-H adds thumbsticks, a stereo speaker, and a second USB port to the RG35XX-Plus, and has a horizontal form factor.

Enabled in this DTS:
- Thumbsticks
- Second USB port

Changelog v1..v2:
- Update copyright
- Spaces -> Tabs
- Add GP ADC joystick axes and mux [1]
- Add EHCI/OHCI1 for second USB port and add vbus supply

Signed-off-by: Ryan Walklin <ryan@testtoast.com>

[1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 .../sun50i-h700-anbernic-rg35xx-h.dts         | 126 ++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
new file mode 100644
index 000000000000..d62cf5cd9d9b
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
@@ -0,0 +1,126 @@
+// 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 "sun50i-h700-anbernic-rg35xx-plus.dts"
+
+/ {
+	model = "Anbernic RG35XX H";
+	compatible = "anbernic,rg35xx-h", "allwinner,sun50i-h700";
+
+	adc-joystick {
+		compatible = "adc-joystick";
+		io-channels = <&adc_mux 0>,
+				  <&adc_mux 1>,
+				  <&adc_mux 2>,
+				  <&adc_mux 3>;
+		pinctrl-0 = <&joy_mux_pin>;
+		pinctrl-names = "default";
+		poll-interval = <60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		axis@0 {
+			reg = <0>;
+			abs-flat = <32>;
+			abs-fuzz = <32>;
+			abs-range = <4096 0>;
+			linux,code = <ABS_X>;
+		};
+
+		axis@1 {
+			reg = <1>;
+			abs-flat = <32>;
+			abs-fuzz = <32>;
+			abs-range = <0 4096>;
+			linux,code = <ABS_Y>;
+		};
+
+		axis@2 {
+			reg = <2>;
+			abs-flat = <32>;
+			abs-fuzz = <32>;
+			abs-range = <0 4096>;
+			linux,code = <ABS_RX>;
+		};
+
+		axis@3 {
+			reg = <3>;
+			abs-flat = <32>;
+			abs-fuzz = <32>;
+			abs-range = <4096 0>;
+			linux,code = <ABS_RY>;
+		};
+	};
+
+	adc_mux: adc-mux {
+		compatible = "io-channel-mux";
+		channels = "left_x", "left_y", "right_x", "right_y";
+		#io-channel-cells = <1>;
+		io-channels = <&gpadc 0>;
+		io-channel-names = "parent";
+		mux-controls = <&gpio_mux>;
+		settle-time-us = <100>;
+	};
+
+	gpio_keys: gpio-keys-thumb {
+		compatible = "gpio-keys";
+
+		button-thumbl {
+			label = "GPIO Thumb Left";
+			gpios = <&pio 4 8 GPIO_ACTIVE_LOW>; /* PE8 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_THUMBL>;
+		};
+
+		button-thumbr {
+			label = "GPIO Thumb Right";
+			gpios = <&pio 4 9 GPIO_ACTIVE_LOW>; /* PE9 */
+			linux,input-type = <EV_KEY>;
+			linux,code = <BTN_THUMBR>;
+		};
+	};
+
+	gpio_mux: mux-controller {
+		compatible = "gpio-mux";
+		mux-gpios = <&pio 8 1 GPIO_ACTIVE_LOW>,
+				<&pio 8 2 GPIO_ACTIVE_LOW>; /* PI1, PI2 */
+		#mux-control-cells = <0>;
+	};
+};
+
+&gpadc {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	channel@0 {
+		reg = <0>;
+	};
+};
+
+&pio {
+	joy_mux_pin: joy-mux-pin {
+		pins = "PI0";
+		function = "gpio_out";
+	};
+};
+
+&ehci1 {
+	status = "okay";
+};
+
+&ohci1 {
+	status = "okay";
+};
+
+&usbotg {
+	dr_mode = "peripheral";
+	status = "okay";
+};
+
+&usbphy {
+	usb1_vbus-supply = <&reg_vcc_usb>;
+};
-- 
2.44.0

Re: [PATCH v2 4/5] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Andre Przywara]

On Sat, 20 Apr 2024 22:43:58 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi,

> The RG35XX-H adds thumbsticks, a stereo speaker, and a second USB port to the RG35XX-Plus, and has a horizontal form factor.
> 
> Enabled in this DTS:
> - Thumbsticks
> - Second USB port
> 
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Add GP ADC joystick axes and mux [1]
> - Add EHCI/OHCI1 for second USB port and add vbus supply
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> 
> [1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t

As mention on patch 2/5, this might be better an optional dependency,
so the GPADC part might be a separate patch.

> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../sun50i-h700-anbernic-rg35xx-h.dts         | 126 ++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> new file mode 100644
> index 000000000000..d62cf5cd9d9b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> @@ -0,0 +1,126 @@
> +// 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 "sun50i-h700-anbernic-rg35xx-plus.dts"
> +
> +/ {
> +	model = "Anbernic RG35XX H";
> +	compatible = "anbernic,rg35xx-h", "allwinner,sun50i-h700";
> +
> +	adc-joystick {
> +		compatible = "adc-joystick";
> +		io-channels = <&adc_mux 0>,
> +				  <&adc_mux 1>,
> +				  <&adc_mux 2>,
> +				  <&adc_mux 3>;
> +		pinctrl-0 = <&joy_mux_pin>;
> +		pinctrl-names = "default";
> +		poll-interval = <60>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		axis@0 {
> +			reg = <0>;
> +			abs-flat = <32>;
> +			abs-fuzz = <32>;
> +			abs-range = <4096 0>;
> +			linux,code = <ABS_X>;
> +		};
> +
> +		axis@1 {
> +			reg = <1>;
> +			abs-flat = <32>;
> +			abs-fuzz = <32>;
> +			abs-range = <0 4096>;
> +			linux,code = <ABS_Y>;
> +		};
> +
> +		axis@2 {
> +			reg = <2>;
> +			abs-flat = <32>;
> +			abs-fuzz = <32>;
> +			abs-range = <0 4096>;
> +			linux,code = <ABS_RX>;
> +		};
> +
> +		axis@3 {
> +			reg = <3>;
> +			abs-flat = <32>;
> +			abs-fuzz = <32>;
> +			abs-range = <4096 0>;
> +			linux,code = <ABS_RY>;
> +		};
> +	};
> +
> +	adc_mux: adc-mux {
> +		compatible = "io-channel-mux";
> +		channels = "left_x", "left_y", "right_x", "right_y";
> +		#io-channel-cells = <1>;
> +		io-channels = <&gpadc 0>;
> +		io-channel-names = "parent";
> +		mux-controls = <&gpio_mux>;
> +		settle-time-us = <100>;
> +	};
> +
> +	gpio_keys: gpio-keys-thumb {

Is there any reason to not just use the existing gpio-keys node?
Either put a label on it in patch 2/5, and reference that below,
outside of the root node, or use an absolute path reference.

> +		compatible = "gpio-keys";
> +
> +		button-thumbl {
> +			label = "GPIO Thumb Left";
> +			gpios = <&pio 4 8 GPIO_ACTIVE_LOW>; /* PE8 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_THUMBL>;
> +		};
> +
> +		button-thumbr {
> +			label = "GPIO Thumb Right";
> +			gpios = <&pio 4 9 GPIO_ACTIVE_LOW>; /* PE9 */
> +			linux,input-type = <EV_KEY>;
> +			linux,code = <BTN_THUMBR>;
> +		};
> +	};
> +
> +	gpio_mux: mux-controller {
> +		compatible = "gpio-mux";
> +		mux-gpios = <&pio 8 1 GPIO_ACTIVE_LOW>,
> +				<&pio 8 2 GPIO_ACTIVE_LOW>; /* PI1, PI2 */
> +		#mux-control-cells = <0>;
> +	};
> +};
> +
> +&gpadc {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	channel@0 {
> +		reg = <0>;
> +	};
> +};
> +
> +&pio {
> +	joy_mux_pin: joy-mux-pin {
> +		pins = "PI0";
> +		function = "gpio_out";
> +	};
> +};
> +
> +&ehci1 {
> +	status = "okay";
> +};
> +
> +&ohci1 {
> +	status = "okay";
> +};
> +
> +&usbotg {
> +	dr_mode = "peripheral";
> +	status = "okay";
> +};
> +
> +&usbphy {
> +	usb1_vbus-supply = <&reg_vcc_usb>;

This is the dodgy USB supply chain. Any chance we can narrow this down,
to maybe one GPIO controlled regulator? Also, where does the boost
controller come into play here?

Cheers,
Andre

> +};

Re: [PATCH v2 4/5] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Chris Morgan]

On Sun, Apr 21, 2024 at 02:06:27AM +0100, Andre Przywara wrote:
> On Sat, 20 Apr 2024 22:43:58 +1200
> Ryan Walklin <ryan@testtoast.com> wrote:
> 
> Hi,
> 
> > The RG35XX-H adds thumbsticks, a stereo speaker, and a second USB port to the RG35XX-Plus, and has a horizontal form factor.
> > 
> > Enabled in this DTS:
> > - Thumbsticks
> > - Second USB port
> > 
> > Changelog v1..v2:
> > - Update copyright
> > - Spaces -> Tabs
> > - Add GP ADC joystick axes and mux [1]
> > - Add EHCI/OHCI1 for second USB port and add vbus supply
> > 
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > 
> > [1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t
> 
> As mention on patch 2/5, this might be better an optional dependency,
> so the GPADC part might be a separate patch.
> 
> > 
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > ---
> >  .../sun50i-h700-anbernic-rg35xx-h.dts         | 126 ++++++++++++++++++
> >  1 file changed, 126 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > new file mode 100644
> > index 000000000000..d62cf5cd9d9b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > @@ -0,0 +1,126 @@
> > +// 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 "sun50i-h700-anbernic-rg35xx-plus.dts"
> > +
> > +/ {
> > +	model = "Anbernic RG35XX H";
> > +	compatible = "anbernic,rg35xx-h", "allwinner,sun50i-h700";
> > +
> > +	adc-joystick {
> > +		compatible = "adc-joystick";
> > +		io-channels = <&adc_mux 0>,
> > +				  <&adc_mux 1>,
> > +				  <&adc_mux 2>,
> > +				  <&adc_mux 3>;
> > +		pinctrl-0 = <&joy_mux_pin>;
> > +		pinctrl-names = "default";
> > +		poll-interval = <60>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		axis@0 {
> > +			reg = <0>;
> > +			abs-flat = <32>;
> > +			abs-fuzz = <32>;
> > +			abs-range = <4096 0>;
> > +			linux,code = <ABS_X>;
> > +		};
> > +
> > +		axis@1 {
> > +			reg = <1>;
> > +			abs-flat = <32>;
> > +			abs-fuzz = <32>;
> > +			abs-range = <0 4096>;
> > +			linux,code = <ABS_Y>;
> > +		};
> > +
> > +		axis@2 {
> > +			reg = <2>;
> > +			abs-flat = <32>;
> > +			abs-fuzz = <32>;
> > +			abs-range = <0 4096>;
> > +			linux,code = <ABS_RX>;
> > +		};
> > +
> > +		axis@3 {
> > +			reg = <3>;
> > +			abs-flat = <32>;
> > +			abs-fuzz = <32>;
> > +			abs-range = <4096 0>;
> > +			linux,code = <ABS_RY>;
> > +		};
> > +	};
> > +
> > +	adc_mux: adc-mux {
> > +		compatible = "io-channel-mux";
> > +		channels = "left_x", "left_y", "right_x", "right_y";
> > +		#io-channel-cells = <1>;
> > +		io-channels = <&gpadc 0>;
> > +		io-channel-names = "parent";
> > +		mux-controls = <&gpio_mux>;
> > +		settle-time-us = <100>;
> > +	};
> > +
> > +	gpio_keys: gpio-keys-thumb {
> 
> Is there any reason to not just use the existing gpio-keys node?
> Either put a label on it in patch 2/5, and reference that below,
> outside of the root node, or use an absolute path reference.

I would also second just putting an alias and adding these to it.
I myself as a preference tend to set the GPIO volume buttons as
a seperate node only so I can enable key repeat on them, otherwise
one node is best.

> 
> > +		compatible = "gpio-keys";
> > +
> > +		button-thumbl {
> > +			label = "GPIO Thumb Left";
> > +			gpios = <&pio 4 8 GPIO_ACTIVE_LOW>; /* PE8 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_THUMBL>;
> > +		};
> > +
> > +		button-thumbr {
> > +			label = "GPIO Thumb Right";
> > +			gpios = <&pio 4 9 GPIO_ACTIVE_LOW>; /* PE9 */
> > +			linux,input-type = <EV_KEY>;
> > +			linux,code = <BTN_THUMBR>;
> > +		};
> > +	};
> > +
> > +	gpio_mux: mux-controller {
> > +		compatible = "gpio-mux";
> > +		mux-gpios = <&pio 8 1 GPIO_ACTIVE_LOW>,
> > +				<&pio 8 2 GPIO_ACTIVE_LOW>; /* PI1, PI2 */
> > +		#mux-control-cells = <0>;
> > +	};
> > +};
> > +
> > +&gpadc {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	status = "okay";
> > +
> > +	channel@0 {
> > +		reg = <0>;
> > +	};
> > +};
> > +
> > +&pio {

After extensive testing with a multimeter and fudging the regulator
voltages up or down, I've been able to figure out the regulator
assignments for each of the different power domains. Schematics would
have helped, but sadly this had to be done the hard way. Based on
past experience with Anbernic I would strongly suspect all devices
have this assignment, but I know for sure my  35XXH does.

        vcc-pa-supply = <&reg_cldo3>;
        vcc-pc-supply = <&reg_cldo3>;
        vcc-pe-supply = <&reg_cldo3>;
        vcc-pf-supply = <&reg_cldo3>;
        vcc-pg-supply = <&reg_aldo4>;
        vcc-ph-supply = <&reg_cldo3>;
        vcc-pi-supply = <&reg_cldo3>;

On my board the reg_cldo3 is a constant 3.3v output, and the reg_aldo4
is a constant 1.8v output.

> > +	joy_mux_pin: joy-mux-pin {
> > +		pins = "PI0";
> > +		function = "gpio_out";
> > +	};
> > +};
> > +
> > +&ehci1 {
> > +	status = "okay";
> > +};
> > +
> > +&ohci1 {
> > +	status = "okay";
> > +};
> > +
> > +&usbotg {
> > +	dr_mode = "peripheral";
> > +	status = "okay";
> > +};
> > +
> > +&usbphy {
> > +	usb1_vbus-supply = <&reg_vcc_usb>;
> 
> This is the dodgy USB supply chain. Any chance we can narrow this down,
> to maybe one GPIO controlled regulator? Also, where does the boost
> controller come into play here?

I haven't figured out the boost regulator yet, but for the host port
I've been able to ascertain there's no less than 2 GPIO controlled
regulators in play. PE5 must be driven high or the USB host port will
not power on at all. If PE5 is driven high then the port kicks on, but
at 3.3v. Once I also enable PI7 the port then reaches 4.6v. I'm not sure
how to get it to a proper 5v yet, I'm still working that part out.

Maybe PE5 is a reset of some kind that's active low, I don't know. I
just know so far/for sure that if PE5 is low then nothing registers on
the USB host port; if PE5 is high but PI7 is low the port sort-of works
at 3.3v, and if both PE5 and PI7 are high the port works consistently
and at 4.6v.

> 
> Cheers,
> Andre
> 
> > +};
> 

Thank you,
Chris

Re: [PATCH v2 4/5] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Ryan Walklin]

On Sun, 21 Apr 2024, at 3:09 PM, Chris Morgan wrote:

Thanks for the review and all the work on the regulators!

>> 
>> Is there any reason to not just use the existing gpio-keys node?
>> Either put a label on it in patch 2/5, and reference that below,
>> outside of the root node, or use an absolute path reference.
>
> I would also second just putting an alias and adding these to it.
> I myself as a preference tend to set the GPIO volume buttons as
> a seperate node only so I can enable key repeat on them, otherwise
> one node is best.
>
Thanks, have split the volume keys out and merged the thumbsticks into the existing node. 

>
> After extensive testing with a multimeter and fudging the regulator
> voltages up or down, I've been able to figure out the regulator
> assignments for each of the different power domains. Schematics would
> have helped, but sadly this had to be done the hard way. Based on
> past experience with Anbernic I would strongly suspect all devices
> have this assignment, but I know for sure my  35XXH does.
>
>         vcc-pa-supply = <&reg_cldo3>;
>         vcc-pc-supply = <&reg_cldo3>;
>         vcc-pe-supply = <&reg_cldo3>;
>         vcc-pf-supply = <&reg_cldo3>;
>         vcc-pg-supply = <&reg_aldo4>;
>         vcc-ph-supply = <&reg_cldo3>;
>         vcc-pi-supply = <&reg_cldo3>;
>
> On my board the reg_cldo3 is a constant 3.3v output, and the reg_aldo4
> is a constant 1.8v output.
>
Nice work! These work well in my testing.

>
> I haven't figured out the boost regulator yet, but for the host port
> I've been able to ascertain there's no less than 2 GPIO controlled
> regulators in play. PE5 must be driven high or the USB host port will
> not power on at all. If PE5 is driven high then the port kicks on, but
> at 3.3v. Once I also enable PI7 the port then reaches 4.6v. I'm not sure
> how to get it to a proper 5v yet, I'm still working that part out.
>
Thanks for working all that out, will hold off any other changes for now.

Ryan

Re: [PATCH v2 4/5] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Andre Przywara]

On Sat, 20 Apr 2024 22:09:40 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

Hi Chris,

many thanks for chiming in and doing all those experiments!

> On Sun, Apr 21, 2024 at 02:06:27AM +0100, Andre Przywara wrote:
> > On Sat, 20 Apr 2024 22:43:58 +1200
> > Ryan Walklin <ryan@testtoast.com> wrote:
> > 
> > Hi,
> >   
> > > The RG35XX-H adds thumbsticks, a stereo speaker, and a second USB port to the RG35XX-Plus, and has a horizontal form factor.
> > > 
> > > Enabled in this DTS:
> > > - Thumbsticks
> > > - Second USB port
> > > 
> > > Changelog v1..v2:
> > > - Update copyright
> > > - Spaces -> Tabs
> > > - Add GP ADC joystick axes and mux [1]
> > > - Add EHCI/OHCI1 for second USB port and add vbus supply
> > > 
> > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > 
> > > [1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t  
> > 
> > As mention on patch 2/5, this might be better an optional dependency,
> > so the GPADC part might be a separate patch.
> >   
> > > 
> > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > ---
> > >  .../sun50i-h700-anbernic-rg35xx-h.dts         | 126 ++++++++++++++++++
> > >  1 file changed, 126 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > > new file mode 100644
> > > index 000000000000..d62cf5cd9d9b
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > > @@ -0,0 +1,126 @@
> > > +// 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 "sun50i-h700-anbernic-rg35xx-plus.dts"
> > > +
> > > +/ {
> > > +	model = "Anbernic RG35XX H";
> > > +	compatible = "anbernic,rg35xx-h", "allwinner,sun50i-h700";
> > > +
> > > +	adc-joystick {
> > > +		compatible = "adc-joystick";
> > > +		io-channels = <&adc_mux 0>,
> > > +				  <&adc_mux 1>,
> > > +				  <&adc_mux 2>,
> > > +				  <&adc_mux 3>;
> > > +		pinctrl-0 = <&joy_mux_pin>;
> > > +		pinctrl-names = "default";
> > > +		poll-interval = <60>;
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +
> > > +		axis@0 {
> > > +			reg = <0>;
> > > +			abs-flat = <32>;
> > > +			abs-fuzz = <32>;
> > > +			abs-range = <4096 0>;
> > > +			linux,code = <ABS_X>;
> > > +		};
> > > +
> > > +		axis@1 {
> > > +			reg = <1>;
> > > +			abs-flat = <32>;
> > > +			abs-fuzz = <32>;
> > > +			abs-range = <0 4096>;
> > > +			linux,code = <ABS_Y>;
> > > +		};
> > > +
> > > +		axis@2 {
> > > +			reg = <2>;
> > > +			abs-flat = <32>;
> > > +			abs-fuzz = <32>;
> > > +			abs-range = <0 4096>;
> > > +			linux,code = <ABS_RX>;
> > > +		};
> > > +
> > > +		axis@3 {
> > > +			reg = <3>;
> > > +			abs-flat = <32>;
> > > +			abs-fuzz = <32>;
> > > +			abs-range = <4096 0>;
> > > +			linux,code = <ABS_RY>;
> > > +		};
> > > +	};
> > > +
> > > +	adc_mux: adc-mux {
> > > +		compatible = "io-channel-mux";
> > > +		channels = "left_x", "left_y", "right_x", "right_y";
> > > +		#io-channel-cells = <1>;
> > > +		io-channels = <&gpadc 0>;
> > > +		io-channel-names = "parent";
> > > +		mux-controls = <&gpio_mux>;
> > > +		settle-time-us = <100>;
> > > +	};
> > > +
> > > +	gpio_keys: gpio-keys-thumb {  
> > 
> > Is there any reason to not just use the existing gpio-keys node?
> > Either put a label on it in patch 2/5, and reference that below,
> > outside of the root node, or use an absolute path reference.  
> 
> I would also second just putting an alias and adding these to it.
> I myself as a preference tend to set the GPIO volume buttons as
> a seperate node only so I can enable key repeat on them, otherwise
> one node is best.
> 
> >   
> > > +		compatible = "gpio-keys";
> > > +
> > > +		button-thumbl {
> > > +			label = "GPIO Thumb Left";
> > > +			gpios = <&pio 4 8 GPIO_ACTIVE_LOW>; /* PE8 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_THUMBL>;
> > > +		};
> > > +
> > > +		button-thumbr {
> > > +			label = "GPIO Thumb Right";
> > > +			gpios = <&pio 4 9 GPIO_ACTIVE_LOW>; /* PE9 */
> > > +			linux,input-type = <EV_KEY>;
> > > +			linux,code = <BTN_THUMBR>;
> > > +		};
> > > +	};
> > > +
> > > +	gpio_mux: mux-controller {
> > > +		compatible = "gpio-mux";
> > > +		mux-gpios = <&pio 8 1 GPIO_ACTIVE_LOW>,
> > > +				<&pio 8 2 GPIO_ACTIVE_LOW>; /* PI1, PI2 */
> > > +		#mux-control-cells = <0>;
> > > +	};
> > > +};
> > > +
> > > +&gpadc {
> > > +	#address-cells = <1>;
> > > +	#size-cells = <0>;
> > > +	status = "okay";
> > > +
> > > +	channel@0 {
> > > +		reg = <0>;
> > > +	};
> > > +};
> > > +
> > > +&pio {  
> 
> After extensive testing with a multimeter and fudging the regulator
> voltages up or down, I've been able to figure out the regulator
> assignments for each of the different power domains. Schematics would
> have helped, but sadly this had to be done the hard way. Based on
> past experience with Anbernic I would strongly suspect all devices
> have this assignment, but I know for sure my  35XXH does.
> 
>         vcc-pa-supply = <&reg_cldo3>;
>         vcc-pc-supply = <&reg_cldo3>;
>         vcc-pe-supply = <&reg_cldo3>;
>         vcc-pf-supply = <&reg_cldo3>;
>         vcc-pg-supply = <&reg_aldo4>;
>         vcc-ph-supply = <&reg_cldo3>;
>         vcc-pi-supply = <&reg_cldo3>;
> 
> On my board the reg_cldo3 is a constant 3.3v output, and the reg_aldo4
> is a constant 1.8v output.

Ah, many thanks for doing this, this is indeed very helpful!
There are more power input pins on the SoC, some are essential, like the
already mentioned VCC-PLL, but also AVCC, I think. The datasheet should
give the voltages required for each. So if CLDO3 is 3.3V, and ALDO4 is
1.8V, both right from reset, then there is a chance that those rails are
(re-)used for those essential lines as well.

Also there are per-subsystem power supplies, like for HDMI and USB. For
HDMI the binding supports a -supply property, but for the USB-PHY we do
not (which should be fixed).
So there must be some 3.3V rail supplying VCC-USB, and that's NOT the VBUS
bus power, but needed for the PHY, as USB 1.x uses 3.3V on the data
lines. And even though this line is needed for USB only, we must mark it
as always-on, as there is no way to specify the consumer, as just
mentioned.

> > > +	joy_mux_pin: joy-mux-pin {
> > > +		pins = "PI0";
> > > +		function = "gpio_out";
> > > +	};
> > > +};
> > > +
> > > +&ehci1 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&ohci1 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&usbotg {
> > > +	dr_mode = "peripheral";
> > > +	status = "okay";
> > > +};
> > > +
> > > +&usbphy {
> > > +	usb1_vbus-supply = <&reg_vcc_usb>;  
> > 
> > This is the dodgy USB supply chain. Any chance we can narrow this down,
> > to maybe one GPIO controlled regulator? Also, where does the boost
> > controller come into play here?  
> 
> I haven't figured out the boost regulator yet, but for the host port
> I've been able to ascertain there's no less than 2 GPIO controlled
> regulators in play. PE5 must be driven high or the USB host port will
> not power on at all. If PE5 is driven high then the port kicks on, but
> at 3.3v. Once I also enable PI7 the port then reaches 4.6v. I'm not sure
> how to get it to a proper 5v yet, I'm still working that part out.
> 
> Maybe PE5 is a reset of some kind that's active low, I don't know. I
> just know so far/for sure that if PE5 is low then nothing registers on
> the USB host port; if PE5 is high but PI7 is low the port sort-of works
> at 3.3v, and if both PE5 and PI7 are high the port works consistently
> and at 4.6v.

Is that with some power provided through the other USB port? Then 4.6V
sound plausible.
Also I would assume there is some regulator or switch that toggles
VBUS between the PMIC input voltage and the boost regulator. The former
would be used when the device is powered through a USB cable, the latter
when it's on battery. I don't think it makes much sense to always use the
boost regulator, even when there is some "native" 5.0V available.
That being said I have no idea how to model this switch in the DT atm, but
maybe this helps with the understanding of the role of those two GPIOs?

Should PE5 really drive VCC-USB and VCC-USB2 on the SoC (I somewhat doubt
that, though), then we must mark this regulator as always-on, see above.

Cheers,
Andre

[PATCH v2 5/5] arm64: dts: allwinner: h700: Add RG35XX DTBs to Makefile

[Ryan Walklin]

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 arch/arm64/boot/dts/allwinner/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 21149b346a60..c2c871d8b71e 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -47,3 +47,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-longanpi-3h.dtb
 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
-- 
2.44.0

Re: [PATCH v2 5/5] arm64: dts: allwinner: h700: Add RG35XX DTBs to Makefile

[Andre Przywara]

On Sat, 20 Apr 2024 22:43:59 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi,

it looks unusual to have this as a separate patch. Please squash the
respective line into the patch that introduces each file.

Cheers,
Andre

> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  arch/arm64/boot/dts/allwinner/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 21149b346a60..c2c871d8b71e 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -47,3 +47,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-longanpi-3h.dtb
>  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