Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

[Scott Wood]

On Mon, 2014-04-28 at 13:53 +0800, Leo Li wrote:
> On Sat, Apr 26, 2014 at 5:45 AM, Scott Wood <scottwood@freescale.com> wrote:
> > On Thu, 2014-04-24 at 14:11 +0800, Dongsheng Wang wrote:
> >> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> >>
> >> Add set_pm_suspend_state & pm_suspend_state functions to set/get
> >> suspend state. When system going to sleep or deep sleep, devices
> >> can get the system suspend state(STANDBY/MEM) through pm_suspend_state
> >> function and to handle different situations.
> >>
> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> >> ---
> >> *v2*
> >> Move pm api from fsl platform to powerpc general framework.
> >
> > What is powerpc-specific about this?
> 
> Generally I agree with you.  But I had the discussion about this topic
> a while ago with the PM maintainer.  He suggestion to go with the
> platform way.
> 
> https://lkml.org/lkml/2013/8/16/505

If what he meant was whether you could do what this patch does, then you
can answer him with, "No, because it got nacked as not being platform or
arch specific."  Oh, and you're still using .valid as the hook to set
the platform state, which is awful -- I think .begin is what you want to
use.

If we did it in powerpc code, then what would we do on ARM?  Copy the
code?  No.

Now, a more legitimate objection to putting it in generic code might be
that "standby" and "mem" are loosely defined and the knowledge of how a
driver should react to each is platform specific -- but your patch
doesn't address that.  You still have the driver itself interpret what
"standby" and "mem" mean.

As for "in analogy with ACPI suspend operations", can someone familiar
with ACPI explain what is meant?

-Scott

Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

[Rafael J. Wysocki]

On Tuesday, April 29, 2014 05:47:12 PM Scott Wood wrote:
> On Mon, 2014-04-28 at 13:53 +0800, Leo Li wrote:
> > On Sat, Apr 26, 2014 at 5:45 AM, Scott Wood <scottwood@freescale.com> wrote:
> > > On Thu, 2014-04-24 at 14:11 +0800, Dongsheng Wang wrote:
> > >> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > >>
> > >> Add set_pm_suspend_state & pm_suspend_state functions to set/get
> > >> suspend state. When system going to sleep or deep sleep, devices
> > >> can get the system suspend state(STANDBY/MEM) through pm_suspend_state
> > >> function and to handle different situations.
> > >>
> > >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > >> ---
> > >> *v2*
> > >> Move pm api from fsl platform to powerpc general framework.
> > >
> > > What is powerpc-specific about this?
> > 
> > Generally I agree with you.  But I had the discussion about this topic
> > a while ago with the PM maintainer.  He suggestion to go with the
> > platform way.
> > 
> > https://lkml.org/lkml/2013/8/16/505
> 
> If what he meant was whether you could do what this patch does, then you
> can answer him with, "No, because it got nacked as not being platform or
> arch specific."  Oh, and you're still using .valid as the hook to set
> the platform state, which is awful -- I think .begin is what you want to
> use.
> 
> If we did it in powerpc code, then what would we do on ARM?  Copy the
> code?  No.
> 
> Now, a more legitimate objection to putting it in generic code might be
> that "standby" and "mem" are loosely defined and the knowledge of how a
> driver should react to each is platform specific -- but your patch
> doesn't address that.  You still have the driver itself interpret what
> "standby" and "mem" mean.
> 
> As for "in analogy with ACPI suspend operations", can someone familiar
> with ACPI explain what is meant?

ACPI defines sleep states S3 and S1 which are mappend to "mem" and
"standby" currently, but that may change in the future.

Generally speaking the meaning of "mem" and "standby" is platform-specific
except that "mem" should be a deeper (lower-power) sleep state than "standby".

Thanks!

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

Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

[Li Yang]

On Wed, Apr 30, 2014 at 6:47 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, 2014-04-28 at 13:53 +0800, Leo Li wrote:
>> On Sat, Apr 26, 2014 at 5:45 AM, Scott Wood <scottwood@freescale.com> wrote:
>> > On Thu, 2014-04-24 at 14:11 +0800, Dongsheng Wang wrote:
>> >> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>> >>
>> >> Add set_pm_suspend_state & pm_suspend_state functions to set/get
>> >> suspend state. When system going to sleep or deep sleep, devices
>> >> can get the system suspend state(STANDBY/MEM) through pm_suspend_state
>> >> function and to handle different situations.
>> >>
>> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>> >> ---
>> >> *v2*
>> >> Move pm api from fsl platform to powerpc general framework.
>> >
>> > What is powerpc-specific about this?
>>
>> Generally I agree with you.  But I had the discussion about this topic
>> a while ago with the PM maintainer.  He suggestion to go with the
>> platform way.
>>
>> https://lkml.org/lkml/2013/8/16/505
>
> If what he meant was whether you could do what this patch does, then you
> can answer him with, "No, because it got nacked as not being platform or
> arch specific."  Oh, and you're still using .valid as the hook to set
> the platform state, which is awful -- I think .begin is what you want to
> use.

I'm not saying the current patch is good for upstream.  Actually I did
say that the patch need to be updated for upstream purpose.  I only
meant that we discussed about having the mem/standby passed by generic
kernel/power interface as you suggested internally and got an negative
feedback.

>
> If we did it in powerpc code, then what would we do on ARM?  Copy the
> code?  No.

If you are saying that this shouldn't be done in arch/powerpc  Yes.
We have determined to use drivers/platform folder for the re-used code
with ARM.  Platform power management code will be moved there.

>
> Now, a more legitimate objection to putting it in generic code might be
> that "standby" and "mem" are loosely defined and the knowledge of how a
> driver should react to each is platform specific -- but your patch
> doesn't address that.  You still have the driver itself interpret what
> "standby" and "mem" mean.
>

Yup, we will address it in next batch.

- Leo

Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

[Scott Wood]

On Fri, 2014-05-09 at 17:33 +0800, Li Yang wrote:
> On Wed, Apr 30, 2014 at 6:47 AM, Scott Wood <scottwood@freescale.com> wrote:
> > On Mon, 2014-04-28 at 13:53 +0800, Leo Li wrote:
> >> On Sat, Apr 26, 2014 at 5:45 AM, Scott Wood <scottwood@freescale.com> wrote:
> >> > On Thu, 2014-04-24 at 14:11 +0800, Dongsheng Wang wrote:
> >> >> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> >> >>
> >> >> Add set_pm_suspend_state & pm_suspend_state functions to set/get
> >> >> suspend state. When system going to sleep or deep sleep, devices
> >> >> can get the system suspend state(STANDBY/MEM) through pm_suspend_state
> >> >> function and to handle different situations.
> >> >>
> >> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> >> >> ---
> >> >> *v2*
> >> >> Move pm api from fsl platform to powerpc general framework.
> >> >
> >> > What is powerpc-specific about this?
> >>
> >> Generally I agree with you.  But I had the discussion about this topic
> >> a while ago with the PM maintainer.  He suggestion to go with the
> >> platform way.
> >>
> >> https://lkml.org/lkml/2013/8/16/505
> >
> > If what he meant was whether you could do what this patch does, then you
> > can answer him with, "No, because it got nacked as not being platform or
> > arch specific."  Oh, and you're still using .valid as the hook to set
> > the platform state, which is awful -- I think .begin is what you want to
> > use.
> 
> I'm not saying the current patch is good for upstream.  Actually I did
> say that the patch need to be updated for upstream purpose. 

I don't follow -- this thread is an upstream submission.

> > Now, a more legitimate objection to putting it in generic code might be
> > that "standby" and "mem" are loosely defined and the knowledge of how a
> > driver should react to each is platform specific -- but your patch
> > doesn't address that.  You still have the driver itself interpret what
> > "standby" and "mem" mean.
> >
> 
> Yup, we will address it in next batch.

Thanks.

-Scott

Re: [PATCH v2 1/2] powerpc/pm: add api to get suspend state which is STANDBY or MEM

[Li Yang]

On Sat, May 10, 2014 at 1:09 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Fri, 2014-05-09 at 17:33 +0800, Li Yang wrote:
>> On Wed, Apr 30, 2014 at 6:47 AM, Scott Wood <scottwood@freescale.com> wrote:
>> > On Mon, 2014-04-28 at 13:53 +0800, Leo Li wrote:
>> >> On Sat, Apr 26, 2014 at 5:45 AM, Scott Wood <scottwood@freescale.com> wrote:
>> >> > On Thu, 2014-04-24 at 14:11 +0800, Dongsheng Wang wrote:
>> >> >> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>> >> >>
>> >> >> Add set_pm_suspend_state & pm_suspend_state functions to set/get
>> >> >> suspend state. When system going to sleep or deep sleep, devices
>> >> >> can get the system suspend state(STANDBY/MEM) through pm_suspend_state
>> >> >> function and to handle different situations.
>> >> >>
>> >> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>> >> >> ---
>> >> >> *v2*
>> >> >> Move pm api from fsl platform to powerpc general framework.
>> >> >
>> >> > What is powerpc-specific about this?
>> >>
>> >> Generally I agree with you.  But I had the discussion about this topic
>> >> a while ago with the PM maintainer.  He suggestion to go with the
>> >> platform way.
>> >>
>> >> https://lkml.org/lkml/2013/8/16/505
>> >
>> > If what he meant was whether you could do what this patch does, then you
>> > can answer him with, "No, because it got nacked as not being platform or
>> > arch specific."  Oh, and you're still using .valid as the hook to set
>> > the platform state, which is awful -- I think .begin is what you want to
>> > use.
>>
>> I'm not saying the current patch is good for upstream.  Actually I did
>> say that the patch need to be updated for upstream purpose.
>
> I don't follow -- this thread is an upstream submission.

Thought you were suggesting to change the generic PM interface for
this as discussed internally.  So I was just providing the information
about previous discussion.  Nothing more.

Regards,
Leo