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

[Coia Prant]

Jakub Kicinski <kuba@xxxxxxxxxx> 于2026年9月13日周日 04:13写道：
> > @@ -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)

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

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