[PATCH v6 1/4] clk: Add support for runtime PM

[Marek Szyprowski]

Hi Michael,

On 2017-03-30 19:11, Michael Turquette wrote:
> Quoting Marek Szyprowski (2017-03-30 00:24:15)
>> On 2017-03-29 22:22, Michael Turquette wrote:
>>> Quoting Marek Szyprowski (2017-03-22 04:35:40)
>>>> Registers for some clocks might be located in the SOC area, which are under the
>>>> power domain. To enable access to those registers respective domain has to be
>>>> turned on. Additionally, registers for such clocks will usually loose its
>>>> contents when power domain is turned off, so additional saving and restoring of
>>>> them might be needed in the clock controller driver.
>>>>
>>>> This patch adds basic infrastructure in the clocks core to allow implementing
>>>> driver for such clocks under power domains. Clock provider can supply a
>>>> struct device pointer, which is the used by clock core for tracking and managing
>>>> clock's controller runtime pm state. Each clk_prepare() operation
>>>> will first call pm_runtime_get_sync() on the supplied device, while
>>>> clk_unprepare() will do pm_runtime_put_sync() at the end.
>>>>
>>>> Additional calls to pm_runtime_get/put functions are required to ensure that any
>>>> register access (like calculating/changing clock rates and unpreparing/disabling
>>>> unused clocks on boot) will be done with clock controller in runtime resumend
>>>> state.
>>> The above is a bit confusing. Is clk_prepare really special? It seems to
>>> me that every single clk_ops callback will need this?
>> clk_prepare/unprepare are special, because they allow sleeping, so they
>> are natural
>> candidates for the place for calling runtime PM operations.
>> clk_enable()/disable()
>> is called under a spinlock, so runtime pm cannot be called efficiently
>> there, but
>> core guarantees that they are called after clk_prepare(), so accessing
>> hw registers
>> is safe. The only remaining calls are not guaranteed to be called always
>> after
>> clk_prepare(), so those additional calls and checks in runtime pm are
>> needed there.
> Right, so any call that holds prepare_lock and might touch the hardware
> needs to first call pm_runtime_get_sync.
>
>>> That leads to my second question: why put this in the clk core? Can the
>>> clk provider driver simply add pm_runtime_{get,put} calls into its
>>> clk_ops callbacks instead? The clk core does not directly touch hardware
>>> (e.g. reading registers) so putting the pm runtime calls into the
>>> provider callbacks should be sufficient.
>> In theory is should be possible to duplicate all kind of clock build blocks
>> (gates, muxes, dividers, ...) with additional runtime pm calls. This
>> would however
>> end in much more code and a bit more complicated locking. Implementing
>> it in clk
>> core made the code simpler. It also turned out that runtime pm
>> integration is
>> needed for more that a single clock provider: besides Samsung SoCs
>> (Exynos 5433
>> and newer, Exynos 4412 ISP, Exynos Audio Subsystem, hacks in Exynos 542x
>> can be
>> also replaced by runtime PM calls), Ulf mentioned that exactly similar
>> pattern
>> is used for some UX500 SoCs (STE). More will probably come once the
>> feature is
>> in, because for now such drivers simply forces runtime active state as a
>> workaround or don't instantiate related power domains at all.
> I agree with the above, but I'm also wondering about the folks that use
> regmap internally to their own clock provider drivers. I guess they will
> have the option to either do their own thing or use this framework-level
> solution.
>
> I've started reviewing the code itself and will respond to those mails
> separately.

Is there a chance to get your comments soon? I would really like to give
this patchset a try in linux-next for a few days, but for now Sylwester
waits for you.

Stephen: what is your opinion on this patchset? Would you like to give it
a try in -next?

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland