Re: [PATCH v2 2/2] arm64: dts: rockchip: Add rk3576 evb2 board

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

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:
> [...]
>> 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.

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

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

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

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

> [...]
> 
>> +    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!

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

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

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

I will drop them in v3.

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

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

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