Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Kevin Hilman]

Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org> writes:

> In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
> is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
> pm_runtime_set_active called from resume path will increase the child_count of
> the device's parent. However, matching pm_runtime_set_suspended is not called
> in suspend routine because of which child_count of the device's parent
> is not balanced, preventing the parent device to idle.
> Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
> reoutine which will make sure that child_counts are balanced.
> This fix has been tested on OMAP4430.
>
> Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>
>
> Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Also Cc'ing Mark Brown as original author of runtime PM for i2-core.

Kevin

> ---
>  drivers/i2c/i2c-core.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 6649176..3146bff
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
>  static int i2c_device_pm_suspend(struct device *dev)
>  {
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	int ret;
>  
>  	if (pm_runtime_suspended(dev))
>  		return 0;
>  
>  	if (pm)
> -		return pm->suspend ? pm->suspend(dev) : 0;
> +		ret = pm->suspend ? pm->suspend(dev) : 0;
> +	else
> +		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
>  
> -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> +	if (!ret) {
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +	}
> +	return ret;
>  }
>  
>  static int i2c_device_pm_resume(struct device *dev)

[PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Vishwanath BS]

In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
pm_runtime_set_active called from resume path will increase the child_count of
the device's parent. However, matching pm_runtime_set_suspended is not called
in suspend routine because of which child_count of the device's parent
is not balanced, preventing the parent device to idle.
Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
reoutine which will make sure that child_counts are balanced.
This fix has been tested on OMAP4430.

Signed-off-by: Partha Basak <p-basak2@ti.com>
Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>

Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Ben Dooks <ben-linux@fluff.org>
---
 drivers/i2c/i2c-core.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..3146bff
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
 static int i2c_device_pm_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret;
 
 	if (pm_runtime_suspended(dev))
 		return 0;
 
 	if (pm)
-		return pm->suspend ? pm->suspend(dev) : 0;
+		ret = pm->suspend ? pm->suspend(dev) : 0;
+	else
+		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
 
-	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
+	if (!ret) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+	return ret;
 }
 
 static int i2c_device_pm_resume(struct device *dev)
-- 
1.7.0.4

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Mark Brown]

On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> Vishwanath BS <vishwanath.bs@ti.com> writes:
> 
> > In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
> > is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
> > pm_runtime_set_active called from resume path will increase the child_count of
> > the device's parent. However, matching pm_runtime_set_suspended is not called
> > in suspend routine because of which child_count of the device's parent
> > is not balanced, preventing the parent device to idle.
> > Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
> > reoutine which will make sure that child_counts are balanced.
> > This fix has been tested on OMAP4430.
> >
> > Signed-off-by: Partha Basak <p-basak2@ti.com>
> > Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> >
> > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > Cc: Kevin Hilman <khilman@deeprootsystems.com>
> > Cc: Ben Dooks <ben-linux@fluff.org>
> 
> Also Cc'ing Mark Brown as original author of runtime PM for i2-core.

Also Jean Delvare who maintains the I2C core.  To be honest Rafael did
all the actual work here (and has since rewritten the code anyway).

> > ---
> >  drivers/i2c/i2c-core.c |   12 ++++++++++--
> >  1 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 6649176..3146bff
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
> >  static int i2c_device_pm_suspend(struct device *dev)
> >  {
> >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +	int ret;
> >  
> >  	if (pm_runtime_suspended(dev))
> >  		return 0;
> >  
> >  	if (pm)
> > -		return pm->suspend ? pm->suspend(dev) : 0;
> > +		ret = pm->suspend ? pm->suspend(dev) : 0;
> > +	else
> > +		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
> >  
> > -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > +	if (!ret) {
> > +		pm_runtime_disable(dev);
> > +		pm_runtime_set_suspended(dev);
> > +		pm_runtime_enable(dev);
> > +	}
> > +	return ret;
> >  }
> >  
> >  static int i2c_device_pm_resume(struct device *dev)

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Rafael J. Wysocki]

On Tuesday, August 31, 2010, Mark Brown wrote:
> On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> > Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org> writes:
> > 
> > > In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
> > > is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
> > > pm_runtime_set_active called from resume path will increase the child_count of
> > > the device's parent. However, matching pm_runtime_set_suspended is not called
> > > in suspend routine because of which child_count of the device's parent
> > > is not balanced, preventing the parent device to idle.
> > > Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
> > > reoutine which will make sure that child_counts are balanced.
> > > This fix has been tested on OMAP4430.
> > >
> > > Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
> > > Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>
> > >
> > > Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > 
> > Also Cc'ing Mark Brown as original author of runtime PM for i2-core.
> 
> Also Jean Delvare who maintains the I2C core.  To be honest Rafael did
> all the actual work here (and has since rewritten the code anyway).

Sorry for the delay.

The fix looks reasonable to me.

Thanks,
Rafael


> > > ---
> > >  drivers/i2c/i2c-core.c |   12 ++++++++++--
> > >  1 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index 6649176..3146bff
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
> > >  static int i2c_device_pm_suspend(struct device *dev)
> > >  {
> > >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > > +	int ret;
> > >  
> > >  	if (pm_runtime_suspended(dev))
> > >  		return 0;
> > >  
> > >  	if (pm)
> > > -		return pm->suspend ? pm->suspend(dev) : 0;
> > > +		ret = pm->suspend ? pm->suspend(dev) : 0;
> > > +	else
> > > +		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > >  
> > > -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > +	if (!ret) {
> > > +		pm_runtime_disable(dev);
> > > +		pm_runtime_set_suspended(dev);
> > > +		pm_runtime_enable(dev);
> > > +	}
> > > +	return ret;
> > >  }
> > >  
> > >  static int i2c_device_pm_resume(struct device *dev)
> 
> 

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Jean Delvare]

On Wed, 1 Sep 2010 01:12:11 +0200, Rafael J. Wysocki wrote:
> On Tuesday, August 31, 2010, Mark Brown wrote:
> > On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> > > Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org> writes:
> > > 
> > > > In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
> > > > is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
> > > > pm_runtime_set_active called from resume path will increase the child_count of
> > > > the device's parent. However, matching pm_runtime_set_suspended is not called
> > > > in suspend routine because of which child_count of the device's parent
> > > > is not balanced, preventing the parent device to idle.
> > > > Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
> > > > reoutine which will make sure that child_counts are balanced.
> > > > This fix has been tested on OMAP4430.
> > > >
> > > > Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
> > > > Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>
> > > >
> > > > Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > > Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > > Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > > 
> > > Also Cc'ing Mark Brown as original author of runtime PM for i2-core.
> > 
> > Also Jean Delvare who maintains the I2C core.  To be honest Rafael did
> > all the actual work here (and has since rewritten the code anyway).
> 
> Sorry for the delay.
> 
> The fix looks reasonable to me.

I take this as an Acked-by. Patch applied, thanks.

> 
> > > > ---
> > > >  drivers/i2c/i2c-core.c |   12 ++++++++++--
> > > >  1 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index 6649176..3146bff
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
> > > >  static int i2c_device_pm_suspend(struct device *dev)
> > > >  {
> > > >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > > > +	int ret;
> > > >  
> > > >  	if (pm_runtime_suspended(dev))
> > > >  		return 0;
> > > >  
> > > >  	if (pm)
> > > > -		return pm->suspend ? pm->suspend(dev) : 0;
> > > > +		ret = pm->suspend ? pm->suspend(dev) : 0;
> > > > +	else
> > > > +		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > >  
> > > > -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > > +	if (!ret) {
> > > > +		pm_runtime_disable(dev);
> > > > +		pm_runtime_set_suspended(dev);
> > > > +		pm_runtime_enable(dev);
> > > > +	}
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  static int i2c_device_pm_resume(struct device *dev)
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jean Delvare

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Kevin Hilman]

[removed non-OMAP folks]

Vishwanath BS <vishwanath.bs@ti.com> writes:

> In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
> is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
> pm_runtime_set_active called from resume path will increase the child_count of
> the device's parent. However, matching pm_runtime_set_suspended is not called
> in suspend routine because of which child_count of the device's parent
> is not balanced, preventing the parent device to idle.
> Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
> reoutine which will make sure that child_counts are balanced.
> This fix has been tested on OMAP4430.

FYI... for OMAP folks.  Now that this is queued for upstream, it will be
included in my pm-backports branch[1] and included in the PM branch until
it gets merged upstream.

Thanks Vishwa/Partha for getting this merged upstream.

Kevin

[1] for a description of the various branches that make up the PM
branch, please see 'What makes up the PM branch' section of the OMAP PM
wiki:

    http://elinux.org/OMAP_Power_Management

RE: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Sripathy, Vishwanath]

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Wednesday, September 01, 2010 2:11 PM
> To: Rafael J. Wysocki
> Cc: Mark Brown; Kevin Hilman; Sripathy, Vishwanath; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Ben Dooks
> Subject: Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
> 
> On Wed, 1 Sep 2010 01:12:11 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, August 31, 2010, Mark Brown wrote:
> > > On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> > > > Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org> writes:
> > > >
> > > > > In current i2c core driver, pm_runtime_set_active call from
> i2c_device_pm_resume
> > > > > is not balanced by pm_runtime_set_suspended call from
> i2c_device_pm_suspend.
> > > > > pm_runtime_set_active called from resume path will increase the
> child_count of
> > > > > the device's parent. However, matching pm_runtime_set_suspended is not
> called
> > > > > in suspend routine because of which child_count of the device's parent
> > > > > is not balanced, preventing the parent device to idle.
> > > > > Issue has been fixed by adding pm_runtime_set_suspended call inside
> suspend
> > > > > reoutine which will make sure that child_counts are balanced.
> > > > > This fix has been tested on OMAP4430.
> > > > >
> > > > > Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
> > > > > Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>
> > > > >
> > > > > Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > > > Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > > > Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > > >
> > > > Also Cc'ing Mark Brown as original author of runtime PM for i2-core.
> > >
> > > Also Jean Delvare who maintains the I2C core.  To be honest Rafael did
> > > all the actual work here (and has since rewritten the code anyway).
> >
> > Sorry for the delay.
> >
> > The fix looks reasonable to me.
> 
> I take this as an Acked-by. Patch applied, thanks.
Hi,
We are seeing one more issue even after making this fix. In summary, after first suspend/resume, CPU Idle no longer works since i2c module is active. 
Basically when the system resumed from the suspend, dpm layer (dpm_resume_end) calls device_resume which intern calls i2c_device_pm_resume (among many other calls). 
i2c_device_pm_resume calls pm_runtime_set_active which brings device to Active state and increases child_count of it's parent. Since the device is active and also it's parent child_count is non 0, these modules will prevent corresponding clock domains to go idle. This will break CPU Idle. This issue happens even if the module was in idle state before performing suspend/resume. In summary, the flow is 
1. i2c module is idle (let's assume system is idle)
2. system suspend is initiated by user
3. i2c_device_pm_suspend gets called which tries to idle i2c, but it's already idled.
4. system is suspended
5. system resumed (because of user event/timers)
6. dpm layer will call i2c_device_pm_resume
7. i2c_device_pm_resume will enable i2c device by calling pm_runtime_set_active
So at the end of suspend/resume, a device that was idled before is getting enabled unnecessarily at the end.

SO I am just wondering is there any real need to call pm_runtime_set_active in resume and pm_runtime_set_suspened in suspend functions?
I just tried suspend/resume and CPU Idle after removing these calls in i2c and it seems to function perfectly fine on OMAP4.

Regards
Vishwa






I 

> 
> >
> > > > > ---
> > > > >  drivers/i2c/i2c-core.c |   12 ++++++++++--
> > > > >  1 files changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > > index 6649176..3146bff
> > > > > --- a/drivers/i2c/i2c-core.c
> > > > > +++ b/drivers/i2c/i2c-core.c
> > > > > @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
> > > > >  static int i2c_device_pm_suspend(struct device *dev)
> > > > >  {
> > > > >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
> NULL;
> > > > > +	int ret;
> > > > >
> > > > >  	if (pm_runtime_suspended(dev))
> > > > >  		return 0;
> > > > >
> > > > >  	if (pm)
> > > > > -		return pm->suspend ? pm->suspend(dev) : 0;
> > > > > +		ret = pm->suspend ? pm->suspend(dev) : 0;
> > > > > +	else
> > > > > +		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > > >
> > > > > -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > > > +	if (!ret) {
> > > > > +		pm_runtime_disable(dev);
> > > > > +		pm_runtime_set_suspended(dev);
> > > > > +		pm_runtime_enable(dev);
> > > > > +	}
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static int i2c_device_pm_resume(struct device *dev)
> > >
> > >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> Jean Delvare

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Rafael J. Wysocki]

On Friday, September 03, 2010, Sripathy, Vishwanath wrote:
> 
> > -----Original Message-----
> > From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> > Sent: Wednesday, September 01, 2010 2:11 PM
> > To: Rafael J. Wysocki
> > Cc: Mark Brown; Kevin Hilman; Sripathy, Vishwanath; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Ben Dooks
> > Subject: Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
> > 
> > On Wed, 1 Sep 2010 01:12:11 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, August 31, 2010, Mark Brown wrote:
> > > > On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> > > > > Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org> writes:
> > > > >
> > > > > > In current i2c core driver, pm_runtime_set_active call from
> > i2c_device_pm_resume
> > > > > > is not balanced by pm_runtime_set_suspended call from
> > i2c_device_pm_suspend.
> > > > > > pm_runtime_set_active called from resume path will increase the
> > child_count of
> > > > > > the device's parent. However, matching pm_runtime_set_suspended is not
> > called
> > > > > > in suspend routine because of which child_count of the device's parent
> > > > > > is not balanced, preventing the parent device to idle.
> > > > > > Issue has been fixed by adding pm_runtime_set_suspended call inside
> > suspend
> > > > > > reoutine which will make sure that child_counts are balanced.
> > > > > > This fix has been tested on OMAP4430.
> > > > > >
> > > > > > Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
> > > > > > Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>
> > > > > >
> > > > > > Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > > > > Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > > > > Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > > > >
> > > > > Also Cc'ing Mark Brown as original author of runtime PM for i2-core.
> > > >
> > > > Also Jean Delvare who maintains the I2C core.  To be honest Rafael did
> > > > all the actual work here (and has since rewritten the code anyway).
> > >
> > > Sorry for the delay.
> > >
> > > The fix looks reasonable to me.
> > 
> > I take this as an Acked-by. Patch applied, thanks.
> Hi,
> We are seeing one more issue even after making this fix. In summary, after first suspend/resume, CPU Idle no longer works since i2c module is active. 
> Basically when the system resumed from the suspend, dpm layer (dpm_resume_end) calls device_resume which intern calls i2c_device_pm_resume (among many other calls). 
> i2c_device_pm_resume calls pm_runtime_set_active which brings device to Active state and increases child_count of it's parent. Since the device is active and also it's parent child_count is non 0, these modules will prevent corresponding clock domains to go idle. This will break CPU Idle. This issue happens even if the module was in idle state before performing suspend/resume. In summary, the flow is 
> 1. i2c module is idle (let's assume system is idle)
> 2. system suspend is initiated by user
> 3. i2c_device_pm_suspend gets called which tries to idle i2c, but it's already idled.
> 4. system is suspended
> 5. system resumed (because of user event/timers)
> 6. dpm layer will call i2c_device_pm_resume
> 7. i2c_device_pm_resume will enable i2c device by calling pm_runtime_set_active
> So at the end of suspend/resume, a device that was idled before is getting enabled unnecessarily at the end.
> 
> SO I am just wondering is there any real need to call pm_runtime_set_active in resume and pm_runtime_set_suspened in suspend functions?
> I just tried suspend/resume and CPU Idle after removing these calls in i2c and it seems to function perfectly fine on OMAP4.

Your analysis appears to be entirely correct.

So, instead of applying the $subject patch it might be better to remove the
block that calls pm_runtime_set_active(dev) from i2c_device_pm_resume().

Are there any objections?

Rafael

Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Jean Delvare]

On Sat, 4 Sep 2010 01:10:10 +0200, Rafael J. Wysocki wrote:
> On Friday, September 03, 2010, Sripathy, Vishwanath wrote:
> > We are seeing one more issue even after making this fix. In summary, after first suspend/resume, CPU Idle no longer works since i2c module is active. 
> > Basically when the system resumed from the suspend, dpm layer (dpm_resume_end) calls device_resume which intern calls i2c_device_pm_resume (among many other calls). 
> > i2c_device_pm_resume calls pm_runtime_set_active which brings device to Active state and increases child_count of it's parent. Since the device is active and also it's parent child_count is non 0, these modules will prevent corresponding clock domains to go idle. This will break CPU Idle. This issue happens even if the module was in idle state before performing suspend/resume. In summary, the flow is 
> > 1. i2c module is idle (let's assume system is idle)
> > 2. system suspend is initiated by user
> > 3. i2c_device_pm_suspend gets called which tries to idle i2c, but it's already idled.
> > 4. system is suspended
> > 5. system resumed (because of user event/timers)
> > 6. dpm layer will call i2c_device_pm_resume
> > 7. i2c_device_pm_resume will enable i2c device by calling pm_runtime_set_active
> > So at the end of suspend/resume, a device that was idled before is getting enabled unnecessarily at the end.
> > 
> > SO I am just wondering is there any real need to call pm_runtime_set_active in resume and pm_runtime_set_suspened in suspend functions?
> > I just tried suspend/resume and CPU Idle after removing these calls in i2c and it seems to function perfectly fine on OMAP4.
> 
> Your analysis appears to be entirely correct.
> 
> So, instead of applying the $subject patch it might be better to remove the
> block that calls pm_runtime_set_active(dev) from i2c_device_pm_resume().
> 
> Are there any objections?

No objection. Just send an updated patch and I'll be happy to apply it.

-- 
Jean Delvare

[PATCH] I2C: Fix for suspend/resume issue in i2c-core

[Vishwanath BS]

In current i2c core driver, pm_runtime_set_active call from i2c_device_pm_resume
is not balanced by pm_runtime_set_suspended call from i2c_device_pm_suspend.
pm_runtime_set_active called from resume path will increase the child_count of
the device's parent. However, matching pm_runtime_set_suspended is not called
in suspend routine because of which child_count of the device's parent
is not balanced, preventing the parent device to idle.
Issue has been fixed by adding pm_runtime_set_suspended call inside suspend
reoutine which will make sure that child_counts are balanced.
This fix has been tested on OMAP4430.

Signed-off-by: Partha Basak <p-basak2-l0cyMroinI0@public.gmane.org>
Signed-off-by: Vishwanath BS <vishwanath.bs-l0cyMroinI0@public.gmane.org>

Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..3146bff
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
 static int i2c_device_pm_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret;
 
 	if (pm_runtime_suspended(dev))
 		return 0;
 
 	if (pm)
-		return pm->suspend ? pm->suspend(dev) : 0;
+		ret = pm->suspend ? pm->suspend(dev) : 0;
+	else
+		ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
 
-	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
+	if (!ret) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+	return ret;
 }
 
 static int i2c_device_pm_resume(struct device *dev)
-- 
1.7.0.4