[...]
>>
>> 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 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()!?
>
>> More precisely, in clk_disable_unused_subtree(), we probably need a
>> pm_runtime_get_sync() before calling clk_core_is_enabled(). And then
>> restore that with a pm_runtime_put() after the clock has been
>> disabled.
>> The similar is needed in clk_unprepare_unused_subtree().
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
Kind regards
Uffe
--
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
