Re: [PATCH net-next v13 2/9] dt-bindings: net: lan9645x: add LAN9645X switch bindings

[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

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 binding makes rx/tx-internal-delay-ps on the MAC-side
  switch port nodes an on/off switch for the full 2 ns RGMII delay…
- [Low] The per-port pattern "^(ethernet-)?port@[0-8]$" in
  properties/ethernet-ports does not actually limit ports to the 9…
- [Low] The subject "dt-bindings: net: lan9645x: add LAN9645X switch
  bindings" does not follow…

--

dt-bindings: net: lan9645x: add LAN9645X switch bindings

This commit adds a DT binding for the Microchip LAN9645x DSA switch. The
smallest SKU, microchip,lan96455s-switch, is the fallback compatible. The
switch register targets are described through reg and reg-names, following
mscc,vsc7514-switch.yaml.

> dt-bindings: net: lan9645x: add LAN9645X switch bindings

[Severity: Low]
This isn't a bug, but the subject doesn't match what
Documentation/devicetree/bindings/submitting-patches.rst asks for. That file
says "Repeating "binding" again should also be avoided". Its example form is
"dt-bindings: iio: adc: Add ROHM BD79100G".

The file lives under net/dsa/, so the usual prefix is "dt-bindings: net: dsa:".
Something like "dt-bindings: net: dsa: Add Microchip LAN9645x switch" would
fit better.

> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
> new file mode 100644
> index 0000000000000..9deb7a427804f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml

[ ... ]

> +  ethernet-ports:
> +    type: object
> +    additionalProperties: true
> +    patternProperties:
> +      "^(ethernet-)?port@[0-8]$":

[Severity: Low]
Does this pattern actually restrict port nodes to the 9 hardware ports
(dev0..dev8)?

ethernet-ports has additionalProperties: true, so nodes that don't match
"^(ethernet-)?port@[0-8]$" are not rejected here. The inherited
dsa.yaml#/$defs/ethernet-ports and ethernet-switch.yaml both accept any
"^(ethernet-)?port@[0-9a-f]+$" node through dsa-port.yaml.

So a node like ethernet-port@9 { reg = <9>; ... } or ethernet-port@a passes
dt_binding_check and dtbs_check. It also skips the local RGMII delay
constraints. The problem only shows up at probe time:

net/dsa/dsa.c:dsa_switch_parse_ports_of() {
    ...
		if (reg >= ds->num_ports) {
			dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%u)\n",
				port, reg, ds->num_ports);
    ...
}

Should the schema reject port nodes outside 0-8?

> +        type: object
> +        description: Ethernet switch ports
> +        additionalProperties: true
> +
> +        allOf:
> +          - if:
> +              properties:
> +                phy-mode:
> +                  contains:
> +                    enum:
> +                      - rgmii
> +                      - rgmii-rxid
> +                      - rgmii-txid
> +                      - rgmii-id
> +            then:
> +              properties:
> +                rx-internal-delay-ps:
> +                  $ref: "#/$defs/internal-delay-ps"
> +                tx-internal-delay-ps:
> +                  $ref: "#/$defs/internal-delay-ps"

[ ... ]

> +$defs:
> +  internal-delay-ps:
> +    description:
> +      Disable the delay line using 0 ps, or enable the 2000 ps delay. The
> +      delay line is not tunable, so no other phase can be selected.
> +    enum: [0, 2000]
> +    default: 0

[Severity: Medium]
Is it intended that rx/tx-internal-delay-ps on the MAC side port nodes turns
the full 2 ns RGMII delay on and off, for all four rgmii phy-modes?

Documentation/devicetree/bindings/net/ethernet-controller.yaml says these
MAC node properties "should be used to indicate fine tuning performed by the
MAC. The values expected here are small. A value of 2000ps, i.e 2ns, and a
phy-mode of 'rgmii' will not be accepted by Reviewers."

The same document says that when the MAC adds the delay, it "must modify the
phy-mode it passes to the PHY, removing the delay it has added. Failure to
remove the delay will result in a non-functioning link."

This schema accepts phy-mode = "rgmii-id" together with
rx-internal-delay-ps = <2000>. In the driver later in this series,
lan9645x_parse_mac_delay() in lan9645x_port.c only checks for 0 or 2000.
lan9645x_rgmii_dll_config() in lan9645x_phylink.c then enables the delay line
directly from the parsed value:

drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c:lan9645x_rgmii_dll_config() {
    ...
	HSIO_DLL_CFG_DLL_ENA_SET(p->rx_internal_delay) |
	HSIO_DLL_CFG_DELAY_ENA_SET(p->rx_internal_delay),
    ...
}

Nothing there checks phy-mode or removes the delay from the interface that
phylink passes to the PHY.

Take port 7 or 8 with a phy-handle, phy-mode "rgmii-id" and
rx-internal-delay-ps = <2000>. Wouldn't the MAC and the PHY each add 2 ns,
leaving a DT that passes validation with a link that doesn't work?

The example also uses phy-mode = "rgmii" on the CPU port. For any RGMII mode
other than 'rgmii-id', ethernet-controller.yaml says it "is probably wrong"
unless justified.

> +
> +examples:

[ ... ]

> +            ethernet-port@7 {
> +                reg = <7>;
> +                ethernet = <&cpu_host_port>;
> +                phy-mode = "rgmii";

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com