lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220103190612.GA2813119@euler> Date: Mon, 3 Jan 2022 11:06:12 -0800 From: Colin Foster <colin.foster@...advantage.com> To: Lee Jones <lee.jones@...aro.org> Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, linux-gpio@...r.kernel.org, Vladimir Oltean <vladimir.oltean@....com>, Linus Walleij <linus.walleij@...aro.org>, katie.morris@...advantage.com Subject: Re: [RFC v1] mfd: pinctrl: RFC only: add and utilze mfd option in pinctrl-ocelot On Mon, Dec 06, 2021 at 08:55:25AM +0000, Lee Jones wrote: > On Fri, 03 Dec 2021, Colin Foster wrote: > > > 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, ®map_config); > > + if (device_is_mfd(pdev)) > > + info->map = dev_get_regmap(dev->parent, "GCB"); > > + else > > + info->map = devm_regmap_init_mmio(dev, base, ®map_config); > > What happens if you were to call the wrong API in either scenario? > > If the answer is 'the call would fail', then why not call one and if > it fails, call the other? With provided commits describing the > reason for the stacked calls of course. Hi Lee, As I said in the other thread, sorry for missing this response. My email blocked it. That's a good idea. I'll keep this in mind when the final "get_mfd_regamp" implementation shakes out. Thanks for the feedback! > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists