| Message ID | 20220911200244.549029-8-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for the the vsc7512 internal copper phys | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Sun, Sep 11, 2022 at 01:02:43PM -0700, Colin Foster wrote: > The Ocelot switch core driver relies heavily on a fixed array of resources > for both ports and peripherals. This is in contrast to existing peripherals > - pinctrl for example - which have a one-to-one mapping of driver <> > resource. As such, these regmaps must be created differently so that > enumeration-based offsets are preserved. > > Register the regmaps to the core MFD device unconditionally so they can be > referenced by the Ocelot switch / Felix DSA systems. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > > v1 from previous RFC: > * New patch > > --- > drivers/mfd/ocelot-core.c | 88 +++++++++++++++++++++++++++++++++++--- > include/linux/mfd/ocelot.h | 5 +++ > 2 files changed, 88 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c > index 1816d52c65c5..aa7fa21b354c 100644 > --- a/drivers/mfd/ocelot-core.c > +++ b/drivers/mfd/ocelot-core.c > @@ -34,16 +34,55 @@ > > #define VSC7512_MIIM0_RES_START 0x7107009c > #define VSC7512_MIIM1_RES_START 0x710700c0 > -#define VSC7512_MIIM_RES_SIZE 0x024 > +#define VSC7512_MIIM_RES_SIZE 0x00000024 > > #define VSC7512_PHY_RES_START 0x710700f0 > -#define VSC7512_PHY_RES_SIZE 0x004 > +#define VSC7512_PHY_RES_SIZE 0x00000004 > > #define VSC7512_GPIO_RES_START 0x71070034 > -#define VSC7512_GPIO_RES_SIZE 0x06c > +#define VSC7512_GPIO_RES_SIZE 0x0000006c > > #define VSC7512_SIO_CTRL_RES_START 0x710700f8 > -#define VSC7512_SIO_CTRL_RES_SIZE 0x100 > +#define VSC7512_SIO_CTRL_RES_SIZE 0x00000100 Split the gratuitous changes to _RES_SIZE to a separate patch please, as they're just noise here? > + > +#define VSC7512_HSIO_RES_START 0x710d0000 > +#define VSC7512_HSIO_RES_SIZE 0x00000128 > + > +#define VSC7512_ANA_RES_START 0x71880000 > +#define VSC7512_ANA_RES_SIZE 0x00010000 > + > +#define VSC7512_QS_RES_START 0x71080000 > +#define VSC7512_QS_RES_SIZE 0x00000100 > + > +#define VSC7512_QSYS_RES_START 0x71800000 > +#define VSC7512_QSYS_RES_SIZE 0x00200000 > + > +#define VSC7512_REW_RES_START 0x71030000 > +#define VSC7512_REW_RES_SIZE 0x00010000 > + > +#define VSC7512_SYS_RES_START 0x71010000 > +#define VSC7512_SYS_RES_SIZE 0x00010000 > + > +#define VSC7512_S0_RES_START 0x71040000 > +#define VSC7512_S1_RES_START 0x71050000 > +#define VSC7512_S2_RES_START 0x71060000 > +#define VSC7512_S_RES_SIZE 0x00000400 > + > +#define VSC7512_GCB_RES_START 0x71070000 > +#define VSC7512_GCB_RES_SIZE 0x0000022c > + > +#define VSC7512_PORT_0_RES_START 0x711e0000 > +#define VSC7512_PORT_1_RES_START 0x711f0000 > +#define VSC7512_PORT_2_RES_START 0x71200000 > +#define VSC7512_PORT_3_RES_START 0x71210000 > +#define VSC7512_PORT_4_RES_START 0x71220000 > +#define VSC7512_PORT_5_RES_START 0x71230000 > +#define VSC7512_PORT_6_RES_START 0x71240000 > +#define VSC7512_PORT_7_RES_START 0x71250000 > +#define VSC7512_PORT_8_RES_START 0x71260000 > +#define VSC7512_PORT_9_RES_START 0x71270000 > +#define VSC7512_PORT_10_RES_START 0x71280000 > +#define VSC7512_PORT_RES_SIZE 0x00010000 > > #define VSC7512_GCB_RST_SLEEP_US 100 > #define VSC7512_GCB_RST_TIMEOUT_US 100000 > @@ -96,6 +135,34 @@ static const struct resource vsc7512_sgpio_resources[] = { > DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"), > }; > > +const struct resource vsc7512_target_io_res[TARGET_MAX] = { > + [ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"), > + [QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"), > + [QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"), > + [REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"), > + [SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"), > + [S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"), > + [S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"), > + [S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"), > + [GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"), > + [HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"), > +}; EXPORT_SYMBOL is required, I believe, for when ocelot_ext is built as module? > + > +const struct resource vsc7512_port_io_res[] = { > + DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"), > + DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"), > + {} > +}; Here too. > + > static const struct mfd_cell vsc7512_devs[] = { > { > .name = "ocelot-pinctrl", > @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = { > static void ocelot_core_try_add_regmap(struct device *dev, > const struct resource *res) > { > - if (dev_get_regmap(dev, res->name)) > + if (!res->start || dev_get_regmap(dev, res->name)) I didn't understand at first what this extra condition here is for. I don't think that adding this extra condition here is the clearest way to deal with the sparsity of the vsc7512_target_io_res[] array, plus it seems to indicate the masking of a more unclean code design. I would propose an alternative below, at the caller site.... > return; > > ocelot_spi_init_regmap(dev, res); > @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev, > > int ocelot_core_init(struct device *dev) > { > + const struct resource *port_res; > int i, ndevs; > > ndevs = ARRAY_SIZE(vsc7512_devs); > @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev) > for (i = 0; i < ndevs; i++) > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); > > + /* > + * Both the target_io_res and tbe port_io_res structs need to be referenced directly by s/tbe/the > + * the ocelot_ext driver, so they can't be attached to the dev directly I don't exactly understand the meaning of "they can't be attached to the dev *directly*". You mean that the "struct mfd_cell vsc7512_devs[]" entry for "mscc,vsc7512-ext-switch" will not have a "resources" property, right? Better to say "using mfd_add_devices()" rather than "directly"? > + */ > + for (i = 0; i < TARGET_MAX; i++) > + ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]); /* * vsc7512_target_io_res[] is a sparse array, skip the missing * elements */ for (i = 0; i < TARGET_MAX; i++) { res = &vsc7512_target_io_res[i]; if (!res->start) continue; ocelot_core_try_add_regmap(dev, res); } Something interesting that I stumbled upon in Documentation/process/6.Followthrough.rst was: | Andrew Morton has suggested that every review comment which does not result | in a code change should result in an additional code comment instead; that | can help future reviewers avoid the questions which came up the first time | around. so if you don't like my alternative, please at least do add a comment in ocelot_core_try_add_regmap(). > + > + for (port_res = vsc7512_port_io_res; port_res->start; port_res++) > + ocelot_core_try_add_regmap(dev, port_res); > + > return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL); > } > EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); > diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h > index dd72073d2d4f..439ff5256cf0 100644 > --- a/include/linux/mfd/ocelot.h > +++ b/include/linux/mfd/ocelot.h > @@ -11,8 +11,13 @@ > #include <linux/regmap.h> > #include <linux/types.h> > > +#include <soc/mscc/ocelot.h> > + > struct resource; > > +extern const struct resource vsc7512_target_io_res[TARGET_MAX]; > +extern const struct resource vsc7512_port_io_res[]; > + > static inline struct regmap * > ocelot_regmap_from_resource_optional(struct platform_device *pdev, > unsigned int index, > -- > 2.25.1 > Actually I don't like this mechanism too much, if at all. I have 4 mutt windows open right now, plus the previous mfd patch at: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=ib-mfd-net-pinctrl-6.0&id=f3e893626abeac3cdd9ba41d3395dc6c1b7d5ad6 to follow what is going on. So I'll copy some code from other places here, to concentrate the discussion in a single place: From patch 8/8: > +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot, > + struct resource *res) > +{ > + return dev_get_regmap(ocelot->dev->parent, res->name); > +} > +static const struct felix_info vsc7512_info = { > + .target_io_res = vsc7512_target_io_res, // exported by drivers/mfd/ocelot-core.c > + .port_io_res = vsc7512_port_io_res, // exported by drivers/mfd/ocelot-core.c > + .init_regmap = ocelot_ext_regmap_init, > +}; From drivers/net/dsa/felix.c: static int felix_init_structs(struct felix *felix, int num_phys_ports) { for (i = 0; i < TARGET_MAX; i++) { struct regmap *target; if (!felix->info->target_io_res[i].name) continue; memcpy(&res, &felix->info->target_io_res[i], sizeof(res)); res.flags = IORESOURCE_MEM; res.start += felix->switch_base; res.end += felix->switch_base; target = felix->info->init_regmap(ocelot, &res); if (IS_ERR(target)) { dev_err(ocelot->dev, "Failed to map device memory space\n"); kfree(port_phy_modes); return PTR_ERR(target); } ocelot->targets[i] = target; } } So here's what I don't like. You export the resources from ocelot-mfd to DSA, to get just their *string* names. Then you let the common code create some bogus res.start and res.end in felix_init_structs(), which you discard in felix->info->init_regmap() - ocelot_ext_regmap_init(), and use just the name. You even discard the IORESOURCE_MEM flag, because what you get back are IORESOURCE_REG resources. This is all very confusing. So you need to retrieve a regmap for each ocelot target that you can. Why don't you make it, via mfd_add_devices() and the "resources" array of struct mfd_cell (i.e. the same mechanism as for every other peripheral), such that the resources used by the DSA device have an index determined by i = 0; i < TARGET_MAX; i++; platform_get_resource(dev, i, IORESOURCE_REG)? This way, DSA needs to know no more than the index of the resource it asks for. [ yes, you'll need to revert your own commit 242bd0c10bbd ("net: dsa: ocelot: felix: add interface for custom regmaps"), which I asked you about if you're sure if this is the final way in which DSA will get its regmaps. Then you'll need to provide a different felix->info operation, such as felix->info->regmap_from_mfd() or something, where just the index is provided. If that isn't provided by the switch, we "fall back" to the code that exists right now, which, when reverted, does create an actual resource, and directly calls ocelot_regmap_init() on it, to create an MMIO regmap from it ]
On Mon, Sep 12, 2022 at 05:08:09PM +0000, Vladimir Oltean wrote: > On Sun, Sep 11, 2022 at 01:02:43PM -0700, Colin Foster wrote: > > The Ocelot switch core driver relies heavily on a fixed array of resources > > for both ports and peripherals. This is in contrast to existing peripherals > > - pinctrl for example - which have a one-to-one mapping of driver <> > > resource. As such, these regmaps must be created differently so that > > enumeration-based offsets are preserved. > > > > Register the regmaps to the core MFD device unconditionally so they can be > > referenced by the Ocelot switch / Felix DSA systems. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > > > v1 from previous RFC: > > * New patch > > > > --- > > drivers/mfd/ocelot-core.c | 88 +++++++++++++++++++++++++++++++++++--- > > include/linux/mfd/ocelot.h | 5 +++ > > 2 files changed, 88 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c > > index 1816d52c65c5..aa7fa21b354c 100644 > > --- a/drivers/mfd/ocelot-core.c > > +++ b/drivers/mfd/ocelot-core.c > > @@ -34,16 +34,55 @@ > > > > #define VSC7512_MIIM0_RES_START 0x7107009c > > #define VSC7512_MIIM1_RES_START 0x710700c0 > > -#define VSC7512_MIIM_RES_SIZE 0x024 > > +#define VSC7512_MIIM_RES_SIZE 0x00000024 > > > > #define VSC7512_PHY_RES_START 0x710700f0 > > -#define VSC7512_PHY_RES_SIZE 0x004 > > +#define VSC7512_PHY_RES_SIZE 0x00000004 > > > > #define VSC7512_GPIO_RES_START 0x71070034 > > -#define VSC7512_GPIO_RES_SIZE 0x06c > > +#define VSC7512_GPIO_RES_SIZE 0x0000006c > > > > #define VSC7512_SIO_CTRL_RES_START 0x710700f8 > > -#define VSC7512_SIO_CTRL_RES_SIZE 0x100 > > +#define VSC7512_SIO_CTRL_RES_SIZE 0x00000100 > > Split the gratuitous changes to _RES_SIZE to a separate patch please, as > they're just noise here? Will do. > > +const struct resource vsc7512_target_io_res[TARGET_MAX] = { > > + [ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"), > > + [QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"), > > + [QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"), > > + [REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"), > > + [SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"), > > + [S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"), > > + [S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"), > > + [S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"), > > + [GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"), > > + [HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"), > > +}; > > EXPORT_SYMBOL is required, I believe, for when ocelot_ext is built as > module? Agreed on this and the other symbol. Thanks. > > > + > > static const struct mfd_cell vsc7512_devs[] = { > > { > > .name = "ocelot-pinctrl", > > @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = { > > static void ocelot_core_try_add_regmap(struct device *dev, > > const struct resource *res) > > { > > - if (dev_get_regmap(dev, res->name)) > > + if (!res->start || dev_get_regmap(dev, res->name)) > > I didn't understand at first what this extra condition here is for. > I don't think that adding this extra condition here is the clearest > way to deal with the sparsity of the vsc7512_target_io_res[] array, plus > it seems to indicate the masking of a more unclean code design. Yes, it was a way to deal with this struct. I see that I should have at least added a comment, but the way you suggest below is cleaner (before try_add_regmap()) > > I would propose an alternative below, at the caller site.... > > > return; > > > > ocelot_spi_init_regmap(dev, res); > > @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev, > > > > int ocelot_core_init(struct device *dev) > > { > > + const struct resource *port_res; > > int i, ndevs; > > > > ndevs = ARRAY_SIZE(vsc7512_devs); > > @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev) > > for (i = 0; i < ndevs; i++) > > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); > > > > + /* > > + * Both the target_io_res and tbe port_io_res structs need to be referenced directly by > > s/tbe/the > > > + * the ocelot_ext driver, so they can't be attached to the dev directly > > I don't exactly understand the meaning of "they can't be attached to the > dev *directly*". You mean that the "struct mfd_cell vsc7512_devs[]" entry > for "mscc,vsc7512-ext-switch" will not have a "resources" property, right? > Better to say "using mfd_add_devices()" rather than "directly"? I'll reword the comment - but I think it might go away entirely with what you're suggesting below. > > > + */ > > + for (i = 0; i < TARGET_MAX; i++) > > + ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]); > > /* > * vsc7512_target_io_res[] is a sparse array, skip the missing > * elements > */ > for (i = 0; i < TARGET_MAX; i++) { > res = &vsc7512_target_io_res[i]; > if (!res->start) > continue; > > ocelot_core_try_add_regmap(dev, res); > } > > Something interesting that I stumbled upon in Documentation/process/6.Followthrough.rst > was: > > | Andrew Morton has suggested that every review comment which does not result > | in a code change should result in an additional code comment instead; that > | can help future reviewers avoid the questions which came up the first time > | around. > > so if you don't like my alternative, please at least do add a comment in > ocelot_core_try_add_regmap(). I'm due for another re-read of this documentation. That's a very practical suggestion. > > > + > > + for (port_res = vsc7512_port_io_res; port_res->start; port_res++) > > + ocelot_core_try_add_regmap(dev, port_res); > > + > > return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL); > > } > > EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); > > diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h > > index dd72073d2d4f..439ff5256cf0 100644 > > --- a/include/linux/mfd/ocelot.h > > +++ b/include/linux/mfd/ocelot.h > > @@ -11,8 +11,13 @@ > > #include <linux/regmap.h> > > #include <linux/types.h> > > > > +#include <soc/mscc/ocelot.h> > > + > > struct resource; > > > > +extern const struct resource vsc7512_target_io_res[TARGET_MAX]; > > +extern const struct resource vsc7512_port_io_res[]; > > + > > static inline struct regmap * > > ocelot_regmap_from_resource_optional(struct platform_device *pdev, > > unsigned int index, > > -- > > 2.25.1 > > > > Actually I don't like this mechanism too much, if at all. I have 4 mutt > windows open right now, plus the previous mfd patch at: > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=ib-mfd-net-pinctrl-6.0&id=f3e893626abeac3cdd9ba41d3395dc6c1b7d5ad6 > to follow what is going on. So I'll copy some code from other places > here, to concentrate the discussion in a single place: > > From patch 8/8: > > > +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot, > > + struct resource *res) > > +{ > > + return dev_get_regmap(ocelot->dev->parent, res->name); > > +} > > > +static const struct felix_info vsc7512_info = { > > + .target_io_res = vsc7512_target_io_res, // exported by drivers/mfd/ocelot-core.c > > + .port_io_res = vsc7512_port_io_res, // exported by drivers/mfd/ocelot-core.c > > + .init_regmap = ocelot_ext_regmap_init, > > +}; > > From drivers/net/dsa/felix.c: > > static int felix_init_structs(struct felix *felix, int num_phys_ports) > { > for (i = 0; i < TARGET_MAX; i++) { > struct regmap *target; > > if (!felix->info->target_io_res[i].name) > continue; > > memcpy(&res, &felix->info->target_io_res[i], sizeof(res)); > res.flags = IORESOURCE_MEM; > res.start += felix->switch_base; > res.end += felix->switch_base; > > target = felix->info->init_regmap(ocelot, &res); > if (IS_ERR(target)) { > dev_err(ocelot->dev, > "Failed to map device memory space\n"); > kfree(port_phy_modes); > return PTR_ERR(target); > } > > ocelot->targets[i] = target; > } > } > > So here's what I don't like. You export the resources from ocelot-mfd to > DSA, to get just their *string* names. Then you let the common code > create some bogus res.start and res.end in felix_init_structs(), which > you discard in felix->info->init_regmap() - ocelot_ext_regmap_init(), > and use just the name. You even discard the IORESOURCE_MEM flag, because > what you get back are IORESOURCE_REG resources. This is all very confusing. > > So you need to retrieve a regmap for each ocelot target that you can. > Why don't you make it, via mfd_add_devices() and the "resources" array > of struct mfd_cell (i.e. the same mechanism as for every other peripheral), > such that the resources used by the DSA device have an index determined > by i = 0; i < TARGET_MAX; i++; platform_get_resource(dev, i, IORESOURCE_REG)? > This way, DSA needs to know no more than the index of the resource it > asks for. That is exactly right. The ocelot_ext version of init_regmap() now uses dev_get_regmap() which only cares about the name and essentially drops the rest of the information. Previous versions hooked into the ocelot-core / ocelot-spi MFD system to request that a new regmap be created (with start and end being honored.) A benefit of this design is that any regmaps that are named the same are automatically shared. A drawback (or maybe a benefit?) is that the users have no control over ranges / flags. I think if this goes the way of index-based that'll work. I'm happy to revert my previous change (sorry it snuck in) but it seems like there'll still have to be some trickery... For reference: enum ocelot_target { ANA = 1, QS, QSYS, REW, SYS, S0, S1, S2, HSIO, PTP, FDMA, GCB, DEV_GMII, TARGET_MAX, }; mfd_add_devices will probably need to add a zero-size resource for HSIO, PTP, FDMA, and anything else that might come along in the future. At this point, regmap_from_mfd(PTP) might have to look like (pseudocode): struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index) { return ocelot_regmap_from_resource_optional(pdev, index-1, config); /* Note this essentially expands to: * res = platform_get_resource(pdev, IORESOURCE_REG, index-1); * return dev_get_regmap(pdev->dev.parent, res->name); * * This essentially throws away everything that my current * implementation does, except for the IORESOURCE_REG flag */ } Then drivers/net/dsa/felix.c felix_init_structs() would have two loops (again, just as an example) for (i = ANA; i < TARGET_MAX; i++) { if (felix->info->regmap_from_mfd) target = felix->info->regmap_from_mfd(ocelot, i); else { /* existing logic back to ocelot_regmap_init() */ } } for (port = 0; port < num_phys_ports; port++) { ... if (felix->info->regmap_from_mfd) target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port); else { /* existing logic back to ocelot_regmap_init() */ } } And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a mechanism to say "don't add a regmap for cell->resources[PTP], even though that resource exists" because... well I suppose it is only in drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of those regmaps invokes different behavior. For instance: if (ocelot->targets[FDMA]) ocelot_fdma_init(pdev, ocelot); I'm not sure whether this last point will have an effect on felix.c in the end. My current patch set of adding the SERDES ports uses the existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that gets rejected outright. That's for another day. I'm happy to make these changes if you see them valid. I saw the fact that dev_get_regmap(dev->parent, res->name) could be used directly in ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib / ocelot-core, but I recognize that the subtle "throw away the IORESOURCE_MEM flag and res->{start,end} information" isn't ideal. > > [ yes, you'll need to revert your own commit 242bd0c10bbd ("net: dsa: > ocelot: felix: add interface for custom regmaps"), which I asked you > about if you're sure if this is the final way in which DSA will get > its regmaps. Then you'll need to provide a different felix->info > operation, such as felix->info->regmap_from_mfd() or something, where > just the index is provided. If that isn't provided by the switch, we > "fall back" to the code that exists right now, which, when reverted, > does create an actual resource, and directly calls ocelot_regmap_init() > on it, to create an MMIO regmap from it ]
On Mon, Sep 12, 2022 at 12:04:45PM -0700, Colin Foster wrote: > That is exactly right. The ocelot_ext version of init_regmap() now uses > dev_get_regmap() which only cares about the name and essentially drops > the rest of the information. Previous versions hooked into the > ocelot-core / ocelot-spi MFD system to request that a new regmap be > created (with start and end being honored.) A benefit of this design is > that any regmaps that are named the same are automatically shared. A > drawback (or maybe a benefit?) is that the users have no control over > ranges / flags. > > I think if this goes the way of index-based that'll work. I'm happy to > revert my previous change (sorry it snuck in) but it seems like there'll > still have to be some trickery... For reference: > > enum ocelot_target { > ANA = 1, > QS, > QSYS, > REW, > SYS, > S0, > S1, > S2, > HSIO, > PTP, > FDMA, > GCB, > DEV_GMII, > TARGET_MAX, > }; > > mfd_add_devices will probably need to add a zero-size resource for HSIO, > PTP, FDMA, and anything else that might come along in the future. At > this point, regmap_from_mfd(PTP) might have to look like (pseudocode): > > struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index) > { > return ocelot_regmap_from_resource_optional(pdev, index-1, config); > > /* Note this essentially expands to: > * res = platform_get_resource(pdev, IORESOURCE_REG, index-1); > * return dev_get_regmap(pdev->dev.parent, res->name); > * > * This essentially throws away everything that my current > * implementation does, except for the IORESOURCE_REG flag > */ > } > > Then drivers/net/dsa/felix.c felix_init_structs() would have two loops > (again, just as an example) > > for (i = ANA; i < TARGET_MAX; i++) { > if (felix->info->regmap_from_mfd) > target = felix->info->regmap_from_mfd(ocelot, i); > else { > /* existing logic back to ocelot_regmap_init() */ > } > } > > for (port = 0; port < num_phys_ports; port++) { > ... > if (felix->info->regmap_from_mfd) > target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port); > else { > /* existing logic back to ocelot_regmap_init() */ > } > } > > And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a > mechanism to say "don't add a regmap for cell->resources[PTP], even > though that resource exists" because... well I suppose it is only in > drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of > those regmaps invokes different behavior. For instance: > > if (ocelot->targets[FDMA]) > ocelot_fdma_init(pdev, ocelot); > > I'm not sure whether this last point will have an effect on felix.c in > the end. My current patch set of adding the SERDES ports uses the > existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the > ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that > gets rejected outright. That's for another day. > > > > I'm happy to make these changes if you see them valid. I saw the fact > that dev_get_regmap(dev->parent, res->name) could be used directly in > ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib / > ocelot-core, but I recognize that the subtle "throw away the > IORESOURCE_MEM flag and res->{start,end} information" isn't ideal. Thinking some more about it, there will have to be even more trickery. Say you solve the problem for the global targets, but then what do you do for the port targets, how do you reference those by index? TARGET_MAX + port? Hmm, that isn't great either. What if we meet half way, and you just get the resources from the platform device by name, instead of by index? You'd have to modify the regmap creation procedure to look at a predefined array of strings, containing names of all targets that are mandatory (a la mscc_ocelot_probe), and match those (a) iteration over target_io_res and strcmp(), in the case of vsc9959 and vsc9953 (b) dev_get_regmap() in the case of ocelot_ext This way there's still no direct communication between ocelot-mfd and DSA, and I have the feeling that the problems we both mention are solved. Hope I'm not missing something.
On Mon, Sep 12, 2022 at 08:23:21PM +0000, Vladimir Oltean wrote: > On Mon, Sep 12, 2022 at 12:04:45PM -0700, Colin Foster wrote: > > That is exactly right. The ocelot_ext version of init_regmap() now uses > > dev_get_regmap() which only cares about the name and essentially drops > > the rest of the information. Previous versions hooked into the > > ocelot-core / ocelot-spi MFD system to request that a new regmap be > > created (with start and end being honored.) A benefit of this design is > > that any regmaps that are named the same are automatically shared. A > > drawback (or maybe a benefit?) is that the users have no control over > > ranges / flags. > > > > I think if this goes the way of index-based that'll work. I'm happy to > > revert my previous change (sorry it snuck in) but it seems like there'll > > still have to be some trickery... For reference: > > > > enum ocelot_target { > > ANA = 1, > > QS, > > QSYS, > > REW, > > SYS, > > S0, > > S1, > > S2, > > HSIO, > > PTP, > > FDMA, > > GCB, > > DEV_GMII, > > TARGET_MAX, > > }; > > > > mfd_add_devices will probably need to add a zero-size resource for HSIO, > > PTP, FDMA, and anything else that might come along in the future. At > > this point, regmap_from_mfd(PTP) might have to look like (pseudocode): > > > > struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index) > > { > > return ocelot_regmap_from_resource_optional(pdev, index-1, config); > > > > /* Note this essentially expands to: > > * res = platform_get_resource(pdev, IORESOURCE_REG, index-1); > > * return dev_get_regmap(pdev->dev.parent, res->name); > > * > > * This essentially throws away everything that my current > > * implementation does, except for the IORESOURCE_REG flag > > */ > > } > > > > Then drivers/net/dsa/felix.c felix_init_structs() would have two loops > > (again, just as an example) > > > > for (i = ANA; i < TARGET_MAX; i++) { > > if (felix->info->regmap_from_mfd) > > target = felix->info->regmap_from_mfd(ocelot, i); > > else { > > /* existing logic back to ocelot_regmap_init() */ > > } > > } > > > > for (port = 0; port < num_phys_ports; port++) { > > ... > > if (felix->info->regmap_from_mfd) > > target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port); > > else { > > /* existing logic back to ocelot_regmap_init() */ > > } > > } > > > > And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a > > mechanism to say "don't add a regmap for cell->resources[PTP], even > > though that resource exists" because... well I suppose it is only in > > drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of > > those regmaps invokes different behavior. For instance: > > > > if (ocelot->targets[FDMA]) > > ocelot_fdma_init(pdev, ocelot); > > > > I'm not sure whether this last point will have an effect on felix.c in > > the end. My current patch set of adding the SERDES ports uses the > > existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the > > ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that > > gets rejected outright. That's for another day. > > > > > > > > I'm happy to make these changes if you see them valid. I saw the fact > > that dev_get_regmap(dev->parent, res->name) could be used directly in > > ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib / > > ocelot-core, but I recognize that the subtle "throw away the > > IORESOURCE_MEM flag and res->{start,end} information" isn't ideal. > > Thinking some more about it, there will have to be even more trickery. > Say you solve the problem for the global targets, but then what do you > do for the port targets, how do you reference those by index? > TARGET_MAX + port? Hmm, that isn't great either. Yep, that's what my example above shows. Not my favorite. > > What if we meet half way, and you just get the resources from the > platform device by name, instead of by index? You'd have to modify the > regmap creation procedure to look at a predefined array of strings, > containing names of all targets that are mandatory (a la mscc_ocelot_probe), > and match those > (a) iteration over target_io_res and strcmp(), in the case of vsc9959 > and vsc9953 > (b) dev_get_regmap() in the case of ocelot_ext > > This way there's still no direct communication between ocelot-mfd and > DSA, and I have the feeling that the problems we both mention are > solved. Hope I'm not missing something. This sounds reasonable. So long as it doesn't muddy up felix / seville too much - I'll take a look. It seems like it would just be moving a lot of the "resource configuration" code from felix_init_structs() into the felix->info->init_regmap(), or similar.
On Mon, Sep 12, 2022 at 02:03:43PM -0700, Colin Foster wrote: > Yep, that's what my example above shows. Not my favorite. Yeah, sorry, I was on mobile and I had exactly one pass at reading your response. You did point this out with the "two loops" comment. > This sounds reasonable. So long as it doesn't muddy up felix / seville > too much - I'll take a look. It seems like it would just be moving > a lot of the "resource configuration" code from felix_init_structs() into > the felix->info->init_regmap(), or similar. Possibly so, yes. I don't have any other comments for this series, btw. Just make sure to grab the attention of one of the maintainers somehow to get Lee's branch pulled, before you send the next version. Not exactly sure how; LPC has just started and most people have their attention focused there, I guess.
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c index 1816d52c65c5..aa7fa21b354c 100644 --- a/drivers/mfd/ocelot-core.c +++ b/drivers/mfd/ocelot-core.c @@ -34,16 +34,55 @@ #define VSC7512_MIIM0_RES_START 0x7107009c #define VSC7512_MIIM1_RES_START 0x710700c0 -#define VSC7512_MIIM_RES_SIZE 0x024 +#define VSC7512_MIIM_RES_SIZE 0x00000024 #define VSC7512_PHY_RES_START 0x710700f0 -#define VSC7512_PHY_RES_SIZE 0x004 +#define VSC7512_PHY_RES_SIZE 0x00000004 #define VSC7512_GPIO_RES_START 0x71070034 -#define VSC7512_GPIO_RES_SIZE 0x06c +#define VSC7512_GPIO_RES_SIZE 0x0000006c #define VSC7512_SIO_CTRL_RES_START 0x710700f8 -#define VSC7512_SIO_CTRL_RES_SIZE 0x100 +#define VSC7512_SIO_CTRL_RES_SIZE 0x00000100 + +#define VSC7512_HSIO_RES_START 0x710d0000 +#define VSC7512_HSIO_RES_SIZE 0x00000128 + +#define VSC7512_ANA_RES_START 0x71880000 +#define VSC7512_ANA_RES_SIZE 0x00010000 + +#define VSC7512_QS_RES_START 0x71080000 +#define VSC7512_QS_RES_SIZE 0x00000100 + +#define VSC7512_QSYS_RES_START 0x71800000 +#define VSC7512_QSYS_RES_SIZE 0x00200000 + +#define VSC7512_REW_RES_START 0x71030000 +#define VSC7512_REW_RES_SIZE 0x00010000 + +#define VSC7512_SYS_RES_START 0x71010000 +#define VSC7512_SYS_RES_SIZE 0x00010000 + +#define VSC7512_S0_RES_START 0x71040000 +#define VSC7512_S1_RES_START 0x71050000 +#define VSC7512_S2_RES_START 0x71060000 +#define VSC7512_S_RES_SIZE 0x00000400 + +#define VSC7512_GCB_RES_START 0x71070000 +#define VSC7512_GCB_RES_SIZE 0x0000022c + +#define VSC7512_PORT_0_RES_START 0x711e0000 +#define VSC7512_PORT_1_RES_START 0x711f0000 +#define VSC7512_PORT_2_RES_START 0x71200000 +#define VSC7512_PORT_3_RES_START 0x71210000 +#define VSC7512_PORT_4_RES_START 0x71220000 +#define VSC7512_PORT_5_RES_START 0x71230000 +#define VSC7512_PORT_6_RES_START 0x71240000 +#define VSC7512_PORT_7_RES_START 0x71250000 +#define VSC7512_PORT_8_RES_START 0x71260000 +#define VSC7512_PORT_9_RES_START 0x71270000 +#define VSC7512_PORT_10_RES_START 0x71280000 +#define VSC7512_PORT_RES_SIZE 0x00010000 #define VSC7512_GCB_RST_SLEEP_US 100 #define VSC7512_GCB_RST_TIMEOUT_US 100000 @@ -96,6 +135,34 @@ static const struct resource vsc7512_sgpio_resources[] = { DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"), }; +const struct resource vsc7512_target_io_res[TARGET_MAX] = { + [ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"), + [QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"), + [QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"), + [REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"), + [SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"), + [S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"), + [S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"), + [S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"), + [GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"), + [HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"), +}; + +const struct resource vsc7512_port_io_res[] = { + DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"), + DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"), + {} +}; + static const struct mfd_cell vsc7512_devs[] = { { .name = "ocelot-pinctrl", @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = { static void ocelot_core_try_add_regmap(struct device *dev, const struct resource *res) { - if (dev_get_regmap(dev, res->name)) + if (!res->start || dev_get_regmap(dev, res->name)) return; ocelot_spi_init_regmap(dev, res); @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev, int ocelot_core_init(struct device *dev) { + const struct resource *port_res; int i, ndevs; ndevs = ARRAY_SIZE(vsc7512_devs); @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev) for (i = 0; i < ndevs; i++) ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); + /* + * Both the target_io_res and tbe port_io_res structs need to be referenced directly by + * the ocelot_ext driver, so they can't be attached to the dev directly + */ + for (i = 0; i < TARGET_MAX; i++) + ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]); + + for (port_res = vsc7512_port_io_res; port_res->start; port_res++) + ocelot_core_try_add_regmap(dev, port_res); + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL); } EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h index dd72073d2d4f..439ff5256cf0 100644 --- a/include/linux/mfd/ocelot.h +++ b/include/linux/mfd/ocelot.h @@ -11,8 +11,13 @@ #include <linux/regmap.h> #include <linux/types.h> +#include <soc/mscc/ocelot.h> + struct resource; +extern const struct resource vsc7512_target_io_res[TARGET_MAX]; +extern const struct resource vsc7512_port_io_res[]; + static inline struct regmap * ocelot_regmap_from_resource_optional(struct platform_device *pdev, unsigned int index,
The Ocelot switch core driver relies heavily on a fixed array of resources for both ports and peripherals. This is in contrast to existing peripherals - pinctrl for example - which have a one-to-one mapping of driver <> resource. As such, these regmaps must be created differently so that enumeration-based offsets are preserved. Register the regmaps to the core MFD device unconditionally so they can be referenced by the Ocelot switch / Felix DSA systems. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- v1 from previous RFC: * New patch --- drivers/mfd/ocelot-core.c | 88 +++++++++++++++++++++++++++++++++++--- include/linux/mfd/ocelot.h | 5 +++ 2 files changed, 88 insertions(+), 5 deletions(-)