Re: [PATCH net-next v4 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

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.
---
dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

This adds a new standalone schema describing the Synopsys DesignWare XPCS
as integrated on the Rockchip RK3568, with a CSR window, csr/eee clocks, a
Naneng Combo SerDes phy, a power domain and four MII child ports.  A few
questions about the property set and the file naming below.

quoted

diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
new file mode 100644
index 0000000000000..0852d0bcb66a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml

@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/rockchip-dwxpcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#

[Severity: Low]

This isn't a bug, but should the file be named after the documented
compatible, i.e. rockchip,rk3568-xpcs.yaml?  The other schemas in
Documentation/devicetree/bindings/net/pcs/ all use that form:
snps,dw-xpcs.yaml, fsl,lynx-pcs.yaml, mediatek,sgmiisys.yaml,
renesas,rzn1-miic.yaml.

A rename would also have to update the $id line above, since it encodes the
same name.

Related: the example at the bottom uses pcs@fda00000 and pcs-mii@0, while
the sibling PCS binding uses the generic ethernet-pcs@... node name, and no
$nodename constraint is added here.  The SoC dtsi added later in the series
copies the pcs@/pcs-mii@ names verbatim.

I chose rockchip-dwxpcs.yaml intentionally, in case Rockchip integrates
the same XPCS IP into other SoCs in the future. The filename is more
generic and can cover multiple compatible strings if needed.

However, I understand the convention is to name the file after the
compatible string (like snps,dw-xpcs.yaml and fsl,lynx-pcs.yaml). If
you prefer rockchip,rk3568-xpcs.yaml, I will rename it.

quoted

+properties:
+  compatible:
+    const: rockchip,rk3568-xpcs

[Severity: Low]

This isn't a bug, but would a set of SoC-specific if/then blocks in
Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml be preferred
over a second schema for the same IP?

To be clear, a snps,dw-xpcs fallback compatible or a $ref to that schema
would be wrong here.  The glue driver added later in the series builds CSR
addresses with only a 3-bit MMD field:

        return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);

and remaps the vendor MMDs in xpcs_rk_mdio_read_remapping() /
xpcs_rk_mdio_write_remapping(), so the generic direct mapping described in
snps,dw-xpcs.yaml would compute different offsets.

So the only question is where the description should live, not whether the
compatible should be shared.

As you noted, a $ref to snps,dw-xpcs.yaml would be wrong because the
Rockchip glue uses a different address mapping (3-bit MMD field with
remapping). A separate schema is the right choice.

[Severity: Low]

Should the sbd_intr_o output be described as an optional interrupts
property?  With additionalProperties: false at the end of the file, a
conforming device tree cannot carry it, although adding it later stays
backward compatible.

The single unnamed 2 MiB window itself does match the driver:
xpcs_rk_init_res() uses platform_get_resource(pdev, IORESOURCE_MEM, 0) and
requires resource_size(res) >= SZ_2M, and the largest computed offset
0x7FFFF << 2 = 0x1FFFFC still falls inside 0x200000, so reg-names and
reg-io-width are not needed.

These are optional and can be added later without breaking ABI. I'll
leave them out for now, since the mainline XPCS driver doesn't use them.

quoted

+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  clocks:
+    items:
+      - description: APB3 bus interface clock (clk_csr_i), required for register access
+      - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
+
+  clock-names:
+    items:
+      - const: csr
+      - const: eee
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: serdes
+
+  power-domains:
+    maxItems: 1

[Severity: Low]

Should resets/reset-names be described here too?  The RK3568 CRU exposes
four dedicated reset lines for this block in
include/dt-bindings/clock/rk3568-cru.h:

        #define SRST_XPCS               150
        #define SRST_XPCS_TX_DIV10      151
        #define SRST_XPCS_RX_DIV10      152
        #define SRST_XPCS_XGXS_RX       153

Since the schema ends with additionalProperties: false, a conforming device
tree cannot express them at all, and the glue driver added later in the
series never requests a reset control, so it depends on those resets already
being de-asserted.

Peer blocks in the same island do describe theirs, e.g. combphy0 with
resets = <&cru SRST_PIPEPHY0> and gmac0 with resets = <&cru SRST_A_GMAC0>.

Note the XPCS core does provide a register-level reset via xpcs_soft_reset()
in drivers/net/pcs/pcs-xpcs.c, so a state-machine restart is possible
without the CRU resets, and an optional resets property can be added later
without breaking the ABI.

These are optional and can be added later without breaking ABI. I'll
leave them out for now, since the mainline XPCS driver doesn't use them.

quoted

+patternProperties:
+  "^pcs-mii@[0-3]$":
+    type: object
+    description:
+      One of the four MII ports of the XPCS. The port is linked to an
+      Ethernet MAC controller via the pcs-handle property in the MAC's
+      device tree node.
+
+    properties:
+      reg:
+        description: MII port number.
+        enum: [0, 1, 2, 3]

[Severity: Low]

This isn't a bug, but is pcs-mii@N the intended name for these per-port
children rather than the generic ethernet-pcs@N?

Resolving a MAC pcs-handle that points at one of these children does work
with the rest of the series: xpcs_rk_create() in
drivers/net/pcs/pcs-xpcs-rk.c reads the child reg, walks up with
of_get_parent() and calls xpcs_create_mdiodev(pxpcs->bus, port), and
rk_pcs_init() in drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c does
of_parse_phandle(np, "pcs-handle", 0).  So the open point is just the node
naming.

Yes, I will use ethernet-pcs@N

quoted

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

[Severity: Medium]

Does requiring phys and phy-names here make dtbs_check fail for the RK3568
boards?  The SoC node added later in the same series omits both, because the
SerDes selection is board specific:

arch/arm64/boot/dts/rockchip/rk3568.dtsi:
        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";

and only the board fills them in:

arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts:
        &xpcs {
                status = "okay";
                phys = <&combphy2 PHY_TYPE_SGMII>;
                phy-names = "serdes";
        };

dtschema validates nodes irrespective of status = "disabled", so wouldn't
every rk3568*.dtb apart from the Photonicat report "'phys', 'phy-names' are
required properties"?

Should phys/phy-names be dropped from required (or made conditional), or
should the SoC dtsi carry them instead?

This is not actually a problem. dtbs_check and CHECK_DTBS only validate
required properties for enabled nodes. As confirmed by Rob Herring:

    "the dtbs_check only checks required properties from yaml if the
     node is enabled. Yes, that is exactly how it works."

The xpcs node in rk3568.dtsi is marked 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 (as Photonicat does).

This is also the standard pattern for SoC nodes with board-specific
connections: keep the SoC node disabled, and let the board dts enable
it with all required properties.

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

Thanks,