Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 Jan 2023 22:21:13 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, netdev@vger.kernel.org,
        Russell King <linux@armlinux.org.uk>,
        Linus Walleij <linus.walleij@linaro.org>,
        UNGLinuxDriver@microchip.com,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Lee Jones <lee@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v3 net-next 12/14] dt-bindings: net: dsa: ocelot: add
 ocelot-ext documentation
Message-ID: <20230119202113.lwya43hjvosjk77a@skbuf>
References: <455e31be-dc87-39b3-c7fe-22384959c556@linaro.org>
 <Yz2mSOXf68S16Xg/@colin-ia-desktop>
 <28b4d9f9-f41a-deca-aa61-26fb65dcc873@linaro.org>
 <20221008000014.vs2m3vei5la2r2nd@skbuf>
 <c9ce1d83-d1ca-4640-bba2-724e18e6e56b@linaro.org>
 <20221010130707.6z63hsl43ipd5run@skbuf>
 <d27d7740-bf35-b8d4-d68c-bb133513fa19@linaro.org>
 <20221010174856.nd3n4soxk7zbmcm7@skbuf>
 <Y8hylFFOw4n5RH83@MSI.localdomain>
 <Y8hylFFOw4n5RH83@MSI.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8hylFFOw4n5RH83@MSI.localdomain>
 <Y8hylFFOw4n5RH83@MSI.localdomain>
Precedence: bulk

Hi Colin,

On Wed, Jan 18, 2023 at 12:28:36PM -1000, Colin Foster wrote:
> Resurrecting this conversation for a quick question / feedback, now that
> steps 1-7 are essentially done with everyone's help.
> 
> I don't want to send out a full RFC / Patch, since I can't currently
> test on hardware this week. But I'd really like feedback on the
> documentation change that is coming up. And I also don't want to
> necessarily do a separate RFC for just this patch.
> 
> What happens here is interrupts and interrupt-names work as expected.
> They're required for the 7514, and optional for the 7512. Fantastic.
> 
> I'm not sure if the "$ref: ethernet-switch.yaml" and
> "$ref: /schemas/net/dsa/dsa.yaml#" have an effect, since removing that
> line outright doesn't seem to have an effect on dt_bindings_check.
> 
> The "fdma" doesn't make sense for the 7512, and seems to be handled
> correctly by way of maxItems for the two scenarios.
> 
> 
> The big miss in this patch is ethernet-switch-port vs dsa-port in the
> two scenarios. It isn't working as I'd hoped, where the 7514 pulls in
> ethernet-switch-port.yaml and the 7512 pulls in dsa-port.yaml. To squash
> errors about the incorrect "ethernet" property I switched this line:
> 
> -        $ref: ethernet-switch-port.yaml#
> +        $ref: /schemas/net/dsa/dsa-port.yaml#
> 
> ... knowing full well that the correct solution should be along the
> lines of "remove this, and only reference them in the conditional". That
> doesn't seem to work though...
> 
> Is what I'm trying to do possible? I utilized
> Documentation/devicetree/bindings/net/dsa/*.yaml and
> Documentation/devicetree/bindings/net/*.yaml and found examples to get
> to my current state.
> 
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> index 5ffe831e59e4..f012c64a0da3 100644
> --- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> +++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> @@ -18,13 +18,50 @@ description: |
>    packets using CPU. Additionally, PTP is supported as well as FDMA for faster
>    packet extraction/injection.
>  
> -$ref: ethernet-switch.yaml#
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: mscc,vsc7514-switch
> +    then:
> +      $ref: ethernet-switch.yaml#
> +      required:
> +        - interrupts
> +        - interrupt-names
> +      properties:
> +        reg:
> +          minItems: 21
> +        reg-names:
> +          minItems: 21
> +        ethernet-ports:
> +          patternProperties:
> +            "^port@[0-9a-f]+$":
> +              $ref: ethernet-switch-port.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          const: mscc,vsc7512-switch
> +    then:
> +      $ref: /schemas/net/dsa/dsa.yaml#
> +      properties:
> +        reg:
> +          maxItems: 20
> +        reg-names:
> +          maxItems: 20
> +        ethernet-ports:
> +          patternProperties:
> +            "^port@[0-9a-f]+$":
> +              $ref: /schemas/net/dsa/dsa-port.yaml#
>  
>  properties:
>    compatible:
> -    const: mscc,vsc7514-switch
> +    enum:
> +      - mscc,vsc7512-switch
> +      - mscc,vsc7514-switch
>  
>    reg:
> +    minItems: 20
>      items:
>        - description: system target
>        - description: rewriter target
> @@ -49,6 +86,7 @@ properties:
>        - description: fdma target
>  
>    reg-names:
> +    minItems: 20
>      items:
>        - const: sys
>        - const: rew
> @@ -100,7 +138,7 @@ properties:
>      patternProperties:
>        "^port@[0-9a-f]+$":
>  
> -        $ref: ethernet-switch-port.yaml#
> +        $ref: /schemas/net/dsa/dsa-port.yaml#
>  
>          unevaluatedProperties: false

I'm not sure at all why this chunk (is sub-schema the right word) even
exists, considering you have the other one?!

>  
> @@ -108,13 +146,12 @@ required:
>    - compatible
>    - reg
>    - reg-names
> -  - interrupts
> -  - interrupt-names
>    - ethernet-ports
>  
>  additionalProperties: false

This should be "unevaluatedProperties: false" I guess? Maybe this is why
deleting the ethernet-switch.yaml or dsa.yaml schema appears to do nothing?

The following delta compared to net-next works for me, I think:

diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
index 5ffe831e59e4..dc3319ea40b9 100644
--- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
@@ -18,13 +18,52 @@ description: |
   packets using CPU. Additionally, PTP is supported as well as FDMA for faster
   packet extraction/injection.
 
-$ref: ethernet-switch.yaml#
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: mscc,vsc7514-switch
+    then:
+      $ref: ethernet-switch.yaml#
+      required:
+        - interrupts
+        - interrupt-names
+      properties:
+        reg:
+          minItems: 21
+        reg-names:
+          minItems: 21
+        ethernet-ports:
+          patternProperties:
+            "^port@[0-9a-f]+$":
+              $ref: ethernet-switch-port.yaml#
+              unevaluatedProperties: false
+
+  - if:
+      properties:
+        compatible:
+          const: mscc,vsc7512-switch
+    then:
+      $ref: /schemas/net/dsa/dsa.yaml#
+      properties:
+        reg:
+          maxItems: 20
+        reg-names:
+          maxItems: 20
+        ethernet-ports:
+          patternProperties:
+            "^port@[0-9a-f]+$":
+              $ref: /schemas/net/dsa/dsa-port.yaml#
+              unevaluatedProperties: false
 
 properties:
   compatible:
-    const: mscc,vsc7514-switch
+    enum:
+      - mscc,vsc7512-switch
+      - mscc,vsc7514-switch
 
   reg:
+    minItems: 20
     items:
       - description: system target
       - description: rewriter target
@@ -49,6 +88,7 @@ properties:
       - description: fdma target
 
   reg-names:
+    minItems: 20
     items:
       - const: sys
       - const: rew
@@ -86,35 +126,16 @@ properties:
       - const: xtr
       - const: fdma
 
-  ethernet-ports:
-    type: object
-
-    properties:
-      '#address-cells':
-        const: 1
-      '#size-cells':
-        const: 0
-
-    additionalProperties: false
-
-    patternProperties:
-      "^port@[0-9a-f]+$":
-
-        $ref: ethernet-switch-port.yaml#
-
-        unevaluatedProperties: false
-
 required:
   - compatible
   - reg
   - reg-names
-  - interrupts
-  - interrupt-names
   - ethernet-ports
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
+  # VSC7514 (Switchdev)
   - |
     switch@1010000 {
       compatible = "mscc,vsc7514-switch";
@@ -154,6 +175,7 @@ examples:
           reg = <0>;
           phy-handle = <&phy0>;
           phy-mode = "internal";
+          ethernet = <&mac_sw>; # fails validation as expected
         };
         port1: port@1 {
           reg = <1>;
@@ -162,5 +184,51 @@ examples:
         };
       };
     };
+  # VSC7512 (DSA)
+  - |
+    ethernet-switch@1{
+      compatible = "mscc,vsc7512-switch";
+      reg = <0x71010000 0x10000>,
+            <0x71030000 0x10000>,
+            <0x71080000 0x100>,
+            <0x710e0000 0x10000>,
+            <0x711e0000 0x100>,
+            <0x711f0000 0x100>,
+            <0x71200000 0x100>,
+            <0x71210000 0x100>,
+            <0x71220000 0x100>,
+            <0x71230000 0x100>,
+            <0x71240000 0x100>,
+            <0x71250000 0x100>,
+            <0x71260000 0x100>,
+            <0x71270000 0x100>,
+            <0x71280000 0x100>,
+            <0x71800000 0x80000>,
+            <0x71880000 0x10000>,
+            <0x71040000 0x10000>,
+            <0x71050000 0x10000>,
+            <0x71060000 0x10000>;
+            reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
+            "port2", "port3", "port4", "port5", "port6",
+            "port7", "port8", "port9", "port10", "qsys",
+            "ana", "s0", "s1", "s2";
+
+            ethernet-ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+          port@0 {
+            reg = <0>;
+            ethernet = <&mac_sw>;
+            phy-handle = <&phy0>;
+            phy-mode = "internal";
+          };
+          port@1 {
+            reg = <1>;
+            phy-handle = <&phy1>;
+            phy-mode = "internal";
+          };
+        };
+      };
 
 ...

Of course this is a completely uneducated attempt on my part.