Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 10 Jul 2021 23:34:11 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, kuba@kernel.org, robh+dt@kernel.org,
        claudiu.manoil@nxp.com, alexandre.belloni@bootlin.com,
        UNGLinuxDriver@microchip.com, linux@armlinux.org.uk,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 net-next 8/8] Update documentation for the VSC7512
 SPI device
Message-ID: <20210710203411.nahqkyy4umqbtfwm@skbuf>
References: <20210710192602.2186370-1-colin.foster@in-advantage.com>
 <20210710192602.2186370-9-colin.foster@in-advantage.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210710192602.2186370-9-colin.foster@in-advantage.com>
Precedence: bulk

On Sat, Jul 10, 2021 at 12:26:02PM -0700, Colin Foster wrote:
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  .../devicetree/bindings/net/dsa/ocelot.txt    | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ocelot.txt b/Documentation/devicetree/bindings/net/dsa/ocelot.txt
> index 7a271d070b72..f5d05bf8b093 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ocelot.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ocelot.txt
> @@ -8,6 +8,7 @@ Currently the switches supported by the felix driver are:
>  
>  - VSC9959 (Felix)
>  - VSC9953 (Seville)
> +- VSC7511, VSC7512, VSC7513, VSC7514 via SPI
>  
>  The VSC9959 switch is found in the NXP LS1028A. It is a PCI device, part of the
>  larger ENETC root complex. As a result, the ethernet-switch node is a sub-node
> @@ -211,3 +212,70 @@ Example:
>  		};
>  	};
>  };
> +
> +The VSC7513 and VSC7514 switches can be controlled internally via the MIPS
> +processor. The VSC7511 and VSC7512 don't have this internal processor, but all
> +four chips can be controlled externally through SPI with the following required
> +properties:
> +
> +- compatible:
> +	Can be "mscc,vsc7511", "mscc,vsc7512", "mscc,vsc7513", or
> +	"mscc,vsc7514".
> +
> +Supported phy modes for all chips are:
> +
> +* phy_mode = "internal": on ports 0, 1, 2, 3
> +
> +Additionally, the VSC7512 and VSC7514 support SGMII and QSGMII on various ports,
> +though that is currently untested.
> +
> +Example for control from a BeagleBone Black
> +
> +&spi0 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	vsc7512: vsc7512@0 {

ethernet-switch@0

> +		compatible = "mscc,vsc7512";
> +		spi-max-frequency = <250000>;
> +		reg = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				ethernet = <&mac>;
> +				phy-mode = "internal";
> +
> +				fixed-link {
> +					speed = <100>;
> +					full-duplex;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				label = "swp1";
> +				status = "okay";

I am not convinced that the status = "okay" lines are useful in the
example.

> +				phy-mode = "internal";

This syntax is ambiguous and does not obviously mean that the port has
an internal copper PHY. Please see this discussion for other meanings of
no 'phy-handle' and no 'fixed-link'.

https://www.mail-archive.com/u-boot@lists.denx.de/msg409571.html

I think it would be in the best interest of everyone to go through
phylink_of_phy_connect() instead of phylink_connect_phy(), aka use the
standard phy-handle property and create an mdio node under
ethernet-switch@0 where the internal PHY OF nodes are defined.

I don't know if this is true for VSC7512 or not, but for example on
NXP SJA1110, the internal PHYs can be accessed in 2 modes:
(a) through SPI transfers
(b) through an MDIO slave access point exposed by the switch chip, which
    can be connected to an external MDIO controller

Some boards will use method (a), and others will use method (b).

Requiring a phy-handle under the port property is an absolutely generic
way to seamlessly deal with both cases. In case (a), the phy-handle
points to a child of an MDIO bus provided by the ocelot driver, in case
(b) the phy-handle points to a child provided by some other MDIO
controller driver.

> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				label = "swp2";
> +				status = "okay";
> +				phy-mode = "internal";
> +			};
> +
> +			port@3 {
> +				reg = <3>;
> +				label = "swp3";
> +				status = "okay";
> +				phy-mode = "internal";
> +			};
> +		};
> +	};
> +};
> -- 
> 2.25.1
>