linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
@ 2022-05-02 15:32 Frank Wunderlich
  2022-05-03 12:05 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Frank Wunderlich @ 2022-05-02 15:32 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
	David S. Miller, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Matthias Brugger, Sean Wang, Landen Chao,
	DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek
  Cc: Frank Wunderlich

From: Frank Wunderlich <frank-w@public-files.de>

Convert txt binding to yaml binding for Mediatek switches.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
 .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
 2 files changed, 435 insertions(+), 327 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt

diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
new file mode 100644
index 000000000000..c1724809d34e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
@@ -0,0 +1,435 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/mediatek.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek MT7530 Ethernet switch
+
+maintainers:
+  - Sean Wang <sean.wang@mediatek.com>
+  - Landen Chao <Landen.Chao@mediatek.com>
+  - DENG Qingfang <dqfext@gmail.com>
+
+description: |
+  Port 5 of mt7530 and mt7621 switch is muxed between:
+  1. GMAC5: GMAC5 can interface with another external MAC or PHY.
+  2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
+     of the SOC. Used in many setups where port 0/4 becomes the WAN port.
+     Note: On a MT7621 SOC with integrated switch: 2nd GMAC can only connected to
+       GMAC5 when the gpios for RGMII2 (GPIO 22-33) are not used and not
+       connected to external component!
+
+  Port 5 modes/configurations:
+  1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
+     GMAC of the SOC.
+     In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
+     GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
+  2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
+     It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
+     and RGMII delay.
+  3. Port 5 is muxed to GMAC5 and can interface to an external phy.
+     Port 5 becomes an extra switch port.
+     Only works on platform where external phy TX<->RX lines are swapped.
+     Like in the Ubiquiti ER-X-SFP.
+  4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
+     Currently a 2nd CPU port is not supported by DSA code.
+
+  Depending on how the external PHY is wired:
+  1. normal: The PHY can only connect to 2nd GMAC but not to the switch
+  2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
+     a ethernet port. But can't interface to the 2nd GMAC.
+
+    Based on the DT the port 5 mode is configured.
+
+  Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
+  When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
+  phy-mode must be set, see also example 2 below!
+  * mt7621: phy-mode = "rgmii-txid";
+  * mt7623: phy-mode = "rgmii";
+
+  CPU-Ports need a phy-mode property:
+    Allowed values on mt7530 and mt7621:
+      - "rgmii"
+      - "trgmii"
+    On mt7531:
+      - "1000base-x"
+      - "2500base-x"
+      - "sgmii"
+
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt7530
+      - mediatek,mt7531
+      - mediatek,mt7621
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  core-supply:
+    description: |
+      Phandle to the regulator node necessary for the core power.
+
+  "#gpio-cells":
+    description: |
+      Must be 2 if gpio-controller is defined.
+    const: 2
+
+  gpio-controller:
+    type: boolean
+    description: |
+      Boolean; if defined, MT7530's LED controller will run on
+      GPIO mode.
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupt-controller:
+    type: boolean
+    description: |
+      Boolean; Enables the internal interrupt controller.
+
+  interrupts:
+    description: |
+      Parent interrupt for the interrupt controller.
+    maxItems: 1
+
+  io-supply:
+    description: |
+      Phandle to the regulator node necessary for the I/O power.
+      See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
+      for details for the regulator setup on these boards.
+
+  mediatek,mcm:
+    type: boolean
+    description: |
+      Boolean; if defined, indicates that either MT7530 is the part
+      on multi-chip module belong to MT7623A has or the remotely standalone
+      chip as the function MT7623N reference board provided for.
+
+  reset-gpios:
+    description: |
+      Should be a gpio specifier for a reset line.
+    maxItems: 1
+
+  reset-names:
+    description: |
+      Should be set to "mcm".
+    const: mcm
+
+  resets:
+    description: |
+      Phandle pointing to the system reset controller with
+      line index for the ethsys.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: "dsa.yaml#"
+  - if:
+      required:
+        - mediatek,mcm
+    then:
+      required:
+        - resets
+        - reset-names
+    else:
+      required:
+        - reset-gpios
+
+  - if:
+      required:
+        - interrupt-controller
+    then:
+      required:
+        - "#interrupt-cells"
+        - interrupts
+
+  - if:
+      properties:
+        compatible:
+          items:
+            - const: mediatek,mt7530
+    then:
+      required:
+        - core-supply
+        - io-supply
+
+
+patternProperties:
+  "^ports$":
+    type: object
+
+    patternProperties:
+      "^port@[0-9]+$":
+        type: object
+        description: Ethernet switch ports
+
+        $ref: dsa-port.yaml#
+
+        properties:
+          reg:
+            description: |
+              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
+
+        unevaluatedProperties: false
+
+        allOf:
+          - if:
+              properties:
+                label:
+                  items:
+                    - const: cpu
+            then:
+              required:
+                - reg
+                - phy-mode
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        switch@0 {
+            compatible = "mediatek,mt7530";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0>;
+
+            core-supply = <&mt6323_vpa_reg>;
+            io-supply = <&mt6323_vemc3v3_reg>;
+            reset-gpios = <&pio 33 0>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    label = "lan0";
+                };
+
+                port@1 {
+                    reg = <1>;
+                    label = "lan1";
+                };
+
+                port@2 {
+                    reg = <2>;
+                    label = "lan2";
+                };
+
+                port@3 {
+                    reg = <3>;
+                    label = "lan3";
+                };
+
+                port@4 {
+                    reg = <4>;
+                    label = "wan";
+                };
+
+                port@6 {
+                    reg = <6>;
+                    label = "cpu";
+                    ethernet = <&gmac0>;
+                    phy-mode = "trgmii";
+                    fixed-link {
+                        speed = <1000>;
+                        full-duplex;
+                    };
+                };
+            };
+        };
+    };
+
+  - |
+    //Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
+
+    eth {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        gmac0: mac@0 {
+            compatible = "mediatek,eth-mac";
+            reg = <0>;
+            phy-mode = "rgmii";
+
+            fixed-link {
+                speed = <1000>;
+                full-duplex;
+                pause;
+            };
+        };
+
+        gmac1: mac@1 {
+            compatible = "mediatek,eth-mac";
+            reg = <1>;
+            phy-mode = "rgmii-txid";
+            phy-handle = <&phy4>;
+        };
+
+        mdio: mdio-bus {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            /* Internal phy */
+            phy4: ethernet-phy@4 {
+                reg = <4>;
+            };
+
+            mt7530: switch@1f {
+                compatible = "mediatek,mt7621";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0x1f>;
+                mediatek,mcm;
+
+                resets = <&rstctrl 2>;
+                reset-names = "mcm";
+
+                ports {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    port@0 {
+                        reg = <0>;
+                        label = "lan0";
+                    };
+
+                    port@1 {
+                        reg = <1>;
+                        label = "lan1";
+                    };
+
+                    port@2 {
+                        reg = <2>;
+                        label = "lan2";
+                    };
+
+                    port@3 {
+                        reg = <3>;
+                        label = "lan3";
+                    };
+
+        /* Commented out. Port 4 is handled by 2nd GMAC.
+                    port@4 {
+                        reg = <4>;
+                        label = "lan4";
+                    };
+        */
+
+                    port@6 {
+                        reg = <6>;
+                        label = "cpu";
+                        ethernet = <&gmac0>;
+                        phy-mode = "rgmii";
+
+                        fixed-link {
+                            speed = <1000>;
+                            full-duplex;
+                            pause;
+                        };
+                    };
+                };
+            };
+        };
+    };
+
+  - |
+    //Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
+
+    eth {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        gmac_0: mac@0 {
+            compatible = "mediatek,eth-mac";
+            reg = <0>;
+            phy-mode = "rgmii";
+
+            fixed-link {
+                speed = <1000>;
+                full-duplex;
+                pause;
+            };
+        };
+
+        mdio0: mdio-bus {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            /* External phy */
+            ephy5: ethernet-phy@7 {
+                reg = <7>;
+            };
+
+            switch@1f {
+                compatible = "mediatek,mt7621";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0x1f>;
+                mediatek,mcm;
+
+                resets = <&rstctrl 2>;
+                reset-names = "mcm";
+
+                ports {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    port@0 {
+                        reg = <0>;
+                        label = "lan0";
+                    };
+
+                    port@1 {
+                        reg = <1>;
+                        label = "lan1";
+                    };
+
+                    port@2 {
+                        reg = <2>;
+                        label = "lan2";
+                    };
+
+                    port@3 {
+                        reg = <3>;
+                        label = "lan3";
+                    };
+
+                    port@4 {
+                        reg = <4>;
+                        label = "lan4";
+                    };
+
+                    port@5 {
+                        reg = <5>;
+                        label = "lan5";
+                        phy-mode = "rgmii";
+                        phy-handle = <&ephy5>;
+                    };
+
+                    cpu_port0: port@6 {
+                        reg = <6>;
+                        label = "cpu";
+                        ethernet = <&gmac_0>;
+                        phy-mode = "rgmii";
+
+                        fixed-link {
+                            speed = <1000>;
+                            full-duplex;
+                            pause;
+                        };
+                    };
+                };
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
deleted file mode 100644
index 18247ebfc487..000000000000
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ /dev/null
@@ -1,327 +0,0 @@
-Mediatek MT7530 Ethernet switch
-================================
-
-Required properties:
-
-- compatible: may be compatible = "mediatek,mt7530"
-	or compatible = "mediatek,mt7621"
-	or compatible = "mediatek,mt7531"
-- #address-cells: Must be 1.
-- #size-cells: Must be 0.
-- mediatek,mcm: Boolean; if defined, indicates that either MT7530 is the part
-	on multi-chip module belong to MT7623A has or the remotely standalone
-	chip as the function MT7623N reference board provided for.
-
-If compatible mediatek,mt7530 is set then the following properties are required
-
-- core-supply: Phandle to the regulator node necessary for the core power.
-- io-supply: Phandle to the regulator node necessary for the I/O power.
-	See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
-	for details for the regulator setup on these boards.
-
-If the property mediatek,mcm isn't defined, following property is required
-
-- reset-gpios: Should be a gpio specifier for a reset line.
-
-Else, following properties are required
-
-- resets : Phandle pointing to the system reset controller with
-	line index for the ethsys.
-- reset-names : Should be set to "mcm".
-
-Required properties for the child nodes within ports container:
-
-- reg: Port address described must be 6 for CPU port and from 0 to 5 for
-	user ports.
-- phy-mode: String, the following values are acceptable for port labeled
-	"cpu":
-	If compatible mediatek,mt7530 or mediatek,mt7621 is set,
-	must be either "trgmii" or "rgmii"
-	If compatible mediatek,mt7531 is set,
-	must be either "sgmii", "1000base-x" or "2500base-x"
-
-Port 5 of mt7530 and mt7621 switch is muxed between:
-1. GMAC5: GMAC5 can interface with another external MAC or PHY.
-2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
-   of the SOC. Used in many setups where port 0/4 becomes the WAN port.
-   Note: On a MT7621 SOC with integrated switch: 2nd GMAC can only connected to
-	 GMAC5 when the gpios for RGMII2 (GPIO 22-33) are not used and not
-	 connected to external component!
-
-Port 5 modes/configurations:
-1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
-   GMAC of the SOC.
-   In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
-   GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
-2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
-   It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
-   and RGMII delay.
-3. Port 5 is muxed to GMAC5 and can interface to an external phy.
-   Port 5 becomes an extra switch port.
-   Only works on platform where external phy TX<->RX lines are swapped.
-   Like in the Ubiquiti ER-X-SFP.
-4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
-   Currently a 2nd CPU port is not supported by DSA code.
-
-Depending on how the external PHY is wired:
-1. normal: The PHY can only connect to 2nd GMAC but not to the switch
-2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
-   a ethernet port. But can't interface to the 2nd GMAC.
-
-Based on the DT the port 5 mode is configured.
-
-Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
-When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
-phy-mode must be set, see also example 2 below!
- * mt7621: phy-mode = "rgmii-txid";
- * mt7623: phy-mode = "rgmii";
-
-Optional properties:
-
-- gpio-controller: Boolean; if defined, MT7530's LED controller will run on
-	GPIO mode.
-- #gpio-cells: Must be 2 if gpio-controller is defined.
-- interrupt-controller: Boolean; Enables the internal interrupt controller.
-
-If interrupt-controller is defined, the following properties are required.
-
-- #interrupt-cells: Must be 1.
-- interrupts: Parent interrupt for the interrupt controller.
-
-See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
-required, optional properties and how the integrated switch subnodes must
-be specified.
-
-Example:
-
-	&mdio0 {
-		switch@0 {
-			compatible = "mediatek,mt7530";
-			#address-cells = <1>;
-			#size-cells = <0>;
-			reg = <0>;
-
-			core-supply = <&mt6323_vpa_reg>;
-			io-supply = <&mt6323_vemc3v3_reg>;
-			reset-gpios = <&pio 33 0>;
-
-			ports {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				reg = <0>;
-				port@0 {
-					reg = <0>;
-					label = "lan0";
-				};
-
-				port@1 {
-					reg = <1>;
-					label = "lan1";
-				};
-
-				port@2 {
-					reg = <2>;
-					label = "lan2";
-				};
-
-				port@3 {
-					reg = <3>;
-					label = "lan3";
-				};
-
-				port@4 {
-					reg = <4>;
-					label = "wan";
-				};
-
-				port@6 {
-					reg = <6>;
-					label = "cpu";
-					ethernet = <&gmac0>;
-					phy-mode = "trgmii";
-					fixed-link {
-						speed = <1000>;
-						full-duplex;
-					};
-				};
-			};
-		};
-	};
-
-Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
-
-&eth {
-	gmac0: mac@0 {
-		compatible = "mediatek,eth-mac";
-		reg = <0>;
-		phy-mode = "rgmii";
-
-		fixed-link {
-			speed = <1000>;
-			full-duplex;
-			pause;
-		};
-	};
-
-	gmac1: mac@1 {
-		compatible = "mediatek,eth-mac";
-		reg = <1>;
-		phy-mode = "rgmii-txid";
-		phy-handle = <&phy4>;
-	};
-
-	mdio: mdio-bus {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		/* Internal phy */
-		phy4: ethernet-phy@4 {
-			reg = <4>;
-		};
-
-		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
-			#address-cells = <1>;
-			#size-cells = <0>;
-			reg = <0x1f>;
-			pinctrl-names = "default";
-			mediatek,mcm;
-
-			resets = <&rstctrl 2>;
-			reset-names = "mcm";
-
-			ports {
-				#address-cells = <1>;
-				#size-cells = <0>;
-
-				port@0 {
-					reg = <0>;
-					label = "lan0";
-				};
-
-				port@1 {
-					reg = <1>;
-					label = "lan1";
-				};
-
-				port@2 {
-					reg = <2>;
-					label = "lan2";
-				};
-
-				port@3 {
-					reg = <3>;
-					label = "lan3";
-				};
-
-/* Commented out. Port 4 is handled by 2nd GMAC.
-				port@4 {
-					reg = <4>;
-					label = "lan4";
-				};
-*/
-
-				cpu_port0: port@6 {
-					reg = <6>;
-					label = "cpu";
-					ethernet = <&gmac0>;
-					phy-mode = "rgmii";
-
-					fixed-link {
-						speed = <1000>;
-						full-duplex;
-						pause;
-					};
-				};
-			};
-		};
-	};
-};
-
-Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
-
-&eth {
-	gmac0: mac@0 {
-		compatible = "mediatek,eth-mac";
-		reg = <0>;
-		phy-mode = "rgmii";
-
-		fixed-link {
-			speed = <1000>;
-			full-duplex;
-			pause;
-		};
-	};
-
-	mdio: mdio-bus {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		/* External phy */
-		ephy5: ethernet-phy@7 {
-			reg = <7>;
-		};
-
-		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
-			#address-cells = <1>;
-			#size-cells = <0>;
-			reg = <0x1f>;
-			pinctrl-names = "default";
-			mediatek,mcm;
-
-			resets = <&rstctrl 2>;
-			reset-names = "mcm";
-
-			ports {
-				#address-cells = <1>;
-				#size-cells = <0>;
-
-				port@0 {
-					reg = <0>;
-					label = "lan0";
-				};
-
-				port@1 {
-					reg = <1>;
-					label = "lan1";
-				};
-
-				port@2 {
-					reg = <2>;
-					label = "lan2";
-				};
-
-				port@3 {
-					reg = <3>;
-					label = "lan3";
-				};
-
-				port@4 {
-					reg = <4>;
-					label = "lan4";
-				};
-
-				port@5 {
-					reg = <5>;
-					label = "lan5";
-					phy-mode = "rgmii";
-					phy-handle = <&ephy5>;
-				};
-
-				cpu_port0: port@6 {
-					reg = <6>;
-					label = "cpu";
-					ethernet = <&gmac0>;
-					phy-mode = "rgmii";
-
-					fixed-link {
-						speed = <1000>;
-						full-duplex;
-						pause;
-					};
-				};
-			};
-		};
-	};
-};
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-02 15:32 [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches Frank Wunderlich
@ 2022-05-03 12:05 ` Krzysztof Kozlowski
  2022-05-03 14:10   ` Aw: " Frank Wunderlich
  2022-05-04 10:21   ` Aw: " Frank Wunderlich
  0 siblings, 2 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-03 12:05 UTC (permalink / raw)
  To: Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek
  Cc: Frank Wunderlich

On 02/05/2022 17:32, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Convert txt binding to yaml binding for Mediatek switches.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
>  .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
>  2 files changed, 435 insertions(+), 327 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> new file mode 100644
> index 000000000000..c1724809d34e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml

Specific name please, so previous (with vendor prefix) was better:
mediatek,mt7530.yaml

> @@ -0,0 +1,435 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

You should CC previous contributors and get their acks on this. You
copied here a lot of description.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/mediatek.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek MT7530 Ethernet switch
> +
> +maintainers:
> +  - Sean Wang <sean.wang@mediatek.com>
> +  - Landen Chao <Landen.Chao@mediatek.com>
> +  - DENG Qingfang <dqfext@gmail.com>
> +
> +description: |
> +  Port 5 of mt7530 and mt7621 switch is muxed between:
> +  1. GMAC5: GMAC5 can interface with another external MAC or PHY.
> +  2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
> +     of the SOC. Used in many setups where port 0/4 becomes the WAN port.
> +     Note: On a MT7621 SOC with integrated switch: 2nd GMAC can only connected to
> +       GMAC5 when the gpios for RGMII2 (GPIO 22-33) are not used and not
> +       connected to external component!
> +
> +  Port 5 modes/configurations:
> +  1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
> +     GMAC of the SOC.
> +     In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
> +     GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
> +  2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
> +     It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
> +     and RGMII delay.
> +  3. Port 5 is muxed to GMAC5 and can interface to an external phy.
> +     Port 5 becomes an extra switch port.
> +     Only works on platform where external phy TX<->RX lines are swapped.
> +     Like in the Ubiquiti ER-X-SFP.
> +  4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
> +     Currently a 2nd CPU port is not supported by DSA code.
> +
> +  Depending on how the external PHY is wired:
> +  1. normal: The PHY can only connect to 2nd GMAC but not to the switch
> +  2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
> +     a ethernet port. But can't interface to the 2nd GMAC.
> +
> +    Based on the DT the port 5 mode is configured.
> +
> +  Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
> +  When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
> +  phy-mode must be set, see also example 2 below!
> +  * mt7621: phy-mode = "rgmii-txid";
> +  * mt7623: phy-mode = "rgmii";
> +
> +  CPU-Ports need a phy-mode property:
> +    Allowed values on mt7530 and mt7621:
> +      - "rgmii"
> +      - "trgmii"
> +    On mt7531:
> +      - "1000base-x"
> +      - "2500base-x"
> +      - "sgmii"
> +
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt7530
> +      - mediatek,mt7531
> +      - mediatek,mt7621
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  core-supply:
> +    description: |
> +      Phandle to the regulator node necessary for the core power.
> +
> +  "#gpio-cells":
> +    description: |
> +      Must be 2 if gpio-controller is defined.
> +    const: 2
> +
> +  gpio-controller:
> +    type: boolean
> +    description: |
> +      Boolean; if defined, MT7530's LED controller will run on

No need to repeat Boolean.

> +      GPIO mode.
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  interrupt-controller:
> +    type: boolean
> +    description: |
> +      Boolean; Enables the internal interrupt controller.

Skip description.

> +
> +  interrupts:
> +    description: |
> +      Parent interrupt for the interrupt controller.

Skip description.

> +    maxItems: 1
> +
> +  io-supply:
> +    description: |
> +      Phandle to the regulator node necessary for the I/O power.
> +      See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
> +      for details for the regulator setup on these boards.
> +
> +  mediatek,mcm:
> +    type: boolean
> +    description: |
> +      Boolean; 

No need to repeat Boolean.

> if defined, indicates that either MT7530 is the part
> +      on multi-chip module belong to MT7623A has or the remotely standalone
> +      chip as the function MT7623N reference board provided for.
> +
> +  reset-gpios:
> +    description: |
> +      Should be a gpio specifier for a reset line.
> +    maxItems: 1
> +
> +  reset-names:
> +    description: |
> +      Should be set to "mcm".
> +    const: mcm
> +
> +  resets:
> +    description: |
> +      Phandle pointing to the system reset controller with
> +      line index for the ethsys.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

What about address/size cells?

> +
> +allOf:
> +  - $ref: "dsa.yaml#"
> +  - if:
> +      required:
> +        - mediatek,mcm

Original bindings had this reversed.

> +    then:
> +      required:
> +        - resets
> +        - reset-names
> +    else:
> +      required:
> +        - reset-gpios
> +
> +  - if:
> +      required:
> +        - interrupt-controller
> +    then:
> +      required:
> +        - "#interrupt-cells"

This should come from dt schema already...

> +        - interrupts
> +
> +  - if:
> +      properties:
> +        compatible:
> +          items:
> +            - const: mediatek,mt7530
> +    then:
> +      required:
> +        - core-supply
> +        - io-supply
> +
> +
> +patternProperties:
> +  "^ports$":

It''s not a pattern, so put it under properties, like regular property.

> +    type: object
> +
> +    patternProperties:
> +      "^port@[0-9]+$":
> +        type: object
> +        description: Ethernet switch ports
> +
> +        $ref: dsa-port.yaml#

This should go to allOf below.

> +
> +        properties:
> +          reg:
> +            description: |
> +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
> +
> +        unevaluatedProperties: false
> +
> +        allOf:
> +          - if:
> +              properties:
> +                label:
> +                  items:
> +                    - const: cpu
> +            then:
> +              required:
> +                - reg
> +                - phy-mode
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio0 {

Just mdio

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        switch@0 {
> +            compatible = "mediatek,mt7530";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0>;
> +
> +            core-supply = <&mt6323_vpa_reg>;
> +            io-supply = <&mt6323_vemc3v3_reg>;
> +            reset-gpios = <&pio 33 0>;

Use GPIO flag define/constant.

> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                port@0 {
> +                    reg = <0>;
> +                    label = "lan0";
> +                };
> +
> +                port@1 {
> +                    reg = <1>;
> +                    label = "lan1";
> +                };
> +
> +                port@2 {
> +                    reg = <2>;
> +                    label = "lan2";
> +                };
> +
> +                port@3 {
> +                    reg = <3>;
> +                    label = "lan3";
> +                };
> +
> +                port@4 {
> +                    reg = <4>;
> +                    label = "wan";
> +                };
> +
> +                port@6 {
> +                    reg = <6>;
> +                    label = "cpu";
> +                    ethernet = <&gmac0>;
> +                    phy-mode = "trgmii";
> +                    fixed-link {
> +                        speed = <1000>;
> +                        full-duplex;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +
> +  - |
> +    //Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
> +
> +    eth {

s/eth/ethernet/

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        gmac0: mac@0 {
> +            compatible = "mediatek,eth-mac";
> +            reg = <0>;
> +            phy-mode = "rgmii";
> +
> +            fixed-link {
> +                speed = <1000>;
> +                full-duplex;
> +                pause;
> +            };
> +        };
> +
> +        gmac1: mac@1 {
> +            compatible = "mediatek,eth-mac";
> +            reg = <1>;
> +            phy-mode = "rgmii-txid";
> +            phy-handle = <&phy4>;
> +        };
> +
> +        mdio: mdio-bus {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            /* Internal phy */
> +            phy4: ethernet-phy@4 {
> +                reg = <4>;
> +            };
> +
> +            mt7530: switch@1f {
> +                compatible = "mediatek,mt7621";
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                reg = <0x1f>;
> +                mediatek,mcm;
> +
> +                resets = <&rstctrl 2>;
> +                reset-names = "mcm";
> +
> +                ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +
> +                    port@0 {
> +                        reg = <0>;
> +                        label = "lan0";
> +                    };
> +
> +                    port@1 {
> +                        reg = <1>;
> +                        label = "lan1";
> +                    };
> +
> +                    port@2 {
> +                        reg = <2>;
> +                        label = "lan2";
> +                    };
> +
> +                    port@3 {
> +                        reg = <3>;
> +                        label = "lan3";
> +                    };
> +
> +        /* Commented out. Port 4 is handled by 2nd GMAC.
> +                    port@4 {
> +                        reg = <4>;
> +                        label = "lan4";
> +                    };
> +        */

Messed up indentation
> +
> +                    port@6 {
> +                        reg = <6>;
> +                        label = "cpu";
> +                        ethernet = <&gmac0>;
> +                        phy-mode = "rgmii";
> +
> +                        fixed-link {
> +                            speed = <1000>;
> +                            full-duplex;
> +                            pause;
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +    };
> +
> +  - |
> +    //Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
> +
> +    eth {

Also ethernet?



Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-03 12:05 ` Krzysztof Kozlowski
@ 2022-05-03 14:10   ` Frank Wunderlich
  2022-05-03 14:40     ` Krzysztof Kozlowski
  2022-05-04 10:21   ` Aw: " Frank Wunderlich
  1 sibling, 1 reply; 9+ messages in thread
From: Frank Wunderlich @ 2022-05-03 14:10 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Greg Ungerer, René van Dorst,
	Mauro Carvalho Chehab
  Cc: Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

Hi,

thank you for first review.

> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>
> On 02/05/2022 17:32, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>
> >
> > Convert txt binding to yaml binding for Mediatek switches.
> >
> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > ---
> >  .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
> >  .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
> >  2 files changed, 435 insertions(+), 327 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> > new file mode 100644
> > index 000000000000..c1724809d34e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>
> Specific name please, so previous (with vendor prefix) was better:
> mediatek,mt7530.yaml

ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".

> > @@ -0,0 +1,435 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>
> You should CC previous contributors and get their acks on this. You
> copied here a lot of description.

added 3 Persons that made commits to txt before to let them know about this change

and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
compatible in subnode.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/dsa/mediatek.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek MT7530 Ethernet switch
> > +
> > +maintainers:
> > +  - Sean Wang <sean.wang@mediatek.com>
> > +  - Landen Chao <Landen.Chao@mediatek.com>
> > +  - DENG Qingfang <dqfext@gmail.com>
> > +
> > +description: |
> > +  Port 5 of mt7530 and mt7621 switch is muxed between:
> > +  1. GMAC5: GMAC5 can interface with another external MAC or PHY.
> > +  2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
> > +     of the SOC. Used in many setups where port 0/4 becomes the WAN port.
> > +     Note: On a MT7621 SOC with integrated switch: 2nd GMAC can only connected to
> > +       GMAC5 when the gpios for RGMII2 (GPIO 22-33) are not used and not
> > +       connected to external component!
> > +
> > +  Port 5 modes/configurations:
> > +  1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
> > +     GMAC of the SOC.
> > +     In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
> > +     GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
> > +  2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
> > +     It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
> > +     and RGMII delay.
> > +  3. Port 5 is muxed to GMAC5 and can interface to an external phy.
> > +     Port 5 becomes an extra switch port.
> > +     Only works on platform where external phy TX<->RX lines are swapped.
> > +     Like in the Ubiquiti ER-X-SFP.
> > +  4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
> > +     Currently a 2nd CPU port is not supported by DSA code.
> > +
> > +  Depending on how the external PHY is wired:
> > +  1. normal: The PHY can only connect to 2nd GMAC but not to the switch
> > +  2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
> > +     a ethernet port. But can't interface to the 2nd GMAC.
> > +
> > +    Based on the DT the port 5 mode is configured.
> > +
> > +  Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
> > +  When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
> > +  phy-mode must be set, see also example 2 below!
> > +  * mt7621: phy-mode = "rgmii-txid";
> > +  * mt7623: phy-mode = "rgmii";
> > +
> > +  CPU-Ports need a phy-mode property:
> > +    Allowed values on mt7530 and mt7621:
> > +      - "rgmii"
> > +      - "trgmii"
> > +    On mt7531:
> > +      - "1000base-x"
> > +      - "2500base-x"
> > +      - "sgmii"
> > +
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt7530
> > +      - mediatek,mt7531
> > +      - mediatek,mt7621
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  core-supply:
> > +    description: |
> > +      Phandle to the regulator node necessary for the core power.
> > +
> > +  "#gpio-cells":
> > +    description: |
> > +      Must be 2 if gpio-controller is defined.
> > +    const: 2
> > +
> > +  gpio-controller:
> > +    type: boolean
> > +    description: |
> > +      Boolean; if defined, MT7530's LED controller will run on
>
> No need to repeat Boolean.

ok, will change

> > +      GPIO mode.
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  interrupt-controller:
> > +    type: boolean
> > +    description: |
> > +      Boolean; Enables the internal interrupt controller.
>
> Skip description.

ok

> > +
> > +  interrupts:
> > +    description: |
> > +      Parent interrupt for the interrupt controller.
>
> Skip description.
ok

> > +    maxItems: 1
> > +
> > +  io-supply:
> > +    description: |
> > +      Phandle to the regulator node necessary for the I/O power.
> > +      See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
> > +      for details for the regulator setup on these boards.
> > +
> > +  mediatek,mcm:
> > +    type: boolean
> > +    description: |
> > +      Boolean;
>
> No need to repeat Boolean.

ack

> > if defined, indicates that either MT7530 is the part
> > +      on multi-chip module belong to MT7623A has or the remotely standalone
> > +      chip as the function MT7623N reference board provided for.
> > +
> > +  reset-gpios:
> > +    description: |
> > +      Should be a gpio specifier for a reset line.
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    description: |
> > +      Should be set to "mcm".
> > +    const: mcm
> > +
> > +  resets:
> > +    description: |
> > +      Phandle pointing to the system reset controller with
> > +      line index for the ethsys.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
>
> What about address/size cells?

you're right even if they are const to a value they need to be set

> > +
> > +allOf:
> > +  - $ref: "dsa.yaml#"
> > +  - if:
> > +      required:
> > +        - mediatek,mcm
>
> Original bindings had this reversed.

i know, but i think it is better readable and i will drop the else-part later.
Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.

i left this as separate commit to be posted later to have a nearly 1:1 conversion here.

> > +    then:
> > +      required:
> > +        - resets
> > +        - reset-names
> > +    else:
> > +      required:
> > +        - reset-gpios
> > +
> > +  - if:
> > +      required:
> > +        - interrupt-controller
> > +    then:
> > +      required:
> > +        - "#interrupt-cells"
>
> This should come from dt schema already...

so i should drop (complete block for interrupt controller)?

> > +        - interrupts
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          items:
> > +            - const: mediatek,mt7530
> > +    then:
> > +      required:
> > +        - core-supply
> > +        - io-supply
> > +
> > +
> > +patternProperties:
> > +  "^ports$":
>
> It''s not a pattern, so put it under properties, like regular property.

can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"

  ports:
    type: object

    patternProperties:
      "^port@[0-9]+$":
        type: object
        description: Ethernet switch ports

        properties:
          reg:
            description: |
              Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.

        unevaluatedProperties: false

        allOf:
          - $ref: dsa-port.yaml#
          - if:
....

basicly this "ports"-property should be required too, right?


> > +    type: object
> > +
> > +    patternProperties:
> > +      "^port@[0-9]+$":
> > +        type: object
> > +        description: Ethernet switch ports
> > +
> > +        $ref: dsa-port.yaml#
>
> This should go to allOf below.

see above

> > +
> > +        properties:
> > +          reg:
> > +            description: |
> > +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
> > +
> > +        unevaluatedProperties: false
> > +
> > +        allOf:
> > +          - if:
> > +              properties:
> > +                label:
> > +                  items:
> > +                    - const: cpu
> > +            then:
> > +              required:
> > +                - reg
> > +                - phy-mode
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    mdio0 {
>
> Just mdio

ok

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        switch@0 {
> > +            compatible = "mediatek,mt7530";
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            reg = <0>;
> > +
> > +            core-supply = <&mt6323_vpa_reg>;
> > +            io-supply = <&mt6323_vemc3v3_reg>;
> > +            reset-gpios = <&pio 33 0>;
>
> Use GPIO flag define/constant.

this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
constants too.

i guess
include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0

for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)

> > +
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +                port@0 {
> > +                    reg = <0>;
> > +                    label = "lan0";
> > +                };
> > +
> > +                port@1 {
> > +                    reg = <1>;
> > +                    label = "lan1";
> > +                };
> > +
> > +                port@2 {
> > +                    reg = <2>;
> > +                    label = "lan2";
> > +                };
> > +
> > +                port@3 {
> > +                    reg = <3>;
> > +                    label = "lan3";
> > +                };
> > +
> > +                port@4 {
> > +                    reg = <4>;
> > +                    label = "wan";
> > +                };
> > +
> > +                port@6 {
> > +                    reg = <6>;
> > +                    label = "cpu";
> > +                    ethernet = <&gmac0>;
> > +                    phy-mode = "trgmii";
> > +                    fixed-link {
> > +                        speed = <1000>;
> > +                        full-duplex;
> > +                    };
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +  - |
> > +    //Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
> > +
> > +    eth {
>
> s/eth/ethernet/

ok

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        gmac0: mac@0 {
> > +            compatible = "mediatek,eth-mac";
> > +            reg = <0>;
> > +            phy-mode = "rgmii";
> > +
> > +            fixed-link {
> > +                speed = <1000>;
> > +                full-duplex;
> > +                pause;
> > +            };
> > +        };
> > +
> > +        gmac1: mac@1 {
> > +            compatible = "mediatek,eth-mac";
> > +            reg = <1>;
> > +            phy-mode = "rgmii-txid";
> > +            phy-handle = <&phy4>;
> > +        };
> > +
> > +        mdio: mdio-bus {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            /* Internal phy */
> > +            phy4: ethernet-phy@4 {
> > +                reg = <4>;
> > +            };
> > +
> > +            mt7530: switch@1f {
> > +                compatible = "mediatek,mt7621";
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +                reg = <0x1f>;
> > +                mediatek,mcm;
> > +
> > +                resets = <&rstctrl 2>;
> > +                reset-names = "mcm";
> > +
> > +                ports {
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +
> > +                    port@0 {
> > +                        reg = <0>;
> > +                        label = "lan0";
> > +                    };
> > +
> > +                    port@1 {
> > +                        reg = <1>;
> > +                        label = "lan1";
> > +                    };
> > +
> > +                    port@2 {
> > +                        reg = <2>;
> > +                        label = "lan2";
> > +                    };
> > +
> > +                    port@3 {
> > +                        reg = <3>;
> > +                        label = "lan3";
> > +                    };
> > +
> > +        /* Commented out. Port 4 is handled by 2nd GMAC.
> > +                    port@4 {
> > +                        reg = <4>;
> > +                        label = "lan4";
> > +                    };
> > +        */
>
> Messed up indentation

will fix it

> > +
> > +                    port@6 {
> > +                        reg = <6>;
> > +                        label = "cpu";
> > +                        ethernet = <&gmac0>;
> > +                        phy-mode = "rgmii";
> > +
> > +                        fixed-link {
> > +                            speed = <1000>;
> > +                            full-duplex;
> > +                            pause;
> > +                        };
> > +                    };
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +  - |
> > +    //Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
> > +
> > +    eth {
>
> Also ethernet?

will do

> Best regards,
> Krzysztof

regards Frank

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-03 14:10   ` Aw: " Frank Wunderlich
@ 2022-05-03 14:40     ` Krzysztof Kozlowski
  2022-05-03 15:03       ` Aw: " Frank Wunderlich
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-03 14:40 UTC (permalink / raw)
  To: Frank Wunderlich, Greg Ungerer, René van Dorst,
	Mauro Carvalho Chehab
  Cc: Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

On 03/05/2022 16:10, Frank Wunderlich wrote:
> Hi,
> 
> thank you for first review.
> 
>> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
>> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>>
>> On 02/05/2022 17:32, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Convert txt binding to yaml binding for Mediatek switches.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>>> ---
>>>  .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
>>>  .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
>>>  2 files changed, 435 insertions(+), 327 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>> new file mode 100644
>>> index 000000000000..c1724809d34e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>
>> Specific name please, so previous (with vendor prefix) was better:
>> mediatek,mt7530.yaml
> 
> ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".
> 
>>> @@ -0,0 +1,435 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>
>> You should CC previous contributors and get their acks on this. You
>> copied here a lot of description.
> 
> added 3 Persons that made commits to txt before to let them know about this change
> 
> and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
> compatible in subnode.

I don't remember such syntax.

(...)

> 
>>> if defined, indicates that either MT7530 is the part
>>> +      on multi-chip module belong to MT7623A has or the remotely standalone
>>> +      chip as the function MT7623N reference board provided for.
>>> +
>>> +  reset-gpios:
>>> +    description: |
>>> +      Should be a gpio specifier for a reset line.
>>> +    maxItems: 1
>>> +
>>> +  reset-names:
>>> +    description: |
>>> +      Should be set to "mcm".
>>> +    const: mcm
>>> +
>>> +  resets:
>>> +    description: |
>>> +      Phandle pointing to the system reset controller with
>>> +      line index for the ethsys.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>
>> What about address/size cells?
> 
> you're right even if they are const to a value they need to be set
> 
>>> +
>>> +allOf:
>>> +  - $ref: "dsa.yaml#"
>>> +  - if:
>>> +      required:
>>> +        - mediatek,mcm
>>
>> Original bindings had this reversed.
> 
> i know, but i think it is better readable and i will drop the else-part later.
> Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
> as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.
> 
> i left this as separate commit to be posted later to have a nearly 1:1 conversion here.

Ah, I missed that actually your syntax is better. No need to
reverse/negate and the changes do not have to be strict 1:1.

> 
>>> +    then:
>>> +      required:
>>> +        - resets
>>> +        - reset-names
>>> +    else:
>>> +      required:
>>> +        - reset-gpios
>>> +
>>> +  - if:
>>> +      required:
>>> +        - interrupt-controller
>>> +    then:
>>> +      required:
>>> +        - "#interrupt-cells"
>>
>> This should come from dt schema already...
> 
> so i should drop (complete block for interrupt controller)?

The interrupts you need. What I mean, you can skip requirement of cells.

> 
>>> +        - interrupts
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          items:
>>> +            - const: mediatek,mt7530
>>> +    then:
>>> +      required:
>>> +        - core-supply
>>> +        - io-supply
>>> +
>>> +
>>> +patternProperties:
>>> +  "^ports$":
>>
>> It''s not a pattern, so put it under properties, like regular property.
> 
> can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"

Yes, subnodes stay with patternProperties.

> 
>   ports:
>     type: object
> 
>     patternProperties:
>       "^port@[0-9]+$":
>         type: object
>         description: Ethernet switch ports
> 
>         properties:
>           reg:
>             description: |
>               Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.
> 
>         unevaluatedProperties: false
> 
>         allOf:
>           - $ref: dsa-port.yaml#
>           - if:
> ....
> 
> basicly this "ports"-property should be required too, right?

Previous binding did not enforce it, I think, but it is reasonable to
require ports.

> 
> 
>>> +    type: object
>>> +
>>> +    patternProperties:
>>> +      "^port@[0-9]+$":
>>> +        type: object
>>> +        description: Ethernet switch ports
>>> +
>>> +        $ref: dsa-port.yaml#
>>
>> This should go to allOf below.
> 
> see above
> 
>>> +
>>> +        properties:
>>> +          reg:
>>> +            description: |
>>> +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
>>> +
>>> +        unevaluatedProperties: false
>>> +
>>> +        allOf:
>>> +          - if:
>>> +              properties:
>>> +                label:
>>> +                  items:
>>> +                    - const: cpu
>>> +            then:
>>> +              required:
>>> +                - reg
>>> +                - phy-mode
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    mdio0 {
>>
>> Just mdio
> 
> ok
> 
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        switch@0 {
>>> +            compatible = "mediatek,mt7530";
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +            reg = <0>;
>>> +
>>> +            core-supply = <&mt6323_vpa_reg>;
>>> +            io-supply = <&mt6323_vemc3v3_reg>;
>>> +            reset-gpios = <&pio 33 0>;
>>
>> Use GPIO flag define/constant.
> 
> this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
> constants too.
> 
> i guess
> include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0
> 
> for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
> header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)

ok, then my comment


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Aw: Re:  Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-03 14:40     ` Krzysztof Kozlowski
@ 2022-05-03 15:03       ` Frank Wunderlich
  2022-05-04  6:51         ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Frank Wunderlich @ 2022-05-03 15:03 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Ungerer, René van Dorst, Mauro Carvalho Chehab,
	Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

Hi,

> Gesendet: Dienstag, 03. Mai 2022 um 16:40 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> Betreff: Re: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>
> On 03/05/2022 16:10, Frank Wunderlich wrote:
> > Hi,
> >
> > thank you for first review.
> >
> >> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> >> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
> >>
> >> On 02/05/2022 17:32, Frank Wunderlich wrote:
> >>> From: Frank Wunderlich <frank-w@public-files.de>
> >>>
> >>> Convert txt binding to yaml binding for Mediatek switches.
> >>>
> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> >>> ---
> >>>  .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
> >>>  .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
> >>>  2 files changed, 435 insertions(+), 327 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>> new file mode 100644
> >>> index 000000000000..c1724809d34e
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>
> >> Specific name please, so previous (with vendor prefix) was better:
> >> mediatek,mt7530.yaml
> >
> > ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".
> >
> >>> @@ -0,0 +1,435 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>
> >> You should CC previous contributors and get their acks on this. You
> >> copied here a lot of description.
> >
> > added 3 Persons that made commits to txt before to let them know about this change
> >
> > and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
> > compatible in subnode.
>
> I don't remember such syntax.
>
> (...)

have not posted this version as it was failing in dtbs_check, this was how i tried:

https://github.com/frank-w/BPI-R2-4.14/blob/8f2033eb6fcae273580263c3f0b31f0d48821740/Documentation/devicetree/bindings/net/dsa/mediatek.yaml#L177


> >>> if defined, indicates that either MT7530 is the part
> >>> +      on multi-chip module belong to MT7623A has or the remotely standalone
> >>> +      chip as the function MT7623N reference board provided for.
> >>> +
> >>> +  reset-gpios:
> >>> +    description: |
> >>> +      Should be a gpio specifier for a reset line.
> >>> +    maxItems: 1
> >>> +
> >>> +  reset-names:
> >>> +    description: |
> >>> +      Should be set to "mcm".
> >>> +    const: mcm
> >>> +
> >>> +  resets:
> >>> +    description: |
> >>> +      Phandle pointing to the system reset controller with
> >>> +      line index for the ethsys.
> >>> +    maxItems: 1
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>
> >> What about address/size cells?
> >
> > you're right even if they are const to a value they need to be set
> >
> >>> +
> >>> +allOf:
> >>> +  - $ref: "dsa.yaml#"
> >>> +  - if:
> >>> +      required:
> >>> +        - mediatek,mcm
> >>
> >> Original bindings had this reversed.
> >
> > i know, but i think it is better readable and i will drop the else-part later.
> > Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
> > as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.
> >
> > i left this as separate commit to be posted later to have a nearly 1:1 conversion here.
>
> Ah, I missed that actually your syntax is better. No need to
> reverse/negate and the changes do not have to be strict 1:1.

yes, but a conversion implies same meaning, so changing things later ;)

> >>> +    then:
> >>> +      required:
> >>> +        - resets
> >>> +        - reset-names
> >>> +    else:
> >>> +      required:
> >>> +        - reset-gpios
> >>> +
> >>> +  - if:
> >>> +      required:
> >>> +        - interrupt-controller
> >>> +    then:
> >>> +      required:
> >>> +        - "#interrupt-cells"
> >>
> >> This should come from dt schema already...
> >
> > so i should drop (complete block for interrupt controller)?
>
> The interrupts you need. What I mean, you can skip requirement of cells.

ok, i drop only the #interrupt-cells

> >>> +        - interrupts
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          items:
> >>> +            - const: mediatek,mt7530
> >>> +    then:
> >>> +      required:
> >>> +        - core-supply
> >>> +        - io-supply
> >>> +
> >>> +
> >>> +patternProperties:
> >>> +  "^ports$":
> >>
> >> It''s not a pattern, so put it under properties, like regular property.
> >
> > can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"
>
> Yes, subnodes stay with patternProperties.
>
> >
> >   ports:
> >     type: object
> >
> >     patternProperties:
> >       "^port@[0-9]+$":
> >         type: object
> >         description: Ethernet switch ports
> >
> >         properties:
> >           reg:
> >             description: |
> >               Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.
> >
> >         unevaluatedProperties: false
> >
> >         allOf:
> >           - $ref: dsa-port.yaml#
> >           - if:
> > ....
> >
> > basicly this "ports"-property should be required too, right?
>
> Previous binding did not enforce it, I think, but it is reasonable to
> require ports.

basicly it is required in dsa.yaml, so it will be redundant here

https://elixir.bootlin.com/linux/v5.18-rc5/source/Documentation/devicetree/bindings/net/dsa/dsa.yaml#L55

this defines it as pattern "^(ethernet-)?ports$" and should be processed by dsa-core. so maybe changing it to same pattern instead of moving up as normal property?

> >>> +    type: object
> >>> +
> >>> +    patternProperties:
> >>> +      "^port@[0-9]+$":
> >>> +        type: object
> >>> +        description: Ethernet switch ports
> >>> +
> >>> +        $ref: dsa-port.yaml#
> >>
> >> This should go to allOf below.
> >
> > see above
> >
> >>> +
> >>> +        properties:
> >>> +          reg:
> >>> +            description: |
> >>> +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
> >>> +
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +        allOf:
> >>> +          - if:
> >>> +              properties:
> >>> +                label:
> >>> +                  items:
> >>> +                    - const: cpu
> >>> +            then:
> >>> +              required:
> >>> +                - reg
> >>> +                - phy-mode
> >>> +
> >>> +unevaluatedProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    mdio0 {
> >>
> >> Just mdio
> >
> > ok
> >
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <0>;
> >>> +        switch@0 {
> >>> +            compatible = "mediatek,mt7530";
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >>> +            reg = <0>;
> >>> +
> >>> +            core-supply = <&mt6323_vpa_reg>;
> >>> +            io-supply = <&mt6323_vemc3v3_reg>;
> >>> +            reset-gpios = <&pio 33 0>;
> >>
> >> Use GPIO flag define/constant.
> >
> > this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
> > constants too.
> >
> > i guess
> > include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0
> >
> > for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
> > header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)
>
> ok, then my comment

you mean adding a comment to the example that GPIO-flags/constants should be used instead of magic numbers?

> Best regards,
> Krzysztof

this is how it looks like without the port-property-change:
https://github.com/frank-w/BPI-R2-4.14/blob/5.18-mt7531-mainline/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml

regards Frank


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-03 15:03       ` Aw: " Frank Wunderlich
@ 2022-05-04  6:51         ` Krzysztof Kozlowski
  2022-05-04  7:44           ` Frank Wunderlich
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-04  6:51 UTC (permalink / raw)
  To: Frank Wunderlich
  Cc: Greg Ungerer, René van Dorst, Mauro Carvalho Chehab,
	Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

On 03/05/2022 17:03, Frank Wunderlich wrote:
> 
> have not posted this version as it was failing in dtbs_check, this was how i tried:
> 
> https://github.com/frank-w/BPI-R2-4.14/blob/8f2033eb6fcae273580263c3f0b31f0d48821740/Documentation/devicetree/bindings/net/dsa/mediatek.yaml#L177

You have mixed up indentation of the second if (and missing -).

(...)

>>>
>>> basicly this "ports"-property should be required too, right?
>>
>> Previous binding did not enforce it, I think, but it is reasonable to
>> require ports.
> 
> basicly it is required in dsa.yaml, so it will be redundant here
> 
> https://elixir.bootlin.com/linux/v5.18-rc5/source/Documentation/devicetree/bindings/net/dsa/dsa.yaml#L55
> 
> this defines it as pattern "^(ethernet-)?ports$" and should be processed by dsa-core. so maybe changing it to same pattern instead of moving up as normal property?

Just keep what is already used in existing DTS.

>>> for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
>>> header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)
>>
>> ok, then my comment
> 
> you mean adding a comment to the example that GPIO-flags/constants should be used instead of magic numbers?

I think something was cut from my reply. I wanted to say:
"ok, then my comment can be skipped"

But I think your check was not correct. I looked at bpi-r2 DTS (mt7623n)
and pio controller uses GPIO flags.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-04  6:51         ` Krzysztof Kozlowski
@ 2022-05-04  7:44           ` Frank Wunderlich
  2022-05-04  7:46             ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Frank Wunderlich @ 2022-05-04  7:44 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Ungerer, René van Dorst, Mauro Carvalho Chehab,
	Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

m 4. Mai 2022 08:51:41 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>On 03/05/2022 17:03, Frank Wunderlich wrote:
>> 
>> have not posted this version as it was failing in dtbs_check, this
>was how i tried:
>> 
>>
>https://github.com/frank-w/BPI-R2-4.14/blob/8f2033eb6fcae273580263c3f0b31f0d48821740/Documentation/devicetree/bindings/net/dsa/mediatek.yaml#L177
>
>You have mixed up indentation of the second if (and missing -).

The "compatible if" should be a child of the "if" above,because phy-mode property only exists for cpu-port. I can try with additional "-" (but i guess this is only needed for allOf)

Rob told me that i cannot check compatible in subnode and this check will be always true...just like my experience.
I can only make the compatible check at top-level and then need to define substructure based on this (so define structure twice). He suggested me adding this to description for now.

Imho this can be added later if really needed...did not found any example checking for compatible in a subnode. All were in top level. Afair these properties are handled by dsa-core/phylink and driver only compares constants set there.

>(...)
>
>>>>
>>>> basicly this "ports"-property should be required too, right?
>>>
>>> Previous binding did not enforce it, I think, but it is reasonable
>to
>>> require ports.
>> 
>> basicly it is required in dsa.yaml, so it will be redundant here
>> 
>>
>https://elixir.bootlin.com/linux/v5.18-rc5/source/Documentation/devicetree/bindings/net/dsa/dsa.yaml#L55
>> 
>> this defines it as pattern "^(ethernet-)?ports$" and should be
>processed by dsa-core. so maybe changing it to same pattern instead of
>moving up as normal property?
>
>Just keep what is already used in existing DTS.

Currently only "ports" is used...so i will change it to "normal" property.

>>>> for 33 there seem no constant..all other references to pio node are
>with numbers too and there seem no binding
>>>> header defining the gpio pins (only functions in
>include/dt-bindings/pinctrl/mt7623-pinfunc.h)
>>>
>>> ok, then my comment
>> 
>> you mean adding a comment to the example that GPIO-flags/constants
>should be used instead of magic numbers?
>
>I think something was cut from my reply. I wanted to say:
>"ok, then my comment can be skipped"

Ok

>But I think your check was not correct. I looked at bpi-r2 DTS
>(mt7623n)
>and pio controller uses GPIO flags.

I see only same as in the example

https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts#L196

>Best regards,
>Krzysztof
regards Frank

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-04  7:44           ` Frank Wunderlich
@ 2022-05-04  7:46             ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-04  7:46 UTC (permalink / raw)
  To: frank-w
  Cc: Greg Ungerer, René van Dorst, Mauro Carvalho Chehab,
	Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

On 04/05/2022 09:44, Frank Wunderlich wrote:
> m 4. Mai 2022 08:51:41 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>> On 03/05/2022 17:03, Frank Wunderlich wrote:
>>>
>>> have not posted this version as it was failing in dtbs_check, this
>> was how i tried:
>>>
>>>
>> https://github.com/frank-w/BPI-R2-4.14/blob/8f2033eb6fcae273580263c3f0b31f0d48821740/Documentation/devicetree/bindings/net/dsa/mediatek.yaml#L177
>>
>> You have mixed up indentation of the second if (and missing -).
> 
> The "compatible if" should be a child of the "if" above,because phy-mode property only exists for cpu-port. I can try with additional "-" (but i guess this is only needed for allOf)
> 
> Rob told me that i cannot check compatible in subnode and this check will be always true...just like my experience.
> I can only make the compatible check at top-level and then need to define substructure based on this (so define structure twice). He suggested me adding this to description for now.
> 
> Imho this can be added later if really needed...did not found any example checking for compatible in a subnode. All were in top level. Afair these properties are handled by dsa-core/phylink and driver only compares constants set there.


Sure.

> 
>> But I think your check was not correct. I looked at bpi-r2 DTS
>> (mt7623n)
>> and pio controller uses GPIO flags.
> 
> I see only same as in the example
> 
> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts#L196

I meant other consumers of pio GPIOs:
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts#L97

https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts#L320

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
  2022-05-03 12:05 ` Krzysztof Kozlowski
  2022-05-03 14:10   ` Aw: " Frank Wunderlich
@ 2022-05-04 10:21   ` Frank Wunderlich
  1 sibling, 0 replies; 9+ messages in thread
From: Frank Wunderlich @ 2022-05-04 10:21 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Frank Wunderlich, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, Sean Wang,
	Landen Chao, DENG Qingfang, netdev, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

Hi

> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>

> > +required:
> > +  - compatible
> > +  - reg
>
> What about address/size cells?

in current devicetrees the address-cells/size-cells are set above the switch on mdio-bus so i would
not add these to required for switch.

https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts#L190

regards Frank

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-04 10:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-02 15:32 [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches Frank Wunderlich
2022-05-03 12:05 ` Krzysztof Kozlowski
2022-05-03 14:10   ` Aw: " Frank Wunderlich
2022-05-03 14:40     ` Krzysztof Kozlowski
2022-05-03 15:03       ` Aw: " Frank Wunderlich
2022-05-04  6:51         ` Krzysztof Kozlowski
2022-05-04  7:44           ` Frank Wunderlich
2022-05-04  7:46             ` Krzysztof Kozlowski
2022-05-04 10:21   ` Aw: " Frank Wunderlich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).