omap: IORESOURCE_IRQ flags not set when defining a GPIO-IRQ from DT

omap: IORESOURCE_IRQ flags not set when defining a GPIO-IRQ from DT

[Javier Martinez Canillas]

Hi Jon,

NOTE: I'm changing $subject to something more relevant to stop adding
noise on the original thread.

On Thu, Feb 28, 2013 at 9:49 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 02/28/2013 06:17 AM, Javier Martinez Canillas wrote:
>> On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:
>>>
>>> [snip]
>>>
>>>> Something like that would definitely solve the GPIO request issue but
>>>> we still have the issue that the current OMAP GPIO controller binding
>>>> does not support #interrupt-cells = <2>.
>>>>
>>>> So, we can't pass the trigger type and level flags for an IRQ-GPIO
>>>> when using an GPIO controller as the interrupt-parent for a device
>>>> node.
>>>>
>>>> Do you have any comments on that issue?
>>>
>>> Can you elaborate a bit more on why you say this is not supported?
>>>
>>> I have been playing with this today on an omap board and if I set the
>>> #interrupt-cells = <2>, then I do see that irq_domain_xlate_onetwocell()
>>> is called and the irq number and flags read as expected. Following which
>>> I then see it will call the omap_irq_type() to set type. So AFAICT it works.
>>>
>>
>> Yes, it does.
>>
>> I (wrongly) assumed that it was not working since the GPIO OMAP
>> binding documentation says that #interrupt-cells should be <2> but all
>> OMAP2+ DTs use <1> instead. Also, when trying to change to <2> I had
>> the kernel hang.
>>
>> But it was indeed that the GPIO bank was not enabled before calling
>> gpio_irq_type() and this made the kernel to hang. Your patch fixed the
>> issue and allowed me to find the cause.
>>
>> The problem was that when using the DT hack of defining the GPIO in
>> the ethernet chip regulator,  the calls to
>> irq_domain_xlate_onetwocell() and gpio_irq_type() were made before the
>> call to gpio_request_one() so the GPIO bank was not enabled.
>>
>> If gpio_request() is called in gpmc_probe_dt(), then the GPIO bank is
>> enabled and gpio_irq_type() succeeds.
>>
>> So, it was just me being stupid and don't understanding the implementation.
>
> No problem. Glad we are on the same page :-)
>
>>> Please note I do see that when the SMC driver calls request_irq() in
>>> smc_drv_probe() it is also settings the trigger type to
>>> IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level
>>> sensitive in DT, then this is being overwritten. We could fix this by
>>> setting SMC_IRQ_FLAGS to -1 for OMAP.
>>>
>>
>> Please note that I'm using a SMSC 911x chip and not a SMSC 91x, so the
>> driver is not smc91x but smc911x. It has the same behaviour though
>> (IRQ flags overwritten somehow), just to be sure that we are on the
>> same page.
>>
>> I don't know if just setting SMC_IRQ_FLAGS to -1 will be enough to fix
>> the smc91x since request_irq() is just passing whatever value is in
>> irq_flags.
>>
>> By looking at the two drivers (smc91x and smsc911x) it seems that the
>> only difference on how they manage the flags is that smc91x does:
>>
>> unsigned long irq_flags = SMC_IRQ_FLAGS;
>> ...
>>        if (irq_flags == -1 || ires->flags & IRQF_TRIGGER_MASK)
>>                 irq_flags = ires->flags & IRQF_TRIGGER_MASK;
>>
>> while smsc911x driver's probe function uses the flags from the
>> resource unconditionally:
>>
>> irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
>>
>> So, at the end both will set irq_flags to whatever is on the
>> IORESOURCE_IRQ struct resource flags member.
>
> Actually, that's not the case for smc91x. By default SMC_IRQ_FLAGS != -1
> (for omap) and so it will not set irq_flags to ires->flags &
> IRQF_TRIGGER_MASK. However, if I force irq_flags to be -1, then I see
> that irq_flags are to 0.
>

Yes, I meant that the behaviour is the same if you define
SMC_IRQ_FLAGS to -1 for omap.

>> The real problem is irq_flags to be 0 instead of the value defined on
>> the second cell of the "interrupts" property.
>
> So the resource flags for each irq is set in
> of_irq_to_resource() which just does ...
>
>         r->start = r->end = irq;
>         r->flags = IORESOURCE_IRQ;
>         r->name = name ? name : dev->full_name;
>
>
> of_irq_to_resource() calls irq_to_parse_and_map() which eventually just
> calls irq_set_irq_type() but type/flags is not returned and not populated.
>
> I am wondering if this is intentional.

Indeed, I was wondering the same.

>The irq_type is being correctly
> configured by irq_set_irq_type() when of_irq_to_resource() is called. In
> the smc driver, if irq_flags are 0, then when request_irq() is called
> the trigger type will not be set again (which is ok). __setup_irq has
> the following ...
>
>         /* Setup the type (level, edge polarity) if configured: */
>         if (new->flags & IRQF_TRIGGER_MASK) {
>                 ret = __irq_set_trigger(desc, irq,
>                         new->flags & IRQF_TRIGGER_MASK);
>
> Cheers
> Jon

I'll try to take a deeper look to this later next week.

Thanks a lot for your help!

Regards,
Javier

Re: omap: IORESOURCE_IRQ flags not set when defining a GPIO-IRQ from DT

[Russell King - ARM Linux]

On Fri, Mar 01, 2013 at 05:17:57PM +0100, Javier Martinez Canillas wrote:
> >> unsigned long irq_flags = SMC_IRQ_FLAGS;
> >> ...
> >>        if (irq_flags == -1 || ires->flags & IRQF_TRIGGER_MASK)
> >>                 irq_flags = ires->flags & IRQF_TRIGGER_MASK;
> >>
> >> while smsc911x driver's probe function uses the flags from the
> >> resource unconditionally:
> >>
> >> irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
> >>
> >> So, at the end both will set irq_flags to whatever is on the
> >> IORESOURCE_IRQ struct resource flags member.
> >
> > Actually, that's not the case for smc91x. By default SMC_IRQ_FLAGS != -1
> > (for omap) and so it will not set irq_flags to ires->flags &
> > IRQF_TRIGGER_MASK. However, if I force irq_flags to be -1, then I see
> > that irq_flags are to 0.

smc91x is complicated by the fact that it started off life before there
was any possibility to pass IRQ flags through resources.  So we ended
up with smc91x.h containing _lots_ of platform specific data, and the
driver could only be built for one platform.

I fixed that by sorting out this IRQ passing method, and changing smc91x
to support both the fixed configuration, and the dynamic-through-IRQflags
method.

There is no reason for any other driver to support the fixed method; that
would be a completely backwards step.

Re: omap: IORESOURCE_IRQ flags not set when defining a GPIO-IRQ from DT

[Jon Hunter]

On 03/01/2013 11:30 AM, Russell King - ARM Linux wrote:
> On Fri, Mar 01, 2013 at 05:17:57PM +0100, Javier Martinez Canillas wrote:
>>>> unsigned long irq_flags = SMC_IRQ_FLAGS;
>>>> ...
>>>>        if (irq_flags == -1 || ires->flags & IRQF_TRIGGER_MASK)
>>>>                 irq_flags = ires->flags & IRQF_TRIGGER_MASK;
>>>>
>>>> while smsc911x driver's probe function uses the flags from the
>>>> resource unconditionally:
>>>>
>>>> irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
>>>>
>>>> So, at the end both will set irq_flags to whatever is on the
>>>> IORESOURCE_IRQ struct resource flags member.
>>>
>>> Actually, that's not the case for smc91x. By default SMC_IRQ_FLAGS != -1
>>> (for omap) and so it will not set irq_flags to ires->flags &
>>> IRQF_TRIGGER_MASK. However, if I force irq_flags to be -1, then I see
>>> that irq_flags are to 0.
> 
> smc91x is complicated by the fact that it started off life before there
> was any possibility to pass IRQ flags through resources.  So we ended
> up with smc91x.h containing _lots_ of platform specific data, and the
> driver could only be built for one platform.
> 
> I fixed that by sorting out this IRQ passing method, and changing smc91x
> to support both the fixed configuration, and the dynamic-through-IRQflags
> method.
> 
> There is no reason for any other driver to support the fixed method; that
> would be a completely backwards step.

Thanks for the history. For OMAP I see SMC_IRQ_FLAGS getting defined as
follows in smc91x.h ...

#ifndef SMC_IRQ_FLAGS
#define SMC_IRQ_FLAGS           IRQF_TRIGGER_RISING
#endif

And so for OMAP devices using smc91x, it is always being configured as
rising-edge. So it would be good to move OMAP to use a dynamic
configuration too.

Cheers
Jon

Re: omap: IORESOURCE_IRQ flags not set when defining a GPIO-IRQ from DT

[Russell King - ARM Linux]

On Fri, Mar 01, 2013 at 04:41:37PM -0600, Jon Hunter wrote:
> Thanks for the history. For OMAP I see SMC_IRQ_FLAGS getting defined as
> follows in smc91x.h ...
> 
> #ifndef SMC_IRQ_FLAGS
> #define SMC_IRQ_FLAGS           IRQF_TRIGGER_RISING
> #endif
> 
> And so for OMAP devices using smc91x, it is always being configured as
> rising-edge. So it would be good to move OMAP to use a dynamic
> configuration too.

Yep.  I think that was requested back when I did the work from those
which remained...