| Message ID | 1502274907-11931-2-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, Quoting Marek Szyprowski (2017-08-09 03:35:03) > 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. > > When one wants to register clock controller, which make use of this feature, he > has to: > 1. Provide a struct device to the core when registering the provider. > 2. Ensure to enable runtime PM for that device before registering clocks. > 3. Make sure that the runtime PM status of the controller device reflects > the HW state. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Overall the idea and the patch look good to me. Calling pm_runtime_get for prepare and pm_runtime_put for unprepared makes perfect sense to me. It means that the power domain that powers that clock hardware must be on for that clock to function properly. It’s worth pointing out that this patch leaves that power domain enabled for for as long as prepare_count is greater than zero. However the rate-change operations only use the pm_runtime functions to program the hardware and modify the registers. At the end of each rate-change call there is a pm_runtime_put. This makes sense to me, but I’m trying to wrap my head around the fact that this patch introduces two behaviors: 1) protect access to the clk hardware by wrapping writes across the bus with pm_runtime calls 2) power the clock logic when the clock is in use (e.g. clk_prepare()) Everything about this patch *seems* correct to me, but I’m trying to figure out what it means to for a clock consumer driver to do something like this: clk_prepare_enable() clk_disable_unprepare() clk_set_rate() Maybe patches #2-5 will help me understand, but I wanted to jot this down in my review before I forget. > @@ -2583,6 +2673,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > goto fail_name; > } > core->ops = hw->init->ops; > + if (dev && pm_runtime_enabled(dev)) { > + core->dev = dev; > + ret = clk_pm_runtime_get(core); > + if (ret) > + goto fail_pm; > + } > if (dev && dev->driver) > core->owner = dev->driver->owner; > core->hw = hw; > @@ -2629,12 +2725,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > } > > ret = __clk_core_init(core); > - if (!ret) > + if (!ret) { > + clk_pm_runtime_put(core); > return hw->clk; > + } I wonder if the above can be moved inside of __clk_core_init? clk_register shouldn't manage any clk_ops directly, only __clk_core_init does this. Regards, Mike
Hi Michael, Thanks for the comments! On 2017-08-10 20:28, Michael Turquette wrote: > Hi Marek, > > Quoting Marek Szyprowski (2017-08-09 03:35:03) >> 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. >> >> When one wants to register clock controller, which make use of this feature, he >> has to: >> 1. Provide a struct device to the core when registering the provider. >> 2. Ensure to enable runtime PM for that device before registering clocks. >> 3. Make sure that the runtime PM status of the controller device reflects >> the HW state. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >> Acked-by: Krzysztof Kozlowski <krzk@kernel.org> > Overall the idea and the patch look good to me. > > Calling pm_runtime_get for prepare and pm_runtime_put for unprepared > makes perfect sense to me. It means that the power domain that powers > that clock hardware must be on for that clock to function properly. > > It’s worth pointing out that this patch leaves that power domain enabled > for for as long as prepare_count is greater than zero. > > However the rate-change operations only use the pm_runtime functions to > program the hardware and modify the registers. At the end of each > rate-change call there is a pm_runtime_put. This makes sense to me, but > I’m trying to wrap my head around the fact that this patch introduces > two behaviors: > > 1) protect access to the clk hardware by wrapping writes across the bus > with pm_runtime calls > 2) power the clock logic when the clock is in use (e.g. clk_prepare()) > > Everything about this patch *seems* correct to me, but I’m trying to > figure out what it means to for a clock consumer driver to do something > like this: > > clk_prepare_enable() > clk_disable_unprepare() > clk_set_rate() > > Maybe patches #2-5 will help me understand, but I wanted to jot this > down in my review before I forget. Well, the question is what will the above sequence do without my patches? IMO the only result of above calls is (optionally) changed rate of parent clocks and appropriate values written to given clock's registers, so next time one calls clk_(prepare)_enable(), the given clock will be enabled with the configured rate. With runtime pm patches the overall result will be the same. However, if the given clock is the only/last enabled clock from the provider, there might be a runtime pm resume/suspend transition (2 times: one from prepare/unprepare calls, the second from set_rate calls). This means that the rate of parent clocks (especially if they comes from the other providers) might or might not change between those transitions. This depends how the runtime pm is implemented and what operations has to be done for putting provider into suspend mode. The final result will be however the same as without runtime pm patches. Clock provider's runtime pm suspend callback should save internal configuration and resume callback must restore it completely, so the provider's internal state is exactly the same as before suspend. If we assume that clock provider might be in a power domain, the internal clock provider's hardware context might be lost after suspend callback. >> @@ -2583,6 +2673,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) >> goto fail_name; >> } >> core->ops = hw->init->ops; >> + if (dev && pm_runtime_enabled(dev)) { >> + core->dev = dev; >> + ret = clk_pm_runtime_get(core); >> + if (ret) >> + goto fail_pm; >> + } >> if (dev && dev->driver) >> core->owner = dev->driver->owner; >> core->hw = hw; >> @@ -2629,12 +2725,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) >> } >> >> ret = __clk_core_init(core); >> - if (!ret) >> + if (!ret) { >> + clk_pm_runtime_put(core); >> return hw->clk; >> + } > I wonder if the above can be moved inside of __clk_core_init? > clk_register shouldn't manage any clk_ops directly, only __clk_core_init > does this. If you prefer that, I will move runtime pm related bits to __clk_core_init then. Best regards
Quoting Marek Szyprowski (2017-08-10 23:18:42) > Hi Michael, > > Thanks for the comments! > > On 2017-08-10 20:28, Michael Turquette wrote: > > Hi Marek, > > > > Quoting Marek Szyprowski (2017-08-09 03:35:03) > >> 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. > >> > >> When one wants to register clock controller, which make use of this feature, he > >> has to: > >> 1. Provide a struct device to the core when registering the provider. > >> 2. Ensure to enable runtime PM for that device before registering clocks. > >> 3. Make sure that the runtime PM status of the controller device reflects > >> the HW state. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > >> Acked-by: Krzysztof Kozlowski <krzk@kernel.org> > > Overall the idea and the patch look good to me. > > > > Calling pm_runtime_get for prepare and pm_runtime_put for unprepared > > makes perfect sense to me. It means that the power domain that powers > > that clock hardware must be on for that clock to function properly. > > > > It’s worth pointing out that this patch leaves that power domain enabled > > for for as long as prepare_count is greater than zero. > > > > However the rate-change operations only use the pm_runtime functions to > > program the hardware and modify the registers. At the end of each > > rate-change call there is a pm_runtime_put. This makes sense to me, but > > I’m trying to wrap my head around the fact that this patch introduces > > two behaviors: > > > > 1) protect access to the clk hardware by wrapping writes across the bus > > with pm_runtime calls > > 2) power the clock logic when the clock is in use (e.g. clk_prepare()) > > > > Everything about this patch *seems* correct to me, but I’m trying to > > figure out what it means to for a clock consumer driver to do something > > like this: > > > > clk_prepare_enable() > > clk_disable_unprepare() > > clk_set_rate() > > > > Maybe patches #2-5 will help me understand, but I wanted to jot this > > down in my review before I forget. > > Well, the question is what will the above sequence do without my patches? > > IMO the only result of above calls is (optionally) changed rate of parent > clocks and appropriate values written to given clock's registers, so next > time one calls clk_(prepare)_enable(), the given clock will be enabled > with the configured rate. > > With runtime pm patches the overall result will be the same. However, if > the given clock is the only/last enabled clock from the provider, there > might be a runtime pm resume/suspend transition (2 times: one from > prepare/unprepare calls, the second from set_rate calls). This means that > the rate of parent clocks (especially if they comes from the other > providers) might or might not change between those transitions. This > depends how the runtime pm is implemented and what operations has to be > done for putting provider into suspend mode. The final result will be > however the same as without runtime pm patches. > > Clock provider's runtime pm suspend callback should save internal > configuration and resume callback must restore it completely, so the > provider's internal state is exactly the same as before suspend. If we > assume that clock provider might be in a power domain, the internal > clock provider's hardware context might be lost after suspend callback. > > >> @@ -2583,6 +2673,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > >> goto fail_name; > >> } > >> core->ops = hw->init->ops; > >> + if (dev && pm_runtime_enabled(dev)) { > >> + core->dev = dev; > >> + ret = clk_pm_runtime_get(core); > >> + if (ret) > >> + goto fail_pm; > >> + } > >> if (dev && dev->driver) > >> core->owner = dev->driver->owner; > >> core->hw = hw; > >> @@ -2629,12 +2725,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > >> } > >> > >> ret = __clk_core_init(core); > >> - if (!ret) > >> + if (!ret) { > >> + clk_pm_runtime_put(core); > >> return hw->clk; > >> + } > > I wonder if the above can be moved inside of __clk_core_init? > > clk_register shouldn't manage any clk_ops directly, only __clk_core_init > > does this. > > If you prefer that, I will move runtime pm related bits to > __clk_core_init then. Please do. After that I can pull v9 into clk-next. Thanks, Mike > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fc58c52a26b4..eb11a6a0e1d0 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,26 @@ 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; + + ret = pm_runtime_get_sync(core->dev); + return ret < 0 ? ret : 0; +} + +static void clk_pm_runtime_put(struct clk_core *core) +{ + if (!core->dev) + return; + + pm_runtime_put_sync(core->dev); +} + /*** locking ***/ static void clk_prepare_lock(void) { @@ -150,6 +172,8 @@ static void clk_enable_unlock(unsigned long flags) static bool clk_core_is_prepared(struct clk_core *core) { + bool ret = false; + /* * .is_prepared is optional for clocks that can prepare * fall back to software usage counter if it is missing @@ -157,11 +181,18 @@ static bool clk_core_is_prepared(struct clk_core *core) if (!core->ops->is_prepared) return core->prepare_count; - return core->ops->is_prepared(core->hw); + if (!clk_pm_runtime_get(core)) { + ret = core->ops->is_prepared(core->hw); + clk_pm_runtime_put(core); + } + + return ret; } static bool clk_core_is_enabled(struct clk_core *core) { + bool ret = false; + /* * .is_enabled is only mandatory for clocks that gate * fall back to software usage counter if .is_enabled is missing @@ -169,7 +200,29 @@ static bool clk_core_is_enabled(struct clk_core *core) if (!core->ops->is_enabled) return core->enable_count; - return core->ops->is_enabled(core->hw); + /* + * Check if clock controller's device is runtime active before + * calling .is_enabled callback. If not, assume that clock is + * disabled, because we might be called from atomic context, from + * which pm_runtime_get() is not allowed. + * This function is called mainly from clk_disable_unused_subtree, + * which ensures proper runtime pm activation of controller before + * taking enable spinlock, but the below check is needed if one tries + * to call it from other places. + */ + if (core->dev) { + pm_runtime_get_noresume(core->dev); + if (!pm_runtime_active(core->dev)) { + ret = false; + goto done; + } + } + + ret = core->ops->is_enabled(core->hw); +done: + clk_pm_runtime_put(core); + + return ret; } /*** helper functions ***/ @@ -489,6 +542,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 +585,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 +600,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) @@ -745,6 +807,9 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) if (core->flags & CLK_IGNORE_UNUSED) return; + if (clk_pm_runtime_get(core)) + return; + if (clk_core_is_prepared(core)) { trace_clk_unprepare(core); if (core->ops->unprepare_unused) @@ -753,6 +818,8 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) core->ops->unprepare(core->hw); trace_clk_unprepare_complete(core); } + + clk_pm_runtime_put(core); } static void clk_disable_unused_subtree(struct clk_core *core) @@ -768,6 +835,9 @@ static void clk_disable_unused_subtree(struct clk_core *core) if (core->flags & CLK_OPS_PARENT_ENABLE) clk_core_prepare_enable(core->parent); + if (clk_pm_runtime_get(core)) + goto unprepare_out; + flags = clk_enable_lock(); if (core->enable_count) @@ -792,6 +862,8 @@ static void clk_disable_unused_subtree(struct clk_core *core) unlock_out: clk_enable_unlock(flags); + clk_pm_runtime_put(core); +unprepare_out: if (core->flags & CLK_OPS_PARENT_ENABLE) clk_core_disable_unprepare(core->parent); } @@ -1038,9 +1110,13 @@ long clk_get_accuracy(struct clk *clk) static unsigned long clk_recalc(struct clk_core *core, unsigned long parent_rate) { - if (core->ops->recalc_rate) - return core->ops->recalc_rate(core->hw, parent_rate); - return parent_rate; + unsigned long rate = parent_rate; + + if (core->ops->recalc_rate && !clk_pm_runtime_get(core)) { + rate = core->ops->recalc_rate(core->hw, parent_rate); + clk_pm_runtime_put(core); + } + return rate; } /** @@ -1565,6 +1641,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; @@ -1581,21 +1658,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; } /** @@ -1826,12 +1910,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); @@ -1844,6 +1932,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(); @@ -2583,6 +2673,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) goto fail_name; } core->ops = hw->init->ops; + if (dev && pm_runtime_enabled(dev)) { + core->dev = dev; + ret = clk_pm_runtime_get(core); + if (ret) + goto fail_pm; + } if (dev && dev->driver) core->owner = dev->driver->owner; core->hw = hw; @@ -2629,12 +2725,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) } ret = __clk_core_init(core); - if (!ret) + if (!ret) { + clk_pm_runtime_put(core); return hw->clk; + } __clk_free_clk(hw->clk); hw->clk = NULL; - fail_parents: kfree(core->parents); fail_parent_names_copy: @@ -2642,6 +2739,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) kfree_const(core->parent_names[i]); kfree(core->parent_names); fail_parent_names: + clk_pm_runtime_put(core); +fail_pm: kfree_const(core->name); fail_name: kfree(core);