Re: [PATCH net-next v4 09/10] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-09-12 20:14:44
Also in:
linux-arm-kernel, linux-devicetree, linux-phy, linux-renesas-soc, linux-rockchip, lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port This enables the GMAC0 SGMII LAN port on the Ariaboard Photonicat by adding the xpcs/xpcs_mii0 overrides, the combphy2 SGMII MAC selection, and an mdio0 node with the YT8521SC PHY. It also adds PHY LED descriptions on both mdio0 and mdio1.
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts index 58c1052ba8ef3..25caa44198843 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
[ ... ]
quoted hunk ↗ jump to hunk
@@ -242,6 +243,7 @@ &combphy1 { &combphy2 { status = "okay"; + rockchip,sgmii-mac-sel = <0>; };
[Severity: Low]
The changelog says:
"Enable combphy2 with rockchip,sgmii-mac-sel = <0> to route the SGMII
interface to GMAC0."
Is that description accurate? In the baseline the node already reads:
&combphy2 {
status = "okay";
};
so the only new thing here is the rockchip,sgmii-mac-sel property. Could
the wording be adjusted to say the property is added to an already enabled
combphy2?
[Severity: Low]
This isn't a bug, but the new properties do not follow the ordering used by
the rest of this file, where properties come roughly alphabetically and
status is last. Here rockchip,sgmii-mac-sel lands after status, in &gmac0
below pcs-handle comes before managed, and in the new &xpcs override status
is first with phys/phy-names after it. Compare the existing &gmac1
override, which ends with status = "okay";. Could these be reordered?
quoted hunk ↗ jump to hunk
&cpu0 {@@ -260,9 +262,18 @@ &cpu3 { cpu-supply = <&vdd_cpu>; }; -/* Motorcomm YT8521SC LAN port (require SGMII) */ +/* Motorcomm YT8521SC LAN port */ &gmac0 { - status = "disabled"; + assigned-clocks = <&cru SCLK_GMAC0_RX_TX>; + assigned-clock-parents = <&xpcs_gmac0_clk>; + pcs-handle = <&xpcs_mii0>; + managed = "in-band-status"; + phy-handle = <&sgmii_phy>; + phy-mode = "sgmii"; + phy-supply = <&vcc_3v3>; + pinctrl-names = "default"; + pinctrl-0 = <&gmac0_miim>; + status = "okay"; };
[ ... ]
quoted hunk ↗ jump to hunk
@@ -350,6 +391,25 @@ rgmii_phy: ethernet-phy@3 { reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>; rx-internal-delay-ps = <1500>; tx-internal-delay-ps = <1500>; + + leds { + #address-cells = <1>; + #size-cells = <0>; + + led@1 { + reg = <1>; + color = <LED_COLOR_ID_AMBER>; + function = LED_FUNCTION_WAN; + default-state = "keep"; + }; + + led@2 { + reg = <2>; + color = <LED_COLOR_ID_GREEN>; + function = LED_FUNCTION_WAN; + default-state = "keep"; + }; + }; }; };
[Severity: Low] This hunk describes the LEDs of the pre-existing RGMII WAN PHY on &mdio1, which is already functional and unaffected by the SGMII enablement. The changelog justifies it only with: "Also add LED configuration for the existing RGMII PHY on mdio1 for consistency." Would it be preferable to split this into its own patch, so this commit contains only the SGMII LAN port enablement? [ ... ]
quoted hunk ↗ jump to hunk
@@ -586,3 +646,13 @@ &xin32k { pinctrl-names = "default"; pinctrl-0 = <&clk32k_out1>; }; + +&xpcs { + status = "okay"; + phys = <&combphy2 PHY_TYPE_SGMII>; + phy-names = "serdes"; +};
[Severity: Medium]
Should phys/phy-names live in rk3568.dtsi rather than in each board file?
The binding lists them as required properties of the XPCS node:
Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
required:
- compatible
- reg
- "#address-cells"
- "#size-cells"
- clocks
- clock-names
- phys
- phy-names
- power-domains
but the SoC node in arch/arm64/boot/dts/rockchip/rk3568.dtsi has neither:
xpcs: pcs@fda00000 {
compatible = "rockchip,rk3568-xpcs";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x0 0xfda00000 0x0 0x200000>;
clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
clock-names = "csr", "eee";
power-domains = <&power RK3568_PD_PIPE>;
status = "disabled";
Does this mean every rk3568 dtb reports pcs@fda00000 as missing the required
phys and phy-names under make CHECK_DTBS=y, including boards that do not
enable the XPCS?
The XPCS to combphy2 connection is internal to the SoC, so each board that
enables it will have to repeat the same phandle, as this patch does. Could
phys = <&combphy2 PHY_TYPE_SGMII>; and phy-names = "serdes"; be moved into
the dtsi node instead?
+
+&xpcs_mii0 {
+ status = "okay";
+};