Re: [PATCH net-next v4 09/10] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

Jakub Kicinski [off-list ref] 于2026年9月13日周日 04:13写道：

quoted

@@ -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?

You're right. combphy2 is already status = "okay" in the baseline; this
patch only adds the rockchip,sgmii-mac-sel property to it. I'll fix the
wording 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?

I'll reorder the properties to follow the file's convention (roughly
alphabetical, with status last)

quoted

@@ -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?

The LED configuration for both PHYs is kept in this patch because the
two PHYs are the same Motorcomm family wired identically on this board,
and describing them together keeps the two mdio nodes consistent. The
change is purely descriptive and does not affect the SGMII enablement.

That said, if you prefer it split out, I can move the mdio1 LED
description into a separate patch in v4.

quoted

@@ -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?

As replied in the earlier patch, the SerDes connection is board-specific
(some boards use combphy1 for SGMII, others use combphy2), so it cannot
be moved into rk3568.dtsi.

Best,
Coia