[PATCH net-next 0/4] Document internal MDIO bus of DSA switch and support it on MT7530

[PATCH net-next 0/4] Document internal MDIO bus of DSA switch and support it on MT7530

[Arınç ÜNAL]

Hello there.

This patch series documents the internal MDIO bus of the switches
controlled by DSA and brings support for it on the MT7530 DSA subdriver.

There are also some dt-bindings improvements included.

Cheers.
Arınç

[PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Arınç ÜNAL]

The port@5 node on the example is missing the ethernet property. Add it.
Remove the MAC bindings on the example as they cannot be validated.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 .../bindings/net/dsa/microchip,lan937x.yaml           | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 8d7e878b84dc..49af4b0d5916 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -68,16 +68,6 @@ examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
 
-    macb0 {
-            #address-cells = <1>;
-            #size-cells = <0>;
-
-            fixed-link {
-                    speed = <1000>;
-                    full-duplex;
-            };
-    };
-
     spi {
             #address-cells = <1>;
             #size-cells = <0>;
@@ -138,6 +128,7 @@ examples:
                                     phy-mode = "rgmii";
                                     tx-internal-delay-ps = <2000>;
                                     rx-internal-delay-ps = <2000>;
+                                    ethernet = <&macb1>;
 
                                     fixed-link {
                                             speed = <1000>;
-- 
2.39.2

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Vladimir Oltean]

On Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  .../bindings/net/dsa/microchip,lan937x.yaml           | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> index 8d7e878b84dc..49af4b0d5916 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> @@ -68,16 +68,6 @@ examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
>  
> -    macb0 {
> -            #address-cells = <1>;
> -            #size-cells = <0>;
> -
> -            fixed-link {
> -                    speed = <1000>;
> -                    full-duplex;
> -            };
> -    };
> -
>      spi {
>              #address-cells = <1>;
>              #size-cells = <0>;
> @@ -138,6 +128,7 @@ examples:
>                                      phy-mode = "rgmii";
>                                      tx-internal-delay-ps = <2000>;
>                                      rx-internal-delay-ps = <2000>;
> +                                    ethernet = <&macb1>;

macb1 instead of macb0: was it intentional?

>  
>                                      fixed-link {
>                                              speed = <1000>;
> -- 
> 2.39.2
> 

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Arınç ÜNAL]

On 13.08.2023 14:07, Vladimir Oltean wrote:
> On Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
>> The port@5 node on the example is missing the ethernet property. Add it.
>> Remove the MAC bindings on the example as they cannot be validated.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   .../bindings/net/dsa/microchip,lan937x.yaml           | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> index 8d7e878b84dc..49af4b0d5916 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> @@ -68,16 +68,6 @@ examples:
>>     - |
>>       #include <dt-bindings/gpio/gpio.h>
>>   
>> -    macb0 {
>> -            #address-cells = <1>;
>> -            #size-cells = <0>;
>> -
>> -            fixed-link {
>> -                    speed = <1000>;
>> -                    full-duplex;
>> -            };
>> -    };
>> -
>>       spi {
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>> @@ -138,6 +128,7 @@ examples:
>>                                       phy-mode = "rgmii";
>>                                       tx-internal-delay-ps = <2000>;
>>                                       rx-internal-delay-ps = <2000>;
>> +                                    ethernet = <&macb1>;
> 
> macb1 instead of macb0: was it intentional?

Yes, port@4 defines macb0. I used macb1 for port@5 here.

Arınç

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Vladimir Oltean]

On Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  .../bindings/net/dsa/microchip,lan937x.yaml           | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> index 8d7e878b84dc..49af4b0d5916 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> @@ -68,16 +68,6 @@ examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
>  
> -    macb0 {
> -            #address-cells = <1>;
> -            #size-cells = <0>;
> -
> -            fixed-link {
> -                    speed = <1000>;
> -                    full-duplex;
> -            };
> -    };
> -
>      spi {
>              #address-cells = <1>;
>              #size-cells = <0>;
> @@ -138,6 +128,7 @@ examples:
>                                      phy-mode = "rgmii";
>                                      tx-internal-delay-ps = <2000>;
>                                      rx-internal-delay-ps = <2000>;
> +                                    ethernet = <&macb1>;
>  
>                                      fixed-link {
>                                              speed = <1000>;
> -- 
> 2.39.2
> 

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Florian Fainelli]

On 8/12/23 02:17, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip,lan937x: add missing ethernet on example

[Rob Herring]

On Sat, 12 Aug 2023 12:17:05 +0300, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  .../bindings/net/dsa/microchip,lan937x.yaml           | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

Add the schema to document the internal MDIO bus. Adjust realtek.yaml
accordingly.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 .../devicetree/bindings/net/dsa/dsa.yaml      | 18 +++++
 .../devicetree/bindings/net/dsa/realtek.yaml  | 66 +++++++++----------
 2 files changed, 50 insertions(+), 34 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660beda..03ccedbc49dc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
       (single device hanging off a CPU port) must not specify this property
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
+  mdio:
+    description: The internal MDIO bus of the switch
+    $ref: /schemas/net/mdio.yaml#
+
+if:
+  required: [ mdio ]
+then:
+  patternProperties:
+    "^(ethernet-)?ports$":
+      patternProperties:
+        "^(ethernet-)?port@[0-9]+$":
+          if:
+            not:
+              required: [ ethernet ]
+          then:
+            required:
+              - phy-handle
+
 additionalProperties: true
 
 $defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..ea7db0890abc 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Realtek switches for unmanaged switches
 
-allOf:
-  - $ref: dsa.yaml#/$defs/ethernet-ports
-
 maintainers:
   - Linus Walleij <linus.walleij@linaro.org>
 
@@ -95,37 +92,38 @@ properties:
       - '#address-cells'
       - '#interrupt-cells'
 
-  mdio:
-    $ref: /schemas/net/mdio.yaml#
-    unevaluatedProperties: false
-
-    properties:
-      compatible:
-        const: realtek,smi-mdio
-
-if:
-  required:
-    - reg
-
-then:
-  $ref: /schemas/spi/spi-peripheral-props.yaml#
-  not:
-    required:
-      - mdc-gpios
-      - mdio-gpios
-      - mdio
-
-  properties:
-    mdc-gpios: false
-    mdio-gpios: false
-    mdio: false
-
-else:
-  required:
-    - mdc-gpios
-    - mdio-gpios
-    - mdio
-    - reset-gpios
+allOf:
+  - $ref: dsa.yaml#/$defs/ethernet-ports
+  - if:
+      required: [ mdio ]
+    then:
+      properties:
+        mdio:
+          properties:
+            compatible:
+              const: realtek,smi-mdio
+
+  - if:
+      required:
+        - reg
+    then:
+      $ref: /schemas/spi/spi-peripheral-props.yaml#
+      not:
+        required:
+          - mdc-gpios
+          - mdio-gpios
+          - mdio
+
+      properties:
+        mdc-gpios: false
+        mdio-gpios: false
+        mdio: false
+    else:
+      required:
+        - mdc-gpios
+        - mdio-gpios
+        - mdio
+        - reset-gpios
 
 required:
   - compatible
-- 
2.39.2

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

I changed this to below. I will wait for reviews before submitting v2.

The realtek.yaml schema extends the mdio.yaml schema. The mdio.yaml schema
is also being referred to through dsa.yaml#/$defs/ethernet-ports now which
means we cannot disallow additional properties by 'unevaluatedProperties:
false' on the dsa.yaml schema.

On the realtek.yaml schema, refer to dsa.yaml#/properties/mdio instead to
point the human readers to the description on the dsa.yaml schema.

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660beda..03ccedbc49dc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
        (single device hanging off a CPU port) must not specify this property
      $ref: /schemas/types.yaml#/definitions/uint32-array
  
+  mdio:
+    description: The internal MDIO bus of the switch
+    $ref: /schemas/net/mdio.yaml#
+
+if:
+  required: [ mdio ]
+then:
+  patternProperties:
+    "^(ethernet-)?ports$":
+      patternProperties:
+        "^(ethernet-)?port@[0-9]+$":
+          if:
+            not:
+              required: [ ethernet ]
+          then:
+            required:
+              - phy-handle
+
  additionalProperties: true
  
  $defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..f4b4fe0509a0 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -96,7 +96,7 @@ properties:
        - '#interrupt-cells'
  
    mdio:
-    $ref: /schemas/net/mdio.yaml#
+    $ref: dsa.yaml#/properties/mdio
      unevaluatedProperties: false
  
      properties:

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

I've realised there are more schemas that extend the mdio.yaml schema. This
is the final state of this patch.

dt-bindings: net: dsa: document internal MDIO bus

Add the schema to document the internal MDIO bus. Require the phy-handle
property on the non-CPU ports if the mdio property is being used.

Define the mdio property on all of the schemas that refer to
dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point
the human readers to the description on the dsa.yaml schema.

Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is
also being referred to through dsa.yaml#/$defs/ethernet-ports now which
means we cannot disallow additional properties by 'unevaluatedProperties:
false' on the dsa.yaml schema.

---
  .../bindings/net/dsa/arrow,xrs700x.yaml        |  4 ++++
  .../devicetree/bindings/net/dsa/brcm,b53.yaml  |  4 ++++
  .../devicetree/bindings/net/dsa/brcm,sf2.yaml  |  4 ++++
  .../devicetree/bindings/net/dsa/dsa.yaml       | 18 ++++++++++++++++++
  .../bindings/net/dsa/hirschmann,hellcreek.yaml |  4 ++++
  .../bindings/net/dsa/mediatek,mt7530.yaml      |  4 ++++
  .../bindings/net/dsa/microchip,ksz.yaml        |  4 ++++
  .../bindings/net/dsa/microchip,lan937x.yaml    |  2 +-
  .../bindings/net/dsa/mscc,ocelot.yaml          |  4 ++++
  .../bindings/net/dsa/nxp,sja1105.yaml          |  4 ++++
  .../devicetree/bindings/net/dsa/qca8k.yaml     |  2 +-
  .../devicetree/bindings/net/dsa/realtek.yaml   |  2 +-
  .../bindings/net/dsa/renesas,rzn1-a5psw.yaml   |  2 +-
  13 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
index 9565a740214629..f0229352e05694 100644
--- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
@@ -29,6 +29,10 @@ properties:
    reg:
      maxItems: 1
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - compatible
    - reg
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
index 4c78c546343f5e..e14562b33bfb97 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
@@ -65,6 +65,10 @@ properties:
                - brcm,bcm63268-switch
            - const: brcm,bcm63xx-switch
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - compatible
    - reg
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
index c745407f2f6853..1bf4317e038687 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
@@ -90,6 +90,10 @@ properties:
                tags enabled (per-packet metadata)
              type: boolean
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - reg
    - interrupts
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660bedaed..03ccedbc49dcc3 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
        (single device hanging off a CPU port) must not specify this property
      $ref: /schemas/types.yaml#/definitions/uint32-array
  
+  mdio:
+    description: The internal MDIO bus of the switch
+    $ref: /schemas/net/mdio.yaml#
+
+if:
+  required: [ mdio ]
+then:
+  patternProperties:
+    "^(ethernet-)?ports$":
+      patternProperties:
+        "^(ethernet-)?port@[0-9]+$":
+          if:
+            not:
+              required: [ ethernet ]
+          then:
+            required:
+              - phy-handle
+
  additionalProperties: true
  
  $defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
index 4021b054f68446..32f17345825d4a 100644
--- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
@@ -67,6 +67,10 @@ properties:
  
      additionalProperties: false
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - compatible
    - reg
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index e532c6b795f4fc..293d1affe75451 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -151,6 +151,10 @@ properties:
        ethsys.
      maxItems: 1
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  patternProperties:
    "^(ethernet-)?ports$":
      type: object
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index e51be1ac036237..01d11c642ecfd4 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -49,6 +49,10 @@ properties:
        Set if the output SYNCLKO clock should be disabled. Do not mix with
        microchip,synclko-125.
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - compatible
    - reg
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 49af4b0d591695..15f24a1716cd44 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -32,7 +32,7 @@ properties:
      maxItems: 1
  
    mdio:
-    $ref: /schemas/net/mdio.yaml#
+    $ref: dsa.yaml#/properties/mdio
      unevaluatedProperties: false
  
  patternProperties:
diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
index fe02d05196e4a6..d781b8c2324836 100644
--- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
@@ -73,6 +73,10 @@ properties:
    little-endian: true
    big-endian: true
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  required:
    - compatible
    - reg
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 4d5f5cc6d031e2..82dda8fae8b16e 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -72,6 +72,10 @@ properties:
            - compatible
            - reg
  
+  mdio:
+    $ref: dsa.yaml#/properties/mdio
+    unevaluatedProperties: false
+
  patternProperties:
    "^(ethernet-)?ports$":
      patternProperties:
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index df64eebebe1856..001b72bcd0746b 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -60,7 +60,7 @@ properties:
        B68 on the QCA832x and B49 on the QCA833x.
  
    mdio:
-    $ref: /schemas/net/mdio.yaml#
+    $ref: dsa.yaml#/properties/mdio
      unevaluatedProperties: false
      description: Qca8k switch have an internal mdio to access switch port.
                   If this is not present, the legacy mapping is used and the
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea39..f4b4fe0509a022 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -96,7 +96,7 @@ properties:
        - '#interrupt-cells'
  
    mdio:
-    $ref: /schemas/net/mdio.yaml#
+    $ref: dsa.yaml#/properties/mdio
      unevaluatedProperties: false
  
      properties:
diff --git a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
index 833d2f68daa144..c58c4ec8613ac1 100644
--- a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
@@ -46,7 +46,7 @@ properties:
      maxItems: 1
  
    mdio:
-    $ref: /schemas/net/mdio.yaml#
+    $ref: dsa.yaml#/properties/mdio
      unevaluatedProperties: false
  
    clocks:

The nxp,sja1105.yaml schema also needed some changes.

dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings

SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus.
Therefore, disallow the mdios property for SJA1105, and the mdio property
for SJA1110.

Require the phy-handle property on the non-CPU ports if the mdios property
is being used.

Refer to dsa.yaml#/properties/mdio to point the human readers to the
description on the dsa.yaml schema.

---
  .../bindings/net/dsa/nxp,sja1105.yaml         | 20 ++++++++++++++++++-
  1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 82dda8fae8b16e..7d92350f1065b2 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -52,7 +52,7 @@ properties:
  
      patternProperties:
        "^mdio@[0-1]$":
-        $ref: /schemas/net/mdio.yaml#
+        $ref: dsa.yaml#/properties/mdio
          unevaluatedProperties: false
  
          properties:
@@ -128,14 +128,32 @@ allOf:
      then:
        properties:
          spi-cpol: false
+        mdios: false
+
        required:
          - spi-cpha
      else:
        properties:
          spi-cpha: false
+        mdio: false
+
        required:
          - spi-cpol
  
+  - if:
+      required: [ mdios ]
+    then:
+      patternProperties:
+        "^(ethernet-)?ports$":
+          patternProperties:
+            "^(ethernet-)?port@[0-9]+$":
+              if:
+                not:
+                  required: [ ethernet ]
+              then:
+                required:
+                  - phy-handle
+
  unevaluatedProperties: false
  
  examples:

Arınç

On 12.08.2023 19:28, Arınç ÜNAL wrote:
> I changed this to below. I will wait for reviews before submitting v2.
> 
> The realtek.yaml schema extends the mdio.yaml schema. The mdio.yaml schema
> is also being referred to through dsa.yaml#/$defs/ethernet-ports now which
> means we cannot disallow additional properties by 'unevaluatedProperties:
> false' on the dsa.yaml schema.
> 
> On the realtek.yaml schema, refer to dsa.yaml#/properties/mdio instead to
> point the human readers to the description on the dsa.yaml schema.
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> index ec74a660beda..03ccedbc49dc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> @@ -31,6 +31,24 @@ properties:
>         (single device hanging off a CPU port) must not specify this property
>       $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> +  mdio:
> +    description: The internal MDIO bus of the switch
> +    $ref: /schemas/net/mdio.yaml#
> +
> +if:
> +  required: [ mdio ]
> +then:
> +  patternProperties:
> +    "^(ethernet-)?ports$":
> +      patternProperties:
> +        "^(ethernet-)?port@[0-9]+$":
> +          if:
> +            not:
> +              required: [ ethernet ]
> +          then:
> +            required:
> +              - phy-handle
> +
>   additionalProperties: true
> 
>   $defs:
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cfd69c2604ea..f4b4fe0509a0 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -96,7 +96,7 @@ properties:
>         - '#interrupt-cells'
> 
>     mdio:
> -    $ref: /schemas/net/mdio.yaml#
> +    $ref: dsa.yaml#/properties/mdio
>       unevaluatedProperties: false
> 
>       properties:
> 
> Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote:
> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index 4d5f5cc6d031e2..82dda8fae8b16e 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -72,6 +72,10 @@ properties:
>            - compatible
>            - reg
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false

sja1105 does not support an "mdio" child property. I haven't checked the
others. Don't add properties that aren't supported.

> +
>  patternProperties:
>    "^(ethernet-)?ports$":
>      patternProperties:
> 
> The nxp,sja1105.yaml schema also needed some changes.
> 
> dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings
> 
> SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus.
> Therefore, disallow the mdios property for SJA1105, and the mdio property
> for SJA1110.
> 
> Require the phy-handle property on the non-CPU ports if the mdios property
> is being used.
> 
> Refer to dsa.yaml#/properties/mdio to point the human readers to the
> description on the dsa.yaml schema.
> 
> ---
>  .../bindings/net/dsa/nxp,sja1105.yaml         | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index 82dda8fae8b16e..7d92350f1065b2 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -52,7 +52,7 @@ properties:
>      patternProperties:
>        "^mdio@[0-1]$":
> -        $ref: /schemas/net/mdio.yaml#
> +        $ref: dsa.yaml#/properties/mdio
>          unevaluatedProperties: false
>          properties:
> @@ -128,14 +128,32 @@ allOf:
>      then:
>        properties:
>          spi-cpol: false
> +        mdios: false
> +
>        required:
>          - spi-cpha
>      else:
>        properties:
>          spi-cpha: false
> +        mdio: false
> +
>        required:
>          - spi-cpol
> +  - if:
> +      required: [ mdios ]
> +    then:
> +      patternProperties:
> +        "^(ethernet-)?ports$":
> +          patternProperties:
> +            "^(ethernet-)?port@[0-9]+$":
> +              if:
> +                not:
> +                  required: [ ethernet ]
> +              then:
> +                required:
> +                  - phy-handle

For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed)
are required for all ports (user, dsa or cpu).

Also, sja1105 does not populate the slave_mii_bus, so it never uses the
fallback where ports implicitly connect to an internal PHY if no phylink
bindings are present.

> +
>  unevaluatedProperties: false
>  examples:
> 
> Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.08.2023 14:53, Vladimir Oltean wrote:
> On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote:
>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> index 4d5f5cc6d031e2..82dda8fae8b16e 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> @@ -72,6 +72,10 @@ properties:
>>             - compatible
>>             - reg
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
> 
> sja1105 does not support an "mdio" child property. I haven't checked the
> others. Don't add properties that aren't supported.

Adding the mdio property to the dsa.yaml schema will allow it on all of the
schemas that refer to dsa.yaml such as this one. This addition here is only
to disallow additional properties under the mdio property for this specific
schema.

That said, my understanding is that the internal MDIO bus exists on all of
the switches controlled by DSA. Whether each individual DSA subdriver
supports registering it does not matter in terms of documenting the
internal MDIO bus for all DSA switches.

SJA1110 uses the mdios property instead because it's got two internal mdio
buses, which is why I invalidate the mdio property for it. If SJA1105 has
also got two internal mdio buses, let me know.

> 
>> +
>>   patternProperties:
>>     "^(ethernet-)?ports$":
>>       patternProperties:
>>
>> The nxp,sja1105.yaml schema also needed some changes.
>>
>> dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings
>>
>> SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus.
>> Therefore, disallow the mdios property for SJA1105, and the mdio property
>> for SJA1110.
>>
>> Require the phy-handle property on the non-CPU ports if the mdios property
>> is being used.
>>
>> Refer to dsa.yaml#/properties/mdio to point the human readers to the
>> description on the dsa.yaml schema.
>>
>> ---
>>   .../bindings/net/dsa/nxp,sja1105.yaml         | 20 ++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> index 82dda8fae8b16e..7d92350f1065b2 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> @@ -52,7 +52,7 @@ properties:
>>       patternProperties:
>>         "^mdio@[0-1]$":
>> -        $ref: /schemas/net/mdio.yaml#
>> +        $ref: dsa.yaml#/properties/mdio
>>           unevaluatedProperties: false
>>           properties:
>> @@ -128,14 +128,32 @@ allOf:
>>       then:
>>         properties:
>>           spi-cpol: false
>> +        mdios: false
>> +
>>         required:
>>           - spi-cpha
>>       else:
>>         properties:
>>           spi-cpha: false
>> +        mdio: false
>> +
>>         required:
>>           - spi-cpol
>> +  - if:
>> +      required: [ mdios ]
>> +    then:
>> +      patternProperties:
>> +        "^(ethernet-)?ports$":
>> +          patternProperties:
>> +            "^(ethernet-)?port@[0-9]+$":
>> +              if:
>> +                not:
>> +                  required: [ ethernet ]
>> +              then:
>> +                required:
>> +                  - phy-handle
> 
> For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed)
> are required for all ports (user, dsa or cpu).
> 
> Also, sja1105 does not populate the slave_mii_bus, so it never uses the
> fallback where ports implicitly connect to an internal PHY if no phylink
> bindings are present.

I'll handle these accordingly with your answer to my question above.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.08.2023 15:59, Arınç ÜNAL wrote:
> On 13.08.2023 14:53, Vladimir Oltean wrote:
>> On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote:
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> index 4d5f5cc6d031e2..82dda8fae8b16e 100644
>>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> @@ -72,6 +72,10 @@ properties:
>>>             - compatible
>>>             - reg
>>> +  mdio:
>>> +    $ref: dsa.yaml#/properties/mdio
>>> +    unevaluatedProperties: false
>>
>> sja1105 does not support an "mdio" child property. I haven't checked the
>> others. Don't add properties that aren't supported.
> 
> Adding the mdio property to the dsa.yaml schema will allow it on all of the
> schemas that refer to dsa.yaml such as this one. This addition here is only
> to disallow additional properties under the mdio property for this specific
> schema.
> 
> That said, my understanding is that the internal MDIO bus exists on all of
> the switches controlled by DSA. Whether each individual DSA subdriver
> supports registering it does not matter in terms of documenting the
> internal MDIO bus for all DSA switches.

On top of this, I'd argue to document the internal MDIO bus on the
ethernet-switch.yaml schema instead.

Arınç

> 
> SJA1110 uses the mdios property instead because it's got two internal mdio
> buses, which is why I invalidate the mdio property for it. If SJA1105 has
> also got two internal mdio buses, let me know.
> 
>>
>>> +
>>>   patternProperties:
>>>     "^(ethernet-)?ports$":
>>>       patternProperties:
>>>
>>> The nxp,sja1105.yaml schema also needed some changes.
>>>
>>> dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings
>>>
>>> SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus.
>>> Therefore, disallow the mdios property for SJA1105, and the mdio property
>>> for SJA1110.
>>>
>>> Require the phy-handle property on the non-CPU ports if the mdios property
>>> is being used.
>>>
>>> Refer to dsa.yaml#/properties/mdio to point the human readers to the
>>> description on the dsa.yaml schema.
>>>
>>> ---
>>>   .../bindings/net/dsa/nxp,sja1105.yaml         | 20 ++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> index 82dda8fae8b16e..7d92350f1065b2 100644
>>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>>> @@ -52,7 +52,7 @@ properties:
>>>       patternProperties:
>>>         "^mdio@[0-1]$":
>>> -        $ref: /schemas/net/mdio.yaml#
>>> +        $ref: dsa.yaml#/properties/mdio
>>>           unevaluatedProperties: false
>>>           properties:
>>> @@ -128,14 +128,32 @@ allOf:
>>>       then:
>>>         properties:
>>>           spi-cpol: false
>>> +        mdios: false
>>> +
>>>         required:
>>>           - spi-cpha
>>>       else:
>>>         properties:
>>>           spi-cpha: false
>>> +        mdio: false
>>> +
>>>         required:
>>>           - spi-cpol
>>> +  - if:
>>> +      required: [ mdios ]
>>> +    then:
>>> +      patternProperties:
>>> +        "^(ethernet-)?ports$":
>>> +          patternProperties:
>>> +            "^(ethernet-)?port@[0-9]+$":
>>> +              if:
>>> +                not:
>>> +                  required: [ ethernet ]
>>> +              then:
>>> +                required:
>>> +                  - phy-handle
>>
>> For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed)
>> are required for all ports (user, dsa or cpu).
>>
>> Also, sja1105 does not populate the slave_mii_bus, so it never uses the
>> fallback where ports implicitly connect to an internal PHY if no phylink
>> bindings are present.
> 
> I'll handle these accordingly with your answer to my question above.
> 
> Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote:
> On top of this, I'd argue to document the internal MDIO bus on the
> ethernet-switch.yaml schema instead.

Why?

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.08.2023 22:02, Vladimir Oltean wrote:
> On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote:
>> On top of this, I'd argue to document the internal MDIO bus on the
>> ethernet-switch.yaml schema instead.
> 
> Why?

Because a generic switch can have an internal MDIO bus, it's not specific
to a DSA controlled switch.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Mon, Aug 14, 2023 at 01:06:19PM +0300, Arınç ÜNAL wrote:
> On 13.08.2023 22:02, Vladimir Oltean wrote:
> > On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote:
> > > On top of this, I'd argue to document the internal MDIO bus on the
> > > ethernet-switch.yaml schema instead.
> > 
> > Why?
> 
> Because a generic switch can have an internal MDIO bus, it's not specific
> to a DSA controlled switch.
> 
> Arınç

I'm not sure it's that simple. Check out arch/mips/boot/dts/mscc/ocelot.dtsi.
Its switch IP ("mscc,vsc7514-switch") is on the same hierarchical level
with the "mscc,ocelot-miim" nodes (so, not a child), because the MDIO controller
isn't part of the address space of the switching IP. Actually that could equally
be considered true for DSA. Placing the "mdio" node as a child of the switch is
one of the possible options, but it has its limitations and is certainly
not the only one.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote:
> > sja1105 does not support an "mdio" child property. I haven't checked the
> > others. Don't add properties that aren't supported.
> 
> Adding the mdio property to the dsa.yaml schema will allow it on all of the
> schemas that refer to dsa.yaml such as this one. This addition here is only
> to disallow additional properties under the mdio property for this specific
> schema.
> 
> That said, my understanding is that the internal MDIO bus exists on all of
> the switches controlled by DSA.

How come?

> Whether each individual DSA subdriver supports registering it does not
> matter in terms of documenting the internal MDIO bus for all DSA
> switches.
> 
> SJA1110 uses the mdios property instead because it's got two internal mdio
> buses, which is why I invalidate the mdio property for it. If SJA1105 has
> also got two internal mdio buses, let me know.

SJA1105 has zero internal MDIO buses and zero internal PHYs.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.08.2023 22:01, Vladimir Oltean wrote:
> On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote:
>>> sja1105 does not support an "mdio" child property. I haven't checked the
>>> others. Don't add properties that aren't supported.
>>
>> Adding the mdio property to the dsa.yaml schema will allow it on all of the
>> schemas that refer to dsa.yaml such as this one. This addition here is only
>> to disallow additional properties under the mdio property for this specific
>> schema.
>>
>> That said, my understanding is that the internal MDIO bus exists on all of
>> the switches controlled by DSA.
> 
> How come?
> 
>> Whether each individual DSA subdriver supports registering it does not
>> matter in terms of documenting the internal MDIO bus for all DSA
>> switches.
>>
>> SJA1110 uses the mdios property instead because it's got two internal mdio
>> buses, which is why I invalidate the mdio property for it. If SJA1105 has
>> also got two internal mdio buses, let me know.
> 
> SJA1105 has zero internal MDIO buses and zero internal PHYs.

Ah okay. I didn't consider the switch architecture where the data interface
of the PHY is connected to the switch, and the PHY management interface is
connected to the mdio bus that the switch is connected to.

The schemas of the switches which already utilise the mdio property:
- /schemas/net/dsa/microchip,lan937x.yaml
- /schemas/net/dsa/qca8k.yaml
- /schemas/net/dsa/realtek.yaml
- /schemas/net/dsa/renesas,rzn1-a5psw.yaml

The schemas of the switches which don't have an internal MDIO bus, meaning
the mdio property must be invalid:
- /schemas/net/mscc,vsc7514-switch.yaml
- /schemas/net/dsa/nxp,sja1105.yaml

The schemas of the switches which I don't know if the switch has got an
internal MDIO bus:
- /schemas/net/dsa/arrow,xrs700x.yaml
   - I believe, because there's phy-handle defined on every port without the
     mdio node on the example, the PHYs are not connected to the internal
     MDIO bus. Therefore, we can invalidate the mdio property for this
     schema.
- /schemas/net/dsa/brcm,b53.yaml
   - Seems ok to keep it valid.
- /schemas/net/dsa/brcm,sf2.yaml
   - Seems ok to keep it valid.
- /schemas/net/dsa/hirschmann,hellcreek.yaml
   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
- /schemas/net/dsa/microchip,ksz.yaml
   - Seems ok to keep it valid.
- /schemas/net/dsa/mscc,ocelot.yaml
   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.

Not json-schema documentation, don't care about:
- ar9331.txt
- lan9303.txt
- lantiq-gswip.txt
- marvell.txt
- vitesse,vsc73xx.txt

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Andrew Lunn]

> Ah okay. I didn't consider the switch architecture where the data interface
> of the PHY is connected to the switch, and the PHY management interface is
> connected to the mdio bus that the switch is connected to.

The generic Linux architecture for PHYs and binding them to a MAC via
a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some
additional shortcuts to support 1:1 mapping if the switch has its own
MDIO bus, without describing it in DT, but this is just in addition to
the generic code.

> Not json-schema documentation, don't care about:
> - ar9331.txt
> - lan9303.txt
> - lantiq-gswip.txt
> - marvell.txt

The marvell switch can have up to 2 MDIO busses. If i remember
correctly, there is also one switch which has one MDIO bus per port.

	   Andrew

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 14.08.2023 16:09, Andrew Lunn wrote:
>> Ah okay. I didn't consider the switch architecture where the data interface
>> of the PHY is connected to the switch, and the PHY management interface is
>> connected to the mdio bus that the switch is connected to.
> 
> The generic Linux architecture for PHYs and binding them to a MAC via
> a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some
> additional shortcuts to support 1:1 mapping if the switch has its own
> MDIO bus, without describing it in DT, but this is just in addition to
> the generic code.

Understood.

> 
>> Not json-schema documentation, don't care about:
>> - ar9331.txt
>> - lan9303.txt
>> - lantiq-gswip.txt
>> - marvell.txt
> 
> The marvell switch can have up to 2 MDIO busses. If i remember
> correctly, there is also one switch which has one MDIO bus per port.

I will work on writing a schema for these once I'm done with this.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

Hi Andrew,

On 14.08.2023 16:09, Andrew Lunn wrote:
>> Ah okay. I didn't consider the switch architecture where the data interface
>> of the PHY is connected to the switch, and the PHY management interface is
>> connected to the mdio bus that the switch is connected to.
> 
> The generic Linux architecture for PHYs and binding them to a MAC via
> a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some
> additional shortcuts to support 1:1 mapping if the switch has its own
> MDIO bus, without describing it in DT, but this is just in addition to
> the generic code.
> 
>> Not json-schema documentation, don't care about:
>> - ar9331.txt
>> - lan9303.txt
>> - lantiq-gswip.txt
>> - marvell.txt
> 
> The marvell switch can have up to 2 MDIO busses. If i remember
> correctly, there is also one switch which has one MDIO bus per port.

I'm writing the json-schema for Marvell switches. I've checked a few
devicetree source files on Linus's Linux tree, I only see two buses used at
the most. The internal bus and another bus with
marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with
marvell,mv88e6250 though. Could the switch that has one MDIO bus per port
be this one? Also, is every bus of this switch a
marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining
are marvell mv88e6xxx-mdio-external buses?

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Andrew Lunn]

> > The marvell switch can have up to 2 MDIO busses. If i remember
> > correctly, there is also one switch which has one MDIO bus per port.
> 
> I'm writing the json-schema for Marvell switches. I've checked a few
> devicetree source files on Linus's Linux tree, I only see two buses used at
> the most.

Sorry, i was ambiguous. Its not a Marvell switch which can have one
MDIO bus per port. I don't remember which switch it is, and it might
be a pure switchdev switch, not a DSA switch.

> The internal bus and another bus with
> marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with
> marvell,mv88e6250 though. Could the switch that has one MDIO bus per port
> be this one? Also, is every bus of this switch a
> marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining
> are marvell mv88e6xxx-mdio-external buses?

Only the 6390 family has two busses. It has an internal MDIO bus with
the same register API as all the other switches. However, unlike the
other families, it is not exposed on pins. And the 6390 has a second
MDIO bus using a slight variant of the registers, which is connected
to the outside world via pins. This second bus then has a compatible
to separate it from the normal MDIO bus.

	Andrew

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.09.2023 04:21, Andrew Lunn wrote:
>>> The marvell switch can have up to 2 MDIO busses. If i remember
>>> correctly, there is also one switch which has one MDIO bus per port.
>>
>> I'm writing the json-schema for Marvell switches. I've checked a few
>> devicetree source files on Linus's Linux tree, I only see two buses used at
>> the most.
> 
> Sorry, i was ambiguous. Its not a Marvell switch which can have one
> MDIO bus per port. I don't remember which switch it is, and it might
> be a pure switchdev switch, not a DSA switch.
> 
>> The internal bus and another bus with
>> marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with
>> marvell,mv88e6250 though. Could the switch that has one MDIO bus per port
>> be this one? Also, is every bus of this switch a
>> marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining
>> are marvell mv88e6xxx-mdio-external buses?
> 
> Only the 6390 family has two busses. It has an internal MDIO bus with
> the same register API as all the other switches. However, unlike the
> other families, it is not exposed on pins. And the 6390 has a second
> MDIO bus using a slight variant of the registers, which is connected
> to the outside world via pins. This second bus then has a compatible
> to separate it from the normal MDIO bus.

OK, I will disallow the external mdio bus for the compatible strings other
than marvell,mv88e6190.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

Arınç,

On Mon, Aug 14, 2023 at 01:06:29PM +0300, Arınç ÜNAL wrote:
> On 13.08.2023 22:01, Vladimir Oltean wrote:
> > SJA1105 has zero internal MDIO buses and zero internal PHYs.
> 
> Ah okay. I didn't consider the switch architecture where the data interface
> of the PHY is connected to the switch, and the PHY management interface is
> connected to the mdio bus that the switch is connected to.
> 
> The schemas of the switches which already utilise the mdio property:
> - /schemas/net/dsa/microchip,lan937x.yaml
> - /schemas/net/dsa/qca8k.yaml
> - /schemas/net/dsa/realtek.yaml
> - /schemas/net/dsa/renesas,rzn1-a5psw.yaml
> 
> The schemas of the switches which don't have an internal MDIO bus, meaning
> the mdio property must be invalid:
> - /schemas/net/mscc,vsc7514-switch.yaml
> - /schemas/net/dsa/nxp,sja1105.yaml
> 
> The schemas of the switches which I don't know if the switch has got an
> internal MDIO bus:
> - /schemas/net/dsa/arrow,xrs700x.yaml
>   - I believe, because there's phy-handle defined on every port without the
>     mdio node on the example, the PHYs are not connected to the internal
>     MDIO bus. Therefore, we can invalidate the mdio property for this
>     schema.
> - /schemas/net/dsa/brcm,b53.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/brcm,sf2.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/hirschmann,hellcreek.yaml
>   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
> - /schemas/net/dsa/microchip,ksz.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/mscc,ocelot.yaml
>   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
> 
> Not json-schema documentation, don't care about:
> - ar9331.txt
> - lan9303.txt
> - lantiq-gswip.txt
> - marvell.txt
> - vitesse,vsc73xx.txt
> 
> Arınç

We have to keep in sight why we're here, and stick to that.

You had issues with a device tree that didn't work, but it passed
validation, and you're trying to enforce extra rules through the
json-schema so that next time, it fails. Verbally, that rule would be:
"if the switch has a ds->slave_mii_bus which does not have an OF
presence, then phylink compatible bindings may be omitted, and that has
a special and valid meaning (the port is connected to an internal PHY on
that ds->slave_mii_bus). If ds->slave_mii_bus has an OF presence, or if
ds->slave_mii_bus is NULL, then phylink-compatible bindings (phy-handle
or fixed-link or managed) are required on all user ports".

So it becomes a question of tracking ds->slave_mii_bus for all drivers.
In essence, it's fundamentally about the ds->slave_mii_bus, not about
the "mdio" child node. See more below.

There are 2 code paths that lead to its creation:

1. DSA registers the bus in dsa_slave_mii_bus_init(), based on the
   presence of ds->ops->phy_read() and ds->ops->phy_write(). Traditionally,
   a slave_mii_bus created this way was always non-OF-based, but Luiz,
   in commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), thought
   it would be a good idea for them to be optionally OF-based (and thus,
   useless at their primary purpose of being able to have internal PHYs
   without a phy-handle). But, it was thought that the framework
   registering an MDIO bus automatically would be a plus. So, ds->slave_mii_bus
   created in this way may or may not have an OF presence, with no way
   to know except to look at device trees (and to presume that they do).

   The drivers that populate ds->ops->phy_read() and ds->ops->phy_write() are:
    |
    +--- dsa_loop_driver: not OF-based
    |
    +--- ksz_switch_ops: OF-based or non-OF-based
    |
    +--- mv88e6060_switch_ops: OF-based or non-OF-based
    |
    +--- lan9303_switch_ops: OF-based or non-OF-based
    |
    +--- rtl8365mb_switch_ops_mdio: OF-based or non-OF-based
    |
    +--- b53_switch_ops: OF-based or non-OF-based
    |
    +--- vsc73xx_ds_ops: OF-based or non-OF-based

2. The switch driver registers the bus, and populates ds->slave_mii_bus with
   a pointer to it.
    |
    +--- Bus is not OF-based (it was registered with mdiobus_register()).
    |    This is the normal case:
    |      * mv88e6xxx_default_mdio_bus() in some cases
    |      * qca8k_mdio_register() in the "qca8k-legacy slave mii" case
    |      * bcm_sf2_mdio_register()
    |      * mt7530_setup_mdio()
    |
    +--- Bus is OF-based (it was registered with of_mdiobus_register()).
         I've no idea why you'd do this, because you have neither the
         benefit of using a non-OF-based phy_connect(), nor the benefit
         of having DSA register the bus for you:
           * mv88e6xxx_default_mdio_bus() when of_get_child_by_name(np, "mdio")
             is non-NULL
           * qca8k_mdio_register() when of_get_child_by_name(priv->dev->of_node, "mdio")
             is non-NULL
           * ksz_mdio_register() - it always wants an "mdio" child node
           * gswip_mdio() - it always wants a child node compatible with
             "lantiq,xrx200-mdio"
           * realtek_smi_setup_mdio() - it always wants a child node
             compatible with "realtek,smi-mdio"

For switches in the first category, the presence of the "mdio" child
node is what makes the ds->slave_mii_bus be OF-based or not, since it is
all the same binding, imposed by Luiz in dsa_switch_setup().

For switches in the second category, it all depends on the way in which
the driver finds the node for of_mdiobus_register().


Having identified all switches which make some sort of use of
ds->slave_mii_bus, the rule would sound like this:

1. If the schema is that of (need to replace this with compatible
   strings, I'm too lazy for that):

   - ksz_switch_ops
   - mv88e6060_switch_ops
   - lan9303_switch_ops
   - rtl8365mb_switch_ops_mdio
   - b53_switch_ops
   - vsc73xx_ds_ops
   - mv88e6xxx
   - qca8k

   and we have an "mdio" child, then phylink bindings are mandatory on user ports.

2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
   then phylink bindings are mandatory on user ports (I haven't checked,
   but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
   in that case, this goes to category 4 below).

3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
   "realtek,smi-mdio", then phylink bindings are mandatory on user ports
   (same comment about the child MDIO note maybe being mandatory).

4. If the switch didn't appear in the above set of rules, then phylink
   bindings are unconditionally mandatory on user ports.

We don't care at all what the drivers that don't use ds->slave_mii_bus
do with the "mdio" child node. It doesn't change the fact that their
user ports can't have missing phylink bindings.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 14.08.2023 17:36, Vladimir Oltean wrote:
> Arınç,
> 
> On Mon, Aug 14, 2023 at 01:06:29PM +0300, Arınç ÜNAL wrote:
>> On 13.08.2023 22:01, Vladimir Oltean wrote:
>>> SJA1105 has zero internal MDIO buses and zero internal PHYs.
>>
>> Ah okay. I didn't consider the switch architecture where the data interface
>> of the PHY is connected to the switch, and the PHY management interface is
>> connected to the mdio bus that the switch is connected to.
>>
>> The schemas of the switches which already utilise the mdio property:
>> - /schemas/net/dsa/microchip,lan937x.yaml
>> - /schemas/net/dsa/qca8k.yaml
>> - /schemas/net/dsa/realtek.yaml
>> - /schemas/net/dsa/renesas,rzn1-a5psw.yaml
>>
>> The schemas of the switches which don't have an internal MDIO bus, meaning
>> the mdio property must be invalid:
>> - /schemas/net/mscc,vsc7514-switch.yaml
>> - /schemas/net/dsa/nxp,sja1105.yaml
>>
>> The schemas of the switches which I don't know if the switch has got an
>> internal MDIO bus:
>> - /schemas/net/dsa/arrow,xrs700x.yaml
>>    - I believe, because there's phy-handle defined on every port without the
>>      mdio node on the example, the PHYs are not connected to the internal
>>      MDIO bus. Therefore, we can invalidate the mdio property for this
>>      schema.
>> - /schemas/net/dsa/brcm,b53.yaml
>>    - Seems ok to keep it valid.
>> - /schemas/net/dsa/brcm,sf2.yaml
>>    - Seems ok to keep it valid.
>> - /schemas/net/dsa/hirschmann,hellcreek.yaml
>>    - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
>> - /schemas/net/dsa/microchip,ksz.yaml
>>    - Seems ok to keep it valid.
>> - /schemas/net/dsa/mscc,ocelot.yaml
>>    - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
>>
>> Not json-schema documentation, don't care about:
>> - ar9331.txt
>> - lan9303.txt
>> - lantiq-gswip.txt
>> - marvell.txt
>> - vitesse,vsc73xx.txt
>>
>> Arınç
> 
> We have to keep in sight why we're here, and stick to that.
> 
> You had issues with a device tree that didn't work, but it passed
> validation, and you're trying to enforce extra rules through the
> json-schema so that next time, it fails. Verbally, that rule would be:
> "if the switch has a ds->slave_mii_bus which does not have an OF
> presence, then phylink compatible bindings may be omitted, and that has
> a special and valid meaning (the port is connected to an internal PHY on
> that ds->slave_mii_bus). If ds->slave_mii_bus has an OF presence, or if
> ds->slave_mii_bus is NULL, then phylink-compatible bindings (phy-handle
> or fixed-link or managed) are required on all user ports".
> 
> So it becomes a question of tracking ds->slave_mii_bus for all drivers.
> In essence, it's fundamentally about the ds->slave_mii_bus, not about
> the "mdio" child node. See more below.

Before I continue commenting, I'd like to state my understanding so we can
make sure we're on the same page. If a driver doesn't use
ds->slave_mii_bus, the switch it controls must not have any internal MDIO
buses. Otherwise the PHYs on these buses couldn't function, and an improper
driver like this would not be on the official Linux source code.

I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
with a switch component. Since the switch component is not designed to be a
standalone IC, the MDIO bus of the SoC could just as well be used without
the need to design an MDIO controller specific to the switch component, to
manage the PHYs. So I see this switch as just another case of a switch
without an internal MDIO bus.

> 
> There are 2 code paths that lead to its creation:
> 
> 1. DSA registers the bus in dsa_slave_mii_bus_init(), based on the
>     presence of ds->ops->phy_read() and ds->ops->phy_write(). Traditionally,
>     a slave_mii_bus created this way was always non-OF-based, but Luiz,
>     in commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), thought
>     it would be a good idea for them to be optionally OF-based (and thus,
>     useless at their primary purpose of being able to have internal PHYs
>     without a phy-handle). But, it was thought that the framework
>     registering an MDIO bus automatically would be a plus. So, ds->slave_mii_bus
>     created in this way may or may not have an OF presence, with no way
>     to know except to look at device trees (and to presume that they do).
> 
>     The drivers that populate ds->ops->phy_read() and ds->ops->phy_write() are:
>      |
>      +--- dsa_loop_driver: not OF-based
>      |
>      +--- ksz_switch_ops: OF-based or non-OF-based
>      |
>      +--- mv88e6060_switch_ops: OF-based or non-OF-based
>      |
>      +--- lan9303_switch_ops: OF-based or non-OF-based
>      |
>      +--- rtl8365mb_switch_ops_mdio: OF-based or non-OF-based
>      |
>      +--- b53_switch_ops: OF-based or non-OF-based
>      |
>      +--- vsc73xx_ds_ops: OF-based or non-OF-based
> 
> 2. The switch driver registers the bus, and populates ds->slave_mii_bus with
>     a pointer to it.
>      |
>      +--- Bus is not OF-based (it was registered with mdiobus_register()).
>      |    This is the normal case:
>      |      * mv88e6xxx_default_mdio_bus() in some cases
>      |      * qca8k_mdio_register() in the "qca8k-legacy slave mii" case
>      |      * bcm_sf2_mdio_register()
>      |      * mt7530_setup_mdio()
>      |
>      +--- Bus is OF-based (it was registered with of_mdiobus_register()).
>           I've no idea why you'd do this, because you have neither the
>           benefit of using a non-OF-based phy_connect(), nor the benefit
>           of having DSA register the bus for you:
>             * mv88e6xxx_default_mdio_bus() when of_get_child_by_name(np, "mdio")
>               is non-NULL
>             * qca8k_mdio_register() when of_get_child_by_name(priv->dev->of_node, "mdio")
>               is non-NULL
>             * ksz_mdio_register() - it always wants an "mdio" child node
>             * gswip_mdio() - it always wants a child node compatible with
>               "lantiq,xrx200-mdio"
>             * realtek_smi_setup_mdio() - it always wants a child node
>               compatible with "realtek,smi-mdio"
> 
> For switches in the first category, the presence of the "mdio" child
> node is what makes the ds->slave_mii_bus be OF-based or not, since it is
> all the same binding, imposed by Luiz in dsa_switch_setup().

Makes sense.

> 
> For switches in the second category, it all depends on the way in which
> the driver finds the node for of_mdiobus_register().

Ok, so some drivers require the mdio child node. Some require it and the
compatible property with a certain string.

MDIO controlled Realtek switches do not need the compatible property under
the mdio child node. There're no compatible strings to make a distinction
between the SMI and MDIO controlled switches so the best we can do is keep
it the way it currently is. Define realtek,smi-mdio as a compatible string
but keep the compatible property optional. I did state this on my reply to
patch 3 but still received reviewed-bys regardless.

> 
> 
> Having identified all switches which make some sort of use of
> ds->slave_mii_bus, the rule would sound like this:
> 
> 1. If the schema is that of (need to replace this with compatible
>     strings, I'm too lazy for that):
> 
>     - ksz_switch_ops
>     - mv88e6060_switch_ops
>     - lan9303_switch_ops
>     - rtl8365mb_switch_ops_mdio
>     - b53_switch_ops
>     - vsc73xx_ds_ops
>     - mv88e6xxx
>     - qca8k
> 
>     and we have an "mdio" child, then phylink bindings are mandatory on user ports.
> 
> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
>     then phylink bindings are mandatory on user ports (I haven't checked,
>     but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
>     in that case, this goes to category 4 below).
> 
> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
>     "realtek,smi-mdio", then phylink bindings are mandatory on user ports
>     (same comment about the child MDIO note maybe being mandatory).
> 
> 4. If the switch didn't appear in the above set of rules, then phylink
>     bindings are unconditionally mandatory on user ports.
> 
> We don't care at all what the drivers that don't use ds->slave_mii_bus
> do with the "mdio" child node. It doesn't change the fact that their
> user ports can't have missing phylink bindings.

I partially agree. I say, for the switches without an internal MDIO bus,
invalidate the mdio child node, and enforce the phylink bindings on the
user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
invalidate the mdio child node, and enforce the phylink bindings on the
user ports if the mdios property is used.

I'd like to add this before I conclude. The way I understand dt-bindings is
that a binding does not have to translate to an action on the driver.
Documenting bindings for the sole purpose of describing hardware is a valid
case. For example, currently, the MT753X DSA subdriver won't, in any way,
register the bus OF-based. Still, the mdio property for the switches which
this driver controls can be documented because the internal mdio bus does
exist on the hardware.

So I'd like to keep the mdio property valid for the switches which their
drivers can only register non-OF-based ds->slave_mii_bus.

In conclusion, what to do:

- Define "the mdio property" and "the enforcement of phylink bindings for
   user ports if mdio property is used" on ethernet-switch.yaml.
     - Invalidate the mdio property on the switches without an internal MDIO
       bus.
- Define "the enforcement of phylink bindings for user ports" on the
   switches without an internal MDIO bus.
- Require "the mdio property" for the switches which their driver requires
   it to function.
- Require "the mdio property" and "the compatible string of the mdio
   property" for the switches which their driver requires them to function.

There's no 1:1 switch to switch compatible string relation, as seen on
Realtek switches so I'll have to figure that out as I go.

I'm open to your comments to this mail but the gap between discussion and
end result has widened a lot on this patch series so I'd like to first
offload this conversation by preparing v2 with what I said here and discuss
further there.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

Hi Arınç,

I am on vacation and I will just reply with some clarification aspects,
without having done any further research on the topic since my last reply.

On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote:
> Before I continue commenting, I'd like to state my understanding so we can
> make sure we're on the same page. If a driver doesn't use
> ds->slave_mii_bus, the switch it controls must not have any internal MDIO
> buses. Otherwise the PHYs on these buses couldn't function, and an improper
> driver like this would not be on the official Linux source code.

A DSA switch port, like any OF-based ethernet-controller which uses
phylink, will use one of the phy-handle, fixed-link or managed properties
to describe the interface connecting the MAC/MAC-side PCS to the PHY.

At its core, ds->slave_mii_bus is nothing more than a mechanism to make
sense of device trees where the above 3 phylink properties are not present.

It is completely false to say that if a driver doesn't have ds->slave_mii_bus,
it must not have an internal MDIO bus. Because you could still describe
that internal MDIO bus like below, without making any use of the sole property
that makes ds->slave_mii_bus useful from a dt-bindings perspective.

ethernet-switch {
	ethernet-ports {
		port@0 {
			reg = <0>;
			phy-handle = <&port0_phy>;
			phy-mode = "internal";
		};
	};

	mdio {
		port0_phy: ethernet-phy@0 {
			reg = <0>;
		};
	};
};

This is the more universal way of describing the port setup in an
OF-based way. There is also the DSA-specific (and old-style, before phylink)
way of describing the same thing, which relies on the non-OF-based
ds->slave_mii_bus, with bindings that look like this:

ethernet-switch {
	ethernet-ports {
		port@0 {
			reg = <0>;
		};
	};
};

But, I would say that the first variant of the binding is preferable,
since it is more universal.

Not all switches that have an internal MDIO bus support the second
variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't).
But, they support the same configuration through the first form.

Furthermore, on the U-Boot mailing lists, I have been suggesting that
the DM_DSA driver for mv88e6xxx should not bother to support the second
version of the binding, since it is just more code to be added to handle
a case which can already be described with the more universal first binding.

> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
> with a switch component. Since the switch component is not designed to be a
> standalone IC, the MDIO bus of the SoC could just as well be used without
> the need to design an MDIO controller specific to the switch component, to
> manage the PHYs. So I see this switch as just another case of a switch
> without an internal MDIO bus.

Well, we need to clarify the semantics of an "internal" MDIO bus.

I would say most discrete chips with DSA switches have this SoC-style
architecture, with separate address spaces for the switching IP, MDIO
bus, GPIO controller, IRQ controllers, temperature sensors etc (see
"mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is
controlled over SPIO instead of MMIO). The dt-bindings of most DSA
switches may or may not reflect that discrete chip organization. Those
drivers and dt-bindings could be reimagined so that DSA is not the
top-level driver.

Yet, I would argue that it's wrong to say that because it isn't an OF
child of the switch, the MDIO bus of the VSC7514 is not internal in the
same way that the Realtek MDIO bus is internal. The switch ports are
connected to internal PHYs on this MDIO bus, and there aren't PHYs on
this MDIO bus that go to other MACs than the switch ports. So, the
VSC7514 MDIO bus could legally be called the internal MDIO bus of the
switch, even if there isn't a parent/child relationship between them.

So, what I'm disagreeing with is your insistence to correlate your
problem with internal MDIO buses. The way in which the problem is
formulated dictates what problem gets solved, and the problem is not
correctly formulated here. It is purely about ds->slave_mii_bus and its
driver-defined OF presence/absence. It is a DSA-specific binding aspect
which not even all DSA switches inherit, let alone bindings outside DSA.

> > For switches in the second category, it all depends on the way in which
> > the driver finds the node for of_mdiobus_register().
> 
> Ok, so some drivers require the mdio child node. Some require it and the
> compatible property with a certain string.
> 
> MDIO controlled Realtek switches do not need the compatible property under
> the mdio child node. There're no compatible strings to make a distinction
> between the SMI and MDIO controlled switches so the best we can do is keep
> it the way it currently is. Define realtek,smi-mdio as a compatible string
> but keep the compatible property optional. I did state this on my reply to
> patch 3 but still received reviewed-bys regardless.

Yes, because.... [1]

> > Having identified all switches which make some sort of use of
> > ds->slave_mii_bus, the rule would sound like this:
> > 
> > 1. If the schema is that of (need to replace this with compatible
> >     strings, I'm too lazy for that):
> > 
> >     - ksz_switch_ops
> >     - mv88e6060_switch_ops
> >     - lan9303_switch_ops
> >     - rtl8365mb_switch_ops_mdio
> >     - b53_switch_ops
> >     - vsc73xx_ds_ops
> >     - mv88e6xxx
> >     - qca8k
> > 
> >     and we have an "mdio" child, then phylink bindings are mandatory on user ports.
> > 
> > 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
> >     then phylink bindings are mandatory on user ports (I haven't checked,
> >     but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
> >     in that case, this goes to category 4 below).
> > 
> > 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
> >     "realtek,smi-mdio", then phylink bindings are mandatory on user ports
> >     (same comment about the child MDIO note maybe being mandatory).
> > 
> > 4. If the switch didn't appear in the above set of rules, then phylink
> >     bindings are unconditionally mandatory on user ports.
> > 
> > We don't care at all what the drivers that don't use ds->slave_mii_bus
> > do with the "mdio" child node. It doesn't change the fact that their
> > user ports can't have missing phylink bindings.
> 
> I partially agree. I say, for the switches without an internal MDIO bus,
> invalidate the mdio child node, and enforce the phylink bindings on the
> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
> invalidate the mdio child node, and enforce the phylink bindings on the
> user ports if the mdios property is used.

Why "if the mdios property is used" and not "always"? :-/

To say it again: because the sja1105 driver does not use ds->slave_mii_bus,
it can make no sense of dt-bindings on user ports which lack phylink properties.
So they are *always* needed. The "mdios" property changes nothing in that regard.

> 
> I'd like to add this before I conclude. The way I understand dt-bindings is
> that a binding does not have to translate to an action on the driver.
> Documenting bindings for the sole purpose of describing hardware is a valid
> case.

[1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
otherwise the same, right? So why would they have different dt-bindings?

> For example, currently, the MT753X DSA subdriver won't, in any way,
> register the bus OF-based. Still, the mdio property for the switches which
> this driver controls can be documented because the internal mdio bus does
> exist on the hardware.

It can, but the whole point is: if ds->slave_mii_bus gains an OF presence,
then it loses its core functionality (that user ports can lack phylink
bindings). This is the entire essence of what this discussion should capture.

> 
> So I'd like to keep the mdio property valid for the switches which their
> drivers can only register non-OF-based ds->slave_mii_bus.
> 
> In conclusion, what to do:
> 
> - Define "the mdio property" and "the enforcement of phylink bindings for
>   user ports if mdio property is used" on ethernet-switch.yaml.
>     - Invalidate the mdio property on the switches without an internal MDIO
>       bus.
> - Define "the enforcement of phylink bindings for user ports" on the
>   switches without an internal MDIO bus.
> - Require "the mdio property" for the switches which their driver requires
>   it to function.
> - Require "the mdio property" and "the compatible string of the mdio
>   property" for the switches which their driver requires them to function.
> 
> There's no 1:1 switch to switch compatible string relation, as seen on
> Realtek switches so I'll have to figure that out as I go.
> 
> I'm open to your comments to this mail but the gap between discussion and
> end result has widened a lot on this patch series so I'd like to first
> offload this conversation by preparing v2 with what I said here and discuss
> further there.

Honestly, from my side, a verbal comment in the dt-bindings document
would have been just fine, as long as it is truthful to the reality it
describes.

You wanted to over-complicate things with an actual schema validation,
and then hooking onto things that are unrelated with the phenomenon that
needs to be captured (like the "mdio" child node, without explicit
regard to whether it is the ds->slave_mii_bus or not).

It's not about internal MDIO buses in general, it's about whether those
internal MDIO buses are used in ds->slave_mii_bus, and their OF
presence/absence! That is absolutely driver-specific and I would only
expect a driver-specific way of enforcing it. I didn't say it's not
hard, and I didn't ask to enforce it, either.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

Hey Vladimir,

On 27.08.2023 15:12, Vladimir Oltean wrote:
> Hi Arınç,
> 
> I am on vacation and I will just reply with some clarification aspects,
> without having done any further research on the topic since my last reply.
> 
> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote:
>> Before I continue commenting, I'd like to state my understanding so we can
>> make sure we're on the same page. If a driver doesn't use
>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO
>> buses. Otherwise the PHYs on these buses couldn't function, and an improper
>> driver like this would not be on the official Linux source code.
> 
> A DSA switch port, like any OF-based ethernet-controller which uses
> phylink, will use one of the phy-handle, fixed-link or managed properties
> to describe the interface connecting the MAC/MAC-side PCS to the PHY.
> 
> At its core, ds->slave_mii_bus is nothing more than a mechanism to make
> sense of device trees where the above 3 phylink properties are not present.
> 
> It is completely false to say that if a driver doesn't have ds->slave_mii_bus,
> it must not have an internal MDIO bus. Because you could still describe
> that internal MDIO bus like below, without making any use of the sole property
> that makes ds->slave_mii_bus useful from a dt-bindings perspective.
> 
> ethernet-switch {
> 	ethernet-ports {
> 		port@0 {
> 			reg = <0>;
> 			phy-handle = <&port0_phy>;
> 			phy-mode = "internal";
> 		};
> 	};
> 
> 	mdio {
> 		port0_phy: ethernet-phy@0 {
> 			reg = <0>;
> 		};
> 	};
> };
> 
> This is the more universal way of describing the port setup in an
> OF-based way. There is also the DSA-specific (and old-style, before phylink)
> way of describing the same thing, which relies on the non-OF-based
> ds->slave_mii_bus, with bindings that look like this:
> 
> ethernet-switch {
> 	ethernet-ports {
> 		port@0 {
> 			reg = <0>;
> 		};
> 	};
> };
> 
> But, I would say that the first variant of the binding is preferable,
> since it is more universal.
> 
> Not all switches that have an internal MDIO bus support the second
> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't).
> But, they support the same configuration through the first form.

Understood.

> 
> Furthermore, on the U-Boot mailing lists, I have been suggesting that
> the DM_DSA driver for mv88e6xxx should not bother to support the second
> version of the binding, since it is just more code to be added to handle
> a case which can already be described with the more universal first binding.

That makes sense.

> 
>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
>> with a switch component. Since the switch component is not designed to be a
>> standalone IC, the MDIO bus of the SoC could just as well be used without
>> the need to design an MDIO controller specific to the switch component, to
>> manage the PHYs. So I see this switch as just another case of a switch
>> without an internal MDIO bus.
> 
> Well, we need to clarify the semantics of an "internal" MDIO bus.
> 
> I would say most discrete chips with DSA switches have this SoC-style
> architecture, with separate address spaces for the switching IP, MDIO
> bus, GPIO controller, IRQ controllers, temperature sensors etc (see
> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is
> controlled over SPIO instead of MMIO). The dt-bindings of most DSA
> switches may or may not reflect that discrete chip organization. Those
> drivers and dt-bindings could be reimagined so that DSA is not the
> top-level driver.
> 
> Yet, I would argue that it's wrong to say that because it isn't an OF
> child of the switch, the MDIO bus of the VSC7514 is not internal in the
> same way that the Realtek MDIO bus is internal. The switch ports are
> connected to internal PHYs on this MDIO bus, and there aren't PHYs on
> this MDIO bus that go to other MACs than the switch ports. So, the
> VSC7514 MDIO bus could legally be called the internal MDIO bus of the
> switch, even if there isn't a parent/child relationship between them.

Good point, I had believed that the management interface of all of the PHYs
being connected to the MDIO bus - which is not part of the switching IP
address space - would be enough to classify the MDIO bus as non-internal.

However, the architecture of separate address spaces for the switching IP
and MDIO bus is used on any type of IC with the switching feature.
Therefore, this characteristic cannot be used to distinguish whether an
MDIO bus is of a switch.

What we can refer to to classify an internal MDIO bus is by confirming the
data interface of all PHYs on the MDIO bus is connected to the switch port
MACs, as you have pointed out here.

Because the architecture of separate address spaces for the switching IP
and MDIO bus is used on any type of IC with the switching feature, it can
differ by driver how the MDIO bus is defined on the dt-bindings. So we
can't make universal bindings of an internal MDIO bus of a switch that
apply to every switch.

So, the correct approach is to define things under the switch-specific
schema which is affine to the driver, as you have already pointed out.
Which schemas to define what will of course differ.

> 
> So, what I'm disagreeing with is your insistence to correlate your
> problem with internal MDIO buses. The way in which the problem is
> formulated dictates what problem gets solved, and the problem is not
> correctly formulated here. It is purely about ds->slave_mii_bus and its
> driver-defined OF presence/absence. It is a DSA-specific binding aspect
> which not even all DSA switches inherit, let alone bindings outside DSA.

Got it.

> 
>>> For switches in the second category, it all depends on the way in which
>>> the driver finds the node for of_mdiobus_register().
>>
>> Ok, so some drivers require the mdio child node. Some require it and the
>> compatible property with a certain string.
>>
>> MDIO controlled Realtek switches do not need the compatible property under
>> the mdio child node. There're no compatible strings to make a distinction
>> between the SMI and MDIO controlled switches so the best we can do is keep
>> it the way it currently is. Define realtek,smi-mdio as a compatible string
>> but keep the compatible property optional. I did state this on my reply to
>> patch 3 but still received reviewed-bys regardless.
> 
> Yes, because.... [1]
> 
>>> Having identified all switches which make some sort of use of
>>> ds->slave_mii_bus, the rule would sound like this:
>>>
>>> 1. If the schema is that of (need to replace this with compatible
>>>      strings, I'm too lazy for that):
>>>
>>>      - ksz_switch_ops
>>>      - mv88e6060_switch_ops
>>>      - lan9303_switch_ops
>>>      - rtl8365mb_switch_ops_mdio
>>>      - b53_switch_ops
>>>      - vsc73xx_ds_ops
>>>      - mv88e6xxx
>>>      - qca8k
>>>
>>>      and we have an "mdio" child, then phylink bindings are mandatory on user ports.
>>>
>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
>>>      then phylink bindings are mandatory on user ports (I haven't checked,
>>>      but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
>>>      in that case, this goes to category 4 below).
>>>
>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
>>>      "realtek,smi-mdio", then phylink bindings are mandatory on user ports
>>>      (same comment about the child MDIO note maybe being mandatory).
>>>
>>> 4. If the switch didn't appear in the above set of rules, then phylink
>>>      bindings are unconditionally mandatory on user ports.
>>>
>>> We don't care at all what the drivers that don't use ds->slave_mii_bus
>>> do with the "mdio" child node. It doesn't change the fact that their
>>> user ports can't have missing phylink bindings.
>>
>> I partially agree. I say, for the switches without an internal MDIO bus,
>> invalidate the mdio child node, and enforce the phylink bindings on the
>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
>> invalidate the mdio child node, and enforce the phylink bindings on the
>> user ports if the mdios property is used.
> 
> Why "if the mdios property is used" and not "always"? :-/
> 
> To say it again: because the sja1105 driver does not use ds->slave_mii_bus,
> it can make no sense of dt-bindings on user ports which lack phylink properties.
> So they are *always* needed. The "mdios" property changes nothing in that regard.

Got it.

> 
>>
>> I'd like to add this before I conclude. The way I understand dt-bindings is
>> that a binding does not have to translate to an action on the driver.
>> Documenting bindings for the sole purpose of describing hardware is a valid
>> case.
> 
> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
> otherwise the same, right? So why would they have different dt-bindings?

Honestly, I'm wondering the answer to this as well. For some reason, when
probing the SMI controlled Realtek switches, instead of just letting
dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
on realtek-smi.c:

- priv->slave_mii_bus is allocated.
- mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
- priv->slave_mii_bus->dev.of_node = mdio_np;
- ds->slave_mii_bus = priv->slave_mii_bus;

> 
>> For example, currently, the MT753X DSA subdriver won't, in any way,
>> register the bus OF-based. Still, the mdio property for the switches which
>> this driver controls can be documented because the internal mdio bus does
>> exist on the hardware.
> 
> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence,
> then it loses its core functionality (that user ports can lack phylink
> bindings). This is the entire essence of what this discussion should capture.

Understood.

> 
>>
>> So I'd like to keep the mdio property valid for the switches which their
>> drivers can only register non-OF-based ds->slave_mii_bus.
>>
>> In conclusion, what to do:
>>
>> - Define "the mdio property" and "the enforcement of phylink bindings for
>>    user ports if mdio property is used" on ethernet-switch.yaml.
>>      - Invalidate the mdio property on the switches without an internal MDIO
>>        bus.
>> - Define "the enforcement of phylink bindings for user ports" on the
>>    switches without an internal MDIO bus.
>> - Require "the mdio property" for the switches which their driver requires
>>    it to function.
>> - Require "the mdio property" and "the compatible string of the mdio
>>    property" for the switches which their driver requires them to function.
>>
>> There's no 1:1 switch to switch compatible string relation, as seen on
>> Realtek switches so I'll have to figure that out as I go.
>>
>> I'm open to your comments to this mail but the gap between discussion and
>> end result has widened a lot on this patch series so I'd like to first
>> offload this conversation by preparing v2 with what I said here and discuss
>> further there.
> 
> Honestly, from my side, a verbal comment in the dt-bindings document
> would have been just fine, as long as it is truthful to the reality it
> describes.
> 
> You wanted to over-complicate things with an actual schema validation,
> and then hooking onto things that are unrelated with the phenomenon that
> needs to be captured (like the "mdio" child node, without explicit
> regard to whether it is the ds->slave_mii_bus or not).
> 
> It's not about internal MDIO buses in general, it's about whether those
> internal MDIO buses are used in ds->slave_mii_bus, and their OF
> presence/absence! That is absolutely driver-specific and I would only
> expect a driver-specific way of enforcing it. I didn't say it's not
> hard, and I didn't ask to enforce it, either.

OK, I believe we're on the same page now, I will start working on properly
enforcing this.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Luiz Angelo Daros de Luca]

> > [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
> > otherwise the same, right? So why would they have different dt-bindings?
>
> Honestly, I'm wondering the answer to this as well. For some reason, when
> probing the SMI controlled Realtek switches, instead of just letting
> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
> on realtek-smi.c:
>
> - priv->slave_mii_bus is allocated.
> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
> - priv->slave_mii_bus->dev.of_node = mdio_np;
> - ds->slave_mii_bus = priv->slave_mii_bus;

I might be able to help here. The Realtek SMI version created a custom
slave_mii driver because it was the only way to associate it with an
MDIO DT node. And that DT node was required to specify the interrupts
for each phy0.
It would work without that mdio node, letting DSA setup handle the
slave bus, but it would rely only on polling for port status.

As we only have a single internal MDIO, the compatible string
"realtek,smi-mdio" would not be necessary if the driver checks for a
"mdio"-named child node. Maybe the code was just inspired by another
DSA driver that uses more MDIO buses or external ones. The "mdio" name
is suggested by docs since it was committed
(https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt).
That name was also kept in the YAML translation
(https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek.yaml).

The Realtek MDIO driver was merged at the same release that included
the change that allows dsa_switch_setup() to reference the "mdio"
OF-node if present. That way, it could avoid creating a custom
slave_mii_bus driver.

I submitted a small series of patches to unify that behavior between
those two drivers:

https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/
(This is my answer to the series opening message to include the first
paragraph ate by the editor)

There was some discussion but not NAC, ACK or RFC. It would have
dropped some lines of code. I can revive it if there is interest.

Regards,

Luiz

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 5.09.2023 05:42, Luiz Angelo Daros de Luca wrote:
>>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
>>> otherwise the same, right? So why would they have different dt-bindings?
>>
>> Honestly, I'm wondering the answer to this as well. For some reason, when
>> probing the SMI controlled Realtek switches, instead of just letting
>> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
>> on realtek-smi.c:
>>
>> - priv->slave_mii_bus is allocated.
>> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
>> - priv->slave_mii_bus->dev.of_node = mdio_np;
>> - ds->slave_mii_bus = priv->slave_mii_bus;
> 
> I might be able to help here. The Realtek SMI version created a custom
> slave_mii driver because it was the only way to associate it with an
> MDIO DT node. And that DT node was required to specify the interrupts
> for each phy0.
> It would work without that mdio node, letting DSA setup handle the
> slave bus, but it would rely only on polling for port status.
> 
> As we only have a single internal MDIO, the compatible string
> "realtek,smi-mdio" would not be necessary if the driver checks for a
> "mdio"-named child node. Maybe the code was just inspired by another
> DSA driver that uses more MDIO buses or external ones. The "mdio" name
> is suggested by docs since it was committed
> (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt).
> That name was also kept in the YAML translation
> (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek.yaml).
> 
> The Realtek MDIO driver was merged at the same release that included
> the change that allows dsa_switch_setup() to reference the "mdio"
> OF-node if present. That way, it could avoid creating a custom
> slave_mii_bus driver.
> 
> I submitted a small series of patches to unify that behavior between
> those two drivers:
> 
> https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/
> (This is my answer to the series opening message to include the first
> paragraph ate by the editor)
> 
> There was some discussion but not NAC, ACK or RFC. It would have
> dropped some lines of code. I can revive it if there is interest.

I'd like this to happen, thanks Luiz!

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 11:42:19PM -0300, Luiz Angelo Daros de Luca wrote:
> I submitted a small series of patches to unify that behavior between
> those two drivers:
> 
> https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/
> (This is my answer to the series opening message to include the first
> paragraph ate by the editor)
> 
> There was some discussion but not NAC, ACK or RFC. It would have
> dropped some lines of code. I can revive it if there is interest.

There was no Ack or Nack from me because I didn't manage to understand
what bothers you if the unified dt-binding has a "compatible" string for
the "mdio" node, which the driver simply ignores.
https://lore.kernel.org/netdev/20220706152923.mhc7vw7xkr7xkot4@skbuf/

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 11:42:19PM -0300, Luiz Angelo Daros de Luca wrote:
> > > [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
> > > otherwise the same, right? So why would they have different dt-bindings?
> >
> > Honestly, I'm wondering the answer to this as well. For some reason, when
> > probing the SMI controlled Realtek switches, instead of just letting
> > dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
> > on realtek-smi.c:
> >
> > - priv->slave_mii_bus is allocated.
> > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
> > - priv->slave_mii_bus->dev.of_node = mdio_np;
> > - ds->slave_mii_bus = priv->slave_mii_bus;
> 
> I might be able to help here. The Realtek SMI version created a custom
> slave_mii driver because it was the only way to associate it with an
> MDIO DT node. And that DT node was required to specify the interrupts
> for each phy0.
> It would work without that mdio node, letting DSA setup handle the
> slave bus, but it would rely only on polling for port status.

It is possible to set up PHY IRQs even if the MDIO bus is not OF-based.
I think that mv88e6xxx_g2_irq_mdio_setup() does that (sets bus->irq[]).

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Andrew Lunn]

> It is possible to set up PHY IRQs even if the MDIO bus is not OF-based.
> I think that mv88e6xxx_g2_irq_mdio_setup() does that (sets bus->irq[]).

Yes. It took me a while to realise you could do this, so there is
probably some complexity in mv88e6xxx i might of been able to avoid if
i had discovered this earlier.

	Andrew

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 4.09.2023 14:33, Arınç ÜNAL wrote:
> Hey Vladimir,
> 
> On 27.08.2023 15:12, Vladimir Oltean wrote:
>> Hi Arınç,
>>
>> I am on vacation and I will just reply with some clarification aspects,
>> without having done any further research on the topic since my last reply.
>>
>> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote:
>>> Before I continue commenting, I'd like to state my understanding so we can
>>> make sure we're on the same page. If a driver doesn't use
>>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO
>>> buses. Otherwise the PHYs on these buses couldn't function, and an improper
>>> driver like this would not be on the official Linux source code.
>>
>> A DSA switch port, like any OF-based ethernet-controller which uses
>> phylink, will use one of the phy-handle, fixed-link or managed properties
>> to describe the interface connecting the MAC/MAC-side PCS to the PHY.
>>
>> At its core, ds->slave_mii_bus is nothing more than a mechanism to make
>> sense of device trees where the above 3 phylink properties are not present.
>>
>> It is completely false to say that if a driver doesn't have ds->slave_mii_bus,
>> it must not have an internal MDIO bus. Because you could still describe
>> that internal MDIO bus like below, without making any use of the sole property
>> that makes ds->slave_mii_bus useful from a dt-bindings perspective.
>>
>> ethernet-switch {
>>     ethernet-ports {
>>         port@0 {
>>             reg = <0>;
>>             phy-handle = <&port0_phy>;
>>             phy-mode = "internal";
>>         };
>>     };
>>
>>     mdio {
>>         port0_phy: ethernet-phy@0 {
>>             reg = <0>;
>>         };
>>     };
>> };
>>
>> This is the more universal way of describing the port setup in an
>> OF-based way. There is also the DSA-specific (and old-style, before phylink)
>> way of describing the same thing, which relies on the non-OF-based
>> ds->slave_mii_bus, with bindings that look like this:
>>
>> ethernet-switch {
>>     ethernet-ports {
>>         port@0 {
>>             reg = <0>;
>>         };
>>     };
>> };
>>
>> But, I would say that the first variant of the binding is preferable,
>> since it is more universal.
>>
>> Not all switches that have an internal MDIO bus support the second
>> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't).
>> But, they support the same configuration through the first form.
> 
> Understood.
> 
>>
>> Furthermore, on the U-Boot mailing lists, I have been suggesting that
>> the DM_DSA driver for mv88e6xxx should not bother to support the second
>> version of the binding, since it is just more code to be added to handle
>> a case which can already be described with the more universal first binding.
> 
> That makes sense.
> 
>>
>>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
>>> with a switch component. Since the switch component is not designed to be a
>>> standalone IC, the MDIO bus of the SoC could just as well be used without
>>> the need to design an MDIO controller specific to the switch component, to
>>> manage the PHYs. So I see this switch as just another case of a switch
>>> without an internal MDIO bus.
>>
>> Well, we need to clarify the semantics of an "internal" MDIO bus.
>>
>> I would say most discrete chips with DSA switches have this SoC-style
>> architecture, with separate address spaces for the switching IP, MDIO
>> bus, GPIO controller, IRQ controllers, temperature sensors etc (see
>> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is
>> controlled over SPIO instead of MMIO). The dt-bindings of most DSA
>> switches may or may not reflect that discrete chip organization. Those
>> drivers and dt-bindings could be reimagined so that DSA is not the
>> top-level driver.
>>
>> Yet, I would argue that it's wrong to say that because it isn't an OF
>> child of the switch, the MDIO bus of the VSC7514 is not internal in the
>> same way that the Realtek MDIO bus is internal. The switch ports are
>> connected to internal PHYs on this MDIO bus, and there aren't PHYs on
>> this MDIO bus that go to other MACs than the switch ports. So, the
>> VSC7514 MDIO bus could legally be called the internal MDIO bus of the
>> switch, even if there isn't a parent/child relationship between them.
> 
> Good point, I had believed that the management interface of all of the PHYs
> being connected to the MDIO bus - which is not part of the switching IP
> address space - would be enough to classify the MDIO bus as non-internal.
> 
> However, the architecture of separate address spaces for the switching IP
> and MDIO bus is used on any type of IC with the switching feature.
> Therefore, this characteristic cannot be used to distinguish whether an
> MDIO bus is of a switch.
> 
> What we can refer to to classify an internal MDIO bus is by confirming the
> data interface of all PHYs on the MDIO bus is connected to the switch port
> MACs, as you have pointed out here.
> 
> Because the architecture of separate address spaces for the switching IP
> and MDIO bus is used on any type of IC with the switching feature, it can
> differ by driver how the MDIO bus is defined on the dt-bindings. So we
> can't make universal bindings of an internal MDIO bus of a switch that
> apply to every switch.
> 
> So, the correct approach is to define things under the switch-specific
> schema which is affine to the driver, as you have already pointed out.
> Which schemas to define what will of course differ.
> 
>>
>> So, what I'm disagreeing with is your insistence to correlate your
>> problem with internal MDIO buses. The way in which the problem is
>> formulated dictates what problem gets solved, and the problem is not
>> correctly formulated here. It is purely about ds->slave_mii_bus and its
>> driver-defined OF presence/absence. It is a DSA-specific binding aspect
>> which not even all DSA switches inherit, let alone bindings outside DSA.
> 
> Got it.
> 
>>
>>>> For switches in the second category, it all depends on the way in which
>>>> the driver finds the node for of_mdiobus_register().
>>>
>>> Ok, so some drivers require the mdio child node. Some require it and the
>>> compatible property with a certain string.
>>>
>>> MDIO controlled Realtek switches do not need the compatible property under
>>> the mdio child node. There're no compatible strings to make a distinction
>>> between the SMI and MDIO controlled switches so the best we can do is keep
>>> it the way it currently is. Define realtek,smi-mdio as a compatible string
>>> but keep the compatible property optional. I did state this on my reply to
>>> patch 3 but still received reviewed-bys regardless.
>>
>> Yes, because.... [1]
>>
>>>> Having identified all switches which make some sort of use of
>>>> ds->slave_mii_bus, the rule would sound like this:
>>>>
>>>> 1. If the schema is that of (need to replace this with compatible
>>>>      strings, I'm too lazy for that):
>>>>
>>>>      - ksz_switch_ops
>>>>      - mv88e6060_switch_ops
>>>>      - lan9303_switch_ops
>>>>      - rtl8365mb_switch_ops_mdio
>>>>      - b53_switch_ops
>>>>      - vsc73xx_ds_ops
>>>>      - mv88e6xxx
>>>>      - qca8k
>>>>
>>>>      and we have an "mdio" child, then phylink bindings are mandatory on user ports.
>>>>
>>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
>>>>      then phylink bindings are mandatory on user ports (I haven't checked,
>>>>      but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
>>>>      in that case, this goes to category 4 below).
>>>>
>>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
>>>>      "realtek,smi-mdio", then phylink bindings are mandatory on user ports
>>>>      (same comment about the child MDIO note maybe being mandatory).
>>>>
>>>> 4. If the switch didn't appear in the above set of rules, then phylink
>>>>      bindings are unconditionally mandatory on user ports.
>>>>
>>>> We don't care at all what the drivers that don't use ds->slave_mii_bus
>>>> do with the "mdio" child node. It doesn't change the fact that their
>>>> user ports can't have missing phylink bindings.
>>>
>>> I partially agree. I say, for the switches without an internal MDIO bus,
>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>> user ports if the mdios property is used.
>>
>> Why "if the mdios property is used" and not "always"? :-/
>>
>> To say it again: because the sja1105 driver does not use ds->slave_mii_bus,
>> it can make no sense of dt-bindings on user ports which lack phylink properties.
>> So they are *always* needed. The "mdios" property changes nothing in that regard.
> 
> Got it.
> 
>>
>>>
>>> I'd like to add this before I conclude. The way I understand dt-bindings is
>>> that a binding does not have to translate to an action on the driver.
>>> Documenting bindings for the sole purpose of describing hardware is a valid
>>> case.
>>
>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
>> otherwise the same, right? So why would they have different dt-bindings?
> 
> Honestly, I'm wondering the answer to this as well. For some reason, when
> probing the SMI controlled Realtek switches, instead of just letting
> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
> on realtek-smi.c:
> 
> - priv->slave_mii_bus is allocated.
> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
> - priv->slave_mii_bus->dev.of_node = mdio_np;
> - ds->slave_mii_bus = priv->slave_mii_bus;
> 
>>
>>> For example, currently, the MT753X DSA subdriver won't, in any way,
>>> register the bus OF-based. Still, the mdio property for the switches which
>>> this driver controls can be documented because the internal mdio bus does
>>> exist on the hardware.
>>
>> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence,
>> then it loses its core functionality (that user ports can lack phylink
>> bindings). This is the entire essence of what this discussion should capture.
> 
> Understood.
> 
>>
>>>
>>> So I'd like to keep the mdio property valid for the switches which their
>>> drivers can only register non-OF-based ds->slave_mii_bus.
>>>
>>> In conclusion, what to do:
>>>
>>> - Define "the mdio property" and "the enforcement of phylink bindings for
>>>    user ports if mdio property is used" on ethernet-switch.yaml.
>>>      - Invalidate the mdio property on the switches without an internal MDIO
>>>        bus.
>>> - Define "the enforcement of phylink bindings for user ports" on the
>>>    switches without an internal MDIO bus.
>>> - Require "the mdio property" for the switches which their driver requires
>>>    it to function.
>>> - Require "the mdio property" and "the compatible string of the mdio
>>>    property" for the switches which their driver requires them to function.
>>>
>>> There's no 1:1 switch to switch compatible string relation, as seen on
>>> Realtek switches so I'll have to figure that out as I go.
>>>
>>> I'm open to your comments to this mail but the gap between discussion and
>>> end result has widened a lot on this patch series so I'd like to first
>>> offload this conversation by preparing v2 with what I said here and discuss
>>> further there.
>>
>> Honestly, from my side, a verbal comment in the dt-bindings document
>> would have been just fine, as long as it is truthful to the reality it
>> describes.
>>
>> You wanted to over-complicate things with an actual schema validation,
>> and then hooking onto things that are unrelated with the phenomenon that
>> needs to be captured (like the "mdio" child node, without explicit
>> regard to whether it is the ds->slave_mii_bus or not).
>>
>> It's not about internal MDIO buses in general, it's about whether those
>> internal MDIO buses are used in ds->slave_mii_bus, and their OF
>> presence/absence! That is absolutely driver-specific and I would only
>> expect a driver-specific way of enforcing it. I didn't say it's not
>> hard, and I didn't ask to enforce it, either.
> 
> OK, I believe we're on the same page now, I will start working on properly
> enforcing this.

I'm writing below as a mix of patch log and discussion.

Phylink bindings are required for ports that are controlled by OF-based
buses. DSA, like any other driver utilising the Linux MDIO infrastructure,
can register a bus. If I understand correctly, non-OF-based registration of
OpenFirmware MDIO buses is a feature specific to DSA which certain DSA
subdrivers make use of.

There's no way to distinguish which port is controlled by which driver's
MDIO bus on the bindings so we can't enforce phylink bindings for all user
ports as this would also enforce phylink bindings on user ports controlled
by a non-OF-based bus.

But we can know when DSA won't create a non-OF-based bus. That leaves us
with only OF-based buses in which case we can enforce phylink bindings for
all user ports. So we need to check each DSA subdriver to see when all
buses will be OF-based.

A DSA subdriver can either let the main DSA driver register the bus or it
can register a bus or buses itself.

The attributes of a DSA subdriver that lets the DSA driver register the
bus:
- ds->ops->phy_read() and ds->ops->phy_write() are present.
- ds->slave_mii_bus is not populated by the DSA subdriver.
- The bus is registered non-OF-based or OF-based. Registered OF-based if
   "mdio" child node is defined.

How each DSA subdriver behaves is documented below.

---

- mscc,vsc7514-switch.yaml
   - mscc,vsc7514-switch
   - mscc,vsc7512-switch

drivers/net/ethernet/mscc/ocelot_vsc7514.c:
- Not a DSA subdriver.
- MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
   to register a bus.

drivers/net/dsa/ocelot/ocelot_ext.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
   to register a bus.

What to do:
- For mscc,vsc7514-switch, enforce phylink bindings for ports.
- For mscc,vsc7512-switch, enforce phylink bindings for user ports.

---

- renesas,rzn1-a5psw.yaml
   - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw

drivers/net/dsa/rzn1_a5psw.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers the bus OF-based only. Registers the bus when "mdio" child node
   is defined.
   -	mdio = of_get_child_by_name(dev->of_node, "mdio");
     if (of_device_is_available(mdio))
       ret = a5psw_probe_mdio(a5psw, mdio);

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports.

---

- realtek.yaml
   - realtek,rtl8365mb
   - realtek,rtl8366rb

drivers/net/dsa/realtek/realtek-smi.c:
- The DSA subdriver won't let the DSA driver register the bus. Registers
   the bus OF-based only. Registering the bus is mandatory. Registers the
   bus when compatible string "realtek,smi-mdio" on a child node is defined.
   - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
     if (!mdio_np)
       return -ENODEV;
     ds->slave_mii_bus = priv->slave_mii_bus;

drivers/net/dsa/realtek/realtek-mdio.c:
- The DSA subdriver lets the DSA driver register the bus.

What to do:
- Document "mdio".
   - Require "mdio". (Can't do because it's not required for MDIO controlled
     switches that share the compatible string with SMI controlled switches.
     This is why I would like Luiz to unify the bus registeration process.)
- Document compatible string "realtek,smi-mdio" on "mdio" child node.
   - Require compatible. (Can't do because the same as above.)
- Enforce phylink bindings for user ports. (Can't do because the same as
   above.)
   - Enforce phylink bindings for user ports if "mdio" is defined.

---

- qca8k.yaml
   - qca,qca8327
   - qca,qca8328
   - qca,qca8334
   - qca,qca8337

drivers/net/dsa/qca/qca8k-8xxx.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
   child node is defined.
   - mdio = of_get_child_by_name(priv->dev->of_node, "mdio");

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- nxp,sja1105.yaml
   - nxp,sja1105e
   - nxp,sja1105t
   - nxp,sja1105p
   - nxp,sja1105q
   - nxp,sja1105r
   - nxp,sja1105s
   - nxp,sja1110a
   - nxp,sja1110b
   - nxp,sja1110c
   - nxp,sja1110d

drivers/net/dsa/sja1105/sja1105_mdio.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers multiple buses OF-based only. Registers the buses when "mdios"
   child node and "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio"
   compatible strings per child node under "mdios" is defined.
   - mdio_node = of_get_child_by_name(switch_node, "mdios");
     if (!mdio_node)
       return 0;
   - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio");
     if (!np)
       return 0;
   - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio");
     if (!np)
       return 0;

What to do:
- Document "mdios".
   - Document child node pattern property under "mdios".
     - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio"
       compatible strings.
- Enforce phylink bindings for user ports.

---

- mscc,ocelot.yaml
   - mscc,vsc9953-switch
   - pci1957,eef0

drivers/net/dsa/ocelot/seville_vsc9953.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
   to register a bus.

drivers/net/dsa/ocelot/felix_vsc9959.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- FSL_ENETC_MDIO, a driver utilising the Linux MDIO infrastructure is used
   to register a bus.

What to do:
- Enforce phylink bindings for user ports.

---

- microchip,lan937x.yaml
   - microchip,lan9370
   - microchip,lan9371
   - microchip,lan9372
   - microchip,lan9373
   - microchip,lan9374
- microchip,ksz.yaml
   - microchip,ksz8765
   - microchip,ksz8794
   - microchip,ksz8795
   - microchip,ksz8863
   - microchip,ksz8873
   - microchip,ksz9477
   - microchip,ksz9897
   - microchip,ksz9896
   - microchip,ksz9567
   - microchip,ksz8565
   - microchip,ksz9893
   - microchip,ksz9563
   - microchip,ksz8563

drivers/net/dsa/microchip/ksz_common.c:
- The DSA subdriver won't let the DSA driver register the bus when "mdio"
   child node is defined. Registers the bus OF-based only. Registers the bus
   when "mdio" child node is defined.
   - mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
     if (!mdio_np)
       return 0;
     ds->slave_mii_bus = bus;

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- hirschmann,hellcreek.yaml
   - hirschmann,hellcreek-de1soc-r1

drivers/net/dsa/hirschmann/hellcreek.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().

What to do:
- Enforce phylink bindings for user ports.

---

- mediatek,mt7530.yaml
   - mediatek,mt7530
   - mediatek,mt7531
   - mediatek,mt7621
   - mediatek,mt7988-switch

drivers/net/dsa/mt7530.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
   child node is defined. (This is after the patch for the MT7530 DSA
   subdriver is applied.)

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- brcm,sf2.yaml
   - brcm,bcm4908-switch
   - brcm,bcm7278-switch-v4.0
   - brcm,bcm7278-switch-v4.8
   - brcm,bcm7445-switch-v4.0

drivers/net/dsa/bcm_sf2.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Requires MDIO_BCM_UNIMAC, a driver utilising the Linux MDIO
   infrastructure to register a bus.
   - dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
     priv->master_mii_bus = of_mdio_find_bus(dn);
     if (!priv->master_mii_bus) {
       of_node_put(dn);
       return -EPROBE_DEFER;
     }

What to do:
- Enforce phylink bindings for user ports.

---

- brcm,b53.yaml
   - brcm,bcm5325
   - brcm,bcm53115
   - brcm,bcm53125
   - brcm,bcm53128
   - brcm,bcm53134
   - brcm,bcm5365
   - brcm,bcm5395
   - brcm,bcm5389
   - brcm,bcm5397
   - brcm,bcm5398
   - brcm,bcm11360-srab, brcm,cygnus-srab
   - brcm,bcm53010-srab, brcm,bcm5301x-srab
   - brcm,bcm53011-srab, brcm,bcm5301x-srab
   - brcm,bcm53012-srab, brcm,bcm5301x-srab
   - brcm,bcm53018-srab, brcm,bcm5301x-srab
   - brcm,bcm53019-srab, brcm,bcm5301x-srab
   - brcm,bcm11404-srab, brcm,omega-srab
   - brcm,bcm11407-srab, brcm,omega-srab
   - brcm,bcm11409-srab, brcm,omega-srab
   - brcm,bcm58310-srab, brcm,omega-srab
   - brcm,bcm58311-srab, brcm,omega-srab
   - brcm,bcm58313-srab, brcm,omega-srab
   - brcm,bcm58522-srab, brcm,nsp-srab
   - brcm,bcm58523-srab, brcm,nsp-srab
   - brcm,bcm58525-srab, brcm,nsp-srab
   - brcm,bcm58622-srab, brcm,nsp-srab
   - brcm,bcm58623-srab, brcm,nsp-srab
   - brcm,bcm58625-srab, brcm,nsp-srab
   - brcm,bcm88312-srab, brcm,nsp-srab
   - brcm,bcm3384-switch, brcm,bcm63xx-switch
   - brcm,bcm6318-switch, brcm,bcm63xx-switch
   - brcm,bcm6328-switch, brcm,bcm63xx-switch
   - brcm,bcm6362-switch, brcm,bcm63xx-switch
   - brcm,bcm6368-switch, brcm,bcm63xx-switch
   - brcm,bcm63268-switch, brcm,bcm63xx-switch

drivers/net/dsa/b53/b53_common.c:
- The DSA subdriver lets the DSA driver register the bus.

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- arrow,xrs700x.yaml
   - arrow,xrs7003e
   - arrow,xrs7003f
   - arrow,xrs7004e
   - arrow,xrs7004f

drivers/net/dsa/xrs700x/xrs700x.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().

What to do:
- Enforce phylink bindings for user ports.

---

Text documents. I will handle these when I replace them with json-schema
documents.

- ar9331.txt
   - qca,ar9331-switch

drivers/net/dsa/qca/ar9331.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers the bus OF-based only. Registering the bus is mandatory.
   Registers the bus when "mdio" child node is defined.
   - mnp = of_get_child_by_name(np, "mdio");
     if (!mnp)
       return -ENODEV;

What to do:
- Document "mdio".
   - Require "mdio".
- Enforce phylink bindings for user ports.

---

- lan9303.txt
   - smsc,lan9303-i2c
   - smsc,lan9303-mdio

drivers/net/dsa/lan9303-core.c:
- The DSA subdriver lets the DSA driver register the bus.

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- lantiq-gswip.txt
   - lantiq,xrx200-gswip
   - lantiq,xrx300-gswip
   - lantiq,xrx330-gswip

drivers/net/dsa/lantiq_gswip.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - No ds->ops->phy_read() or ds->ops->phy_write().
- Registers the bus OF-based only. Registers the bus when compatible string
   "lantiq,xrx200-mdio" on a child node is defined.
   - mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio");
     if (mdio_np)
       err = gswip_mdio(priv, mdio_np);

What to do:
- Document "mdio".
- Document compatible string "realtek,smi-mdio" on "mdio" child node.
   - Require compatible.
- Enforce phylink bindings for user ports.

---

- marvell.txt
   - marvell,mv88e6085
   - marvell,mv88e6190
   - marvell,mv88e6250

drivers/net/dsa/mv88e6xxx/chip.c:
- The DSA subdriver won't let the DSA driver register the bus.
   - ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
- Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
   child node is defined.

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

---

- vitesse,vsc73xx.txt
   - vitesse,vsc7385
   - vitesse,vsc7388
   - vitesse,vsc7395
   - vitesse,vsc7398

drivers/net/dsa/vitesse-vsc73xx-core.c:
- The DSA subdriver lets the DSA driver register the bus.

What to do:
- Document "mdio".
- Enforce phylink bindings for user ports if "mdio" is defined.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 9.09.2023 11:53, Arınç ÜNAL wrote:
> On 4.09.2023 14:33, Arınç ÜNAL wrote:
>> Hey Vladimir,
>>
>> On 27.08.2023 15:12, Vladimir Oltean wrote:
>>> Hi Arınç,
>>>
>>> I am on vacation and I will just reply with some clarification aspects,
>>> without having done any further research on the topic since my last reply.
>>>
>>> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote:
>>>> Before I continue commenting, I'd like to state my understanding so we can
>>>> make sure we're on the same page. If a driver doesn't use
>>>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO
>>>> buses. Otherwise the PHYs on these buses couldn't function, and an improper
>>>> driver like this would not be on the official Linux source code.
>>>
>>> A DSA switch port, like any OF-based ethernet-controller which uses
>>> phylink, will use one of the phy-handle, fixed-link or managed properties
>>> to describe the interface connecting the MAC/MAC-side PCS to the PHY.
>>>
>>> At its core, ds->slave_mii_bus is nothing more than a mechanism to make
>>> sense of device trees where the above 3 phylink properties are not present.
>>>
>>> It is completely false to say that if a driver doesn't have ds->slave_mii_bus,
>>> it must not have an internal MDIO bus. Because you could still describe
>>> that internal MDIO bus like below, without making any use of the sole property
>>> that makes ds->slave_mii_bus useful from a dt-bindings perspective.
>>>
>>> ethernet-switch {
>>>     ethernet-ports {
>>>         port@0 {
>>>             reg = <0>;
>>>             phy-handle = <&port0_phy>;
>>>             phy-mode = "internal";
>>>         };
>>>     };
>>>
>>>     mdio {
>>>         port0_phy: ethernet-phy@0 {
>>>             reg = <0>;
>>>         };
>>>     };
>>> };
>>>
>>> This is the more universal way of describing the port setup in an
>>> OF-based way. There is also the DSA-specific (and old-style, before phylink)
>>> way of describing the same thing, which relies on the non-OF-based
>>> ds->slave_mii_bus, with bindings that look like this:
>>>
>>> ethernet-switch {
>>>     ethernet-ports {
>>>         port@0 {
>>>             reg = <0>;
>>>         };
>>>     };
>>> };
>>>
>>> But, I would say that the first variant of the binding is preferable,
>>> since it is more universal.
>>>
>>> Not all switches that have an internal MDIO bus support the second
>>> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't).
>>> But, they support the same configuration through the first form.
>>
>> Understood.
>>
>>>
>>> Furthermore, on the U-Boot mailing lists, I have been suggesting that
>>> the DM_DSA driver for mv88e6xxx should not bother to support the second
>>> version of the binding, since it is just more code to be added to handle
>>> a case which can already be described with the more universal first binding.
>>
>> That makes sense.
>>
>>>
>>>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
>>>> with a switch component. Since the switch component is not designed to be a
>>>> standalone IC, the MDIO bus of the SoC could just as well be used without
>>>> the need to design an MDIO controller specific to the switch component, to
>>>> manage the PHYs. So I see this switch as just another case of a switch
>>>> without an internal MDIO bus.
>>>
>>> Well, we need to clarify the semantics of an "internal" MDIO bus.
>>>
>>> I would say most discrete chips with DSA switches have this SoC-style
>>> architecture, with separate address spaces for the switching IP, MDIO
>>> bus, GPIO controller, IRQ controllers, temperature sensors etc (see
>>> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is
>>> controlled over SPIO instead of MMIO). The dt-bindings of most DSA
>>> switches may or may not reflect that discrete chip organization. Those
>>> drivers and dt-bindings could be reimagined so that DSA is not the
>>> top-level driver.
>>>
>>> Yet, I would argue that it's wrong to say that because it isn't an OF
>>> child of the switch, the MDIO bus of the VSC7514 is not internal in the
>>> same way that the Realtek MDIO bus is internal. The switch ports are
>>> connected to internal PHYs on this MDIO bus, and there aren't PHYs on
>>> this MDIO bus that go to other MACs than the switch ports. So, the
>>> VSC7514 MDIO bus could legally be called the internal MDIO bus of the
>>> switch, even if there isn't a parent/child relationship between them.
>>
>> Good point, I had believed that the management interface of all of the PHYs
>> being connected to the MDIO bus - which is not part of the switching IP
>> address space - would be enough to classify the MDIO bus as non-internal.
>>
>> However, the architecture of separate address spaces for the switching IP
>> and MDIO bus is used on any type of IC with the switching feature.
>> Therefore, this characteristic cannot be used to distinguish whether an
>> MDIO bus is of a switch.
>>
>> What we can refer to to classify an internal MDIO bus is by confirming the
>> data interface of all PHYs on the MDIO bus is connected to the switch port
>> MACs, as you have pointed out here.
>>
>> Because the architecture of separate address spaces for the switching IP
>> and MDIO bus is used on any type of IC with the switching feature, it can
>> differ by driver how the MDIO bus is defined on the dt-bindings. So we
>> can't make universal bindings of an internal MDIO bus of a switch that
>> apply to every switch.
>>
>> So, the correct approach is to define things under the switch-specific
>> schema which is affine to the driver, as you have already pointed out.
>> Which schemas to define what will of course differ.
>>
>>>
>>> So, what I'm disagreeing with is your insistence to correlate your
>>> problem with internal MDIO buses. The way in which the problem is
>>> formulated dictates what problem gets solved, and the problem is not
>>> correctly formulated here. It is purely about ds->slave_mii_bus and its
>>> driver-defined OF presence/absence. It is a DSA-specific binding aspect
>>> which not even all DSA switches inherit, let alone bindings outside DSA.
>>
>> Got it.
>>
>>>
>>>>> For switches in the second category, it all depends on the way in which
>>>>> the driver finds the node for of_mdiobus_register().
>>>>
>>>> Ok, so some drivers require the mdio child node. Some require it and the
>>>> compatible property with a certain string.
>>>>
>>>> MDIO controlled Realtek switches do not need the compatible property under
>>>> the mdio child node. There're no compatible strings to make a distinction
>>>> between the SMI and MDIO controlled switches so the best we can do is keep
>>>> it the way it currently is. Define realtek,smi-mdio as a compatible string
>>>> but keep the compatible property optional. I did state this on my reply to
>>>> patch 3 but still received reviewed-bys regardless.
>>>
>>> Yes, because.... [1]
>>>
>>>>> Having identified all switches which make some sort of use of
>>>>> ds->slave_mii_bus, the rule would sound like this:
>>>>>
>>>>> 1. If the schema is that of (need to replace this with compatible
>>>>>      strings, I'm too lazy for that):
>>>>>
>>>>>      - ksz_switch_ops
>>>>>      - mv88e6060_switch_ops
>>>>>      - lan9303_switch_ops
>>>>>      - rtl8365mb_switch_ops_mdio
>>>>>      - b53_switch_ops
>>>>>      - vsc73xx_ds_ops
>>>>>      - mv88e6xxx
>>>>>      - qca8k
>>>>>
>>>>>      and we have an "mdio" child, then phylink bindings are mandatory on user ports.
>>>>>
>>>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
>>>>>      then phylink bindings are mandatory on user ports (I haven't checked,
>>>>>      but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
>>>>>      in that case, this goes to category 4 below).
>>>>>
>>>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
>>>>>      "realtek,smi-mdio", then phylink bindings are mandatory on user ports
>>>>>      (same comment about the child MDIO note maybe being mandatory).
>>>>>
>>>>> 4. If the switch didn't appear in the above set of rules, then phylink
>>>>>      bindings are unconditionally mandatory on user ports.
>>>>>
>>>>> We don't care at all what the drivers that don't use ds->slave_mii_bus
>>>>> do with the "mdio" child node. It doesn't change the fact that their
>>>>> user ports can't have missing phylink bindings.
>>>>
>>>> I partially agree. I say, for the switches without an internal MDIO bus,
>>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
>>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>>> user ports if the mdios property is used.
>>>
>>> Why "if the mdios property is used" and not "always"? :-/
>>>
>>> To say it again: because the sja1105 driver does not use ds->slave_mii_bus,
>>> it can make no sense of dt-bindings on user ports which lack phylink properties.
>>> So they are *always* needed. The "mdios" property changes nothing in that regard.
>>
>> Got it.
>>
>>>
>>>>
>>>> I'd like to add this before I conclude. The way I understand dt-bindings is
>>>> that a binding does not have to translate to an action on the driver.
>>>> Documenting bindings for the sole purpose of describing hardware is a valid
>>>> case.
>>>
>>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
>>> otherwise the same, right? So why would they have different dt-bindings?
>>
>> Honestly, I'm wondering the answer to this as well. For some reason, when
>> probing the SMI controlled Realtek switches, instead of just letting
>> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
>> on realtek-smi.c:
>>
>> - priv->slave_mii_bus is allocated.
>> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
>> - priv->slave_mii_bus->dev.of_node = mdio_np;
>> - ds->slave_mii_bus = priv->slave_mii_bus;
>>
>>>
>>>> For example, currently, the MT753X DSA subdriver won't, in any way,
>>>> register the bus OF-based. Still, the mdio property for the switches which
>>>> this driver controls can be documented because the internal mdio bus does
>>>> exist on the hardware.
>>>
>>> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence,
>>> then it loses its core functionality (that user ports can lack phylink
>>> bindings). This is the entire essence of what this discussion should capture.
>>
>> Understood.
>>
>>>
>>>>
>>>> So I'd like to keep the mdio property valid for the switches which their
>>>> drivers can only register non-OF-based ds->slave_mii_bus.
>>>>
>>>> In conclusion, what to do:
>>>>
>>>> - Define "the mdio property" and "the enforcement of phylink bindings for
>>>>    user ports if mdio property is used" on ethernet-switch.yaml.
>>>>      - Invalidate the mdio property on the switches without an internal MDIO
>>>>        bus.
>>>> - Define "the enforcement of phylink bindings for user ports" on the
>>>>    switches without an internal MDIO bus.
>>>> - Require "the mdio property" for the switches which their driver requires
>>>>    it to function.
>>>> - Require "the mdio property" and "the compatible string of the mdio
>>>>    property" for the switches which their driver requires them to function.
>>>>
>>>> There's no 1:1 switch to switch compatible string relation, as seen on
>>>> Realtek switches so I'll have to figure that out as I go.
>>>>
>>>> I'm open to your comments to this mail but the gap between discussion and
>>>> end result has widened a lot on this patch series so I'd like to first
>>>> offload this conversation by preparing v2 with what I said here and discuss
>>>> further there.
>>>
>>> Honestly, from my side, a verbal comment in the dt-bindings document
>>> would have been just fine, as long as it is truthful to the reality it
>>> describes.
>>>
>>> You wanted to over-complicate things with an actual schema validation,
>>> and then hooking onto things that are unrelated with the phenomenon that
>>> needs to be captured (like the "mdio" child node, without explicit
>>> regard to whether it is the ds->slave_mii_bus or not).
>>>
>>> It's not about internal MDIO buses in general, it's about whether those
>>> internal MDIO buses are used in ds->slave_mii_bus, and their OF
>>> presence/absence! That is absolutely driver-specific and I would only
>>> expect a driver-specific way of enforcing it. I didn't say it's not
>>> hard, and I didn't ask to enforce it, either.
>>
>> OK, I believe we're on the same page now, I will start working on properly
>> enforcing this.
> 
> I'm writing below as a mix of patch log and discussion.
> 
> Phylink bindings are required for ports that are controlled by OF-based
> buses. DSA, like any other driver utilising the Linux MDIO infrastructure,
> can register a bus. If I understand correctly, non-OF-based registration of
> OpenFirmware MDIO buses is a feature specific to DSA which certain DSA
> subdrivers make use of.
> 
> There's no way to distinguish which port is controlled by which driver's
> MDIO bus on the bindings so we can't enforce phylink bindings for all user
> ports as this would also enforce phylink bindings on user ports controlled
> by a non-OF-based bus.
> 
> But we can know when DSA won't create a non-OF-based bus. That leaves us
> with only OF-based buses in which case we can enforce phylink bindings for
> all user ports. So we need to check each DSA subdriver to see when all
> buses will be OF-based.
> 
> A DSA subdriver can either let the main DSA driver register the bus or it
> can register a bus or buses itself.
> 
> The attributes of a DSA subdriver that lets the DSA driver register the
> bus:
> - ds->ops->phy_read() and ds->ops->phy_write() are present.
> - ds->slave_mii_bus is not populated by the DSA subdriver.
> - The bus is registered non-OF-based or OF-based. Registered OF-based if
>    "mdio" child node is defined.
> 
> How each DSA subdriver behaves is documented below.
> 
> ---
> 
> - mscc,vsc7514-switch.yaml
>    - mscc,vsc7514-switch
>    - mscc,vsc7512-switch
> 
> drivers/net/ethernet/mscc/ocelot_vsc7514.c:
> - Not a DSA subdriver.
> - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
>    to register a bus.
> 
> drivers/net/dsa/ocelot/ocelot_ext.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
>    to register a bus.
> 
> What to do:
> - For mscc,vsc7514-switch, enforce phylink bindings for ports.
> - For mscc,vsc7512-switch, enforce phylink bindings for user ports.
> 
> ---
> 
> - renesas,rzn1-a5psw.yaml
>    - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw
> 
> drivers/net/dsa/rzn1_a5psw.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus OF-based only. Registers the bus when "mdio" child node
>    is defined.
>    -    mdio = of_get_child_by_name(dev->of_node, "mdio");
>      if (of_device_is_available(mdio))
>        ret = a5psw_probe_mdio(a5psw, mdio);
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - realtek.yaml
>    - realtek,rtl8365mb
>    - realtek,rtl8366rb
> 
> drivers/net/dsa/realtek/realtek-smi.c:
> - The DSA subdriver won't let the DSA driver register the bus. Registers
>    the bus OF-based only. Registering the bus is mandatory. Registers the
>    bus when compatible string "realtek,smi-mdio" on a child node is defined.
>    - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
>      if (!mdio_np)
>        return -ENODEV;
>      ds->slave_mii_bus = priv->slave_mii_bus;
> 
> drivers/net/dsa/realtek/realtek-mdio.c:
> - The DSA subdriver lets the DSA driver register the bus.
> 
> What to do:
> - Document "mdio".
>    - Require "mdio". (Can't do because it's not required for MDIO controlled
>      switches that share the compatible string with SMI controlled switches.
>      This is why I would like Luiz to unify the bus registeration process.)
> - Document compatible string "realtek,smi-mdio" on "mdio" child node.
>    - Require compatible. (Can't do because the same as above.)
> - Enforce phylink bindings for user ports. (Can't do because the same as
>    above.)
>    - Enforce phylink bindings for user ports if "mdio" is defined.

I rediscovered that the reg property can be used to distinguish the SMI and
MDIO controlled Realtek switches. So I can indeed write a proper schema.
I'd still like the bus registration to be unified to eliminate unnecessary
complexity.

> 
> ---
> 
> - qca8k.yaml
>    - qca,qca8327
>    - qca,qca8328
>    - qca,qca8334
>    - qca,qca8337
> 
> drivers/net/dsa/qca/qca8k-8xxx.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
>    child node is defined.
>    - mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - nxp,sja1105.yaml
>    - nxp,sja1105e
>    - nxp,sja1105t
>    - nxp,sja1105p
>    - nxp,sja1105q
>    - nxp,sja1105r
>    - nxp,sja1105s
>    - nxp,sja1110a
>    - nxp,sja1110b
>    - nxp,sja1110c
>    - nxp,sja1110d
> 
> drivers/net/dsa/sja1105/sja1105_mdio.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers multiple buses OF-based only. Registers the buses when "mdios"
>    child node and "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio"
>    compatible strings per child node under "mdios" is defined.
>    - mdio_node = of_get_child_by_name(switch_node, "mdios");
>      if (!mdio_node)
>        return 0;
>    - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio");
>      if (!np)
>        return 0;
>    - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio");
>      if (!np)
>        return 0;
> 
> What to do:
> - Document "mdios".
>    - Document child node pattern property under "mdios".
>      - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio"
>        compatible strings.
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - mscc,ocelot.yaml
>    - mscc,vsc9953-switch
>    - pci1957,eef0
> 
> drivers/net/dsa/ocelot/seville_vsc9953.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used
>    to register a bus.
> 
> drivers/net/dsa/ocelot/felix_vsc9959.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - FSL_ENETC_MDIO, a driver utilising the Linux MDIO infrastructure is used
>    to register a bus.
> 
> What to do:
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - microchip,lan937x.yaml
>    - microchip,lan9370
>    - microchip,lan9371
>    - microchip,lan9372
>    - microchip,lan9373
>    - microchip,lan9374
> - microchip,ksz.yaml
>    - microchip,ksz8765
>    - microchip,ksz8794
>    - microchip,ksz8795
>    - microchip,ksz8863
>    - microchip,ksz8873
>    - microchip,ksz9477
>    - microchip,ksz9897
>    - microchip,ksz9896
>    - microchip,ksz9567
>    - microchip,ksz8565
>    - microchip,ksz9893
>    - microchip,ksz9563
>    - microchip,ksz8563
> 
> drivers/net/dsa/microchip/ksz_common.c:
> - The DSA subdriver won't let the DSA driver register the bus when "mdio"
>    child node is defined. Registers the bus OF-based only. Registers the bus
>    when "mdio" child node is defined.
>    - mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
>      if (!mdio_np)
>        return 0;
>      ds->slave_mii_bus = bus;
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - hirschmann,hellcreek.yaml
>    - hirschmann,hellcreek-de1soc-r1
> 
> drivers/net/dsa/hirschmann/hellcreek.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> 
> What to do:
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - mediatek,mt7530.yaml
>    - mediatek,mt7530
>    - mediatek,mt7531
>    - mediatek,mt7621
>    - mediatek,mt7988-switch
> 
> drivers/net/dsa/mt7530.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
>    child node is defined. (This is after the patch for the MT7530 DSA
>    subdriver is applied.)
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - brcm,sf2.yaml
>    - brcm,bcm4908-switch
>    - brcm,bcm7278-switch-v4.0
>    - brcm,bcm7278-switch-v4.8
>    - brcm,bcm7445-switch-v4.0
> 
> drivers/net/dsa/bcm_sf2.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Requires MDIO_BCM_UNIMAC, a driver utilising the Linux MDIO
>    infrastructure to register a bus.
>    - dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
>      priv->master_mii_bus = of_mdio_find_bus(dn);
>      if (!priv->master_mii_bus) {
>        of_node_put(dn);
>        return -EPROBE_DEFER;
>      }
> 
> What to do:
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - brcm,b53.yaml
>    - brcm,bcm5325
>    - brcm,bcm53115
>    - brcm,bcm53125
>    - brcm,bcm53128
>    - brcm,bcm53134
>    - brcm,bcm5365
>    - brcm,bcm5395
>    - brcm,bcm5389
>    - brcm,bcm5397
>    - brcm,bcm5398
>    - brcm,bcm11360-srab, brcm,cygnus-srab
>    - brcm,bcm53010-srab, brcm,bcm5301x-srab
>    - brcm,bcm53011-srab, brcm,bcm5301x-srab
>    - brcm,bcm53012-srab, brcm,bcm5301x-srab
>    - brcm,bcm53018-srab, brcm,bcm5301x-srab
>    - brcm,bcm53019-srab, brcm,bcm5301x-srab
>    - brcm,bcm11404-srab, brcm,omega-srab
>    - brcm,bcm11407-srab, brcm,omega-srab
>    - brcm,bcm11409-srab, brcm,omega-srab
>    - brcm,bcm58310-srab, brcm,omega-srab
>    - brcm,bcm58311-srab, brcm,omega-srab
>    - brcm,bcm58313-srab, brcm,omega-srab
>    - brcm,bcm58522-srab, brcm,nsp-srab
>    - brcm,bcm58523-srab, brcm,nsp-srab
>    - brcm,bcm58525-srab, brcm,nsp-srab
>    - brcm,bcm58622-srab, brcm,nsp-srab
>    - brcm,bcm58623-srab, brcm,nsp-srab
>    - brcm,bcm58625-srab, brcm,nsp-srab
>    - brcm,bcm88312-srab, brcm,nsp-srab
>    - brcm,bcm3384-switch, brcm,bcm63xx-switch
>    - brcm,bcm6318-switch, brcm,bcm63xx-switch
>    - brcm,bcm6328-switch, brcm,bcm63xx-switch
>    - brcm,bcm6362-switch, brcm,bcm63xx-switch
>    - brcm,bcm6368-switch, brcm,bcm63xx-switch
>    - brcm,bcm63268-switch, brcm,bcm63xx-switch
> 
> drivers/net/dsa/b53/b53_common.c:
> - The DSA subdriver lets the DSA driver register the bus.
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - arrow,xrs700x.yaml
>    - arrow,xrs7003e
>    - arrow,xrs7003f
>    - arrow,xrs7004e
>    - arrow,xrs7004f
> 
> drivers/net/dsa/xrs700x/xrs700x.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> 
> What to do:
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> Text documents. I will handle these when I replace them with json-schema
> documents.
> 
> - ar9331.txt
>    - qca,ar9331-switch
> 
> drivers/net/dsa/qca/ar9331.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus OF-based only. Registering the bus is mandatory.
>    Registers the bus when "mdio" child node is defined.
>    - mnp = of_get_child_by_name(np, "mdio");
>      if (!mnp)
>        return -ENODEV;
> 
> What to do:
> - Document "mdio".
>    - Require "mdio".
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - lan9303.txt
>    - smsc,lan9303-i2c
>    - smsc,lan9303-mdio
> 
> drivers/net/dsa/lan9303-core.c:
> - The DSA subdriver lets the DSA driver register the bus.
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - lantiq-gswip.txt
>    - lantiq,xrx200-gswip
>    - lantiq,xrx300-gswip
>    - lantiq,xrx330-gswip
> 
> drivers/net/dsa/lantiq_gswip.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus OF-based only. Registers the bus when compatible string
>    "lantiq,xrx200-mdio" on a child node is defined.
>    - mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio");
>      if (mdio_np)
>        err = gswip_mdio(priv, mdio_np);
> 
> What to do:
> - Document "mdio".
> - Document compatible string "realtek,smi-mdio" on "mdio" child node.

This should be "lantiq,xrx200-mdio" instead.

Arınç

>    - Require compatible.
> - Enforce phylink bindings for user ports.
> 
> ---
> 
> - marvell.txt
>    - marvell,mv88e6085
>    - marvell,mv88e6190
>    - marvell,mv88e6250
> 
> drivers/net/dsa/mv88e6xxx/chip.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>    - ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
> - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
>    child node is defined.
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - vitesse,vsc73xx.txt
>    - vitesse,vsc7385
>    - vitesse,vsc7388
>    - vitesse,vsc7395
>    - vitesse,vsc7398
> 
> drivers/net/dsa/vitesse-vsc73xx-core.c:
> - The DSA subdriver lets the DSA driver register the bus.
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 9.09.2023 11:53, Arınç ÜNAL wrote:
> On 4.09.2023 14:33, Arınç ÜNAL wrote:
>> Hey Vladimir,
>>
>> On 27.08.2023 15:12, Vladimir Oltean wrote:
>>> Hi Arınç,
>>>
>>> I am on vacation and I will just reply with some clarification aspects,
>>> without having done any further research on the topic since my last reply.
>>>
>>> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote:
>>>> Before I continue commenting, I'd like to state my understanding so we can
>>>> make sure we're on the same page. If a driver doesn't use
>>>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO
>>>> buses. Otherwise the PHYs on these buses couldn't function, and an improper
>>>> driver like this would not be on the official Linux source code.
>>>
>>> A DSA switch port, like any OF-based ethernet-controller which uses
>>> phylink, will use one of the phy-handle, fixed-link or managed properties
>>> to describe the interface connecting the MAC/MAC-side PCS to the PHY.
>>>
>>> At its core, ds->slave_mii_bus is nothing more than a mechanism to make
>>> sense of device trees where the above 3 phylink properties are not present.
>>>
>>> It is completely false to say that if a driver doesn't have ds->slave_mii_bus,
>>> it must not have an internal MDIO bus. Because you could still describe
>>> that internal MDIO bus like below, without making any use of the sole property
>>> that makes ds->slave_mii_bus useful from a dt-bindings perspective.
>>>
>>> ethernet-switch {
>>>     ethernet-ports {
>>>         port@0 {
>>>             reg = <0>;
>>>             phy-handle = <&port0_phy>;
>>>             phy-mode = "internal";
>>>         };
>>>     };
>>>
>>>     mdio {
>>>         port0_phy: ethernet-phy@0 {
>>>             reg = <0>;
>>>         };
>>>     };
>>> };
>>>
>>> This is the more universal way of describing the port setup in an
>>> OF-based way. There is also the DSA-specific (and old-style, before phylink)
>>> way of describing the same thing, which relies on the non-OF-based
>>> ds->slave_mii_bus, with bindings that look like this:
>>>
>>> ethernet-switch {
>>>     ethernet-ports {
>>>         port@0 {
>>>             reg = <0>;
>>>         };
>>>     };
>>> };
>>>
>>> But, I would say that the first variant of the binding is preferable,
>>> since it is more universal.
>>>
>>> Not all switches that have an internal MDIO bus support the second
>>> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't).
>>> But, they support the same configuration through the first form.
>>
>> Understood.
>>
>>>
>>> Furthermore, on the U-Boot mailing lists, I have been suggesting that
>>> the DM_DSA driver for mv88e6xxx should not bother to support the second
>>> version of the binding, since it is just more code to be added to handle
>>> a case which can already be described with the more universal first binding.
>>
>> That makes sense.
>>
>>>
>>>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC
>>>> with a switch component. Since the switch component is not designed to be a
>>>> standalone IC, the MDIO bus of the SoC could just as well be used without
>>>> the need to design an MDIO controller specific to the switch component, to
>>>> manage the PHYs. So I see this switch as just another case of a switch
>>>> without an internal MDIO bus.
>>>
>>> Well, we need to clarify the semantics of an "internal" MDIO bus.
>>>
>>> I would say most discrete chips with DSA switches have this SoC-style
>>> architecture, with separate address spaces for the switching IP, MDIO
>>> bus, GPIO controller, IRQ controllers, temperature sensors etc (see
>>> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is
>>> controlled over SPIO instead of MMIO). The dt-bindings of most DSA
>>> switches may or may not reflect that discrete chip organization. Those
>>> drivers and dt-bindings could be reimagined so that DSA is not the
>>> top-level driver.
>>>
>>> Yet, I would argue that it's wrong to say that because it isn't an OF
>>> child of the switch, the MDIO bus of the VSC7514 is not internal in the
>>> same way that the Realtek MDIO bus is internal. The switch ports are
>>> connected to internal PHYs on this MDIO bus, and there aren't PHYs on
>>> this MDIO bus that go to other MACs than the switch ports. So, the
>>> VSC7514 MDIO bus could legally be called the internal MDIO bus of the
>>> switch, even if there isn't a parent/child relationship between them.
>>
>> Good point, I had believed that the management interface of all of the PHYs
>> being connected to the MDIO bus - which is not part of the switching IP
>> address space - would be enough to classify the MDIO bus as non-internal.
>>
>> However, the architecture of separate address spaces for the switching IP
>> and MDIO bus is used on any type of IC with the switching feature.
>> Therefore, this characteristic cannot be used to distinguish whether an
>> MDIO bus is of a switch.
>>
>> What we can refer to to classify an internal MDIO bus is by confirming the
>> data interface of all PHYs on the MDIO bus is connected to the switch port
>> MACs, as you have pointed out here.
>>
>> Because the architecture of separate address spaces for the switching IP
>> and MDIO bus is used on any type of IC with the switching feature, it can
>> differ by driver how the MDIO bus is defined on the dt-bindings. So we
>> can't make universal bindings of an internal MDIO bus of a switch that
>> apply to every switch.
>>
>> So, the correct approach is to define things under the switch-specific
>> schema which is affine to the driver, as you have already pointed out.
>> Which schemas to define what will of course differ.
>>
>>>
>>> So, what I'm disagreeing with is your insistence to correlate your
>>> problem with internal MDIO buses. The way in which the problem is
>>> formulated dictates what problem gets solved, and the problem is not
>>> correctly formulated here. It is purely about ds->slave_mii_bus and its
>>> driver-defined OF presence/absence. It is a DSA-specific binding aspect
>>> which not even all DSA switches inherit, let alone bindings outside DSA.
>>
>> Got it.
>>
>>>
>>>>> For switches in the second category, it all depends on the way in which
>>>>> the driver finds the node for of_mdiobus_register().
>>>>
>>>> Ok, so some drivers require the mdio child node. Some require it and the
>>>> compatible property with a certain string.
>>>>
>>>> MDIO controlled Realtek switches do not need the compatible property under
>>>> the mdio child node. There're no compatible strings to make a distinction
>>>> between the SMI and MDIO controlled switches so the best we can do is keep
>>>> it the way it currently is. Define realtek,smi-mdio as a compatible string
>>>> but keep the compatible property optional. I did state this on my reply to
>>>> patch 3 but still received reviewed-bys regardless.
>>>
>>> Yes, because.... [1]
>>>
>>>>> Having identified all switches which make some sort of use of
>>>>> ds->slave_mii_bus, the rule would sound like this:
>>>>>
>>>>> 1. If the schema is that of (need to replace this with compatible
>>>>>      strings, I'm too lazy for that):
>>>>>
>>>>>      - ksz_switch_ops
>>>>>      - mv88e6060_switch_ops
>>>>>      - lan9303_switch_ops
>>>>>      - rtl8365mb_switch_ops_mdio
>>>>>      - b53_switch_ops
>>>>>      - vsc73xx_ds_ops
>>>>>      - mv88e6xxx
>>>>>      - qca8k
>>>>>
>>>>>      and we have an "mdio" child, then phylink bindings are mandatory on user ports.
>>>>>
>>>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
>>>>>      then phylink bindings are mandatory on user ports (I haven't checked,
>>>>>      but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
>>>>>      in that case, this goes to category 4 below).
>>>>>
>>>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
>>>>>      "realtek,smi-mdio", then phylink bindings are mandatory on user ports
>>>>>      (same comment about the child MDIO note maybe being mandatory).
>>>>>
>>>>> 4. If the switch didn't appear in the above set of rules, then phylink
>>>>>      bindings are unconditionally mandatory on user ports.
>>>>>
>>>>> We don't care at all what the drivers that don't use ds->slave_mii_bus
>>>>> do with the "mdio" child node. It doesn't change the fact that their
>>>>> user ports can't have missing phylink bindings.
>>>>
>>>> I partially agree. I say, for the switches without an internal MDIO bus,
>>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x,
>>>> invalidate the mdio child node, and enforce the phylink bindings on the
>>>> user ports if the mdios property is used.
>>>
>>> Why "if the mdios property is used" and not "always"? :-/
>>>
>>> To say it again: because the sja1105 driver does not use ds->slave_mii_bus,
>>> it can make no sense of dt-bindings on user ports which lack phylink properties.
>>> So they are *always* needed. The "mdios" property changes nothing in that regard.
>>
>> Got it.
>>
>>>
>>>>
>>>> I'd like to add this before I conclude. The way I understand dt-bindings is
>>>> that a binding does not have to translate to an action on the driver.
>>>> Documenting bindings for the sole purpose of describing hardware is a valid
>>>> case.
>>>
>>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are
>>> otherwise the same, right? So why would they have different dt-bindings?
>>
>> Honestly, I'm wondering the answer to this as well. For some reason, when
>> probing the SMI controlled Realtek switches, instead of just letting
>> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio()
>> on realtek-smi.c:
>>
>> - priv->slave_mii_bus is allocated.
>> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
>> - priv->slave_mii_bus->dev.of_node = mdio_np;
>> - ds->slave_mii_bus = priv->slave_mii_bus;
>>
>>>
>>>> For example, currently, the MT753X DSA subdriver won't, in any way,
>>>> register the bus OF-based. Still, the mdio property for the switches which
>>>> this driver controls can be documented because the internal mdio bus does
>>>> exist on the hardware.
>>>
>>> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence,
>>> then it loses its core functionality (that user ports can lack phylink
>>> bindings). This is the entire essence of what this discussion should capture.
>>
>> Understood.
>>
>>>
>>>>
>>>> So I'd like to keep the mdio property valid for the switches which their
>>>> drivers can only register non-OF-based ds->slave_mii_bus.
>>>>
>>>> In conclusion, what to do:
>>>>
>>>> - Define "the mdio property" and "the enforcement of phylink bindings for
>>>>    user ports if mdio property is used" on ethernet-switch.yaml.
>>>>      - Invalidate the mdio property on the switches without an internal MDIO
>>>>        bus.
>>>> - Define "the enforcement of phylink bindings for user ports" on the
>>>>    switches without an internal MDIO bus.
>>>> - Require "the mdio property" for the switches which their driver requires
>>>>    it to function.
>>>> - Require "the mdio property" and "the compatible string of the mdio
>>>>    property" for the switches which their driver requires them to function.
>>>>
>>>> There's no 1:1 switch to switch compatible string relation, as seen on
>>>> Realtek switches so I'll have to figure that out as I go.
>>>>
>>>> I'm open to your comments to this mail but the gap between discussion and
>>>> end result has widened a lot on this patch series so I'd like to first
>>>> offload this conversation by preparing v2 with what I said here and discuss
>>>> further there.
>>>
>>> Honestly, from my side, a verbal comment in the dt-bindings document
>>> would have been just fine, as long as it is truthful to the reality it
>>> describes.
>>>
>>> You wanted to over-complicate things with an actual schema validation,
>>> and then hooking onto things that are unrelated with the phenomenon that
>>> needs to be captured (like the "mdio" child node, without explicit
>>> regard to whether it is the ds->slave_mii_bus or not).
>>>
>>> It's not about internal MDIO buses in general, it's about whether those
>>> internal MDIO buses are used in ds->slave_mii_bus, and their OF
>>> presence/absence! That is absolutely driver-specific and I would only
>>> expect a driver-specific way of enforcing it. I didn't say it's not
>>> hard, and I didn't ask to enforce it, either.
>>
>> OK, I believe we're on the same page now, I will start working on properly
>> enforcing this.
> 
> I'm writing below as a mix of patch log and discussion.
> 
> Phylink bindings are required for ports that are controlled by OF-based
> buses. DSA, like any other driver utilising the Linux MDIO infrastructure,
> can register a bus. If I understand correctly, non-OF-based registration of
> OpenFirmware MDIO buses is a feature specific to DSA which certain DSA
> subdrivers make use of.
> 
> There's no way to distinguish which port is controlled by which driver's
> MDIO bus on the bindings so we can't enforce phylink bindings for all user
> ports as this would also enforce phylink bindings on user ports controlled
> by a non-OF-based bus.
> 
> But we can know when DSA won't create a non-OF-based bus. That leaves us
> with only OF-based buses in which case we can enforce phylink bindings for
> all user ports. So we need to check each DSA subdriver to see when all
> buses will be OF-based.

We also need to decide the phylink bindings for user ports.

Phylink bindings for CPU and DSA ports:

allOf:
   - required:
       - phy-mode
   - oneOf:
       - required:
           - fixed-link
       - required:
           - phy-handle
       - required:
           - managed

On one of the mscc,ocelot.yaml examples, "phy-handle" and "managed" are
defined on the same user port. Assuming the example is correct, we must
allow more than 1 of these properties to be used at the same time for user
ports.

We need to at least allow "phy-handle" and "managed" to be used at the same
time. Does "managed" also depend on "phy-handle"?

For example:

oneOf:
   - required:
       - fixed-link
   - anyOf:
       - required:
           - phy-handle
       - required:
           - managed

dependencies:
   managed: [ phy-handle ]

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Andrew Lunn]

Please trim the text when replying.


> I'm writing below as a mix of patch log and discussion.
> 
> Phylink bindings are required for ports that are controlled by OF-based
> buses. DSA, like any other driver utilising the Linux MDIO infrastructure,
> can register a bus. If I understand correctly, non-OF-based registration of
> OpenFirmware MDIO buses is a feature specific to DSA which certain DSA
> subdrivers make use of.

This is not really DSA specific. Any MAC driver, or MDIO driver, can
call mdiobus_regsiter(), or of_mdiobus_register() and pass a NULL
pointer if there is no OF node available. It then requires that the
MAC driver uses an function like phy_find_first(), or knows via other
means what address the PHY uses on the bus. For DSA, that other means
is assuming a 1:1 mapping between port number and bus address.

   Andrew

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 10.09.2023 00:16, Andrew Lunn wrote:
> Please trim the text when replying.
> 
> 
>> I'm writing below as a mix of patch log and discussion.
>>
>> Phylink bindings are required for ports that are controlled by OF-based
>> buses. DSA, like any other driver utilising the Linux MDIO infrastructure,
>> can register a bus. If I understand correctly, non-OF-based registration of
>> OpenFirmware MDIO buses is a feature specific to DSA which certain DSA
>> subdrivers make use of.
> 
> This is not really DSA specific. Any MAC driver, or MDIO driver, can
> call mdiobus_regsiter(), or of_mdiobus_register() and pass a NULL
> pointer if there is no OF node available. It then requires that the
> MAC driver uses an function like phy_find_first(), or knows via other
> means what address the PHY uses on the bus. For DSA, that other means
> is assuming a 1:1 mapping between port number and bus address.

Understood. At least a phy-handle on the DSA user port for each PHY 
controlled by non-DSA drivers is always needed correct? Otherwise DSA 
wouldn't know which PHY to map the DSA switch port?

And that means DSA requires OF-based buses to map the ports controlled 
by non-DSA driver buses to PHYs?

I'm trying to understand if phylink bindings for DSA user ports that are 
controlled by non-DSA driver buses are always necessary.

Would this also apply to MAC drivers that control switches?

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote:
> What to do:
> - For mscc,vsc7514-switch, enforce phylink bindings for ports.
> - For mscc,vsc7512-switch, enforce phylink bindings for user ports.

you can also look at dsa_switches_apply_workarounds[], and if the switch
isn't there, then you can replace "user ports" with "ports" here and
everywhere.

> - renesas,rzn1-a5psw.yaml
>   - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw
> 
> What to do:
> - Document "mdio".

Not clear here and for all the schemas quoted below.. is "mdio" not documented already?

> - realtek.yaml
>   - realtek,rtl8365mb
>   - realtek,rtl8366rb
> 
> drivers/net/dsa/realtek/realtek-mdio.c:
> - The DSA subdriver lets the DSA driver register the bus.
> 
> What to do:
> - Document "mdio".
>   - Require "mdio". (Can't do because it's not required for MDIO controlled
>     switches that share the compatible string with SMI controlled switches.
>     This is why I would like Luiz to unify the bus registeration process.)
> - Document compatible string "realtek,smi-mdio" on "mdio" child node.
>   - Require compatible. (Can't do because the same as above.)
> - Enforce phylink bindings for user ports. (Can't do because the same as
>   above.)
>   - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - qca8k.yaml
>   - qca,qca8327
>   - qca,qca8328
>   - qca,qca8334
>   - qca,qca8337
> 
> drivers/net/dsa/qca/qca8k-8xxx.c:
> - The DSA subdriver won't let the DSA driver register the bus.
>   - No ds->ops->phy_read() or ds->ops->phy_write().
> - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio"
>   child node is defined.
>   - mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
> 
> What to do:
> - Document "mdio".
> - Enforce phylink bindings for user ports if "mdio" is defined.
> 
> ---
> 
> - nxp,sja1105.yaml
>   - nxp,sja1105e
>   - nxp,sja1105t
>   - nxp,sja1105p
>   - nxp,sja1105q
>   - nxp,sja1105r
>   - nxp,sja1105s
>   - nxp,sja1110a
>   - nxp,sja1110b
>   - nxp,sja1110c
>   - nxp,sja1110d
> 
> What to do:
> - Document "mdios".
>   - Document child node pattern property under "mdios".
>     - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio"
>       compatible strings.
> ---
> 
> - microchip,lan937x.yaml
>   - microchip,lan9370
>   - microchip,lan9371
>   - microchip,lan9372
>   - microchip,lan9373
>   - microchip,lan9374
> - microchip,ksz.yaml
>   - microchip,ksz8765
>   - microchip,ksz8794
>   - microchip,ksz8795
>   - microchip,ksz8863
>   - microchip,ksz8873
>   - microchip,ksz9477
>   - microchip,ksz9897
>   - microchip,ksz9896
>   - microchip,ksz9567
>   - microchip,ksz8565
>   - microchip,ksz9893
>   - microchip,ksz9563
>   - microchip,ksz8563
> 
> What to do:
> - Document "mdio".

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 12.09.2023 01:51, Vladimir Oltean wrote:
> On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote:
>> What to do:
>> - For mscc,vsc7514-switch, enforce phylink bindings for ports.
>> - For mscc,vsc7512-switch, enforce phylink bindings for user ports.
> 
> you can also look at dsa_switches_apply_workarounds[], and if the switch
> isn't there, then you can replace "user ports" with "ports" here and
> everywhere.

The phylink bindings for user ports I ended up making by looking up the 
existing devicetrees are different than the phylink bindings for the 
shared (CPU and DSA) ports currently enforced on all switches.

My phylink bindings for user ports:

             allOf:
               - anyOf:
                   - required: [ fixed-link ]
                   - required: [ phy-handle ]
                   - required: [ managed ]

               - if:
                   required: [ fixed-link ]
                 then:
                   not:
                     required: [ managed ]

The phylink bindings for shared ports enforced on all switches on 
dsa-port.yaml:

   allOf:
     - required:
         - phy-mode
     - oneOf:
         - required:
             - fixed-link
         - required:
             - phy-handle
         - required:
             - managed

Here's what I understand:

- For switches in dsa_switches_apply_workarounds[]
   - Enforce the latter for shared ports.
   - Enforce the former for user ports.

- For switches not in dsa_switches_apply_workarounds[]
   - Enforce the former for all ports.

> 
>> - renesas,rzn1-a5psw.yaml
>>    - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw
>>
>> What to do:
>> - Document "mdio".
> 
> Not clear here and for all the schemas quoted below.. is "mdio" not documented already?

They are, or rather I didn't care while constructing this text. I will 
mention "mdio" is already documented per schema on the patch log.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
> On 12.09.2023 01:51, Vladimir Oltean wrote:
> > On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote:
> > > What to do:
> > > - For mscc,vsc7514-switch, enforce phylink bindings for ports.
> > > - For mscc,vsc7512-switch, enforce phylink bindings for user ports.
> > 
> > you can also look at dsa_switches_apply_workarounds[], and if the switch
> > isn't there, then you can replace "user ports" with "ports" here and
> > everywhere.
> 
> The phylink bindings for user ports I ended up making by looking up the
> existing devicetrees are different than the phylink bindings for the shared
> (CPU and DSA) ports currently enforced on all switches.
> 
> My phylink bindings for user ports:
> 
>             allOf:
>               - anyOf:
>                   - required: [ fixed-link ]
>                   - required: [ phy-handle ]
>                   - required: [ managed ]
> 
>               - if:
>                   required: [ fixed-link ]
>                 then:
>                   not:
>                     required: [ managed ]

Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
which should be fixed. It's the same phylink that gets used in both cases,
user ports and shared ports :)

> 
> The phylink bindings for shared ports enforced on all switches on
> dsa-port.yaml:
> 
>   allOf:
>     - required:
>         - phy-mode
>     - oneOf:
>         - required:
>             - fixed-link
>         - required:
>             - phy-handle
>         - required:
>             - managed
> 
> Here's what I understand:
> 
> - For switches in dsa_switches_apply_workarounds[]
>   - Enforce the latter for shared ports.
>   - Enforce the former for user ports.
> 
> - For switches not in dsa_switches_apply_workarounds[]
>   - Enforce the former for all ports.

No, no. We enforce the dt-schema regardless of switch presence in
dsa_switches_apply_workarounds[], to encourage users to fix device trees
(those who run schema validation). The kernel workaround consists in
doing something (skipping phylink) for the device trees where the schema
warns on shared ports. But there should be a single sub-schema for
validating phylink bindings, whatever port kind it is.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 12.09.2023 22:34, Vladimir Oltean wrote:
> On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
>> The phylink bindings for user ports I ended up making by looking up the
>> existing devicetrees are different than the phylink bindings for the shared
>> (CPU and DSA) ports currently enforced on all switches.
>>
>> My phylink bindings for user ports:
>>
>>              allOf:
>>                - anyOf:
>>                    - required: [ fixed-link ]
>>                    - required: [ phy-handle ]
>>                    - required: [ managed ]
>>
>>                - if:
>>                    required: [ fixed-link ]
>>                  then:
>>                    not:
>>                      required: [ managed ]
> 
> Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
> which should be fixed. It's the same phylink that gets used in both cases,
> user ports and shared ports :)

One more thing, I don't recall phy-mode being required to be defined for
user ports as it will default to GMII. I don't believe this is the same
case for shared ports so phy-mode is required only for them?

> 
>>
>> The phylink bindings for shared ports enforced on all switches on
>> dsa-port.yaml:
>>
>>    allOf:
>>      - required:
>>          - phy-mode
>>      - oneOf:
>>          - required:
>>              - fixed-link
>>          - required:
>>              - phy-handle
>>          - required:
>>              - managed
>>
>> Here's what I understand:
>>
>> - For switches in dsa_switches_apply_workarounds[]
>>    - Enforce the latter for shared ports.
>>    - Enforce the former for user ports.
>>
>> - For switches not in dsa_switches_apply_workarounds[]
>>    - Enforce the former for all ports.
> 
> No, no. We enforce the dt-schema regardless of switch presence in
> dsa_switches_apply_workarounds[], to encourage users to fix device trees
> (those who run schema validation). The kernel workaround consists in
> doing something (skipping phylink) for the device trees where the schema
> warns on shared ports. But there should be a single sub-schema for
> validating phylink bindings, whatever port kind it is.

Hmm, like writing phylink.yaml and then referring to it under the port
pattern node? This could prevent a lot of repetition.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
> On 12.09.2023 22:34, Vladimir Oltean wrote:
> > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
> > > The phylink bindings for user ports I ended up making by looking up the
> > > existing devicetrees are different than the phylink bindings for the shared
> > > (CPU and DSA) ports currently enforced on all switches.
> > > 
> > > My phylink bindings for user ports:
> > > 
> > >              allOf:
> > >                - anyOf:
> > >                    - required: [ fixed-link ]
> > >                    - required: [ phy-handle ]
> > >                    - required: [ managed ]
> > > 
> > >                - if:
> > >                    required: [ fixed-link ]
> > >                  then:
> > >                    not:
> > >                      required: [ managed ]
> > 
> > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
> > which should be fixed. It's the same phylink that gets used in both cases,
> > user ports and shared ports :)
> 
> One more thing, I don't recall phy-mode being required to be defined for
> user ports as it will default to GMII. I don't believe this is the same
> case for shared ports so phy-mode is required only for them?

phy-mode is not strictly required, but I think there is a strong
preference to set it. IIRC, when looking at the DSA device trees, there
was no case where phy-mode would be absent on CPU/DSA ports if the other
link properties were also present, so we required it too. There were no
complaints in 1 year since dsa_shared_port_validate_of() is there. The
requirement can be relaxed to just a warning and no error in the kernel,
and the removal of "required" in the schema, if it helps making it
common with user ports.

I think that the fallback to PHY_INTERFACE_MODE_GMII applies only if
there is a phy_device (phy-handle). But otherwise, I don't remember if
the PHY_INTERFACE_MODE_NA passed to phylink_create() will persist at
runtime, or cause an error somewhere.

> > > The phylink bindings for shared ports enforced on all switches on
> > > dsa-port.yaml:
> > > 
> > >    allOf:
> > >      - required:
> > >          - phy-mode
> > >      - oneOf:
> > >          - required:
> > >              - fixed-link
> > >          - required:
> > >              - phy-handle
> > >          - required:
> > >              - managed
> > > 
> > > Here's what I understand:
> > > 
> > > - For switches in dsa_switches_apply_workarounds[]
> > >    - Enforce the latter for shared ports.
> > >    - Enforce the former for user ports.
> > > 
> > > - For switches not in dsa_switches_apply_workarounds[]
> > >    - Enforce the former for all ports.
> > 
> > No, no. We enforce the dt-schema regardless of switch presence in
> > dsa_switches_apply_workarounds[], to encourage users to fix device trees
> > (those who run schema validation). The kernel workaround consists in
> > doing something (skipping phylink) for the device trees where the schema
> > warns on shared ports. But there should be a single sub-schema for
> > validating phylink bindings, whatever port kind it is.
> 
> Hmm, like writing phylink.yaml and then referring to it under the port
> pattern node? This could prevent a lot of repetition.
> 
> Arınç

Yes, that would sound good.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.09.2023 10:42, Vladimir Oltean wrote:
> On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
>> On 12.09.2023 22:34, Vladimir Oltean wrote:
>>> Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
>>> which should be fixed. It's the same phylink that gets used in both cases,
>>> user ports and shared ports :)
>>
>> One more thing, I don't recall phy-mode being required to be defined for
>> user ports as it will default to GMII. I don't believe this is the same
>> case for shared ports so phy-mode is required only for them?
> 
> phy-mode is not strictly required, but I think there is a strong
> preference to set it. IIRC, when looking at the DSA device trees, there
> was no case where phy-mode would be absent on CPU/DSA ports if the other
> link properties were also present, so we required it too. There were no
> complaints in 1 year since dsa_shared_port_validate_of() is there. The
> requirement can be relaxed to just a warning and no error in the kernel,
> and the removal of "required" in the schema, if it helps making it
> common with user ports.

I'd say no need as it doesn't make it complicated that much. See below.

> 
> I think that the fallback to PHY_INTERFACE_MODE_GMII applies only if
> there is a phy_device (phy-handle). But otherwise, I don't remember if
> the PHY_INTERFACE_MODE_NA passed to phylink_create() will persist at
> runtime, or cause an error somewhere.
> 
>>>> The phylink bindings for shared ports enforced on all switches on
>>>> dsa-port.yaml:
>>>>
>>>>     allOf:
>>>>       - required:
>>>>           - phy-mode
>>>>       - oneOf:
>>>>           - required:
>>>>               - fixed-link
>>>>           - required:
>>>>               - phy-handle
>>>>           - required:
>>>>               - managed
>>>>
>>>> Here's what I understand:
>>>>
>>>> - For switches in dsa_switches_apply_workarounds[]
>>>>     - Enforce the latter for shared ports.
>>>>     - Enforce the former for user ports.
>>>>
>>>> - For switches not in dsa_switches_apply_workarounds[]
>>>>     - Enforce the former for all ports.
>>>
>>> No, no. We enforce the dt-schema regardless of switch presence in
>>> dsa_switches_apply_workarounds[], to encourage users to fix device trees
>>> (those who run schema validation). The kernel workaround consists in
>>> doing something (skipping phylink) for the device trees where the schema
>>> warns on shared ports. But there should be a single sub-schema for
>>> validating phylink bindings, whatever port kind it is.
>>
>> Hmm, like writing phylink.yaml and then referring to it under the port
>> pattern node? This could prevent a lot of repetition.
>>
>> Arınç
> 
> Yes, that would sound good.

If I understand correctly, these phylink rules are for switch ports. The
fixed-link, phy-handle, and managed properties are described on
ethernet-controller.yaml so I thought it would make sense to define the
rules there and refer to them where they're needed.

Example:

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
index 480120469953..7279ab31aea7 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
@@ -65,16 +65,8 @@ if:
      - required: [ ethernet ]
      - required: [ link ]
  then:
-  allOf:
-    - required:
-        - phy-mode
-    - oneOf:
-        - required:
-            - fixed-link
-        - required:
-            - phy-handle
-        - required:
-            - managed
+  $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch
+  required: [ phy-mode ]
  
  additionalProperties: true
  
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index e532c6b795f4..742aaf1a5ef2 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -179,6 +179,15 @@ required:
    - compatible
    - reg
  
+if:
+  required: [ mdio ]
+then:
+  patternProperties:
+    "^(ethernet-)?ports$":
+      patternProperties:
+        "^(ethernet-)?port@[0-9]+$":
+          $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch
+
  $defs:
    mt7530-dsa-port:
      patternProperties:
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 9f6a5ccbcefe..d7256f33d946 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -284,6 +284,21 @@ allOf:
              controllers that have configurable TX internal delays. If this
              property is present then the MAC applies the TX delay.
  
+$defs:
+  phylink-switch:
+    description: phylink bindings for switch ports
+    allOf:
+      - anyOf:
+          - required: [ fixed-link ]
+          - required: [ phy-handle ]
+          - required: [ managed ]
+
+      - if:
+          required: [ fixed-link ]
+        then:
+          not:
+            required: [ managed ]
+
  additionalProperties: true
  
  ...

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 01:59:17PM +0300, Arınç ÜNAL wrote:
> If I understand correctly, these phylink rules are for switch ports. The
> fixed-link, phy-handle, and managed properties are described on
> ethernet-controller.yaml so I thought it would make sense to define the
> rules there and refer to them where they're needed.
> 
> Example:
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> index 480120469953..7279ab31aea7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> @@ -65,16 +65,8 @@ if:
>      - required: [ ethernet ]
>      - required: [ link ]
>  then:
> -  allOf:
> -    - required:
> -        - phy-mode
> -    - oneOf:
> -        - required:
> -            - fixed-link
> -        - required:
> -            - phy-handle
> -        - required:
> -            - managed
> +  $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch
> +  required: [ phy-mode ]
>  additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e532c6b795f4..742aaf1a5ef2 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -179,6 +179,15 @@ required:
>    - compatible
>    - reg
> +if:
> +  required: [ mdio ]
> +then:
> +  patternProperties:
> +    "^(ethernet-)?ports$":
> +      patternProperties:
> +        "^(ethernet-)?port@[0-9]+$":
> +          $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch
> +
>  $defs:
>    mt7530-dsa-port:
>      patternProperties:
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 9f6a5ccbcefe..d7256f33d946 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -284,6 +284,21 @@ allOf:
>              controllers that have configurable TX internal delays. If this
>              property is present then the MAC applies the TX delay.
> +$defs:
> +  phylink-switch:
> +    description: phylink bindings for switch ports
> +    allOf:
> +      - anyOf:
> +          - required: [ fixed-link ]
> +          - required: [ phy-handle ]
> +          - required: [ managed ]
> +
> +      - if:
> +          required: [ fixed-link ]
> +        then:
> +          not:
> +            required: [ managed ]
> +
>  additionalProperties: true
>  ...
> 
> Arınç

I don't think they're for switch ports only. Any driver which uses
phylink_fwnode_phy_connect() or its derivatives gets subject to the same
bindings. But putting the sub-schema in ethernet-controller.yaml makes
sense, just maybe not naming it "phylink-switch".

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.09.2023 14:04, Vladimir Oltean wrote:
> I don't think they're for switch ports only. Any driver which uses
> phylink_fwnode_phy_connect() or its derivatives gets subject to the same
> bindings. But putting the sub-schema in ethernet-controller.yaml makes
> sense, just maybe not naming it "phylink-switch".

Got it. Should we disallow managed altogether when fixed-link is also
defined, or just with in-band-status value?

Currently:

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 9f6a5ccbcefe..3b5946a4be34 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -284,6 +284,21 @@ allOf:
              controllers that have configurable TX internal delays. If this
              property is present then the MAC applies the TX delay.
  
+$defs:
+  phylink:
+    description: phylink bindings for ethernet controllers
+    allOf:
+      - anyOf:
+          - required: [ fixed-link ]
+          - required: [ phy-handle ]
+          - required: [ managed ]
+
+      - if:
+          required: [ fixed-link ]
+        then:
+          properties:
+            managed: false
+
  additionalProperties: true
  
  ...

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 02:35:11PM +0300, Arınç ÜNAL wrote:
> On 13.09.2023 14:04, Vladimir Oltean wrote:
> > I don't think they're for switch ports only. Any driver which uses
> > phylink_fwnode_phy_connect() or its derivatives gets subject to the same
> > bindings. But putting the sub-schema in ethernet-controller.yaml makes
> > sense, just maybe not naming it "phylink-switch".
> 
> Got it. Should we disallow managed altogether when fixed-link is also
> defined, or just with in-band-status value?

Just with the "in-band-status" value - just like phylink_parse_mode()
implies. If not possible, just leave that condition out.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.09.2023 16:00, Vladimir Oltean wrote:
> On Wed, Sep 13, 2023 at 02:35:11PM +0300, Arınç ÜNAL wrote:
>> On 13.09.2023 14:04, Vladimir Oltean wrote:
>>> I don't think they're for switch ports only. Any driver which uses
>>> phylink_fwnode_phy_connect() or its derivatives gets subject to the same
>>> bindings. But putting the sub-schema in ethernet-controller.yaml makes
>>> sense, just maybe not naming it "phylink-switch".
>>
>> Got it. Should we disallow managed altogether when fixed-link is also
>> defined, or just with in-band-status value?
> 
> Just with the "in-band-status" value - just like phylink_parse_mode()
> implies. If not possible, just leave that condition out.

I can rewrite the property to allow values other than in-band-status.

       - if:
           required: [ fixed-link ]
         then:
           properties:
             managed:
               const: auto

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Russell King (Oracle)]

On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote:
> On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
> > On 12.09.2023 22:34, Vladimir Oltean wrote:
> > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
> > > > The phylink bindings for user ports I ended up making by looking up the
> > > > existing devicetrees are different than the phylink bindings for the shared
> > > > (CPU and DSA) ports currently enforced on all switches.
> > > > 
> > > > My phylink bindings for user ports:
> > > > 
> > > >              allOf:
> > > >                - anyOf:
> > > >                    - required: [ fixed-link ]
> > > >                    - required: [ phy-handle ]
> > > >                    - required: [ managed ]
> > > > 
> > > >                - if:
> > > >                    required: [ fixed-link ]
> > > >                  then:
> > > >                    not:
> > > >                      required: [ managed ]
> > > 
> > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
> > > which should be fixed. It's the same phylink that gets used in both cases,
> > > user ports and shared ports :)
> > 
> > One more thing, I don't recall phy-mode being required to be defined for
> > user ports as it will default to GMII. I don't believe this is the same
> > case for shared ports so phy-mode is required only for them?
> 
> phy-mode is not strictly required, but I think there is a strong
> preference to set it. IIRC, when looking at the DSA device trees, there
> was no case where phy-mode would be absent on CPU/DSA ports if the other
> link properties were also present, so we required it too. There were no
> complaints in 1 year since dsa_shared_port_validate_of() is there. The
> requirement can be relaxed to just a warning and no error in the kernel,
> and the removal of "required" in the schema, if it helps making it
> common with user ports.

However, phylink pretty much requires phy-mode to be specified to be
something sane for shared ports, so I wouldn't be in favour of relaxing
the checkinng in dsa_shared_port_validate_of()... not unless you're
now going to accept the approach I originally proposed to have DSA
drivers tell the core (and thus phylink) what phy-mode and other link
parameters should be used when they are missing from DT.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote:
> However, phylink pretty much requires phy-mode to be specified to be
> something sane for shared ports, so I wouldn't be in favour of relaxing
> the checkinng in dsa_shared_port_validate_of()... not unless you're
> now going to accept the approach I originally proposed to have DSA
> drivers tell the core (and thus phylink) what phy-mode and other link
> parameters should be used when they are missing from DT.

Ok, so with a missing phy-mode on the CPU port, phylink_parse_fixedlink() ->
phy_lookup_setting() will return NULL and that will print a phylink_warn(),
but other than that, phylink_mac_link_up() does get called at the right
speed and duplex.

I agree that for sane behavior it should be specified, but it appears
that even with PHY_INTERFACE_MODE_NA something can be hacked up...

[    4.818368] sja1105 spi0.1: Failed to read phy-mode or phy-interface-type property for port 4
[    4.864667] sja1105 spi0.1: OF node /soc/spi@2100000/ethernet-switch@1/ports/port@4 of CPU port 4 lacks the required "phy-mode" property
[    4.882957] sja1105 spi0.1: pl->link_config.speed 1000 pl->link_config.duplex 1 pl->supported 00,00000000,00000000,00000240
[    4.894189] sja1105 spi0.1: phy_setting speed -1 duplex -1 bit -1
[    4.900283] sja1105 spi0.1: fixed link full duplex 1000Mbps not recognised
[    4.907798] sja1105 spi0.1: configuring for fixed/ link mode
[    4.916183] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL)
[    4.934770] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL)
[    4.951619] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL)
[    4.968349] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL)
[    4.984017] fsl-gianfar soc:ethernet@2d90000 eth2: entered promiscuous mode
[    4.991327] DSA: tree 0 setup
[    4.995129] sja1105 spi0.1: sja1105_mac_link_up: port 4 interface  speed 1000 duplex 1
[    5.005004] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off

diff --git a/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts b/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts
index 1ea32fff4120..0bfffcb51af9 100644
--- a/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts
+++ b/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts
@@ -90,7 +90,7 @@ port@3 {
 			port@4 {
 				/* Internal port connected to eth2 */
 				ethernet = <&enet2>;
-				phy-mode = "rgmii";
+//				phy-mode = "rgmii";
 				rx-internal-delay-ps = <0>;
 				tx-internal-delay-ps = <0>;
 				reg = <4>;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index a23d980d28f5..dba1fa545a9c 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -327,6 +327,8 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv)
 			mii->xmii_mode[i] = XMII_MODE_SGMII;
 			mii->special[i] = true;
 			break;
+		case PHY_INTERFACE_MODE_NA:
+			break;
 unsupported:
 		default:
 			dev_err(dev, "Unsupported PHY mode %s on port %d!\n",
@@ -1205,11 +1207,10 @@ static int sja1105_parse_ports_node(struct sja1105_private *priv,
 		/* Get PHY mode from DT */
 		err = of_get_phy_mode(child, &phy_mode);
 		if (err) {
-			dev_err(dev, "Failed to read phy-mode or "
+			dev_warn(dev, "Failed to read phy-mode or "
 				"phy-interface-type property for port %d\n",
 				index);
-			of_node_put(child);
-			return -ENODEV;
+			phy_mode = PHY_INTERFACE_MODE_NA;
 		}
 
 		phy_node = of_parse_phandle(child, "phy-handle", 0);
@@ -1383,6 +1384,8 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 {
 	struct sja1105_private *priv = ds->priv;
 
+	dev_err(ds->dev, "%s: port %d interface %s speed %d duplex %d\n", __func__, port, phy_modes(interface), speed, duplex);
+
 	sja1105_adjust_port_config(priv, port, speed);
 
 	sja1105_inhibit_tx(priv, BIT(port), false);
@@ -1414,7 +1417,10 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 		 * config (the xMII Mode table cannot be dynamically
 		 * reconfigured), and we have to program that early.
 		 */
-		__set_bit(phy_mode, config->supported_interfaces);
+		if (phy_mode == PHY_INTERFACE_MODE_NA)
+			phy_interface_set_rgmii(config->supported_interfaces);
+		else
+			__set_bit(phy_mode, config->supported_interfaces);
 	}
 
 	/* The MAC does not support pause frames, and also doesn't
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0d7354955d62..674689011059 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -841,6 +841,15 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 	if (autoneg)
 		phylink_set(pl->supported, Autoneg);
 
+	phylink_err(pl, "pl->link_config.speed %d pl->link_config.duplex %d pl->supported %*pb\n",
+		    pl->link_config.speed, pl->link_config.duplex, __ETHTOOL_LINK_MODE_MASK_NBITS,
+		    pl->supported);
+
+	phylink_err(pl, "phy_setting speed %d duplex %d bit %d\n",
+		    s ? s->speed : -1,
+		    s ? s->duplex : -1,
+		    s ? s->bit : -1);
+
 	if (s) {
 		__set_bit(s->bit, pl->supported);
 		__set_bit(s->bit, pl->link_config.lp_advertising);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 5f01bd4f9dec..34e5dc48f0ff 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1927,6 +1927,16 @@ static const char * const dsa_switches_apply_workarounds[] = {
 #if IS_ENABLED(CONFIG_NET_DSA_SMSC_LAN9303_I2C)
 	"smsc,lan9303-i2c",
 #endif
+	"nxp,sja1105e",
+	"nxp,sja1105t",
+	"nxp,sja1105p",
+	"nxp,sja1105q",
+	"nxp,sja1105r",
+	"nxp,sja1105s",
+	"nxp,sja1110a",
+	"nxp,sja1110b",
+	"nxp,sja1110c",
+	"nxp,sja1110d",
 	NULL,
 };
 

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Russell King (Oracle)]

On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote:
> On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote:
> > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
> > > On 12.09.2023 22:34, Vladimir Oltean wrote:
> > > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
> > > > > The phylink bindings for user ports I ended up making by looking up the
> > > > > existing devicetrees are different than the phylink bindings for the shared
> > > > > (CPU and DSA) ports currently enforced on all switches.
> > > > > 
> > > > > My phylink bindings for user ports:
> > > > > 
> > > > >              allOf:
> > > > >                - anyOf:
> > > > >                    - required: [ fixed-link ]
> > > > >                    - required: [ phy-handle ]
> > > > >                    - required: [ managed ]
> > > > > 
> > > > >                - if:
> > > > >                    required: [ fixed-link ]
> > > > >                  then:
> > > > >                    not:
> > > > >                      required: [ managed ]
> > > > 
> > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
> > > > which should be fixed. It's the same phylink that gets used in both cases,
> > > > user ports and shared ports :)
> > > 
> > > One more thing, I don't recall phy-mode being required to be defined for
> > > user ports as it will default to GMII. I don't believe this is the same
> > > case for shared ports so phy-mode is required only for them?
> > 
> > phy-mode is not strictly required, but I think there is a strong
> > preference to set it. IIRC, when looking at the DSA device trees, there
> > was no case where phy-mode would be absent on CPU/DSA ports if the other
> > link properties were also present, so we required it too. There were no
> > complaints in 1 year since dsa_shared_port_validate_of() is there. The
> > requirement can be relaxed to just a warning and no error in the kernel,
> > and the removal of "required" in the schema, if it helps making it
> > common with user ports.
> 
> However, phylink pretty much requires phy-mode to be specified to be
> something sane for shared ports, so I wouldn't be in favour of relaxing
> the checkinng in dsa_shared_port_validate_of()... not unless you're
> now going to accept the approach I originally proposed to have DSA
> drivers tell the core (and thus phylink) what phy-mode and other link
> parameters should be used when they are missing from DT.

You mean the approach that I picked up using software nodes that got
thrown out by the software node people? That approach that I picked
up from you and tried to get merged?

No, that's not going to happen, and it's not a question of whether
_I_ am going to accept that approach or not. So don't throw that
back on me, please.

If this is something that we want to solve, we need to stop being so
devisive (your language above is so) and try to come up with a
solution that is acceptable to everyone... the swnode approach
doesn't seem to be it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Russell King (Oracle)]

On Thu, Sep 14, 2023 at 07:06:11PM +0100, Russell King (Oracle) wrote:
> On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote:
> > On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote:
> > > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
> > > > On 12.09.2023 22:34, Vladimir Oltean wrote:
> > > > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote:
> > > > > > The phylink bindings for user ports I ended up making by looking up the
> > > > > > existing devicetrees are different than the phylink bindings for the shared
> > > > > > (CPU and DSA) ports currently enforced on all switches.
> > > > > > 
> > > > > > My phylink bindings for user ports:
> > > > > > 
> > > > > >              allOf:
> > > > > >                - anyOf:
> > > > > >                    - required: [ fixed-link ]
> > > > > >                    - required: [ phy-handle ]
> > > > > >                    - required: [ managed ]
> > > > > > 
> > > > > >                - if:
> > > > > >                    required: [ fixed-link ]
> > > > > >                  then:
> > > > > >                    not:
> > > > > >                      required: [ managed ]
> > > > > 
> > > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug
> > > > > which should be fixed. It's the same phylink that gets used in both cases,
> > > > > user ports and shared ports :)
> > > > 
> > > > One more thing, I don't recall phy-mode being required to be defined for
> > > > user ports as it will default to GMII. I don't believe this is the same
> > > > case for shared ports so phy-mode is required only for them?
> > > 
> > > phy-mode is not strictly required, but I think there is a strong
> > > preference to set it. IIRC, when looking at the DSA device trees, there
> > > was no case where phy-mode would be absent on CPU/DSA ports if the other
> > > link properties were also present, so we required it too. There were no
> > > complaints in 1 year since dsa_shared_port_validate_of() is there. The
> > > requirement can be relaxed to just a warning and no error in the kernel,
> > > and the removal of "required" in the schema, if it helps making it
> > > common with user ports.
> > 
> > However, phylink pretty much requires phy-mode to be specified to be
> > something sane for shared ports, so I wouldn't be in favour of relaxing
> > the checkinng in dsa_shared_port_validate_of()... not unless you're
> > now going to accept the approach I originally proposed to have DSA
> > drivers tell the core (and thus phylink) what phy-mode and other link
> > parameters should be used when they are missing from DT.
> 
> You mean the approach that I picked up using software nodes that got
> thrown out by the software node people? That approach that I picked
> up from you and tried to get merged?
> 
> No, that's not going to happen, and it's not a question of whether
> _I_ am going to accept that approach or not. So don't throw that
> back on me, please.
> 
> If this is something that we want to solve, we need to stop being so
> devisive (your language above is so) and try to come up with a
> solution that is acceptable to everyone... the swnode approach
> doesn't seem to be it.

Oh dear. I must be going mad!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

Hi Russell,

On Thu, Sep 14, 2023 at 07:07:13PM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 14, 2023 at 07:06:11PM +0100, Russell King (Oracle) wrote:
> > On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote:
> > > > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote:
> > > > > One more thing, I don't recall phy-mode being required to be defined for
> > > > > user ports as it will default to GMII. I don't believe this is the same
> > > > > case for shared ports so phy-mode is required only for them?
> > > > 
> > > > phy-mode is not strictly required, but I think there is a strong
> > > > preference to set it. IIRC, when looking at the DSA device trees, there
> > > > was no case where phy-mode would be absent on CPU/DSA ports if the other
> > > > link properties were also present, so we required it too. There were no
> > > > complaints in 1 year since dsa_shared_port_validate_of() is there. The
> > > > requirement can be relaxed to just a warning and no error in the kernel,
> > > > and the removal of "required" in the schema, if it helps making it
> > > > common with user ports.
> > > 
> > > However, phylink pretty much requires phy-mode to be specified to be
> > > something sane for shared ports, so I wouldn't be in favour of relaxing
> > > the checkinng in dsa_shared_port_validate_of()... not unless you're
> > > now going to accept the approach I originally proposed to have DSA
> > > drivers tell the core (and thus phylink) what phy-mode and other link
> > > parameters should be used when they are missing from DT.
> > 
> > You mean the approach that I picked up using software nodes that got
> > thrown out by the software node people? That approach that I picked
> > up from you and tried to get merged?
> > 
> > No, that's not going to happen, and it's not a question of whether
> > _I_ am going to accept that approach or not. So don't throw that
> > back on me, please.
> > 
> > If this is something that we want to solve, we need to stop being so
> > devisive (your language above is so) and try to come up with a
> > solution that is acceptable to everyone... the swnode approach
> > doesn't seem to be it.
> 
> Oh dear. I must be going mad!

So first things first: I am not advocating for making phy-mode fully
optional in the sense that you say (if absent, then write non-OF code
through which DSA infers the phy-mode from drivers). I'm happy with the
current form of the code.

I was just trying to add some nuance to this bizarre aspect signalled by
Arınç - phy-mode is not required for user ports, presumably because when
it is absent, user ports will default to GMII. That isn't an intrinsic
feature of user ports, but rather of having a phydev, and so, because
DSA/CPU ports can also have a phydev, logically it means that phy-mode
can also be omitted in that particular case, with the same result.

Our missing_phy_mode check from dsa_shared_port_validate_of() is theoretically
more restrictive than it needs to be, because it artificially prohibits
that behavior, and it results in an inexplicable difference in the phylink
dt-bindings for user vs shared ports. So that's where my relaxation
proposal came from: we could make missing_phy_mode non-fatal, and that
would permit the configurations which can work to work, and the ones
which can't work will fail elsewhere. Just like for the user ports.

Where I wasn't absolutely clear is that I don't want Arınç to change any
of that. Right now, on DSA shared ports, phy-mode is required and on user
ports it isn't. The difference is a bit strange (arbitrary considering
the example above) and should maybe be settled at some point in the
future, but for now, the dt-bindings should document it like that.

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote:
> Adding the mdio property to the dsa.yaml schema will allow it on all of the
> schemas that refer to dsa.yaml such as this one. This addition here is only
> to disallow additional properties under the mdio property for this specific
> schema.

So, how about not adding it to dsa.yaml, but to individual switch schemas,
along with their specific handling of phylink bindings on user ports?

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Rob Herring]

On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote:
> I've realised there are more schemas that extend the mdio.yaml schema. This
> is the final state of this patch.
> 
> dt-bindings: net: dsa: document internal MDIO bus
> 
> Add the schema to document the internal MDIO bus. Require the phy-handle
> property on the non-CPU ports if the mdio property is being used.
> 
> Define the mdio property on all of the schemas that refer to
> dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point
> the human readers to the description on the dsa.yaml schema.
> 
> Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is
> also being referred to through dsa.yaml#/$defs/ethernet-ports now which
> means we cannot disallow additional properties by 'unevaluatedProperties:
> false' on the dsa.yaml schema.
> 
> ---
>  .../bindings/net/dsa/arrow,xrs700x.yaml        |  4 ++++
>  .../devicetree/bindings/net/dsa/brcm,b53.yaml  |  4 ++++
>  .../devicetree/bindings/net/dsa/brcm,sf2.yaml  |  4 ++++
>  .../devicetree/bindings/net/dsa/dsa.yaml       | 18 ++++++++++++++++++
>  .../bindings/net/dsa/hirschmann,hellcreek.yaml |  4 ++++
>  .../bindings/net/dsa/mediatek,mt7530.yaml      |  4 ++++
>  .../bindings/net/dsa/microchip,ksz.yaml        |  4 ++++
>  .../bindings/net/dsa/microchip,lan937x.yaml    |  2 +-
>  .../bindings/net/dsa/mscc,ocelot.yaml          |  4 ++++
>  .../bindings/net/dsa/nxp,sja1105.yaml          |  4 ++++
>  .../devicetree/bindings/net/dsa/qca8k.yaml     |  2 +-
>  .../devicetree/bindings/net/dsa/realtek.yaml   |  2 +-
>  .../bindings/net/dsa/renesas,rzn1-a5psw.yaml   |  2 +-
>  13 files changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
> index 9565a740214629..f0229352e05694 100644
> --- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
> @@ -29,6 +29,10 @@ properties:
>    reg:
>      maxItems: 1
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> index 4c78c546343f5e..e14562b33bfb97 100644
> --- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> @@ -65,6 +65,10 @@ properties:
>                - brcm,bcm63268-switch
>            - const: brcm,bcm63xx-switch
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
> index c745407f2f6853..1bf4317e038687 100644
> --- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
> @@ -90,6 +90,10 @@ properties:
>                tags enabled (per-packet metadata)
>              type: boolean
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - reg
>    - interrupts
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> index ec74a660bedaed..03ccedbc49dcc3 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> @@ -31,6 +31,24 @@ properties:
>        (single device hanging off a CPU port) must not specify this property
>      $ref: /schemas/types.yaml#/definitions/uint32-array
> +  mdio:
> +    description: The internal MDIO bus of the switch
> +    $ref: /schemas/net/mdio.yaml#
> +
> +if:
> +  required: [ mdio ]
> +then:
> +  patternProperties:
> +    "^(ethernet-)?ports$":
> +      patternProperties:
> +        "^(ethernet-)?port@[0-9]+$":
> +          if:
> +            not:
> +              required: [ ethernet ]
> +          then:
> +            required:
> +              - phy-handle
> +
>  additionalProperties: true
>  $defs:
> diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
> index 4021b054f68446..32f17345825d4a 100644
> --- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
> @@ -67,6 +67,10 @@ properties:
>      additionalProperties: false
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e532c6b795f4fc..293d1affe75451 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -151,6 +151,10 @@ properties:
>        ethsys.
>      maxItems: 1
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  patternProperties:
>    "^(ethernet-)?ports$":
>      type: object
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index e51be1ac036237..01d11c642ecfd4 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -49,6 +49,10 @@ properties:
>        Set if the output SYNCLKO clock should be disabled. Do not mix with
>        microchip,synclko-125.
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> index 49af4b0d591695..15f24a1716cd44 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> @@ -32,7 +32,7 @@ properties:
>      maxItems: 1
>    mdio:
> -    $ref: /schemas/net/mdio.yaml#
> +    $ref: dsa.yaml#/properties/mdio
>      unevaluatedProperties: false
>  patternProperties:
> diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> index fe02d05196e4a6..d781b8c2324836 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> @@ -73,6 +73,10 @@ properties:
>    little-endian: true
>    big-endian: true
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index 4d5f5cc6d031e2..82dda8fae8b16e 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -72,6 +72,10 @@ properties:
>            - compatible
>            - reg
> +  mdio:
> +    $ref: dsa.yaml#/properties/mdio
> +    unevaluatedProperties: false
> +
>  patternProperties:
>    "^(ethernet-)?ports$":
>      patternProperties:
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index df64eebebe1856..001b72bcd0746b 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -60,7 +60,7 @@ properties:
>        B68 on the QCA832x and B49 on the QCA833x.
>    mdio:
> -    $ref: /schemas/net/mdio.yaml#
> +    $ref: dsa.yaml#/properties/mdio
>      unevaluatedProperties: false

Just from a schema standpoint, this is pointless indirection as 
dsa.yaml#/properties/mdio is just a reference to /schemas/net/mdio.yaml#.

As it seems an MDIO bus is not universal for DSA, it seems you'll be 
dropping this change anyways.

Rob

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 21.08.2023 20:44, Rob Herring wrote:
> On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote:
>> I've realised there are more schemas that extend the mdio.yaml schema. This
>> is the final state of this patch.
>>
>> dt-bindings: net: dsa: document internal MDIO bus
>>
>> Add the schema to document the internal MDIO bus. Require the phy-handle
>> property on the non-CPU ports if the mdio property is being used.
>>
>> Define the mdio property on all of the schemas that refer to
>> dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point
>> the human readers to the description on the dsa.yaml schema.
>>
>> Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is
>> also being referred to through dsa.yaml#/$defs/ethernet-ports now which
>> means we cannot disallow additional properties by 'unevaluatedProperties:
>> false' on the dsa.yaml schema.
>>
>> ---
>>   .../bindings/net/dsa/arrow,xrs700x.yaml        |  4 ++++
>>   .../devicetree/bindings/net/dsa/brcm,b53.yaml  |  4 ++++
>>   .../devicetree/bindings/net/dsa/brcm,sf2.yaml  |  4 ++++
>>   .../devicetree/bindings/net/dsa/dsa.yaml       | 18 ++++++++++++++++++
>>   .../bindings/net/dsa/hirschmann,hellcreek.yaml |  4 ++++
>>   .../bindings/net/dsa/mediatek,mt7530.yaml      |  4 ++++
>>   .../bindings/net/dsa/microchip,ksz.yaml        |  4 ++++
>>   .../bindings/net/dsa/microchip,lan937x.yaml    |  2 +-
>>   .../bindings/net/dsa/mscc,ocelot.yaml          |  4 ++++
>>   .../bindings/net/dsa/nxp,sja1105.yaml          |  4 ++++
>>   .../devicetree/bindings/net/dsa/qca8k.yaml     |  2 +-
>>   .../devicetree/bindings/net/dsa/realtek.yaml   |  2 +-
>>   .../bindings/net/dsa/renesas,rzn1-a5psw.yaml   |  2 +-
>>   13 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
>> index 9565a740214629..f0229352e05694 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
>> @@ -29,6 +29,10 @@ properties:
>>     reg:
>>       maxItems: 1
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>> index 4c78c546343f5e..e14562b33bfb97 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>> @@ -65,6 +65,10 @@ properties:
>>                 - brcm,bcm63268-switch
>>             - const: brcm,bcm63xx-switch
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
>> index c745407f2f6853..1bf4317e038687 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
>> @@ -90,6 +90,10 @@ properties:
>>                 tags enabled (per-packet metadata)
>>               type: boolean
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - reg
>>     - interrupts
>> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> index ec74a660bedaed..03ccedbc49dcc3 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> @@ -31,6 +31,24 @@ properties:
>>         (single device hanging off a CPU port) must not specify this property
>>       $ref: /schemas/types.yaml#/definitions/uint32-array
>> +  mdio:
>> +    description: The internal MDIO bus of the switch
>> +    $ref: /schemas/net/mdio.yaml#
>> +
>> +if:
>> +  required: [ mdio ]
>> +then:
>> +  patternProperties:
>> +    "^(ethernet-)?ports$":
>> +      patternProperties:
>> +        "^(ethernet-)?port@[0-9]+$":
>> +          if:
>> +            not:
>> +              required: [ ethernet ]
>> +          then:
>> +            required:
>> +              - phy-handle
>> +
>>   additionalProperties: true
>>   $defs:
>> diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
>> index 4021b054f68446..32f17345825d4a 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
>> @@ -67,6 +67,10 @@ properties:
>>       additionalProperties: false
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> index e532c6b795f4fc..293d1affe75451 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> @@ -151,6 +151,10 @@ properties:
>>         ethsys.
>>       maxItems: 1
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   patternProperties:
>>     "^(ethernet-)?ports$":
>>       type: object
>> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>> index e51be1ac036237..01d11c642ecfd4 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>> @@ -49,6 +49,10 @@ properties:
>>         Set if the output SYNCLKO clock should be disabled. Do not mix with
>>         microchip,synclko-125.
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> index 49af4b0d591695..15f24a1716cd44 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> @@ -32,7 +32,7 @@ properties:
>>       maxItems: 1
>>     mdio:
>> -    $ref: /schemas/net/mdio.yaml#
>> +    $ref: dsa.yaml#/properties/mdio
>>       unevaluatedProperties: false
>>   patternProperties:
>> diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
>> index fe02d05196e4a6..d781b8c2324836 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
>> @@ -73,6 +73,10 @@ properties:
>>     little-endian: true
>>     big-endian: true
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> index 4d5f5cc6d031e2..82dda8fae8b16e 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> @@ -72,6 +72,10 @@ properties:
>>             - compatible
>>             - reg
>> +  mdio:
>> +    $ref: dsa.yaml#/properties/mdio
>> +    unevaluatedProperties: false
>> +
>>   patternProperties:
>>     "^(ethernet-)?ports$":
>>       patternProperties:
>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
>> index df64eebebe1856..001b72bcd0746b 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
>> @@ -60,7 +60,7 @@ properties:
>>         B68 on the QCA832x and B49 on the QCA833x.
>>     mdio:
>> -    $ref: /schemas/net/mdio.yaml#
>> +    $ref: dsa.yaml#/properties/mdio
>>       unevaluatedProperties: false
> 
> Just from a schema standpoint, this is pointless indirection as
> dsa.yaml#/properties/mdio is just a reference to /schemas/net/mdio.yaml#.

Sure, this is only to point the human readers to the description on the
dsa.yaml schema which describes the property as the internal MDIO bus of an
ethernet switch. Let me know if you find this unnecessary.

> 
> As it seems an MDIO bus is not universal for DSA, it seems you'll be
> dropping this change anyways.

For now, I don't think that'll be the case.

Arınç

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Vladimir Oltean]

On Sat, Aug 12, 2023 at 12:17:06PM +0300, Arınç ÜNAL wrote:
> Add the schema to document the internal MDIO bus. Adjust realtek.yaml
> accordingly.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  .../devicetree/bindings/net/dsa/dsa.yaml      | 18 +++++
>  .../devicetree/bindings/net/dsa/realtek.yaml  | 66 +++++++++----------
>  2 files changed, 50 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> index ec74a660beda..03ccedbc49dc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> @@ -31,6 +31,24 @@ properties:
>        (single device hanging off a CPU port) must not specify this property
>      $ref: /schemas/types.yaml#/definitions/uint32-array
>  
> +  mdio:
> +    description: The internal MDIO bus of the switch
> +    $ref: /schemas/net/mdio.yaml#
> +
> +if:
> +  required: [ mdio ]
> +then:
> +  patternProperties:
> +    "^(ethernet-)?ports$":
> +      patternProperties:
> +        "^(ethernet-)?port@[0-9]+$":
> +          if:
> +            not:
> +              required: [ ethernet ]

To match only on user ports, this must also exclude DSA ports ("required: [ link ]").

> +          then:
> +            required:
> +              - phy-handle

No. The only thing permitted by the slave_mii_bus is to do something meaningful
when phylink bindings ("phy-handle", "fixed-link" or "managed") are absent. But
the presence of slave_mii_bus does not imply that user ports have a required
phy-handle. They might be SerDes ports or xMII ports. So they might use "managed"
or "fixed-link". The only thing that you can enforce is that, if the slave_mii_bus
has an OF presence, then user ports must have phylink bindings.

> +
>  additionalProperties: true
>  
>  $defs:
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cfd69c2604ea..ea7db0890abc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: Realtek switches for unmanaged switches
>  
> -allOf:
> -  - $ref: dsa.yaml#/$defs/ethernet-ports
> -
>  maintainers:
>    - Linus Walleij <linus.walleij@linaro.org>
>  
> @@ -95,37 +92,38 @@ properties:
>        - '#address-cells'
>        - '#interrupt-cells'
>  
> -  mdio:
> -    $ref: /schemas/net/mdio.yaml#
> -    unevaluatedProperties: false
> -
> -    properties:
> -      compatible:
> -        const: realtek,smi-mdio
> -
> -if:
> -  required:
> -    - reg
> -
> -then:
> -  $ref: /schemas/spi/spi-peripheral-props.yaml#
> -  not:
> -    required:
> -      - mdc-gpios
> -      - mdio-gpios
> -      - mdio
> -
> -  properties:
> -    mdc-gpios: false
> -    mdio-gpios: false
> -    mdio: false
> -
> -else:
> -  required:
> -    - mdc-gpios
> -    - mdio-gpios
> -    - mdio
> -    - reset-gpios
> +allOf:
> +  - $ref: dsa.yaml#/$defs/ethernet-ports
> +  - if:
> +      required: [ mdio ]
> +    then:
> +      properties:
> +        mdio:
> +          properties:
> +            compatible:
> +              const: realtek,smi-mdio
> +
> +  - if:
> +      required:
> +        - reg
> +    then:
> +      $ref: /schemas/spi/spi-peripheral-props.yaml#
> +      not:
> +        required:
> +          - mdc-gpios
> +          - mdio-gpios
> +          - mdio
> +
> +      properties:
> +        mdc-gpios: false
> +        mdio-gpios: false
> +        mdio: false
> +    else:
> +      required:
> +        - mdc-gpios
> +        - mdio-gpios
> +        - mdio
> +        - reset-gpios
>  
>  required:
>    - compatible
> -- 
> 2.39.2
> 

Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

[Arınç ÜNAL]

On 13.08.2023 14:15, Vladimir Oltean wrote:
> On Sat, Aug 12, 2023 at 12:17:06PM +0300, Arınç ÜNAL wrote:
>> Add the schema to document the internal MDIO bus. Adjust realtek.yaml
>> accordingly.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   .../devicetree/bindings/net/dsa/dsa.yaml      | 18 +++++
>>   .../devicetree/bindings/net/dsa/realtek.yaml  | 66 +++++++++----------
>>   2 files changed, 50 insertions(+), 34 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> index ec74a660beda..03ccedbc49dc 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
>> @@ -31,6 +31,24 @@ properties:
>>         (single device hanging off a CPU port) must not specify this property
>>       $ref: /schemas/types.yaml#/definitions/uint32-array
>>   
>> +  mdio:
>> +    description: The internal MDIO bus of the switch
>> +    $ref: /schemas/net/mdio.yaml#
>> +
>> +if:
>> +  required: [ mdio ]
>> +then:
>> +  patternProperties:
>> +    "^(ethernet-)?ports$":
>> +      patternProperties:
>> +        "^(ethernet-)?port@[0-9]+$":
>> +          if:
>> +            not:
>> +              required: [ ethernet ]
> 
> To match only on user ports, this must also exclude DSA ports ("required: [ link ]").
> 
>> +          then:
>> +            required:
>> +              - phy-handle
> 
> No. The only thing permitted by the slave_mii_bus is to do something meaningful
> when phylink bindings ("phy-handle", "fixed-link" or "managed") are absent. But
> the presence of slave_mii_bus does not imply that user ports have a required
> phy-handle. They might be SerDes ports or xMII ports. So they might use "managed"
> or "fixed-link". The only thing that you can enforce is that, if the slave_mii_bus
> has an OF presence, then user ports must have phylink bindings.

Got it. This should be the correct schema then. I don't check for ethernet
or link as when the mdio property is used, these bindings apply to all
ports. DSA and CPU ports are then further restricted with the dsa-port.yaml
schema.

if:
   required: [ mdio ]
then:
   patternProperties:
     "^(ethernet-)?ports$":
       patternProperties:
         "^(ethernet-)?port@[0-9]+$":
           oneOf:
             - required:
                 - fixed-link
             - required:
                 - phy-handle
             - required:
                 - managed

Arınç

[PATCH 3/4] dt-bindings: net: dsa: realtek: require compatible property under mdio node

[Arınç ÜNAL]

The compatible property must be defined under the mdio node. Enforce it.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index ea7db0890abc..f600e65fc990 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -103,6 +103,9 @@ allOf:
             compatible:
               const: realtek,smi-mdio
 
+          required:
+            - compatible
+
   - if:
       required:
         - reg
-- 
2.39.2

Re: [PATCH 3/4] dt-bindings: net: dsa: realtek: require compatible property under mdio node

[Arınç ÜNAL]

I will drop this patch because only the SMI controlled switches must define
the compatible string. There's no way to make a distinction between the SMI
and MDIO controlled switches on the schema so this would mean requiring the
compatible property for the MDIO controlled switches too which is wrong.

Arınç

On 12.08.2023 12:17, Arınç ÜNAL wrote:
> The compatible property must be defined under the mdio node. Enforce it.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>   Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index ea7db0890abc..f600e65fc990 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -103,6 +103,9 @@ allOf:
>               compatible:
>                 const: realtek,smi-mdio
>   
> +          required:
> +            - compatible
> +
>     - if:
>         required:
>           - reg

Re: [PATCH 3/4] dt-bindings: net: dsa: realtek: require compatible property under mdio node

[Vladimir Oltean]

On Sat, Aug 12, 2023 at 12:17:07PM +0300, Arınç ÜNAL wrote:
> The compatible property must be defined under the mdio node. Enforce it.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index ea7db0890abc..f600e65fc990 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -103,6 +103,9 @@ allOf:
>              compatible:
>                const: realtek,smi-mdio
>  
> +          required:
> +            - compatible
> +
>    - if:
>        required:
>          - reg
> -- 
> 2.39.2
> 

Re: [PATCH 3/4] dt-bindings: net: dsa: realtek: require compatible property under mdio node

[Linus Walleij]

On Sat, Aug 12, 2023 at 11:17 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:

> The compatible property must be defined under the mdio node. Enforce it.
>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Right.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/4] dt-bindings: net: dsa: realtek: require compatible property under mdio node

[Florian Fainelli]

On 8/12/23 02:17, Arınç ÜNAL wrote:
> The compatible property must be defined under the mdio node. Enforce it.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH 4/4] net: dsa: mt7530: register OF node for internal MDIO bus

[Arınç ÜNAL]

From: David Bauer <mail@david-bauer.net>

The MT753x switches provide a switch-internal MDIO bus for the embedded
PHYs.

Register a OF sub-node on the switch OF-node for this internal MDIO bus.
This allows to configure the embedded PHYs using device-tree.

Signed-off-by: David Bauer <mail@david-bauer.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8fbda739c1b3..9bb805de397a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2152,10 +2152,13 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 {
 	struct dsa_switch *ds = priv->ds;
 	struct device *dev = priv->dev;
+	struct device_node *np, *mnp;
 	struct mii_bus *bus;
 	static int idx;
 	int ret;
 
+	np = priv->dev->of_node;
+
 	bus = devm_mdiobus_alloc(dev);
 	if (!bus)
 		return -ENOMEM;
@@ -2174,7 +2177,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	if (priv->irq)
 		mt7530_setup_mdio_irq(priv);
 
-	ret = devm_mdiobus_register(dev, bus);
+	mnp = of_get_child_by_name(np, "mdio");
+	ret = devm_of_mdiobus_register(dev, bus, mnp);
+	of_node_put(mnp);
 	if (ret) {
 		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
 		if (priv->irq)
-- 
2.39.2