netdev - Re: [net-next][PATCH v4] dt-bindings: dsa: Add lan9303 yaml

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 9 Oct 2022 01:56:28 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jerry Ray <jerry.ray@...rochip.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next][PATCH v4] dt-bindings: dsa: Add lan9303 yaml

On Mon, Oct 03, 2022 at 11:46:24AM -0500, Jerry Ray wrote:
> ---
> v3->v4:
>  - Addressed v3 community feedback

More specifically?

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    // Ethernet switch connected via mdio to the host
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        phy-handle = <&lan9303switch>;
> +        phy-mode = "rmii";
> +        fixed-link {
> +            speed = <100>;
> +            full-duplex;
> +        };

I see the phy-handle to the switch is inherited from the .txt dt-binding,
but I don't understand it. The switch is an mdio_device, not a phy_device,
so what will this do?

Also, any reasonable host driver will error out if it finds a phy-handle
and a fixed-link in its OF node. So one of phy-handle or fixed-link must
be dropped, they are bogus.

Even better, just stick to the mdio node as root and drop the DSA master
OF node, like other DSA dt-binding examples do. You can have dangling
phandles, so "ethernet = <&ethernet>" below is not an issue.

> +        mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            lan9303switch: switch@0 {
> +                compatible = "smsc,lan9303-mdio";
> +                reg = <0>;
> +                dsa,member = <0 0>;

Redundant, please remove.

> +                ethernet-ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                        port@0 {
> +                            reg = <0>;
> +                            phy-mode = "rmii";

FWIW, RMII has a MAC mode and a PHY mode. Two RMII interfaces connected
in MAC mode to one another don't work. You'll have problems if you also
have an RMII PHY connected to one of the xMII ports, and you describe
phy-mode = "rmii" for both. There exists a "rev-rmii" phy-mode to denote
an RMII interface working in PHY mode. Wonder if you should be using
that here.

> +                            ethernet = <&ethernet>;
> +                            fixed-link {
> +                                speed = <100>;
> +                                full-duplex;
> +                            };
> +                        };
> +                        port@1 {
> +                            reg = <1>;
> +                            max-speed = <100>;
> +                            label = "lan1";
> +                        };
> +                        port@2 {
> +                            reg = <2>;
> +                            max-speed = <100>;
> +                            label = "lan2";
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    // Ethernet switch connected via i2c to the host
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        phy-mode = "rmii";
> +            speed = <100>;
> +        fixed-link {
> +            full-duplex;
> +        };
> +    };

No need for this node.

> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        lan9303: switch@1a {
> +            compatible = "smsc,lan9303-i2c";
> +            reg = <0x1a>;
> +            ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                port@0 {
> +                    reg = <0>;
> +                    phy-mode = "rmii";
> +                    ethernet = <&ethernet>;
> +                    fixed-link {
> +                        speed = <100>;
> +                        full-duplex;
> +                    };
> +                };
> +                port@1 {
> +                    reg = <1>;
> +                    max-speed = <100>;
> +                    label = "lan1";
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    max-speed = <100>;
> +                    label = "lan2";
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d58b55c5ae5..89055ff2838a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13386,6 +13386,14 @@ L:	netdev@...r.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/microchip/lan743x_*
>  
> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
> +M:	Jerry Ray <jerry.ray@...rochip.com>
> +M:	UNGLinuxDriver@...rochip.com
> +L:	netdev@...r.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
> +F:	drivers/net/dsa/lan9303*
> +

Separate patch please? Changes to the MAINTAINERS file get applied to
the "net" tree.

>  MICROCHIP LAN966X ETHERNET DRIVER
>  M:	Horatiu Vultur <horatiu.vultur@...rochip.com>
>  M:	UNGLinuxDriver@...rochip.com
> -- 
> 2.25.1
>