[PATCH 1/5] clk: add support for runtime pm

[m.szyprowski@samsung.com (Marek Szyprowski)]

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