* [PATCH v3 0/4] *** SUBJECT HERE ***
@ 2024-04-24 11:09 Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 1/4] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants Ryan Walklin
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Ryan Walklin @ 2024-04-24 11:09 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Chris Morgan
Cc: devicetree, linux-sunxi, Ryan Walklin
*** BLURB HERE ***
Ryan Walklin (4):
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
.../devicetree/bindings/arm/sunxi.yaml | 15 +
arch/arm64/boot/dts/allwinner/Makefile | 3 +
.../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
.../sun50i-h700-anbernic-rg35xx-h.dts | 46 +++
.../sun50i-h700-anbernic-rg35xx-plus.dts | 53 +++
5 files changed, 464 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/4] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants
2024-04-24 11:09 [PATCH v3 0/4] *** SUBJECT HERE *** Ryan Walklin
@ 2024-04-24 11:09 ` Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS Ryan Walklin
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Ryan Walklin @ 2024-04-24 11:09 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Chris Morgan
Cc: devicetree, linux-sunxi, Ryan Walklin, Krzysztof Kozlowski
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
Use three separate device descriptions rather than enum as per existing sunxi binding style.
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-24 11:09 [PATCH v3 0/4] *** SUBJECT HERE *** Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 1/4] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants Ryan Walklin
@ 2024-04-24 11:09 ` Ryan Walklin
2024-04-25 0:25 ` Andre Przywara
2024-04-24 11:09 ` [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS Ryan Walklin
3 siblings, 1 reply; 14+ messages in thread
From: Ryan Walklin @ 2024-04-24 11:09 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Chris Morgan
Cc: devicetree, linux-sunxi, 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
- USB-C charging port
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
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
Changelog v1..v2:
- Update copyright
- Spaces -> Tabs
- Add cpufreq support
- Remove MMC aliases
- Fix GPIO button and regulator node names
- Note unused AXP717 regulators
- Update regulators for SD slots
- Remove unused I2C3 device
- Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
- Add chassis-type
- Address USB EHCI/OHCI0 correctly and add usb vbus supply
- Add PIO vcc-pg-supply
- Correct boost regulator voltage and name
Changelog v2..v3:
- Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
- Add dtb to Makefile
- Remove unnecessary duplicated PLL regulator
- Remove unimplemented/not-present drive-vbus feature from AXP717
- Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
- Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
- Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
- Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
- Fix indentation
- Correct boot/always-on states and voltages for various regulators from vendor BSP
- Change USB-OTG mode to "peripheral" and correct comment
- Correct and add remaining PIO supplies
- Move volume key GPIOs to separate block to allow key repeat
- Alphabetically orrder gamepad GPIOs
- Move changelog and links below fold-line
- Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
- Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
---
arch/arm64/boot/dts/allwinner/Makefile | 1 +
.../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
2 files changed, 348 insertions(+)
create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 21149b346a60..6f997157968e 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -47,3 +47,4 @@ 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
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..3b53485019f1
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
@@ -0,0 +1,347 @@
+// 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 <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_gamepad: gpio-keys-gamepad {
+ compatible = "gpio-keys";
+
+ 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-down {
+ label = "D-Pad Down";
+ gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_DPAD_DOWN>;
+ };
+
+ 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-left {
+ label = "D-Pad left";
+ gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_DPAD_LEFT>;
+ };
+
+ button-menu {
+ label = "Key Menu";
+ gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_MODE>;
+ };
+
+ 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-right {
+ label = "D-Pad Right";
+ gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_DPAD_RIGHT>;
+ };
+
+ button-select {
+ label = "Key Select";
+ gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_SELECT>;
+ };
+ button-start {
+ label = "Key Start";
+ gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
+ linux,input-type = <EV_KEY>;
+ linux,code = <BTN_START>;
+ };
+
+ 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-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>;
+ };
+ };
+
+ gpio-keys-volume {
+ compatible = "gpio-keys";
+ autorepeat;
+
+ 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_vcc5v: regulator-vcc5v { /* USB-C power input */
+ compatible = "regulator-fixed";
+ regulator-name = "vcc-5v";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ };
+};
+
+&cpu0 {
+ cpu-supply = <®_dcdc1>;
+};
+
+&mmc0 {
+ vmmc-supply = <®_cldo3>;
+ disable-wp;
+ cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
+ bus-width = <4>;
+ status = "okay";
+};
+
+&mmc2 {
+ vmmc-supply = <®_cldo4>;
+ vqmmc-supply = <®_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>;
+
+ vin1-supply = <®_vcc5v>;
+ vin2-supply = <®_vcc5v>;
+ vin3-supply = <®_vcc5v>;
+ vin4-supply = <®_vcc5v>;
+
+ regulators {
+ reg_dcdc1: dcdc1 {
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-name = "vdd-cpu";
+ };
+
+ reg_dcdc2: dcdc2 {
+ regulator-always-on;
+ regulator-min-microvolt = <940000>;
+ regulator-max-microvolt = <940000>;
+ regulator-name = "vdd-gpu-sys";
+ };
+
+ reg_dcdc3: dcdc3 {
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-name = "vdd-dram";
+ };
+
+ reg_aldo1: aldo1 { /* SDC2 */
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-name = "vcc-sd2";
+ };
+
+ reg_aldo2: aldo2 {
+ /* unused */
+ };
+
+ reg_aldo3: aldo3 {
+ regulator-always-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-name = "axp717-aldo3";
+ };
+
+ reg_aldo4: aldo4 { /* 5096000.codec */
+ regulator-always-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-name = "vcc-pg";
+ };
+
+ 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 { /* 5096000.codec */
+ /* unused */
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "axp717-cldo1";
+ };
+
+ reg_cldo2: cldo2 {
+ /* unused */
+
+ };
+
+ reg_cldo3: cldo3 {
+ regulator-always-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-io";
+ };
+
+ reg_cldo4: cldo4 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-wifi";
+ };
+
+ reg_boost: boost {
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5200000>;
+ regulator-name = "boost";
+ };
+
+ reg_cpusldo: cpusldo {
+ /* unused */
+ };
+ };
+ };
+};
+
+&uart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_ph_pins>;
+ status = "okay";
+};
+
+&pio {
+ vcc-pa-supply = <®_cldo3>;
+ vcc-pc-supply = <®_cldo3>;
+ vcc-pe-supply = <®_cldo3>;
+ vcc-pf-supply = <®_cldo3>;
+ vcc-pg-supply = <®_aldo4>;
+ vcc-ph-supply = <®_cldo3>;
+ vcc-pi-supply = <®_cldo3>;
+};
+
+/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
+&usbotg {
+ dr_mode = "peripheral"; /* USB type-C receptable */
+ status = "okay";
+};
+
+&usbphy {
+ status = "okay";
+};
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS
2024-04-24 11:09 [PATCH v3 0/4] *** SUBJECT HERE *** Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 1/4] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS Ryan Walklin
@ 2024-04-24 11:09 ` Ryan Walklin
2024-04-25 0:32 ` Andre Przywara
2024-04-24 11:09 ` [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS Ryan Walklin
3 siblings, 1 reply; 14+ messages in thread
From: Ryan Walklin @ 2024-04-24 11:09 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Chris Morgan
Cc: devicetree, linux-sunxi, 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
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
Changelog v1..v2:
- Update copyright
- Spaces -> Tabs
- Remove redundant &uart0 node and DTS version tag
- Add GPIO bank comments
- Use generic pwrseq name
Changelog v2..v3:
- Add DTB to Makefile
- Correct Wifi vqmmc-supply to ALDO4
- Move changelog below fold-line
---
arch/arm64/boot/dts/allwinner/Makefile | 1 +
.../sun50i-h700-anbernic-rg35xx-plus.dts | 53 +++++++++++++++++++
2 files changed, 54 insertions(+)
create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 6f997157968e..4217328b1889 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -48,3 +48,4 @@ 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
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..7e9d45eccb10
--- /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 = <®_cldo4>;
+ vqmmc-supply = <®_aldo4>;
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS
2024-04-24 11:09 [PATCH v3 0/4] *** SUBJECT HERE *** Ryan Walklin
` (2 preceding siblings ...)
2024-04-24 11:09 ` [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS Ryan Walklin
@ 2024-04-24 11:09 ` Ryan Walklin
2024-04-25 0:35 ` Andre Przywara
3 siblings, 1 reply; 14+ messages in thread
From: Ryan Walklin @ 2024-04-24 11:09 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Chris Morgan
Cc: devicetree, linux-sunxi, 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
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
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
Changelog v2..v3:
- Add DTB to Makefile
- Remove USB vbus supply
- Remove GPADC joysticks until required patches land [1]
- Move thumbsticks into existing gpio gamepad node
- Move changelog and links below fold-line
[1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t
---
arch/arm64/boot/dts/allwinner/Makefile | 1 +
.../sun50i-h700-anbernic-rg35xx-h.dts | 46 +++++++++++++++++++
2 files changed, 47 insertions(+)
create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 4217328b1889..c2c871d8b71e 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -49,3 +49,4 @@ 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
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..3f4435ff9b71
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
@@ -0,0 +1,46 @@
+// 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";
+};
+
+&gpio_keys_gamepad {
+
+ 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>;
+ };
+};
+
+&ehci1 {
+ status = "okay";
+};
+
+&ohci1 {
+ status = "okay";
+};
+
+&usbotg {
+ dr_mode = "peripheral";
+ status = "okay";
+};
+
+&usbphy {
+ status = "okay";
+};
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-24 11:09 ` [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS Ryan Walklin
@ 2024-04-25 0:25 ` Andre Przywara
2024-04-25 1:58 ` Chris Morgan
0 siblings, 1 reply; 14+ messages in thread
From: Andre Przywara @ 2024-04-25 0:25 UTC (permalink / raw)
To: Ryan Walklin, Chris Morgan
Cc: Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jernej Skrabec, Samuel Holland, devicetree, linux-sunxi
On Wed, 24 Apr 2024 23:09:45 +1200
Ryan Walklin <ryan@testtoast.com> wrote:
Hi Ryan (and Chris),
many thanks for the changes, that looks really close now. Only a few
smaller comments this time.
> 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
> - USB-C charging port
>
> 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
>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Add cpufreq support
> - Remove MMC aliases
> - Fix GPIO button and regulator node names
> - Note unused AXP717 regulators
> - Update regulators for SD slots
> - Remove unused I2C3 device
> - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> - Add chassis-type
> - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> - Add PIO vcc-pg-supply
> - Correct boost regulator voltage and name
>
> Changelog v2..v3:
> - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> - Add dtb to Makefile
> - Remove unnecessary duplicated PLL regulator
> - Remove unimplemented/not-present drive-vbus feature from AXP717
> - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> - Fix indentation
> - Correct boot/always-on states and voltages for various regulators from vendor BSP
> - Change USB-OTG mode to "peripheral" and correct comment
> - Correct and add remaining PIO supplies
> - Move volume key GPIOs to separate block to allow key repeat
> - Alphabetically orrder gamepad GPIOs
> - Move changelog and links below fold-line
> - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
>
> ---
> arch/arm64/boot/dts/allwinner/Makefile | 1 +
> .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> 2 files changed, 348 insertions(+)
> create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
>
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 21149b346a60..6f997157968e 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -47,3 +47,4 @@ 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
> 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..3b53485019f1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> @@ -0,0 +1,347 @@
> +// 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 <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_gamepad: gpio-keys-gamepad {
> + compatible = "gpio-keys";
> +
> + 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-down {
> + label = "D-Pad Down";
> + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_DPAD_DOWN>;
> + };
> +
> + 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-left {
> + label = "D-Pad left";
> + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_DPAD_LEFT>;
> + };
> +
> + button-menu {
> + label = "Key Menu";
> + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_MODE>;
> + };
> +
> + 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-right {
> + label = "D-Pad Right";
> + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_DPAD_RIGHT>;
> + };
> +
> + button-select {
> + label = "Key Select";
> + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_SELECT>;
> + };
> + button-start {
> + label = "Key Start";
> + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> + linux,input-type = <EV_KEY>;
> + linux,code = <BTN_START>;
> + };
> +
> + 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-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>;
> + };
> + };
> +
> + gpio-keys-volume {
> + compatible = "gpio-keys";
> + autorepeat;
> +
> + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> + compatible = "regulator-fixed";
> + regulator-name = "vcc-5v";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> +};
> +
> +&cpu0 {
> + cpu-supply = <®_dcdc1>;
> +};
> +
> +&mmc0 {
> + vmmc-supply = <®_cldo3>;
> + disable-wp;
> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> + bus-width = <4>;
> + status = "okay";
> +};
> +
> +&mmc2 {
> + vmmc-supply = <®_cldo4>;
> + vqmmc-supply = <®_aldo1>;
This is now fixed to 1.8V, which doesn't look right. Either it's not
the right regulator, or you should extend its range to cover 3.3V as
well.
> + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> + bus-width = <4>;
> + status = "okay";
> +};
> +
> +&ohci0 {
> + status = "okay";
> +};
> +
> +&ehci0 {
> + status = "okay";
> +};
> +
> +&r_rsb {
> + status = "okay";
This is indented with spaces, not a tab.
> +
> + axp717: pmic@3a3 {
> + compatible = "x-powers,axp717";
> + reg = <0x3a3>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + interrupt-parent = <&nmi_intc>;
> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +
> + vin1-supply = <®_vcc5v>;
> + vin2-supply = <®_vcc5v>;
> + vin3-supply = <®_vcc5v>;
> + vin4-supply = <®_vcc5v>;
> +
> + regulators {
> + reg_dcdc1: dcdc1 {
> + regulator-always-on;
> + regulator-boot-on;
boot-on doesn't make much sense here: that allows it to be turned off,
which we don't want. Also the binding documentation in regulator.yaml
says that it's only intended "where software cannot read the state of
the regulator", which is not true here.
regulator-always-on is all we need - technically speaking not even
that, since cpu0 is a consumer, but we need to play safe here.
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-name = "vdd-cpu";
> + };
> +
> + reg_dcdc2: dcdc2 {
> + regulator-always-on;
> + regulator-min-microvolt = <940000>;
> + regulator-max-microvolt = <940000>;
> + regulator-name = "vdd-gpu-sys";
> + };
> +
> + reg_dcdc3: dcdc3 {
> + regulator-always-on;
> + regulator-boot-on;
Same here, please remove that last line. We must not turn that off, and
the state is readable, so it's not needed. We need always-on here,
since it has no consumer.
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-name = "vdd-dram";
> + };
> +
> + reg_aldo1: aldo1 { /* SDC2 */
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "vcc-sd2";
> + };
> +
> + reg_aldo2: aldo2 {
> + /* unused */
> + };
> +
> + reg_aldo3: aldo3 {
> + regulator-always-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "axp717-aldo3";
So do we know for sure that's critical? And do we have any clue what
this supplies?
There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
are not critical.
> + };
> +
> + reg_aldo4: aldo4 { /* 5096000.codec */
> + regulator-always-on;
Is that necessary? What happens if that is turned off? Looks like only
the WiFi and potentially audio is affected? I think it can go then,
also pg-supply would reference it, so it would effectively be enabled
anyways.
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "vcc-pg";
> + };
> +
> + 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 { /* 5096000.codec */
> + /* unused */
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
Looks a bit odd to have an "unused" comment, but also a voltage range
specified. Judging from the comment this might be supplying some audio
circuitry, which we don't need at the moment?
The rest looks fine to me.
Cheers,
Andre
> + regulator-name = "axp717-cldo1";
> + };
> +
> + reg_cldo2: cldo2 {
> + /* unused */
> +
> + };
> +
> + reg_cldo3: cldo3 {
> + regulator-always-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-io";
> + };
> +
> + reg_cldo4: cldo4 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-wifi";
> + };
> +
> + reg_boost: boost {
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5200000>;
> + regulator-name = "boost";
> + };
> +
> + reg_cpusldo: cpusldo {
> + /* unused */
> + };
> + };
> + };
> +};
> +
> +&uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_ph_pins>;
> + status = "okay";
> +};
> +
> +&pio {
> + vcc-pa-supply = <®_cldo3>;
> + vcc-pc-supply = <®_cldo3>;
> + vcc-pe-supply = <®_cldo3>;
> + vcc-pf-supply = <®_cldo3>;
> + vcc-pg-supply = <®_aldo4>;
> + vcc-ph-supply = <®_cldo3>;
> + vcc-pi-supply = <®_cldo3>;
> +};
> +
> +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> +&usbotg {
> + dr_mode = "peripheral"; /* USB type-C receptable */
> + status = "okay";
> +};
> +
> +&usbphy {
> + status = "okay";
> +};
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS
2024-04-24 11:09 ` [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS Ryan Walklin
@ 2024-04-25 0:32 ` Andre Przywara
0 siblings, 0 replies; 14+ messages in thread
From: Andre Przywara @ 2024-04-25 0:32 UTC (permalink / raw)
To: Ryan Walklin
Cc: Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jernej Skrabec, Samuel Holland, Chris Morgan, devicetree,
linux-sunxi
On Wed, 24 Apr 2024 23:09:46 +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
>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
> Changelog v1..v2:
> - Update copyright
> - Spaces -> Tabs
> - Remove redundant &uart0 node and DTS version tag
> - Add GPIO bank comments
> - Use generic pwrseq name
>
> Changelog v2..v3:
> - Add DTB to Makefile
> - Correct Wifi vqmmc-supply to ALDO4
> - Move changelog below fold-line
>
> ---
> arch/arm64/boot/dts/allwinner/Makefile | 1 +
> .../sun50i-h700-anbernic-rg35xx-plus.dts | 53 +++++++++++++++++++
> 2 files changed, 54 insertions(+)
> create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
>
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 6f997157968e..4217328b1889 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -48,3 +48,4 @@ 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
> 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..7e9d45eccb10
> --- /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 = <®_cldo4>;
> + vqmmc-supply = <®_aldo4>;
> + 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";
Those four lines here need to be indented with two tabs.
With that fixed:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> + };
> +};
> +
> +/* 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 */
> + };
> +};
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS
2024-04-24 11:09 ` [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS Ryan Walklin
@ 2024-04-25 0:35 ` Andre Przywara
0 siblings, 0 replies; 14+ messages in thread
From: Andre Przywara @ 2024-04-25 0:35 UTC (permalink / raw)
To: Ryan Walklin
Cc: Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jernej Skrabec, Samuel Holland, Chris Morgan, devicetree,
linux-sunxi
On Wed, 24 Apr 2024 23:09:47 +1200
Ryan Walklin <ryan@testtoast.com> wrote:
> 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
>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
> 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
>
> Changelog v2..v3:
> - Add DTB to Makefile
> - Remove USB vbus supply
> - Remove GPADC joysticks until required patches land [1]
> - Move thumbsticks into existing gpio gamepad node
> - Move changelog and links below fold-line
>
> [1]: https://lore.kernel.org/linux-sunxi/20240417170423.20640-1-macroalpha82@gmail.com/T/#t
>
> ---
> arch/arm64/boot/dts/allwinner/Makefile | 1 +
> .../sun50i-h700-anbernic-rg35xx-h.dts | 46 +++++++++++++++++++
> 2 files changed, 47 insertions(+)
> create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
>
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 4217328b1889..c2c871d8b71e 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -49,3 +49,4 @@ 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
> 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..3f4435ff9b71
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> @@ -0,0 +1,46 @@
> +// 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";
> +};
> +
> +&gpio_keys_gamepad {
> +
> + 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>;
> + };
> +};
> +
> +&ehci1 {
> + status = "okay";
> +};
> +
> +&ohci1 {
> + status = "okay";
> +};
> +
> +&usbotg {
> + dr_mode = "peripheral";
> + status = "okay";
> +};
> +
> +&usbphy {
> + status = "okay";
> +};
Those two nodes look redundant, since they exist already in the base
.dts. I guess &usbphy needs to be adjusted later, but &usbotg is likely
to stay the same? Anyway, it doesn't really hurt, so:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 0:25 ` Andre Przywara
@ 2024-04-25 1:58 ` Chris Morgan
2024-04-25 5:39 ` Ryan Walklin
2024-04-25 10:08 ` Andre Przywara
0 siblings, 2 replies; 14+ messages in thread
From: Chris Morgan @ 2024-04-25 1:58 UTC (permalink / raw)
To: Andre Przywara
Cc: Ryan Walklin, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, devicetree,
linux-sunxi
On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
> On Wed, 24 Apr 2024 23:09:45 +1200
> Ryan Walklin <ryan@testtoast.com> wrote:
>
> Hi Ryan (and Chris),
>
> many thanks for the changes, that looks really close now. Only a few
> smaller comments this time.
>
> > 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
> > - USB-C charging port
> >
> > 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
> >
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > ---
> > Changelog v1..v2:
> > - Update copyright
> > - Spaces -> Tabs
> > - Add cpufreq support
> > - Remove MMC aliases
> > - Fix GPIO button and regulator node names
> > - Note unused AXP717 regulators
> > - Update regulators for SD slots
> > - Remove unused I2C3 device
> > - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> > - Add chassis-type
> > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > - Add PIO vcc-pg-supply
> > - Correct boost regulator voltage and name
> >
> > Changelog v2..v3:
> > - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> > - Add dtb to Makefile
> > - Remove unnecessary duplicated PLL regulator
> > - Remove unimplemented/not-present drive-vbus feature from AXP717
> > - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> > - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> > - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> > - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> > - Fix indentation
> > - Correct boot/always-on states and voltages for various regulators from vendor BSP
> > - Change USB-OTG mode to "peripheral" and correct comment
> > - Correct and add remaining PIO supplies
> > - Move volume key GPIOs to separate block to allow key repeat
> > - Alphabetically orrder gamepad GPIOs
> > - Move changelog and links below fold-line
> > - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> > - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
> >
> > ---
> > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> > 2 files changed, 348 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > index 21149b346a60..6f997157968e 100644
> > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > @@ -47,3 +47,4 @@ 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
> > 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..3b53485019f1
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > @@ -0,0 +1,347 @@
> > +// 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 <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_gamepad: gpio-keys-gamepad {
> > + compatible = "gpio-keys";
> > +
> > + 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-down {
> > + label = "D-Pad Down";
> > + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_DPAD_DOWN>;
> > + };
> > +
> > + 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-left {
> > + label = "D-Pad left";
> > + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_DPAD_LEFT>;
> > + };
> > +
> > + button-menu {
> > + label = "Key Menu";
> > + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_MODE>;
> > + };
> > +
> > + 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-right {
> > + label = "D-Pad Right";
> > + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_DPAD_RIGHT>;
> > + };
> > +
> > + button-select {
> > + label = "Key Select";
> > + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_SELECT>;
> > + };
> > + button-start {
> > + label = "Key Start";
> > + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > + linux,input-type = <EV_KEY>;
> > + linux,code = <BTN_START>;
> > + };
> > +
> > + 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-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>;
> > + };
> > + };
> > +
> > + gpio-keys-volume {
> > + compatible = "gpio-keys";
> > + autorepeat;
> > +
> > + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc-5v";
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + };
> > +};
> > +
> > +&cpu0 {
> > + cpu-supply = <®_dcdc1>;
> > +};
> > +
> > +&mmc0 {
> > + vmmc-supply = <®_cldo3>;
> > + disable-wp;
> > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > + bus-width = <4>;
> > + status = "okay";
> > +};
> > +
> > +&mmc2 {
> > + vmmc-supply = <®_cldo4>;
> > + vqmmc-supply = <®_aldo1>;
>
> This is now fixed to 1.8V, which doesn't look right. Either it's not
> the right regulator, or you should extend its range to cover 3.3V as
> well.
The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
bluetooth). If you raise this regulator too high the system becomes
unstable.
>
> > + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > + bus-width = <4>;
> > + status = "okay";
> > +};
> > +
> > +&ohci0 {
> > + status = "okay";
> > +};
> > +
> > +&ehci0 {
> > + status = "okay";
> > +};
> > +
> > +&r_rsb {
> > + status = "okay";
>
> This is indented with spaces, not a tab.
>
> > +
> > + axp717: pmic@3a3 {
> > + compatible = "x-powers,axp717";
> > + reg = <0x3a3>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + interrupt-parent = <&nmi_intc>;
> > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +
> > + vin1-supply = <®_vcc5v>;
> > + vin2-supply = <®_vcc5v>;
> > + vin3-supply = <®_vcc5v>;
> > + vin4-supply = <®_vcc5v>;
> > +
> > + regulators {
> > + reg_dcdc1: dcdc1 {
> > + regulator-always-on;
> > + regulator-boot-on;
>
> boot-on doesn't make much sense here: that allows it to be turned off,
> which we don't want. Also the binding documentation in regulator.yaml
> says that it's only intended "where software cannot read the state of
> the regulator", which is not true here.
> regulator-always-on is all we need - technically speaking not even
> that, since cpu0 is a consumer, but we need to play safe here.
>
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <1100000>;
> > + regulator-name = "vdd-cpu";
> > + };
> > +
> > + reg_dcdc2: dcdc2 {
> > + regulator-always-on;
> > + regulator-min-microvolt = <940000>;
> > + regulator-max-microvolt = <940000>;
> > + regulator-name = "vdd-gpu-sys";
> > + };
> > +
> > + reg_dcdc3: dcdc3 {
> > + regulator-always-on;
> > + regulator-boot-on;
>
> Same here, please remove that last line. We must not turn that off, and
> the state is readable, so it's not needed. We need always-on here,
> since it has no consumer.
>
> > + regulator-min-microvolt = <1100000>;
> > + regulator-max-microvolt = <1100000>;
> > + regulator-name = "vdd-dram";
> > + };
> > +
> > + reg_aldo1: aldo1 { /* SDC2 */
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-name = "vcc-sd2";
> > + };
> > +
> > + reg_aldo2: aldo2 {
> > + /* unused */
> > + };
> > +
> > + reg_aldo3: aldo3 {
> > + regulator-always-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-name = "axp717-aldo3";
>
> So do we know for sure that's critical? And do we have any clue what
> this supplies?
> There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
> are not critical.
>
> > + };
> > +
> > + reg_aldo4: aldo4 { /* 5096000.codec */
> > + regulator-always-on;
>
> Is that necessary? What happens if that is turned off? Looks like only
> the WiFi and potentially audio is affected? I think it can go then,
> also pg-supply would reference it, so it would effectively be enabled
> anyways.
I think this does something critical, as in my testing tinkering with
this regulator or turning it off locks up the system.
>
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-name = "vcc-pg";
> > + };
> > +
> > + 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 { /* 5096000.codec */
> > + /* unused */
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
>
> Looks a bit odd to have an "unused" comment, but also a voltage range
> specified. Judging from the comment this might be supplying some audio
> circuitry, which we don't need at the moment?
>
> The rest looks fine to me.
>
> Cheers,
> Andre
>
> > + regulator-name = "axp717-cldo1";
> > + };
> > +
> > + reg_cldo2: cldo2 {
> > + /* unused */
> > +
> > + };
> > +
> > + reg_cldo3: cldo3 {
> > + regulator-always-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-name = "vcc-io";
> > + };
> > +
> > + reg_cldo4: cldo4 {
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-name = "vcc-wifi";
> > + };
> > +
> > + reg_boost: boost {
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5200000>;
> > + regulator-name = "boost";
> > + };
> > +
> > + reg_cpusldo: cpusldo {
> > + /* unused */
> > + };
> > + };
> > + };
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_ph_pins>;
> > + status = "okay";
> > +};
> > +
> > +&pio {
> > + vcc-pa-supply = <®_cldo3>;
> > + vcc-pc-supply = <®_cldo3>;
> > + vcc-pe-supply = <®_cldo3>;
> > + vcc-pf-supply = <®_cldo3>;
> > + vcc-pg-supply = <®_aldo4>;
> > + vcc-ph-supply = <®_cldo3>;
> > + vcc-pi-supply = <®_cldo3>;
> > +};
> > +
> > +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> > +&usbotg {
> > + dr_mode = "peripheral"; /* USB type-C receptable */
> > + status = "okay";
> > +};
> > +
> > +&usbphy {
> > + status = "okay";
> > +};
>
Thank you,
Chris
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 1:58 ` Chris Morgan
@ 2024-04-25 5:39 ` Ryan Walklin
2024-04-25 10:08 ` Andre Przywara
1 sibling, 0 replies; 14+ messages in thread
From: Ryan Walklin @ 2024-04-25 5:39 UTC (permalink / raw)
To: Andre Przywara
Cc: Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jernej Skrabec, Samuel Holland, devicetree, linux-sunxi
On Thu, 25 Apr 2024, at 1:58 PM, Chris Morgan wrote:
> On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
>> On Wed, 24 Apr 2024 23:09:45 +1200
>> Ryan Walklin <ryan@testtoast.com> wrote:
>>
>> Hi Ryan (and Chris),
>>
>> many thanks for the changes, that looks really close now. Only a few
>> smaller comments this time.
>>
Thanks both for the review.
>> > +&mmc2 {
>> > + vmmc-supply = <®_cldo4>;
>> > + vqmmc-supply = <®_aldo1>;
>>
>> This is now fixed to 1.8V, which doesn't look right. Either it's not
>> the right regulator, or you should extend its range to cover 3.3V as
>> well.
>
> The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> bluetooth). If you raise this regulator too high the system becomes
> unstable.
>
Ideally LV signalling would work for UHS but unsure if that is achievable. FWIW the vendor BSP does refer to the vqmmc supply being ALDO1 but allows a range up to 3.5v. Will test out a 3.3v max and confirm it is unstable.
>> > +&r_rsb {
>> > + status = "okay";
>>
>> This is indented with spaces, not a tab.
Fixed, ta.
>> > + regulators {
>> > + reg_dcdc1: dcdc1 {
>> > + regulator-always-on;
>> > + regulator-boot-on;
>>
>> boot-on doesn't make much sense here: that allows it to be turned off,
>> which we don't want. Also the binding documentation in regulator.yaml
>> says that it's only intended "where software cannot read the state of
>> the regulator", which is not true here.
>> regulator-always-on is all we need - technically speaking not even
>> that, since cpu0 is a consumer, but we need to play safe here.
Thanks for the explanation, this and others fixed.
>> > + reg_aldo3: aldo3 {
>> > + regulator-always-on;
>> > + regulator-min-microvolt = <1800000>;
>> > + regulator-max-microvolt = <1800000>;
>> > + regulator-name = "axp717-aldo3";
>>
>> So do we know for sure that's critical? And do we have any clue what
>> this supplies?
>> There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
>> are not critical.
>>
Unsure currently, but can try with the HDMI patchset and see if I can identify VCC_HDMI at least. At least one of the audio-codec-connected regulators is presumably AVCC for the amp.
>> > + };
>> > +
>> > + reg_aldo4: aldo4 { /* 5096000.codec */
>> > + regulator-always-on;
>>
>> Is that necessary? What happens if that is turned off? Looks like only
>> the WiFi and potentially audio is affected? I think it can go then,
>> also pg-supply would reference it, so it would effectively be enabled
>> anyways.
>
> I think this does something critical, as in my testing tinkering with
> this regulator or turning it off locks up the system.
Agreed, unclear what else it is powering but at least the G-bank of GPIOs and also possibly VCC18_DRAM for the DRAM controller?
>> > + reg_cldo1: cldo1 { /* 5096000.codec */
>> > + /* unused */
>> > + regulator-min-microvolt = <3300000>;
>> > + regulator-max-microvolt = <3300000>;
>>
>> Looks a bit odd to have an "unused" comment, but also a voltage range
>> specified. Judging from the comment this might be supplying some audio
>> circuitry, which we don't need at the moment?
Thanks, have removed the range for now.
Ryan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 1:58 ` Chris Morgan
2024-04-25 5:39 ` Ryan Walklin
@ 2024-04-25 10:08 ` Andre Przywara
2024-04-25 15:35 ` Chris Morgan
1 sibling, 1 reply; 14+ messages in thread
From: Andre Przywara @ 2024-04-25 10:08 UTC (permalink / raw)
To: Chris Morgan
Cc: Ryan Walklin, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, devicetree,
linux-sunxi
On Wed, 24 Apr 2024 20:58:33 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:
Hi,
> On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
> > On Wed, 24 Apr 2024 23:09:45 +1200
> > Ryan Walklin <ryan@testtoast.com> wrote:
> >
> > Hi Ryan (and Chris),
> >
> > many thanks for the changes, that looks really close now. Only a few
> > smaller comments this time.
> >
> > > 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
> > > - USB-C charging port
> > >
> > > 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
> > >
> > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > ---
> > > Changelog v1..v2:
> > > - Update copyright
> > > - Spaces -> Tabs
> > > - Add cpufreq support
> > > - Remove MMC aliases
> > > - Fix GPIO button and regulator node names
> > > - Note unused AXP717 regulators
> > > - Update regulators for SD slots
> > > - Remove unused I2C3 device
> > > - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> > > - Add chassis-type
> > > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > > - Add PIO vcc-pg-supply
> > > - Correct boost regulator voltage and name
> > >
> > > Changelog v2..v3:
> > > - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> > > - Add dtb to Makefile
> > > - Remove unnecessary duplicated PLL regulator
> > > - Remove unimplemented/not-present drive-vbus feature from AXP717
> > > - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> > > - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> > > - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> > > - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> > > - Fix indentation
> > > - Correct boot/always-on states and voltages for various regulators from vendor BSP
> > > - Change USB-OTG mode to "peripheral" and correct comment
> > > - Correct and add remaining PIO supplies
> > > - Move volume key GPIOs to separate block to allow key repeat
> > > - Alphabetically orrder gamepad GPIOs
> > > - Move changelog and links below fold-line
> > > - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> > > - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
> > >
> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
> > >
> > > ---
> > > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > > .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> > > 2 files changed, 348 insertions(+)
> > > create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > index 21149b346a60..6f997157968e 100644
> > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > @@ -47,3 +47,4 @@ 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
> > > 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..3b53485019f1
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > @@ -0,0 +1,347 @@
> > > +// 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 <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_gamepad: gpio-keys-gamepad {
> > > + compatible = "gpio-keys";
> > > +
> > > + 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-down {
> > > + label = "D-Pad Down";
> > > + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_DPAD_DOWN>;
> > > + };
> > > +
> > > + 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-left {
> > > + label = "D-Pad left";
> > > + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_DPAD_LEFT>;
> > > + };
> > > +
> > > + button-menu {
> > > + label = "Key Menu";
> > > + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_MODE>;
> > > + };
> > > +
> > > + 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-right {
> > > + label = "D-Pad Right";
> > > + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_DPAD_RIGHT>;
> > > + };
> > > +
> > > + button-select {
> > > + label = "Key Select";
> > > + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_SELECT>;
> > > + };
> > > + button-start {
> > > + label = "Key Start";
> > > + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > > + linux,input-type = <EV_KEY>;
> > > + linux,code = <BTN_START>;
> > > + };
> > > +
> > > + 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-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>;
> > > + };
> > > + };
> > > +
> > > + gpio-keys-volume {
> > > + compatible = "gpio-keys";
> > > + autorepeat;
> > > +
> > > + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "vcc-5v";
> > > + regulator-min-microvolt = <5000000>;
> > > + regulator-max-microvolt = <5000000>;
> > > + };
> > > +};
> > > +
> > > +&cpu0 {
> > > + cpu-supply = <®_dcdc1>;
> > > +};
> > > +
> > > +&mmc0 {
> > > + vmmc-supply = <®_cldo3>;
> > > + disable-wp;
> > > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > > + bus-width = <4>;
> > > + status = "okay";
> > > +};
> > > +
> > > +&mmc2 {
> > > + vmmc-supply = <®_cldo4>;
> > > + vqmmc-supply = <®_aldo1>;
> >
> > This is now fixed to 1.8V, which doesn't look right. Either it's not
> > the right regulator, or you should extend its range to cover 3.3V as
> > well.
>
> The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> bluetooth). If you raise this regulator too high the system becomes
> unstable.
Are you sure? This is mmc2, so PortC, not the WiFi/BT on PortG.
And I was under the impression that the SD specification *requires*
starting negotiation at 3.3V I/O, and then only later on switching to 1.8V,
if both sides agree (UHS-1 capable controller and card).
I wonder if some non-UHS cards might not even work at 1.8V? I can imagine
that some cards don't really care, but it sounds being against the spec.
Also what's weird is that vqmmc is this aldo1 1.8V regulator, but the PortC
supply is cldo3, so 3.3V. So is there by any chance some kind of
(GPIO controlled?) switch, that level shifts the SDC pins between the two
regulators? If that's the case, I think we can describe that later, for
now we should just stick to one regulator.
> > > + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > > + bus-width = <4>;
> > > + status = "okay";
> > > +};
> > > +
> > > +&ohci0 {
> > > + status = "okay";
> > > +};
> > > +
> > > +&ehci0 {
> > > + status = "okay";
> > > +};
> > > +
> > > +&r_rsb {
> > > + status = "okay";
> >
> > This is indented with spaces, not a tab.
> >
> > > +
> > > + axp717: pmic@3a3 {
> > > + compatible = "x-powers,axp717";
> > > + reg = <0x3a3>;
> > > + interrupt-controller;
> > > + #interrupt-cells = <1>;
> > > + interrupt-parent = <&nmi_intc>;
> > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > +
> > > + vin1-supply = <®_vcc5v>;
> > > + vin2-supply = <®_vcc5v>;
> > > + vin3-supply = <®_vcc5v>;
> > > + vin4-supply = <®_vcc5v>;
> > > +
> > > + regulators {
> > > + reg_dcdc1: dcdc1 {
> > > + regulator-always-on;
> > > + regulator-boot-on;
> >
> > boot-on doesn't make much sense here: that allows it to be turned off,
> > which we don't want. Also the binding documentation in regulator.yaml
> > says that it's only intended "where software cannot read the state of
> > the regulator", which is not true here.
> > regulator-always-on is all we need - technically speaking not even
> > that, since cpu0 is a consumer, but we need to play safe here.
> >
> > > + regulator-min-microvolt = <900000>;
> > > + regulator-max-microvolt = <1100000>;
> > > + regulator-name = "vdd-cpu";
> > > + };
> > > +
> > > + reg_dcdc2: dcdc2 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <940000>;
> > > + regulator-max-microvolt = <940000>;
> > > + regulator-name = "vdd-gpu-sys";
> > > + };
> > > +
> > > + reg_dcdc3: dcdc3 {
> > > + regulator-always-on;
> > > + regulator-boot-on;
> >
> > Same here, please remove that last line. We must not turn that off, and
> > the state is readable, so it's not needed. We need always-on here,
> > since it has no consumer.
> >
> > > + regulator-min-microvolt = <1100000>;
> > > + regulator-max-microvolt = <1100000>;
> > > + regulator-name = "vdd-dram";
> > > + };
> > > +
> > > + reg_aldo1: aldo1 { /* SDC2 */
> > > + regulator-min-microvolt = <1800000>;
> > > + regulator-max-microvolt = <1800000>;
> > > + regulator-name = "vcc-sd2";
> > > + };
> > > +
> > > + reg_aldo2: aldo2 {
> > > + /* unused */
> > > + };
> > > +
> > > + reg_aldo3: aldo3 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <1800000>;
> > > + regulator-max-microvolt = <1800000>;
> > > + regulator-name = "axp717-aldo3";
> >
> > So do we know for sure that's critical? And do we have any clue what
> > this supplies?
> > There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
> > are not critical.
> >
> > > + };
> > > +
> > > + reg_aldo4: aldo4 { /* 5096000.codec */
> > > + regulator-always-on;
> >
> > Is that necessary? What happens if that is turned off? Looks like only
> > the WiFi and potentially audio is affected? I think it can go then,
> > also pg-supply would reference it, so it would effectively be enabled
> > anyways.
>
> I think this does something critical, as in my testing tinkering with
> this regulator or turning it off locks up the system.
In this case we typically add a comment stating that, so that people know.
We can amend this should we figure out that it supplies.
Cheers,
Andre
>
> >
> > > + regulator-min-microvolt = <1800000>;
> > > + regulator-max-microvolt = <1800000>;
> > > + regulator-name = "vcc-pg";
> > > + };
> > > +
> > > + 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 { /* 5096000.codec */
> > > + /* unused */
> > > + regulator-min-microvolt = <3300000>;
> > > + regulator-max-microvolt = <3300000>;
> >
> > Looks a bit odd to have an "unused" comment, but also a voltage range
> > specified. Judging from the comment this might be supplying some audio
> > circuitry, which we don't need at the moment?
> >
> > The rest looks fine to me.
> >
> > Cheers,
> > Andre
> >
> > > + regulator-name = "axp717-cldo1";
> > > + };
> > > +
> > > + reg_cldo2: cldo2 {
> > > + /* unused */
> > > +
> > > + };
> > > +
> > > + reg_cldo3: cldo3 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <3300000>;
> > > + regulator-max-microvolt = <3300000>;
> > > + regulator-name = "vcc-io";
> > > + };
> > > +
> > > + reg_cldo4: cldo4 {
> > > + regulator-min-microvolt = <3300000>;
> > > + regulator-max-microvolt = <3300000>;
> > > + regulator-name = "vcc-wifi";
> > > + };
> > > +
> > > + reg_boost: boost {
> > > + regulator-min-microvolt = <5000000>;
> > > + regulator-max-microvolt = <5200000>;
> > > + regulator-name = "boost";
> > > + };
> > > +
> > > + reg_cpusldo: cpusldo {
> > > + /* unused */
> > > + };
> > > + };
> > > + };
> > > +};
> > > +
> > > +&uart0 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&uart0_ph_pins>;
> > > + status = "okay";
> > > +};
> > > +
> > > +&pio {
> > > + vcc-pa-supply = <®_cldo3>;
> > > + vcc-pc-supply = <®_cldo3>;
> > > + vcc-pe-supply = <®_cldo3>;
> > > + vcc-pf-supply = <®_cldo3>;
> > > + vcc-pg-supply = <®_aldo4>;
> > > + vcc-ph-supply = <®_cldo3>;
> > > + vcc-pi-supply = <®_cldo3>;
> > > +};
> > > +
> > > +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> > > +&usbotg {
> > > + dr_mode = "peripheral"; /* USB type-C receptable */
> > > + status = "okay";
> > > +};
> > > +
> > > +&usbphy {
> > > + status = "okay";
> > > +};
> >
>
> Thank you,
> Chris
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 10:08 ` Andre Przywara
@ 2024-04-25 15:35 ` Chris Morgan
2024-04-25 16:54 ` Andre Przywara
0 siblings, 1 reply; 14+ messages in thread
From: Chris Morgan @ 2024-04-25 15:35 UTC (permalink / raw)
To: Andre Przywara
Cc: Ryan Walklin, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, devicetree,
linux-sunxi
On Thu, Apr 25, 2024 at 11:08:46AM +0100, Andre Przywara wrote:
> On Wed, 24 Apr 2024 20:58:33 -0500
> Chris Morgan <macromorgan@hotmail.com> wrote:
>
> Hi,
>
> > On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
> > > On Wed, 24 Apr 2024 23:09:45 +1200
> > > Ryan Walklin <ryan@testtoast.com> wrote:
> > >
> > > Hi Ryan (and Chris),
> > >
> > > many thanks for the changes, that looks really close now. Only a few
> > > smaller comments this time.
> > >
> > > > 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
> > > > - USB-C charging port
> > > >
> > > > 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
> > > >
> > > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > > ---
> > > > Changelog v1..v2:
> > > > - Update copyright
> > > > - Spaces -> Tabs
> > > > - Add cpufreq support
> > > > - Remove MMC aliases
> > > > - Fix GPIO button and regulator node names
> > > > - Note unused AXP717 regulators
> > > > - Update regulators for SD slots
> > > > - Remove unused I2C3 device
> > > > - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> > > > - Add chassis-type
> > > > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > > > - Add PIO vcc-pg-supply
> > > > - Correct boost regulator voltage and name
> > > >
> > > > Changelog v2..v3:
> > > > - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> > > > - Add dtb to Makefile
> > > > - Remove unnecessary duplicated PLL regulator
> > > > - Remove unimplemented/not-present drive-vbus feature from AXP717
> > > > - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> > > > - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> > > > - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> > > > - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> > > > - Fix indentation
> > > > - Correct boot/always-on states and voltages for various regulators from vendor BSP
> > > > - Change USB-OTG mode to "peripheral" and correct comment
> > > > - Correct and add remaining PIO supplies
> > > > - Move volume key GPIOs to separate block to allow key repeat
> > > > - Alphabetically orrder gamepad GPIOs
> > > > - Move changelog and links below fold-line
> > > > - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> > > > - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
> > > >
> > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
> > > >
> > > > ---
> > > > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > > > .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> > > > 2 files changed, 348 insertions(+)
> > > > create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > index 21149b346a60..6f997157968e 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > @@ -47,3 +47,4 @@ 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
> > > > 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..3b53485019f1
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > > @@ -0,0 +1,347 @@
> > > > +// 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 <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_gamepad: gpio-keys-gamepad {
> > > > + compatible = "gpio-keys";
> > > > +
> > > > + 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-down {
> > > > + label = "D-Pad Down";
> > > > + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_DPAD_DOWN>;
> > > > + };
> > > > +
> > > > + 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-left {
> > > > + label = "D-Pad left";
> > > > + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_DPAD_LEFT>;
> > > > + };
> > > > +
> > > > + button-menu {
> > > > + label = "Key Menu";
> > > > + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_MODE>;
> > > > + };
> > > > +
> > > > + 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-right {
> > > > + label = "D-Pad Right";
> > > > + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_DPAD_RIGHT>;
> > > > + };
> > > > +
> > > > + button-select {
> > > > + label = "Key Select";
> > > > + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_SELECT>;
> > > > + };
> > > > + button-start {
> > > > + label = "Key Start";
> > > > + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > > > + linux,input-type = <EV_KEY>;
> > > > + linux,code = <BTN_START>;
> > > > + };
> > > > +
> > > > + 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-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>;
> > > > + };
> > > > + };
> > > > +
> > > > + gpio-keys-volume {
> > > > + compatible = "gpio-keys";
> > > > + autorepeat;
> > > > +
> > > > + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> > > > + compatible = "regulator-fixed";
> > > > + regulator-name = "vcc-5v";
> > > > + regulator-min-microvolt = <5000000>;
> > > > + regulator-max-microvolt = <5000000>;
> > > > + };
> > > > +};
> > > > +
> > > > +&cpu0 {
> > > > + cpu-supply = <®_dcdc1>;
> > > > +};
> > > > +
> > > > +&mmc0 {
> > > > + vmmc-supply = <®_cldo3>;
> > > > + disable-wp;
> > > > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > > > + bus-width = <4>;
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&mmc2 {
> > > > + vmmc-supply = <®_cldo4>;
> > > > + vqmmc-supply = <®_aldo1>;
> > >
> > > This is now fixed to 1.8V, which doesn't look right. Either it's not
> > > the right regulator, or you should extend its range to cover 3.3V as
> > > well.
> >
> > The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> > bluetooth). If you raise this regulator too high the system becomes
> > unstable.
>
> Are you sure? This is mmc2, so PortC, not the WiFi/BT on PortG.
> And I was under the impression that the SD specification *requires*
> starting negotiation at 3.3V I/O, and then only later on switching to 1.8V,
> if both sides agree (UHS-1 capable controller and card).
> I wonder if some non-UHS cards might not even work at 1.8V? I can imagine
> that some cards don't really care, but it sounds being against the spec.
>
> Also what's weird is that vqmmc is this aldo1 1.8V regulator, but the PortC
> supply is cldo3, so 3.3V. So is there by any chance some kind of
> (GPIO controlled?) switch, that level shifts the SDC pins between the two
> regulators? If that's the case, I think we can describe that later, for
> now we should just stick to one regulator.
I got this mixed up, sorry. On my H700 the IO is 3.3v when GPIO PE4 is
pulled high, but 1.1v when GPIO PE4 is left floating or pulled low.
Note this is for mmc2 like you say (which is the 2nd SD card slot).
It's the mmc1 (the wifi) where the IO runs at a constant 1.8v.
>
> > > > + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > > > + bus-width = <4>;
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&ohci0 {
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&ehci0 {
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&r_rsb {
> > > > + status = "okay";
> > >
> > > This is indented with spaces, not a tab.
> > >
> > > > +
> > > > + axp717: pmic@3a3 {
> > > > + compatible = "x-powers,axp717";
> > > > + reg = <0x3a3>;
> > > > + interrupt-controller;
> > > > + #interrupt-cells = <1>;
> > > > + interrupt-parent = <&nmi_intc>;
> > > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > +
> > > > + vin1-supply = <®_vcc5v>;
> > > > + vin2-supply = <®_vcc5v>;
> > > > + vin3-supply = <®_vcc5v>;
> > > > + vin4-supply = <®_vcc5v>;
> > > > +
> > > > + regulators {
> > > > + reg_dcdc1: dcdc1 {
> > > > + regulator-always-on;
> > > > + regulator-boot-on;
> > >
> > > boot-on doesn't make much sense here: that allows it to be turned off,
> > > which we don't want. Also the binding documentation in regulator.yaml
> > > says that it's only intended "where software cannot read the state of
> > > the regulator", which is not true here.
> > > regulator-always-on is all we need - technically speaking not even
> > > that, since cpu0 is a consumer, but we need to play safe here.
> > >
> > > > + regulator-min-microvolt = <900000>;
> > > > + regulator-max-microvolt = <1100000>;
> > > > + regulator-name = "vdd-cpu";
> > > > + };
> > > > +
> > > > + reg_dcdc2: dcdc2 {
> > > > + regulator-always-on;
> > > > + regulator-min-microvolt = <940000>;
> > > > + regulator-max-microvolt = <940000>;
> > > > + regulator-name = "vdd-gpu-sys";
> > > > + };
> > > > +
> > > > + reg_dcdc3: dcdc3 {
> > > > + regulator-always-on;
> > > > + regulator-boot-on;
> > >
> > > Same here, please remove that last line. We must not turn that off, and
> > > the state is readable, so it's not needed. We need always-on here,
> > > since it has no consumer.
> > >
> > > > + regulator-min-microvolt = <1100000>;
> > > > + regulator-max-microvolt = <1100000>;
> > > > + regulator-name = "vdd-dram";
> > > > + };
> > > > +
> > > > + reg_aldo1: aldo1 { /* SDC2 */
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <1800000>;
> > > > + regulator-name = "vcc-sd2";
> > > > + };
> > > > +
> > > > + reg_aldo2: aldo2 {
> > > > + /* unused */
> > > > + };
> > > > +
> > > > + reg_aldo3: aldo3 {
> > > > + regulator-always-on;
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <1800000>;
> > > > + regulator-name = "axp717-aldo3";
> > >
> > > So do we know for sure that's critical? And do we have any clue what
> > > this supplies?
> > > There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
> > > are not critical.
> > >
> > > > + };
> > > > +
> > > > + reg_aldo4: aldo4 { /* 5096000.codec */
> > > > + regulator-always-on;
> > >
> > > Is that necessary? What happens if that is turned off? Looks like only
> > > the WiFi and potentially audio is affected? I think it can go then,
> > > also pg-supply would reference it, so it would effectively be enabled
> > > anyways.
> >
> > I think this does something critical, as in my testing tinkering with
> > this regulator or turning it off locks up the system.
>
> In this case we typically add a comment stating that, so that people know.
> We can amend this should we figure out that it supplies.
>
> Cheers,
> Andre
>
> >
> > >
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <1800000>;
> > > > + regulator-name = "vcc-pg";
> > > > + };
> > > > +
> > > > + 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 { /* 5096000.codec */
> > > > + /* unused */
> > > > + regulator-min-microvolt = <3300000>;
> > > > + regulator-max-microvolt = <3300000>;
> > >
> > > Looks a bit odd to have an "unused" comment, but also a voltage range
> > > specified. Judging from the comment this might be supplying some audio
> > > circuitry, which we don't need at the moment?
> > >
> > > The rest looks fine to me.
> > >
> > > Cheers,
> > > Andre
> > >
> > > > + regulator-name = "axp717-cldo1";
> > > > + };
> > > > +
> > > > + reg_cldo2: cldo2 {
> > > > + /* unused */
> > > > +
> > > > + };
> > > > +
> > > > + reg_cldo3: cldo3 {
> > > > + regulator-always-on;
> > > > + regulator-min-microvolt = <3300000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + regulator-name = "vcc-io";
> > > > + };
> > > > +
> > > > + reg_cldo4: cldo4 {
> > > > + regulator-min-microvolt = <3300000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + regulator-name = "vcc-wifi";
> > > > + };
> > > > +
> > > > + reg_boost: boost {
> > > > + regulator-min-microvolt = <5000000>;
> > > > + regulator-max-microvolt = <5200000>;
> > > > + regulator-name = "boost";
> > > > + };
> > > > +
> > > > + reg_cpusldo: cpusldo {
> > > > + /* unused */
> > > > + };
> > > > + };
> > > > + };
> > > > +};
> > > > +
> > > > +&uart0 {
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&uart0_ph_pins>;
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&pio {
> > > > + vcc-pa-supply = <®_cldo3>;
> > > > + vcc-pc-supply = <®_cldo3>;
> > > > + vcc-pe-supply = <®_cldo3>;
> > > > + vcc-pf-supply = <®_cldo3>;
> > > > + vcc-pg-supply = <®_aldo4>;
> > > > + vcc-ph-supply = <®_cldo3>;
> > > > + vcc-pi-supply = <®_cldo3>;
> > > > +};
> > > > +
> > > > +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> > > > +&usbotg {
> > > > + dr_mode = "peripheral"; /* USB type-C receptable */
> > > > + status = "okay";
> > > > +};
> > > > +
> > > > +&usbphy {
> > > > + status = "okay";
> > > > +};
> > >
> >
> > Thank you,
> > Chris
> >
>
Thank you,
Chris
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 15:35 ` Chris Morgan
@ 2024-04-25 16:54 ` Andre Przywara
2024-04-25 17:42 ` Chris Morgan
0 siblings, 1 reply; 14+ messages in thread
From: Andre Przywara @ 2024-04-25 16:54 UTC (permalink / raw)
To: Chris Morgan
Cc: Ryan Walklin, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, devicetree,
linux-sunxi
On Thu, 25 Apr 2024 10:35:49 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:
> On Thu, Apr 25, 2024 at 11:08:46AM +0100, Andre Przywara wrote:
> > On Wed, 24 Apr 2024 20:58:33 -0500
> > Chris Morgan <macromorgan@hotmail.com> wrote:
> >
> > Hi,
> >
> > > On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
> > > > On Wed, 24 Apr 2024 23:09:45 +1200
> > > > Ryan Walklin <ryan@testtoast.com> wrote:
> > > >
> > > > Hi Ryan (and Chris),
> > > >
> > > > many thanks for the changes, that looks really close now. Only a few
> > > > smaller comments this time.
> > > >
> > > > > 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
> > > > > - USB-C charging port
> > > > >
> > > > > 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
> > > > >
> > > > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > > > ---
> > > > > Changelog v1..v2:
> > > > > - Update copyright
> > > > > - Spaces -> Tabs
> > > > > - Add cpufreq support
> > > > > - Remove MMC aliases
> > > > > - Fix GPIO button and regulator node names
> > > > > - Note unused AXP717 regulators
> > > > > - Update regulators for SD slots
> > > > > - Remove unused I2C3 device
> > > > > - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> > > > > - Add chassis-type
> > > > > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > > > > - Add PIO vcc-pg-supply
> > > > > - Correct boost regulator voltage and name
> > > > >
> > > > > Changelog v2..v3:
> > > > > - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> > > > > - Add dtb to Makefile
> > > > > - Remove unnecessary duplicated PLL regulator
> > > > > - Remove unimplemented/not-present drive-vbus feature from AXP717
> > > > > - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> > > > > - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> > > > > - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> > > > > - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> > > > > - Fix indentation
> > > > > - Correct boot/always-on states and voltages for various regulators from vendor BSP
> > > > > - Change USB-OTG mode to "peripheral" and correct comment
> > > > > - Correct and add remaining PIO supplies
> > > > > - Move volume key GPIOs to separate block to allow key repeat
> > > > > - Alphabetically orrder gamepad GPIOs
> > > > > - Move changelog and links below fold-line
> > > > > - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> > > > > - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
> > > > >
> > > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
> > > > >
> > > > > ---
> > > > > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > > > > .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> > > > > 2 files changed, 348 insertions(+)
> > > > > create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > index 21149b346a60..6f997157968e 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > @@ -47,3 +47,4 @@ 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
> > > > > 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..3b53485019f1
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > > > @@ -0,0 +1,347 @@
> > > > > +// 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 <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_gamepad: gpio-keys-gamepad {
> > > > > + compatible = "gpio-keys";
> > > > > +
> > > > > + 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-down {
> > > > > + label = "D-Pad Down";
> > > > > + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_DPAD_DOWN>;
> > > > > + };
> > > > > +
> > > > > + 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-left {
> > > > > + label = "D-Pad left";
> > > > > + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_DPAD_LEFT>;
> > > > > + };
> > > > > +
> > > > > + button-menu {
> > > > > + label = "Key Menu";
> > > > > + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_MODE>;
> > > > > + };
> > > > > +
> > > > > + 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-right {
> > > > > + label = "D-Pad Right";
> > > > > + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_DPAD_RIGHT>;
> > > > > + };
> > > > > +
> > > > > + button-select {
> > > > > + label = "Key Select";
> > > > > + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_SELECT>;
> > > > > + };
> > > > > + button-start {
> > > > > + label = "Key Start";
> > > > > + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > > > > + linux,input-type = <EV_KEY>;
> > > > > + linux,code = <BTN_START>;
> > > > > + };
> > > > > +
> > > > > + 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-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>;
> > > > > + };
> > > > > + };
> > > > > +
> > > > > + gpio-keys-volume {
> > > > > + compatible = "gpio-keys";
> > > > > + autorepeat;
> > > > > +
> > > > > + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> > > > > + compatible = "regulator-fixed";
> > > > > + regulator-name = "vcc-5v";
> > > > > + regulator-min-microvolt = <5000000>;
> > > > > + regulator-max-microvolt = <5000000>;
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&cpu0 {
> > > > > + cpu-supply = <®_dcdc1>;
> > > > > +};
> > > > > +
> > > > > +&mmc0 {
> > > > > + vmmc-supply = <®_cldo3>;
> > > > > + disable-wp;
> > > > > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > > > > + bus-width = <4>;
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&mmc2 {
> > > > > + vmmc-supply = <®_cldo4>;
> > > > > + vqmmc-supply = <®_aldo1>;
> > > >
> > > > This is now fixed to 1.8V, which doesn't look right. Either it's not
> > > > the right regulator, or you should extend its range to cover 3.3V as
> > > > well.
> > >
> > > The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> > > bluetooth). If you raise this regulator too high the system becomes
> > > unstable.
> >
> > Are you sure? This is mmc2, so PortC, not the WiFi/BT on PortG.
> > And I was under the impression that the SD specification *requires*
> > starting negotiation at 3.3V I/O, and then only later on switching to 1.8V,
> > if both sides agree (UHS-1 capable controller and card).
> > I wonder if some non-UHS cards might not even work at 1.8V? I can imagine
> > that some cards don't really care, but it sounds being against the spec.
> >
> > Also what's weird is that vqmmc is this aldo1 1.8V regulator, but the PortC
> > supply is cldo3, so 3.3V. So is there by any chance some kind of
> > (GPIO controlled?) switch, that level shifts the SDC pins between the two
> > regulators? If that's the case, I think we can describe that later, for
> > now we should just stick to one regulator.
>
> I got this mixed up, sorry. On my H700 the IO is 3.3v when GPIO PE4 is
> pulled high, but 1.1v when GPIO PE4 is left floating or pulled low.
Ah, that makes more sense. Did you measure that on some PortC pin, or on
the contacts in the SD card slot?
So it looks like PE4 could control this power switcher. Maybe it's 1.1V
because the other regulator is not set up properly? Is there some AXP rail
that is at 1.1V?
> Note this is for mmc2 like you say (which is the 2nd SD card slot).
> It's the mmc1 (the wifi) where the IO runs at a constant 1.8v.
Ok, thanks for confirming this!
Cheers,
Andre.
> > > > > + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > > > > + bus-width = <4>;
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&ohci0 {
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&ehci0 {
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&r_rsb {
> > > > > + status = "okay";
> > > >
> > > > This is indented with spaces, not a tab.
> > > >
> > > > > +
> > > > > + axp717: pmic@3a3 {
> > > > > + compatible = "x-powers,axp717";
> > > > > + reg = <0x3a3>;
> > > > > + interrupt-controller;
> > > > > + #interrupt-cells = <1>;
> > > > > + interrupt-parent = <&nmi_intc>;
> > > > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > +
> > > > > + vin1-supply = <®_vcc5v>;
> > > > > + vin2-supply = <®_vcc5v>;
> > > > > + vin3-supply = <®_vcc5v>;
> > > > > + vin4-supply = <®_vcc5v>;
> > > > > +
> > > > > + regulators {
> > > > > + reg_dcdc1: dcdc1 {
> > > > > + regulator-always-on;
> > > > > + regulator-boot-on;
> > > >
> > > > boot-on doesn't make much sense here: that allows it to be turned off,
> > > > which we don't want. Also the binding documentation in regulator.yaml
> > > > says that it's only intended "where software cannot read the state of
> > > > the regulator", which is not true here.
> > > > regulator-always-on is all we need - technically speaking not even
> > > > that, since cpu0 is a consumer, but we need to play safe here.
> > > >
> > > > > + regulator-min-microvolt = <900000>;
> > > > > + regulator-max-microvolt = <1100000>;
> > > > > + regulator-name = "vdd-cpu";
> > > > > + };
> > > > > +
> > > > > + reg_dcdc2: dcdc2 {
> > > > > + regulator-always-on;
> > > > > + regulator-min-microvolt = <940000>;
> > > > > + regulator-max-microvolt = <940000>;
> > > > > + regulator-name = "vdd-gpu-sys";
> > > > > + };
> > > > > +
> > > > > + reg_dcdc3: dcdc3 {
> > > > > + regulator-always-on;
> > > > > + regulator-boot-on;
> > > >
> > > > Same here, please remove that last line. We must not turn that off, and
> > > > the state is readable, so it's not needed. We need always-on here,
> > > > since it has no consumer.
> > > >
> > > > > + regulator-min-microvolt = <1100000>;
> > > > > + regulator-max-microvolt = <1100000>;
> > > > > + regulator-name = "vdd-dram";
> > > > > + };
> > > > > +
> > > > > + reg_aldo1: aldo1 { /* SDC2 */
> > > > > + regulator-min-microvolt = <1800000>;
> > > > > + regulator-max-microvolt = <1800000>;
> > > > > + regulator-name = "vcc-sd2";
> > > > > + };
> > > > > +
> > > > > + reg_aldo2: aldo2 {
> > > > > + /* unused */
> > > > > + };
> > > > > +
> > > > > + reg_aldo3: aldo3 {
> > > > > + regulator-always-on;
> > > > > + regulator-min-microvolt = <1800000>;
> > > > > + regulator-max-microvolt = <1800000>;
> > > > > + regulator-name = "axp717-aldo3";
> > > >
> > > > So do we know for sure that's critical? And do we have any clue what
> > > > this supplies?
> > > > There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
> > > > are not critical.
> > > >
> > > > > + };
> > > > > +
> > > > > + reg_aldo4: aldo4 { /* 5096000.codec */
> > > > > + regulator-always-on;
> > > >
> > > > Is that necessary? What happens if that is turned off? Looks like only
> > > > the WiFi and potentially audio is affected? I think it can go then,
> > > > also pg-supply would reference it, so it would effectively be enabled
> > > > anyways.
> > >
> > > I think this does something critical, as in my testing tinkering with
> > > this regulator or turning it off locks up the system.
> >
> > In this case we typically add a comment stating that, so that people know.
> > We can amend this should we figure out that it supplies.
> >
> > Cheers,
> > Andre
> >
> > >
> > > >
> > > > > + regulator-min-microvolt = <1800000>;
> > > > > + regulator-max-microvolt = <1800000>;
> > > > > + regulator-name = "vcc-pg";
> > > > > + };
> > > > > +
> > > > > + 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 { /* 5096000.codec */
> > > > > + /* unused */
> > > > > + regulator-min-microvolt = <3300000>;
> > > > > + regulator-max-microvolt = <3300000>;
> > > >
> > > > Looks a bit odd to have an "unused" comment, but also a voltage range
> > > > specified. Judging from the comment this might be supplying some audio
> > > > circuitry, which we don't need at the moment?
> > > >
> > > > The rest looks fine to me.
> > > >
> > > > Cheers,
> > > > Andre
> > > >
> > > > > + regulator-name = "axp717-cldo1";
> > > > > + };
> > > > > +
> > > > > + reg_cldo2: cldo2 {
> > > > > + /* unused */
> > > > > +
> > > > > + };
> > > > > +
> > > > > + reg_cldo3: cldo3 {
> > > > > + regulator-always-on;
> > > > > + regulator-min-microvolt = <3300000>;
> > > > > + regulator-max-microvolt = <3300000>;
> > > > > + regulator-name = "vcc-io";
> > > > > + };
> > > > > +
> > > > > + reg_cldo4: cldo4 {
> > > > > + regulator-min-microvolt = <3300000>;
> > > > > + regulator-max-microvolt = <3300000>;
> > > > > + regulator-name = "vcc-wifi";
> > > > > + };
> > > > > +
> > > > > + reg_boost: boost {
> > > > > + regulator-min-microvolt = <5000000>;
> > > > > + regulator-max-microvolt = <5200000>;
> > > > > + regulator-name = "boost";
> > > > > + };
> > > > > +
> > > > > + reg_cpusldo: cpusldo {
> > > > > + /* unused */
> > > > > + };
> > > > > + };
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&uart0 {
> > > > > + pinctrl-names = "default";
> > > > > + pinctrl-0 = <&uart0_ph_pins>;
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&pio {
> > > > > + vcc-pa-supply = <®_cldo3>;
> > > > > + vcc-pc-supply = <®_cldo3>;
> > > > > + vcc-pe-supply = <®_cldo3>;
> > > > > + vcc-pf-supply = <®_cldo3>;
> > > > > + vcc-pg-supply = <®_aldo4>;
> > > > > + vcc-ph-supply = <®_cldo3>;
> > > > > + vcc-pi-supply = <®_cldo3>;
> > > > > +};
> > > > > +
> > > > > +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> > > > > +&usbotg {
> > > > > + dr_mode = "peripheral"; /* USB type-C receptable */
> > > > > + status = "okay";
> > > > > +};
> > > > > +
> > > > > +&usbphy {
> > > > > + status = "okay";
> > > > > +};
> > > >
> > >
> > > Thank you,
> > > Chris
> > >
> >
>
> Thank you,
> Chris
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS
2024-04-25 16:54 ` Andre Przywara
@ 2024-04-25 17:42 ` Chris Morgan
0 siblings, 0 replies; 14+ messages in thread
From: Chris Morgan @ 2024-04-25 17:42 UTC (permalink / raw)
To: Andre Przywara
Cc: Ryan Walklin, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, devicetree,
linux-sunxi
On Thu, Apr 25, 2024 at 05:54:07PM +0100, Andre Przywara wrote:
> On Thu, 25 Apr 2024 10:35:49 -0500
> Chris Morgan <macromorgan@hotmail.com> wrote:
>
> > On Thu, Apr 25, 2024 at 11:08:46AM +0100, Andre Przywara wrote:
> > > On Wed, 24 Apr 2024 20:58:33 -0500
> > > Chris Morgan <macromorgan@hotmail.com> wrote:
> > >
> > > Hi,
> > >
> > > > On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
> > > > > On Wed, 24 Apr 2024 23:09:45 +1200
> > > > > Ryan Walklin <ryan@testtoast.com> wrote:
> > > > >
> > > > > Hi Ryan (and Chris),
> > > > >
> > > > > many thanks for the changes, that looks really close now. Only a few
> > > > > smaller comments this time.
> > > > >
> > > > > > 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
> > > > > > - USB-C charging port
> > > > > >
> > > > > > 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
> > > > > >
> > > > > > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > > > > > ---
> > > > > > Changelog v1..v2:
> > > > > > - Update copyright
> > > > > > - Spaces -> Tabs
> > > > > > - Add cpufreq support
> > > > > > - Remove MMC aliases
> > > > > > - Fix GPIO button and regulator node names
> > > > > > - Note unused AXP717 regulators
> > > > > > - Update regulators for SD slots
> > > > > > - Remove unused I2C3 device
> > > > > > - Update NMI interrupt controller for AXP717, requires H616 support patch in dt-next [1]
> > > > > > - Add chassis-type
> > > > > > - Address USB EHCI/OHCI0 correctly and add usb vbus supply
> > > > > > - Add PIO vcc-pg-supply
> > > > > > - Correct boost regulator voltage and name
> > > > > >
> > > > > > Changelog v2..v3:
> > > > > > - Remove cpufreq support (patch still pending for 6.10, will followup once opp table merged)
> > > > > > - Add dtb to Makefile
> > > > > > - Remove unnecessary duplicated PLL regulator
> > > > > > - Remove unimplemented/not-present drive-vbus feature from AXP717
> > > > > > - Rename CLDO3 to "vcc-io", inferring function from board testing by Chris Morgan
> > > > > > - Correct MMC1 vmmc-supply to CLDO3 and MMC2 to CLDO4
> > > > > > - Reduce DCDC1 "vdd-cpu" supply voltage range to 0.9v-1.1v to match lowest OPP voltage
> > > > > > - Identify DCDC2 as GPU supply - rename to "vdd-gpu-sys", remove always-on and used fixed 0.94v voltage
> > > > > > - Fix indentation
> > > > > > - Correct boot/always-on states and voltages for various regulators from vendor BSP
> > > > > > - Change USB-OTG mode to "peripheral" and correct comment
> > > > > > - Correct and add remaining PIO supplies
> > > > > > - Move volume key GPIOs to separate block to allow key repeat
> > > > > > - Alphabetically orrder gamepad GPIOs
> > > > > > - Move changelog and links below fold-line
> > > > > > - Remove USB 3.3v VCC-USB and VCC-SD2 regulators pending further hardware investigation (to be submitted as subsequent patch)
> > > > > > - Constrain boost regulator voltage to 5.0v to 5.2v to capture default voltage of 5.126v
> > > > > >
> > > > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=d47bca77bf3ab475c33b3929c33c80aeb49df35c
> > > > > >
> > > > > > ---
> > > > > > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > > > > > .../sun50i-h700-anbernic-rg35xx-2024.dts | 347 ++++++++++++++++++
> > > > > > 2 files changed, 348 insertions(+)
> > > > > > create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > index 21149b346a60..6f997157968e 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > @@ -47,3 +47,4 @@ 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
> > > > > > 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..3b53485019f1
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> > > > > > @@ -0,0 +1,347 @@
> > > > > > +// 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 <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_gamepad: gpio-keys-gamepad {
> > > > > > + compatible = "gpio-keys";
> > > > > > +
> > > > > > + 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-down {
> > > > > > + label = "D-Pad Down";
> > > > > > + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; /* PE0 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_DPAD_DOWN>;
> > > > > > + };
> > > > > > +
> > > > > > + 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-left {
> > > > > > + label = "D-Pad left";
> > > > > > + gpios = <&pio 0 8 GPIO_ACTIVE_LOW>; /* PA8 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_DPAD_LEFT>;
> > > > > > + };
> > > > > > +
> > > > > > + button-menu {
> > > > > > + label = "Key Menu";
> > > > > > + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; /* PE3 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_MODE>;
> > > > > > + };
> > > > > > +
> > > > > > + 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-right {
> > > > > > + label = "D-Pad Right";
> > > > > > + gpios = <&pio 0 9 GPIO_ACTIVE_LOW>; /* PA9 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_DPAD_RIGHT>;
> > > > > > + };
> > > > > > +
> > > > > > + button-select {
> > > > > > + label = "Key Select";
> > > > > > + gpios = <&pio 0 5 GPIO_ACTIVE_LOW>; /* PA5 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_SELECT>;
> > > > > > + };
> > > > > > + button-start {
> > > > > > + label = "Key Start";
> > > > > > + gpios = <&pio 0 4 GPIO_ACTIVE_LOW>; /* PA4 */
> > > > > > + linux,input-type = <EV_KEY>;
> > > > > > + linux,code = <BTN_START>;
> > > > > > + };
> > > > > > +
> > > > > > + 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-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>;
> > > > > > + };
> > > > > > + };
> > > > > > +
> > > > > > + gpio-keys-volume {
> > > > > > + compatible = "gpio-keys";
> > > > > > + autorepeat;
> > > > > > +
> > > > > > + 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_vcc5v: regulator-vcc5v { /* USB-C power input */
> > > > > > + compatible = "regulator-fixed";
> > > > > > + regulator-name = "vcc-5v";
> > > > > > + regulator-min-microvolt = <5000000>;
> > > > > > + regulator-max-microvolt = <5000000>;
> > > > > > + };
> > > > > > +};
> > > > > > +
> > > > > > +&cpu0 {
> > > > > > + cpu-supply = <®_dcdc1>;
> > > > > > +};
> > > > > > +
> > > > > > +&mmc0 {
> > > > > > + vmmc-supply = <®_cldo3>;
> > > > > > + disable-wp;
> > > > > > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > > > > > + bus-width = <4>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&mmc2 {
> > > > > > + vmmc-supply = <®_cldo4>;
> > > > > > + vqmmc-supply = <®_aldo1>;
> > > > >
> > > > > This is now fixed to 1.8V, which doesn't look right. Either it's not
> > > > > the right regulator, or you should extend its range to cover 3.3V as
> > > > > well.
> > > >
> > > > The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> > > > bluetooth). If you raise this regulator too high the system becomes
> > > > unstable.
> > >
> > > Are you sure? This is mmc2, so PortC, not the WiFi/BT on PortG.
> > > And I was under the impression that the SD specification *requires*
> > > starting negotiation at 3.3V I/O, and then only later on switching to 1.8V,
> > > if both sides agree (UHS-1 capable controller and card).
> > > I wonder if some non-UHS cards might not even work at 1.8V? I can imagine
> > > that some cards don't really care, but it sounds being against the spec.
> > >
> > > Also what's weird is that vqmmc is this aldo1 1.8V regulator, but the PortC
> > > supply is cldo3, so 3.3V. So is there by any chance some kind of
> > > (GPIO controlled?) switch, that level shifts the SDC pins between the two
> > > regulators? If that's the case, I think we can describe that later, for
> > > now we should just stick to one regulator.
> >
> > I got this mixed up, sorry. On my H700 the IO is 3.3v when GPIO PE4 is
> > pulled high, but 1.1v when GPIO PE4 is left floating or pulled low.
>
> Ah, that makes more sense. Did you measure that on some PortC pin, or on
> the contacts in the SD card slot?
> So it looks like PE4 could control this power switcher. Maybe it's 1.1V
> because the other regulator is not set up properly? Is there some AXP rail
> that is at 1.1V?
No, none of the other regulators at the time were running at 1.1v,
except for the RAM. I tweaked the value slightly higher and didn't see
a coresponding change, so I'm assuming the 1.1v input voltage is just
a fixed/always on regulator of some kind and not an offshoot of the
RAM's regulator. The CPU and GPU regulator was running at 0.9v, and
all the others were either off or 1.8 or 3.3v.
I measured the IO pins of the SD card slot to get the values.
Thank you,
Chris
>
> > Note this is for mmc2 like you say (which is the 2nd SD card slot).
> > It's the mmc1 (the wifi) where the IO runs at a constant 1.8v.
>
> Ok, thanks for confirming this!
>
> Cheers,
> Andre.
>
> > > > > > + cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> > > > > > + bus-width = <4>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&ohci0 {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&ehci0 {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&r_rsb {
> > > > > > + status = "okay";
> > > > >
> > > > > This is indented with spaces, not a tab.
> > > > >
> > > > > > +
> > > > > > + axp717: pmic@3a3 {
> > > > > > + compatible = "x-powers,axp717";
> > > > > > + reg = <0x3a3>;
> > > > > > + interrupt-controller;
> > > > > > + #interrupt-cells = <1>;
> > > > > > + interrupt-parent = <&nmi_intc>;
> > > > > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > > +
> > > > > > + vin1-supply = <®_vcc5v>;
> > > > > > + vin2-supply = <®_vcc5v>;
> > > > > > + vin3-supply = <®_vcc5v>;
> > > > > > + vin4-supply = <®_vcc5v>;
> > > > > > +
> > > > > > + regulators {
> > > > > > + reg_dcdc1: dcdc1 {
> > > > > > + regulator-always-on;
> > > > > > + regulator-boot-on;
> > > > >
> > > > > boot-on doesn't make much sense here: that allows it to be turned off,
> > > > > which we don't want. Also the binding documentation in regulator.yaml
> > > > > says that it's only intended "where software cannot read the state of
> > > > > the regulator", which is not true here.
> > > > > regulator-always-on is all we need - technically speaking not even
> > > > > that, since cpu0 is a consumer, but we need to play safe here.
> > > > >
> > > > > > + regulator-min-microvolt = <900000>;
> > > > > > + regulator-max-microvolt = <1100000>;
> > > > > > + regulator-name = "vdd-cpu";
> > > > > > + };
> > > > > > +
> > > > > > + reg_dcdc2: dcdc2 {
> > > > > > + regulator-always-on;
> > > > > > + regulator-min-microvolt = <940000>;
> > > > > > + regulator-max-microvolt = <940000>;
> > > > > > + regulator-name = "vdd-gpu-sys";
> > > > > > + };
> > > > > > +
> > > > > > + reg_dcdc3: dcdc3 {
> > > > > > + regulator-always-on;
> > > > > > + regulator-boot-on;
> > > > >
> > > > > Same here, please remove that last line. We must not turn that off, and
> > > > > the state is readable, so it's not needed. We need always-on here,
> > > > > since it has no consumer.
> > > > >
> > > > > > + regulator-min-microvolt = <1100000>;
> > > > > > + regulator-max-microvolt = <1100000>;
> > > > > > + regulator-name = "vdd-dram";
> > > > > > + };
> > > > > > +
> > > > > > + reg_aldo1: aldo1 { /* SDC2 */
> > > > > > + regulator-min-microvolt = <1800000>;
> > > > > > + regulator-max-microvolt = <1800000>;
> > > > > > + regulator-name = "vcc-sd2";
> > > > > > + };
> > > > > > +
> > > > > > + reg_aldo2: aldo2 {
> > > > > > + /* unused */
> > > > > > + };
> > > > > > +
> > > > > > + reg_aldo3: aldo3 {
> > > > > > + regulator-always-on;
> > > > > > + regulator-min-microvolt = <1800000>;
> > > > > > + regulator-max-microvolt = <1800000>;
> > > > > > + regulator-name = "axp717-aldo3";
> > > > >
> > > > > So do we know for sure that's critical? And do we have any clue what
> > > > > this supplies?
> > > > > There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
> > > > > are not critical.
> > > > >
> > > > > > + };
> > > > > > +
> > > > > > + reg_aldo4: aldo4 { /* 5096000.codec */
> > > > > > + regulator-always-on;
> > > > >
> > > > > Is that necessary? What happens if that is turned off? Looks like only
> > > > > the WiFi and potentially audio is affected? I think it can go then,
> > > > > also pg-supply would reference it, so it would effectively be enabled
> > > > > anyways.
> > > >
> > > > I think this does something critical, as in my testing tinkering with
> > > > this regulator or turning it off locks up the system.
> > >
> > > In this case we typically add a comment stating that, so that people know.
> > > We can amend this should we figure out that it supplies.
> > >
> > > Cheers,
> > > Andre
> > >
> > > >
> > > > >
> > > > > > + regulator-min-microvolt = <1800000>;
> > > > > > + regulator-max-microvolt = <1800000>;
> > > > > > + regulator-name = "vcc-pg";
> > > > > > + };
> > > > > > +
> > > > > > + 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 { /* 5096000.codec */
> > > > > > + /* unused */
> > > > > > + regulator-min-microvolt = <3300000>;
> > > > > > + regulator-max-microvolt = <3300000>;
> > > > >
> > > > > Looks a bit odd to have an "unused" comment, but also a voltage range
> > > > > specified. Judging from the comment this might be supplying some audio
> > > > > circuitry, which we don't need at the moment?
> > > > >
> > > > > The rest looks fine to me.
> > > > >
> > > > > Cheers,
> > > > > Andre
> > > > >
> > > > > > + regulator-name = "axp717-cldo1";
> > > > > > + };
> > > > > > +
> > > > > > + reg_cldo2: cldo2 {
> > > > > > + /* unused */
> > > > > > +
> > > > > > + };
> > > > > > +
> > > > > > + reg_cldo3: cldo3 {
> > > > > > + regulator-always-on;
> > > > > > + regulator-min-microvolt = <3300000>;
> > > > > > + regulator-max-microvolt = <3300000>;
> > > > > > + regulator-name = "vcc-io";
> > > > > > + };
> > > > > > +
> > > > > > + reg_cldo4: cldo4 {
> > > > > > + regulator-min-microvolt = <3300000>;
> > > > > > + regulator-max-microvolt = <3300000>;
> > > > > > + regulator-name = "vcc-wifi";
> > > > > > + };
> > > > > > +
> > > > > > + reg_boost: boost {
> > > > > > + regulator-min-microvolt = <5000000>;
> > > > > > + regulator-max-microvolt = <5200000>;
> > > > > > + regulator-name = "boost";
> > > > > > + };
> > > > > > +
> > > > > > + reg_cpusldo: cpusldo {
> > > > > > + /* unused */
> > > > > > + };
> > > > > > + };
> > > > > > + };
> > > > > > +};
> > > > > > +
> > > > > > +&uart0 {
> > > > > > + pinctrl-names = "default";
> > > > > > + pinctrl-0 = <&uart0_ph_pins>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&pio {
> > > > > > + vcc-pa-supply = <®_cldo3>;
> > > > > > + vcc-pc-supply = <®_cldo3>;
> > > > > > + vcc-pe-supply = <®_cldo3>;
> > > > > > + vcc-pf-supply = <®_cldo3>;
> > > > > > + vcc-pg-supply = <®_aldo4>;
> > > > > > + vcc-ph-supply = <®_cldo3>;
> > > > > > + vcc-pi-supply = <®_cldo3>;
> > > > > > +};
> > > > > > +
> > > > > > +/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
> > > > > > +&usbotg {
> > > > > > + dr_mode = "peripheral"; /* USB type-C receptable */
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&usbphy {
> > > > > > + status = "okay";
> > > > > > +};
> > > > >
> > > >
> > > > Thank you,
> > > > Chris
> > > >
> > >
> >
> > Thank you,
> > Chris
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-04-25 17:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 11:09 [PATCH v3 0/4] *** SUBJECT HERE *** Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 1/4] dt-bindings: arm: sunxi: document Anbernic RG35XX handheld gaming device variants Ryan Walklin
2024-04-24 11:09 ` [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS Ryan Walklin
2024-04-25 0:25 ` Andre Przywara
2024-04-25 1:58 ` Chris Morgan
2024-04-25 5:39 ` Ryan Walklin
2024-04-25 10:08 ` Andre Przywara
2024-04-25 15:35 ` Chris Morgan
2024-04-25 16:54 ` Andre Przywara
2024-04-25 17:42 ` Chris Morgan
2024-04-24 11:09 ` [PATCH v3 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS Ryan Walklin
2024-04-25 0:32 ` Andre Przywara
2024-04-24 11:09 ` [PATCH v3 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS Ryan Walklin
2024-04-25 0:35 ` Andre Przywara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).