[PATCH 0/2] net: smsc911x: get IRQ flags from chip if not present in IORESOURCE_IRQ

[PATCH 0/2] net: smsc911x: get IRQ flags from chip if not present in IORESOURCE_IRQ

[Javier Martinez Canillas]

Hello,

This is the second attempt to solve the issue that IRQ flags when
defined  using Device Trees, are not passed to drivers that try
to obtain them from a IORESOURCE_IRQ struct resource.

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
while 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 want to get the IRQ edge/level flags
defined in the Device Tree from a struct resource will not be able
to get it.
    
Drivers can get the IRQ flags by using irq_get_irq_data(irq) and
irqd_get_trigger_type(irq_data) but this will unnecessary expose
irq_data to callers and also is more error prone.

This problem was first discussed here [1] and a first approach to
solved was discussed here [2]. This was by saving the IRQ edge/level
flags on the struct resource but the feedback was that IORESOURCE_IRQ
is not supposed to be used with DT but with architectures supporting ACPI/PnP.
    
So, this second patch-set is composed of the following patches:

[PATCH 1/2] genirq: add function to get IRQ edge/level flags
[PATCH 2/2] net: smsc911x: get IRQ flags from chip if not present in IORESOURCE_IRQ

And adds an irq_get_trigger_type() function to obtain the edge/level
flags for an IRQ and use it as a fallback if a driver does not find
the flags on an IORESOURCE_IRQ struct resource.

Thanks a lot and best regards,
Javier

[1]: https://patchwork.kernel.org/patch/2194911/
[2]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/96908

[PATCH 1/2] genirq: add function to get IRQ edge/level flags

[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
while 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
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 want to get the IRQ edge/level flags
defined in the Device Tree from a struct resource will not be able
to get it.

Drivers can get the IRQ flags by using irq_get_irq_data(irq) and
irqd_get_trigger_type(irq_data) but this will unnecessary expose
irq_data to callers and also is more error prone.

So, is better to add an irq_get_trigger_type() function to obtain
the edge/level flags for an IRQ.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 include/linux/irq.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index bc4e066..93231cb 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -579,6 +579,12 @@ static inline struct msi_desc *irq_data_get_msi(struct irq_data *d)
 	return d->msi_desc;
 }
 
+static inline u32 irq_get_trigger_type(unsigned int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	return d ? d->state_use_accessors & IRQD_TRIGGER_MASK : 0;
+}
+
 int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 		struct module *owner);
 
-- 
1.7.7.6

[PATCH 2/2] net: smsc911x: get IRQ flags from chip if not present in IORESOURCE_IRQ

[Javier Martinez Canillas]

When defining an IRQ trigger type and level flags from a Device Tree
a call to of_irq_to_resource() is made to parse the "interrupts"
property cells and return a struct resource.

But the flags are not saved on this struct resource which means that
drivers that try to obtain this information from an IORESOURCE_IRQ
will not be able to get it.

So, is more safe to fallback and query this information from the irq
chip directly if it was not found on the struct resource.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/net/ethernet/smsc/smsc911x.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index da5cc9a..3d535b3 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2390,6 +2390,10 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 	pdata = netdev_priv(dev);
 	dev->irq = irq_res->start;
 	irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
+
+	if (!irq_flags)
+		irq_flags = irq_get_trigger_type(dev->irq);
+
 	pdata->ioaddr = ioremap_nocache(res->start, res_size);
 
 	pdata->dev = dev;
-- 
1.7.7.6

Re: [PATCH 1/2] genirq: add function to get IRQ edge/level flags

[Stephen Warren]

On 04/12/2013 12:05 PM, Javier Martinez Canillas wrote:
...
> So, is better to add an irq_get_trigger_type() function to obtain
> the edge/level flags for an IRQ.

> diff --git a/include/linux/irq.h b/include/linux/irq.h

> +static inline u32 irq_get_trigger_type(unsigned int irq)
> +{
> +	struct irq_data *d = irq_get_irq_data(irq);
> +	return d ? d->state_use_accessors & IRQD_TRIGGER_MASK : 0;

Should the direct access to d->state_use_accessors be replaced with a
call to irqd_get_trigger_type()? Perhaps since this is inside the IRQ
code header, there's no need to use the accessor function?

Re: [PATCH 1/2] genirq: add function to get IRQ edge/level flags

[Javier Martinez Canillas]

On 04/12/2013 11:56 PM, Stephen Warren wrote:
> On 04/12/2013 12:05 PM, Javier Martinez Canillas wrote:
> ...
>> So, is better to add an irq_get_trigger_type() function to obtain
>> the edge/level flags for an IRQ.
> 
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
> 
>> +static inline u32 irq_get_trigger_type(unsigned int irq)
>> +{
>> +	struct irq_data *d = irq_get_irq_data(irq);
>> +	return d ? d->state_use_accessors & IRQD_TRIGGER_MASK : 0;
> 
> Should the direct access to d->state_use_accessors be replaced with a
> call to irqd_get_trigger_type()? Perhaps since this is inside the IRQ
> code header, there's no need to use the accessor function?
> 

Yes, it's better to use irqd_get_trigger_type() instead of a direct access to
d->state_use_accessors since that function is inline so it should be the same
performance wise and it will be easier to maintain.

I'll send a v2 changing that.

Thanks a lot and best regards,
Javier