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

Vladimir Oltean
2021-11-16T11:11:17Z

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
>