| Message ID | 20220628081709.829811-2-colin.foster@in-advantage.com |
|---|---|
| State | New |
| Headers | show |
| Series | add support for VSC7512 control over SPI | expand |
On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote: > On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote: > > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote: > > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and > > > > dev_get_regmap(), maybe those could be of help? > > > > > > ah, I see I haven't really brought anything of value to the table, > > > dev_get_regmap() was discussed around v1 or so. I'll read the > > > discussions again in a couple of hours to remember what was wrong with > > > it such that you aren't using it anymore. > > > > It would be nice if you can comment after here to clarify that. > > Because in another series (not related to this anyhow) somebody > > insisted to use dev_get_regmap(). > > To add some info: The main issue at that time was "how do I get a spi > regmap instead of an mmio regmap from the device". V1 was very early, > and was before I knew about the pinctrl / mdio drivers that were to > come, so that led to the existing MFD implementation. > > I came across the IORESOURCE_REG, which seems to be created for exactly > this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG > doesn't get used much compared to IORESOURCE_MEM. > > Though I'll revisit this again. The switch portion of this driver (no > longer included in this patch set) is actually quite different from the > rest of the MFD in how it allocates regmaps, and that might have been > a major contributor at the time. So maybe I dismissed it at the time, > but it actually makes sense for the MFD portion now. I'm sorry, I can't actually understand what went wrong, I'll need some help from you, Colin. So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked about dev_get_regmap(dev->parent, res->name) yourself and proposed a relatively simple interface where the mfd child drivers would just request a regmap by its name: https://patchwork.kernel.org/project/netdevbpf/patch/20211119224313.2803941-4-colin.foster@in-advantage.com/ In fact you implemented just this (Dec. 6, 2021): https://patchwork.kernel.org/project/netdevbpf/patch/20211203211611.946658-1-colin.foster@in-advantage.com/#24637477 it's just that the pattern went something like: @@ -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); where Lee Jones (rightfully) commented asking why can't you just first check whether dev->parent has any GCB regmap to give you, and only then resort to call devm_regmap_init_mmio? A small comment with a small and pretty actionable change to be made. As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after v1 where you came back with proposed mfd patches: https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-2-colin.foster@in-advantage.com/ And while dev_get_regmap() was technically still there, its usage pattern changed radically. It was now just used as a sort of optimization in ocelot_mfd_regmap_init() to not create twice a regmap that already existed. What you introduced in RFC v5 instead was this "interface for MFD children to get regmaps": https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-3-colin.foster@in-advantage.com/ to which Lee replied that "This is almost certainly not the right way to do whatever it is you're trying to do!" And rightfully so. What happened to dev_get_regmap() as the "interface for MFD children to get regmaps" I wonder? dev_get_regmap() just wants a name, not a full blown resource. When you're a mfd child, you don't have a full resource, you just know the name of the regmap you want to use. Only the top dog needs to have access to the resources. And DSA as a MFD child is not top dog. So of course I expect it to change. Otherwise said, what is currently done in felix_init_structs() needs to be more or less replicated in its entirety in drivers/mfd/ocelot-core.c. All the regmaps of the switch SoC, created at mfd parent probe time, not on demand, and attached via devres to the mfd parent device, so that children can get them via dev_get_regmap. Next thing would be to modify the felix DSA driver so that it only attempts to create regmaps if it can do so (if it has the resource structures). If it doesn't have the resource structures, it calls dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the regmaps that way. If that fails => so sad, too bad, fail to probe, bye. The point is that the ocelot-ext driver you're trying to introduce should have no resources in struct resource *target_io_res, *port_io_res, *imdio_res, etc. I really don't know why you're so fixated on this "regmap from resource" thing when the resource shouldn't even be of concern to the driver when used as a mfd child. The contract is _not_ "here's the resource, give me the regmap". The contract is "I want the regmap named XXX". And in order to fulfill that contract, a mfd child driver should _not_ call a symbol exported by the ocelot parent driver directly (for the builtin vs module dependency reasons you've mentioned yourself), but get the regmap from the list of regmaps managed by devres using devm_regmap_init(). Yes, there would need to exist a split between the target strings and their start and end offsets, because the ocelot-ext driver would still call dev_get_regmap() by the name. But that's a fairly minor rework, and by the way, it turns out that the introduction of felix->info->init_regmap() was indeed not the right thing to do, so you'll need to change that again. I am really not sure what went with the plan above. You said a while ago that you don't like the fact that regmaps exist in the parent independently of the drivers that use them and continue to do so even after the children unbind, and that feels "wrong". Yes, because?
Hi Vladimir, Wow, thanks for the summary! This is jogging my memory. On Wed, Jun 29, 2022 at 05:53:06PM +0000, Vladimir Oltean wrote: > On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote: > > On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote: > > > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > > > > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote: > > > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and > > > > > dev_get_regmap(), maybe those could be of help? > > > > > > > > ah, I see I haven't really brought anything of value to the table, > > > > dev_get_regmap() was discussed around v1 or so. I'll read the > > > > discussions again in a couple of hours to remember what was wrong with > > > > it such that you aren't using it anymore. > > > > > > It would be nice if you can comment after here to clarify that. > > > Because in another series (not related to this anyhow) somebody > > > insisted to use dev_get_regmap(). > > > > To add some info: The main issue at that time was "how do I get a spi > > regmap instead of an mmio regmap from the device". V1 was very early, > > and was before I knew about the pinctrl / mdio drivers that were to > > come, so that led to the existing MFD implementation. > > > > I came across the IORESOURCE_REG, which seems to be created for exactly > > this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG > > doesn't get used much compared to IORESOURCE_MEM. > > > > Though I'll revisit this again. The switch portion of this driver (no > > longer included in this patch set) is actually quite different from the > > rest of the MFD in how it allocates regmaps, and that might have been > > a major contributor at the time. So maybe I dismissed it at the time, > > but it actually makes sense for the MFD portion now. > > I'm sorry, I can't actually understand what went wrong, I'll need some > help from you, Colin. > > So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked > about dev_get_regmap(dev->parent, res->name) yourself and proposed a > relatively simple interface where the mfd child drivers would just > request a regmap by its name: > https://patchwork.kernel.org/project/netdevbpf/patch/20211119224313.2803941-4-colin.foster@in-advantage.com/ > > In fact you implemented just this (Dec. 6, 2021): > https://patchwork.kernel.org/project/netdevbpf/patch/20211203211611.946658-1-colin.foster@in-advantage.com/#24637477 > it's just that the pattern went something like: > > @@ -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); > > where Lee Jones (rightfully) commented asking why can't you just first > check whether dev->parent has any GCB regmap to give you, and only then > resort to call devm_regmap_init_mmio? A small comment with a small > and pretty actionable change to be made. > > > As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after > v1 where you came back with proposed mfd patches: > https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-2-colin.foster@in-advantage.com/ > > And while dev_get_regmap() was technically still there, its usage > pattern changed radically. It was now just used as a sort of > optimization in ocelot_mfd_regmap_init() to not create twice a regmap > that already existed. > What you introduced in RFC v5 instead was this "interface for MFD > children to get regmaps": > https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-3-colin.foster@in-advantage.com/ > > to which Lee replied that "This is almost certainly not the right way to > do whatever it is you're trying to do!" > > And rightfully so. What happened to dev_get_regmap() as the "interface > for MFD children to get regmaps" I wonder? > dev_get_regmap() just wants a name, not a full blown resource. > When you're a mfd child, you don't have a full resource, you just know > the name of the regmap you want to use. Only the top dog needs to have > access to the resources. And DSA as a MFD child is not top dog. > So of course I expect it to change. Otherwise said, what is currently > done in felix_init_structs() needs to be more or less replicated in its > entirety in drivers/mfd/ocelot-core.c. > All the regmaps of the switch SoC, created at mfd parent probe time, not > on demand, and attached via devres to the mfd parent device, so that > children can get them via dev_get_regmap. > I think you're pointing out exactly where I went wrong. And I think it might be related to my starting with the felix dsa driver before migrating to the MFD setup. My misconception was "the felix dsa driver should be able to create whatever regmaps from resources it needs" and maybe I was being to optimistic that I wouldn't need to change felix dsa's implementation. Or naive. > Next thing would be to modify the felix DSA driver so that it only > attempts to create regmaps if it can do so (if it has the resource > structures). If it doesn't have the resource structures, it calls > dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the > regmaps that way. If that fails => so sad, too bad, fail to probe, bye. > > The point is that the ocelot-ext driver you're trying to introduce > should have no resources in struct resource *target_io_res, *port_io_res, > *imdio_res, etc. I really don't know why you're so fixated on this > "regmap from resource" thing when the resource shouldn't even be of > concern to the driver when used as a mfd child. > > The contract is _not_ "here's the resource, give me the regmap". > The contract is "I want the regmap named XXX". And in order to fulfill > that contract, a mfd child driver should _not_ call a symbol exported by > the ocelot parent driver directly (for the builtin vs module dependency > reasons you've mentioned yourself), but get the regmap from the list of > regmaps managed by devres using devm_regmap_init(). > > Yes, there would need to exist a split between the target strings and > their start and end offsets, because the ocelot-ext driver would still > call dev_get_regmap() by the name. But that's a fairly minor rework, and > by the way, it turns out that the introduction of felix->info->init_regmap() > was indeed not the right thing to do, so you'll need to change that again. > > I am really not sure what went with the plan above. You said a while ago > that you don't like the fact that regmaps exist in the parent > independently of the drivers that use them and continue to do so even > after the children unbind, and that feels "wrong". Yes, because? I liked the idea of the MFD being "code complete" so if future regmaps were needed for the felix dsa driver came about, it wouldn't require changes to the "parent." But I think that was a bad goal - especially since MFD requires all the resources anyway. Also at the time, I was trying a hybrid "create it if it doesn't exist, return it if was already created" approach. I backed that out after an RFC. Focusing only on the non-felix drivers: it seems trivial for the parent to create _all_ the possible child regmaps, register them to the parent via by way of regmap_attach_dev(). At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to initalize like (untested, and apologies for indentation): regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); if (IS_ERR(regs)) { map = dev_get_regmap(dev->parent, name); } else { map = devm_regmap_init_mmio(dev, regs, config); } In that case, "name" would either be hard-coded to match what is in drivers/mfd/ocelot-core.c. The other option is to fall back to platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in resource->name. I'll be able to deal with that when I try it. (hopefully this evening) This seems to be a solid design that I missed! As you mention, it'll require changes to felix dsa... but not as much as I had feared. And I think it solves all my fears about modules to boot. This seems too good to be true - but maybe I was too deep and needed to take this step back.
On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote: > I liked the idea of the MFD being "code complete" so if future regmaps > were needed for the felix dsa driver came about, it wouldn't require > changes to the "parent." But I think that was a bad goal - especially > since MFD requires all the resources anyway. > > Also at the time, I was trying a hybrid "create it if it doesn't exist, > return it if was already created" approach. I backed that out after an > RFC. > > Focusing only on the non-felix drivers: it seems trivial for the parent > to create _all_ the possible child regmaps, register them to the parent > via by way of regmap_attach_dev(). > > At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to > initalize like (untested, and apologies for indentation): > > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(regs)) { > map = dev_get_regmap(dev->parent, name); > } else { > map = devm_regmap_init_mmio(dev, regs, config); > } Again, those dev_err(dev, "invalid resource\n"); prints you were complaining about earlier are self-inflicted IMO, and caused exactly by this pattern. I get why you prefer to call larger building blocks if possible, but in this case, devm_platform_get_and_ioremap_resource() calls exactly 2 sub-functions: platform_get_resource() and devm_ioremap_resource(). The IS_ERR() that you check for is caused by devm_ioremap_resource() being passed a NULL pointer, and same goes for the print. Just call them individually, and put your dev_get_regmap() hook in case platform_get_resource() returns NULL, rather than passing NULL to devm_ioremap_resource() and waiting for that to fail. > In that case, "name" would either be hard-coded to match what is in > drivers/mfd/ocelot-core.c. The other option is to fall back to > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in > resource->name. I'll be able to deal with that when I try it. (hopefully > this evening) I'm not exactly clear on what you'd do with the REG resource once you get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;" from the device tree, what next, who's going to set up the SPI regmap for you? > This seems to be a solid design that I missed! As you mention, it'll > require changes to felix dsa... but not as much as I had feared. And I > think it solves all my fears about modules to boot. This seems too good > to be true - but maybe I was too deep and needed to take this step back.
On Wed, Jun 29, 2022 at 11:08:05PM +0000, Vladimir Oltean wrote: > On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote: > > I liked the idea of the MFD being "code complete" so if future regmaps > > were needed for the felix dsa driver came about, it wouldn't require > > changes to the "parent." But I think that was a bad goal - especially > > since MFD requires all the resources anyway. > > > > Also at the time, I was trying a hybrid "create it if it doesn't exist, > > return it if was already created" approach. I backed that out after an > > RFC. > > > > Focusing only on the non-felix drivers: it seems trivial for the parent > > to create _all_ the possible child regmaps, register them to the parent > > via by way of regmap_attach_dev(). > > > > At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to > > initalize like (untested, and apologies for indentation): > > > > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > if (IS_ERR(regs)) { > > map = dev_get_regmap(dev->parent, name); > > } else { > > map = devm_regmap_init_mmio(dev, regs, config); > > } > > Again, those dev_err(dev, "invalid resource\n"); prints you were > complaining about earlier are self-inflicted IMO, and caused exactly by > this pattern. I get why you prefer to call larger building blocks if > possible, but in this case, devm_platform_get_and_ioremap_resource() > calls exactly 2 sub-functions: platform_get_resource() and > devm_ioremap_resource(). The IS_ERR() that you check for is caused by > devm_ioremap_resource() being passed a NULL pointer, and same goes for > the print. Just call them individually, and put your dev_get_regmap() > hook in case platform_get_resource() returns NULL, rather than passing > NULL to devm_ioremap_resource() and waiting for that to fail. I see that now. Hoping this next version removes a lot of this unnecessary complexity. > > > In that case, "name" would either be hard-coded to match what is in > > drivers/mfd/ocelot-core.c. The other option is to fall back to > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in > > resource->name. I'll be able to deal with that when I try it. (hopefully > > this evening) > > I'm not exactly clear on what you'd do with the REG resource once you > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;" > from the device tree, what next, who's going to set up the SPI regmap > for you? The REG resource would only get the resource name, while the MFD core driver would set up the regmaps. e.g. drivers/mfd/ocelot-core.c has (annotated): static const struct resource vsc7512_sgpio_resources[] = { DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") }; Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the gpio resource, and gets the resource by index. So for this there seem to be two options: Option 1: drivers/pinctrl/pinctrl-ocelot.c: res = platform_get_resource(pdev, IORESOURCE_REG, 0); map = dev_get_regmap(dev->parent, res->name); OR Option 2: include/linux/mfd/ocelot.h has something like: #define GCB_GPIO_REGMAP_NAME "gcb_gpio" and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to: map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME); (With error checking, macro reuse, etc.) I like option 1, since it then makes ocelot-pinctrl.c have no reliance on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are set up in advance during the start of ocelot_core_init, just before devm_mfd_add_devices is called. I should be able to test this all tonight. > > > This seems to be a solid design that I missed! As you mention, it'll > > require changes to felix dsa... but not as much as I had feared. And I > > think it solves all my fears about modules to boot. This seems too good > > to be true - but maybe I was too deep and needed to take this step back.
On Wed, 29 Jun 2022, Colin Foster wrote: > On Wed, Jun 29, 2022 at 11:08:05PM +0000, Vladimir Oltean wrote: > > On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote: > > > I liked the idea of the MFD being "code complete" so if future regmaps > > > were needed for the felix dsa driver came about, it wouldn't require > > > changes to the "parent." But I think that was a bad goal - especially > > > since MFD requires all the resources anyway. > > > > > > Also at the time, I was trying a hybrid "create it if it doesn't exist, > > > return it if was already created" approach. I backed that out after an > > > RFC. > > > > > > Focusing only on the non-felix drivers: it seems trivial for the parent > > > to create _all_ the possible child regmaps, register them to the parent > > > via by way of regmap_attach_dev(). > > > > > > At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to > > > initalize like (untested, and apologies for indentation): > > > > > > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > > if (IS_ERR(regs)) { > > > map = dev_get_regmap(dev->parent, name); > > > } else { > > > map = devm_regmap_init_mmio(dev, regs, config); > > > } > > > > Again, those dev_err(dev, "invalid resource\n"); prints you were > > complaining about earlier are self-inflicted IMO, and caused exactly by > > this pattern. I get why you prefer to call larger building blocks if > > possible, but in this case, devm_platform_get_and_ioremap_resource() > > calls exactly 2 sub-functions: platform_get_resource() and > > devm_ioremap_resource(). The IS_ERR() that you check for is caused by > > devm_ioremap_resource() being passed a NULL pointer, and same goes for > > the print. Just call them individually, and put your dev_get_regmap() > > hook in case platform_get_resource() returns NULL, rather than passing > > NULL to devm_ioremap_resource() and waiting for that to fail. > > I see that now. Hoping this next version removes a lot of this > unnecessary complexity. > > > > > > In that case, "name" would either be hard-coded to match what is in > > > drivers/mfd/ocelot-core.c. The other option is to fall back to > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in > > > resource->name. I'll be able to deal with that when I try it. (hopefully > > > this evening) > > > > I'm not exactly clear on what you'd do with the REG resource once you > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;" > > from the device tree, what next, who's going to set up the SPI regmap > > for you? > > The REG resource would only get the resource name, while the MFD core > driver would set up the regmaps. > > e.g. drivers/mfd/ocelot-core.c has (annotated): > static const struct resource vsc7512_sgpio_resources[] = { > DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") }; > > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the > gpio resource, and gets the resource by index. > > So for this there seem to be two options: > Option 1: > drivers/pinctrl/pinctrl-ocelot.c: > res = platform_get_resource(pdev, IORESOURCE_REG, 0); > map = dev_get_regmap(dev->parent, res->name); > > > OR Option 2: > include/linux/mfd/ocelot.h has something like: > #define GCB_GPIO_REGMAP_NAME "gcb_gpio" > > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to: > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME); > > (With error checking, macro reuse, etc.) > > > I like option 1, since it then makes ocelot-pinctrl.c have no reliance > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are > set up in advance during the start of ocelot_core_init, just before > devm_mfd_add_devices is called. > > > I should be able to test this all tonight. Thank you Vladimir for stepping in to clarify previous points. Well done both of you for this collaborative effort. Great to see!
On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote: > > > In that case, "name" would either be hard-coded to match what is in > > > drivers/mfd/ocelot-core.c. The other option is to fall back to > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in > > > resource->name. I'll be able to deal with that when I try it. (hopefully > > > this evening) > > > > I'm not exactly clear on what you'd do with the REG resource once you > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;" > > from the device tree, what next, who's going to set up the SPI regmap > > for you? > > The REG resource would only get the resource name, while the MFD core > driver would set up the regmaps. > > e.g. drivers/mfd/ocelot-core.c has (annotated): > static const struct resource vsc7512_sgpio_resources[] = { > DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") }; > > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the > gpio resource, and gets the resource by index. > > So for this there seem to be two options: > Option 1: > drivers/pinctrl/pinctrl-ocelot.c: > res = platform_get_resource(pdev, IORESOURCE_REG, 0); > map = dev_get_regmap(dev->parent, res->name); > > > OR Option 2: > include/linux/mfd/ocelot.h has something like: > #define GCB_GPIO_REGMAP_NAME "gcb_gpio" > > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to: > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME); > > (With error checking, macro reuse, etc.) > > > I like option 1, since it then makes ocelot-pinctrl.c have no reliance > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are > set up in advance during the start of ocelot_core_init, just before > devm_mfd_add_devices is called. > > > I should be able to test this all tonight. I see what you mean now with the named resources from drivers/mfd/ocelot-core.c. I don't particularly like the platform_get_resource(0) option, because it's not as obvious/searchable what resource the pinctrl driver is asking for. I suppose a compromise variant might be to combine the 2 options. Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c, then create a _single_ resource table in the MFD driver, indexed by enum ocelot_target: static const struct resource vsc7512_resources[TARGET_MAX] = { [ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"), ... }; then provide the exact same resource table to all children. In the pinctrl driver you can then do: res = platform_get_resource(pdev, IORESOURCE_REG, GPIO); map = dev_get_regmap(dev->parent, res->name); and you get both the benefit of not hardcoding the string twice, and the benefit of having some obvious keyword which can be used to link the mfd driver to the child driver via grep, for those trying to understand what goes on. In addition, if there's a single resource table used for all peripherals, theoretically you need to modify less code in mfd/ocelot-core.c in case one driver or another needs access to one more regmap, if that regmap happened to be needed by some other driver already. Plus fewer tables to lug around, in general.
On Thu, Jun 30, 2022 at 01:11:56PM +0000, Vladimir Oltean wrote: > On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote: > > > > In that case, "name" would either be hard-coded to match what is in > > > > drivers/mfd/ocelot-core.c. The other option is to fall back to > > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in > > > > resource->name. I'll be able to deal with that when I try it. (hopefully > > > > this evening) > > > > > > I'm not exactly clear on what you'd do with the REG resource once you > > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;" > > > from the device tree, what next, who's going to set up the SPI regmap > > > for you? > > > > The REG resource would only get the resource name, while the MFD core > > driver would set up the regmaps. > > > > e.g. drivers/mfd/ocelot-core.c has (annotated): > > static const struct resource vsc7512_sgpio_resources[] = { > > DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") }; > > > > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the > > gpio resource, and gets the resource by index. > > > > So for this there seem to be two options: > > Option 1: > > drivers/pinctrl/pinctrl-ocelot.c: > > res = platform_get_resource(pdev, IORESOURCE_REG, 0); > > map = dev_get_regmap(dev->parent, res->name); > > > > > > OR Option 2: > > include/linux/mfd/ocelot.h has something like: > > #define GCB_GPIO_REGMAP_NAME "gcb_gpio" > > > > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to: > > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME); > > > > (With error checking, macro reuse, etc.) > > > > > > I like option 1, since it then makes ocelot-pinctrl.c have no reliance > > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are > > set up in advance during the start of ocelot_core_init, just before > > devm_mfd_add_devices is called. > > > > > > I should be able to test this all tonight. > > I see what you mean now with the named resources from drivers/mfd/ocelot-core.c. > I don't particularly like the platform_get_resource(0) option, because > it's not as obvious/searchable what resource the pinctrl driver is > asking for. > > I suppose a compromise variant might be to combine the 2 options. > Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c, > then create a _single_ resource table in the MFD driver, indexed by enum > ocelot_target: > > static const struct resource vsc7512_resources[TARGET_MAX] = { > [ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"), > ... > }; > > then provide the exact same resource table to all children. > > In the pinctrl driver you can then do: > res = platform_get_resource(pdev, IORESOURCE_REG, GPIO); > map = dev_get_regmap(dev->parent, res->name); > > and you get both the benefit of not hardcoding the string twice, and the > benefit of having some obvious keyword which can be used to link the mfd > driver to the child driver via grep, for those trying to understand what > goes on. > > In addition, if there's a single resource table used for all peripherals, > theoretically you need to modify less code in mfd/ocelot-core.c in case > one driver or another needs access to one more regmap, if that regmap > happened to be needed by some other driver already. Plus fewer tables to > lug around, in general. Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet, but I'm liking this: static inline struct regmap * ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index, const struct regmap_config *config) { struct device *dev = &pdev->dev; struct resource *res; u32 __iomem *regs; res = platform_get_resource(pdev, IORESOURCE_MEM, index); if (res) { regs = devm_ioremap_resource(dev, res); if (IS_ERR(regs)) return ERR_CAST(regs); return devm_regmap_init_mmio(dev, regs, config); } /* * Fall back to using REG and getting the resource from the parent * device, which is possible in an MFD configuration */ res = platform_get_resource(pdev, IORESOURCE_REG, index); if (!res) return ERR_PTR(-ENOENT); return (dev_get_regmap(dev->parent, res->name)); } So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain an inline helper function. And so long as ocelot_core_init does this: static void ocelot_core_try_add_regmap(struct device *dev, const struct resource *res) { if (!dev_get_regmap(dev, res->name)) { ocelot_spi_init_regmap(dev, res); } } static void ocelot_core_try_add_regmaps(struct device *dev, const struct mfd_cell *cell) { int i; for (i = 0; i < cell->num_resources; i++) { ocelot_core_try_add_regmap(dev, &cell->resources[i]); } } int ocelot_core_init(struct device *dev) { int i, ndevs; ndevs = ARRAY_SIZE(vsc7512_devs); for (i = 0; i < ndevs; i++) ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL); } EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux game still) I like the enum / macro idea for cleanup, but I think that's a different problem I can address. The main question I have now is this: The ocelot_regmap_from_resource now has nothing to do with the ocelot MFD system. It is generic. (If you listen carefully, you might hear me cheering) I can keep this in linux/mfd/ocelot.h, but is this actually something that belongs elsewhere? platform? device? mfd-core? And yes, I like the idea of changing the driver to "ocelot_regmap_from_resource(pdev, GPIO, config);" from "ocelot_regmap_from_resource(pdev, 0, config);"
On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote: > Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet, > but I'm liking this: > > static inline struct regmap * > ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index, > const struct regmap_config *config) > { > struct device *dev = &pdev->dev; > struct resource *res; > u32 __iomem *regs; > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > if (res) { > regs = devm_ioremap_resource(dev, res); > if (IS_ERR(regs)) > return ERR_CAST(regs); > return devm_regmap_init_mmio(dev, regs, config); > } > > /* > * Fall back to using REG and getting the resource from the parent > * device, which is possible in an MFD configuration > */ > res = platform_get_resource(pdev, IORESOURCE_REG, index); > if (!res) > return ERR_PTR(-ENOENT); > > return (dev_get_regmap(dev->parent, res->name)); parentheses not needed around dev_get_regmap. > } > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain > an inline helper function. And so long as ocelot_core_init does this: > > static void ocelot_core_try_add_regmap(struct device *dev, > const struct resource *res) > { > if (!dev_get_regmap(dev, res->name)) { > ocelot_spi_init_regmap(dev, res); > } > } > > static void ocelot_core_try_add_regmaps(struct device *dev, > const struct mfd_cell *cell) > { > int i; > > for (i = 0; i < cell->num_resources; i++) { > ocelot_core_try_add_regmap(dev, &cell->resources[i]); > } > } > > int ocelot_core_init(struct device *dev) > { > int i, ndevs; > > ndevs = ARRAY_SIZE(vsc7512_devs); > > for (i = 0; i < ndevs; i++) > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); Dumb question, why just "try"? > > return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, > ndevs, NULL, 0, NULL); > } > EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); > > we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux > game still) > > > I like the enum / macro idea for cleanup, but I think that's a different > problem I can address. The main question I have now is this: > > The ocelot_regmap_from_resource now has nothing to do with the ocelot > MFD system. It is generic. (If you listen carefully, you might hear me > cheering) > > I can keep this in linux/mfd/ocelot.h, but is this actually something > that belongs elsewhere? platform? device? mfd-core? Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(), but I'm not 100% clear where it should sit. Platform devices are independent of regmap, regmap is independent of platform devices, device core of both. FWIW platform devices are always built-in and have no config option; regmap is bool and is selected by others. Logically, the melting pot of regmaps and platform devices is mfd. However, it seems that include/linux/mfd/core.h only provides API for mfd parent drivers, not children. So a new header would be needed? Alternatively, you could just duplicate this logic in the drivers (by the way, only spelling out the function name takes up half of the implementation). How many times would it be needed? Felix DSA would roll its own thing, as mentioned. I'm thinking, let it be open coded for now, let's agree on the entire solution in terms of operations that are actually being done, and we can revisit proper placement for this later. > And yes, I like the idea of changing the driver to > "ocelot_regmap_from_resource(pdev, GPIO, config);" from > "ocelot_regmap_from_resource(pdev, 0, config);" Sorry, I just realized we need to junk this idea with GPIO instead of 0. Presenting the entire resource table to all peripherals implies that there is no more than one single peripheral of each kind. This is not true for the MDIO controllers, where the driver would need to know it has to request the region corresponding to MIIM1 or MIIM2 according to some crystal ball.
On Fri, Jul 01, 2022 at 04:21:28PM +0000, Vladimir Oltean wrote: > On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote: > > Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet, > > but I'm liking this: > > > > static inline struct regmap * > > ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index, > > const struct regmap_config *config) > > { > > struct device *dev = &pdev->dev; > > struct resource *res; > > u32 __iomem *regs; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > > if (res) { > > regs = devm_ioremap_resource(dev, res); > > if (IS_ERR(regs)) > > return ERR_CAST(regs); > > return devm_regmap_init_mmio(dev, regs, config); > > } > > > > /* > > * Fall back to using REG and getting the resource from the parent > > * device, which is possible in an MFD configuration > > */ > > res = platform_get_resource(pdev, IORESOURCE_REG, index); > > if (!res) > > return ERR_PTR(-ENOENT); > > > > return (dev_get_regmap(dev->parent, res->name)); > > parentheses not needed around dev_get_regmap. Oops. While I have your ear: do I need to check for dev->parent == NULL before calling dev_get_regmap? I see find_dr will call (dev->parent)->devres_head... but specifically "does every device have a valid parent?" > > > } > > > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain > > an inline helper function. And so long as ocelot_core_init does this: > > > > static void ocelot_core_try_add_regmap(struct device *dev, > > const struct resource *res) > > { > > if (!dev_get_regmap(dev, res->name)) { > > ocelot_spi_init_regmap(dev, res); > > } > > } > > > > static void ocelot_core_try_add_regmaps(struct device *dev, > > const struct mfd_cell *cell) > > { > > int i; > > > > for (i = 0; i < cell->num_resources; i++) { > > ocelot_core_try_add_regmap(dev, &cell->resources[i]); > > } > > } > > > > int ocelot_core_init(struct device *dev) > > { > > int i, ndevs; > > > > ndevs = ARRAY_SIZE(vsc7512_devs); > > > > for (i = 0; i < ndevs; i++) > > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); > > Dumb question, why just "try"? Because of this conditional: > > if (!dev_get_regmap(dev, res->name)) { Don't add it if it is already there. This might get interesting... The soc uses the HSIO regmap by way of syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If dev->parent has all the regmaps, what role does syscon play? But that's a problem for another day... > > > > > return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, > > ndevs, NULL, 0, NULL); > > } > > EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT); > > > > we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux > > game still) > > > > > > I like the enum / macro idea for cleanup, but I think that's a different > > problem I can address. The main question I have now is this: > > > > The ocelot_regmap_from_resource now has nothing to do with the ocelot > > MFD system. It is generic. (If you listen carefully, you might hear me > > cheering) > > > > I can keep this in linux/mfd/ocelot.h, but is this actually something > > that belongs elsewhere? platform? device? mfd-core? > > Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(), > but I'm not 100% clear where it should sit. Platform devices are independent > of regmap, regmap is independent of platform devices, device core of both. > > FWIW platform devices are always built-in and have no config option; > regmap is bool and is selected by others. > > Logically, the melting pot of regmaps and platform devices is mfd. > However, it seems that include/linux/mfd/core.h only provides API for > mfd parent drivers, not children. So a new header would be needed? > > Alternatively, you could just duplicate this logic in the drivers > (by the way, only spelling out the function name takes up half of the > implementation). How many times would it be needed? Felix DSA would roll > its own thing, as mentioned. I'm thinking, let it be open coded for now, > let's agree on the entire solution in terms of operations that are > actually being done, and we can revisit proper placement for this later. I came to the same conclusion. Hopefully I'll button up v12 today. > > > And yes, I like the idea of changing the driver to > > "ocelot_regmap_from_resource(pdev, GPIO, config);" from > > "ocelot_regmap_from_resource(pdev, 0, config);" > > Sorry, I just realized we need to junk this idea with GPIO instead of 0. > Presenting the entire resource table to all peripherals implies that > there is no more than one single peripheral of each kind. This is not > true for the MDIO controllers, where the driver would need to know it > has to request the region corresponding to MIIM1 or MIIM2 according to > some crystal ball.
On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote: > While I have your ear: do I need to check for dev->parent == NULL before > calling dev_get_regmap? I see find_dr will call > (dev->parent)->devres_head... but specifically "does every device have a > valid parent?" While the technical answer is "no", the practical answer is "pretty much". Platform devices sit at least on the "platform" bus created in drivers/base/platform.c, and they are reparented to the "platform_bus" struct device named "platform" within platform_device_add(), if they don't have a parent. Additionally, for MMIO-controlled platform devices in Ocelot, these have as parent a platform device probed by the drivers/bus/simple-pm-bus.c driver on the "ahb@70000000" simple-bus OF node. That simple-bus platform device has as parent the "platform_bus" device mentioned above. So it's a pretty long way to the top in the device hierarchy, I wouldn't concern myself too much with checking for NULL, unless you intend to call dev_get_regmap() on a parent's parent's parent, or things like that. > > > } > > > > > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain > > > an inline helper function. And so long as ocelot_core_init does this: > > > > > > static void ocelot_core_try_add_regmap(struct device *dev, > > > const struct resource *res) > > > { > > > if (!dev_get_regmap(dev, res->name)) { > > > ocelot_spi_init_regmap(dev, res); > > > } > > > } > > > > > > static void ocelot_core_try_add_regmaps(struct device *dev, > > > const struct mfd_cell *cell) > > > { > > > int i; > > > > > > for (i = 0; i < cell->num_resources; i++) { > > > ocelot_core_try_add_regmap(dev, &cell->resources[i]); > > > } > > > } > > > > > > int ocelot_core_init(struct device *dev) > > > { > > > int i, ndevs; > > > > > > ndevs = ARRAY_SIZE(vsc7512_devs); > > > > > > for (i = 0; i < ndevs; i++) > > > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]); > > > > Dumb question, why just "try"? > > Because of this conditional: > > > if (!dev_get_regmap(dev, res->name)) { > Don't add it if it is already there. Hmm. So that's because you add regmaps iterating by the resource table of each device. What if you keep a single resource table for regmap creation purposes, and the device resource tables as separate? > This might get interesting... The soc uses the HSIO regmap by way of > syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If > dev->parent has all the regmaps, what role does syscon play? > > But that's a problem for another day... Interesting question. I think part of the reason why syscon exists is to not have OF nodes with overlapping address regions. In that sense, its need does not go away here - I expect the layout of OF nodes beneath the ocelot SPI device to be the same as their AHB variants. But in terms of driver implementation, I don't know. Even if the OF nodes for your MFD functions will contain all the regs that their AHB variants do, I'd personally still be inclined to also hardcode those as resources in the ocelot mfd parent driver and use those - case in which the OF regs will more or less exist just as a formality. Maybe because the HSIO syscon is already compatible with "simple-mfd", devices beneath it should just probe. I haven't studied how syscon_node_to_regmap() behaves when the syscon itself is probed as a MFD function. If that "just works", then the phy-ocelot-serdes.c driver might not need to be modified.
diff --git a/MAINTAINERS b/MAINTAINERS index 36f0a205c54a..4d9ccec78f18 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14413,6 +14413,11 @@ F: net/dsa/tag_ocelot.c F: net/dsa/tag_ocelot_8021q.c F: tools/testing/selftests/drivers/net/ocelot/* +OCELOT EXTERNAL SWITCH CONTROL +M: Colin Foster <colin.foster@in-advantage.com> +S: Supported +F: include/linux/mfd/ocelot.h + OCXL (Open Coherent Accelerator Processor Interface OpenCAPI) DRIVER M: Frederic Barrat <fbarrat@linux.ibm.com> M: Andrew Donnellan <ajd@linux.ibm.com> diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h new file mode 100644 index 000000000000..5c95e4ee38a6 --- /dev/null +++ b/include/linux/mfd/ocelot.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/* Copyright 2022 Innovative Advantage Inc. */ + +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/types.h> + +struct resource; + +static inline struct regmap * +ocelot_platform_init_regmap_from_resource(struct platform_device *pdev, + unsigned int index, + const struct regmap_config *config) +{ + struct resource *res; + u32 __iomem *regs; + + regs = devm_platform_get_and_ioremap_resource(pdev, index, &res); + + if (!res) + return ERR_PTR(-ENOENT); + else if (IS_ERR(regs)) + return ERR_CAST(regs); + else + return devm_regmap_init_mmio(&pdev->dev, regs, config); +}
Several ocelot-related modules are designed for MMIO / regmaps. As such, they often use a combination of devm_platform_get_and_ioremap_resource and devm_regmap_init_mmio. Operating in an MFD might be different, in that it could be memory mapped, or it could be SPI, I2C... In these cases a fallback to use IORESOURCE_REG instead of IORESOURCE_MEM becomes necessary. When this happens, there's redundant logic that needs to be implemented in every driver. In order to avoid this redundancy, utilize a single function that, if the MFD scenario is enabled, will perform this fallback logic. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- MAINTAINERS | 5 +++++ include/linux/mfd/ocelot.h | 27 +++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 include/linux/mfd/ocelot.h