Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
From:   Benjamin Bara <bbara93@gmail.com>
Date:   Mon, 02 Oct 2023 11:23:35 +0200
Subject: [PATCH RFC 4/4] clk: consider rates when calculating subtree
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <20231002-ccf-set-multiple-v1-4-2df5e9eb3738@skidata.com>
References: <20231002-ccf-set-multiple-v1-0-2df5e9eb3738@skidata.com>
In-Reply-To: <20231002-ccf-set-multiple-v1-0-2df5e9eb3738@skidata.com>
To:     Maxime Ripard <mripard@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>
Cc:     Frank Oltmanns <frank@oltmanns.dev>,
        Adam Ford <aford173@gmail.com>, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
Precedence: bulk

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