Re: [PATCH net-next v11 2/6] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] 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 device tree binding for the Synopsys DesignWare XPCS on
the Rockchip RK3568 SoC. The XPCS is accessed over APB3 and connected to a
Naneng Combo SerDes PHY. The binding covers reg, the csr and eee clocks,
one phy and one power domain, and on purpose leaves out the CRU reset
lines.

> 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..a331880eb11c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> @@ -0,0 +1,69 @@

[ ... ]

> +  phys:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - phys
> +  - power-domains
> +
> +additionalProperties: false

[Severity: Medium]
The properties list has no resets or interrupts entries, and
additionalProperties is false. Would a DT that fully describes this block
then fail dt_binding_check?

The SoC has four CRU reset inputs for this IP, defined 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

The commit message says they are left out because "no in-tree user
requests them". Documentation/devicetree/bindings/writing-bindings.rst
says the opposite:

  - DO attempt to make bindings complete even if a driver doesn't support some
    features. For example, if a device has an interrupt, then include the
    'interrupts' property even if the driver is only polled mode.

  - DON'T refer to Linux or "device driver" in bindings. Bindings should be
    based on what the hardware has, not what an OS and driver currently support.

Could resets and reset-names be added now, so the binding matches the
hardware?

The commit message also says the resets "can be added later as optional
without breaking ABI". Once DTs ship without them, though, they would have
to stay optional for good.

Could a driver then ever rely on putting the TX/RX div10 and XGXS RX
domains into a known state? The in-IP soft reset may not cover those
domains.

The same question applies to interrupts. The generic binding for this IP,
snps,dw-xpcs.yaml, documents an auto-negotiation interrupt output:

  interrupts:
    description:
      System interface interrupt output (sbd_intr_o) indicating Clause 73/37
      auto-negotiation events':' Page received, AN is completed or incompatible
      link partner.
    maxItems: 1

Is sbd_intr_o routed to the GIC on RK3568? If it is, should this binding
have an interrupts property too? If it isn't, could the commit message say
so?

The later patches in this series leave this binding unchanged, so neither
point is addressed later in the series.

> +
> +examples:

[ ... ]

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