* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 13:31 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 13:31 UTC (permalink / raw)
To: Len Brown, Pavel Machek, Rafael J. Wysocki, Greg Kroah-Hartman
Cc: Grygorii Strashko, santosh.shilimkar, linux-pm, linux-kernel,
linux-arm-kernel
The following warning is shown and clk_enable() failed on Keystone platform
if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
[ 0.564486] ------------[ cut here ]------------
[ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
[ 0.577070] Modules linked in:
[ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
[ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
[ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
[ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
[ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
[ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
[ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
[ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
[ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
[ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
[ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
[ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
[ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
[ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
[ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
[ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
[ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
[ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
[ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
[ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
[ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
[ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
[ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
[ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
[ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
This happens because the clk_enable() is called without clk_prepare()
from pm_clk_resume().
Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
set.
Actually, This is remainder fix of the earlier commit c122f27e1c
"base: power - use clk_prepare_enable and clk_prepare_disable".
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 9d8fde7..60d389a 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
list_for_each_entry_reverse(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
if (ce->status == PCE_STATUS_ENABLED)
- clk_disable(ce->clk);
+ clk_disable_unprepare(ce->clk);
ce->status = PCE_STATUS_ACQUIRED;
}
}
@@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
list_for_each_entry(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
- clk_enable(ce->clk);
+ clk_prepare_enable(ce->clk);
ce->status = PCE_STATUS_ENABLED;
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 13:31 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 13:31 UTC (permalink / raw)
To: Len Brown, Pavel Machek, Rafael J. Wysocki, Greg Kroah-Hartman
Cc: Grygorii Strashko, santosh.shilimkar, linux-pm, linux-kernel,
linux-arm-kernel
The following warning is shown and clk_enable() failed on Keystone platform
if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
[ 0.564486] ------------[ cut here ]------------
[ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
[ 0.577070] Modules linked in:
[ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
[ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
[ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
[ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
[ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
[ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
[ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
[ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
[ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
[ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
[ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
[ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
[ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
[ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
[ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
[ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
[ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
[ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
[ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
[ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
[ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
[ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
[ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
[ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
[ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
This happens because the clk_enable() is called without clk_prepare()
from pm_clk_resume().
Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
set.
Actually, This is remainder fix of the earlier commit c122f27e1c
"base: power - use clk_prepare_enable and clk_prepare_disable".
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 9d8fde7..60d389a 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
list_for_each_entry_reverse(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
if (ce->status == PCE_STATUS_ENABLED)
- clk_disable(ce->clk);
+ clk_disable_unprepare(ce->clk);
ce->status = PCE_STATUS_ACQUIRED;
}
}
@@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
list_for_each_entry(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
- clk_enable(ce->clk);
+ clk_prepare_enable(ce->clk);
ce->status = PCE_STATUS_ENABLED;
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 13:31 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 13:31 UTC (permalink / raw)
To: linux-arm-kernel
The following warning is shown and clk_enable() failed on Keystone platform
if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
[ 0.564486] ------------[ cut here ]------------
[ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
[ 0.577070] Modules linked in:
[ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
[ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
[ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
[ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
[ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
[ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
[ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
[ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
[ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
[ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
[ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
[ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
[ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
[ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
[ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
[ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
[ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
[ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
[ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
[ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
[ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
[ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
[ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
[ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
[ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
This happens because the clk_enable() is called without clk_prepare()
from pm_clk_resume().
Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
set.
Actually, This is remainder fix of the earlier commit c122f27e1c
"base: power - use clk_prepare_enable and clk_prepare_disable".
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 9d8fde7..60d389a 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
list_for_each_entry_reverse(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
if (ce->status == PCE_STATUS_ENABLED)
- clk_disable(ce->clk);
+ clk_disable_unprepare(ce->clk);
ce->status = PCE_STATUS_ACQUIRED;
}
}
@@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
list_for_each_entry(ce, &psd->clock_list, node) {
if (ce->status < PCE_STATUS_ERROR) {
- clk_enable(ce->clk);
+ clk_prepare_enable(ce->clk);
ce->status = PCE_STATUS_ENABLED;
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 13:31 ` Grygorii Strashko
@ 2013-11-20 18:42 ` Stephen Boyd
-1 siblings, 0 replies; 25+ messages in thread
From: Stephen Boyd @ 2013-11-20 18:42 UTC (permalink / raw)
To: Grygorii Strashko, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm
On 11/20/13 05:31, Grygorii Strashko wrote:
> The following warning is shown and clk_enable() failed on Keystone platform
> if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
> [ 0.564486] ------------[ cut here ]------------
> [ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
> [ 0.577070] Modules linked in:
> [ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
> [ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
> [ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
> [ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
> [ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
> [ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
> [ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
> [ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
> [ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
> [ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
> [ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
> [ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
> [ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
> [ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
> [ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
> [ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
> [ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
> [ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
> [ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
> [ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
> [ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
> [ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
> [ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
> [ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
> [ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
Please omit the timestamps here.
>
> This happens because the clk_enable() is called without clk_prepare()
> from pm_clk_resume().
>
> Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
> instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
> set.
> Actually, This is remainder fix of the earlier commit c122f27e1c
> "base: power - use clk_prepare_enable and clk_prepare_disable".
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/base/power/clock_ops.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 9d8fde7..60d389a 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
> if (ce->status < PCE_STATUS_ERROR) {
> if (ce->status == PCE_STATUS_ENABLED)
> - clk_disable(ce->clk);
> + clk_disable_unprepare(ce->clk);
> ce->status = PCE_STATUS_ACQUIRED;
> }
> }
> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>
> list_for_each_entry(ce, &psd->clock_list, node) {
> if (ce->status < PCE_STATUS_ERROR) {
> - clk_enable(ce->clk);
> + clk_prepare_enable(ce->clk);
> ce->status = PCE_STATUS_ENABLED;
> }
> }
This is inside a spin_lock_irqsave(). You should be getting scheduling
while atomic warnings with this change. Are you testing with
DEBUG_ATOMIC_SLEEP=y?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 18:42 ` Stephen Boyd
0 siblings, 0 replies; 25+ messages in thread
From: Stephen Boyd @ 2013-11-20 18:42 UTC (permalink / raw)
To: linux-arm-kernel
On 11/20/13 05:31, Grygorii Strashko wrote:
> The following warning is shown and clk_enable() failed on Keystone platform
> if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
> [ 0.564486] ------------[ cut here ]------------
> [ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
> [ 0.577070] Modules linked in:
> [ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
> [ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
> [ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
> [ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
> [ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
> [ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
> [ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
> [ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
> [ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
> [ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
> [ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
> [ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
> [ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
> [ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
> [ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
> [ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
> [ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
> [ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
> [ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
> [ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
> [ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
> [ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
> [ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
> [ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
> [ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
Please omit the timestamps here.
>
> This happens because the clk_enable() is called without clk_prepare()
> from pm_clk_resume().
>
> Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
> instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
> set.
> Actually, This is remainder fix of the earlier commit c122f27e1c
> "base: power - use clk_prepare_enable and clk_prepare_disable".
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/base/power/clock_ops.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 9d8fde7..60d389a 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
> if (ce->status < PCE_STATUS_ERROR) {
> if (ce->status == PCE_STATUS_ENABLED)
> - clk_disable(ce->clk);
> + clk_disable_unprepare(ce->clk);
> ce->status = PCE_STATUS_ACQUIRED;
> }
> }
> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>
> list_for_each_entry(ce, &psd->clock_list, node) {
> if (ce->status < PCE_STATUS_ERROR) {
> - clk_enable(ce->clk);
> + clk_prepare_enable(ce->clk);
> ce->status = PCE_STATUS_ENABLED;
> }
> }
This is inside a spin_lock_irqsave(). You should be getting scheduling
while atomic warnings with this change. Are you testing with
DEBUG_ATOMIC_SLEEP=y?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 18:42 ` Stephen Boyd
(?)
@ 2013-11-20 19:06 ` Grygorii Strashko
-1 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 19:06 UTC (permalink / raw)
To: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm
Hi Stephen,
On 11/20/2013 08:42 PM, Stephen Boyd wrote:
> On 11/20/13 05:31, Grygorii Strashko wrote:
>> The following warning is shown and clk_enable() failed on Keystone platform
>> if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
>> [ 0.564486] ------------[ cut here ]------------
>> [ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
>> [ 0.577070] Modules linked in:
>> [ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
>> [ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
>> [ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
>> [ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
>> [ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
>> [ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
>> [ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
>> [ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
>> [ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
>> [ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
>> [ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
>> [ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
>> [ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
>> [ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
>> [ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
>> [ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
>> [ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
>> [ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
>> [ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
>> [ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
>> [ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
>> [ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
>> [ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
>> [ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
>> [ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
>
> Please omit the timestamps here.
ok.
>
>>
>> This happens because the clk_enable() is called without clk_prepare()
>> from pm_clk_resume().
>>
>> Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
>> instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
>> set.
>> Actually, This is remainder fix of the earlier commit c122f27e1c
>> "base: power - use clk_prepare_enable and clk_prepare_disable".
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/base/power/clock_ops.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 9d8fde7..60d389a 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> if (ce->status == PCE_STATUS_ENABLED)
>> - clk_disable(ce->clk);
>> + clk_disable_unprepare(ce->clk);
>> ce->status = PCE_STATUS_ACQUIRED;
>> }
>> }
>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>
>> list_for_each_entry(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> - clk_enable(ce->clk);
>> + clk_prepare_enable(ce->clk);
>> ce->status = PCE_STATUS_ENABLED;
>> }
>> }
>
> This is inside a spin_lock_irqsave(). You should be getting scheduling
> while atomic warnings with this change. Are you testing with
> DEBUG_ATOMIC_SLEEP=y?
Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
I agree with you.
So, I see two option here:
1) split above loops on two
2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
In my opinion option [2] is better.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 19:06 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 19:06 UTC (permalink / raw)
To: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm
Hi Stephen,
On 11/20/2013 08:42 PM, Stephen Boyd wrote:
> On 11/20/13 05:31, Grygorii Strashko wrote:
>> The following warning is shown and clk_enable() failed on Keystone platform
>> if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
>> [ 0.564486] ------------[ cut here ]------------
>> [ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
>> [ 0.577070] Modules linked in:
>> [ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
>> [ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
>> [ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
>> [ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
>> [ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
>> [ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
>> [ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
>> [ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
>> [ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
>> [ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
>> [ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
>> [ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
>> [ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
>> [ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
>> [ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
>> [ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
>> [ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
>> [ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
>> [ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
>> [ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
>> [ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
>> [ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
>> [ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
>> [ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
>> [ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
>
> Please omit the timestamps here.
ok.
>
>>
>> This happens because the clk_enable() is called without clk_prepare()
>> from pm_clk_resume().
>>
>> Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
>> instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
>> set.
>> Actually, This is remainder fix of the earlier commit c122f27e1c
>> "base: power - use clk_prepare_enable and clk_prepare_disable".
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/base/power/clock_ops.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 9d8fde7..60d389a 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> if (ce->status == PCE_STATUS_ENABLED)
>> - clk_disable(ce->clk);
>> + clk_disable_unprepare(ce->clk);
>> ce->status = PCE_STATUS_ACQUIRED;
>> }
>> }
>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>
>> list_for_each_entry(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> - clk_enable(ce->clk);
>> + clk_prepare_enable(ce->clk);
>> ce->status = PCE_STATUS_ENABLED;
>> }
>> }
>
> This is inside a spin_lock_irqsave(). You should be getting scheduling
> while atomic warnings with this change. Are you testing with
> DEBUG_ATOMIC_SLEEP=y?
Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
I agree with you.
So, I see two option here:
1) split above loops on two
2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
In my opinion option [2] is better.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 19:06 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 19:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi Stephen,
On 11/20/2013 08:42 PM, Stephen Boyd wrote:
> On 11/20/13 05:31, Grygorii Strashko wrote:
>> The following warning is shown and clk_enable() failed on Keystone platform
>> if CONFIG_PM_RUNTIME is set and Runtime PM is enabled for Davinci gpio driver:
>> [ 0.564486] ------------[ cut here ]------------
>> [ 0.569221] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883__clk_enable+0x8c/0x98()
>> [ 0.577070] Modules linked in:
>> [ 0.580231] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-10116-g11d59938-dirty #259
>> [ 0.588368] [<c0014d34>] (unwind_backtrace+0x0/0xf8) from [<c0011eb8>] (show_stack+0x10/0x14)
>> [ 0.597018] [<c0011eb8>] (show_stack+0x10/0x14) from [<c03c5df0>] (dump_stack+0x64/0xa4)
>> [ 0.605235] [<c03c5df0>] (dump_stack+0x64/0xa4) from [<c00216b8>] (warn_slowpath_common+0x68/0x88)
>> [ 0.614315] [<c00216b8>] (warn_slowpath_common+0x68/0x88) from [<c00216f4>] (warn_slowpath_null+0x1c/0x24)
>> [ 0.624092] [<c00216f4>] (warn_slowpath_null+0x1c/0x24) from [<c0287a60>] (__clk_enable+0x8c/0x98)
>> [ 0.633173] [<c0287a60>] (__clk_enable+0x8c/0x98) from [<c0287c54>] (clk_enable+0x18/0x2c)
>> [ 0.641556] [<c0287c54>] (clk_enable+0x18/0x2c) from [<c023677c>] (pm_clk_resume+0x5c/0x80)
>> [ 0.650025] [<c023677c>] (pm_clk_resume+0x5c/0x80) from [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18)
>> [ 0.659800] [<c001eba8>] (keystone_pm_runtime_resume+0xc/0x18) from [<c02344e4>] (__rpm_callback+0x34/0x70)
>> [ 0.669662] [<c02344e4>] (__rpm_callback+0x34/0x70) from [<c0234548>] (rpm_callback+0x28/0x88)
>> [ 0.678390] [<c0234548>] (rpm_callback+0x28/0x88) from [<c023590c>] (rpm_resume+0x3c8/0x684)
>> [ 0.686942] [<c023590c>] (rpm_resume+0x3c8/0x684) from [<c0235e40>] (__pm_runtime_resume+0x4c/0x64)
>> [ 0.696111] [<c0235e40>] (__pm_runtime_resume+0x4c/0x64) from [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac)
>> [ 0.705983] [<c0204798>] (davinci_gpio_probe+0x20c/0x5ac) from [<c02310cc>] (platform_drv_probe+0x18/0x48)
>> [ 0.715758] [<c02310cc>] (platform_drv_probe+0x18/0x48) from [<c022fa0c>] (driver_probe_device+0x80/0x21c)
>> [ 0.725532] [<c022fa0c>] (driver_probe_device+0x80/0x21c) from [<c022fc34>] (__driver_attach+0x8c/0x90)
>> [ 0.735044] [<c022fc34>] (__driver_attach+0x8c/0x90) from [<c022e35c>] (bus_for_each_dev+0x60/0x94)
>> [ 0.744207] [<c022e35c>] (bus_for_each_dev+0x60/0x94) from [<c022f24c>] (bus_add_driver+0xdc/0x1d0)
>> [ 0.753371] [<c022f24c>] (bus_add_driver+0xdc/0x1d0) from [<c023002c>] (driver_register+0x78/0xf4)
>> [ 0.762449] [<c023002c>] (driver_register+0x78/0xf4) from [<c00088fc>] (do_one_initcall+0xec/0x148)
>> [ 0.771616] [<c00088fc>] (do_one_initcall+0xec/0x148) from [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8)
>> [ 0.781303] [<c0538c0c>] (kernel_init_freeable+0xfc/0x1c8) from [<c03c19d0>] (kernel_init+0x8/0x110)
>> [ 0.790553] [<c03c19d0>] (kernel_init+0x8/0x110) from [<c000e318>] (ret_from_fork+0x14/0x3c)
>> [ 0.799117] ---[ end trace 6588a0ec7fbc847a ]---
>
> Please omit the timestamps here.
ok.
>
>>
>> This happens because the clk_enable() is called without clk_prepare()
>> from pm_clk_resume().
>>
>> Hence, switch to use clk_prepare_enable()/clk_disable_unprepare()
>> instead of clk_enable()/clk_disable() in case if CONFIG_PM_RUNTIME is
>> set.
>> Actually, This is remainder fix of the earlier commit c122f27e1c
>> "base: power - use clk_prepare_enable and clk_prepare_disable".
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/base/power/clock_ops.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 9d8fde7..60d389a 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> if (ce->status == PCE_STATUS_ENABLED)
>> - clk_disable(ce->clk);
>> + clk_disable_unprepare(ce->clk);
>> ce->status = PCE_STATUS_ACQUIRED;
>> }
>> }
>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>
>> list_for_each_entry(ce, &psd->clock_list, node) {
>> if (ce->status < PCE_STATUS_ERROR) {
>> - clk_enable(ce->clk);
>> + clk_prepare_enable(ce->clk);
>> ce->status = PCE_STATUS_ENABLED;
>> }
>> }
>
> This is inside a spin_lock_irqsave(). You should be getting scheduling
> while atomic warnings with this change. Are you testing with
> DEBUG_ATOMIC_SLEEP=y?
Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
I agree with you.
So, I see two option here:
1) split above loops on two
2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
In my opinion option [2] is better.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 19:06 ` Grygorii Strashko
@ 2013-11-20 19:53 ` Stephen Boyd
-1 siblings, 0 replies; 25+ messages in thread
From: Stephen Boyd @ 2013-11-20 19:53 UTC (permalink / raw)
To: Grygorii Strashko, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm
On 11/20/13 11:06, Grygorii Strashko wrote:
> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>> if (ce->status < PCE_STATUS_ERROR) {
>>> if (ce->status == PCE_STATUS_ENABLED)
>>> - clk_disable(ce->clk);
>>> + clk_disable_unprepare(ce->clk);
>>> ce->status = PCE_STATUS_ACQUIRED;
>>> }
>>> }
>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>
>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>> if (ce->status < PCE_STATUS_ERROR) {
>>> - clk_enable(ce->clk);
>>> + clk_prepare_enable(ce->clk);
>>> ce->status = PCE_STATUS_ENABLED;
>>> }
>>> }
>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>> while atomic warnings with this change. Are you testing with
>> DEBUG_ATOMIC_SLEEP=y?
> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
> I agree with you.
>
> So, I see two option here:
> 1) split above loops on two
> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>
> In my opinion option [2] is better.
>
Doesn't that mean the clock will always be prepared as long as the
device is present? That doesn't sound good. I would like the clocks to
be disabled and unprepared as long as the device is suspended.
What is the lock protecting? The linked list or something more? Can we
remove the locks?
It looks like even if you just remove the locks here, the PM core is
free to call this function with irqs disabled if pm_runtime_irq_safe()
has been called on the device. Perhaps runtime PM can only do the
clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
calls should happen in the system suspend callbacks?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 19:53 ` Stephen Boyd
0 siblings, 0 replies; 25+ messages in thread
From: Stephen Boyd @ 2013-11-20 19:53 UTC (permalink / raw)
To: linux-arm-kernel
On 11/20/13 11:06, Grygorii Strashko wrote:
> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>> if (ce->status < PCE_STATUS_ERROR) {
>>> if (ce->status == PCE_STATUS_ENABLED)
>>> - clk_disable(ce->clk);
>>> + clk_disable_unprepare(ce->clk);
>>> ce->status = PCE_STATUS_ACQUIRED;
>>> }
>>> }
>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>
>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>> if (ce->status < PCE_STATUS_ERROR) {
>>> - clk_enable(ce->clk);
>>> + clk_prepare_enable(ce->clk);
>>> ce->status = PCE_STATUS_ENABLED;
>>> }
>>> }
>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>> while atomic warnings with this change. Are you testing with
>> DEBUG_ATOMIC_SLEEP=y?
> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
> I agree with you.
>
> So, I see two option here:
> 1) split above loops on two
> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>
> In my opinion option [2] is better.
>
Doesn't that mean the clock will always be prepared as long as the
device is present? That doesn't sound good. I would like the clocks to
be disabled and unprepared as long as the device is suspended.
What is the lock protecting? The linked list or something more? Can we
remove the locks?
It looks like even if you just remove the locks here, the PM core is
free to call this function with irqs disabled if pm_runtime_irq_safe()
has been called on the device. Perhaps runtime PM can only do the
clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
calls should happen in the system suspend callbacks?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 19:53 ` Stephen Boyd
(?)
@ 2013-11-20 20:11 ` Grygorii Strashko
-1 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 20:11 UTC (permalink / raw)
To: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette
On 11/20/2013 09:53 PM, Stephen Boyd wrote:
> On 11/20/13 11:06, Grygorii Strashko wrote:
>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>> - clk_disable(ce->clk);
>>>> + clk_disable_unprepare(ce->clk);
>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>> }
>>>> }
>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>
>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> - clk_enable(ce->clk);
>>>> + clk_prepare_enable(ce->clk);
>>>> ce->status = PCE_STATUS_ENABLED;
>>>> }
>>>> }
>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>> while atomic warnings with this change. Are you testing with
>>> DEBUG_ATOMIC_SLEEP=y?
>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>> I agree with you.
>>
>> So, I see two option here:
>> 1) split above loops on two
>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>
>> In my opinion option [2] is better.
>>
>
> Doesn't that mean the clock will always be prepared as long as the
> device is present? That doesn't sound good. I would like the clocks to
> be disabled and unprepared as long as the device is suspended.
Yep (
>
> What is the lock protecting? The linked list or something more? Can we
> remove the locks?
Looks like it's protecting linked list pm_clock_entry'es.
>
> It looks like even if you just remove the locks here, the PM core is
> free to call this function with irqs disabled if pm_runtime_irq_safe()
> has been called on the device. Perhaps runtime PM can only do the
> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
> calls should happen in the system suspend callbacks?
Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
But clk_prepare() has to be called at least once before clk_enable() :((
So, solution with suspend/resume will not fix current problem :( unfortunately.
FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
(also similar solution is used by Davinci, but issue has not been detected because
PM runtime hasn't been used by Davinci IP drivers before)
Regards,
- grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 20:11 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 20:11 UTC (permalink / raw)
To: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman
Cc: santosh.shilimkar, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette
On 11/20/2013 09:53 PM, Stephen Boyd wrote:
> On 11/20/13 11:06, Grygorii Strashko wrote:
>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>> - clk_disable(ce->clk);
>>>> + clk_disable_unprepare(ce->clk);
>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>> }
>>>> }
>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>
>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> - clk_enable(ce->clk);
>>>> + clk_prepare_enable(ce->clk);
>>>> ce->status = PCE_STATUS_ENABLED;
>>>> }
>>>> }
>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>> while atomic warnings with this change. Are you testing with
>>> DEBUG_ATOMIC_SLEEP=y?
>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>> I agree with you.
>>
>> So, I see two option here:
>> 1) split above loops on two
>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>
>> In my opinion option [2] is better.
>>
>
> Doesn't that mean the clock will always be prepared as long as the
> device is present? That doesn't sound good. I would like the clocks to
> be disabled and unprepared as long as the device is suspended.
Yep (
>
> What is the lock protecting? The linked list or something more? Can we
> remove the locks?
Looks like it's protecting linked list pm_clock_entry'es.
>
> It looks like even if you just remove the locks here, the PM core is
> free to call this function with irqs disabled if pm_runtime_irq_safe()
> has been called on the device. Perhaps runtime PM can only do the
> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
> calls should happen in the system suspend callbacks?
Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
But clk_prepare() has to be called at least once before clk_enable() :((
So, solution with suspend/resume will not fix current problem :( unfortunately.
FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
(also similar solution is used by Davinci, but issue has not been detected because
PM runtime hasn't been used by Davinci IP drivers before)
Regards,
- grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 20:11 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-20 20:11 UTC (permalink / raw)
To: linux-arm-kernel
On 11/20/2013 09:53 PM, Stephen Boyd wrote:
> On 11/20/13 11:06, Grygorii Strashko wrote:
>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>> - clk_disable(ce->clk);
>>>> + clk_disable_unprepare(ce->clk);
>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>> }
>>>> }
>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>
>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>> - clk_enable(ce->clk);
>>>> + clk_prepare_enable(ce->clk);
>>>> ce->status = PCE_STATUS_ENABLED;
>>>> }
>>>> }
>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>> while atomic warnings with this change. Are you testing with
>>> DEBUG_ATOMIC_SLEEP=y?
>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>> I agree with you.
>>
>> So, I see two option here:
>> 1) split above loops on two
>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>
>> In my opinion option [2] is better.
>>
>
> Doesn't that mean the clock will always be prepared as long as the
> device is present? That doesn't sound good. I would like the clocks to
> be disabled and unprepared as long as the device is suspended.
Yep (
>
> What is the lock protecting? The linked list or something more? Can we
> remove the locks?
Looks like it's protecting linked list pm_clock_entry'es.
>
> It looks like even if you just remove the locks here, the PM core is
> free to call this function with irqs disabled if pm_runtime_irq_safe()
> has been called on the device. Perhaps runtime PM can only do the
> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
> calls should happen in the system suspend callbacks?
Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
But clk_prepare() has to be called at least once before clk_enable() :((
So, solution with suspend/resume will not fix current problem :( unfortunately.
FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
(also similar solution is used by Davinci, but issue has not been detected because
PM runtime hasn't been used by Davinci IP drivers before)
Regards,
- grygorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 20:11 ` Grygorii Strashko
(?)
@ 2013-11-20 20:32 ` Santosh Shilimkar
-1 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-20 20:32 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette, Kevin Hilman, Rafael J. Wysocki
+ Kevin and Rafiel,
On Wednesday 20 November 2013 03:11 PM, Grygorii Strashko wrote:
> On 11/20/2013 09:53 PM, Stephen Boyd wrote:
>> On 11/20/13 11:06, Grygorii Strashko wrote:
>>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>>> - clk_disable(ce->clk);
>>>>> + clk_disable_unprepare(ce->clk);
>>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>>> }
>>>>> }
>>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>>
>>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> - clk_enable(ce->clk);
>>>>> + clk_prepare_enable(ce->clk);
>>>>> ce->status = PCE_STATUS_ENABLED;
>>>>> }
>>>>> }
>>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>>> while atomic warnings with this change. Are you testing with
>>>> DEBUG_ATOMIC_SLEEP=y?
>>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>>> I agree with you.
>>>
I suspected this and thats what I was trying to mention off-list
about sleeping inside locks.
>>> So, I see two option here:
>>> 1) split above loops on two
>>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>>
>>> In my opinion option [2] is better.
>>>
>>
>> Doesn't that mean the clock will always be prepared as long as the
>> device is present? That doesn't sound good. I would like the clocks to
>> be disabled and unprepared as long as the device is suspended.
>
> Yep (
>
>>
>> What is the lock protecting? The linked list or something more? Can we
>> remove the locks?
>
> Looks like it's protecting linked list pm_clock_entry'es.
>
>>
>> It looks like even if you just remove the locks here, the PM core is
>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>> has been called on the device. Perhaps runtime PM can only do the
>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>> calls should happen in the system suspend callbacks?
>
> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
> But clk_prepare() has to be called at least once before clk_enable() :((
> So, solution with suspend/resume will not fix current problem :( unfortunately.
>
> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
> (also similar solution is used by Davinci, but issue has not been detected because
> PM runtime hasn't been used by Davinci IP drivers before)
>
One way to deal with this is to have clk_unprepare()/clk_prepare()
called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
can't have that as part of runtime code.
Kevin/Rafael might have better ideas here.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 20:32 ` Santosh Shilimkar
0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-20 20:32 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette, Kevin Hilman, Rafael J. Wysocki
+ Kevin and Rafiel,
On Wednesday 20 November 2013 03:11 PM, Grygorii Strashko wrote:
> On 11/20/2013 09:53 PM, Stephen Boyd wrote:
>> On 11/20/13 11:06, Grygorii Strashko wrote:
>>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>>> - clk_disable(ce->clk);
>>>>> + clk_disable_unprepare(ce->clk);
>>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>>> }
>>>>> }
>>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>>
>>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> - clk_enable(ce->clk);
>>>>> + clk_prepare_enable(ce->clk);
>>>>> ce->status = PCE_STATUS_ENABLED;
>>>>> }
>>>>> }
>>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>>> while atomic warnings with this change. Are you testing with
>>>> DEBUG_ATOMIC_SLEEP=y?
>>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>>> I agree with you.
>>>
I suspected this and thats what I was trying to mention off-list
about sleeping inside locks.
>>> So, I see two option here:
>>> 1) split above loops on two
>>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>>
>>> In my opinion option [2] is better.
>>>
>>
>> Doesn't that mean the clock will always be prepared as long as the
>> device is present? That doesn't sound good. I would like the clocks to
>> be disabled and unprepared as long as the device is suspended.
>
> Yep (
>
>>
>> What is the lock protecting? The linked list or something more? Can we
>> remove the locks?
>
> Looks like it's protecting linked list pm_clock_entry'es.
>
>>
>> It looks like even if you just remove the locks here, the PM core is
>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>> has been called on the device. Perhaps runtime PM can only do the
>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>> calls should happen in the system suspend callbacks?
>
> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
> But clk_prepare() has to be called at least once before clk_enable() :((
> So, solution with suspend/resume will not fix current problem :( unfortunately.
>
> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
> (also similar solution is used by Davinci, but issue has not been detected because
> PM runtime hasn't been used by Davinci IP drivers before)
>
One way to deal with this is to have clk_unprepare()/clk_prepare()
called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
can't have that as part of runtime code.
Kevin/Rafael might have better ideas here.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-20 20:32 ` Santosh Shilimkar
0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-20 20:32 UTC (permalink / raw)
To: linux-arm-kernel
+ Kevin and Rafiel,
On Wednesday 20 November 2013 03:11 PM, Grygorii Strashko wrote:
> On 11/20/2013 09:53 PM, Stephen Boyd wrote:
>> On 11/20/13 11:06, Grygorii Strashko wrote:
>>> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>>>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>>> list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> if (ce->status == PCE_STATUS_ENABLED)
>>>>> - clk_disable(ce->clk);
>>>>> + clk_disable_unprepare(ce->clk);
>>>>> ce->status = PCE_STATUS_ACQUIRED;
>>>>> }
>>>>> }
>>>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>>>
>>>>> list_for_each_entry(ce, &psd->clock_list, node) {
>>>>> if (ce->status < PCE_STATUS_ERROR) {
>>>>> - clk_enable(ce->clk);
>>>>> + clk_prepare_enable(ce->clk);
>>>>> ce->status = PCE_STATUS_ENABLED;
>>>>> }
>>>>> }
>>>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>>>> while atomic warnings with this change. Are you testing with
>>>> DEBUG_ATOMIC_SLEEP=y?
>>> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and
>>> I agree with you.
>>>
I suspected this and thats what I was trying to mention off-list
about sleeping inside locks.
>>> So, I see two option here:
>>> 1) split above loops on two
>>> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>>>
>>> In my opinion option [2] is better.
>>>
>>
>> Doesn't that mean the clock will always be prepared as long as the
>> device is present? That doesn't sound good. I would like the clocks to
>> be disabled and unprepared as long as the device is suspended.
>
> Yep (
>
>>
>> What is the lock protecting? The linked list or something more? Can we
>> remove the locks?
>
> Looks like it's protecting linked list pm_clock_entry'es.
>
>>
>> It looks like even if you just remove the locks here, the PM core is
>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>> has been called on the device. Perhaps runtime PM can only do the
>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>> calls should happen in the system suspend callbacks?
>
> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
> But clk_prepare() has to be called at least once before clk_enable() :((
> So, solution with suspend/resume will not fix current problem :( unfortunately.
>
> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
> (also similar solution is used by Davinci, but issue has not been detected because
> PM runtime hasn't been used by Davinci IP drivers before)
>
One way to deal with this is to have clk_unprepare()/clk_prepare()
called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
can't have that as part of runtime code.
Kevin/Rafael might have better ideas here.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-20 20:32 ` Santosh Shilimkar
(?)
@ 2013-11-22 18:43 ` Kevin Hilman
-1 siblings, 0 replies; 25+ messages in thread
From: Kevin Hilman @ 2013-11-22 18:43 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: Grygorii Strashko, Stephen Boyd, Len Brown, Pavel Machek,
Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel,
linux-arm-kernel, linux-pm, Mike Turquette, Rafael J. Wysocki
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
[...]
>>> It looks like even if you just remove the locks here, the PM core is
>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>> has been called on the device. Perhaps runtime PM can only do the
>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>> calls should happen in the system suspend callbacks?
>>
>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>> But clk_prepare() has to be called at least once before clk_enable() :((
>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>
>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>> (also similar solution is used by Davinci, but issue has not been detected because
>> PM runtime hasn't been used by Davinci IP drivers before)
>>
> One way to deal with this is to have clk_unprepare()/clk_prepare()
> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
> can't have that as part of runtime code.
That doesn't solve the irq_safe problem that Stephen pointed out without
being very careful. Basically, if you have _any_ potentially sleeping
calls in this path, you can never allow devices to use
pm_runtime_irq_safe().
Also, I don't like having the clk_enable in the PM core but the
clk_prepare in the platform-specific pm_domain. That seems prone for
platforms to get wrong.
I'll need to think about this a little more before having any idea what
to do here.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-22 18:43 ` Kevin Hilman
0 siblings, 0 replies; 25+ messages in thread
From: Kevin Hilman @ 2013-11-22 18:43 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: Grygorii Strashko, Stephen Boyd, Len Brown, Pavel Machek,
Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel,
linux-arm-kernel, linux-pm, Mike Turquette, Rafael J. Wysocki
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
[...]
>>> It looks like even if you just remove the locks here, the PM core is
>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>> has been called on the device. Perhaps runtime PM can only do the
>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>> calls should happen in the system suspend callbacks?
>>
>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>> But clk_prepare() has to be called at least once before clk_enable() :((
>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>
>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>> (also similar solution is used by Davinci, but issue has not been detected because
>> PM runtime hasn't been used by Davinci IP drivers before)
>>
> One way to deal with this is to have clk_unprepare()/clk_prepare()
> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
> can't have that as part of runtime code.
That doesn't solve the irq_safe problem that Stephen pointed out without
being very careful. Basically, if you have _any_ potentially sleeping
calls in this path, you can never allow devices to use
pm_runtime_irq_safe().
Also, I don't like having the clk_enable in the PM core but the
clk_prepare in the platform-specific pm_domain. That seems prone for
platforms to get wrong.
I'll need to think about this a little more before having any idea what
to do here.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-22 18:43 ` Kevin Hilman
0 siblings, 0 replies; 25+ messages in thread
From: Kevin Hilman @ 2013-11-22 18:43 UTC (permalink / raw)
To: linux-arm-kernel
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
[...]
>>> It looks like even if you just remove the locks here, the PM core is
>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>> has been called on the device. Perhaps runtime PM can only do the
>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>> calls should happen in the system suspend callbacks?
>>
>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>> But clk_prepare() has to be called at least once before clk_enable() :((
>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>
>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>> (also similar solution is used by Davinci, but issue has not been detected because
>> PM runtime hasn't been used by Davinci IP drivers before)
>>
> One way to deal with this is to have clk_unprepare()/clk_prepare()
> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
> can't have that as part of runtime code.
That doesn't solve the irq_safe problem that Stephen pointed out without
being very careful. Basically, if you have _any_ potentially sleeping
calls in this path, you can never allow devices to use
pm_runtime_irq_safe().
Also, I don't like having the clk_enable in the PM core but the
clk_prepare in the platform-specific pm_domain. That seems prone for
platforms to get wrong.
I'll need to think about this a little more before having any idea what
to do here.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-22 18:43 ` Kevin Hilman
(?)
@ 2013-11-22 19:01 ` Santosh Shilimkar
-1 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-22 19:01 UTC (permalink / raw)
To: Kevin Hilman
Cc: Grygorii Strashko, Stephen Boyd, Len Brown, Pavel Machek,
Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel,
linux-arm-kernel, linux-pm, Mike Turquette, Rafael J. Wysocki
On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> [...]
>
>>>> It looks like even if you just remove the locks here, the PM core is
>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>> has been called on the device. Perhaps runtime PM can only do the
>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>> calls should happen in the system suspend callbacks?
>>>
>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>
>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>> (also similar solution is used by Davinci, but issue has not been detected because
>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>
>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>> can't have that as part of runtime code.
>
> That doesn't solve the irq_safe problem that Stephen pointed out without
> being very careful. Basically, if you have _any_ potentially sleeping
> calls in this path, you can never allow devices to use
> pm_runtime_irq_safe().
>
You are right. I realized that after reading the code.
> Also, I don't like having the clk_enable in the PM core but the
> clk_prepare in the platform-specific pm_domain. That seems prone for
> platforms to get wrong.
>
Agree.
> I'll need to think about this a little more before having any idea what
> to do here.
>
Thanks Kevin for looking into it.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-22 19:01 ` Santosh Shilimkar
0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-22 19:01 UTC (permalink / raw)
To: Kevin Hilman
Cc: Grygorii Strashko, Stephen Boyd, Len Brown, Pavel Machek,
Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel,
linux-arm-kernel, linux-pm, Mike Turquette, Rafael J. Wysocki
On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> [...]
>
>>>> It looks like even if you just remove the locks here, the PM core is
>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>> has been called on the device. Perhaps runtime PM can only do the
>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>> calls should happen in the system suspend callbacks?
>>>
>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>
>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>> (also similar solution is used by Davinci, but issue has not been detected because
>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>
>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>> can't have that as part of runtime code.
>
> That doesn't solve the irq_safe problem that Stephen pointed out without
> being very careful. Basically, if you have _any_ potentially sleeping
> calls in this path, you can never allow devices to use
> pm_runtime_irq_safe().
>
You are right. I realized that after reading the code.
> Also, I don't like having the clk_enable in the PM core but the
> clk_prepare in the platform-specific pm_domain. That seems prone for
> platforms to get wrong.
>
Agree.
> I'll need to think about this a little more before having any idea what
> to do here.
>
Thanks Kevin for looking into it.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-22 19:01 ` Santosh Shilimkar
0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shilimkar @ 2013-11-22 19:01 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> [...]
>
>>>> It looks like even if you just remove the locks here, the PM core is
>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>> has been called on the device. Perhaps runtime PM can only do the
>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>> calls should happen in the system suspend callbacks?
>>>
>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>
>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>> (also similar solution is used by Davinci, but issue has not been detected because
>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>
>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>> can't have that as part of runtime code.
>
> That doesn't solve the irq_safe problem that Stephen pointed out without
> being very careful. Basically, if you have _any_ potentially sleeping
> calls in this path, you can never allow devices to use
> pm_runtime_irq_safe().
>
You are right. I realized that after reading the code.
> Also, I don't like having the clk_enable in the PM core but the
> clk_prepare in the platform-specific pm_domain. That seems prone for
> platforms to get wrong.
>
Agree.
> I'll need to think about this a little more before having any idea what
> to do here.
>
Thanks Kevin for looking into it.
Regards,
Santosh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
2013-11-22 19:01 ` Santosh Shilimkar
(?)
@ 2013-11-25 10:05 ` Grygorii Strashko
-1 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-25 10:05 UTC (permalink / raw)
To: Santosh Shilimkar, Kevin Hilman
Cc: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette, Rafael J. Wysocki
On 11/22/2013 09:01 PM, Santosh Shilimkar wrote:
> On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
>> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>>
>> [...]
>>
>>>>> It looks like even if you just remove the locks here, the PM core is
>>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>>> has been called on the device. Perhaps runtime PM can only do the
>>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>>> calls should happen in the system suspend callbacks?
>>>>
>>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>>
>>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>>> (also similar solution is used by Davinci, but issue has not been detected because
>>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>>
>>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>>> can't have that as part of runtime code.
>>
>> That doesn't solve the irq_safe problem that Stephen pointed out without
>> being very careful. Basically, if you have _any_ potentially sleeping
>> calls in this path, you can never allow devices to use
>> pm_runtime_irq_safe().
>>
> You are right. I realized that after reading the code.
>
>> Also, I don't like having the clk_enable in the PM core but the
>> clk_prepare in the platform-specific pm_domain. That seems prone for
>> platforms to get wrong.
>>
> Agree.
>
>> I'll need to think about this a little more before having any idea what
>> to do here.
>>
> Thanks Kevin for looking into it.
May be it can be solved through CCF by introducing additional flag like
CLK_SET_NO_PREPARE for cases when clocks perpetration isn't really needed.
So, when this flag is set CCF will just auto-increment "prepare_count"
when clock is registered.
Of Course, this will not solve the case when clock need to be prepared and
driver wants to use PM Runtime, but for SoCs like Keystone that can be
fast & simple solution.
Regards,
- grygrorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-25 10:05 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-25 10:05 UTC (permalink / raw)
To: Santosh Shilimkar, Kevin Hilman
Cc: Stephen Boyd, Len Brown, Pavel Machek, Rafael J. Wysocki,
Greg Kroah-Hartman, linux-kernel, linux-arm-kernel, linux-pm,
Mike Turquette, Rafael J. Wysocki
On 11/22/2013 09:01 PM, Santosh Shilimkar wrote:
> On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
>> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>>
>> [...]
>>
>>>>> It looks like even if you just remove the locks here, the PM core is
>>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>>> has been called on the device. Perhaps runtime PM can only do the
>>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>>> calls should happen in the system suspend callbacks?
>>>>
>>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>>
>>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>>> (also similar solution is used by Davinci, but issue has not been detected because
>>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>>
>>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>>> can't have that as part of runtime code.
>>
>> That doesn't solve the irq_safe problem that Stephen pointed out without
>> being very careful. Basically, if you have _any_ potentially sleeping
>> calls in this path, you can never allow devices to use
>> pm_runtime_irq_safe().
>>
> You are right. I realized that after reading the code.
>
>> Also, I don't like having the clk_enable in the PM core but the
>> clk_prepare in the platform-specific pm_domain. That seems prone for
>> platforms to get wrong.
>>
> Agree.
>
>> I'll need to think about this a little more before having any idea what
>> to do here.
>>
> Thanks Kevin for looking into it.
May be it can be solved through CCF by introducing additional flag like
CLK_SET_NO_PREPARE for cases when clocks perpetration isn't really needed.
So, when this flag is set CCF will just auto-increment "prepare_count"
when clock is registered.
Of Course, this will not solve the case when clock need to be prepared and
driver wants to use PM Runtime, but for SoCs like Keystone that can be
fast & simple solution.
Regards,
- grygrorii
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set
@ 2013-11-25 10:05 ` Grygorii Strashko
0 siblings, 0 replies; 25+ messages in thread
From: Grygorii Strashko @ 2013-11-25 10:05 UTC (permalink / raw)
To: linux-arm-kernel
On 11/22/2013 09:01 PM, Santosh Shilimkar wrote:
> On Friday 22 November 2013 01:43 PM, Kevin Hilman wrote:
>> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>>
>> [...]
>>
>>>>> It looks like even if you just remove the locks here, the PM core is
>>>>> free to call this function with irqs disabled if pm_runtime_irq_safe()
>>>>> has been called on the device. Perhaps runtime PM can only do the
>>>>> clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
>>>>> calls should happen in the system suspend callbacks?
>>>>
>>>> Even don't know what to say :( On Keystone clk_unprepare()/clk_prepare() are NOPs.
>>>> But clk_prepare() has to be called at least once before clk_enable() :((
>>>> So, solution with suspend/resume will not fix current problem :( unfortunately.
>>>>
>>>> FYI, Now pm_clk_suspend/pm_clk_resume are called from arch/arm/mach-keystone/pm_domain.c
>>>> (also similar solution is used by Davinci, but issue has not been detected because
>>>> PM runtime hasn't been used by Davinci IP drivers before)
>>>>
>>> One way to deal with this is to have clk_unprepare()/clk_prepare()
>>> called from dev_pm_domain ops before calling pm_clk_[suspend/resume]() if we
>>> can't have that as part of runtime code.
>>
>> That doesn't solve the irq_safe problem that Stephen pointed out without
>> being very careful. Basically, if you have _any_ potentially sleeping
>> calls in this path, you can never allow devices to use
>> pm_runtime_irq_safe().
>>
> You are right. I realized that after reading the code.
>
>> Also, I don't like having the clk_enable in the PM core but the
>> clk_prepare in the platform-specific pm_domain. That seems prone for
>> platforms to get wrong.
>>
> Agree.
>
>> I'll need to think about this a little more before having any idea what
>> to do here.
>>
> Thanks Kevin for looking into it.
May be it can be solved through CCF by introducing additional flag like
CLK_SET_NO_PREPARE for cases when clocks perpetration isn't really needed.
So, when this flag is set CCF will just auto-increment "prepare_count"
when clock is registered.
Of Course, this will not solve the case when clock need to be prepared and
driver wants to use PM Runtime, but for SoCs like Keystone that can be
fast & simple solution.
Regards,
- grygrorii
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-11-25 10:09 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-20 13:31 [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME is set Grygorii Strashko
2013-11-20 13:31 ` Grygorii Strashko
2013-11-20 13:31 ` Grygorii Strashko
2013-11-20 18:42 ` Stephen Boyd
2013-11-20 18:42 ` Stephen Boyd
2013-11-20 19:06 ` Grygorii Strashko
2013-11-20 19:06 ` Grygorii Strashko
2013-11-20 19:06 ` Grygorii Strashko
2013-11-20 19:53 ` Stephen Boyd
2013-11-20 19:53 ` Stephen Boyd
2013-11-20 20:11 ` Grygorii Strashko
2013-11-20 20:11 ` Grygorii Strashko
2013-11-20 20:11 ` Grygorii Strashko
2013-11-20 20:32 ` Santosh Shilimkar
2013-11-20 20:32 ` Santosh Shilimkar
2013-11-20 20:32 ` Santosh Shilimkar
2013-11-22 18:43 ` Kevin Hilman
2013-11-22 18:43 ` Kevin Hilman
2013-11-22 18:43 ` Kevin Hilman
2013-11-22 19:01 ` Santosh Shilimkar
2013-11-22 19:01 ` Santosh Shilimkar
2013-11-22 19:01 ` Santosh Shilimkar
2013-11-25 10:05 ` Grygorii Strashko
2013-11-25 10:05 ` Grygorii Strashko
2013-11-25 10:05 ` Grygorii Strashko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.