Message-ID: <3f262579-eec1-4b21-9b18-1d1d612e715b@arinc9.com>
Date:   Sat, 12 Aug 2023 01:45:29 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Daniel Golle <daniel@...rotopia.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Sean Wang <sean.wang@...iatek.com>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa:
 mediatek,mt7530: document MDIO-bus

On 10.08.2023 01:01, Vladimir Oltean wrote:
> On Wed, Aug 09, 2023 at 12:03:19PM +0300, Arınç ÜNAL wrote:
>> On 8.08.2023 15:17, Vladimir Oltean wrote:
>>> On Sat, Aug 05, 2023 at 11:15:15PM +0300, Arınç ÜNAL wrote:
>>>> I don't see a reason to resubmit this without addressing the requested
>>>> change.
>>>>
>>>>>> Wouldn't we just skip the whole issue by documenting the need for defining all PHYs
>>>>>> used on the switch when defining the MDIO bus?
>>>>>
>>>>> Good idea, please do that.
>>>>
>>>> https://lore.kernel.org/netdev/0f501bb6-18a0-1713-b08c-6ad244c022ec@arinc9.com/
>>>>
>>>> Arınç
>>>
>>> Arınç, where do you see that comment being added? AFAIU, it is a
>>> characteristic of the generic __of_mdiobus_register() code to set
>>> mdio->phy_mask = ~0, and nothing specific to the mt7530.
>>
>> What I believe is specific to DSA is, 1:1 mapping of the port reg to the
>> PHY reg on the mdio bus is disabled if the mdio bus is defined. Therefore,
>> I believe a notice like below fits mediatek,mt7530.yaml.
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> index e532c6b795f4..c59d58252cd5 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> @@ -128,6 +128,15 @@ properties:
>>         See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt for
>>         details for the regulator setup on these boards.
>> +  mdio:
>> +    $ref: /schemas/net/mdio.yaml#
>> +    unevaluatedProperties: false
>> +    description:
>> +      Node for the internal MDIO bus connected to the embedded ethernet-PHYs.
>> +      For every port defined under the "^(ethernet-)?ports$" node, a PHY must be
>> +      defined under here and a phy-handle property must be defined under the
>> +      port node to point to the PHY node.
>> +
>>     mediatek,mcm:
>>       type: boolean
>>       description:
>>
>> Arınç
> 
> In that case, putting the comment here would make more sense, no?
> (and maybe enforcing an actual schema, but I've no idea how to do that)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> index 480120469953..5a415f12f162 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> @@ -59,7 +59,14 @@ properties:
>         - rtl8_4t
>         - seville
> 
> -# CPU and DSA ports must have phylink-compatible link descriptions
> +# CPU and DSA ports must have phylink-compatible link descriptions.
> +# On user ports, these are also supported, but are optional and may be omitted,
> +# meaning that these ports are implicitly connected to a PHY on an internal
> +# MDIO bus of the switch that isn't described in the device tree. If the switch
> +# does have a child node for the internal MDIO bus, the phylink-compatible
> +# bindings are also required (even if this is not enforced here). The detection
> +# of an internal MDIO bus is model-specific and may involve matching on the
> +# "mdio" node name or compatible string.
>   if:
>     oneOf:
>       - required: [ ethernet ]
> 
> Since commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), DSA as a
> framework also supports auto-creating an internal MDIO bus based on the
> presence of the "mdio" node name, so I guess it makes sense for the
> "mdio" to appear in the generic dsa.yaml if there's nothing else that's
> special about it.

I agree with this. I've done this which works. It's even found a port
node with the ethernet property missing, as it should've.

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/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 8d7e878b84dc..fe1e2008995d 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -78,6 +78,16 @@ examples:
              };
      };
  
+    macb1 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            fixed-link {
+                    speed = <1000>;
+                    full-duplex;
+            };
+    };
+
      spi {
              #address-cells = <1>;
              #size-cells = <0>;
@@ -138,6 +148,7 @@ examples:
                                      phy-mode = "rgmii";
                                      tx-internal-delay-ps = <2000>;
                                      rx-internal-delay-ps = <2000>;
+                                    ethernet = <&macb0>;
  
                                      fixed-link {
                                              speed = <1000>;
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..f600e65fc990 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@...aro.org>
  
@@ -95,37 +92,41 @@ 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
+
+          required:
+            - compatible
+
+  - 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


> 
> Also, in the earlier patch version you had replied to David Bauer:
> 
> | > While i was not aware of this side effect, I don't see how this breaks the ABI.
> |
> | Your patch doesn't break it, my then-intention of doing PHY muxing by
> | utilising this would. Your first patch is perfectly fine as is.
> 
> Could you please clarify what is your valid use case for not having a
> phy-handle to a PHY on an MDIO bus that is otherwise present in OF?

I had one possible use case, PHY muxing, but it has nothing to do with
the PHY registers of the PHYs on the internal MDIO bus so it's not a
valid use case.

> It doesn't _have_ to be broken. Since DSA knows the addresses of the
> internal PHYs, it can circumvent the lack of auto-scanning by manually
> calling get_phy_device() at the right (port-based) MDIO addresses.
> But any patch would need to have a clear reason before being considered
> for merging.

I think circumventing the mdio node for ports without a phy-handle will
bring unnecessary complexity and confusion as I may define a port with
reg = <1> with phy-handle to a phy with reg = <4>. In that case, a port
with reg = <4> without a phy-handle would try the phy with reg <4> and fail.

This is of course if I understood correctly that "(port-based) MDIO
addresses" means reading the MDIO address of a phy from the reg of a
port defined under the ports node.

Arınç

Powered by blists - more mailing lists