[PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[Benjamin Bara]

Hi!

This is a spin-off of my initial series[1] with the core-related parts
picked out. Without the core part, the rest of the series only partly
makes sense, therefore I wanted to clarify the state of this first.
That's also the reason why it is marked as RFC for now.

Background:
The CCF is currently very rigid in terms of dealing with multiple rate
changes in a single clk_set_rate() call. However, with the
CLK_SET_RATE_PARENT property, it is very likely that a shared clock has
a couple of children which are changed "by accident" when the common
parent is changed. These children might be clock inputs of hardware
modules, which might have set a required rate previously. These required
rates are most likely still expected after the parent has been changed
by another clock (e.g. a sibling). Currently, it is not very trivial to
get these required rates inside of a clock driver's
{determine,round}_rate() op. Therefore, I think the core should also
participate in the process of ensuring that consumer-set requirements
are still fulfilled after a rate has changed.

Idea:
The idea is to have three rates set per clock, which need to be
considered during the whole process:

1. req_rate: This is the rate required by a consumer. It is set during a
   clk_set_rate() call.
2. new_rate: This is the "new planned rate" for the clock, which it will
   set, once the "finding new rates" process is finished.
3. req_rate_tmp: This rate is set if the clock is "required to change"
   during the process. It basically means that the clock is an ancestor
   of a rate-change trigger.

With these available, the core is able to validate the changed subtree
in a more simple way. It also allows us to re-enter calc_new_rates(),
which is one of the key components of clk_set_rate().

Thanks & regards
Benjamin

[1] https://lore.kernel.org/lkml/20230918-imx8mp-dtsi-v1-0-1d008b3237c0@skidata.com/

---
Benjamin Bara (4):
      clk: only set req_rate if it is set by consumer
      clk: reset new_rates for next run
      clk: introduce req_rate_tmp
      clk: consider rates when calculating subtree

 drivers/clk/clk.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 107 insertions(+), 18 deletions(-)
---
base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
change-id: 20230927-ccf-set-multiple-3c291416fc98

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH RFC 1/4] clk: only set req_rate if it is set by consumer

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Currently, the req_rate is set during initialization and during
re-parenting. Therefore, it is not clear whether the req_rate is really
required by a consumer or just set by accident. Fix this by only setting
the req_rate when it is really required (clk_set_rate() is called by
consumer).

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..82f954121e4d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -24,6 +24,8 @@
 
 #include "clk.h"
 
+#define CLK_RATE_UNSET -1UL
+
 static DEFINE_SPINLOCK(enable_lock);
 static DEFINE_MUTEX(prepare_lock);
 
@@ -1832,7 +1834,6 @@ static unsigned long clk_recalc(struct clk_core *core,
 /**
  * __clk_recalc_rates
  * @core: first clk in the subtree
- * @update_req: Whether req_rate should be updated with the new rate
  * @msg: notification type (see include/linux/clk.h)
  *
  * Walks the subtree of clks starting with clk and recalculates rates as it
@@ -1842,8 +1843,7 @@ static unsigned long clk_recalc(struct clk_core *core,
  * clk_recalc_rates also propagates the POST_RATE_CHANGE notification,
  * if necessary.
  */
-static void __clk_recalc_rates(struct clk_core *core, bool update_req,
-			       unsigned long msg)
+static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
 {
 	unsigned long old_rate;
 	unsigned long parent_rate = 0;
@@ -1857,8 +1857,6 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
 		parent_rate = core->parent->rate;
 
 	core->rate = clk_recalc(core, parent_rate);
-	if (update_req)
-		core->req_rate = core->rate;
 
 	/*
 	 * ignore NOTIFY_STOP and NOTIFY_BAD return values for POST_RATE_CHANGE
@@ -1868,13 +1866,13 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
 		__clk_notify(core, msg, old_rate, core->rate);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		__clk_recalc_rates(child, update_req, msg);
+		__clk_recalc_rates(child, msg);
 }
 
 static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
 {
 	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
-		__clk_recalc_rates(core, false, 0);
+		__clk_recalc_rates(core, 0);
 
 	return clk_core_get_rate_nolock(core);
 }
@@ -2455,7 +2453,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	/* change the rates */
 	clk_change_rate(top);
 
-	core->req_rate = req_rate;
 err:
 	clk_pm_runtime_put(core);
 
@@ -2485,6 +2482,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
  */
 int clk_set_rate(struct clk *clk, unsigned long rate)
 {
+	unsigned long old_req_rate;
 	int ret;
 
 	if (!clk)
@@ -2493,6 +2491,9 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	old_req_rate = clk->core->req_rate;
+	clk->core->req_rate = rate;
+
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
@@ -2501,6 +2502,9 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	if (clk->exclusive_count)
 		clk_core_rate_protect(clk->core);
 
+	if (ret)
+		clk->core->req_rate = old_req_rate;
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -2528,6 +2532,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate);
  */
 int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 {
+	unsigned long old_req_rate;
 	int ret;
 
 	if (!clk)
@@ -2536,6 +2541,9 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	old_req_rate = clk->core->req_rate;
+	clk->core->req_rate = rate;
+
 	/*
 	 * The temporary protection removal is not here, on purpose
 	 * This function is meant to be used instead of clk_rate_protect,
@@ -2546,6 +2554,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	if (!ret) {
 		clk_core_rate_protect(clk->core);
 		clk->exclusive_count++;
+	} else {
+		clk->core->req_rate = old_req_rate;
 	}
 
 	clk_prepare_unlock();
@@ -2723,7 +2733,7 @@ static void clk_core_reparent(struct clk_core *core,
 {
 	clk_reparent(core, new_parent);
 	__clk_recalc_accuracies(core);
-	__clk_recalc_rates(core, true, POST_RATE_CHANGE);
+	__clk_recalc_rates(core, POST_RATE_CHANGE);
 }
 
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
@@ -2807,9 +2817,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 
 	/* propagate rate an accuracy recalculation accordingly */
 	if (ret) {
-		__clk_recalc_rates(core, true, ABORT_RATE_CHANGE);
+		__clk_recalc_rates(core, ABORT_RATE_CHANGE);
 	} else {
-		__clk_recalc_rates(core, true, POST_RATE_CHANGE);
+		__clk_recalc_rates(core, POST_RATE_CHANGE);
 		__clk_recalc_accuracies(core);
 	}
 
@@ -3706,7 +3716,7 @@ static void clk_core_reparent_orphans_nolock(void)
 			__clk_set_parent_before(orphan, parent);
 			__clk_set_parent_after(orphan, parent, NULL);
 			__clk_recalc_accuracies(orphan);
-			__clk_recalc_rates(orphan, true, 0);
+			__clk_recalc_rates(orphan, 0);
 
 			/*
 			 * __clk_init_parent() will set the initial req_rate to
@@ -3888,7 +3898,8 @@ static int __clk_core_init(struct clk_core *core)
 		rate = parent->rate;
 	else
 		rate = 0;
-	core->rate = core->req_rate = rate;
+	core->rate = rate;
+	core->req_rate = CLK_RATE_UNSET;
 
 	/*
 	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks

-- 
2.34.1

[PATCH RFC 2/4] clk: reset new_rates for next run

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

During clk_set_rate(), new_rates are calculated for all clocks that are
part of the changed clock subtree. These values are temporary and only
valid during the current call. Therefore, reset them to prepare a clean
state for the next call.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 82f954121e4d..5f183cba300c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2163,6 +2163,17 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 	}
 }
 
+static void clk_reset_temp_rates(struct clk_core *core)
+{
+	struct clk_core *child;
+
+	core->new_rate = CLK_RATE_UNSET;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		clk_reset_temp_rates(child);
+	}
+}
+
 /*
  * calculate the new rates returning the topmost clock that has to be
  * changed.
@@ -2244,7 +2255,9 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		top = clk_calc_new_rates(parent, best_parent_rate);
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	/* only set new_rates if we found a valid change path */
+	if (top)
+		clk_calc_subtree(core, new_rate, parent, p_index);
 
 	return top;
 }
@@ -2347,6 +2360,7 @@ static void clk_change_rate(struct clk_core *core)
 
 	trace_clk_set_rate_complete(core, core->new_rate);
 
+	core->new_rate = CLK_RATE_UNSET;
 	core->rate = clk_recalc(core, best_parent_rate);
 
 	if (core->flags & CLK_SET_RATE_UNGATE) {
@@ -2361,7 +2375,7 @@ static void clk_change_rate(struct clk_core *core)
 		__clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
 
 	if (core->flags & CLK_RECALC_NEW_RATES)
-		(void)clk_calc_new_rates(core, core->new_rate);
+		(void)clk_calc_new_rates(core, core->rate);
 
 	/*
 	 * Use safe iteration, as change_rate can actually swap parents
@@ -2437,8 +2451,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EINVAL;
 
 	ret = clk_pm_runtime_get(core);
-	if (ret)
+	if (ret) {
+		clk_reset_temp_rates(top);
 		return ret;
+	}
 
 	/* notify that we are about to change rates */
 	fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
@@ -2454,6 +2470,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	clk_change_rate(top);
 
 err:
+	clk_reset_temp_rates(top);
+
 	clk_pm_runtime_put(core);
 
 	return ret;
@@ -3900,6 +3918,7 @@ static int __clk_core_init(struct clk_core *core)
 		rate = 0;
 	core->rate = rate;
 	core->req_rate = CLK_RATE_UNSET;
+	core->new_rate = CLK_RATE_UNSET;
 
 	/*
 	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks

-- 
2.34.1

[PATCH RFC 3/4] clk: introduce req_rate_tmp

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

A clock currently has a req_rate, which is required by the consumer, and
a next_rate, which is the outcoming rate of the ongoing clk_set_rate()
call. This commit introduces req_rate_tmp, which is a temporary required
rate for the ongoing change. This means, the clock is an ancestor of the
change trigger and a descendant of the top-most clock to change. This
helps us to identify if the clock is changed "by accident" (new_rate
set, req_rate_tmp not set).

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5f183cba300c..3e7dd97b71c3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -68,6 +68,7 @@ struct clk_core {
 	u8			new_parent_index;
 	unsigned long		rate;
 	unsigned long		req_rate;
+	unsigned long		req_rate_tmp;
 	unsigned long		new_rate;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
@@ -2168,6 +2169,7 @@ static void clk_reset_temp_rates(struct clk_core *core)
 	struct clk_core *child;
 
 	core->new_rate = CLK_RATE_UNSET;
+	core->req_rate_tmp = CLK_RATE_UNSET;
 
 	hlist_for_each_entry(child, &core->children, child_node) {
 		clk_reset_temp_rates(child);
@@ -2256,8 +2258,17 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 
 out:
 	/* only set new_rates if we found a valid change path */
-	if (top)
+	if (top) {
+		/*
+		 * The current clock is an ancestor of the trigger and therefore
+		 * is a clock which needs to be changed in this run. Clocks
+		 * with new_rate set but req_rate_tmp unset are changed "by
+		 * accident". On these clocks, the new_rate potentially
+		 * conflicts with req_rate.
+		 */
+		core->req_rate_tmp = new_rate;
 		clk_calc_subtree(core, new_rate, parent, p_index);
+	}
 
 	return top;
 }
@@ -3917,7 +3928,7 @@ static int __clk_core_init(struct clk_core *core)
 	else
 		rate = 0;
 	core->rate = rate;
-	core->req_rate = CLK_RATE_UNSET;
+	core->req_rate = core->req_rate_tmp = CLK_RATE_UNSET;
 	core->new_rate = CLK_RATE_UNSET;
 
 	/*

-- 
2.34.1

[PATCH RFC 4/4] clk: consider rates when calculating subtree

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

With req_rate_tmp and new_rate, we can identify if a clock is changed by
accident. Additionally, with req_rate, we are able to find out if a
clock consumer requires a certain rate. This enables us to find
"unwanted clock changes", meaning a clock is changed although its
consumer is expecting its current rate. On such a finding, the
"calculate new rates" process is started again, but this time for the
affected clock. During every execution of this process, we identify the
clocks which are required to be changed to get the requested rate. If a
clock needs to be changed a second time, we found a conflict. There are
various options to resolve the conflict, but for now we just error out.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3e7dd97b71c3..296d0fa510de 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2145,7 +2145,10 @@ static int __clk_speculate_rates(struct clk_core *core,
 	return ret;
 }
 
-static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
+static struct clk_core *clk_calc_new_rates(struct clk_core *core,
+					   unsigned long rate);
+
+static int clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 			     struct clk_core *new_parent, u8 p_index)
 {
 	struct clk_core *child;
@@ -2160,8 +2163,28 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 
 	hlist_for_each_entry(child, &core->children, child_node) {
 		child->new_rate = clk_recalc(child, new_rate);
-		clk_calc_subtree(child, child->new_rate, NULL, 0);
+
+		/*
+		 * A new rate of a shared parent clock might clash with a
+		 * required rate of a sibling. Ensure that the new rates are as
+		 * close as possible to the required ones. During this process,
+		 * the shared parent clock is not allowed to be changed again,
+		 * meaning it should have found the optimal rate already in the
+		 * initial clk_core_determine_round_nolock().
+		 */
+		if (child->req_rate != CLK_RATE_UNSET &&
+		    child->req_rate != child->new_rate) {
+			pr_debug("%s: set back to req=%lu\n", child->name,
+				 child->req_rate);
+			if (!clk_calc_new_rates(child, child->req_rate))
+				return -EINVAL;
+		}
+
+		if (clk_calc_subtree(child, child->new_rate, NULL, 0))
+			return -EINVAL;
 	}
+
+	return 0;
 }
 
 static void clk_reset_temp_rates(struct clk_core *core)
@@ -2259,6 +2282,22 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 out:
 	/* only set new_rates if we found a valid change path */
 	if (top) {
+		/*
+		 * If req_rate_tmp is set, the current clock is already required
+		 * to be changed in this run. However, if the new rate is not
+		 * the same as the temporary required one, the driver did not
+		 * consider its active rate (which is required by at least one
+		 * of its descendants from a previous run).
+		 */
+		if (core->req_rate_tmp != CLK_RATE_UNSET &&
+		    core->req_rate_tmp != new_rate) {
+			pr_warn("%s: %s: conflict: req=%lu new=%lu\n", __func__,
+				core->name, core->req_rate_tmp, new_rate);
+			pr_err(".{determine,round}_rate() of %s requires fix\n",
+			       core->name);
+			return NULL;
+		}
+
 		/*
 		 * The current clock is an ancestor of the trigger and therefore
 		 * is a clock which needs to be changed in this run. Clocks
@@ -2267,7 +2306,16 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		 * conflicts with req_rate.
 		 */
 		core->req_rate_tmp = new_rate;
-		clk_calc_subtree(core, new_rate, parent, p_index);
+
+		/*
+		 * Calculating the subtree potentially leads to a re-entrance
+		 * into clk_calc_new_rates(). This is required to set accidently
+		 * changed consumer-required clocks, back to their required
+		 * rate. This can fail in case of a conflict (shared parent
+		 * needs to be changed again).
+		 */
+		if (clk_calc_subtree(core, new_rate, parent, p_index))
+			return NULL;
 	}
 
 	return top;

-- 
2.34.1

Re: [PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]

Hi,

On Mon, Oct 02, 2023 at 11:23:31AM +0200, Benjamin Bara wrote:
> This is a spin-off of my initial series[1] with the core-related parts
> picked out. Without the core part, the rest of the series only partly
> makes sense, therefore I wanted to clarify the state of this first.
> That's also the reason why it is marked as RFC for now.
> 
> Background:
> The CCF is currently very rigid in terms of dealing with multiple rate
> changes in a single clk_set_rate() call. However, with the
> CLK_SET_RATE_PARENT property, it is very likely that a shared clock has
> a couple of children which are changed "by accident" when the common
> parent is changed. These children might be clock inputs of hardware
> modules, which might have set a required rate previously. These required
> rates are most likely still expected after the parent has been changed
> by another clock (e.g. a sibling). Currently, it is not very trivial to
> get these required rates inside of a clock driver's
> {determine,round}_rate() op. Therefore, I think the core should also
> participate in the process of ensuring that consumer-set requirements
> are still fulfilled after a rate has changed.
> 
> Idea:
> The idea is to have three rates set per clock, which need to be
> considered during the whole process:
> 
> 1. req_rate: This is the rate required by a consumer. It is set during a
>    clk_set_rate() call.
> 2. new_rate: This is the "new planned rate" for the clock, which it will
>    set, once the "finding new rates" process is finished.
> 3. req_rate_tmp: This rate is set if the clock is "required to change"
>    during the process. It basically means that the clock is an ancestor
>    of a rate-change trigger.
> 
> With these available, the core is able to validate the changed subtree
> in a more simple way. It also allows us to re-enter calc_new_rates(),
> which is one of the key components of clk_set_rate().

There's a couple of things you didn't reply on the first version and you
didn't really expand it here:

  - Most clocks don't care, and only the clocks that have used
    clk_set_rate_exclusive actually care. clk_set_rate never provided
    that guarantee, you're effectively merging clk_set_rate and
    clk_set_rate_exclusive. This might or might not be a good idea (it's
    probably not unless you want to track down regressions forever), but
    we should really tie this to clk_set_rate_exclusive or merge both.

  - Why do we need a new req_rate, and why req_rate can't be changed to
    accomodate your changes.

  - Why do you even need the core to be involved in the first place? You say you
    think it does, but you haven't answered any of my mails to provide more
    details and figure out if we can do it without it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[Benjamin Bara]

Hi Maxime,

thank you for the feedback!

On Mon, 2 Oct 2023 at 14:27, Maxime Ripard <mripard@kernel.org> wrote:
> There's a couple of things you didn't reply on the first version and
> you didn't really expand it here:

Sorry for that, wanted to get the reduced series out first to have a
better discussion base. Planned to reply to them and link to the
spin-off later, probably should have mentioned that :/ Thanks for
summarizing!

> Most clocks don't care, and only the clocks that have used
> clk_set_rate_exclusive actually care.

I think that is one of the main points I don't understand yet... Why? I
mean, the point of calling clk_set_rate() is to get a certain rate for a
clock, right? Why should the clock not care if it is changed to
something completely different? Maybe I am a bit biased here because I
use the imx8mp as a reference. On this platform, most hardware blocks
have an own divider and therefore the clocks which are connected to the
blocks are mostly "exclusive". E.g. the tree for a panel looks like
this:
-osc_24m (oscillator)
-- video_pll1_ref_sel (mux)
--- video_pll1 (configurable; shared)
---- video_pll1_bypass (mux; shared)
----- video_pll1_out (gate; shared)
------ media_disp2_pix (divider; "panel exclusive")
------- media_disp2_pix_root_clk (gate; "panel exclusive")
-------- <PANEL>

> clk_set_rate never provided that guarantee, you're effectively merging
> clk_set_rate and clk_set_rate_exclusive.

Ah, I guess I see what you mean... Since we would error out now on a
"conflict", this becomes very close to the "exclusiveness concept".
However, what I actually try to achieve is to leave the rest of the
subtree unaffected by a change (if required and possible).

> This might or might not be a good idea (it's probably not unless you
> want to track down regressions forever), but we should really tie this
> to clk_set_rate_exclusive or merge both.

I see that the current "conflict handling" might fit very well for
clk_set_rate_exclusive(). However, I think it's pretty hard to use
clk_set_rate_exclusive() in a multi-platform driver, as the other
competing consumers are not known. But maybe it makes sense to have the
same path and decide on a conflict whether we are allowed to do the
change or not (exclusive/protected).

> Why do we need a new req_rate, and why req_rate can't be changed to
> accomodate your changes.

For me, the existing req_rate is a "persistent" rate. It is the rate a
consumer requires the clock to have. It's something typically for leaves
of the clock-tree, which are directly connected to (probably
multi-platform) clock-consuming blocks, e.g. the dividers mentioned.
The new req_rate is "temporary". It is rather important for the !leaves
and indicate that a clock is required to change during this
clk_set_rate() call, in order to fulfill the requested rate.

Short example, let's say we have something like this:
- Video PLL
-- LVDS divider
--- LVDS bridge (HW block)
-- CRTC divider
--- Panel (HW block)

From a hardware-description point of view, the CRTC divider is exclusive
to the panel and the LVDS divider exclusive to the LVDS bridge. However,
the Video PLL is not the only possible parent of both and it should also
not be set exclusively by one of them.

When a CRTC rate of 35M is required by the panel, it would be set to the
following:
- Video PLL:     req_tmp=35M, req=-1,  new=35M
-- LVDS divider: req_tmp=-1,  req=-1,  new=35M (div=1)
-- CRTC divider: req_tmp=35M, req=35M, new=35M (div=1)

Next, the LVDS bridge requires 245M, which would be a multiple of
35M. The Video PLL is configured again, this time "by" the LVDS divider:
- Video PLL:     req_tmp=245M, req=-1,   new=245M
-- LVDS divider: req_tmp=245M, req=245M, new=245M (div=1)
-- CRTC divider: req_tmp=-1,   req=35M,  new=245M (div=1)

So without additional interaction (current behaviour), we would set the
CRTC divider to 245M, which contradicts with the unchanged previous
requirement stored in req. As req_tmp == -1, we know that the new rate
of the CRTC divider is not crucial for the actual requested change (LVDS
=> 245M). Therefore, what I would like to achieve is to have some
component/process that tells the CRTC divider to set its div to 7, as
this won't affect the ongoing requested change and would restore a
required rate of a different component, which was changed "unintended".

> Why do you even need the core to be involved in the first place? You
> say you think it does, but you haven't answered any of my mails to
> provide more details and figure out if we can do it without it.

We already have this functionality (calc required new rates) inside the
core and the core currently is the only one knowing all the context
about the tree-structure and the required and new rates. So I think in
the example above, calling calc_new_rates() again, this time with the
CRTC divider and req, might be the simplest solution to the problem.

I think as the Video PLL isn't directly consumed, we don't really have a
different possibility to achieve the same outcome, except of starting
Video PLL already with 245M (e.g. via device-tree).

Just for the sake of completeness:
A "conflict" occurs if this call would try to re-configure Video PLL
again (if req_tmp is already set; by not involving req here, we
basically avoid the "exclusiveness"). IMO, there are different ways to
proceed on a conflict: A possible clk_set_rate() option would be to
ignore a potential re-change of Video PLL by the second calc_new_rates()
and just set a somewhat close to the req rate for CRTC divider. A
possible clk_set_rate_exclusive() option is the one implemented here:
error out if we cannot guarantee the existing required rates for the
rest of the subtree.

Thanks & regards
Benjamin

Re: [PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10234 bytes --]

On Tue, Oct 03, 2023 at 09:44:07AM +0200, Benjamin Bara wrote:
> Hi Maxime,
>
> thank you for the feedback!
>
> On Mon, 2 Oct 2023 at 14:27, Maxime Ripard <mripard@kernel.org> wrote:
> > There's a couple of things you didn't reply on the first version and
> > you didn't really expand it here:
>
> Sorry for that, wanted to get the reduced series out first to have a
> better discussion base. Planned to reply to them and link to the
> spin-off later, probably should have mentioned that :/ Thanks for
> summarizing!

That's fine, don't worry :)

> > Most clocks don't care, and only the clocks that have used
> > clk_set_rate_exclusive actually care.
> 
> I think that is one of the main points I don't understand yet... Why? I
> mean, the point of calling clk_set_rate() is to get a certain rate for a
> clock, right? Why should the clock not care if it is changed to
> something completely different?

Because it doesn't matter for most clocks, and doesn't really affect the
system in any way. Or there's never any conflicting rate change that
would impact them. I guess it's on a per-clock basis so it's hard to
generalize, but the fact alone that the CCF is 10+ years old and we only
have to discuss it now is kind of a proof that things mostly work out
for most cases :)

Also, some systems have likely side-stepped the entire problem by
switching different outputs to different PLLs for example if the SoCs
allow to.

And while the vast majority of those drivers don't really care about the
rate being strictly enforced, I would expect a reasonable amount of them
to check the return code of clk_set_rate(). So if you start strictly
enforcing the rate, you'll also have to fail more often because there's
configuration that you just can't handle.

And that would lead to more drivers failing for something they don't
really care about :)

> Maybe I am a bit biased here because I use the imx8mp as a reference.
> On this platform, most hardware blocks have an own divider and
> therefore the clocks which are connected to the blocks are mostly
> "exclusive". E.g. the tree for a panel looks like
> this:
> -osc_24m (oscillator)
> -- video_pll1_ref_sel (mux)
> --- video_pll1 (configurable; shared)
> ---- video_pll1_bypass (mux; shared)
> ----- video_pll1_out (gate; shared)
> ------ media_disp2_pix (divider; "panel exclusive")
> ------- media_disp2_pix_root_clk (gate; "panel exclusive")
> -------- <PANEL>

I guess that's mostly a typical clock tree for a panel. However, notice
that you only took a handful of clocks as an example, and not the 50-100
others in your system. While the panel clock rate is critical, the
register clock speed of the timers probably won't be as much.

And that's what I was mentioning really: out of the 50-100 clocks in
your system right now, you only really care about the rate of ~5, and
most of them are going to be in different subtrees.

So, while your problem is indeed a concern, it's also a corner case.

> > clk_set_rate never provided that guarantee, you're effectively merging
> > clk_set_rate and clk_set_rate_exclusive.
> 
> Ah, I guess I see what you mean... Since we would error out now on a
> "conflict", this becomes very close to the "exclusiveness concept".

Yeah, exactly. clk_set_rate() gains the "I want this rate to be enforced
forever" property that clk_set_rate_exclusive() provides already.

> However, what I actually try to achieve is to leave the rest of the
> subtree unaffected by a change (if required and possible).
> 
> > This might or might not be a good idea (it's probably not unless you
> > want to track down regressions forever), but we should really tie this
> > to clk_set_rate_exclusive or merge both.
> 
> I see that the current "conflict handling" might fit very well for
> clk_set_rate_exclusive(). However, I think it's pretty hard to use
> clk_set_rate_exclusive() in a multi-platform driver, as the other
> competing consumers are not known.

You don't know that with clk_set_rate either though?

> But maybe it makes sense to have the same path and decide on a
> conflict whether we are allowed to do the change or not
> (exclusive/protected).
> 
> > Why do we need a new req_rate, and why req_rate can't be changed to
> > accomodate your changes.
> 
> For me, the existing req_rate is a "persistent" rate. It is the rate a
> consumer requires the clock to have. It's something typically for leaves
> of the clock-tree, which are directly connected to (probably
> multi-platform) clock-consuming blocks, e.g. the dividers mentioned.
> The new req_rate is "temporary". It is rather important for the !leaves
> and indicate that a clock is required to change during this
> clk_set_rate() call, in order to fulfill the requested rate.
> 
> Short example, let's say we have something like this:
> - Video PLL
> -- LVDS divider
> --- LVDS bridge (HW block)
> -- CRTC divider
> --- Panel (HW block)
> 
> From a hardware-description point of view, the CRTC divider is exclusive
> to the panel and the LVDS divider exclusive to the LVDS bridge. However,
> the Video PLL is not the only possible parent of both and it should also
> not be set exclusively by one of them.
> 
> When a CRTC rate of 35M is required by the panel, it would be set to the
> following:
> - Video PLL:     req_tmp=35M, req=-1,  new=35M
> -- LVDS divider: req_tmp=-1,  req=-1,  new=35M (div=1)
> -- CRTC divider: req_tmp=35M, req=35M, new=35M (div=1)
> 
> Next, the LVDS bridge requires 245M, which would be a multiple of
> 35M. The Video PLL is configured again, this time "by" the LVDS divider:
> - Video PLL:     req_tmp=245M, req=-1,   new=245M
> -- LVDS divider: req_tmp=245M, req=245M, new=245M (div=1)
> -- CRTC divider: req_tmp=-1,   req=35M,  new=245M (div=1)
> 
> So without additional interaction (current behaviour), we would set the
> CRTC divider to 245M, which contradicts with the unchanged previous
> requirement stored in req. As req_tmp == -1, we know that the new rate
> of the CRTC divider is not crucial for the actual requested change (LVDS
> => 245M). Therefore, what I would like to achieve is to have some
> component/process that tells the CRTC divider to set its div to 7, as
> this won't affect the ongoing requested change and would restore a
> required rate of a different component, which was changed "unintended".

My point earlier was that if we configure the Video PLL from the start
to be 245MHz, then we don't need to worry about it.

For the temporary requested rate, it's not clear to me why we should
store it into the struct clk_core itself. It looks to be transient by
nature, so it would be better in the clk_rate_request structure.
clk_rate_request are now instantiated by functions, maybe we could add a
pointer to the clk_rate_request that triggered it (some kind of
"parent", but most likely to go from the child clock to the parent)?

> > Why do you even need the core to be involved in the first place? You
> > say you think it does, but you haven't answered any of my mails to
> > provide more details and figure out if we can do it without it.
> 
> We already have this functionality (calc required new rates) inside the
> core and the core currently is the only one knowing all the context
> about the tree-structure and the required and new rates.

I mean, that whole discussion kind of proves that we don't have that
functionality in the core.

> So I think in the example above, calling calc_new_rates() again, this
> time with the CRTC divider and req, might be the simplest solution to
> the problem.
> 
> I think as the Video PLL isn't directly consumed, we don't really have a
> different possibility to achieve the same outcome, except of starting
> Video PLL already with 245M (e.g. via device-tree).

It doesn't need to happen in the device tree, but that sounds completely
reasonable to me.

> Just for the sake of completeness:
> A "conflict" occurs if this call would try to re-configure Video PLL
> again (if req_tmp is already set; by not involving req here, we
> basically avoid the "exclusiveness"). IMO, there are different ways to
> proceed on a conflict: A possible clk_set_rate() option would be to
> ignore a potential re-change of Video PLL by the second calc_new_rates()
> and just set a somewhat close to the req rate for CRTC divider. A
> possible clk_set_rate_exclusive() option is the one implemented here:
> error out if we cannot guarantee the existing required rates for the
> rest of the subtree.

The problem here is that you effectively want to coordinate clocks to
change their rate. Most of the logic of whether or not they care about
it, and how they care about it cannot be embedded in the clock
framework.

If we want to address this properly, I think we would need to switch the
entire clock framework to some kind of state like KMS does.

Something like:

  - All the affected clocks by a configuration (rate, parent, phase,
    accuracy, etc.) change are supposed to be within the state.

  - Whenever someone does a clk_set_rate call, we build up a state with
    the clock it was called on and all its child clocks.

  - We introduce a new hook in clk_ops that will take that state and the
    clock tells whether it's ok, or whether it needs to modify some
    parameters. We call every clock, starting from the top most clock,
    asking whether it's ok or if they need to change anything. If they
    need to change anything, we start again with the new set of
    parameters.

  - Clocks are free to pull into the state more clocks (ie, their
    parent), which in turn will pull their child.

  - Once every driver agrees, we "commit" that state.

That way, we can keep the driver requirements in the driver, and every
clock affected by a rate change can adapt.

That's a major undertaking, and we would need a bunch of helpers to
maintain compatibility with the current API we have. Plus tons of kunit
tests.

Again, if we have the option to just run the PLL at some multiple of
both, I really think for everyone's sanity that we should do that.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]