Re: [PATCH 15/15] dt-bindings: net: dsa: mediatek,mt7530: add mediatek,mt7988-switch

[Arınç ÜNAL]

On 30.03.2023 18:23, Daniel Golle wrote:
> Add documentation for the built-in switch which can be found in the
> MediaTek MT7988 SoC.
>
> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> ---
>   .../bindings/net/dsa/mediatek,mt7530.yaml     | 26 +++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index 5ae9cd8f99a24..15953f0e9d1a6 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -11,16 +11,23 @@ maintainers:
>     - Landen Chao <Landen.Chao@xxxxxxxxxxxx>
>     - DENG Qingfang <dqfext@xxxxxxxxx>
>     - Sean Wang <sean.wang@xxxxxxxxxxxx>
> +  - Daniel Golle <daniel@xxxxxxxxxxxxxx>

Please put it in alphabetical order by the first name.

>   
>   description: |
> -  There are two versions of MT7530, standalone and in a multi-chip module.
> +  There are three versions of MT7530, standalone, in a multi-chip module and
> +  built-into a SoC.

I assume you put this to point out the situation with MT7988?

This brings to light an underlying problem with the description as the 
MT7620 SoCs described below have the MT7530 switch built into the SoC, 
instead of being part of the MCM.

The switch IP on MT7988 is for sure not MT7530 so either fix this and 
the text below as a separate patch or let me handle it.

>   
>     MT7530 is a part of the multi-chip module in MT7620AN, MT7620DA, MT7620DAN,
>     MT7620NN, MT7621AT, MT7621DAT, MT7621ST and MT7623AI SoCs.
>   
> +  The MT7988 SoC comes a built-in switch similar to MT7531 as well as 4 Gigabit

s/comes a/comes with a

> +  Ethernet PHYs and the switch registers are directly mapped into SoC's memory
> +  map rather than using MDIO. It comes with an internally connected 10G CPU port
> +  and 4 user ports connected to the built-in Gigabit Ethernet PHYs.

Are you sure this is not the MT7531 IP built into the SoC, like MT7530 
on the MT7620 SoCs? Maybe DENG Qingfang would like to clarify as they 
did for MT7530.

> +
>     MT7530 in MT7620AN, MT7620DA, MT7620DAN and MT7620NN SoCs has got 10/100 PHYs
>     and the switch registers are directly mapped into SoC's memory map rather than
> -  using MDIO. The DSA driver currently doesn't support this.
> +  using MDIO. The DSA driver currently doesn't support MT7620 variants.
>   
>     There is only the standalone version of MT7531.

Can you put the MT7988 information below here instead.

>   
> @@ -81,6 +88,10 @@ properties:
>             Multi-chip module MT7530 in MT7621AT, MT7621DAT and MT7621ST SoCs
>           const: mediatek,mt7621
>   
> +      - description:
> +          Built-in switch of the MT7988 SoC
> +        const: mediatek,mt7988-switch
> +
>     reg:
>       maxItems: 1
>   
> @@ -268,6 +279,17 @@ allOf:
>         required:
>           - mediatek,mcm
>   
> +  - if:
> +      properties:
> +        compatible:
> +          const: mediatek,mt7988-switch
> +    then:
> +      $ref: "#/$defs/mt7530-dsa-port"

The CPU ports bindings for MT7530 don't match the characteristics of the 
switch on the MT7988 SoC you described above. We need new definitions 
for the CPU ports on the switch on the MT7988 SoC.

What's the CPU port number? Does it accept rgmii or only the 10G phy-mode?

> +      properties:
> +        gpio-controller: false
> +        mediatek,mcm: false
> +        reset-names: false

I'd rather not add reset-names here and do:

  - if:
      required:
        - mediatek,mcm
    then:
      properties:
        reset-gpios: false

      required:
        - resets
        - reset-names
    else:
      properties:
        resets: false
        reset-names: false

I can handle this if you'd like.

Arınç