Patchwork pm: add suspend_mem and suspend_standby support

login
register
mail settings
Submitter Jean-Christophe PLAGNIOL-VILLARD
Date Oct. 6, 2012, 4:14 p.m.
Message ID <20121006161429.GD12462@game.jcrosoft.org>
Download mbox
Permalink /patch/189745/
State New
Headers show

Pull-request

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

Comments

Jean-Christophe PLAGNIOL-VILLARD - Oct. 6, 2012, 4:14 p.m.
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.
Rafael J. Wysocki - Oct. 6, 2012, 10:18 p.m.
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
Greg KH - Oct. 7, 2012, 4:01 a.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 7, 2012, 1:09 p.m.
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.
Jean-Christophe PLAGNIOL-VILLARD - Oct. 7, 2012, 1:12 p.m.
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.
Rafael J. Wysocki - Oct. 7, 2012, 8:02 p.m.
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
Rafael J. Wysocki - Oct. 7, 2012, 8:06 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 9, 2012, 11:46 a.m.
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.
Greg KH - Oct. 9, 2012, 2:58 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 9, 2012, 3:17 p.m.
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.
Alan Stern - Oct. 9, 2012, 3:44 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 9, 2012, 3:52 p.m.
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.
Rafael J. Wysocki - Oct. 9, 2012, 10:09 p.m.
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
Rafael J. Wysocki - Oct. 9, 2012, 10:12 p.m.
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
Rafael J. Wysocki - Oct. 9, 2012, 10:18 p.m.
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
Rafael J. Wysocki - Oct. 9, 2012, 10:52 p.m.
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@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
hvaibhav@ti.com - Oct. 10, 2012, 7:17 a.m.
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
> 
>
Daniel Mack - Oct. 10, 2012, 9:20 a.m.
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
hvaibhav@ti.com - Oct. 10, 2012, 9:29 a.m.
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
> 
>
Daniel Mack - Oct. 10, 2012, 9:33 a.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 10, 2012, 9:43 a.m.
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.
Rafael J. Wysocki - Oct. 10, 2012, 11:05 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - Oct. 13, 2012, 6:46 a.m.
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.
Rafael J. Wysocki - Oct. 14, 2012, 7:12 a.m.
[...]

> > > 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
Daniel Mack - Oct. 31, 2012, 10:27 a.m.
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
Bedia, Vaibhav - Oct. 31, 2012, 10:44 a.m.
(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