The goal of this patch is to allow drivers to be probed even if at the time of
the DT parsing some of their ressources are not available yet.
In the current situation, the resource of a platform device are filled from the
DT at the time the device is created (of_device_alloc()). The drawbackof this
is that a device sitting close to the top of the DT (ahb for example) but
depending on ressources that are initialized later (IRQ domain dynamically
created for example) will fail to probe because the ressources don't exist
at this time.
This patch fills the resource structure only before the device is probed and
will defer the probe if the resource are not available yet.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Reviewed-by: Gregory CLEMENT <[email protected]>
---
drivers/base/platform.c | 6 ++++
drivers/of/platform.c | 71 +++++++++++++++++++++++++++++----------------
include/linux/of_platform.h | 10 +++++++
3 files changed, 62 insertions(+), 25 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848..8e37d8b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -481,6 +481,12 @@ static int platform_drv_probe(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret;
+ if (_dev->of_node) {
+ ret = of_platform_device_populate_resources(dev);
+ if (ret < 0)
+ return drv->prevent_deferred_probe ? ret : -EPROBE_DEFER;
+ }
+
if (ACPI_HANDLE(_dev))
acpi_dev_pm_attach(_dev, true);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..64a8eb8 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
struct device *parent)
{
struct platform_device *dev;
- int rc, i, num_reg = 0, num_irq;
- struct resource *res, temp_res;
dev = platform_device_alloc("", -1);
if (!dev)
return NULL;
- /* count the io and irq resources */
- if (of_can_translate_address(np))
- while (of_address_to_resource(np, num_reg, &temp_res) == 0)
- num_reg++;
- num_irq = of_irq_count(np);
-
- /* Populate the resource table */
- if (num_irq || num_reg) {
- res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
- if (!res) {
- platform_device_put(dev);
- return NULL;
- }
-
- dev->num_resources = num_reg + num_irq;
- dev->resource = res;
- for (i = 0; i < num_reg; i++, res++) {
- rc = of_address_to_resource(np, i, res);
- WARN_ON(rc);
- }
- WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
- }
-
dev->dev.of_node = of_node_get(np);
#if defined(CONFIG_MICROBLAZE)
dev->dev.dma_mask = &dev->archdata.dma_mask;
@@ -233,6 +208,52 @@ static struct platform_device *of_platform_device_create_pdata(
return dev;
}
+int of_platform_device_populate_resources(struct platform_device *dev)
+{
+ struct device_node *np;
+ int rc = 0, i, nreg = 0, nirq;
+
+ np = dev->dev.of_node;
+
+ /* count the io and irq resources */
+ if (of_can_translate_address(np)) {
+ struct resource temp_res;
+ while (of_address_to_resource(np, nreg, &temp_res) == 0)
+ nreg++;
+ }
+ nirq = of_irq_count(np);
+
+ /* Populate the resource table */
+ if (nirq || nreg) {
+ struct resource *res;
+
+ res = krealloc(dev->resource, sizeof(*res) * (nirq + nreg),
+ GFP_KERNEL);
+ if (!res) {
+ kfree(dev->resource);
+ dev->resource = NULL;
+ return -ENOMEM;
+ }
+ memset(res, 0, sizeof(*res) * (nirq + nreg));
+ dev->resource = res;
+ dev->num_resources = nreg + nirq;
+
+ for (i = 0; i < nreg; i++, res++) {
+ rc = of_address_to_resource(np, i, res);
+ WARN_ON(rc);
+ if (rc)
+ break;
+ }
+
+ if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
+ rc = -ENOENT;
+ WARN_ON(rc);
+ }
+ }
+ return rc;
+}
+EXPORT_SYMBOL(of_platform_device_populate_resources);
+
/**
* of_platform_device_create - Alloc, initialize and register an of_device
* @np: pointer to node to create device for
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..315e1e3 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -53,6 +53,16 @@ struct of_dev_auxdata {
extern const struct of_device_id of_default_bus_match_table[];
+/* Populate the resource for a platform device */
+#ifdef CONFIG_OF
+int of_platform_device_populate_resources(struct platform_device *dev);
+#else
+static inline int of_platform_device_populate_resources(
+ struct platform_device *)
+{
+ return -ENOSYS;
+}
+#endif
/* Platform drivers register/unregister */
extern struct platform_device *of_device_alloc(struct device_node *np,
const char *bus_id,
--
1.8.5.3
Hi all,
I forgot to add the link to the discussion that lead to this patch:
https://lkml.org/lkml/2014/2/12/306.
Jean-Jacques
2014-02-13 10:57 GMT+01:00 Jean-Jacques Hiblot <[email protected]>:
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their ressources are not available yet.
>
> In the current situation, the resource of a platform device are filled from the
> DT at the time the device is created (of_device_alloc()). The drawbackof this
> is that a device sitting close to the top of the DT (ahb for example) but
> depending on ressources that are initialized later (IRQ domain dynamically
> created for example) will fail to probe because the ressources don't exist
> at this time.
>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the resource are not available yet.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Reviewed-by: Gregory CLEMENT <[email protected]>
> ---
> drivers/base/platform.c | 6 ++++
> drivers/of/platform.c | 71 +++++++++++++++++++++++++++++----------------
> include/linux/of_platform.h | 10 +++++++
> 3 files changed, 62 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index bc78848..8e37d8b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,12 @@ static int platform_drv_probe(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret;
>
> + if (_dev->of_node) {
> + ret = of_platform_device_populate_resources(dev);
> + if (ret < 0)
> + return drv->prevent_deferred_probe ? ret : -EPROBE_DEFER;
> + }
> +
> if (ACPI_HANDLE(_dev))
> acpi_dev_pm_attach(_dev, true);
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..64a8eb8 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
> struct device *parent)
> {
> struct platform_device *dev;
> - int rc, i, num_reg = 0, num_irq;
> - struct resource *res, temp_res;
>
> dev = platform_device_alloc("", -1);
> if (!dev)
> return NULL;
>
> - /* count the io and irq resources */
> - if (of_can_translate_address(np))
> - while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> - num_reg++;
> - num_irq = of_irq_count(np);
> -
> - /* Populate the resource table */
> - if (num_irq || num_reg) {
> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> - if (!res) {
> - platform_device_put(dev);
> - return NULL;
> - }
> -
> - dev->num_resources = num_reg + num_irq;
> - dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> - WARN_ON(rc);
> - }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> - }
> -
> dev->dev.of_node = of_node_get(np);
> #if defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> @@ -233,6 +208,52 @@ static struct platform_device *of_platform_device_create_pdata(
> return dev;
> }
>
> +int of_platform_device_populate_resources(struct platform_device *dev)
> +{
> + struct device_node *np;
> + int rc = 0, i, nreg = 0, nirq;
> +
> + np = dev->dev.of_node;
> +
> + /* count the io and irq resources */
> + if (of_can_translate_address(np)) {
> + struct resource temp_res;
> + while (of_address_to_resource(np, nreg, &temp_res) == 0)
> + nreg++;
> + }
> + nirq = of_irq_count(np);
> +
> + /* Populate the resource table */
> + if (nirq || nreg) {
> + struct resource *res;
> +
> + res = krealloc(dev->resource, sizeof(*res) * (nirq + nreg),
> + GFP_KERNEL);
> + if (!res) {
> + kfree(dev->resource);
> + dev->resource = NULL;
> + return -ENOMEM;
> + }
> + memset(res, 0, sizeof(*res) * (nirq + nreg));
> + dev->resource = res;
> + dev->num_resources = nreg + nirq;
> +
> + for (i = 0; i < nreg; i++, res++) {
> + rc = of_address_to_resource(np, i, res);
> + WARN_ON(rc);
> + if (rc)
> + break;
> + }
> +
> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
> + rc = -ENOENT;
> + WARN_ON(rc);
> + }
> + }
> + return rc;
> +}
> +EXPORT_SYMBOL(of_platform_device_populate_resources);
> +
> /**
> * of_platform_device_create - Alloc, initialize and register an of_device
> * @np: pointer to node to create device for
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..315e1e3 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -53,6 +53,16 @@ struct of_dev_auxdata {
>
> extern const struct of_device_id of_default_bus_match_table[];
>
> +/* Populate the resource for a platform device */
> +#ifdef CONFIG_OF
> +int of_platform_device_populate_resources(struct platform_device *dev);
> +#else
> +static inline int of_platform_device_populate_resources(
> + struct platform_device *)
> +{
> + return -ENOSYS;
> +}
> +#endif
> /* Platform drivers register/unregister */
> extern struct platform_device *of_device_alloc(struct device_node *np,
> const char *bus_id,
> --
> 1.8.5.3
>
On Thu, Feb 13, 2014 at 10:57:09AM +0100, Jean-Jacques Hiblot wrote:
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their ressources are not available yet.
>
> In the current situation, the resource of a platform device are filled from the
> DT at the time the device is created (of_device_alloc()). The drawbackof this
> is that a device sitting close to the top of the DT (ahb for example) but
> depending on ressources that are initialized later (IRQ domain dynamically
> created for example) will fail to probe because the ressources don't exist
> at this time.
>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the resource are not available yet.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Reviewed-by: Gregory CLEMENT <[email protected]>
> ---
> drivers/base/platform.c | 6 ++++
> drivers/of/platform.c | 71 +++++++++++++++++++++++++++++----------------
> include/linux/of_platform.h | 10 +++++++
> 3 files changed, 62 insertions(+), 25 deletions(-)
I need some others to ack this before I can take it...
On Tue, 18 Feb 2014 12:22:05 -0800, Greg KH <[email protected]> wrote:
> On Thu, Feb 13, 2014 at 10:57:09AM +0100, Jean-Jacques Hiblot wrote:
> > The goal of this patch is to allow drivers to be probed even if at the time of
> > the DT parsing some of their ressources are not available yet.
> >
> > In the current situation, the resource of a platform device are filled from the
> > DT at the time the device is created (of_device_alloc()). The drawbackof this
> > is that a device sitting close to the top of the DT (ahb for example) but
> > depending on ressources that are initialized later (IRQ domain dynamically
> > created for example) will fail to probe because the ressources don't exist
> > at this time.
> >
> > This patch fills the resource structure only before the device is probed and
> > will defer the probe if the resource are not available yet.
> >
> > Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> > Reviewed-by: Gregory CLEMENT <[email protected]>
> > ---
> > drivers/base/platform.c | 6 ++++
> > drivers/of/platform.c | 71 +++++++++++++++++++++++++++++----------------
> > include/linux/of_platform.h | 10 +++++++
> > 3 files changed, 62 insertions(+), 25 deletions(-)
>
> I need some others to ack this before I can take it...
Yes, please let me review it first, and I'll probably want to take it
through the devicetree branch.
g.
On Thu, 13 Feb 2014 10:57:09 +0100, Jean-Jacques Hiblot <[email protected]> wrote:
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their ressources are not available yet.
>
> In the current situation, the resource of a platform device are filled from the
> DT at the time the device is created (of_device_alloc()). The drawbackof this
> is that a device sitting close to the top of the DT (ahb for example) but
> depending on ressources that are initialized later (IRQ domain dynamically
> created for example) will fail to probe because the ressources don't exist
> at this time.
>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the resource are not available yet.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Reviewed-by: Gregory CLEMENT <[email protected]>
Hi Jean-Jacques. Thanks for looking at this, it is a longstanding
problem. However, I'm leary of this approach because it makes failed
probes (which are intended to be cheap) into an expensive operation. If
a device goes through multiple rounds of deferred probe, then every time
it will attempt to decode the needed resources. Parsing 'reg' isn't too
bad, but parsing the interrupts may not be. I think it needs to be more
nuanced and only recalculate the resources that failed in previous
attempts...
...however, the other argument that correctness is more important than
efficiency here and it would be better to completely teardown the
resources table, or at least the interrupt entries, after a failed
probe or driver removal to handle the case where the interrupt
controller is re-bound to its driver and gets a new set of interrupt
numbers...
I think this patch can be reworked into something better though...
> ---
> drivers/base/platform.c | 6 ++++
> drivers/of/platform.c | 71 +++++++++++++++++++++++++++++----------------
> include/linux/of_platform.h | 10 +++++++
> 3 files changed, 62 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index bc78848..8e37d8b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,12 @@ static int platform_drv_probe(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret;
>
> + if (_dev->of_node) {
> + ret = of_platform_device_populate_resources(dev);
> + if (ret < 0)
> + return drv->prevent_deferred_probe ? ret : -EPROBE_DEFER;
> + }
> +
Get rid of the _dev->of_node() test. It should be embedded in the
function. Also, rename it to: of_platform_device_prepare()
> if (ACPI_HANDLE(_dev))
> acpi_dev_pm_attach(_dev, true);
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..64a8eb8 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
> struct device *parent)
> {
> struct platform_device *dev;
> - int rc, i, num_reg = 0, num_irq;
> - struct resource *res, temp_res;
>
> dev = platform_device_alloc("", -1);
> if (!dev)
> return NULL;
>
> - /* count the io and irq resources */
> - if (of_can_translate_address(np))
> - while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> - num_reg++;
> - num_irq = of_irq_count(np);
> -
> - /* Populate the resource table */
> - if (num_irq || num_reg) {
> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> - if (!res) {
> - platform_device_put(dev);
> - return NULL;
> - }
> -
> - dev->num_resources = num_reg + num_irq;
> - dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> - WARN_ON(rc);
> - }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> - }
> -
> dev->dev.of_node = of_node_get(np);
> #if defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> @@ -233,6 +208,52 @@ static struct platform_device *of_platform_device_create_pdata(
> return dev;
> }
>
> +int of_platform_device_populate_resources(struct platform_device *dev)
> +{
> + struct device_node *np;
> + int rc = 0, i, nreg = 0, nirq;
> +
> + np = dev->dev.of_node;
> +
> + /* count the io and irq resources */
> + if (of_can_translate_address(np)) {
> + struct resource temp_res;
> + while (of_address_to_resource(np, nreg, &temp_res) == 0)
> + nreg++;
> + }
The above snippet should be encapsulated in an of_reg_count() helper function.
> + nirq = of_irq_count(np);
> +
> + /* Populate the resource table */
> + if (nirq || nreg) {
> + struct resource *res;
> +
> + res = krealloc(dev->resource, sizeof(*res) * (nirq + nreg),
> + GFP_KERNEL);
> + if (!res) {
> + kfree(dev->resource);
> + dev->resource = NULL;
> + return -ENOMEM;
> + }
> + memset(res, 0, sizeof(*res) * (nirq + nreg));
> + dev->resource = res;
> + dev->num_resources = nreg + nirq;
> +
> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
> + rc = -ENOENT;
> + WARN_ON(rc);
> + }
> + }
> + return rc;
> +}
Reallocation is not necessary. We can know exactly how many tuples are
in the reg and interrupts properties. Once allocated the array will be
the right size. (may need to fix of_irq_count() though).
Also, the reg resources will always be correct. They don't need to be
recalculated each time through. IRQs however are the problem area. A
simplistic implementation which is still better than current mainline
would be the following:
if (!nirq && !nreg)
return 0;
if (!dev->resource) {
res = kzalloc(dev->resource, sizeof(*res) * (nirq + nreg),
GFP_KERNEL);
if (!res)
return -ENOMEM;
for (i = 0; i < nreg; i++, res++) {
rc = of_address_to_resource(np, i, res);
if (WARN_ON(rc)) {
/* THIS IS BAD; don't try to defer probing */
kfree(res);
return rc;
}
}
dev->resource = res;
dev->num_resources = nreg + nirq;
if (of_irq_to_resource_table(np, dev->resource, nirq) != nirq)
return -EPROBE_DEFER;
return 0;
}
/* See which IRQ resources need to be redone */
for (i = 0, res = &dev->resource[nreg]; i < nirq; i++, res++)
if (!res->flags && !of_irq_to_resource(np, i, res))
return -EPROBE_DEFER;
return 0;
It isn't optimal because it cannot handle irq controllers being
replugged, but still better than the current code.
Can you respin and let me know if the above works for you?
Thanks,
g.
> +EXPORT_SYMBOL(of_platform_device_populate_resources);
> +
> /**
> * of_platform_device_create - Alloc, initialize and register an of_device
> * @np: pointer to node to create device for
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..315e1e3 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -53,6 +53,16 @@ struct of_dev_auxdata {
>
> extern const struct of_device_id of_default_bus_match_table[];
>
> +/* Populate the resource for a platform device */
> +#ifdef CONFIG_OF
> +int of_platform_device_populate_resources(struct platform_device *dev);
> +#else
> +static inline int of_platform_device_populate_resources(
> + struct platform_device *)
> +{
> + return -ENOSYS;
> +}
> +#endif
> /* Platform drivers register/unregister */
> extern struct platform_device *of_device_alloc(struct device_node *np,
> const char *bus_id,
> --
> 1.8.5.3
>
The goal of this patch is to allow drivers to be probed even if at the time of
the DT parsing some of their ressources are not available yet.
In the current situation, the resource of a platform device are filled from the
DT at the time the device is created (of_device_alloc()). The drawbackof this
is that a device sitting close to the top of the DT (ahb for example) but
depending on ressources that are initialized later (IRQ domain dynamically
created for example) will fail to probe because the ressources don't exist
at this time.
This patch fills the resource structure only before the device is probed and
will defer the probe if the resource are not available yet.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Reviewed-by: Gregory CLEMENT <[email protected]>
---
Hi Grant,
I reworked the patch as you proposed. To keep the overhead minimum, nirq and
nreg are computed only the first time.
In this implementation, only the missing IRQ ressources are re-tried for. It could
easily be changed to re-parse all the IRQs though (replace if (!res->flags)
with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)).
drivers/base/platform.c | 5 +++
drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++-----------
include/linux/of_platform.h | 10 +++++
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848..cee9b8d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret;
+ ret = of_platform_device_prepare(dev);
+ if (ret)
+ goto error;
+
if (ACPI_HANDLE(_dev))
acpi_dev_pm_attach(_dev, true);
@@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev)
if (ret && ACPI_HANDLE(_dev))
acpi_dev_pm_detach(_dev, true);
+error:
if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
dev_warn(_dev, "probe deferral not supported\n");
ret = -ENXIO;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..a4e2602 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
struct device *parent)
{
struct platform_device *dev;
- int rc, i, num_reg = 0, num_irq;
- struct resource *res, temp_res;
dev = platform_device_alloc("", -1);
if (!dev)
return NULL;
- /* count the io and irq resources */
- if (of_can_translate_address(np))
- while (of_address_to_resource(np, num_reg, &temp_res) == 0)
- num_reg++;
- num_irq = of_irq_count(np);
-
- /* Populate the resource table */
- if (num_irq || num_reg) {
- res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
- if (!res) {
- platform_device_put(dev);
- return NULL;
- }
-
- dev->num_resources = num_reg + num_irq;
- dev->resource = res;
- for (i = 0; i < num_reg; i++, res++) {
- rc = of_address_to_resource(np, i, res);
- WARN_ON(rc);
- }
- WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
- }
-
dev->dev.of_node = of_node_get(np);
#if defined(CONFIG_MICROBLAZE)
dev->dev.dma_mask = &dev->archdata.dma_mask;
@@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata(
return dev;
}
+static int of_reg_count(struct device_node *np)
+{
+ int nreg = 0;
+ if (of_can_translate_address(np)) {
+ struct resource temp_res;
+ while (of_address_to_resource(np, nreg, &temp_res) == 0)
+ nreg++;
+ }
+ return nreg;
+}
+
+int of_platform_device_prepare(struct platform_device *dev)
+{
+ struct device_node *np;
+ int i, irq_index;
+ struct resource *res;
+
+ /*
+ * This function applies only devices described in the DT.
+ * Other platform devices have their ressources already populated.
+ */
+ np = dev->dev.of_node;
+ if (!np)
+ return 0;
+
+ /* Populate the resource table */
+ if (!dev->resource) {
+ int rc, nreg = 0, nirq;
+ /* count the io and irq resources */
+ nreg = of_reg_count(np);
+ nirq = of_irq_count(np);
+
+ if (!nirq && !nreg)
+ return 0;
+
+ res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ dev->resource = res;
+ dev->num_resources = nreg + nirq;
+
+ for (i = 0; i < nreg; i++, res++) {
+ rc = of_address_to_resource(np, i, res);
+ if (WARN_ON(rc)) {
+ /* THIS IS BAD; don't try to defer probing */
+ dev->num_resources = 0;
+ dev->resource = NULL;
+ kfree(res);
+ return rc;
+ }
+ }
+
+ if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
+ /* IRQ controller is yet available. defer probing */
+ return -EPROBE_DEFER;
+ }
+
+ return 0;
+ }
+
+ /* See which IRQ resources need to be redone */
+ irq_index = 0;
+ for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
+ if (!res->flags) {
+ if (!of_irq_to_resource(np, irq_index, res))
+ return -EPROBE_DEFER;
+ irq_index++;
+ } else if (res->flags & IORESOURCE_IRQ)
+ irq_index++;
+ }
+ return 0;
+}
+EXPORT_SYMBOL(of_platform_device_prepare);
+
/**
* of_platform_device_create - Alloc, initialize and register an of_device
* @np: pointer to node to create device for
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..4e487ff 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -53,6 +53,16 @@ struct of_dev_auxdata {
extern const struct of_device_id of_default_bus_match_table[];
+/* Populate the resource for a platform device */
+#ifdef CONFIG_OF
+int of_platform_device_prepare(struct platform_device *dev);
+#else
+static inline int of_platform_device_prepare(
+ struct platform_device *dev)
+{
+ return 0;
+}
+#endif
/* Platform drivers register/unregister */
extern struct platform_device *of_device_alloc(struct device_node *np,
const char *bus_id,
--
1.9.0
Hi Jean-Jacques,
Sorry for top posting.
As I know, there have been several attempts to solve the same problem already:)
[1] https://lkml.org/lkml/2013/9/18/216
[2] https://lkml.org/lkml/2013/11/22/520
[3] https://lkml.org/lkml/2014/1/8/240
There are some questions related to your approach:
1) How to distinguish between cases "IRQ domain not ready" and "wrong IRQ data in DT" or other IRQ parsing errors?
Otherwise, Driver's probe will be deffered wrongly and forever,
Thierry Reding has tried to solve this in [1].
2) How will be handled driver reloading situation?
The worst case (sparse IRQ enabled):
- remove driver A
- remove driver B (irq-controller)
- load driver B <--- different set of Linux IRQ numbers can be assigned
- load driver A <--- oops. IRQ resources table contains invalid data
Best regards,
Grygorii Strashko
=============================================
The goal of this patch is to allow drivers to be probed even if at the time of
the DT parsing some of their ressources are not available yet.
In the current situation, the resource of a platform device are filled from the
DT at the time the device is created (of_device_alloc()). The drawbackof this
is that a device sitting close to the top of the DT (ahb for example) but
depending on ressources that are initialized later (IRQ domain dynamically
created for example) will fail to probe because the ressources don't exist
at this time.
This patch fills the resource structure only before the device is probed and
will defer the probe if the resource are not available yet.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Reviewed-by: Gregory CLEMENT <[email protected]>
---
Hi Grant,
I reworked the patch as you proposed. To keep the overhead minimum, nirq and
nreg are computed only the first time.
In this implementation, only the missing IRQ ressources are re-tried for. It could
easily be changed to re-parse all the IRQs though (replace if (!res->flags)
with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)).
drivers/base/platform.c | 5 +++
drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++-----------
include/linux/of_platform.h | 10 +++++
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848..cee9b8d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret;
+ ret = of_platform_device_prepare(dev);
+ if (ret)
+ goto error;
+
if (ACPI_HANDLE(_dev))
acpi_dev_pm_attach(_dev, true);
@@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev)
if (ret && ACPI_HANDLE(_dev))
acpi_dev_pm_detach(_dev, true);
+error:
if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
dev_warn(_dev, "probe deferral not supported\n");
ret = -ENXIO;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..a4e2602 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
struct device *parent)
{
struct platform_device *dev;
- int rc, i, num_reg = 0, num_irq;
- struct resource *res, temp_res;
dev = platform_device_alloc("", -1);
if (!dev)
return NULL;
- /* count the io and irq resources */
- if (of_can_translate_address(np))
- while (of_address_to_resource(np, num_reg, &temp_res) == 0)
- num_reg++;
- num_irq = of_irq_count(np);
-
- /* Populate the resource table */
- if (num_irq || num_reg) {
- res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
- if (!res) {
- platform_device_put(dev);
- return NULL;
- }
-
- dev->num_resources = num_reg + num_irq;
- dev->resource = res;
- for (i = 0; i < num_reg; i++, res++) {
- rc = of_address_to_resource(np, i, res);
- WARN_ON(rc);
- }
- WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
- }
-
dev->dev.of_node = of_node_get(np);
#if defined(CONFIG_MICROBLAZE)
dev->dev.dma_mask = &dev->archdata.dma_mask;
@@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata(
return dev;
}
+static int of_reg_count(struct device_node *np)
+{
+ int nreg = 0;
+ if (of_can_translate_address(np)) {
+ struct resource temp_res;
+ while (of_address_to_resource(np, nreg, &temp_res) == 0)
+ nreg++;
+ }
+ return nreg;
+}
+
+int of_platform_device_prepare(struct platform_device *dev)
+{
+ struct device_node *np;
+ int i, irq_index;
+ struct resource *res;
+
+ /*
+ * This function applies only devices described in the DT.
+ * Other platform devices have their ressources already populated.
+ */
+ np = dev->dev.of_node;
+ if (!np)
+ return 0;
+
+ /* Populate the resource table */
+ if (!dev->resource) {
+ int rc, nreg = 0, nirq;
+ /* count the io and irq resources */
+ nreg = of_reg_count(np);
+ nirq = of_irq_count(np);
+
+ if (!nirq && !nreg)
+ return 0;
+
+ res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ dev->resource = res;
+ dev->num_resources = nreg + nirq;
+
+ for (i = 0; i < nreg; i++, res++) {
+ rc = of_address_to_resource(np, i, res);
+ if (WARN_ON(rc)) {
+ /* THIS IS BAD; don't try to defer probing */
+ dev->num_resources = 0;
+ dev->resource = NULL;
+ kfree(res);
+ return rc;
+ }
+ }
+
+ if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
+ /* IRQ controller is yet available. defer probing */
+ return -EPROBE_DEFER;
+ }
+
+ return 0;
+ }
+
+ /* See which IRQ resources need to be redone */
+ irq_index = 0;
+ for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
+ if (!res->flags) {
+ if (!of_irq_to_resource(np, irq_index, res))
+ return -EPROBE_DEFER;
+ irq_index++;
+ } else if (res->flags & IORESOURCE_IRQ)
+ irq_index++;
+ }
+ return 0;
+}
+EXPORT_SYMBOL(of_platform_device_prepare);
+
/**
* of_platform_device_create - Alloc, initialize and register an of_device
* @np: pointer to node to create device for
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..4e487ff 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -53,6 +53,16 @@ struct of_dev_auxdata {
extern const struct of_device_id of_default_bus_match_table[];
+/* Populate the resource for a platform device */
+#ifdef CONFIG_OF
+int of_platform_device_prepare(struct platform_device *dev);
+#else
+static inline int of_platform_device_prepare(
+ struct platform_device *dev)
+{
+ return 0;
+}
+#endif
/* Platform drivers register/unregister */
extern struct platform_device *of_device_alloc(struct device_node *np,
const char *bus_id,
--
1.9.0
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Grygorii,
2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <[email protected]>:
> Hi Jean-Jacques,
>
> Sorry for top posting.
>
> As I know, there have been several attempts to solve the same problem already:)
> [1] https://lkml.org/lkml/2013/9/18/216
> [2] https://lkml.org/lkml/2013/11/22/520
> [3] https://lkml.org/lkml/2014/1/8/240
>
> There are some questions related to your approach:
> 1) How to distinguish between cases "IRQ domain not ready" and "wrong IRQ data in DT" or other IRQ parsing errors?
> Otherwise, Driver's probe will be deffered wrongly and forever,
> Thierry Reding has tried to solve this in [1].
This approach doesn't really care about the cause of the problem. I'm
of the opinion that never-ending deferred probing is not a big issue,
being not triggered so often after start-up (only when a new device is
probed). But if we need to make it right, then we would have to change
a bit the API of irq_create_of_mapping() and irq_of_parse_and_map()
(or maybe duplicate this one to keep the patch small) to return a real
error code instead a simple 0. Then would should be able to
distinguish the different error causes.
>
> 2) How will be handled driver reloading situation?
> The worst case (sparse IRQ enabled):
> - remove driver A
> - remove driver B (irq-controller)
> - load driver B <--- different set of Linux IRQ numbers can be assigned
> - load driver A <--- oops. IRQ resources table contains invalid data
>
It's not handled in the current implementation. But if all interrupts
entries are re-parsed (see my comment for Grant), it should be all
right.
Another problem would appear if the DT is dynamically updated and the
number of resource is changed. In the 1st version of the patch, this
was handled but it made the function more expensive.
Jean-Jacques
>
>
> Best regards,
> Grygorii Strashko
>
> =============================================
>
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their ressources are not available yet.
>
> In the current situation, the resource of a platform device are filled from the
> DT at the time the device is created (of_device_alloc()). The drawbackof this
> is that a device sitting close to the top of the DT (ahb for example) but
> depending on ressources that are initialized later (IRQ domain dynamically
> created for example) will fail to probe because the ressources don't exist
> at this time.
>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the resource are not available yet.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Reviewed-by: Gregory CLEMENT <[email protected]>
> ---
>
> Hi Grant,
>
> I reworked the patch as you proposed. To keep the overhead minimum, nirq and
> nreg are computed only the first time.
> In this implementation, only the missing IRQ ressources are re-tried for. It could
> easily be changed to re-parse all the IRQs though (replace if (!res->flags)
> with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)).
>
> drivers/base/platform.c | 5 +++
> drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++-----------
> include/linux/of_platform.h | 10 +++++
> 3 files changed, 90 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index bc78848..cee9b8d 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret;
>
> + ret = of_platform_device_prepare(dev);
> + if (ret)
> + goto error;
> +
> if (ACPI_HANDLE(_dev))
> acpi_dev_pm_attach(_dev, true);
>
> @@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev)
> if (ret && ACPI_HANDLE(_dev))
> acpi_dev_pm_detach(_dev, true);
>
> +error:
> if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> dev_warn(_dev, "probe deferral not supported\n");
> ret = -ENXIO;
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..a4e2602 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
> struct device *parent)
> {
> struct platform_device *dev;
> - int rc, i, num_reg = 0, num_irq;
> - struct resource *res, temp_res;
>
> dev = platform_device_alloc("", -1);
> if (!dev)
> return NULL;
>
> - /* count the io and irq resources */
> - if (of_can_translate_address(np))
> - while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> - num_reg++;
> - num_irq = of_irq_count(np);
> -
> - /* Populate the resource table */
> - if (num_irq || num_reg) {
> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> - if (!res) {
> - platform_device_put(dev);
> - return NULL;
> - }
> -
> - dev->num_resources = num_reg + num_irq;
> - dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> - WARN_ON(rc);
> - }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> - }
> -
> dev->dev.of_node = of_node_get(np);
> #if defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> @@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata(
> return dev;
> }
>
> +static int of_reg_count(struct device_node *np)
> +{
> + int nreg = 0;
> + if (of_can_translate_address(np)) {
> + struct resource temp_res;
> + while (of_address_to_resource(np, nreg, &temp_res) == 0)
> + nreg++;
> + }
> + return nreg;
> +}
> +
> +int of_platform_device_prepare(struct platform_device *dev)
> +{
> + struct device_node *np;
> + int i, irq_index;
> + struct resource *res;
> +
> + /*
> + * This function applies only devices described in the DT.
> + * Other platform devices have their ressources already populated.
> + */
> + np = dev->dev.of_node;
> + if (!np)
> + return 0;
> +
> + /* Populate the resource table */
> + if (!dev->resource) {
> + int rc, nreg = 0, nirq;
> + /* count the io and irq resources */
> + nreg = of_reg_count(np);
> + nirq = of_irq_count(np);
> +
> + if (!nirq && !nreg)
> + return 0;
> +
> + res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + dev->resource = res;
> + dev->num_resources = nreg + nirq;
> +
> + for (i = 0; i < nreg; i++, res++) {
> + rc = of_address_to_resource(np, i, res);
> + if (WARN_ON(rc)) {
> + /* THIS IS BAD; don't try to defer probing */
> + dev->num_resources = 0;
> + dev->resource = NULL;
> + kfree(res);
> + return rc;
> + }
> + }
> +
> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
> + /* IRQ controller is yet available. defer probing */
> + return -EPROBE_DEFER;
> + }
> +
> + return 0;
> + }
> +
> + /* See which IRQ resources need to be redone */
> + irq_index = 0;
> + for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
> + if (!res->flags) {
> + if (!of_irq_to_resource(np, irq_index, res))
> + return -EPROBE_DEFER;
> + irq_index++;
> + } else if (res->flags & IORESOURCE_IRQ)
> + irq_index++;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(of_platform_device_prepare);
> +
> /**
> * of_platform_device_create - Alloc, initialize and register an of_device
> * @np: pointer to node to create device for
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..4e487ff 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -53,6 +53,16 @@ struct of_dev_auxdata {
>
> extern const struct of_device_id of_default_bus_match_table[];
>
> +/* Populate the resource for a platform device */
> +#ifdef CONFIG_OF
> +int of_platform_device_prepare(struct platform_device *dev);
> +#else
> +static inline int of_platform_device_prepare(
> + struct platform_device *dev)
> +{
> + return 0;
> +}
> +#endif
> /* Platform drivers register/unregister */
> extern struct platform_device *of_device_alloc(struct device_node *np,
> const char *bus_id,
> --
> 1.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 21, 2014 at 02:18:40PM +0100, Jean-Jacques Hiblot wrote:
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their ressources are not available yet.
>
> In the current situation, the resource of a platform device are filled from the
> DT at the time the device is created (of_device_alloc()). The drawbackof this
> is that a device sitting close to the top of the DT (ahb for example) but
> depending on ressources that are initialized later (IRQ domain dynamically
> created for example) will fail to probe because the ressources don't exist
> at this time.
>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the resource are not available yet.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Reviewed-by: Gregory CLEMENT <[email protected]>
Tested-by: Ludovic Desroches <[email protected]>
I wanted to add support for the external ethernet controller on
at91sam9n12ek board and I was facing an issue because the irq domain
doesn't exist yet (gpio controller as parent interrupt).
This patch solves this trouble.
> ---
>
> Hi Grant,
>
> I reworked the patch as you proposed. To keep the overhead minimum, nirq and
> nreg are computed only the first time.
> In this implementation, only the missing IRQ ressources are re-tried for. It could
> easily be changed to re-parse all the IRQs though (replace if (!res->flags)
> with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)).
>
> drivers/base/platform.c | 5 +++
> drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++-----------
> include/linux/of_platform.h | 10 +++++
> 3 files changed, 90 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index bc78848..cee9b8d 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret;
>
> + ret = of_platform_device_prepare(dev);
> + if (ret)
> + goto error;
> +
> if (ACPI_HANDLE(_dev))
> acpi_dev_pm_attach(_dev, true);
>
> @@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev)
> if (ret && ACPI_HANDLE(_dev))
> acpi_dev_pm_detach(_dev, true);
>
> +error:
> if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> dev_warn(_dev, "probe deferral not supported\n");
> ret = -ENXIO;
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..a4e2602 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
> struct device *parent)
> {
> struct platform_device *dev;
> - int rc, i, num_reg = 0, num_irq;
> - struct resource *res, temp_res;
>
> dev = platform_device_alloc("", -1);
> if (!dev)
> return NULL;
>
> - /* count the io and irq resources */
> - if (of_can_translate_address(np))
> - while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> - num_reg++;
> - num_irq = of_irq_count(np);
> -
> - /* Populate the resource table */
> - if (num_irq || num_reg) {
> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> - if (!res) {
> - platform_device_put(dev);
> - return NULL;
> - }
> -
> - dev->num_resources = num_reg + num_irq;
> - dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> - WARN_ON(rc);
> - }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> - }
> -
> dev->dev.of_node = of_node_get(np);
> #if defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> @@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata(
> return dev;
> }
>
> +static int of_reg_count(struct device_node *np)
> +{
> + int nreg = 0;
> + if (of_can_translate_address(np)) {
> + struct resource temp_res;
> + while (of_address_to_resource(np, nreg, &temp_res) == 0)
> + nreg++;
> + }
> + return nreg;
> +}
> +
> +int of_platform_device_prepare(struct platform_device *dev)
> +{
> + struct device_node *np;
> + int i, irq_index;
> + struct resource *res;
> +
> + /*
> + * This function applies only devices described in the DT.
> + * Other platform devices have their ressources already populated.
> + */
> + np = dev->dev.of_node;
> + if (!np)
> + return 0;
> +
> + /* Populate the resource table */
> + if (!dev->resource) {
> + int rc, nreg = 0, nirq;
> + /* count the io and irq resources */
> + nreg = of_reg_count(np);
> + nirq = of_irq_count(np);
> +
> + if (!nirq && !nreg)
> + return 0;
> +
> + res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + dev->resource = res;
> + dev->num_resources = nreg + nirq;
> +
> + for (i = 0; i < nreg; i++, res++) {
> + rc = of_address_to_resource(np, i, res);
> + if (WARN_ON(rc)) {
> + /* THIS IS BAD; don't try to defer probing */
> + dev->num_resources = 0;
> + dev->resource = NULL;
> + kfree(res);
> + return rc;
> + }
> + }
> +
> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
> + /* IRQ controller is yet available. defer probing */
> + return -EPROBE_DEFER;
> + }
> +
> + return 0;
> + }
> +
> + /* See which IRQ resources need to be redone */
> + irq_index = 0;
> + for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
> + if (!res->flags) {
> + if (!of_irq_to_resource(np, irq_index, res))
> + return -EPROBE_DEFER;
> + irq_index++;
> + } else if (res->flags & IORESOURCE_IRQ)
> + irq_index++;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(of_platform_device_prepare);
> +
> /**
> * of_platform_device_create - Alloc, initialize and register an of_device
> * @np: pointer to node to create device for
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..4e487ff 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -53,6 +53,16 @@ struct of_dev_auxdata {
>
> extern const struct of_device_id of_default_bus_match_table[];
>
> +/* Populate the resource for a platform device */
> +#ifdef CONFIG_OF
> +int of_platform_device_prepare(struct platform_device *dev);
> +#else
> +static inline int of_platform_device_prepare(
> + struct platform_device *dev)
> +{
> + return 0;
> +}
> +#endif
> /* Platform drivers register/unregister */
> extern struct platform_device *of_device_alloc(struct device_node *np,
> const char *bus_id,
> --
> 1.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Grant,
2014-02-21 17:22 GMT+01:00 Jean-Jacques Hiblot <[email protected]>:
> Hi Grygorii,
>
> 2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <[email protected]>:
>> Hi Jean-Jacques,
>>
>> Sorry for top posting.
>>
>> As I know, there have been several attempts to solve the same problem already:)
>> [1] https://lkml.org/lkml/2013/9/18/216
>> [2] https://lkml.org/lkml/2013/11/22/520
>> [3] https://lkml.org/lkml/2014/1/8/240
>>
>> There are some questions related to your approach:
>> 1) How to distinguish between cases "IRQ domain not ready" and "wrong IRQ data in DT" or other IRQ parsing errors?
>> Otherwise, Driver's probe will be deffered wrongly and forever,
>> Thierry Reding has tried to solve this in [1].
>
> This approach doesn't really care about the cause of the problem. I'm
> of the opinion that never-ending deferred probing is not a big issue,
> being not triggered so often after start-up (only when a new device is
> probed). But if we need to make it right, then we would have to change
> a bit the API of irq_create_of_mapping() and irq_of_parse_and_map()
> (or maybe duplicate this one to keep the patch small) to return a real
> error code instead a simple 0. Then would should be able to
> distinguish the different error causes.
What do you think of the 2nd version of the patch? Is it all right to
allways return EPROBE_DEFER or should we try to discriminate the error
cause?
Jean-Jacques
>
>>
>> 2) How will be handled driver reloading situation?
>> The worst case (sparse IRQ enabled):
>> - remove driver A
>> - remove driver B (irq-controller)
>> - load driver B <--- different set of Linux IRQ numbers can be assigned
>> - load driver A <--- oops. IRQ resources table contains invalid data
>>
>
> It's not handled in the current implementation. But if all interrupts
> entries are re-parsed (see my comment for Grant), it should be all
> right.
> Another problem would appear if the DT is dynamically updated and the
> number of resource is changed. In the 1st version of the patch, this
> was handled but it made the function more expensive.
>
> Jean-Jacques
>>
>>
>> Best regards,
>> Grygorii Strashko
>>
>> =============================================
>>
>> The goal of this patch is to allow drivers to be probed even if at the time of
>> the DT parsing some of their ressources are not available yet.
>>
>> In the current situation, the resource of a platform device are filled from the
>> DT at the time the device is created (of_device_alloc()). The drawbackof this
>> is that a device sitting close to the top of the DT (ahb for example) but
>> depending on ressources that are initialized later (IRQ domain dynamically
>> created for example) will fail to probe because the ressources don't exist
>> at this time.
>>
>> This patch fills the resource structure only before the device is probed and
>> will defer the probe if the resource are not available yet.
>>
>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>> Reviewed-by: Gregory CLEMENT <[email protected]>
>> ---
>>
>> Hi Grant,
>>
>> I reworked the patch as you proposed. To keep the overhead minimum, nirq and
>> nreg are computed only the first time.
>> In this implementation, only the missing IRQ ressources are re-tried for. It could
>> easily be changed to re-parse all the IRQs though (replace if (!res->flags)
>> with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)).
>>
>> drivers/base/platform.c | 5 +++
>> drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++-----------
>> include/linux/of_platform.h | 10 +++++
>> 3 files changed, 90 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index bc78848..cee9b8d 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
>> struct platform_device *dev = to_platform_device(_dev);
>> int ret;
>>
>> + ret = of_platform_device_prepare(dev);
>> + if (ret)
>> + goto error;
>> +
>> if (ACPI_HANDLE(_dev))
>> acpi_dev_pm_attach(_dev, true);
>>
>> @@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev)
>> if (ret && ACPI_HANDLE(_dev))
>> acpi_dev_pm_detach(_dev, true);
>>
>> +error:
>> if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
>> dev_warn(_dev, "probe deferral not supported\n");
>> ret = -ENXIO;
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 404d1da..a4e2602 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np,
>> struct device *parent)
>> {
>> struct platform_device *dev;
>> - int rc, i, num_reg = 0, num_irq;
>> - struct resource *res, temp_res;
>>
>> dev = platform_device_alloc("", -1);
>> if (!dev)
>> return NULL;
>>
>> - /* count the io and irq resources */
>> - if (of_can_translate_address(np))
>> - while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>> - num_reg++;
>> - num_irq = of_irq_count(np);
>> -
>> - /* Populate the resource table */
>> - if (num_irq || num_reg) {
>> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
>> - if (!res) {
>> - platform_device_put(dev);
>> - return NULL;
>> - }
>> -
>> - dev->num_resources = num_reg + num_irq;
>> - dev->resource = res;
>> - for (i = 0; i < num_reg; i++, res++) {
>> - rc = of_address_to_resource(np, i, res);
>> - WARN_ON(rc);
>> - }
>> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>> - }
>> -
>> dev->dev.of_node = of_node_get(np);
>> #if defined(CONFIG_MICROBLAZE)
>> dev->dev.dma_mask = &dev->archdata.dma_mask;
>> @@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata(
>> return dev;
>> }
>>
>> +static int of_reg_count(struct device_node *np)
>> +{
>> + int nreg = 0;
>> + if (of_can_translate_address(np)) {
>> + struct resource temp_res;
>> + while (of_address_to_resource(np, nreg, &temp_res) == 0)
>> + nreg++;
>> + }
>> + return nreg;
>> +}
>> +
>> +int of_platform_device_prepare(struct platform_device *dev)
>> +{
>> + struct device_node *np;
>> + int i, irq_index;
>> + struct resource *res;
>> +
>> + /*
>> + * This function applies only devices described in the DT.
>> + * Other platform devices have their ressources already populated.
>> + */
>> + np = dev->dev.of_node;
>> + if (!np)
>> + return 0;
>> +
>> + /* Populate the resource table */
>> + if (!dev->resource) {
>> + int rc, nreg = 0, nirq;
>> + /* count the io and irq resources */
>> + nreg = of_reg_count(np);
>> + nirq = of_irq_count(np);
>> +
>> + if (!nirq && !nreg)
>> + return 0;
>> +
>> + res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL);
>> + if (!res)
>> + return -ENOMEM;
>> +
>> + dev->resource = res;
>> + dev->num_resources = nreg + nirq;
>> +
>> + for (i = 0; i < nreg; i++, res++) {
>> + rc = of_address_to_resource(np, i, res);
>> + if (WARN_ON(rc)) {
>> + /* THIS IS BAD; don't try to defer probing */
>> + dev->num_resources = 0;
>> + dev->resource = NULL;
>> + kfree(res);
>> + return rc;
>> + }
>> + }
>> +
>> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) {
>> + /* IRQ controller is yet available. defer probing */
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + /* See which IRQ resources need to be redone */
>> + irq_index = 0;
>> + for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
>> + if (!res->flags) {
>> + if (!of_irq_to_resource(np, irq_index, res))
>> + return -EPROBE_DEFER;
>> + irq_index++;
>> + } else if (res->flags & IORESOURCE_IRQ)
>> + irq_index++;
>> + }
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(of_platform_device_prepare);
>> +
>> /**
>> * of_platform_device_create - Alloc, initialize and register an of_device
>> * @np: pointer to node to create device for
>> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
>> index 05cb4a9..4e487ff 100644
>> --- a/include/linux/of_platform.h
>> +++ b/include/linux/of_platform.h
>> @@ -53,6 +53,16 @@ struct of_dev_auxdata {
>>
>> extern const struct of_device_id of_default_bus_match_table[];
>>
>> +/* Populate the resource for a platform device */
>> +#ifdef CONFIG_OF
>> +int of_platform_device_prepare(struct platform_device *dev);
>> +#else
>> +static inline int of_platform_device_prepare(
>> + struct platform_device *dev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> /* Platform drivers register/unregister */
>> extern struct platform_device *of_device_alloc(struct device_node *np,
>> const char *bus_id,
>> --
>> 1.9.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel