| Message ID | 1371650753-11452-5-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Roger, Roger Quadros <rogerq@ti.com> writes: > Runtime suspend the controller during bus suspend and resume it > during bus resume. This will ensure that the USB Host power domain > enters lower power state and does not prevent the SoC from > endering deeper sleep states. > > Remote wakeup will come up as an interrupt while the controller > is suspended, so tackle it carefully using a workqueue. I don't think you need a special workqueue here. The workqueue of the PM core (pm_wq) will be used if you just use an asynchronous 'get' in the ISR. Then, the driver's own runtime PM callbacks can be used instead of an additional workqueue. Another thing to worry about here when using runtime PM to implement suspend/resume is that runtime PM can be disabled from userspace (e.g. echo disabled > /sys/devices/.../power/control.) If that's the case, all the pm_runtime_suspended() checks will always fail becuase that call also checks if runtime PM is disabled. You'll likely want to use the pm_runtime_status_suspended() check instead, which checks only the status, and doesn't matter if runtime PM is currently disabled. Because of the corner issues here, please test system suspend/resume when runtime PM has been disabled. Kevin
Hi, On Wed, Jun 19, 2013 at 05:05:51PM +0300, Roger Quadros wrote: > Runtime suspend the controller during bus suspend and resume it > during bus resume. This will ensure that the USB Host power domain > enters lower power state and does not prevent the SoC from > endering deeper sleep states. > > Remote wakeup will come up as an interrupt while the controller > is suspended, so tackle it carefully using a workqueue. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/usb/host/ehci-omap.c | 82 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 82 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > index 16d7150..91f14f1 100644 > --- a/drivers/usb/host/ehci-omap.c > +++ b/drivers/usb/host/ehci-omap.c > @@ -44,6 +44,8 @@ > #include <linux/usb/hcd.h> > #include <linux/of.h> > #include <linux/dma-mapping.h> > +#include <linux/workqueue.h> > +#include <linux/spinlock.h> > > #include "ehci.h" > > @@ -69,6 +71,7 @@ static const char hcd_name[] = "ehci-omap"; > struct omap_hcd { > struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */ > int nports; > + struct work_struct work; > }; > > static inline void ehci_write(void __iomem *base, u32 reg, u32 val) > @@ -81,6 +84,76 @@ static inline u32 ehci_read(void __iomem *base, u32 reg) > return __raw_readl(base + reg); > } > > +static void omap_ehci_work(struct work_struct *work) > +{ > + struct omap_hcd *omap = container_of(work, struct omap_hcd, work); > + struct ehci_hcd *ehci = container_of((void *) omap, > + struct ehci_hcd, priv); > + struct usb_hcd *hcd = ehci_to_hcd(ehci); > + struct device *dev = hcd->self.controller; > + > + pm_runtime_get_sync(dev); > + enable_irq(hcd->irq); > + /* > + * enable_irq() should preempt us with a pending IRQ > + * so we can be sure that IRQ handler completes before > + * we call pm_runtime_put_sync() > + */ > + pm_runtime_put_sync(dev); > +} > + > +static irqreturn_t omap_ehci_irq(struct usb_hcd *hcd) > +{ > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > + struct device *dev = hcd->self.controller; > + irqreturn_t ret; > + > + if (pm_runtime_suspended(dev)) { > + schedule_work(&omap->work); > + disable_irq_nosync(hcd->irq); > + ret = IRQ_HANDLED; looks like this could be done as: if (pm_runtime_suspended(dev)) { pm_runtime_get(dev); omap->flags |= OMAP_EHCI_IRQ_PENDING; disable_irq_nosync(hcd->irq); ret = IRQ_HANDLED; } then on your ->runtime_resume(): runtime_resume(dev) { ... if (omap->flags & OMAP_EHCI_IRQ_PENDING) { process_pending_irqs(omap); omap->flags &= ~OMAP_EHCI_IRQ_PENDING; } return 0; } or something similar
On 06/19/2013 08:39 PM, Kevin Hilman wrote: > Hi Roger, > > Roger Quadros <rogerq@ti.com> writes: > >> Runtime suspend the controller during bus suspend and resume it >> during bus resume. This will ensure that the USB Host power domain >> enters lower power state and does not prevent the SoC from >> endering deeper sleep states. >> >> Remote wakeup will come up as an interrupt while the controller >> is suspended, so tackle it carefully using a workqueue. > > I don't think you need a special workqueue here. The workqueue of the PM > core (pm_wq) will be used if you just use an asynchronous 'get' in the > ISR. Then, the driver's own runtime PM callbacks can be used instead of > an additional workqueue. > > Another thing to worry about here when using runtime PM to implement > suspend/resume is that runtime PM can be disabled from userspace (e.g. > echo disabled > /sys/devices/.../power/control.) If that's the case, > all the pm_runtime_suspended() checks will always fail becuase that > call also checks if runtime PM is disabled. You'll likely want to use > the pm_runtime_status_suspended() check instead, which checks only the > status, and doesn't matter if runtime PM is currently disabled. > > Because of the corner issues here, please test system suspend/resume > when runtime PM has been disabled. > Good points. Thanks. I'll need to think about it some more. cheers, -roger
On 06/20/2013 03:11 PM, Felipe Balbi wrote: > Hi, > > On Wed, Jun 19, 2013 at 05:05:51PM +0300, Roger Quadros wrote: >> Runtime suspend the controller during bus suspend and resume it >> during bus resume. This will ensure that the USB Host power domain >> enters lower power state and does not prevent the SoC from >> endering deeper sleep states. >> >> Remote wakeup will come up as an interrupt while the controller >> is suspended, so tackle it carefully using a workqueue. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/usb/host/ehci-omap.c | 82 ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 82 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c >> index 16d7150..91f14f1 100644 >> --- a/drivers/usb/host/ehci-omap.c >> +++ b/drivers/usb/host/ehci-omap.c >> @@ -44,6 +44,8 @@ >> #include <linux/usb/hcd.h> >> #include <linux/of.h> >> #include <linux/dma-mapping.h> >> +#include <linux/workqueue.h> >> +#include <linux/spinlock.h> >> >> #include "ehci.h" >> >> @@ -69,6 +71,7 @@ static const char hcd_name[] = "ehci-omap"; >> struct omap_hcd { >> struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */ >> int nports; >> + struct work_struct work; >> }; >> >> static inline void ehci_write(void __iomem *base, u32 reg, u32 val) >> @@ -81,6 +84,76 @@ static inline u32 ehci_read(void __iomem *base, u32 reg) >> return __raw_readl(base + reg); >> } >> >> +static void omap_ehci_work(struct work_struct *work) >> +{ >> + struct omap_hcd *omap = container_of(work, struct omap_hcd, work); >> + struct ehci_hcd *ehci = container_of((void *) omap, >> + struct ehci_hcd, priv); >> + struct usb_hcd *hcd = ehci_to_hcd(ehci); >> + struct device *dev = hcd->self.controller; >> + >> + pm_runtime_get_sync(dev); >> + enable_irq(hcd->irq); >> + /* >> + * enable_irq() should preempt us with a pending IRQ >> + * so we can be sure that IRQ handler completes before >> + * we call pm_runtime_put_sync() >> + */ >> + pm_runtime_put_sync(dev); >> +} >> + >> +static irqreturn_t omap_ehci_irq(struct usb_hcd *hcd) >> +{ >> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >> + struct device *dev = hcd->self.controller; >> + irqreturn_t ret; >> + >> + if (pm_runtime_suspended(dev)) { >> + schedule_work(&omap->work); >> + disable_irq_nosync(hcd->irq); >> + ret = IRQ_HANDLED; > > looks like this could be done as: > > if (pm_runtime_suspended(dev)) { > pm_runtime_get(dev); > omap->flags |= OMAP_EHCI_IRQ_PENDING; > disable_irq_nosync(hcd->irq); > ret = IRQ_HANDLED; > } > > then on your ->runtime_resume(): > > runtime_resume(dev) > { > ... > > if (omap->flags & OMAP_EHCI_IRQ_PENDING) { > process_pending_irqs(omap); OK, thanks. But I'm not sure if the generic ehci_irq handler is able to run in a process context. Maybe if we replace spin_lock(&ehci->lock); with spin_lock_irqsave() there, it will work. Alan is this a doable option? > omap->flags &= ~OMAP_EHCI_IRQ_PENDING; > } > > return 0; > } > > or something similar > cheers, -roger
On Thu, 20 Jun 2013, Roger Quadros wrote: > > runtime_resume(dev) > > { > > ... > > > > if (omap->flags & OMAP_EHCI_IRQ_PENDING) { > > process_pending_irqs(omap); > > OK, thanks. > > But I'm not sure if the generic ehci_irq handler is able to > run in a process context. Maybe if we replace spin_lock(&ehci->lock); > with spin_lock_irqsave() there, it will work. > > Alan is this a doable option? ehci_irq() will work okay in process context, provided the caller disables interrupts. But maybe none of this will be needed after Roger redesigns the controller suspend to work at the right time. Or if it is, we could adopt a simpler alternative: the controller's resume routine could always call usb_hcd_resume_root_hub(). After all, about the only reason for doing a runtime resume of the controller is because the root hub needs to do something. Alan Stern
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index 16d7150..91f14f1 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -44,6 +44,8 @@ #include <linux/usb/hcd.h> #include <linux/of.h> #include <linux/dma-mapping.h> +#include <linux/workqueue.h> +#include <linux/spinlock.h> #include "ehci.h" @@ -69,6 +71,7 @@ static const char hcd_name[] = "ehci-omap"; struct omap_hcd { struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */ int nports; + struct work_struct work; }; static inline void ehci_write(void __iomem *base, u32 reg, u32 val) @@ -81,6 +84,76 @@ static inline u32 ehci_read(void __iomem *base, u32 reg) return __raw_readl(base + reg); } +static void omap_ehci_work(struct work_struct *work) +{ + struct omap_hcd *omap = container_of(work, struct omap_hcd, work); + struct ehci_hcd *ehci = container_of((void *) omap, + struct ehci_hcd, priv); + struct usb_hcd *hcd = ehci_to_hcd(ehci); + struct device *dev = hcd->self.controller; + + pm_runtime_get_sync(dev); + enable_irq(hcd->irq); + /* + * enable_irq() should preempt us with a pending IRQ + * so we can be sure that IRQ handler completes before + * we call pm_runtime_put_sync() + */ + pm_runtime_put_sync(dev); +} + +static irqreturn_t omap_ehci_irq(struct usb_hcd *hcd) +{ + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; + struct device *dev = hcd->self.controller; + irqreturn_t ret; + + if (pm_runtime_suspended(dev)) { + schedule_work(&omap->work); + disable_irq_nosync(hcd->irq); + ret = IRQ_HANDLED; + } else + ret = ehci_irq(hcd); + + return ret; +} + +#ifdef CONFIG_PM +static int omap_ehci_bus_suspend(struct usb_hcd *hcd) +{ + struct device *dev; + int ret; + + dev = hcd->self.controller; + ret = ehci_bus_suspend(hcd); + if (ret) + return ret; + + ret = pm_runtime_put_sync(dev); + if (ret < 0 && !(ret == -EBUSY || ret == -EAGAIN)) { + dev_err(dev, "Failed to runtime suspend :%d\n", ret); + return ret; + } + + return 0; +} + +static int omap_ehci_bus_resume(struct usb_hcd *hcd) +{ + struct device *dev; + int ret; + + dev = hcd->self.controller; + ret = pm_runtime_get_sync(dev); + if (ret < 0) { + dev_err(dev, "Failed to runtime resume :%d\n", ret); + return ret; + } + + return ehci_bus_resume(hcd); +} +#endif /* CONFIG_PM */ + /* configure so an HC device and id are always provided */ /* always called with process context; sleeping is OK */ @@ -88,6 +161,11 @@ static struct hc_driver __read_mostly ehci_omap_hc_driver; static const struct ehci_driver_overrides ehci_omap_overrides __initdata = { .extra_priv_size = sizeof(struct omap_hcd), +#ifdef CONFIG_PM + .bus_suspend = omap_ehci_bus_suspend, + .bus_resume = omap_ehci_bus_resume, +#endif + .irq = omap_ehci_irq, }; /** @@ -163,6 +241,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev) omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; omap->nports = pdata->nports; + INIT_WORK(&omap->work, omap_ehci_work); platform_set_drvdata(pdev, hcd); @@ -257,6 +336,9 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; int i; + if (pm_runtime_suspended(dev)) + pm_runtime_get_sync(dev); + usb_remove_hcd(hcd); for (i = 0; i < omap->nports; i++) {
Runtime suspend the controller during bus suspend and resume it during bus resume. This will ensure that the USB Host power domain enters lower power state and does not prevent the SoC from endering deeper sleep states. Remote wakeup will come up as an interrupt while the controller is suspended, so tackle it carefully using a workqueue. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/host/ehci-omap.c | 82 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 82 insertions(+), 0 deletions(-)