| Message ID | 20160201220657.GO19432@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 1, 2016 at 11:06 PM, Tony Lindgren <tony@atomide.com> wrote: > * Tony Lindgren <tony@atomide.com> [160201 10:12]: >> * Ulf Hansson <ulf.hansson@linaro.org> [160201 08:45]: >> > On 28 January 2016 at 17:58, Tony Lindgren <tony@atomide.com> wrote: >> > > >> > > The MMC hardware will not get idled properly any longer blocking any >> > > deeper idle states. >> > > >> > >> Did the driver not probe successfully the second try? If so, what happened. >> > > >> > > It probes fine after a -EPROBE_DEFER on the vmmc i2c regulator. >> > > But the PM runtime usecounts are wrong. >> > >> > Okay. How did you verify this? >> >> Well that was just based on what I see in the dmesg: >> >> omap_device: omap_device_enable() called from invalid state 1 > > So we're now missing the idling of hardare after -EPROBE_DEFER.. > Does the following patch work for you guys? > > Regards, > > Tony > > 8< ----------------------------- > From: Tony Lindgren <tony@atomide.com> > Date: Mon, 1 Feb 2016 13:40:46 -0800 > Subject: [PATCH] PM / runtime: Fix PM runtime reinit > > Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe > error and driver unbind") added calls to reinit the PM runtime. This > however broke things for idling the hardware at least if the driver > probing has pm_runtime_set_autosuspend_delay() and -EPROBE_DEFER > happens. > > Fix the problem by adding a check for configured autosuspend if > RPM_ACTIVE is set. Then reset the autosuspend, and suspend the > device to make sure the hardware gets idled. > > Let's also cut down one level of nestedness and remove a negative > test by returning early if pm_runtime_enabled(dev) as there is > currently nothing for us to do in that case. > > Fixes: 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at > probe error and driver unbind") > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev) > */ > void pm_runtime_reinit(struct device *dev) > { > - if (!pm_runtime_enabled(dev)) { > - if (dev->power.runtime_status == RPM_ACTIVE) > + if (pm_runtime_enabled(dev)) > + return; > + > + if (dev->power.runtime_status == RPM_ACTIVE) { > + if (dev->power.use_autosuspend) { > + __pm_runtime_use_autosuspend(dev, false); > + pm_runtime_suspend(dev); This won't work, because runtime PM is disabled at this point. What about doing this instead: if (dev->power.use_autosuspend) __pm_runtime_use_autosuspend(dev, false); pm_runtime_set_suspended(dev); > + } else { > pm_runtime_set_suspended(dev); > - if (dev->power.irq_safe) { > - spin_lock_irq(&dev->power.lock); > - dev->power.irq_safe = 0; > - spin_unlock_irq(&dev->power.lock); > - if (dev->parent) > - pm_runtime_put(dev->parent); > } > } > + > + if (dev->power.irq_safe) { > + spin_lock_irq(&dev->power.lock); > + dev->power.irq_safe = 0; > + spin_unlock_irq(&dev->power.lock); > + if (dev->parent) > + pm_runtime_put(dev->parent); > + } > } > > /** -- 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
* Rafael J. Wysocki <rafael@kernel.org> [160201 14:18]: > On Mon, Feb 1, 2016 at 11:06 PM, Tony Lindgren <tony@atomide.com> wrote: > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev) > > */ > > void pm_runtime_reinit(struct device *dev) > > { > > - if (!pm_runtime_enabled(dev)) { > > - if (dev->power.runtime_status == RPM_ACTIVE) > > + if (pm_runtime_enabled(dev)) > > + return; > > + > > + if (dev->power.runtime_status == RPM_ACTIVE) { > > + if (dev->power.use_autosuspend) { > > + __pm_runtime_use_autosuspend(dev, false); > > + pm_runtime_suspend(dev); > > This won't work, because runtime PM is disabled at this point. Hmm right OK. It does work from idling the hardware point of view though.. > What about doing this instead: > > if (dev->power.use_autosuspend) > __pm_runtime_use_autosuspend(dev, false); > > pm_runtime_set_suspended(dev); ..while this does not work. The hardware is never idled in this case. What else does __pm_runtime_use_autosuspend() set initially that changes things here? 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 Mon, Feb 1, 2016 at 11:29 PM, Tony Lindgren <tony@atomide.com> wrote: > * Rafael J. Wysocki <rafael@kernel.org> [160201 14:18]: >> On Mon, Feb 1, 2016 at 11:06 PM, Tony Lindgren <tony@atomide.com> wrote: >> > --- a/drivers/base/power/runtime.c >> > +++ b/drivers/base/power/runtime.c >> > @@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev) >> > */ >> > void pm_runtime_reinit(struct device *dev) >> > { >> > - if (!pm_runtime_enabled(dev)) { >> > - if (dev->power.runtime_status == RPM_ACTIVE) >> > + if (pm_runtime_enabled(dev)) >> > + return; >> > + >> > + if (dev->power.runtime_status == RPM_ACTIVE) { >> > + if (dev->power.use_autosuspend) { >> > + __pm_runtime_use_autosuspend(dev, false); >> > + pm_runtime_suspend(dev); >> >> This won't work, because runtime PM is disabled at this point. > > Hmm right OK. It does work from idling the hardware point > of view though.. pm_runtime_suspend() with runtime PM disabled is a NOP. It will do nothing and return -EACCES. >> What about doing this instead: >> >> if (dev->power.use_autosuspend) >> __pm_runtime_use_autosuspend(dev, false); >> >> pm_runtime_set_suspended(dev); > > ..while this does not work. The hardware is never idled > in this case. I'm not sure what you mean. pm_runtime_set_suspended() sets the status to RPM_SUSPENDED for devices with runtime PM disabled. It has nothing to do with "idling" in principle. > What else does __pm_runtime_use_autosuspend() set initially > that changes things here? The usage counter, if the delay is negative. I'll look at this in detail, but not right now, sorry. I'm working on something else ATM and I was hoping that Ulf would be able to figure out what's going on here. Thanks, Rafael -- 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
--- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev) */ void pm_runtime_reinit(struct device *dev) { - if (!pm_runtime_enabled(dev)) { - if (dev->power.runtime_status == RPM_ACTIVE) + if (pm_runtime_enabled(dev)) + return; + + if (dev->power.runtime_status == RPM_ACTIVE) { + if (dev->power.use_autosuspend) { + __pm_runtime_use_autosuspend(dev, false); + pm_runtime_suspend(dev); + } else { pm_runtime_set_suspended(dev); - if (dev->power.irq_safe) { - spin_lock_irq(&dev->power.lock); - dev->power.irq_safe = 0; - spin_unlock_irq(&dev->power.lock); - if (dev->parent) - pm_runtime_put(dev->parent); } } + + if (dev->power.irq_safe) { + spin_lock_irq(&dev->power.lock); + dev->power.irq_safe = 0; + spin_unlock_irq(&dev->power.lock); + if (dev->parent) + pm_runtime_put(dev->parent); + } } /**