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

[Quentin Schulz <quentin.schulz@cherry.de>]

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