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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Rob Herring's Reviewed-by is kept on the binding patch in Documen
  tation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml,…
- [Low] The schema does not actually limit port indexes to 0-8.
- [Low] The binding seems to leave out the register-based extraction
  interrupt.

--

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

This adds a DT binding for the Microchip LAN9645x DSA switch family. The
SKU variants fall back to microchip,lan96455s-switch. The binding
describes 24 register targets through reg/reg-names, three interrupts and
the per-port RGMII internal delay constraints, and adds the file to
MAINTAINERS.

>     Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
>     Signed-off-by: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>     Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>
> ---
> Changes in v14:

[ ... ]

> - Describe all switch register targets in reg, and the analyzer and
>   PTP interrupts.

[ ... ]

> Changes in v12:
> - Remove Rob's Reviewed-by

[ ... ]

> - Describe the switch register targets in reg, with a matching reg-names,
>   following mscc,vsc7514-switch.yaml.

[Severity: Low]
Does Rob's Reviewed-by still apply to this version?

In v14, reg/reg-names becomes a fixed, ordered list of 24 targets. v14
also adds new interrupts and interrupt-names properties ("ptp", "ptp-ext",
"ana"). All of this is new DT ABI.

The v12 changelog dropped this tag for a similar reg/reg-names change.
Here the tag sits after the Signed-off-by, which suggests it was carried
over from an earlier posting.

Did Rob review v14? If not, should the tag be dropped so that the new
reg-names order and interrupt-names get DT maintainer review?

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

[ ... ]

> +  reg:
> +    items:
> +      - description: General configuration block target
> +      - description:
> +          CPU device queue system target, the register based frame injection
> +          and extraction interface

[ ... ]

> +  interrupts:
> +    items:
> +      - description: PTP timestamp ready interrupt
> +      - description: PTP synchronization interrupt from the PTP pins
> +      - description: Analyzer interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: ptp
> +      - const: ptp-ext
> +      - const: ana

[Severity: Low]
Is an interrupt for register-based extraction missing here?

The qs entry in reg is described as "the register based frame injection
and extraction interface". However, interrupts lists only ptp, ptp-ext and
ana, and there is no minItems.

The other bindings for this switch IP family all describe an xtr interrupt
next to the QS block:

  mscc,vsc7514-switch.yaml:      ptp_rdy, xtr, fdma
  microchip,lan966x-switch.yaml: xtr, fdma, ana, ptp, ptp-ext
  microchip,sparx5-switch.yaml:  xtr, fdma, ptp

The commit message names mscc,vsc7514-switch.yaml as the model. The driver
in this series also sets up manual extraction through QS_XTR_GRP_CFG(0) in
lan9645x_port_cpu_init().

If LAN9645x routes an xtr interrupt, how would this binding describe it?
Since the list is fixed, adding it later would need a fourth item plus
minItems: 3. I could not tell from the tree whether the hardware has a
separate xtr line, so this may be fine as it is.

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

[ ... ]

> +$ref: dsa.yaml#/$defs/ethernet-ports

[Severity: Low]
Does this actually limit port indexes to 0-8?

ethernet-ports has additionalProperties: true, so the local pattern only
constrains nodes that match it. The top-level $ref adds the broader
pattern from dsa.yaml:

Documentation/devicetree/bindings/net/dsa/dsa.yaml:
          "^(ethernet-)?port@[0-9a-f]+$":
            description: Ethernet switch ports
            $ref: dsa-port.yaml#
            unevaluatedProperties: false

So nodes such as ethernet-port@9 or ethernet-port@10 pass
dt_binding_check. They also skip the local RGMII rule that limits
rx/tx-internal-delay-ps to [0, 2000].

A DT like that only fails at probe time, in dsa_switch_parse_ports_of():

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);
			of_node_put(port);
			err = -EINVAL;
			goto out_put_node;
    ...
}

renesas,rzn1-a5psw.yaml, which this binding follows, has the same pattern.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-dsa_lan9645x_switch_driver_base-v17-0-83a471c5bddb%40microchip.com