| Message ID | 2287003.09eTeKUr1V@avalon |
|---|---|
| State | Not Applicable |
| Headers | show |
> The i2c@e6520000 node is probed before the gpio@e6051000 node. The > of_i2c_register_devices() function tries to register all children, including > hdmi@39. It tries to parse and map the I2C client IRQ by calling > irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't > probed yet. The adv7511 driver later probes the hdmi@39 device and gets > client->irq set to 0. I've got this strange feeling of deja vu... Ah, here: Thierry Reding tackled this problem a year ago. His series: https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference resolution) He did a V2 (which never made it to the i2c list). Seems like the first two patches made it and the rest got stalled without discussion? https://lkml.org/lkml/2013/9/18/216 Adding Thierry to the queue. Maybe he can bring some light to what happened to his series. Regards, Wolfram
Hi Laurent, On 10/25/2014 07:13 PM, Laurent Pinchart wrote: > Hello, > > I recently ran into an issue with the OF IRQ parsing code in the I2C core > (of_i2c_register_devices in drivers/i2c/i2c-core.c). > > My DT contains the following nodes. > > gpio1: gpio@e6051000 { > ... > #interrupt-cells = <2>; > interrupt-controller; > clocks = <&mstp9_clks R8A7790_CLK_GPIO1>; > }; > > iic2: i2c@e6520000 { > #address-cells = <1>; > #size-cells = <0>; > ... > hdmi@39 { > compatible = "adi,adv7511w"; > reg = <0x39>; > interrupt-parent = <&gpio1>; > interrupts = <15 IRQ_TYPE_EDGE_FALLING>; > ... > }; > }; > > mstp9_clks: mstp9_clks@e6150994 { > ... > }; > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > of_i2c_register_devices() function tries to register all children, including > hdmi@39. It tries to parse and map the I2C client IRQ by calling > irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't > probed yet. The adv7511 driver later probes the hdmi@39 device and gets > client->irq set to 0. > > We can't control the probe order. Maybe I'm missing something, but I think your i2c adapter is probed with a subsys_initcall (as many other adapters). Otherwise, I can't see why it would be probed before the the gpio controller. I think this initcall is your problem. Have you tried just using platform_driver's probe?
Hi Ezequiel, On Thursday 30 October 2014 08:58:28 Ezequiel Garcia wrote: > On 10/25/2014 07:13 PM, Laurent Pinchart wrote: > > Hello, > > > > I recently ran into an issue with the OF IRQ parsing code in the I2C core > > (of_i2c_register_devices in drivers/i2c/i2c-core.c). > > > > My DT contains the following nodes. > > > > gpio1: gpio@e6051000 { > > ... > > #interrupt-cells = <2>; > > interrupt-controller; > > clocks = <&mstp9_clks R8A7790_CLK_GPIO1>; > > }; > > > > iic2: i2c@e6520000 { > > #address-cells = <1>; > > #size-cells = <0>; > > ... > > hdmi@39 { > > compatible = "adi,adv7511w"; > > reg = <0x39>; > > interrupt-parent = <&gpio1>; > > interrupts = <15 IRQ_TYPE_EDGE_FALLING>; > > ... > > }; > > }; > > > > mstp9_clks: mstp9_clks@e6150994 { > > ... > > }; > > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > of_i2c_register_devices() function tries to register all children, > > including hdmi@39. It tries to parse and map the I2C client IRQ by > > calling irq_of_parse_and_map(), which returns 0 as the interrupt > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39 > > device and gets client->irq set to 0. > > > > We can't control the probe order. > > Maybe I'm missing something, but I think your i2c adapter is probed with > a subsys_initcall (as many other adapters). Otherwise, I can't see why > it would be probed before the the gpio controller. > > I think this initcall is your problem. Have you tried just using > platform_driver's probe? My I2C controller driver uses module_platform_driver(). The reason why the GPIO controller is probed later is because the GPIO requires a clock, and the clock device is probed after the I2C controller, resulting in a deferred probing the first time the GPIO controller is probed. In the general case probe ordering can't be controlled, especially with DT. I believe we thus need a generic solution.
Hi Wolfram and Thierry, On Monday 27 October 2014 13:58:19 Wolfram Sang wrote: > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > of_i2c_register_devices() function tries to register all children, > > including hdmi@39. It tries to parse and map the I2C client IRQ by > > calling irq_of_parse_and_map(), which returns 0 as the interrupt > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39 > > device and gets client->irq set to 0. > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding > tackled this problem a year ago. His series: Thanks for the pointer. > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference > resolution) > > He did a V2 (which never made it to the i2c list). Seems like the first > two patches made it and the rest got stalled without discussion? > > https://lkml.org/lkml/2013/9/18/216 > > Adding Thierry to the queue. Maybe he can bring some light to what > happened to his series. That's exactly what I need :-) Thierry, do you plan to respin the series ?
On Mon, Oct 27, 2014 at 01:58:19PM +0100, Wolfram Sang wrote: > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > of_i2c_register_devices() function tries to register all children, including > > hdmi@39. It tries to parse and map the I2C client IRQ by calling > > irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't > > probed yet. The adv7511 driver later probes the hdmi@39 device and gets > > client->irq set to 0. > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding > tackled this problem a year ago. His series: > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference > resolution) > > He did a V2 (which never made it to the i2c list). Seems like the first > two patches made it and the rest got stalled without discussion? > > https://lkml.org/lkml/2013/9/18/216 > > Adding Thierry to the queue. Maybe he can bring some light to what > happened to his series. I tried to fix it in a proper way, but it seems people were uneasy with how invasive the change was. At some point I lost interest. People ended up merging something that was similar, but side-stepped the issue of propagating error codes all the way up by introducing a new API and in case of of_irq_parse_one() failing doing an additional check to see if the reason was the missing IRQ domain. See: 9ec36cafe43b of/irq: do irq resolution in platform_get_irq I suspect a similar thing could be done for I2C. Thierry
On Thu, Oct 30, 2014 at 02:53:42PM +0200, Laurent Pinchart wrote: > Hi Wolfram and Thierry, > > On Monday 27 October 2014 13:58:19 Wolfram Sang wrote: > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > > of_i2c_register_devices() function tries to register all children, > > > including hdmi@39. It tries to parse and map the I2C client IRQ by > > > calling irq_of_parse_and_map(), which returns 0 as the interrupt > > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39 > > > device and gets client->irq set to 0. > > > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding > > tackled this problem a year ago. His series: > > Thanks for the pointer. > > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference > > resolution) > > > > He did a V2 (which never made it to the i2c list). Seems like the first > > two patches made it and the rest got stalled without discussion? > > > > https://lkml.org/lkml/2013/9/18/216 > > > > Adding Thierry to the queue. Maybe he can bring some light to what > > happened to his series. > > That's exactly what I need :-) Thierry, do you plan to respin the series ? Not really. Like I said in my reply to Wolfram, a different set of patches was merged subsequently to solve this issue for platform devices, which makes my patchset mostly obsolete. But I think the solution that you proposed would work well if you do: - int irq = irq_of_parse_and_map(dev->of_node, 0); + int irq = of_irq_get(dev->of_node, 0); And then changing irq_create_of_mapping() should no longer be necessary. I still think it's kind of lame to handle -EPROBE_DEFER specially as was done in of_irq_get(), but given how worried people were about the more invasive changes needed to propagate the correct error code all the way up it seems like that's as good as it's going to get. Thierry
Hi Thierry, On Thursday 30 October 2014 13:56:46 Thierry Reding wrote: > On Mon, Oct 27, 2014 at 01:58:19PM +0100, Wolfram Sang wrote: > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > > of_i2c_register_devices() function tries to register all children, > > > including hdmi@39. It tries to parse and map the I2C client IRQ by > > > calling irq_of_parse_and_map(), which returns 0 as the interrupt > > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39 > > > device and gets client->irq set to 0. > > > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding > > tackled this problem a year ago. His series: > > > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference > > resolution) > > > > He did a V2 (which never made it to the i2c list). Seems like the first > > two patches made it and the rest got stalled without discussion? > > > > https://lkml.org/lkml/2013/9/18/216 > > > > Adding Thierry to the queue. Maybe he can bring some light to what > > happened to his series. > > I tried to fix it in a proper way, but it seems people were uneasy with > how invasive the change was. It was a bit invasive indeed and I can share the uneasiness, but on the other hand there was no real nack. I think I still prefer your approach, but can live with something simpler. > At some point I lost interest. People ended up merging something that was > similar, but side-stepped the issue of propagating error codes all the way > up by introducing a new API and in case of of_irq_parse_one() failing doing > an additional check to see if the reason was the missing IRQ domain. > > See: > > 9ec36cafe43b of/irq: do irq resolution in platform_get_irq > > I suspect a similar thing could be done for I2C. That could work. We would need to introduce a new i2c_get_irq() function though. Wolfram, would you be fine with that ?
Hi Thierry, On Thursday 30 October 2014 14:02:09 Thierry Reding wrote: > On Thu, Oct 30, 2014 at 02:53:42PM +0200, Laurent Pinchart wrote: > > On Monday 27 October 2014 13:58:19 Wolfram Sang wrote: > > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The > > > > of_i2c_register_devices() function tries to register all children, > > > > including hdmi@39. It tries to parse and map the I2C client IRQ by > > > > calling irq_of_parse_and_map(), which returns 0 as the interrupt > > > > controller isn't probed yet. The adv7511 driver later probes the > > > > hdmi@39 > > > > device and gets client->irq set to 0. > > > > > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding > > > > > tackled this problem a year ago. His series: > > Thanks for the pointer. > > > > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference > > > resolution) > > > > > > He did a V2 (which never made it to the i2c list). Seems like the first > > > two patches made it and the rest got stalled without discussion? > > > > > > https://lkml.org/lkml/2013/9/18/216 > > > > > > Adding Thierry to the queue. Maybe he can bring some light to what > > > happened to his series. > > > > That's exactly what I need :-) Thierry, do you plan to respin the series ? > > Not really. Like I said in my reply to Wolfram, a different set of > patches was merged subsequently to solve this issue for platform > devices, which makes my patchset mostly obsolete. But I think the > solution that you proposed would work well if you do: > > - int irq = irq_of_parse_and_map(dev->of_node, 0); > + int irq = of_irq_get(dev->of_node, 0); > > And then changing irq_create_of_mapping() should no longer be necessary. It looks like it would work, yes. I'll test it and will resubmit my patch. > I still think it's kind of lame to handle -EPROBE_DEFER specially as was > done in of_irq_get(), but given how worried people were about the more > invasive changes needed to propagate the correct error code all the way > up it seems like that's as good as it's going to get.
> > See: > > > > 9ec36cafe43b of/irq: do irq resolution in platform_get_irq > > > > I suspect a similar thing could be done for I2C. > > That could work. We would need to introduce a new i2c_get_irq() function > though. Wolfram, would you be fine with that ? I'd think it will look pretty similar to platform_get_irq, no? That is fine with me.
Hi Wolfram, On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote: > > > See: > > > 9ec36cafe43b of/irq: do irq resolution in platform_get_irq > > > > > > I suspect a similar thing could be done for I2C. > > > > That could work. We would need to introduce a new i2c_get_irq() function > > though. Wolfram, would you be fine with that ? > > I'd think it will look pretty similar to platform_get_irq, no? That is > fine with me. It would, but as Thierry pointed out it should be possible to hide the details in the I2C core. I'll submit a patch shortly.
On Thu, Oct 30, 2014 at 03:22:49PM +0200, Laurent Pinchart wrote: > Hi Wolfram, > > On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote: > > > > See: > > > > 9ec36cafe43b of/irq: do irq resolution in platform_get_irq > > > > > > > > I suspect a similar thing could be done for I2C. > > > > > > That could work. We would need to introduce a new i2c_get_irq() function > > > though. Wolfram, would you be fine with that ? > > > > I'd think it will look pretty similar to platform_get_irq, no? That is > > fine with me. > > It would, but as Thierry pointed out it should be possible to hide the details > in the I2C core. I'll submit a patch shortly. i2c_device_probe() seems exactly the right place to do this. It's in fact the equivalent of what I had proposed in my original patch series where this was done in platform_drv_probe(). Thierry
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 2f90ac6a7f79..73ff1e7d2382 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -631,6 +631,14 @@ static int i2c_device_probe(struct device *dev) if (!client) return 0; + if (!client->irq && dev->of_node) { + int irq = irq_of_parse_and_map(dev->of_node, 0); + if (irq < 0) + return irq; + + client->irq = irq; + } + driver = to_i2c_driver(dev->driver); if (!driver->probe || !driver->id_table) return -ENODEV; @@ -1409,7 +1417,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap) continue; } - info.irq = irq_of_parse_and_map(node, 0); info.of_node = of_node_get(node); info.archdata = &dev_ad; @@ -1423,7 +1430,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap) dev_err(&adap->dev, "of_i2c: Failure registering %s\n", node->full_name); of_node_put(node); - irq_dispose_mapping(info.irq); continue; } } diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h index bfec136a6d1e..81a14d89a5bd 100644 --- a/include/linux/of_irq.h +++ b/include/linux/of_irq.h @@ -34,7 +34,7 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index, extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq); extern int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_args *out_irq); -extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data); +extern int irq_create_of_mapping(struct of_phandle_args *irq_data); extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r); extern int of_irq_to_resource_table(struct device_node *dev, diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 6534ff6ce02e..386fd09f4c85 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -466,7 +466,7 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base, } EXPORT_SYMBOL_GPL(irq_create_strict_mappings); -unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) +int irq_create_of_mapping(struct of_phandle_args *irq_data) { struct irq_domain *domain; irq_hw_number_t hwirq; @@ -477,7 +477,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) if (!domain) { pr_warn("no irq domain found for %s !\n", of_node_full_name(irq_data->np)); - return 0; + return -EPROBE_DEFER; } /* If domain has no translation, then we assume interrupt line */
Hello, I recently ran into an issue with the OF IRQ parsing code in the I2C core (of_i2c_register_devices in drivers/i2c/i2c-core.c). My DT contains the following nodes. gpio1: gpio@e6051000 { ... #interrupt-cells = <2>; interrupt-controller; clocks = <&mstp9_clks R8A7790_CLK_GPIO1>; }; iic2: i2c@e6520000 { #address-cells = <1>; #size-cells = <0>; ... hdmi@39 { compatible = "adi,adv7511w"; reg = <0x39>; interrupt-parent = <&gpio1>; interrupts = <15 IRQ_TYPE_EDGE_FALLING>; ... }; }; mstp9_clks: mstp9_clks@e6150994 { ... }; The i2c@e6520000 node is probed before the gpio@e6051000 node. The of_i2c_register_devices() function tries to register all children, including hdmi@39. It tries to parse and map the I2C client IRQ by calling irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't probed yet. The adv7511 driver later probes the hdmi@39 device and gets client->irq set to 0. We can't control the probe order. One option to solve the problem would be to defer probing of the I2C client until the interrupt control is available. The following patch is a naive implementation. Before turning it into a real upstream candidate (changing the return type of irq_create_of_mapping without checking the callers is a no-go) I'd like to get feedback on the approach.