| Message ID | 1384954307-27094-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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?
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
+ 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
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
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
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
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; } }
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(-)