Re: [RFC PATCH v4 net-next 00/23] add support for VSC75XX control over SPI

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Tue, Nov 16, 2021 at 11:10:59AM +0000, Vladimir Oltean wrote:
> On Mon, Nov 15, 2021 at 10:23:05PM -0800, Colin Foster wrote:
> > My apologies for this next RFC taking so long. Life got in the way.
> > 
> > 
> > The patch set in general is to add support for the VSC7511, VSC7512,
> > VSC7513 and VSC7514 devices controlled over SPI. The driver is
> > relatively functional for the internal phy ports (0-3) on the VSC7512.
> > As I'll discuss, it is not yet functional for other ports yet.
> > 
> > I still think there are enough updates to bounce by the community
> > in case I'm terribly off base or doomed to chase my tail.
> > 
> 
> I would try to get rid of some of the patches now, instead of gathering
> a larger and larger pile. Here is a list of patches that I think could
> be submitted right away (possibly as part of independent series;
> parallelize as much as you can):
> 
> [01/23] net: dsa: ocelot: remove unnecessary pci_bar variables
> [02/23] net: mdio: mscc-miim: convert to a regmap implementation
> [03/23] net: dsa: ocelot: seville: utilize of_mdiobus_register
> [04/23] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio …
> [05/23] net: dsa: ocelot: felix: Remove requirement for PCS in felix devices
> [06/23] net: dsa: ocelot: felix: add interface for custom regmaps
> [07/23] net: dsa: ocelot: felix: add per-device-per-port quirks
> [08/23] net: mscc: ocelot: split register definitions to a separate file
> [09/23] net: mscc: ocelot: expose ocelot wm functions
> [18/23] net: phy: lynx: refactor Lynx PCS module to use generic phylink_pcs
> [19/23] net: dsa: felix: name change for clarity from pcs to mdio_device
> [20/23] net: dsa: seville: name change for clarity from pcs to mdio_device
> [21/23] net: ethernet: enetc: name change for clarity from pcs to mdio_device
> [22/23] net: pcs: lynx: use a common naming scheme for all lynx_pcs variables
> 
> Now that you've submitted them all already, let's wait for some feedback
> before resending smaller chunks.

Thank you for your response as always!

This will make v5 a lot easier for me to keep straight. I think this one
might also be a good one to submit earlier, since it actually does
change behavior:

pinctrl: ocelot: update pinctrl to automatic base address

I'd probably want to drag this trivial one along as well:

pinctrl: ocelot: combine get resource and ioremap into single call

> 
> > 
> > The main changes for V4 are trying to get pinctrl-ocelot and
> > pinctrl-microchip-sgpio functional. Without pinctrl-ocelot,
> > communication to external phys won't work. Without communication to
> > external phys, PCS ports 4-7 on the dev board won't work. Nor will any
> > fiber ports. 
> > 
> > 
> > The hardware setup I'm using for development is a beaglebone black, with
> > jumpers from SPI0 to the microchip VSC7512 dev board. The microchip dev 
> > board has been modified to not boot from flash, but wait for SPI. An
> > ethernet cable is connected from the beaglebone ethernet to port 0 of
> > the dev board.
> > 
> > 
> > The device tree I'm using for the VSC7512 is below. Note that ports 4-7
> > are still not expected to work, but left in as placeholders for when
> > they do.
> > 
> > 
> > &spi0 {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	status = "okay";
> > 
> > 	ethernet-switch@0{
> > 		 compatible = "mscc,vsc7512";
> > 		 spi-max-frequency = <250000>;
> 
> Can't go faster than 250 KHz? That's sad.

Haven't tried faster speeds. During the early days there was an error on
my setup due to too long of wires. Crosstalk / capacitance... I slowed
the SPI bus to a crawl while debugging that and never turned it back up. 

If it'll help anyone: transitions on the MOSI I believe were causing
glitches on the CS line. This made the VSC7512 sad.

> 
> > 		 reg = <0>;
> > 
> > 		 ports {
> 
> there's an extra preceding space here, for all 4 lines from "compatible" to "ports".

I'll fix it. Thanks.

> 
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			port@0 {
> > 				reg = <0>;
> > 				label = "cpu";
> > 				status = "okay";
> > 				ethernet = <&mac_sw>;
> > 				phy-handle = <&sw_phy0>;
> > 				phy-mode = "internal";
> > 			};
> > 
> > 			port@1 {
> > 				reg = <1>;
> > 				label = "swp1";
> > 				status = "okay";
> > 				phy-handle = <&sw_phy1>;
> > 				phy-mode = "internal";
> > 			};
> > 
> > 			port@4 {
> > 				reg = <4>;
> > 				label = "swp4";
> > 				status = "okay";
> > 				phy-handle = <&sw_phy4>;
> > 				phy-mode = "sgmii";
> > 			};
> > 		};
> > 
> > 		mdio {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			sw_phy0: ethernet-phy@0 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> 
> PHY nodes don't need #address-cells and #size-cells.

Thank you. I'll remove these and the ones below.

> 
> > 				reg = <0x0>;
> > 			};
> > 
> > 			sw_phy1: ethernet-phy@1 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x1>;
> > 			};
> > 
> > 			sw_phy2: ethernet-phy@2 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x2>;
> > 			};
> > 
> > 			sw_phy3: ethernet-phy@3 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x3>;
> > 			};
> > 
> > 			sw_phy4: ethernet-phy@4 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x4>;
> > 			};
> > 
> > 			sw_phy5: ethernet-phy@5 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x5>;
> > 			};
> > 
> > 			sw_phy6: ethernet-phy@6 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x6>;
> > 			};
> > 
> > 			sw_phy7: ethernet-phy@7 {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 				reg = <0x7>;
> > 			};
> > 		};
> > 
> > 		gpio: pinctrl {
> > 			compatible = "mscc,ocelot-pinctrl";
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> 
> I don't think that #address-cells and #size-cells are needed here, since
> "led-shift-reg-pins" does not have any address unit.
> 
> > 			#gpio_cells = <2>;
> > 			gpio-ranges = <&gpio 0 0 22>;
> > 
> > 			led_shift_reg_pins: led-shift-reg-pins {
> > 				pins = "GPIO_0", "GPIO_1", "GPIO_2", "GPIO_3";
> > 				function = "sg0";
> > 			};
> > 		};
> > 
> > 		sgpio: sgpio {
> > 			compatible = "mscc,ocelot-sgpio";
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			bus-frequency=<12500000>;
> > 			clocks = <&ocelot_clock>;
> > 			microchip,sgpio-port-ranges = <0 31>;
> > 
> > 			sgpio_in0: sgpio@0 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <0>;
> > 				gpio-controller;
> > 				#gpio-cells = <3>;
> > 				ngpios = <32>;
> > 			};
> > 
> > 			sgpio_out1: sgpio@1 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <1>;
> > 				gpio-controller;
> > 				gpio-cells = <3>;
> > 				ngpios = <32>;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > 
> > My main focus is getting the ocelot-pinctrl driver fully functional. My
> > current hope is that it would correctly set GPIO pins 0-3 into the "sg0"
> > state. That is not the case right now, and I'll be looking into why. The
> > behavior I'm hoping for is to be able to configure the sgpio LEDs for
> > activity at the very least. Link status would be a bonus.
> > 
> > 
> > I do have pinctrl by way of debugfs and sysfs. There aren't any debug
> > LEDs that are attached to unused pins, unfortunately. That would've been
> > really helpful. So there's a key takeaway for dev-board manufacturers.
> > 
> > 
> > As you'll see, the main changes to the three drivers I'm utilizing
> > (mscc_miim, pinctrl-ocelot, and pinctrl-microchip-sgpio) follow a
> > similar path. First, convert everything to regmap. Second, expose
> > whatever functions are necessary to fully utilize an external regmap.
> > 
> > 
> > One thing to note: I've been following a pattern of adding "offset"
> > variables to these drivers. I'm looking for feedback here, because I
> > don't like it - however I feel like it is the "least bad" interface I
> > could come up with. 
> > 
> > 
> > Specifically, ocelot has a regmap for GCB. ocelot-pinctrl would create a
> > smaller regmap at an address of "GCB + 0x34".
> > 
> > 
> > There are three options I saw here:
> > 1. Have vsc7512_spi create a new regmap at GCB + 0x34 and pass that to
> > ocelot-pinctrl
> > 2. Give ocelot-pinctrl the concept of a "parent bus" by which it could
> > request a regmap. 
> > 3. Keep the same GCB regmap, but pass in 0x34 as an offset.
> > 
> > 
> > I will admit that option 2 sounds very enticing, but I don't know if
> > that type of interaction exists. If not, implementing it is probably
> > outside the scope of a first patch set. As such, I opted for option 3.
> 
> I think that type of interaction is called "mfd", potentially even "syscon".

Before diving in, I'd come across mfd and thought that might be the
answer. I'll reconsider it now that I have several months of staring at
kernel code under my belt. Maybe an mfd that does SPI setup and chip
resetting. Then I could remove all SPI code from ocelot_vsc7512_spi.

> 
> > 
> > 
> > Version 4 also fixes some logic for MDIO probing. It wasn't using the
> > device tree by way of of_mdiobus_register. Now it is.
> > 
> > 
> > The relevant boot log for the switch / MDIO bus is here. As expected,
> > devices 4-7 are missing. If nothing else, that is telling me that the
> > device tree is working.
> > 
> > [    4.005195] mdio_bus spi0.0-mii:03: using lookup tables for GPIO lookup
> > [    4.005205] mdio_bus spi0.0-mii:03: No GPIO consumer reset found
> > [    4.006586] mdio_bus spi0.0-mii: MDIO device at address 4 is missing.
> > [    4.014333] mdio_bus spi0.0-mii: MDIO device at address 5 is missing.
> > [    4.022009] mdio_bus spi0.0-mii: MDIO device at address 6 is missing.
> > [    4.029573] mdio_bus spi0.0-mii: MDIO device at address 7 is missing.
> > [    8.386624] vsc7512 spi0.0: PHY [spi0.0-mii:00] driver [Generic PHY] (irq=POLL)
> > [    8.397222] vsc7512 spi0.0: configuring for phy/internal link mode
> > [    8.419484] vsc7512 spi0.0 swp1 (uninitialized): PHY [spi0.0-mii:01] driver [Generic PHY] (irq=POLL)
> > [    8.437278] vsc7512 spi0.0 swp2 (uninitialized): PHY [spi0.0-mii:02] driver [Generic PHY] (irq=POLL)
> > [    8.452867] vsc7512 spi0.0 swp3 (uninitialized): PHY [spi0.0-mii:03] driver [Generic PHY] (irq=POLL)
> > [    8.465007] vsc7512 spi0.0 swp4 (uninitialized): no phy at 4
> > [    8.470721] vsc7512 spi0.0 swp4 (uninitialized): failed to connect to PHY: -ENODEV
> > [    8.478388] vsc7512 spi0.0 swp4 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 4
> > [    8.489636] vsc7512 spi0.0 swp5 (uninitialized): no phy at 5
> > [    8.495371] vsc7512 spi0.0 swp5 (uninitialized): failed to connect to PHY: -ENODEV
> > [    8.502996] vsc7512 spi0.0 swp5 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 5
> > [    8.514186] vsc7512 spi0.0 swp6 (uninitialized): no phy at 6
> > [    8.519882] vsc7512 spi0.0 swp6 (uninitialized): failed to connect to PHY: -ENODEV
> > [    8.527539] vsc7512 spi0.0 swp6 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 6
> > [    8.538716] vsc7512 spi0.0 swp7 (uninitialized): no phy at 7
> > [    8.544451] vsc7512 spi0.0 swp7 (uninitialized): failed to connect to PHY: -ENODEV
> > [    8.552079] vsc7512 spi0.0 swp7 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 7
> > [    8.571962] device eth0 entered promiscuous mode
> > [    8.576684] DSA: tree 0 setup
> > [   10.490093] vsc7512 spi0.0: Link is Up - 100Mbps/Full - flow control off
> > 
> > 
> > Much later on, I created a bridge with STP (and two ports jumped
> > together) as a test. Everything seems to be working as expected. 
> > 
> > 
> > [59839.920340] cpsw-switch 4a100000.switch: starting ndev. mode: dual_mac
> > [59840.013636] SMSC LAN8710/LAN8720 4a101000.mdio:00: attached PHY driver (mii_bus:phy_addr=4a101000.mdio:00, irq=POLL)
> > [59840.031444] 8021q: adding VLAN 0 to HW filter on device eth0
> > [59840.057406] vsc7512 spi0.0 swp1: configuring for phy/internal link mode
> > [59840.089302] vsc7512 spi0.0 swp2: configuring for phy/internal link mode
> > [59840.121514] vsc7512 spi0.0 swp3: configuring for phy/internal link mode
> > [59840.167589] br0: port 1(swp1) entered blocking state
> > [59840.172818] br0: port 1(swp1) entered disabled state
> > [59840.191078] device swp1 entered promiscuous mode
> > [59840.224855] br0: port 2(swp2) entered blocking state
> > [59840.229893] br0: port 2(swp2) entered disabled state
> > [59840.245844] device swp2 entered promiscuous mode
> > [59840.270839] br0: port 3(swp3) entered blocking state
> > [59840.276003] br0: port 3(swp3) entered disabled state
> > [59840.291674] device swp3 entered promiscuous mode
> > [59840.663239] vsc7512 spi0.0: Link is Down
> > [59841.691641] vsc7512 spi0.0: Link is Up - 100Mbps/Full - flow control off
> > [59842.167897] cpsw-switch 4a100000.switch eth0: Link is Up - 100Mbps/Full - flow control off
> > [59842.176481] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> > [59843.216121] vsc7512 spi0.0 swp1: Link is Up - 1Gbps/Full - flow control rx/tx
> > [59843.231076] IPv6: ADDRCONF(NETDEV_CHANGE): swp1: link becomes ready
> > [59843.237593] br0: port 1(swp1) entered blocking state
> > [59843.242629] br0: port 1(swp1) entered listening state
> > [59843.301447] vsc7512 spi0.0 swp3: Link is Up - 1Gbps/Full - flow control rx/tx
> > [59843.309027] IPv6: ADDRCONF(NETDEV_CHANGE): swp3: link becomes ready
> > [59843.315544] br0: port 3(swp3) entered blocking state
> > [59843.320545] br0: port 3(swp3) entered listening state
> > [59845.042058] br0: port 3(swp3) entered blocking state
> > [59858.401566] br0: port 1(swp1) entered learning state
> > [59871.841910] br0: received packet on swp1 with own address as source address (addr:24:76:25:76:35:37, vlan:0)
> > [59873.761495] br0: port 1(swp1) entered forwarding state
> > [59873.766703] br0: topology change detected, propagating
> > [59873.776278] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
> > [59902.561908] br0: received packet on swp1 with own address as source address (addr:24:76:25:76:35:37, vlan:0)
> > [59926.494446] vsc7512 spi0.0 swp2: Link is Up - 1Gbps/Full - flow control rx/tx
> > [59926.501959] IPv6: ADDRCONF(NETDEV_CHANGE): swp2: link becomes ready
> > [59926.508702] br0: port 2(swp2) entered blocking state
> > [59926.513868] br0: port 2(swp2) entered listening state
> > [59941.601540] br0: port 2(swp2) entered learning state
> > [59956.961493] br0: port 2(swp2) entered forwarding state
> > [59956.966711] br0: topology change detected, propagating
> > [59968.481839] br0: received packet on swp1 with own address as source address (addr:24:76:25:76:35:37, vlan:0)
> > 
> > 
> > In order to make this work, I have modified the cpsw driver, and now the
> > cpsw_new driver, to allow for frames over 1500 bytes. Otherwise the
> > tagging protocol will not work between the beaglebone and the VSC7512. I
> > plan to eventually try to get those changes in mainline, but I don't
> > want to get distracted from my initial goal.
> > 
> > 
> > RFC history:
> > v1 (accidentally named vN)
> > 	* Initial architecture. Not functional
> > 	* General concepts laid out
> > 
> > v2
> > 	* Near functional. No CPU port communication, but control over all
> > 	external ports
> > 	* Cleaned up regmap implementation from v1
> > 
> > v3
> > 	* Functional
> > 	* Shared MDIO transactions routed through mdio-mscc-miim
> > 	* CPU / NPI port enabled by way of vsc7512_enable_npi_port /
> > 	felix->info->enable_npi_port
> > 	* NPI port tagging functional - Requires a CPU port driver that supports
> > 	frames of 1520 bytes. Verified with a patch to the cpsw driver
> > 
> > v4
> >     * Functional
> >     * Device tree fixes
> >     * Add hooks for pinctrl-ocelot - some functionality by way of sysfs
> >     * Add hooks for pinctrl-microsemi-sgpio - not yet fully functional
> >     * Remove lynx_pcs interface for a generic phylink_pcs. The goal here
> >     is to have an ocelot_pcs that will work for each configuration of
> >     every port. 
> > 
> > 
> > 
> > Colin Foster (23):
> >   net: dsa: ocelot: remove unnecessary pci_bar variables
> >   net: mdio: mscc-miim: convert to a regmap implementation
> >   net: dsa: ocelot: seville: utilize of_mdiobus_register
> >   net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect
> >     mdio access
> >   net: dsa: ocelot: felix: Remove requirement for PCS in felix devices
> >   net: dsa: ocelot: felix: add interface for custom regmaps
> >   net: dsa: ocelot: felix: add per-device-per-port quirks
> >   net: mscc: ocelot: split register definitions to a separate file
> >   net: mscc: ocelot: expose ocelot wm functions
> >   pinctrl: ocelot: combine get resource and ioremap into single call
> >   pinctrl: ocelot: update pinctrl to automatic base address
> >   pinctrl: ocelot: convert pinctrl to regmap
> >   pinctrl: ocelot: expose ocelot_pinctrl_core_probe interface
> >   pinctrl: microchip-sgpio: update to support regmap
> >   device property: add helper function fwnode_get_child_node_count
> >   pinctrl: microchip-sgpio: change device tree matches to use nodes
> >     instead of device
> >   pinctrl: microchip-sgpio: expose microchip_sgpio_core_probe interface
> >   net: phy: lynx: refactor Lynx PCS module to use generic phylink_pcs
> >   net: dsa: felix: name change for clarity from pcs to mdio_device
> >   net: dsa: seville: name change for clarity from pcs to mdio_device
> >   net: ethernet: enetc: name change for clarity from pcs to mdio_device
> >   net: pcs: lynx: use a common naming scheme for all lynx_pcs variables
> >   net: dsa: ocelot: felix: add support for VSC75XX control over SPI
> > 
> >  drivers/base/property.c                       |  20 +-
> >  drivers/net/dsa/ocelot/Kconfig                |  16 +
> >  drivers/net/dsa/ocelot/Makefile               |   7 +
> >  drivers/net/dsa/ocelot/felix.c                |  29 +-
> >  drivers/net/dsa/ocelot/felix.h                |  10 +-
> >  drivers/net/dsa/ocelot/felix_mdio.c           |  54 +
> >  drivers/net/dsa/ocelot/felix_mdio.h           |  13 +
> >  drivers/net/dsa/ocelot/felix_vsc9959.c        |  38 +-
> >  drivers/net/dsa/ocelot/ocelot_vsc7512_spi.c   | 946 ++++++++++++++++++
> >  drivers/net/dsa/ocelot/seville_vsc9953.c      | 136 +--
> >  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  12 +-
> >  .../net/ethernet/freescale/dpaa2/dpaa2-mac.h  |   3 +-
> >  .../net/ethernet/freescale/enetc/enetc_pf.c   |  27 +-
> >  .../net/ethernet/freescale/enetc/enetc_pf.h   |   4 +-
> >  drivers/net/ethernet/mscc/Makefile            |   3 +-
> >  drivers/net/ethernet/mscc/ocelot.c            |   8 +
> >  drivers/net/ethernet/mscc/ocelot_devlink.c    |  31 +
> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 548 +---------
> >  drivers/net/ethernet/mscc/vsc7514_regs.c      | 522 ++++++++++
> >  drivers/net/mdio/mdio-mscc-miim.c             | 167 +++-
> >  drivers/net/pcs/pcs-lynx.c                    |  36 +-
> >  drivers/pinctrl/pinctrl-microchip-sgpio.c     | 127 ++-
> >  drivers/pinctrl/pinctrl-ocelot.c              | 207 ++--
> >  include/linux/mdio/mdio-mscc-miim.h           |  19 +
> >  include/linux/pcs-lynx.h                      |   9 +-
> >  include/linux/property.h                      |   1 +
> >  include/soc/mscc/ocelot.h                     |  60 ++
> >  include/soc/mscc/vsc7514_regs.h               |  27 +
> >  28 files changed, 2219 insertions(+), 861 deletions(-)
> >  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
> >  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h
> >  create mode 100644 drivers/net/dsa/ocelot/ocelot_vsc7512_spi.c
> >  create mode 100644 drivers/net/ethernet/mscc/vsc7514_regs.c
> >  create mode 100644 include/linux/mdio/mdio-mscc-miim.h
> >  create mode 100644 include/soc/mscc/vsc7514_regs.h
> > 
> > -- 
> > 2.25.1
> >