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

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:

quoted

On Mon, May 03, 2021 at 10:11:27PM -0700, Colin Foster wrote:

quoted

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 :)

quoted

quoted

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 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!)

quoted

quoted

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

quoted

quoted

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