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

[Jens Emil Schulz Ostergaard]

On Mon, 2026-10-19 at 10:08 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?

Dismissed, intended.

This is the binding model of sja1105 and microchip,lan937x.yaml, which
also limits the values to 0 and 2000 ps on RGMII ports. sja1105 and ksz
configure the MAC delay lines from rx/tx-internal-delay-ps and pass
phy-mode unmodified to the PHY in the same way.



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