[RFC,1/2] clk: introduce clk_invalidate_rate()
diff mbox series

Message ID 20190919102518.25126-2-narmstrong@baylibre.com
State Not Applicable
Headers show
Series
  • clk: meson: g12a: handle clock hw changes while in suspend
Related show

Commit Message

Neil Armstrong Sept. 19, 2019, 10:25 a.m. UTC
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(+)

Comments

Stephen Boyd Sept. 27, 2019, 12:14 a.m. UTC | #1
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".
Neil Armstrong Sept. 27, 2019, 6:40 a.m. UTC | #2
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
Jerome Brunet Sept. 27, 2019, 4:38 p.m. UTC | #3
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

Patch
diff mbox series

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