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

Colin Foster
2021-11-16T15:32:29Z

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