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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] DT binding and SoC dtsi disagree:
  Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml…

--

arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

This enables the SGMII LAN port on the Ariaboard Photonicat by turning on
gmac0, the xpcs node and its port 0 sub-node, adding the YT8521SC PHY on
mdio0, and adding PHY LED nodes for both the new and existing PHYs.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 58c1052ba8ef3..fdaa4a2a4328b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts

[ ... ]

> @@ -586,3 +646,13 @@ &xin32k {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&clk32k_out1>;
>  };
> +
> +&xpcs {
> +	phys = <&combphy2 PHY_TYPE_SGMII>;
> +	phy-names = "serdes";
> +	status = "okay";
> +};

[Severity: Medium]

Should phys/phy-names live in the board file rather than in rk3568.dtsi?

Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml lists
both of them as mandatory:

    required:
      - compatible
      - reg
      - "#address-cells"
      - "#size-cells"
      - clocks
      - clock-names
      - phys
      - phy-names
      - power-domains

    additionalProperties: false

But the SoC node in arch/arm64/boot/dts/rockchip/rk3568.dtsi supplies
neither:

    xpcs: ethernet-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";
            ...
    };

Since dtschema selects on compatible regardless of status = "disabled",
doesn't every other rk3568 board DTB now fail make CHECK_DTBS=1 with
"'phys' is a required property" and "'phy-names' is a required property",
with rk3568-photonicat being the only board that happens to satisfy the
binding?

The SerDes here also looks SoC-internal rather than board-specific: it is
combphy2 in SGMII mode, the glue driver asks for devm_phy_get(dev,
"serdes"), and the binding's own example already uses
phys = <&combphy2 PHY_TYPE_SGMII>.

Would it be better to put phys/phy-names in rk3568.dtsi, so board files
only flip status and set rockchip,sgmii-mac-sel, or alternatively to drop
the two properties from the binding's required: list?

I checked the following commits in the series and did not find a patch that
adds these properties to rk3568.dtsi or relaxes the binding, and the
baseline has no xpcs node at all, so the disagreement appears to come from
this series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com