This serie aime to support when the suspend/resume firmware alters the
clock tree, leading to an incorrect representation of the clock tree
after a resume from suspend-to-mem.
For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
alters the CPU clock tree in various ways.
Since we know which part of the tree is possibly altered, we introduce here
the clk_invalidate_rate() function that will rebuild the tree from the
hardware registers in case parents and dividers have changed.
Finally we call clk_invalidate_rate() from a new resume callback to refresh
the CPU clock tree after a resume.
With the clock tree refreshed, CCF can now handle the new clock tree
configuration and avoid crashing the system on further DVFS set_rates.
Neil Armstrong (2):
clk: introduce clk_invalidate_rate()
clk: meson: g12a: add suspend-resume hooks
drivers/clk/clk.c | 70 +++++++++++++++++++++++++++++++++++++++
drivers/clk/meson/g12a.c | 71 +++++++++++++++++++++++++++++++++++-----
include/linux/clk.h | 13 ++++++++
3 files changed, 146 insertions(+), 8 deletions(-)
--
2.22.0
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 <[email protected]>
---
drivers/clk/clk.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 13 +++++++++
2 files changed, 83 insertions(+)
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
--
2.22.0
Neil Armstrong <[email protected]> writes:
> This serie aime to support when the suspend/resume firmware alters the
> clock tree, leading to an incorrect representation of the clock tree
> after a resume from suspend-to-mem.
>
> For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
> alters the CPU clock tree in various ways.
>
> Since we know which part of the tree is possibly altered, we introduce here
> the clk_invalidate_rate() function that will rebuild the tree from the
> hardware registers in case parents and dividers have changed.
>
> Finally we call clk_invalidate_rate() from a new resume callback to refresh
> the CPU clock tree after a resume.
>
> With the clock tree refreshed, CCF can now handle the new clock tree
> configuration and avoid crashing the system on further DVFS set_rates.
For clarification, does this series work without the other proposed
fixes[1]? or is this dependent on that?
Kevin
[1] https://lore.kernel.org/linux-amlogic/[email protected]/
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 <[email protected]>
> ---
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 26/09/2019 22:46, Kevin Hilman wrote:
> Neil Armstrong <[email protected]> writes:
>
>> This serie aime to support when the suspend/resume firmware alters the
>> clock tree, leading to an incorrect representation of the clock tree
>> after a resume from suspend-to-mem.
>>
>> For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
>> alters the CPU clock tree in various ways.
>>
>> Since we know which part of the tree is possibly altered, we introduce here
>> the clk_invalidate_rate() function that will rebuild the tree from the
>> hardware registers in case parents and dividers have changed.
>>
>> Finally we call clk_invalidate_rate() from a new resume callback to refresh
>> the CPU clock tree after a resume.
>>
>> With the clock tree refreshed, CCF can now handle the new clock tree
>> configuration and avoid crashing the system on further DVFS set_rates.
>
> For clarification, does this series work without the other proposed
> fixes[1]? or is this dependent on that?
>
> Kevin
>
> [1] https://lore.kernel.org/linux-amlogic/[email protected]/
>
These are independent, but you'll need both to have suspend/resume fully working.
Neil
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 <[email protected]>
>> ---
>
> 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 <[email protected]> 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 <[email protected]>
>>> ---
>>
>> 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