[RFC v5 net-next 00/13] add support for VSC75XX control over SPI

[RFC v5 net-next 00/13] add support for VSC75XX control over SPI

[Colin Foster]

The patch set in general is to add support for the VSC7511, VSC7512,
VSC7513 and VSC7514 devices controlled over SPI. The driver is
believed to be fully functional for the internal phy ports (0-3) 
on the VSC7512. As I'll discuss, it is not yet functional for other
ports yet.


V5 is an overhaul of the framework. Instead of struct spi_device being
called directly into drivers/net/dsa/ocelot/ocelot_spi.c, it is now done
through drivers/net/mfd/ocelot-spi.c. The MFD layer is handled in
drivers/net/mfd/ocelot-core.c. At the end of this patch set, three
optional devices are supported: ocelot-pinctrl to control pins, 
mdio-mscc-miim to communicate on the external MDIO bus, and
ocelot-ext to manage the switch.

This structure seems to really clean things up. "ocelot_ext" comes in
around 650 lines now, down from 950 in v4. Much of that has moved to
ocelot-spi.c. All references to SPI have been removed from ocelot_ext. I
know lines of a file isn't a great metric of software design quality,
but I think they are related in this case.

My hope at the end of this RFC is to be able to split this into several
smaller patch sets. One set would add MFD + pinctrl, and possibly MDIO.
To the best of my current knowledge those drivers are functionally
complete and possibly ready for full "PATCH" status instead of RFC. 

Of note: the MFD uses hard-coded resources, while the MMIO version uses
DTS resources. If nothing else, I'll update the relevant documentation
accordingly.

A separate patch set would be to add switch capabilities. Since the
external phys are now functional, I can turn my focus to those
additional ports. 

That leads to a direct question: Is it acceptable to do switch
capabilities in two patch sets? Set 1 would add internal phy
functionality that currently works, set 2 would add PCS / SGMII support
to ports 4-7? For full transparency: I don't have Fib{re,er} 
hardware so I don't plan to develop that support. I simply won't be able
to test it.

As I mentioned in the last RFC:
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, and has changed in
architecture significantly since V4:

&spi0 {
	#address-cells = <1>;
	#size-cells = <0>;
	status = "okay";

	ocelot-chip@0 {
		compatible = "mscc,vsc7512_mfd_spi";
		spi-max-frequency = <250000>;
		reg = <0>;

		ethernet-switch@0 {
			compatible = "mscc,vsc7512-ext-switch";
			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 {
					reg = <0x0>;
				};

				sw_phy1: ethernet-phy@1 {
					reg = <0x1>;
				};

				sw_phy2: ethernet-phy@2 {
					reg = <0x2>;
				};

				sw_phy3: ethernet-phy@3 {
					reg = <0x3>;
				};
			};
		};

		mdio1: mdio1 {
			compatible = "mscc,ocelot-miim";
			pinctrl-names = "default";
			pinctrl-0 = <&miim1>;
			#address-cells = <1>;
			#size-cells = <0>;

			sw_phy4: ethernet-phy@4 {
				reg = <0x4>;
			};

			sw_phy5: ethernet-phy@5 {
				reg = <0x5>;
			};

			sw_phy6: ethernet-phy@6 {
				reg = <0x6>;
			};

			sw_phy7: ethernet-phy@7 {
				reg = <0x7>;
			};
		};

		gpio: pinctrl@0 {
			compatible = "mscc,ocelot-pinctrl";
			gpio-controller;
			#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";
			};

			miim1: miim1 {
				pins = "GPIO_14", "GPIO_15";
				function = "miim";
			};
		};

        /* Non-functional at the moment */
		sgpio: sgpio {
			compatible = "mscc,ocelot-sgpio";
			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>;
			};
		};
	};
};



The relevant boot log for the switch / MDIO bus is here. Of note: ports
4-7 are now properly probed before the switch comes up.

[    1.357533] SPI driver ocelot_mfd_spi has no spi_device_id for mscc,vsc7514_mfd_spi
[    1.357561] SPI driver ocelot_mfd_spi has no spi_device_id for mscc,vsc7513_mfd_spi
[    1.357571] SPI driver ocelot_mfd_spi has no spi_device_id for mscc,vsc7512_mfd_spi
[    1.357581] SPI driver ocelot_mfd_spi has no spi_device_id for mscc,vsc7511_mfd_spi
[    3.159000] ocelot_mfd_spi spi0.0: configured SPI bus for speed 250000, rx padding bytes 0
[    3.167549] ocelot_mfd_spi spi0.0: initializing SPI interface for chip
[    3.175088] ocelot_mfd_spi spi0.0: resetting ocelot chip
[    3.301006] ocelot_mfd_spi spi0.0: initializing SPI interface for chip
[    3.309537] pinctrl-ocelot pinctrl-ocelot: DMA mask not set
[    3.315694] gpiochip_find_base: found new base at 2026
[    3.315737] gpio gpiochip4: (ocelot-gpio): created GPIO range 0->21 ==> pinctrl-ocelot PIN 0->21
[    3.321957] gpio gpiochip4: (ocelot-gpio): added GPIO chardev (254:4)
[    3.322059] gpio gpiochip4: registered GPIOs 2026 to 2047 on ocelot-gpio
[    3.322076] pinctrl-ocelot pinctrl-ocelot: driver registered
[    3.331370] mscc-miim ocelot-miim1: DMA mask not set
[    3.337044] mdio_bus ocelot-miim1-mii: GPIO lookup for consumer reset
[    3.337065] mdio_bus ocelot-miim1-mii: using device tree for GPIO lookup
[    3.337088] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/mdio1[0]'
[    3.337141] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/mdio1[0]'
[    3.337190] mdio_bus ocelot-miim1-mii: using lookup tables for GPIO lookup
[    3.337202] mdio_bus ocelot-miim1-mii: No GPIO consumer reset found
[    3.337215] libphy: mscc_miim: probed
[    3.343764] mdio_bus ocelot-miim1-mii:04: GPIO lookup for consumer reset
[    3.343788] mdio_bus ocelot-miim1-mii:04: using device tree for GPIO lookup
[    3.343809] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/mdio1/ethernet-phy@4[0]'
[    3.343873] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/mdio1/ethernet-phy@4[0]'
[    3.343928] mdio_bus ocelot-miim1-mii:04: using lookup tables for GPIO lookup
[    3.343939] mdio_bus ocelot-miim1-mii:04: No GPIO consumer reset found
# Repeated for 5, 6 and 7
[    3.355921] ocelot-ext-switch ocelot-ext-switch: DMA mask not set
[    3.864119] mdio_bus ocelot-ext-switch-mii: GPIO lookup for consumer reset
[    3.864151] mdio_bus ocelot-ext-switch-mii: using device tree for GPIO lookup
[    3.864176] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio[0]'
[    3.864236] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio[0]'
[    3.864291] mdio_bus ocelot-ext-switch-mii: using lookup tables for GPIO lookup
[    3.864303] mdio_bus ocelot-ext-switch-mii: No GPIO consumer reset found
[    4.363826] libphy: ocelot_ext MDIO bus: probed
[    4.371316] mdio_bus ocelot-ext-switch-mii:00: GPIO lookup for consumer reset
[    4.371340] mdio_bus ocelot-ext-switch-mii:00: using device tree for GPIO lookup
[    4.371360] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio/ethernet-phy@0[0]'
[    4.371433] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio/ethernet-phy@0[0]'
[    4.371495] mdio_bus ocelot-ext-switch-mii:00: using lookup tables for GPIO lookup
[    4.371507] mdio_bus ocelot-ext-switch-mii:00: No GPIO consumer reset found
[    4.374476] mdio_bus ocelot-ext-switch-mii:01: GPIO lookup for consumer reset
[    4.374504] mdio_bus ocelot-ext-switch-mii:01: using device tree for GPIO lookup
[    4.374524] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio/ethernet-phy@1[0]'
[    4.374597] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@48000000/segment@0/target-module@30000/spi@0/ocelot-chip@0/ethernet-switch@0/mdio/ethernet-phy@1[0]'
[    4.374657] mdio_bus ocelot-ext-switch-mii:01: using lookup tables for GPIO lookup
[    4.374669] mdio_bus ocelot-ext-switch-mii:01: No GPIO consumer reset found
# Repeated for 2 and 3
[    8.105647] ocelot-ext-switch ocelot-ext-switch: PHY [ocelot-ext-switch-mii:00] driver [Generic PHY] (irq=POLL)
[    8.119027] ocelot-ext-switch ocelot-ext-switch: configuring for phy/internal link mode
[    8.145065] ocelot-ext-switch ocelot-ext-switch swp1 (uninitialized): PHY [ocelot-ext-switch-mii:01] driver [Generic PHY] (irq=POLL)
[    8.163599] ocelot-ext-switch ocelot-ext-switch swp2 (uninitialized): PHY [ocelot-ext-switch-mii:02] driver [Generic PHY] (irq=POLL)
[    8.182016] ocelot-ext-switch ocelot-ext-switch swp3 (uninitialized): PHY [ocelot-ext-switch-mii:03] driver [Generic PHY] (irq=POLL)
[    8.200313] ocelot-ext-switch ocelot-ext-switch swp4 (uninitialized): PHY [ocelot-miim1-mii:04] driver [Generic PHY] (irq=POLL)
[    8.218238] ocelot-ext-switch ocelot-ext-switch swp5 (uninitialized): PHY [ocelot-miim1-mii:05] driver [Generic PHY] (irq=POLL)
[    8.236204] ocelot-ext-switch ocelot-ext-switch swp6 (uninitialized): PHY [ocelot-miim1-mii:06] driver [Generic PHY] (irq=POLL)
[    8.254153] ocelot-ext-switch ocelot-ext-switch swp7 (uninitialized): PHY [ocelot-miim1-mii:07] driver [Generic PHY] (irq=POLL)
[    8.277362] device eth0 entered promiscuous mode
[    8.282119] DSA: tree 0 setup
[    8.285250] ocelot_mfd_spi spi0.0: ocelot mfd core setup complete
[    8.297532] ocelot_mfd_spi spi0.0: ocelot spi mfd probed
[   10.259154] ocelot-ext-switch ocelot-ext-switch: Link is Up - 100Mbps/Full - flow control off

Then I enable ports 1-3 on a bridge with STP with an intentional loop of
port 2 plugged directly into port 3:

[   21.040578] cpsw-switch 4a100000.switch: starting ndev. mode: dual_mac
[   21.133226] SMSC LAN8710/LAN8720 4a101000.mdio:00: attached PHY driver (mii_bus:phy_addr=4a101000.mdio:00, irq=POLL)
[   21.150341] 8021q: adding VLAN 0 to HW filter on device eth0
[   21.175435] ocelot-ext-switch ocelot-ext-switch swp1: configuring for phy/internal link mode
[   21.210804] ocelot-ext-switch ocelot-ext-switch swp2: configuring for phy/internal link mode
[   21.245110] ocelot-ext-switch ocelot-ext-switch swp3: configuring for phy/internal link mode
[   21.310462] br0: port 1(swp1) entered blocking state
[   21.315670] br0: port 1(swp1) entered disabled state
[   21.334172] device swp1 entered promiscuous mode
[   21.365554] br0: port 2(swp2) entered blocking state
[   21.370594] br0: port 2(swp2) entered disabled state
[   21.388041] device swp2 entered promiscuous mode
[   21.410549] br0: port 3(swp3) entered blocking state
[   21.415726] br0: port 3(swp3) entered disabled state
[   21.432923] device swp3 entered promiscuous mode
[   21.713724] ocelot-ext-switch ocelot-ext-switch: Link is Down
[   22.741120] ocelot-ext-switch ocelot-ext-switch: Link is Up - 100Mbps/Full - flow control off
[   23.382081] cpsw-switch 4a100000.switch eth0: Link is Up - 100Mbps/Full - flow control off
[   23.392704] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[   24.339517] ocelot-ext-switch ocelot-ext-switch swp2: Link is Up - 1Gbps/Full - flow control rx/tx
[   24.348688] IPv6: ADDRCONF(NETDEV_CHANGE): swp2: link becomes ready
[   24.355215] br0: port 2(swp2) entered blocking state
[   24.360220] br0: port 2(swp2) entered listening state
[   24.427317] ocelot-ext-switch ocelot-ext-switch swp3: Link is Up - 1Gbps/Full - flow control rx/tx
[   24.436649] IPv6: ADDRCONF(NETDEV_CHANGE): swp3: link becomes ready
[   24.443458] br0: port 3(swp3) entered blocking state
[   24.448507] br0: port 3(swp3) entered listening state
[   25.463130] ocelot-ext-switch ocelot-ext-switch swp1: Link is Up - 1Gbps/Full - flow control rx/tx
[   25.472385] IPv6: ADDRCONF(NETDEV_CHANGE): swp1: link becomes ready
[   25.479062] br0: port 1(swp1) entered blocking state
[   25.484241] br0: port 1(swp1) entered listening state
[   26.091585] br0: port 3(swp3) entered blocking state
[   39.531066] br0: port 2(swp2) entered learning state
[   40.811057] br0: port 1(swp1) entered learning state
[   54.891054] br0: port 2(swp2) entered forwarding state
[   54.896328] br0: topology change detected, propagating
[   54.906787] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
[   55.531415] br0: received packet on swp2 with own address as source address (addr:*, vlan:0)
[   56.171057] br0: port 1(swp1) entered forwarding state
[   56.176312] br0: topology change detected, propagating
[   90.091617] br0: received packet on swp2 with own address as source address (addr:*, vlan:0)

(I'm curious about those swp2 packets that trickle through...)

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. I also had to change
bonecommon.dtsi to avoid using VLAN 0.


Lastly, this patch set relies on changes that have been merged in from
linux-pinctrl/next. kernel-test-robot will complain that the last two
patches won't compile and might not even apply. The path forward might
be to get the MFD / pinctrl into linux-pinctrl before the next merge
window. I doubt the ocelot-ext will be fully functional by then, though
it does feel close!


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. 

v5
    * Restructured to MFD
    * Several commits were split out, submitted, and accepted
    * pinctrl-ocelot believed to be fully functional (requires commits
    from the linux-pinctrl tree)
    * External MDIO bus believed to be fully functional


Colin Foster (13):
  mfd: ocelot: add support for external mfd control over SPI for the
    VSC7512
  mfd: ocelot: offer an interface for MFD children to get regmaps
  net: mscc: ocelot: expose ocelot wm functions
  net: dsa: felix: add configurable device quirks
  net: mdio: mscc-miim: add ability to externally register phy reset
    control
  net: dsa: ocelot: add external ocelot switch control
  mfd: ocelot: enable the external switch interface
  mfd: add interface to check whether a device is mfd
  net: mdio: mscc-miim: add local dev variable to cleanup probe function
  net: mdio: mscc-miim: add MFD functionality through ocelot-core
  mfd: ocelot-core: add control for the external mdio interface
  pinctrl: ocelot: add MFD functionality through ocelot-core
  mfd: ocelot: add ocelot-pinctrl as a supported child interface

 drivers/mfd/Kconfig                        |  15 +
 drivers/mfd/Makefile                       |   3 +
 drivers/mfd/mfd-core.c                     |   5 +
 drivers/mfd/ocelot-core.c                  | 203 +++++++
 drivers/mfd/ocelot-mfd.h                   |  19 +
 drivers/mfd/ocelot-spi.c                   | 374 ++++++++++++
 drivers/net/dsa/ocelot/Kconfig             |  15 +
 drivers/net/dsa/ocelot/Makefile            |   5 +
 drivers/net/dsa/ocelot/felix.c             |   7 +-
 drivers/net/dsa/ocelot/felix.h             |   1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c     |   1 +
 drivers/net/dsa/ocelot/ocelot_ext.c        | 644 +++++++++++++++++++++
 drivers/net/dsa/ocelot/seville_vsc9953.c   |   4 +-
 drivers/net/ethernet/mscc/ocelot_devlink.c |  31 +
 drivers/net/ethernet/mscc/ocelot_vsc7514.c |  28 -
 drivers/net/mdio/mdio-mscc-miim.c          |  50 +-
 drivers/pinctrl/pinctrl-ocelot.c           |  30 +-
 include/linux/mdio/mdio-mscc-miim.h        |   3 +-
 include/linux/mfd/core.h                   |  10 +
 include/soc/mscc/ocelot.h                  |  19 +
 20 files changed, 1410 insertions(+), 57 deletions(-)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-mfd.h
 create mode 100644 drivers/mfd/ocelot-spi.c
 create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c

-- 
2.25.1

[RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

Create a single SPI MFD ocelot device that manages the SPI bus on the
external chip and can handle requests for regmaps. This should allow any
ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
utilize regmaps.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/Kconfig       |  15 ++
 drivers/mfd/Makefile      |   3 +
 drivers/mfd/ocelot-core.c | 149 +++++++++++++++
 drivers/mfd/ocelot-mfd.h  |  19 ++
 drivers/mfd/ocelot-spi.c  | 374 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 560 insertions(+)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-mfd.h
 create mode 100644 drivers/mfd/ocelot-spi.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3fb480818599..af76c9780a10 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -954,6 +954,21 @@ config MFD_MENF21BMC
 	  This driver can also be built as a module. If so the module
 	  will be called menf21bmc.
 
+config MFD_OCELOT_CORE
+	tristate "Microsemi Ocelot External Control Support"
+	select MFD_CORE
+	help
+	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
+	  VSC7513, VSC7514) controlled externally.
+
+config MFD_OCELOT_SPI
+	tristate "Microsemi Ocelot SPI interface"
+	depends on MFD_OCELOT_CORE
+	depends on SPI_MASTER
+	select REGMAP_SPI
+	help
+	  Say yes here to add control to the MFD_OCELOT chips via SPI.
+
 config EZX_PCAP
 	bool "Motorola EZXPCAP Support"
 	depends on SPI_MASTER
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0b1b629aef3e..dff83f474fb5 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -120,6 +120,9 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
+obj-$(CONFIG_MFD_OCELOT_CORE)	+= ocelot-core.o
+obj-$(CONFIG_MFD_OCELOT_SPI)	+= ocelot-spi.o
+
 obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
 obj-$(CONFIG_MFD_CPCAP)		+= motorola-cpcap.o
 
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
new file mode 100644
index 000000000000..a65619a8190b
--- /dev/null
+++ b/drivers/mfd/ocelot-core.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021 Innovative Advantage Inc.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/spi/spi.h>
+#include <linux/kconfig.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "ocelot-mfd.h"
+
+#define REG(reg, offset)	[reg] = offset
+
+enum ocelot_mfd_gcb_regs {
+	GCB_SOFT_RST,
+	GCB_REG_MAX,
+};
+
+enum ocelot_mfd_gcb_regfields {
+	GCB_SOFT_RST_CHIP_RST,
+	GCB_REGFIELD_MAX,
+};
+
+static const u32 vsc7512_gcb_regmap[] = {
+	REG(GCB_SOFT_RST,	0x0008),
+};
+
+static const struct reg_field vsc7512_mfd_gcb_regfields[GCB_REGFIELD_MAX] = {
+	[GCB_SOFT_RST_CHIP_RST] = REG_FIELD(vsc7512_gcb_regmap[GCB_SOFT_RST], 0, 0),
+};
+
+struct ocelot_mfd_core {
+	struct ocelot_mfd_config *config;
+	struct regmap *gcb_regmap;
+	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
+};
+
+static const struct resource vsc7512_gcb_resource = {
+	.start	= 0x71070000,
+	.end	= 0x7107022b,
+	.name	= "devcpu_gcb",
+};
+
+static int ocelot_mfd_reset(struct ocelot_mfd_core *core)
+{
+	int ret;
+
+	dev_info(core->config->dev, "resetting ocelot chip\n");
+
+	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
+	 * suggest how to do a reset over SPI, and the register strategy isn't
+	 * possible.
+	 */
+	msleep(100);
+
+	ret = core->config->init_bus(core->config);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
+				  int size)
+{
+	if (res->name)
+		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
+	else
+		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
+}
+EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
+
+static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
+					     const struct resource *res)
+{
+	struct device *dev = core->config->dev;
+	struct regmap *regmap;
+	char name[32];
+
+	ocelot_mfd_get_resource_name(name, res, sizeof(name) - 1);
+
+	regmap = dev_get_regmap(dev, name);
+
+	if (!regmap)
+		regmap = core->config->get_regmap(core->config, res, name);
+
+	return regmap;
+}
+
+int ocelot_mfd_init(struct ocelot_mfd_config *config)
+{
+	struct device *dev = config->dev;
+	const struct reg_field *regfield;
+	struct ocelot_mfd_core *core;
+	int i, ret;
+
+	core = devm_kzalloc(dev, sizeof(struct ocelot_mfd_config), GFP_KERNEL);
+	if (!core)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, core);
+
+	core->config = config;
+
+	/* Create regmaps and regfields here */
+	core->gcb_regmap = ocelot_mfd_regmap_init(core, &vsc7512_gcb_resource);
+	if (!core->gcb_regmap)
+		return -ENOMEM;
+
+	for (i = 0; i < GCB_REGFIELD_MAX; i++) {
+		regfield = &vsc7512_mfd_gcb_regfields[i];
+		core->gcb_regfields[i] =
+			devm_regmap_field_alloc(dev, core->gcb_regmap,
+						*regfield);
+		if (!core->gcb_regfields[i])
+			return -ENOMEM;
+	}
+
+	/* Prepare the chip */
+	ret = ocelot_mfd_reset(core);
+	if (ret) {
+		dev_err(dev, "ocelot mfd reset failed with code %d\n", ret);
+		return ret;
+	}
+
+	/* Create and loop over all child devices here */
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_mfd_init);
+
+int ocelot_mfd_remove(struct ocelot_mfd_config *config)
+{
+	/* Loop over all children and remove them */
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_mfd_remove);
+
+MODULE_DESCRIPTION("Ocelot Chip MFD driver");
+MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/ocelot-mfd.h b/drivers/mfd/ocelot-mfd.h
new file mode 100644
index 000000000000..6af8b8c5a316
--- /dev/null
+++ b/drivers/mfd/ocelot-mfd.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright 2021 Innovative Advantage Inc.
+ */
+
+#include <linux/regmap.h>
+
+struct ocelot_mfd_config {
+	struct device *dev;
+	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
+				     const struct resource *res,
+				     const char *name);
+	int (*init_bus)(struct ocelot_mfd_config *config);
+};
+
+void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
+				  int size);
+int ocelot_mfd_init(struct ocelot_mfd_config *config);
+int ocelot_mfd_remove(struct ocelot_mfd_config *config);
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
new file mode 100644
index 000000000000..65ceb68f27af
--- /dev/null
+++ b/drivers/mfd/ocelot-spi.c
@@ -0,0 +1,374 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021 Innovative Advantage Inc.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/spi/spi.h>
+#include <linux/iopoll.h>
+#include <linux/kconfig.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "ocelot-mfd.h"
+
+#define REG(reg, offset)	[reg] = offset
+
+struct ocelot_spi {
+	int spi_padding_bytes;
+	struct spi_device *spi;
+	struct ocelot_mfd_config config;
+	struct regmap *cpuorg_regmap;
+	const u32 *map;
+};
+
+enum ocelot_dev_cpuorg_regs {
+	DEV_CPUORG_IF_CTRL,
+	DEV_CPUORG_IF_CFGSTAT,
+	DEV_CPUORG_REG_MAX,
+};
+
+static const u32 vsc7512_dev_cpuorg_regmap[] = {
+	REG(DEV_CPUORG_IF_CTRL,		0x0000),
+	REG(DEV_CPUORG_IF_CFGSTAT,	0x0004),
+};
+
+static const struct resource vsc7512_dev_cpuorg_resource = {
+	.start	= 0x71000000,
+	.end	= 0x710002ff,
+	.name	= "devcpu_org",
+};
+
+#define VSC7512_BYTE_ORDER_LE 0x00000000
+#define VSC7512_BYTE_ORDER_BE 0x81818181
+#define VSC7512_BIT_ORDER_MSB 0x00000000
+#define VSC7512_BIT_ORDER_LSB 0x42424242
+
+static struct ocelot_spi *
+config_to_ocelot_spi(struct ocelot_mfd_config *config)
+{
+	return container_of(config, struct ocelot_spi, config);
+}
+
+static int ocelot_spi_init_bus(struct ocelot_spi *ocelot_spi)
+{
+	struct spi_device *spi;
+	struct device *dev;
+	u32 val, check;
+	int err;
+
+	spi = ocelot_spi->spi;
+	dev = &spi->dev;
+
+	dev_info(dev, "initializing SPI interface for chip\n");
+
+	val = 0;
+
+#ifdef __LITTLE_ENDIAN
+	val |= VSC7512_BYTE_ORDER_LE;
+#else
+	val |= VSC7512_BYTE_ORDER_BE;
+#endif
+
+	err = regmap_write(ocelot_spi->cpuorg_regmap,
+			   ocelot_spi->map[DEV_CPUORG_IF_CTRL], val);
+	if (err)
+		return err;
+
+	val = ocelot_spi->spi_padding_bytes;
+	err = regmap_write(ocelot_spi->cpuorg_regmap,
+			   ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], val);
+	if (err)
+		return err;
+
+	check = val | 0x02000000;
+
+	err = regmap_read(ocelot_spi->cpuorg_regmap,
+			  ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], &val);
+	if (err)
+		return err;
+
+	if (check != val) {
+		dev_err(dev, "Error configuring SPI bus. V: 0x%08x != 0x%08x\n",
+			val, check);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int ocelot_spi_init_bus_from_config(struct ocelot_mfd_config *config)
+{
+	struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
+
+	return ocelot_spi_init_bus(ocelot_spi);
+}
+
+static unsigned int ocelot_spi_translate_address(unsigned int reg)
+{
+	return cpu_to_be32((reg & 0xffffff) >> 2);
+}
+
+struct ocelot_spi_regmap_context {
+	struct spi_device *spi;
+	u32 base;
+	int padding_bytes;
+};
+
+static int ocelot_spi_reg_read(void *context, unsigned int reg,
+			       unsigned int *val)
+{
+	struct ocelot_spi_regmap_context *regmap_context = context;
+	struct spi_transfer tx, padding, rx;
+	struct ocelot_spi *ocelot_spi;
+	struct spi_message msg;
+	struct spi_device *spi;
+	unsigned int addr;
+	u8 *tx_buf;
+
+	WARN_ON(!val);
+
+	spi = regmap_context->spi;
+
+	ocelot_spi = spi_get_drvdata(spi);
+
+	addr = ocelot_spi_translate_address(reg + regmap_context->base);
+	tx_buf = (u8 *)&addr;
+
+	spi_message_init(&msg);
+
+	memset(&tx, 0, sizeof(struct spi_transfer));
+
+	/* Ignore the first byte for the 24-bit address */
+	tx.tx_buf = &tx_buf[1];
+	tx.len = 3;
+
+	spi_message_add_tail(&tx, &msg);
+
+	if (regmap_context->padding_bytes > 0) {
+		u8 dummy_buf[16] = {0};
+
+		memset(&padding, 0, sizeof(struct spi_transfer));
+
+		/* Just toggle the clock for padding bytes */
+		padding.len = regmap_context->padding_bytes;
+		padding.tx_buf = dummy_buf;
+		padding.dummy_data = 1;
+
+		spi_message_add_tail(&padding, &msg);
+	}
+
+	memset(&rx, 0, sizeof(struct spi_transfer));
+	rx.rx_buf = val;
+	rx.len = 4;
+
+	spi_message_add_tail(&rx, &msg);
+
+	return spi_sync(spi, &msg);
+}
+
+static int ocelot_spi_reg_write(void *context, unsigned int reg,
+				unsigned int val)
+{
+	struct ocelot_spi_regmap_context *regmap_context = context;
+	struct spi_transfer tx[2] = {0};
+	struct spi_message msg;
+	struct spi_device *spi;
+	unsigned int addr;
+	u8 *tx_buf;
+
+	spi = regmap_context->spi;
+
+	addr = ocelot_spi_translate_address(reg + regmap_context->base);
+	tx_buf = (u8 *)&addr;
+
+	spi_message_init(&msg);
+
+	/* Ignore the first byte for the 24-bit address and set the write bit */
+	tx_buf[1] |= BIT(7);
+	tx[0].tx_buf = &tx_buf[1];
+	tx[0].len = 3;
+
+	spi_message_add_tail(&tx[0], &msg);
+
+	memset(&tx[1], 0, sizeof(struct spi_transfer));
+	tx[1].tx_buf = &val;
+	tx[1].len = 4;
+
+	spi_message_add_tail(&tx[1], &msg);
+
+	return spi_sync(spi, &msg);
+}
+
+static const struct regmap_config ocelot_spi_regmap_config = {
+	.reg_bits = 24,
+	.reg_stride = 4,
+	.val_bits = 32,
+
+	.reg_read = ocelot_spi_reg_read,
+	.reg_write = ocelot_spi_reg_write,
+
+	.max_register = 0xffffffff,
+	.use_single_write = true,
+	.use_single_read = true,
+	.can_multi_write = false,
+
+	.reg_format_endian = REGMAP_ENDIAN_BIG,
+	.val_format_endian = REGMAP_ENDIAN_NATIVE,
+};
+
+static struct regmap *
+ocelot_spi_get_regmap(struct ocelot_mfd_config *config,
+		      const struct resource *res, const char *name)
+{
+	struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
+	struct ocelot_spi_regmap_context *context;
+	struct regmap_config regmap_config;
+	struct regmap *regmap;
+	struct device *dev;
+
+
+	dev = &ocelot_spi->spi->dev;
+
+	/* Don't re-allocate another regmap if we have one */
+	regmap = dev_get_regmap(dev, name);
+	if (regmap)
+		return regmap;
+
+	context = devm_kzalloc(dev, sizeof(struct ocelot_spi_regmap_context),
+			       GFP_KERNEL);
+
+	if (IS_ERR(context))
+		return ERR_CAST(context);
+
+	context->base = res->start;
+	context->spi = ocelot_spi->spi;
+	context->padding_bytes = ocelot_spi->spi_padding_bytes;
+
+	memcpy(&regmap_config, &ocelot_spi_regmap_config,
+	       sizeof(ocelot_spi_regmap_config));
+
+	regmap_config.name = name;
+	regmap_config.max_register = res->end - res->start;
+
+	regmap = devm_regmap_init(dev, NULL, context, &regmap_config);
+	if (IS_ERR(regmap))
+		return ERR_CAST(regmap);
+
+	return regmap;
+}
+
+static int ocelot_spi_probe(struct spi_device *spi)
+{
+	struct ocelot_spi *ocelot_spi;
+	struct device *dev;
+	char name[32];
+	int err;
+
+	dev = &spi->dev;
+
+	ocelot_spi = devm_kzalloc(dev, sizeof(struct ocelot_spi),
+				      GFP_KERNEL);
+
+	if (!ocelot_spi)
+		return -ENOMEM;
+
+	if (spi->max_speed_hz <= 500000) {
+		ocelot_spi->spi_padding_bytes = 0;
+	} else {
+		/*
+		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
+		 * on the side of more padding bytes, as having too few can be
+		 * difficult to detect at runtime.
+		 */
+		ocelot_spi->spi_padding_bytes = 1 +
+			(spi->max_speed_hz / 1000000 + 2) / 8;
+	}
+
+	ocelot_spi->spi = spi;
+	ocelot_spi->map = vsc7512_dev_cpuorg_regmap;
+
+	spi->bits_per_word = 8;
+
+	err = spi_setup(spi);
+	if (err < 0) {
+		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
+		return err;
+	}
+
+	dev_info(dev, "configured SPI bus for speed %d, rx padding bytes %d\n",
+			spi->max_speed_hz, ocelot_spi->spi_padding_bytes);
+
+	/* Ensure we have devcpu_org regmap before we call ocelot_mfd_init */
+	ocelot_mfd_get_resource_name(name, &vsc7512_dev_cpuorg_resource,
+				     sizeof(name) - 1);
+
+	/*
+	 * Since we created dev, we know there isn't a regmap, so create one
+	 * here directly.
+	 */
+	ocelot_spi->cpuorg_regmap =
+		ocelot_spi_get_regmap(&ocelot_spi->config,
+				      &vsc7512_dev_cpuorg_resource, name);
+	if (!ocelot_spi->cpuorg_regmap)
+		return -ENOMEM;
+
+	ocelot_spi->config.init_bus = ocelot_spi_init_bus_from_config;
+	ocelot_spi->config.get_regmap = ocelot_spi_get_regmap;
+	ocelot_spi->config.dev = dev;
+
+	spi_set_drvdata(spi, ocelot_spi);
+
+	/*
+	 * The chip must be set up for SPI before it gets initialized and reset.
+	 * Do this once here before calling mfd_init
+	 */
+	err = ocelot_spi_init_bus(ocelot_spi);
+	if (err) {
+		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
+		return err;
+	}
+
+	err = ocelot_mfd_init(&ocelot_spi->config);
+	if (err < 0) {
+		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
+		return err;
+	}
+
+	dev_info(&spi->dev, "ocelot spi mfd probed\n");
+
+	return 0;
+}
+
+static int ocelot_spi_remove(struct spi_device *spi)
+{
+	struct ocelot_spi *ocelot_spi;
+
+	ocelot_spi = spi_get_drvdata(spi);
+	devm_kfree(&spi->dev, ocelot_spi);
+	return 0;
+}
+
+const struct of_device_id ocelot_mfd_of_match[] = {
+	{ .compatible = "mscc,vsc7514_mfd_spi" },
+	{ .compatible = "mscc,vsc7513_mfd_spi" },
+	{ .compatible = "mscc,vsc7512_mfd_spi" },
+	{ .compatible = "mscc,vsc7511_mfd_spi" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ocelot_mfd_of_match);
+
+static struct spi_driver ocelot_mfd_spi_driver = {
+	.driver = {
+		.name = "ocelot_mfd_spi",
+		.of_match_table = of_match_ptr(ocelot_mfd_of_match),
+	},
+	.probe = ocelot_spi_probe,
+	.remove = ocelot_spi_remove,
+};
+module_spi_driver(ocelot_mfd_spi_driver);
+
+MODULE_DESCRIPTION("Ocelot Chip MFD SPI driver");
+MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
+MODULE_LICENSE("Dual MIT/GPL");
-- 
2.25.1

[RFC v5 net-next 02/13] mfd: ocelot: offer an interface for MFD children to get regmaps

[Colin Foster]

Child devices need to get a regmap from a resource struct, specifically
from the MFD parent. The MFD parent has the interface to the hardware
layer, which could be I2C, SPI, PCIe, etc.

This is somewhat a hack... ideally child devices would interface with the
struct device* directly, by way of a function like
devm_get_regmap_from_resource which would be akin to
devm_get_and_ioremap_resource. A less ideal option would be to interface
directly with MFD to get a regmap from the parent.

This solution is even less ideal than both of the two suggestions, so is
intentionally left in a separate commit after the initial MFD addition.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/ocelot-core.c |  9 +++++++++
 include/soc/mscc/ocelot.h | 12 ++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index a65619a8190b..09132ea52760 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -94,6 +94,15 @@ static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
 	return regmap;
 }
 
+struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
+						   const struct resource *res)
+{
+	struct ocelot_mfd_core *core = dev_get_drvdata(dev);
+
+	return ocelot_mfd_regmap_init(core, res);
+}
+EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
+
 int ocelot_mfd_init(struct ocelot_mfd_config *config)
 {
 	struct device *dev = config->dev;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 3e9454b00562..a641c9cc6f3f 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -968,4 +968,16 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_MFD_OCELOT_CORE)
+struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
+						   const struct resource *res);
+#else
+static inline regmap *
+ocelot_mfd_get_regmap_from_resource(struct device *dev,
+				    const struct resource *res)
+{
+	return NULL;
+}
+#endif
+
 #endif
-- 
2.25.1

[RFC v5 net-next 03/13] net: mscc: ocelot: expose ocelot wm functions

[Colin Foster]

Expose ocelot_wm functions so they can be shared with other drivers.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_devlink.c | 31 ++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 28 -------------------
 include/soc/mscc/ocelot.h                  |  5 ++++
 3 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_devlink.c b/drivers/net/ethernet/mscc/ocelot_devlink.c
index b8737efd2a85..d9ea75a14f2f 100644
--- a/drivers/net/ethernet/mscc/ocelot_devlink.c
+++ b/drivers/net/ethernet/mscc/ocelot_devlink.c
@@ -487,6 +487,37 @@ static void ocelot_watermark_init(struct ocelot *ocelot)
 	ocelot_setup_sharing_watermarks(ocelot);
 }
 
+/* Watermark encode
+ * Bit 8:   Unit; 0:1, 1:16
+ * Bit 7-0: Value to be multiplied with unit
+ */
+u16 ocelot_wm_enc(u16 value)
+{
+	WARN_ON(value >= 16 * BIT(8));
+
+	if (value >= BIT(8))
+		return BIT(8) | (value / 16);
+
+	return value;
+}
+EXPORT_SYMBOL(ocelot_wm_enc);
+
+u16 ocelot_wm_dec(u16 wm)
+{
+	if (wm & BIT(8))
+		return (wm & GENMASK(7, 0)) * 16;
+
+	return wm;
+}
+EXPORT_SYMBOL(ocelot_wm_dec);
+
+void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse)
+{
+	*inuse = (val & GENMASK(23, 12)) >> 12;
+	*maxuse = val & GENMASK(11, 0);
+}
+EXPORT_SYMBOL(ocelot_wm_stat);
+
 /* Pool size and type are fixed up at runtime. Keeping this structure to
  * look up the cell size multipliers.
  */
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 4f4a495a60ad..5e526545ef67 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -307,34 +307,6 @@ static int ocelot_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-/* Watermark encode
- * Bit 8:   Unit; 0:1, 1:16
- * Bit 7-0: Value to be multiplied with unit
- */
-static u16 ocelot_wm_enc(u16 value)
-{
-	WARN_ON(value >= 16 * BIT(8));
-
-	if (value >= BIT(8))
-		return BIT(8) | (value / 16);
-
-	return value;
-}
-
-static u16 ocelot_wm_dec(u16 wm)
-{
-	if (wm & BIT(8))
-		return (wm & GENMASK(7, 0)) * 16;
-
-	return wm;
-}
-
-static void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse)
-{
-	*inuse = (val & GENMASK(23, 12)) >> 12;
-	*maxuse = val & GENMASK(11, 0);
-}
-
 static const struct ocelot_ops ocelot_ops = {
 	.reset			= ocelot_reset,
 	.wm_enc			= ocelot_wm_enc,
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index a641c9cc6f3f..37bc9d647bc2 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -812,6 +812,11 @@ void ocelot_deinit(struct ocelot *ocelot);
 void ocelot_init_port(struct ocelot *ocelot, int port);
 void ocelot_deinit_port(struct ocelot *ocelot, int port);
 
+/* Watermark interface */
+u16 ocelot_wm_enc(u16 value);
+u16 ocelot_wm_dec(u16 wm);
+void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse);
+
 /* DSA callbacks */
 void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data);
 void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data);
-- 
2.25.1

[RFC v5 net-next 04/13] net: dsa: felix: add configurable device quirks

[Colin Foster]

The define FELIX_MAC_QUIRKS was used directly in the felix.c shared driver.
Other devices (VSC7512 for example) don't require the same quirks, so they
need to be configured on a per-device basis.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/felix.c           | 7 +++++--
 drivers/net/dsa/ocelot/felix.h           | 1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 1 +
 drivers/net/dsa/ocelot/seville_vsc9953.c | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f4fc403fbc1e..757ae35f3d56 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -840,9 +840,12 @@ static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
 					phy_interface_t interface)
 {
 	struct ocelot *ocelot = ds->priv;
+	struct felix *felix;
+
+	felix = ocelot_to_felix(ocelot);
 
 	ocelot_phylink_mac_link_down(ocelot, port, link_an_mode, interface,
-				     FELIX_MAC_QUIRKS);
+				     felix->info->quirks);
 }
 
 static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
@@ -857,7 +860,7 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 
 	ocelot_phylink_mac_link_up(ocelot, port, phydev, link_an_mode,
 				   interface, speed, duplex, tx_pause, rx_pause,
-				   FELIX_MAC_QUIRKS);
+				   felix->info->quirks);
 
 	if (felix->info->port_sched_speed_set)
 		felix->info->port_sched_speed_set(ocelot, port, speed);
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 515bddc012c0..69c97f35a607 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -26,6 +26,7 @@ struct felix_info {
 	u16				vcap_pol_base2;
 	u16				vcap_pol_max2;
 	const struct ptp_clock_info	*ptp_caps;
+	u32				quirks;
 
 	/* Some Ocelot switches are integrated into the SoC without the
 	 * extraction IRQ line connected to the ARM GIC. By enabling this
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 110d6c403bdd..c6ee393fd35c 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2229,6 +2229,7 @@ static const struct felix_info felix_info_vsc9959 = {
 	.num_mact_rows		= 2048,
 	.num_ports		= 6,
 	.num_tx_queues		= OCELOT_NUM_TC,
+	.quirks			= FELIX_MAC_QUIRKS,
 	.quirk_no_xtr_irq	= true,
 	.ptp_caps		= &vsc9959_ptp_caps,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index e110550e3507..a7db8781310b 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1097,6 +1097,7 @@ static const struct felix_info seville_info_vsc9953 = {
 	.vcap_pol_max		= VSC9953_VCAP_POLICER_MAX,
 	.vcap_pol_base2		= VSC9953_VCAP_POLICER_BASE2,
 	.vcap_pol_max2		= VSC9953_VCAP_POLICER_MAX2,
+	.quirks			= FELIX_MAC_QUIRKS,
 	.num_mact_rows		= 2048,
 	.num_ports		= 10,
 	.num_tx_queues		= OCELOT_NUM_TC,
-- 
2.25.1

[RFC v5 net-next 05/13] net: mdio: mscc-miim: add ability to externally register phy reset control

[Colin Foster]

The ocelot-ext driver requires the phys to be externally controlled by an
optional parameter. This commit exposes that variable so it can be
utilized.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/seville_vsc9953.c |  3 ++-
 drivers/net/mdio/mdio-mscc-miim.c        | 10 ++++++----
 include/linux/mdio/mdio-mscc-miim.h      |  3 ++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index a7db8781310b..95619705e486 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1021,7 +1021,8 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 
 	rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus",
 			     ocelot->targets[GCB],
-			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK]);
+			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
+			     NULL, 0);
 
 	if (rc) {
 		dev_err(dev, "failed to setup MDIO bus\n");
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 7d2abaf2b2c9..e16ab2ffacf6 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -188,7 +188,8 @@ static const struct regmap_config mscc_miim_regmap_config = {
 };
 
 int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
-		    struct regmap *mii_regmap, int status_offset)
+		    struct regmap *mii_regmap, int status_offset,
+		    struct regmap *phy_regmap, int phy_offset)
 {
 	struct mscc_miim_dev *miim;
 	struct mii_bus *bus;
@@ -210,6 +211,8 @@ int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
 
 	miim->regs = mii_regmap;
 	miim->mii_status_offset = status_offset;
+	miim->phy_regs = phy_regmap;
+	miim->phy_reset_offset = phy_offset;
 
 	*pbus = bus;
 
@@ -257,15 +260,14 @@ static int mscc_miim_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = mscc_miim_setup(&pdev->dev, &bus, "mscc_miim", mii_regmap, 0);
+	ret = mscc_miim_setup(&pdev->dev, &bus, "mscc_miim", mii_regmap, 0,
+			      phy_regmap, 0);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
 		return ret;
 	}
 
 	miim = bus->priv;
-	miim->phy_regs = phy_regmap;
-	miim->phy_reset_offset = 0;
 
 	ret = of_mdiobus_register(bus, pdev->dev.of_node);
 	if (ret < 0) {
diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
index 5b4ed2c3cbb9..5a95e43f73f9 100644
--- a/include/linux/mdio/mdio-mscc-miim.h
+++ b/include/linux/mdio/mdio-mscc-miim.h
@@ -14,6 +14,7 @@
 
 int mscc_miim_setup(struct device *device, struct mii_bus **bus,
 		    const char *name, struct regmap *mii_regmap,
-		    int status_offset);
+		    int status_offset, struct regmap *phy_regmap,
+		    int phy_offset);
 
 #endif
-- 
2.25.1

[RFC v5 net-next 06/13] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

Add control of an external VSC7512 chip by way of the ocelot-mfd interface.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/Kconfig      |  15 +
 drivers/net/dsa/ocelot/Makefile     |   5 +
 drivers/net/dsa/ocelot/ocelot_ext.c | 644 ++++++++++++++++++++++++++++
 include/soc/mscc/ocelot.h           |   2 +
 4 files changed, 666 insertions(+)
 create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 220b0b027b55..60a930f48014 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -1,4 +1,19 @@
 # SPDX-License-Identifier: GPL-2.0-only
+config NET_DSA_MSCC_OCELOT_EXT
+	tristate "Ocelot External Ethernet switch support"
+	depends on NET_DSA && SPI
+	depends on NET_VENDOR_MICROSEMI
+	select MDIO_MSCC_MIIM
+	select MFD_OCELOT_CORE
+	select MFD_OCELOT_SPI
+	select MSCC_OCELOT_SWITCH_LIB
+	select NET_DSA_TAG_OCELOT_8021Q
+	select NET_DSA_TAG_OCELOT
+	help
+	  This driver supports the VSC7511, VSC7512, VSC7513 and VSC7514 chips
+	  when controlled through SPI. It can be used with the Microsemi dev
+	  boards and an external CPU or custom hardware.
+
 config NET_DSA_MSCC_FELIX
 	tristate "Ocelot / Felix Ethernet switch support"
 	depends on NET_DSA && PCI
diff --git a/drivers/net/dsa/ocelot/Makefile b/drivers/net/dsa/ocelot/Makefile
index f6dd131e7491..d7f3f5a4461c 100644
--- a/drivers/net/dsa/ocelot/Makefile
+++ b/drivers/net/dsa/ocelot/Makefile
@@ -1,11 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_MSCC_FELIX) += mscc_felix.o
+obj-$(CONFIG_NET_DSA_MSCC_OCELOT_EXT) += mscc_ocelot_ext.o
 obj-$(CONFIG_NET_DSA_MSCC_SEVILLE) += mscc_seville.o
 
 mscc_felix-objs := \
 	felix.o \
 	felix_vsc9959.o
 
+mscc_ocelot_ext-objs := \
+	felix.o \
+	ocelot_ext.o
+
 mscc_seville-objs := \
 	felix.o \
 	seville_vsc9953.o
diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
new file mode 100644
index 000000000000..e35f21ba5dc3
--- /dev/null
+++ b/drivers/net/dsa/ocelot/ocelot_ext.c
@@ -0,0 +1,644 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021 Innovative Advantage Inc.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/iopoll.h>
+#include <linux/kconfig.h>
+#include <linux/mdio/mdio-mscc-miim.h>
+#include <linux/of_mdio.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <soc/mscc/ocelot_ana.h>
+#include <soc/mscc/ocelot_dev.h>
+#include <soc/mscc/ocelot_qsys.h>
+#include <soc/mscc/ocelot_vcap.h>
+#include <soc/mscc/ocelot_ptp.h>
+#include <soc/mscc/ocelot_sys.h>
+#include <soc/mscc/ocelot.h>
+#include <soc/mscc/vsc7514_regs.h>
+#include "felix.h"
+
+struct ocelot_ext_data {
+	struct felix felix;
+};
+
+static const u32 vsc7512_gcb_regmap[] = {
+	REG(GCB_SOFT_RST,			0x0008),
+	REG(GCB_MIIM_MII_STATUS,		0x009c),
+	REG(GCB_PHY_PHY_CFG,			0x00f0),
+	REG(GCB_PHY_PHY_STAT,			0x00f4),
+};
+
+static const u32 *vsc7512_regmap[TARGET_MAX] = {
+	[ANA] = vsc7514_ana_regmap,
+	[QS] = vsc7514_qs_regmap,
+	[QSYS] = vsc7514_qsys_regmap,
+	[REW] = vsc7514_rew_regmap,
+	[SYS] = vsc7514_sys_regmap,
+	[S0] = vsc7514_vcap_regmap,
+	[S1] = vsc7514_vcap_regmap,
+	[S2] = vsc7514_vcap_regmap,
+	[PTP] = vsc7514_ptp_regmap,
+	[GCB] = vsc7512_gcb_regmap,
+	[DEV_GMII] = vsc7514_dev_gmii_regmap,
+};
+
+#define VSC7512_BYTE_ORDER_LE 0x00000000
+#define VSC7512_BYTE_ORDER_BE 0x81818181
+#define VSC7512_BIT_ORDER_MSB 0x00000000
+#define VSC7512_BIT_ORDER_LSB 0x42424242
+
+static void ocelot_ext_reset_phys(struct ocelot *ocelot)
+{
+	ocelot_write(ocelot, 0, GCB_PHY_PHY_CFG);
+	ocelot_write(ocelot, 0x1ff, GCB_PHY_PHY_CFG);
+	mdelay(500);
+}
+
+static int ocelot_ext_reset(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct device *dev = ocelot->dev;
+	struct device_node *mdio_node;
+	int retries = 100;
+	int err, val;
+
+	ocelot_ext_reset_phys(ocelot);
+
+	mdio_node = of_get_child_by_name(dev->of_node, "mdio");
+	if (!mdio_node)
+		dev_info(ocelot->dev,
+			 "mdio children not found in device tree\n");
+
+	err = of_mdiobus_register(felix->imdio, mdio_node);
+	if (err) {
+		dev_err(ocelot->dev, "error registering MDIO bus\n");
+		return err;
+	}
+
+	felix->ds->slave_mii_bus = felix->imdio;
+
+	/* We might need to reset the switch core here, if that is possible */
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
+	if (err)
+		return err;
+
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+	if (err)
+		return err;
+
+	do {
+		msleep(1);
+		regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],
+				  &val);
+	} while (val && --retries);
+
+	if (!retries)
+		return -ETIMEDOUT;
+
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
+
+	return err;
+}
+
+static u32 ocelot_offset_from_reg_base(struct ocelot *ocelot, u32 target,
+				       u32 reg)
+{
+	return ocelot->map[target][reg & REG_MASK];
+}
+
+static const struct ocelot_ops vsc7512_ops = {
+	.reset		= ocelot_ext_reset,
+	.wm_enc		= ocelot_wm_enc,
+	.wm_dec		= ocelot_wm_dec,
+	.wm_stat	= ocelot_wm_stat,
+	.port_to_netdev	= felix_port_to_netdev,
+	.netdev_to_port	= felix_netdev_to_port,
+};
+
+static const struct resource vsc7512_target_io_res[TARGET_MAX] = {
+	[ANA] = {
+		.start	= 0x71880000,
+		.end	= 0x7188ffff,
+		.name	= "ana",
+	},
+	[QS] = {
+		.start	= 0x71080000,
+		.end	= 0x710800ff,
+		.name	= "qs",
+	},
+	[QSYS] = {
+		.start	= 0x71800000,
+		.end	= 0x719fffff,
+		.name	= "qsys",
+	},
+	[REW] = {
+		.start	= 0x71030000,
+		.end	= 0x7103ffff,
+		.name	= "rew",
+	},
+	[SYS] = {
+		.start	= 0x71010000,
+		.end	= 0x7101ffff,
+		.name	= "sys",
+	},
+	[S0] = {
+		.start	= 0x71040000,
+		.end	= 0x710403ff,
+		.name	= "s0",
+	},
+	[S1] = {
+		.start	= 0x71050000,
+		.end	= 0x710503ff,
+		.name	= "s1",
+	},
+	[S2] = {
+		.start	= 0x71060000,
+		.end	= 0x710603ff,
+		.name	= "s2",
+	},
+	[GCB] =	{
+		.start	= 0x71070000,
+		.end	= 0x7107022b,
+		.name	= "devcpu_gcb",
+	},
+};
+
+static const struct resource vsc7512_port_io_res[] = {
+	{
+		.start	= 0x711e0000,
+		.end	= 0x711effff,
+		.name	= "port0",
+	},
+	{
+		.start	= 0x711f0000,
+		.end	= 0x711fffff,
+		.name	= "port1",
+	},
+	{
+		.start	= 0x71200000,
+		.end	= 0x7120ffff,
+		.name	= "port2",
+	},
+	{
+		.start	= 0x71210000,
+		.end	= 0x7121ffff,
+		.name	= "port3",
+	},
+	{
+		.start	= 0x71220000,
+		.end	= 0x7122ffff,
+		.name	= "port4",
+	},
+	{
+		.start	= 0x71230000,
+		.end	= 0x7123ffff,
+		.name	= "port5",
+	},
+	{
+		.start	= 0x71240000,
+		.end	= 0x7124ffff,
+		.name	= "port6",
+	},
+	{
+		.start	= 0x71250000,
+		.end	= 0x7125ffff,
+		.name	= "port7",
+	},
+	{
+		.start	= 0x71260000,
+		.end	= 0x7126ffff,
+		.name	= "port8",
+	},
+	{
+		.start	= 0x71270000,
+		.end	= 0x7127ffff,
+		.name	= "port9",
+	},
+	{
+		.start	= 0x71280000,
+		.end	= 0x7128ffff,
+		.name	= "port10",
+	},
+};
+
+static const struct reg_field vsc7512_regfields[REGFIELD_MAX] = {
+	[ANA_ADVLEARN_VLAN_CHK] = REG_FIELD(ANA_ADVLEARN, 11, 11),
+	[ANA_ADVLEARN_LEARN_MIRROR] = REG_FIELD(ANA_ADVLEARN, 0, 10),
+	[ANA_ANEVENTS_MSTI_DROP] = REG_FIELD(ANA_ANEVENTS, 27, 27),
+	[ANA_ANEVENTS_ACLKILL] = REG_FIELD(ANA_ANEVENTS, 26, 26),
+	[ANA_ANEVENTS_ACLUSED] = REG_FIELD(ANA_ANEVENTS, 25, 25),
+	[ANA_ANEVENTS_AUTOAGE] = REG_FIELD(ANA_ANEVENTS, 24, 24),
+	[ANA_ANEVENTS_VS2TTL1] = REG_FIELD(ANA_ANEVENTS, 23, 23),
+	[ANA_ANEVENTS_STORM_DROP] = REG_FIELD(ANA_ANEVENTS, 22, 22),
+	[ANA_ANEVENTS_LEARN_DROP] = REG_FIELD(ANA_ANEVENTS, 21, 21),
+	[ANA_ANEVENTS_AGED_ENTRY] = REG_FIELD(ANA_ANEVENTS, 20, 20),
+	[ANA_ANEVENTS_CPU_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 19, 19),
+	[ANA_ANEVENTS_AUTO_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 18, 18),
+	[ANA_ANEVENTS_LEARN_REMOVE] = REG_FIELD(ANA_ANEVENTS, 17, 17),
+	[ANA_ANEVENTS_AUTO_LEARNED] = REG_FIELD(ANA_ANEVENTS, 16, 16),
+	[ANA_ANEVENTS_AUTO_MOVED] = REG_FIELD(ANA_ANEVENTS, 15, 15),
+	[ANA_ANEVENTS_DROPPED] = REG_FIELD(ANA_ANEVENTS, 14, 14),
+	[ANA_ANEVENTS_CLASSIFIED_DROP] = REG_FIELD(ANA_ANEVENTS, 13, 13),
+	[ANA_ANEVENTS_CLASSIFIED_COPY] = REG_FIELD(ANA_ANEVENTS, 12, 12),
+	[ANA_ANEVENTS_VLAN_DISCARD] = REG_FIELD(ANA_ANEVENTS, 11, 11),
+	[ANA_ANEVENTS_FWD_DISCARD] = REG_FIELD(ANA_ANEVENTS, 10, 10),
+	[ANA_ANEVENTS_MULTICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 9, 9),
+	[ANA_ANEVENTS_UNICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 8, 8),
+	[ANA_ANEVENTS_DEST_KNOWN] = REG_FIELD(ANA_ANEVENTS, 7, 7),
+	[ANA_ANEVENTS_BUCKET3_MATCH] = REG_FIELD(ANA_ANEVENTS, 6, 6),
+	[ANA_ANEVENTS_BUCKET2_MATCH] = REG_FIELD(ANA_ANEVENTS, 5, 5),
+	[ANA_ANEVENTS_BUCKET1_MATCH] = REG_FIELD(ANA_ANEVENTS, 4, 4),
+	[ANA_ANEVENTS_BUCKET0_MATCH] = REG_FIELD(ANA_ANEVENTS, 3, 3),
+	[ANA_ANEVENTS_CPU_OPERATION] = REG_FIELD(ANA_ANEVENTS, 2, 2),
+	[ANA_ANEVENTS_DMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 1, 1),
+	[ANA_ANEVENTS_SMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 0, 0),
+	[ANA_TABLES_MACACCESS_B_DOM] = REG_FIELD(ANA_TABLES_MACACCESS, 18, 18),
+	[ANA_TABLES_MACTINDX_BUCKET] = REG_FIELD(ANA_TABLES_MACTINDX, 10, 11),
+	[ANA_TABLES_MACTINDX_M_INDEX] = REG_FIELD(ANA_TABLES_MACTINDX, 0, 9),
+	[GCB_SOFT_RST_SWC_RST] = REG_FIELD(GCB_SOFT_RST, 1, 1),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_VLD] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 20, 20),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_FP] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 8, 19),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_PORTNO] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 4, 7),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_SEL] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 1, 3),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_T] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 0, 0),
+	[SYS_RESET_CFG_CORE_ENA] = REG_FIELD(SYS_RESET_CFG, 2, 2),
+	[SYS_RESET_CFG_MEM_ENA] = REG_FIELD(SYS_RESET_CFG, 1, 1),
+	[SYS_RESET_CFG_MEM_INIT] = REG_FIELD(SYS_RESET_CFG, 0, 0),
+	/* Replicated per number of ports (12), register size 4 per port */
+	[QSYS_SWITCH_PORT_MODE_PORT_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 14, 14, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_SCH_NEXT_CFG] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 11, 13, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_YEL_RSRVD] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 10, 10, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_INGRESS_DROP_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 9, 9, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_TX_PFC_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 1, 8, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_TX_PFC_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 0, 0, 12, 4),
+	[SYS_PORT_MODE_DATA_WO_TS] = REG_FIELD_ID(SYS_PORT_MODE, 5, 6, 12, 4),
+	[SYS_PORT_MODE_INCL_INJ_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 3, 4, 12, 4),
+	[SYS_PORT_MODE_INCL_XTR_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 1, 2, 12, 4),
+	[SYS_PORT_MODE_INCL_HDR_ERR] = REG_FIELD_ID(SYS_PORT_MODE, 0, 0, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_START] = REG_FIELD_ID(SYS_PAUSE_CFG, 10, 18, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_STOP] = REG_FIELD_ID(SYS_PAUSE_CFG, 1, 9, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_ENA] = REG_FIELD_ID(SYS_PAUSE_CFG, 0, 1, 12, 4),
+};
+
+static const struct ocelot_stat_layout vsc7512_stats_layout[] = {
+	{ .offset = 0x00,	.name = "rx_octets", },
+	{ .offset = 0x01,	.name = "rx_unicast", },
+	{ .offset = 0x02,	.name = "rx_multicast", },
+	{ .offset = 0x03,	.name = "rx_broadcast", },
+	{ .offset = 0x04,	.name = "rx_shorts", },
+	{ .offset = 0x05,	.name = "rx_fragments", },
+	{ .offset = 0x06,	.name = "rx_jabbers", },
+	{ .offset = 0x07,	.name = "rx_crc_align_errs", },
+	{ .offset = 0x08,	.name = "rx_sym_errs", },
+	{ .offset = 0x09,	.name = "rx_frames_below_65_octets", },
+	{ .offset = 0x0A,	.name = "rx_frames_65_to_127_octets", },
+	{ .offset = 0x0B,	.name = "rx_frames_128_to_255_octets", },
+	{ .offset = 0x0C,	.name = "rx_frames_256_to_511_octets", },
+	{ .offset = 0x0D,	.name = "rx_frames_512_to_1023_octets", },
+	{ .offset = 0x0E,	.name = "rx_frames_1024_to_1526_octets", },
+	{ .offset = 0x0F,	.name = "rx_frames_over_1526_octets", },
+	{ .offset = 0x10,	.name = "rx_pause", },
+	{ .offset = 0x11,	.name = "rx_control", },
+	{ .offset = 0x12,	.name = "rx_longs", },
+	{ .offset = 0x13,	.name = "rx_classified_drops", },
+	{ .offset = 0x14,	.name = "rx_red_prio_0", },
+	{ .offset = 0x15,	.name = "rx_red_prio_1", },
+	{ .offset = 0x16,	.name = "rx_red_prio_2", },
+	{ .offset = 0x17,	.name = "rx_red_prio_3", },
+	{ .offset = 0x18,	.name = "rx_red_prio_4", },
+	{ .offset = 0x19,	.name = "rx_red_prio_5", },
+	{ .offset = 0x1A,	.name = "rx_red_prio_6", },
+	{ .offset = 0x1B,	.name = "rx_red_prio_7", },
+	{ .offset = 0x1C,	.name = "rx_yellow_prio_0", },
+	{ .offset = 0x1D,	.name = "rx_yellow_prio_1", },
+	{ .offset = 0x1E,	.name = "rx_yellow_prio_2", },
+	{ .offset = 0x1F,	.name = "rx_yellow_prio_3", },
+	{ .offset = 0x20,	.name = "rx_yellow_prio_4", },
+	{ .offset = 0x21,	.name = "rx_yellow_prio_5", },
+	{ .offset = 0x22,	.name = "rx_yellow_prio_6", },
+	{ .offset = 0x23,	.name = "rx_yellow_prio_7", },
+	{ .offset = 0x24,	.name = "rx_green_prio_0", },
+	{ .offset = 0x25,	.name = "rx_green_prio_1", },
+	{ .offset = 0x26,	.name = "rx_green_prio_2", },
+	{ .offset = 0x27,	.name = "rx_green_prio_3", },
+	{ .offset = 0x28,	.name = "rx_green_prio_4", },
+	{ .offset = 0x29,	.name = "rx_green_prio_5", },
+	{ .offset = 0x2A,	.name = "rx_green_prio_6", },
+	{ .offset = 0x2B,	.name = "rx_green_prio_7", },
+	{ .offset = 0x40,	.name = "tx_octets", },
+	{ .offset = 0x41,	.name = "tx_unicast", },
+	{ .offset = 0x42,	.name = "tx_multicast", },
+	{ .offset = 0x43,	.name = "tx_broadcast", },
+	{ .offset = 0x44,	.name = "tx_collision", },
+	{ .offset = 0x45,	.name = "tx_drops", },
+	{ .offset = 0x46,	.name = "tx_pause", },
+	{ .offset = 0x47,	.name = "tx_frames_below_65_octets", },
+	{ .offset = 0x48,	.name = "tx_frames_65_to_127_octets", },
+	{ .offset = 0x49,	.name = "tx_frames_128_255_octets", },
+	{ .offset = 0x4A,	.name = "tx_frames_256_511_octets", },
+	{ .offset = 0x4B,	.name = "tx_frames_512_1023_octets", },
+	{ .offset = 0x4C,	.name = "tx_frames_1024_1526_octets", },
+	{ .offset = 0x4D,	.name = "tx_frames_over_1526_octets", },
+	{ .offset = 0x4E,	.name = "tx_yellow_prio_0", },
+	{ .offset = 0x4F,	.name = "tx_yellow_prio_1", },
+	{ .offset = 0x50,	.name = "tx_yellow_prio_2", },
+	{ .offset = 0x51,	.name = "tx_yellow_prio_3", },
+	{ .offset = 0x52,	.name = "tx_yellow_prio_4", },
+	{ .offset = 0x53,	.name = "tx_yellow_prio_5", },
+	{ .offset = 0x54,	.name = "tx_yellow_prio_6", },
+	{ .offset = 0x55,	.name = "tx_yellow_prio_7", },
+	{ .offset = 0x56,	.name = "tx_green_prio_0", },
+	{ .offset = 0x57,	.name = "tx_green_prio_1", },
+	{ .offset = 0x58,	.name = "tx_green_prio_2", },
+	{ .offset = 0x59,	.name = "tx_green_prio_3", },
+	{ .offset = 0x5A,	.name = "tx_green_prio_4", },
+	{ .offset = 0x5B,	.name = "tx_green_prio_5", },
+	{ .offset = 0x5C,	.name = "tx_green_prio_6", },
+	{ .offset = 0x5D,	.name = "tx_green_prio_7", },
+	{ .offset = 0x5E,	.name = "tx_aged", },
+	{ .offset = 0x80,	.name = "drop_local", },
+	{ .offset = 0x81,	.name = "drop_tail", },
+	{ .offset = 0x82,	.name = "drop_yellow_prio_0", },
+	{ .offset = 0x83,	.name = "drop_yellow_prio_1", },
+	{ .offset = 0x84,	.name = "drop_yellow_prio_2", },
+	{ .offset = 0x85,	.name = "drop_yellow_prio_3", },
+	{ .offset = 0x86,	.name = "drop_yellow_prio_4", },
+	{ .offset = 0x87,	.name = "drop_yellow_prio_5", },
+	{ .offset = 0x88,	.name = "drop_yellow_prio_6", },
+	{ .offset = 0x89,	.name = "drop_yellow_prio_7", },
+	{ .offset = 0x8A,	.name = "drop_green_prio_0", },
+	{ .offset = 0x8B,	.name = "drop_green_prio_1", },
+	{ .offset = 0x8C,	.name = "drop_green_prio_2", },
+	{ .offset = 0x8D,	.name = "drop_green_prio_3", },
+	{ .offset = 0x8E,	.name = "drop_green_prio_4", },
+	{ .offset = 0x8F,	.name = "drop_green_prio_5", },
+	{ .offset = 0x90,	.name = "drop_green_prio_6", },
+	{ .offset = 0x91,	.name = "drop_green_prio_7", },
+};
+
+static void vsc7512_phylink_validate(struct ocelot *ocelot, int port,
+				     unsigned long *supported,
+				     struct phylink_link_state *state)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	if (state->interface != PHY_INTERFACE_MODE_NA &&
+	    state->interface != ocelot_port->phy_mode) {
+		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+		return;
+	}
+
+	phylink_set_port_modes(mask);
+
+	phylink_set(mask, Pause);
+	phylink_set(mask, Autoneg);
+	phylink_set(mask, Asym_Pause);
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+	phylink_set(mask, 1000baseT_Half);
+	phylink_set(mask, 1000baseT_Full);
+
+	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
+	bitmap_and(state->advertising, state->advertising, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+}
+
+static int vsc7512_prevalidate_phy_mode(struct ocelot *ocelot, int port,
+					phy_interface_t phy_mode)
+{
+	switch (phy_mode) {
+	case PHY_INTERFACE_MODE_INTERNAL:
+		if (port < 4)
+			return 0;
+		return -EOPNOTSUPP;
+	case PHY_INTERFACE_MODE_SGMII:
+		if (port < 8)
+			return 0;
+		return -EOPNOTSUPP;
+	case PHY_INTERFACE_MODE_QSGMII:
+		if (port == 7 || port == 8 || port == 10)
+			return 0;
+		return -EOPNOTSUPP;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int vsc7512_port_setup_tc(struct dsa_switch *ds, int port,
+				 enum tc_setup_type type, void *type_data)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct vcap_props vsc7512_vcap_props[] = {
+	[VCAP_ES0] = {
+		.action_type_width = 0,
+		.action_table = {
+			[ES0_ACTION_TYPE_NORMAL] = {
+				.width = 73,
+				.count = 1,
+			},
+		},
+		.target = S0,
+		.keys = vsc7514_vcap_es0_keys,
+		.actions = vsc7514_vcap_es0_actions,
+	},
+	[VCAP_IS1] = {
+		.action_type_width = 0,
+		.action_table = {
+			[IS1_ACTION_TYPE_NORMAL] = {
+				.width = 78,
+				.count = 4,
+			},
+		},
+		.target = S1,
+		.keys = vsc7514_vcap_is1_keys,
+		.actions = vsc7514_vcap_is1_actions,
+	},
+	[VCAP_IS2] = {
+		.action_type_width = 1,
+		.action_table = {
+			[IS2_ACTION_TYPE_NORMAL] = {
+				.width = 49,
+				.count = 2,
+			},
+			[IS2_ACTION_TYPE_SMAC_SIP] = {
+				.width = 6,
+				.count = 4,
+			},
+		},
+		.target = S2,
+		.keys = vsc7514_vcap_is2_keys,
+		.actions = vsc7514_vcap_is2_actions,
+	},
+};
+
+static struct regmap *vsc7512_regmap_init(struct ocelot *ocelot,
+					  struct resource *res)
+{
+	struct device *dev = ocelot->dev;
+	struct regmap *regmap;
+
+	regmap = ocelot_mfd_get_regmap_from_resource(dev->parent, res);
+	if (IS_ERR(regmap))
+		return ERR_CAST(regmap);
+
+	return regmap;
+}
+
+static int vsc7512_mdio_bus_alloc(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct device *dev = ocelot->dev;
+	u32 mii_offset, phy_offset;
+	struct mii_bus *bus;
+	int err;
+
+	mii_offset = ocelot_offset_from_reg_base(ocelot, GCB,
+						 GCB_MIIM_MII_STATUS);
+
+	phy_offset = ocelot_offset_from_reg_base(ocelot, GCB, GCB_PHY_PHY_CFG);
+
+	err = mscc_miim_setup(dev, &bus, "ocelot_ext MDIO bus",
+			       ocelot->targets[GCB], mii_offset,
+			       ocelot->targets[GCB], phy_offset);
+	if (err) {
+		dev_err(dev, "failed to setup MDIO bus\n");
+		return err;
+	}
+
+	felix->imdio = bus;
+
+	return err;
+}
+
+
+static void vsc7512_mdio_bus_free(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	if (felix->imdio)
+		mdiobus_unregister(felix->imdio);
+}
+
+static const struct felix_info ocelot_ext_info = {
+	.target_io_res			= vsc7512_target_io_res,
+	.port_io_res			= vsc7512_port_io_res,
+	.regfields			= vsc7512_regfields,
+	.map				= vsc7512_regmap,
+	.ops				= &vsc7512_ops,
+	.stats_layout			= vsc7512_stats_layout,
+	.num_stats			= ARRAY_SIZE(vsc7512_stats_layout),
+	.vcap				= vsc7512_vcap_props,
+	.num_mact_rows			= 1024,
+	.num_ports			= 11,
+	.num_tx_queues			= OCELOT_NUM_TC,
+	.mdio_bus_alloc			= vsc7512_mdio_bus_alloc,
+	.mdio_bus_free			= vsc7512_mdio_bus_free,
+	.phylink_validate		= vsc7512_phylink_validate,
+	.prevalidate_phy_mode		= vsc7512_prevalidate_phy_mode,
+	.port_setup_tc			= vsc7512_port_setup_tc,
+	.init_regmap			= vsc7512_regmap_init,
+};
+
+static int ocelot_ext_probe(struct platform_device *pdev)
+{
+	struct ocelot_ext_data *ocelot_ext;
+	struct dsa_switch *ds;
+	struct ocelot *ocelot;
+	struct felix *felix;
+	struct device *dev;
+	int err;
+
+	dev = &pdev->dev;
+
+	ocelot_ext = devm_kzalloc(dev, sizeof(struct ocelot_ext_data),
+				  GFP_KERNEL);
+
+	if (!ocelot_ext)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ocelot_ext);
+
+	felix = &ocelot_ext->felix;
+
+	ocelot = &felix->ocelot;
+	ocelot->dev = dev;
+
+	ocelot->num_flooding_pgids = 1;
+
+	felix->info = &ocelot_ext_info;
+
+	ds = kzalloc(sizeof(*ds), GFP_KERNEL);
+	if (!ds) {
+		err = -ENOMEM;
+		dev_err(dev, "Failed to allocate DSA switch\n");
+		return err;
+	}
+
+	ds->dev = dev;
+	ds->num_ports = felix->info->num_ports;
+	ds->num_tx_queues = felix->info->num_tx_queues;
+
+	ds->ops = &felix_switch_ops;
+	ds->priv = ocelot;
+	felix->ds = ds;
+	felix->tag_proto = DSA_TAG_PROTO_OCELOT;
+
+	err = dsa_register_switch(ds);
+
+	if (err) {
+		dev_err(dev, "Failed to register DSA switch: %d\n", err);
+		goto err_register_ds;
+	}
+
+	return 0;
+
+err_register_ds:
+	kfree(ds);
+	return err;
+}
+
+static int ocelot_ext_remove(struct platform_device *pdev)
+{
+	struct ocelot_ext_data *ocelot_ext;
+	struct felix *felix;
+
+	ocelot_ext = dev_get_drvdata(&pdev->dev);
+	felix = &ocelot_ext->felix;
+
+	dsa_unregister_switch(felix->ds);
+
+	kfree(felix->ds);
+
+	devm_kfree(&pdev->dev, ocelot_ext);
+
+	return 0;
+}
+
+const struct of_device_id ocelot_ext_switch_of_match[] = {
+	{ .compatible = "mscc,vsc7512-ext-switch" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
+
+static struct platform_driver ocelot_ext_switch_driver = {
+	.driver = {
+		.name = "ocelot-ext-switch",
+		.of_match_table = of_match_ptr(ocelot_ext_switch_of_match),
+	},
+	.probe = ocelot_ext_probe,
+	.remove = ocelot_ext_remove,
+};
+module_platform_driver(ocelot_ext_switch_driver);
+
+MODULE_DESCRIPTION("External Ocelot Switch driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 37bc9d647bc2..c56eaedc5e58 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -399,6 +399,8 @@ enum ocelot_reg {
 	GCB_MIIM_MII_STATUS,
 	GCB_MIIM_MII_CMD,
 	GCB_MIIM_MII_DATA,
+	GCB_PHY_PHY_CFG,
+	GCB_PHY_PHY_STAT,
 	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
 	DEV_PORT_MISC,
 	DEV_EVENTS,
-- 
2.25.1

[RFC v5 net-next 07/13] mfd: ocelot: enable the external switch interface

[Colin Foster]

Add the ocelot-ext child device to the MFD. This will enable device-tree
configuration of the MFD to include the external switch, if desired.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/ocelot-core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 09132ea52760..52aa7b824d02 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -6,6 +6,7 @@
 #include <asm/byteorder.h>
 #include <linux/spi/spi.h>
 #include <linux/kconfig.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -103,6 +104,13 @@ struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
 }
 EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
 
+static const struct mfd_cell vsc7512_devs[] = {
+	{
+		.name = "ocelot-ext-switch",
+		.of_compatible = "mscc,vsc7512-ext-switch",
+	},
+};
+
 int ocelot_mfd_init(struct ocelot_mfd_config *config)
 {
 	struct device *dev = config->dev;
@@ -139,7 +147,10 @@ int ocelot_mfd_init(struct ocelot_mfd_config *config)
 		return ret;
 	}
 
-	/* Create and loop over all child devices here */
+	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, vsc7512_devs,
+			      ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
+
+	dev_info(dev, "ocelot mfd core setup complete\n");
 
 	return 0;
 }
@@ -147,7 +158,7 @@ EXPORT_SYMBOL(ocelot_mfd_init);
 
 int ocelot_mfd_remove(struct ocelot_mfd_config *config)
 {
-	/* Loop over all children and remove them */
+	mfd_remove_devices(config->dev);
 
 	return 0;
 }
-- 
2.25.1

[RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Colin Foster]

Some drivers will need to create regmaps differently based on whether they
are a child of an MFD or a standalone device. An example of this would be
if a regmap were directly memory-mapped or an external bus. In the
memory-mapped case a call to devm_regmap_init_mmio would return the correct
regmap. In the case of an MFD, the regmap would need to be requested from
the parent device.

This addition allows the driver to correctly reason about these scenarios.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/mfd-core.c   |  5 +++++
 include/linux/mfd/core.h | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 684a011a6396..905f508a31b4 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
 	.name	= "mfd_device",
 };
 
+int device_is_mfd(struct platform_device *pdev)
+{
+	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
+}
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 0bc7cba798a3..c0719436b652 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -10,6 +10,7 @@
 #ifndef MFD_CORE_H
 #define MFD_CORE_H
 
+#include <generated/autoconf.h>
 #include <linux/platform_device.h>
 
 #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
@@ -123,6 +124,15 @@ struct mfd_cell {
 	int			num_parent_supplies;
 };
 
+#ifdef CONFIG_MFD_CORE
+int device_is_mfd(struct platform_device *pdev);
+#else
+static inline int device_is_mfd(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 /*
  * Convenience functions for clients using shared cells.  Refcounting
  * happens automatically, with the cell's enable/disable callbacks
-- 
2.25.1

[RFC v5 net-next 09/13] net: mdio: mscc-miim: add local dev variable to cleanup probe function

[Colin Foster]

Create a local device *dev in order to not dereference the platform_device
several times throughout the probe function.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/mdio/mdio-mscc-miim.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index e16ab2ffacf6..00757e77fab0 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -223,6 +223,7 @@ EXPORT_SYMBOL(mscc_miim_setup);
 static int mscc_miim_probe(struct platform_device *pdev)
 {
 	struct regmap *mii_regmap, *phy_regmap = NULL;
+	struct device *dev = &pdev->dev;
 	void __iomem *regs, *phy_regs;
 	struct mscc_miim_dev *miim;
 	struct resource *res;
@@ -231,47 +232,46 @@ static int mscc_miim_probe(struct platform_device *pdev)
 
 	regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
 	if (IS_ERR(regs)) {
-		dev_err(&pdev->dev, "Unable to map MIIM registers\n");
+		dev_err(dev, "Unable to map MIIM registers\n");
 		return PTR_ERR(regs);
 	}
 
-	mii_regmap = devm_regmap_init_mmio(&pdev->dev, regs,
-					   &mscc_miim_regmap_config);
+	mii_regmap = devm_regmap_init_mmio(dev, regs, &mscc_miim_regmap_config);
 
 	if (IS_ERR(mii_regmap)) {
-		dev_err(&pdev->dev, "Unable to create MIIM regmap\n");
+		dev_err(dev, "Unable to create MIIM regmap\n");
 		return PTR_ERR(mii_regmap);
 	}
 
 	/* This resource is optional */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	if (res) {
-		phy_regs = devm_ioremap_resource(&pdev->dev, res);
+		phy_regs = devm_ioremap_resource(dev, res);
 		if (IS_ERR(phy_regs)) {
-			dev_err(&pdev->dev, "Unable to map internal phy registers\n");
+			dev_err(dev, "Unable to map internal phy registers\n");
 			return PTR_ERR(phy_regs);
 		}
 
-		phy_regmap = devm_regmap_init_mmio(&pdev->dev, phy_regs,
+		phy_regmap = devm_regmap_init_mmio(dev, phy_regs,
 						   &mscc_miim_regmap_config);
 		if (IS_ERR(phy_regmap)) {
-			dev_err(&pdev->dev, "Unable to create phy register regmap\n");
+			dev_err(dev, "Unable to create phy register regmap\n");
 			return PTR_ERR(phy_regmap);
 		}
 	}
 
-	ret = mscc_miim_setup(&pdev->dev, &bus, "mscc_miim", mii_regmap, 0,
-			      phy_regmap, 0);
+	ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_regmap, 0, phy_regmap,
+			      0);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
+		dev_err(dev, "Unable to setup the MDIO bus\n");
 		return ret;
 	}
 
 	miim = bus->priv;
 
-	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	ret = of_mdiobus_register(bus, dev->of_node);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
+		dev_err(dev, "Cannot register MDIO bus (%d)\n", ret);
 		return ret;
 	}
 
-- 
2.25.1

[RFC v5 net-next 10/13] net: mdio: mscc-miim: add MFD functionality through ocelot-core

[Colin Foster]

In the MFD case we need to request a regmap from the parent device instead
of using mmio. This allows for the driver to be used in either case.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/mdio/mdio-mscc-miim.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 00757e77fab0..d35cca4672b4 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -11,11 +11,13 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mdio/mdio-mscc-miim.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <soc/mscc/ocelot.h>
 
 #define MSCC_MIIM_REG_STATUS		0x0
 #define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
@@ -230,13 +232,21 @@ static int mscc_miim_probe(struct platform_device *pdev)
 	struct mii_bus *bus;
 	int ret;
 
-	regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
-	if (IS_ERR(regs)) {
-		dev_err(dev, "Unable to map MIIM registers\n");
-		return PTR_ERR(regs);
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!device_is_mfd(pdev)) {
+		regs = devm_ioremap_resource(dev, res);
+		if (IS_ERR(regs)) {
+			dev_err(dev, "Unable to map MIIM registers\n");
+			return PTR_ERR(regs);
+		}
 
-	mii_regmap = devm_regmap_init_mmio(dev, regs, &mscc_miim_regmap_config);
+		mii_regmap = devm_regmap_init_mmio(dev, regs,
+						   &mscc_miim_regmap_config);
+	} else {
+		mii_regmap = ocelot_mfd_get_regmap_from_resource(dev->parent,
+								 res);
+	}
 
 	if (IS_ERR(mii_regmap)) {
 		dev_err(dev, "Unable to create MIIM regmap\n");
-- 
2.25.1

[RFC v5 net-next 11/13] mfd: ocelot-core: add control for the external mdio interface

[Colin Foster]

Utilize the mscc-miim-mdio driver as a child of the ocelot MFD.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/ocelot-core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 52aa7b824d02..c67e433f467c 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -104,7 +104,22 @@ struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
 }
 EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
 
+static const struct resource vsc7512_miim1_resources[] = {
+	{
+		.start = 0x710700c0,
+		.end = 0x710700e3,
+		.name = "gcb_miim1",
+		.flags = IORESOURCE_MEM,
+	},
+};
+
 static const struct mfd_cell vsc7512_devs[] = {
+	{
+		.name = "ocelot-miim1",
+		.of_compatible = "mscc,ocelot-miim",
+		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
+		.resources = vsc7512_miim1_resources,
+	},
 	{
 		.name = "ocelot-ext-switch",
 		.of_compatible = "mscc,vsc7512-ext-switch",
-- 
2.25.1

[RFC v5 net-next 12/13] pinctrl: ocelot: add MFD functionality through ocelot-core

[Colin Foster]

In the MFD case we need to requrest a regmap from the patern device instead
of using mmio. This allows for the driver to be used in either case.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/pinctrl/pinctrl-ocelot.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 42aab9ba049a..d07ac7a0b487 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -10,6 +10,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/mfd/core.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
@@ -20,6 +21,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"
@@ -1123,6 +1125,9 @@ static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+#if defined(REG)
+#undef REG
+#endif
 #define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32)))
 
 static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev,
@@ -1805,6 +1810,7 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct ocelot_pinctrl *info;
 	struct regmap *pincfg;
+	struct resource *res;
 	void __iomem *base;
 	int ret;
 	struct regmap_config regmap_config = {
@@ -1819,16 +1825,28 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 
 	info->desc = (struct pinctrl_desc *)device_get_match_data(dev);
 
-	base = devm_ioremap_resource(dev,
-			platform_get_resource(pdev, IORESOURCE_MEM, 0));
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (IS_ERR(res)) {
+		dev_err(dev, "Failed to get resource\n");
+		return PTR_ERR(res);
+	}
 
 	info->stride = 1 + (info->desc->npins - 1) / 32;
 
-	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
+	if (!device_is_mfd(pdev)) {
+		base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		regmap_config.max_register =
+			OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
+
+		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	} else {
+		info->map = ocelot_mfd_get_regmap_from_resource(dev->parent,
+								res);
+	}
 
-	info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
 	if (IS_ERR(info->map)) {
 		dev_err(dev, "Failed to create regmap\n");
 		return PTR_ERR(info->map);
-- 
2.25.1

[RFC v5 net-next 13/13] mfd: ocelot: add ocelot-pinctrl as a supported child interface

[Colin Foster]

The ocelot-pinctrl device is able to be utilized by ocelot_mfd. This simply
enables that child driver.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/ocelot-core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index c67e433f467c..71e062934812 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -113,7 +113,22 @@ static const struct resource vsc7512_miim1_resources[] = {
 	},
 };
 
+static const struct resource vsc7512_pinctrl_resources[] = {
+	{
+		.start = 0x71070034,
+		.end = 0x7107009f,
+		.name = "gcb_gpio",
+		.flags = IORESOURCE_MEM,
+	},
+};
+
 static const struct mfd_cell vsc7512_devs[] = {
+	{
+		.name = "pinctrl-ocelot",
+		.of_compatible = "mscc,ocelot-pinctrl",
+		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
+		.resources = vsc7512_pinctrl_resources,
+	},
 	{
 		.name = "ocelot-miim1",
 		.of_compatible = "mscc,ocelot-miim",
@@ -164,6 +179,10 @@ int ocelot_mfd_init(struct ocelot_mfd_config *config)
 
 	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, vsc7512_devs,
 			      ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
+	if (ret) {
+		dev_err(dev, "error adding mfd devices\n");
+		return ret;
+	}
 
 	dev_info(dev, "ocelot mfd core setup complete\n");
 
-- 
2.25.1

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> Create a single SPI MFD ocelot device that manages the SPI bus on the
> external chip and can handle requests for regmaps. This should allow any
> ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
> utilize regmaps.

We're going to need Mark Brown to have a look at this Regmap implementation.

> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/Kconfig       |  15 ++
>  drivers/mfd/Makefile      |   3 +
>  drivers/mfd/ocelot-core.c | 149 +++++++++++++++
>  drivers/mfd/ocelot-mfd.h  |  19 ++

Drop the '-mfd' part please.

>  drivers/mfd/ocelot-spi.c  | 374 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 560 insertions(+)
>  create mode 100644 drivers/mfd/ocelot-core.c
>  create mode 100644 drivers/mfd/ocelot-mfd.h
>  create mode 100644 drivers/mfd/ocelot-spi.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3fb480818599..af76c9780a10 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -954,6 +954,21 @@ config MFD_MENF21BMC
>  	  This driver can also be built as a module. If so the module
>  	  will be called menf21bmc.
>  
> +config MFD_OCELOT_CORE

You can drop the "_CORE" part.

> +	tristate "Microsemi Ocelot External Control Support"
> +	select MFD_CORE
> +	help
> +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> +	  VSC7513, VSC7514) controlled externally.

Please describe the device in more detail here.

I'm not sure what an "External Control Support" is.

> +config MFD_OCELOT_SPI
> +	tristate "Microsemi Ocelot SPI interface"
> +	depends on MFD_OCELOT_CORE
> +	depends on SPI_MASTER
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to add control to the MFD_OCELOT chips via SPI.
> +
>  config EZX_PCAP
>  	bool "Motorola EZXPCAP Support"
>  	depends on SPI_MASTER
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 0b1b629aef3e..dff83f474fb5 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -120,6 +120,9 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
>  
>  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
>  
> +obj-$(CONFIG_MFD_OCELOT_CORE)	+= ocelot-core.o
> +obj-$(CONFIG_MFD_OCELOT_SPI)	+= ocelot-spi.o
> +
>  obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
>  obj-$(CONFIG_MFD_CPCAP)		+= motorola-cpcap.o
>  
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> new file mode 100644
> index 000000000000..a65619a8190b
> --- /dev/null
> +++ b/drivers/mfd/ocelot-core.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2021 Innovative Advantage Inc.

Author?

Short device description?

> + */
> +
> +#include <asm/byteorder.h>

These, if required, usually go at the bottom.

> +#include <linux/spi/spi.h>
> +#include <linux/kconfig.h>

What's this for?

> +#include <linux/module.h>
> +#include <linux/regmap.h>

These should be alphabetical.

> +#include "ocelot-mfd.h"
> +
> +#define REG(reg, offset)	[reg] = offset

What does this save, really?

> +enum ocelot_mfd_gcb_regs {

Please remove the term 'mfd\|MFD' from everywhere.

> +	GCB_SOFT_RST,
> +	GCB_REG_MAX,
> +};
> +
> +enum ocelot_mfd_gcb_regfields {
> +	GCB_SOFT_RST_CHIP_RST,
> +	GCB_REGFIELD_MAX,
> +};
> +
> +static const u32 vsc7512_gcb_regmap[] = {
> +	REG(GCB_SOFT_RST,	0x0008),
> +};
> +
> +static const struct reg_field vsc7512_mfd_gcb_regfields[GCB_REGFIELD_MAX] = {
> +	[GCB_SOFT_RST_CHIP_RST] = REG_FIELD(vsc7512_gcb_regmap[GCB_SOFT_RST], 0, 0),
> +};
> +
> +struct ocelot_mfd_core {
> +	struct ocelot_mfd_config *config;
> +	struct regmap *gcb_regmap;
> +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> +};

Not sure about this at all.

Which driver did you take your inspiration from?

> +static const struct resource vsc7512_gcb_resource = {
> +	.start	= 0x71070000,
> +	.end	= 0x7107022b,

No magic numbers please.

> +	.name	= "devcpu_gcb",

What is a 'devcpu_gcb'?

> +};
> +
> +static int ocelot_mfd_reset(struct ocelot_mfd_core *core)
> +{
> +	int ret;
> +
> +	dev_info(core->config->dev, "resetting ocelot chip\n");

These types of calls are not useful in production code.

> +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);

No magic numbers please.  I have no idea what this is doing.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> +	 * suggest how to do a reset over SPI, and the register strategy isn't
> +	 * possible.
> +	 */
> +	msleep(100);
> +
> +	ret = core->config->init_bus(core->config);

You're not writing a bus.  I doubt you need ops call-backs.

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> +				  int size)
> +{
> +	if (res->name)
> +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> +	else
> +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> +}
> +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);

What is this used for?

You should not be hand rolling device resource names like this.

This sort of code belongs in the bus/class API.

Please use those instead.

> +static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
> +					     const struct resource *res)
> +{
> +	struct device *dev = core->config->dev;
> +	struct regmap *regmap;
> +	char name[32];
> +
> +	ocelot_mfd_get_resource_name(name, res, sizeof(name) - 1);
> +
> +	regmap = dev_get_regmap(dev, name);
> +
> +	if (!regmap)
> +		regmap = core->config->get_regmap(core->config, res, name);
> +
> +	return regmap;
> +}
> +
> +int ocelot_mfd_init(struct ocelot_mfd_config *config)
> +{
> +	struct device *dev = config->dev;
> +	const struct reg_field *regfield;
> +	struct ocelot_mfd_core *core;
> +	int i, ret;
> +
> +	core = devm_kzalloc(dev, sizeof(struct ocelot_mfd_config), GFP_KERNEL);
> +	if (!core)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, core);
> +
> +	core->config = config;
> +
> +	/* Create regmaps and regfields here */
> +	core->gcb_regmap = ocelot_mfd_regmap_init(core, &vsc7512_gcb_resource);
> +	if (!core->gcb_regmap)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < GCB_REGFIELD_MAX; i++) {
> +		regfield = &vsc7512_mfd_gcb_regfields[i];
> +		core->gcb_regfields[i] =
> +			devm_regmap_field_alloc(dev, core->gcb_regmap,
> +						*regfield);
> +		if (!core->gcb_regfields[i])
> +			return -ENOMEM;
> +	}
> +
> +	/* Prepare the chip */
> +	ret = ocelot_mfd_reset(core);
> +	if (ret) {
> +		dev_err(dev, "ocelot mfd reset failed with code %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Create and loop over all child devices here */

These need to all go in now please.

> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mfd_init);
> +
> +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> +{
> +	/* Loop over all children and remove them */

Use devm_* then you won't have to.

> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mfd_remove);
> +
> +MODULE_DESCRIPTION("Ocelot Chip MFD driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/ocelot-mfd.h b/drivers/mfd/ocelot-mfd.h
> new file mode 100644
> index 000000000000..6af8b8c5a316
> --- /dev/null
> +++ b/drivers/mfd/ocelot-mfd.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright 2021 Innovative Advantage Inc.
> + */
> +
> +#include <linux/regmap.h>
> +
> +struct ocelot_mfd_config {
> +	struct device *dev;
> +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> +				     const struct resource *res,
> +				     const char *name);
> +	int (*init_bus)(struct ocelot_mfd_config *config);

Please re-work and delete this 'config' concept.

See other drivers in this sub-directory for reference.

> +};
> +
> +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> +				  int size);
> +int ocelot_mfd_init(struct ocelot_mfd_config *config);
> +int ocelot_mfd_remove(struct ocelot_mfd_config *config);
> diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> new file mode 100644
> index 000000000000..65ceb68f27af
> --- /dev/null
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -0,0 +1,374 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2021 Innovative Advantage Inc.

As above.

> + */
> +
> +#include <asm/byteorder.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iopoll.h>
> +#include <linux/kconfig.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>

As above.

> +#include "ocelot-mfd.h"
> +
> +#define REG(reg, offset)	[reg] = offset
> +
> +struct ocelot_spi {
> +	int spi_padding_bytes;
> +	struct spi_device *spi;
> +	struct ocelot_mfd_config config;
> +	struct regmap *cpuorg_regmap;
> +	const u32 *map;
> +};
> +
> +enum ocelot_dev_cpuorg_regs {
> +	DEV_CPUORG_IF_CTRL,
> +	DEV_CPUORG_IF_CFGSTAT,
> +	DEV_CPUORG_REG_MAX,
> +};
> +
> +static const u32 vsc7512_dev_cpuorg_regmap[] = {
> +	REG(DEV_CPUORG_IF_CTRL,		0x0000),
> +	REG(DEV_CPUORG_IF_CFGSTAT,	0x0004),
> +};
> +
> +static const struct resource vsc7512_dev_cpuorg_resource = {
> +	.start	= 0x71000000,
> +	.end	= 0x710002ff,
> +	.name	= "devcpu_org",
> +};
> +
> +#define VSC7512_BYTE_ORDER_LE 0x00000000
> +#define VSC7512_BYTE_ORDER_BE 0x81818181
> +#define VSC7512_BIT_ORDER_MSB 0x00000000
> +#define VSC7512_BIT_ORDER_LSB 0x42424242
> +
> +static struct ocelot_spi *
> +config_to_ocelot_spi(struct ocelot_mfd_config *config)
> +{
> +	return container_of(config, struct ocelot_spi, config);
> +}
> +
> +static int ocelot_spi_init_bus(struct ocelot_spi *ocelot_spi)
> +{
> +	struct spi_device *spi;
> +	struct device *dev;
> +	u32 val, check;
> +	int err;
> +
> +	spi = ocelot_spi->spi;
> +	dev = &spi->dev;
> +
> +	dev_info(dev, "initializing SPI interface for chip\n");
> +
> +	val = 0;
> +
> +#ifdef __LITTLE_ENDIAN
> +	val |= VSC7512_BYTE_ORDER_LE;
> +#else
> +	val |= VSC7512_BYTE_ORDER_BE;
> +#endif
> +
> +	err = regmap_write(ocelot_spi->cpuorg_regmap,
> +			   ocelot_spi->map[DEV_CPUORG_IF_CTRL], val);
> +	if (err)
> +		return err;
> +
> +	val = ocelot_spi->spi_padding_bytes;
> +	err = regmap_write(ocelot_spi->cpuorg_regmap,
> +			   ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], val);
> +	if (err)
> +		return err;
> +
> +	check = val | 0x02000000;
> +
> +	err = regmap_read(ocelot_spi->cpuorg_regmap,
> +			  ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], &val);
> +	if (err)
> +		return err;
> +
> +	if (check != val) {
> +		dev_err(dev, "Error configuring SPI bus. V: 0x%08x != 0x%08x\n",
> +			val, check);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ocelot_spi_init_bus_from_config(struct ocelot_mfd_config *config)
> +{
> +	struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
> +
> +	return ocelot_spi_init_bus(ocelot_spi);
> +}
> +
> +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> +{
> +	return cpu_to_be32((reg & 0xffffff) >> 2);
> +}
> +
> +struct ocelot_spi_regmap_context {
> +	struct spi_device *spi;
> +	u32 base;
> +	int padding_bytes;
> +};
> +
> +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct ocelot_spi_regmap_context *regmap_context = context;
> +	struct spi_transfer tx, padding, rx;
> +	struct ocelot_spi *ocelot_spi;
> +	struct spi_message msg;
> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	WARN_ON(!val);
> +
> +	spi = regmap_context->spi;
> +
> +	ocelot_spi = spi_get_drvdata(spi);
> +
> +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> +	tx_buf = (u8 *)&addr;
> +
> +	spi_message_init(&msg);
> +
> +	memset(&tx, 0, sizeof(struct spi_transfer));
> +
> +	/* Ignore the first byte for the 24-bit address */
> +	tx.tx_buf = &tx_buf[1];
> +	tx.len = 3;
> +
> +	spi_message_add_tail(&tx, &msg);
> +
> +	if (regmap_context->padding_bytes > 0) {
> +		u8 dummy_buf[16] = {0};
> +
> +		memset(&padding, 0, sizeof(struct spi_transfer));
> +
> +		/* Just toggle the clock for padding bytes */
> +		padding.len = regmap_context->padding_bytes;
> +		padding.tx_buf = dummy_buf;
> +		padding.dummy_data = 1;
> +
> +		spi_message_add_tail(&padding, &msg);
> +	}
> +
> +	memset(&rx, 0, sizeof(struct spi_transfer));
> +	rx.rx_buf = val;
> +	rx.len = 4;
> +
> +	spi_message_add_tail(&rx, &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static int ocelot_spi_reg_write(void *context, unsigned int reg,
> +				unsigned int val)
> +{
> +	struct ocelot_spi_regmap_context *regmap_context = context;
> +	struct spi_transfer tx[2] = {0};
> +	struct spi_message msg;
> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	spi = regmap_context->spi;
> +
> +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> +	tx_buf = (u8 *)&addr;
> +
> +	spi_message_init(&msg);
> +
> +	/* Ignore the first byte for the 24-bit address and set the write bit */
> +	tx_buf[1] |= BIT(7);
> +	tx[0].tx_buf = &tx_buf[1];
> +	tx[0].len = 3;
> +
> +	spi_message_add_tail(&tx[0], &msg);
> +
> +	memset(&tx[1], 0, sizeof(struct spi_transfer));
> +	tx[1].tx_buf = &val;
> +	tx[1].len = 4;
> +
> +	spi_message_add_tail(&tx[1], &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static const struct regmap_config ocelot_spi_regmap_config = {
> +	.reg_bits = 24,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +
> +	.reg_read = ocelot_spi_reg_read,
> +	.reg_write = ocelot_spi_reg_write,
> +
> +	.max_register = 0xffffffff,
> +	.use_single_write = true,
> +	.use_single_read = true,
> +	.can_multi_write = false,
> +
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +static struct regmap *
> +ocelot_spi_get_regmap(struct ocelot_mfd_config *config,
> +		      const struct resource *res, const char *name)
> +{
> +	struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
> +	struct ocelot_spi_regmap_context *context;
> +	struct regmap_config regmap_config;
> +	struct regmap *regmap;
> +	struct device *dev;
> +
> +
> +	dev = &ocelot_spi->spi->dev;
> +
> +	/* Don't re-allocate another regmap if we have one */
> +	regmap = dev_get_regmap(dev, name);
> +	if (regmap)
> +		return regmap;
> +
> +	context = devm_kzalloc(dev, sizeof(struct ocelot_spi_regmap_context),
> +			       GFP_KERNEL);
> +
> +	if (IS_ERR(context))
> +		return ERR_CAST(context);
> +
> +	context->base = res->start;
> +	context->spi = ocelot_spi->spi;
> +	context->padding_bytes = ocelot_spi->spi_padding_bytes;
> +
> +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> +	       sizeof(ocelot_spi_regmap_config));
> +
> +	regmap_config.name = name;
> +	regmap_config.max_register = res->end - res->start;
> +
> +	regmap = devm_regmap_init(dev, NULL, context, &regmap_config);
> +	if (IS_ERR(regmap))
> +		return ERR_CAST(regmap);
> +
> +	return regmap;
> +}
> +
> +static int ocelot_spi_probe(struct spi_device *spi)
> +{
> +	struct ocelot_spi *ocelot_spi;
> +	struct device *dev;
> +	char name[32];
> +	int err;
> +
> +	dev = &spi->dev;

Put this on the declaration line.

> +	ocelot_spi = devm_kzalloc(dev, sizeof(struct ocelot_spi),

sizeof(*ocelot_spi)

> +				      GFP_KERNEL);
> +

No '\n'.

> +	if (!ocelot_spi)
> +		return -ENOMEM;
> +
> +	if (spi->max_speed_hz <= 500000) {
> +		ocelot_spi->spi_padding_bytes = 0;
> +	} else {
> +		/*
> +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> +		 * on the side of more padding bytes, as having too few can be
> +		 * difficult to detect at runtime.
> +		 */
> +		ocelot_spi->spi_padding_bytes = 1 +
> +			(spi->max_speed_hz / 1000000 + 2) / 8;

Please explain what this means or define the values (or both).

> +	}
> +
> +	ocelot_spi->spi = spi;

Why are you saving this?

> +	ocelot_spi->map = vsc7512_dev_cpuorg_regmap;

Why not just set up the regmap here?

> +	spi->bits_per_word = 8;
> +
> +	err = spi_setup(spi);
> +	if (err < 0) {
> +		dev_err(&spi->dev, "Error %d initializing SPI\n", err);

The error code usually comes at the end.

> +		return err;
> +	}
> +
> +	dev_info(dev, "configured SPI bus for speed %d, rx padding bytes %d\n",
> +			spi->max_speed_hz, ocelot_spi->spi_padding_bytes);

When would this be useful?

Don't we already have debug interfaces to find this out?

> +	/* Ensure we have devcpu_org regmap before we call ocelot_mfd_init */

because ...

> +	ocelot_mfd_get_resource_name(name, &vsc7512_dev_cpuorg_resource,
> +				     sizeof(name) - 1);

This is an ugly interface.  I think it needs to go.

> +	/*
> +	 * Since we created dev, we know there isn't a regmap, so create one
> +	 * here directly.
> +	 */

Sorry, what 'dev'?  When did we create that?

> +	ocelot_spi->cpuorg_regmap =
> +		ocelot_spi_get_regmap(&ocelot_spi->config,
> +				      &vsc7512_dev_cpuorg_resource, name);
> +	if (!ocelot_spi->cpuorg_regmap)
> +		return -ENOMEM;
> +
> +	ocelot_spi->config.init_bus = ocelot_spi_init_bus_from_config;
> +	ocelot_spi->config.get_regmap = ocelot_spi_get_regmap;
> +	ocelot_spi->config.dev = dev;

Please remove this API.

> +	spi_set_drvdata(spi, ocelot_spi);
> +
> +	/*
> +	 * The chip must be set up for SPI before it gets initialized and reset.
> +	 * Do this once here before calling mfd_init
> +	 */
> +	err = ocelot_spi_init_bus(ocelot_spi);
> +	if (err) {
> +		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);

Doesn't this already print out an error message?

> +		return err;
> +	}
> +
> +	err = ocelot_mfd_init(&ocelot_spi->config);
> +	if (err < 0) {
> +		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
> +		return err;
> +	}
> +
> +	dev_info(&spi->dev, "ocelot spi mfd probed\n");

Please, remove all of these.

> +	return 0;
> +}
> +
> +static int ocelot_spi_remove(struct spi_device *spi)
> +{
> +	struct ocelot_spi *ocelot_spi;
> +
> +	ocelot_spi = spi_get_drvdata(spi);
> +	devm_kfree(&spi->dev, ocelot_spi);

Why use devm_* if you're going to free anyway?

> +	return 0;
> +}
> +
> +const struct of_device_id ocelot_mfd_of_match[] = {
> +	{ .compatible = "mscc,vsc7514_mfd_spi" },
> +	{ .compatible = "mscc,vsc7513_mfd_spi" },
> +	{ .compatible = "mscc,vsc7512_mfd_spi" },
> +	{ .compatible = "mscc,vsc7511_mfd_spi" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ocelot_mfd_of_match);
> +
> +static struct spi_driver ocelot_mfd_spi_driver = {
> +	.driver = {
> +		.name = "ocelot_mfd_spi",
> +		.of_match_table = of_match_ptr(ocelot_mfd_of_match),
> +	},
> +	.probe = ocelot_spi_probe,
> +	.remove = ocelot_spi_remove,
> +};
> +module_spi_driver(ocelot_mfd_spi_driver);
> +
> +MODULE_DESCRIPTION("Ocelot Chip MFD SPI driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("Dual MIT/GPL");

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 02/13] mfd: ocelot: offer an interface for MFD children to get regmaps

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> Child devices need to get a regmap from a resource struct, specifically
> from the MFD parent. The MFD parent has the interface to the hardware
> layer, which could be I2C, SPI, PCIe, etc.
> 
> This is somewhat a hack... ideally child devices would interface with the
> struct device* directly, by way of a function like
> devm_get_regmap_from_resource which would be akin to
> devm_get_and_ioremap_resource. A less ideal option would be to interface
> directly with MFD to get a regmap from the parent.
> 
> This solution is even less ideal than both of the two suggestions, so is
> intentionally left in a separate commit after the initial MFD addition.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/ocelot-core.c |  9 +++++++++
>  include/soc/mscc/ocelot.h | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index a65619a8190b..09132ea52760 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -94,6 +94,15 @@ static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
>  	return regmap;
>  }
>  
> +struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
> +						   const struct resource *res)
> +{
> +	struct ocelot_mfd_core *core = dev_get_drvdata(dev);
> +
> +	return ocelot_mfd_regmap_init(core, res);
> +}
> +EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);

This is almost certainly not the right way to do whatever it is you're
trying to do!

Please don't try to upstream "somewhat a hack"s into the Mainline
kernel.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 07/13] mfd: ocelot: enable the external switch interface

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> Add the ocelot-ext child device to the MFD. This will enable device-tree
> configuration of the MFD to include the external switch, if desired.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/ocelot-core.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Please squash this into the driver creation patch.

> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 09132ea52760..52aa7b824d02 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -6,6 +6,7 @@
>  #include <asm/byteorder.h>
>  #include <linux/spi/spi.h>
>  #include <linux/kconfig.h>
> +#include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -103,6 +104,13 @@ struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
>  }
>  EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
>  
> +static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "ocelot-ext-switch",
> +		.of_compatible = "mscc,vsc7512-ext-switch",
> +	},
> +};
> +
>  int ocelot_mfd_init(struct ocelot_mfd_config *config)
>  {
>  	struct device *dev = config->dev;
> @@ -139,7 +147,10 @@ int ocelot_mfd_init(struct ocelot_mfd_config *config)
>  		return ret;
>  	}
>  
> -	/* Create and loop over all child devices here */
> +	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, vsc7512_devs,
> +			      ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> +
> +	dev_info(dev, "ocelot mfd core setup complete\n");
>  
>  	return 0;
>  }
> @@ -147,7 +158,7 @@ EXPORT_SYMBOL(ocelot_mfd_init);
>  
>  int ocelot_mfd_remove(struct ocelot_mfd_config *config)
>  {
> -	/* Loop over all children and remove them */
> +	mfd_remove_devices(config->dev);
>  
>  	return 0;
>  }

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> Some drivers will need to create regmaps differently based on whether they
> are a child of an MFD or a standalone device. An example of this would be
> if a regmap were directly memory-mapped or an external bus. In the
> memory-mapped case a call to devm_regmap_init_mmio would return the correct
> regmap. In the case of an MFD, the regmap would need to be requested from
> the parent device.
> 
> This addition allows the driver to correctly reason about these scenarios.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/mfd-core.c   |  5 +++++
>  include/linux/mfd/core.h | 10 ++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 684a011a6396..905f508a31b4 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
>  	.name	= "mfd_device",
>  };
>  
> +int device_is_mfd(struct platform_device *pdev)
> +{
> +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> +}
> +

Why is this device different to any other that has ever been
mainlined?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 11/13] mfd: ocelot-core: add control for the external mdio interface

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> Utilize the mscc-miim-mdio driver as a child of the ocelot MFD.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/ocelot-core.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Squash please.

> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 52aa7b824d02..c67e433f467c 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -104,7 +104,22 @@ struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
>  }
>  EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
>  
> +static const struct resource vsc7512_miim1_resources[] = {
> +	{
> +		.start = 0x710700c0,
> +		.end = 0x710700e3,
> +		.name = "gcb_miim1",
> +		.flags = IORESOURCE_MEM,
> +	},
> +};
> +
>  static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "ocelot-miim1",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> +		.resources = vsc7512_miim1_resources,
> +	},
>  	{
>  		.name = "ocelot-ext-switch",
>  		.of_compatible = "mscc,vsc7512-ext-switch",

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 13/13] mfd: ocelot: add ocelot-pinctrl as a supported child interface

[Lee Jones]

On Sat, 18 Dec 2021, Colin Foster wrote:

> The ocelot-pinctrl device is able to be utilized by ocelot_mfd. This simply
> enables that child driver.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/ocelot-core.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Squash please.

> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index c67e433f467c..71e062934812 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -113,7 +113,22 @@ static const struct resource vsc7512_miim1_resources[] = {
>  	},
>  };
>  
> +static const struct resource vsc7512_pinctrl_resources[] = {
> +	{
> +		.start = 0x71070034,
> +		.end = 0x7107009f,
> +		.name = "gcb_gpio",
> +		.flags = IORESOURCE_MEM,
> +	},
> +};
> +
>  static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "pinctrl-ocelot",
> +		.of_compatible = "mscc,ocelot-pinctrl",
> +		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
> +		.resources = vsc7512_pinctrl_resources,
> +	},
>  	{
>  		.name = "ocelot-miim1",
>  		.of_compatible = "mscc,ocelot-miim",
> @@ -164,6 +179,10 @@ int ocelot_mfd_init(struct ocelot_mfd_config *config)
>  
>  	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, vsc7512_devs,
>  			      ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "error adding mfd devices\n");
> +		return ret;
> +	}
>  
>  	dev_info(dev, "ocelot mfd core setup complete\n");
>  

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 02/13] mfd: ocelot: offer an interface for MFD children to get regmaps

[Alexandre Belloni]

Hello Lee,

On 29/12/2021 15:23:59+0000, Lee Jones wrote:
> On Sat, 18 Dec 2021, Colin Foster wrote:
> 
> > Child devices need to get a regmap from a resource struct, specifically
> > from the MFD parent. The MFD parent has the interface to the hardware
> > layer, which could be I2C, SPI, PCIe, etc.
> > 
> > This is somewhat a hack... ideally child devices would interface with the
> > struct device* directly, by way of a function like
> > devm_get_regmap_from_resource which would be akin to
> > devm_get_and_ioremap_resource. A less ideal option would be to interface
> > directly with MFD to get a regmap from the parent.
> > 
> > This solution is even less ideal than both of the two suggestions, so is
> > intentionally left in a separate commit after the initial MFD addition.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/mfd/ocelot-core.c |  9 +++++++++
> >  include/soc/mscc/ocelot.h | 12 ++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index a65619a8190b..09132ea52760 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -94,6 +94,15 @@ static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
> >  	return regmap;
> >  }
> >  
> > +struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
> > +						   const struct resource *res)
> > +{
> > +	struct ocelot_mfd_core *core = dev_get_drvdata(dev);
> > +
> > +	return ocelot_mfd_regmap_init(core, res);
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
> 
> This is almost certainly not the right way to do whatever it is you're
> trying to do!
> 
> Please don't try to upstream "somewhat a hack"s into the Mainline
> kernel.
> 

Please elaborate on the correct way to do that. What we have here is a
SoC (vsc7514) that has MMIO devices. This SoC has a MIPS CPU and
everything is fine when using it. However, the CPU can be disabled and
the SoC connected to another CPU using SPI or PCIe. What Colin is doing
here is using this SoC over SPI. Don't tell me this is not an MFD
because this is exactly what this is, a single chip with a collection of
devices that are also available separately.

The various drivers for the VSC7514 have been written using regmap
exactly for this use case. The missing piece is probing the devices over
SPI instead of MMIO.

Notice that all of that gets worse when using PCIe on architectures that
don't have device tree support and Clément will submit multiple series
trying to fix that.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC v5 net-next 02/13] mfd: ocelot: offer an interface for MFD children to get regmaps

[Lee Jones]

> > This is almost certainly not the right way to do whatever it is you're
> > trying to do!
> > 
> > Please don't try to upstream "somewhat a hack"s into the Mainline
> > kernel.
> > 
> 
> Please elaborate on the correct way to do that. What we have here is a
> SoC (vsc7514) that has MMIO devices. This SoC has a MIPS CPU and
> everything is fine when using it. However, the CPU can be disabled and
> the SoC connected to another CPU using SPI or PCIe. What Colin is doing
> here is using this SoC over SPI. Don't tell me this is not an MFD

When did anyone say that?

> because this is exactly what this is, a single chip with a collection of
> devices that are also available separately.
> 
> The various drivers for the VSC7514 have been written using regmap
> exactly for this use case. The missing piece is probing the devices over
> SPI instead of MMIO.
> 
> Notice that all of that gets worse when using PCIe on architectures that
> don't have device tree support and Clément will submit multiple series
> trying to fix that.

Okay, it sounds like I'm missing some information, let's see if we can
get to the bottom of this so that I can provide some informed
guidance.

> On 29/12/2021 15:23:59+0000, Lee Jones wrote:
> > On Sat, 18 Dec 2021, Colin Foster wrote:
> > 
> > > Child devices need to get a regmap from a resource struct,
> > > specifically from the MFD parent.

Child devices usually fetch the Regmap from a pointer the parent
provides.  Usually via either 'driver_data' or 'platform_data'.

However, we can't do that here because ...

> > > The MFD parent has the interface to the hardware
> > > layer, which could be I2C, SPI, PCIe, etc.
> > > 
> > > This is somewhat a hack... ideally child devices would interface with the
> > > struct device* directly, by way of a function like
> > > devm_get_regmap_from_resource which would be akin to
> > > devm_get_and_ioremap_resource.

However, we can't do that here because ...

> > > A less ideal option would be to interface
> > > directly with MFD to get a regmap from the parent.
> > > 
> > > This solution is even less ideal than both of the two suggestions, so is
> > > intentionally left in a separate commit after the initial MFD addition.
> > > 
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> > >  drivers/mfd/ocelot-core.c |  9 +++++++++
> > >  include/soc/mscc/ocelot.h | 12 ++++++++++++
> > >  2 files changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > > index a65619a8190b..09132ea52760 100644
> > > --- a/drivers/mfd/ocelot-core.c
> > > +++ b/drivers/mfd/ocelot-core.c
> > > @@ -94,6 +94,15 @@ static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
> > >  	return regmap;
> > >  }
> > >  
> > > +struct regmap *ocelot_mfd_get_regmap_from_resource(struct device *dev,
> > > +						   const struct resource *res)
> > > +{
> > > +	struct ocelot_mfd_core *core = dev_get_drvdata(dev);
> > > +
> > > +	return ocelot_mfd_regmap_init(core, res);
> > > +}
> > > +EXPORT_SYMBOL(ocelot_mfd_get_regmap_from_resource);
> > 
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:
> On Sat, 18 Dec 2021, Colin Foster wrote:
> 
> > Create a single SPI MFD ocelot device that manages the SPI bus on the
> > external chip and can handle requests for regmaps. This should allow any
> > ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
> > utilize regmaps.

Hi Lee,

Thanks for your feedback.

> 
> We're going to need Mark Brown to have a look at this Regmap implementation.
> 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/mfd/Kconfig       |  15 ++
> >  drivers/mfd/Makefile      |   3 +
> >  drivers/mfd/ocelot-core.c | 149 +++++++++++++++
> >  drivers/mfd/ocelot-mfd.h  |  19 ++
> 
> Drop the '-mfd' part please.

Done. 

> 
> >  drivers/mfd/ocelot-spi.c  | 374 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 560 insertions(+)
> >  create mode 100644 drivers/mfd/ocelot-core.c
> >  create mode 100644 drivers/mfd/ocelot-mfd.h
> >  create mode 100644 drivers/mfd/ocelot-spi.c
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3fb480818599..af76c9780a10 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -954,6 +954,21 @@ config MFD_MENF21BMC
> >  	  This driver can also be built as a module. If so the module
> >  	  will be called menf21bmc.
> >  
> > +config MFD_OCELOT_CORE
> 
> You can drop the "_CORE" part.

Done. The idea here though was taken after the madera driver, since it
seemed fairly recent and does what I expect this Ocelot driver to end up
doing - allow control over either SPI or I2C.

> 
> > +	tristate "Microsemi Ocelot External Control Support"
> > +	select MFD_CORE
> > +	help
> > +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> > +	  VSC7513, VSC7514) controlled externally.
> 
> Please describe the device in more detail here.
> 
> I'm not sure what an "External Control Support" is.

A second paragraph "All four of these chips can be controlled internally
(MMIO) or externally via SPI, I2C, PCIe. This enables control of these
chips over one or more of these buses"

> 
> > +config MFD_OCELOT_SPI
> > +	tristate "Microsemi Ocelot SPI interface"
> > +	depends on MFD_OCELOT_CORE
> > +	depends on SPI_MASTER
> > +	select REGMAP_SPI
> > +	help
> > +	  Say yes here to add control to the MFD_OCELOT chips via SPI.
> > +
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 0b1b629aef3e..dff83f474fb5 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -120,6 +120,9 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
> >  
> >  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
> >  
> > +obj-$(CONFIG_MFD_OCELOT_CORE)	+= ocelot-core.o
> > +obj-$(CONFIG_MFD_OCELOT_SPI)	+= ocelot-spi.o
> > +
> >  obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
> >  obj-$(CONFIG_MFD_CPCAP)		+= motorola-cpcap.o
> >  
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > new file mode 100644
> > index 000000000000..a65619a8190b
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> 
> Author?
> 
> Short device description?

Added. 

> 
> > + */
> > +
> > +#include <asm/byteorder.h>
> 
> These, if required, usually go at the bottom.

Moved between regmap and ocelot.h

> 
> > +#include <linux/spi/spi.h>
> > +#include <linux/kconfig.h>
> 
> What's this for?

Both were left over from my initial "DSA" implementation that had all
this lumped together. They only belong in ocelot-spi.c, and I didn't
remove them here. Thanks for catching these!

> 
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> 
> These should be alphabetical.
> 
> > +#include "ocelot-mfd.h"
> > +
> > +#define REG(reg, offset)	[reg] = offset
> 
> What does this save, really?

Fair point. This was done for consistency with other ocelot
drivers that use the upper bits in the enumeration as a separate index.
Specifically the enumeration in include/soc/mscc/ocelot.h and
drivers/net/ethernet/mscc/vsc7514_regs.c. It has no other benefit than
just that, and I have no problem removing it if that's desired.

> 
> > +enum ocelot_mfd_gcb_regs {
> 
> Please remove the term 'mfd\|MFD' from everywhere.

"ocelot_init" conflicts with a symbol in
drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
now.

> 
> > +	GCB_SOFT_RST,
> > +	GCB_REG_MAX,
> > +};
> > +
> > +enum ocelot_mfd_gcb_regfields {
> > +	GCB_SOFT_RST_CHIP_RST,
> > +	GCB_REGFIELD_MAX,
> > +};
> > +
> > +static const u32 vsc7512_gcb_regmap[] = {
> > +	REG(GCB_SOFT_RST,	0x0008),
> > +};
> > +
> > +static const struct reg_field vsc7512_mfd_gcb_regfields[GCB_REGFIELD_MAX] = {
> > +	[GCB_SOFT_RST_CHIP_RST] = REG_FIELD(vsc7512_gcb_regmap[GCB_SOFT_RST], 0, 0),
> > +};
> > +
> > +struct ocelot_mfd_core {
> > +	struct ocelot_mfd_config *config;
> > +	struct regmap *gcb_regmap;
> > +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> > +};
> 
> Not sure about this at all.
> 
> Which driver did you take your inspiration from?

Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.

> 
> > +static const struct resource vsc7512_gcb_resource = {
> > +	.start	= 0x71070000,
> > +	.end	= 0x7107022b,
> 
> No magic numbers please.

I've gotten conflicting feedback on this. Several of the ocelot drivers
(drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
all passed in through the device tree. 

https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/

> 
> > +	.name	= "devcpu_gcb",
> 
> What is a 'devcpu_gcb'?

It matches the datasheet of the CPU's general configuation block.

> 
> > +};
> > +
> > +static int ocelot_mfd_reset(struct ocelot_mfd_core *core)
> > +{
> > +	int ret;
> > +
> > +	dev_info(core->config->dev, "resetting ocelot chip\n");
> 
> These types of calls are not useful in production code.

Removed.

> 
> > +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
> 
> No magic numbers please.  I have no idea what this is doing.

I'm not sure how much more verbose it can be... I suppose a define for
"RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
blinded by having stared at this code for the last several months.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> > +	 * suggest how to do a reset over SPI, and the register strategy isn't
> > +	 * possible.
> > +	 */
> > +	msleep(100);
> > +
> > +	ret = core->config->init_bus(core->config);
> 
> You're not writing a bus.  I doubt you need ops call-backs.

In the case of SPI, the chip needs to be configured both before and
after reset. It sets up the bus for endianness, padding bytes, etc. This
is currently achieved by running "ocelot_spi_init_bus" once during SPI
probe, and once immediately after chip reset.

For other control mediums I doubt this is necessary. Perhaps "init_bus"
is a misnomer in this scenario...

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > +				  int size)
> > +{
> > +	if (res->name)
> > +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> > +	else
> > +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
> 
> What is this used for?
> 
> You should not be hand rolling device resource names like this.
> 
> This sort of code belongs in the bus/class API.
> 
> Please use those instead.

The idea here was to allow shared regmaps across different devices. The
"devcpu_gcb" might be used in two ways - either everyone shares the same
regmap across the "GCB" range, or everyone creates their own. 

This was more useful when the ocelot-core.c had a copy of the 
"devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
remove that, but also feel like the full switch driver (patch 6 of this
set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
ocelot-core does.

Admittedly, there are complications. There should probably be more
checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
regmap for ocelot_ext exactly matches the existing regmap for
ocelot_core.

There's yet another complexity with these, and I'm not sure what the
answer is. Currently all regmaps are tied to the ocelot_spi device...
ocelot_spi calls devm_regmap_init. So those regmaps hang around if
they're created by a module that has been removed. At least until the
entire MFD module is removed. Maybe there's something I haven't seen yet
where the devres or similar has a reference count. I don't know the best
path forward on this one.

> 
> > +static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
> > +					     const struct resource *res)
> > +{
> > +	struct device *dev = core->config->dev;
> > +	struct regmap *regmap;
> > +	char name[32];
> > +
> > +	ocelot_mfd_get_resource_name(name, res, sizeof(name) - 1);
> > +
> > +	regmap = dev_get_regmap(dev, name);
> > +
> > +	if (!regmap)
> > +		regmap = core->config->get_regmap(core->config, res, name);
> > +
> > +	return regmap;
> > +}
> > +
> > +int ocelot_mfd_init(struct ocelot_mfd_config *config)
> > +{
> > +	struct device *dev = config->dev;
> > +	const struct reg_field *regfield;
> > +	struct ocelot_mfd_core *core;
> > +	int i, ret;
> > +
> > +	core = devm_kzalloc(dev, sizeof(struct ocelot_mfd_config), GFP_KERNEL);
> > +	if (!core)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, core);
> > +
> > +	core->config = config;
> > +
> > +	/* Create regmaps and regfields here */
> > +	core->gcb_regmap = ocelot_mfd_regmap_init(core, &vsc7512_gcb_resource);
> > +	if (!core->gcb_regmap)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < GCB_REGFIELD_MAX; i++) {
> > +		regfield = &vsc7512_mfd_gcb_regfields[i];
> > +		core->gcb_regfields[i] =
> > +			devm_regmap_field_alloc(dev, core->gcb_regmap,
> > +						*regfield);
> > +		if (!core->gcb_regfields[i])
> > +			return -ENOMEM;
> > +	}
> > +
> > +	/* Prepare the chip */
> > +	ret = ocelot_mfd_reset(core);
> > +	if (ret) {
> > +		dev_err(dev, "ocelot mfd reset failed with code %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Create and loop over all child devices here */
> 
> These need to all go in now please.

I'll squash them, as I saw you suggested in your other responses. I
tried to keep them separate, especially since adding ocelot_ext to this
commit (which has no functionality until this one) makes it quite a
large single commit. That's why I went the path I did, which was to roll
them in one at a time.

> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_init);
> > +
> > +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> > +{
> > +	/* Loop over all children and remove them */
> 
> Use devm_* then you won't have to.

Yeah, I was more worried than I needed to be when I wrote that comment.
The only thing called to clean everything up is mfd_remove_devices();

> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_remove);
> > +
> > +MODULE_DESCRIPTION("Ocelot Chip MFD driver");
> > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/mfd/ocelot-mfd.h b/drivers/mfd/ocelot-mfd.h
> > new file mode 100644
> > index 000000000000..6af8b8c5a316
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-mfd.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> > + */
> > +
> > +#include <linux/regmap.h>
> > +
> > +struct ocelot_mfd_config {
> > +	struct device *dev;
> > +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> > +				     const struct resource *res,
> > +				     const char *name);
> > +	int (*init_bus)(struct ocelot_mfd_config *config);
> 
> Please re-work and delete this 'config' concept.
> 
> See other drivers in this sub-directory for reference.

Do you have a specific example? I had focused on madera for no specific
reason. But I really dislike the idea of throwing all of the structure
definition for the MFD inside of something like
"include/linux/mfd/ocelot/core.h", especially since all the child
drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 

It seemed to me that without the concept of
"mfd_get_regmap_from_resource" this sort of back-and-forth was actually
necessary.

> 
> > +};
> > +
> > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > +				  int size);
> > +int ocelot_mfd_init(struct ocelot_mfd_config *config);
> > +int ocelot_mfd_remove(struct ocelot_mfd_config *config);
> > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > new file mode 100644
> > index 000000000000..65ceb68f27af
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-spi.c
> > @@ -0,0 +1,374 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> 
> As above.
> 
> > + */
> > +
> > +#include <asm/byteorder.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> 
> As above.

Done.

> > +static int ocelot_spi_probe(struct spi_device *spi)
> > +{
> > +	struct ocelot_spi *ocelot_spi;
> > +	struct device *dev;
> > +	char name[32];
> > +	int err;
> > +
> > +	dev = &spi->dev;
> 
> Put this on the declaration line.
> 
> > +	ocelot_spi = devm_kzalloc(dev, sizeof(struct ocelot_spi),
> 
> sizeof(*ocelot_spi)
> 
> > +				      GFP_KERNEL);
> > +
> 
> No '\n'.

Done. I must've changed a variable name and missed this one. Oops.

> 
> > +	if (!ocelot_spi)
> > +		return -ENOMEM;
> > +
> > +	if (spi->max_speed_hz <= 500000) {
> > +		ocelot_spi->spi_padding_bytes = 0;
> > +	} else {
> > +		/*
> > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> > +		 * on the side of more padding bytes, as having too few can be
> > +		 * difficult to detect at runtime.
> > +		 */
> > +		ocelot_spi->spi_padding_bytes = 1 +
> > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> 
> Please explain what this means or define the values (or both).

I can certainly elaborate the comment. Searching the manual for the term
"if_cfgstat" will take you right to the equation, and a description of
what padding bytes are, etc. 

> 
> > +	}
> > +
> > +	ocelot_spi->spi = spi;
> 
> Why are you saving this?

This file keeps the regmap_{read,write} implementations, so is needed
for spi_sync() for any regmap. There might be a better way to infer
this... but it seemed pretty nice to have each regmap only carry along
an instance of "ocelot_spi_regmap_context."

> 
> > +	ocelot_spi->map = vsc7512_dev_cpuorg_regmap;
> 
> Why not just set up the regmap here?

I think this was fairly unclear, and that is my fault. I have updated 
the header of this file to better explain its purpose.

Basically ocelot-spi is not much more than an interface by which others
can get regmaps. It handles the low-level protocols (endianness, padding
bytes, address translation...) and leaves everything else to the
children.

So when a VSC7511/7513 comes on line, it will probably need to dish out 5
regmaps for just net ports. If it is a 7512/7514, it would probably need
to dish out 11. If PTP is desired... another regmap. Pinctrl, SGPIO,
MDIO... All those are either separate regmaps, shared regmaps, or 
sub-regmaps. Likely occupying either overlapping or identical regions.

"ocelot-spi" is designed to not care about those features. And
"ocelot-core" really isn't either. It is mostly a conduit to say
"here's your regmap" and be on it's merry way.

> 
> > +	spi->bits_per_word = 8;
> > +
> > +	err = spi_setup(spi);
> > +	if (err < 0) {
> > +		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> 
> The error code usually comes at the end.
> 
> > +		return err;
> > +	}
> > +
> > +	dev_info(dev, "configured SPI bus for speed %d, rx padding bytes %d\n",
> > +			spi->max_speed_hz, ocelot_spi->spi_padding_bytes);
> 
> When would this be useful?
> 
> Don't we already have debug interfaces to find this out?

Padding bytes, no. If someone were to put a scope on the SPI lines this
message is likely helpful to see what might be going on. The expectation
would be (from memory) 3 address bytes, followed by spi_padding_bytes of 
0x88888888, then the actual data.

That said, I won't lose any sleep if this one gets cut.

> 
> > +	/* Ensure we have devcpu_org regmap before we call ocelot_mfd_init */
> 
> because ...

That isn't clear. I'm fixing the comment.

Children of ocelot_mfd might require the "dev_cpuorg" regmap region. By
having this region named and registered to the device, child devices
don't need to allocate new regmaps, they can benefit from
dev_get_regmap()

> 
> > +	ocelot_mfd_get_resource_name(name, &vsc7512_dev_cpuorg_resource,
> > +				     sizeof(name) - 1);
> 
> This is an ugly interface.  I think it needs to go.

I agree that it is an ugly interface. It is only exposed so that
ocelot_spi can get the same regmap name as ocelot_mfd->children. It
isn't much better than hard-coding the name. But if I give up the idea
that children can share this regmap, I can take away this interface and
have it not be linked to ocelot_mfd.

> 
> > +	/*
> > +	 * Since we created dev, we know there isn't a regmap, so create one
> > +	 * here directly.
> > +	 */
> 
> Sorry, what 'dev'?  When did we create that?

Unclear comment. 

"Since this is a newly probed device, we know it doesn't have a regmap
so a call to dev_get_regmap isn't necessary - just create it"

> 
> > +	ocelot_spi->cpuorg_regmap =
> > +		ocelot_spi_get_regmap(&ocelot_spi->config,
> > +				      &vsc7512_dev_cpuorg_resource, name);
> > +	if (!ocelot_spi->cpuorg_regmap)
> > +		return -ENOMEM;
> > +
> > +	ocelot_spi->config.init_bus = ocelot_spi_init_bus_from_config;
> > +	ocelot_spi->config.get_regmap = ocelot_spi_get_regmap;
> > +	ocelot_spi->config.dev = dev;
> 
> Please remove this API.

I might need to look at this more. But get_regmap is the main purpose of
ocelot-spi, and init_bus is a necessary call for any SPI communication
after a chip reset. Should I2C, or even MMIO be added, it can be done
without modification to ocelot-core.

> 
> > +	spi_set_drvdata(spi, ocelot_spi);
> > +
> > +	/*
> > +	 * The chip must be set up for SPI before it gets initialized and reset.
> > +	 * Do this once here before calling mfd_init
> > +	 */
> > +	err = ocelot_spi_init_bus(ocelot_spi);
> > +	if (err) {
> > +		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> 
> Doesn't this already print out an error message?

In some cases ocelot_spi_init_bus does. I don't know about whether a
err return from spi_driver->probe does or not.

> 
> > +		return err;
> > +	}
> > +
> > +	err = ocelot_mfd_init(&ocelot_spi->config);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
> > +		return err;
> > +	}
> > +
> > +	dev_info(&spi->dev, "ocelot spi mfd probed\n");
> 
> Please, remove all of these.

Done

> 
> > +	return 0;
> > +}
> > +
> > +static int ocelot_spi_remove(struct spi_device *spi)
> > +{
> > +	struct ocelot_spi *ocelot_spi;
> > +
> > +	ocelot_spi = spi_get_drvdata(spi);
> > +	devm_kfree(&spi->dev, ocelot_spi);
> 
> Why use devm_* if you're going to free anyway?

Good point. It looks like none of this remove is necessary.

> 
> > +	return 0;
> > +}
> > +
> > +const struct of_device_id ocelot_mfd_of_match[] = {
> > +	{ .compatible = "mscc,vsc7514_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7513_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7512_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7511_mfd_spi" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_mfd_of_match);
> > +
> > +static struct spi_driver ocelot_mfd_spi_driver = {
> > +	.driver = {
> > +		.name = "ocelot_mfd_spi",
> > +		.of_match_table = of_match_ptr(ocelot_mfd_of_match),
> > +	},
> > +	.probe = ocelot_spi_probe,
> > +	.remove = ocelot_spi_remove,
> > +};
> > +module_spi_driver(ocelot_mfd_spi_driver);
> > +
> > +MODULE_DESCRIPTION("Ocelot Chip MFD SPI driver");
> > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > +MODULE_LICENSE("Dual MIT/GPL");
> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Colin Foster]

On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote:
> On Sat, 18 Dec 2021, Colin Foster wrote:
> 
> > Some drivers will need to create regmaps differently based on whether they
> > are a child of an MFD or a standalone device. An example of this would be
> > if a regmap were directly memory-mapped or an external bus. In the
> > memory-mapped case a call to devm_regmap_init_mmio would return the correct
> > regmap. In the case of an MFD, the regmap would need to be requested from
> > the parent device.
> > 
> > This addition allows the driver to correctly reason about these scenarios.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/mfd/mfd-core.c   |  5 +++++
> >  include/linux/mfd/core.h | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 684a011a6396..905f508a31b4 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
> >  	.name	= "mfd_device",
> >  };
> >  
> > +int device_is_mfd(struct platform_device *pdev)
> > +{
> > +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> > +}
> > +
> 
> Why is this device different to any other that has ever been
> mainlined?

Hi Lee,

First, let me apologize for not responding to your response from the
related RFC from earlier this month. It had been blocked by my spam
filter and I had not seen it until just now. I'll have to check that
more diligently now.

Moving on...

That's a question I keep asking myself. Either there's something I'm
missing, or there's something new I'm doing.

This is taking existing drivers that work via MMIO regmaps and making
them interface-independent. As Vladimir pointed out here:
https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/
device_is_mfd could be dropped in lieu of an mfd-specific probe
function.

If there's something I'm missing, please let me know. But it feels like
devm_get_regmap_from_resource at the end of the day would be the best
solution to the design, and that doesn't exist. And implementing
something like that is a task that I feel I'm not capable of tackling at
this time.

> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Lee Jones]

On Wed, 29 Dec 2021, Colin Foster wrote:

> On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote:
> > On Sat, 18 Dec 2021, Colin Foster wrote:
> > 
> > > Some drivers will need to create regmaps differently based on whether they
> > > are a child of an MFD or a standalone device. An example of this would be
> > > if a regmap were directly memory-mapped or an external bus. In the
> > > memory-mapped case a call to devm_regmap_init_mmio would return the correct
> > > regmap. In the case of an MFD, the regmap would need to be requested from
> > > the parent device.
> > > 
> > > This addition allows the driver to correctly reason about these scenarios.
> > > 
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> > >  drivers/mfd/mfd-core.c   |  5 +++++
> > >  include/linux/mfd/core.h | 10 ++++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > index 684a011a6396..905f508a31b4 100644
> > > --- a/drivers/mfd/mfd-core.c
> > > +++ b/drivers/mfd/mfd-core.c
> > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
> > >  	.name	= "mfd_device",
> > >  };
> > >  
> > > +int device_is_mfd(struct platform_device *pdev)
> > > +{
> > > +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> > > +}
> > > +
> > 
> > Why is this device different to any other that has ever been
> > mainlined?
> 
> Hi Lee,
> 
> First, let me apologize for not responding to your response from the
> related RFC from earlier this month. It had been blocked by my spam
> filter and I had not seen it until just now. I'll have to check that
> more diligently now.
> 
> Moving on...
> 
> That's a question I keep asking myself. Either there's something I'm
> missing, or there's something new I'm doing.
> 
> This is taking existing drivers that work via MMIO regmaps and making
> them interface-independent. As Vladimir pointed out here:
> https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/
> device_is_mfd could be dropped in lieu of an mfd-specific probe
> function.
> 
> If there's something I'm missing, please let me know. But it feels like
> devm_get_regmap_from_resource at the end of the day would be the best
> solution to the design, and that doesn't exist. And implementing
> something like that is a task that I feel I'm not capable of tackling at
> this time.

I'm really not a fan of leaking any MFD API outside of drivers/mfd.
MFD isn't a tangible thing.  It's a Linuxiusm, something we made up, a
figment of your imagination.

What happens if you were to all dev_get_regmap() in the non-MFD case
or when you call devm_regmap_init_mmio() when the driver was
registered via the MFD framework?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Colin Foster]

On Thu, Dec 30, 2021 at 01:43:53PM +0000, Lee Jones wrote:
> On Wed, 29 Dec 2021, Colin Foster wrote:
> 
> > On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote:
> > > On Sat, 18 Dec 2021, Colin Foster wrote:
> > > 
> > > > Some drivers will need to create regmaps differently based on whether they
> > > > are a child of an MFD or a standalone device. An example of this would be
> > > > if a regmap were directly memory-mapped or an external bus. In the
> > > > memory-mapped case a call to devm_regmap_init_mmio would return the correct
> > > > regmap. In the case of an MFD, the regmap would need to be requested from
> > > > the parent device.
> > > > 
> > > > This addition allows the driver to correctly reason about these scenarios.
> > > > 
> > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > > ---
> > > >  drivers/mfd/mfd-core.c   |  5 +++++
> > > >  include/linux/mfd/core.h | 10 ++++++++++
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > > index 684a011a6396..905f508a31b4 100644
> > > > --- a/drivers/mfd/mfd-core.c
> > > > +++ b/drivers/mfd/mfd-core.c
> > > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
> > > >  	.name	= "mfd_device",
> > > >  };
> > > >  
> > > > +int device_is_mfd(struct platform_device *pdev)
> > > > +{
> > > > +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> > > > +}
> > > > +
> > > 
> > > Why is this device different to any other that has ever been
> > > mainlined?
> > 
> > Hi Lee,
> > 
> > First, let me apologize for not responding to your response from the
> > related RFC from earlier this month. It had been blocked by my spam
> > filter and I had not seen it until just now. I'll have to check that
> > more diligently now.
> > 
> > Moving on...
> > 
> > That's a question I keep asking myself. Either there's something I'm
> > missing, or there's something new I'm doing.
> > 
> > This is taking existing drivers that work via MMIO regmaps and making
> > them interface-independent. As Vladimir pointed out here:
> > https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/
> > device_is_mfd could be dropped in lieu of an mfd-specific probe
> > function.
> > 
> > If there's something I'm missing, please let me know. But it feels like
> > devm_get_regmap_from_resource at the end of the day would be the best
> > solution to the design, and that doesn't exist. And implementing
> > something like that is a task that I feel I'm not capable of tackling at
> > this time.
> 
> I'm really not a fan of leaking any MFD API outside of drivers/mfd.
> MFD isn't a tangible thing.  It's a Linuxiusm, something we made up, a
> figment of your imagination.
> 
> What happens if you were to all dev_get_regmap() in the non-MFD case
> or when you call devm_regmap_init_mmio() when the driver was
> registered via the MFD framework?

I'd imagine dev_get_regmap in a non-MFD case would be the same as
dev_get_and_ioremap_resource() followed by devm_regmap_init_mmio().

In the MFD case it would possibly request the regmap from the parent,
which could reason about how to create the regmap. As you understand,
this is exactly the behavior I created in this patch set. I did it by
way of ocelot_get_regmap_from_resource, and admit it isn't the best way.
But it certainly seems there isn't an existing method that I'm missing.

I'm coming from a pretty narrow field of view, but believe my use-case
is a valid one. If that is true, and there isn't another design I should
use... this is the opportunity to create it. Implementing
ocelot_get_regmap_from_resource is a way to achieve my needs without
affecting anyone else. 

Going one step further and implementing mfd_get_regmap_from_parent (or
similar) would creep into the design of MFD. I don't know enough about
MFD and the users to suggest this. I wouldn't want to start venturing
down that path without blessing from the community. And this would
indirectly affect every MFD driver.

Going all in and implementing device_get_regmap_from_resource... I don't
know that I'd be comfortable even starting down that path knowing that
it would affect every device. Perhaps it would have to utilize something
like IORESOURCE_REG that seems to only get utilized in a handful of 
files:
https://elixir.bootlin.com/linux/v5.16-rc7/C/ident/IORESOURCE_REG

> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Lee Jones]

On Wed, 29 Dec 2021, Colin Foster wrote:
> On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:

[...]

> > > +	tristate "Microsemi Ocelot External Control Support"
> > > +	select MFD_CORE
> > > +	help
> > > +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> > > +	  VSC7513, VSC7514) controlled externally.
> > 
> > Please describe the device in more detail here.
> > 
> > I'm not sure what an "External Control Support" is.
> 
> A second paragraph "All four of these chips can be controlled internally
> (MMIO) or externally via SPI, I2C, PCIe. This enables control of these
> chips over one or more of these buses"

Where?  Or do you mean that you'll add one?

> > Please remove the term 'mfd\|MFD' from everywhere.
> 
> "ocelot_init" conflicts with a symbol in
> drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
> now.

Then rename the other one.  Or call this one 'core', or something.

> > > +struct ocelot_mfd_core {
> > > +	struct ocelot_mfd_config *config;
> > > +	struct regmap *gcb_regmap;
> > > +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> > > +};
> > 
> > Not sure about this at all.
> > 
> > Which driver did you take your inspiration from?
> 
> Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.

I doubt you need it.  Please try to remove it.

> > > +static const struct resource vsc7512_gcb_resource = {
> > > +	.start	= 0x71070000,
> > > +	.end	= 0x7107022b,
> > 
> > No magic numbers please.
> 
> I've gotten conflicting feedback on this. Several of the ocelot drivers
> (drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
> Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
> all passed in through the device tree. 
> 
> https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/

Ref or quote?

I'm not brain grepping it searching for what you might be referring to.

I'm not sure what you're trying to say here.  I'm asking you to define
this numbers please.

> > > +	.name	= "devcpu_gcb",
> > 
> > What is a 'devcpu_gcb'?
> 
> It matches the datasheet of the CPU's general configuation block.

Please could you quote that part for me?

> > > +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
> > 
> > No magic numbers please.  I have no idea what this is doing.
> 
> I'm not sure how much more verbose it can be... I suppose a define for
> "RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
> blinded by having stared at this code for the last several months.

Yes please.  '1' could mean anything.

'CLEAR' is just as opaque.

What actually happens when you clear that register bit?

> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> > > +	 * suggest how to do a reset over SPI, and the register strategy isn't
> > > +	 * possible.
> > > +	 */
> > > +	msleep(100);
> > > +
> > > +	ret = core->config->init_bus(core->config);
> > 
> > You're not writing a bus.  I doubt you need ops call-backs.
> 
> In the case of SPI, the chip needs to be configured both before and
> after reset. It sets up the bus for endianness, padding bytes, etc. This
> is currently achieved by running "ocelot_spi_init_bus" once during SPI
> probe, and once immediately after chip reset.
> 
> For other control mediums I doubt this is necessary. Perhaps "init_bus"
> is a misnomer in this scenario...

Please find a clearer way to do this without function pointers.

> > > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > > +				  int size)
> > > +{
> > > +	if (res->name)
> > > +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> > > +	else
> > > +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> > > +}
> > > +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
> > 
> > What is this used for?
> > 
> > You should not be hand rolling device resource names like this.
> > 
> > This sort of code belongs in the bus/class API.
> > 
> > Please use those instead.
> 
> The idea here was to allow shared regmaps across different devices. The
> "devcpu_gcb" might be used in two ways - either everyone shares the same
> regmap across the "GCB" range, or everyone creates their own. 
> 
> This was more useful when the ocelot-core.c had a copy of the 
> "devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
> remove that, but also feel like the full switch driver (patch 6 of this
> set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
> ocelot-core does.
> 
> Admittedly, there are complications. There should probably be more
> checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
> regmap for ocelot_ext exactly matches the existing regmap for
> ocelot_core.
> 
> There's yet another complexity with these, and I'm not sure what the
> answer is. Currently all regmaps are tied to the ocelot_spi device...
> ocelot_spi calls devm_regmap_init. So those regmaps hang around if
> they're created by a module that has been removed. At least until the
> entire MFD module is removed. Maybe there's something I haven't seen yet
> where the devres or similar has a reference count. I don't know the best
> path forward on this one.

Why are you worrying about creating them 2 different ways?

If it's possible for them to all create and use their own regmaps,
what's preventing you from just do that all the time?

> > > +	/* Create and loop over all child devices here */
> > 
> > These need to all go in now please.
> 
> I'll squash them, as I saw you suggested in your other responses. I
> tried to keep them separate, especially since adding ocelot_ext to this
> commit (which has no functionality until this one) makes it quite a
> large single commit. That's why I went the path I did, which was to roll
> them in one at a time.

This is not an MFD until they are present.

> > > +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> > > +{
> > > +	/* Loop over all children and remove them */
> > 
> > Use devm_* then you won't have to.
> 
> Yeah, I was more worried than I needed to be when I wrote that comment.
> The only thing called to clean everything up is mfd_remove_devices();

Use devm_mfd_add_devices(), then you don't have to.

[...]

> > > +#include <linux/regmap.h>
> > > +
> > > +struct ocelot_mfd_config {
> > > +	struct device *dev;
> > > +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> > > +				     const struct resource *res,
> > > +				     const char *name);
> > > +	int (*init_bus)(struct ocelot_mfd_config *config);
> > 
> > Please re-work and delete this 'config' concept.
> > 
> > See other drivers in this sub-directory for reference.
> 
> Do you have a specific example? I had focused on madera for no specific
> reason. But I really dislike the idea of throwing all of the structure
> definition for the MFD inside of something like
> "include/linux/mfd/ocelot/core.h", especially since all the child
> drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 
> 
> It seemed to me that without the concept of
> "mfd_get_regmap_from_resource" this sort of back-and-forth was actually
> necessary.

Things like regmaps are usually passed in via driver_data or
platform_data.  Almost anything is better than call-backs.

[...]

> > > +	if (!ocelot_spi)
> > > +		return -ENOMEM;
> > > +
> > > +	if (spi->max_speed_hz <= 500000) {
> > > +		ocelot_spi->spi_padding_bytes = 0;
> > > +	} else {
> > > +		/*
> > > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> > > +		 * on the side of more padding bytes, as having too few can be
> > > +		 * difficult to detect at runtime.
> > > +		 */
> > > +		ocelot_spi->spi_padding_bytes = 1 +
> > > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> > 
> > Please explain what this means or define the values (or both).
> 
> I can certainly elaborate the comment. Searching the manual for the term
> "if_cfgstat" will take you right to the equation, and a description of
> what padding bytes are, etc. 

You shouldn't insist for your readers to RTFM.

If the code doesn't read well or is overly complicated, change it.

If the complexity is required, document it in comments.

> > > +	ocelot_spi->spi = spi;
> > 
> > Why are you saving this?
> 
> This file keeps the regmap_{read,write} implementations, so is needed
> for spi_sync() for any regmap. There might be a better way to infer
> this... but it seemed pretty nice to have each regmap only carry along
> an instance of "ocelot_spi_regmap_context."

I still need Mark to look at your Regmap implementation.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 08/13] mfd: add interface to check whether a device is mfd

[Lee Jones]

On Thu, 30 Dec 2021, Colin Foster wrote:

> On Thu, Dec 30, 2021 at 01:43:53PM +0000, Lee Jones wrote:
> > On Wed, 29 Dec 2021, Colin Foster wrote:
> > 
> > > On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote:
> > > > On Sat, 18 Dec 2021, Colin Foster wrote:
> > > > 
> > > > > Some drivers will need to create regmaps differently based on whether they
> > > > > are a child of an MFD or a standalone device. An example of this would be
> > > > > if a regmap were directly memory-mapped or an external bus. In the
> > > > > memory-mapped case a call to devm_regmap_init_mmio would return the correct
> > > > > regmap. In the case of an MFD, the regmap would need to be requested from
> > > > > the parent device.
> > > > > 
> > > > > This addition allows the driver to correctly reason about these scenarios.
> > > > > 
> > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > > > ---
> > > > >  drivers/mfd/mfd-core.c   |  5 +++++
> > > > >  include/linux/mfd/core.h | 10 ++++++++++
> > > > >  2 files changed, 15 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > > > index 684a011a6396..905f508a31b4 100644
> > > > > --- a/drivers/mfd/mfd-core.c
> > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = {
> > > > >  	.name	= "mfd_device",
> > > > >  };
> > > > >  
> > > > > +int device_is_mfd(struct platform_device *pdev)
> > > > > +{
> > > > > +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> > > > > +}
> > > > > +
> > > > 
> > > > Why is this device different to any other that has ever been
> > > > mainlined?
> > > 
> > > Hi Lee,
> > > 
> > > First, let me apologize for not responding to your response from the
> > > related RFC from earlier this month. It had been blocked by my spam
> > > filter and I had not seen it until just now. I'll have to check that
> > > more diligently now.
> > > 
> > > Moving on...
> > > 
> > > That's a question I keep asking myself. Either there's something I'm
> > > missing, or there's something new I'm doing.
> > > 
> > > This is taking existing drivers that work via MMIO regmaps and making
> > > them interface-independent. As Vladimir pointed out here:
> > > https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/
> > > device_is_mfd could be dropped in lieu of an mfd-specific probe
> > > function.
> > > 
> > > If there's something I'm missing, please let me know. But it feels like
> > > devm_get_regmap_from_resource at the end of the day would be the best
> > > solution to the design, and that doesn't exist. And implementing
> > > something like that is a task that I feel I'm not capable of tackling at
> > > this time.
> > 
> > I'm really not a fan of leaking any MFD API outside of drivers/mfd.
> > MFD isn't a tangible thing.  It's a Linuxiusm, something we made up, a
> > figment of your imagination.
> > 
> > What happens if you were to all dev_get_regmap() in the non-MFD case
> > or when you call devm_regmap_init_mmio() when the driver was
> > registered via the MFD framework?
> 
> I'd imagine dev_get_regmap in a non-MFD case would be the same as
> dev_get_and_ioremap_resource() followed by devm_regmap_init_mmio().
> 
> In the MFD case it would possibly request the regmap from the parent,
> which could reason about how to create the regmap. As you understand,
> this is exactly the behavior I created in this patch set. I did it by
> way of ocelot_get_regmap_from_resource, and admit it isn't the best way.
> But it certainly seems there isn't an existing method that I'm missing.
> 
> I'm coming from a pretty narrow field of view, but believe my use-case
> is a valid one. If that is true, and there isn't another design I should
> use... this is the opportunity to create it. Implementing
> ocelot_get_regmap_from_resource is a way to achieve my needs without
> affecting anyone else. 
> 
> Going one step further and implementing mfd_get_regmap_from_parent (or
> similar) would creep into the design of MFD. I don't know enough about
> MFD and the users to suggest this. I wouldn't want to start venturing
> down that path without blessing from the community. And this would
> indirectly affect every MFD driver.
> 
> Going all in and implementing device_get_regmap_from_resource... I don't
> know that I'd be comfortable even starting down that path knowing that
> it would affect every device. Perhaps it would have to utilize something
> like IORESOURCE_REG that seems to only get utilized in a handful of 
> files:
> https://elixir.bootlin.com/linux/v5.16-rc7/C/ident/IORESOURCE_REG

Let's speak to Mark and see if he can provide any insight.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

Hi Lee,

Thank you for all your feedback. I expect to create another RFC in the
next week or two with all the changes you suggest.

On Mon, Jan 10, 2022 at 12:16:59PM +0000, Lee Jones wrote:
> On Wed, 29 Dec 2021, Colin Foster wrote:
> > On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:
> 
> [...]
> 
> > > > +	tristate "Microsemi Ocelot External Control Support"
> > > > +	select MFD_CORE
> > > > +	help
> > > > +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> > > > +	  VSC7513, VSC7514) controlled externally.
> > > 
> > > Please describe the device in more detail here.
> > > 
> > > I'm not sure what an "External Control Support" is.
> > 
> > A second paragraph "All four of these chips can be controlled internally
> > (MMIO) or externally via SPI, I2C, PCIe. This enables control of these
> > chips over one or more of these buses"
> 
> Where?  Or do you mean that you'll add one?

I meant I added one. Sorry that wasn't clear.

> 
> > > Please remove the term 'mfd\|MFD' from everywhere.
> > 
> > "ocelot_init" conflicts with a symbol in
> > drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
> > now.
> 
> Then rename the other one.  Or call this one 'core', or something.

I'll add "core" or something to this one.

> 
> > > > +struct ocelot_mfd_core {
> > > > +	struct ocelot_mfd_config *config;
> > > > +	struct regmap *gcb_regmap;
> > > > +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> > > > +};
> > > 
> > > Not sure about this at all.
> > > 
> > > Which driver did you take your inspiration from?
> > 
> > Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.
> 
> I doubt you need it.  Please try to remove it.

Fair point. I'll remove it here.

> 
> > > > +static const struct resource vsc7512_gcb_resource = {
> > > > +	.start	= 0x71070000,
> > > > +	.end	= 0x7107022b,
> > > 
> > > No magic numbers please.
> > 
> > I've gotten conflicting feedback on this. Several of the ocelot drivers
> > (drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
> > Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
> > all passed in through the device tree. 
> > 
> > https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/
> 
> Ref or quote?
> 
> I'm not brain grepping it searching for what you might be referring to.
> 
> I'm not sure what you're trying to say here.  I'm asking you to define
> this numbers please.

I'll define the numbers as you suggest.


The quote I was referring to is this:

> The last option I haven't put much consideration toward would be to
> move some of the decision making to the device tree. The main ocelot
> driver appears to leave a lot of these addresses out. For instance
> Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
> That added DT complexity could remove needs for lines like this:
> > > +              ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> But that would probably impose DT changes on Seville and Felix, which
> is the last thing I want to do.

The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs.

> 
> > > > +	.name	= "devcpu_gcb",
> > > 
> > > What is a 'devcpu_gcb'?
> > 
> > It matches the datasheet of the CPU's general configuation block.
> 
> Please could you quote that part for me?

Hmm... I'm not sure exactly what you mean by this.

https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf

There are 64 instances of "DEVCPU_GCB" in the main datasheet. 
Chapter 6 of this PDF has an attached PDF around the phrase "Information
about the registers for this product is available in the attached file"

Section 2.3 of that attached PDF is dedicated entirely to DEVCPU_GCB 
registers. Table 1 defines that register block starting at 0x71070000.
The entry from that table is
| DEVCPU_GCB | 0x71070000 | General configuration block. | Page 63 |
This document has 175 references to the phrase "DEVCPU_GCB".

> 
> > > > +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
> > > 
> > > No magic numbers please.  I have no idea what this is doing.
> > 
> > I'm not sure how much more verbose it can be... I suppose a define for
> > "RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
> > blinded by having stared at this code for the last several months.
> 
> Yes please.  '1' could mean anything.
> 
> 'CLEAR' is just as opaque.
> 
> What actually happens when you clear that register bit?

Agreed. I'll fix this.

> 
> > > 
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> > > > +	 * suggest how to do a reset over SPI, and the register strategy isn't
> > > > +	 * possible.
> > > > +	 */
> > > > +	msleep(100);
> > > > +
> > > > +	ret = core->config->init_bus(core->config);
> > > 
> > > You're not writing a bus.  I doubt you need ops call-backs.
> > 
> > In the case of SPI, the chip needs to be configured both before and
> > after reset. It sets up the bus for endianness, padding bytes, etc. This
> > is currently achieved by running "ocelot_spi_init_bus" once during SPI
> > probe, and once immediately after chip reset.
> > 
> > For other control mediums I doubt this is necessary. Perhaps "init_bus"
> > is a misnomer in this scenario...
> 
> Please find a clearer way to do this without function pointers.

Understood.

> 
> > > > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > > > +				  int size)
> > > > +{
> > > > +	if (res->name)
> > > > +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> > > > +	else
> > > > +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
> > > 
> > > What is this used for?
> > > 
> > > You should not be hand rolling device resource names like this.
> > > 
> > > This sort of code belongs in the bus/class API.
> > > 
> > > Please use those instead.
> > 
> > The idea here was to allow shared regmaps across different devices. The
> > "devcpu_gcb" might be used in two ways - either everyone shares the same
> > regmap across the "GCB" range, or everyone creates their own. 
> > 
> > This was more useful when the ocelot-core.c had a copy of the 
> > "devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
> > remove that, but also feel like the full switch driver (patch 6 of this
> > set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
> > ocelot-core does.
> > 
> > Admittedly, there are complications. There should probably be more
> > checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
> > regmap for ocelot_ext exactly matches the existing regmap for
> > ocelot_core.
> > 
> > There's yet another complexity with these, and I'm not sure what the
> > answer is. Currently all regmaps are tied to the ocelot_spi device...
> > ocelot_spi calls devm_regmap_init. So those regmaps hang around if
> > they're created by a module that has been removed. At least until the
> > entire MFD module is removed. Maybe there's something I haven't seen yet
> > where the devres or similar has a reference count. I don't know the best
> > path forward on this one.
> 
> Why are you worrying about creating them 2 different ways?
> 
> If it's possible for them to all create and use their own regmaps,
> what's preventing you from just do that all the time?

There isn't really any worry, there just might be efficiencies to be
had if two children share the same regmap. But so long as any regmap is
created with unique names, there's no reason multiple regmaps can't
overlap the same regions. In those cases, maybe syscon would be the best
thing to implement if it becomes needed.

I have nothing against making every child regmap be unique if that's the
desire.

> 
> > > > +	/* Create and loop over all child devices here */
> > > 
> > > These need to all go in now please.
> > 
> > I'll squash them, as I saw you suggested in your other responses. I
> > tried to keep them separate, especially since adding ocelot_ext to this
> > commit (which has no functionality until this one) makes it quite a
> > large single commit. That's why I went the path I did, which was to roll
> > them in one at a time.
> 
> This is not an MFD until they are present.

Sounds good. I'll squash before the next RFC.

> 
> > > > +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> > > > +{
> > > > +	/* Loop over all children and remove them */
> > > 
> > > Use devm_* then you won't have to.
> > 
> > Yeah, I was more worried than I needed to be when I wrote that comment.
> > The only thing called to clean everything up is mfd_remove_devices();
> 
> Use devm_mfd_add_devices(), then you don't have to.
> 
> [...]

Ooh. Thanks!

> 
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +struct ocelot_mfd_config {
> > > > +	struct device *dev;
> > > > +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> > > > +				     const struct resource *res,
> > > > +				     const char *name);
> > > > +	int (*init_bus)(struct ocelot_mfd_config *config);
> > > 
> > > Please re-work and delete this 'config' concept.
> > > 
> > > See other drivers in this sub-directory for reference.
> > 
> > Do you have a specific example? I had focused on madera for no specific
> > reason. But I really dislike the idea of throwing all of the structure
> > definition for the MFD inside of something like
> > "include/linux/mfd/ocelot/core.h", especially since all the child
> > drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 
> > 
> > It seemed to me that without the concept of
> > "mfd_get_regmap_from_resource" this sort of back-and-forth was actually
> > necessary.
> 
> Things like regmaps are usually passed in via driver_data or
> platform_data.  Almost anything is better than call-backs.
> 
> [...]

I'll work to clean this up for the next RFC.

> 
> > > > +	if (!ocelot_spi)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	if (spi->max_speed_hz <= 500000) {
> > > > +		ocelot_spi->spi_padding_bytes = 0;
> > > > +	} else {
> > > > +		/*
> > > > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> > > > +		 * on the side of more padding bytes, as having too few can be
> > > > +		 * difficult to detect at runtime.
> > > > +		 */
> > > > +		ocelot_spi->spi_padding_bytes = 1 +
> > > > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> > > 
> > > Please explain what this means or define the values (or both).
> > 
> > I can certainly elaborate the comment. Searching the manual for the term
> > "if_cfgstat" will take you right to the equation, and a description of
> > what padding bytes are, etc. 
> 
> You shouldn't insist for your readers to RTFM.
> 
> If the code doesn't read well or is overly complicated, change it.
> 
> If the complexity is required, document it in comments.

Understood. I'll elaborate.

> 
> > > > +	ocelot_spi->spi = spi;
> > > 
> > > Why are you saving this?
> > 
> > This file keeps the regmap_{read,write} implementations, so is needed
> > for spi_sync() for any regmap. There might be a better way to infer
> > this... but it seemed pretty nice to have each regmap only carry along
> > an instance of "ocelot_spi_regmap_context."
> 
> I still need Mark to look at your Regmap implementation.

Thank you. And again, I appreciate all the feedback.

> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Lee Jones]

> > > > No magic numbers please.
> > > 
> > > I've gotten conflicting feedback on this. Several of the ocelot drivers
> > > (drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
> > > Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
> > > all passed in through the device tree. 
> > > 
> > > https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/
> > 
> > Ref or quote?
> > 
> > I'm not brain grepping it searching for what you might be referring to.
> > 
> > I'm not sure what you're trying to say here.  I'm asking you to define
> > this numbers please.
> 
> I'll define the numbers as you suggest.
> 
> The quote I was referring to is this:
> 
> > The last option I haven't put much consideration toward would be to
> > move some of the decision making to the device tree. The main ocelot
> > driver appears to leave a lot of these addresses out. For instance
> > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
> > That added DT complexity could remove needs for lines like this:
> > > > +              ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> > But that would probably impose DT changes on Seville and Felix, which
> > is the last thing I want to do.
> 
> The thing with putting the targets in the device tree is that you're
> inflicting yourself unnecessary pain. Take a look at
> Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
> they mark the "ptp" target as optional because it wasn't needed when
> they first published the device tree, and now they need to maintain
> compatibility with those old blobs.

I wasn't asking you to put it in DT, just to define the numbers.

> > > There's yet another complexity with these, and I'm not sure what the
> > > answer is. Currently all regmaps are tied to the ocelot_spi device...
> > > ocelot_spi calls devm_regmap_init. So those regmaps hang around if
> > > they're created by a module that has been removed. At least until the
> > > entire MFD module is removed. Maybe there's something I haven't seen yet
> > > where the devres or similar has a reference count. I don't know the best
> > > path forward on this one.
> > 
> > Why are you worrying about creating them 2 different ways?
> > 
> > If it's possible for them to all create and use their own regmaps,
> > what's preventing you from just do that all the time?
> 
> There isn't really any worry, there just might be efficiencies to be
> had if two children share the same regmap. But so long as any regmap is
> created with unique names, there's no reason multiple regmaps can't
> overlap the same regions. In those cases, maybe syscon would be the best
> thing to implement if it becomes needed.
> 
> I have nothing against making every child regmap be unique if that's the
> desire.

Unless something has changed or my understanding is not correct,
regmap does not support over-lapping register ranges.

However, even if that is required, I still think we can come up with
something cleaner than creating a whole API based around creating
and fetching different regmap configurations depending on how the
system was initialised.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Tue, Jan 11, 2022 at 10:13:43AM +0000, Lee Jones wrote:

> Unless something has changed or my understanding is not correct,
> regmap does not support over-lapping register ranges.

If there's no caches and we're always going direct to hardware it will
work a lot of the time since the buses generally have concurrency
protection at the lowest level, though if the drivers ever do any
read/modify/write operations the underlying hardware bus isn't going to
know about it so you could get data corruption if two drivers decide to
try to operate on the same register.  If there's caches things will
probably go badly since the cache will tend to amplify the
read/modify/write issues.

> However, even if that is required, I still think we can come up with
> something cleaner than creating a whole API based around creating
> and fetching different regmap configurations depending on how the
> system was initialised.

Yeah, I'd expect the usual pattern is to have wrapper drivers that
instantiate a regmap then have the bulk of the driver be a library that
they call into should work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

Hi Mark and Lee,

On Tue, Jan 11, 2022 at 01:44:28PM +0000, Mark Brown wrote:
> On Tue, Jan 11, 2022 at 10:13:43AM +0000, Lee Jones wrote:
> 
> > Unless something has changed or my understanding is not correct,
> > regmap does not support over-lapping register ranges.
> 
> If there's no caches and we're always going direct to hardware it will
> work a lot of the time since the buses generally have concurrency
> protection at the lowest level, though if the drivers ever do any
> read/modify/write operations the underlying hardware bus isn't going to
> know about it so you could get data corruption if two drivers decide to
> try to operate on the same register.  If there's caches things will
> probably go badly since the cache will tend to amplify the
> read/modify/write issues.

Good point about caches. No, nothing in these drivers utilize caches
currently, but that doesn't mean it shouldn't... or won't. Any
concurrency in this specific case would be around the SPI bus.

I think the "overlapping regmaps" already exist in the current drivers... 
but actually I'm not so sure anymore.

Either way, this is helping nudge me in the right direction.

> 
> > However, even if that is required, I still think we can come up with
> > something cleaner than creating a whole API based around creating
> > and fetching different regmap configurations depending on how the
> > system was initialised.
> 
> Yeah, I'd expect the usual pattern is to have wrapper drivers that
> instantiate a regmap then have the bulk of the driver be a library that
> they call into should work.

Understood. And I think this can make sense and clean things up. The
"ocelot_core" mfd will register every regmap range, regardless of
whether any child actually uses them. Every child can then get regmaps
by name, via dev_get_regmap. That'll get rid of the back-and-forth
regmap hooks.

I think I know where to go from here. Thank you both! I'll send along
another RFC soon, once I can get this all cleaned up.

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Lee Jones]

On Tue, 11 Jan 2022, Colin Foster wrote:

> Hi Mark and Lee,
> 
> On Tue, Jan 11, 2022 at 01:44:28PM +0000, Mark Brown wrote:
> > On Tue, Jan 11, 2022 at 10:13:43AM +0000, Lee Jones wrote:
> > 
> > > Unless something has changed or my understanding is not correct,
> > > regmap does not support over-lapping register ranges.
> > 
> > If there's no caches and we're always going direct to hardware it will
> > work a lot of the time since the buses generally have concurrency
> > protection at the lowest level, though if the drivers ever do any
> > read/modify/write operations the underlying hardware bus isn't going to
> > know about it so you could get data corruption if two drivers decide to
> > try to operate on the same register.  If there's caches things will
> > probably go badly since the cache will tend to amplify the
> > read/modify/write issues.
> 
> Good point about caches. No, nothing in these drivers utilize caches
> currently, but that doesn't mean it shouldn't... or won't. Any
> concurrency in this specific case would be around the SPI bus.
> 
> I think the "overlapping regmaps" already exist in the current drivers... 
> but actually I'm not so sure anymore.
> 
> Either way, this is helping nudge me in the right direction.
> 
> > 
> > > However, even if that is required, I still think we can come up with
> > > something cleaner than creating a whole API based around creating
> > > and fetching different regmap configurations depending on how the
> > > system was initialised.
> > 
> > Yeah, I'd expect the usual pattern is to have wrapper drivers that
> > instantiate a regmap then have the bulk of the driver be a library that
> > they call into should work.
> 
> Understood. And I think this can make sense and clean things up. The
> "ocelot_core" mfd will register every regmap range, regardless of
> whether any child actually uses them. Every child can then get regmaps
> by name, via dev_get_regmap. That'll get rid of the back-and-forth
> regmap hooks.

I was under the impression that MFD would not always be used?

Didn't you have a use-case where the child devices could be used
independently of anything else?

If not, why don't you just register a single Regmap covering the whole
range?  Then let the Regmap API deal with the concurrency handling.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

Hi Lee,

On Tue, Jan 11, 2022 at 05:00:11PM +0000, Lee Jones wrote:
> On Tue, 11 Jan 2022, Colin Foster wrote:
> 
> > Hi Mark and Lee,
> > 
> > > 
> > > > However, even if that is required, I still think we can come up with
> > > > something cleaner than creating a whole API based around creating
> > > > and fetching different regmap configurations depending on how the
> > > > system was initialised.
> > > 
> > > Yeah, I'd expect the usual pattern is to have wrapper drivers that
> > > instantiate a regmap then have the bulk of the driver be a library that
> > > they call into should work.
> > 
> > Understood. And I think this can make sense and clean things up. The
> > "ocelot_core" mfd will register every regmap range, regardless of
> > whether any child actually uses them. Every child can then get regmaps
> > by name, via dev_get_regmap. That'll get rid of the back-and-forth
> > regmap hooks.
> 
> I was under the impression that MFD would not always be used?
> 
> Didn't you have a use-case where the child devices could be used
> independently of anything else?
> 
> If not, why don't you just register a single Regmap covering the whole
> range?  Then let the Regmap API deal with the concurrency handling.

That's exactly the use-case I was considering.

An example:
"mscc,ocelot-miim" exists. It can currently be used in two different
scenarios: directly with devicetree, or indirectly as in
drivers/net/dsa/ocelot/seville_vsc9953.c

mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus",
                ocelot->targets[GCB],
                ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK]);

The "GCB_MIIM_MII_STATUS" parameter is the offset from the base for that
regmap. See commit (b99658452355 "net: dsa: ocelot: felix: utilize
shared mscc-miim driver for indirect…")

(My apologies if the formatting of that commit refernece is incorrect)

But in that case... the Seville driver makes a devcpu_gcb regmap located
at 0x71070000. That regmap is created over the entire "GCB range". That
gets passed into the mscc-miim driver, along with the base register
location of the MIIM periperal.

At the same time, mscc-miim can be probed independently, at which point it
would create a smaller regmap at 0x7107009c
(Documentation/devicetree/bindings/mscc-miim.txt)

So the mscc-miim driver supports multiple use-cases. I expect the same
type of "offset" idea can be reasonably added to the following drivers,
all of which already exist but need to support the same type of
use-case:

mscc,ocelot-pinctrl, mscc,ocelot-sgpio, mscc,ocelot-miim, and
mscc,vsc7514-serdes. As I'm bringing up different parts of the hardware,
there might be more components that become necessary.

With the exception of vsc7514-serdes, those all exist outside of MFD.
The vsc7512-serdes driver currently relies on syscon / MFD, which adds a
different complexity. One that I think probably merits a separate probe
function.

> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Alexandre Belloni]

On 11/01/2022 10:28:15-0800, Colin Foster wrote:
> At the same time, mscc-miim can be probed independently, at which point it
> would create a smaller regmap at 0x7107009c
> (Documentation/devicetree/bindings/mscc-miim.txt)
> 
> So the mscc-miim driver supports multiple use-cases. I expect the same
> type of "offset" idea can be reasonably added to the following drivers,
> all of which already exist but need to support the same type of
> use-case:
> 
> mscc,ocelot-pinctrl, mscc,ocelot-sgpio, mscc,ocelot-miim, and
> mscc,vsc7514-serdes. As I'm bringing up different parts of the hardware,
> there might be more components that become necessary.

Indeed, I guess at some point you'll need the irqchip driver too. Until
now, what I did was handling the irq controller inside the mfd driver as
reusing the irqchip driver is not trivial. Ths create a bit of code
duplication but it is not that bad.

> 
> With the exception of vsc7514-serdes, those all exist outside of MFD.
> The vsc7512-serdes driver currently relies on syscon / MFD, which adds a
> different complexity. One that I think probably merits a separate probe
> function.
> 
> > 
> > -- 
> > Lee Jones [李琼斯]
> > Principal Technical Lead - Developer Services
> > Linaro.org │ Open source software for Arm SoCs
> > Follow Linaro: Facebook | Twitter | Blog

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster]

On Tue, Jan 11, 2022 at 10:28:15AM -0800, Colin Foster wrote:
> Hi Lee,
> 
> On Tue, Jan 11, 2022 at 05:00:11PM +0000, Lee Jones wrote:
> > On Tue, 11 Jan 2022, Colin Foster wrote:
> > 
> > > Hi Mark and Lee,
> > > 
> > > > 
> > > > > However, even if that is required, I still think we can come up with
> > > > > something cleaner than creating a whole API based around creating
> > > > > and fetching different regmap configurations depending on how the
> > > > > system was initialised.
> > > > 
> > > > Yeah, I'd expect the usual pattern is to have wrapper drivers that
> > > > instantiate a regmap then have the bulk of the driver be a library that
> > > > they call into should work.

I'm re-reading this with a fresh set of eyes. I've been jumping back and
forth between the relatively small drivers (sgpio, pinctrl) and more
complex ones (felix). I was looking at this from the narrow scope of the
smaller drivers, which typically only handle a small regmap... only a
handful of registers each. For those, there's no problem, and is pretty
much what I've done.

The Felix driver is different. Currently the VSC7512 has to allocate 20
different regmaps. Nine for different features, some optional. 11 for
each of the different ports. The VSC7511 will likely have 19 different
ones because their ranges might not be identical. Same with the 7513,
7514.

In this example code, the resources are getting defined and allocated
entirely within the felix system itself:

(drivers/net/dsa/ocelot/felix_vsc9959.c):
static const struct resource vsc9959_target_io_res[TARGET_MAX] = {
        [ANA] = {
                .start  = 0x0280000,
                .end    = 0x028ffff,
                .name   = "ana",
        },
        [QS] = {
                .start  = 0x0080000,
                .end    = 0x00800ff,
                .name   = "qs",
        },
        ...
};

(drivers/net/dsa/ocelot/felix.c):
for (i = 0; i < TARGET_MAX; i++) {
        struct regmap *target;

        if (!felix->info->target_io_res[i].name)
                continue;

        memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
        res.flags = IORESOURCE_MEM;
        res.start += felix->switch_base;
        res.end += felix->switch_base;

        target = felix->info->init_regmap(ocelot, &res);
        if (IS_ERR(target)) {
                dev_err(ocelot->dev,
                        "Failed to map device memory space\n");
                kfree(port_phy_modes);
                return PTR_ERR(target);
        }

        ocelot->targets[i] = target;
}

So Felix will say "give me regmaps from this array of resources."
Resources have been added as development of Felix has progressed - in
this type of scenario they should be able to do exactly that without
having to "pre-register" with MFD. More specifically: why should adding
precision time protocol to drivers/net/dsa/felix/ocelot_ext.c have any
effect on drivers/mfd/ocelot-core.c?

The patch that I submitted utilized the function
ocelot_get_regmap_from_resource. Does this qualify as a "wrapper driver
that instantates a regmap"? The more I think about it, the more I think
that's exacly what the current implementation is. It just creates
regmaps for all the child devices... and it creates a lot of regmaps...
and it will have a lot of child devices...

Maybe something will come to me in the next week or two - clearly I'm
prone to changing my mind. But in the meantime I'll focus on cleaning up
the rest of the changes that were suggested and prepare a new RFC.

Thanks, and I'm looking forward to continuing work on this for
(hopefully) 5.18!

Colin Foster