Hi All,

On 2017-04-06 14:46, Marek Szyprowski wrote:

Hi Michael,

On 2017-03-30 19:11, Michael Turquette wrote:

quoted

Quoting Marek Szyprowski (2017-03-30 00:24:15)

quoted

On 2017-03-29 22:22, Michael Turquette wrote:

quoted

Quoting Marek Szyprowski (2017-03-22 04:35:40)

quoted

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.

quoted

quoted

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?

Mike, Stephen: any comments? We already missed v4.12 merge window...

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