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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 5

- [Low] Concern (validated as a false positive): the new schema was said
  to fork the DW XPCS description in two, to lack the conventional…
- [Low] Concern (validated as a false positive): the four `ethernet-pcs-
  mii@[0-3]` child nodes were said to have no in-tree implementation,…
- [Low] Concern (validated as a false positive): the binding omits
  `resets`/`reset-names` for the RK3568 XPCS soft resets (SRST_XPCS,…
- [Low] Concern (validated as a false positive): the mandatory `eee`
  clock is a documented property that no in-tree XPCS code requests…
- [Low] Concern (validated as a false positive): the binding omits `reg-
  names` (direct/indirect) and `reg-io-width`, so the CSR mapping style…

--

dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

This adds a new YAML binding for the Synopsys DesignWare XPCS as
integrated on the RK3568, described as an APB3-mapped block with a
Naneng Combo SerDes PHY and four MII child ports.  All the observations
below are documentation-level questions only; the accompanying driver
patches in the series do implement what the schema describes.

> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> new file mode 100644
> index 0000000000000..703fcff0e3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> @@ -0,0 +1,110 @@

[ ... ]

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

[Severity: Low]
This isn't a bug, but would it be worth saying in the commit message why
this gets its own schema and a single compatible, rather than the usual
"rockchip,rk3568-xpcs", "snps,dw-xpcs" fallback?

The glue driver added later in the series maps registers differently from
the generic one:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_addr_format() {
      return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
  }

and remaps MDIO_MMD_PMAPMD/MDIO_MMD_VEND2 per port in
xpcs_rk_mdio_read_remapping(), so a snps,dw-xpcs fallback would let
pcs-xpcs-plat bind and touch the wrong MMDs.  Recording that reasoning in
the changelog would save the next reader the same investigation.

> +  reg:
> +    maxItems: 1

[Severity: Low]
The existing snps,dw-xpcs binding describes reg-names (direct/indirect)
and reg-io-width; neither appears here.  Is the intent that the compatible
alone implies a direct-mapped 2 MiB window with a fixed 4-byte stride?

That is what the driver assumes:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_res() {
      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
      ...
      if (resource_size(res) < SZ_2M) {

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_read_reg() {
      ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;

Would it help to state the 2 MiB size and the 4-byte register stride in
the description so the omission of reg-names and reg-io-width is clearly
deliberate?

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

[Severity: Low]
The fixed [csr, eee] list differs from the names the generic XPCS code
looks up ("core" and "pad" in pcs-xpcs.c, "csr" in pcs-xpcs-plat.c).  Is
the exclusion of core/pad intentional here?

Both documented clocks are used by the Rockchip glue:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_clk() {
      pxpcs->csr_clk = devm_clk_get(dev, "csr");
      ...
      pxpcs->eee_clk = devm_clk_get(dev, "eee");
      ...
      pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
                                               1000000000UL) - 1;

so the naming looks consistent with the implementation; a sentence in the
changelog noting that this integration does not use the core/pad clocks
would make the difference from snps,dw-xpcs obvious.

[ ... ]

> +patternProperties:
> +  "^ethernet-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.

[Severity: Low]
This isn't a bug, but the changelog doesn't mention the two patches that
consume these child nodes, which makes the new ABI look unimplemented when
the binding patch is read on its own.

The consumers are:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_create() {
      if (of_property_read_u32(np, "reg", &port))
              return ERR_PTR(-EINVAL);
      ...
      xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);

  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_pcs_init() {
      pcs_node = of_parse_phandle(np, "pcs-handle", 0);
      ...
      xpcs = xpcs_rk_create(priv->device, pcs_node);

Could the commit message reference them?

[ ... ]

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

[Severity: Low]
The RK3568 CRU defines four XPCS soft resets:

  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

but the schema has no resets/reset-names, so a device tree cannot express
them.  Is that because bring-up relies only on the power domain, the
SerDes PHY and the XPCS register-level soft reset?

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_probe() {
      ret = dev_pm_genpd_rpm_always_on(dev, true);
      ...
      ret = phy_init(pxpcs->serdes_phy);
      ...
      ret = phy_power_on(pxpcs->serdes_phy);

A grep at the end of the series shows SRST_XPCS* matching only the
dt-bindings header, with no driver or DTS user, so this may well be
intentional; a note confirming the resets are left at their SoC defaults
would settle it.

Documenting them now as optional properties would also stay backwards
compatible if they turn out to be needed later.

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