Skip to contentSkip to conversation

I2C OF IRQ parsing issue due to probe ordering

PATCH Laurent Pinchart 4 participants 12 messages Oct 25, 2014 – Oct 30, 2014

Subsystems (6)
GPIO I2C subsystem host drivers Device tree I2C +2 more subsystems
lore
Share & tools
lore

Laurent Pinchart

author
#0 lore raw

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.

drivers/i2c/i2c-core.c +8 -2
index 2f90ac6a7f79..73ff1e7d2382 100644
@@ -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;
 		}
 	}
include/linux/of_irq.h +1 -1
index bfec136a6d1e..81a14d89a5bd 100644
@@ -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,
kernel/irq/irqdomain.c +2 -2
index 6534ff6ce02e..386fd09f4c85 100644
@@ -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 */
Signature · 3 lines
Regards,

Laurent Pinchart
#1 lore raw
Quoted 7 lines

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

author
#2 lore raw

Hi Wolfram and Thierry,

Quoted 9 lines · On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
Quoted 6 lines

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.

Quoted 10 lines

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 ?

Signature · 3 lines
Regards,

Laurent Pinchart
#3 lore raw
Quoted 21 lines · Laurent Pinchart

Hi Wolfram and Thierry,

Quoted 9 lines · On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
Quoted 6 lines

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.

Quoted 6 lines

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?

Quoted 6 lines
Quoted 4 lines

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

author
#4 lore raw

Hi Thierry,

Quoted 36 lines · On Thursday 30 October 2014 14:02:09 Thierry Reding wrote:
Quoted 20 lines · Laurent Pinchart
Quoted 11 lines · On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 6 lines
Quoted 4 lines

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:

(quoted diff) +1 -1
-		int irq = irq_of_parse_and_map(dev->of_node, 0);
+		int irq = of_irq_get(dev->of_node, 0);
Quoted 1 line

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.

Quoted 4 lines

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.

Signature · 3 lines
Regards,

Laurent Pinchart
#5 lore raw
In reply to Wolfram Sang
Quoted 14 lines · Wolfram Sang
Quoted 7 lines

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)

Quoted 7 lines

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

author
#6 lore raw

Hi Thierry,

Quoted 17 lines · On Thursday 30 October 2014 13:56:46 Thierry Reding wrote:
Quoted 16 lines · Wolfram Sang
Quoted 6 lines

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?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 10 lines

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 ?

Signature · 3 lines
Regards,

Laurent Pinchart
#7 lore raw
Quoted 9 lines
Quoted 6 lines

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.

author
#8 lore raw

Hi Wolfram,

Quoted 10 lines · On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote:
Quoted 7 lines
Quoted 4 lines

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.

Signature · 3 lines
Regards,

Laurent Pinchart
#9 lore raw
Quoted 8 lines · Laurent Pinchart

Hi Wolfram,

Quoted 5 lines · On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote:
Quoted 5 lines
Quoted 4 lines

See:
9ec36cafe43b of/irq: do irq resolution in platform_get_irq

I suspect a similar thing could be done for I2C.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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

#10 lore raw
In reply to Laurent Pinchart

Hi Laurent,

Quoted 31 lines · Laurent Pinchart

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 {
                ...
        };
Quoted 8 lines

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?

Signature · 2 lines
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
author
#11 lore raw

Hi Ezequiel,

Quoted 39 lines · On Thursday 30 October 2014 08:58:28 Ezequiel Garcia wrote:
Quoted 38 lines · Laurent Pinchart

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.

Quoted 8 lines
Quoted 1 line

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.

Signature · 3 lines
Regards,

Laurent Pinchart