Re: [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control

[Colin Foster]

Hi Vladimir and Andrew,

On Tue, May 04, 2021 at 12:59:43PM +0000, Vladimir Oltean wrote:
> On Tue, May 04, 2021 at 02:31:34PM +0200, Andrew Lunn wrote:
> > On Mon, May 03, 2021 at 10:11:27PM -0700, Colin Foster wrote:
> > > Add support for control for VSC75XX chips over SPI control. Starting with the
> > > VSC9959 code, this will utilize a spi bus instead of PCIe or memory-mapped IO to
> > > control the chip.
> > 
> > Hi Colin
> > 
> > Please fix your subject line for the next version. vN should of been
> > v1. The number is important so we can tell revisions apart.
> 
> Yes, it was my indication to use --subject-prefix="[PATCH vN net-next]",
> I was expecting Colin to replace N with 1, 2, 3 etc but I didn't make
> that clear enough :)
> 

Ha. Yes, I suppose I took that too literally. I'll fix it in vO :)

> > > 
> > > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/rpi-vsc7512-spi-overlay.dts |  124 ++
> > >  drivers/net/dsa/ocelot/Kconfig                |   11 +
> > >  drivers/net/dsa/ocelot/Makefile               |    5 +
> > >  drivers/net/dsa/ocelot/felix_vsc7512_spi.c    | 1214 +++++++++++++++++
> > >  include/soc/mscc/ocelot.h                     |   15 +
> > 
> > Please split this patch up. The DT overlay will probably be merged via
> > ARM SOC, not netdev. You also need to document the device tree
> > binding, as a separate patch.

I will take this out of the patch, though the feedback is helpful. I
suspect that the end result will be an example in
Documentation/devicetree/bindings/net/dsa/ocelot.txt because there isn't
any commercial hardware available with this functionality, as far as I
know. (If there is I'd love to get my hands on it!)

> > 
> > > +	fragment@3 {
> > > +		target = <&spi0>;
> > > +		__overlay__ {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +			cs-gpios = <&gpio 8 1>;
> > > +			status = "okay";
> > > +
> > > +			vsc7512: vsc7512@0{
> > > +				compatible = "mscc,vsc7512";
> > > +				spi-max-frequency = <250000>;
> > > +				reg = <0>;
> > > +
> > > +				ports {
> > > +					#address-cells = <1>;
> > > +					#size-cells = <0>;
> > > +
> > > +					port@0 {
> > > +						reg = <0>;
> > > +						ethernet = <&ethernet>;
> > > +						phy-mode = "internal";
> 
> Additionally, being a completely off-chip switch, are you sure that the
> phy-mode is "internal"?

No, I'm not sure. I don't remember my justification but I had come
across something that made me believe that there needed to be at least
one "internal phy-mode" for DSA to work. This might actually make sense,
however, since it would be the port internal to the on-chip processor.

My hope was that I would've been able to test this with actual hardware
a couple weeks ago and see everything in action. Unfortunately there 
seems to be a hardware issue on my setup I'll need EE support to
troubleshoot.

When the hardware is finally communicating, I plan to do this type of
functional verification. I've been in charge of writing the interface
layer of this chip family in the past, but I have coworkers who are
familiar with the actual operation who's advice I'll seek.

> 
> > > +						fixed-link {
> > > +							speed = <1000>;
> > > +							full-duplex;
> > > +						};
> > > +					};
> > > +
> > > +					port@1 {
> > > +						reg = <1>;
> > > +						label = "swp1";
> > > +						status = "disabled";
> > > +					};
> > > +
> > > +					port@2 {
> > > +						reg = <2>;
> > > +						label = "swp2";
> > > +						status = "disabled";
> > > +					};
> > > +static void vsc7512_phylink_validate(struct ocelot *ocelot, int port,
> > > +				     unsigned long *supported,
> > > +				     struct phylink_link_state *state)
> > > +{
> > > +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = {
> > > +		0,
> > > +	};
> > 
> > This function seems out of place. Why would SPI access change what the
> > ports are capable of doing? Please split this up into more
> > patches. Keep the focus of this patch as being adding SPI support.
> 
> What is going on is that this is just the way in which the drivers are
> structured. Colin is not really "adding SPI support" to any of the
> existing DSA switches that are supported (VSC9953, VSC9959) as much as
> "adding support for a new switch which happens to be controlled over
> SPI" (VSC7512).
> The layering is as follows:
> - drivers/net/dsa/ocelot/felix_vsc7512_spi.c: deals with the most
>   hardware specific SoC support. The regmap is defined here, so are the
>   port capabilities.
> - drivers/net/dsa/ocelot/felix.c: common integration with DSA
> - drivers/net/ethernet/mscc/ocelot*.c: the SoC-independent hardware
>   support.
> 
> I'm not actually sure that splitting the port PHY mode support in a
> separate patch is possible while keeping functional intermediate
> results. But I do agree about the rest, splitting the device tree
> changes, etc.