mbox series

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

Message ID 20211116062328.1949151-1-colin.foster@in-advantage.com (mailing list archive)
Headers show
Series add support for VSC75XX control over SPI | expand

Message

Colin Foster Nov. 16, 2021, 6:23 a.m. UTC
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.


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>;
		 reg = <0>;

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


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

Comments

Andy Shevchenko Nov. 16, 2021, 10:34 a.m. UTC | #1
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.


Since series touches fwnode, please Cc next time to Daniel Scally.
It also appears [1] that somewhere in PHY code a bug is hidden
(at least I think so).

[1]: https://lore.kernel.org/lkml/20211113204141.520924-1-djrscally@gmail.com/T/#u
Vladimir Oltean Nov. 16, 2021, 11:10 a.m. UTC | #2
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
>
Colin Foster Nov. 16, 2021, 3:04 p.m. UTC | #3
On Tue, Nov 16, 2021 at 12:34:17PM +0200, Andy Shevchenko 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.
> 
> 
> Since series touches fwnode, please Cc next time to Daniel Scally.

Thank you. I will do this next time.

For my future reference, is there a way that I could have known this? Or
is this just knowledge that comes with experience? The email list I got
was from running the patch set through get_maintainers.

> It also appears [1] that somewhere in PHY code a bug is hidden
> (at least I think so).

Thank you for this information. I'll keep an eye out!

> 
> [1]: https://lore.kernel.org/lkml/20211113204141.520924-1-djrscally@gmail.com/T/#u
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Colin Foster Nov. 16, 2021, 3:32 p.m. UTC | #4
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
> >
Andy Shevchenko Nov. 16, 2021, 5:05 p.m. UTC | #5
On Tue, Nov 16, 2021 at 07:04:04AM -0800, Colin Foster wrote:
> On Tue, Nov 16, 2021 at 12:34:17PM +0200, Andy Shevchenko 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.
> > 
> > Since series touches fwnode, please Cc next time to Daniel Scally.
> 
> Thank you. I will do this next time.
> 
> For my future reference, is there a way that I could have known this? Or
> is this just knowledge that comes with experience? The email list I got
> was from running the patch set through get_maintainers.

It's just an ad-hoc since there are a few independent teams are doing
something that related to fwnode APIs: Daniel due to camera work for ACPI
enabled platforms, Qian due to a design bug with fwnode, Anand due to IIO
drivers, you are due to PHY (and possibly others I have no knowledge about).

I haven't asked for the others I listed above since their work probably
doesn't cross what you are doing, but camera might be closer.

> > It also appears [1] that somewhere in PHY code a bug is hidden
> > (at least I think so).
> 
> Thank you for this information. I'll keep an eye out!
> 
> > [1]: https://lore.kernel.org/lkml/20211113204141.520924-1-djrscally@gmail.com/T/#u
Vladimir Oltean Nov. 16, 2021, 5:41 p.m. UTC | #6
On Tue, Nov 16, 2021 at 07:32:08AM -0800, Colin Foster wrote:
> > > 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.

That sounds acceptable to me.
Vladimir Oltean Nov. 16, 2021, 10:56 p.m. UTC | #7
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 wanted to do some regression-testing with this patch set on the
Seville switch, but up until now I've been trying to actually make it
compile. See the changes required for that. Note that "can compile"
doesn't mean "can compile without warnings". Please check the build
reports on each individual patch on Patchwork and make sure the next
submission is warning-free. Note that there's a considerable amount of
drivers to build-test in both on and off configurations.
https://patchwork.kernel.org/project/netdevbpf/patch/20211116062328.1949151-21-colin.foster@in-advantage.com/

-- >8 -------------------------------------------------------------------------
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index b1032b7abaea..fbe78357ca94 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1127,11 +1127,13 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
 		struct phylink_pcs *phylink_pcs = felix->pcs[port];
+		struct mdio_device *mdio_device;
 
 		if (!phylink_pcs)
 			continue;
 
-		mdio_device_free(phylink_pcs->mdio);
+		mdio_device = lynx_get_mdio_device(phylink_pcs);
+		mdio_device_free(mdio_device);
 		lynx_pcs_destroy(phylink_pcs);
 	}
 	mdiobus_unregister(felix->imdio);
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 268c09042824..12a87d8f977d 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1037,7 +1037,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 			continue;
 
 		mdio_device = mdio_device_create(felix->imdio, addr);
-		if (IS_ERR(pcs))
+		if (IS_ERR(mdio_device))
 			continue;
 
 		phylink_pcs = lynx_pcs_create(mdio_device);
@@ -1066,7 +1066,7 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
 		if (!phylink_pcs)
 			continue;
 
-		mdio_device = lynx_pcs_get_mdio(phylink_pcs);
+		mdio_device = lynx_get_mdio_device(phylink_pcs);
 		mdio_device_free(mdio_device);
 		lynx_pcs_destroy(phylink_pcs);
 	}
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 3d93ac1376c6..3ab581b777eb 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -8,6 +8,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
+#include <linux/pcs-lynx.h>
 #include "enetc_ierb.h"
 #include "enetc_pf.h"
 
@@ -983,7 +984,7 @@ static void enetc_pl_mac_config(struct phylink_config *config,
 
 	priv = netdev_priv(pf->si->ndev);
 	if (pf->pcs)
-		phylink_set_pcs(priv->phylink, &pf->pcs);
+		phylink_set_pcs(priv->phylink, pf->pcs);
 }
 
 static void enetc_force_rgmii_mac(struct enetc_hw *hw, int speed, int duplex)
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index f8d2494b335c..5f9fc9252c79 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <soc/mscc/ocelot.h>
 
 #include "core.h"
 #include "pinconf.h"
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 6aeb7eac73f5..7571becba545 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -946,11 +946,12 @@ int ocelot_pinctrl_core_probe(struct device *dev,
 			      struct regmap *pincfg_base, u32 pincfg_offset,
 			      struct device_node *device_node);
 #else
-int ocelot_pinctrl_core_probe(struct device *dev,
-			      struct pinctrl_desc *pinctrl_desc,
-			      struct regmap *regmap_base, u32 regmap_offset,
-			      struct regmap *pincfg_base, u32 pincfg_offset,
-			      struct device_node *device_node)
+static inline int
+ocelot_pinctrl_core_probe(struct device *dev,
+			  struct pinctrl_desc *pinctrl_desc,
+			  struct regmap *regmap_base, u32 regmap_offset,
+			  struct regmap *pincfg_base, u32 pincfg_offset,
+			  struct device_node *device_node)
 {
 	return -EOPNOTSUPP;
 }
@@ -960,8 +961,9 @@ int ocelot_pinctrl_core_probe(struct device *dev,
 int microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
 			       struct regmap *regmap, u32 offset);
 #else
-int microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
-			       struct regmap *regmap, u32 offset)
+static inline int
+microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
+			   struct regmap *regmap, u32 offset)
 {
 	return -EOPNOTSUPP;
 }
-- >8 -------------------------------------------------------------------------
Colin Foster Nov. 16, 2021, 11:44 p.m. UTC | #8
On Tue, Nov 16, 2021 at 10:56:54PM +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 wanted to do some regression-testing with this patch set on the
> Seville switch, but up until now I've been trying to actually make it
> compile. See the changes required for that. Note that "can compile"
> doesn't mean "can compile without warnings". Please check the build
> reports on each individual patch on Patchwork and make sure the next
> submission is warning-free. Note that there's a considerable amount of
> drivers to build-test in both on and off configurations.
> https://patchwork.kernel.org/project/netdevbpf/patch/20211116062328.1949151-21-colin.foster@in-advantage.com/

I'm very embarrassed. I scrambled at the end to try to clean things up
and didn't run enough tests. Sorry about that!

> 
> -- >8 -------------------------------------------------------------------------
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index b1032b7abaea..fbe78357ca94 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1127,11 +1127,13 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
>  
>  	for (port = 0; port < ocelot->num_phys_ports; port++) {
>  		struct phylink_pcs *phylink_pcs = felix->pcs[port];
> +		struct mdio_device *mdio_device;
>  
>  		if (!phylink_pcs)
>  			continue;
>  
> -		mdio_device_free(phylink_pcs->mdio);
> +		mdio_device = lynx_get_mdio_device(phylink_pcs);
> +		mdio_device_free(mdio_device);
>  		lynx_pcs_destroy(phylink_pcs);
>  	}
>  	mdiobus_unregister(felix->imdio);
> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> index 268c09042824..12a87d8f977d 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -1037,7 +1037,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
>  			continue;
>  
>  		mdio_device = mdio_device_create(felix->imdio, addr);
> -		if (IS_ERR(pcs))
> +		if (IS_ERR(mdio_device))
>  			continue;
>  
>  		phylink_pcs = lynx_pcs_create(mdio_device);
> @@ -1066,7 +1066,7 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
>  		if (!phylink_pcs)
>  			continue;
>  
> -		mdio_device = lynx_pcs_get_mdio(phylink_pcs);
> +		mdio_device = lynx_get_mdio_device(phylink_pcs);
>  		mdio_device_free(mdio_device);
>  		lynx_pcs_destroy(phylink_pcs);
>  	}
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 3d93ac1376c6..3ab581b777eb 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -8,6 +8,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
> +#include <linux/pcs-lynx.h>
>  #include "enetc_ierb.h"
>  #include "enetc_pf.h"
>  
> @@ -983,7 +984,7 @@ static void enetc_pl_mac_config(struct phylink_config *config,
>  
>  	priv = netdev_priv(pf->si->ndev);
>  	if (pf->pcs)
> -		phylink_set_pcs(priv->phylink, &pf->pcs);
> +		phylink_set_pcs(priv->phylink, pf->pcs);
>  }
>  
>  static void enetc_force_rgmii_mac(struct enetc_hw *hw, int speed, int duplex)
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index f8d2494b335c..5f9fc9252c79 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> +#include <soc/mscc/ocelot.h>
>  
>  #include "core.h"
>  #include "pinconf.h"
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 6aeb7eac73f5..7571becba545 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -946,11 +946,12 @@ int ocelot_pinctrl_core_probe(struct device *dev,
>  			      struct regmap *pincfg_base, u32 pincfg_offset,
>  			      struct device_node *device_node);
>  #else
> -int ocelot_pinctrl_core_probe(struct device *dev,
> -			      struct pinctrl_desc *pinctrl_desc,
> -			      struct regmap *regmap_base, u32 regmap_offset,
> -			      struct regmap *pincfg_base, u32 pincfg_offset,
> -			      struct device_node *device_node)
> +static inline int
> +ocelot_pinctrl_core_probe(struct device *dev,
> +			  struct pinctrl_desc *pinctrl_desc,
> +			  struct regmap *regmap_base, u32 regmap_offset,
> +			  struct regmap *pincfg_base, u32 pincfg_offset,
> +			  struct device_node *device_node)
>  {
>  	return -EOPNOTSUPP;
>  }
> @@ -960,8 +961,9 @@ int ocelot_pinctrl_core_probe(struct device *dev,
>  int microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
>  			       struct regmap *regmap, u32 offset);
>  #else
> -int microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
> -			       struct regmap *regmap, u32 offset)
> +static inline int
> +microchip_sgpio_core_probe(struct device *dev, struct device_node *node,
> +			   struct regmap *regmap, u32 offset)
>  {
>  	return -EOPNOTSUPP;
>  }
> -- >8 -------------------------------------------------------------------------
Vladimir Oltean Nov. 16, 2021, 11:47 p.m. UTC | #9
On Tue, Nov 16, 2021 at 03:44:13PM -0800, Colin Foster wrote:
> On Tue, Nov 16, 2021 at 10:56:54PM +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 wanted to do some regression-testing with this patch set on the
> > Seville switch, but up until now I've been trying to actually make it
> > compile. See the changes required for that. Note that "can compile"
> > doesn't mean "can compile without warnings". Please check the build
> > reports on each individual patch on Patchwork and make sure the next
> > submission is warning-free. Note that there's a considerable amount of
> > drivers to build-test in both on and off configurations.
> > https://patchwork.kernel.org/project/netdevbpf/patch/20211116062328.1949151-21-colin.foster@in-advantage.com/
> 
> I'm very embarrassed. I scrambled at the end to try to clean things up
> and didn't run enough tests. Sorry about that!

Don't worry, just make a note of one more thing to check next time.
My T1040RDB doesn't seem to want to boot right now, I don't know what's
up with it. My regression testing might be delayed for a little bit,
sorry for that.
Linus Walleij Nov. 19, 2021, 1:58 a.m. UTC | #10
Hi Colin,

nice work!

On Tue, Nov 16, 2021 at 7:23 AM Colin Foster
<colin.foster@in-advantage.com> wrote:

>   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

Can these patches be broken out to its own series and handled
separately from the DSA stuff or is there build-time dependencies?

Yours,
Linus Walleij
Colin Foster Nov. 19, 2021, 2:14 a.m. UTC | #11
Hi Linus,

On Fri, Nov 19, 2021 at 02:58:47AM +0100, Linus Walleij wrote:
> Hi Colin,
> 
> nice work!

Thanks :)

> 
> On Tue, Nov 16, 2021 at 7:23 AM Colin Foster
> <colin.foster@in-advantage.com> wrote:
> 
> >   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
> 
> Can these patches be broken out to its own series and handled
> separately from the DSA stuff or is there build-time dependencies?

These should all be able to be a separate series if I did my job right. 
Everything should have no functional change except for this:

> >   pinctrl: ocelot: update pinctrl to automatic base address

Fortunately this was tested by Clément and didn't seem to have any
ill-fated side effects. 

I assume this isn't something I wouldn't want to submit to net-next...
is there a different place (tree? board? list?) where those should be
submitted?

> 
> Yours,
> Linus Walleij
Linus Walleij Nov. 21, 2021, 11:59 p.m. UTC | #12
On Fri, Nov 19, 2021 at 3:14 AM Colin Foster
<colin.foster@in-advantage.com> wrote:
> On Fri, Nov 19, 2021 at 02:58:47AM +0100, Linus Walleij wrote:

> > >   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
> >
> > Can these patches be broken out to its own series and handled
> > separately from the DSA stuff or is there build-time dependencies?
>
> These should all be able to be a separate series if I did my job right.
> Everything should have no functional change except for this:
>
> > >   pinctrl: ocelot: update pinctrl to automatic base address
>
> Fortunately this was tested by Clément and didn't seem to have any
> ill-fated side effects.
>
> I assume this isn't something I wouldn't want to submit to net-next...
> is there a different place (tree? board? list?) where those should be
> submitted?

To the pinctrl maintainer i.e. me + linux-gpio@vger.kernel.org
then I can just apply them if there are no more review comments.

Yours,
Linus Walleij