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

[Jens Emil Schulz Ostergaard]

On Fri, 2026-10-02 at 21:14 +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 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.
> 

I will retitle.

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

Dismissed. This follows the binding model introduced for sja1105 in
commit ac41ac81e331 ("dt-bindings: net: dsa: sja1105: add
{rx,tx}-internal-delay-ps") and used by microchip,lan937x.yaml since
commit 8926d94e5c50, which also restricts the values to 0 and 2000 ps.
The MAC delay lines are configured only from rx/tx-internal-delay-ps,
independently of phy-mode and of whether the port uses a PHY or a
fixed-link, and phy-mode is passed unmodified to the PHY. The hardware
has a fixed 2 ns delay line, 0 and 2000 are the only values it can
express. For the CPU port, phy-mode "rgmii" with a fixed-link matches
the sja1105 and lan937x examples.

> > +
> > +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