Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
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 -- 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
RE: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
-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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jean Delvare
--
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
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
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
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.
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jean Delvare
--
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
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
[removed non-OMAP folks] 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. 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 -- 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
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
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).
---
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-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
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.
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)
--
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
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
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.
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)
--
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
