Message-Id: <20211203211611.946658-1-colin.foster@in-advantage.com>
Date:   Fri,  3 Dec 2021 13:16:11 -0800
From:   Colin Foster <colin.foster@...advantage.com>
To:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-gpio@...r.kernel.org
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        katie.morris@...advantage.com
Subject: [RFC v1] mfd: pinctrl: RFC only: add and utilze mfd option in pinctrl-ocelot

This is a psuedo-commit, but one that tells the complete story of what I'm
looking at. During an actual submission this'll be broken up into two
commits, but I'd like to get some feedback on whether this is the correct
path for me to be going down.

Background:

Microchip has a family of chips - the VSC7511, 7512, 7513, and 7514. The
last two have an internal MIPS processor, which are supported by
drivers/net/ethernet/mscc/ocelot_*. The former two lack this processor.

All four chips can be configured externally via a number of interfaces:
SPI, I2C, PCIe... This is currently not supported and is my end goal.

The networking portion of these chips have been reused in other products as
well. These utilize the common code by way of mscc_ocelot_switch_lib and
net/dsa/ocelot/*. Specifically the "Felix" driver.

Current status:

I've put out a few RFCs on the "ocelot_spi" driver. It utilizes Felix and
invokes much of the network portion of the hardware (VSC7512). It works
great! Thanks community :)

There's more hardware that needs to get configured, however. Currently that
includes general pin configuration, and an optional serial GPIO expander.
The former is supported by drivers/pinctrl/pinctrl-ocelot.c and the latter
by drivers/pinctrl/pinctrl-microchip-sgpio.c.

These drivers have been updated to use regmap instead of iomem, but that
isn't the complete story. There are two options I know about, and maybe
others I don't.

Option 1 - directly hook into the driver:

This was the path that was done in
commit b99658452355 ("net: dsa: ocelot: felix: utilize shared mscc-miim
driver for indirect MDIO access").
This is in the net-next tree. In this case the Seville driver passes in its
regmap to the mscc_miim_setup function, which bypasses mscc_miim_probe but
allows the same driver to be used.

This was my initial implementation to hook into pinctrl-ocelot and
pinctrl-microchip-sgpio. The good thing about this implementation is I have
direct control over the order of things happening. For instance, pinctrl
might need to be configured before the MDIO bus gets probed.

The bad thing about this implementation is... it doesn't work yet. My
memory is fuzzy on this, but I recall noticing that the application of a
devicetree pinctrl function happens in the driver probe. I ventured down
this path of walking the devicetree, applying pincfg, etc. That was a path
to darkness that I have abandoned.

What is functioning is I have debugfs / sysfs control, so I do have the
ability to do some runtime testing and verification.

Option 2 - MFD (this "patch")

It really seems like anything in drivers/net/dsa/ should avoid
drivers/pinctl, and that MFD is the answer. This adds some complexity to
pinctrl-ocelot, and I'm not sure whether that breaks the concept of MFD. So
it seems like I'm either doing something unique, or I'm doing something
wrong.

I err on the assumption that I'm doing something wrong.

pinctrl-ocelot gets its resources the device tree by way of
platform_get_and_ioremap_resource. This driver has been updated to support
regmap in the pinctrl tree:
commit 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")

The problem comes about when this driver is probed by way of
"mfd_add_devices". In an ideal world it seems like this would be handled by
resources. MFD adds resources to the device before it gets probed. The
probe happens and the driver is happy because platform_get_resource
succeeds.

In this scenario the device gets probed, but needs to know how to get its
regmap... not its resource. In the "I'm running from an internal chip"
scenario, the existing process of "devm_regmap_init_mmio" will suffice. But
in the "I'm running from an externally controlled setup via {SPI,I2C,PCIe}"
the process needs to be "get me this regmap from my parent". It seems like
dev_get_regmap is a perfect candidate for this.

Perhaps there is something I'm missing in the concept of resources /
regmaps. But it seems like pinctrl-ocelot needs to know whether it is in an
MFD scenario, and that concept didn't exist. Hence the addition of
device_is_mfd as part of this patch. Since "device_is_mfd" didn't exist, it
feels like I might be breaking the concept of MFD.

I think this would lend itself to a pretty elegant architecture for the
VSC751X externally controlled chips. In a manner similar to
drivers/mfd/madera* there would be small drivers handling the prococol
layers for SPI, I2C... A core driver would handle the register mappings,
and could be gotten by dev_get_regmap. Every sub-device (DSA, pinctrl,
other pinctrl, other things I haven't considered yet) could either rely on
dev->parent directly, or in this case adjust. I can't imagine a scenario
where someone would want pinctrl for the VSC7512 without the DSA side of
things... but that would be possible in this architecture that would
otherwise not.

Signed-off-by: Colin Foster <colin.foster@...advantage.com>
---
 drivers/mfd/mfd-core.c           | 6 ++++++
 drivers/pinctrl/pinctrl-ocelot.c | 7 ++++++-
 include/linux/mfd/core.h         | 2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 684a011a6396..2ba6a692499b 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -33,6 +33,12 @@ static struct device_type mfd_dev_type = {
 	.name	= "mfd_device",
 };
 
+int device_is_mfd(struct platform_device *pdev)
+{
+	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
+}
+EXPORT_SYMBOL(device_is_mfd);
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 0a36ec8775a3..758fbc225244 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -10,6 +10,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/mfd/core.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
@@ -1368,7 +1369,11 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 
 	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
 
-	info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	if (device_is_mfd(pdev))
+		info->map = dev_get_regmap(dev->parent, "GCB");
+	else
+		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);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 0bc7cba798a3..9980bcc8456d 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -123,6 +123,8 @@ struct mfd_cell {
 	int			num_parent_supplies;
 };
 
+int device_is_mfd(struct platform_device *pdev);
+
 /*
  * Convenience functions for clients using shared cells.  Refcounting
  * happens automatically, with the cell's enable/disable callbacks
-- 
2.25.1

Powered by blists - more mailing lists