| Message ID | CABxcv=m3PhTXi=dRGPm_02-Edt_jOSCeVnTVKo_NiJkCzcKSew@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/23/2014 10:24 AM, Javier Martinez Canillas wrote: > Hello Nishanth and Tony, > > On Wed, Apr 23, 2014 at 3:30 AM, Nishanth Menon <nm@ti.com> wrote: >> On 01:08-20140423, Javier Martinez Canillas wrote: >> [...] >>>> on AM335x-sk: >>>>> So this makes only am335x-sk to fail with the gpiolib irpchip >>>>> conversion as reported by Peter and you. >>>>> >>>>> Do you know what special use of GPIO have this board to behave >>>>> differently than the other boards? I've looked at the DTS but didn't >>>>> find anything evident. >>>> >>>> I could not find anything interesting yet. Peter did mention that >>>> reverting d04b76626e94 helped make the platform boot fine. I am trying >>>> to add a few prints and see which specific line does things go bad.. >>>> and if so why.. unfortunately, I am using the board remotely as well.. >>>> Will try to track this down in the next few hours and post back. >>>> >>> >>> Great, thanks a lot for your help and sorry for the inconvenience! >> >> Yep - If I am correct, and as we all suspected, GPIO0_7 which controls >> the VTT regulator for DDR is getting configured as input, instead of >> output. >> http://slexy.org/raw/s2gReMRZI6 (with diff: >> http://slexy.org/view/s20nueCE8H - ignore many of the prints as I was >> trying to truncate and isolate - had to switch to non-relaxed versions >> of writes to force sequencing with barriers to trace it down..) >> >> >> Anyways, the key log is [0]: >> gpiochip_irq_map -> calls >> irq_set_irq_type(irq, chip->irq_default_type); >> which inturn triggers: gpio-omap.c's gpio_irq_type >> in this, logic: >> if (!LINE_USED(bank->mod_usage, offset)) is invoked prior to >> setting the input type for the GPIO 0_7 (you can see the logic >> walks through setting every GPIO to input!). >> >> The original usage as far as I could discern was that this function was >> only called after probe got completed as the gpio requests were >> triggered. > > Thanks a lot for figuring out what was going on here. I think that is > not correct for gpiochip_irq_map() to call this function. After all > creating a mapping doesn't mean that the GPIO is actually used as an > IRQ. > >> >> There are probably many hacks possible, but a cleaner solution might >> involve gpio_irqchip knowing when to set the type and knowing which gpios >> not to mess with. >> >> Example hack: >> Since we call gpiochip_irqchip_add with IRQ_TYPE_NONE, >> in gpio-omap's gpio_irq_type could do: >> if (type == IRQ_TYPE_NONE) >> return 0; >> boots, http://slexy.org/raw/s24M8lHqZX - but ofcourse, there'd be other >> side effects I have'nt thought through.. > > Linus, what do you think of the following patch? > > From ede333e85e0320d32e8c2d123560808ed7e43ece Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Date: Wed, 23 Apr 2014 09:13:54 +0200 > Subject: [PATCH 1/1] gpio: don't call irq_set_irq_type() on IRQ domain map > function > > Creating a mapping for a GPIO pin into the Linux virtual IRQ > space does not necessarily mean that the GPIO is going to be > used as an interrupt line, it only means that it could be used. > > So, calling irq_set_irq_type() is not correct since that is > already done either when a driver calls request_irq() or when > the OF core calls of_irq_to_resource() because a device node > defined a GPIO controller phandle as its "interrupt-parent". > > In either case irq_set_irq_type() will be called and the GPIO > controller driver will take any required action for an IRQ. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/gpio/gpiolib.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index c12fe9d..b493781 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1402,7 +1402,6 @@ static int gpiochip_irq_map(struct irq_domain > *d, unsigned int irq, > #else > irq_set_noprobe(irq); > #endif > - irq_set_irq_type(irq, chip->irq_default_type); > > return 0; > } > Thanks, this makes am335x-evmsk boot with linux-next. Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 23, 2014 at 9:24 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > Linus, what do you think of the following patch? > > From ede333e85e0320d32e8c2d123560808ed7e43ece Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Date: Wed, 23 Apr 2014 09:13:54 +0200 > Subject: [PATCH 1/1] gpio: don't call irq_set_irq_type() on IRQ domain map > function (...) So no setting a default type in the mapping function... > - irq_set_irq_type(irq, chip->irq_default_type); But there are drivers exploiting this to set up the hardware to some default state :-( What about this: if (chip->irq_default_type != IRQ_TYPE_NONE) irq_set_irq_type(irq, chip->irq_default_type); This way you can pass IRQ_TYPE_NONE and nothing happens in the mapping. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/23/2014 08:01 AM, Linus Walleij wrote: > On Wed, Apr 23, 2014 at 9:24 AM, Javier Martinez Canillas > <javier@dowhile0.org> wrote: > >> Linus, what do you think of the following patch? >> >> From ede333e85e0320d32e8c2d123560808ed7e43ece Mon Sep 17 00:00:00 2001 >> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Date: Wed, 23 Apr 2014 09:13:54 +0200 >> Subject: [PATCH 1/1] gpio: don't call irq_set_irq_type() on IRQ domain map >> function > (...) > > So no setting a default type in the mapping function... > >> - irq_set_irq_type(irq, chip->irq_default_type); > > But there are drivers exploiting this to set up the hardware to some > default state :-( > > What about this: > > if (chip->irq_default_type != IRQ_TYPE_NONE) > irq_set_irq_type(irq, chip->irq_default_type); > > This way you can pass IRQ_TYPE_NONE and nothing happens in > the mapping. What if these drivers depend on IRQ_TYPE_NONE to do something for the GPIO pins? would you expect these drivers to pass IRQ_TYPE_DEFAULT? OR I wonder if we could pass some flag like -1 for platforms that dont care?
On Wed, Apr 23, 2014 at 3:29 PM, Nishanth Menon <nm@ti.com> wrote: > On 04/23/2014 08:01 AM, Linus Walleij wrote: >> What about this: >> >> if (chip->irq_default_type != IRQ_TYPE_NONE) >> irq_set_irq_type(irq, chip->irq_default_type); >> >> This way you can pass IRQ_TYPE_NONE and nothing happens in >> the mapping. > > What if these drivers depend on IRQ_TYPE_NONE to do something for the > GPIO pins? Yeah :-( > would you expect these drivers to pass IRQ_TYPE_DEFAULT? Actually that sounds like a good idea. Maybe we can go over the few drivers that pass IRQ_TYPE_NONE and see what they actually want. There are not *too* many users of this call yet. > OR I wonder > if we could pass some flag like -1 for platforms that dont care? The flags parameter to gpiochip_irqchip_add() is unsigned... Switching to IRQ_TYPE_DEFAULT for drivers that want this is likely the sane thing to do. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index c12fe9d..b493781 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1402,7 +1402,6 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, #else irq_set_noprobe(irq); #endif - irq_set_irq_type(irq, chip->irq_default_type); return 0;