| Message ID | 512E93B9.8020806@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. The real problem is irq_flags to be 0 instead of the value defined on the second cell of the "interrupts" property. when irq_domain_xlate_onetwocell() is called for the ethernet GPIO-IRQ I see that both the cells size and the second cell with the flag values are set correctly (2 and IRQF_TRIGGER_LOW). But even when gpio_irq_type() succeeds it seems that the struct resource IRQ flags passed to the smsc911x driver is still overwritten or not set correctly since flags & IRQF_TRIGGER_MASK is always 0. > In general we do need to fix the gpio binding for omap to default to > #interrupt-cell = <2>, as this should work. However, before we can do > that we need to fix the issue of ensuring the gpio module is enabled if > being used as an interrupt source without having to call gpio_request() > first. > Indeed, although I still wonder why the flags are not set correctly for the smsc911x driver. I had only tested it with this device so I don't know if this is a general issue of IORESOURCE_IRQ structs not been initialized correctly when using GPIOs as IRQ on OMAP or if is only related to this driver. > We should probably add the following patch as well to avoid any hangs if > the bank is not enabled, when omap_irq_type is called. > > commit 5e298de564e09f5ca4148a9bc0ed5d16b4742f14 > Author: Jon Hunter <jon-hunter@ti.com> > Date: Wed Feb 27 17:14:11 2013 -0600 > > gpio/omap: warn if gpio bank is not enabled on setting irq type > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index f1fbedb2..cbdc796 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d, > unsigned type) > int retval; > unsigned long flags; > > + if (WARN_ON(!bank->mod_usage)) > + return -EINVAL; > + > #ifdef CONFIG_ARCH_OMAP1 > if (d->irq > IH_MPUIO_BASE) > gpio = OMAP_MPUIO(d->irq - IH_MPUIO_BASE); > > > Cheers > Jon > > Thanks a lot for your help and best regards, Javier
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
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index f1fbedb2..cbdc796 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type) int retval; unsigned long flags; + if (WARN_ON(!bank->mod_usage)) + return -EINVAL; + #ifdef CONFIG_ARCH_OMAP1 if (d->irq > IH_MPUIO_BASE)