Re: [PATCH v2] amba: Remove deferred device addition
On Thu, Mar 4, 2021 at 6:12 AM Russell King - ARM Linux admin
wrote:
>
> On Wed, Mar 03, 2021 at 08:08:44PM -0800, Saravana Kannan wrote:
> > Marek,
> >
> > I tested it and saw the device get added before the resources were
> > available and the uevent file looked okay. Would you mind testing it
> > further?
>
> To put it bluntly, if you have tested this, the testing was not very
> effective. Deleting the lines that are removed by the patch so we can
> see what the new code looks like below:
>
> > > +int amba_device_add(struct amba_device *dev, struct resource *parent)
> > > {
> > > + int ret;
> > >
> > > WARN_ON(dev->irq[0] == (unsigned int)-1);
> > > WARN_ON(dev->irq[1] == (unsigned int)-1);
> > >
> > > ret = request_resource(parent, >res);
> > > if (ret)
> > > + return ret;
> > >
> > > + /* If primecell ID isn't hard-coded, figure it out */
> > > + if (dev->periphid) {
> > > + ret = amba_read_periphid(dev);
>
> So, if the peripheral ID has _already_ been set, we attempt to read the
> peripheral ID from the device. Isn't that just wrong?
>
> > > + if (ret && ret != -EPROBE_DEFER)
> > > + goto err_release;
> > > /*
> > > +* AMBA device uevents require reading its pid and cid
> > > +* registers. To do this, the device must be on, clocked
> > > and
> > > +* out of reset. However in some cases those resources
> > > might
> > > +* not yet be available. If that's the case, we suppress
> > > the
> > > +* generation of uevents until we can read the pid and cid
> > > +* registers. See also amba_match().
> > > */
> > > + if (ret)
> > > + dev_set_uevent_suppress(>dev, true);
> > > }
>
> If the peripheral ID has not been set, we don't attempt to read it, and
> we generate an add event when the amba device is added with a zero
> peripheral ID.
>
> I guess that if() statement should be negated - and with such an error,
> I fail to see how this code could have been properly tested.
Yeah, the if() needs to be flipped. I even flipped it and then
unflipped it before I sent the patch. Thanks for catching it.
It worked in my testing because the device didn't have hard coded PID.
So it worked out fine.
But I now realize I still have a chicken-and-egg problem if ALL amba
drivers are modules. amba_match() will never be called because none of
the amba drivers have been loaded. None of the amba drivers would be
loaded (depending on the set up) because none of the uevents were sent
out. But there's a simple fix for this. I'll send that as part of v3.
Marek,
It'd still be nice if you can test this with the if() above flipped.
If all your amba drivers are modules and loaded based on uevents,
manually loading one of them will kick off everything.
-Saravana
Re: [PATCH v2] amba: Remove deferred device addition
On Wed, Mar 03, 2021 at 08:08:44PM -0800, Saravana Kannan wrote:
> Marek,
>
> I tested it and saw the device get added before the resources were
> available and the uevent file looked okay. Would you mind testing it
> further?
To put it bluntly, if you have tested this, the testing was not very
effective. Deleting the lines that are removed by the patch so we can
see what the new code looks like below:
> > +int amba_device_add(struct amba_device *dev, struct resource *parent)
> > {
> > + int ret;
> >
> > WARN_ON(dev->irq[0] == (unsigned int)-1);
> > WARN_ON(dev->irq[1] == (unsigned int)-1);
> >
> > ret = request_resource(parent, >res);
> > if (ret)
> > + return ret;
> >
> > + /* If primecell ID isn't hard-coded, figure it out */
> > + if (dev->periphid) {
> > + ret = amba_read_periphid(dev);
So, if the peripheral ID has _already_ been set, we attempt to read the
peripheral ID from the device. Isn't that just wrong?
> > + if (ret && ret != -EPROBE_DEFER)
> > + goto err_release;
> > /*
> > +* AMBA device uevents require reading its pid and cid
> > +* registers. To do this, the device must be on, clocked
> > and
> > +* out of reset. However in some cases those resources
> > might
> > +* not yet be available. If that's the case, we suppress
> > the
> > +* generation of uevents until we can read the pid and cid
> > +* registers. See also amba_match().
> > */
> > + if (ret)
> > + dev_set_uevent_suppress(>dev, true);
> > }
If the peripheral ID has not been set, we don't attempt to read it, and
we generate an add event when the amba device is added with a zero
peripheral ID.
I guess that if() statement should be negated - and with such an error,
I fail to see how this code could have been properly tested.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2] amba: Remove deferred device addition
On Wed, Mar 3, 2021 at 8:00 PM 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: Russell King
> Signed-off-by: Saravana Kannan
> ---
> drivers/amba/bus.c | 293 ++---
> 1 file changed, 115 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 939ca220bf78..fac4110b2f58 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(>res);
> + tmp = ioremap(dev->res.start, size);
> + if (!tmp)
> + return -ENOMEM;
> +
> + ret = dev_pm_domain_attach(>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, "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, 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(>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);
> @@ -373,98 +463,43 @@ static void amba_device_release(struct device *dev)
> kfree(d);
> }
Heh... after sending this patch I went
Re: [PATCH v2] amba: Remove deferred device addition
On Wed, Mar 3, 2021 at 8:00 PM 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: Russell King
> Signed-off-by: Saravana Kannan
> ---
> drivers/amba/bus.c | 293 ++---
> 1 file changed, 115 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 939ca220bf78..fac4110b2f58 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(>res);
> + tmp = ioremap(dev->res.start, size);
> + if (!tmp)
> + return -ENOMEM;
> +
> + ret = dev_pm_domain_attach(>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, "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, 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(>kobj, KOBJ_ADD);
Greg,
This is the only "ugly" part of this patch. I hope this is not a
problem as this patch finally gets amba devices to behave normally and
deletes about 60 lines of code.
Marek,
I tested it and saw the device get added before the resources were
available and the uevent file looked okay. Would you mind testing it
further?
Thanks,
[PATCH v2] 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: Russell King
Signed-off-by: Saravana Kannan
---
drivers/amba/bus.c | 293 ++---
1 file changed, 115 insertions(+), 178 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 939ca220bf78..fac4110b2f58 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(>res);
+ tmp = ioremap(dev->res.start, size);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = dev_pm_domain_attach(>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, "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, 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(>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);
@@ -373,98 +463,43 @@ static void amba_device_release(struct device *dev)
kfree(d);
}
-static int amba_device_try_add(struct amba_device *dev, struct resource
*parent)
+/**
+ * amba_device_add - add a previously allocated AMBA device structure
+ * @dev: AMBA device allocated by amba_device_alloc
+ * @parent: resource parent for this devices resources
+ *
+ * Claim the resource, and read the device cell ID if not already
+ *
