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

[netdev-bot+sashiko]

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

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

- [Medium] The 'required:' list of the new schema mandates 'phys' and
  'phy-names', but the SoC-level node added by the companion patch…
- [Low] Concern: the binding invents a per-port child-node ABI
  ('ethernet-pcs-mii@[0-3]') and mandatory property set that no in-tree…
- [Low] Concern: the binding omits 'resets'/'reset-names' (RK3568 CRU
  exposes SRST_XPCS, SRST_XPCS_TX_DIV10, SRST_XPCS_RX_DIV10,…

--

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

This adds a new YAML binding for the Synopsys DesignWare XPCS block
integrated on the Rockchip RK3568 SoC.  The block is described as being
accessed over APB3, fed by a Naneng Combo SerDes PHY, and exposing four
MII ports as child nodes selected through pcs-handle from a MAC node.

A few questions below, mostly about how the required property list lines
up with the dtsi node added in the same series.

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

[ ... ]

> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: serdes
> +
> +  power-domains:
> +    maxItems: 1
> +
> +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 as a standalone patch this per-port child node
scheme documents an ABI that nothing in the tree consumes yet.  With the
rest of the series applied it does line up:

drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_create() {
	if (of_property_read_u32(np, "reg", &port))
		return ERR_PTR(-EINVAL);
	...
	pcs_np = of_get_parent(np);
	...
	if (!of_match_node(xpcs_rk_of_ids, pcs_np))

and the consumer side resolves the same node:

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

Would it be worth mentioning in the changelog which patch in the series
implements and consumes the ethernet-pcs-mii@N children, so the ABI is
not read as unexercised?

> +
> +    properties:
> +      reg:
> +        description: MII port number.
> +        enum: [0, 1, 2, 3]
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +  - clock-names
> +  - phys
> +  - phy-names
> +  - power-domains

[Severity: Medium]

Does making phys and phy-names mandatory here conflict with the SoC node
added by "arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes"?

arch/arm64/boot/dts/rockchip/rk3568.dtsi:

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

The SerDes link is only filled in at board level:

arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts:

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

dt-validate checks disabled nodes too, so wouldn't make CHECK_DTBS=y
report missing required properties phys and phy-names for the xpcs node
on every rk3568 board dtb other than Photonicat?

Should phys/phy-names be dropped from the required list (or gated), or
should rk3568.dtsi carry them instead?

> +
> +additionalProperties: false

[Severity: Low]

This isn't a bug, but the RK3568 CRU exposes SRST_XPCS,
SRST_XPCS_TX_DIV10, SRST_XPCS_RX_DIV10 and SRST_XPCS_XGXS_RX for this
block, and the schema has no resets/reset-names, while
additionalProperties: false forbids describing them.

Nothing in the tree needs them today: git grep SRST_XPCS at the end of
the series only matches include/dt-bindings/clock/rk3568-cru.h,
drivers/net/pcs/pcs-xpcs-rk.c requests no reset control, and
resynchronisation goes through the in-IP soft reset:

drivers/net/pcs/pcs-xpcs.c:
	static int xpcs_soft_reset(struct dw_xpcs *xpcs,
				   const struct dw_xpcs_compat *compat)

The power-cycle case is also avoided in the driver, which keeps PD_PIPE
powered:

drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_serdes_phy_poweron() {
	ret = dev_pm_genpd_rpm_always_on(dev, true);

Adding optional resets later stays backward compatible, so this is only
a suggestion: is it intentional to leave the CRU reset lines out of the
binding?

[ ... ]

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