[RFC] intel-lpss: remove .prepare() callback

[RFC] intel-lpss: remove .prepare() callback

[Zhang Rui]

The .prepare() callback of intel-lpss driver does nothing but wakes up its
children. I don't know if there is any reason to do so, but to me, this is
not preferred because it should be the child device driver to do so when
necessary, not the parent device driver.
Plus, .prepare() does not support asynchronization.

For example, on MS Surface Pro 4, there are 4 intel-lpss devices which
are runtime suspended before system suspend, resuming each of them takes
more than 100 milliseconds. Thus the .prepare() of intel-lpss driver takes
400ms+ on my surface pro 4, and I've seen platforms with 16 intel lpss
devices.

With this patch applied, the child devices are resumed in the .suspend()
stage of the child device, and they are done in parallel, thus only 100ms
is needed, no matter how many intel lpss devices there are.

I have tested it on three different platforms and didn't find any obvious
problem caused by this patch, and it indeed reduces the suspend time a lot.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/mfd/intel-lpss.c | 11 -----------
 drivers/mfd/intel-lpss.h |  2 --
 2 files changed, 13 deletions(-)

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 0e0ab9b..5ff31e9 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -482,17 +482,6 @@ static int resume_lpss_device(struct device *dev, void *data)
 	return 0;
 }
 
-int intel_lpss_prepare(struct device *dev)
-{
-	/*
-	 * Resume both child devices before entering system sleep. This
-	 * ensures that they are in proper state before they get suspended.
-	 */
-	device_for_each_child_reverse(dev, NULL, resume_lpss_device);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(intel_lpss_prepare);
-
 int intel_lpss_suspend(struct device *dev)
 {
 	struct intel_lpss *lpss = dev_get_drvdata(dev);
diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index 865bbea..7913b58 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -31,13 +31,11 @@ int intel_lpss_probe(struct device *dev,
 void intel_lpss_remove(struct device *dev);
 
 #ifdef CONFIG_PM
-int intel_lpss_prepare(struct device *dev);
 int intel_lpss_suspend(struct device *dev);
 int intel_lpss_resume(struct device *dev);
 
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
-	.prepare = intel_lpss_prepare,		\
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
-- 
2.7.4

Re: [RFC] intel-lpss: remove .prepare() callback

[Andy Shevchenko]

On Thu, 2018-01-25 at 16:12 +0800, Zhang Rui wrote:
> The .prepare() callback of intel-lpss driver does nothing but wakes up
> its
> children. I don't know if there is any reason to do so, but to me,
> this is
> not preferred because it should be the child device driver to do so
> when
> necessary, not the parent device driver.
> Plus, .prepare() does not support asynchronization.
> 
> For example, on MS Surface Pro 4, there are 4 intel-lpss devices which
> are runtime suspended before system suspend, resuming each of them
> takes
> more than 100 milliseconds. Thus the .prepare() of intel-lpss driver
> takes
> 400ms+ on my surface pro 4, and I've seen platforms with 16 intel lpss
> devices.
> 
> With this patch applied, the child devices are resumed in the
> .suspend()
> stage of the child device, and they are done in parallel, thus only
> 100ms
> is needed, no matter how many intel lpss devices there are.
> 
> I have tested it on three different platforms and didn't find any
> obvious
> problem caused by this patch, and it indeed reduces the suspend time a
> lot.

> @@ -482,17 +482,6 @@ static int resume_lpss_device(struct device *dev,
> -	device_for_each_child_reverse(dev, NULL, resume_lpss_device);

Besides introduced compiler warning, did you check the latest linux-pm
changes? 

commit 8425ec7faff005500aad89b9fc00e5ba91ac57b9
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Jan 3 01:34:53 2018 +0100

    PM / mfd: intel-lpss: Use DPM_FLAG_SMART_SUSPEND


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC] intel-lpss: remove .prepare() callback

[Zhang Rui]

Hi, Andy,

On Thu, 2018-01-25 at 14:28 +0200, Andy Shevchenko wrote:
> On Thu, 2018-01-25 at 16:12 +0800, Zhang Rui wrote:
> > 
> > The .prepare() callback of intel-lpss driver does nothing but wakes
> > up
> > its
> > children. I don't know if there is any reason to do so, but to me,
> > this is
> > not preferred because it should be the child device driver to do so
> > when
> > necessary, not the parent device driver.
> > Plus, .prepare() does not support asynchronization.
> > 
> > For example, on MS Surface Pro 4, there are 4 intel-lpss devices
> > which
> > are runtime suspended before system suspend, resuming each of them
> > takes
> > more than 100 milliseconds. Thus the .prepare() of intel-lpss
> > driver
> > takes
> > 400ms+ on my surface pro 4, and I've seen platforms with 16 intel
> > lpss
> > devices.
> > 
> > With this patch applied, the child devices are resumed in the
> > .suspend()
> > stage of the child device, and they are done in parallel, thus only
> > 100ms
> > is needed, no matter how many intel lpss devices there are.
> > 
> > I have tested it on three different platforms and didn't find any
> > obvious
> > problem caused by this patch, and it indeed reduces the suspend
> > time a
> > lot.
> > 
> > @@ -482,17 +482,6 @@ static int resume_lpss_device(struct device
> > *dev,
> > -	device_for_each_child_reverse(dev, NULL,
> > resume_lpss_device);
> Besides introduced compiler warning, did you check the latest linux-
> pm
> changes? 
> 
> commit 8425ec7faff005500aad89b9fc00e5ba91ac57b9
> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Date:   Wed Jan 3 01:34:53 2018 +0100
> 
>     PM / mfd: intel-lpss: Use DPM_FLAG_SMART_SUSPEND
> 
No, thanks for the pointer, I will check this.

BTW, is there any reason that we need this .prepare() callback?

thanks,
rui