Re: [PATCH net-next v13 2/9] dt-bindings: net: lan9645x: add LAN9645X switch bindings
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-02 21:14:18
Also in:
linux-devicetree, linux-renesas-soc, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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"
[ ... ]
quoted hunk ↗ jump to hunk
+$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:
[ ... ]
quoted hunk ↗ jump to hunk
+ 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