[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Ulf Hansson]

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

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Ulf Hansson]

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
>

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Mark Brown]

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.

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121027/e3ecc64f/attachment.sig>

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Linus Walleij]

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

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Ulf Hansson]

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

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Alan Stern]

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

[PATCH] spi/pl022: Activate resourses before deactivate them in suspend

[Ulf Hansson]

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