On Friday, September 03, 2010, Sripathy, Vishwanath wrote: > > > -----Original Message----- > > From: Jean Delvare [mailto:kh...@linux-fr.org] > > Sent: Wednesday, September 01, 2010 2:11 PM > > To: Rafael J. Wysocki > > Cc: Mark Brown; Kevin Hilman; Sripathy, Vishwanath; > > linux-...@vger.kernel.org; > > linux-omap@vger.kernel.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...@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-bas...@ti.com> > > > > > > Signed-off-by: Vishwanath BS <vishwanath...@ti.com> > > > > > > > > > > > > Cc: Rafael J. Wysocki <r...@sisk.pl> > > > > > > Cc: Kevin Hilman <khil...@deeprootsystems.com> > > > > > > Cc: Ben Dooks <ben-li...@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). > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
