| Message ID | 20220926002928.2744638-9-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, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 113 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Colin, On Sun, Sep 25, 2022 at 05:29:22PM -0700, Colin Foster wrote: > During development, it was believed that a wrapper for ocelot_regmap_init() > would be sufficient for the felix driver to work in non-mmio scenarios. > This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix: > add interface for custom regmaps") > > As the external ocelot DSA driver grew closer to an acceptable state, it > was realized that most of the parameters that were passed in from struct > resource *res were useless and ignored. This is due to the fact that the > external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name). > > Instead of simply ignoring those parameters, refactor the API to only > require the name as an argument. MMIO scenarios this will reconstruct the > struct resource before calling ocelot_regmap_init(ocelot, resource). MFD > scenarios need only call dev_get_regmap(dev, name). > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- I don't like how this turned out. I was expecting you not to look at the exported resources from the ocelot-core anymore - that was kind of the point of using just the names rather than the whole resource definitions. I am also sorry for the mess that the felix driver currently is in, and the fact that some things may have confused you. I will prepare a patch set which offers an alternative to this, and send it for review.
Hi Vladimir, On Tue, Sep 27, 2022 at 08:53:53PM +0300, Vladimir Oltean wrote: > Hi Colin, > > On Sun, Sep 25, 2022 at 05:29:22PM -0700, Colin Foster wrote: > > During development, it was believed that a wrapper for ocelot_regmap_init() > > would be sufficient for the felix driver to work in non-mmio scenarios. > > This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix: > > add interface for custom regmaps") > > > > As the external ocelot DSA driver grew closer to an acceptable state, it > > was realized that most of the parameters that were passed in from struct > > resource *res were useless and ignored. This is due to the fact that the > > external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name). > > > > Instead of simply ignoring those parameters, refactor the API to only > > require the name as an argument. MMIO scenarios this will reconstruct the > > struct resource before calling ocelot_regmap_init(ocelot, resource). MFD > > scenarios need only call dev_get_regmap(dev, name). > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > I don't like how this turned out. I was expecting you not to look at the > exported resources from the ocelot-core anymore - that was kind of the > point of using just the names rather than the whole resource definitions. I see your point. The init_regmap(name) interface collides with the *_io_res arrays. Changing the init_regmap() interface doesn't really change the underlying issue - *_io_res[] is the thing that you're suggesting to go. I'm interested to see where this is going. I feel like it might be a constant names[] array, then felix_vsc9959_init_regmap() where the specific name <> resource mapping happens. Maybe a common felix_match_resource_to_name(name, res, len)? That would definitely remove the need for exporting the vsc7512_*_io_res[] arrays, which I didn't understand from your v1 review. Something like: include/soc/mscc/ocelot.h #define OCELOT_RES_NAME_ANA "ana" const char *ocelot_target_names[TARGET_MAX] = {[ANA] = OCELOT_RES_NAME_ANA}; ... drivers/net/dsa/ocelot/felix.c for (i = 0; i < TARGET_MAX; i++) target = felix->info->init_regmap(ocelot_target_names[i]); ... drivers/net/dsa/ocelot/felix_vsc9959.c static const struct resource vsc9959_target_io_res[TARGET_MAX] = ...; vsc9959_init_regmap(name) { /* more logic for port_io_res, but you get the point */ return felix_init_regmap(name, &vsc9959_target_io_res, TARGET_MAX); } > > I am also sorry for the mess that the felix driver currently is in, and > the fact that some things may have confused you. Vladimir, you might be the last person on earth who owes me an apology.
On Tue, Sep 27, 2022 at 11:43:46AM -0700, Colin Foster wrote: > I see your point. The init_regmap(name) interface collides with the > *_io_res arrays. Changing the init_regmap() interface doesn't really > change the underlying issue - *_io_res[] is the thing that you're > suggesting to go. > > I'm interested to see where this is going. I feel like it might be a > constant names[] array, then felix_vsc9959_init_regmap() where the > specific name <> resource mapping happens. Maybe a common > felix_match_resource_to_name(name, res, len)? > > That would definitely remove the need for exporting the > vsc7512_*_io_res[] arrays, which I didn't understand from your v1 > review. Yes, having an array of strings, meaning which targets are required by each driver, is what I wanted to see. Isn't that what I said in v1? > vsc9959_init_regmap(name) > { > /* more logic for port_io_res, but you get the point */ > return felix_init_regmap(name, &vsc9959_target_io_res, TARGET_MAX); > } Yeah, wait a minute, you'll see. > > I am also sorry for the mess that the felix driver currently is in, and > > the fact that some things may have confused you. > > Vladimir, you might be the last person on earth who owes me an apology. I have some more comments on the other patches. This driver looks weird not only because the hardware is complicated and all over the place, but also because you're working on a driver (felix) which was designed around NXP variations of Microchip hardware, and this really transpires especially around the probing and dt-bindings. The goal, otherwise, would be for you to have dt-bindings for vsc7512 that are identical to what Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml provides. It doesn't matter how the driver probes, that is to some extent independent from how the drivers look like. Anyway, I'm getting ahead of myself.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index a8196cdedcc5..b01482b24e7a 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -1318,11 +1318,48 @@ static int felix_parse_dt(struct felix *felix, phy_interface_t *port_phy_modes) return err; } +struct regmap *felix_init_regmap(struct ocelot *ocelot, const char *name) +{ + struct felix *felix = ocelot_to_felix(ocelot); + const struct resource *match = NULL; + struct resource res; + int i; + + for (i = 0; i < TARGET_MAX; i++) { + if (!felix->info->target_io_res[i].name) + continue; + + if (!strcmp(name, felix->info->target_io_res[i].name)) { + match = &felix->info->target_io_res[i]; + break; + } + } + + if (!match) { + for (i = 0; i < ocelot->num_phys_ports; i++) { + if (!strcmp(name, felix->info->port_io_res[i].name)) { + match = &felix->info->port_io_res[i]; + break; + } + } + } + + if (!match) + return ERR_PTR(-EINVAL); + + memcpy(&res, match, sizeof(res)); + res.flags = IORESOURCE_MEM; + res.start += felix->switch_base; + res.end += felix->switch_base; + + return ocelot_regmap_init(ocelot, &res); +} + static int felix_init_structs(struct felix *felix, int num_phys_ports) { struct ocelot *ocelot = &felix->ocelot; phy_interface_t *port_phy_modes; - struct resource res; + const char *name; int port, i, err; ocelot->num_phys_ports = num_phys_ports; @@ -1358,15 +1395,12 @@ 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; + name = felix->info->target_io_res[i].name; - memcpy(&res, &felix->info->target_io_res[i], sizeof(res)); - res.flags = IORESOURCE_MEM; - res.start += felix->switch_base; - res.end += felix->switch_base; + if (!name) + continue; - target = felix->info->init_regmap(ocelot, &res); + target = felix->info->init_regmap(ocelot, name); if (IS_ERR(target)) { dev_err(ocelot->dev, "Failed to map device memory space\n"); @@ -1398,12 +1432,8 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports) return -ENOMEM; } - memcpy(&res, &felix->info->port_io_res[port], sizeof(res)); - res.flags = IORESOURCE_MEM; - res.start += felix->switch_base; - res.end += felix->switch_base; - - target = felix->info->init_regmap(ocelot, &res); + name = felix->info->port_io_res[port].name; + target = felix->info->init_regmap(ocelot, name); if (IS_ERR(target)) { dev_err(ocelot->dev, "Failed to map memory space for port %d\n", diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index f94a445c2542..e623806eb8ee 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -57,8 +57,7 @@ struct felix_info { void (*tas_guard_bands_update)(struct ocelot *ocelot, int port); void (*port_sched_speed_set)(struct ocelot *ocelot, int port, u32 speed); - struct regmap *(*init_regmap)(struct ocelot *ocelot, - struct resource *res); + struct regmap *(*init_regmap)(struct ocelot *ocelot, const char *name); }; /* Methods for initializing the hardware resources specific to a tagging @@ -97,5 +96,6 @@ struct felix { struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port); int felix_netdev_to_port(struct net_device *dev); +struct regmap *felix_init_regmap(struct ocelot *ocelot, const char *name); #endif diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 2fd2bb499e9c..e20d5d5d2de9 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -2615,7 +2615,7 @@ static const struct felix_info felix_info_vsc9959 = { .port_setup_tc = vsc9959_port_setup_tc, .port_sched_speed_set = vsc9959_sched_speed_set, .tas_guard_bands_update = vsc9959_tas_guard_bands_update, - .init_regmap = ocelot_regmap_init, + .init_regmap = felix_init_regmap, }; static irqreturn_t felix_irq_handler(int irq, void *data) diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index e589d07f84db..7c698e19d818 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -1079,7 +1079,7 @@ static const struct felix_info seville_info_vsc9953 = { .mdio_bus_free = vsc9953_mdio_bus_free, .phylink_validate = vsc9953_phylink_validate, .port_modes = vsc9953_port_modes, - .init_regmap = ocelot_regmap_init, + .init_regmap = felix_init_regmap, }; static int seville_probe(struct platform_device *pdev)
During development, it was believed that a wrapper for ocelot_regmap_init() would be sufficient for the felix driver to work in non-mmio scenarios. This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix: add interface for custom regmaps") As the external ocelot DSA driver grew closer to an acceptable state, it was realized that most of the parameters that were passed in from struct resource *res were useless and ignored. This is due to the fact that the external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name). Instead of simply ignoring those parameters, refactor the API to only require the name as an argument. MMIO scenarios this will reconstruct the struct resource before calling ocelot_regmap_init(ocelot, resource). MFD scenarios need only call dev_get_regmap(dev, name). Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- v3 * Assign match = NULL for the default case * Don't export felix_init_regmap symbol - the felix.o object is compiled directly into "mscc_felix-objs" and "mscc_seville-objs" v2 * New patch --- drivers/net/dsa/ocelot/felix.c | 58 ++++++++++++++++++------ drivers/net/dsa/ocelot/felix.h | 4 +- drivers/net/dsa/ocelot/felix_vsc9959.c | 2 +- drivers/net/dsa/ocelot/seville_vsc9953.c | 2 +- 4 files changed, 48 insertions(+), 18 deletions(-)