[PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

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 <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org>
Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

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 <jjhiblot@traphandler.com>:
> 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 <jjhiblot@traphandler.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  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
>

Re: [PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[Greg KH]

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 <jjhiblot@traphandler.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  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...

Re: [PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Tue, 18 Feb 2014 12:22:05 -0800, Greg KH <gregkh@linuxfoundation.org> 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 <jjhiblot@traphandler.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >  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.

Re: [PATCH] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Thu, 13 Feb 2014 10:57:09 +0100, Jean-Jacques Hiblot <jjhiblot@traphandler.com> 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 <jjhiblot@traphandler.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

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
> 

[PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

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 <jjhiblot@traphandler.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---

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

RE: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Strashko, Grygorii]

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 <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org>
Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---

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
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

Hi Grygorii,

2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <grygorii.strashko@ti.com>:
> 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 <jjhiblot@traphandler.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>
> 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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

Hi Grant,

2014-02-21 17:22 GMT+01:00 Jean-Jacques Hiblot <jjhiblot@traphandler.com>:
> Hi Grygorii,
>
> 2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <grygorii.strashko@ti.com>:
>> 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 <jjhiblot@traphandler.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>
>> 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
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Thu, 27 Feb 2014 17:43:15 +0100, Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:
> Hi Grant,
> 
> 2014-02-21 17:22 GMT+01:00 Jean-Jacques Hiblot <jjhiblot@traphandler.com>:
> > Hi Grygorii,
> >
> > 2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <grygorii.strashko@ti.com>:
> >> 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?

The error cause must be determined. It is explicitly allowed for a
single interrupt to be empty, and I believe there are users.

g.

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Ludovic Desroches]

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 <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org>
> Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Tested-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

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
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Fri, 21 Feb 2014 14:18:40 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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.

Hi Jean-Jacques

[...]
> +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;

I believe we already talked about the above test. This function must
only process devices created by of_platform_populate(). Merely checking
the of_node pointer is not a sufficient test because there are other
paths in the kernel for creating platform_devices that might get a node
pointer attached to it.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Russell King - ARM Linux]

On Sat, Mar 08, 2014 at 07:37:58AM +0000, Grant Likely wrote:
> I believe we already talked about the above test. This function must
> only process devices created by of_platform_populate(). Merely checking
> the of_node pointer is not a sufficient test because there are other
> paths in the kernel for creating platform_devices that might get a node
> pointer attached to it.

There definitely are: there are a number of platform device drivers
which create a new platform device structure, and copy the of_node
pointer across.

However, that's really sick code, and can result in the platform device
driver probing the device it just created... this practise should be
outlawed.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

Hi Grant,

Sorry for the delay, I was having nice vacations.

2014-03-08 8:37 GMT+01:00 Grant Likely <grant.likely@linaro.org>:
> On Fri, 21 Feb 2014 14:18:40 +0100, Jean-Jacques Hiblot <jjhiblot@traphandler.com> 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.
>
> Hi Jean-Jacques
>
> [...]
>> +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;
>
> I believe we already talked about the above test. This function must
> only process devices created by of_platform_populate(). Merely checking
> the of_node pointer is not a sufficient test because there are other
> paths in the kernel for creating platform_devices that might get a node
> pointer attached to it.
Yes we talked about this, but only to move it into of_platform_device_prepare().
Is there an existing way to know for sure that the device has been
created by of_platform_populate() ? I could not find one.

Jean-Jacques
>
> g.
>

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Mon, 17 Mar 2014 12:07:37 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> wrote:
> Hi Grant,
> 
> Sorry for the delay, I was having nice vacations.
> 
> 2014-03-08 8:37 GMT+01:00 Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> > On Fri, 21 Feb 2014 14:18:40 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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.
> >
> > Hi Jean-Jacques
> >
> > [...]
> >> +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;
> >
> > I believe we already talked about the above test. This function must
> > only process devices created by of_platform_populate(). Merely checking
> > the of_node pointer is not a sufficient test because there are other
> > paths in the kernel for creating platform_devices that might get a node
> > pointer attached to it.
> Yes we talked about this, but only to move it into of_platform_device_prepare().
> Is there an existing way to know for sure that the device has been
> created by of_platform_populate() ? I could not find one.

No there isn't. That information is used at population time and then
discarded. We'd need to add a flag or registry to mark those devices.

g.

> 
> Jean-Jacques
> >
> > g.
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

2014-03-17 15:24 GMT+01:00 Grant Likely <grant.likely@secretlab.ca>:
> On Mon, 17 Mar 2014 12:07:37 +0100, Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:
>> Hi Grant,
>>
>> Sorry for the delay, I was having nice vacations.
>>
>> 2014-03-08 8:37 GMT+01:00 Grant Likely <grant.likely@linaro.org>:
>> > On Fri, 21 Feb 2014 14:18:40 +0100, Jean-Jacques Hiblot <jjhiblot@traphandler.com> 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.
>> >
>> > Hi Jean-Jacques
>> >
>> > [...]
>> >> +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;
>> >
>> > I believe we already talked about the above test. This function must
>> > only process devices created by of_platform_populate(). Merely checking
>> > the of_node pointer is not a sufficient test because there are other
>> > paths in the kernel for creating platform_devices that might get a node
>> > pointer attached to it.
>> Yes we talked about this, but only to move it into of_platform_device_prepare().
>> Is there an existing way to know for sure that the device has been
>> created by of_platform_populate() ? I could not find one.
>
> No there isn't. That information is used at population time and then
> discarded. We'd need to add a flag or registry to mark those devices.
OK. As I see it, this could be done either by adding a new flag to
struct device or by maintaining a private list of "of created" devices
in of/platform.c.
I favor modifying struct device as it's is by far the simplest but
it's also more intrusive. Any advice ?

Jean-Jacques

>
> g.
>
>>
>> Jean-Jacques
>> >
>> > g.
>> >
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH v2] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Mon, 17 Mar 2014 16:20:03 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> wrote:
> 2014-03-17 15:24 GMT+01:00 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > On Mon, 17 Mar 2014 12:07:37 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> wrote:
> >> Hi Grant,
> >>
> >> Sorry for the delay, I was having nice vacations.
> >>
> >> 2014-03-08 8:37 GMT+01:00 Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> >> > On Fri, 21 Feb 2014 14:18:40 +0100, Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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.
> >> >
> >> > Hi Jean-Jacques
> >> >
> >> > [...]
> >> >> +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;
> >> >
> >> > I believe we already talked about the above test. This function must
> >> > only process devices created by of_platform_populate(). Merely checking
> >> > the of_node pointer is not a sufficient test because there are other
> >> > paths in the kernel for creating platform_devices that might get a node
> >> > pointer attached to it.
> >> Yes we talked about this, but only to move it into of_platform_device_prepare().
> >> Is there an existing way to know for sure that the device has been
> >> created by of_platform_populate() ? I could not find one.
> >
> > No there isn't. That information is used at population time and then
> > discarded. We'd need to add a flag or registry to mark those devices.
> OK. As I see it, this could be done either by adding a new flag to
> struct device or by maintaining a private list of "of created" devices
> in of/platform.c.
> I favor modifying struct device as it's is by far the simplest but
> it's also more intrusive. Any advice ?

I think that by far is the easiest. It is that, or
of_platform_populate() would need to maintain a list of devices that are
to be used as a platform bus.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

Hi Grant,

This is the 3rd version of the patch. It's been reworked to screen the devices
not created by of_device_alloc() and check the reason why an OF IRQ description
can't be translated into a resource.

Jean-Jacques

Changes since V2:
 * only fill the resources for devices created by of_device_alloc(). Note:
   this is done by adding a new flag in struct device {}.
 * use deferred probing only if the IRQ domain cannot be found. 
  
Jean-Jacques Hiblot (2):
  of: irq: Added of_find_irq_domain() to get the domain of an irq
  dt: platform driver: Fill the resources before probe and defer if
    needed

 drivers/base/platform.c     |   5 ++
 drivers/of/irq.c            |  20 ++++++++
 drivers/of/platform.c       | 114 ++++++++++++++++++++++++++++++++++----------
 include/linux/device.h      |   1 +
 include/linux/of_irq.h      |   2 +
 include/linux/of_platform.h |  10 ++++
 6 files changed, 127 insertions(+), 25 deletions(-)

-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] of: irq: Added of_find_irq_domain() to get the domain of an irq

[Jean-Jacques Hiblot]

This functions does the same parsing as irq_of_parse_and_map but instead
of doing the mapping, it returns a pointer to the irq domain

Signed-off-by: Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org>
---
 drivers/of/irq.c       | 20 ++++++++++++++++++++
 include/linux/of_irq.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9bcf2cf..7d3184f 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -46,6 +46,26 @@ unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
 /**
+ * of_find_irq_domain - Parse the DT and returns a pointer to the irq domain
+ * @dev: Device node of the device whose interrupt is concerned
+ * @index: Index of the interrupt to get the domain for
+ *
+ * This function is a wrapper that chains of_irq_parse_one() and
+ * irq_find_host() to make things easier to callers
+ */
+struct irq_domain *of_find_irq_domain(struct device_node *dev, int index)
+{
+	int rc;
+	struct of_phandle_args oirq;
+
+	rc = of_irq_parse_one(dev, index, &oirq);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return irq_find_host(oirq.np);
+}
+
+/**
  * of_irq_find_parent - Given a device node, find its interrupt parent node
  * @child: pointer to device node
  *
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 3f23b44..600ff0a 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -58,6 +58,8 @@ static inline int of_irq_count(struct device_node *dev)
  * so declare it here regardless of the CONFIG_OF_IRQ setting.
  */
 extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
+extern struct irq_domain *of_find_irq_domain(struct device_node *dev,
+					     int index);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
 
 #else /* !CONFIG_OF */
-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Jean-Jacques Hiblot]

The goal of this patch is to allow drivers to be probed even if at the time of
the DT parsing some of their IRQ ressources are not available yet.

In the current situation, the IRQ resources of a platform device are filled from
the DT at the time the device is created (of_device_alloc()). The drawback of
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 IRQ resource is not yet available.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/base/platform.c     |   5 ++
 drivers/of/platform.c       | 114 ++++++++++++++++++++++++++++++++++----------
 include/linux/device.h      |   1 +
 include/linux/of_platform.h |  10 ++++
 4 files changed, 105 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..ba6be4a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,41 +141,17 @@ 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;
 #endif
 	dev->dev.parent = parent;
+	dev->dev.of_device = 1;
 
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
@@ -233,6 +209,94 @@ 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 and
+	 * created by of_device_alloc().
+	 * Other platform devices have their ressources already populated.
+	 */
+	np = dev->dev.of_node;
+	if (!np || !dev->dev.of_device)
+		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)
+			/*
+			 * Not all irqs can be translated. redo the irq
+			 * resources to check if the probe can be deferred
+			 */
+			goto redo_irq_resources;
+
+		return 0;
+	}
+
+redo_irq_resources:
+	/* 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) {
+			int rc;
+			/*
+			 * If the IRQ can't be translated to a resource, check
+			 * if its IRQ domain exists.
+			 */
+			rc = of_irq_to_resource(np, irq_index, res);
+			if (!rc) {
+				if (of_find_irq_domain(np, irq_index) == NULL)
+					return -EPROBE_DEFER;
+				return rc;
+			}
+			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/device.h b/include/linux/device.h
index 952b010..eb6ac66 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -785,6 +785,7 @@ struct device {
 
 	bool			offline_disabled:1;
 	bool			offline:1;
+	bool			of_device:1;
 };
 
 static inline struct device *kobj_to_dev(struct kobject *kobj)
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

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Rob Herring]

On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
<jjhiblot@traphandler.com> 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 IRQ ressources are not available yet.
>
> In the current situation, the IRQ resources of a platform device are filled from
> the DT at the time the device is created (of_device_alloc()). The drawback of
> 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.

s/ressources/resources/

>
> This patch fills the resource structure only before the device is probed and
> will defer the probe if the IRQ resource is not yet available.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  drivers/base/platform.c     |   5 ++
>  drivers/of/platform.c       | 114 ++++++++++++++++++++++++++++++++++----------
>  include/linux/device.h      |   1 +
>  include/linux/of_platform.h |  10 ++++
>  4 files changed, 105 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..ba6be4a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -141,41 +141,17 @@ 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;
>  #endif
>         dev->dev.parent = parent;
> +       dev->dev.of_device = 1;
>
>         if (bus_id)
>                 dev_set_name(&dev->dev, "%s", bus_id);
> @@ -233,6 +209,94 @@ static struct platform_device *of_platform_device_create_pdata(
>         return dev;
>  }
>
> +static int of_reg_count(struct device_node *np)

This belongs in of/address.c. It can be more efficiently written as:

int of_reg_count(struct device_node *dev)
{
const __be32 *prop;
unsigned int psize;
struct device_node *parent;
struct of_bus *bus;
int na, ns;

/* Get parent & match bus type */
parent = of_get_parent(dev);
if (!parent)
  return 0;
bus = of_match_bus(parent);
bus->count_cells(dev, &na, &ns);
of_node_put(parent);
if (!OF_CHECK_COUNTS(na, ns))
  return 0;

/* Get "reg" or "assigned-addresses" property */
prop = of_get_property(dev, bus->addresses, &psize);
if (!prop)
  return 0;
psize /= 4;

return psize / (na + ns);
}


> +{
> +       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 and
> +        * created by of_device_alloc().
> +        * Other platform devices have their ressources already populated.
> +        */
> +       np = dev->dev.of_node;
> +       if (!np || !dev->dev.of_device)
> +               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);

I think of_irq_count should be changed to return errors from
of_irq_parse_one. It's complicated by the fact you want to distinguish
checking the index is too big versus all other errors.

> +
> +               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);

You are incrementing res, so this kfree is wrong. Using "res + i"
instead would work, but...

> +                               return rc;
> +                       }
> +               }
> +
> +               if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq)

...then res needs to change here.

Doesn't this give a rc may not be initialized warning? You don't need
to check rc. You need to check !nirq instead.

> +                       /*
> +                        * Not all irqs can be translated. redo the irq
> +                        * resources to check if the probe can be deferred
> +                        */
> +                       goto redo_irq_resources;
> +
> +               return 0;

But then, you don't really need any of this. You can just fall-thru
and the code below will work.

> +       }
> +
> +redo_irq_resources:
> +       /* 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) {
> +                       int rc;
> +                       /*
> +                        * If the IRQ can't be translated to a resource, check
> +                        * if its IRQ domain exists.
> +                        */
> +                       rc = of_irq_to_resource(np, irq_index, res);
> +                       if (!rc) {
> +                               if (of_find_irq_domain(np, irq_index) == NULL)
> +                                       return -EPROBE_DEFER;
> +                               return rc;
> +                       }

I would move this check into of_irq_to_resource_table and also make it
skip entries with IORESOURCE_IRQ set. Then this can become:

for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) {
  if (resource_type(res) != IORESOURCE_MEM)
    break;
}
rc = of_irq_to_resource_table(np, res, dev->num_resources - i);
return (rc < 0) ? rc : 0;


Rob

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140411 10:33]:
> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
> <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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 IRQ ressources are not available yet.
> >
> > In the current situation, the IRQ resources of a platform device are filled from
> > the DT at the time the device is created (of_device_alloc()). The drawback of
> > 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.
> 
> s/ressources/resources/

While I've tested these two patches and they fix the issue for me. I have
some serious doubts again about this whole ------ up string parsing
pile of ---- called device tree.

Do we really need to sprinkle more of_* hacks to the irq subsystem?

There's nothing wrong with with the irq subsystem. Platform bus is just
calling the irq subsystem at the wrong time with uninitialized data.

It seems that we're again papering over the fact that there's nothing
coordinating the setting up of various resources with device tree.
That seems to be the repeating never ending pattern as we've already
seen with GPIOs, pinctrl, and IRQchips. We end up adding all kinds of
cross subsystem calls that were never needed earlier.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Rob Herring]

On Fri, Apr 18, 2014 at 3:52 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140411 10:33]:
>> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
>> <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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 IRQ ressources are not available yet.
>> >
>> > In the current situation, the IRQ resources of a platform device are filled from
>> > the DT at the time the device is created (of_device_alloc()). The drawback of
>> > 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.
>>
>> s/ressources/resources/
>
> While I've tested these two patches and they fix the issue for me. I have
> some serious doubts again about this whole ------ up string parsing
> pile of ---- called device tree.
>
> Do we really need to sprinkle more of_* hacks to the irq subsystem?
>
> There's nothing wrong with with the irq subsystem. Platform bus is just
> calling the irq subsystem at the wrong time with uninitialized data.

This patch doesn't even touch the irq subsystem.

> It seems that we're again papering over the fact that there's nothing
> coordinating the setting up of various resources with device tree.
> That seems to be the repeating never ending pattern as we've already
> seen with GPIOs, pinctrl, and IRQchips. We end up adding all kinds of
> cross subsystem calls that were never needed earlier.

This problem may be made worse with DT, but this problem has existed
for as long as I have worked on the kernel. It is worse with DT since
platforms have less control over the init ordering and loose some
ability to do tricks like changing the initcall levels. We didn't even
try to do pinctrl in any generic way before DT.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140418 14:39]:
> On Fri, Apr 18, 2014 at 3:52 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140411 10:33]:
> >> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
> >> <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org> 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 IRQ ressources are not available yet.
> >> >
> >> > In the current situation, the IRQ resources of a platform device are filled from
> >> > the DT at the time the device is created (of_device_alloc()). The drawback of
> >> > 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.
> >>
> >> s/ressources/resources/
> >
> > While I've tested these two patches and they fix the issue for me. I have
> > some serious doubts again about this whole ------ up string parsing
> > pile of ---- called device tree.
> >
> > Do we really need to sprinkle more of_* hacks to the irq subsystem?
> >
> > There's nothing wrong with with the irq subsystem. Platform bus is just
> > calling the irq subsystem at the wrong time with uninitialized data.
> 
> This patch doesn't even touch the irq subsystem.

Heh, it depends on the previous patch in this series adding another
of_blah_blah function to drivers/of/irq.c.
 
> > It seems that we're again papering over the fact that there's nothing
> > coordinating the setting up of various resources with device tree.
> > That seems to be the repeating never ending pattern as we've already
> > seen with GPIOs, pinctrl, and IRQchips. We end up adding all kinds of
> > cross subsystem calls that were never needed earlier.
> 
> This problem may be made worse with DT, but this problem has existed
> for as long as I have worked on the kernel. It is worse with DT since
> platforms have less control over the init ordering and loose some
> ability to do tricks like changing the initcall levels. We didn't even
> try to do pinctrl in any generic way before DT.

Oh come on, let's stop pretending it's not broken. And it's way worse with
device tree as there's nothing making sure the resources for a driver
are set up before the driver probes. And we've been unable to fix just
this issue alone for about six months now. It's also broken beyond that.
It's called of_platform_bus yet it won't even pass the platform_data
as auxdata to the devices on a sub-bus instantatiated like I2C.

Regards,

Tony


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Russell King - ARM Linux]

On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> Oh come on, let's stop pretending it's not broken. And it's way worse with
> device tree as there's nothing making sure the resources for a driver
> are set up before the driver probes. And we've been unable to fix just
> this issue alone for about six months now. It's also broken beyond that.
> It's called of_platform_bus yet it won't even pass the platform_data
> as auxdata to the devices on a sub-bus instantatiated like I2C.

Isn't there a much simpler solution to the platform device IRQ problem?

Rather than trying to fix it at the point where the resources are
created, why not just *not* have DT create the IRQ resources in the
first place, and instead have platform_get_irq() (which is the function
which should be used to get an IRQ) be the actor to do whatever is
necessary to return the IRQ(s) ?

Yes, I know we have some drivers which use platform_get_resources() with
IORESOURCE_IRQ, but they should really use the right accessor.  And those
who just dereference the resource array directly... get what's coming
(though of course they have to be fixed.)

It has the benefit that you're in a path where you /can/ return
-EPROBE_DEFER too and not have to mess around with notifiers or other
silly stuff like that.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> [140418 16:04]:
> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> > device tree as there's nothing making sure the resources for a driver
> > are set up before the driver probes. And we've been unable to fix just
> > this issue alone for about six months now. It's also broken beyond that.
> > It's called of_platform_bus yet it won't even pass the platform_data
> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> 
> Isn't there a much simpler solution to the platform device IRQ problem?
> 
> Rather than trying to fix it at the point where the resources are
> created, why not just *not* have DT create the IRQ resources in the
> first place, and instead have platform_get_irq() (which is the function
> which should be used to get an IRQ) be the actor to do whatever is
> necessary to return the IRQ(s) ?

Yeah why not. I don't see why we would need to do all this of_* special
trickery for much anything beyond parsing the binding.
 
> Yes, I know we have some drivers which use platform_get_resources() with
> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> who just dereference the resource array directly... get what's coming
> (though of course they have to be fixed.)

$ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
179

But might be scriptable to some extent..
 
> It has the benefit that you're in a path where you /can/ return
> -EPROBE_DEFER too and not have to mess around with notifiers or other
> silly stuff like that.

And then maybe we can make of_platform_probe() or some bus function
do the most of the -EPROBE_DEFER ping pong before the driver even
probes?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Rob Herring]

On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [140418 16:04]:
>> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
>> > Oh come on, let's stop pretending it's not broken. And it's way worse with
>> > device tree as there's nothing making sure the resources for a driver
>> > are set up before the driver probes. And we've been unable to fix just
>> > this issue alone for about six months now. It's also broken beyond that.
>> > It's called of_platform_bus yet it won't even pass the platform_data
>> > as auxdata to the devices on a sub-bus instantatiated like I2C.
>>
>> Isn't there a much simpler solution to the platform device IRQ problem?
>>
>> Rather than trying to fix it at the point where the resources are
>> created, why not just *not* have DT create the IRQ resources in the
>> first place, and instead have platform_get_irq() (which is the function
>> which should be used to get an IRQ) be the actor to do whatever is
>> necessary to return the IRQ(s) ?
>
> Yeah why not. I don't see why we would need to do all this of_* special
> trickery for much anything beyond parsing the binding.

That can work, but it will still need something like
of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
not.

You could also go in the other direction and don't create the device
until the resources can be resolved. Unlike any of the other
solutions, that would work for amba bus as well although we may never
have a case where we need this with the amba bus. This would require
making of_platform_populate be callable multiple times, but there are
already some other reasons for wanting to do that. Specifically, I
would like the core code to call of_platform_populate with default
options and then only platforms with non-default options need a call
to of_platform_populate.


>> Yes, I know we have some drivers which use platform_get_resources() with
>> IORESOURCE_IRQ, but they should really use the right accessor.  And those
>> who just dereference the resource array directly... get what's coming
>> (though of course they have to be fixed.)
>
> $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> 179

Certainly, this is worthwhile clean-up no matter what the solution.

Rob

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 06:47]:
> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> [140418 16:04]:
> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> >> > device tree as there's nothing making sure the resources for a driver
> >> > are set up before the driver probes. And we've been unable to fix just
> >> > this issue alone for about six months now. It's also broken beyond that.
> >> > It's called of_platform_bus yet it won't even pass the platform_data
> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> >>
> >> Isn't there a much simpler solution to the platform device IRQ problem?
> >>
> >> Rather than trying to fix it at the point where the resources are
> >> created, why not just *not* have DT create the IRQ resources in the
> >> first place, and instead have platform_get_irq() (which is the function
> >> which should be used to get an IRQ) be the actor to do whatever is
> >> necessary to return the IRQ(s) ?
> >
> > Yeah why not. I don't see why we would need to do all this of_* special
> > trickery for much anything beyond parsing the binding.
> 
> That can work, but it will still need something like
> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
> not.

Right. Naturally let's do whatever it takes to first fix this issue
in a minimal way first for the -rc cycle so we can do the longer term
changes needed.
 
> You could also go in the other direction and don't create the device
> until the resources can be resolved. Unlike any of the other
> solutions, that would work for amba bus as well although we may never
> have a case where we need this with the amba bus. This would require
> making of_platform_populate be callable multiple times, but there are
> already some other reasons for wanting to do that. Specifically, I
> would like the core code to call of_platform_populate with default
> options and then only platforms with non-default options need a call
> to of_platform_populate.

I like this idea as this would also probably remove the the numerous
dmesg errors we are currently getting for drivers reprobing with
-EPROBE_DEFER.

In the long term we should have platform bus just call a set of
standardized functions implemented by whatever the data source might
be. That way we can limit the use of of_* functions in device drivers
to just parsing of custom bindings in the drivers and use bus specific
functions for everything else.

> >> Yes, I know we have some drivers which use platform_get_resources() with
> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> >> who just dereference the resource array directly... get what's coming
> >> (though of course they have to be fixed.)
> >
> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> > 179
> 
> Certainly, this is worthwhile clean-up no matter what the solution.

Yeah agreed. But let's also consider the IORESOURCE_IRQ as just another
source for for the bus or driver data in addition to the DT parsed data.
Both sources of data should work just fine with platform_bus even
without cleaning up the drivers.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Rob Herring]

On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 06:47]:
>> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> > * Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> [140418 16:04]:
>> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
>> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
>> >> > device tree as there's nothing making sure the resources for a driver
>> >> > are set up before the driver probes. And we've been unable to fix just
>> >> > this issue alone for about six months now. It's also broken beyond that.
>> >> > It's called of_platform_bus yet it won't even pass the platform_data
>> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
>> >>
>> >> Isn't there a much simpler solution to the platform device IRQ problem?
>> >>
>> >> Rather than trying to fix it at the point where the resources are
>> >> created, why not just *not* have DT create the IRQ resources in the
>> >> first place, and instead have platform_get_irq() (which is the function
>> >> which should be used to get an IRQ) be the actor to do whatever is
>> >> necessary to return the IRQ(s) ?
>> >
>> > Yeah why not. I don't see why we would need to do all this of_* special
>> > trickery for much anything beyond parsing the binding.
>>
>> That can work, but it will still need something like
>> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
>> not.
>
> Right. Naturally let's do whatever it takes to first fix this issue
> in a minimal way first for the -rc cycle so we can do the longer term
> changes needed.

I'm not really convinced there is a simple and safe enough solution for
3.15. We should also be willing to tag a solution for stable if we take
it for -rc (although that decision could be deferred).

>> You could also go in the other direction and don't create the device
>> until the resources can be resolved. Unlike any of the other
>> solutions, that would work for amba bus as well although we may never
>> have a case where we need this with the amba bus. This would require
>> making of_platform_populate be callable multiple times, but there are
>> already some other reasons for wanting to do that. Specifically, I
>> would like the core code to call of_platform_populate with default
>> options and then only platforms with non-default options need a call
>> to of_platform_populate.
>
> I like this idea as this would also probably remove the the numerous
> dmesg errors we are currently getting for drivers reprobing with
> -EPROBE_DEFER.

One issue with my proposal is with supporting modules. IIUC, deferred
probe will continue trying forever and loading modules can cause probe
to succeed. If devices are not created and on the deferred probe list,
then they will not get probed when a module load fixes the dependency.

> In the long term we should have platform bus just call a set of
> standardized functions implemented by whatever the data source might
> be. That way we can limit the use of of_* functions in device drivers
> to just parsing of custom bindings in the drivers and use bus specific
> functions for everything else.
>
>> >> Yes, I know we have some drivers which use platform_get_resources() with
>> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
>> >> who just dereference the resource array directly... get what's coming
>> >> (though of course they have to be fixed.)
>> >
>> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
>> > 179
>>
>> Certainly, this is worthwhile clean-up no matter what the solution.
>
> Yeah agreed. But let's also consider the IORESOURCE_IRQ as just another
> source for for the bus or driver data in addition to the DT parsed data.
> Both sources of data should work just fine with platform_bus even
> without cleaning up the drivers

Ah, right. Except for those drivers you need to work with deferred probe
would have to use platform_get_irq. That fact makes this solution quite
a bit easier.

Something like this is what you had in mind?

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e714709..5b47210 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -13,6 +13,7 @@
 #include <linux/string.h>
 #include <linux/platform_device.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/dma-mapping.h>
@@ -87,7 +88,11 @@ int platform_get_irq(struct platform_device *dev,
unsigned int num)
 		return -ENXIO;
 	return dev->archdata.irqs[num];
 #else
-	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+	struct resource *r;
+	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node)
+		return of_irq_get(dev->dev.of_node, num);
+
+	r = platform_get_resource(dev, IORESOURCE_IRQ, num);

 	return r ? r->start : -ENXIO;
 #endif
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 7d3184f..30449ad 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
index, struct resource *r)
 EXPORT_SYMBOL_GPL(of_irq_to_resource);

 /**
+ * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
+ * @dev: pointer to device tree node
+ * @index: zero-based index of the irq
+ *
+ * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
+ * is not yet created.
+ *
+ */
+int of_irq_get(struct device_node *dev, int index)
+{
+	int irq = irq_of_parse_and_map(dev, index);
+
+	if (!irq && of_find_irq_domain(dev, index) == NULL)
+		return -EPROBE_DEFER;
+
+	return irq;
+}
+EXPORT_SYMBOL_GPL(of_irq_get);
+
+/**
  * of_irq_count - Count the number of IRQs a node uses
  * @dev: pointer to device tree node
  */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 12:01]:
> On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 06:47]:
> >> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >> > * Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> [140418 16:04]:
> >> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> >> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> >> >> > device tree as there's nothing making sure the resources for a driver
> >> >> > are set up before the driver probes. And we've been unable to fix just
> >> >> > this issue alone for about six months now. It's also broken beyond that.
> >> >> > It's called of_platform_bus yet it won't even pass the platform_data
> >> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> >> >>
> >> >> Isn't there a much simpler solution to the platform device IRQ problem?
> >> >>
> >> >> Rather than trying to fix it at the point where the resources are
> >> >> created, why not just *not* have DT create the IRQ resources in the
> >> >> first place, and instead have platform_get_irq() (which is the function
> >> >> which should be used to get an IRQ) be the actor to do whatever is
> >> >> necessary to return the IRQ(s) ?
> >> >
> >> > Yeah why not. I don't see why we would need to do all this of_* special
> >> > trickery for much anything beyond parsing the binding.
> >>
> >> That can work, but it will still need something like
> >> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
> >> not.
> >
> > Right. Naturally let's do whatever it takes to first fix this issue
> > in a minimal way first for the -rc cycle so we can do the longer term
> > changes needed.
> 
> I'm not really convinced there is a simple and safe enough solution for
> 3.15. We should also be willing to tag a solution for stable if we take
> it for -rc (although that decision could be deferred).

Yes the fix needs to also go to stable. And this is a regression
between legacy booting and DT based booting so we do have good
reasons to fix this for the -rc cycle. Who knows how many people are
hitting this and are tinkering with the driver initcall levels to
work around it. Ideally the fix, even if intrusive, would already
get us a little bit into the right direction.

> >> You could also go in the other direction and don't create the device
> >> until the resources can be resolved. Unlike any of the other
> >> solutions, that would work for amba bus as well although we may never
> >> have a case where we need this with the amba bus. This would require
> >> making of_platform_populate be callable multiple times, but there are
> >> already some other reasons for wanting to do that. Specifically, I
> >> would like the core code to call of_platform_populate with default
> >> options and then only platforms with non-default options need a call
> >> to of_platform_populate.
> >
> > I like this idea as this would also probably remove the the numerous
> > dmesg errors we are currently getting for drivers reprobing with
> > -EPROBE_DEFER.
> 
> One issue with my proposal is with supporting modules. IIUC, deferred
> probe will continue trying forever and loading modules can cause probe
> to succeed. If devices are not created and on the deferred probe list,
> then they will not get probed when a module load fixes the dependency.
> 
> > In the long term we should have platform bus just call a set of
> > standardized functions implemented by whatever the data source might
> > be. That way we can limit the use of of_* functions in device drivers
> > to just parsing of custom bindings in the drivers and use bus specific
> > functions for everything else.
> >
> >> >> Yes, I know we have some drivers which use platform_get_resources() with
> >> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> >> >> who just dereference the resource array directly... get what's coming
> >> >> (though of course they have to be fixed.)
> >> >
> >> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> >> > 179
> >>
> >> Certainly, this is worthwhile clean-up no matter what the solution.
> >
> > Yeah agreed. But let's also consider the IORESOURCE_IRQ as just another
> > source for for the bus or driver data in addition to the DT parsed data.
> > Both sources of data should work just fine with platform_bus even
> > without cleaning up the drivers
> 
> Ah, right. Except for those drivers you need to work with deferred probe
> would have to use platform_get_irq. That fact makes this solution quite
> a bit easier.

Yes fixing up the known broken drivers is also doable for the -rc
cycle.
 
> Something like this is what you had in mind?
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index e714709..5b47210 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/dma-mapping.h>
> @@ -87,7 +88,11 @@ int platform_get_irq(struct platform_device *dev,
> unsigned int num)
>  		return -ENXIO;
>  	return dev->archdata.irqs[num];
>  #else
> -	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> +	struct resource *r;
> +	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node)
> +		return of_irq_get(dev->dev.of_node, num);
> +
> +	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> 
>  	return r ? r->start : -ENXIO;
>  #endif
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 7d3184f..30449ad 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
> index, struct resource *r)
>  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> 
>  /**
> + * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
> + * @dev: pointer to device tree node
> + * @index: zero-based index of the irq
> + *
> + * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
> + * is not yet created.
> + *
> + */
> +int of_irq_get(struct device_node *dev, int index)
> +{
> +	int irq = irq_of_parse_and_map(dev, index);
> +
> +	if (!irq && of_find_irq_domain(dev, index) == NULL)
> +		return -EPROBE_DEFER;
> +
> +	return irq;
> +}

Yeah something like that. That might also work as a pretty
minimal fix as long as we fix the known broken drivers to use
platform_get_irq().

But for the long run, how about we define some kind of Linux
generic bus ops:

struct bus_ops {
	struct resource *	(*get_resource)(struct device *dev, unsigned int type, unsigned int num);
	int			(get_irq)(struct device *dev, unsigned int num);
	...
};

And then DT code would register static of_irq_get() as a get_irq()
for the platform_bus. And naturally we'd implement these functions
for legacy resources too. And that way platform_bus code stays clear
of implementation details and just goes through the list of
registered data sources. It may even make sense to do it for the
fix to keep the of_* functions private to drivers/of/*.c and the
bus code generic.

> +EXPORT_SYMBOL_GPL(of_irq_get);

Probably no need to export this? I don't think we want other
code to use this directly..

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [140421 13:26]:
> * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 12:01]:
> > On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > > * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 06:47]:
> > >> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > >> > * Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> [140418 16:04]:
> > >> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> > >> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> > >> >> > device tree as there's nothing making sure the resources for a driver
> > >> >> > are set up before the driver probes. And we've been unable to fix just
> > >> >> > this issue alone for about six months now. It's also broken beyond that.
> > >> >> > It's called of_platform_bus yet it won't even pass the platform_data
> > >> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> > >> >>
> > >> >> Isn't there a much simpler solution to the platform device IRQ problem?
> > >> >>
> > >> >> Rather than trying to fix it at the point where the resources are
> > >> >> created, why not just *not* have DT create the IRQ resources in the
> > >> >> first place, and instead have platform_get_irq() (which is the function
> > >> >> which should be used to get an IRQ) be the actor to do whatever is
> > >> >> necessary to return the IRQ(s) ?
> > >> >
> > >> > Yeah why not. I don't see why we would need to do all this of_* special
> > >> > trickery for much anything beyond parsing the binding.
> > >>
> > >> That can work, but it will still need something like
> > >> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
> > >> not.
> > >
> > > Right. Naturally let's do whatever it takes to first fix this issue
> > > in a minimal way first for the -rc cycle so we can do the longer term
> > > changes needed.
> > 
> > I'm not really convinced there is a simple and safe enough solution for
> > 3.15. We should also be willing to tag a solution for stable if we take
> > it for -rc (although that decision could be deferred).
> 
> Yes the fix needs to also go to stable. And this is a regression
> between legacy booting and DT based booting so we do have good
> reasons to fix this for the -rc cycle. Who knows how many people are
> hitting this and are tinkering with the driver initcall levels to
> work around it. Ideally the fix, even if intrusive, would already
> get us a little bit into the right direction.
> 
> > >> You could also go in the other direction and don't create the device
> > >> until the resources can be resolved. Unlike any of the other
> > >> solutions, that would work for amba bus as well although we may never
> > >> have a case where we need this with the amba bus. This would require
> > >> making of_platform_populate be callable multiple times, but there are
> > >> already some other reasons for wanting to do that. Specifically, I
> > >> would like the core code to call of_platform_populate with default
> > >> options and then only platforms with non-default options need a call
> > >> to of_platform_populate.
> > >
> > > I like this idea as this would also probably remove the the numerous
> > > dmesg errors we are currently getting for drivers reprobing with
> > > -EPROBE_DEFER.
> > 
> > One issue with my proposal is with supporting modules. IIUC, deferred
> > probe will continue trying forever and loading modules can cause probe
> > to succeed. If devices are not created and on the deferred probe list,
> > then they will not get probed when a module load fixes the dependency.
> > 
> > > In the long term we should have platform bus just call a set of
> > > standardized functions implemented by whatever the data source might
> > > be. That way we can limit the use of of_* functions in device drivers
> > > to just parsing of custom bindings in the drivers and use bus specific
> > > functions for everything else.
> > >
> > >> >> Yes, I know we have some drivers which use platform_get_resources() with
> > >> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> > >> >> who just dereference the resource array directly... get what's coming
> > >> >> (though of course they have to be fixed.)
> > >> >
> > >> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> > >> > 179
> > >>
> > >> Certainly, this is worthwhile clean-up no matter what the solution.
> > >
> > > Yeah agreed. But let's also consider the IORESOURCE_IRQ as just another
> > > source for for the bus or driver data in addition to the DT parsed data.
> > > Both sources of data should work just fine with platform_bus even
> > > without cleaning up the drivers
> > 
> > Ah, right. Except for those drivers you need to work with deferred probe
> > would have to use platform_get_irq. That fact makes this solution quite
> > a bit easier.
> 
> Yes fixing up the known broken drivers is also doable for the -rc
> cycle.
>  
> > Something like this is what you had in mind?
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index e714709..5b47210 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/string.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/dma-mapping.h>
> > @@ -87,7 +88,11 @@ int platform_get_irq(struct platform_device *dev,
> > unsigned int num)
> >  		return -ENXIO;
> >  	return dev->archdata.irqs[num];
> >  #else
> > -	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +	struct resource *r;
> > +	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node)
> > +		return of_irq_get(dev->dev.of_node, num);
> > +
> > +	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > 
> >  	return r ? r->start : -ENXIO;
> >  #endif
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 7d3184f..30449ad 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
> > index, struct resource *r)
> >  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> > 
> >  /**
> > + * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
> > + * @dev: pointer to device tree node
> > + * @index: zero-based index of the irq
> > + *
> > + * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
> > + * is not yet created.
> > + *
> > + */
> > +int of_irq_get(struct device_node *dev, int index)
> > +{
> > +	int irq = irq_of_parse_and_map(dev, index);
> > +
> > +	if (!irq && of_find_irq_domain(dev, index) == NULL)
> > +		return -EPROBE_DEFER;
> > +
> > +	return irq;
> > +}
> 
> Yeah something like that. That might also work as a pretty
> minimal fix as long as we fix the known broken drivers to use
> platform_get_irq().

Actually, the above code for of_irq_get() won't help as we're still
calling irq_of_parse_and_map() before we should. So the nasty warnings
are still there if the irqdomain is not yet found.
 
> But for the long run, how about we define some kind of Linux
> generic bus ops:
> 
> struct bus_ops {
> 	struct resource *	(*get_resource)(struct device *dev, unsigned int type, unsigned int num);
> 	int			(get_irq)(struct device *dev, unsigned int num);
> 	...
> };
> 
> And then DT code would register static of_irq_get() as a get_irq()
> for the platform_bus. And naturally we'd implement these functions
> for legacy resources too. And that way platform_bus code stays clear
> of implementation details and just goes through the list of
> registered data sources. It may even make sense to do it for the
> fix to keep the of_* functions private to drivers/of/*.c and the
> bus code generic.
> 
> > +EXPORT_SYMBOL_GPL(of_irq_get);
> 
> Probably no need to export this? I don't think we want other
> code to use this directly..
> 
> Regards,
> 
> Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [140421 20:06]:
> * Tony Lindgren <tony@atomide.com> [140421 13:26]:
> > * Rob Herring <robherring2@gmail.com> [140421 12:01]:
> > > Something like this is what you had in mind?
... 
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
> > > index, struct resource *r)
> > >  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> > > 
> > >  /**
> > > + * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
> > > + * @dev: pointer to device tree node
> > > + * @index: zero-based index of the irq
> > > + *
> > > + * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
> > > + * is not yet created.
> > > + *
> > > + */
> > > +int of_irq_get(struct device_node *dev, int index)
> > > +{
> > > +	int irq = irq_of_parse_and_map(dev, index);
> > > +
> > > +	if (!irq && of_find_irq_domain(dev, index) == NULL)
> > > +		return -EPROBE_DEFER;
> > > +
> > > +	return irq;
> > > +}
> > 
> > Yeah something like that. That might also work as a pretty
> > minimal fix as long as we fix the known broken drivers to use
> > platform_get_irq().
> 
> Actually, the above code for of_irq_get() won't help as we're still
> calling irq_of_parse_and_map() before we should. So the nasty warnings
> are still there if the irqdomain is not yet found.

OK so to fix the warning part of the problem we first need to not
try to map uninitialized irqdomains and then downgrade the current
warning to a dev_dbg.

So looks like the current minimal fix to my original problem the
first patch from Jean-Jacques in this series, and the following patch.

This works for drivers that currently do of_irq_parse_and_map(),
then your patch is also needed to make things work properly with
platform_get_irq().

8< ------------------
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 21 Apr 2014 19:33:43 -0700
Subject: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts

Currently we get the following kind of errors if we try to use interrupt
phandles to irqchips that have not yet initialized:

irq: no irq domain found for /ocp/pinmux@48002030 !
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
(show_stack+0x14/0x1c)
(dump_stack+0x6c/0xa0)
(warn_slowpath_common+0x64/0x84)
(warn_slowpath_null+0x1c/0x24)
(of_device_alloc+0x144/0x184)
(of_platform_device_create_pdata+0x44/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)

This is because we're wrongly trying to populate resources that are not
yet available. It's perfectly valid to create irqchips dynamically, so
let's fix up the issue by populating the interrupt resources at the
driver probe time instead.

Let's fix the problem by using of_find_irq_domain() recently introduced
by Jean-Jacques Hiblot <jjhiblot@traphandler.com>. This way we can
avoid calling irq_of_parse_and_map() unnecesssarily with incomplete
data.

And then we also need to accept the fact that some irqdomains do not
exist that early on, and only get initialized later on. So we can
make the current WARN_ON into just into a pr_debug().

Note that this patch only solves the problem for drivers that are
currently doing of_irq_parse_and_map(). A follow-up patch is needed
to make platform_get_irq() to work without relying on the populated
resources.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -425,13 +425,17 @@ int of_irq_count(struct device_node *dev)
 int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
 		int nr_irqs)
 {
-	int i;
+	int i, found = 0;
 
-	for (i = 0; i < nr_irqs; i++, res++)
+	for (i = 0; i < nr_irqs; i++, res++) {
+		if (!of_find_irq_domain(dev, i))
+			continue;
 		if (!of_irq_to_resource(dev, i, res))
 			break;
+		found++;
+	}
 
-	return i;
+	return found;
 }
 EXPORT_SYMBOL_GPL(of_irq_to_resource_table);
 
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -168,7 +168,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
-		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
+		if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
+			pr_debug("not all legacy IRQ resources mapped for %s\n",
+				 np->name);
 	}
 
 	dev->dev.of_node = of_node_get(np);

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Rob Herring]

On Mon, Apr 21, 2014 at 11:57 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [140421 20:06]:
>> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [140421 13:26]:
>> > * Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [140421 12:01]:

[...]

> 8< ------------------
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Mon, 21 Apr 2014 19:33:43 -0700
> Subject: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
>
> Currently we get the following kind of errors if we try to use interrupt
> phandles to irqchips that have not yet initialized:
>
> irq: no irq domain found for /ocp/pinmux@48002030 !
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 of_device_alloc+0x144/0x184()
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-00038-g42a9708 #1012
> (show_stack+0x14/0x1c)
> (dump_stack+0x6c/0xa0)
> (warn_slowpath_common+0x64/0x84)
> (warn_slowpath_null+0x1c/0x24)
> (of_device_alloc+0x144/0x184)
> (of_platform_device_create_pdata+0x44/0x9c)
> (of_platform_bus_create+0xd0/0x170)
> (of_platform_bus_create+0x12c/0x170)
> (of_platform_populate+0x60/0x98)
>
> This is because we're wrongly trying to populate resources that are not
> yet available. It's perfectly valid to create irqchips dynamically, so
> let's fix up the issue by populating the interrupt resources at the
> driver probe time instead.
>
> Let's fix the problem by using of_find_irq_domain() recently introduced
> by Jean-Jacques Hiblot <jjhiblot-dLKeG7h1OhBDOHtkgc7UlQ@public.gmane.org>. This way we can
> avoid calling irq_of_parse_and_map() unnecesssarily with incomplete
> data.
>
> And then we also need to accept the fact that some irqdomains do not
> exist that early on, and only get initialized later on. So we can
> make the current WARN_ON into just into a pr_debug().
>
> Note that this patch only solves the problem for drivers that are
> currently doing of_irq_parse_and_map(). A follow-up patch is needed
> to make platform_get_irq() to work without relying on the populated
> resources.
>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -425,13 +425,17 @@ int of_irq_count(struct device_node *dev)
>  int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>                 int nr_irqs)
>  {
> -       int i;
> +       int i, found = 0;
>
> -       for (i = 0; i < nr_irqs; i++, res++)
> +       for (i = 0; i < nr_irqs; i++, res++) {
> +               if (!of_find_irq_domain(dev, i))
> +                       continue;

I don't think this is necessary. We either need to resolve all
interrupts or none. Having some of them resolved does not help us. The
following line will catch the latter case. I'll send out a full proper
series soon.

>                 if (!of_irq_to_resource(dev, i, res))
>                         break;
> +               found++;
> +       }
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Russell King - ARM Linux]

On Mon, Apr 21, 2014 at 02:01:05PM -0500, Rob Herring wrote:
> Ah, right. Except for those drivers you need to work with deferred probe
> would have to use platform_get_irq. That fact makes this solution quite
> a bit easier.
> 
> Something like this is what you had in mind?

The below is what I had in mind... :)  This moves it to the right place
for deferred probing to work - the alternative is we need some way to
"register" a device which can't be probed immediately, track it's
resources, have some way for it to be notified when those resources
become available (or poll for them becoming available... eg at the
same time as the deferred probing trigger) and then make it properly
available to drivers.  That brings up all sorts of questions about
whether it should be registered in sysfs, what if another device with
the same name comes along, etc.

So the solution below is very much a simple and nice solution to the
problem - it makes full use of our existing infrastructure to deal with
the problem.

> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index e714709..5b47210 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/dma-mapping.h>
> @@ -87,7 +88,11 @@ int platform_get_irq(struct platform_device *dev,
> unsigned int num)
>  		return -ENXIO;
>  	return dev->archdata.irqs[num];
>  #else
> -	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> +	struct resource *r;
> +	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node)
> +		return of_irq_get(dev->dev.of_node, num);
> +
> +	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> 
>  	return r ? r->start : -ENXIO;
>  #endif
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 7d3184f..30449ad 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
> index, struct resource *r)
>  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> 
>  /**
> + * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
> + * @dev: pointer to device tree node
> + * @index: zero-based index of the irq
> + *
> + * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
> + * is not yet created.
> + *
> + */
> +int of_irq_get(struct device_node *dev, int index)
> +{
> +	int irq = irq_of_parse_and_map(dev, index);
> +
> +	if (!irq && of_find_irq_domain(dev, index) == NULL)
> +		return -EPROBE_DEFER;
> +
> +	return irq;
> +}
> +EXPORT_SYMBOL_GPL(of_irq_get);
> +
> +/**
>   * of_irq_count - Count the number of IRQs a node uses
>   * @dev: pointer to device tree node
>   */

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before probe and defer if needed

[Grant Likely]

On Mon, 21 Apr 2014 08:47:41 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [140418 16:04]:
> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> >> > device tree as there's nothing making sure the resources for a driver
> >> > are set up before the driver probes. And we've been unable to fix just
> >> > this issue alone for about six months now. It's also broken beyond that.
> >> > It's called of_platform_bus yet it won't even pass the platform_data
> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> >>
> >> Isn't there a much simpler solution to the platform device IRQ problem?
> >>
> >> Rather than trying to fix it at the point where the resources are
> >> created, why not just *not* have DT create the IRQ resources in the
> >> first place, and instead have platform_get_irq() (which is the function
> >> which should be used to get an IRQ) be the actor to do whatever is
> >> necessary to return the IRQ(s) ?
> >
> > Yeah why not. I don't see why we would need to do all this of_* special
> > trickery for much anything beyond parsing the binding.
> 
> That can work, but it will still need something like
> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
> not.
> 
> You could also go in the other direction and don't create the device
> until the resources can be resolved. Unlike any of the other
> solutions, that would work for amba bus as well although we may never
> have a case where we need this with the amba bus. This would require
> making of_platform_populate be callable multiple times, but there are
> already some other reasons for wanting to do that. Specifically, I
> would like the core code to call of_platform_populate with default
> options and then only platforms with non-default options need a call
> to of_platform_populate.

No it wouldn't. It would require of_platform_populate to do the right
thing internally and know when to create devices from nodes that get
flagged by calling of_platform_populate().

> >> Yes, I know we have some drivers which use platform_get_resources() with
> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> >> who just dereference the resource array directly... get what's coming
> >> (though of course they have to be fixed.)
> >
> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> > 179
> 
> Certainly, this is worthwhile clean-up no matter what the solution.

Yes, and it works no matter what the data source is; platform_data, dt,
acpi, and works on any platform.

g.