pm: add suspend_mem and suspend_standby support

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:

  Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)

are available in the git repository at:


  git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem

for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:

  arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)

----------------------------------------------------------------
pm: add suspend_mem and suspend_standby support

Today when we go to suspend we can not knwon at drivers level if we go in
STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
suspend_standby.

If a bus or drivers does not need to care about this distinction we fallback to
suspend. This will allow to do not update the current bus or drivers.

This is needed as example by at91 OHCI, UDC and atmel serial

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

----------------------------------------------------------------
Jean-Christophe PLAGNIOL-VILLARD (6):
      pm: add suspend_mem and suspend_standby support
      platform: pm: add suspend_mem and suspend_standby support
      tty: atmel_serial: switch pm ops
      usb: ohci-at91: switch pm ops
      usb: at91_ude: switch pm ops
      arm: at91: drop at91_suspend_entering_slow_clock

 arch/arm/mach-at91/include/mach/board.h |    3 ---
 arch/arm/mach-at91/pm.c                 |   24 ------------------------
 drivers/base/platform.c                 |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/main.c               |   12 ++++++++++++
 drivers/tty/serial/atmel_serial.c       |   52 +++++++++++++++++++++++++++-------------------------
 drivers/usb/gadget/at91_udc.c           |   34 +++++++++++++++++++++++-----------
 drivers/usb/host/ohci-at91.c            |   38 ++++++++++++++++++++++++--------------
 include/linux/platform_device.h         |    6 ++++++
 include/linux/pm.h                      |    9 +++++++++
 kernel/power/suspend.c                  |   16 ++++++++++++++--
 10 files changed, 155 insertions(+), 79 deletions(-)

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> Hi,
> 
> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> 
>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> 
> are available in the git repository at:
> 
> 
>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> 
> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> 
>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> 
> ----------------------------------------------------------------
> pm: add suspend_mem and suspend_standby support
> 
> Today when we go to suspend we can not knwon at drivers level if we go in
> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> suspend_standby.

No way. Device drivers shouldn't be concerned about that.

> If a bus or drivers does not need to care about this distinction we fallback to
> suspend. This will allow to do not update the current bus or drivers.
> 
> This is needed as example by at91 OHCI, UDC and atmel serial

I wonder why.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Greg Kroah-Hartman]

On Sun, Oct 07, 2012 at 12:18:21AM +0200, Rafael J. Wysocki wrote:
> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Hi,
> > 
> > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > 
> >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > 
> > are available in the git repository at:
> > 
> > 
> >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > 
> > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > 
> >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > 
> > ----------------------------------------------------------------
> > pm: add suspend_mem and suspend_standby support
> > 
> > Today when we go to suspend we can not knwon at drivers level if we go in
> > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > suspend_standby.
> 
> No way. Device drivers shouldn't be concerned about that.
> 
> > If a bus or drivers does not need to care about this distinction we fallback to
> > suspend. This will allow to do not update the current bus or drivers.
> > 
> > This is needed as example by at91 OHCI, UDC and atmel serial
> 
> I wonder why.

Yes, that's odd, please post the patches, I don't take random git pull
requests, sorry.

greg k-h

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 21:01 Sat 06 Oct     , Greg Kroah-Hartman wrote:
> On Sun, Oct 07, 2012 at 12:18:21AM +0200, Rafael J. Wysocki wrote:
> > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > Hi,
> > > 
> > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > 
> > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > 
> > > are available in the git repository at:
> > > 
> > > 
> > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > 
> > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > 
> > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > 
> > > ----------------------------------------------------------------
> > > pm: add suspend_mem and suspend_standby support
> > > 
> > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > suspend_standby.
> > 
> > No way. Device drivers shouldn't be concerned about that.
> > 
> > > If a bus or drivers does not need to care about this distinction we fallback to
> > > suspend. This will allow to do not update the current bus or drivers.
> > > 
> > > This is needed as example by at91 OHCI, UDC and atmel serial
> > 
> > I wonder why.
> 
> Yes, that's odd, please post the patches, I don't take random git pull
> requests, sorry.
the patch did not follow the first e-mail do not why I've already resend them
this afternoon

sorry

Best Regards
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Saturday 06 of October 2012 21:01:28 Greg Kroah-Hartman wrote:
> On Sun, Oct 07, 2012 at 12:18:21AM +0200, Rafael J. Wysocki wrote:
> > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > Hi,
> > > 
> > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > 
> > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > 
> > > are available in the git repository at:
> > > 
> > > 
> > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > 
> > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > 
> > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > 
> > > ----------------------------------------------------------------
> > > pm: add suspend_mem and suspend_standby support
> > > 
> > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > suspend_standby.
> > 
> > No way. Device drivers shouldn't be concerned about that.
> > 
> > > If a bus or drivers does not need to care about this distinction we fallback to
> > > suspend. This will allow to do not update the current bus or drivers.
> > > 
> > > This is needed as example by at91 OHCI, UDC and atmel serial
> > 
> > I wonder why.
> 
> Yes, that's odd, please post the patches, I don't take random git pull
> requests, sorry.

Well, this particular one would be for me, but I'm not going to take it too. :-)

I'll have a look at the patches tomorrow, but the rule is we're not adding more
device PM callbacks on a whim.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Hi,
> > 
> > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > 
> >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > 
> > are available in the git repository at:
> > 
> > 
> >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > 
> > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > 
> >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > 
> > ----------------------------------------------------------------
> > pm: add suspend_mem and suspend_standby support
> > 
> > Today when we go to suspend we can not knwon at drivers level if we go in
> > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > suspend_standby.
> 
> No way. Device drivers shouldn't be concerned about that.
I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
need special handling when switching to slow_clock

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > Hi,
> > > 
> > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > 
> > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > 
> > > are available in the git repository at:
> > > 
> > > 
> > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > 
> > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > 
> > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > 
> > > ----------------------------------------------------------------
> > > pm: add suspend_mem and suspend_standby support
> > > 
> > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > suspend_standby.
> > 
> > No way. Device drivers shouldn't be concerned about that.
> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> need special handling when switching to slow_clock

Well, my answer to that is: please fix your platform code instead of
hacking the PM core to work around its problems.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > Hi,
> > > > 
> > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > 
> > > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > 
> > > > are available in the git repository at:
> > > > 
> > > > 
> > > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > 
> > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > 
> > > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > 
> > > > ----------------------------------------------------------------
> > > > pm: add suspend_mem and suspend_standby support
> > > > 
> > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > suspend_standby.
> > > 
> > > No way. Device drivers shouldn't be concerned about that.
> > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > need special handling when switching to slow_clock
> 
> Well, my answer to that is: please fix your platform code instead of
> hacking the PM core to work around its problems.
how can I fix drivers pm issue when I no way to known at driver level the
real suspend, the PM core is supposed to proivde the right information to the
drivers so the driver can put it's in the right pm mode. If the pm core can not
provide such inforation the PM core is broken as we will have to do dirty
hack.

Any generic framework is supposed to evolve for real user need here I've one
so I udpate the core to mach a need

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Greg Kroah-Hartman]

On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > Hi,
> > > > > 
> > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > 
> > > > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > 
> > > > > are available in the git repository at:
> > > > > 
> > > > > 
> > > > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > 
> > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > 
> > > > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > 
> > > > > ----------------------------------------------------------------
> > > > > pm: add suspend_mem and suspend_standby support
> > > > > 
> > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > suspend_standby.
> > > > 
> > > > No way. Device drivers shouldn't be concerned about that.
> > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > need special handling when switching to slow_clock
> > 
> > Well, my answer to that is: please fix your platform code instead of
> > hacking the PM core to work around its problems.
> how can I fix drivers pm issue when I no way to known at driver level the
> real suspend, the PM core is supposed to proivde the right information to the
> drivers so the driver can put it's in the right pm mode. If the pm core can not
> provide such inforation the PM core is broken as we will have to do dirty
> hack.

Why do you need to know the difference in your driver?  We used to
provide this information a long time ago, but it turned out to not be
needed at all and just caused problems.

> Any generic framework is supposed to evolve for real user need here I've one
> so I udpate the core to mach a need

I'll push back and ask again why your driver cares about this?  It
shouldn't.

greg k-h

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > > 
> > > > > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > > 
> > > > > > are available in the git repository at:
> > > > > > 
> > > > > > 
> > > > > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > > 
> > > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > > 
> > > > > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > > 
> > > > > > ----------------------------------------------------------------
> > > > > > pm: add suspend_mem and suspend_standby support
> > > > > > 
> > > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > > suspend_standby.
> > > > > 
> > > > > No way. Device drivers shouldn't be concerned about that.
> > > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > > need special handling when switching to slow_clock
> > > 
> > > Well, my answer to that is: please fix your platform code instead of
> > > hacking the PM core to work around its problems.
> > how can I fix drivers pm issue when I no way to known at driver level the
> > real suspend, the PM core is supposed to proivde the right information to the
> > drivers so the driver can put it's in the right pm mode. If the pm core can not
> > provide such inforation the PM core is broken as we will have to do dirty
> > hack.
> 
> Why do you need to know the difference in your driver?  We used to
> provide this information a long time ago, but it turned out to not be
> needed at all and just caused problems.
because on at91 I need to handle the mem standby at drviers level.

We do it today already by a hack in different drivers at91_udc (usb device),
atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
clock - this is done at soc level - but those IP have issue and need specific
care before doing so. Ohterwise when the SoC will wakeup but those IP will not

in this patch series I send the update of those 3 drivers too
and kill the hack
> 
> > Any generic framework is supposed to evolve for real user need here I've one
> > so I udpate the core to mach a need
> 
> I'll push back and ask again why your driver cares about this?  It
> shouldn't.

I'm doing it right now in the kernel but in a dirty way and want to fix it.
And honestly If I could not do it I will

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Alan Stern]

On Tue, 9 Oct 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:

> > Why do you need to know the difference in your driver?  We used to
> > provide this information a long time ago, but it turned out to not be
> > needed at all and just caused problems.
> because on at91 I need to handle the mem standby at drviers level.
> 
> We do it today already by a hack in different drivers at91_udc (usb device),
> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> clock - this is done at soc level - but those IP have issue and need specific
> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> 
> in this patch series I send the update of those 3 drivers too
> and kill the hack

How about adding a platform-specific callback routine to tell drivers 
whether or not they will run from the slow clock?  Something like:

	bool at91_suspend_will_use_slow_clock(void);

That would not involve making any changes to the driver core, and your 
three drivers could still get the information they need.

Alan Stern

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:44 Tue 09 Oct     , Alan Stern wrote:
> On Tue, 9 Oct 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > > Why do you need to know the difference in your driver?  We used to
> > > provide this information a long time ago, but it turned out to not be
> > > needed at all and just caused problems.
> > because on at91 I need to handle the mem standby at drviers level.
> > 
> > We do it today already by a hack in different drivers at91_udc (usb device),
> > atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> > to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> > clock - this is done at soc level - but those IP have issue and need specific
> > care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> > 
> > in this patch series I send the update of those 3 drivers too
> > and kill the hack
> 
> How about adding a platform-specific callback routine to tell drivers 
> whether or not they will run from the slow clock?  Something like:
> 
> 	bool at91_suspend_will_use_slow_clock(void);
> 
> That would not involve making any changes to the driver core, and your 
> three drivers could still get the information they need.
I already do so and I want to drop it as the IP is shared with multiple ARCH
and I want to have a clean code no hack

and no platform data I do DT and drop board code so far, I've a dt code
without any platform data provide via c code

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Tuesday 09 of October 2012 17:52:41 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:44 Tue 09 Oct     , Alan Stern wrote:
> > On Tue, 9 Oct 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > 
> > > > Why do you need to know the difference in your driver?  We used to
> > > > provide this information a long time ago, but it turned out to not be
> > > > needed at all and just caused problems.
> > > because on at91 I need to handle the mem standby at drviers level.
> > > 
> > > We do it today already by a hack in different drivers at91_udc (usb device),
> > > atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> > > to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> > > clock - this is done at soc level - but those IP have issue and need specific
> > > care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> > > 
> > > in this patch series I send the update of those 3 drivers too
> > > and kill the hack
> > 
> > How about adding a platform-specific callback routine to tell drivers 
> > whether or not they will run from the slow clock?  Something like:
> > 
> > 	bool at91_suspend_will_use_slow_clock(void);
> > 
> > That would not involve making any changes to the driver core, and your 
> > three drivers could still get the information they need.
> I already do so and I want to drop it as the IP is shared with multiple ARCH
> and I want to have a clean code no hack

So, what about actually adding a platform layer to your PM instead of doing
all PM in device drivers?

ACPI has exactly the same problem that you have, but it's never has to tell
device drivers what system sleep state is going to be entered.  Drivers just
call acpi_pm_device_sleep_state() and that tells them what power state to
put the device into (which depends on the sleep state being entered, but
drivers really don't need to know that state).

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> > On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > > > 
> > > > > > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > > > 
> > > > > > > are available in the git repository at:
> > > > > > > 
> > > > > > > 
> > > > > > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > > > 
> > > > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > > > 
> > > > > > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > > > 
> > > > > > > ----------------------------------------------------------------
> > > > > > > pm: add suspend_mem and suspend_standby support
> > > > > > > 
> > > > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > > > suspend_standby.
> > > > > > 
> > > > > > No way. Device drivers shouldn't be concerned about that.
> > > > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > > > need special handling when switching to slow_clock
> > > > 
> > > > Well, my answer to that is: please fix your platform code instead of
> > > > hacking the PM core to work around its problems.
> > > how can I fix drivers pm issue when I no way to known at driver level the
> > > real suspend, the PM core is supposed to proivde the right information to the
> > > drivers so the driver can put it's in the right pm mode. If the pm core can not
> > > provide such inforation the PM core is broken as we will have to do dirty
> > > hack.
> > 
> > Why do you need to know the difference in your driver?  We used to
> > provide this information a long time ago, but it turned out to not be
> > needed at all and just caused problems.
> because on at91 I need to handle the mem standby at drviers level.

This is not an answer.  You're basically saying "it has to be done this way,
because it has to be done this way".

> We do it today already by a hack in different drivers at91_udc (usb device),
> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> clock - this is done at soc level - but those IP have issue and need specific
> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> 
> in this patch series I send the update of those 3 drivers too
> and kill the hack

Well, let's start over.  What's the difference between "suspend to RAM" (mem)
and "standby" on your platform?

> > > Any generic framework is supposed to evolve for real user need here I've one
> > > so I udpate the core to mach a need

Yes, if there are more users needing a change.  If there's only one, I think not
really.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Vaibhav Hiremath]

On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
> On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
>>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
>>>>>>>>
>>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
>>>>>>>>
>>>>>>>> are available in the git repository at:
>>>>>>>>
>>>>>>>>
>>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
>>>>>>>>
>>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
>>>>>>>>
>>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
>>>>>>>>
>>>>>>>> ----------------------------------------------------------------
>>>>>>>> pm: add suspend_mem and suspend_standby support
>>>>>>>>
>>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
>>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
>>>>>>>> suspend_standby.
>>>>>>>
>>>>>>> No way. Device drivers shouldn't be concerned about that.
>>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
>>>>>> need special handling when switching to slow_clock
>>>>>
>>>>> Well, my answer to that is: please fix your platform code instead of
>>>>> hacking the PM core to work around its problems.
>>>> how can I fix drivers pm issue when I no way to known at driver level the
>>>> real suspend, the PM core is supposed to proivde the right information to the
>>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
>>>> provide such inforation the PM core is broken as we will have to do dirty
>>>> hack.
>>>
>>> Why do you need to know the difference in your driver?  We used to
>>> provide this information a long time ago, but it turned out to not be
>>> needed at all and just caused problems.
>> because on at91 I need to handle the mem standby at drviers level.
> 
> This is not an answer.  You're basically saying "it has to be done this way,
> because it has to be done this way".
> 
>> We do it today already by a hack in different drivers at91_udc (usb device),
>> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
>> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
>> clock - this is done at soc level - but those IP have issue and need specific
>> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
>>
>> in this patch series I send the update of those 3 drivers too
>> and kill the hack
> 
> Well, let's start over.  What's the difference between "suspend to RAM" (mem)
> and "standby" on your platform?
> 
>>>> Any generic framework is supposed to evolve for real user need here I've one
>>>> so I udpate the core to mach a need
> 
> Yes, if there are more users needing a change.  If there's only one, I think not
> really.
> 

We came across such a need while supporting various low-power modes in
AM335x SoC. I also wanted to put similar proposal, its good that Jean
submitted patches and we are having this discussion early.

Let me try to describe the need and issue here,

In case of AM33xx device, we have different low-power modes supported
by HW, for this discussion I will just talk about DeepSleep-0 and
StandBy mode supported by HW (few other modes also described in spec)-

Below is the Spec definition of these two low-power modes,

PowerMode    Application state             States
========================================================================
DeepSleep0   Everything is preserved       Master Oscillator = OFF
             including SDRAM.              VDD_MPU = 0.95v
                                           VDD_CORE = 0.95v
                                           PD_WKUP = ON
                                           PD_MPU = OFF
                                           PD_PER = OFF
                                           PD_GFX = OFF
                                           All IOs & RTC supplies are ON


Standby      Everything is preserved       Master Oscillator = ON
             including SDRAM.              One PLL is ON
             Only required module clocks   VDD_MPU = 0.95v
             are enabled rest are clock    VDD_CORE = OPP50
             gated                         PD_WKUP = ON
                                           PD_MPU = ON
                                           PD_PER = ON
                                           PD_GFX = OFF
                                           All IOs & RTC supplies are ON
                                           If more PLLs /power domains/
                                           modules are required, the
                                           power will increase
                                           accordingly



Now we have two major parameters here, Power Consumption and Latency
(Sleep + Wakeup). Since DeepSleep-0 is as good as off state, it gives
very less power consumption Vs huge Latency number, whereas, in case of
standby the power consumption is more but the Latency is reduced.

In addition to that, there are certain HW related parameters and issues
contribute to these parameters. For example,

In case of LCD driver the suspend/resume latency is in the range of
10msec, which can be reduced if driver knows that he is not entering or
entered in DeepSleep-0 and choose not to do certain things.
Another example would be, USB driver, we have seen Latency numbers in
the range of 200msec, and of which 100 msec can simply be taken away if
driver knows about the sleep state.


I understand that, there is trade-off between power and latency numbers,
but for certain use-cases latency is more important and it is not
possible without telling driver about the low-power state.


Thanks,
Vaibhav
> Thanks,
> Rafael
> 
> 

pm: add suspend_mem and suspend_standby support

[Daniel Mack]

On 10.10.2012 09:17, Vaibhav Hiremath wrote:
> On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
>> On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
>>>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
>>>>>>>>>
>>>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
>>>>>>>>>
>>>>>>>>> are available in the git repository at:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
>>>>>>>>>
>>>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
>>>>>>>>>
>>>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
>>>>>>>>>
>>>>>>>>> ----------------------------------------------------------------
>>>>>>>>> pm: add suspend_mem and suspend_standby support
>>>>>>>>>
>>>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
>>>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
>>>>>>>>> suspend_standby.
>>>>>>>>
>>>>>>>> No way. Device drivers shouldn't be concerned about that.
>>>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
>>>>>>> need special handling when switching to slow_clock
>>>>>>
>>>>>> Well, my answer to that is: please fix your platform code instead of
>>>>>> hacking the PM core to work around its problems.
>>>>> how can I fix drivers pm issue when I no way to known at driver level the
>>>>> real suspend, the PM core is supposed to proivde the right information to the
>>>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
>>>>> provide such inforation the PM core is broken as we will have to do dirty
>>>>> hack.
>>>>
>>>> Why do you need to know the difference in your driver?  We used to
>>>> provide this information a long time ago, but it turned out to not be
>>>> needed at all and just caused problems.
>>> because on at91 I need to handle the mem standby at drviers level.
>>
>> This is not an answer.  You're basically saying "it has to be done this way,
>> because it has to be done this way".
>>
>>> We do it today already by a hack in different drivers at91_udc (usb device),
>>> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
>>> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
>>> clock - this is done at soc level - but those IP have issue and need specific
>>> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
>>>
>>> in this patch series I send the update of those 3 drivers too
>>> and kill the hack
>>
>> Well, let's start over.  What's the difference between "suspend to RAM" (mem)
>> and "standby" on your platform?
>>
>>>>> Any generic framework is supposed to evolve for real user need here I've one
>>>>> so I udpate the core to mach a need
>>
>> Yes, if there are more users needing a change.  If there's only one, I think not
>> really.
>>
> 
> We came across such a need while supporting various low-power modes in
> AM335x SoC. I also wanted to put similar proposal, its good that Jean
> submitted patches and we are having this discussion early.
> 
> Let me try to describe the need and issue here,
> 
> In case of AM33xx device, we have different low-power modes supported
> by HW, for this discussion I will just talk about DeepSleep-0 and
> StandBy mode supported by HW (few other modes also described in spec)-
> 
> Below is the Spec definition of these two low-power modes,

Thanks for this summary, which raises a question for me that might be
slightly unreleated to the general discussion, but still:

I was just looking at the code that ships with the BSP kernel, and the
approach there is to load a binary firmare blob to the M3 UMEM and then
communicates with mailbox commands in order to put the system to suspend.

Is this how it should be done in mainline as well or are there any plans
to implement that behaviour natively?


Thanks,
Daniel

pm: add suspend_mem and suspend_standby support

[Hiremath, Vaibhav]

On Wed, Oct 10, 2012 at 14:50:25, Daniel Mack wrote:
> On 10.10.2012 09:17, Vaibhav Hiremath wrote:
> > On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
> >> On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> >>>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> >>>>>>>>>
> >>>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> >>>>>>>>>
> >>>>>>>>> are available in the git repository at:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> >>>>>>>>>
> >>>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> >>>>>>>>>
> >>>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> >>>>>>>>>
> >>>>>>>>> ----------------------------------------------------------------
> >>>>>>>>> pm: add suspend_mem and suspend_standby support
> >>>>>>>>>
> >>>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
> >>>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> >>>>>>>>> suspend_standby.
> >>>>>>>>
> >>>>>>>> No way. Device drivers shouldn't be concerned about that.
> >>>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> >>>>>>> need special handling when switching to slow_clock
> >>>>>>
> >>>>>> Well, my answer to that is: please fix your platform code instead of
> >>>>>> hacking the PM core to work around its problems.
> >>>>> how can I fix drivers pm issue when I no way to known at driver level the
> >>>>> real suspend, the PM core is supposed to proivde the right information to the
> >>>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
> >>>>> provide such inforation the PM core is broken as we will have to do dirty
> >>>>> hack.
> >>>>
> >>>> Why do you need to know the difference in your driver?  We used to
> >>>> provide this information a long time ago, but it turned out to not be
> >>>> needed at all and just caused problems.
> >>> because on at91 I need to handle the mem standby at drviers level.
> >>
> >> This is not an answer.  You're basically saying "it has to be done this way,
> >> because it has to be done this way".
> >>
> >>> We do it today already by a hack in different drivers at91_udc (usb device),
> >>> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> >>> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> >>> clock - this is done at soc level - but those IP have issue and need specific
> >>> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> >>>
> >>> in this patch series I send the update of those 3 drivers too
> >>> and kill the hack
> >>
> >> Well, let's start over.  What's the difference between "suspend to RAM" (mem)
> >> and "standby" on your platform?
> >>
> >>>>> Any generic framework is supposed to evolve for real user need here I've one
> >>>>> so I udpate the core to mach a need
> >>
> >> Yes, if there are more users needing a change.  If there's only one, I think not
> >> really.
> >>
> > 
> > We came across such a need while supporting various low-power modes in
> > AM335x SoC. I also wanted to put similar proposal, its good that Jean
> > submitted patches and we are having this discussion early.
> > 
> > Let me try to describe the need and issue here,
> > 
> > In case of AM33xx device, we have different low-power modes supported
> > by HW, for this discussion I will just talk about DeepSleep-0 and
> > StandBy mode supported by HW (few other modes also described in spec)-
> > 
> > Below is the Spec definition of these two low-power modes,
> 
> Thanks for this summary, which raises a question for me that might be
> slightly unreleated to the general discussion, but still:
> 
> I was just looking at the code that ships with the BSP kernel, and the
> approach there is to load a binary firmare blob to the M3 UMEM and then
> communicates with mailbox commands in order to put the system to suspend.
> 

Ohh, Great, then you are aware of AM33xx power management support.
As you are aware, M3 here works as a soft-PRCM block, so certain things has 
to be done on M3 while entering into low-power state.

> Is this how it should be done in mainline as well or are there any plans
> to implement that behaviour natively?
> 

Yes, we will follow similar approach for Mainline, its under development and 
soon you will see patches getting submitted to the list for review. The 
first step is to get deepsleep-0 support in Mainline and then other will 
follow.


Thanks,
Vaibhav
> 
> Thanks,
> Daniel
> 
> 

pm: add suspend_mem and suspend_standby support

[Daniel Mack]

On 10.10.2012 11:29, Hiremath, Vaibhav wrote:
> On Wed, Oct 10, 2012 at 14:50:25, Daniel Mack wrote:
>> On 10.10.2012 09:17, Vaibhav Hiremath wrote:
>>> On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
>>>> On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
>>>>>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
>>>>>>>>>>>
>>>>>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
>>>>>>>>>>>
>>>>>>>>>>> are available in the git repository at:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
>>>>>>>>>>>
>>>>>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
>>>>>>>>>>>
>>>>>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
>>>>>>>>>>>
>>>>>>>>>>> ----------------------------------------------------------------
>>>>>>>>>>> pm: add suspend_mem and suspend_standby support
>>>>>>>>>>>
>>>>>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
>>>>>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
>>>>>>>>>>> suspend_standby.
>>>>>>>>>>
>>>>>>>>>> No way. Device drivers shouldn't be concerned about that.
>>>>>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
>>>>>>>>> need special handling when switching to slow_clock
>>>>>>>>
>>>>>>>> Well, my answer to that is: please fix your platform code instead of
>>>>>>>> hacking the PM core to work around its problems.
>>>>>>> how can I fix drivers pm issue when I no way to known at driver level the
>>>>>>> real suspend, the PM core is supposed to proivde the right information to the
>>>>>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
>>>>>>> provide such inforation the PM core is broken as we will have to do dirty
>>>>>>> hack.
>>>>>>
>>>>>> Why do you need to know the difference in your driver?  We used to
>>>>>> provide this information a long time ago, but it turned out to not be
>>>>>> needed at all and just caused problems.
>>>>> because on at91 I need to handle the mem standby at drviers level.
>>>>
>>>> This is not an answer.  You're basically saying "it has to be done this way,
>>>> because it has to be done this way".
>>>>
>>>>> We do it today already by a hack in different drivers at91_udc (usb device),
>>>>> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
>>>>> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
>>>>> clock - this is done at soc level - but those IP have issue and need specific
>>>>> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
>>>>>
>>>>> in this patch series I send the update of those 3 drivers too
>>>>> and kill the hack
>>>>
>>>> Well, let's start over.  What's the difference between "suspend to RAM" (mem)
>>>> and "standby" on your platform?
>>>>
>>>>>>> Any generic framework is supposed to evolve for real user need here I've one
>>>>>>> so I udpate the core to mach a need
>>>>
>>>> Yes, if there are more users needing a change.  If there's only one, I think not
>>>> really.
>>>>
>>>
>>> We came across such a need while supporting various low-power modes in
>>> AM335x SoC. I also wanted to put similar proposal, its good that Jean
>>> submitted patches and we are having this discussion early.
>>>
>>> Let me try to describe the need and issue here,
>>>
>>> In case of AM33xx device, we have different low-power modes supported
>>> by HW, for this discussion I will just talk about DeepSleep-0 and
>>> StandBy mode supported by HW (few other modes also described in spec)-
>>>
>>> Below is the Spec definition of these two low-power modes,
>>
>> Thanks for this summary, which raises a question for me that might be
>> slightly unreleated to the general discussion, but still:
>>
>> I was just looking at the code that ships with the BSP kernel, and the
>> approach there is to load a binary firmare blob to the M3 UMEM and then
>> communicates with mailbox commands in order to put the system to suspend.
>>
> 
> Ohh, Great, then you are aware of AM33xx power management support.
> As you are aware, M3 here works as a soft-PRCM block, so certain things has 
> to be done on M3 while entering into low-power state.
> 
>> Is this how it should be done in mainline as well or are there any plans
>> to implement that behaviour natively?
>>
> 
> Yes, we will follow similar approach for Mainline, its under development and 
> soon you will see patches getting submitted to the list for review. The 
> first step is to get deepsleep-0 support in Mainline and then other will 
> follow.

Ok, great. I'll test them as soon as they hit my mailbox :)


Thanks,
Daniel

pm: add suspend_mem and suspend_standby support

[Daniel Mack]

On 10.10.2012 11:29, Hiremath, Vaibhav wrote:
> On Wed, Oct 10, 2012 at 14:50:25, Daniel Mack wrote:
>> On 10.10.2012 09:17, Vaibhav Hiremath wrote:
>>> On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
>>>> On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
>>>>>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
>>>>>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
>>>>>>>>>>>
>>>>>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
>>>>>>>>>>>
>>>>>>>>>>> are available in the git repository at:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
>>>>>>>>>>>
>>>>>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
>>>>>>>>>>>
>>>>>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
>>>>>>>>>>>
>>>>>>>>>>> ----------------------------------------------------------------
>>>>>>>>>>> pm: add suspend_mem and suspend_standby support
>>>>>>>>>>>
>>>>>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
>>>>>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
>>>>>>>>>>> suspend_standby.
>>>>>>>>>>
>>>>>>>>>> No way. Device drivers shouldn't be concerned about that.
>>>>>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
>>>>>>>>> need special handling when switching to slow_clock
>>>>>>>>
>>>>>>>> Well, my answer to that is: please fix your platform code instead of
>>>>>>>> hacking the PM core to work around its problems.
>>>>>>> how can I fix drivers pm issue when I no way to known at driver level the
>>>>>>> real suspend, the PM core is supposed to proivde the right information to the
>>>>>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
>>>>>>> provide such inforation the PM core is broken as we will have to do dirty
>>>>>>> hack.
>>>>>>
>>>>>> Why do you need to know the difference in your driver?  We used to
>>>>>> provide this information a long time ago, but it turned out to not be
>>>>>> needed at all and just caused problems.
>>>>> because on at91 I need to handle the mem standby at drviers level.
>>>>
>>>> This is not an answer.  You're basically saying "it has to be done this way,
>>>> because it has to be done this way".
>>>>
>>>>> We do it today already by a hack in different drivers at91_udc (usb device),
>>>>> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
>>>>> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
>>>>> clock - this is done at soc level - but those IP have issue and need specific
>>>>> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
>>>>>
>>>>> in this patch series I send the update of those 3 drivers too
>>>>> and kill the hack
>>>>
>>>> Well, let's start over.  What's the difference between "suspend to RAM" (mem)
>>>> and "standby" on your platform?
>>>>
>>>>>>> Any generic framework is supposed to evolve for real user need here I've one
>>>>>>> so I udpate the core to mach a need
>>>>
>>>> Yes, if there are more users needing a change.  If there's only one, I think not
>>>> really.
>>>>
>>>
>>> We came across such a need while supporting various low-power modes in
>>> AM335x SoC. I also wanted to put similar proposal, its good that Jean
>>> submitted patches and we are having this discussion early.
>>>
>>> Let me try to describe the need and issue here,
>>>
>>> In case of AM33xx device, we have different low-power modes supported
>>> by HW, for this discussion I will just talk about DeepSleep-0 and
>>> StandBy mode supported by HW (few other modes also described in spec)-
>>>
>>> Below is the Spec definition of these two low-power modes,
>>
>> Thanks for this summary, which raises a question for me that might be
>> slightly unreleated to the general discussion, but still:
>>
>> I was just looking at the code that ships with the BSP kernel, and the
>> approach there is to load a binary firmare blob to the M3 UMEM and then
>> communicates with mailbox commands in order to put the system to suspend.
>>
> 
> Ohh, Great, then you are aware of AM33xx power management support.
> As you are aware, M3 here works as a soft-PRCM block, so certain things has 
> to be done on M3 while entering into low-power state.
> 
>> Is this how it should be done in mainline as well or are there any plans
>> to implement that behaviour natively?
>>
> 
> Yes, we will follow similar approach for Mainline, its under development and 
> soon you will see patches getting submitted to the list for review. The 
> first step is to get deepsleep-0 support in Mainline and then other will 
> follow.

May I ask whether there is anything yet I can test?


Thanks,
Daniel

AM33XX suspend-resume support (Was RE: pm: add suspend_mem and suspend_standby support)

[Bedia, Vaibhav]

(Changed $SUBJECT to reflect the topic being discussed)

On Wed, Oct 31, 2012 at 15:57:13, Daniel Mack wrote:

[...]
> 
> May I ask whether there is anything yet I can test?

I am working on basic suspend-resume patches for AM33XX which builds on top of the
current linux-omap/master. Will be posting it later this week.

Regards,
Vaibhav

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:47 Wed 10 Oct     , Vaibhav Hiremath wrote:
> 
> 
> On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
> > On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> >>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> >>>>>>>>
> >>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> >>>>>>>>
> >>>>>>>> are available in the git repository at:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> >>>>>>>>
> >>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> >>>>>>>>
> >>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> >>>>>>>>
> >>>>>>>> ----------------------------------------------------------------
> >>>>>>>> pm: add suspend_mem and suspend_standby support
> >>>>>>>>
> >>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
> >>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> >>>>>>>> suspend_standby.
> >>>>>>>
> >>>>>>> No way. Device drivers shouldn't be concerned about that.
> >>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> >>>>>> need special handling when switching to slow_clock
> >>>>>
> >>>>> Well, my answer to that is: please fix your platform code instead of
> >>>>> hacking the PM core to work around its problems.
> >>>> how can I fix drivers pm issue when I no way to known at driver level the
> >>>> real suspend, the PM core is supposed to proivde the right information to the
> >>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
> >>>> provide such inforation the PM core is broken as we will have to do dirty
> >>>> hack.
> >>>
> >>> Why do you need to know the difference in your driver?  We used to
> >>> provide this information a long time ago, but it turned out to not be
> >>> needed at all and just caused problems.
> >> because on at91 I need to handle the mem standby at drviers level.
> > 
> > This is not an answer.  You're basically saying "it has to be done this way,
> > because it has to be done this way".
> > 
> >> We do it today already by a hack in different drivers at91_udc (usb device),
> >> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> >> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> >> clock - this is done at soc level - but those IP have issue and need specific
> >> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> >>
> >> in this patch series I send the update of those 3 drivers too
> >> and kill the hack
> > 
> > Well, let's start over.  What's the difference between "suspend to RAM" (mem)
> > and "standby" on your platform?
> > 
> >>>> Any generic framework is supposed to evolve for real user need here I've one
> >>>> so I udpate the core to mach a need
> > 
> > Yes, if there are more users needing a change.  If there's only one, I think not
> > really.
> > 
> 
> We came across such a need while supporting various low-power modes in
> AM335x SoC. I also wanted to put similar proposal, its good that Jean
> submitted patches and we are having this discussion early.
> 
> Let me try to describe the need and issue here,
> 
> In case of AM33xx device, we have different low-power modes supported
> by HW, for this discussion I will just talk about DeepSleep-0 and
> StandBy mode supported by HW (few other modes also described in spec)-
> 
> Below is the Spec definition of these two low-power modes,
> 
> PowerMode    Application state             States
> ========================================================================
> DeepSleep0   Everything is preserved       Master Oscillator = OFF
>              including SDRAM.              VDD_MPU = 0.95v
>                                            VDD_CORE = 0.95v
>                                            PD_WKUP = ON
>                                            PD_MPU = OFF
>                                            PD_PER = OFF
>                                            PD_GFX = OFF
>                                            All IOs & RTC supplies are ON
> 
> 
> Standby      Everything is preserved       Master Oscillator = ON
>              including SDRAM.              One PLL is ON
>              Only required module clocks   VDD_MPU = 0.95v
>              are enabled rest are clock    VDD_CORE = OPP50
>              gated                         PD_WKUP = ON
>                                            PD_MPU = ON
>                                            PD_PER = ON
>                                            PD_GFX = OFF
>                                            All IOs & RTC supplies are ON
>                                            If more PLLs /power domains/
>                                            modules are required, the
>                                            power will increase
>                                            accordingly
> 
> 
> 
> Now we have two major parameters here, Power Consumption and Latency
> (Sleep + Wakeup). Since DeepSleep-0 is as good as off state, it gives
> very less power consumption Vs huge Latency number, whereas, in case of
> standby the power consumption is more but the Latency is reduced.
> 
> In addition to that, there are certain HW related parameters and issues
> contribute to these parameters. For example,
> 
> In case of LCD driver the suspend/resume latency is in the range of
> 10msec, which can be reduced if driver knows that he is not entering or
> entered in DeepSleep-0 and choose not to do certain things.
> Another example would be, USB driver, we have seen Latency numbers in
> the range of 200msec, and of which 100 msec can simply be taken away if
> driver knows about the sleep state.
> 
> 
> I understand that, there is trade-off between power and latency numbers,
> but for certain use-cases latency is more important and it is not
> possible without telling driver about the low-power state.

I work on ST SoC where I have also such standby mode too and similar issue

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

[...]

> > > I understand that, there is trade-off between power and latency numbers,
> > > but for certain use-cases latency is more important and it is not
> > > possible without telling driver about the low-power state.
> > 
> > I understand the problem at hand, but the question is who's going to tell
> > the driver about that.
> > 
> > Arguably, the PM core is not the right source of that information, because
> > if a driver is told "we're going into standby", it will have to ask the
> > platform about what "standby" actually means.
> 
> who is going to tell the kernel to go to standby and which
> 
> the sysfs handle via PM core
> 
> so here the one who alwasy have the information is the pm core

The core tells the _platform_ "the user wants us to go into standby" or
"the user wants us to go into suspend", but it doesn't really know what
"standby" or "suspend" actually mean. It can't, because these things are
not universally well defined and there are differences between platforms
in that area.  Different platforms may require different things to be
done to devices for "standby" and so drivers should not assume that
"standby" will always mean the same thing.

For example, if your driver is written for and ARM SoC and then the same
IP block happens to be used on an x86-based thing, whatever the driver did
and was specific to ARM is no longer valid.  So if your ARM SoC required
drivers to do something special during a "suspend" transition which wasn't
necessary to do during a "standby" transition, it doesn't necessarily mean that
the new platform will do so as well. The "mem" and "standby" things may have
different meanings for the new platform and for this reason drivers _have_ _to_
ask the _platform_ what states to put the devices into _or_, alternatively,
leave the platform-specific handling of the devices to the platform (that's
what device drivers on ACPI-based x86 do).  They should not expect the PM core
to tell them that, simply because the PM core has no idea what that may be.

And by the way, ACPI-based ARM platforms are reportedly in the works and they
_will_ use the same IP blocks that are already in use on ARM.  On those
platforms drivers will be supposed to use ACPI for power management and
the whole design you're proposing will not make sense any more for them.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Wednesday 10 of October 2012 12:47:31 Vaibhav Hiremath wrote:
> 
> On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
> > On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> >>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> >>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> >>>>>>>>
> >>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> >>>>>>>>
> >>>>>>>> are available in the git repository at:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> >>>>>>>>
> >>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> >>>>>>>>
> >>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> >>>>>>>>
> >>>>>>>> ----------------------------------------------------------------
> >>>>>>>> pm: add suspend_mem and suspend_standby support
> >>>>>>>>
> >>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
> >>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> >>>>>>>> suspend_standby.
> >>>>>>>
> >>>>>>> No way. Device drivers shouldn't be concerned about that.
> >>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> >>>>>> need special handling when switching to slow_clock
> >>>>>
> >>>>> Well, my answer to that is: please fix your platform code instead of
> >>>>> hacking the PM core to work around its problems.
> >>>> how can I fix drivers pm issue when I no way to known at driver level the
> >>>> real suspend, the PM core is supposed to proivde the right information to the
> >>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
> >>>> provide such inforation the PM core is broken as we will have to do dirty
> >>>> hack.
> >>>
> >>> Why do you need to know the difference in your driver?  We used to
> >>> provide this information a long time ago, but it turned out to not be
> >>> needed at all and just caused problems.
> >> because on at91 I need to handle the mem standby at drviers level.
> > 
> > This is not an answer.  You're basically saying "it has to be done this way,
> > because it has to be done this way".
> > 
> >> We do it today already by a hack in different drivers at91_udc (usb device),
> >> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> >> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> >> clock - this is done at soc level - but those IP have issue and need specific
> >> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> >>
> >> in this patch series I send the update of those 3 drivers too
> >> and kill the hack
> > 
> > Well, let's start over.  What's the difference between "suspend to RAM" (mem)
> > and "standby" on your platform?
> > 
> >>>> Any generic framework is supposed to evolve for real user need here I've one
> >>>> so I udpate the core to mach a need
> > 
> > Yes, if there are more users needing a change.  If there's only one, I think not
> > really.
> > 
> 
> We came across such a need while supporting various low-power modes in
> AM335x SoC. I also wanted to put similar proposal, its good that Jean
> submitted patches and we are having this discussion early.
> 
> Let me try to describe the need and issue here,
> 
> In case of AM33xx device, we have different low-power modes supported
> by HW, for this discussion I will just talk about DeepSleep-0 and
> StandBy mode supported by HW (few other modes also described in spec)-
> 
> Below is the Spec definition of these two low-power modes,
> 
> PowerMode    Application state             States
> ========================================================================
> DeepSleep0   Everything is preserved       Master Oscillator = OFF
>              including SDRAM.              VDD_MPU = 0.95v
>                                            VDD_CORE = 0.95v
>                                            PD_WKUP = ON
>                                            PD_MPU = OFF
>                                            PD_PER = OFF
>                                            PD_GFX = OFF
>                                            All IOs & RTC supplies are ON
> 
> 
> Standby      Everything is preserved       Master Oscillator = ON
>              including SDRAM.              One PLL is ON
>              Only required module clocks   VDD_MPU = 0.95v
>              are enabled rest are clock    VDD_CORE = OPP50
>              gated                         PD_WKUP = ON
>                                            PD_MPU = ON
>                                            PD_PER = ON
>                                            PD_GFX = OFF
>                                            All IOs & RTC supplies are ON
>                                            If more PLLs /power domains/
>                                            modules are required, the
>                                            power will increase
>                                            accordingly
> 
> 
> 
> Now we have two major parameters here, Power Consumption and Latency
> (Sleep + Wakeup). Since DeepSleep-0 is as good as off state, it gives
> very less power consumption Vs huge Latency number, whereas, in case of
> standby the power consumption is more but the Latency is reduced.
> 
> In addition to that, there are certain HW related parameters and issues
> contribute to these parameters. For example,
> 
> In case of LCD driver the suspend/resume latency is in the range of
> 10msec, which can be reduced if driver knows that he is not entering or
> entered in DeepSleep-0 and choose not to do certain things.
> Another example would be, USB driver, we have seen Latency numbers in
> the range of 200msec, and of which 100 msec can simply be taken away if
> driver knows about the sleep state.
> 
> 
> I understand that, there is trade-off between power and latency numbers,
> but for certain use-cases latency is more important and it is not
> possible without telling driver about the low-power state.

I understand the problem at hand, but the question is who's going to tell
the driver about that.

Arguably, the PM core is not the right source of that information, because
if a driver is told "we're going into standby", it will have to ask the
platform about what "standby" actually means.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

On 01:05 Thu 11 Oct     , Rafael J. Wysocki wrote:
> On Wednesday 10 of October 2012 12:47:31 Vaibhav Hiremath wrote:
> > 
> > On 10/10/2012 3:42 AM, Rafael J. Wysocki wrote:
> > > On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >> On 07:58 Tue 09 Oct     , Greg Kroah-Hartman wrote:
> > >>> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>>> On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > >>>>> On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>>>>> On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > >>>>>>> On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>>>>>>> Hi,
> > >>>>>>>>
> > >>>>>>>> The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > >>>>>>>>
> > >>>>>>>>   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > >>>>>>>>
> > >>>>>>>> are available in the git repository at:
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > >>>>>>>>
> > >>>>>>>> for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > >>>>>>>>
> > >>>>>>>>   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > >>>>>>>>
> > >>>>>>>> ----------------------------------------------------------------
> > >>>>>>>> pm: add suspend_mem and suspend_standby support
> > >>>>>>>>
> > >>>>>>>> Today when we go to suspend we can not knwon at drivers level if we go in
> > >>>>>>>> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > >>>>>>>> suspend_standby.
> > >>>>>>>
> > >>>>>>> No way. Device drivers shouldn't be concerned about that.
> > >>>>>> I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > >>>>>> need special handling when switching to slow_clock
> > >>>>>
> > >>>>> Well, my answer to that is: please fix your platform code instead of
> > >>>>> hacking the PM core to work around its problems.
> > >>>> how can I fix drivers pm issue when I no way to known at driver level the
> > >>>> real suspend, the PM core is supposed to proivde the right information to the
> > >>>> drivers so the driver can put it's in the right pm mode. If the pm core can not
> > >>>> provide such inforation the PM core is broken as we will have to do dirty
> > >>>> hack.
> > >>>
> > >>> Why do you need to know the difference in your driver?  We used to
> > >>> provide this information a long time ago, but it turned out to not be
> > >>> needed at all and just caused problems.
> > >> because on at91 I need to handle the mem standby at drviers level.
> > > 
> > > This is not an answer.  You're basically saying "it has to be done this way,
> > > because it has to be done this way".
> > > 
> > >> We do it today already by a hack in different drivers at91_udc (usb device),
> > >> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> > >> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> > >> clock - this is done at soc level - but those IP have issue and need specific
> > >> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> > >>
> > >> in this patch series I send the update of those 3 drivers too
> > >> and kill the hack
> > > 
> > > Well, let's start over.  What's the difference between "suspend to RAM" (mem)
> > > and "standby" on your platform?
> > > 
> > >>>> Any generic framework is supposed to evolve for real user need here I've one
> > >>>> so I udpate the core to mach a need
> > > 
> > > Yes, if there are more users needing a change.  If there's only one, I think not
> > > really.
> > > 
> > 
> > We came across such a need while supporting various low-power modes in
> > AM335x SoC. I also wanted to put similar proposal, its good that Jean
> > submitted patches and we are having this discussion early.
> > 
> > Let me try to describe the need and issue here,
> > 
> > In case of AM33xx device, we have different low-power modes supported
> > by HW, for this discussion I will just talk about DeepSleep-0 and
> > StandBy mode supported by HW (few other modes also described in spec)-
> > 
> > Below is the Spec definition of these two low-power modes,
> > 
> > PowerMode    Application state             States
> > ========================================================================
> > DeepSleep0   Everything is preserved       Master Oscillator = OFF
> >              including SDRAM.              VDD_MPU = 0.95v
> >                                            VDD_CORE = 0.95v
> >                                            PD_WKUP = ON
> >                                            PD_MPU = OFF
> >                                            PD_PER = OFF
> >                                            PD_GFX = OFF
> >                                            All IOs & RTC supplies are ON
> > 
> > 
> > Standby      Everything is preserved       Master Oscillator = ON
> >              including SDRAM.              One PLL is ON
> >              Only required module clocks   VDD_MPU = 0.95v
> >              are enabled rest are clock    VDD_CORE = OPP50
> >              gated                         PD_WKUP = ON
> >                                            PD_MPU = ON
> >                                            PD_PER = ON
> >                                            PD_GFX = OFF
> >                                            All IOs & RTC supplies are ON
> >                                            If more PLLs /power domains/
> >                                            modules are required, the
> >                                            power will increase
> >                                            accordingly
> > 
> > 
> > 
> > Now we have two major parameters here, Power Consumption and Latency
> > (Sleep + Wakeup). Since DeepSleep-0 is as good as off state, it gives
> > very less power consumption Vs huge Latency number, whereas, in case of
> > standby the power consumption is more but the Latency is reduced.
> > 
> > In addition to that, there are certain HW related parameters and issues
> > contribute to these parameters. For example,
> > 
> > In case of LCD driver the suspend/resume latency is in the range of
> > 10msec, which can be reduced if driver knows that he is not entering or
> > entered in DeepSleep-0 and choose not to do certain things.
> > Another example would be, USB driver, we have seen Latency numbers in
> > the range of 200msec, and of which 100 msec can simply be taken away if
> > driver knows about the sleep state.
> > 
> > 
> > I understand that, there is trade-off between power and latency numbers,
> > but for certain use-cases latency is more important and it is not
> > possible without telling driver about the low-power state.
> 
> I understand the problem at hand, but the question is who's going to tell
> the driver about that.
> 
> Arguably, the PM core is not the right source of that information, because
> if a driver is told "we're going into standby", it will have to ask the
> platform about what "standby" actually means.
who is going to tell the kernel to go to standby and which

the sysfs handle via PM core

so here the one who alwasy have the information is the pm core

Best Regards,
J.

pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Tuesday 09 of October 2012 07:58:54 Greg Kroah-Hartman wrote:
> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 22:02 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 00:18 Sun 07 Oct     , Rafael J. Wysocki wrote:
> > > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > > 
> > > > > >   Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > > 
> > > > > > are available in the git repository at:
> > > > > > 
> > > > > > 
> > > > > >   git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > > 
> > > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > > 
> > > > > >   arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > > 
> > > > > > ----------------------------------------------------------------
> > > > > > pm: add suspend_mem and suspend_standby support
> > > > > > 
> > > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > > suspend_standby.
> > > > > 
> > > > > No way. Device drivers shouldn't be concerned about that.
> > > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > > need special handling when switching to slow_clock
> > > 
> > > Well, my answer to that is: please fix your platform code instead of
> > > hacking the PM core to work around its problems.
> > how can I fix drivers pm issue when I no way to known at driver level the
> > real suspend, the PM core is supposed to proivde the right information to the
> > drivers so the driver can put it's in the right pm mode. If the pm core can not
> > provide such inforation the PM core is broken as we will have to do dirty
> > hack.
> 
> Why do you need to know the difference in your driver?  We used to
> provide this information a long time ago, but it turned out to not be
> needed at all and just caused problems.

We actualy have never made a difference between "standby" and "suspend" as
far as drivers are concerned.  What we used to have was one "suspend" callback
for suspend to RAM and all phases of hibernation, so we needed to pass an
argument indicating what operation was to be carried out to it.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/6] pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

Today when we go to suspend we can not knwon at drivers level if we go in
STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
suspend_standby.

If a bus does not need to care about this distinction we fallback to
suspend. This will allow to do not update the current bus or drivers.

This is needed as example by at91 OHCI and atmel serial

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
---
 drivers/base/power/main.c |   12 ++++++++++++
 include/linux/pm.h        |    9 +++++++++
 kernel/power/suspend.c    |   16 ++++++++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index a3c1404..581e135 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -218,9 +218,21 @@ static void dpm_wait_for_children(struct device *dev, bool async)
  */
 static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
 {
+	pm_callback_t callback = NULL;
+
 	switch (state.event) {
 #ifdef CONFIG_SUSPEND
 	case PM_EVENT_SUSPEND:
+		switch (state.param) {
+		case PM_SUSPEND_MEM:
+			callback = ops->suspend_mem;
+			break;
+		case PM_SUSPEND_STANDBY:
+			callback = ops->suspend_standby;
+			break;
+		}
+		if (callback)
+			return callback;
 		return ops->suspend;
 	case PM_EVENT_RESUME:
 		return ops->resume;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 007e687..aff344b 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -49,6 +49,7 @@ extern const char power_group_name[];		/* = "power" */
 
 typedef struct pm_message {
 	int event;
+	int param;
 } pm_message_t;
 
 /**
@@ -265,6 +266,8 @@ struct dev_pm_ops {
 	int (*prepare)(struct device *dev);
 	void (*complete)(struct device *dev);
 	int (*suspend)(struct device *dev);
+	int (*suspend_mem)(struct device *dev);
+	int (*suspend_standby)(struct device *dev);
 	int (*resume)(struct device *dev);
 	int (*freeze)(struct device *dev);
 	int (*thaw)(struct device *dev);
@@ -295,8 +298,12 @@ struct dev_pm_ops {
 	.thaw = resume_fn, \
 	.poweroff = suspend_fn, \
 	.restore = resume_fn,
+#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn) \
+	.suspend_standby = suspend_standby_fn, \
+	.suspend_mem = suspend_mem_fn,
 #else
 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
+#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn)
 #endif
 
 #ifdef CONFIG_PM_RUNTIME
@@ -414,6 +421,8 @@ const struct dev_pm_ops name = { \
 #define PMSG_FREEZE	((struct pm_message){ .event = PM_EVENT_FREEZE, })
 #define PMSG_QUIESCE	((struct pm_message){ .event = PM_EVENT_QUIESCE, })
 #define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
+#define PMSG_SUSPEND_MEM	((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_MEM, })
+#define PMSG_SUSPEND_STANDBY	((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_STANDBY, })
 #define PMSG_HIBERNATE	((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
 #define PMSG_RESUME	((struct pm_message){ .event = PM_EVENT_RESUME, })
 #define PMSG_THAW	((struct pm_message){ .event = PM_EVENT_THAW, })
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c8b7446..9505b55 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -126,6 +126,18 @@ void __attribute__ ((weak)) arch_suspend_enable_irqs(void)
 	local_irq_enable();
 }
 
+static pm_message_t suspend_state_to_pm_state(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return PMSG_SUSPEND_STANDBY;
+	case PM_SUSPEND_MEM:
+		return PMSG_SUSPEND_MEM;
+	default:
+		return PMSG_SUSPEND;
+	}
+}
+
 /**
  * suspend_enter - Make the system enter the given sleep state.
  * @state: System sleep state to enter.
@@ -143,7 +155,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 			goto Platform_finish;
 	}
 
-	error = dpm_suspend_end(PMSG_SUSPEND);
+	error = dpm_suspend_end(suspend_state_to_pm_state(state));
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to power down\n");
 		goto Platform_finish;
@@ -215,7 +227,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 	suspend_console();
 	ftrace_stop();
 	suspend_test_start();
-	error = dpm_suspend_start(PMSG_SUSPEND);
+	error = dpm_suspend_start(suspend_state_to_pm_state(state));
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to suspend\n");
 		goto Recover_platform;
-- 
1.7.10.4

[PATCH 2/6] platform: pm: add suspend_mem and suspend_standby support

[Jean-Christophe PLAGNIOL-VILLARD]

If a drivers does not need to care about this distinction we fallback to
suspend. This will allow to do not update the current drivers.

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
---
 drivers/base/platform.c         |   40 +++++++++++++++++++++++++++++++++++++++
 include/linux/platform_device.h |    6 ++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 8727e9c..f4822e2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -740,6 +740,46 @@ static int platform_legacy_resume(struct device *dev)
 
 #ifdef CONFIG_SUSPEND
 
+int platform_pm_suspend_mem(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	int ret = 0;
+
+	if (!drv)
+		return 0;
+
+	if (drv->pm) {
+		if (drv->pm->suspend_mem)
+			ret = drv->pm->suspend_mem(dev);
+		else if (drv->pm->suspend)
+			ret = drv->pm->suspend(dev);
+	} else {
+		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
+	}
+
+	return ret;
+}
+
+int platform_pm_suspend_standby(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	int ret = 0;
+
+	if (!drv)
+		return 0;
+
+	if (drv->pm) {
+		if (drv->pm->suspend_standby)
+			ret = drv->pm->suspend_standby(dev);
+		else if (drv->pm->suspend)
+			ret = drv->pm->suspend(dev);
+	} else {
+		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
+	}
+
+	return ret;
+}
+
 int platform_pm_suspend(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5711e95..a7c2275 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -262,9 +262,13 @@ static inline char *early_platform_driver_setup_func(void)		\
 
 #ifdef CONFIG_SUSPEND
 extern int platform_pm_suspend(struct device *dev);
+extern int platform_pm_suspend_mem(struct device *dev);
+extern int platform_pm_suspend_standby(struct device *dev);
 extern int platform_pm_resume(struct device *dev);
 #else
 #define platform_pm_suspend		NULL
+#define platform_pm_suspend_mem		NULL
+#define platform_pm_suspend_standby	NULL
 #define platform_pm_resume		NULL
 #endif
 
@@ -283,6 +287,8 @@ extern int platform_pm_restore(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define USE_PLATFORM_PM_SLEEP_OPS \
 	.suspend = platform_pm_suspend, \
+	.suspend_standby = platform_pm_suspend_standby, \
+	.suspend_mem = platform_pm_suspend_mem, \
 	.resume = platform_pm_resume, \
 	.freeze = platform_pm_freeze, \
 	.thaw = platform_pm_thaw, \
-- 
1.7.10.4

[PATCH 3/6] tty: atmel_serial: switch pm ops

[Jean-Christophe PLAGNIOL-VILLARD]

so we can detect when we enter in slow_clock mode and drop
at91_suspend_entering_slow_clock

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
Cc: linux-serial at vger.kernel.org
---
 drivers/tty/serial/atmel_serial.c |   52 +++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 3d7e1ee..7a51a0c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1720,20 +1720,10 @@ static struct uart_driver atmel_uart = {
 };
 
 #ifdef CONFIG_PM
-static bool atmel_serial_clk_will_stop(void)
+static int atmel_serial_suspend(struct device *dev, int slow_clock)
 {
-#ifdef CONFIG_ARCH_AT91
-	return at91_suspend_entering_slow_clock();
-#else
-	return false;
-#endif
-}
-
-static int atmel_serial_suspend(struct platform_device *pdev,
-				pm_message_t state)
-{
-	struct uart_port *port = platform_get_drvdata(pdev);
-	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	struct atmel_uart_port *atmel_port = dev_get_drvdata(dev);
+	struct uart_port *port = &atmel_port->uart;
 
 	if (atmel_is_console_port(port) && console_suspend_enabled) {
 		/* Drain the TX shifter */
@@ -1742,30 +1732,43 @@ static int atmel_serial_suspend(struct platform_device *pdev,
 	}
 
 	/* we can not wake up if we're running on slow clock */
-	atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
-	if (atmel_serial_clk_will_stop())
-		device_set_wakeup_enable(&pdev->dev, 0);
+	atmel_port->may_wakeup = device_may_wakeup(dev);
+	if (slow_clock)
+		device_set_wakeup_enable(dev, 0);
 
 	uart_suspend_port(&atmel_uart, port);
 
 	return 0;
 }
 
-static int atmel_serial_resume(struct platform_device *pdev)
+static int atmel_serial_suspend_standby(struct device *dev)
 {
-	struct uart_port *port = platform_get_drvdata(pdev);
-	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	return atmel_serial_suspend(dev, 0);
+}
+
+static int atmel_serial_suspend_mem(struct device *dev)
+{
+	return atmel_serial_suspend(dev, 1);
+}
+
+static int atmel_serial_resume(struct device *dev)
+{
+	struct atmel_uart_port *atmel_port = dev_get_drvdata(dev);
+	struct uart_port *port = &atmel_port->uart;
 
 	uart_resume_port(&atmel_uart, port);
-	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
+	device_set_wakeup_enable(dev, atmel_port->may_wakeup);
 
 	return 0;
 }
-#else
-#define atmel_serial_suspend NULL
-#define atmel_serial_resume NULL
 #endif
 
+static struct dev_pm_ops atmel_serial_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(atmel_serial_suspend_standby, atmel_serial_resume)
+	SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(atmel_serial_suspend_standby,
+				atmel_serial_suspend_mem)
+};
+
 static int __devinit atmel_serial_probe(struct platform_device *pdev)
 {
 	struct atmel_uart_port *port;
@@ -1877,11 +1880,10 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
 static struct platform_driver atmel_serial_driver = {
 	.probe		= atmel_serial_probe,
 	.remove		= __devexit_p(atmel_serial_remove),
-	.suspend	= atmel_serial_suspend,
-	.resume		= atmel_serial_resume,
 	.driver		= {
 		.name	= "atmel_usart",
 		.owner	= THIS_MODULE,
+		.pm	= &atmel_serial_pm_ops,
 		.of_match_table	= of_match_ptr(atmel_serial_dt_ids),
 	},
 };
-- 
1.7.10.4

[PATCH 4/6] usb: ohci-at91: switch pm ops

[Jean-Christophe PLAGNIOL-VILLARD]

so we can detect when we enter in slow_clock mode and drop
at91_suspend_entering_slow_clock

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
Cc: linux-usb at vger.kernel.org
---
 drivers/usb/host/ohci-at91.c |   38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 0bf72f9..316897d 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -668,14 +668,12 @@ static int __devexit ohci_hcd_at91_drv_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-
-static int
-ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t mesg)
+static int ohci_hcd_at91_drv_suspend(struct device *dev, int slow_clock)
 {
-	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
+	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
 
-	if (device_may_wakeup(&pdev->dev))
+	if (device_may_wakeup(dev))
 		enable_irq_wake(hcd->irq);
 
 	/*
@@ -685,7 +683,7 @@ ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t mesg)
 	 *
 	 * REVISIT: some boards will be able to turn VBUS off...
 	 */
-	if (at91_suspend_entering_slow_clock()) {
+	if (slow_clock) {
 		ohci_usb_reset (ohci);
 		/* flush the writes */
 		(void) ohci_readl (ohci, &ohci->regs->control);
@@ -695,11 +693,21 @@ ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t mesg)
 	return 0;
 }
 
-static int ohci_hcd_at91_drv_resume(struct platform_device *pdev)
+static int ohci_hcd_at91_drv_suspend_standby(struct device *dev)
+{
+	return ohci_hcd_at91_drv_suspend(dev, 0);
+}
+
+static int ohci_hcd_at91_drv_suspend_mem(struct device *dev)
+{
+	return ohci_hcd_at91_drv_suspend(dev, 1);
+}
+
+static int ohci_hcd_at91_drv_resume(struct device *dev)
 {
-	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
+	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 
-	if (device_may_wakeup(&pdev->dev))
+	if (device_may_wakeup(dev))
 		disable_irq_wake(hcd->irq);
 
 	if (!clocked)
@@ -708,22 +716,24 @@ static int ohci_hcd_at91_drv_resume(struct platform_device *pdev)
 	ohci_finish_controller_resume(hcd);
 	return 0;
 }
-#else
-#define ohci_hcd_at91_drv_suspend NULL
-#define ohci_hcd_at91_drv_resume  NULL
 #endif
 
+static struct dev_pm_ops ohci_hcd_at91_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ohci_hcd_at91_drv_suspend_standby, ohci_hcd_at91_drv_resume)
+	SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(ohci_hcd_at91_drv_suspend_standby,
+				ohci_hcd_at91_drv_suspend_mem)
+};
+
 MODULE_ALIAS("platform:at91_ohci");
 
 static struct platform_driver ohci_hcd_at91_driver = {
 	.probe		= ohci_hcd_at91_drv_probe,
 	.remove		= __devexit_p(ohci_hcd_at91_drv_remove),
 	.shutdown	= usb_hcd_platform_shutdown,
-	.suspend	= ohci_hcd_at91_drv_suspend,
-	.resume		= ohci_hcd_at91_drv_resume,
 	.driver		= {
 		.name	= "at91_ohci",
 		.owner	= THIS_MODULE,
+		.pm	= &ohci_hcd_at91_pm_ops,
 		.of_match_table	= of_match_ptr(at91_ohci_dt_ids),
 	},
 };
-- 
1.7.10.4

[PATCH 5/6] usb: at91_ude: switch pm ops

[Jean-Christophe PLAGNIOL-VILLARD]

so we can detect when we enter in slow_clock mode and drop
at91_suspend_entering_slow_clock

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
Cc: linux-usb at vger.kernel.org 
---
 drivers/usb/gadget/at91_udc.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
index 89d90b5..ba5b0af 100644
--- a/drivers/usb/gadget/at91_udc.c
+++ b/drivers/usb/gadget/at91_udc.c
@@ -1910,10 +1910,10 @@ static int __exit at91udc_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int at91udc_suspend(struct platform_device *pdev, pm_message_t mesg)
+static int at91udc_suspend(struct device *dev, int slow_clock)
 {
-	struct at91_udc *udc = platform_get_drvdata(pdev);
-	int		wake = udc->driver && device_may_wakeup(&pdev->dev);
+	struct at91_udc *udc = dev_get_drvdata(dev);
+	int		wake = udc->driver && device_may_wakeup(dev);
 	unsigned long	flags;
 
 	/* Unless we can act normally to the host (letting it wake us up
@@ -1923,7 +1923,7 @@ static int at91udc_suspend(struct platform_device *pdev, pm_message_t mesg)
 	 */
 	if ((!udc->suspended && udc->addr)
 			|| !wake
-			|| at91_suspend_entering_slow_clock()) {
+			|| slow_clock) {
 		spin_lock_irqsave(&udc->lock, flags);
 		pullup(udc, 0);
 		wake = 0;
@@ -1937,9 +1937,19 @@ static int at91udc_suspend(struct platform_device *pdev, pm_message_t mesg)
 	return 0;
 }
 
-static int at91udc_resume(struct platform_device *pdev)
+static int at91udc_suspend_standby(struct device *dev)
 {
-	struct at91_udc *udc = platform_get_drvdata(pdev);
+	return at91udc_suspend(dev, 0);
+}
+
+static int at91udc_suspend_mem(struct device *dev)
+{
+	return at91udc_suspend(dev, 1);
+}
+
+static int at91udc_resume(struct device *dev)
+{
+	struct at91_udc *udc = dev_get_drvdata(dev);
 	unsigned long	flags;
 
 	if (gpio_is_valid(udc->board.vbus_pin) && !udc->board.vbus_polled &&
@@ -1956,11 +1966,14 @@ static int at91udc_resume(struct platform_device *pdev)
 	}
 	return 0;
 }
-#else
-#define	at91udc_suspend	NULL
-#define	at91udc_resume	NULL
 #endif
 
+static struct dev_pm_ops at91_udc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(at91udc_suspend_standby, at91udc_resume)
+	SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(at91udc_suspend_standby,
+				at91udc_suspend_mem)
+};
+
 #if defined(CONFIG_OF)
 static const struct of_device_id at91_udc_dt_ids[] = {
 	{ .compatible = "atmel,at91rm9200-udc" },
@@ -1973,11 +1986,10 @@ MODULE_DEVICE_TABLE(of, at91_udc_dt_ids);
 static struct platform_driver at91_udc_driver = {
 	.remove		= __exit_p(at91udc_remove),
 	.shutdown	= at91udc_shutdown,
-	.suspend	= at91udc_suspend,
-	.resume		= at91udc_resume,
 	.driver		= {
 		.name	= (char *) driver_name,
 		.owner	= THIS_MODULE,
+		.pm	= &at91_udc_pm_ops,
 		.of_match_table	= of_match_ptr(at91_udc_dt_ids),
 	},
 };
-- 
1.7.10.4

[PATCH 6/6] arm: at91: drop at91_suspend_entering_slow_clock

[Jean-Christophe PLAGNIOL-VILLARD]

as the driver needing to knwon if we enter in slow_clock mode (SUSPEND_MEM)
have been swtich to pm ops

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-pm at vger.kernel.org
---
 arch/arm/mach-at91/include/mach/board.h |    3 ---
 arch/arm/mach-at91/pm.c                 |   24 ------------------------
 2 files changed, 27 deletions(-)

diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index c55a436..e3ca86a 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -190,7 +190,4 @@ extern void __init at91_add_device_can(struct at91_can_data *data);
 extern void __init at91_gpio_leds(struct gpio_led *leds, int nr);
 extern void __init at91_pwm_leds(struct gpio_led *leds, int nr);
 
-/* FIXME: this needs a better location, but gets stuff building again */
-extern int at91_suspend_entering_slow_clock(void);
-
 #endif
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 2c2d865..5e8d770 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -116,15 +116,11 @@ static int at91_pm_valid_state(suspend_state_t state)
 	}
 }
 
-
-static suspend_state_t target_state;
-
 /*
  * Called after processes are frozen, but before we shutdown devices.
  */
 static int at91_pm_begin(suspend_state_t state)
 {
-	target_state = state;
 	return 0;
 }
 
@@ -172,23 +168,6 @@ static int at91_pm_verify_clocks(void)
 	return 1;
 }
 
-/*
- * Call this from platform driver suspend() to see how deeply to suspend.
- * For example, some controllers (like OHCI) need one of the PLL clocks
- * in order to act as a wakeup source, and those are not available when
- * going into slow clock mode.
- *
- * REVISIT: generalize as clk_will_be_available(clk)?  Other platforms have
- * the very same problem (but not using at91 main_clk), and it'd be better
- * to add one generic API rather than lots of platform-specific ones.
- */
-int at91_suspend_entering_slow_clock(void)
-{
-	return (target_state == PM_SUSPEND_MEM);
-}
-EXPORT_SYMBOL(at91_suspend_entering_slow_clock);
-
-
 static void (*slow_clock)(void __iomem *pmc, void __iomem *ramc0,
 			  void __iomem *ramc1, int memctrl);
 
@@ -283,7 +262,6 @@ static int at91_pm_enter(suspend_state_t state)
 			at91_aic_read(AT91_AIC_IPR) & at91_aic_read(AT91_AIC_IMR));
 
 error:
-	target_state = PM_SUSPEND_ON;
 	at91_irq_resume();
 	at91_gpio_resume();
 	return 0;
@@ -294,10 +272,8 @@ error:
  */
 static void at91_pm_end(void)
 {
-	target_state = PM_SUSPEND_ON;
 }
 
-
 static const struct platform_suspend_ops at91_pm_ops = {
 	.valid	= at91_pm_valid_state,
 	.begin	= at91_pm_begin,
-- 
1.7.10.4

[PATCH 1/6] pm: add suspend_mem and suspend_standby support

[Rafael J. Wysocki]

On Sunday 07 of October 2012 09:27:15 Jean-Christophe PLAGNIOL-VILLARD wrote:
> Today when we go to suspend we can not knwon at drivers level if we go in
> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> suspend_standby.
> 
> If a bus does not need to care about this distinction we fallback to
> suspend. This will allow to do not update the current bus or drivers.
> 
> This is needed as example by at91 OHCI and atmel serial
> 
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-pm at vger.kernel.org

So for me, this patch is a total no-go.

Apart from the fact that I don't see any difference between .suspend()
and .suspend_mem(), it doesn't cover the late/early and noirq phases.

> ---
>  drivers/base/power/main.c |   12 ++++++++++++
>  include/linux/pm.h        |    9 +++++++++
>  kernel/power/suspend.c    |   16 ++++++++++++++--
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index a3c1404..581e135 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -218,9 +218,21 @@ static void dpm_wait_for_children(struct device *dev, bool async)
>   */
>  static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
>  {
> +	pm_callback_t callback = NULL;
> +
>  	switch (state.event) {
>  #ifdef CONFIG_SUSPEND
>  	case PM_EVENT_SUSPEND:
> +		switch (state.param) {
> +		case PM_SUSPEND_MEM:
> +			callback = ops->suspend_mem;
> +			break;
> +		case PM_SUSPEND_STANDBY:
> +			callback = ops->suspend_standby;
> +			break;
> +		}
> +		if (callback)
> +			return callback;
>  		return ops->suspend;
>  	case PM_EVENT_RESUME:
>  		return ops->resume;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 007e687..aff344b 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -49,6 +49,7 @@ extern const char power_group_name[];		/* = "power" */
>  
>  typedef struct pm_message {
>  	int event;
> +	int param;
>  } pm_message_t;

Please don't do that.  We have the struct platform_suspend_ops with callbacks
that take the state argument and we pass the information about the system sleep
state being entered in there.  The way this information is interpreted depends
on the platform and the actions of device drivers resulting from that must be
platform-dependent too.  Your design makes a very strong assumption that the
interpretation of "standby" by the platform and device drivers will always
be the same, which generally need not be the case.

[And what if the given platform actually has more than two sleep states?
Are you going to add a separate callback for each of them?]

Make your drivers ask the platform what power state to put the devices into
instead of doing this.
 
>  /**
> @@ -265,6 +266,8 @@ struct dev_pm_ops {
>  	int (*prepare)(struct device *dev);
>  	void (*complete)(struct device *dev);
>  	int (*suspend)(struct device *dev);
> +	int (*suspend_mem)(struct device *dev);
> +	int (*suspend_standby)(struct device *dev);
>  	int (*resume)(struct device *dev);
>  	int (*freeze)(struct device *dev);
>  	int (*thaw)(struct device *dev);
> @@ -295,8 +298,12 @@ struct dev_pm_ops {
>  	.thaw = resume_fn, \
>  	.poweroff = suspend_fn, \
>  	.restore = resume_fn,
> +#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn) \
> +	.suspend_standby = suspend_standby_fn, \
> +	.suspend_mem = suspend_mem_fn,
>  #else
>  #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> +#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn)
>  #endif
>  
>  #ifdef CONFIG_PM_RUNTIME
> @@ -414,6 +421,8 @@ const struct dev_pm_ops name = { \
>  #define PMSG_FREEZE	((struct pm_message){ .event = PM_EVENT_FREEZE, })
>  #define PMSG_QUIESCE	((struct pm_message){ .event = PM_EVENT_QUIESCE, })
>  #define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
> +#define PMSG_SUSPEND_MEM	((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_MEM, })
> +#define PMSG_SUSPEND_STANDBY	((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_STANDBY, })
>  #define PMSG_HIBERNATE	((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
>  #define PMSG_RESUME	((struct pm_message){ .event = PM_EVENT_RESUME, })
>  #define PMSG_THAW	((struct pm_message){ .event = PM_EVENT_THAW, })
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c8b7446..9505b55 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -126,6 +126,18 @@ void __attribute__ ((weak)) arch_suspend_enable_irqs(void)
>  	local_irq_enable();
>  }
>  
> +static pm_message_t suspend_state_to_pm_state(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_STANDBY:
> +		return PMSG_SUSPEND_STANDBY;
> +	case PM_SUSPEND_MEM:
> +		return PMSG_SUSPEND_MEM;
> +	default:
> +		return PMSG_SUSPEND;
> +	}
> +}

Well, this is more than ugly.

> +
>  /**
>   * suspend_enter - Make the system enter the given sleep state.
>   * @state: System sleep state to enter.
> @@ -143,7 +155,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>  			goto Platform_finish;
>  	}
>  
> -	error = dpm_suspend_end(PMSG_SUSPEND);
> +	error = dpm_suspend_end(suspend_state_to_pm_state(state));
>  	if (error) {
>  		printk(KERN_ERR "PM: Some devices failed to power down\n");
>  		goto Platform_finish;
> @@ -215,7 +227,7 @@ int suspend_devices_and_enter(suspend_state_t state)
>  	suspend_console();
>  	ftrace_stop();
>  	suspend_test_start();
> -	error = dpm_suspend_start(PMSG_SUSPEND);
> +	error = dpm_suspend_start(suspend_state_to_pm_state(state));
>  	if (error) {
>  		printk(KERN_ERR "PM: Some devices failed to suspend\n");
>  		goto Recover_platform;

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.