Re: [PATCH v3] amba: Remove deferred device addition
On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski wrote: > > Hi Saravana, > > On 05.03.2021 19:02, Saravana Kannan wrote: > > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski > > wrote: > >> On 04.03.2021 20:51, Saravana Kannan wrote: > >>> The uevents generated for an amba device need PID and CID information > >>> that's available only when the amba device is powered on, clocked and > >>> out of reset. So, if those resources aren't available, the information > >>> can't be read to generate the uevents. To workaround this requirement, > >>> if the resources weren't available, the device addition was deferred and > >>> retried periodically. > >>> > >>> However, this deferred addition retry isn't based on resources becoming > >>> available. Instead, it's retried every 5 seconds and causes arbitrary > >>> probe delays for amba devices and their consumers. > >>> > >>> Also, maintaining a separate deferred-probe like mechanism is > >>> maintenance headache. > >>> > >>> With this commit, instead of deferring the device addition, we simply > >>> defer the generation of uevents for the device and probing of the device > >>> (because drivers needs PID and CID to match) until the PID and CID > >>> information can be read. This allows us to delete all the amba specific > >>> deferring code and also avoid the arbitrary probing delays. > >>> > >>> Cc: Rob Herring > >>> Cc: Ulf Hansson > >>> Cc: John Stultz > >>> Cc: Saravana Kannan > >>> Cc: Linus Walleij > >>> Cc: Sudeep Holla > >>> Cc: Nicolas Saenz Julienne > >>> Cc: Geert Uytterhoeven > >>> Cc: Marek Szyprowski > >>> Cc: Russell King > >>> Signed-off-by: Saravana Kannan > >>> --- > >>> > >>> v1 -> v2: > >>> - Dropped RFC tag > >>> - Complete rewrite to not use stub devices. > >>> v2 -> v3: > >>> - Flipped the if() condition for hard-coded periphids. > >>> - Added a stub driver to handle the case where all amba drivers are > >>> modules loaded by uevents. > >>> - Cc Marek after I realized I forgot to add him. > >>> > >>> Marek, > >>> > >>> Would you mind testing this? It looks okay with my limited testing. > >> It looks it works fine on my test systems. I've checked current > >> linux-next and this patch. You can add: > >> > >> Tested-by: Marek Szyprowski > > Hi Marek, > > > > Thanks! Does your test set up have amda drivers that are loaded based > > on uevents? That's the one I couldn't test. > > I've checked both, the built-in and all amba drivers compiled as > modules, loaded by udev. Both works fine here. > > >> I've briefly scanned the code and I'm curious how does it work. Does it > >> depend on the recently introduced "fw_devlink=on" feature? I don't see > >> other mechanism, which would trigger matching amba device if pm domains, > >> clocks or resets were not available on time to read pid/cid while adding > >> a device... > > No, it does not depend on fw_devlink or device links in any way. > > > > When a device is attempted to be probed (when it's added or during > > deferred probe), it's matched with all the drivers on the bus. > > When a new driver is registered to a bus, all devices in that bus are > > matched with the driver to see if they'll work together. > > That's how match is called. And match() can return -EPROBE_DEFER and > > that'll cause the device to be put in the deferred probe list by > > driver core. > > > > The tricky part in this patch was the uevent handling and the > > chicken-and-egg issue I talk about in the comments. > > Thanks for the explanation. This EPROBE_DEFER support in match() > callback must be something added after I crafted that periodic retry > based workaround. > I think it got in just a few months before your patches, but your patches worked :) I actually don't like match returning -EPROBE_DEFER, but I can work around it for some of my fw_devlink optimization plans. More context here: https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=fhe...@mail.gmail.com/ -Saravana
Re: [PATCH v3] amba: Remove deferred device addition
Hi Saravana, On 05.03.2021 19:02, Saravana Kannan wrote: > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski > wrote: >> On 04.03.2021 20:51, Saravana Kannan wrote: >>> The uevents generated for an amba device need PID and CID information >>> that's available only when the amba device is powered on, clocked and >>> out of reset. So, if those resources aren't available, the information >>> can't be read to generate the uevents. To workaround this requirement, >>> if the resources weren't available, the device addition was deferred and >>> retried periodically. >>> >>> However, this deferred addition retry isn't based on resources becoming >>> available. Instead, it's retried every 5 seconds and causes arbitrary >>> probe delays for amba devices and their consumers. >>> >>> Also, maintaining a separate deferred-probe like mechanism is >>> maintenance headache. >>> >>> With this commit, instead of deferring the device addition, we simply >>> defer the generation of uevents for the device and probing of the device >>> (because drivers needs PID and CID to match) until the PID and CID >>> information can be read. This allows us to delete all the amba specific >>> deferring code and also avoid the arbitrary probing delays. >>> >>> Cc: Rob Herring >>> Cc: Ulf Hansson >>> Cc: John Stultz >>> Cc: Saravana Kannan >>> Cc: Linus Walleij >>> Cc: Sudeep Holla >>> Cc: Nicolas Saenz Julienne >>> Cc: Geert Uytterhoeven >>> Cc: Marek Szyprowski >>> Cc: Russell King >>> Signed-off-by: Saravana Kannan >>> --- >>> >>> v1 -> v2: >>> - Dropped RFC tag >>> - Complete rewrite to not use stub devices. >>> v2 -> v3: >>> - Flipped the if() condition for hard-coded periphids. >>> - Added a stub driver to handle the case where all amba drivers are >>> modules loaded by uevents. >>> - Cc Marek after I realized I forgot to add him. >>> >>> Marek, >>> >>> Would you mind testing this? It looks okay with my limited testing. >> It looks it works fine on my test systems. I've checked current >> linux-next and this patch. You can add: >> >> Tested-by: Marek Szyprowski > Hi Marek, > > Thanks! Does your test set up have amda drivers that are loaded based > on uevents? That's the one I couldn't test. I've checked both, the built-in and all amba drivers compiled as modules, loaded by udev. Both works fine here. >> I've briefly scanned the code and I'm curious how does it work. Does it >> depend on the recently introduced "fw_devlink=on" feature? I don't see >> other mechanism, which would trigger matching amba device if pm domains, >> clocks or resets were not available on time to read pid/cid while adding >> a device... > No, it does not depend on fw_devlink or device links in any way. > > When a device is attempted to be probed (when it's added or during > deferred probe), it's matched with all the drivers on the bus. > When a new driver is registered to a bus, all devices in that bus are > matched with the driver to see if they'll work together. > That's how match is called. And match() can return -EPROBE_DEFER and > that'll cause the device to be put in the deferred probe list by > driver core. > > The tricky part in this patch was the uevent handling and the > chicken-and-egg issue I talk about in the comments. Thanks for the explanation. This EPROBE_DEFER support in match() callback must be something added after I crafted that periodic retry based workaround. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
Re: [PATCH v3] amba: Remove deferred device addition
On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski wrote: > > Hi Saravana, > > On 04.03.2021 20:51, Saravana Kannan wrote: > > The uevents generated for an amba device need PID and CID information > > that's available only when the amba device is powered on, clocked and > > out of reset. So, if those resources aren't available, the information > > can't be read to generate the uevents. To workaround this requirement, > > if the resources weren't available, the device addition was deferred and > > retried periodically. > > > > However, this deferred addition retry isn't based on resources becoming > > available. Instead, it's retried every 5 seconds and causes arbitrary > > probe delays for amba devices and their consumers. > > > > Also, maintaining a separate deferred-probe like mechanism is > > maintenance headache. > > > > With this commit, instead of deferring the device addition, we simply > > defer the generation of uevents for the device and probing of the device > > (because drivers needs PID and CID to match) until the PID and CID > > information can be read. This allows us to delete all the amba specific > > deferring code and also avoid the arbitrary probing delays. > > > > Cc: Rob Herring > > Cc: Ulf Hansson > > Cc: John Stultz > > Cc: Saravana Kannan > > Cc: Linus Walleij > > Cc: Sudeep Holla > > Cc: Nicolas Saenz Julienne > > Cc: Geert Uytterhoeven > > Cc: Marek Szyprowski > > Cc: Russell King > > Signed-off-by: Saravana Kannan > > --- > > > > v1 -> v2: > > - Dropped RFC tag > > - Complete rewrite to not use stub devices. > > v2 -> v3: > > - Flipped the if() condition for hard-coded periphids. > > - Added a stub driver to handle the case where all amba drivers are > >modules loaded by uevents. > > - Cc Marek after I realized I forgot to add him. > > > > Marek, > > > > Would you mind testing this? It looks okay with my limited testing. > > It looks it works fine on my test systems. I've checked current > linux-next and this patch. You can add: > > Tested-by: Marek Szyprowski Hi Marek, Thanks! Does your test set up have amda drivers that are loaded based on uevents? That's the one I couldn't test. > I've briefly scanned the code and I'm curious how does it work. Does it > depend on the recently introduced "fw_devlink=on" feature? I don't see > other mechanism, which would trigger matching amba device if pm domains, > clocks or resets were not available on time to read pid/cid while adding > a device... No, it does not depend on fw_devlink or device links in any way. When a device is attempted to be probed (when it's added or during deferred probe), it's matched with all the drivers on the bus. When a new driver is registered to a bus, all devices in that bus are matched with the driver to see if they'll work together. That's how match is called. And match() can return -EPROBE_DEFER and that'll cause the device to be put in the deferred probe list by driver core. The tricky part in this patch was the uevent handling and the chicken-and-egg issue I talk about in the comments. Russell, Does this look good now? Plan to pick it up some time? Thanks, Saravana > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
Re: [PATCH v3] amba: Remove deferred device addition
Hi Saravana, On 04.03.2021 20:51, Saravana Kannan wrote: > The uevents generated for an amba device need PID and CID information > that's available only when the amba device is powered on, clocked and > out of reset. So, if those resources aren't available, the information > can't be read to generate the uevents. To workaround this requirement, > if the resources weren't available, the device addition was deferred and > retried periodically. > > However, this deferred addition retry isn't based on resources becoming > available. Instead, it's retried every 5 seconds and causes arbitrary > probe delays for amba devices and their consumers. > > Also, maintaining a separate deferred-probe like mechanism is > maintenance headache. > > With this commit, instead of deferring the device addition, we simply > defer the generation of uevents for the device and probing of the device > (because drivers needs PID and CID to match) until the PID and CID > information can be read. This allows us to delete all the amba specific > deferring code and also avoid the arbitrary probing delays. > > Cc: Rob Herring > Cc: Ulf Hansson > Cc: John Stultz > Cc: Saravana Kannan > Cc: Linus Walleij > Cc: Sudeep Holla > Cc: Nicolas Saenz Julienne > Cc: Geert Uytterhoeven > Cc: Marek Szyprowski > Cc: Russell King > Signed-off-by: Saravana Kannan > --- > > v1 -> v2: > - Dropped RFC tag > - Complete rewrite to not use stub devices. > v2 -> v3: > - Flipped the if() condition for hard-coded periphids. > - Added a stub driver to handle the case where all amba drivers are >modules loaded by uevents. > - Cc Marek after I realized I forgot to add him. > > Marek, > > Would you mind testing this? It looks okay with my limited testing. It looks it works fine on my test systems. I've checked current linux-next and this patch. You can add: Tested-by: Marek Szyprowski I've briefly scanned the code and I'm curious how does it work. Does it depend on the recently introduced "fw_devlink=on" feature? I don't see other mechanism, which would trigger matching amba device if pm domains, clocks or resets were not available on time to read pid/cid while adding a device... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
[PATCH v3] amba: Remove deferred device addition
The uevents generated for an amba device need PID and CID information
that's available only when the amba device is powered on, clocked and
out of reset. So, if those resources aren't available, the information
can't be read to generate the uevents. To workaround this requirement,
if the resources weren't available, the device addition was deferred and
retried periodically.
However, this deferred addition retry isn't based on resources becoming
available. Instead, it's retried every 5 seconds and causes arbitrary
probe delays for amba devices and their consumers.
Also, maintaining a separate deferred-probe like mechanism is
maintenance headache.
With this commit, instead of deferring the device addition, we simply
defer the generation of uevents for the device and probing of the device
(because drivers needs PID and CID to match) until the PID and CID
information can be read. This allows us to delete all the amba specific
deferring code and also avoid the arbitrary probing delays.
Cc: Rob Herring
Cc: Ulf Hansson
Cc: John Stultz
Cc: Saravana Kannan
Cc: Linus Walleij
Cc: Sudeep Holla
Cc: Nicolas Saenz Julienne
Cc: Geert Uytterhoeven
Cc: Marek Szyprowski
Cc: Russell King
Signed-off-by: Saravana Kannan
---
v1 -> v2:
- Dropped RFC tag
- Complete rewrite to not use stub devices.
v2 -> v3:
- Flipped the if() condition for hard-coded periphids.
- Added a stub driver to handle the case where all amba drivers are
modules loaded by uevents.
- Cc Marek after I realized I forgot to add him.
Marek,
Would you mind testing this? It looks okay with my limited testing.
-Saravana
drivers/amba/bus.c | 329 +
1 file changed, 151 insertions(+), 178 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 939ca220bf78..836d6d23bba3 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -149,11 +149,101 @@ static struct attribute *amba_dev_attrs[] = {
};
ATTRIBUTE_GROUPS(amba_dev);
+static int amba_read_periphid(struct amba_device *dev)
+{
+ u32 size;
+ void __iomem *tmp;
+ u32 pid, cid;
+ struct reset_control *rstc;
+ int i, ret;
+
+ /*
+* Dynamically calculate the size of the resource
+* and use this for iomap
+*/
+ size = resource_size(&dev->res);
+ tmp = ioremap(dev->res.start, size);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = dev_pm_domain_attach(&dev->dev, true);
+ if (ret)
+ goto err_pm;
+
+ ret = amba_get_enable_pclk(dev);
+ if (ret)
+ goto err_clk;
+
+ /*
+* Find reset control(s) of the amba bus and de-assert them.
+*/
+ rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
+ if (IS_ERR(rstc)) {
+ ret = PTR_ERR(rstc);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&dev->dev, "can't get reset: %d\n",
+ ret);
+ goto err_reset;
+ }
+ reset_control_deassert(rstc);
+ reset_control_put(rstc);
+
+ /*
+* Read pid and cid based on size of resource
+* they are located at end of region
+*/
+ for (pid = 0, i = 0; i < 4; i++)
+ pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) <<
+ (i * 8);
+ for (cid = 0, i = 0; i < 4; i++)
+ cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
+ (i * 8);
+
+ if (cid == CORESIGHT_CID) {
+ /* set the base to the start of the last 4k block */
+ void __iomem *csbase = tmp + size - 4096;
+
+ dev->uci.devarch =
+ readl(csbase + UCI_REG_DEVARCH_OFFSET);
+ dev->uci.devtype =
+ readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
+ }
+
+ amba_put_disable_pclk(dev);
+
+ if (cid == AMBA_CID || cid == CORESIGHT_CID) {
+ dev->periphid = pid;
+ dev->cid = cid;
+ }
+
+ if (!dev->periphid)
+ ret = -ENODEV;
+
+ return ret;
+
+err_reset:
+ amba_put_disable_pclk(dev);
+err_clk:
+ dev_pm_domain_detach(&dev->dev, true);
+err_pm:
+ iounmap(tmp);
+ return ret;
+}
+
static int amba_match(struct device *dev, struct device_driver *drv)
{
struct amba_device *pcdev = to_amba_device(dev);
struct amba_driver *pcdrv = to_amba_driver(drv);
+ if (!pcdev->periphid) {
+ int ret = amba_read_periphid(pcdev);
+
+ if (ret)
+ return ret;
+ dev_set_uevent_suppress(dev, false);
+ kobject_uevent(&dev->kobj, KOBJ_ADD);
+ }
+
/* When driver_override is set, only bind to the matching driver */
if (pcdev->driver_override)
return !strcmp(pcdev->driver_override, drv->name);
@@ -332,6 +422,42 @@ static int __init amba_init(v
