Hi Quentin,

Thank you for your patient review.

On 1/7/2026 11:46 PM, Quentin Schulz wrote:

Hi Chaoyi,

On 1/7/26 8:03 AM, Chaoyi Chen wrote:
[...]

quoted

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?

Hmm, I will delete them in v3.

quoted

+        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.

I think one exception is the regulator labels. Even though their
phandles are unused, they match the names on the schematic, so it
seems meaningful to keep them.

quoted

+    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?

I checked other boards that use the hym8563 device and couldn't find
a similar approach. Could you give an example?

quoted

+    };
+
+    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?

In v1 we set the pinctrl inside the SATA node. To keep the pins from
being reused by mistake we added this regulator in v2.

quoted

+        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?

It's a good idea. Actually, we have two regulators here, one for USB0
and another for USB1. I'll try to rename them in v2.

Additionally, why is this always-on? I would assume the USB controller is capable of controlling its regulator(s)?

Oh, it should be remove.

[...]

quoted

+    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?

I rechecked the schematic, and found that this regulator should be set
to vcc-supply. You are right, thank you!

quoted

+        compatible = "regulator-fixed";
+        regulator-name = "vcc_ufs_s0";
+        regulator-boot-on;
+        regulator-always-on;

Why always on?

[...]

This is related to the hardware design. The power rail is always on
in hardware.

quoted

+&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.

No, c22 compatible also works. I will use it in v3.

quoted

+        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.

[...]

quoted

+&sdhci {
+    bus-width = <8>;
+    full-pwr-cycle-in-suspend;
+    max-frequency = <200000000>;

Already that value in rk3576.dtsi.

I will drop them in v3.

quoted

+    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.

I will drop them in v3.

quoted

+    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.

[...]

Oh, that's true. I will fix it in v3.

quoted

+    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.

No. BT and Wi-Fi functions are controlled by two separate sets of GPIOs.

-- 
Best, 
Chaoyi