| Message ID | 1349423012-18048-1-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mark, Just a kind remember on this. Do you see any problem merging this? Kind regards Ulf Hansson On 5 October 2012 09:43, Ulf Hansson <ulf.hansson@stericsson.com> wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > To be able to deactivate resourses in suspend, the resourses must > first be surely active. This is done with a pm_runtime_get_sync. > Once the resourses are restored to active state again in resume, > the runtime pm usage count can be decreased with a pm_runtime_put. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/spi/spi-pl022.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c > index 9194641..c3590e0 100644 > --- a/drivers/spi/spi-pl022.c > +++ b/drivers/spi/spi-pl022.c > @@ -2350,6 +2350,8 @@ static int pl022_suspend(struct device *dev) > dev_warn(dev, "cannot suspend master\n"); > return ret; > } > + > + pm_runtime_get_sync(dev); > pl022_suspend_resources(pl022); > > dev_dbg(dev, "suspended\n"); > @@ -2362,6 +2364,7 @@ static int pl022_resume(struct device *dev) > int ret; > > pl022_resume_resources(pl022); > + pm_runtime_put(dev); > > /* Start the queue running */ > ret = spi_master_resume(pl022->master); > -- > 1.7.10 >
On Fri, Oct 12, 2012 at 04:42:53PM +0200, Ulf Hansson wrote: > Hi Mark, > > Just a kind remember on this. Do you see any problem merging this? > > Kind regards > Ulf Hansson > > On 5 October 2012 09:43, Ulf Hansson <ulf.hansson@stericsson.com> wrote: Don't top post and don't send contentless nags less than a week after your original mail, especially not in the merge window when only urgent bug fixes should be applied.
On Fri, Oct 05, 2012 at 09:43:32AM +0200, Ulf Hansson wrote: > To be able to deactivate resourses in suspend, the resourses must > first be surely active. This is done with a pm_runtime_get_sync. > Once the resourses are restored to active state again in resume, > the runtime pm usage count can be decreased with a pm_runtime_put. The PM core will ensure devices are runtime resumed before we enter suspend precisely due to this sort of issue.
On Sat, Oct 27, 2012 at 11:46 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Fri, Oct 05, 2012 at 09:43:32AM +0200, Ulf Hansson wrote: > >> To be able to deactivate resourses in suspend, the resourses must >> first be surely active. This is done with a pm_runtime_get_sync. >> Once the resourses are restored to active state again in resume, >> the runtime pm usage count can be decreased with a pm_runtime_put. > > The PM core will ensure devices are runtime resumed before we enter > suspend precisely due to this sort of issue. I asked the very same question to Ulf (in speech, sorry so you couldn't see it...) So I guess we are talking about drivers/base/main.c in device_prepare() pm_runtime_get_noresume() is called and in device_complete() pm_runtime_put_sync() is called. Both put into current for in commit 88d26136a256576e444db312179e17af6dd0ea87 on sep 19th. Yes it seems like it will do the job. Ulf can you comment on this... Yours, Linus Walleij
On 28 October 2012 20:52, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Oct 27, 2012 at 11:46 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Fri, Oct 05, 2012 at 09:43:32AM +0200, Ulf Hansson wrote: >> >>> To be able to deactivate resourses in suspend, the resourses must >>> first be surely active. This is done with a pm_runtime_get_sync. >>> Once the resourses are restored to active state again in resume, >>> the runtime pm usage count can be decreased with a pm_runtime_put. >> >> The PM core will ensure devices are runtime resumed before we enter >> suspend precisely due to this sort of issue. > > I asked the very same question to Ulf (in speech, sorry > so you couldn't see it...) > > So I guess we are talking about drivers/base/main.c > > in device_prepare() > pm_runtime_get_noresume() is called This will increase the "usage_counter" for the device. It will not "runtime_resume" the device, though it will prevent it from being "runtime_suspended". > and in device_complete() > pm_runtime_put_sync() is called. > > Both put into current for in > commit 88d26136a256576e444db312179e17af6dd0ea87 > on sep 19th. > > Yes it seems like it will do the job. > > Ulf can you comment on this... > > Yours, > Linus Walleij Kind regards Ulf Hansson
On Sun, 28 Oct 2012, Ulf Hansson wrote: > On 28 October 2012 20:52, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sat, Oct 27, 2012 at 11:46 PM, Mark Brown > > <broonie@opensource.wolfsonmicro.com> wrote: > >> On Fri, Oct 05, 2012 at 09:43:32AM +0200, Ulf Hansson wrote: > >> > >>> To be able to deactivate resourses in suspend, the resourses must > >>> first be surely active. This is done with a pm_runtime_get_sync. > >>> Once the resourses are restored to active state again in resume, > >>> the runtime pm usage count can be decreased with a pm_runtime_put. > >> > >> The PM core will ensure devices are runtime resumed before we enter > >> suspend precisely due to this sort of issue. > > > > I asked the very same question to Ulf (in speech, sorry > > so you couldn't see it...) > > > > So I guess we are talking about drivers/base/main.c > > > > in device_prepare() > > pm_runtime_get_noresume() is called > > This will increase the "usage_counter" for the device. It will not > "runtime_resume" the device, though it will prevent it from being > "runtime_suspended". Indeed. The PM core does _not_ insure that devices are runtime resumed before going into system suspend. Some subsystems may do this (the PCI subsystem does, for example -- see drivers/pci/pci-driver.c:pci_pm_prepare()), but the PM core doesn't. Alan Stern
On 28 October 2012 22:09, Alan Stern <stern@rowland.harvard.edu> wrote: > On Sun, 28 Oct 2012, Ulf Hansson wrote: > >> On 28 October 2012 20:52, Linus Walleij <linus.walleij@linaro.org> wrote: >> > On Sat, Oct 27, 2012 at 11:46 PM, Mark Brown >> > <broonie@opensource.wolfsonmicro.com> wrote: >> >> On Fri, Oct 05, 2012 at 09:43:32AM +0200, Ulf Hansson wrote: >> >> >> >>> To be able to deactivate resourses in suspend, the resourses must >> >>> first be surely active. This is done with a pm_runtime_get_sync. >> >>> Once the resourses are restored to active state again in resume, >> >>> the runtime pm usage count can be decreased with a pm_runtime_put. >> >> >> >> The PM core will ensure devices are runtime resumed before we enter >> >> suspend precisely due to this sort of issue. >> > >> > I asked the very same question to Ulf (in speech, sorry >> > so you couldn't see it...) >> > >> > So I guess we are talking about drivers/base/main.c >> > >> > in device_prepare() >> > pm_runtime_get_noresume() is called >> >> This will increase the "usage_counter" for the device. It will not >> "runtime_resume" the device, though it will prevent it from being >> "runtime_suspended". > > Indeed. The PM core does _not_ insure that devices are runtime resumed > before going into system suspend. Some subsystems may do this (the PCI > subsystem does, for example -- see > drivers/pci/pci-driver.c:pci_pm_prepare()), but the PM core doesn't. > > Alan Stern > Thanks for clarifying this Alan. Although, I am having second thoughts around this patch even if it does what I expect it to do. I think that the pm_runtime_get_sync (and the corrsponding "put") should maybe be done in the suspend|resume functions of the amba bus (drivers/amba/bus.c) instead. The reason is because the amba bus is responsible for the apb_pclk - clk, which also should be handled during suspend and this is not the case right now. Unless we think each amba device driver should handle this clock during suspend|resume... Kind regards Ulf Hansson
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 9194641..c3590e0 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -2350,6 +2350,8 @@ static int pl022_suspend(struct device *dev) dev_warn(dev, "cannot suspend master\n"); return ret; } + + pm_runtime_get_sync(dev); pl022_suspend_resources(pl022); dev_dbg(dev, "suspended\n"); @@ -2362,6 +2364,7 @@ static int pl022_resume(struct device *dev) int ret; pl022_resume_resources(pl022); + pm_runtime_put(dev); /* Start the queue running */ ret = spi_master_resume(pl022->master);