Re: [RFC PATCH v2 net-next 8/8] Update documentation for the VSC7512 SPI device

[Colin Foster <colin.foster@in-advantage.com>]

On Sat, Jul 10, 2021 at 11:34:11PM +0300, Vladimir Oltean wrote:
> 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.

Fair enough

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

Yes, the Ocelot chips have the same functionality with the indirect /
direct access. It seems like this would be coupled with the other
discussion of updating mdio-mscc-miim.c so that the MDIO bus can be
defined.

And thank you for pointing out some examples. Having some starting
points really helps!

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