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

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

On Fri, Oct 03, 2025 at 09:47:40PM -0500, Jimmy Hon wrote:
> On Thu, Oct 2, 2025 at 9:40 PM Laurent Pinchart wrote:
> > 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>;
> > > > +       };
>
> The majority of the properties should be in alphabetical order. So the
> startup-delay-us and vin-supply are out of place.

OK I'll move those.

> > > > +
> > > > +       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.
>
> The nice thing about the G610 GPU is that OpenCL support via Mesa's
> RustICL was added earlier this year. So even in a headless cluster,
> the GPU can still be useful.

Yes, that's why I decided to move it to the SoM in the end.

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

-- 
Regards,

Laurent Pinchart