Re: [PATCH net-next v15 2/9] dt-bindings: net: dsa: add Microchip LAN9645x switch

[netdev-bot+sashiko@kernel.org]

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

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

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 (microchip,lan96455s-switch.yaml) and its RGMII
  delay design contradict the in-tree RGMII contract in ethernet-…
- [Low] The binding (microchip,lan96455s-switch.yaml) does not reject
  port nodes beyond port 8, and those nodes escape its RGMII delay…

--

dt-bindings: net: dsa: add Microchip LAN9645x switch

This adds a YAML binding for the Microchip LAN9645x DSA switch family,
with microchip,lan96455s-switch as the fallback compatible. The switch
registers are described with one reg entry per register target. RGMII
ports may set a fixed 0 or 2000 ps MAC internal delay.

> 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..517bc4c00de30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
> @@ -0,0 +1,208 @@

[ ... ]

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

[Severity: Low]
Can port nodes above port 8 get past this schema?

The local pattern only matches port@0 through port@8. However,
ethernet-ports has additionalProperties: true, and the inherited
dsa.yaml#/$defs/ethernet-ports checks "^(ethernet-)?port@[0-9a-f]+$"
against dsa-port.yaml. That means port@9 through port@f are evaluated
and pass.

Those nodes also skip the local RGMII if/then. For example, port@9 with
phy-mode = "rgmii" and rx-internal-delay-ps = <5000> passes, because
ethernet-controller.yaml only types that property as uint32.

The hardware has only 9 ports, so these nodes are rejected only 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",
    ...
}

The description says the last SKU digit gives 5, 7 or 9 usable front
ports. No compatible-based if block limits which port@N nodes the 5-port
and 7-port SKUs may declare, so a bare microchip,lan96455s-switch node can
describe all nine ports.

renesas,rzn1-a5psw.yaml, which the changelog names as the model, has the
same pattern.

Should the schema reject port nodes outside the range each SKU supports?

> +        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]
Does this delay definition conflict with the RGMII rules in
ethernet-controller.yaml?

The if/then above applies this 0/2000 ps enum to all four RGMII modes.
That means the schema accepts phy-mode = "rgmii" together with
tx-internal-delay-ps = <2000>. The informative section of
ethernet-controller.yaml says:

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

That section also says delays not added by the PCB are software
configuration and are not described in DT. If the MAC adds the delay, it
has to remove that delay from the phy-mode it passes to the PHY.

The schema also accepts rgmii-id, rgmii-rxid or rgmii-txid with a 2000 ps
MAC delay. In the driver added later in the series ("net: dsa: lan9645x:
add basic dsa driver for LAN9645X"), nothing seems to remove that delay
again. lan9645x_parse_mac_delay() in lan9645x_port.c sets the flag only
from the DT property:

	*out = val == 2000;

lan9645x_phylink_mac_config() in lan9645x_phylink.c then calls
lan9645x_rgmii_dll_config() for any RGMII mode. That function enables the
DLL delay whenever the flag is set:

	if (phy_interface_mode_is_rgmii(state->interface))
		lan9645x_rgmii_dll_config(p);

Meanwhile phylink passes the unmodified phy-mode to the PHY.

Take port 7 or 8 with an external PHY, phy-mode = "rgmii-id" and
tx-internal-delay-ps = <2000>. Both the MAC and the PHY would add 2 ns,
for about 4 ns of skew. Wouldn't that give a non-working link, even
though the DT passes dt_binding_check?

The if also has no else branch. Non-RGMII ports (gmii, rmii, BASE-X,
QSGMII) therefore accept any uint32 value for rx-internal-delay-ps and
tx-internal-delay-ps.

Could the MAC delay be derived from phy-mode instead of these properties,
with the phy-mode passed to the PHY adjusted to match?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-dsa_lan9645x_switch_driver_base-v15-0-562f316f78b9%40microchip.com