diff mbox series

[RFC,v1] dt-bindings: net: dsa: convert binding for mediatek switches

Message ID 20220502153238.85090-1-linux@fw-web.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [RFC,v1] dt-bindings: net: dsa: convert binding for mediatek switches | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Frank Wunderlich May 2, 2022, 3:32 p.m. UTC
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

Comments

Krzysztof Kozlowski May 3, 2022, 12:05 p.m. UTC | #1
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
Frank Wunderlich May 3, 2022, 2:10 p.m. UTC | #2
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
Krzysztof Kozlowski May 3, 2022, 2:40 p.m. UTC | #3
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
Frank Wunderlich May 3, 2022, 3:03 p.m. UTC | #4
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
Krzysztof Kozlowski May 4, 2022, 6:51 a.m. UTC | #5
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
Frank Wunderlich May 4, 2022, 7:44 a.m. UTC | #6
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
Krzysztof Kozlowski May 4, 2022, 7:46 a.m. UTC | #7
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
Frank Wunderlich May 4, 2022, 10:21 a.m. UTC | #8
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
diff mbox series

Patch

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;
-					};
-				};
-			};
-		};
-	};
-};