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

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

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.

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

> 		 reg = <0>;
> 
> 		 ports {

there's an extra preceding space here, for all 4 lines from "compatible" to "ports".

> 			#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@2 {
> 				reg = <2>;
> 				label = "swp2";
> 				status = "okay";
> 				phy-handle = <&sw_phy2>;
> 				phy-mode = "internal";
> 			};
> 
> 			port@3 {
> 				reg = <3>;
> 				label = "swp3";
> 				status = "okay";
> 				phy-handle = <&sw_phy3>;
> 				phy-mode = "internal";
> 			};
> 
> 			port@4 {
> 				reg = <4>;
> 				label = "swp4";
> 				status = "okay";
> 				phy-handle = <&sw_phy4>;
> 				phy-mode = "sgmii";
> 			};
> 
> 			port@5 {
> 				reg = <5>;
> 				label = "swp5";
> 				status = "okay";
> 				phy-handle = <&sw_phy5>;
> 				phy-mode = "sgmii";
> 			};
> 
> 			port@6 {
> 				reg = <6>;
> 				label = "swp6";
> 				status = "okay";
> 				phy-handle = <&sw_phy6>;
> 				phy-mode = "sgmii";
> 			};
> 
> 			port@7 {
> 				reg = <7>;
> 				label = "swp7";
> 				status = "okay";
> 				phy-handle = <&sw_phy7>;
> 				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.

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

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