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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (disproved as a defect): the binding does not describe
  the CRU reset lines SRST_XPCS / SRST_XPCS_TX_DIV10 /…
- [Low] The binding's description
  (Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml…

--

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

This adds a new YAML binding for the Synopsys DesignWare XPCS as integrated
on the Rockchip RK3568, with four MII ports described as
ethernet-pcs-mii@N child nodes.  The node requires clocks, phys and a power
domain, and the CRU reset lines are deliberately left undescribed.

Two questions about the new schema below.

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

[ ... ]

> +  The XPCS is accessed over the APB3 bus and internally connected to a
> +  Naneng Combo SerDes PHY.
> +  It supports 1000BASE-X, SGMII and QSGMII modes.
> +
> +  The block contains four MII ports that can be individually enabled and
> +  routed to one of the Ethernet GMAC controllers via the pcs-handle
> +  property in the MAC device tree node.

[Severity: Low]
Is the routing really controlled from the XPCS side?  On RK3568 the
SGMII data path mux looks like a single pipe-GRF bit owned by the combphy
driver, not a per-port property of this node:

drivers/phy/rockchip/phy-rockchip-naneng-combphy.c:

    static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
        ...
        .pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 },

and in the PHY_TYPE_SGMII case of rk3568_combphy_cfg():

    rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
                                 priv->sgmii_mac_sel > 0);

priv->sgmii_mac_sel comes from the rockchip,sgmii-mac-sel property and
defaults to 1 (GMAC1) in rockchip_combphy_parse_dt().

That means there is one SGMII path shared by two GMACs rather than four
independently routable ports, doesn't it?

Can a device tree then set gmac0's pcs-handle = <&xpcs_mii0> while the
combphy keeps routing to GMAC1?  Both nodes validate, the XPCS probes and
is configured, and neither driver can notice the disagreement because the
two properties describe the same mux from opposite ends, so the link would
just never come up.

Would it be worth stating here that the MII port to MAC association has to
agree with the combphy's rockchip,sgmii-mac-sel, and how the four ports map
onto the two available GMACs in QSGMII mode, instead of describing per-port
routing via pcs-handle?

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

[ ... ]

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

[Severity: Low]
With no resets property described and additionalProperties: false here, a
device tree cannot express the CRU reset lines SRST_XPCS,
SRST_XPCS_TX_DIV10, SRST_XPCS_RX_DIV10 and SRST_XPCS_XGXS_RX from
include/dt-bindings/clock/rk3568-cru.h at all.  Does that matter for an
unusual bootloader hand-off where the block needs to be re-reset?

For what it is worth, both halves of the rationale in the commit message
check out: the glue driver drivers/net/pcs/pcs-xpcs-rk.c added later in the
series contains no reset_control_* call (it brings the block up through
devm_phy_get("serdes")/phy_power_on plus PD_PIPE runtime PM), and adding an
optional resets/reset-names pair to the schema later stays backwards
compatible.  So this is only a question about whether the DIV10 and XGXS_RX
resets are needed in practice, not an objection to the documented omission.

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