[PATCH 1/5] clk: add support for runtime pm
Marek Szyprowski
m.szyprowski at samsung.com
Tue Sep 13 06:13:30 PDT 2016
Hi Ulf,
Thanks for looking into this patch!
On 2016-09-13 10:49, Ulf Hansson wrote:
> On 1 September 2016 at 15:45, Marek Szyprowski <m.szyprowski at samsung.com> wrote:
>> 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 is indeed correct, I can confirm that the UX500 SoC's PRCC clock
> controllers also needs to be managed like this.
>
>> 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() at the end.
> This make sense!
>
>> Additional calls to pm_runtime_get/put functions are required to ensure that any
>> register access (like calculating/chaning clock rates) will be done with clock
> /s/chaning/changing
>
>> controller in active runtime state.
> /s/active runtime/runtime resumed
>
>> Special handling of the case when runtime pm is disabled for clock controller's
>> device is needed to let this feature work properly also during system sleep
>> suspend/resume operations (runtime pm is first disabled before entering sleep
>> state's, but controller is usually still operational until its suspend pm
>> callback is called).
> 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.
>
>> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> I would also like to extend the change log to describe a little bit of
> how a clk provider should interact with this new and nice feature.
> Something like:
>
> *) It needs to provide a struct device to the core when registering
> the provider.
> **) It needs to enable runtime PM.
> ***) It needs to make sure the runtime PM status of the controller
> device reflects the HW state.
Right, this definitely has to be added. Thank you for reminding about
such obvious
things.
>> ---
>> drivers/clk/clk.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 76 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 820a939fb6bb..a1934e9b4e95 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -21,6 +21,7 @@
>> #include <linux/of.h>
>> #include <linux/device.h>
>> #include <linux/init.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/sched.h>
>> #include <linux/clkdev.h>
>>
>> @@ -46,6 +47,7 @@ struct clk_core {
>> const struct clk_ops *ops;
>> struct clk_hw *hw;
>> struct module *owner;
>> + struct device *dev;
>> struct clk_core *parent;
>> const char **parent_names;
>> struct clk_core **parents;
>> @@ -87,6 +89,42 @@ struct clk {
>> struct hlist_node clks_node;
>> };
>>
>> +/*** runtime pm ***/
>> +static int clk_pm_runtime_get(struct clk_core *core)
>> +{
>> + int ret = 0;
>> +
>> + if (!core->dev)
>> + return 0;
>> +
>> + if (pm_runtime_enabled(core->dev)) {
> Why do you need to check for this?
This was a workaround, which let it work during the system sleep transition
state.
>
>> + ret = pm_runtime_get_sync(core->dev);
>> + } else {
>> + if (!pm_runtime_status_suspended(core->dev))
>> + pm_runtime_get_noresume(core->dev);
> This looks weird. I guess it's related to the system PM case somehow?
>
>> + }
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +static void clk_pm_runtime_put(struct clk_core *core)
>> +{
> Similar comments as for clk_pm_runtime_get().
>
>> + if (!core->dev)
>> + return;
>> +
>> + if (pm_runtime_enabled(core->dev))
>> + pm_runtime_put(core->dev);
>> + else
>> + pm_runtime_put_noidle(core->dev);
>> +}
>> +
>> +static bool clk_pm_runtime_suspended(struct clk_core *core)
>> +{
>> + if (!core->dev)
>> + return 0;
>> +
>> + return pm_runtime_suspended(core->dev);
>> +}
>> +
>> /*** locking ***/
>> static void clk_prepare_lock(void)
>> {
>> @@ -150,6 +188,9 @@ static void clk_enable_unlock(unsigned long flags)
>>
>> 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.
>
>> /*
>> * .is_prepared is optional for clocks that can prepare
>> * fall back to software usage counter if it is missing
>> @@ -162,6 +203,9 @@ static bool clk_core_is_prepared(struct clk_core *core)
>>
>> static bool clk_core_is_enabled(struct clk_core *core)
>> {
>> + if (clk_pm_runtime_suspended(core))
>> + return false;
>> +
> Similar comment as for clk_core_is_prepared().
>
>> /*
>> * .is_enabled is only mandatory for clocks that gate
>> * fall back to software usage counter if .is_enabled is missing
>> @@ -489,6 +533,8 @@ static void clk_core_unprepare(struct clk_core *core)
>> if (core->ops->unprepare)
>> core->ops->unprepare(core->hw);
>>
>> + clk_pm_runtime_put(core);
>> +
>> trace_clk_unprepare_complete(core);
>> clk_core_unprepare(core->parent);
>> }
>> @@ -530,10 +576,14 @@ static int clk_core_prepare(struct clk_core *core)
>> return 0;
>>
>> if (core->prepare_count == 0) {
>> - ret = clk_core_prepare(core->parent);
>> + ret = clk_pm_runtime_get(core);
>> if (ret)
>> return ret;
>>
>> + ret = clk_core_prepare(core->parent);
>> + if (ret)
>> + goto runtime_put;
>> +
>> trace_clk_prepare(core);
>>
>> if (core->ops->prepare)
>> @@ -541,15 +591,18 @@ static int clk_core_prepare(struct clk_core *core)
>>
>> trace_clk_prepare_complete(core);
>>
>> - if (ret) {
>> - clk_core_unprepare(core->parent);
>> - return ret;
>> - }
>> + if (ret)
>> + goto unprepare;
>> }
>>
>> core->prepare_count++;
>>
>> return 0;
>> +unprepare:
>> + clk_core_unprepare(core->parent);
>> +runtime_put:
>> + clk_pm_runtime_put(core);
>> + return ret;
>> }
>>
>> static int clk_core_prepare_lock(struct clk_core *core)
>> @@ -1563,6 +1616,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>> {
>> struct clk_core *top, *fail_clk;
>> unsigned long rate = req_rate;
>> + int ret = 0;
>>
>> if (!core)
>> return 0;
>> @@ -1579,21 +1633,28 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>> if (!top)
>> return -EINVAL;
>>
>> + ret = clk_pm_runtime_get(core);
>> + if (ret)
>> + return ret;
>> +
>> /* notify that we are about to change rates */
>> fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
>> if (fail_clk) {
>> pr_debug("%s: failed to set %s rate\n", __func__,
>> fail_clk->name);
>> clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
>> - return -EBUSY;
>> + ret = -EBUSY;
>> + goto err;
>> }
>>
>> /* change the rates */
>> clk_change_rate(top);
>>
>> core->req_rate = req_rate;
>> +err:
>> + clk_pm_runtime_put(core);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> /**
>> @@ -1824,12 +1885,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>> p_rate = parent->rate;
>> }
>>
>> + ret = clk_pm_runtime_get(core);
>> + if (ret)
>> + goto out;
>> +
>> /* propagate PRE_RATE_CHANGE notifications */
>> ret = __clk_speculate_rates(core, p_rate);
>>
>> /* abort if a driver objects */
>> if (ret & NOTIFY_STOP_MASK)
>> - goto out;
>> + goto runtime_put;
>>
>> /* do the re-parent */
>> ret = __clk_set_parent(core, parent, p_index);
>> @@ -1842,6 +1907,8 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>> __clk_recalc_accuracies(core);
>> }
>>
>> +runtime_put:
>> + clk_pm_runtime_put(core);
>> out:
>> clk_prepare_unlock();
>>
>> @@ -2546,6 +2613,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>> goto fail_name;
>> }
>> core->ops = hw->init->ops;
>> + core->dev = dev;
>> if (dev && dev->driver)
>> core->owner = dev->driver->owner;
>> core->hw = hw;
>> --
>> 1.9.1
>>
> 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().
> 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
More information about the linux-arm-kernel
mailing list