[RFC v7 net-next 00/13] add support for VSC7512 control over SPI

[RFC v7 net-next 00/13] add support for VSC7512 control over SPI

[Colin Foster]

The patch set in general is to add support for the VSC7512, and
eventually the VSC7511, 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. It is not yet functional for SGMII,
QSGMII, and SerDes ports.

I have mentioned previously:
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 relevant sections of the device tree I'm using for the VSC7512 is
below. Notably the SGPIO LEDs follow link status and speed from network
triggers.

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.


Of note: The Felix driver had the ability to register the internal MDIO
bus. I am no longer using that in the switch driver, it is now an
additional sub-device under the MFD.

I also made use of IORESOURCE_REG, which removed the "device_is_mfd"
requirement.


/ {
	vscleds {
		compatible = "gpio-leds";
		vscled@0 {
			label = "port0led";
			gpios = <&sgpio_out1 0 0 GPIO_ACTIVE_LOW>;
			default-state = "off";
			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:link";
		};
		vscled@1 {
			label = "port0led1";
			gpios = <&sgpio_out1 0 1 GPIO_ACTIVE_LOW>;
			default-state = "off";
			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:1Gbps";
		};
[ ... ]
		vscled@71 {
			label = "port7led1";
			gpios = <&sgpio_out1 7 1 GPIO_ACTIVE_LOW>;
			default-state = "off";
			linux,default-trigger = "ocelot-miim1-mii:07:1Gbps";
		};
	};
};

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

	ocelot-chip@0 {
		compatible = "mscc,vsc7512_mfd_spi";
		spi-max-frequency = <2500000>;
		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";
				};
			};
		};

		mdio0: mdio0@0 {
			compatible = "mscc,ocelot-miim";
			#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@1 {
			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";
			};
		};

		sgpio: sgpio {
			compatible = "mscc,ocelot-sgpio";
			#address-cells = <1>;
			#size-cells = <0>;
			bus-frequency=<12500000>;
			clocks = <&ocelot_clock>;
			microchip,sgpio-port-ranges = <0 15>;
			pinctrl-names = "default";
			pinctrl-0 = <&led_shift_reg_pins>;

			sgpio_in0: sgpio@0 {
				compatible = "microchip,sparx5-sgpio-bank";
				reg = <0>;
				gpio-controller;
				#gpio-cells = <3>;
				ngpios = <64>;
			};

			sgpio_out1: sgpio@1 {
				compatible = "microchip,sparx5-sgpio-bank";
				reg = <1>;
				gpio-controller;
				#gpio-cells = <3>;
				ngpios = <64>;
			};
		};

		hsio: syscon {
			compatible = "mscc,ocelot-hsio", "syscon", "simple-mfd";

			serdes: serdes {
				compatible = "mscc,vsc7514-serdes";
				#phy-cells = <2>;
			};
		};
	};
};


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

v6
    * Applied several suggestions from the last RFC from Lee Jones. I
      hope I didn't miss anything.
    * Clean up MFD core - SPI interaction. They no longer use callbacks.
    * regmaps get registered to the child device, and don't attempt to
      get shared. It seems if a regmap is to be shared, that should be
      solved with syscon, not dev or mfd.

v7
    * Applied as much as I could from Lee and Vladimir's suggestions. As
      always, the feedback is greatly appreciated!
    * Remove "ocelot_spi" container complication
    * Move internal MDIO bus from ocelot_ext to MFD, with a devicetree
      change to match
    * Add initial HSIO support
    * Switch to IORESOURCE_REG for resource definitions

Colin Foster (13):
  pinctrl: ocelot: allow pinctrl-ocelot to be loaded as a module
  pinctrl: microchip-sgpio: allow sgpio driver to be used as a module
  net: mdio: mscc-miim: add local dev variable to cleanup probe function
  net: ocelot: add interface to get regmaps when exernally controlled
  net: mdio: mscc-miim: add ability to be used in a non-mmio
    configuration
  pinctrl: ocelot: add ability to be used in a non-mmio configuration
  pinctrl: microchip-sgpio: add ability to be used in a non-mmio
    configuration
  phy: ocelot-serdes: add ability to be used in mfd configuration
  resource: add define macro for register address resources
  mfd: ocelot: add support for the vsc7512 chip via spi
  net: mscc: ocelot: expose ocelot wm functions
  net: dsa: felix: add configurable device quirks
  net: dsa: ocelot: add external ocelot switch control

 drivers/mfd/Kconfig                        |  24 +
 drivers/mfd/Makefile                       |   3 +
 drivers/mfd/ocelot-core.c                  | 192 +++++++
 drivers/mfd/ocelot-spi.c                   | 313 ++++++++++++
 drivers/mfd/ocelot.h                       |  42 ++
 drivers/net/dsa/ocelot/Kconfig             |  14 +
 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        | 567 +++++++++++++++++++++
 drivers/net/dsa/ocelot/seville_vsc9953.c   |   1 +
 drivers/net/ethernet/mscc/ocelot_devlink.c |  31 ++
 drivers/net/ethernet/mscc/ocelot_vsc7514.c |  28 -
 drivers/net/mdio/mdio-mscc-miim.c          |  49 +-
 drivers/phy/mscc/phy-ocelot-serdes.c       |  11 +
 drivers/pinctrl/Kconfig                    |   4 +-
 drivers/pinctrl/pinctrl-microchip-sgpio.c  |  26 +-
 drivers/pinctrl/pinctrl-ocelot.c           |  35 +-
 include/linux/ioport.h                     |   5 +
 include/soc/mscc/ocelot.h                  |  19 +
 21 files changed, 1318 insertions(+), 60 deletions(-)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-spi.c
 create mode 100644 drivers/mfd/ocelot.h
 create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c

-- 
2.25.1

From 3b52c7edd85e8d9c0b6ab43f8682b73d002def6c Mon Sep 17 00:00:00 2001
From: Colin Foster <colin.foster@in-advantage.com>
Date: Sun, 6 Mar 2022 17:25:07 -0800

[RFC v7 net-next 01/13] pinctrl: ocelot: allow pinctrl-ocelot to be loaded as a module

[Colin Foster]

Work is being done to allow external control of Ocelot chips. When pinctrl
drivers are used internally, it wouldn't make much sense to allow them to
be loaded as modules. In the case where the Ocelot chip is controlled
externally, this scenario becomes practical.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pinctrl/Kconfig          | 2 +-
 drivers/pinctrl/pinctrl-ocelot.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 6fc56d6598e2..1b367f423ceb 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -311,7 +311,7 @@ config PINCTRL_MICROCHIP_SGPIO
 	  LED controller.
 
 config PINCTRL_OCELOT
-	bool "Pinctrl driver for the Microsemi Ocelot and Jaguar2 SoCs"
+	tristate "Pinctrl driver for the Microsemi Ocelot and Jaguar2 SoCs"
 	depends on OF
 	depends on HAS_IOMEM
 	select GPIOLIB
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index fc969208d904..b6ad3ffb4596 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1778,6 +1778,7 @@ static const struct of_device_id ocelot_pinctrl_of_match[] = {
 	{ .compatible = "microchip,lan966x-pinctrl", .data = &lan966x_desc },
 	{},
 };
+MODULE_DEVICE_TABLE(of, ocelot_pinctrl_of_match);
 
 static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 {
@@ -1866,3 +1867,6 @@ static struct platform_driver ocelot_pinctrl_driver = {
 	.probe = ocelot_pinctrl_probe,
 };
 builtin_platform_driver(ocelot_pinctrl_driver);
+
+MODULE_DESCRIPTION("Ocelot Chip Pinctrl Driver");
+MODULE_LICENSE("Dual MIT/GPL");
-- 
2.25.1

[RFC v7 net-next 02/13] pinctrl: microchip-sgpio: allow sgpio driver to be used as a module

[Colin Foster]

As the commit message suggests, this simply adds the ability to select
SGPIO pinctrl as a module. This becomes more practical when the SGPIO
hardware exists on an external chip, controlled indirectly by I2C or SPI.
This commit enables that level of control.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pinctrl/Kconfig                   | 2 +-
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 1b367f423ceb..7ff00c560775 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -292,7 +292,7 @@ config PINCTRL_MCP23S08
 	  corresponding interrupt-controller.
 
 config PINCTRL_MICROCHIP_SGPIO
-	bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
+	tristate "Pinctrl driver for Microsemi/Microchip Serial GPIO"
 	depends on OF
 	depends on HAS_IOMEM
 	select GPIOLIB
diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index 639f1130e989..e8e47a6808e6 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -913,6 +913,7 @@ static const struct of_device_id microchip_sgpio_gpio_of_match[] = {
 		/* sentinel */
 	}
 };
+MODULE_DEVICE_TABLE(of, microchip_sgpio_gpio_of_match);
 
 static struct platform_driver microchip_sgpio_pinctrl_driver = {
 	.driver = {
@@ -923,3 +924,6 @@ static struct platform_driver microchip_sgpio_pinctrl_driver = {
 	.probe = microchip_sgpio_probe,
 };
 builtin_platform_driver(microchip_sgpio_pinctrl_driver);
+
+MODULE_DESCRIPTION("Microchip SGPIO Pinctrl Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[RFC v7 net-next 03/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>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/mdio/mdio-mscc-miim.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 7d2abaf2b2c9..6b14f3cf3891 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -220,6 +220,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;
@@ -228,38 +229,37 @@ 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);
+	ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_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;
 	}
 
@@ -267,9 +267,9 @@ static int mscc_miim_probe(struct platform_device *pdev)
 	miim->phy_regs = phy_regmap;
 	miim->phy_reset_offset = 0;
 
-	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 v7 net-next 04/13] net: ocelot: add interface to get regmaps when exernally controlled

[Colin Foster]

Ocelot chips have several peripherals: pinctrl, sgpio, miim... If the chip
is in a configuration where it is being externally controlled via SPI, the
child device will need to request a resource from the parent.

Add the function call that will be used in those scenarios so that drivers
can be updated before the full functionality is added.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 include/soc/mscc/ocelot.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index ee3c59639d70..998616511ffb 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -1018,4 +1018,11 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
 }
 #endif
 
+static inline struct regmap *
+ocelot_get_regmap_from_resource(struct device *child,
+				const struct resource *res)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif
-- 
2.25.1

[RFC v7 net-next 05/13] net: mdio: mscc-miim: add ability to be used in a non-mmio configuration

[Colin Foster]

There are a few Ocelot chips that contain the logic for this bus, but are
controlled externally. Specifically the VSC7511, 7512, 7513, and 7514. In
the externally controlled configurations these registers are not
memory-mapped.

Add support for these non-memory-mapped configurations.

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

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 6b14f3cf3891..3153bac874fd 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -16,6 +16,7 @@
 #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)
@@ -229,11 +230,20 @@ 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(dev, "Unable to map MIIM registers\n");
-		return PTR_ERR(regs);
-	}
+		/* Fall back to using IORESOURCE_REG, which is possible in an
+		 * MFD configuration
+		 */
+		res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+		if (!res) {
+			dev_err(dev, "Unable to get MIIM resource\n");
+			return -ENODEV;
+		}
 
-	mii_regmap = devm_regmap_init_mmio(dev, regs, &mscc_miim_regmap_config);
+		mii_regmap = ocelot_get_regmap_from_resource(dev, res);
+	} else {
+		mii_regmap = devm_regmap_init_mmio(dev, regs,
+						   &mscc_miim_regmap_config);
+	}
 
 	if (IS_ERR(mii_regmap)) {
 		dev_err(dev, "Unable to create MIIM regmap\n");
@@ -251,10 +261,15 @@ static int mscc_miim_probe(struct platform_device *pdev)
 
 		phy_regmap = devm_regmap_init_mmio(dev, phy_regs,
 						   &mscc_miim_regmap_config);
-		if (IS_ERR(phy_regmap)) {
-			dev_err(dev, "Unable to create phy register regmap\n");
-			return PTR_ERR(phy_regmap);
-		}
+	} else {
+		res = platform_get_resource(pdev, IORESOURCE_REG, 1);
+		if (res)
+			phy_regmap = ocelot_get_regmap_from_resource(dev, res);
+	}
+
+	if (IS_ERR(phy_regmap)) {
+		dev_err(dev, "Unable to create phy register regmap\n");
+		return PTR_ERR(phy_regmap);
 	}
 
 	ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_regmap, 0);
-- 
2.25.1

[RFC v7 net-next 06/13] pinctrl: ocelot: add ability to be used in a non-mmio configuration

[Colin Foster]

There are a few Ocelot chips that contain pinctrl logic, but can be
controlled externally. Specifically the VSC7511, 7512, 7513 and 7514. In
the externally controlled configurations these registers are not
memory-mapped.

Add support for these non-memory-mapped configurations.

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

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index b6ad3ffb4596..e327bf00d447 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <soc/mscc/ocelot.h>
 
 #include "core.h"
 #include "pinconf.h"
@@ -1123,6 +1124,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 +1809,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 +1824,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);
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(base)) {
+		/*
+		 * Fall back to using IORESOURCE_REG, which is possible in an
+		 * MFD configuration
+		 */
+		res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+		if (!res) {
+			dev_err(dev, "Failed to get resource\n");
+			return -ENODEV;
+		}
 
-	info->stride = 1 + (info->desc->npins - 1) / 32;
+		info->map = ocelot_get_regmap_from_resource(dev, res);
+	} else {
+		regmap_config.max_register =
+			OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
 
-	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
+		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	}
+
+	info->stride = 1 + (info->desc->npins - 1) / 32;
 
-	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 v7 net-next 07/13] pinctrl: microchip-sgpio: add ability to be used in a non-mmio configuration

[Colin Foster]

There are a few Ocelot chips that can contain SGPIO logic, but can be
controlled externally. Specifically the VSC7511, 7512, 7513, and 7514. In
the externally controlled configurations these registers are not
memory-mapped.

Add support for these non-memory-mapped configurations.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index e8e47a6808e6..a91272936c8f 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -19,6 +19,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <soc/mscc/ocelot.h>
 
 #include "core.h"
 #include "pinconf.h"
@@ -819,6 +820,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 	struct fwnode_handle *fwnode;
 	struct reset_control *reset;
 	struct sgpio_priv *priv;
+	struct resource *res;
 	struct clk *clk;
 	u32 __iomem *regs;
 	u32 val;
@@ -851,11 +853,23 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(regs))
-		return PTR_ERR(regs);
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(regs)) {
+		/*
+		 * Fall back to using IORESOURCE_REG, which is possible in an
+		 * MFD configuration
+		 */
+		res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+		if (!res) {
+			dev_err(dev, "Failed to get resource\n");
+			return -ENODEV;
+		}
+
+		priv->regs = ocelot_get_regmap_from_resource(dev, res);
+	} else {
+		priv->regs = devm_regmap_init_mmio(dev, regs, &regmap_config);
+	}
 
-	priv->regs = devm_regmap_init_mmio(dev, regs, &regmap_config);
 	if (IS_ERR(priv->regs))
 		return PTR_ERR(priv->regs);
 
-- 
2.25.1

[RFC v7 net-next 08/13] phy: ocelot-serdes: add ability to be used in mfd configuration

[Colin Foster]

When ocelot-serdes is used in an MFD configuration, it might need to get
regmaps from an mfd instead of syscon. Add this ability to be used in
either configuration.

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

diff --git a/drivers/phy/mscc/phy-ocelot-serdes.c b/drivers/phy/mscc/phy-ocelot-serdes.c
index 76f596365176..ae1284e356e7 100644
--- a/drivers/phy/mscc/phy-ocelot-serdes.c
+++ b/drivers/phy/mscc/phy-ocelot-serdes.c
@@ -15,6 +15,7 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <soc/mscc/ocelot.h>
 #include <soc/mscc/ocelot_hsio.h>
 #include <dt-bindings/phy/phy-ocelot-serdes.h>
 
@@ -492,8 +493,10 @@ static int serdes_phy_create(struct serdes_ctrl *ctrl, u8 idx, struct phy **phy)
 
 static int serdes_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct phy_provider *provider;
 	struct serdes_ctrl *ctrl;
+	struct resource *res;
 	unsigned int i;
 	int ret;
 
@@ -502,7 +505,15 @@ static int serdes_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctrl->dev = &pdev->dev;
+
 	ctrl->regs = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(ctrl->regs)) {
+		/* Fall back to using IORESOURCE_REG, if possible */
+		res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+		if (!res)
+			ctrl->regs = ocelot_get_regmap_from_resource(dev, res);
+	}
+
 	if (IS_ERR(ctrl->regs))
 		return PTR_ERR(ctrl->regs);
 
-- 
2.25.1

[RFC v7 net-next 09/13] resource: add define macro for register address resources

[Colin Foster]

DEFINE_RES_ macros have been created for the commonly used resource types,
but not IORESOURCE_REG. Add the macro so it can be used in a similar manner
to all other resource types.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 include/linux/ioport.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..69ecf5cfc277 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -171,6 +171,11 @@ enum {
 #define DEFINE_RES_MEM(_start, _size)					\
 	DEFINE_RES_MEM_NAMED((_start), (_size), NULL)
 
+#define DEFINE_RES_REG_NAMED(_start, _size, _name)			\
+	DEFINE_RES_NAMED((_start), (_size), (_name), IORESOURCE_REG)
+#define DEFINE_RES_REG(_start, _size)					\
+	DEFINE_RES_MEM_NAMED((_start), (_size), NULL)
+
 #define DEFINE_RES_IRQ_NAMED(_irq, _name)				\
 	DEFINE_RES_NAMED((_irq), 1, (_name), IORESOURCE_IRQ)
 #define DEFINE_RES_IRQ(_irq)						\
-- 
2.25.1

[RFC v7 net-next 10/13] mfd: ocelot: add support for the vsc7512 chip via spi

[Colin Foster]

The VSC7512 is a networking chip that contains several peripherals. Many of
these peripherals are currently supported by the VSC7513 and VSC7514 chips,
but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
controlled externally.

Utilize the existing drivers by referencing the chip as an MFD. Add support
for the two MDIO buses, the internal phys, pinctrl, serial GPIO, and HSIO.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/Kconfig       |  24 +++
 drivers/mfd/Makefile      |   3 +
 drivers/mfd/ocelot-core.c | 189 +++++++++++++++++++++++
 drivers/mfd/ocelot-spi.c  | 313 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/ocelot.h      |  42 +++++
 include/soc/mscc/ocelot.h |   5 +
 6 files changed, 576 insertions(+)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-spi.c
 create mode 100644 drivers/mfd/ocelot.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ba0b3eb131f1..d4312a5252d0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -948,6 +948,30 @@ config MFD_MENF21BMC
 	  This driver can also be built as a module. If so the module
 	  will be called menf21bmc.
 
+config MFD_OCELOT
+	tristate "Microsemi Ocelot External Control Support"
+	select MFD_CORE
+	help
+	  Ocelot is a family of networking chips that support multiple ethernet
+	  and fibre interfaces. In addition to networking, they contain several
+	  other functions, including pictrl, MDIO, and communication with
+	  external chips. While some chips have an internal processor capable of
+	  running an OS, others don't. All chips can be controlled externally
+	  through different interfaces, including SPI, I2C, and PCIe.
+
+	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
+	  VSC7513, VSC7514) controlled externally.
+
+	  If unsure, say N
+
+config MFD_OCELOT_SPI
+	tristate "Microsemi Ocelot SPI interface"
+	depends on MFD_OCELOT
+	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 df1ecc4a4c95..12513843067a 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)	+= 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..36e4326a853a
--- /dev/null
+++ b/drivers/mfd/ocelot-core.c
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * MFD core driver for the Ocelot chip family.
+ *
+ * The VSC7511, 7512, 7513, and 7514 can be controlled internally via an
+ * on-chip MIPS processor, or externally via SPI, I2C, PCIe. This core driver is
+ * intended to be the bus-agnostic glue between, for example, the SPI bus and
+ * the MFD children.
+ *
+ * Copyright 2021 Innovative Advantage Inc.
+ *
+ * Author: Colin Foster <colin.foster@in-advantage.com>
+ */
+
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <soc/mscc/ocelot.h>
+
+#include <asm/byteorder.h>
+
+#include "ocelot.h"
+
+#define GCB_SOFT_RST 0x0008
+
+#define SOFT_CHIP_RST 0x1
+
+#define VSC7512_GCB_RES_START	0x71070000
+#define VSC7512_GCB_RES_SIZE	0x14
+
+#define VSC7512_MIIM0_RES_START	0x7107009c
+#define VSC7512_MIIM0_RES_SIZE	0x24
+
+#define VSC7512_MIIM1_RES_START	0x710700c0
+#define VSC7512_MIIM1_RES_SIZE	0x24
+
+#define VSC7512_PHY_RES_START	0x710700f0
+#define VSC7512_PHY_RES_SIZE	0x4
+
+#define VSC7512_HSIO_RES_START	0x710d0000
+#define VSC7512_HSIO_RES_SIZE	0x10000
+
+#define VSC7512_GPIO_RES_START	0x71070034
+#define VSC7512_GPIO_RES_SIZE	0x6c
+
+#define VSC7512_SIO_RES_START	0x710700f8
+#define VSC7512_SIO_RES_SIZE	0x100
+
+static const struct resource vsc7512_gcb_resource =
+	DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE,
+			     "devcpu_gcb_chip_regs");
+
+static int ocelot_reset(struct ocelot_core *core)
+{
+	int ret;
+
+	/*
+	 * Reset the entire chip here to put it into a completely known state.
+	 * Other drivers may want to reset their own subsystems. The register
+	 * self-clears, so one write is all that is needed
+	 */
+	ret = regmap_write(core->gcb_regmap, GCB_SOFT_RST, SOFT_CHIP_RST);
+	if (ret)
+		return ret;
+
+	msleep(100);
+
+	return ret;
+}
+
+static struct regmap *ocelot_devm_regmap_init(struct ocelot_core *core,
+					      struct device *child,
+					      const struct resource *res)
+{
+	/*
+	 * Call directly into ocelot_spi to get a new regmap. This will need to
+	 * be expanded if additional bus types are to be supported in the
+	 * future.
+	 */
+	return ocelot_spi_devm_get_regmap(core, child, res);
+}
+
+struct regmap *ocelot_get_regmap_from_resource(struct device *child,
+					       const struct resource *res)
+{
+	struct ocelot_core *core = dev_get_drvdata(child->parent);
+
+	return ocelot_devm_regmap_init(core, child, res);
+}
+EXPORT_SYMBOL(ocelot_get_regmap_from_resource);
+
+static const struct resource vsc7512_miim0_resources[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_MIIM0_RES_START, VSC7512_MIIM0_RES_SIZE,
+			     "gcb_miim0"),
+	DEFINE_RES_REG_NAMED(VSC7512_PHY_RES_START, VSC7512_PHY_RES_SIZE,
+			     "gcb_phy"),
+};
+
+static const struct resource vsc7512_miim1_resources[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_MIIM1_RES_START, VSC7512_MIIM1_RES_SIZE,
+			     "gcb_miim1"),
+};
+
+static const struct resource vsc7512_hsio_resources[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE,
+			     "hsio"),
+};
+
+static const struct resource vsc7512_pinctrl_resources[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_GPIO_RES_START, VSC7512_GPIO_RES_SIZE,
+			     "gcb_gpio"),
+};
+
+static const struct resource vsc7512_sgpio_resources[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_SIO_RES_START, VSC7512_SIO_RES_SIZE,
+			     "gcb_sio"),
+};
+
+static const struct mfd_cell vsc7512_devs[] = {
+	{
+		.name = "ocelot-pinctrl",
+		.of_compatible = "mscc,ocelot-pinctrl",
+		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
+		.resources = vsc7512_pinctrl_resources,
+	}, {
+		.name = "ocelot-sgpio",
+		.of_compatible = "mscc,ocelot-sgpio",
+		.num_resources = ARRAY_SIZE(vsc7512_sgpio_resources),
+		.resources = vsc7512_sgpio_resources,
+	}, {
+		.name = "ocelot-miim0",
+		.of_compatible = "mscc,ocelot-miim",
+		.num_resources = ARRAY_SIZE(vsc7512_miim0_resources),
+		.resources = vsc7512_miim0_resources,
+	}, {
+		.name = "ocelot-miim1",
+		.of_compatible = "mscc,ocelot-miim",
+		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
+		.resources = vsc7512_miim1_resources,
+	}, {
+		.name = "ocelot-serdes",
+		.of_compatible = "mscc,vsc7514-serdes",
+		.num_resources = ARRAY_SIZE(vsc7512_hsio_resources),
+		.resources = vsc7512_hsio_resources,
+	},
+};
+
+int ocelot_core_init(struct ocelot_core *core)
+{
+	struct device *dev = core->dev;
+	int ret;
+
+	dev_set_drvdata(dev, core);
+
+	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
+						   &vsc7512_gcb_resource);
+	if (IS_ERR(core->gcb_regmap))
+		return -ENOMEM;
+
+	ret = ocelot_reset(core);
+	if (ret) {
+		dev_err(dev, "Failed to reset device: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * A chip reset will clear the SPI configuration, so it needs to be done
+	 * again before we can access any registers
+	 */
+	ret = ocelot_spi_initialize(core);
+	if (ret) {
+		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
+				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_core_init);
+
+MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
+MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
new file mode 100644
index 000000000000..c788e239c9a7
--- /dev/null
+++ b/drivers/mfd/ocelot-spi.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * SPI core driver for the Ocelot chip family.
+ *
+ * This driver will handle everything necessary to allow for communication over
+ * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
+ * are to prepare the chip's SPI interface for a specific bus speed, and a host
+ * processor's endianness. This will create and distribute regmaps for any MFD
+ * children.
+ *
+ * Copyright 2021 Innovative Advantage Inc.
+ *
+ * Author: Colin Foster <colin.foster@in-advantage.com>
+ */
+
+#include <linux/iopoll.h>
+#include <linux/kconfig.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include <asm/byteorder.h>
+
+#include "ocelot.h"
+
+#define DEV_CPUORG_IF_CTRL	0x0000
+#define DEV_CPUORG_IF_CFGSTAT	0x0004
+
+#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
+#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
+#define CFGSTAT_IF_NUM_SI	(2 << 24)
+#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
+
+
+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
+
+int ocelot_spi_initialize(struct ocelot_core *core)
+{
+	u32 val, check;
+	int err;
+
+#ifdef __LITTLE_ENDIAN
+	val = VSC7512_BYTE_ORDER_LE;
+#else
+	val = VSC7512_BYTE_ORDER_BE;
+#endif
+
+	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
+	if (err)
+		return err;
+
+	val = core->spi_padding_bytes;
+	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
+	if (err)
+		return err;
+
+	/*
+	 * After we write the interface configuration, read it back here. This
+	 * will verify several different things. The first is that the number of
+	 * padding bytes actually got written correctly. These are found in bits
+	 * 0:3.
+	 *
+	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
+	 * and will be set if the register access is too fast. This would be in
+	 * the condition that the number of padding bytes is insufficient for
+	 * the SPI bus frequency.
+	 *
+	 * The last check is for bits 31:24, which define the interface by which
+	 * the registers are being accessed. Since we're accessing them via the
+	 * serial interface, it must return IF_NUM_SI.
+	 */
+	check = val | CFGSTAT_IF_NUM_SI;
+
+	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
+	if (err)
+		return err;
+
+	if (check != val)
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_spi_initialize);
+
+/*
+ * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
+ * the register locations are defined as 32-bit. The least significant two bits
+ * get shifted out, as register accesses must always be word-aligned, leaving
+ * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
+ * payload can be optimized for bit / byte order to match whatever architecture
+ * the controlling CPU has.
+ */
+static unsigned int ocelot_spi_translate_address(unsigned int reg)
+{
+	return cpu_to_be32((reg & 0xffffff) >> 2);
+}
+
+struct ocelot_spi_regmap_context {
+	u32 base;
+	struct ocelot_core *core;
+};
+
+static int ocelot_spi_reg_read(void *context, unsigned int reg,
+			       unsigned int *val)
+{
+	struct ocelot_spi_regmap_context *regmap_context = context;
+	struct ocelot_core *core = regmap_context->core;
+	struct spi_transfer tx, padding, rx;
+	struct spi_message msg;
+	struct spi_device *spi;
+	unsigned int addr;
+	u8 *tx_buf;
+
+	spi = core->spi;
+
+	addr = ocelot_spi_translate_address(reg + regmap_context->base);
+	tx_buf = (u8 *)&addr;
+
+	spi_message_init(&msg);
+
+	memset(&tx, 0, sizeof(tx));
+
+	/* 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 (core->spi_padding_bytes > 0) {
+		u8 dummy_buf[16] = {0};
+
+		memset(&padding, 0, sizeof(padding));
+
+		/* Just toggle the clock for padding bytes */
+		padding.len = core->spi_padding_bytes;
+		padding.tx_buf = dummy_buf;
+		padding.dummy_data = 1;
+
+		spi_message_add_tail(&padding, &msg);
+	}
+
+	memset(&rx, 0, sizeof(rx));
+	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 ocelot_core *core = regmap_context->core;
+	struct spi_transfer tx[2] = {0};
+	struct spi_message msg;
+	struct spi_device *spi;
+	unsigned int addr;
+	u8 *tx_buf;
+
+	spi = core->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,
+};
+
+struct regmap *
+ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
+			   const struct resource *res)
+{
+	struct ocelot_spi_regmap_context *context;
+	struct regmap_config regmap_config;
+
+	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
+	if (IS_ERR(context))
+		return ERR_CAST(context);
+
+	context->base = res->start;
+	context->core = core;
+
+	memcpy(&regmap_config, &ocelot_spi_regmap_config,
+	       sizeof(ocelot_spi_regmap_config));
+
+	regmap_config.name = res->name;
+	regmap_config.max_register = res->end - res->start;
+
+	return devm_regmap_init(child, NULL, context, &regmap_config);
+}
+
+static int ocelot_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ocelot_core *core;
+	int err;
+
+	core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
+	if (!core)
+		return -ENOMEM;
+
+	if (spi->max_speed_hz <= 500000) {
+		core->spi_padding_bytes = 0;
+	} else {
+		/*
+		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG.
+		 * Register access time is 1us, so we need to configure and send
+		 * out enough padding bytes between the read request and data
+		 * transmission that lasts at least 1 microsecond.
+		 */
+		core->spi_padding_bytes = 1 +
+			(spi->max_speed_hz / 1000000 + 2) / 8;
+	}
+
+	core->spi = spi;
+
+	spi->bits_per_word = 8;
+
+	err = spi_setup(spi);
+	if (err < 0) {
+		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
+		return err;
+	}
+
+	core->cpuorg_regmap =
+		ocelot_spi_devm_get_regmap(core, dev,
+					   &vsc7512_dev_cpuorg_resource);
+	if (IS_ERR(core->cpuorg_regmap))
+		return -ENOMEM;
+
+	core->dev = dev;
+
+	/*
+	 * The chip must be set up for SPI before it gets initialized and reset.
+	 * This must be done before calling init, and after a chip reset is
+	 * performed.
+	 */
+	err = ocelot_spi_initialize(core);
+	if (err) {
+		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
+		return err;
+	}
+
+	err = ocelot_core_init(core);
+	if (err < 0) {
+		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+const struct of_device_id ocelot_spi_of_match[] = {
+	{ .compatible = "mscc,vsc7512_mfd_spi" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ocelot_spi_of_match);
+
+static struct spi_driver ocelot_spi_driver = {
+	.driver = {
+		.name = "ocelot_mfd_spi",
+		.of_match_table = of_match_ptr(ocelot_spi_of_match),
+	},
+	.probe = ocelot_spi_probe,
+};
+module_spi_driver(ocelot_spi_driver);
+
+MODULE_DESCRIPTION("SPI Controlled Ocelot Chip Driver");
+MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
+MODULE_LICENSE("Dual MIT/GPL");
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
new file mode 100644
index 000000000000..20d3853dd6d2
--- /dev/null
+++ b/drivers/mfd/ocelot.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright 2021 Innovative Advantage Inc.
+ */
+
+#include <linux/kconfig.h>
+#include <linux/regmap.h>
+
+struct ocelot_core {
+	struct device *dev;
+	struct regmap *gcb_regmap;
+	struct regmap *cpuorg_regmap;
+
+#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)
+	int spi_padding_bytes;
+	struct spi_device *spi;
+#endif
+};
+
+void ocelot_get_resource_name(char *name, const struct resource *res,
+			      int size);
+int ocelot_core_init(struct ocelot_core *core);
+int ocelot_remove(struct ocelot_core *core);
+
+#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)
+struct regmap *ocelot_spi_devm_get_regmap(struct ocelot_core *core,
+					  struct device *child,
+					  const struct resource *res);
+int ocelot_spi_initialize(struct ocelot_core *core);
+#else
+static inline struct regmap *ocelot_spi_devm_get_regmap(
+		struct ocelot_core *core, struct device *child,
+		const struct resource *res)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int ocelot_spi_initialize(struct ocelot_core *core)
+{
+	return -EOPNOTSUPP;
+}
+#endif
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 998616511ffb..d9e2710d5646 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -1018,11 +1018,16 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_MFD_OCELOT)
+struct regmap *ocelot_get_regmap_from_resource(struct device *child,
+					       const struct resource *res);
+#else
 static inline struct regmap *
 ocelot_get_regmap_from_resource(struct device *child,
 				const struct resource *res)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
+#endif
 
 #endif
-- 
2.25.1

[RFC v7 net-next 11/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 d9e2710d5646..480bf21da404 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -852,6 +852,11 @@ void ocelot_deinit_port(struct ocelot *ocelot, int port);
 void ocelot_port_set_dsa_8021q_cpu(struct ocelot *ocelot, int port);
 void ocelot_port_unset_dsa_8021q_cpu(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 v7 net-next 12/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>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.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 7cc67097948b..7cf4afc6ed9a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -997,9 +997,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,
@@ -1014,7 +1017,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 f083b06fdfe9..0323383dee1e 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -33,6 +33,7 @@ struct felix_info {
 	u16				vcap_pol_base2;
 	u16				vcap_pol_max2;
 	const struct ptp_clock_info	*ptp_caps;
+	unsigned long			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 ead3316742f6..1f79ed4ccef4 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2220,6 +2220,7 @@ static const struct felix_info felix_info_vsc9959 = {
 	.num_mact_rows		= 2048,
 	.num_ports		= VSC9959_NUM_PORTS,
 	.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 68ef8f111bbe..58665abf9d02 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1092,6 +1092,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		= VSC9953_NUM_PORTS,
 	.num_tx_queues		= OCELOT_NUM_TC,
-- 
2.25.1

[RFC v7 net-next 13/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.

Currently the four copper phy ports are fully functional. Communication to
external phys is also functional, but the SGMII / QSGMII interfaces are
currently non-functional.

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

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 36e4326a853a..c6afe1791ecc 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -142,6 +142,9 @@ static const struct mfd_cell vsc7512_devs[] = {
 		.of_compatible = "mscc,vsc7514-serdes",
 		.num_resources = ARRAY_SIZE(vsc7512_hsio_resources),
 		.resources = vsc7512_hsio_resources,
+	}, {
+		.name = "ocelot-ext-switch",
+		.of_compatible = "mscc,vsc7512-ext-switch",
 	},
 };
 
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 220b0b027b55..f40b2c7171ad 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -1,4 +1,18 @@
 # 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 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..9755b13573c9
--- /dev/null
+++ b/drivers/net/dsa/ocelot/ocelot_ext.c
@@ -0,0 +1,567 @@
+// 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/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"
+
+#define VSC7512_NUM_PORTS		11
+
+static const u32 vsc7512_port_modes[VSC7512_NUM_PORTS] = {
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+	OCELOT_PORT_MODE_SGMII,
+	OCELOT_PORT_MODE_SGMII | OCELOT_PORT_MODE_QSGMII,
+};
+
+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,
+};
+
+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)
+{
+	int retries = 100;
+	int err, val;
+
+	ocelot_ext_reset_phys(ocelot);
+
+	/* Initialize chip memories */
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+	if (err)
+		return err;
+
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
+	if (err)
+		return err;
+
+	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
+	 * 100us) before enabling the switch core
+	 */
+	do {
+		msleep(1);
+		err = regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],
+					&val);
+		if (err)
+			return err;
+	} while (val && --retries);
+
+	if (!retries)
+		return -ETIMEDOUT;
+
+	return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
+}
+
+static const struct ocelot_ops ocelot_ext_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 ocelot_ext_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 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 *ocelot_ext_regmap_init(struct ocelot *ocelot,
+					  struct resource *res)
+{
+	return ocelot_get_regmap_from_resource(ocelot->dev, res);
+}
+
+static const struct felix_info vsc7512_info = {
+	.target_io_res			= vsc7512_target_io_res,
+	.port_io_res			= vsc7512_port_io_res,
+	.regfields			= vsc7512_regfields,
+	.map				= vsc7512_regmap,
+	.ops				= &ocelot_ext_ops,
+	.stats_layout			= vsc7512_stats_layout,
+	.num_stats			= ARRAY_SIZE(vsc7512_stats_layout),
+	.vcap				= vsc7512_vcap_props,
+	.num_mact_rows			= 1024,
+	.num_ports			= VSC7512_NUM_PORTS,
+	.num_tx_queues			= OCELOT_NUM_TC,
+	.phylink_validate		= ocelot_ext_phylink_validate,
+	.port_modes			= vsc7512_port_modes,
+	.init_regmap			= ocelot_ext_regmap_init,
+};
+
+static int ocelot_ext_probe(struct platform_device *pdev)
+{
+	struct dsa_switch *ds;
+	struct ocelot *ocelot;
+	struct felix *felix;
+	struct device *dev;
+	int err;
+
+	dev = &pdev->dev;
+
+	felix = kzalloc(sizeof(*felix), GFP_KERNEL);
+	if (!felix)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, felix);
+
+	ocelot = &felix->ocelot;
+	ocelot->dev = dev;
+
+	ocelot->num_flooding_pgids = 1;
+
+	felix->info = &vsc7512_info;
+
+	ds = kzalloc(sizeof(*ds), GFP_KERNEL);
+	if (!ds) {
+		err = -ENOMEM;
+		dev_err(dev, "Failed to allocate DSA switch\n");
+		goto err_free_felix;
+	}
+
+	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_free_ds;
+	}
+
+	return 0;
+
+err_free_ds:
+	kfree(ds);
+err_free_felix:
+	kfree(felix);
+	return err;
+}
+
+static int ocelot_ext_remove(struct platform_device *pdev)
+{
+	struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+	if (!felix)
+		return 0;
+
+	dsa_unregister_switch(felix->ds);
+
+	kfree(felix->ds);
+	kfree(felix);
+
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	return 0;
+}
+
+static void ocelot_ext_shutdown(struct platform_device *pdev)
+{
+	struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+	if (!felix)
+		return;
+
+	dsa_switch_shutdown(felix->ds);
+
+	dev_set_drvdata(&pdev->dev, NULL);
+}
+
+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,
+	.shutdown = ocelot_ext_shutdown,
+};
+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 480bf21da404..da028c28a103 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -397,6 +397,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

Re: [RFC v7 net-next 13/13] net: dsa: ocelot: add external ocelot switch control

[Vladimir Oltean]

On Sun, Mar 06, 2022 at 06:12:08PM -0800, Colin Foster wrote:
> +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),

If you add GCB_SOFT_RST_SWC_RST to ocelot_regfields, can't you just use that?

> +	[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[] = {

Why not use ocelot_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 ocelot_ext_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;
> +	}

You might want to check
git log -4 --author="Russell King" drivers/net/dsa/ocelot/
especially commit e57a15401e82 ("net: dsa: ocelot: remove interface checks").
And you can/should in fact use phylink_generic_validate, since there
aren't any special constraints that I know of.

> +
> +	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 struct vcap_props vsc7512_vcap_props[] = {

Why not vsc7514_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,
> +	},
> +};

Re: [RFC v7 net-next 10/13] mfd: ocelot: add support for the vsc7512 chip via spi

[Vladimir Oltean]

On Sun, Mar 06, 2022 at 06:12:05PM -0800, Colin Foster wrote:
> The VSC7512 is a networking chip that contains several peripherals. Many of
> these peripherals are currently supported by the VSC7513 and VSC7514 chips,
> but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
> controlled externally.
> 
> Utilize the existing drivers by referencing the chip as an MFD. Add support
> for the two MDIO buses, the internal phys, pinctrl, serial GPIO, and HSIO.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> +#define VSC7512_MIIM0_RES_START	0x7107009c
> +#define VSC7512_MIIM0_RES_SIZE	0x24
> +
> +#define VSC7512_MIIM1_RES_START	0x710700c0
> +#define VSC7512_MIIM1_RES_SIZE	0x24
> +
> +#define VSC7512_PHY_RES_START	0x710700f0
> +#define VSC7512_PHY_RES_SIZE	0x4
> +
> +#define VSC7512_HSIO_RES_START	0x710d0000
> +#define VSC7512_HSIO_RES_SIZE	0x10000
> +
> +#define VSC7512_GPIO_RES_START	0x71070034
> +#define VSC7512_GPIO_RES_SIZE	0x6c
> +
> +#define VSC7512_SIO_RES_START	0x710700f8
> +#define VSC7512_SIO_RES_SIZE	0x100
> +
> +static const struct resource vsc7512_gcb_resource =
> +	DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE,
> +			     "devcpu_gcb_chip_regs");
> +static const struct resource vsc7512_miim0_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM0_RES_START, VSC7512_MIIM0_RES_SIZE,
> +			     "gcb_miim0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PHY_RES_START, VSC7512_PHY_RES_SIZE,
> +			     "gcb_phy"),
> +};
> +
> +static const struct resource vsc7512_miim1_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM1_RES_START, VSC7512_MIIM1_RES_SIZE,
> +			     "gcb_miim1"),
> +};
> +
> +static const struct resource vsc7512_hsio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE,
> +			     "hsio"),
> +};
> +
> +static const struct resource vsc7512_pinctrl_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_GPIO_RES_START, VSC7512_GPIO_RES_SIZE,
> +			     "gcb_gpio"),
> +};
> +
> +static const struct resource vsc7512_sgpio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_SIO_RES_START, VSC7512_SIO_RES_SIZE,
> +			     "gcb_sio"),
> +};
> +
> +static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "ocelot-pinctrl",
> +		.of_compatible = "mscc,ocelot-pinctrl",
> +		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
> +		.resources = vsc7512_pinctrl_resources,
> +	}, {
> +		.name = "ocelot-sgpio",
> +		.of_compatible = "mscc,ocelot-sgpio",
> +		.num_resources = ARRAY_SIZE(vsc7512_sgpio_resources),
> +		.resources = vsc7512_sgpio_resources,
> +	}, {
> +		.name = "ocelot-miim0",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim0_resources),
> +		.resources = vsc7512_miim0_resources,
> +	}, {
> +		.name = "ocelot-miim1",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> +		.resources = vsc7512_miim1_resources,

I'm looking at mfd_match_of_node_to_dev() and I don't really understand
how the first MDIO bus matches the first mfd_cell's resources, and the
second MDIO bus the second mfd_cell? By order of definition?

> +	}, {
> +		.name = "ocelot-serdes",
> +		.of_compatible = "mscc,vsc7514-serdes",
> +		.num_resources = ARRAY_SIZE(vsc7512_hsio_resources),
> +		.resources = vsc7512_hsio_resources,
> +	},
> +};
> +
> +int ocelot_core_init(struct ocelot_core *core)
> +{
> +	struct device *dev = core->dev;
> +	int ret;
> +
> +	dev_set_drvdata(dev, core);
> +
> +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> +						   &vsc7512_gcb_resource);
> +	if (IS_ERR(core->gcb_regmap))
> +		return -ENOMEM;
> +
> +	ret = ocelot_reset(core);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * A chip reset will clear the SPI configuration, so it needs to be done
> +	 * again before we can access any registers
> +	 */
> +	ret = ocelot_spi_initialize(core);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_core_init);
> -- 
> 2.25.1
>

Re: [RFC v7 net-next 00/13] add support for VSC7512 control over SPI

[Vladimir Oltean]

On Sun, Mar 06, 2022 at 06:11:55PM -0800, Colin Foster wrote:
> The patch set in general is to add support for the VSC7512, and
> eventually the VSC7511, 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. It is not yet functional for SGMII,
> QSGMII, and SerDes ports.
> 
> I have mentioned previously:
> 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 relevant sections of the device tree I'm using for the VSC7512 is
> below. Notably the SGPIO LEDs follow link status and speed from network
> triggers.
> 
> 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.
> 
> 
> Of note: The Felix driver had the ability to register the internal MDIO
> bus. I am no longer using that in the switch driver, it is now an
> additional sub-device under the MFD.
> 
> I also made use of IORESOURCE_REG, which removed the "device_is_mfd"
> requirement.
> 
> 
> / {
> 	vscleds {
> 		compatible = "gpio-leds";
> 		vscled@0 {
> 			label = "port0led";
> 			gpios = <&sgpio_out1 0 0 GPIO_ACTIVE_LOW>;
> 			default-state = "off";
> 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:link";
> 		};
> 		vscled@1 {
> 			label = "port0led1";
> 			gpios = <&sgpio_out1 0 1 GPIO_ACTIVE_LOW>;
> 			default-state = "off";
> 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:1Gbps";
> 		};
> [ ... ]
> 		vscled@71 {
> 			label = "port7led1";
> 			gpios = <&sgpio_out1 7 1 GPIO_ACTIVE_LOW>;
> 			default-state = "off";
> 			linux,default-trigger = "ocelot-miim1-mii:07:1Gbps";
> 		};
> 	};
> };
> 
> &spi0 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	status = "okay";
> 
> 	ocelot-chip@0 {
> 		compatible = "mscc,vsc7512_mfd_spi";
> 		spi-max-frequency = <2500000>;
> 		reg = <0>;
> 
> 		ethernet-switch@0 {

I'm not exactly clear on what exactly does the bus address (@0)
represent here and in other (but not all) sub-nodes.
dtc probably warns that there shouldn't be any unit address, since
#address-cells and #size-cells are both 0 for ocelot-chip@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";
> 				};
> 			};
> 		};
> 
> 		mdio0: mdio0@0 {
> 			compatible = "mscc,ocelot-miim";
> 			#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@1 {
> 			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";
> 			};
> 		};
> 
> 		sgpio: sgpio {
> 			compatible = "mscc,ocelot-sgpio";
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			bus-frequency=<12500000>;
> 			clocks = <&ocelot_clock>;
> 			microchip,sgpio-port-ranges = <0 15>;
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&led_shift_reg_pins>;
> 
> 			sgpio_in0: sgpio@0 {
> 				compatible = "microchip,sparx5-sgpio-bank";
> 				reg = <0>;
> 				gpio-controller;
> 				#gpio-cells = <3>;
> 				ngpios = <64>;
> 			};
> 
> 			sgpio_out1: sgpio@1 {
> 				compatible = "microchip,sparx5-sgpio-bank";
> 				reg = <1>;
> 				gpio-controller;
> 				#gpio-cells = <3>;
> 				ngpios = <64>;
> 			};
> 		};
> 
> 		hsio: syscon {
> 			compatible = "mscc,ocelot-hsio", "syscon", "simple-mfd";
> 
> 			serdes: serdes {
> 				compatible = "mscc,vsc7514-serdes";
> 				#phy-cells = <2>;
> 			};
> 		};
> 	};
> };

The switch-related portion of this patch set looks good enough to me.
I'll let somebody else with more knowledge provide feedback on the
mfd/pinctrl/gpio/phylink/led integration aspects.

Re: [RFC v7 net-next 13/13] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

On Tue, Mar 08, 2022 at 02:14:40PM +0000, Vladimir Oltean wrote:
> On Sun, Mar 06, 2022 at 06:12:08PM -0800, Colin Foster wrote:
> > +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),
> 
> If you add GCB_SOFT_RST_SWC_RST to ocelot_regfields, can't you just use that?

Interesting idea. I'll look and see if there are any bitfield
differences between the 7512 and 7514. If no, I'll do the same I did
with vsc7514_*_regmap structs. 

> 
> > +	[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[] = {
> 
> Why not use ocelot_stats_layout?

Same as above (and below). I can probably remove all of these.

> 
> > +	{ .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 ocelot_ext_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;
> > +	}
> 
> You might want to check
> git log -4 --author="Russell King" drivers/net/dsa/ocelot/
> especially commit e57a15401e82 ("net: dsa: ocelot: remove interface checks").
> And you can/should in fact use phylink_generic_validate, since there
> aren't any special constraints that I know of.

Oh, nice. Thanks. By the end of all this it feels like there'll be
nothing left in this file (in a good way!)

There might be more implications for me regarding commit 
79fda660bdbb ("net: dsa: ocelot: populate supported_interfaces") since it
seems to be based on the assumption that every sub-driver only supports
a single interface mode. I still have my homework to do there though.

> 
> > +
> > +	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 struct vcap_props vsc7512_vcap_props[] = {
> 
> Why not vsc7514_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,
> > +	},
> > +};

Re: [RFC v7 net-next 00/13] add support for VSC7512 control over SPI

[Colin Foster]

On Tue, Mar 08, 2022 at 02:39:57PM +0000, Vladimir Oltean wrote:
> On Sun, Mar 06, 2022 at 06:11:55PM -0800, Colin Foster wrote:
> > The patch set in general is to add support for the VSC7512, and
> > eventually the VSC7511, 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. It is not yet functional for SGMII,
> > QSGMII, and SerDes ports.
> > 
> > I have mentioned previously:
> > 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 relevant sections of the device tree I'm using for the VSC7512 is
> > below. Notably the SGPIO LEDs follow link status and speed from network
> > triggers.
> > 
> > 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.
> > 
> > 
> > Of note: The Felix driver had the ability to register the internal MDIO
> > bus. I am no longer using that in the switch driver, it is now an
> > additional sub-device under the MFD.
> > 
> > I also made use of IORESOURCE_REG, which removed the "device_is_mfd"
> > requirement.
> > 
> > 
> > / {
> > 	vscleds {
> > 		compatible = "gpio-leds";
> > 		vscled@0 {
> > 			label = "port0led";
> > 			gpios = <&sgpio_out1 0 0 GPIO_ACTIVE_LOW>;
> > 			default-state = "off";
> > 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:link";
> > 		};
> > 		vscled@1 {
> > 			label = "port0led1";
> > 			gpios = <&sgpio_out1 0 1 GPIO_ACTIVE_LOW>;
> > 			default-state = "off";
> > 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:1Gbps";
> > 		};
> > [ ... ]
> > 		vscled@71 {
> > 			label = "port7led1";
> > 			gpios = <&sgpio_out1 7 1 GPIO_ACTIVE_LOW>;
> > 			default-state = "off";
> > 			linux,default-trigger = "ocelot-miim1-mii:07:1Gbps";
> > 		};
> > 	};
> > };
> > 
> > &spi0 {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	status = "okay";
> > 
> > 	ocelot-chip@0 {
> > 		compatible = "mscc,vsc7512_mfd_spi";
> > 		spi-max-frequency = <2500000>;
> > 		reg = <0>;
> > 
> > 		ethernet-switch@0 {
> 
> I'm not exactly clear on what exactly does the bus address (@0)
> represent here and in other (but not all) sub-nodes.
> dtc probably warns that there shouldn't be any unit address, since
> #address-cells and #size-cells are both 0 for ocelot-chip@0.

They most likely shouldn't be there. There are some warnings (make W=1
...) but they're hidden inside all sorts of warnings from am33*.dtsi
warnings. I should have been looking for those.

You're right. A lot of "has a unit name, but no reg or ranges property"
Removing the @s and giving them all unique names resolves these
warnings.

> 
> > 			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";
> > 				};
> > 			};
> > 		};
> > 
> > 		mdio0: mdio0@0 {
> > 			compatible = "mscc,ocelot-miim";
> > 			#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@1 {
> > 			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";
> > 			};
> > 		};
> > 
> > 		sgpio: sgpio {
> > 			compatible = "mscc,ocelot-sgpio";
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			bus-frequency=<12500000>;
> > 			clocks = <&ocelot_clock>;
> > 			microchip,sgpio-port-ranges = <0 15>;
> > 			pinctrl-names = "default";
> > 			pinctrl-0 = <&led_shift_reg_pins>;
> > 
> > 			sgpio_in0: sgpio@0 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <0>;
> > 				gpio-controller;
> > 				#gpio-cells = <3>;
> > 				ngpios = <64>;
> > 			};
> > 
> > 			sgpio_out1: sgpio@1 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <1>;
> > 				gpio-controller;
> > 				#gpio-cells = <3>;
> > 				ngpios = <64>;
> > 			};
> > 		};
> > 
> > 		hsio: syscon {
> > 			compatible = "mscc,ocelot-hsio", "syscon", "simple-mfd";
> > 
> > 			serdes: serdes {
> > 				compatible = "mscc,vsc7514-serdes";
> > 				#phy-cells = <2>;
> > 			};
> > 		};
> > 	};
> > };
> 
> The switch-related portion of this patch set looks good enough to me.
> I'll let somebody else with more knowledge provide feedback on the
> mfd/pinctrl/gpio/phylink/led integration aspects.

Thanks for looking. As I mentioned - I don't have any intention to make
a .dts/.dtsi for this rather obscure dev environment. It seems like it
wouldn't be useful. But the feedback has really helped keep me on track,
and hopefully avoiding scenarios where two wrongs make a right.

Re: [RFC v7 net-next 10/13] mfd: ocelot: add support for the vsc7512 chip via spi

[Lee Jones]

On Sun, 06 Mar 2022, Colin Foster wrote:

> The VSC7512 is a networking chip that contains several peripherals. Many of
> these peripherals are currently supported by the VSC7513 and VSC7514 chips,
> but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
> controlled externally.
> 
> Utilize the existing drivers by referencing the chip as an MFD. Add support
> for the two MDIO buses, the internal phys, pinctrl, serial GPIO, and HSIO.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/Kconfig       |  24 +++
>  drivers/mfd/Makefile      |   3 +
>  drivers/mfd/ocelot-core.c | 189 +++++++++++++++++++++++
>  drivers/mfd/ocelot-spi.c  | 313 ++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/ocelot.h      |  42 +++++
>  include/soc/mscc/ocelot.h |   5 +
>  6 files changed, 576 insertions(+)
>  create mode 100644 drivers/mfd/ocelot-core.c
>  create mode 100644 drivers/mfd/ocelot-spi.c
>  create mode 100644 drivers/mfd/ocelot.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ba0b3eb131f1..d4312a5252d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -948,6 +948,30 @@ config MFD_MENF21BMC
>  	  This driver can also be built as a module. If so the module
>  	  will be called menf21bmc.
>  
> +config MFD_OCELOT
> +	tristate "Microsemi Ocelot External Control Support"
> +	select MFD_CORE
> +	help
> +	  Ocelot is a family of networking chips that support multiple ethernet
> +	  and fibre interfaces. In addition to networking, they contain several
> +	  other functions, including pictrl, MDIO, and communication with
> +	  external chips. While some chips have an internal processor capable of
> +	  running an OS, others don't. All chips can be controlled externally
> +	  through different interfaces, including SPI, I2C, and PCIe.
> +
> +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> +	  VSC7513, VSC7514) controlled externally.
> +
> +	  If unsure, say N
> +
> +config MFD_OCELOT_SPI
> +	tristate "Microsemi Ocelot SPI interface"
> +	depends on MFD_OCELOT
> +	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 df1ecc4a4c95..12513843067a 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)	+= 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..36e4326a853a
> --- /dev/null
> +++ b/drivers/mfd/ocelot-core.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * MFD core driver for the Ocelot chip family.

Please drop all references to 'MFD'.

'Core' is fine, as is 'Parent'.

> + * The VSC7511, 7512, 7513, and 7514 can be controlled internally via an
> + * on-chip MIPS processor, or externally via SPI, I2C, PCIe. This core driver is
> + * intended to be the bus-agnostic glue between, for example, the SPI bus and
> + * the MFD children.
> + *
> + * Copyright 2021 Innovative Advantage Inc.

Out of date?

> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <soc/mscc/ocelot.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "ocelot.h"
> +
> +#define GCB_SOFT_RST 0x0008

Tab these out to match the others?

> +#define SOFT_CHIP_RST 0x1
> +
> +#define VSC7512_GCB_RES_START	0x71070000
> +#define VSC7512_GCB_RES_SIZE	0x14
> +
> +#define VSC7512_MIIM0_RES_START	0x7107009c
> +#define VSC7512_MIIM0_RES_SIZE	0x24
> +
> +#define VSC7512_MIIM1_RES_START	0x710700c0
> +#define VSC7512_MIIM1_RES_SIZE	0x24
> +
> +#define VSC7512_PHY_RES_START	0x710700f0
> +#define VSC7512_PHY_RES_SIZE	0x4
> +
> +#define VSC7512_HSIO_RES_START	0x710d0000
> +#define VSC7512_HSIO_RES_SIZE	0x10000
> +
> +#define VSC7512_GPIO_RES_START	0x71070034
> +#define VSC7512_GPIO_RES_SIZE	0x6c
> +
> +#define VSC7512_SIO_RES_START	0x710700f8
> +#define VSC7512_SIO_RES_SIZE	0x100
> +
> +static const struct resource vsc7512_gcb_resource =
> +	DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE,
> +			     "devcpu_gcb_chip_regs");
> +
> +static int ocelot_reset(struct ocelot_core *core)
> +{
> +	int ret;
> +
> +	/*
> +	 * Reset the entire chip here to put it into a completely known state.
> +	 * Other drivers may want to reset their own subsystems. The register
> +	 * self-clears, so one write is all that is needed
> +	 */
> +	ret = regmap_write(core->gcb_regmap, GCB_SOFT_RST, SOFT_CHIP_RST);
> +	if (ret)
> +		return ret;
> +
> +	msleep(100);
> +
> +	return ret;
> +}
> +
> +static struct regmap *ocelot_devm_regmap_init(struct ocelot_core *core,
> +					      struct device *child,
> +					      const struct resource *res)
> +{
> +	/*
> +	 * Call directly into ocelot_spi to get a new regmap. This will need to
> +	 * be expanded if additional bus types are to be supported in the
> +	 * future.
> +	 */
> +	return ocelot_spi_devm_get_regmap(core, child, res);
> +}
> +
> +struct regmap *ocelot_get_regmap_from_resource(struct device *child,
> +					       const struct resource *res)
> +{
> +	struct ocelot_core *core = dev_get_drvdata(child->parent);
> +
> +	return ocelot_devm_regmap_init(core, child, res);
> +}
> +EXPORT_SYMBOL(ocelot_get_regmap_from_resource);

What's the reason for having an additional function when one would do?

> +static const struct resource vsc7512_miim0_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM0_RES_START, VSC7512_MIIM0_RES_SIZE,
> +			     "gcb_miim0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PHY_RES_START, VSC7512_PHY_RES_SIZE,
> +			     "gcb_phy"),
> +};
> +
> +static const struct resource vsc7512_miim1_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM1_RES_START, VSC7512_MIIM1_RES_SIZE,
> +			     "gcb_miim1"),
> +};
> +
> +static const struct resource vsc7512_hsio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE,
> +			     "hsio"),
> +};
> +
> +static const struct resource vsc7512_pinctrl_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_GPIO_RES_START, VSC7512_GPIO_RES_SIZE,
> +			     "gcb_gpio"),
> +};
> +
> +static const struct resource vsc7512_sgpio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_SIO_RES_START, VSC7512_SIO_RES_SIZE,
> +			     "gcb_sio"),
> +};
> +
> +static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "ocelot-pinctrl",
> +		.of_compatible = "mscc,ocelot-pinctrl",
> +		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
> +		.resources = vsc7512_pinctrl_resources,
> +	}, {
> +		.name = "ocelot-sgpio",
> +		.of_compatible = "mscc,ocelot-sgpio",
> +		.num_resources = ARRAY_SIZE(vsc7512_sgpio_resources),
> +		.resources = vsc7512_sgpio_resources,
> +	}, {
> +		.name = "ocelot-miim0",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim0_resources),
> +		.resources = vsc7512_miim0_resources,
> +	}, {
> +		.name = "ocelot-miim1",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> +		.resources = vsc7512_miim1_resources,
> +	}, {
> +		.name = "ocelot-serdes",
> +		.of_compatible = "mscc,vsc7514-serdes",
> +		.num_resources = ARRAY_SIZE(vsc7512_hsio_resources),
> +		.resources = vsc7512_hsio_resources,
> +	},
> +};
> +
> +int ocelot_core_init(struct ocelot_core *core)
> +{
> +	struct device *dev = core->dev;
> +	int ret;
> +
> +	dev_set_drvdata(dev, core);
> +
> +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> +						   &vsc7512_gcb_resource);

This just ends up calling ocelot_spi_devm_get_regmap() right?

Why not call that from inside ocelot-spi.c where 'core' was allocated?

> +	if (IS_ERR(core->gcb_regmap))
> +		return -ENOMEM;
> +
> +	ret = ocelot_reset(core);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * A chip reset will clear the SPI configuration, so it needs to be done
> +	 * again before we can access any registers
> +	 */
> +	ret = ocelot_spi_initialize(core);

Not a fan of calling back into the file which called us.

And what happens if SPI isn't controlling us?

Doesn't the documentation above say this device can also be
communicated with via I2C and PCIe?

> +	if (ret) {
> +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_core_init);
> +
> +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> new file mode 100644
> index 000000000000..c788e239c9a7
> --- /dev/null
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * SPI core driver for the Ocelot chip family.
> + *
> + * This driver will handle everything necessary to allow for communication over
> + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> + * processor's endianness. This will create and distribute regmaps for any MFD

As above, please drop references to MFD.

> + * children.
> + *
> + * Copyright 2021 Innovative Advantage Inc.
> + *
> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + */
> +
> +#include <linux/iopoll.h>
> +#include <linux/kconfig.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "ocelot.h"
> +
> +#define DEV_CPUORG_IF_CTRL	0x0000
> +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> +
> +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> +
> +
> +static const struct resource vsc7512_dev_cpuorg_resource = {
> +	.start	= 0x71000000,
> +	.end	= 0x710002ff,

No magic numbers.  Please define these addresses.

> +	.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
> +
> +int ocelot_spi_initialize(struct ocelot_core *core)
> +{
> +	u32 val, check;
> +	int err;
> +
> +#ifdef __LITTLE_ENDIAN
> +	val = VSC7512_BYTE_ORDER_LE;
> +#else
> +	val = VSC7512_BYTE_ORDER_BE;
> +#endif

Not a fan of ifdefery in the middle of C files.

Please create a macro or define somewhere.

> +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> +	if (err)
> +		return err;

Comment.

> +	val = core->spi_padding_bytes;
> +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> +	if (err)
> +		return err;

Comment.

> +	/*
> +	 * After we write the interface configuration, read it back here. This
> +	 * will verify several different things. The first is that the number of
> +	 * padding bytes actually got written correctly. These are found in bits
> +	 * 0:3.
> +	 *
> +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> +	 * and will be set if the register access is too fast. This would be in
> +	 * the condition that the number of padding bytes is insufficient for
> +	 * the SPI bus frequency.
> +	 *
> +	 * The last check is for bits 31:24, which define the interface by which
> +	 * the registers are being accessed. Since we're accessing them via the
> +	 * serial interface, it must return IF_NUM_SI.
> +	 */
> +	check = val | CFGSTAT_IF_NUM_SI;
> +
> +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> +	if (err)
> +		return err;
> +
> +	if (check != val)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_spi_initialize);
> +
> +/*
> + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> + * the register locations are defined as 32-bit. The least significant two bits
> + * get shifted out, as register accesses must always be word-aligned, leaving
> + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> + * payload can be optimized for bit / byte order to match whatever architecture
> + * the controlling CPU has.
> + */
> +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> +{
> +	return cpu_to_be32((reg & 0xffffff) >> 2);
> +}
> +
> +struct ocelot_spi_regmap_context {
> +	u32 base;
> +	struct ocelot_core *core;
> +};
> +
> +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct ocelot_spi_regmap_context *regmap_context = context;
> +	struct ocelot_core *core = regmap_context->core;
> +	struct spi_transfer tx, padding, rx;
> +	struct spi_message msg;

How big are all of these?

We will receive warnings if they occupy too much stack space.

> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	spi = core->spi;
> +
> +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> +	tx_buf = (u8 *)&addr;
> +
> +	spi_message_init(&msg);
> +
> +	memset(&tx, 0, sizeof(tx));
> +
> +	/* 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 (core->spi_padding_bytes > 0) {
> +		u8 dummy_buf[16] = {0};
> +
> +		memset(&padding, 0, sizeof(padding));
> +
> +		/* Just toggle the clock for padding bytes */
> +		padding.len = core->spi_padding_bytes;
> +		padding.tx_buf = dummy_buf;
> +		padding.dummy_data = 1;
> +
> +		spi_message_add_tail(&padding, &msg);
> +	}
> +
> +	memset(&rx, 0, sizeof(rx));
> +	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 ocelot_core *core = regmap_context->core;
> +	struct spi_transfer tx[2] = {0};
> +	struct spi_message msg;
> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	spi = core->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,
> +};
> +
> +struct regmap *
> +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> +			   const struct resource *res)

This seems to always initialise a new Regmap.

To me 'get' implies that it could fetch an already existing one.

... and *perhaps* init a new one if none exists..

> +{
> +	struct ocelot_spi_regmap_context *context;
> +	struct regmap_config regmap_config;
> +
> +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> +	if (IS_ERR(context))
> +		return ERR_CAST(context);
> +
> +	context->base = res->start;
> +	context->core = core;
> +
> +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> +	       sizeof(ocelot_spi_regmap_config));
> +
> +	regmap_config.name = res->name;
> +	regmap_config.max_register = res->end - res->start;
> +
> +	return devm_regmap_init(child, NULL, context, &regmap_config);
> +}
> +
> +static int ocelot_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ocelot_core *core;

This would be more in keeping with current drivers if you dropped the
'_core' part of the struct name and called the variable ddata.

> +	int err;
> +
> +	core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> +	if (!core)
> +		return -ENOMEM;

If you save 'core' (or preferably ddata) here and place it in the
device's driver_data slot via dev_set_drvdata(), you can just pass
around the 'struct device' which is more in keeping with current
implementations.

> +	if (spi->max_speed_hz <= 500000) {
> +		core->spi_padding_bytes = 0;
> +	} else {
> +		/*
> +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG.
> +		 * Register access time is 1us, so we need to configure and send
> +		 * out enough padding bytes between the read request and data
> +		 * transmission that lasts at least 1 microsecond.
> +		 */
> +		core->spi_padding_bytes = 1 +
> +			(spi->max_speed_hz / 1000000 + 2) / 8;
> +	}
> +
> +	core->spi = spi;
> +
> +	spi->bits_per_word = 8;
> +
> +	err = spi_setup(spi);
> +	if (err < 0) {
> +		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> +		return err;
> +	}
> +
> +	core->cpuorg_regmap =
> +		ocelot_spi_devm_get_regmap(core, dev,
> +					   &vsc7512_dev_cpuorg_resource);
> +	if (IS_ERR(core->cpuorg_regmap))
> +		return -ENOMEM;
> +
> +	core->dev = dev;
> +
> +	/*
> +	 * The chip must be set up for SPI before it gets initialized and reset.
> +	 * This must be done before calling init, and after a chip reset is
> +	 * performed.
> +	 */
> +	err = ocelot_spi_initialize(core);
> +	if (err) {
> +		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> +		return err;
> +	}
> +
> +	err = ocelot_core_init(core);
> +	if (err < 0) {
> +		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct of_device_id ocelot_spi_of_match[] = {
> +	{ .compatible = "mscc,vsc7512_mfd_spi" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ocelot_spi_of_match);
> +
> +static struct spi_driver ocelot_spi_driver = {
> +	.driver = {
> +		.name = "ocelot_mfd_spi",
> +		.of_match_table = of_match_ptr(ocelot_spi_of_match),
> +	},
> +	.probe = ocelot_spi_probe,
> +};
> +module_spi_driver(ocelot_spi_driver);
> +
> +MODULE_DESCRIPTION("SPI Controlled Ocelot Chip Driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
> new file mode 100644
> index 000000000000..20d3853dd6d2
> --- /dev/null
> +++ b/drivers/mfd/ocelot.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright 2021 Innovative Advantage Inc.
> + */
> +
> +#include <linux/kconfig.h>
> +#include <linux/regmap.h>
> +
> +struct ocelot_core {
> +	struct device *dev;
> +	struct regmap *gcb_regmap;
> +	struct regmap *cpuorg_regmap;
> +
> +#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)

I'd drop this personally.

> +	int spi_padding_bytes;
> +	struct spi_device *spi;
> +#endif
> +};
> +
> +void ocelot_get_resource_name(char *name, const struct resource *res,
> +			      int size);
> +int ocelot_core_init(struct ocelot_core *core);
> +int ocelot_remove(struct ocelot_core *core);

This doesn't appear to be relevant.

> +#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)
> +struct regmap *ocelot_spi_devm_get_regmap(struct ocelot_core *core,
> +					  struct device *child,
> +					  const struct resource *res);
> +int ocelot_spi_initialize(struct ocelot_core *core);
> +#else
> +static inline struct regmap *ocelot_spi_devm_get_regmap(
> +		struct ocelot_core *core, struct device *child,
> +		const struct resource *res)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline int ocelot_spi_initialize(struct ocelot_core *core)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 998616511ffb..d9e2710d5646 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -1018,11 +1018,16 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_MFD_OCELOT)
> +struct regmap *ocelot_get_regmap_from_resource(struct device *child,
> +					       const struct resource *res);
> +#else
>  static inline struct regmap *
>  ocelot_get_regmap_from_resource(struct device *child,
>  				const struct resource *res)
>  {
>  	return ERR_PTR(-EOPNOTSUPP);
>  }
> +#endif
>  
>  #endif

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

Re: [RFC v7 net-next 10/13] mfd: ocelot: add support for the vsc7512 chip via spi

[Lee Jones]

[Adding everyone/lists back on Cc]

On Thu, 14 Apr 2022, Colin Foster wrote:

> Hi Lee,
> 
> Thanks for the feedback. I agree with (and have made) your suggestions.
> Additional comments below.

I'm swamped right now, so I cannot do a full re-review, but please see
in-line for a couple of (most likely flippant i.e. not fully
thought out comments).

Please submit the changes you end up making off the back of this
review and I'll conduct another on the next version you send.

> On Wed, Apr 13, 2022 at 09:32:22AM +0100, Lee Jones wrote:
> > On Sun, 06 Mar 2022, Colin Foster wrote:
> > 
> [...]
> > > +
> > > +int ocelot_core_init(struct ocelot_core *core)
> > > +{
> > > +	struct device *dev = core->dev;
> > > +	int ret;
> > > +
> > > +	dev_set_drvdata(dev, core);
> > > +
> > > +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> > > +						   &vsc7512_gcb_resource);
> > 
> > This just ends up calling ocelot_spi_devm_get_regmap() right?
> > 
> > Why not call that from inside ocelot-spi.c where 'core' was allocated?
> 
> core->gcb_regmap doesn't handle anything more than chip reset. This will
> have to happen regardless of the interface.
> 
> The "spi" part uses the core->cpuorg_regmap, which is needed for
> configuring the SPI bus. In the case of I2C, this cpu_org regmap
> (likely?) wouldn't be necessary, but the gcb_regmap absolutely would.
> That's why gcb is allocated in core and cpuorg is allocated in SPI.
> 
> The previous RFC had cpuorg_regmap hidden inside a private struct to
> emphasize this separation. As you pointed out, there was a lot of
> bouncing between "core" structs and "spi" structs that got ugly.
> 
> (Looking at this more now... the value of cpuorg_regmap should have been
> in the CONFIG_MFD_OCELOT_SPI ifdef, which might have made this
> distinction more clear)

The TL;DR of my review point would be to make this as simple as
possible.  If you can call a single function, instead of needlessly
sending the thread of execution through three, please do.

> > > +	if (IS_ERR(core->gcb_regmap))
> > > +		return -ENOMEM;
> > > +
> > > +	ret = ocelot_reset(core);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to reset device: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * A chip reset will clear the SPI configuration, so it needs to be done
> > > +	 * again before we can access any registers
> > > +	 */
> > > +	ret = ocelot_spi_initialize(core);
> > 
> > Not a fan of calling back into the file which called us.
> > 
> > And what happens if SPI isn't controlling us?
> > 
> > Doesn't the documentation above say this device can also be
> > communicated with via I2C and PCIe?
> 
> During the last RFC this was done through a callback. You had requested
> I not use callbacks.
> 
> From that exchange:
> """"
> > > > +	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.
> """"

Yes, I remember.

This is an improvement on that, but it doesn't mean it's ideal.

> The idea is that we set up the SPI bus so we can read registers. The
> protocol changes based on bus speed, so this is necessary.
> 
> This initial setup is done in ocelot-spi.c, before ocelot_core_init is
> called.
> 
> We then reset the chip by writing some registers. This chip reset also
> clears the SPI configuration, so we need to reconfigure the SPI bus
> before we can read any additional registers.
> 
> Short of using function pointers, I imagine this will have to be
> something akin to:
> 
> if (core->is_spi) {
>     ocelot_spi_initalize();
> }

What about if, instead of calling from SPI into Core, which calls back
into SPI again, you do this from SPI instead:

[flippant - I haven't fully assessed the viability of this suggestion]

foo_type spi_probe(bar_type baz)
{
  setup_spi();

  core_init();

  more_spi_stuff();
}

> I feel if the additional buses are added, they'll have to implement this
> type of change. But as I don't (and don't plan to) have hardware to
> build those interfaces out, right now ocelot_core assumes the bus is
> SPI.

What are the chances of someone else coming along and implementing the
other interfaces?  You might very well be over-complicating this
implementation for support that may never be required.

> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> > > +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(ocelot_core_init);
> > > +
> > > +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> > > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > > new file mode 100644
> > > index 000000000000..c788e239c9a7
> > > --- /dev/null
> > > +++ b/drivers/mfd/ocelot-spi.c
> > > @@ -0,0 +1,313 @@
> > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > > +/*
> > > + * SPI core driver for the Ocelot chip family.
> > > + *
> > > + * This driver will handle everything necessary to allow for communication over
> > > + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> > > + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> > > + * processor's endianness. This will create and distribute regmaps for any MFD
> > 
> > As above, please drop references to MFD.
> > 
> > > + * children.
> > > + *
> > > + * Copyright 2021 Innovative Advantage Inc.
> > > + *
> > > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > > + */
> > > +
> > > +#include <linux/iopoll.h>
> > > +#include <linux/kconfig.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#include <asm/byteorder.h>
> > > +
> > > +#include "ocelot.h"
> > > +
> > > +#define DEV_CPUORG_IF_CTRL	0x0000
> > > +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> > > +
> > > +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> > > +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> > > +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> > > +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> > > +
> > > +
> > > +static const struct resource vsc7512_dev_cpuorg_resource = {
> > > +	.start	= 0x71000000,
> > > +	.end	= 0x710002ff,
> > 
> > No magic numbers.  Please define these addresses.
> 
> I missed these. Thanks.
> 
> > 
> > > +	.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
> > > +
> > > +int ocelot_spi_initialize(struct ocelot_core *core)
> > > +{
> > > +	u32 val, check;
> > > +	int err;
> > > +
> > > +#ifdef __LITTLE_ENDIAN
> > > +	val = VSC7512_BYTE_ORDER_LE;
> > > +#else
> > > +	val = VSC7512_BYTE_ORDER_BE;
> > > +#endif
> > 
> > Not a fan of ifdefery in the middle of C files.
> > 
> > Please create a macro or define somewhere.
> 
> I'll clear this up in comments and move things around. This macro
> specifically tends to lend itself to this type of ifdef dropping:
> 
> https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/__LITTLE_ENDIAN

I see that the majority of implementations exist in header files as I
would expect.  With respect to the others, past acceptance and what is
acceptable in other subsystems has little bearing on what will be
accepted here and now.

> The comment I'm adding is:
>         /*
>          * The SPI address must be big-endian, but we want the payload to match
>          * our CPU. These are two bits (0 and 1) but they're repeated such that
>          * the write from any configuration will be valid. The four
>          * configurations are:
>          *
>          * 0b00: little-endian, MSB first
>          * |            111111   | 22221111 | 33222222 |
>          * | 76543210 | 54321098 | 32109876 | 10987654 |
>          *
>          * 0b01: big-endian, MSB first
>          * | 33222222 | 22221111 | 111111   |          |
>          * | 10987654 | 32109876 | 54321098 | 76543210 |
>          *
>          * 0b10: little-endian, LSB first
>          * |              111111 | 11112222 | 22222233 |
>          * | 01234567 | 89012345 | 67890123 | 45678901 |
>          *
>          * 0b11: big-endian, LSB first
>          * | 22222233 | 11112222 |   111111 |          |
>          * | 45678901 | 67890123 | 89012345 | 01234567 |
>          */
> 
> With this info, would you recommend:
> 1. A file-scope static const at the top of this file
> 2. A macro assigned to one of those sequences
> 3. A function to "detect" which architecture we're running

I do not have a strong opinion.

Just tuck the #iferry away somewhere in a header file.

> > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> > > +	if (err)
> > > +		return err;
> > 
> > Comment.
> > 
> > > +	val = core->spi_padding_bytes;
> > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> > > +	if (err)
> > > +		return err;
> > 
> > Comment.
> 
> Adding:
> 
> /*
>  * Apply the number of padding bytes between a read request and the data
>  * payload. Some registers have access times of up to 1us, so if the
>  * first payload bit is shifted out too quickly, the read will fail.
>  */
> 
> > 
> > > +	/*
> > > +	 * After we write the interface configuration, read it back here. This
> > > +	 * will verify several different things. The first is that the number of
> > > +	 * padding bytes actually got written correctly. These are found in bits
> > > +	 * 0:3.
> > > +	 *
> > > +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> > > +	 * and will be set if the register access is too fast. This would be in
> > > +	 * the condition that the number of padding bytes is insufficient for
> > > +	 * the SPI bus frequency.
> > > +	 *
> > > +	 * The last check is for bits 31:24, which define the interface by which
> > > +	 * the registers are being accessed. Since we're accessing them via the
> > > +	 * serial interface, it must return IF_NUM_SI.
> > > +	 */
> > > +	check = val | CFGSTAT_IF_NUM_SI;
> > > +
> > > +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (check != val)
> > > +		return -ENODEV;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(ocelot_spi_initialize);
> > > +
> > > +/*
> > > + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> > > + * the register locations are defined as 32-bit. The least significant two bits
> > > + * get shifted out, as register accesses must always be word-aligned, leaving
> > > + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> > > + * payload can be optimized for bit / byte order to match whatever architecture
> > > + * the controlling CPU has.
> > > + */
> > > +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> > > +{
> > > +	return cpu_to_be32((reg & 0xffffff) >> 2);
> > > +}
> > > +
> > > +struct ocelot_spi_regmap_context {
> > > +	u32 base;
> > > +	struct ocelot_core *core;
> > > +};
> > > +
> > > +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> > > +			       unsigned int *val)
> > > +{
> > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > +	struct ocelot_core *core = regmap_context->core;
> > > +	struct spi_transfer tx, padding, rx;
> > > +	struct spi_message msg;
> > 
> > How big are all of these?
> > 
> > We will receive warnings if they occupy too much stack space.
> 
> Looking at the structs they're on the order of 10s of bytes. Maybe 70
> bytes per instance (back of napkin calculation)
> 
> But it seems very common to stack-allocate spi_transfers:
> 
> https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/spi/spi.c#L4097
> https://elixir.bootlin.com/linux/v5.18-rc2/source/include/linux/spi/spi.h#L1244
> 
> Do you have a feel for at what point that becomes a concern?

That's fine.  I just want you to bear this in mind.

I wish to prevent adding yet more W=1 level warnings into the kernel.

> > > +	struct spi_device *spi;
> > > +	unsigned int addr;
> > > +	u8 *tx_buf;
> > > +
> > > +	spi = core->spi;
> > > +
> > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > +	tx_buf = (u8 *)&addr;
> > > +
> > > +	spi_message_init(&msg);
> > > +
> > > +	memset(&tx, 0, sizeof(tx));
> > > +
> > > +	/* 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 (core->spi_padding_bytes > 0) {
> > > +		u8 dummy_buf[16] = {0};
> > > +
> > > +		memset(&padding, 0, sizeof(padding));
> > > +
> > > +		/* Just toggle the clock for padding bytes */
> > > +		padding.len = core->spi_padding_bytes;
> > > +		padding.tx_buf = dummy_buf;
> > > +		padding.dummy_data = 1;
> > > +
> > > +		spi_message_add_tail(&padding, &msg);
> > > +	}
> > > +
> > > +	memset(&rx, 0, sizeof(rx));
> > > +	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 ocelot_core *core = regmap_context->core;
> > > +	struct spi_transfer tx[2] = {0};
> > > +	struct spi_message msg;
> > > +	struct spi_device *spi;
> > > +	unsigned int addr;
> > > +	u8 *tx_buf;
> > > +
> > > +	spi = core->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,
> > > +};
> > > +
> > > +struct regmap *
> > > +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> > > +			   const struct resource *res)
> > 
> > This seems to always initialise a new Regmap.
> > 
> > To me 'get' implies that it could fetch an already existing one.
> > 
> > ... and *perhaps* init a new one if none exists..
> 
> That's exactly what my intention was when I started.
> 
> But it seems like *if* that is something that is required, it should be
> done through a syscon / device tree implementation and not be snuck into
> this regmap getter. I was trying to do too much.
> 
> I'm renaming to "init"
> 
> > 
> > > +{
> > > +	struct ocelot_spi_regmap_context *context;
> > > +	struct regmap_config regmap_config;
> > > +
> > > +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> > > +	if (IS_ERR(context))
> > > +		return ERR_CAST(context);
> > > +
> > > +	context->base = res->start;
> > > +	context->core = core;
> > > +
> > > +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > > +	       sizeof(ocelot_spi_regmap_config));
> > > +
> > > +	regmap_config.name = res->name;
> > > +	regmap_config.max_register = res->end - res->start;
> > > +
> > > +	return devm_regmap_init(child, NULL, context, &regmap_config);
> > > +}
> > > +
> > > +static int ocelot_spi_probe(struct spi_device *spi)
> > > +{
> > > +	struct device *dev = &spi->dev;
> > > +	struct ocelot_core *core;
> > 
> > This would be more in keeping with current drivers if you dropped the
> > '_core' part of the struct name and called the variable ddata.
> 
> There's already a "struct ocelot" defined in include/soc/mscc/ocelot.h.
> I suppose it could be renamed to align with what it actually is: the
> "switch" component of the ocelot chip.
> 
> Vladimir, Alexandre, Horaitu, others:
> Any opinions about this becoming "struct ocelot" and the current struct
> being "struct ocelot_switch"?
> 
> Or maybe a technical / philosophical question: is "ocelot" the switch
> core that can be implemented in other hardware? Or is it the chip family
> entirely, (pinctrl, sgpio, etc.) who's switch core was brought into
> other products?
> 
> The existing struct change would hit about 30 files.
> https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/ocelot

That's not ideal.

Please consider using 'ocelot_ddata' for now and consider a larger
overhaul at a later date, if it makes sense to do so.

[...]

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

Re: [RFC v7 net-next 10/13] mfd: ocelot: add support for the vsc7512 chip via spi

[Colin Foster]

Thanks for the feedback Lee,

I'll do some final cleanup (hopefully this month...?) and prepare
another patch set.


Something I plan to do, lest anyone object, is send the next patch set
that explicitly states that ports 4-10 currently aren't supported, and 
inform a user as such. One main reason for this is that the additional
ports rely on the drivers/phy/mscc/phy-ocelot-serdes driver, which
utilizes syscon_node_to_regmap. The same issue comes up, where syscon of
course only supports mmio. I can foresee that being worthy of its own
rounds of reviews, and will change the .

But I'm quite new to this process - so if that isn't an acceptable path
forward I understand.


On Tue, Apr 19, 2022 at 10:07:04AM +0100, Lee Jones wrote:
> [Adding everyone/lists back on Cc]

Oops... I'm not sure how I did that. Thanks!

> 
> On Thu, 14 Apr 2022, Colin Foster wrote:
> 
> > Hi Lee,
> > 
> > Thanks for the feedback. I agree with (and have made) your suggestions.
> > Additional comments below.
> 
> I'm swamped right now, so I cannot do a full re-review, but please see
> in-line for a couple of (most likely flippant i.e. not fully
> thought out comments).
> 
> Please submit the changes you end up making off the back of this
> review and I'll conduct another on the next version you send.
> 
> > On Wed, Apr 13, 2022 at 09:32:22AM +0100, Lee Jones wrote:
> > > On Sun, 06 Mar 2022, Colin Foster wrote:
> > > 
> > [...]
> > > > +
> > > > +int ocelot_core_init(struct ocelot_core *core)
> > > > +{
> > > > +	struct device *dev = core->dev;
> > > > +	int ret;
> > > > +
> > > > +	dev_set_drvdata(dev, core);
> > > > +
> > > > +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> > > > +						   &vsc7512_gcb_resource);
> > > 
> > > This just ends up calling ocelot_spi_devm_get_regmap() right?
> > > 
> > > Why not call that from inside ocelot-spi.c where 'core' was allocated?
> > 
> > core->gcb_regmap doesn't handle anything more than chip reset. This will
> > have to happen regardless of the interface.
> > 
> > The "spi" part uses the core->cpuorg_regmap, which is needed for
> > configuring the SPI bus. In the case of I2C, this cpu_org regmap
> > (likely?) wouldn't be necessary, but the gcb_regmap absolutely would.
> > That's why gcb is allocated in core and cpuorg is allocated in SPI.
> > 
> > The previous RFC had cpuorg_regmap hidden inside a private struct to
> > emphasize this separation. As you pointed out, there was a lot of
> > bouncing between "core" structs and "spi" structs that got ugly.
> > 
> > (Looking at this more now... the value of cpuorg_regmap should have been
> > in the CONFIG_MFD_OCELOT_SPI ifdef, which might have made this
> > distinction more clear)
> 
> The TL;DR of my review point would be to make this as simple as
> possible.  If you can call a single function, instead of needlessly
> sending the thread of execution through three, please do.
> 
> > > > +	if (IS_ERR(core->gcb_regmap))
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = ocelot_reset(core);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to reset device: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * A chip reset will clear the SPI configuration, so it needs to be done
> > > > +	 * again before we can access any registers
> > > > +	 */
> > > > +	ret = ocelot_spi_initialize(core);
> > > 
> > > Not a fan of calling back into the file which called us.
> > > 
> > > And what happens if SPI isn't controlling us?
> > > 
> > > Doesn't the documentation above say this device can also be
> > > communicated with via I2C and PCIe?
> > 
> > During the last RFC this was done through a callback. You had requested
> > I not use callbacks.
> > 
> > From that exchange:
> > """"
> > > > > +	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.
> > """"
> 
> Yes, I remember.
> 
> This is an improvement on that, but it doesn't mean it's ideal.
> 
> > The idea is that we set up the SPI bus so we can read registers. The
> > protocol changes based on bus speed, so this is necessary.
> > 
> > This initial setup is done in ocelot-spi.c, before ocelot_core_init is
> > called.
> > 
> > We then reset the chip by writing some registers. This chip reset also
> > clears the SPI configuration, so we need to reconfigure the SPI bus
> > before we can read any additional registers.
> > 
> > Short of using function pointers, I imagine this will have to be
> > something akin to:
> > 
> > if (core->is_spi) {
> >     ocelot_spi_initalize();
> > }
> 
> What about if, instead of calling from SPI into Core, which calls back
> into SPI again, you do this from SPI instead:
> 
> [flippant - I haven't fully assessed the viability of this suggestion]
> 
> foo_type spi_probe(bar_type baz)
> {
>   setup_spi();
> 
>   core_init();
> 
>   more_spi_stuff();
> }
> 
> > I feel if the additional buses are added, they'll have to implement this
> > type of change. But as I don't (and don't plan to) have hardware to
> > build those interfaces out, right now ocelot_core assumes the bus is
> > SPI.
> 
> What are the chances of someone else coming along and implementing the
> other interfaces?  You might very well be over-complicating this
> implementation for support that may never be required.

I had one person email me about this already, though I understand they
went another direction.

But I could see this back-and-forth going away. I'll take another look
at it and try to clean it up a little more.

> 
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> > > > +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_core_init);
> > > > +
> > > > +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> > > > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > > > new file mode 100644
> > > > index 000000000000..c788e239c9a7
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/ocelot-spi.c
> > > > @@ -0,0 +1,313 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > > > +/*
> > > > + * SPI core driver for the Ocelot chip family.
> > > > + *
> > > > + * This driver will handle everything necessary to allow for communication over
> > > > + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> > > > + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> > > > + * processor's endianness. This will create and distribute regmaps for any MFD
> > > 
> > > As above, please drop references to MFD.
> > > 
> > > > + * children.
> > > > + *
> > > > + * Copyright 2021 Innovative Advantage Inc.
> > > > + *
> > > > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > > > + */
> > > > +
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/kconfig.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/spi/spi.h>
> > > > +
> > > > +#include <asm/byteorder.h>
> > > > +
> > > > +#include "ocelot.h"
> > > > +
> > > > +#define DEV_CPUORG_IF_CTRL	0x0000
> > > > +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> > > > +
> > > > +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> > > > +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> > > > +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> > > > +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> > > > +
> > > > +
> > > > +static const struct resource vsc7512_dev_cpuorg_resource = {
> > > > +	.start	= 0x71000000,
> > > > +	.end	= 0x710002ff,
> > > 
> > > No magic numbers.  Please define these addresses.
> > 
> > I missed these. Thanks.
> > 
> > > 
> > > > +	.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
> > > > +
> > > > +int ocelot_spi_initialize(struct ocelot_core *core)
> > > > +{
> > > > +	u32 val, check;
> > > > +	int err;
> > > > +
> > > > +#ifdef __LITTLE_ENDIAN
> > > > +	val = VSC7512_BYTE_ORDER_LE;
> > > > +#else
> > > > +	val = VSC7512_BYTE_ORDER_BE;
> > > > +#endif
> > > 
> > > Not a fan of ifdefery in the middle of C files.
> > > 
> > > Please create a macro or define somewhere.
> > 
> > I'll clear this up in comments and move things around. This macro
> > specifically tends to lend itself to this type of ifdef dropping:
> > 
> > https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/__LITTLE_ENDIAN
> 
> I see that the majority of implementations exist in header files as I
> would expect.  With respect to the others, past acceptance and what is
> acceptable in other subsystems has little bearing on what will be
> accepted here and now.
> 
> > The comment I'm adding is:
> >         /*
> >          * The SPI address must be big-endian, but we want the payload to match
> >          * our CPU. These are two bits (0 and 1) but they're repeated such that
> >          * the write from any configuration will be valid. The four
> >          * configurations are:
> >          *
> >          * 0b00: little-endian, MSB first
> >          * |            111111   | 22221111 | 33222222 |
> >          * | 76543210 | 54321098 | 32109876 | 10987654 |
> >          *
> >          * 0b01: big-endian, MSB first
> >          * | 33222222 | 22221111 | 111111   |          |
> >          * | 10987654 | 32109876 | 54321098 | 76543210 |
> >          *
> >          * 0b10: little-endian, LSB first
> >          * |              111111 | 11112222 | 22222233 |
> >          * | 01234567 | 89012345 | 67890123 | 45678901 |
> >          *
> >          * 0b11: big-endian, LSB first
> >          * | 22222233 | 11112222 |   111111 |          |
> >          * | 45678901 | 67890123 | 89012345 | 01234567 |
> >          */
> > 
> > With this info, would you recommend:
> > 1. A file-scope static const at the top of this file
> > 2. A macro assigned to one of those sequences
> > 3. A function to "detect" which architecture we're running
> 
> I do not have a strong opinion.
> 
> Just tuck the #iferry away somewhere in a header file.

Will do

> 
> > > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> > > > +	if (err)
> > > > +		return err;
> > > 
> > > Comment.
> > > 
> > > > +	val = core->spi_padding_bytes;
> > > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> > > > +	if (err)
> > > > +		return err;
> > > 
> > > Comment.
> > 
> > Adding:
> > 
> > /*
> >  * Apply the number of padding bytes between a read request and the data
> >  * payload. Some registers have access times of up to 1us, so if the
> >  * first payload bit is shifted out too quickly, the read will fail.
> >  */
> > 
> > > 
> > > > +	/*
> > > > +	 * After we write the interface configuration, read it back here. This
> > > > +	 * will verify several different things. The first is that the number of
> > > > +	 * padding bytes actually got written correctly. These are found in bits
> > > > +	 * 0:3.
> > > > +	 *
> > > > +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> > > > +	 * and will be set if the register access is too fast. This would be in
> > > > +	 * the condition that the number of padding bytes is insufficient for
> > > > +	 * the SPI bus frequency.
> > > > +	 *
> > > > +	 * The last check is for bits 31:24, which define the interface by which
> > > > +	 * the registers are being accessed. Since we're accessing them via the
> > > > +	 * serial interface, it must return IF_NUM_SI.
> > > > +	 */
> > > > +	check = val | CFGSTAT_IF_NUM_SI;
> > > > +
> > > > +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	if (check != val)
> > > > +		return -ENODEV;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_spi_initialize);
> > > > +
> > > > +/*
> > > > + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> > > > + * the register locations are defined as 32-bit. The least significant two bits
> > > > + * get shifted out, as register accesses must always be word-aligned, leaving
> > > > + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> > > > + * payload can be optimized for bit / byte order to match whatever architecture
> > > > + * the controlling CPU has.
> > > > + */
> > > > +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> > > > +{
> > > > +	return cpu_to_be32((reg & 0xffffff) >> 2);
> > > > +}
> > > > +
> > > > +struct ocelot_spi_regmap_context {
> > > > +	u32 base;
> > > > +	struct ocelot_core *core;
> > > > +};
> > > > +
> > > > +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> > > > +			       unsigned int *val)
> > > > +{
> > > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > > +	struct ocelot_core *core = regmap_context->core;
> > > > +	struct spi_transfer tx, padding, rx;
> > > > +	struct spi_message msg;
> > > 
> > > How big are all of these?
> > > 
> > > We will receive warnings if they occupy too much stack space.
> > 
> > Looking at the structs they're on the order of 10s of bytes. Maybe 70
> > bytes per instance (back of napkin calculation)
> > 
> > But it seems very common to stack-allocate spi_transfers:
> > 
> > https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/spi/spi.c#L4097
> > https://elixir.bootlin.com/linux/v5.18-rc2/source/include/linux/spi/spi.h#L1244
> > 
> > Do you have a feel for at what point that becomes a concern?
> 
> That's fine.  I just want you to bear this in mind.
> 
> I wish to prevent adding yet more W=1 level warnings into the kernel.
> 
> > > > +	struct spi_device *spi;
> > > > +	unsigned int addr;
> > > > +	u8 *tx_buf;
> > > > +
> > > > +	spi = core->spi;
> > > > +
> > > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > > +	tx_buf = (u8 *)&addr;
> > > > +
> > > > +	spi_message_init(&msg);
> > > > +
> > > > +	memset(&tx, 0, sizeof(tx));
> > > > +
> > > > +	/* 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 (core->spi_padding_bytes > 0) {
> > > > +		u8 dummy_buf[16] = {0};
> > > > +
> > > > +		memset(&padding, 0, sizeof(padding));
> > > > +
> > > > +		/* Just toggle the clock for padding bytes */
> > > > +		padding.len = core->spi_padding_bytes;
> > > > +		padding.tx_buf = dummy_buf;
> > > > +		padding.dummy_data = 1;
> > > > +
> > > > +		spi_message_add_tail(&padding, &msg);
> > > > +	}
> > > > +
> > > > +	memset(&rx, 0, sizeof(rx));
> > > > +	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 ocelot_core *core = regmap_context->core;
> > > > +	struct spi_transfer tx[2] = {0};
> > > > +	struct spi_message msg;
> > > > +	struct spi_device *spi;
> > > > +	unsigned int addr;
> > > > +	u8 *tx_buf;
> > > > +
> > > > +	spi = core->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,
> > > > +};
> > > > +
> > > > +struct regmap *
> > > > +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> > > > +			   const struct resource *res)
> > > 
> > > This seems to always initialise a new Regmap.
> > > 
> > > To me 'get' implies that it could fetch an already existing one.
> > > 
> > > ... and *perhaps* init a new one if none exists..
> > 
> > That's exactly what my intention was when I started.
> > 
> > But it seems like *if* that is something that is required, it should be
> > done through a syscon / device tree implementation and not be snuck into
> > this regmap getter. I was trying to do too much.
> > 
> > I'm renaming to "init"
> > 
> > > 
> > > > +{
> > > > +	struct ocelot_spi_regmap_context *context;
> > > > +	struct regmap_config regmap_config;
> > > > +
> > > > +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> > > > +	if (IS_ERR(context))
> > > > +		return ERR_CAST(context);
> > > > +
> > > > +	context->base = res->start;
> > > > +	context->core = core;
> > > > +
> > > > +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > > > +	       sizeof(ocelot_spi_regmap_config));
> > > > +
> > > > +	regmap_config.name = res->name;
> > > > +	regmap_config.max_register = res->end - res->start;
> > > > +
> > > > +	return devm_regmap_init(child, NULL, context, &regmap_config);
> > > > +}
> > > > +
> > > > +static int ocelot_spi_probe(struct spi_device *spi)
> > > > +{
> > > > +	struct device *dev = &spi->dev;
> > > > +	struct ocelot_core *core;
> > > 
> > > This would be more in keeping with current drivers if you dropped the
> > > '_core' part of the struct name and called the variable ddata.
> > 
> > There's already a "struct ocelot" defined in include/soc/mscc/ocelot.h.
> > I suppose it could be renamed to align with what it actually is: the
> > "switch" component of the ocelot chip.
> > 
> > Vladimir, Alexandre, Horaitu, others:
> > Any opinions about this becoming "struct ocelot" and the current struct
> > being "struct ocelot_switch"?
> > 
> > Or maybe a technical / philosophical question: is "ocelot" the switch
> > core that can be implemented in other hardware? Or is it the chip family
> > entirely, (pinctrl, sgpio, etc.) who's switch core was brought into
> > other products?
> > 
> > The existing struct change would hit about 30 files.
> > https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/ocelot
> 
> That's not ideal.
> 
> Please consider using 'ocelot_ddata' for now and consider a larger
> overhaul at a later date, if it makes sense to do so.
> 
> [...]
> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog