Hi, (sorry for top-posting) Moiz, are we going to have another set of this ? On Thu, Aug 04, 2011 at 05:18:37PM -0700, Kevin Hilman wrote: > Moiz Sonasath <m-sonasath@xxxxxx> writes: > > > From: Axel Haslam <axelhaslam@xxxxxx> > > > > As Documented in runtime documentation, drivers can call put_sync > > in contexts where sleep is allowed, in contexts where sleep is not > > possible, the drivers need to mark these as to be made interrupt > > safe and also use put_sync_suspend instead of put_sync as the idle > > callbacks will be called with interrupt enabled - which is not > > a good thing to happen in isr context. > > There are several things happening in this patch, and I think this needs > to be broken into separate fixes/features. > > Re: the put_sync_suspend change, for mainline, I posted a fix that > allows a "normal" _put_sync() to work from IRQ-safe context: > > https://lists.linux-foundation.org/pipermail/linux-pm/2011-July/032261.html > > That will be merging for v3.2, likely for v3.1-rc and possible for > v3.0.stable, so while changing the driver is harmless, it's no longer > necessary. > > [...] > > > A call to pm_runtime_irq_safe, will indefently prevent the parent from > > sleeping by doing a call to get-sync on the parent. this is "to > > prevent a irq-safe child to wait for a non-irq safe parent." For this > > reason, the parent runtime-pm suspend is blocked, and we dont let L3 > > and CORE enter into low power. > > First, you should describe why you need to use pm_runtime_irq_safe() in > the first place, since the rest of the "workarounds" in this patch are a > result of using that. For those of us not intimately familar with this > driver, a description of why IRQ-safe callbacks are needed would be most > helpful. > > It might be that a simpler solution would be switching to using > asynchronous runtime PM calls so that IRQ-safe mode is not required. > > > As a workaround, we call the parent runtime functions on the child > > runtime hooks, for this to work, the parent has to be set to ignere > > children, otherwise, even with a "timmed" autosuspend call, will > > return BUSY, as the child is not yet suspended. > > Hmm, why is the child not yet suspended when the parent's autosuspend > delay expires? > > Also, It's not clear to me how the parent is eventually suspending at > all. After pm_runtime_irq_safe() is called on the child, the parent > should *never* runtime suspend. Not only does pm_runtime_irq_safe() do > a get_sync() on the parent, but rpm_suspend() has an explict check for > !dev->power.irq_safe before suspending the parent. > > Something else must be going on (actually, going wrong) in order for the > parent device to runtime suspend. > > Is the problem with L3/CORE not runtime suspending? or is it a problem > with system suspend? If system suspend, are you using current mainline? > There is now support at the omap_device level[1] to ensure all devices are > idled at the omap_device level during system suspend. > > > This patch is based off: > > https://lkml.org/lkml/2011/7/20/357 > > > > Signed-off-by: Axel Haslam <axelhaslam@xxxxxx> > > Reported-by: Colin Cross <ccross@xxxxxxxxxx> > > Signed-off-by: Moiz Sonasath <m-sonasath@xxxxxx> > > --- > > drivers/usb/musb/musb_core.c | 9 +++++++++ > > drivers/usb/musb/musb_gadget.c | 2 +- > > drivers/usb/musb/omap2430.c | 4 +++- > > 3 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > > index c71b037..d22bc73 100644 > > --- a/drivers/usb/musb/musb_core.c > > +++ b/drivers/usb/musb/musb_core.c > > @@ -1941,7 +1941,11 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl) > > > > pm_runtime_use_autosuspend(musb->controller); > > pm_runtime_set_autosuspend_delay(musb->controller, 200); > > + pm_runtime_use_autosuspend(musb->controller->parent); > > + pm_runtime_set_autosuspend_delay(musb->controller->parent, 500); > > + pm_suspend_ignore_children(musb->controller->parent,true); > > Running checkpatch would have yelled at you on this line: > > ERROR: space required after that ',' (ctx:VxV) > > > pm_runtime_enable(musb->controller); > > + pm_runtime_irq_safe(musb->controller); > > > > spin_lock_init(&musb->lock); > > musb->board_mode = plat->mode; > > @@ -2375,6 +2379,9 @@ static int musb_runtime_suspend(struct device *dev) > > > > musb_save_context(musb); > > > > + pm_runtime_mark_last_busy(musb->controller->parent); > > + pm_runtime_put_autosuspend(musb->controller->parent); > > + > > return 0; > > } > > > > @@ -2392,6 +2399,8 @@ static int musb_runtime_resume(struct device *dev) > > * Also context restore without save does not make > > * any sense > > */ > > + if (pm_runtime_suspended(dev->parent)) > > Why this check? > > > + pm_runtime_get_sync(dev->parent); > > if (!first) > > musb_restore_context(musb); > > first = 0; > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > index 548338c..d5d8f3a 100644 > > --- a/drivers/usb/musb/musb_gadget.c > > +++ b/drivers/usb/musb/musb_gadget.c > > @@ -1710,7 +1710,7 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on) > > } > > spin_unlock_irqrestore(&musb->lock, flags); > > > > - pm_runtime_put(musb->controller); > > + pm_runtime_put_sync_suspend(musb->controller); > > > > return 0; > > } > > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > > index c5d4c44..8b6888d 100644 > > --- a/drivers/usb/musb/omap2430.c > > +++ b/drivers/usb/musb/omap2430.c > > @@ -471,6 +471,7 @@ static int __init omap2430_probe(struct platform_device *pdev) > > } > > > > pm_runtime_enable(&pdev->dev); > > + pm_runtime_irq_safe(&pdev->dev); > > > > return 0; > > > > @@ -516,7 +517,8 @@ static int omap2430_runtime_resume(struct device *dev) > > struct musb *musb = glue_to_musb(glue); > > > > omap2430_low_level_init(musb); > > - otg_set_suspend(musb->xceiv, 0); > > + if (musb->xceiv) > > + otg_set_suspend(musb->xceiv, 0); > > This looks like an unrelated fix that should have it's own patch and > description. > > > > > return 0; > > } > > Kevin > > > > [1] This is merged in mainline for v3.1 > > commit c03f007a8bf0e092caeb6856a5c8a850df10b974 > Author: Kevin Hilman <khilman@xxxxxx> > Date: Tue Jul 12 22:48:19 2011 +0200 > > OMAP: PM: omap_device: add system PM methods for PM domain handling > > In the omap_device PM domain callbacks, use omap_device idle/enable to > automatically manage device idle states during system suspend/resume. > > If an omap_device has not already been runtime suspended, the > ->suspend_noirq() method of the PM domain will use omap_device_idle() > to idle the HW after calling the driver's ->runtime_suspend() > callback. Similarily, upon resume, if the device was suspended during > ->suspend_noirq(), the ->resume_noirq() method of the PM domain will > use omap_device_enable() to enable the HW and then call the driver's > ->runtime_resume() callback. > > If a device has already been runtime suspended, the noirq methods of > the PM domain leave the device runtime suspended by default. > > However, if a driver needs to runtime resume a device during suspend > (for example, to change its wakeup settings), it may do so using > pm_runtime_get* in it's ->suspend() callback. > > Signed-off-by: Kevin Hilman <khilman@xxxxxx> > Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx> -- balbi
Attachment:
signature.asc
Description: Digital signature
