From mboxrd@z Thu Jan 1 00:00:00 1970 From: m.szyprowski@samsung.com (Marek Szyprowski) Date: Tue, 13 Sep 2016 15:13:30 +0200 Subject: [PATCH 1/5] clk: add support for runtime pm In-Reply-To: References: <1472737551-15272-1-git-send-email-m.szyprowski@samsung.com> <1472737551-15272-2-git-send-email-m.szyprowski@samsung.com> Message-ID: <76bb5b71-dd4e-e1ad-df0b-b84833812502@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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 > 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 >> #include >> #include >> +#include >> #include >> #include >> >> @@ -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