Re: [PATCH 3/3] arm64: dts: rockchip: Add rk3588s-orangepi-cm5-base device tree

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jimmy,

On Thu, Oct 02, 2025 at 07:01:53PM -0500, Jimmy Hon wrote:
> A few nitpicks below
> 
> [ snip ]
> > +
> > +#include "rk3588s-orangepi-cm5.dtsi"
> > +
> > +/ {
> > +       model = "Xunlong Orange Pi CM5 Base";
> > +       compatible = "xunlong,orangepi-cm5-base", "xunlong,orangepi-cm5", "rockchip,rk3588s";
> > +
> > +       aliases {
> > +               ethernet0 = &gmac1;
> > +               mmc0 = &sdhci;
> 
> Since sdhci is enabled in the SoM.dtsi, this alias should probably go
> there instead.

Good point, I'll do that.

> > +               mmc1 = &sdmmc;
> > +       };
> > +
> 
> [ snip ]
> 
> > +
> > +       vbus_5v0: vbus-5v0 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vbus_5v0";
> > +               regulator-min-microvolt = <5000000>;
> > +               regulator-max-microvolt = <5000000>;
> > +               enable-active-high;
> > +               gpio = <&gpio0 RK_PD3 GPIO_ACTIVE_HIGH>;
> > +               vin-supply = <&vcc5v0_sys>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&vbus_5v0_en_pin>;
> 
> The property names in these regulators are not as organized as the
> regulators for the CPU/NPU.

Which properties in particular ? There are more properties in these
regulators, but otherwise the order seem to match.

> > +       };
> > +
> > +       vcc_3v3: regulator-vcc-3v3 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vcc_3v3";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               startup-delay-us = <50000>;
> > +               enable-active-high;
> > +               gpio = <&gpio4 RK_PA3 GPIO_ACTIVE_HIGH>;
> > +               vin-supply = <&vcc5v0_sys>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&vcc_3v3_en_pin>;
> > +       };
> > +
> > +       vcc5v0_sys: regulator-vcc-5v0 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vcc5v0_sys";
> > +               regulator-always-on;
> > +               regulator-boot-on;
> > +               regulator-min-microvolt = <5000000>;
> > +               regulator-max-microvolt = <5000000>;
> > +       };
> > +};
> 
> [ snip ]
> 
> > +
> > +&gmac1 {
> > +       clock_in_out = "output";
> > +       phy-handle = <&rgmii_phy>;
> > +       phy-mode = "rgmii-id";
> > +       phy-supply = <&vcc_3v3>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&gmac1_miim
> > +                    &gmac1_rx_bus2
> > +                    &gmac1_tx_bus2
> > +                    &gmac1_rgmii_clk
> > +                    &gmac1_rgmii_bus>;
> > +       tx_delay = <0x42>;
> 
> When using "rgmii-id", tx_delay will be ignored. Does the ethernet
> work without this property?

I have to confess this was blindly copied from the BSP :-/ I'll drop the
property and test.

> See the comment by Jonas in another review.
> https://lore.kernel.org/linux-rockchip/da752790-da17-4d26-b9b2-8240b38b3276@kwiboo.se/
> 
> > +       status = "okay";
> > +};
> > +
> > +&gpu {
> > +       mali-supply = <&vdd_gpu_s0>;
> > +       status = "okay";
> > +};
> 
> This is a feature in the SoC itself, so it's not board specific and
> can be put into the SoM.dtsi.

I'm a bit in two minds here. If a carrier board doesn't have a display
output, the GPU isn't very useful (although in theory the GPU can be
used without a display). That's why I decided to enable it in the
carrier board. I suppose it doesn't hurt to enable it in the SoM, worst
case it won't be used and so won't be powered up. I'll move it to the
SoM.

> [ snip ]
> 
> > +
> > +&pd_gpu {
> > +       domain-supply = <&vdd_gpu_s0>;
> > +};
> 
> Same comment regarding moving to the SoM.dtsi

OK.

-- 
Regards,

Laurent Pinchart