Re: [PATCH v2 2/2] arm64: dts: rockchip: Add rk3399-evb-ind board

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

Hi Quentin,

On 2025/4/30 16:39, Quentin Schulz wrote:
> Hi Chaoyi Chen,
>
> On 4/30/25 9:48 AM, Chaoyi Chen wrote:
>> From: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>>
>> General feature for rk3399 industry evaluation board:
>> - Rockchip RK3399
>
> Quick question, is this really an RK3399 or rather an RK3399K (the 
> industrial grade)? If it's a K variant, then we need a new 
> rk3399-k.dtsi with the appropriate OPPs and include that instead.

Yes, it is really an RK3399.


>
>> - 4GB LPDDR4
>> - emmc5.1
>> - SDIO3.0 compatible TF card
>> - 1x HDMI2.0a TX
>> - 1x HDMI1.4b RX with TC358749XBG HDMI to MIPI CSI2 bridge chip
>> - 1x type-c DisplayPort
>> - 3x USB3.0 Host
>> - 1x USB2.0 Host
>> - 1x Ethernet / USB3.0 to Ethernet
>>
>> Tested with HDMI/GPU/USB2.0/USB3.0/TF card/emmc.
>>
>> Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - Remove gmac
>> - Add rk809 PMIC
>> - Add CPU supply
>> - Fix io-domain for sdmmc
>> - Enable vopl
>>
>>   arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>>   .../boot/dts/rockchip/rk3399-evb-ind.dts      | 466 ++++++++++++++++++
>>   2 files changed, 467 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-evb-ind.dts
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
>> b/arch/arm64/boot/dts/rockchip/Makefile
>> index 3e8771ef69ba..8a3adb7482ca 100644
>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>> @@ -40,6 +40,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-px5-evb.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-r88.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-eaidk-610.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-evb.dtb
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-evb-ind.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-ficus.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-firefly.dtb
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-gru-bob.dtb
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb-ind.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3399-evb-ind.dts
>> new file mode 100644
>> index 000000000000..ebfe7e56b3f5
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-evb-ind.dts
>> @@ -0,0 +1,466 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2025 Rockchip Electronics Co., Ltd.
>> + */
>> +
>> +/dts-v1/;
>> +#include "rk3399-base.dtsi"
>> +
>
> You're supposed to include one device tree include with OPPs. I see 
> that the rk3399-evb is the only one that does that.
>
> According to 296602b8e5f7 ("arm64: dts: rockchip: Move RK3399 OPPs to 
> dtsi files for SoC variants"), this was done because:
>
>     The only exception to the "include only a SoC variant dtsi" is 
> found in
>     rk3399-evb.dts, which includes rk3399-base.dtsi instead of 
> rk3399.dtsi.
>     This is intentional, because this board dts file doesn't enable 
> the TSADC,
>     so including rk3399.dtsi would enable the SoC to go into higher 
> OPPs with
>     no thermal throttling in place.  Let's hope that people interested 
> in this
>     board will fix this in the future.
>
> Soooo, maybe you could fix that now? At the very least for the 
> industrial variant (but it would also be most welcomed for the 
> "normal" EVB).
>
I think this should include "rk3399.dtsi", since this is just the normal 
version of RK3399. I will investigate further about rk3399-evb.


>> +/ {
>> +    model = "Rockchip RK3399 EVB IND LPDDR4 Board";
>> +    compatible = "rockchip,rk3399-evb-ind", "rockchip,rk3399";
>> +
>> +    aliases {
>> +        ethernet0 = &gmac;
>
> Considering gmac is gone from the DT, maybe you want to have that one 
> in the patch that will add Ethernet support?

Yes, there will be an additional series. I will remove them in v3 for now.


>
>> +        mmc0 = &sdhci;
>> +        mmc1 = &sdmmc;
>> +    };
>> +
>> +    chosen {
>> +        stdout-path = "serial2:1500000n8";
>> +    };
>> +
>> +    vcc5v0_sys: regulator-vcc5v0-sys {
>> +        compatible = "regulator-fixed";
>> +        enable-active-high;
>> +        gpio = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
>> +        regulator-name = "vcc5v0_sys";
>> +        regulator-always-on;
>> +        regulator-boot-on;
>> +        regulator-max-microvolt = <5000000>;
>> +        regulator-min-microvolt = <5000000>;
>> +    };
>> +};
>> +
>> +&cpu_l0 {
>> +    cpu-supply = <&vdd_cpu_l>;
>> +};
>> +
>> +&cpu_l1 {
>> +    cpu-supply = <&vdd_cpu_l>;
>> +};
>> +
>> +&cpu_l2 {
>> +    cpu-supply = <&vdd_cpu_l>;
>> +};
>> +
>> +&cpu_l3 {
>> +    cpu-supply = <&vdd_cpu_l>;
>> +};
>> +
>> +&cpu_b0 {
>> +    cpu-supply = <&vdd_cpu_b>;
>> +};
>> +
>> +&cpu_b1 {
>> +    cpu-supply = <&vdd_cpu_b>;
>> +};
>> +
>
> Please order the nodes alphabetically, so cpu_bX before cpu_lX. See 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html

Will fix in v3.


>
>> +&emmc_phy {
>> +    status = "okay";
>> +};
>> +
>> +&gpu {
>> +    mali-supply = <&vdd_gpu>;
>> +    status = "okay";
>> +};
>> +
>> +&hdmi {
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&hdmi_i2c_xfer>, <&hdmi_cec>;
>> +    status = "okay";
>> +};
>> +
>> +&hdmi_sound {
>> +    status = "okay";
>> +};
>> +
>
> hdmi_sound requires i2s2 controller to be enabled too, but I don't see 
> it in this device tree?

Will fix in v3.


>
>> +&i2c0 {
>> +    clock-frequency = <400000>;
>> +    i2c-scl-falling-time-ns = <4>;
>> +    i2c-scl-rising-time-ns = <168>;
>> +    status = "okay";
>> +
>> +    rk809: pmic@20 {
>> +        compatible = "rockchip,rk809";
>> +        reg = <0x20>;
>> +        interrupt-parent = <&gpio1>;
>> +        interrupts = <RK_PC5 IRQ_TYPE_LEVEL_LOW>;
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&pmic_int_l>;
>> +        rockchip,system-power-controller;
>> +        #clock-cells = <1>;
>> +        pmic-reset-func = <0>;
>> +        wakeup-source;
>> +        clock-output-names = "xin32k", "rk808-clkout2";
>> +
>
> Please order properties correctly, see 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html

Will fix in v3.


>
>> +        vcc1-supply = <&vcc5v0_sys>;
>> +        vcc2-supply = <&vcc5v0_sys>;
>> +        vcc3-supply = <&vcc5v0_sys>;
>> +        vcc4-supply = <&vcc5v0_sys>;
>> +        vcc5-supply = <&vcc_buck5>;
>> +        vcc6-supply = <&vcc_buck5>;
>> +        vcc7-supply = <&vcc5v0_sys>;
>> +        vcc8-supply = <&vcc3v3_sys>;
>> +        vcc9-supply = <&vcc5v0_sys>;
>> +
>> +        regulators {
>> +            vdd_center: DCDC_REG1 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-initial-mode = <0x2>;
>> +                regulator-max-microvolt = <1350000>;
>> +                regulator-min-microvolt = <750000>;
>> +                regulator-ramp-delay = <6001>;
>> +                regulator-name = "vdd_center";
>
> Missing blank line between last property and first child node, see 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html. 
> Same remark for all other regulators.

Will fix them in v3.


>
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vdd_cpu_l: DCDC_REG2 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-initial-mode = <0x2>;
>> +                regulator-max-microvolt = <1350000>;
>> +                regulator-min-microvolt = <750000>;
>> +                regulator-ramp-delay = <6001>;
>> +                regulator-name = "vdd_cpu_l";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc_ddr: DCDC_REG3 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-initial-mode = <0x2>;
>> +                regulator-name = "vcc_ddr";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc3v3_sys: DCDC_REG4 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-initial-mode = <0x2>;
>> +                regulator-max-microvolt = <3300000>;
>> +                regulator-min-microvolt = <3300000>;
>> +                regulator-name = "vcc3v3_sys";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                    regulator-suspend-microvolt = <3300000>;
>> +                };
>> +            };
>> +
>> +            vcc_buck5: DCDC_REG5 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <2200000>;
>> +                regulator-min-microvolt = <2200000>;
>> +                regulator-name = "vcc_buck5";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                    regulator-suspend-microvolt = <2200000>;
>> +                };
>> +            };
>> +
>> +            vcca_0v9: LDO_REG1 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <900000>;
>> +                regulator-min-microvolt = <900000>;
>> +                regulator-name = "vcca_0v9";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc_1v8: LDO_REG2 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <1800000>;
>> +                regulator-min-microvolt = <1800000>;
>> +                regulator-name = "vcc_1v8";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                    regulator-suspend-microvolt = <1800000>;
>> +                };
>> +            };
>> +
>> +            vcc0v9_soc: LDO_REG3 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <900000>;
>> +                regulator-min-microvolt = <900000>;
>> +                regulator-name = "vcc0v9_soc";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                    regulator-suspend-microvolt = <900000>;
>> +                };
>> +            };
>> +
>> +            vcca_1v8: LDO_REG4 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <1800000>;
>> +                regulator-min-microvolt = <1800000>;
>> +                regulator-name = "vcca_1v8";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vdd1v5_dvp: LDO_REG5 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <1500000>;
>> +                regulator-min-microvolt = <1500000>;
>> +                regulator-name = "vdd1v5_dvp";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc_1v5: LDO_REG6 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <1500000>;
>> +                regulator-min-microvolt = <1500000>;
>> +                regulator-name = "vcc_1v5";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc_3v0: LDO_REG7 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <3000000>;
>> +                regulator-min-microvolt = <3000000>;
>> +                regulator-name = "vcc_3v0";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vccio_sd: LDO_REG8 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <3300000>;
>> +                regulator-min-microvolt = <1800000>;
>> +                regulator-name = "vccio_sd";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc_sd: LDO_REG9 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-max-microvolt = <3300000>;
>> +                regulator-min-microvolt = <3300000>;
>> +                regulator-name = "vcc_sd";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +
>> +            vcc5v0_usb: SWITCH_REG1 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-name = "vcc5v0_usb";
>> +                regulator-state-mem {
>> +                    regulator-on-in-suspend;
>> +                };
>> +            };
>> +
>> +            vccio_3v3: SWITCH_REG2 {
>> +                regulator-always-on;
>> +                regulator-boot-on;
>> +                regulator-name = "vccio_3v3";
>> +                regulator-state-mem {
>> +                    regulator-off-in-suspend;
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +    vdd_cpu_b: tcs4525@1c {
>
> Please set the node name to regulator@1c.

Will fix in v3.

>
>> +        compatible = "tcs,tcs4525";
>> +        reg = <0x1c>;
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&vsel1_gpio>;
>> +        fcs,suspend-voltage-selector = <1>;
>> +        vin-supply = <&vcc5v0_sys>;
>> +        vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
>> +        regulator-compatible = "fan53555-reg";
>> +        regulator-always-on;
>> +        regulator-boot-on;
>> +        regulator-initial-state = <3>;
>> +        regulator-max-microvolt = <1500000>;
>> +        regulator-min-microvolt = <712500>;
>> +        regulator-name = "vdd_cpu_b";
>> +        regulator-ramp-delay = <1000>;
>> +        regulator-state-mem {
>> +            regulator-off-in-suspend;
>> +        };
>
> Properties order and blank line before child node, see 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html

Will fix in v3.


>
>> +    };
>> +
>> +    vdd_gpu: tcs4526@10 {
>
> Please set node name to regulator@10.
>
> Please order child nodes in busses based on their address, i.e.:
>
> vdd_gpu
> vdd_cpu_b
> rk809
>
> in this case.

Will fix in v3.


>
>
>> +        compatible = "tcs,tcs4525";
>> +        reg = <0x10>;
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&vsel2_gpio>;
>> +        fcs,suspend-voltage-selector = <1>;
>> +        vin-supply = <&vcc5v0_sys>;
>> +        vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
>> +        regulator-compatible = "fan53555-reg";
>> +        regulator-always-on;
>> +        regulator-boot-on;
>> +        regulator-initial-state = <3>;
>> +        regulator-max-microvolt = <1500000>;
>> +        regulator-min-microvolt = <712500>;
>> +        regulator-name = "vdd_gpu";
>> +        regulator-ramp-delay = <1000>;
>> +        regulator-state-mem {
>> +            regulator-off-in-suspend;
>> +        };
>
> Properties order and blank line before child node, see 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html

Will fix in v3.


>
>> +    };
>> +};
>> +
>> +&io_domains {
>> +    audio-supply = <&vcca_1v8>;
>> +    bt656-supply = <&vcc_3v0>;
>> +    gpio1830-supply = <&vcc_3v0>;
>> +    sdmmc-supply = <&vccio_sd>;
>> +    status = "okay";
>> +};
>
> What about pmu_io_domains too?
>
Sorry, I missed it. Will fix in v3.


>> +
>> +&sdmmc {
>> +    bus-width = <4>;
>> +    cap-mmc-highspeed;
>> +    cap-sd-highspeed;
>> +    disable-wp;
>> +    no-sdio;
>> +    no-mmc;
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_bus4>;
>> +    sd-uhs-sdr104;
>> +    vmmc-supply = <&vcc_sd>;
>> +    vqmmc-supply = <&vccio_sd>;
>> +    status = "okay";
>> +};
>> +
>> +&sdhci {
>
> This should be before sdmmc

Will fix in v3.


>
>> +    bus-width = <8>;
>> +    keep-power-in-suspend;
>> +    mmc-hs400-1_8v;
>> +    mmc-hs400-enhanced-strobe;
>> +    no-sdio;
>> +    no-sd;
>> +    non-removable;
>> +    status = "okay";
>> +};
>> +
>> +&tcphy0 {
>> +    status = "okay";
>> +};
>> +
>> +&tcphy1 {
>> +    status = "okay";
>> +};
>> +
>
> What about tsadc?

Oh, there's a tsadc. I'll add it in v3.


>
>> +&u2phy0 {
>> +    status = "okay";
>> +};
>> +
>> +&u2phy0_host {
>> +    status = "okay";
>> +};
>> +
>> +&u2phy0_otg {
>> +    status = "okay";
>> +};
>> +
>> +&u2phy1 {
>> +    status = "okay";
>> +};
>> +
>> +&u2phy1_host {
>> +    status = "okay";
>> +};
>> +
>> +&u2phy1_otg {
>> +    status = "okay";
>> +};
>> +
>> +&uart2 {
>> +    status = "okay";
>> +};
>> +
>> +&usbdrd_dwc3_0 {
>> +    status = "okay";
>> +};
>> +
>> +&usbdrd3_0 {
>> +    status = "okay";
>> +};
>> +
>> +&usbdrd3_1 {
>> +    status = "okay";
>> +};
>> +
>> +&usbdrd_dwc3_1 {
>> +    dr_mode = "host";
>> +    status = "okay";
>> +};
>> +
>> +&usb_host0_ehci {
>> +    status = "okay";
>> +};
>> +
>> +&usb_host0_ohci {
>> +    status = "okay";
>> +};
>> +
>> +&usb_host1_ehci {
>> +    status = "okay";
>> +};
>> +
>> +&usb_host1_ohci {
>> +    status = "okay";
>> +};
>> +
>> +&pinctrl {
>
> This needs to be ordered alphabetically, see 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html 
> (so before sdhci here).

Thank you for the quick reply! I will fix them in v3.


> Cheers,
> Quentin
>
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip