of/irq: store IRQ trigger/level in struct resource flags

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

A commonly used variant is two cell on which #interrupt-cells = <2> and the first cell defines the index of the interrupt in the controller and the second cell is used to specify any of the following flags:

- bits[3:0] trigger type and level flags
1 = low-to-high edge triggered
2 = high-to-low edge triggered
4 = active high level-sensitive
8 = active low level-sensitive

An example of an interrupt controller which use the two cell format is the OMAP GPIO controller that allows GPIO lines to be used as IRQ (Documentation/devicetree/bindings/gpio/gpio-omap.txt)

But setting #interrupt-cells = <2> on the OMAP GPIO device node and specifying the GPIO-IRQ type and level flags on the second cell does not store this value on the populated IORESOURCE_IRQ struct resource.

This is because when using an IRQ from an interrupt controller and setting both cells (e.g:)

	interrupt-parent = <&gpio6>;
	interrupts = <16 8>;

A call to of_irq_to_resource() is made and this function calls to irq_of_parse_and_map_type() to get the virtual IRQ mapped to the real index for this interrupt controller. This IRQ number is populated on the struct resource:

int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)

{
	int irq = irq_of_parse_and_map(dev, index);
	..
	r->start = r->end = irq;
}

irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to the correct xlate function handler according to "#interrupt-cells" (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to irq_set_irq_type() to set the IRQ type.

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

Quoted 4 lines · Rob Herring

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

...

Quoted 6 lines

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 3 lines

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)

Quoted 12 lines · Stephen Warren

Quoted 4 lines · Rob Herring

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

...

Quoted 6 lines

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 8 lines

Quoted 3 lines

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)

Hi Stephen,

I'm glad you agree :-)

I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for the IRQ with irqd_get_trigger_type() but I prefer $subject because:

a) This works in the non-DT case with board files and filling the resources from platform data in arch/arm/mach-omap2/gpmc-smsc911x.c. So this is definitely a bug on the DT core.

b) I don't see why of_irq_to_resource() should discard the type/level flags when filling the IORESOURCE_IRQ if it was specified on the DT.

c) We will have to change all drivers that expect to get the IRQ type flags from a IORESOURCE_IRQ struct resource.

Quoted 24 lines · Javier Martinez Canillas

Quoted 12 lines · Stephen Warren

Quoted 4 lines · Rob Herring

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

...

Quoted 6 lines

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 8 lines

Quoted 3 lines

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)

Hi Stephen,

Quoted 4 lines

I'm glad you agree :-)

I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for the IRQ with irqd_get_trigger_type() but I prefer $subject because:

irqd_get_trigger_type probably is not meant for outside of irqchips. Creating an irq_get_irq_type function which takes an irq number would be the right function as that does not expose struct irq_data.

Quoted 3 lines

a) This works in the non-DT case with board files and filling the resources from platform data in arch/arm/mach-omap2/gpmc-smsc911x.c. So this is definitely a bug on the DT core.

And hackery/abuse like this:

arch/arm/mach-omap2/board-3630sdp.c:32: .flags =
GPMC_MUX_ADD_DATA | IORESOURCE_IRQ_LOWLEVEL,

Quoted 3 lines

b) I don't see why of_irq_to_resource() should discard the type/level flags when filling the IORESOURCE_IRQ if it was specified on the DT.

Quoted 2 lines

c) We will have to change all drivers that expect to get the IRQ type flags from a IORESOURCE_IRQ struct resource.

I'm not convinced that is a high number of drivers. Nearly all the occurrences of IORESOURCE_IRQ_ in drivers/ are for ISA (acpi/pnp) and drivers for ISA devices.

Rob

Quoted 30 lines · Rob Herring

Quoted 28 lines · Javier Martinez Canillas

Quoted 12 lines · Stephen Warren

Quoted 4 lines · Rob Herring

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

...

Quoted 6 lines

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 8 lines

Quoted 3 lines

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)

Hi Stephen,

I'm glad you agree :-)

I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for the IRQ with irqd_get_trigger_type() but I prefer $subject because:

Quoted 4 lines

irqd_get_trigger_type probably is not meant for outside of irqchips. Creating an irq_get_irq_type function which takes an irq number would be the right function as that does not expose struct irq_data.

Ok, I can add an irqd_get_trigger_type() that just return the flags to

Quoted 24 lines · Javier Martinez Canillas

Quoted 12 lines · Stephen Warren

Quoted 4 lines · Rob Herring

Quoted 3 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

...

Quoted 6 lines

Quoted 5 lines

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Quoted 8 lines

Quoted 3 lines

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)

Hi Stephen,

Quoted 4 lines

I'm glad you agree :-)

I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for the IRQ with irqd_get_trigger_type() but I prefer $subject because:

irqd_get_trigger_type probably is not meant for outside of irqchips. Creating an irq_get_irq_type function which takes an irq number would be the right function as that does not expose struct irq_data.

Quoted 48 lines · Javier Martinez Canillas

According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt.

A commonly used variant is two cell on which #interrupt-cells = <2> and the first cell defines the index of the interrupt in the controller and the second cell is used to specify any of the following flags:

- bits[3:0] trigger type and level flags
1 = low-to-high edge triggered
2 = high-to-low edge triggered
4 = active high level-sensitive
8 = active low level-sensitive

An example of an interrupt controller which use the two cell format is the OMAP GPIO controller that allows GPIO lines to be used as IRQ (Documentation/devicetree/bindings/gpio/gpio-omap.txt)

But setting #interrupt-cells = <2> on the OMAP GPIO device node and specifying the GPIO-IRQ type and level flags on the second cell does not store this value on the populated IORESOURCE_IRQ struct resource.

This is because when using an IRQ from an interrupt controller and setting both cells (e.g:)

	interrupt-parent = <&gpio6>;
	interrupts = <16 8>;

A call to of_irq_to_resource() is made and this function calls to irq_of_parse_and_map_type() to get the virtual IRQ mapped to the real index for this interrupt controller. This IRQ number is populated on the struct resource:

int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)

{
	int irq = irq_of_parse_and_map(dev, index);
	..
	r->start = r->end = irq;
}

irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to the correct xlate function handler according to "#interrupt-cells" (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to irq_set_irq_type() to set the IRQ type.

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

Quoted 2 lines

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses.

Rob

Quoted 8 lines · Rob Herring

Quoted 2 lines · Javier Martinez Canillas

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ.

The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ.

Quoted 3 lines

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses.

Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core.

It does this:

irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

Since of_irq_to_resource() doesn't fill the trigger/level flags on the IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property.

A previous discussion about this can be found here [1].

Quoted 30 lines · Javier Martinez Canillas

Quoted 8 lines · Rob Herring

Quoted 2 lines · Javier Martinez Canillas

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ.

The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ.

Quoted 3 lines

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses.

Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core.

It does this:

Quoted 8 lines

irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

Since of_irq_to_resource() doesn't fill the trigger/level flags on the IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property.

A previous discussion about this can be found here [1].

I can't remember if there was ever a reason for not returning the IRQ flags, but I don't have any major objection to doing so if drivers find them useful. The one concern I do have however is if it will cause any problems with drivers that expect flags == IORESOURCE_IRQ without any additional flags. Any users doing that are buggy anyway, but I do want to be careful about breakage.

I'll go over your patch and reply with comments.

g.

Quoted 28 lines · Javier Martinez Canillas

Quoted 8 lines · Rob Herring

Quoted 2 lines · Javier Martinez Canillas

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ.

The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ.

Quoted 3 lines

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses.

Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core.

Quoted 8 lines

It does this:

irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

Since of_irq_to_resource() doesn't fill the trigger/level flags on the IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property.

Quoted 29 lines · Grant Likely

Quoted 28 lines · Javier Martinez Canillas

Quoted 8 lines · Rob Herring

Quoted 2 lines · Javier Martinez Canillas

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this?

Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ.

The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ.

Quoted 3 lines

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses.

Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core.

Quoted 8 lines

Quoted 8 lines

It does this:

irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

Since of_irq_to_resource() doesn't fill the trigger/level flags on the IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property.

why do you need to known this in you driver this need to be handle in the irq chip

I does use irq gpio without at all

Best Regards,
J.

Quoted 1 line · Grant Likely

On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas

Quoted 37 lines

Quoted 37 lines · Rob Herring

Quoted 10 lines · Javier Martinez Canillas

Quoted 3 lines

This means that drivers that need the IRQ type/level flags defined in
the DT won't be able to get it.

But the interrupt controllers that need the information should be able
to get to it via irqd_get_trigger_type. What problem exactly are you
trying to fix? What driver would use this?

Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ.

The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ.

Quoted 3 lines

My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are
ISA specific and therefore should not be used on non-ISA buses.

Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core.

It does this:

irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

Quoted 5 lines

Quoted 5 lines

Since of_irq_to_resource() doesn't fill the trigger/level flags on the
IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property.

why do you need to known this in you driver this need to be handle in the irq chip

irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to the correct xlate function handler according to "#interrupt-cells" (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to irq_set_irq_type() to set the IRQ type.

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Signed-off-by Javier Martinez Canillas <[email protected]>

On Fri, 5 Apr 2013 09:48:08 +0200, Javier Martinez Canillas <[email protected]> wrote:
[...]

Quoted 12 lines

irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to the correct xlate function handler according to "#interrupt-cells" (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to irq_set_irq_type() to set the IRQ type.

But the type is never returned so it can't be saved on the IRQ struct resource flags member.

This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it.

Signed-off-by Javier Martinez Canillas <[email protected]>

[...]

include/linux/of_irq.h extern int of_irq_map_one(struct device_node *device, int index, · line 72

Quoted 12 lines

include/linux/of_irq.h +4 -0

index 535cecf..98aec57 100644
@@ -66,6 +66,10 @@ extern int of_irq_map_one(struct device_node *device, int index,
 extern unsigned int irq_create_of_mapping(struct device_node *controller,
 					  const u32 *intspec,
 					  unsigned int intsize);
+extern unsigned int irq_create_of_mapping_type(struct device_node *controller,
+					       const u32 *intspec,
+					       unsigned int intsize,
+					       unsigned int *otype);

I count 11 users of irq_create_of_mapping(). That's a managable number to update.

Instead of creating a new function, please modify the existing one and split it off into a separate patch.

Otherwise the patch looks fine to me.

g.