| Message ID | 20190919102518.25126-2-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: meson: g12a: handle clock hw changes while in suspend | expand |
Quoting Neil Armstrong (2019-09-19 03:25:17) > This introduces the clk_invalidate_rate() call used to recalculate the > rate and parent tree of a particular clock if it's known that the > underlying registers set has been altered by the firmware, like from > a suspend/resume handler running in trusted cpu mode. > > The call refreshes the actual parent and when changed, instructs CCF > the parent has changed. Finally the call will recalculate the rate of > each part of the tree to make sure the CCF cached tree is in sync with > the hardware. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- The knee-jerk reaction to these patches is that it shouldn't be a consumer API (i.e. taking a struct clk) but a provider API (i.e. taking a struct clk_hw). I haven't looked in any more detail but just know that it's a non-starter to be a consumer based API because we don't want random consumers out there to be telling the CCF or provider drivers that some clk has lost state and needs to be "refreshed".
On 27/09/2019 02:14, Stephen Boyd wrote: > Quoting Neil Armstrong (2019-09-19 03:25:17) >> This introduces the clk_invalidate_rate() call used to recalculate the >> rate and parent tree of a particular clock if it's known that the >> underlying registers set has been altered by the firmware, like from >> a suspend/resume handler running in trusted cpu mode. >> >> The call refreshes the actual parent and when changed, instructs CCF >> the parent has changed. Finally the call will recalculate the rate of >> each part of the tree to make sure the CCF cached tree is in sync with >> the hardware. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- > > The knee-jerk reaction to these patches is that it shouldn't be a > consumer API (i.e. taking a struct clk) but a provider API (i.e. taking > a struct clk_hw). I haven't looked in any more detail but just know that > it's a non-starter to be a consumer based API because we don't want > random consumers out there to be telling the CCF or provider drivers > that some clk has lost state and needs to be "refreshed". > Totally agree, I hesitated and obviously did the wrong choice, but this is a nit, the main algorithm is not tied to the API level. Should I resend it with clk_hw ? the difference will be small and the main subject is the resync algorithm. Neil
On Fri 27 Sep 2019 at 08:40, Neil Armstrong <narmstrong@baylibre.com> wrote: > On 27/09/2019 02:14, Stephen Boyd wrote: >> Quoting Neil Armstrong (2019-09-19 03:25:17) >>> This introduces the clk_invalidate_rate() call used to recalculate the >>> rate and parent tree of a particular clock if it's known that the >>> underlying registers set has been altered by the firmware, like from >>> a suspend/resume handler running in trusted cpu mode. >>> >>> The call refreshes the actual parent and when changed, instructs CCF >>> the parent has changed. Finally the call will recalculate the rate of >>> each part of the tree to make sure the CCF cached tree is in sync with >>> the hardware. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >> >> The knee-jerk reaction to these patches is that it shouldn't be a >> consumer API (i.e. taking a struct clk) but a provider API (i.e. taking >> a struct clk_hw). I haven't looked in any more detail but just know that >> it's a non-starter to be a consumer based API because we don't want >> random consumers out there to be telling the CCF or provider drivers >> that some clk has lost state and needs to be "refreshed". >> > > Totally agree, I hesitated and obviously did the wrong choice, but > this is a nit, the main algorithm is not tied to the API level. > > Should I resend it with clk_hw ? the difference will be small and > the main subject is the resync algorithm. Independent of the point above (partly a least), I wonder what will happen in some particular use cases * If clock is changed while in suspend. This clock can be a parent of the clock invalidated but currently is not. What happen, if later, it becomes the parent ? Since it is not parent on resume it won't be invalidated. CCF might still take a decision based on an invalid cached value. * If a mux is changed while in suspend, the parent is not correct anymore. The proposed patch recurse through the parents, it might not invalidate what we need/expect ... things are getting a bit unpredictable IOW, this change take a leaf clock and tries to tell CCF that any parent of this clock should not be trusted, but it might get it wrong in some cases. I think we should do it in the opposite way: * Mark the "rogue" clock with a flag (CLK_REFRESH ?) * Let CCF update the children of these clocks based on the new status Back to Stephen point, I don't know which API it should be, but I think the platform (fw driver or power stuff - not only clock provider) should be able somehow to trigger the mechanism to let CCF know something sketchy may have happened. For the parameter, maybe there should not be any (no struct clk or clk_hw) ? Maybe it would better if we let CCF refresh all the "rogue" clocks ? > > Neil
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ca99e9db6575..8acf38ce3cc4 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2557,6 +2557,76 @@ int clk_set_parent(struct clk *clk, struct clk *parent) } EXPORT_SYMBOL_GPL(clk_set_parent); +/** + * __clk_invalidate_tree + * @core: first clk in the subtree + * + * Walks the subtree of clks starting with clk and recalculates the parents, + * then accuracies and rates as it goes. + */ +static int __clk_invalidate_tree(struct clk_core *core) +{ + struct clk_core *parent, *old_parent; + int ret, i, num_parents; + + num_parents = core->num_parents; + + for (i = 0; i < num_parents; i++) { + parent = clk_core_get_parent_by_index(core, i); + if (!parent) + continue; + + ret = __clk_invalidate_tree(parent); + if (ret) + return ret; + } + + parent = __clk_init_parent(core); + + if (parent != core->parent) { + old_parent = __clk_set_parent_before(core, parent); + __clk_set_parent_after(core, parent, old_parent); + } + + __clk_recalc_accuracies(core); + __clk_recalc_rates(core, 0); + + return 0; +} + +static int clk_core_invalidate_rate(struct clk_core *core) +{ + int ret; + + clk_prepare_lock(); + + ret = __clk_invalidate_tree(core); + + clk_prepare_unlock(); + + return ret; +} + +/** + * clk_invalidate_rate - invalidate and recalc rate of the clock and it's tree + * @clk: the clk whose rate is too be invalidated + * + * If it's known the actual hardware state of a clock tree has changed, + * this call will invalidate the cached rate of the clk and it's possible + * parents tree to permit recalculation of the actual rate. + * + * Returns 0 on success, -EERROR otherwise. + * If clk is NULL then returns 0. + */ +int clk_invalidate_rate(struct clk *clk) +{ + if (!clk) + return 0; + + return clk_core_invalidate_rate(clk->core); +} +EXPORT_SYMBOL_GPL(clk_invalidate_rate); + static int clk_core_set_phase_nolock(struct clk_core *core, int degrees) { int ret = -EINVAL; diff --git a/include/linux/clk.h b/include/linux/clk.h index 853a8f181394..46db47ffb7b2 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -629,6 +629,19 @@ long clk_round_rate(struct clk *clk, unsigned long rate); */ int clk_set_rate(struct clk *clk, unsigned long rate); +/** + * clk_invalidate_rate - invalidate and recalc rate of the clock and it's tree + * @clk: the clk whose rate is too be invalidated + * + * If it's known the actual hardware state of a clock tree has changed, + * this call will invalidate the cached rate of the clk and it's possible + * parents tree to permit recalculation of the actual rate. + * + * Returns 0 on success, -EERROR otherwise. + * If clk is NULL then returns 0. + */ +int clk_invalidate_rate(struct clk *clk); + /** * clk_set_rate_exclusive- set the clock rate and claim exclusivity over * clock source
This introduces the clk_invalidate_rate() call used to recalculate the rate and parent tree of a particular clock if it's known that the underlying registers set has been altered by the firmware, like from a suspend/resume handler running in trusted cpu mode. The call refreshes the actual parent and when changed, instructs CCF the parent has changed. Finally the call will recalculate the rate of each part of the tree to make sure the CCF cached tree is in sync with the hardware. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/clk.c | 70 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 +++++++++ 2 files changed, 83 insertions(+)