2022-05-03 01:22:09

by Frank Wunderlich

[permalink] [raw]
Subject: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

From: Frank Wunderlich <[email protected]>

Convert txt binding to yaml binding for Mediatek switches.

Signed-off-by: Frank Wunderlich <[email protected]>
---
.../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 <[email protected]>
+ - Landen Chao <[email protected]>
+ - DENG Qingfang <[email protected]>
+
+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


2022-05-03 15:53:57

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

On 02/05/2022 17:32, Frank Wunderlich wrote:
> From: Frank Wunderlich <[email protected]>
>
> Convert txt binding to yaml binding for Mediatek switches.
>
> Signed-off-by: Frank Wunderlich <[email protected]>
> ---
> .../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 <[email protected]>
> + - Landen Chao <[email protected]>
> + - DENG Qingfang <[email protected]>
> +
> +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

2022-05-03 16:23:47

by Frank Wunderlich

[permalink] [raw]
Subject: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

Hi,

thank you for first review.

> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> Von: "Krzysztof Kozlowski" <[email protected]>
> 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 <[email protected]>
> >
> > Convert txt binding to yaml binding for Mediatek switches.
> >
> > Signed-off-by: Frank Wunderlich <[email protected]>
> > ---
> > .../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 <[email protected]>
> > + - Landen Chao <[email protected]>
> > + - DENG Qingfang <[email protected]>
> > +
> > +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

2022-05-03 17:49:08

by Krzysztof Kozlowski

[permalink] [raw]
Subject: 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" <[email protected]>
>> 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 <[email protected]>
>>>
>>> Convert txt binding to yaml binding for Mediatek switches.
>>>
>>> Signed-off-by: Frank Wunderlich <[email protected]>
>>> ---
>>> .../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

2022-05-03 19:25:50

by Frank Wunderlich

[permalink] [raw]
Subject: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

Hi,

> Gesendet: Dienstag, 03. Mai 2022 um 16:40 Uhr
> Von: "Krzysztof Kozlowski" <[email protected]>
> 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" <[email protected]>
> >> 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 <[email protected]>
> >>>
> >>> Convert txt binding to yaml binding for Mediatek switches.
> >>>
> >>> Signed-off-by: Frank Wunderlich <[email protected]>
> >>> ---
> >>> .../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

2022-05-04 17:42:36

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

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

2022-05-06 10:11:36

by Frank Wunderlich

[permalink] [raw]
Subject: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

Hi

> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> Von: "Krzysztof Kozlowski" <[email protected]>

> > +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