On 02/28/2013 06:17 AM, Javier Martinez Canillas wrote:

On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter [off-list ref] wrote:

quoted

On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:

[snip]

quoted

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 :-)

quoted

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