Re: [PATCH v2 2/2] arm64: dts: rockchip: Add rk3576 evb2 board
From: Quentin Schulz
Date: Wed Jan 07 2026 - 10:46:36 EST
Hi Chaoyi,
On 1/7/26 8:03 AM, Chaoyi Chen wrote:
[...]
diff --git a/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts b/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts
new file mode 100644
index 000000000000..52788c514ec0
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts
@@ -0,0 +1,997 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2025 Rockchip Electronics Co., Ltd.
+ *
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/pinctrl/rockchip.h>
+#include <dt-bindings/soc/rockchip,vop2.h>
+#include "rk3576.dtsi"
+
+/ {
+ model = "Rockchip RK3576 EVB2 V10 Board";
+ compatible = "rockchip,rk3576-evb2-v10", "rockchip,rk3576";
+
+ aliases {
+ ethernet0 = &gmac0;
+ ethernet1 = &gmac1;
+ };
+
+ chosen: chosen {
Why a label here?
There are also many other instances of nodes being labelled but whose label is never used. I would understand for some if you want to have DTSOs working with this DTB, but here chosen really doesn't make much sense to me?
+ stdout-path = "serial0:1500000n8";
+ };
+
+ adc_keys: adc-keys {
Are we expecting to extend this node from another DT? Why the label?
Won't comment on all other labeled-but-no-phandle-use instances, please check.
+ vcc3v3_rtc_s5: regulator-vcc3v3-rtc-s5 {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc3v3_rtc_s5";
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <&vcc_sys>;
If this is for the rtc, shouldn't we declare this dependency in the RTC device node and not have it always-on?
+ };
+
+ vcc3v3_sata_pwren: vcc3v3-sata-pwren {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc3v3_sata_pwren";
+ enable-active-high;
+ regulator-boot-on;
+ regulator-always-on;
Why do we have this always-on? Seems like we're missing a dependency on this regulator in the SATA controller?
+ gpio = <&gpio4 RK_PC7 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&satapm_pwren>;
+ };
+
+ vcc5v0_device: regulator-vcc5v0-device {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc5v0_device";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <&vcc12v_dcin>;
+ };
+
+ vcc5v0_host: regulator-vcc5v0-host {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc5v0_host";
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ enable-active-high;
+ gpio = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
+ vin-supply = <&vcc5v0_device>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&usb_host_pwren>;
+ };
+
I assume both of the above are related to USB operating in host or device mode? Maybe there's a way to have something more useful to the user in regulator-name (and possibly the regulator node name) so that they have an idea what this pertains to?
Additionally, why is this always-on? I would assume the USB controller is capable of controlling its regulator(s)?
[...]
+ vcc_ufs_s0: regulator-vcc-ufs-s0 {
We also have another regulator for UFS that is mentioned in the UFS controller node but not this one, why?
+ compatible = "regulator-fixed";
+ regulator-name = "vcc_ufs_s0";
+ regulator-boot-on;
+ regulator-always-on;
Why always on?
[...]
+&mdio0 {
+ rgmii_phy0: ethernet-phy@1 {
+ compatible = "ethernet-phy-id4f51.e91b";
Is MDIO auto-detection broken such that you need to specify the PHY vendor and product id? Which PHY is that? Why can't you use c22 or c45 compatible? A comment would be nice.
+ reg = <0x1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&rgmii_phy0_rst>;
+ reset-assert-us = <20000>;
+ reset-deassert-us = <100000>;
+ reset-gpios = <&gpio3 RK_PD3 GPIO_ACTIVE_LOW>;
+ };
+};
+
+&mdio1 {
+ rgmii_phy1: ethernet-phy@1 {
+ compatible = "ethernet-phy-id4f51.e91b";
Ditto.
[...]
+&sdhci {
+ bus-width = <8>;
+ full-pwr-cycle-in-suspend;
+ max-frequency = <200000000>;
Already that value in rk3576.dtsi.
+ mmc-hs400-1_8v;
+ mmc-hs400-enhanced-strobe;
+ no-sdio;
+ no-sd;
+ non-removable;
+ status = "okay";
+};
+
+&sdmmc {
+ bus-width = <4>;
+ cap-sd-highspeed;
+ disable-wp;
+ max-frequency = <200000000>;
Already that value in rk3576.dtsi.
+ no-sdio;
+ no-mmc;
+ sd-uhs-sdr104;
+ vqmmc-supply = <&vccio_sd_s0>;
+ status = "okay";
+};
+
+&saradc {
This is not alphabetically sorted, it should be before &sata0.
[...]
+ bluetooth {
+ compatible = "brcm,bcm43438-bt";
+ clocks = <&hym8563>;
+ clock-names = "lpo";
+ device-wakeup-gpios = <&gpio1 RK_PD4 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio0>;
+ interrupts = <RK_PB1 IRQ_TYPE_LEVEL_HIGH>;
+ pinctrl-0 = <&bt_reg_on &bt_wake_host &host_wake_bt>;
+ pinctrl-names = "default";
+ shutdown-gpios = <&gpio1 RK_PC7 GPIO_ACTIVE_HIGH>;
Is this GPIO only controlling Bluetooth or also WiFi? I've seen a few combo chips where there's a common GPIO that controls both WiFi and Bluetooth. Making this bluetooth-specific means we need Bluetooth on for WiFi to work, a bit unexpected and should probably be modeled another way.
Cheers,
Quentin