[PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Sebastian Andrzej Siewior]

This is mostly an equivalent for the 8250-omap driver based on a patch
of Dave Gerlach for the omap-serial driver (which is not yet merged).

>From his changelog:
|By optionally putting the pins into sleep state in the suspend callback
|we can accomplish two things.
|- minimize current leakage from pins and thus save power,
|- prevent the IP from driving pins output in an uncontrolled manner,
|  which may happen if the power domain drops the domain regulator.

The pinctlr change is put before enable_wakeup logic as per Nishanth
Menon (slightly reworded):
|When wakeup is enabled, I/O daisy chain based wakeup is used by
|reconfiguring the padconfig register. However, this gets overriden by
|sleep/wakeup configuration. Therefore we need first to allow pinctrl to
|play with the wakeup bits as needed beyond the sleep configuration.

Cc: Dave Gerlach <d-gerlach@ti.com>
Cc: Nishanth Menon <nm@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 57a8b1241b85..1681875f2996 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1156,6 +1156,7 @@ static int omap8250_suspend(struct device *dev)
 	serial8250_suspend_port(priv->line);
 	flush_work(&priv->qos_work);
 
+	pinctrl_pm_select_sleep_state(dev);
 	if (device_may_wakeup(dev))
 		omap8250_enable_wakeup(priv, true);
 	else
@@ -1167,6 +1168,7 @@ static int omap8250_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 
+	pinctrl_pm_select_default_state(dev);
 	if (device_may_wakeup(dev))
 		omap8250_enable_wakeup(priv, false);
 
-- 
2.1.1

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Nishanth Menon]

On 11/06/2014 01:07 PM, Sebastian Andrzej Siewior wrote:
> This is mostly an equivalent for the 8250-omap driver based on a patch
> of Dave Gerlach for the omap-serial driver (which is not yet merged).
> 
> From his changelog:
> |By optionally putting the pins into sleep state in the suspend callback
> |we can accomplish two things.
> |- minimize current leakage from pins and thus save power,
> |- prevent the IP from driving pins output in an uncontrolled manner,
> |  which may happen if the power domain drops the domain regulator.
> 
> The pinctlr change is put before enable_wakeup logic as per Nishanth
> Menon (slightly reworded):
> |When wakeup is enabled, I/O daisy chain based wakeup is used by
> |reconfiguring the padconfig register. However, this gets overriden by
> |sleep/wakeup configuration. Therefore we need first to allow pinctrl to
> |play with the wakeup bits as needed beyond the sleep configuration.
> 
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 57a8b1241b85..1681875f2996 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1156,6 +1156,7 @@ static int omap8250_suspend(struct device *dev)
>  	serial8250_suspend_port(priv->line);
>  	flush_work(&priv->qos_work);
>  
> +	pinctrl_pm_select_sleep_state(dev);
>  	if (device_may_wakeup(dev))
>  		omap8250_enable_wakeup(priv, true);
>  	else
> @@ -1167,6 +1168,7 @@ static int omap8250_resume(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>  
> +	pinctrl_pm_select_default_state(dev);
>  	if (device_may_wakeup(dev))
>  		omap8250_enable_wakeup(priv, false);
>  
> 
sounds good to me *IF* omap8250_enable_wakeup (wakeirq handling) is
the way we want to continue doing things for daisychain? -> Tony, can
you comment?
http://marc.info/?l=linux-omap&m=141222144402707&w=2

I wonder if wakeirq explicit handling is valid anymore and something
given the potential race and alternate approach proposed?

-- 
Regards,
Nishanth Menon

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Sebastian Andrzej Siewior]

On 11/06/2014 08:15 PM, Nishanth Menon wrote:
> sounds good to me *IF* omap8250_enable_wakeup (wakeirq handling) is
> the way we want to continue doing things for daisychain? -> Tony, can
> you comment?
> http://marc.info/?l=linux-omap&m=141222144402707&w=2
> 
> I wonder if wakeirq explicit handling is valid anymore and something
> given the potential race and alternate approach proposed?

The wakeirq logic is already in the driver. So if we go for the
alternate approach, the pinctrl patch is obsolete? Or does it mean we
get rid of the map8250_enable_wakeup() including the second irq we have
now (and keep the pinctl change)?

Sebastian

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Nishanth Menon]

On Thu, Nov 6, 2014 at 1:42 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 11/06/2014 08:15 PM, Nishanth Menon wrote:
>> sounds good to me *IF* omap8250_enable_wakeup (wakeirq handling) is
>> the way we want to continue doing things for daisychain? -> Tony, can
>> you comment?
>> http://marc.info/?l=linux-omap&m=141222144402707&w=2
>>
>> I wonder if wakeirq explicit handling is valid anymore and something
>> given the potential race and alternate approach proposed?
>
> The wakeirq logic is already in the driver. So if we go for the
> alternate approach, the pinctrl patch is obsolete? Or does it mean we
> get rid of the map8250_enable_wakeup() including the second irq we have
> now (and keep the pinctl change)?
based on Thomas's feedback we should probably not carry forward
wakeirq as a seperate irq with it's own request_irq instead it psuedo
triggers uart's interrupt irq -> I am not clear about the direction
Tony wants drivers to take in the approach in the thread above - all I
wondered was if it had any conflict of pin_sleep -> if the generic
handler retriggers the irq before the suspend_handler, then
configuring sleep is gonna break daisychain support. not sure if there
is any specific direction drivers should take.

Again, Tony is the right guy to comment about this.

-- 
---
Regards,
Nishanth Menon

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [141106 11:59]:
> On Thu, Nov 6, 2014 at 1:42 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > On 11/06/2014 08:15 PM, Nishanth Menon wrote:
> >> sounds good to me *IF* omap8250_enable_wakeup (wakeirq handling) is
> >> the way we want to continue doing things for daisychain? -> Tony, can
> >> you comment?
> >> http://marc.info/?l=linux-omap&m=141222144402707&w=2
> >>
> >> I wonder if wakeirq explicit handling is valid anymore and something
> >> given the potential race and alternate approach proposed?
> >
> > The wakeirq logic is already in the driver. So if we go for the
> > alternate approach, the pinctrl patch is obsolete? Or does it mean we
> > get rid of the map8250_enable_wakeup() including the second irq we have
> > now (and keep the pinctl change)?
>
> based on Thomas's feedback we should probably not carry forward
> wakeirq as a seperate irq with it's own request_irq instead it psuedo
> triggers uart's interrupt irq -> I am not clear about the direction

The pinctrl change in this patch and the wake-up events are a separate
issue.

As some omaps need the RX pin remuxed temporarily to GPIO for wake-up
events, the $subject patch seems just fine to me.

For the wake-up interrupts, the issue Thomas was concerned was
re-entrant interrupts and  handling the wake-up interrupts in various
different ways. From hardware point of view the wake-up events behave
like a separate IRQ controller, so the request_irq part is fine.

> Tony wants drivers to take in the approach in the thread above - all I
> wondered was if it had any conflict of pin_sleep -> if the generic
> handler retriggers the irq before the suspend_handler, then
> configuring sleep is gonna break daisychain support. not sure if there
> is any specific direction drivers should take.

The wake-irq is disabled during runtime and should be enabled so
that should not be an issue.
 
> Again, Tony is the right guy to comment about this.

Yeah I'll repost the patches for the generic wake-irq handling.
Fixing up the drivers to use that is trivial once we've agreed on
the generic wake-up event handling.

Regards,

Tony

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Sebastian Andrzej Siewior]

On 11/06/2014 09:35 PM, Tony Lindgren wrote:
> The pinctrl change in this patch and the wake-up events are a separate
> issue.

So this patch can go in and wake-up issue will be fixed once the
infrastructure is there. No misunderstanding here?

> Tony
> 

Sebastian

Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [141125 10:07]:
> On 11/06/2014 09:35 PM, Tony Lindgren wrote:
> > The pinctrl change in this patch and the wake-up events are a separate
> > issue.
> 
> So this patch can go in and wake-up issue will be fixed once the
> infrastructure is there. No misunderstanding here?

Yes it will be mostly just removal of code.

Regards,

Tony