On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
<[email protected]> wrote:
> The goal of this patch is to allow drivers to be probed even if at the time of
> the DT parsing some of their 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 <[email protected]>
> ---
> 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
* Rob Herring <[email protected]> [140411 10:33]:
> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
> <[email protected]> wrote:
> > The goal of this patch is to allow drivers to be probed even if at the time of
> > the DT parsing some of their 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
On Fri, Apr 18, 2014 at 3:52 PM, Tony Lindgren <[email protected]> wrote:
> * Rob Herring <[email protected]> [140411 10:33]:
>> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
>> <[email protected]> wrote:
>> > The goal of this patch is to allow drivers to be probed even if at the time of
>> > the DT parsing some of their 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
* Rob Herring <[email protected]> [140418 14:39]:
> On Fri, Apr 18, 2014 at 3:52 PM, Tony Lindgren <[email protected]> wrote:
> > * Rob Herring <[email protected]> [140411 10:33]:
> >> On Fri, Mar 21, 2014 at 9:46 AM, Jean-Jacques Hiblot
> >> <[email protected]> wrote:
> >> > The goal of this patch is to allow drivers to be probed even if at the time of
> >> > the DT parsing some of their 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
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.
* Russell King - ARM Linux <[email protected]> [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
On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
> * Russell King - ARM Linux <[email protected]> [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
* Rob Herring <[email protected]> [140421 06:47]:
> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
> > * Russell King - ARM Linux <[email protected]> [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
On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <[email protected]> wrote:
> * Rob Herring <[email protected]> [140421 06:47]:
>> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
>> > * Russell King - ARM Linux <[email protected]> [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
*/
* Rob Herring <[email protected]> [140421 12:01]:
> On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <[email protected]> wrote:
> > * Rob Herring <[email protected]> [140421 06:47]:
> >> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
> >> > * Russell King - ARM Linux <[email protected]> [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
* Tony Lindgren <[email protected]> [140421 13:26]:
> * Rob Herring <[email protected]> [140421 12:01]:
> > On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <[email protected]> wrote:
> > > * Rob Herring <[email protected]> [140421 06:47]:
> > >> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
> > >> > * Russell King - ARM Linux <[email protected]> [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
* Tony Lindgren <[email protected]> [140421 20:06]:
> * Tony Lindgren <[email protected]> [140421 13:26]:
> > * Rob Herring <[email protected]> [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 <[email protected]>
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 <[email protected]>. 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 <[email protected]>
--- 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);
On Mon, 21 Apr 2014 08:47:41 -0500, Rob Herring <[email protected]> wrote:
> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <[email protected]> wrote:
> > * Russell King - ARM Linux <[email protected]> [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.
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.
On Mon, Apr 21, 2014 at 11:57 PM, Tony Lindgren <[email protected]> wrote:
> * Tony Lindgren <[email protected]> [140421 20:06]:
>> * Tony Lindgren <[email protected]> [140421 13:26]:
>> > * Rob Herring <[email protected]> [140421 12:01]:
[...]
> 8< ------------------
> From: Tony Lindgren <[email protected]>
> 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 <[email protected]>. 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 <[email protected]>
>
> --- 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++;
> + }