Hi Ulf,
On 2016-09-13 17:03, Ulf Hansson wrote:
[...]
This needs to be clarified. I agree we need to cover system PM as
well, but let's try be a bit more precise about it.
Right, I wasn't precise here. I've developed this code on older (v4.1 and
v4.6)
kernels, which had a code which disables runtime pm during system sleep
transition
time. Maybe I need to revisit it and consider your change merged to
v4.8-rc1, which
keeps runtime pm enabled during system sleep transitions.
Right, I see.
static bool clk_core_is_prepared(struct clk_core *core)
{
+ if (clk_pm_runtime_suspended(core))
+ return false;
+
This isn't safe, as even if the clock controller is runtime resumed at
this point, that's *not* a guarantee that is stays runtime resumed
while invoking the ->ops->is_prepared().
Instead you must call a pm_runtime_get_noresume() before you check the
runtime PM status, as that should avoid the device from being runtime
suspended. Then when the ->ops->is_prepared() has been invoked, we
should call pm_runtime_put().
Although, I am not sure the above change becomes entirely correct as I
think we are mixing the runtime PM status with the clock prepare
status here. In other words, the next time the clock controller
becomes runtime resumed, it may very well restore some register
context which may prepare the clock, unless someone explicitly has
unprepared it.
Of course, it all depends on how clk_core_is_prepared() is used by the
clock framework.
clk_core_is_prepared() is mainly used by disable_unused_tree_*. You are
right that it mixes a bit clock prepared state with runtime pm active
state of clock controller's, but I assumed here that clock cannot be
prepared if runtime pm state of controller is suspended. Other approach
here would be to call pm_runtime_get(), check status and then
pm_runtime_put(). If you prefer such approach, I will change it.
Using pm_runtime_get|put() would work for the clk_core_is_prepared()
case, although perhaps not for the clk_core_is_enabled() case.
The reason is that I guess the clk_core_is_enabled() API may be called
from atomic context? Thus we would need to enable
pm_runtime_irq_safe() for the clock provider device, which I *really*
would like to avoid.
I've checked clk_core_is_enabled() is only used for implementing disabling
of unused clock trees or implementing ->is_enabled() callback, which is used
for the same purpose, so it should be safe to use standard
pm_runtime_get/put
there.
There should be no other usecases for ->is_enabled() method, as it itself is
not really race prone, as other caller might enable/disable given clock in
meantime.
I will remove clk_pm_runtime_suspended() usage then.
[...]
I believe we are also accessing the clock controller HW from the
late_initcall_sync(clk_disable_unused) function.
This was indirectly handled by the runtime pm state check in is_prepared
and is_enabled().
I see.
Although, I was thinking that you explicitly would like to
disable/unprepare unused clocks in this phase, so then it isn't
sufficient to rely on the runtime PM status to know whether the clock
is prepared/enabled.
Perhaps, this is the only case when you actually need a
pm_runtime_get|put() around the ->is_enabled|prepared()!?
Right.
[...]
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
