On Fri, Aug 12, 2011 at 2:01 AM, Felipe Balbi <balbi@xxxxxx> wrote: > Hi, > > (sorry for top-posting) > > Moiz, are we going to have another set of this ? The problem was with pm_runtime() functions getting called from atomic context and we did a work around by calling the runtime functions from a work queue. That seems to fix the issue. We will post that out as an alternative to marking the irq_safe and thus causing the parent to never idle. > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
