Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, Apr 18, 2014 at 02:37:51PM -0700, Tony Lindgren wrote: > * Thierry Reding [140411 11:40]: > > On Fri, Apr 11, 2014 at 10:20:28AM +0100, Russell King - ARM Linux wrote: > > > So what happens if a device driver probe function: > > > > > > - creates a new platform device > > > - copies the resources from the original to the new device > > > - copies the of_node from the original to the new device > > > - registers the new device > > > > > > Yes, it's broken (because it can result in the same driver being re-probed > > > by the new device) but we *do* have stuff in the kernel tree which does > > > this. > > Grr. Care to list some examples? See also if what I'm suggesting below > if that might work for the cases you're describing. Until recently, ahci_imx did exactly this, but that's been fixed now (by reworking ahci into a library.) I'm not immediately aware of any other drivers pulling this trick. -- 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Thierry Reding [140411 11:40]: > On Fri, Apr 11, 2014 at 10:20:28AM +0100, Russell King - ARM Linux wrote: > > On Thu, Apr 10, 2014 at 02:38:09PM -0700, Tony Lindgren wrote: > > > 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. > > > > > > Note that at least currently we cannot dynamically allocate the resources > > > as bus > > > specific code may add legacy resources with > > > platform_device_add_resources() > > > before the driver probe. At least omap_device_alloc() currently relies on > > > num_resources to determine if legacy resources should be added. Some of > > > these > > > will clear automatically when mach-omap2 boots with DT only, but there are > > > probably other places too where platform_device_add_resources() modifies > > > things before driver probe. > > > > > > This patch was discussed quite a bit earlier, but so far it seems we don't > > > have any better options to fix the problem. For the earlier discussion, > > > please see: > > > > > > https://lkml.org/lkml/2013/11/22/520 > > > > > > The addition of of_platform_probe() is based on patches posted earlier by > > > Thierry Reding . > > > > > > Signed-off-by: Tony Lindgren > > > > So what happens if a device driver probe function: > > > > - creates a new platform device > > - copies the resources from the original to the new device > > - copies the of_node from the original to the new device > > - registers the new device > > > > Yes, it's broken (because it can result in the same driver being re-probed > > by the new device) but we *do* have stuff in the kernel tree which does > > this. Grr. Care to list some examples? See also if what I'm suggesting below if that might work for the cases you're describing. > From what I can tell the only clean solution would be to allow the OF > functions to properly propagate errors. My earlier attempt was exactly > that, but was deemed too invasive. Frankly, I think sprinkling new of_* functions all over the subsystems is going to be a never ending task to try to fix this and other similar issues. For a long term solution it makes sense to not probe the driver at all until all it's resources are in place. > But that doesn't really solve the case you describe above either. So I > think the only good generic solution would be for all resources to be > resolved by the driver's .probe() function so that resources aren't > "cached" in the device node. For the other resources than interrupts I think what Russell describes can be worked around by keeping the initial populating of the resources except for the interrupts. Then we can just overwrite the resources in of_device_resource_populate(). No idea which drivers Russell refers to above on the copied devices. But presumably the copied device does not need interrupts without a driver probe? Regards, Tony 8< -- From: Tony Lindgren Date: Fri, 11 Apr 2014 07:52:00 -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+0x1
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Thierry Reding [140411 11:46]: > On Thu, Apr 10, 2014 at 07:29:36PM -0500, Rob Herring wrote: > > On Thu, Apr 10, 2014 at 4:38 PM, Tony Lindgren wrote: > > > 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. > > > > > > Note that at least currently we cannot dynamically allocate the resources > > > as bus > > > specific code may add legacy resources with > > > platform_device_add_resources() > > > before the driver probe. At least omap_device_alloc() currently relies on > > > num_resources to determine if legacy resources should be added. Some of > > > these > > > will clear automatically when mach-omap2 boots with DT only, but there are > > > probably other places too where platform_device_add_resources() modifies > > > things before driver probe. > > > > > > This patch was discussed quite a bit earlier, but so far it seems we don't > > > have any better options to fix the problem. For the earlier discussion, > > > please see: > > > > > > https://lkml.org/lkml/2013/11/22/520 > > > > There is a newer solution here which Grant seemed happier with: > > > > http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html > > I wonder why Grant seems to be happier with that solution than with my > original proposal. That new solution does essentially the same thing. > One of the main issues raised during review of my original proposal was > that it had to modify the core, but this new solution does that as well. > > Another thing that people weren't happy about was that my solution was > more intrusive because it required a bunch of changes to the of_irq_*() > helpers to make them propagate a proper error code. The new solution > doesn't do that, but instead works around the lack of proper error > propagation by trying to find an IRQ domain (which the of_irq_*() > helpers will do anyway). Yeah the problem I see with the patches above is that there's nothing wrong with the irq subsystem. Replied along those lines to the thread above. Cheers, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Thu, Apr 10, 2014 at 07:29:36PM -0500, Rob Herring wrote: > On Thu, Apr 10, 2014 at 4:38 PM, Tony Lindgren wrote: > > 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. > > > > Note that at least currently we cannot dynamically allocate the resources > > as bus > > specific code may add legacy resources with platform_device_add_resources() > > before the driver probe. At least omap_device_alloc() currently relies on > > num_resources to determine if legacy resources should be added. Some of > > these > > will clear automatically when mach-omap2 boots with DT only, but there are > > probably other places too where platform_device_add_resources() modifies > > things before driver probe. > > > > This patch was discussed quite a bit earlier, but so far it seems we don't > > have any better options to fix the problem. For the earlier discussion, > > please see: > > > > https://lkml.org/lkml/2013/11/22/520 > > There is a newer solution here which Grant seemed happier with: > > http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html I wonder why Grant seems to be happier with that solution than with my original proposal. That new solution does essentially the same thing. One of the main issues raised during review of my original proposal was that it had to modify the core, but this new solution does that as well. Another thing that people weren't happy about was that my solution was more intrusive because it required a bunch of changes to the of_irq_*() helpers to make them propagate a proper error code. The new solution doesn't do that, but instead works around the lack of proper error propagation by trying to find an IRQ domain (which the of_irq_*() helpers will do anyway). Thierry pgpVAMYWEpnPz.pgp Description: PGP signature
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, Apr 11, 2014 at 10:20:28AM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 10, 2014 at 02:38:09PM -0700, Tony Lindgren wrote: > > 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. > > > > Note that at least currently we cannot dynamically allocate the resources > > as bus > > specific code may add legacy resources with platform_device_add_resources() > > before the driver probe. At least omap_device_alloc() currently relies on > > num_resources to determine if legacy resources should be added. Some of > > these > > will clear automatically when mach-omap2 boots with DT only, but there are > > probably other places too where platform_device_add_resources() modifies > > things before driver probe. > > > > This patch was discussed quite a bit earlier, but so far it seems we don't > > have any better options to fix the problem. For the earlier discussion, > > please see: > > > > https://lkml.org/lkml/2013/11/22/520 > > > > The addition of of_platform_probe() is based on patches posted earlier by > > Thierry Reding . > > > > Signed-off-by: Tony Lindgren > > So what happens if a device driver probe function: > > - creates a new platform device > - copies the resources from the original to the new device > - copies the of_node from the original to the new device > - registers the new device > > Yes, it's broken (because it can result in the same driver being re-probed > by the new device) but we *do* have stuff in the kernel tree which does > this. From what I can tell the only clean solution would be to allow the OF functions to properly propagate errors. My earlier attempt was exactly that, but was deemed too invasive. But that doesn't really solve the case you describe above either. So I think the only good generic solution would be for all resources to be resolved by the driver's .probe() function so that resources aren't "cached" in the device node. Thierry pgp_WH5fq5wcI.pgp Description: PGP signature
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Thu, Apr 10, 2014 at 02:38:09PM -0700, Tony Lindgren wrote: > 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. > > Note that at least currently we cannot dynamically allocate the resources as > bus > specific code may add legacy resources with platform_device_add_resources() > before the driver probe. At least omap_device_alloc() currently relies on > num_resources to determine if legacy resources should be added. Some of these > will clear automatically when mach-omap2 boots with DT only, but there are > probably other places too where platform_device_add_resources() modifies > things before driver probe. > > This patch was discussed quite a bit earlier, but so far it seems we don't > have any better options to fix the problem. For the earlier discussion, > please see: > > https://lkml.org/lkml/2013/11/22/520 > > The addition of of_platform_probe() is based on patches posted earlier by > Thierry Reding . > > Signed-off-by: Tony Lindgren So what happens if a device driver probe function: - creates a new platform device - copies the resources from the original to the new device - copies the of_node from the original to the new device - registers the new device Yes, it's broken (because it can result in the same driver being re-probed by the new device) but we *do* have stuff in the kernel tree which does this. -- 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Thu, Apr 10, 2014 at 4:38 PM, Tony Lindgren wrote: > 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. > > Note that at least currently we cannot dynamically allocate the resources as > bus > specific code may add legacy resources with platform_device_add_resources() > before the driver probe. At least omap_device_alloc() currently relies on > num_resources to determine if legacy resources should be added. Some of these > will clear automatically when mach-omap2 boots with DT only, but there are > probably other places too where platform_device_add_resources() modifies > things before driver probe. > > This patch was discussed quite a bit earlier, but so far it seems we don't > have any better options to fix the problem. For the earlier discussion, > please see: > > https://lkml.org/lkml/2013/11/22/520 There is a newer solution here which Grant seemed happier with: http://lkml.iu.edu/hypermail/linux/kernel/1403.2/03666.html Does it work for you? > The addition of of_platform_probe() is based on patches posted earlier by > Thierry Reding . I fail to see how this patch does EPROBE_DEFER and solves the problem. Don't you need Thierry's other patches for that? I'm probably missing something. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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.
Note that at least currently we cannot dynamically allocate the resources as bus
specific code may add legacy resources with platform_device_add_resources()
before the driver probe. At least omap_device_alloc() currently relies on
num_resources to determine if legacy resources should be added. Some of these
will clear automatically when mach-omap2 boots with DT only, but there are
probably other places too where platform_device_add_resources() modifies
things before driver probe.
This patch was discussed quite a bit earlier, but so far it seems we don't
have any better options to fix the problem. For the earlier discussion,
please see:
https://lkml.org/lkml/2013/11/22/520
The addition of of_platform_probe() is based on patches posted earlier by
Thierry Reding .
Signed-off-by: Tony Lindgren
---
Greg, this is still broken, can you please pick this up?
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -483,6 +483,10 @@ static int platform_drv_probe(struct device *_dev)
acpi_dev_pm_attach(_dev, true);
+ ret = of_platform_probe(dev);
+ if (ret)
+ return ret;
+
ret = drv->probe(dev);
if (ret)
acpi_dev_pm_detach(_dev, true);
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,7 +141,7 @@ 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;
+ int num_reg = 0, num_irq;
struct resource *res, temp_res;
dev = platform_device_alloc("", -1);
@@ -154,7 +154,14 @@ struct platform_device *of_device_alloc(struct device_node
*np,
num_reg++;
num_irq = of_irq_count(np);
- /* Populate the resource table */
+ /*
+* Only allocate the resources for us to use later on. Note that bus
+* specific code may also add in additional legacy resources using
+* platform_device_add_resources(), and may even rely on us allocating
+* the basic resources here to do so. So we cannot allocate the
+* resources lazily until the legacy code has been fixed to not rely
+* on allocating resources here.
+*/
if (num_irq || num_reg) {
res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
if (!res) {
@@ -164,11 +171,7 @@ struct platform_device *of_device_alloc(struct device_node
*np,
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);
+ /* See of_device_resource_populate for populating the data */
}
dev->dev.of_node = of_node_get(np);
@@ -187,6 +190,50 @@ struct platform_device *of_device_alloc(struct device_node
*np,
EXPORT_SYMBOL(of_device_alloc);
/**
+ * of_device_resource_populate - Populate device resources from device tree
+ * @dev: pointer to platform device
+ *
+ * The device interrupts are not necessarily available for all
+ * irqdomains initially so we need to populate them lazily at
+ * device probe time from of_platform_populate.
+ */
+static int of_device_resource_populate(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int rc, i, num_reg = 0, num_irq;
+ struct resource *res, temp_res;
+
+ res = pdev->resource;
+
+ /*
+* Count the io and irq resources again. Currently we cannot rely on
+* pdev->num_resources as bus specific code may have changed that
+* with platform_device_add_resources(). But the resources we allocated
+* earlier are still there and available for us to populate.
+*/
+ if (of_can_translate_address(np))
+ while (of_address_to_resource(np, num_reg, &temp_res)
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Paul Walmsley [140106 15:43]: > On Tue, 31 Dec 2013, Rob Herring wrote: > > On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley wrote: > > > On Tue, 10 Dec 2013, Paul Walmsley wrote: > > >> Is it possible to get this patch, or something similar, merged for > > >> v3.13-rc? > > >> > > >> Once something like PM is broken, it's pretty easy for other broken > > >> patches to make it into the tree, since it becomes very difficult to test > > >> without turning into a maintainer denial-of-service attack. > > > > > > Ping. Could you please provide some guidance here about what you'd need > > > to get this solved for v3.13-rc? > > > > I agree with doing the platform_get_irq modification and feel free to > > add my ack. I'm going to be offline for the next week. > > Thanks Rob! > > Tony, would you consider sending this patch upstream for v3.13-rc along > with your 37xxevm fix patches? Hmm I'd like to see a proper patch posted for platform_get_irq() by Thierry as that one is really his patch. And it also had a comment from Grant. After that I can queue it if nobody else is picking it as I have a dependency to it. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
Hi Tony, Rob,
On Tue, 31 Dec 2013, Rob Herring wrote:
> On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley wrote:
> > Hi Grant, Rob,
> >
> > On Tue, 10 Dec 2013, Paul Walmsley wrote:
> >
> >> On Sun, 24 Nov 2013, Grant Likely wrote:
> >>
> >> > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren
> >> > wrote:
> >> > > * Tony Lindgren [131122 17:16]:
> >> > > > * Tony Lindgren [131122 17:09]:
> >> > > > > * Russell King - ARM Linux [131122 16:56]:
> >> > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> >> > > > > > > + /* See of_device_resource_notify for populating
> >> > > > > > > interrupts */
> >> > > > > > > + for (i = 0; i < num_irq; i++, res++) {
> >> > > > > > > + res->flags = IORESOURCE_IRQ;
> >> > > > > > > + res->start = -EPROBE_DEFER;
> >> > > > > > > + res->end = -EPROBE_DEFER;
> >> > > > > >
> >> > > > > > NAK. Definitely a bad idea to start introducing magic values
> >> > > > > > other into
> >> > > > > > resources. Please don't do this.
> >> > > > >
> >> > > > > Do you have any better ideas on how to sort out this issue then?
> >> > > >
> >> > > > I guess we could allocate all the resources lazily here, I'll take a
> >> > > > look
> >> > > > at that.
> >> > >
> >> > > Here's a version that allocates the resources lazily with the notifier.
> >> > > Seems to boot, need to play with it a bit more though to make sure
> >> > > we're
> >> > > not overwriting resources for any legacy devices.
> >> >
> >> > Blurg. Using a notifier really feels like we don't have a good handle on
> >> > a reasonable solution yet. Basically it means we're hooking into the
> >> > driver core without /looking/ like we're hooking into the driver core. I
> >> > don't think this is any better, but I don't have a better suggestion at
> >> > the moment. :-(
> >>
> >> Unfortunately this patch, or something that accomplishes the same results,
> >> is somewhat high-priority for us on OMAP. OMAP37xx got prematurely
> >> converted to DT booting, and now dynamic power management is broken:
> >>
> >> http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2
> >>
> >> Tony writes that this patch is one of the two patches needed to get things
> >> working again:
> >>
> >> http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2
> >>
> >> Is it possible to get this patch, or something similar, merged for
> >> v3.13-rc?
> >>
> >> Once something like PM is broken, it's pretty easy for other broken
> >> patches to make it into the tree, since it becomes very difficult to test
> >> without turning into a maintainer denial-of-service attack.
> >
> > Ping. Could you please provide some guidance here about what you'd need
> > to get this solved for v3.13-rc?
>
> I agree with doing the platform_get_irq modification and feel free to
> add my ack. I'm going to be offline for the next week.
Thanks Rob!
Tony, would you consider sending this patch upstream for v3.13-rc along
with your 37xxevm fix patches?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley wrote:
> Hi Grant, Rob,
>
> On Tue, 10 Dec 2013, Paul Walmsley wrote:
>
>> On Sun, 24 Nov 2013, Grant Likely wrote:
>>
>> > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren wrote:
>> > > * Tony Lindgren [131122 17:16]:
>> > > > * Tony Lindgren [131122 17:09]:
>> > > > > * Russell King - ARM Linux [131122 16:56]:
>> > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
>> > > > > > > + /* See of_device_resource_notify for populating
>> > > > > > > interrupts */
>> > > > > > > + for (i = 0; i < num_irq; i++, res++) {
>> > > > > > > + res->flags = IORESOURCE_IRQ;
>> > > > > > > + res->start = -EPROBE_DEFER;
>> > > > > > > + res->end = -EPROBE_DEFER;
>> > > > > >
>> > > > > > NAK. Definitely a bad idea to start introducing magic values
>> > > > > > other into
>> > > > > > resources. Please don't do this.
>> > > > >
>> > > > > Do you have any better ideas on how to sort out this issue then?
>> > > >
>> > > > I guess we could allocate all the resources lazily here, I'll take a
>> > > > look
>> > > > at that.
>> > >
>> > > Here's a version that allocates the resources lazily with the notifier.
>> > > Seems to boot, need to play with it a bit more though to make sure we're
>> > > not overwriting resources for any legacy devices.
>> >
>> > Blurg. Using a notifier really feels like we don't have a good handle on
>> > a reasonable solution yet. Basically it means we're hooking into the
>> > driver core without /looking/ like we're hooking into the driver core. I
>> > don't think this is any better, but I don't have a better suggestion at
>> > the moment. :-(
>>
>> Unfortunately this patch, or something that accomplishes the same results,
>> is somewhat high-priority for us on OMAP. OMAP37xx got prematurely
>> converted to DT booting, and now dynamic power management is broken:
>>
>> http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2
>>
>> Tony writes that this patch is one of the two patches needed to get things
>> working again:
>>
>> http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2
>>
>> Is it possible to get this patch, or something similar, merged for
>> v3.13-rc?
>>
>> Once something like PM is broken, it's pretty easy for other broken
>> patches to make it into the tree, since it becomes very difficult to test
>> without turning into a maintainer denial-of-service attack.
>
> Ping. Could you please provide some guidance here about what you'd need
> to get this solved for v3.13-rc?
I agree with doing the platform_get_irq modification and feel free to
add my ack. I'm going to be offline for the next week.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
Hi Grant, Rob,
On Tue, 10 Dec 2013, Paul Walmsley wrote:
> On Sun, 24 Nov 2013, Grant Likely wrote:
>
> > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren wrote:
> > > * Tony Lindgren [131122 17:16]:
> > > > * Tony Lindgren [131122 17:09]:
> > > > > * Russell King - ARM Linux [131122 16:56]:
> > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > > > > > + /* See of_device_resource_notify for populating
> > > > > > > interrupts */
> > > > > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > > > > + res->flags = IORESOURCE_IRQ;
> > > > > > > + res->start = -EPROBE_DEFER;
> > > > > > > + res->end = -EPROBE_DEFER;
> > > > > >
> > > > > > NAK. Definitely a bad idea to start introducing magic values other
> > > > > > into
> > > > > > resources. Please don't do this.
> > > > >
> > > > > Do you have any better ideas on how to sort out this issue then?
> > > >
> > > > I guess we could allocate all the resources lazily here, I'll take a
> > > > look
> > > > at that.
> > >
> > > Here's a version that allocates the resources lazily with the notifier.
> > > Seems to boot, need to play with it a bit more though to make sure we're
> > > not overwriting resources for any legacy devices.
> >
> > Blurg. Using a notifier really feels like we don't have a good handle on
> > a reasonable solution yet. Basically it means we're hooking into the
> > driver core without /looking/ like we're hooking into the driver core. I
> > don't think this is any better, but I don't have a better suggestion at
> > the moment. :-(
>
> Unfortunately this patch, or something that accomplishes the same results,
> is somewhat high-priority for us on OMAP. OMAP37xx got prematurely
> converted to DT booting, and now dynamic power management is broken:
>
> http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2
>
> Tony writes that this patch is one of the two patches needed to get things
> working again:
>
> http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2
>
> Is it possible to get this patch, or something similar, merged for
> v3.13-rc?
>
> Once something like PM is broken, it's pretty easy for other broken
> patches to make it into the tree, since it becomes very difficult to test
> without turning into a maintainer denial-of-service attack.
Ping. Could you please provide some guidance here about what you'd need
to get this solved for v3.13-rc?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Thierry Reding [131211 07:14]: > > So how about we make the platform_get_irq() modification for starters, > so that the OMAP issues can be resolved and then turn our attention to > coming up with a more generic approach. If indeed we end up with what > you're proposing we can easily switch platform_get_irq() to use it. Yeah we should do the minimal fix for the $Subject bug for the -rc cycle so I can fix the PM test regression fix for the few omap3 platforms already made DT only. > One of the things I'm not so sure about is how to handle other types of > resources, since they'll be somewhat more specific to the type of > device. That's true also for devices other than platform devices. Let's > look at I2C devices for instance. The way to get at the IRQ number is > via the i2c_client->irq member. Yes. And BTW, there's also another related glitch where there's no easy way to configure auxdata from arch/arm/mach code for some devices. For example, consider something like this that's fairly common: 1. i2c bus driver configured with DT and can take board specific auxdata 2. i2c bus intializes connected devices as configured with DT but cannot pass board specific auxdata as the auxdata for the i2c bus is empty 3. i2c connected pmic driver intializes as configured with DT, but cannot get board specifc auxdata So we should probably add some parent or master auxdata table that's also checked if the bus specific auxdata table is empty. > device_get_irq() breaks down at that point because unless you want to > add special cases for all sorts of devices you have no way of getting at > the data if it hasn't been instantiated from DT (or ACPI). Actually, all > devices instantiated from platform data will suffer from this if I'm not > mistaken. > > If you're saying that the device_get_irq() API should only cover cases > where some sort of firmware interface exists such as DT or ACPI, then I > suppose it could be made to work and drivers could rely on the subsystem > specific locations as fallback. I guess for I2C drivers you'd have to do > something like this: > > err = device_get_irq(&client->dev, 0); > if (err >= 0) > client->irq = err; > > And then continue to use client->irq as usual. That obviously has the > side-effect of having to do this for every single driver, whereas the > original patches were meant to solve this in the core. > > Given enough time we could probably migrate everyone to the new > interfaces, but it certainly is a lot of churn. Perhaps another > alternative would be to unify this some more by making each subsystem > provide special hooks that device_get_irq() and friends can call for > fallback when none of the "firmware" functions were successful. Ideally of course we'd have Linux generic way of doing this that works for platform data, DT and ACPI without changing the existing interfaces :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Wed, Dec 11, 2013 at 01:45:53PM +, Grant Likely wrote:
> On Thu, 28 Nov 2013 16:46:23 +0100, Thierry Reding
> wrote:
> > On Wed, Nov 27, 2013 at 03:56:29PM +, Grant Likely wrote:
> > > On Mon, 25 Nov 2013 10:49:55 +0100, Thierry Reding
> > > wrote:
> > > >
> > > > I should maybe add: one issue that was raised during review of my
> > > > initial patch series was that we'll also need to cope with situations
> > > > like the following:
> > > >
> > > > 1) device's interrupt parent is probed (assigned IRQ base X)
> > > > 2) device is probed (interrupt parent there, therefore gets
> > > >assigned IRQ (X + z)
> > > > 3) device in removed
> > > > 4) device's interrupt parent is removed
> > > > 5) device is probed (deferred because interrupt parent isn't
> > > >there)
> > > > 6) device's interrupt parent is probed (assigned IRQ base Y)
> > > > 7) device is probed, gets assigned IRQ (Y + z)
> > > >
> > > > So not only do we have to track which resources are interrupt resources,
> > > > but we also need to have them reassigned everytime the device is probed,
> > > > therefore interrupt mappings need to be properly disposed and the values
> > > > invalidated when probing is deferred or the device removed.
> > >
> > > Yes, that is a problem, but the only way to handle that is to always
> > > recalcuate all resource references at probe time. I don't feel good
> > > about handling that in the core. I'd rather move drivers away from
> > > referencing the resources table directly and instead use an API. Then
> > > the resources table could be missing entirely.
> >
> > Are you suggesting something like this?
> >
> > ---
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b799f166..c894d1af3a5e 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -13,6 +13,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -87,7 +88,12 @@ 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) && dev->dev.of_node)
> > + return irq_of_parse_and_map(dev->dev.of_node, num);
> > +
> > + r = platform_get_resource(dev, IORESOURCE_IRQ, num);
>
> Yes. Or even more generically we could have a device_get_irq() function:
>
> int device_get_irq(struct device *dev, unsigned int num)
> {
> if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> return irq_of_parse_and_map(dev->of_node, num);
> /* An ACPI hook could go here */
> return 0
> }
>
> It would be callable by any device driver, and platform_get_irq() could
> call it too.
So how about we make the platform_get_irq() modification for starters,
so that the OMAP issues can be resolved and then turn our attention to
coming up with a more generic approach. If indeed we end up with what
you're proposing we can easily switch platform_get_irq() to use it.
One of the things I'm not so sure about is how to handle other types of
resources, since they'll be somewhat more specific to the type of
device. That's true also for devices other than platform devices. Let's
look at I2C devices for instance. The way to get at the IRQ number is
via the i2c_client->irq member.
device_get_irq() breaks down at that point because unless you want to
add special cases for all sorts of devices you have no way of getting at
the data if it hasn't been instantiated from DT (or ACPI). Actually, all
devices instantiated from platform data will suffer from this if I'm not
mistaken.
If you're saying that the device_get_irq() API should only cover cases
where some sort of firmware interface exists such as DT or ACPI, then I
suppose it could be made to work and drivers could rely on the subsystem
specific locations as fallback. I guess for I2C drivers you'd have to do
something like this:
err = device_get_irq(&client->dev, 0);
if (err >= 0)
client->irq = err;
And then continue to use client->irq as usual. That obviously has the
side-effect of having to do this for every single driver, whereas the
original patches were meant to solve this in the core.
Given enough time we could probably migrate everyone to the new
interfaces, but it certainly is a lot of churn. Perhaps another
alternative would be to unify this some more by making each subsystem
provide special hooks that device_get_irq() and friends can call for
fallback when none of the "firmware" functions were successful.
Thierry
pgpnunZEeyZyh.pgp
Description: PGP signature
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Thu, 28 Nov 2013 16:46:23 +0100, Thierry Reding
wrote:
> On Wed, Nov 27, 2013 at 03:56:29PM +, Grant Likely wrote:
> > On Mon, 25 Nov 2013 10:49:55 +0100, Thierry Reding
> > wrote:
> > >
> > > I should maybe add: one issue that was raised during review of my
> > > initial patch series was that we'll also need to cope with situations
> > > like the following:
> > >
> > > 1) device's interrupt parent is probed (assigned IRQ base X)
> > > 2) device is probed (interrupt parent there, therefore gets
> > > assigned IRQ (X + z)
> > > 3) device in removed
> > > 4) device's interrupt parent is removed
> > > 5) device is probed (deferred because interrupt parent isn't
> > > there)
> > > 6) device's interrupt parent is probed (assigned IRQ base Y)
> > > 7) device is probed, gets assigned IRQ (Y + z)
> > >
> > > So not only do we have to track which resources are interrupt resources,
> > > but we also need to have them reassigned everytime the device is probed,
> > > therefore interrupt mappings need to be properly disposed and the values
> > > invalidated when probing is deferred or the device removed.
> >
> > Yes, that is a problem, but the only way to handle that is to always
> > recalcuate all resource references at probe time. I don't feel good
> > about handling that in the core. I'd rather move drivers away from
> > referencing the resources table directly and instead use an API. Then
> > the resources table could be missing entirely.
>
> Are you suggesting something like this?
>
> ---
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b799f166..c894d1af3a5e 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -87,7 +88,12 @@ 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) && dev->dev.of_node)
> + return irq_of_parse_and_map(dev->dev.of_node, num);
> +
> + r = platform_get_resource(dev, IORESOURCE_IRQ, num);
Yes. Or even more generically we could have a device_get_irq() function:
int device_get_irq(struct device *dev, unsigned int num)
{
if (IS_ENABLED(CONFIG_OF) && dev->of_node)
return irq_of_parse_and_map(dev->of_node, num);
/* An ACPI hook could go here */
return 0
}
It would be callable by any device driver, and platform_get_irq() could
call it too.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
Hi Grant, Rob,
On Sun, 24 Nov 2013, Grant Likely wrote:
> On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren wrote:
> > * Tony Lindgren [131122 17:16]:
> > > * Tony Lindgren [131122 17:09]:
> > > > * Russell King - ARM Linux [131122 16:56]:
> > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > > > > + /* See of_device_resource_notify for populating
> > > > > > interrupts */
> > > > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > > > + res->flags = IORESOURCE_IRQ;
> > > > > > + res->start = -EPROBE_DEFER;
> > > > > > + res->end = -EPROBE_DEFER;
> > > > >
> > > > > NAK. Definitely a bad idea to start introducing magic values other
> > > > > into
> > > > > resources. Please don't do this.
> > > >
> > > > Do you have any better ideas on how to sort out this issue then?
> > >
> > > I guess we could allocate all the resources lazily here, I'll take a look
> > > at that.
> >
> > Here's a version that allocates the resources lazily with the notifier.
> > Seems to boot, need to play with it a bit more though to make sure we're
> > not overwriting resources for any legacy devices.
>
> Blurg. Using a notifier really feels like we don't have a good handle on
> a reasonable solution yet. Basically it means we're hooking into the
> driver core without /looking/ like we're hooking into the driver core. I
> don't think this is any better, but I don't have a better suggestion at
> the moment. :-(
Unfortunately this patch, or something that accomplishes the same results,
is somewhat high-priority for us on OMAP. OMAP37xx got prematurely
converted to DT booting, and now dynamic power management is broken:
http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2
Tony writes that this patch is one of the two patches needed to get things
working again:
http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2
Is it possible to get this patch, or something similar, merged for
v3.13-rc?
Once something like PM is broken, it's pretty easy for other broken
patches to make it into the tree, since it becomes very difficult to test
without turning into a maintainer denial-of-service attack.
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Wed, Nov 27, 2013 at 03:56:29PM +, Grant Likely wrote:
> On Mon, 25 Nov 2013 10:49:55 +0100, Thierry Reding
> wrote:
> > On Mon, Nov 25, 2013 at 10:25:50AM +0100, Thierry Reding wrote:
> > > On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote:
> > > > On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren
> > > > wrote:
> > > > > 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 !
> > > > > 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 based
> > > > > on a notifier call instead.
> > > > >
> > > > > Signed-off-by: Tony Lindgren
> > > > >
> > > > > ---
> > > > >
> > > > > Rob & Grant, care to merge this for the -rc if this looks OK to you?
> > > > >
> > > > > These happen for example when using interrupts-extended for omap
> > > > > wake-up interrupts where the irq domain is created by pinctrl-single.c
> > > > > at module_init time.
> > > > >
> > > > > --- a/drivers/of/platform.c
> > > > > +++ b/drivers/of/platform.c
> > > > > @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> > > > > dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * The device interrupts are not necessarily available for all
> > > > > + * irqdomains initially so we need to populate them using a
> > > > > + * notifier.
> > > > > + */
> > > > > +static int of_device_resource_notify(struct notifier_block *nb,
> > > > > + unsigned long event, void *dev)
> > > > > +{
> > > > > + struct platform_device *pdev = to_platform_device(dev);
> > > > > + struct device_node *np = pdev->dev.of_node;
> > > > > + struct resource *res = pdev->resource;
> > > > > + struct resource *irqr = NULL;
> > > > > + int num_irq, i, found = 0;
> > > > > +
> > > > > + if (event != BUS_NOTIFY_BIND_DRIVER)
> > > > > + return 0;
> > > > > +
> > > > > + if (!np)
> > > > > + goto out;
> > > > > +
> > > > > + num_irq = of_irq_count(np);
> > > > > + if (!num_irq)
> > > > > + goto out;
> > > > > +
> > > > > + for (i = 0; i < pdev->num_resources; i++, res++) {
> > > > > + if (res->flags != IORESOURCE_IRQ ||
> > > > > + res->start != -EPROBE_DEFER ||
> > > > > + res->end != -EPROBE_DEFER)
> > > > > + continue;
> > > > > +
> > > > > + if (!irqr)
> > > > > + irqr = res;
> > > > > + found++;
> > > > > + }
> > > > > +
> > > > > + if (!found)
> > > > > + goto out;
> > > > > +
> > > > > + if (found != num_irq) {
> > > > > + dev_WARN(dev, "error populating irq resources: %i !=
> > > > > %i\n",
> > > > > + found, num_irq);
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> > > > > +
> > > > > +out:
> > > > > + return NOTIFY_DONE;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * of_device_alloc - Allocate and initialize an of_device
> > > > > * @np: device node to assign to device
> > > > > @@ -168,7 +218,13 @@ 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);
> > > > > +
> > > > > + /* See of_device_resource_notify for populating
> > > > > interrupts */
> > > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > > + res->flags = IORESOURCE_IRQ;
> > > > > + res->start = -EPROBE_DEFER;
> > > > > + res->end = -EPROBE_DEFER;
> > > > > + }
> > > >
> > > > I actually like the idea of completely allocating the resource structure
> > > > but leaving some entries empty. However, I agree with rmk that putting
> > > > garbage
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Grant Likely [131124 13:37]: > > I actually like the idea of completely allocating the resource structure > but leaving some entries empty. However, I agree with rmk that putting > garbage into a resource structure is a bad idea. What about changing the > value of flags to 0 or some other value to be obviously an empty > property and give the follow up parsing some context about which ones it > needs to attempt to recalculate? If we want to play it safe, we should probably introduce something like this to ioport.h: +#define IORESOURCE_IRQ_DEFERRED(1<<6) Then we can populate IRQ resources initially with that. And later on during the driver probe, we know it's safe to populate the resource if res->flags & IORESOURCE_IRQ_DEFERRED. That fixes the $Subject bug, and gets us a little bit further for making more changes later on. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Mon, 25 Nov 2013 10:49:55 +0100, Thierry Reding
wrote:
> On Mon, Nov 25, 2013 at 10:25:50AM +0100, Thierry Reding wrote:
> > On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote:
> > > On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren
> > > wrote:
> > > > 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 !
> > > > 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 based
> > > > on a notifier call instead.
> > > >
> > > > Signed-off-by: Tony Lindgren
> > > >
> > > > ---
> > > >
> > > > Rob & Grant, care to merge this for the -rc if this looks OK to you?
> > > >
> > > > These happen for example when using interrupts-extended for omap
> > > > wake-up interrupts where the irq domain is created by pinctrl-single.c
> > > > at module_init time.
> > > >
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> > > > dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > > > }
> > > >
> > > > +/*
> > > > + * The device interrupts are not necessarily available for all
> > > > + * irqdomains initially so we need to populate them using a
> > > > + * notifier.
> > > > + */
> > > > +static int of_device_resource_notify(struct notifier_block *nb,
> > > > +unsigned long event, void *dev)
> > > > +{
> > > > + struct platform_device *pdev = to_platform_device(dev);
> > > > + struct device_node *np = pdev->dev.of_node;
> > > > + struct resource *res = pdev->resource;
> > > > + struct resource *irqr = NULL;
> > > > + int num_irq, i, found = 0;
> > > > +
> > > > + if (event != BUS_NOTIFY_BIND_DRIVER)
> > > > + return 0;
> > > > +
> > > > + if (!np)
> > > > + goto out;
> > > > +
> > > > + num_irq = of_irq_count(np);
> > > > + if (!num_irq)
> > > > + goto out;
> > > > +
> > > > + for (i = 0; i < pdev->num_resources; i++, res++) {
> > > > + if (res->flags != IORESOURCE_IRQ ||
> > > > + res->start != -EPROBE_DEFER ||
> > > > + res->end != -EPROBE_DEFER)
> > > > + continue;
> > > > +
> > > > + if (!irqr)
> > > > + irqr = res;
> > > > + found++;
> > > > + }
> > > > +
> > > > + if (!found)
> > > > + goto out;
> > > > +
> > > > + if (found != num_irq) {
> > > > + dev_WARN(dev, "error populating irq resources: %i !=
> > > > %i\n",
> > > > +found, num_irq);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> > > > +
> > > > +out:
> > > > + return NOTIFY_DONE;
> > > > +}
> > > > +
> > > > /**
> > > > * of_device_alloc - Allocate and initialize an of_device
> > > > * @np: device node to assign to device
> > > > @@ -168,7 +218,13 @@ 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);
> > > > +
> > > > + /* See of_device_resource_notify for populating
> > > > interrupts */
> > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > + res->flags = IORESOURCE_IRQ;
> > > > + res->start = -EPROBE_DEFER;
> > > > + res->end = -EPROBE_DEFER;
> > > > + }
> > >
> > > I actually like the idea of completely allocating the resource structure
> > > but leaving some entries empty. However, I agree with rmk that putting
> > > garbage into a resource structure is a bad idea. What about changing the
> > > value of flags to 0 or some other value to be obviously an empty
> > > property and give the follow up parsing some context about which ones it
> > > needs
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Mon, 25 Nov 2013 10:25:50 +0100, Thierry Reding
wrote:
> On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote:
> > On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren wrote:
> > > 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 !
> > > 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 based
> > > on a notifier call instead.
> > >
> > > Signed-off-by: Tony Lindgren
> > >
> > > ---
> > >
> > > Rob & Grant, care to merge this for the -rc if this looks OK to you?
> > >
> > > These happen for example when using interrupts-extended for omap
> > > wake-up interrupts where the irq domain is created by pinctrl-single.c
> > > at module_init time.
> > >
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> > > dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > > }
> > >
> > > +/*
> > > + * The device interrupts are not necessarily available for all
> > > + * irqdomains initially so we need to populate them using a
> > > + * notifier.
> > > + */
> > > +static int of_device_resource_notify(struct notifier_block *nb,
> > > + unsigned long event, void *dev)
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dev);
> > > + struct device_node *np = pdev->dev.of_node;
> > > + struct resource *res = pdev->resource;
> > > + struct resource *irqr = NULL;
> > > + int num_irq, i, found = 0;
> > > +
> > > + if (event != BUS_NOTIFY_BIND_DRIVER)
> > > + return 0;
> > > +
> > > + if (!np)
> > > + goto out;
> > > +
> > > + num_irq = of_irq_count(np);
> > > + if (!num_irq)
> > > + goto out;
> > > +
> > > + for (i = 0; i < pdev->num_resources; i++, res++) {
> > > + if (res->flags != IORESOURCE_IRQ ||
> > > + res->start != -EPROBE_DEFER ||
> > > + res->end != -EPROBE_DEFER)
> > > + continue;
> > > +
> > > + if (!irqr)
> > > + irqr = res;
> > > + found++;
> > > + }
> > > +
> > > + if (!found)
> > > + goto out;
> > > +
> > > + if (found != num_irq) {
> > > + dev_WARN(dev, "error populating irq resources: %i != %i\n",
> > > + found, num_irq);
> > > + goto out;
> > > + }
> > > +
> > > + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> > > +
> > > +out:
> > > + return NOTIFY_DONE;
> > > +}
> > > +
> > > /**
> > > * of_device_alloc - Allocate and initialize an of_device
> > > * @np: device node to assign to device
> > > @@ -168,7 +218,13 @@ 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);
> > > +
> > > + /* See of_device_resource_notify for populating interrupts */
> > > + for (i = 0; i < num_irq; i++, res++) {
> > > + res->flags = IORESOURCE_IRQ;
> > > + res->start = -EPROBE_DEFER;
> > > + res->end = -EPROBE_DEFER;
> > > + }
> >
> > I actually like the idea of completely allocating the resource structure
> > but leaving some entries empty. However, I agree with rmk that putting
> > garbage into a resource structure is a bad idea. What about changing the
> > value of flags to 0 or some other value to be obviously an empty
> > property and give the follow up parsing some context about which ones it
> > needs to attempt to recalculate?
>
> When I worked on this a while back I came to the same conclusion. It's
> nice to allocate all the resources at once, because the number of them
> doesn't change, only their actually values.
>
> However it seems to me like there's no way with the way platform_device
> is currently defined to pass along enough context to allow it to obtain
> the correct set of resources that need to be populated.
>
> We can't really set flags to 0 because then we loose all information
> about the type of resource, which is the only thing that could remotely
> b
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Thierry Reding [131125 01:51]: > On Mon, Nov 25, 2013 at 10:25:50AM +0100, Thierry Reding wrote: > > On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote: > > > > > > I actually like the idea of completely allocating the resource structure > > > but leaving some entries empty. However, I agree with rmk that putting > > > garbage into a resource structure is a bad idea. What about changing the > > > value of flags to 0 or some other value to be obviously an empty > > > property and give the follow up parsing some context about which ones it > > > needs to attempt to recalculate? > > > > When I worked on this a while back I came to the same conclusion. It's > > nice to allocate all the resources at once, because the number of them > > doesn't change, only their actually values. > > I should maybe add: one issue that was raised during review of my > initial patch series was that we'll also need to cope with situations > like the following: > > 1) device's interrupt parent is probed (assigned IRQ base X) > 2) device is probed (interrupt parent there, therefore gets > assigned IRQ (X + z) > 3) device in removed > 4) device's interrupt parent is removed > 5) device is probed (deferred because interrupt parent isn't > there) > 6) device's interrupt parent is probed (assigned IRQ base Y) > 7) device is probed, gets assigned IRQ (Y + z) > > So not only do we have to track which resources are interrupt resources, > but we also need to have them reassigned everytime the device is probed, > therefore interrupt mappings need to be properly disposed and the values > invalidated when probing is deferred or the device removed. > > Having a dynamic list of properties all of a sudden doesn't sound like > such a bad idea after all. It makes handling this kind of situation > rather trivial, especially per-type lists. Those lists will be empty at > first and populated during the first probe. When probing fails or when a > device is unloaded, we dispose the mappings and empty the lists, so that > subsequent probes will start from scratch. It certainly sounds like a > bit of a waste of CPU cycles, but on the other hand it makes the code > much simpler. Looks like we cannot yet use devm_allocate, but that seems like a nice solution in the long run. I just posted an updated patch to fix the $Subject bug for the -rc cycle to this thread with more comments regarding dynamically allocating the resources. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Thierry Reding [131125 01:36]:
> On Sat, Nov 23, 2013 at 08:32:40AM -0800, Tony Lindgren wrote:
> > * Rob Herring [131123 07:43]:
> > > On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren wrote:
> > > > * Tony Lindgren [131122 17:16]:
> > > >> * Tony Lindgren [131122 17:09]:
> > > >> > * Russell King - ARM Linux [131122 16:56]:
> > > >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > >> > > > + /* See of_device_resource_notify for populating
> > > >> > > > interrupts */
> > > >> > > > + for (i = 0; i < num_irq; i++, res++) {
> > > >> > > > + res->flags = IORESOURCE_IRQ;
> > > >> > > > + res->start = -EPROBE_DEFER;
> > > >> > > > + res->end = -EPROBE_DEFER;
> > > >> > >
> > > >> > > NAK. Definitely a bad idea to start introducing magic values
> > > >> > > other into
> > > >> > > resources. Please don't do this.
> > > >> >
> > > >> > Do you have any better ideas on how to sort out this issue then?
> > > >>
> > > >> I guess we could allocate all the resources lazily here, I'll take a
> > > >> look
> > > >> at that.
> > > >
> > > > Here's a version that allocates the resources lazily with the notifier.
> > > > Seems to boot, need to play with it a bit more though to make sure we're
> > > > not overwriting resources for any legacy devices.
> > >
> > > Have you seen Thierry's series[1]? While your approach is certainly
> > > more concise, it seems like a work-around for the problem. I don't
> > > think a notifier is the right long term solution.
> >
> > OK cool. I think we can fix the $Subject bug first without making all
> > those changes, then do the rest of the reorg for v3.14.
> >
> > The bug is that we try to populate IRQ resources at a wrong time
> > when they may not exist.
> >
> > Based on a quick look it seems we could combine Thierry's addition
> > of the new function of_platform_probe(struct platform_device *pdev)
> > and use that to allocate all resources at driver probe time like my
> > patch is doing. And then there's no need for the notifier.
>
> My series already does the allocation at probe time as well. That was
> the whole point. The reason why I added of_platform_probe() is because I
> think we'll be needing this for other types of resources in the future
> as well, so it could serve as a central place to do all of that.
Yeah, that's the way to go in the long run. However, we currently do have
some dependencies to that data being there and bus specific code may
add legacy resources to it with platform_device_add_resources(). At least
omap_device_alloc() depends on that until v3.14 when mach-omap2 is booting
in DT only mode.
> There was also a proposal[0] by Arnd a few weeks ago that solved this in
> a more generic way. I've said it before, and I'll say again that the
> idea scares me somewhat, but it does solve some interesting aspects and
> has the potential to get rid of a whole lot of boilerplate code. While
> the original purpose was to handle all things devm_*(), I suspect that
> same mechanism could be used to resolve DT references at probe time.
Yes devm_alloc() should work once the legacy dependencies are out of the
way. We would need to audit where pdev->num_resources or pdev->resource is
being relied on, and where platform_device_add_resources() is being called
before the device probe. So I doubt we can do that all as a fix for the
-rc cycle.
Below is what I think might be limited enough to fix the $Subject bug for
the -rc cycle. I took the of_platform_probe() from your patch 08/10 to
avoid the notifier. Also added Greg to Cc as it's now touching
drivers/base/platform.c too.
Seems to work for my test cases, does this work for you guys?
Regards,
Tony
> [0]: http://www.spinics.net/lists/devicetree/msg10684.html
8< -
From: Tony Lindgren
Date: Mon, 25 Nov 2013 11:12:52 -0800
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(
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Mon, Nov 25, 2013 at 10:25:50AM +0100, Thierry Reding wrote:
> On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote:
> > On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren wrote:
> > > 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 !
> > > 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 based
> > > on a notifier call instead.
> > >
> > > Signed-off-by: Tony Lindgren
> > >
> > > ---
> > >
> > > Rob & Grant, care to merge this for the -rc if this looks OK to you?
> > >
> > > These happen for example when using interrupts-extended for omap
> > > wake-up interrupts where the irq domain is created by pinctrl-single.c
> > > at module_init time.
> > >
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> > > dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > > }
> > >
> > > +/*
> > > + * The device interrupts are not necessarily available for all
> > > + * irqdomains initially so we need to populate them using a
> > > + * notifier.
> > > + */
> > > +static int of_device_resource_notify(struct notifier_block *nb,
> > > + unsigned long event, void *dev)
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dev);
> > > + struct device_node *np = pdev->dev.of_node;
> > > + struct resource *res = pdev->resource;
> > > + struct resource *irqr = NULL;
> > > + int num_irq, i, found = 0;
> > > +
> > > + if (event != BUS_NOTIFY_BIND_DRIVER)
> > > + return 0;
> > > +
> > > + if (!np)
> > > + goto out;
> > > +
> > > + num_irq = of_irq_count(np);
> > > + if (!num_irq)
> > > + goto out;
> > > +
> > > + for (i = 0; i < pdev->num_resources; i++, res++) {
> > > + if (res->flags != IORESOURCE_IRQ ||
> > > + res->start != -EPROBE_DEFER ||
> > > + res->end != -EPROBE_DEFER)
> > > + continue;
> > > +
> > > + if (!irqr)
> > > + irqr = res;
> > > + found++;
> > > + }
> > > +
> > > + if (!found)
> > > + goto out;
> > > +
> > > + if (found != num_irq) {
> > > + dev_WARN(dev, "error populating irq resources: %i != %i\n",
> > > + found, num_irq);
> > > + goto out;
> > > + }
> > > +
> > > + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> > > +
> > > +out:
> > > + return NOTIFY_DONE;
> > > +}
> > > +
> > > /**
> > > * of_device_alloc - Allocate and initialize an of_device
> > > * @np: device node to assign to device
> > > @@ -168,7 +218,13 @@ 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);
> > > +
> > > + /* See of_device_resource_notify for populating interrupts */
> > > + for (i = 0; i < num_irq; i++, res++) {
> > > + res->flags = IORESOURCE_IRQ;
> > > + res->start = -EPROBE_DEFER;
> > > + res->end = -EPROBE_DEFER;
> > > + }
> >
> > I actually like the idea of completely allocating the resource structure
> > but leaving some entries empty. However, I agree with rmk that putting
> > garbage into a resource structure is a bad idea. What about changing the
> > value of flags to 0 or some other value to be obviously an empty
> > property and give the follow up parsing some context about which ones it
> > needs to attempt to recalculate?
>
> When I worked on this a while back I came to the same conclusion. It's
> nice to allocate all the resources at once, because the number of them
> doesn't change, only their actually values.
I should maybe add: one issue that was raised during review of my
initial patch series was that we'll also need to cope with situations
like the following:
1) device's interrupt parent is probed (assigned IRQ base X)
2) device is probed (interrupt parent there, therefore gets
assigned IRQ (X + z)
3) device in removed
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Sat, Nov 23, 2013 at 08:32:40AM -0800, Tony Lindgren wrote:
> * Rob Herring [131123 07:43]:
> > On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren wrote:
> > > * Tony Lindgren [131122 17:16]:
> > >> * Tony Lindgren [131122 17:09]:
> > >> > * Russell King - ARM Linux [131122 16:56]:
> > >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > >> > > > + /* See of_device_resource_notify for populating
> > >> > > > interrupts */
> > >> > > > + for (i = 0; i < num_irq; i++, res++) {
> > >> > > > + res->flags = IORESOURCE_IRQ;
> > >> > > > + res->start = -EPROBE_DEFER;
> > >> > > > + res->end = -EPROBE_DEFER;
> > >> > >
> > >> > > NAK. Definitely a bad idea to start introducing magic values other
> > >> > > into
> > >> > > resources. Please don't do this.
> > >> >
> > >> > Do you have any better ideas on how to sort out this issue then?
> > >>
> > >> I guess we could allocate all the resources lazily here, I'll take a look
> > >> at that.
> > >
> > > Here's a version that allocates the resources lazily with the notifier.
> > > Seems to boot, need to play with it a bit more though to make sure we're
> > > not overwriting resources for any legacy devices.
> >
> > Have you seen Thierry's series[1]? While your approach is certainly
> > more concise, it seems like a work-around for the problem. I don't
> > think a notifier is the right long term solution.
>
> OK cool. I think we can fix the $Subject bug first without making all
> those changes, then do the rest of the reorg for v3.14.
>
> The bug is that we try to populate IRQ resources at a wrong time
> when they may not exist.
>
> Based on a quick look it seems we could combine Thierry's addition
> of the new function of_platform_probe(struct platform_device *pdev)
> and use that to allocate all resources at driver probe time like my
> patch is doing. And then there's no need for the notifier.
My series already does the allocation at probe time as well. That was
the whole point. The reason why I added of_platform_probe() is because I
think we'll be needing this for other types of resources in the future
as well, so it could serve as a central place to do all of that.
There was also a proposal[0] by Arnd a few weeks ago that solved this in
a more generic way. I've said it before, and I'll say again that the
idea scares me somewhat, but it does solve some interesting aspects and
has the potential to get rid of a whole lot of boilerplate code. While
the original purpose was to handle all things devm_*(), I suspect that
same mechanism could be used to resolve DT references at probe time.
Thierry
[0]: http://www.spinics.net/lists/devicetree/msg10684.html
pgpIJwxN8xh69.pgp
Description: PGP signature
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Sun, Nov 24, 2013 at 09:36:51PM +, Grant Likely wrote:
> On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren wrote:
> > 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 !
> > 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 based
> > on a notifier call instead.
> >
> > Signed-off-by: Tony Lindgren
> >
> > ---
> >
> > Rob & Grant, care to merge this for the -rc if this looks OK to you?
> >
> > These happen for example when using interrupts-extended for omap
> > wake-up interrupts where the irq domain is created by pinctrl-single.c
> > at module_init time.
> >
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> > dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > }
> >
> > +/*
> > + * The device interrupts are not necessarily available for all
> > + * irqdomains initially so we need to populate them using a
> > + * notifier.
> > + */
> > +static int of_device_resource_notify(struct notifier_block *nb,
> > +unsigned long event, void *dev)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res = pdev->resource;
> > + struct resource *irqr = NULL;
> > + int num_irq, i, found = 0;
> > +
> > + if (event != BUS_NOTIFY_BIND_DRIVER)
> > + return 0;
> > +
> > + if (!np)
> > + goto out;
> > +
> > + num_irq = of_irq_count(np);
> > + if (!num_irq)
> > + goto out;
> > +
> > + for (i = 0; i < pdev->num_resources; i++, res++) {
> > + if (res->flags != IORESOURCE_IRQ ||
> > + res->start != -EPROBE_DEFER ||
> > + res->end != -EPROBE_DEFER)
> > + continue;
> > +
> > + if (!irqr)
> > + irqr = res;
> > + found++;
> > + }
> > +
> > + if (!found)
> > + goto out;
> > +
> > + if (found != num_irq) {
> > + dev_WARN(dev, "error populating irq resources: %i != %i\n",
> > +found, num_irq);
> > + goto out;
> > + }
> > +
> > + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> > +
> > +out:
> > + return NOTIFY_DONE;
> > +}
> > +
> > /**
> > * of_device_alloc - Allocate and initialize an of_device
> > * @np: device node to assign to device
> > @@ -168,7 +218,13 @@ 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);
> > +
> > + /* See of_device_resource_notify for populating interrupts */
> > + for (i = 0; i < num_irq; i++, res++) {
> > + res->flags = IORESOURCE_IRQ;
> > + res->start = -EPROBE_DEFER;
> > + res->end = -EPROBE_DEFER;
> > + }
>
> I actually like the idea of completely allocating the resource structure
> but leaving some entries empty. However, I agree with rmk that putting
> garbage into a resource structure is a bad idea. What about changing the
> value of flags to 0 or some other value to be obviously an empty
> property and give the follow up parsing some context about which ones it
> needs to attempt to recalculate?
When I worked on this a while back I came to the same conclusion. It's
nice to allocate all the resources at once, because the number of them
doesn't change, only their actually values.
However it seems to me like there's no way with the way platform_device
is currently defined to pass along enough context to allow it to obtain
the correct set of resources that need to be populated.
We can't really set flags to 0 because then we loose all information
about the type of resource, which is the only thing that could remotely
be used to track interrupt-type resources and recalculate only those. I
was looking at perhaps modifying the platform_device struct to use a
different means of storing the resources that would make this easier.
One possibility woul
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, 22 Nov 2013 16:43:35 -0800, Tony Lindgren wrote:
> 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 !
> 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 based
> on a notifier call instead.
>
> Signed-off-by: Tony Lindgren
>
> ---
>
> Rob & Grant, care to merge this for the -rc if this looks OK to you?
>
> These happen for example when using interrupts-extended for omap
> wake-up interrupts where the irq domain is created by pinctrl-single.c
> at module_init time.
>
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
> dev_set_name(dev, "%s.%d", node->name, magic - 1);
> }
>
> +/*
> + * The device interrupts are not necessarily available for all
> + * irqdomains initially so we need to populate them using a
> + * notifier.
> + */
> +static int of_device_resource_notify(struct notifier_block *nb,
> + unsigned long event, void *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res = pdev->resource;
> + struct resource *irqr = NULL;
> + int num_irq, i, found = 0;
> +
> + if (event != BUS_NOTIFY_BIND_DRIVER)
> + return 0;
> +
> + if (!np)
> + goto out;
> +
> + num_irq = of_irq_count(np);
> + if (!num_irq)
> + goto out;
> +
> + for (i = 0; i < pdev->num_resources; i++, res++) {
> + if (res->flags != IORESOURCE_IRQ ||
> + res->start != -EPROBE_DEFER ||
> + res->end != -EPROBE_DEFER)
> + continue;
> +
> + if (!irqr)
> + irqr = res;
> + found++;
> + }
> +
> + if (!found)
> + goto out;
> +
> + if (found != num_irq) {
> + dev_WARN(dev, "error populating irq resources: %i != %i\n",
> + found, num_irq);
> + goto out;
> + }
> +
> + WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
> +
> +out:
> + return NOTIFY_DONE;
> +}
> +
> /**
> * of_device_alloc - Allocate and initialize an of_device
> * @np: device node to assign to device
> @@ -168,7 +218,13 @@ 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);
> +
> + /* See of_device_resource_notify for populating interrupts */
> + for (i = 0; i < num_irq; i++, res++) {
> + res->flags = IORESOURCE_IRQ;
> + res->start = -EPROBE_DEFER;
> + res->end = -EPROBE_DEFER;
> + }
I actually like the idea of completely allocating the resource structure
but leaving some entries empty. However, I agree with rmk that putting
garbage into a resource structure is a bad idea. What about changing the
value of flags to 0 or some other value to be obviously an empty
property and give the follow up parsing some context about which ones it
needs to attempt to recalculate?
However, I still don't like the notifier approach of actually triggering
the fixup. We need something better.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren wrote:
> * Tony Lindgren [131122 17:16]:
> > * Tony Lindgren [131122 17:09]:
> > > * Russell King - ARM Linux [131122 16:56]:
> > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > > > + /* See of_device_resource_notify for populating
> > > > > interrupts */
> > > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > > + res->flags = IORESOURCE_IRQ;
> > > > > + res->start = -EPROBE_DEFER;
> > > > > + res->end = -EPROBE_DEFER;
> > > >
> > > > NAK. Definitely a bad idea to start introducing magic values other into
> > > > resources. Please don't do this.
> > >
> > > Do you have any better ideas on how to sort out this issue then?
> >
> > I guess we could allocate all the resources lazily here, I'll take a look
> > at that.
>
> Here's a version that allocates the resources lazily with the notifier.
> Seems to boot, need to play with it a bit more though to make sure we're
> not overwriting resources for any legacy devices.
Blurg. Using a notifier really feels like we don't have a good handle on
a reasonable solution yet. Basically it means we're hooking into the
driver core without /looking/ like we're hooking into the driver core. I
don't think this is any better, but I don't have a better suggestion at
the moment. :-(
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Rob Herring [131123 07:43]:
> On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren wrote:
> > * Tony Lindgren [131122 17:16]:
> >> * Tony Lindgren [131122 17:09]:
> >> > * Russell King - ARM Linux [131122 16:56]:
> >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> >> > > > + /* See of_device_resource_notify for populating
> >> > > > interrupts */
> >> > > > + for (i = 0; i < num_irq; i++, res++) {
> >> > > > + res->flags = IORESOURCE_IRQ;
> >> > > > + res->start = -EPROBE_DEFER;
> >> > > > + res->end = -EPROBE_DEFER;
> >> > >
> >> > > NAK. Definitely a bad idea to start introducing magic values other
> >> > > into
> >> > > resources. Please don't do this.
> >> >
> >> > Do you have any better ideas on how to sort out this issue then?
> >>
> >> I guess we could allocate all the resources lazily here, I'll take a look
> >> at that.
> >
> > Here's a version that allocates the resources lazily with the notifier.
> > Seems to boot, need to play with it a bit more though to make sure we're
> > not overwriting resources for any legacy devices.
>
> Have you seen Thierry's series[1]? While your approach is certainly
> more concise, it seems like a work-around for the problem. I don't
> think a notifier is the right long term solution.
OK cool. I think we can fix the $Subject bug first without making all
those changes, then do the rest of the reorg for v3.14.
The bug is that we try to populate IRQ resources at a wrong time
when they may not exist.
Based on a quick look it seems we could combine Thierry's addition
of the new function of_platform_probe(struct platform_device *pdev)
and use that to allocate all resources at driver probe time like my
patch is doing. And then there's no need for the notifier.
Regards,
Tony
> [1] http://www.spinics.net/lists/arm-kernel/msg274110.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren wrote:
> * Tony Lindgren [131122 17:16]:
>> * Tony Lindgren [131122 17:09]:
>> > * Russell King - ARM Linux [131122 16:56]:
>> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
>> > > > + /* See of_device_resource_notify for populating
>> > > > interrupts */
>> > > > + for (i = 0; i < num_irq; i++, res++) {
>> > > > + res->flags = IORESOURCE_IRQ;
>> > > > + res->start = -EPROBE_DEFER;
>> > > > + res->end = -EPROBE_DEFER;
>> > >
>> > > NAK. Definitely a bad idea to start introducing magic values other into
>> > > resources. Please don't do this.
>> >
>> > Do you have any better ideas on how to sort out this issue then?
>>
>> I guess we could allocate all the resources lazily here, I'll take a look
>> at that.
>
> Here's a version that allocates the resources lazily with the notifier.
> Seems to boot, need to play with it a bit more though to make sure we're
> not overwriting resources for any legacy devices.
Have you seen Thierry's series[1]? While your approach is certainly
more concise, it seems like a work-around for the problem. I don't
think a notifier is the right long term solution.
Rob
[1] http://www.spinics.net/lists/arm-kernel/msg274110.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Tony Lindgren [131122 17:16]:
> * Tony Lindgren [131122 17:09]:
> > * Russell King - ARM Linux [131122 16:56]:
> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > > + /* See of_device_resource_notify for populating
> > > > interrupts */
> > > > + for (i = 0; i < num_irq; i++, res++) {
> > > > + res->flags = IORESOURCE_IRQ;
> > > > + res->start = -EPROBE_DEFER;
> > > > + res->end = -EPROBE_DEFER;
> > >
> > > NAK. Definitely a bad idea to start introducing magic values other into
> > > resources. Please don't do this.
> >
> > Do you have any better ideas on how to sort out this issue then?
>
> I guess we could allocate all the resources lazily here, I'll take a look
> at that.
Here's a version that allocates the resources lazily with the notifier.
Seems to boot, need to play with it a bit more though to make sure we're
not overwriting resources for any legacy devices.
Regards,
Tony
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,13 +141,47 @@ 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;
+ 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;
+
+ if (bus_id)
+ dev_set_name(&dev->dev, "%s", bus_id);
+ else
+ of_device_make_bus_id(&dev->dev);
+
+ /* See of_device_resource_notify for populating the resources */
+
+ return dev;
+}
+EXPORT_SYMBOL(of_device_alloc);
+
+/*
+ * The device interrupts are not necessarily available for all
+ * irqdomains initially so we need to populate them using a
+ * notifier.
+ */
+static int of_device_resource_notify(struct notifier_block *nb,
+unsigned long event, void *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct device_node *np = pdev->dev.of_node;
+ int rc, i, num_reg = 0, num_irq;
+ struct resource *res, temp_res;
+
+ if (event != BUS_NOTIFY_BIND_DRIVER)
+ return 0;
+
+ if (!np)
+ goto out;
+
/* count the io and irq resources */
if (of_can_translate_address(np))
while (of_address_to_resource(np, num_reg, &temp_res) == 0)
@@ -158,12 +192,12 @@ struct platform_device *of_device_alloc(struct
device_node *np,
if (num_irq || num_reg) {
res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
if (!res) {
- platform_device_put(dev);
- return NULL;
+ platform_device_put(pdev);
+ goto out;
}
- dev->num_resources = num_reg + num_irq;
- dev->resource = res;
+ pdev->num_resources = num_reg + num_irq;
+ pdev->resource = res;
for (i = 0; i < num_reg; i++, res++) {
rc = of_address_to_resource(np, i, res);
WARN_ON(rc);
@@ -171,20 +205,9 @@ struct platform_device *of_device_alloc(struct device_node
*np,
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;
-
- if (bus_id)
- dev_set_name(&dev->dev, "%s", bus_id);
- else
- of_device_make_bus_id(&dev->dev);
-
- return dev;
+out:
+ return NOTIFY_DONE;
}
-EXPORT_SYMBOL(of_device_alloc);
/**
* of_platform_device_create_pdata - Alloc, initialize and register an
of_device
@@ -447,6 +470,8 @@ int of_platform_bus_probe(struct device_node *root,
}
EXPORT_SYMBOL(of_platform_bus_probe);
+static struct notifier_block resource_nb;
+
/**
* of_platform_populate() - Populate platform_devices from device tree data
* @root: parent of the first level to probe or NULL for the root of the tree
@@ -478,6 +503,11 @@ int of_platform_populate(struct device_node *root,
if (!root)
return -EINVAL;
+ if (!resource_nb.notifier_call) {
+ resource_nb.notifier_call = of_device_resource_notify,
+ bus_register_notifier(&platform_bus_type, &resource_nb);
+ }
+
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent,
true);
if (rc)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Tony Lindgren [131122 17:09]:
> * Russell King - ARM Linux [131122 16:56]:
> > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > > + /* See of_device_resource_notify for populating interrupts */
> > > + for (i = 0; i < num_irq; i++, res++) {
> > > + res->flags = IORESOURCE_IRQ;
> > > + res->start = -EPROBE_DEFER;
> > > + res->end = -EPROBE_DEFER;
> >
> > NAK. Definitely a bad idea to start introducing magic values other into
> > resources. Please don't do this.
>
> Do you have any better ideas on how to sort out this issue then?
I guess we could allocate all the resources lazily here, I'll take a look
at that.
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Russell King - ARM Linux [131122 16:56]:
> On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> > + /* See of_device_resource_notify for populating interrupts */
> > + for (i = 0; i < num_irq; i++, res++) {
> > + res->flags = IORESOURCE_IRQ;
> > + res->start = -EPROBE_DEFER;
> > + res->end = -EPROBE_DEFER;
>
> NAK. Definitely a bad idea to start introducing magic values other into
> resources. Please don't do this.
Do you have any better ideas on how to sort out this issue then?
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
* Tony Lindgren [131122 16:44]:
> @@ -168,7 +218,13 @@ 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);
> +
> + /* See of_device_resource_notify for populating interrupts */
> + for (i = 0; i < num_irq; i++, res++) {
> + res->flags = IORESOURCE_IRQ;
> + res->start = -EPROBE_DEFER;
> + res->end = -EPROBE_DEFER;
> + }
Hmm actually we want to use some other value here as res->start
is not an int. Maybe some kind of hwirq_max + EPROBE_DEFER.
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of/platform: Fix no irq domain found errors when populating interrupts
On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote:
> + /* See of_device_resource_notify for populating interrupts */
> + for (i = 0; i < num_irq; i++, res++) {
> + res->flags = IORESOURCE_IRQ;
> + res->start = -EPROBE_DEFER;
> + res->end = -EPROBE_DEFER;
NAK. Definitely a bad idea to start introducing magic values other into
resources. Please don't do this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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 !
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 based
on a notifier call instead.
Signed-off-by: Tony Lindgren
---
Rob & Grant, care to merge this for the -rc if this looks OK to you?
These happen for example when using interrupts-extended for omap
wake-up interrupts where the irq domain is created by pinctrl-single.c
at module_init time.
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -130,6 +130,56 @@ void of_device_make_bus_id(struct device *dev)
dev_set_name(dev, "%s.%d", node->name, magic - 1);
}
+/*
+ * The device interrupts are not necessarily available for all
+ * irqdomains initially so we need to populate them using a
+ * notifier.
+ */
+static int of_device_resource_notify(struct notifier_block *nb,
+unsigned long event, void *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *res = pdev->resource;
+ struct resource *irqr = NULL;
+ int num_irq, i, found = 0;
+
+ if (event != BUS_NOTIFY_BIND_DRIVER)
+ return 0;
+
+ if (!np)
+ goto out;
+
+ num_irq = of_irq_count(np);
+ if (!num_irq)
+ goto out;
+
+ for (i = 0; i < pdev->num_resources; i++, res++) {
+ if (res->flags != IORESOURCE_IRQ ||
+ res->start != -EPROBE_DEFER ||
+ res->end != -EPROBE_DEFER)
+ continue;
+
+ if (!irqr)
+ irqr = res;
+ found++;
+ }
+
+ if (!found)
+ goto out;
+
+ if (found != num_irq) {
+ dev_WARN(dev, "error populating irq resources: %i != %i\n",
+found, num_irq);
+ goto out;
+ }
+
+ WARN_ON(of_irq_to_resource_table(np, irqr, num_irq) != num_irq);
+
+out:
+ return NOTIFY_DONE;
+}
+
/**
* of_device_alloc - Allocate and initialize an of_device
* @np: device node to assign to device
@@ -168,7 +218,13 @@ 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);
+
+ /* See of_device_resource_notify for populating interrupts */
+ for (i = 0; i < num_irq; i++, res++) {
+ res->flags = IORESOURCE_IRQ;
+ res->start = -EPROBE_DEFER;
+ res->end = -EPROBE_DEFER;
+ }
}
dev->dev.of_node = of_node_get(np);
@@ -447,6 +503,8 @@ int of_platform_bus_probe(struct device_node *root,
}
EXPORT_SYMBOL(of_platform_bus_probe);
+static struct notifier_block resource_nb;
+
/**
* of_platform_populate() - Populate platform_devices from device tree data
* @root: parent of the first level to probe or NULL for the root of the tree
@@ -478,6 +536,11 @@ int of_platform_populate(struct device_node *root,
if (!root)
return -EINVAL;
+ if (!resource_nb.notifier_call) {
+ resource_nb.notifier_call = of_device_resource_notify,
+ bus_register_notifier(&platform_bus_type, &resource_nb);
+ }
+
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent,
true);
if (rc)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
