Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Tue, 20 Sep 2016, Chen, Yu C wrote:
> > -Original Message-
> > From: Wysocki, Rafael J
> > Sent: Tuesday, September 13, 2016 8:16 PM
> > To: Lee Jones; Chen, Yu C
> > Cc: linux-kernel@vger.kernel.org; Andy Shevchenko; Mika Westerberg
> > Subject: Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
> > lpss unnecessarily
> > 
> > On 9/13/2016 10:24 AM, Lee Jones wrote:
> > > On Mon, 12 Sep 2016, Chen Yu wrote:
> > >> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> > >>> On Sun, 04 Sep 2016, Chen Yu wrote:
> > >>>
> > >>>> We have report that the intel_lpss_prepare() takes too much time
> > >>>> during suspend, and this is because we first resume the devices
> > >>>> from runtime suspend by resume_lpss_device(), to make sure they are
> > >>>> in proper state before system suspend, which takes 100ms for each
> > >>>> LPSS devices(PCI power state from D3_cold to D0). And since
> > >>>> resume_lpss_device() resumes the devices synchronously, we might
> > >>>> get huge latency if we have many LPSS devices.
> > >>>>
> > >>>> So first try is to use pm_request_resume() instead, to make the
> > >>>> runtime resume process asynchronously. Unfortunately the
> > >>>> asynchronous runtime resume relies on pm_wq, which is freezed at
> > >>>> early stage. So we choose another method, that is to avoid resuming
> > >>>> runtime-suspended devices, if they are already runtime suspended.
> > >>>> This is safe because for LPSS driver, the runtime suspend and
> > >>>> system suspend are of the same hook - i.e., intel_lpss_suspend().
> > >>>> And moreover, this device is neither runtime wakeup source nor system
> > wakeup source.
> > >>>>
> > >>>> Cc: Andy Shevchenko <andriy.shevche...@linux.intel.com>
> > >>>> Cc: Mika Westerberg <mika.westerb...@linux.intel.com>
> > >>>> Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > >>>> Signed-off-by: Chen Yu <yu.c.c...@intel.com>
> > >>>> ---
> > >>>>   drivers/mfd/intel-lpss.c | 9 +
> > >>>>   1 file changed, 9 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > >>>> index 41b1138..6dcc9a0 100644
> > >>>> --- a/drivers/mfd/intel-lpss.c
> > >>>> +++ b/drivers/mfd/intel-lpss.c
> > >>>> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev,
> > void *data)
> > >>>>   int intel_lpss_prepare(struct device *dev)
> > >>>>   {
> > >>>>/*
> > >>>> +   * This is safe because:
> > >>>> +   * 1. The runtime suspend and system suspend
> > >>>> +   * are of the same hook.
> > >>>> +   * 2. This device is neither runtime wakeup source
> > >>>> +   * nor system wakeup source.
> > >>>> +   */
> > >>>> +  if (pm_runtime_status_suspended(dev))
> > >>>> +  return 1;
> > >>> What's '1'?
> > >>>
> > >> According to the comment in device_prepare():
> > >>
> > >> A positive return value from ->prepare() means "this device appears
> > >> to be runtime-suspended and its state is fine, so if it really is
> > >> runtime-suspended, you can leave it in that state provided that you
> > >> will do the same thing with all of its descendants".
> > > Are there no defines for this?
> > >
> > 
> > Not at the moment, but I guess they can be added if really necessary. :-)
> > 
> > But that said it doesn't have to be 1 or any specific value. Any positive 
> > number
> > will have the same effect.
> Thanks for point it out,  Hi Lee, should I  repost a patch set to define the 
> return value
> and make this one based on that? 

I think that would be a great way to move forward.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Tue, 20 Sep 2016, Chen, Yu C wrote:
> > -Original Message-
> > From: Wysocki, Rafael J
> > Sent: Tuesday, September 13, 2016 8:16 PM
> > To: Lee Jones; Chen, Yu C
> > Cc: linux-kernel@vger.kernel.org; Andy Shevchenko; Mika Westerberg
> > Subject: Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
> > lpss unnecessarily
> > 
> > On 9/13/2016 10:24 AM, Lee Jones wrote:
> > > On Mon, 12 Sep 2016, Chen Yu wrote:
> > >> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> > >>> On Sun, 04 Sep 2016, Chen Yu wrote:
> > >>>
> > >>>> We have report that the intel_lpss_prepare() takes too much time
> > >>>> during suspend, and this is because we first resume the devices
> > >>>> from runtime suspend by resume_lpss_device(), to make sure they are
> > >>>> in proper state before system suspend, which takes 100ms for each
> > >>>> LPSS devices(PCI power state from D3_cold to D0). And since
> > >>>> resume_lpss_device() resumes the devices synchronously, we might
> > >>>> get huge latency if we have many LPSS devices.
> > >>>>
> > >>>> So first try is to use pm_request_resume() instead, to make the
> > >>>> runtime resume process asynchronously. Unfortunately the
> > >>>> asynchronous runtime resume relies on pm_wq, which is freezed at
> > >>>> early stage. So we choose another method, that is to avoid resuming
> > >>>> runtime-suspended devices, if they are already runtime suspended.
> > >>>> This is safe because for LPSS driver, the runtime suspend and
> > >>>> system suspend are of the same hook - i.e., intel_lpss_suspend().
> > >>>> And moreover, this device is neither runtime wakeup source nor system
> > wakeup source.
> > >>>>
> > >>>> Cc: Andy Shevchenko 
> > >>>> Cc: Mika Westerberg 
> > >>>> Cc: Rafael J. Wysocki 
> > >>>> Signed-off-by: Chen Yu 
> > >>>> ---
> > >>>>   drivers/mfd/intel-lpss.c | 9 +
> > >>>>   1 file changed, 9 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > >>>> index 41b1138..6dcc9a0 100644
> > >>>> --- a/drivers/mfd/intel-lpss.c
> > >>>> +++ b/drivers/mfd/intel-lpss.c
> > >>>> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev,
> > void *data)
> > >>>>   int intel_lpss_prepare(struct device *dev)
> > >>>>   {
> > >>>>/*
> > >>>> +   * This is safe because:
> > >>>> +   * 1. The runtime suspend and system suspend
> > >>>> +   * are of the same hook.
> > >>>> +   * 2. This device is neither runtime wakeup source
> > >>>> +   * nor system wakeup source.
> > >>>> +   */
> > >>>> +  if (pm_runtime_status_suspended(dev))
> > >>>> +  return 1;
> > >>> What's '1'?
> > >>>
> > >> According to the comment in device_prepare():
> > >>
> > >> A positive return value from ->prepare() means "this device appears
> > >> to be runtime-suspended and its state is fine, so if it really is
> > >> runtime-suspended, you can leave it in that state provided that you
> > >> will do the same thing with all of its descendants".
> > > Are there no defines for this?
> > >
> > 
> > Not at the moment, but I guess they can be added if really necessary. :-)
> > 
> > But that said it doesn't have to be 1 or any specific value. Any positive 
> > number
> > will have the same effect.
> Thanks for point it out,  Hi Lee, should I  repost a patch set to define the 
> return value
> and make this one based on that? 

I think that would be a great way to move forward.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Chen, Yu C]

Hi,
> -Original Message-
> From: Wysocki, Rafael J
> Sent: Tuesday, September 13, 2016 8:16 PM
> To: Lee Jones; Chen, Yu C
> Cc: linux-kernel@vger.kernel.org; Andy Shevchenko; Mika Westerberg
> Subject: Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
> lpss unnecessarily
> 
> On 9/13/2016 10:24 AM, Lee Jones wrote:
> > On Mon, 12 Sep 2016, Chen Yu wrote:
> >> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> >>> On Sun, 04 Sep 2016, Chen Yu wrote:
> >>>
> >>>> We have report that the intel_lpss_prepare() takes too much time
> >>>> during suspend, and this is because we first resume the devices
> >>>> from runtime suspend by resume_lpss_device(), to make sure they are
> >>>> in proper state before system suspend, which takes 100ms for each
> >>>> LPSS devices(PCI power state from D3_cold to D0). And since
> >>>> resume_lpss_device() resumes the devices synchronously, we might
> >>>> get huge latency if we have many LPSS devices.
> >>>>
> >>>> So first try is to use pm_request_resume() instead, to make the
> >>>> runtime resume process asynchronously. Unfortunately the
> >>>> asynchronous runtime resume relies on pm_wq, which is freezed at
> >>>> early stage. So we choose another method, that is to avoid resuming
> >>>> runtime-suspended devices, if they are already runtime suspended.
> >>>> This is safe because for LPSS driver, the runtime suspend and
> >>>> system suspend are of the same hook - i.e., intel_lpss_suspend().
> >>>> And moreover, this device is neither runtime wakeup source nor system
> wakeup source.
> >>>>
> >>>> Cc: Andy Shevchenko <andriy.shevche...@linux.intel.com>
> >>>> Cc: Mika Westerberg <mika.westerb...@linux.intel.com>
> >>>> Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> >>>> Signed-off-by: Chen Yu <yu.c.c...@intel.com>
> >>>> ---
> >>>>   drivers/mfd/intel-lpss.c | 9 +
> >>>>   1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> >>>> index 41b1138..6dcc9a0 100644
> >>>> --- a/drivers/mfd/intel-lpss.c
> >>>> +++ b/drivers/mfd/intel-lpss.c
> >>>> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev,
> void *data)
> >>>>   int intel_lpss_prepare(struct device *dev)
> >>>>   {
> >>>>  /*
> >>>> + * This is safe because:
> >>>> + * 1. The runtime suspend and system suspend
> >>>> + * are of the same hook.
> >>>> + * 2. This device is neither runtime wakeup source
> >>>> + * nor system wakeup source.
> >>>> + */
> >>>> +if (pm_runtime_status_suspended(dev))
> >>>> +return 1;
> >>> What's '1'?
> >>>
> >> According to the comment in device_prepare():
> >>
> >> A positive return value from ->prepare() means "this device appears
> >> to be runtime-suspended and its state is fine, so if it really is
> >> runtime-suspended, you can leave it in that state provided that you
> >> will do the same thing with all of its descendants".
> > Are there no defines for this?
> >
> 
> Not at the moment, but I guess they can be added if really necessary. :-)
> 
> But that said it doesn't have to be 1 or any specific value. Any positive 
> number
> will have the same effect.
Thanks for point it out,  Hi Lee, should I  repost a patch set to define the 
return value
and make this one based on that? 

Thanks,
Yu

RE: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Chen, Yu C]

Hi,
> -Original Message-
> From: Wysocki, Rafael J
> Sent: Tuesday, September 13, 2016 8:16 PM
> To: Lee Jones; Chen, Yu C
> Cc: linux-kernel@vger.kernel.org; Andy Shevchenko; Mika Westerberg
> Subject: Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
> lpss unnecessarily
> 
> On 9/13/2016 10:24 AM, Lee Jones wrote:
> > On Mon, 12 Sep 2016, Chen Yu wrote:
> >> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> >>> On Sun, 04 Sep 2016, Chen Yu wrote:
> >>>
> >>>> We have report that the intel_lpss_prepare() takes too much time
> >>>> during suspend, and this is because we first resume the devices
> >>>> from runtime suspend by resume_lpss_device(), to make sure they are
> >>>> in proper state before system suspend, which takes 100ms for each
> >>>> LPSS devices(PCI power state from D3_cold to D0). And since
> >>>> resume_lpss_device() resumes the devices synchronously, we might
> >>>> get huge latency if we have many LPSS devices.
> >>>>
> >>>> So first try is to use pm_request_resume() instead, to make the
> >>>> runtime resume process asynchronously. Unfortunately the
> >>>> asynchronous runtime resume relies on pm_wq, which is freezed at
> >>>> early stage. So we choose another method, that is to avoid resuming
> >>>> runtime-suspended devices, if they are already runtime suspended.
> >>>> This is safe because for LPSS driver, the runtime suspend and
> >>>> system suspend are of the same hook - i.e., intel_lpss_suspend().
> >>>> And moreover, this device is neither runtime wakeup source nor system
> wakeup source.
> >>>>
> >>>> Cc: Andy Shevchenko 
> >>>> Cc: Mika Westerberg 
> >>>> Cc: Rafael J. Wysocki 
> >>>> Signed-off-by: Chen Yu 
> >>>> ---
> >>>>   drivers/mfd/intel-lpss.c | 9 +
> >>>>   1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> >>>> index 41b1138..6dcc9a0 100644
> >>>> --- a/drivers/mfd/intel-lpss.c
> >>>> +++ b/drivers/mfd/intel-lpss.c
> >>>> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev,
> void *data)
> >>>>   int intel_lpss_prepare(struct device *dev)
> >>>>   {
> >>>>  /*
> >>>> + * This is safe because:
> >>>> + * 1. The runtime suspend and system suspend
> >>>> + * are of the same hook.
> >>>> + * 2. This device is neither runtime wakeup source
> >>>> + * nor system wakeup source.
> >>>> + */
> >>>> +if (pm_runtime_status_suspended(dev))
> >>>> +return 1;
> >>> What's '1'?
> >>>
> >> According to the comment in device_prepare():
> >>
> >> A positive return value from ->prepare() means "this device appears
> >> to be runtime-suspended and its state is fine, so if it really is
> >> runtime-suspended, you can leave it in that state provided that you
> >> will do the same thing with all of its descendants".
> > Are there no defines for this?
> >
> 
> Not at the moment, but I guess they can be added if really necessary. :-)
> 
> But that said it doesn't have to be 1 or any specific value. Any positive 
> number
> will have the same effect.
Thanks for point it out,  Hi Lee, should I  repost a patch set to define the 
return value
and make this one based on that? 

Thanks,
Yu

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Rafael J. Wysocki]

On 9/13/2016 10:24 AM, Lee Jones wrote:

On Mon, 12 Sep 2016, Chen Yu wrote:

On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:

On Sun, 04 Sep 2016, Chen Yu wrote:


We have report that the intel_lpss_prepare() takes too much time during
suspend, and this is because we first resume the devices from runtime
suspend by resume_lpss_device(), to make sure they are in proper state
before system suspend, which takes 100ms for each LPSS devices(PCI power
state from D3_cold to D0). And since resume_lpss_device() resumes the
devices synchronously, we might get huge latency if we have many
LPSS devices.

So first try is to use pm_request_resume() instead, to make the runtime
resume process asynchronously. Unfortunately the asynchronous runtime
resume relies on pm_wq, which is freezed at early stage. So we choose
another method, that is to avoid resuming runtime-suspended devices,
if they are already runtime suspended. This is safe because for LPSS
driver, the runtime suspend and system suspend are of the same
hook - i.e., intel_lpss_suspend(). And moreover, this device is
neither runtime wakeup source nor system wakeup source.

Cc: Andy Shevchenko 
Cc: Mika Westerberg 
Cc: Rafael J. Wysocki 
Signed-off-by: Chen Yu 
---
  drivers/mfd/intel-lpss.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 41b1138..6dcc9a0 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
*data)
  int intel_lpss_prepare(struct device *dev)
  {
/*
+* This is safe because:
+* 1. The runtime suspend and system suspend
+* are of the same hook.
+* 2. This device is neither runtime wakeup source
+* nor system wakeup source.
+*/
+   if (pm_runtime_status_suspended(dev))
+   return 1;

What's '1'?


According to the comment in device_prepare():

A positive return value from ->prepare() means "this device appears
to be runtime-suspended and its state is fine, so if it really is
runtime-suspended, you can leave it in that state provided that you
will do the same thing with all of its descendants".

Are there no defines for this?



Not at the moment, but I guess they can be added if really necessary. :-)

But that said it doesn't have to be 1 or any specific value. Any 
positive number will have the same effect.



Thanks,

Rafael

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Rafael J. Wysocki]

On 9/13/2016 10:24 AM, Lee Jones wrote:

On Mon, 12 Sep 2016, Chen Yu wrote:

On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:

On Sun, 04 Sep 2016, Chen Yu wrote:


We have report that the intel_lpss_prepare() takes too much time during
suspend, and this is because we first resume the devices from runtime
suspend by resume_lpss_device(), to make sure they are in proper state
before system suspend, which takes 100ms for each LPSS devices(PCI power
state from D3_cold to D0). And since resume_lpss_device() resumes the
devices synchronously, we might get huge latency if we have many
LPSS devices.

So first try is to use pm_request_resume() instead, to make the runtime
resume process asynchronously. Unfortunately the asynchronous runtime
resume relies on pm_wq, which is freezed at early stage. So we choose
another method, that is to avoid resuming runtime-suspended devices,
if they are already runtime suspended. This is safe because for LPSS
driver, the runtime suspend and system suspend are of the same
hook - i.e., intel_lpss_suspend(). And moreover, this device is
neither runtime wakeup source nor system wakeup source.

Cc: Andy Shevchenko 
Cc: Mika Westerberg 
Cc: Rafael J. Wysocki 
Signed-off-by: Chen Yu 
---
  drivers/mfd/intel-lpss.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 41b1138..6dcc9a0 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
*data)
  int intel_lpss_prepare(struct device *dev)
  {
/*
+* This is safe because:
+* 1. The runtime suspend and system suspend
+* are of the same hook.
+* 2. This device is neither runtime wakeup source
+* nor system wakeup source.
+*/
+   if (pm_runtime_status_suspended(dev))
+   return 1;

What's '1'?


According to the comment in device_prepare():

A positive return value from ->prepare() means "this device appears
to be runtime-suspended and its state is fine, so if it really is
runtime-suspended, you can leave it in that state provided that you
will do the same thing with all of its descendants".

Are there no defines for this?



Not at the moment, but I guess they can be added if really necessary. :-)

But that said it doesn't have to be 1 or any specific value. Any 
positive number will have the same effect.



Thanks,

Rafael

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Mon, 12 Sep 2016, Chen Yu wrote:
> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> > On Sun, 04 Sep 2016, Chen Yu wrote:
> > 
> > > We have report that the intel_lpss_prepare() takes too much time during
> > > suspend, and this is because we first resume the devices from runtime
> > > suspend by resume_lpss_device(), to make sure they are in proper state
> > > before system suspend, which takes 100ms for each LPSS devices(PCI power
> > > state from D3_cold to D0). And since resume_lpss_device() resumes the
> > > devices synchronously, we might get huge latency if we have many
> > > LPSS devices.
> > > 
> > > So first try is to use pm_request_resume() instead, to make the runtime
> > > resume process asynchronously. Unfortunately the asynchronous runtime
> > > resume relies on pm_wq, which is freezed at early stage. So we choose
> > > another method, that is to avoid resuming runtime-suspended devices,
> > > if they are already runtime suspended. This is safe because for LPSS
> > > driver, the runtime suspend and system suspend are of the same
> > > hook - i.e., intel_lpss_suspend(). And moreover, this device is
> > > neither runtime wakeup source nor system wakeup source.
> > > 
> > > Cc: Andy Shevchenko 
> > > Cc: Mika Westerberg 
> > > Cc: Rafael J. Wysocki 
> > > Signed-off-by: Chen Yu 
> > > ---
> > >  drivers/mfd/intel-lpss.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > > index 41b1138..6dcc9a0 100644
> > > --- a/drivers/mfd/intel-lpss.c
> > > +++ b/drivers/mfd/intel-lpss.c
> > > @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, 
> > > void *data)
> > >  int intel_lpss_prepare(struct device *dev)
> > >  {
> > >   /*
> > > +  * This is safe because:
> > > +  * 1. The runtime suspend and system suspend
> > > +  * are of the same hook.
> > > +  * 2. This device is neither runtime wakeup source
> > > +  * nor system wakeup source.
> > > +  */
> > > + if (pm_runtime_status_suspended(dev))
> > > + return 1;
> > 
> > What's '1'?
> > 
> According to the comment in device_prepare():
> 
> A positive return value from ->prepare() means "this device appears
> to be runtime-suspended and its state is fine, so if it really is
> runtime-suspended, you can leave it in that state provided that you
> will do the same thing with all of its descendants". 

Are there no defines for this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Mon, 12 Sep 2016, Chen Yu wrote:
> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> > On Sun, 04 Sep 2016, Chen Yu wrote:
> > 
> > > We have report that the intel_lpss_prepare() takes too much time during
> > > suspend, and this is because we first resume the devices from runtime
> > > suspend by resume_lpss_device(), to make sure they are in proper state
> > > before system suspend, which takes 100ms for each LPSS devices(PCI power
> > > state from D3_cold to D0). And since resume_lpss_device() resumes the
> > > devices synchronously, we might get huge latency if we have many
> > > LPSS devices.
> > > 
> > > So first try is to use pm_request_resume() instead, to make the runtime
> > > resume process asynchronously. Unfortunately the asynchronous runtime
> > > resume relies on pm_wq, which is freezed at early stage. So we choose
> > > another method, that is to avoid resuming runtime-suspended devices,
> > > if they are already runtime suspended. This is safe because for LPSS
> > > driver, the runtime suspend and system suspend are of the same
> > > hook - i.e., intel_lpss_suspend(). And moreover, this device is
> > > neither runtime wakeup source nor system wakeup source.
> > > 
> > > Cc: Andy Shevchenko 
> > > Cc: Mika Westerberg 
> > > Cc: Rafael J. Wysocki 
> > > Signed-off-by: Chen Yu 
> > > ---
> > >  drivers/mfd/intel-lpss.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > > index 41b1138..6dcc9a0 100644
> > > --- a/drivers/mfd/intel-lpss.c
> > > +++ b/drivers/mfd/intel-lpss.c
> > > @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, 
> > > void *data)
> > >  int intel_lpss_prepare(struct device *dev)
> > >  {
> > >   /*
> > > +  * This is safe because:
> > > +  * 1. The runtime suspend and system suspend
> > > +  * are of the same hook.
> > > +  * 2. This device is neither runtime wakeup source
> > > +  * nor system wakeup source.
> > > +  */
> > > + if (pm_runtime_status_suspended(dev))
> > > + return 1;
> > 
> > What's '1'?
> > 
> According to the comment in device_prepare():
> 
> A positive return value from ->prepare() means "this device appears
> to be runtime-suspended and its state is fine, so if it really is
> runtime-suspended, you can leave it in that state provided that you
> will do the same thing with all of its descendants". 

Are there no defines for this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Chen Yu]

Hi,
On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> On Sun, 04 Sep 2016, Chen Yu wrote:
> 
> > We have report that the intel_lpss_prepare() takes too much time during
> > suspend, and this is because we first resume the devices from runtime
> > suspend by resume_lpss_device(), to make sure they are in proper state
> > before system suspend, which takes 100ms for each LPSS devices(PCI power
> > state from D3_cold to D0). And since resume_lpss_device() resumes the
> > devices synchronously, we might get huge latency if we have many
> > LPSS devices.
> > 
> > So first try is to use pm_request_resume() instead, to make the runtime
> > resume process asynchronously. Unfortunately the asynchronous runtime
> > resume relies on pm_wq, which is freezed at early stage. So we choose
> > another method, that is to avoid resuming runtime-suspended devices,
> > if they are already runtime suspended. This is safe because for LPSS
> > driver, the runtime suspend and system suspend are of the same
> > hook - i.e., intel_lpss_suspend(). And moreover, this device is
> > neither runtime wakeup source nor system wakeup source.
> > 
> > Cc: Andy Shevchenko 
> > Cc: Mika Westerberg 
> > Cc: Rafael J. Wysocki 
> > Signed-off-by: Chen Yu 
> > ---
> >  drivers/mfd/intel-lpss.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > index 41b1138..6dcc9a0 100644
> > --- a/drivers/mfd/intel-lpss.c
> > +++ b/drivers/mfd/intel-lpss.c
> > @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
> > *data)
> >  int intel_lpss_prepare(struct device *dev)
> >  {
> > /*
> > +* This is safe because:
> > +* 1. The runtime suspend and system suspend
> > +* are of the same hook.
> > +* 2. This device is neither runtime wakeup source
> > +* nor system wakeup source.
> > +*/
> > +   if (pm_runtime_status_suspended(dev))
> > +   return 1;
> 
> What's '1'?
> 
According to the comment in device_prepare():

A positive return value from ->prepare() means "this device appears
to be runtime-suspended and its state is fine, so if it really is
runtime-suspended, you can leave it in that state provided that you
will do the same thing with all of its descendants". 

Thanks,
Yu

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Chen Yu]

Hi,
On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> On Sun, 04 Sep 2016, Chen Yu wrote:
> 
> > We have report that the intel_lpss_prepare() takes too much time during
> > suspend, and this is because we first resume the devices from runtime
> > suspend by resume_lpss_device(), to make sure they are in proper state
> > before system suspend, which takes 100ms for each LPSS devices(PCI power
> > state from D3_cold to D0). And since resume_lpss_device() resumes the
> > devices synchronously, we might get huge latency if we have many
> > LPSS devices.
> > 
> > So first try is to use pm_request_resume() instead, to make the runtime
> > resume process asynchronously. Unfortunately the asynchronous runtime
> > resume relies on pm_wq, which is freezed at early stage. So we choose
> > another method, that is to avoid resuming runtime-suspended devices,
> > if they are already runtime suspended. This is safe because for LPSS
> > driver, the runtime suspend and system suspend are of the same
> > hook - i.e., intel_lpss_suspend(). And moreover, this device is
> > neither runtime wakeup source nor system wakeup source.
> > 
> > Cc: Andy Shevchenko 
> > Cc: Mika Westerberg 
> > Cc: Rafael J. Wysocki 
> > Signed-off-by: Chen Yu 
> > ---
> >  drivers/mfd/intel-lpss.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > index 41b1138..6dcc9a0 100644
> > --- a/drivers/mfd/intel-lpss.c
> > +++ b/drivers/mfd/intel-lpss.c
> > @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
> > *data)
> >  int intel_lpss_prepare(struct device *dev)
> >  {
> > /*
> > +* This is safe because:
> > +* 1. The runtime suspend and system suspend
> > +* are of the same hook.
> > +* 2. This device is neither runtime wakeup source
> > +* nor system wakeup source.
> > +*/
> > +   if (pm_runtime_status_suspended(dev))
> > +   return 1;
> 
> What's '1'?
> 
According to the comment in device_prepare():

A positive return value from ->prepare() means "this device appears
to be runtime-suspended and its state is fine, so if it really is
runtime-suspended, you can leave it in that state provided that you
will do the same thing with all of its descendants". 

Thanks,
Yu

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Sun, 04 Sep 2016, Chen Yu wrote:

> We have report that the intel_lpss_prepare() takes too much time during
> suspend, and this is because we first resume the devices from runtime
> suspend by resume_lpss_device(), to make sure they are in proper state
> before system suspend, which takes 100ms for each LPSS devices(PCI power
> state from D3_cold to D0). And since resume_lpss_device() resumes the
> devices synchronously, we might get huge latency if we have many
> LPSS devices.
> 
> So first try is to use pm_request_resume() instead, to make the runtime
> resume process asynchronously. Unfortunately the asynchronous runtime
> resume relies on pm_wq, which is freezed at early stage. So we choose
> another method, that is to avoid resuming runtime-suspended devices,
> if they are already runtime suspended. This is safe because for LPSS
> driver, the runtime suspend and system suspend are of the same
> hook - i.e., intel_lpss_suspend(). And moreover, this device is
> neither runtime wakeup source nor system wakeup source.
> 
> Cc: Andy Shevchenko 
> Cc: Mika Westerberg 
> Cc: Rafael J. Wysocki 
> Signed-off-by: Chen Yu 
> ---
>  drivers/mfd/intel-lpss.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 41b1138..6dcc9a0 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
> *data)
>  int intel_lpss_prepare(struct device *dev)
>  {
>   /*
> +  * This is safe because:
> +  * 1. The runtime suspend and system suspend
> +  * are of the same hook.
> +  * 2. This device is neither runtime wakeup source
> +  * nor system wakeup source.
> +  */
> + if (pm_runtime_status_suspended(dev))
> + return 1;

What's '1'?

> + /*
>* Resume both child devices before entering system sleep. This
>* ensures that they are in proper state before they get suspended.
>*/

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones]

On Sun, 04 Sep 2016, Chen Yu wrote:

> We have report that the intel_lpss_prepare() takes too much time during
> suspend, and this is because we first resume the devices from runtime
> suspend by resume_lpss_device(), to make sure they are in proper state
> before system suspend, which takes 100ms for each LPSS devices(PCI power
> state from D3_cold to D0). And since resume_lpss_device() resumes the
> devices synchronously, we might get huge latency if we have many
> LPSS devices.
> 
> So first try is to use pm_request_resume() instead, to make the runtime
> resume process asynchronously. Unfortunately the asynchronous runtime
> resume relies on pm_wq, which is freezed at early stage. So we choose
> another method, that is to avoid resuming runtime-suspended devices,
> if they are already runtime suspended. This is safe because for LPSS
> driver, the runtime suspend and system suspend are of the same
> hook - i.e., intel_lpss_suspend(). And moreover, this device is
> neither runtime wakeup source nor system wakeup source.
> 
> Cc: Andy Shevchenko 
> Cc: Mika Westerberg 
> Cc: Rafael J. Wysocki 
> Signed-off-by: Chen Yu 
> ---
>  drivers/mfd/intel-lpss.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 41b1138..6dcc9a0 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev, void 
> *data)
>  int intel_lpss_prepare(struct device *dev)
>  {
>   /*
> +  * This is safe because:
> +  * 1. The runtime suspend and system suspend
> +  * are of the same hook.
> +  * 2. This device is neither runtime wakeup source
> +  * nor system wakeup source.
> +  */
> + if (pm_runtime_status_suspended(dev))
> + return 1;

What's '1'?

> + /*
>* Resume both child devices before entering system sleep. This
>* ensures that they are in proper state before they get suspended.
>*/

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog