| Message ID | 1415300850-21452-1-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 -- 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 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.
* 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 -- 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 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 -- 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
* 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 -- 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/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);
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(+)