> -----Original Message----- > From: Jean Delvare [mailto:khali@xxxxxxxxxxxx] > Sent: Wednesday, September 01, 2010 2:11 PM > To: Rafael J. Wysocki > Cc: Mark Brown; Kevin Hilman; Sripathy, Vishwanath; linux-i2c@xxxxxxxxxxxxxxx; > linux-omap@xxxxxxxxxxxxxxx; 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@xxxxxx> 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@xxxxxx> > > > > > Signed-off-by: Vishwanath BS <vishwanath.bs@xxxxxx> > > > > > > > > > > Cc: Rafael J. Wysocki <rjw@xxxxxxx> > > > > > Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> > > > > > Cc: Ben Dooks <ben-linux@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
