[0/3] tty: serial: 8250 introduce mctrl_gpio helpers
diff mbox

Message ID 20170728064004.GI10026@atomide.com
State New
Headers show

Commit Message

Tony Lindgren July 28, 2017, 6:40 a.m. UTC
* yegorslists@googlemail.com <yegorslists@googlemail.com> [170727 06:18]:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> This patch series aims to reintroduce the  mctrl_gpio helpers for 8250
> UARTs.
> 
> There are some UARTs that use GPIO signals as a wakeup-sourse.
> The first patch addresses this issue and tries to destinguish GPIO usage
> via searching for "wakeup-sourse" property. Though it must be decided whether
> this property is secure to use for this purpose.

The wakeup-source part you should be able to handle pretty much
out of box with Linux generic wakeirqs if configured. See for example
drivers/i2c/i2c-core.c for the dev_pm_set_dedicated_wake_irq() part.
As long as the 8250 driver has runtime PM implemented it will wake
up the 8250 device.

This should work just fine also with am335x gpios, just configure the
secondary wakeup gpio interrupt using interrupts-extended in device
tree. Typically the interrupts are named "irq" and "wakeup". And if the
pin is used as gpio, you can just dev_pm_clear_wake_irq() during
runtime.

If having issues, we're still missing the wakeirq level configuration,
the patch below should do the trick there.

Regards,

Tony

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

Comments

andriy.shevchenko@linux.intel.com July 28, 2017, 10:36 a.m. UTC | #1
On Thu, 2017-07-27 at 23:40 -0700, Tony Lindgren wrote:
> * yegorslists@googlemail.com <yegorslists@googlemail.com> [170727
> 06:18]:
> > From: Yegor Yefremov <yegorslists@googlemail.com>
> > 
> > This patch series aims to reintroduce the  mctrl_gpio helpers for
> > 8250
> > UARTs.
> > 
> > There are some UARTs that use GPIO signals as a wakeup-sourse.
> > The first patch addresses this issue and tries to destinguish GPIO
> > usage
> > via searching for "wakeup-sourse" property. Though it must be
> > decided whether
> > this property is secure to use for this purpose.
> 
> The wakeup-source part you should be able to handle pretty much
> out of box with Linux generic wakeirqs if configured. See for example
> drivers/i2c/i2c-core.c for the dev_pm_set_dedicated_wake_irq() part.

Here are 3 cases:
1) out-of-band interrupt (usually GPIO) which can be wake source;
2) in-band interrupt (have not seen this for UART, though it's supported
by tty framework for a long time AFAIU);
3) special case when Out-of-band interrupt uses the pin in UART pool of
pins.

We are talking here about case 3).

> As long as the 8250 driver has runtime PM implemented

As you remember it has not (the ugly hack which is used right now is not
correct implementation of RPM).

>  it will wake
> up the 8250 device.

See above, in case 3) we would be able to achieve that when pin mode is
switched to GPIO and back. I don't remember if OMAP uses this approach.

> This should work just fine also with am335x gpios, just configure the
> secondary wakeup gpio interrupt using interrupts-extended in device
> tree. Typically the interrupts are named "irq" and "wakeup". And if
> the
> pin is used as gpio, you can just dev_pm_clear_wake_irq() during
> runtime.

I think this one is not related to case 3).

> If having issues, we're still missing the wakeirq level configuration,
> the patch below should do the trick there.

>  	err = request_threaded_irq(irq, NULL,
> handle_threaded_wake_irq,
> -				   IRQF_ONESHOT, dev_name(dev),
> wirq);
> +				   irq_get_trigger_type(irq) |
> IRQF_ONESHOT,

This is not needed if you use DT or ACPI and framework (serial in this
case) checks for interrupt correctly.
Tony Lindgren July 28, 2017, 11:29 a.m. UTC | #2
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [170728 03:37]:
> On Thu, 2017-07-27 at 23:40 -0700, Tony Lindgren wrote:
> > * yegorslists@googlemail.com <yegorslists@googlemail.com> [170727
> > 06:18]:
> > > From: Yegor Yefremov <yegorslists@googlemail.com>
> > > 
> > > This patch series aims to reintroduce the  mctrl_gpio helpers for
> > > 8250
> > > UARTs.
> > > 
> > > There are some UARTs that use GPIO signals as a wakeup-sourse.
> > > The first patch addresses this issue and tries to destinguish GPIO
> > > usage
> > > via searching for "wakeup-sourse" property. Though it must be
> > > decided whether
> > > this property is secure to use for this purpose.
> > 
> > The wakeup-source part you should be able to handle pretty much
> > out of box with Linux generic wakeirqs if configured. See for example
> > drivers/i2c/i2c-core.c for the dev_pm_set_dedicated_wake_irq() part.
> 
> Here are 3 cases:
> 1) out-of-band interrupt (usually GPIO) which can be wake source;
> 2) in-band interrupt (have not seen this for UART, though it's supported
> by tty framework for a long time AFAIU);

And case 2) is somewhat useless as it requires the uart to be
clocked.

> 3) special case when Out-of-band interrupt uses the pin in UART pool of
> pins.
> 
> We are talking here about case 3).

OK at least for am335x/am437x, changing it's mode between gpio and
uart use can be done with pinctrl framework with named pinctrl
named modes.

> > As long as the 8250 driver has runtime PM implemented
> 
> As you remember it has not (the ugly hack which is used right now is not
> correct implementation of RPM).
> 
> >  it will wake
> > up the 8250 device.
> 
> See above, in case 3) we would be able to achieve that when pin mode is
> switched to GPIO and back. I don't remember if OMAP uses this approach.

Yes that's needed for am335x and am437x to get a wake-up event from
rx pin for example. Not needed for omap3/4/5.

> > This should work just fine also with am335x gpios, just configure the
> > secondary wakeup gpio interrupt using interrupts-extended in device
> > tree. Typically the interrupts are named "irq" and "wakeup". And if
> > the
> > pin is used as gpio, you can just dev_pm_clear_wake_irq() during
> > runtime.
> 
> I think this one is not related to case 3).

Hmm but it seems we already have that working for 8250_omap for
omap3/4/5 where rx pin provides wakeup events when 8250_omap is
in runtime suspend state. See the dev_pm_set_dedicated_wake_irq()
parts in 8250_omap.c. For am335x/am437x I think the only thing
missing is toggling between uart and gpio mode for the rx pin
using pinctrl named modes. There's an example for am335x in
omap_hsmmc.c with pinctrl_pm_select_idle_state() if that helps.

But maybe you have something in case 3) that I don't follow?

> > If having issues, we're still missing the wakeirq level configuration,
> > the patch below should do the trick there.
> 
> >  	err = request_threaded_irq(irq, NULL,
> > handle_threaded_wake_irq,
> > -				   IRQF_ONESHOT, dev_name(dev),
> > wirq);
> > +				   irq_get_trigger_type(irq) |
> > IRQF_ONESHOT,
> 
> This is not needed if you use DT or ACPI and framework (serial in this
> case) checks for interrupt correctly.

I think it's needed in the DT gpio interrupt case as some
gpio controllers need to configure edge vs level.

Regards,

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

Patch
diff mbox

diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -198,7 +198,8 @@  int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
 	 * so we use a threaded irq.
 	 */
 	err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
-				   IRQF_ONESHOT, dev_name(dev), wirq);
+				   irq_get_trigger_type(irq) | IRQF_ONESHOT,
+				   dev_name(dev), wirq);
 	if (err)
 		goto err_free;