Re: [PATCH net-next v4 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

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

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: add XPCS and fixed-clock nodes

This adds the RK3568 Synopsys DesignWare XPCS node (with four pcs-mii
sub-nodes) plus two 125 MHz fixed-clock nodes used as GMAC reference
clocks.  The XPCS node and its sub-nodes are disabled by default and are
meant to be completed and enabled at the board level.

A couple of questions about the new nodes below.

quoted

diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index 3bc653f027f1f..989e164c0eb39 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi

@@ -110,6 +110,51 @@ sata0: sata@fc000000 {
              status = "disabled";
      };

+     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";

[Severity: Medium]
Does this node validate against the binding added earlier in the same
series?  Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
lists phys and phy-names unconditionally:

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

The node here supplies neither, and dt-validate does not skip nodes with
status = "disabled", so dtbs_check should report "'phys' is a required
property" and "'phy-names' is a required property" for every rk3568 dtb
that includes this dtsi.

At the tip of the series only rk3568-photonicat.dts fills them in:

&xpcs {
        status = "okay";
        phys = <&combphy2 PHY_TYPE_SGMII>;
        phy-names = "serdes";
};

so rk3568-evb, rk3568-rock-3a, rk3568-roc-pc, rk3568-radxa-cm3j,
rk3568-wolfvision-pf5 and friends would still be missing them.

If which combphy carries SGMII is genuinely a board-level choice, should
phys/phy-names be moved out of the unconditional required: list in the
binding instead, or should the dtsi provide a default?

As I replied in the earlier patch, dtbs_check and CHECK_DTBS only
validate required properties for enabled nodes, as confirmed by Rob
Herring. The xpcs node in rk3568.dtsi is status = "disabled", so
boards that include rk3568.dtsi without enabling XPCS will not trigger
any validation warnings. Only boards that enable it need to provide
phys and phy-names, and they do so at the board level.

The phys/phy-names properties cannot be moved into rk3568.dtsi anyway,
because the SerDes connection is board-specific: some boards use
combphy1 for SGMII, others use combphy2 (like Photonicat).

quoted

+     xpcs_gmac0_clk: xpcs-gmac0-clock {
+             compatible = "fixed-clock";
+             clock-frequency = <125000000>;
+             clock-output-names = "clk_gmac0_xpcs_mii";
+             #clock-cells = <0>;
+     };
+
+     xpcs_gmac1_clk: xpcs-gmac1-clock {
+             compatible = "fixed-clock";
+             clock-frequency = <125000000>;
+             clock-output-names = "clk_gmac1_xpcs_mii";
+             #clock-cells = <0>;
+     };

[Severity: Low]
These two node names take the deprecated branch of the fixed-clock
$nodename schema.  Documentation/devicetree/bindings/clock/fixed-clock.yaml
has:

  $nodename:
    anyOf:
      - description:
          Preferred name is 'clock-<freq>' with <freq> being the output
          frequency as defined in the 'clock-frequency' property.
        pattern: "^clock-([0-9]+|[a-z0-9-]+)$"
      - description: Any name allowed
        deprecated: true

xpcs-gmac0-clock and xpcs-gmac1-clock do not match the preferred
pattern, so they land on the "Any name allowed" branch.  Newer Rockchip
dtsi files use the preferred form already, for example rk3528.dtsi with
xin24m: clock-xin24m and gmac0_clk: clock-gmac50m.

Could these be named clock-xpcs-gmac0 and clock-xpcs-gmac1?  The labels
xpcs_gmac0_clk / xpcs_gmac1_clk and all references would stay the same.

You're right, xpcs-gmac0-clock and xpcs-gmac1-clock do not match the
preferred clock-<freq> pattern in fixed-clock.yaml. I'll rename them to
clock-xpcs-gmac0 and clock-xpcs-gmac1. The labels xpcs_gmac0_clk and
xpcs_gmac1_clk will be match clock-output-names.

Best,
Coia