Re: [PATCH net-next v10 1/6] 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] The new binding
  Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml…

--

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

This adds a new binding for the Synopsys DesignWare XPCS on the Rockchip
RK3568 SoC. The XPCS sits on the APB3 bus, connects to a Naneng Combo
SerDes PHY, and exposes four MII ports as ethernet-pcs-mii@N child nodes.
The commit message says the CRU reset lines are left out on purpose.

> 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

[ ... ]

> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: serdes
> +
> +  power-domains:
> +    maxItems: 1
> +

[ ... ]

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

[Severity: Low]
Should this binding describe the XPCS reset inputs? The CRU binding
header already has four reset IDs for this block:

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

The commit message gives this reason for leaving them out:

  The CRU reset lines (SRST_XPCS*) are intentionally not described: no
  in-tree user requests them, and bring-up relies on the PD_PIPE power
  domain, the SerDes PHY and the in-IP soft reset.

That reason is based on what drivers use today, not on what the hardware
has. Documentation/devicetree/bindings/writing-bindings.rst says:

  DO attempt to make bindings complete even if a driver doesn't support
  some features.

The schema has no resets or reset-names property and ends with
additionalProperties: false. A DT that fully describes the hardware with
something like resets = <&cru SRST_XPCS>, ... would then fail
dt_binding_check and dtbs_check.

The in-IP soft reset also only acts inside the PCS register block. It
does not appear to cover the TX/RX div10 or XGXS RX reset domains.

Could resets and reset-names be added now, even as optional properties?

On a related point that I have not been able to confirm: the generic
snps,dw-xpcs binding documents an sbd_intr_o interrupts output, and
additionalProperties: false also rejects that here. Does RK3568 route
that interrupt to the GIC? If it does, should interrupts be listed too?

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