Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter <jon-hunter@ti.com>]

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.

> 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. 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