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

[Jens Emil Schulz Ostergaard]

This is my reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=2

> --- Patch [2]: [PATCH net-next v11 2/9] dt-bindings: net: lan9645x: add LAN9645X switch bindings ---
> commit 1b95e77c1cab7610f3d242846549a69514641335
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@xxxxxxxxxxxxx>
>
> dt-bindings: net: lan9645x: add LAN9645X switch bindings
>
> This adds a new DSA binding for the Microchip LAN9645x switch family,
> using the smallest SKU (microchip,lan96455s-switch) as the fallback
> compatible, and adds the file to the existing MAINTAINERS entry.
>
> > 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..043fb48922b41
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
> > @@ -0,0 +1,111 @@
>
> [ ... ]
>
> > +  ethernet-ports:
> > +    type: object
> > +    patternProperties:
> > +      "^ethernet-port@[0-8]$":
>
> Discovered by: opus-5 · Missed by: sonnet-5, sashiko-gemini · Confirmed by: sonnet-5
> Are the constraints under this pattern actually reachable for all legal
> port node names?
>
> The top-level $ref pulls in dsa.yaml#/$defs/ethernet-ports, which accepts:
>
>   patternProperties:
>     "^(ethernet-)?ports$":
>       patternProperties:
>         "^(ethernet-)?port@[0-9a-f]+$":
>           description: Ethernet switch ports
>           $ref: dsa-port.yaml#
>           unevaluatedProperties: false
>
> So both port@N and ethernet-port@N are valid spellings, and indices are
> not limited to 0-8. The local ethernet-ports object here has no
> additionalProperties or unevaluatedProperties of its own, so children whose
> names do not match "^ethernet-port@[0-8]$" are simply not checked against
> the local constraints.
>
> That means these two both pass dt_binding_check:
>
>   ethernet-ports {
>       port@7 {
>           phy-mode = "rgmii";
>           rx-internal-delay-ps = <500>;   /* const: 2000 not applied */
>       };
>   };
>
>   ethernet-ports {
>       ethernet-port@9 {                   /* index > 8 not rejected */
>           reg = <9>;
>       };
>   };
>
> Would using the "^(ethernet-)?port@[0-8]$" spelling here, as
> Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.yaml does with
> "^(ethernet-)?port@6$", make the new per-port constraints effective?
>

Yes, I will update the regex.

> > +        type: object
> > +        description: Ethernet switch ports
> > +
> > +        $ref: dsa-port.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          rx-internal-delay-ps:
> > +            const: 2000
> > +
> > +          tx-internal-delay-ps:
> > +            const: 2000
>
> Discovered by: opus-5, sashiko-gemini · Missed by: sonnet-5 · Confirmed by: sonnet-5
> Should these two properties be guarded by an rgmii phy-mode conditional?
>
> As written they are declared for every port index 0-8 with no phy-mode
> condition, and since they live in the same schema object that carries
> unevaluatedProperties: false, they count as evaluated for any phy-mode.
> This passes validation:
>
>   ethernet-port@0 {
>       reg = <0>;
>       phy-mode = "gmii";
>       rx-internal-delay-ps = <2000>;
>   };
>
> Per the driver, only ports 7 and 8 have RGMII pins
> (lan9645x_phylink_get_caps(): 0..4 GMII/cuphy, 5..6 SerDes, 7..8 RGMII),
> and only lan9645x_rgmii_dll_config() programs the delay lines, with
> RGMII_IDX(port) defined for ports 4/7/8 only. On a gmii port the value is
> stored in p->rx_internal_delay by lan9645x_port_setup() and then never
> used, with no diagnostic.
>
> The existing pattern is a conditional, see
> Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.yaml:
>
>   "^(ethernet-)?port@6$":
>     allOf:
>       - if:
>           properties:
>             phy-mode:
>               contains:
>                 enum:
>                   - rgmii
>         then:
>           properties:
>             rx-internal-delay-ps: ...
>
> Documentation/devicetree/bindings/net/ethernet-controller.yaml also defines
> these properties only inside an rgmii* phy-mode conditional.

Yes, I will a conditional here.

>
> Separately, ethernet-controller.yaml states:
>
>   A value of 2000ps, i.e 2ns, and a phy-mode of "rgmii" will not be
>   accepted by Reviewers
>
> which is the combination this binding mandates with const: 2000 and shows
> in the example below. Is a phy-mode of "rgmii" together with a 2000 ps
> internal delay intended here, rather than one of the rgmii-rxid /
> rgmii-txid / rgmii-id modes?

I think ethernet-controller.yaml talks about the situation where there is a PHY
present. The intent here is a mac to mac connection. I was specifically
instructed to model it like this, and not do use rgmii-*id as a way to
communicate delay programming of the MAC. It is also clear from
Documentation/networking/phy.rst
that this is the right way to model mac to mac.

>
> > +
> > +$ref: dsa.yaml#/$defs/ethernet-ports
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - ethernet-ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    ethernet-switch@4000 {
> > +        compatible = "microchip,lan96459f-switch", "microchip,lan96455s-switch";
> > +        reg = <0x4000 0x244>;
> > +
> > +        ethernet-ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            ethernet-port@0 {
> > +                reg = <0>;
> > +                phy-mode = "gmii";
> > +                phy-handle = <&cuphy0>;
> > +            };
>
> [ ... ]
>
> > +
> > +            ethernet-port@7 {
> > +                reg = <7>;
> > +                phy-mode = "rgmii";
> > +                ethernet = <&cpu_host_port>;
> > +                rx-internal-delay-ps = <2000>;
> > +                tx-internal-delay-ps = <2000>;
> > +
> > +                fixed-link {
> > +                    speed = <1000>;
> > +                    full-duplex;
> > +                    pause;
> > +                };
> > +            };
> > +        };
> > +    };
> > +...
> > +
>       ^^^
>
> Discovered by: opus-5 · Missed by: sonnet-5, sashiko-gemini · Confirmed by: sonnet-5
> This isn't a bug, but there is a blank line after the end-of-document
> marker at the end of the file, which git diff --check reports as "new blank
> line at EOF". The other bindings in this directory end with "..." and
> nothing after it.

I will remove this.

>
> Cross-instance finding from sashiko-gemini
(008974e58f4042f8866239fc681c104931d4e9cd342a5b5be37f42504a115cf0):
> Discovered by: sashiko-gemini, opus-5 · Missed by: sonnet-5 · Confirmed by: opus-5
> The schema lacks an `mdio` node definition while setting `unevaluatedProperties: false` at the root,
which inadvertently prevents instantiation of the integrated PHYs referenced in the example.