Date:	Tue, 8 Oct 2013 09:05:30 -0700
From:	Tony Lindgren <tony@...mide.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Haojian Zhuang <haojian.zhuang@...vell.com>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Jon Hunter <jgchunter@...il.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Grygorii Strashko <grygorii.strashko@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Peter Ujfalusi <peter.ujfalusi@...com>,
	Prakash Manjunathappa <prakash.pm@...com>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Benoît Cousson <bcousson@...libre.com>,
	Linux-OMAP <linux-omap@...r.kernel.org>,
	Roger Quadros <rogerq@...com>
Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <linus.walleij@...aro.org> [131008 05:18]:
> On Thu, Oct 3, 2013 at 7:42 AM, Tony Lindgren <tony@...mide.com> wrote:
> 
> > This patch adds support for interrupts in a way that
> > should be pretty generic, and works for the omaps that
> > support wake-up interrupts. On omaps, there's an
> > interrupt enable and interrupt status bit for each pin.
> 
> So to be clear: is this enable and status bit in the *same*
> register as all other settings? Then it warrants having this
> under pinctrl-single still and I feel a bit better about this.

Yes there's a status bit on most omaps in the same register
for each pin.

There is a PRM interrupt that has wake-up events for internal
domain wake-ups and io chain wake-ups. The PRM interrupt is
already a chained IRQ. After we get the wake-up interrupt we
need to check the pinctrl registers that have wake-up bit
enabled for the wake-up status bit.

> Second: how does this relate to the case where say
> gpio-omap is using the same pins as a back-end (this
> is a real usecase right)?

The GPIO is a separate hardware block on omaps and has
it's own separate wake-up events.

> So gpio-omap already supports gpio_to_irq() and
> it does not support enable_irq_wake() on the GPIO
> lines as well.

Yeah those are specific to the GPIO block, and not related
to this driver. The GPIO lines have their own wake-up
mechanism that works for retention idle.

For off-idle, only for the first GPIO bank stays powered.
The other GPIO banks get powered down in off-idle mode.
 
> How does this play together? Is it like the pins have a set
> of wakeup IRQs and then the GPIO block has *another*
> set of wakeup IRQs?

Yes that's correct, they are completely separate.
 
> And if you do enable_irq_wake() on the GPIO line,
> should this not fall through to the pinctrl driver, so we
> should add pinctrl_gpio_enable_wake() and
> pinctrl_gpio_disable_wake() just like we have
> pinctrl_gpio_direction_input() and friends today,
> so that this request falls through to the pinctrl driver
> when a wakeup on a certain GPIO line is requested?

That's not needed for most cases at this point, the GPIO
block is already handling it's internal wake-up events.

Eventually we should also handle the corner case of
GPIO wake-up line connected to a GPIO bank that's not
the first bank for off-idle. For most low-power hardware
designs that's not needed as the critial pins typically
are placed in the first GPIO bank for this reason.

But let's first figure out how we want to handle the mapping
of wake-up interrupts to device interrupts in general.

Most omap devices have a dedicated io chain wake-up line, so
that's really the key thing to get working first.
 
> Now gpio-omap.c does not seem to use the pinctrl
> back-end commands, instead of using e.g.
> pinctrl_gpio_request() and GPIO to pin ranges it
> seems to use this hack:
> 
> static void _enable_gpio_module(struct gpio_bank *bank, unsigned offset)
> {
>         if (bank->regs->pinctrl) {
>                 void __iomem *reg = bank->base + bank->regs->pinctrl;
> 
>                 /* Claim the pin for MPU */
>                 __raw_writel(__raw_readl(reg) | (1 << offset), reg);
>         }
> 
> Can't we please make the OMAP GPIO driver use the
> pinctrl back-end with gpio ranges properly *before* we proceed
> to doing this kind of stuff? I think it is already looking
> like a bit of layered hacks :-(

Heh, that's not true. And I can totally see where the confusion
comes from :)

The naming "pinctrl" in the GPIO driver is a bit confusing and
it's use predates the pinctrl framework and has been in the GPIO
driver for probably 10 years or so.

The above does not have anything to do with the pinctrl
framework or this pinctrl driver. That is to mux the GPIO pins
inside the GPIO block between the ARM core and the DSP.
 
> Haojian is already using the pinctrl-single as backend to
> another GPIO controller (I think it's pinctrl-pl061.c) so surely
> this should be possible to do right.

Sure we can make the GPIO off-idle wake-up handling automatic
for the GPIO not in the first bank, but that's really a separate
patch.
 
> IIRC there are also other OMAP GPIO blocks so we need
> to look at the large picture here, for all of them.

Sorry I don't know which other OMAP GPIO blocks you're talking
about, care to be a bit more specific?

All the omaps I've seen use the gpio-omap.c. The other GPIOs
are typically on I2C chips.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists