diff mbox series

[07/13] clk: detect unintended rate changes

Message ID 20230918-imx8mp-dtsi-v1-7-1d008b3237c0@skidata.com
State New
Headers show
Series imx8mp: first clock propagation attempt (for LVDS) | expand

Commit Message

Benjamin Bara Sept. 17, 2023, 10:40 p.m. UTC 
From: Benjamin Bara <benjamin.bara@skidata.com>

As we now keep track of the clocks which are allowed to change - namely
the ones which are along the ancestor line between the rate trigger and
the top-most changed clock, we can run through the subtree of changes
and look for unexpected ones. Shared parents must set their rate in a
way, that all consumer-configured rates are respected. As this is
sometimes not possible and clocks sometime doesn't require the *exact*
rate, we might have to find a way to find out if it is *exact enough*.
Then we could fix it in the core.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

Comments

Maxime Ripard Sept. 19, 2023, 7:22 a.m. UTC | #1 
Hi,

On Mon, Sep 18, 2023 at 12:40:03AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> As we now keep track of the clocks which are allowed to change - namely
> the ones which are along the ancestor line between the rate trigger and
> the top-most changed clock

I'm not sure that the fact that only a clock between the clock
triggering the rate change and the top-most changed clock was allowed to
change has ever been a thing.

This puts a fairly big pressure on the tree propagation code, and
whether or not that can be done is completely context-dependent.

Devices like UART, I2C or audio devices are rate change sensitive, and
yet usually have internal dividers to accomodate for their parent rate
so don't usually care as long as they are notified.

Similarly, all the non-rate-sensitive devices (like pretty much all
bus/registers clocks) don't care at all about what the rate is, so that
requirement is making a rate change going through less likely, without a
particular reason only for a handful of devices (display in this case).

Also, this rules out any child of a clock from being allowed to change?
That looks wild to me :)

> we can run through the subtree of changes and look for unexpected
> ones.

Again, I'm not sure we can say that those changes were unexpected. They
very much were expected to change to the CCF so far.

> Shared parents must set their rate in a way, that all
> consumer-configured rates are respected.

Again, that entirely depends on the context: the clock tree topology,
the devices involved, what their drivers support, etc. I'm sure it's
true in your case, but I'm not sure we can make that a generic
statement.

> As this is sometimes not possible and clocks sometime doesn't require
> the *exact* rate, we might have to find a way to find out if it is
> *exact enough*. Then we could fix it in the core.

And "exact enough" entirely depends on the context again, so really, I'm
not sure we can (and should!) fix that at the framework level.

Maxime
diff mbox series

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 82c65ed432c5..faececc44c28 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2280,6 +2280,74 @@  static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	return top;
 }
 
+/*
+ * If the changed clock is consumer-configured, but not an ancestor of the
+ * trigger, it is most likely an unintended change. As a workaround, we try to
+ * set the rate back to the old without changing the parent. If this is not
+ * possible, the change should not have been suggested in the first place.
+ */
+static struct clk_core *clk_detect_unintended_rate_changes(struct clk_core *core,
+							   bool fix)
+{
+	struct clk_core *child, *tmp_clk;
+
+	if (core->rate == core->new_rate)
+		return NULL;
+
+	if (core->set_rate && core != rate_trigger_clk->core &&
+	    !clk_core_is_ancestor(rate_trigger_clk->core, core)) {
+		struct clk_core *parent = core->new_parent ? : core->parent;
+		struct clk_rate_request req;
+
+		pr_debug("%s: unintended change by %s (%lu -> %lu)\n", core->name,
+			 rate_trigger_clk->core->name, core->rate, core->new_rate);
+
+		if (fix) {
+			clk_hw_init_rate_request(core->hw, &req, core->rate);
+			req.best_parent_rate = parent->new_rate;
+			req.best_parent_hw = parent->hw;
+
+			if (clk_core_round_rate_nolock(core, &req))
+				return core;
+
+			/* TODO: how close is close enough? */
+			if (req.rate != core->rate) {
+				pr_debug("%s: %s fix failed, req=%lu, sugg=%lu\n",
+					 __func__, core->name, core->rate, req.rate);
+				return core;
+			}
+			if (req.best_parent_rate != parent->new_rate ||
+			    req.best_parent_hw != parent->hw) {
+				pr_debug("%s: %s fix failed, req=%s@%lu, sugg=%s@%lu\n",
+					 __func__, core->name, parent->name,
+					 parent->new_rate,
+					 req.best_parent_hw->core->name,
+					 req.best_parent_rate);
+				return core;
+			}
+
+			core->new_rate = core->rate;
+		}
+		return NULL;
+	}
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		if (child->new_parent && child->new_parent != core)
+			continue;
+		tmp_clk = clk_detect_unintended_rate_changes(child, fix);
+		if (tmp_clk)
+			return tmp_clk;
+	}
+
+	if (core->new_child) {
+		tmp_clk = clk_detect_unintended_rate_changes(core->new_child, fix);
+		if (tmp_clk)
+			return tmp_clk;
+	}
+
+	return NULL;
+}
+
 /*
  * Notify about rate changes in a subtree. Always walk down the whole tree
  * so that in case of an error we can walk down the whole tree again and
@@ -2484,6 +2552,21 @@  static int clk_core_set_rate_nolock(struct clk_core *core,
 		goto err;
 	}
 
+	/*
+	 * The notifying process offers the possibility to fix the rates of
+	 * unrelated clocks along the tree. After that, run a detection to find
+	 * clocks which are potentially wrongly configured now. These might be
+	 * fixed by the core in the future.
+	 */
+	fail_clk = clk_detect_unintended_rate_changes(top, false);
+	if (fail_clk) {
+		pr_err("%s: unintended rate change cannot be fixed\n",
+		       fail_clk->name);
+		ret = -EINVAL;
+		goto err;
+	}
+
+
 	/* change the rates */
 	clk_change_rate(top);