Skip to contentSkip to conversation

clk: More clock rate fixes and tests

PATCH Superseded Maxime Ripard 2 participants 30 messages Aug 15, 2022 – Aug 16, 2022

Subsystems (5)

v8 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

v8 cover clk: More clock rate fixes and tests

Hi,

Thanks to the feedback I got on the previous series, I found and fixed a number of bugs in the clock framework and how it deals with rates, especially when it comes to orphan clocks.

In order to make sure this doesn't pop up again as a regression, I've extended the number of tests.

The first patch reintroduces the clk_set_rate_range call on clk_put, but this time will only do so if there was a range set on that clock to begin with. It should be less intrusive, and reduce the number of potential side effects considerably.

We then have a fix for the qcom rcg2 issue that has been reported recently.

All the other patches should be probably be flagged as fixes, but they've never seem to have shown any real-world issues until now, and they aren't all really trivial to backport either, so I'm not sure it's worth it.

There's also some documentation improvements for recalc_rate and clk_get_rate to hopefully make the documentation less ambiguous and acknowledge that recalc_rate() returning 0 on error is fine.

Let me know what you think,
Maxime

Changes from v7:
- Dropped the RPi fixes
- Rebased on 6.0-rc1

Changes from v6:
- Fixed a kernel build bot warning

Changes from v5:
- Rebased on current next (next-20220711)
- Dropped clk_get_rate_range, and used a custom function instead
- Switched all tests to use clk_hw_get_clk() instead of struct clk_hw->clk
- Removed some intermediate variables
- Added some comments
- Dropped clk_get_parent() changes
- Dropped test on clk_hw pointer non-NULL in clk_hw_get_name
- Made clk_has_parent more const

Changes from v4:
- Fix build breakage on SAM9x60

Changes from v3:
- constness warning fix in clk_core_forward_rate_req

Changes from v2:
- Rebased on top of current next
- Fixed locking issue in clk_get_rate_range

Changes from v1:
- Rebased on top of next-20220428
- Dropped the patch to prevent non-orphan clocks from registering if their recalc_rate hook returns 0
- Added some patches to clarify the clk_get_rate and recalc_rate documentation
- Dropped the patch to skip the range setup on an orphan clock that was introducing a regression on RaspberryPi3 when a monitor wasn't connected at boot
- Added a patch to skip the rate clamping in clk_round_rate() when

    min_rate == max_rate == 0

- Added a new set of functions to query the clk boundaries and fix a regression with the RaspberryPi4
- Fixed all the drivers hand-crafting their clk_rate_request
- Reworded the test suite descriptions
- Reordered a few patches to ease the review
- Reworded some commit logs to better explain the issues they address
- Collected the Tested-by of Alexander and Marek
- More tests

Maxime Ripard (25):
clk: test: Switch to clk_hw_get_clk
clk: Drop the rate range on clk_put()
clk: Skip clamping when rounding if there's no boundaries
clk: Mention that .recalc_rate can return 0 on error
clk: Clarify clk_get_rate() expectations
clk: tests: Add test suites description
clk: tests: Add reference to the orphan mux bug report
clk: tests: Add tests for uncached clock
clk: tests: Add tests for single parent mux
clk: tests: Add tests for mux with multiple parents
clk: tests: Add some tests for orphan with multiple parents
clk: Take into account uncached clocks in clk_set_rate_range()
clk: Set req_rate on reparenting
clk: Change clk_core_init_rate_req prototype
clk: Move clk_core_init_rate_req() from clk_core_round_rate_nolock()
to its caller
clk: Introduce clk_hw_init_rate_request()
clk: Add our request boundaries in clk_core_init_rate_req
clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
clk: Introduce clk_core_has_parent()
clk: Constify clk_has_parent()
clk: Stop forwarding clk_rate_requests to the parent
clk: Zero the clk_rate_request structure
clk: Introduce the clk_hw_get_rate_range function
clk: qcom: clk-rcg2: Take clock boundaries into consideration for
gfx3d
clk: tests: Add missing test case for ranges

 drivers/clk/at91/clk-generated.c  |    5 +-
 drivers/clk/at91/clk-master.c     |    9 +-
 drivers/clk/at91/clk-peripheral.c |    4 +-
 drivers/clk/clk-composite.c       |    6 +-
 drivers/clk/clk-divider.c         |   20 +-
 drivers/clk/clk.c                 |  284 ++++--
 drivers/clk/clk_test.c            | 1413 ++++++++++++++++++++++++++++-
 drivers/clk/qcom/clk-rcg2.c       |    9 +
 include/linux/clk-provider.h      |   18 +-
 include/linux/clk.h               |    2 +-
 10 files changed, 1661 insertions(+), 109 deletions(-)
Signature · 1 line
2.37.1
#1 lore raw

PATCH v8 1/25 clk: test: Switch to clk_hw_get_clk

Following the clk_hw->clk pointer is equivalent to calling clk_hw_get_clk(), but will make the job harder if we need to rework that part in the future.

---
 drivers/clk/clk_test.c | 74 +++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 19 deletions(-)
drivers/clk/clk_test.c +55 -19
index 6731a822f4e3..7646356f30cb 100644
@@ -160,12 +160,14 @@ static void clk_test_get_rate(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, ctx->rate);
+
+	clk_put(clk);
 }
 
 /*
@@ -179,7 +181,7 @@ static void clk_test_set_get_rate(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -189,6 +191,8 @@ static void clk_test_set_get_rate(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(clk);
 }
 
 /*
@@ -202,7 +206,7 @@ static void clk_test_set_set_get_rate(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -216,6 +220,8 @@ static void clk_test_set_set_get_rate(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -226,7 +232,7 @@ static void clk_test_round_set_get_rate(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rounded_rate, set_rate;
 
 	rounded_rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1);
@@ -240,6 +246,8 @@ static void clk_test_round_set_get_rate(struct kunit *test)
 	set_rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, set_rate, 0);
 	KUNIT_EXPECT_EQ(test, rounded_rate, set_rate);
+
+	clk_put(clk);
 }
 
 static struct kunit_case clk_test_cases[] = {
@@ -314,7 +322,7 @@ static void clk_test_orphan_transparent_parent_mux_set_range(struct kunit *test)
 {
 	struct clk_single_parent_ctx *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate, new_rate;
 
 	rate = clk_get_rate(clk);
@@ -329,6 +337,8 @@ static void clk_test_orphan_transparent_parent_mux_set_range(struct kunit *test)
 	new_rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, new_rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, new_rate);
+
+	clk_put(clk);
 }
 
 static struct kunit_case clk_orphan_transparent_single_parent_mux_test_cases[] = {
@@ -352,7 +362,7 @@ static void clk_range_test_set_range(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -365,6 +375,8 @@ static void clk_range_test_set_range(struct kunit *test)
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -375,13 +387,15 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 
 	KUNIT_EXPECT_LT(test,
 			clk_set_rate_range(clk,
 					   DUMMY_CLOCK_RATE_1 + 1000,
 					   DUMMY_CLOCK_RATE_1),
 			0);
+
+	clk_put(clk);
 }
 
 /*
@@ -420,7 +434,7 @@ static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -433,6 +447,8 @@ static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -443,7 +459,7 @@ static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -460,6 +476,8 @@ static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -472,7 +490,7 @@ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kuni
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	long rounded;
 
 	KUNIT_ASSERT_EQ(test,
@@ -489,6 +507,8 @@ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kuni
 			0);
 
 	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
+
+	clk_put(clk);
 }
 
 /*
@@ -499,7 +519,7 @@ static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -512,6 +532,8 @@ static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -522,7 +544,7 @@ static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -539,6 +561,8 @@ static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -551,7 +575,7 @@ static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kun
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	long rounded;
 
 	KUNIT_ASSERT_EQ(test,
@@ -568,6 +592,8 @@ static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kun
 			0);
 
 	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
+
+	clk_put(clk);
 }
 
 /*
@@ -582,7 +608,7 @@ static void clk_range_test_set_range_get_rate_raised(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -598,6 +624,8 @@ static void clk_range_test_set_range_get_rate_raised(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(clk);
 }
 
 /*
@@ -612,7 +640,7 @@ static void clk_range_test_set_range_get_rate_lowered(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -628,6 +656,8 @@ static void clk_range_test_set_range_get_rate_lowered(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 static struct kunit_case clk_range_test_cases[] = {
@@ -664,7 +694,7 @@ static void clk_range_test_set_range_rate_maximized(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -700,6 +730,8 @@ static void clk_range_test_set_range_rate_maximized(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
 }
 
 /*
@@ -714,7 +746,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	struct clk *user1, *user2;
 	unsigned long rate;
 
@@ -758,6 +790,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 
 	clk_put(user2);
 	clk_put(user1);
+	clk_put(clk);
 }
 
 static struct kunit_case clk_range_maximize_test_cases[] = {
@@ -785,7 +818,7 @@ static void clk_range_test_set_range_rate_minimized(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	unsigned long rate;
 
 	KUNIT_ASSERT_EQ(test,
@@ -821,6 +854,8 @@ static void clk_range_test_set_range_rate_minimized(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(clk);
 }
 
 /*
@@ -835,7 +870,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = hw->clk;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
 	struct clk *user1, *user2;
 	unsigned long rate;
 
@@ -875,6 +910,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 
 	clk_put(user2);
 	clk_put(user1);
+	clk_put(clk);
 }
 
 static struct kunit_case clk_range_minimize_test_cases[] = {
Signature · 1 line
2.37.1
#2 lore raw

PATCH v8 2/25 clk: Drop the rate range on clk_put()

In reply to Maxime Ripard

When clk_put() is called we don't make another clk_set_rate() call to re-evaluate the rate boundaries. This is unlike clk_set_rate_range() that evaluates the rate again each time it is called.

However, clk_put() is essentially equivalent to clk_set_rate_range() since after clk_put() completes the consumer's boundaries shouldn't be enforced anymore.

Let's add a call to clk_set_rate_range() in clk_put() to make sure those rate boundaries are dropped and the clock provider drivers can react. In order to be as non-intrusive as possible, we'll just make that call if the clock had non-default boundaries.

Also add a few tests to make sure this case is covered.

Fixes c80ac50cbb37 ("clk: Always set the rate on clk_set_range_rate")
---
 drivers/clk/clk.c      |  45 +++++++++++------
 drivers/clk/clk_test.c | 110 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+), 14 deletions(-)
drivers/clk/clk.c +31 -14
index 7fc191c15507..a5e0ab8bd6be 100644
@@ -2325,19 +2325,15 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL_GPL(clk_set_rate_exclusive);
 
-/**
- * clk_set_rate_range - set a rate range for a clock source
- * @clk: clock source
- * @min: desired minimum clock rate in Hz, inclusive
- * @max: desired maximum clock rate in Hz, inclusive
- *
- * Returns success (0) or negative errno.
- */
-int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
+static int clk_set_rate_range_nolock(struct clk *clk,
+				     unsigned long min,
+				     unsigned long max)
 {
 	int ret = 0;
 	unsigned long old_min, old_max, rate;
 
+	lockdep_assert_held(&prepare_lock);
+
 	if (!clk)
 		return 0;
 
@@ -2350,8 +2346,6 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		return -EINVAL;
 	}
 
-	clk_prepare_lock();
-
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
@@ -2395,6 +2389,28 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	if (clk->exclusive_count)
 		clk_core_rate_protect(clk->core);
 
+	return ret;
+}
+
+/**
+ * clk_set_rate_range - set a rate range for a clock source
+ * @clk: clock source
+ * @min: desired minimum clock rate in Hz, inclusive
+ * @max: desired maximum clock rate in Hz, inclusive
+ *
+ * Return: 0 for success or negative errno on failure.
+ */
+int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
+{
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	clk_prepare_lock();
+
+	ret = clk_set_rate_range_nolock(clk, min, max);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -4348,9 +4364,10 @@ void __clk_put(struct clk *clk)
 	}
 
 	hlist_del(&clk->clks_node);
-	if (clk->min_rate > clk->core->req_rate ||
-	    clk->max_rate < clk->core->req_rate)
-		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+
+	/* If we had any boundaries on that clock, let's drop them. */
+	if (clk->min_rate > 0 || clk->max_rate < ULONG_MAX)
+		clk_set_rate_range_nolock(clk, 0, ULONG_MAX);
 
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
drivers/clk/clk_test.c +110 -0

128 lines not on this page · raw message

Signature · 1 line
2.37.1
#3 lore raw

PATCH v8 3/25 clk: Skip clamping when rounding if there's no boundaries

In reply to Maxime Ripard

Commit 948fb0969eae ("clk: Always clamp the rounded rate") recently started to clamp the request rate in the clk_rate_request passed as an argument of clk_core_determine_round_nolock() with the min_rate and max_rate fields of that same request.

While the clk_rate_requests created by the framework itself always have those fields set, some drivers will create it themselves and don't always fill min_rate and max_rate.

In such a case, we end up clamping the rate with a minimum and maximum of 0, thus always rounding the rate to 0.

Let's skip the clamping if both min_rate and max_rate are set to 0 and complain so that it gets fixed.

Fixes 948fb0969eae ("clk: Always clamp the rounded rate")
---
 drivers/clk/clk.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/clk/clk.c +13 -1
index a5e0ab8bd6be..9d63163244d4 100644
@@ -1341,7 +1341,19 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
 	if (!core)
 		return 0;
 
-	req->rate = clamp(req->rate, req->min_rate, req->max_rate);
+	/*
+	 * Some clock providers hand-craft their clk_rate_requests and
+	 * might not fill min_rate and max_rate.
+	 *
+	 * If it's the case, clamping the rate is equivalent to setting
+	 * the rate to 0 which is bad. Skip the clamping but complain so
+	 * that it gets fixed, hopefully.
+	 */
+	if (!req->min_rate && !req->max_rate)
+		pr_warn("%s: %s: clk_rate_request has initialized min or max rate.\n",
+			__func__, core->name);
+	else
+		req->rate = clamp(req->rate, req->min_rate, req->max_rate);
 
 	/*
 	 * At this point, core protection will be disabled
Signature · 1 line
2.37.1
#4 lore raw

PATCH v8 4/25 clk: Mention that .recalc_rate can return 0 on error

In reply to Maxime Ripard

Multiple platforms (amlogic, imx8) return 0 when the clock rate cannot be determined properly by the recalc_rate hook. Mention in the documentation that the framework is ok with that.

---
 include/linux/clk-provider.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
include/linux/clk-provider.h +3 -2
index 1615010aa0ec..9a14cfa0d201 100644
@@ -118,8 +118,9 @@ struct clk_duty {
  *
  * @recalc_rate	Recalculate the rate of this clock, by querying hardware. The
  *		parent rate is an input parameter.  It is up to the caller to
- *		ensure that the prepare_mutex is held across this call.
- *		Returns the calculated rate.  Optional, but recommended - if
+ *		ensure that the prepare_mutex is held across this call. If the
+ *		driver cannot figure out a rate for this clock, it must return
+ *		0. Returns the calculated rate. Optional, but recommended - if
  *		this op is not set then clock rate will be initialized to 0.
  *
  * @round_rate:	Given a target rate as input, returns the closest rate actually
Signature · 1 line
2.37.1
#5 lore raw

PATCH v8 5/25 clk: Clarify clk_get_rate() expectations

In reply to Maxime Ripard

As shown by a number of clock users already, clk_get_rate() can be called whether or not the clock is enabled.

Similarly, a number of clock drivers will return a rate of 0 whenever the rate cannot be figured out.

Since it was a bit ambiguous before, let's make it clear in the clk_get_rate() documentation.

---
 drivers/clk/clk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clk/clk.c +3 -2
index 9d63163244d4..caa2eb640441 100644
@@ -1672,8 +1672,9 @@ static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
  * @clk: the clk whose rate is being returned
  *
  * Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE flag
- * is set, which means a recalc_rate will be issued.
- * If clk is NULL then returns 0.
+ * is set, which means a recalc_rate will be issued. Can be called regardless of
+ * the clock enabledness. If clk is NULL, or if an error occurred, then returns
+ * 0.
  */
 unsigned long clk_get_rate(struct clk *clk)
 {
Signature · 1 line
2.37.1
#6 lore raw

PATCH v8 6/25 clk: tests: Add test suites description

In reply to Maxime Ripard

We start to have a few test suites, and we'll add more, so it will get pretty confusing to figure out what is supposed to be tested in what suite.

Let's add some comments to explain what setup they create, and what we should be testing in every suite.

---
 drivers/clk/clk_test.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
drivers/clk/clk_test.c +33 -0
index 7d9da88c39ee..1a7cb482ec58 100644
@@ -258,6 +258,11 @@ static struct kunit_case clk_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests exercise the rate API with simple scenarios
+ */
 static struct kunit_suite clk_test_suite = {
 	.name = "clk-test",
 	.init = clk_test_init,
@@ -346,6 +351,14 @@ static struct kunit_case clk_orphan_transparent_single_parent_mux_test_cases[] =
 	{}
 };
 
+/*
+ * Test suite for a basic mux clock with one parent. The parent is
+ * registered after its child. The clock will thus be an orphan when
+ * registered, but will no longer be when the tests run.
+ *
+ * These tests make sure a clock that used to be orphan has a sane,
+ * consistent, behaviour.
+ */
 static struct kunit_suite clk_orphan_transparent_single_parent_test_suite = {
 	.name = "clk-orphan-transparent-single-parent-test",
 	.init = clk_orphan_transparent_single_parent_mux_test_init,
@@ -675,6 +688,12 @@ static struct kunit_case clk_range_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests exercise the rate range API: clk_set_rate_range(),
+ * clk_set_min_rate(), clk_set_max_rate(), clk_drop_range().
+ */
 static struct kunit_suite clk_range_test_suite = {
 	.name = "clk-range-test",
 	.init = clk_test_init,
@@ -856,6 +875,13 @@ static struct kunit_case clk_range_maximize_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests exercise the rate range API: clk_set_rate_range(),
+ * clk_set_min_rate(), clk_set_max_rate(), clk_drop_range(), with a
+ * driver that will always try to run at the highest possible rate.
+ */
 static struct kunit_suite clk_range_maximize_test_suite = {
 	.name = "clk-range-maximize-test",
 	.init = clk_maximize_test_init,
@@ -1029,6 +1055,13 @@ static struct kunit_case clk_range_minimize_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests exercise the rate range API: clk_set_rate_range(),
+ * clk_set_min_rate(), clk_set_max_rate(), clk_drop_range(), with a
+ * driver that will always try to run at the lowest possible rate.
+ */
 static struct kunit_suite clk_range_minimize_test_suite = {
 	.name = "clk-range-minimize-test",
 	.init = clk_minimize_test_init,
Signature · 1 line
2.37.1
#7 lore raw

PATCH v8 7/25 clk: tests: Add reference to the orphan mux bug report

In reply to Maxime Ripard

Some more context might be useful for unit-tests covering a previously reported bug, so let's add a link to the discussion for that bug.

---
 drivers/clk/clk_test.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk_test.c +3 -0
index 1a7cb482ec58..b8e32406a6e4 100644
@@ -322,6 +322,9 @@ static void clk_orphan_transparent_single_parent_mux_test_exit(struct kunit *tes
 /*
  * Test that a mux-only clock, with an initial rate within a range,
  * will still have the same rate after the range has been enforced.
+ *
+ * See:
+ * https://lore.kernel.org/linux-clk/[email protected]/
  */
 static void clk_test_orphan_transparent_parent_mux_set_range(struct kunit *test)
 {
Signature · 1 line
2.37.1
#8 lore raw

PATCH v8 8/25 clk: tests: Add tests for uncached clock

In reply to Maxime Ripard

The clock framework supports clocks that can have their rate changed without the kernel knowing about it using the CLK_GET_RATE_NOCACHE flag.

As its name suggests, this flag turns off the rate caching in the clock framework, reading out the rate from the hardware any time we need to read it.

Let's add a couple of tests to make sure it works as intended.

---
 drivers/clk/clk_test.c | 93 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +92 -1
index b8e32406a6e4..b269420dafcc 100644
@@ -270,6 +270,96 @@ static struct kunit_suite clk_test_suite = {
 	.test_cases = clk_test_cases,
 };
 
+static int clk_uncached_test_init(struct kunit *test)
+{
+	struct clk_dummy_context *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->rate = DUMMY_CLOCK_INIT_RATE;
+	ctx->hw.init = CLK_HW_INIT_NO_PARENT("test-clk",
+					     &clk_dummy_rate_ops,
+					     CLK_GET_RATE_NOCACHE);
+
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Test that for an uncached clock, the clock framework doesn't cache
+ * the rate and clk_get_rate() will return the underlying clock rate
+ * even if it changed.
+ */
+static void clk_test_uncached_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
+
+	/* We change the rate behind the clock framework's back */
+	ctx->rate = DUMMY_CLOCK_RATE_1;
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that for an uncached clock, clk_set_rate_range() will work
+ * properly if the rate hasn't changed.
+ */
+static void clk_test_uncached_set_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
+}
+
+static struct kunit_case clk_uncached_test_cases[] = {
+	KUNIT_CASE(clk_test_uncached_get_rate),
+	KUNIT_CASE(clk_test_uncached_set_range),
+	{}
+};
+
+/*
+ * Test suite for a basic, uncached, rate clock, without any parent.
+ *
+ * These tests exercise the rate API with simple scenarios
+ */
+static struct kunit_suite clk_uncached_test_suite = {
+	.name = "clk-uncached-test",
+	.init = clk_uncached_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_uncached_test_cases,
+};
+
 struct clk_single_parent_ctx {
 	struct clk_dummy_context parent_ctx;
 	struct clk_hw hw;
@@ -1077,6 +1167,7 @@ kunit_test_suites(
 	&clk_orphan_transparent_single_parent_test_suite,
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
-	&clk_range_minimize_test_suite
+	&clk_range_minimize_test_suite,
+	&clk_uncached_test_suite
 );
 MODULE_LICENSE("GPL v2");
Signature · 1 line
2.37.1
#9 lore raw

PATCH v8 9/25 clk: tests: Add tests for single parent mux

In reply to Maxime Ripard

We have a few tests for a mux with a single parent, testing the case where it used to be orphan.

Let's leverage most of the code but register the clock properly to test a few trivial things.

---
 drivers/clk/clk_test.c | 194 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 185 insertions(+), 9 deletions(-)
drivers/clk/clk_test.c +185 -9
index b269420dafcc..06c9220873bb 100644
@@ -365,6 +365,189 @@ struct clk_single_parent_ctx {
 	struct clk_hw hw;
 };
 
+static int clk_single_parent_mux_test_init(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->parent_ctx.rate = DUMMY_CLOCK_INIT_RATE;
+	ctx->parent_ctx.hw.init =
+		CLK_HW_INIT_NO_PARENT("parent-clk",
+				      &clk_dummy_rate_ops,
+				      0);
+
+	ret = clk_hw_register(NULL, &ctx->parent_ctx.hw);
+	if (ret)
+		return ret;
+
+	ctx->hw.init = CLK_HW_INIT("test-clk", "parent-clk",
+				   &clk_dummy_single_parent_ops,
+				   CLK_SET_RATE_PARENT);
+
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void
+clk_single_parent_mux_test_exit(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	clk_hw_unregister(&ctx->parent_ctx.hw);
+}
+
+/*
+ * Test that for a clock with a single parent, clk_get_parent() actually
+ * returns the parent.
+ */
+static void
+clk_test_single_parent_mux_get_parent(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent = clk_hw_get_clk(&ctx->parent_ctx.hw, NULL);
+
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(clk), parent));
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+/*
+ * Test that for a clock that can't modify its rate and with a single
+ * parent, if we set disjoints range on the parent and then the child,
+ * the second will return an error.
+ *
+ * FIXME: clk_set_rate_range() only considers the current clock when
+ * evaluating whether ranges are disjoints and not the upstream clocks
+ * ranges.
+ */
+static void
+clk_test_single_parent_mux_set_range_disjoint_child_last(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	parent = clk_get_parent(clk);
+	KUNIT_ASSERT_PTR_NE(test, parent, NULL);
+
+	ret = clk_set_rate_range(parent, 1000, 2000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(clk, 3000, 4000);
+	KUNIT_EXPECT_LT(test, ret, 0);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that for a clock that can't modify its rate and with a single
+ * parent, if we set disjoints range on the child and then the parent,
+ * the second will return an error.
+ *
+ * FIXME: clk_set_rate_range() only considers the current clock when
+ * evaluating whether ranges are disjoints and not the downstream clocks
+ * ranges.
+ */
+static void
+clk_test_single_parent_mux_set_range_disjoint_parent_last(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	parent = clk_get_parent(clk);
+	KUNIT_ASSERT_PTR_NE(test, parent, NULL);
+
+	ret = clk_set_rate_range(clk, 1000, 2000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(parent, 3000, 4000);
+	KUNIT_EXPECT_LT(test, ret, 0);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that for a clock that can't modify its rate and with a single
+ * parent, if we set a range on the parent and a more restrictive one on
+ * the child, and then call clk_round_rate(), the boundaries of the
+ * two clocks are taken into account.
+ */
+static void
+clk_test_single_parent_mux_set_range_round_rate_child_smaller(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long rate;
+	int ret;
+
+	parent = clk_get_parent(clk);
+	KUNIT_ASSERT_PTR_NE(test, parent, NULL);
+
+	ret = clk_set_rate_range(parent, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1 + 1000, DUMMY_CLOCK_RATE_2 - 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	clk_put(clk);
+}
+
+static struct kunit_case clk_single_parent_mux_test_cases[] = {
+	KUNIT_CASE(clk_test_single_parent_mux_get_parent),
+	KUNIT_CASE(clk_test_single_parent_mux_set_range_disjoint_child_last),
+	KUNIT_CASE(clk_test_single_parent_mux_set_range_disjoint_parent_last),
+	KUNIT_CASE(clk_test_single_parent_mux_set_range_round_rate_child_smaller),
+	{}
+};
+
+/*
+ * Test suite for a basic mux clock with one parent, with
+ * CLK_SET_RATE_PARENT on the child.
+ *
+ * These tests exercise the consumer API and check that the state of the
+ * child and parent are sane and consistent.
+ */
+static struct kunit_suite
+clk_single_parent_mux_test_suite = {
+	.name = "clk-single-parent-mux-test",
+	.init = clk_single_parent_mux_test_init,
+	.exit = clk_single_parent_mux_test_exit,
+	.test_cases = clk_single_parent_mux_test_cases,
+};
+
 static int clk_orphan_transparent_single_parent_mux_test_init(struct kunit *test)
 {
 	struct clk_single_parent_ctx *ctx;
@@ -401,14 +584,6 @@ static int clk_orphan_transparent_single_parent_mux_test_init(struct kunit *test
 	return 0;
 }
 
-static void clk_orphan_transparent_single_parent_mux_test_exit(struct kunit *test)
-{
-	struct clk_single_parent_ctx *ctx = test->priv;
-
-	clk_hw_unregister(&ctx->hw);
-	clk_hw_unregister(&ctx->parent_ctx.hw);
-}
-
 /*
  * Test that a mux-only clock, with an initial rate within a range,
  * will still have the same rate after the range has been enforced.
@@ -455,7 +630,7 @@ static struct kunit_case clk_orphan_transparent_single_parent_mux_test_cases[] =
 static struct kunit_suite clk_orphan_transparent_single_parent_test_suite = {
 	.name = "clk-orphan-transparent-single-parent-test",
 	.init = clk_orphan_transparent_single_parent_mux_test_init,
-	.exit = clk_orphan_transparent_single_parent_mux_test_exit,
+	.exit = clk_single_parent_mux_test_exit,
 	.test_cases = clk_orphan_transparent_single_parent_mux_test_cases,
 };
 
@@ -1168,6 +1343,7 @@ kunit_test_suites(
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
 	&clk_range_minimize_test_suite,
+	&clk_single_parent_mux_test_suite,
 	&clk_uncached_test_suite
 );
 MODULE_LICENSE("GPL v2");
Signature · 1 line
2.37.1
#10 lore raw

PATCH v8 10/25 clk: tests: Add tests for mux with multiple parents

In reply to Maxime Ripard

We'll need to test a few corner cases that occur when we have a mux clock whose default parent is missing.

For now, let's create the context structure and the trivial ops, along with a test suite that just tests trivial things for now, without considering the orphan case.

---
 drivers/clk/clk_test.c | 121 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)
drivers/clk/clk_test.c +121 -0
index 06c9220873bb..1ccafd4fabff 100644
@@ -108,6 +108,39 @@ static const struct clk_ops clk_dummy_single_parent_ops = {
 	.get_parent = clk_dummy_single_get_parent,
 };
 
+struct clk_multiple_parent_ctx {
+	struct clk_dummy_context parents_ctx[2];
+	struct clk_hw hw;
+	u8 current_parent;
+};
+
+static int clk_multiple_parents_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_multiple_parent_ctx *ctx =
+		container_of(hw, struct clk_multiple_parent_ctx, hw);
+
+	if (index >= clk_hw_get_num_parents(hw))
+		return -EINVAL;
+
+	ctx->current_parent = index;
+
+	return 0;
+}
+
+static u8 clk_multiple_parents_mux_get_parent(struct clk_hw *hw)
+{
+	struct clk_multiple_parent_ctx *ctx =
+		container_of(hw, struct clk_multiple_parent_ctx, hw);
+
+	return ctx->current_parent;
+}
+
+static const struct clk_ops clk_multiple_parents_mux_ops = {
+	.get_parent = clk_multiple_parents_mux_get_parent,
+	.set_parent = clk_multiple_parents_mux_set_parent,
+	.determine_rate = __clk_mux_determine_rate_closest,
+};
+
 static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
 {
 	struct clk_dummy_context *ctx;
@@ -360,6 +393,93 @@ static struct kunit_suite clk_uncached_test_suite = {
 	.test_cases = clk_uncached_test_cases,
 };
 
+static int
+clk_multiple_parents_mux_test_init(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx;
+	const char *parents[2] = { "parent-0", "parent-1"};
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
+							    &clk_dummy_rate_ops,
+							    0);
+	ctx->parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
+	ret = clk_hw_register(NULL, &ctx->parents_ctx[0].hw);
+	if (ret)
+		return ret;
+
+	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
+							    &clk_dummy_rate_ops,
+							    0);
+	ctx->parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
+	ret = clk_hw_register(NULL, &ctx->parents_ctx[1].hw);
+	if (ret)
+		return ret;
+
+	ctx->current_parent = 0;
+	ctx->hw.init = CLK_HW_INIT_PARENTS("test-mux", parents,
+					   &clk_multiple_parents_mux_ops,
+					   CLK_SET_RATE_PARENT);
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void
+clk_multiple_parents_mux_test_exit(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	clk_hw_unregister(&ctx->parents_ctx[0].hw);
+	clk_hw_unregister(&ctx->parents_ctx[1].hw);
+}
+
+/*
+ * Test that for a clock with multiple parents, clk_get_parent()
+ * actually returns the current one.
+ */
+static void
+clk_test_multiple_parents_mux_get_parent(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent = clk_hw_get_clk(&ctx->parents_ctx[0].hw, NULL);
+
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(clk), parent));
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+static struct kunit_case clk_multiple_parents_mux_test_cases[] = {
+	KUNIT_CASE(clk_test_multiple_parents_mux_get_parent),
+	{}
+};
+
+/*
+ * Test suite for a basic mux clock with two parents, with
+ * CLK_SET_RATE_PARENT on the child.
+ *
+ * These tests exercise the consumer API and check that the state of the
+ * child and parents are sane and consistent.
+ */
+static struct kunit_suite
+clk_multiple_parents_mux_test_suite = {
+	.name = "clk-multiple-parents-mux-test",
+	.init = clk_multiple_parents_mux_test_init,
+	.exit = clk_multiple_parents_mux_test_exit,
+	.test_cases = clk_multiple_parents_mux_test_cases,
+};
+
 struct clk_single_parent_ctx {
 	struct clk_dummy_context parent_ctx;
 	struct clk_hw hw;
@@ -1339,6 +1459,7 @@ static struct kunit_suite clk_range_minimize_test_suite = {
 
 kunit_test_suites(
 	&clk_test_suite,
+	&clk_multiple_parents_mux_test_suite,
 	&clk_orphan_transparent_single_parent_test_suite,
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
Signature · 1 line
2.37.1
#11 lore raw

PATCH v8 11/25 clk: tests: Add some tests for orphan with multiple parents

In reply to Maxime Ripard

Let's leverage the dummy mux with multiple parents we have to create a mux whose default parent will never be registered, and thus will always be orphan by default.

We can then create some tests to make sure that the clock API behaves properly in such a case, and that the transition to a non-orphan clock when we change the parent is done properly.

---
 drivers/clk/clk_test.c | 237 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 237 insertions(+)
drivers/clk/clk_test.c +237 -0
index 1ccafd4fabff..ceed49c5a88b 100644
@@ -480,6 +480,242 @@ clk_multiple_parents_mux_test_suite = {
 	.test_cases = clk_multiple_parents_mux_test_cases,
 };
 
+static int
+clk_orphan_transparent_multiple_parent_mux_test_init(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx;
+	const char *parents[2] = { "missing-parent", "proper-parent"};
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("proper-parent",
+							    &clk_dummy_rate_ops,
+							    0);
+	ctx->parents_ctx[1].rate = DUMMY_CLOCK_INIT_RATE;
+	ret = clk_hw_register(NULL, &ctx->parents_ctx[1].hw);
+	if (ret)
+		return ret;
+
+	ctx->hw.init = CLK_HW_INIT_PARENTS("test-orphan-mux", parents,
+					   &clk_multiple_parents_mux_ops,
+					   CLK_SET_RATE_PARENT);
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void
+clk_orphan_transparent_multiple_parent_mux_test_exit(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	clk_hw_unregister(&ctx->parents_ctx[1].hw);
+}
+
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet and is
+ * thus orphan, clk_get_parent() will return NULL.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_get_parent(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+
+	KUNIT_EXPECT_PTR_EQ(test, clk_get_parent(clk), NULL);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet,
+ * calling clk_set_parent() to a valid parent will properly update the
+ * mux parent and its orphan status.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent, *new_parent;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_parent = clk_get_parent(clk);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+	KUNIT_EXPECT_TRUE(test, clk_is_match(parent, new_parent));
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a mux that started orphan but got switched to a valid
+ * parent, the rate of the mux and its new parent are consistent.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long parent_rate, rate;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, parent_rate, 0);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, parent_rate, rate);
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a mux that started orphan but got switched to a valid
+ * parent, calling clk_set_rate_range() will affect the parent state if
+ * its rate is out of range.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long rate;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet,
+ * calling clk_set_rate_range() will succeed, and will be taken into
+ * account when rounding a rate.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a mux that started orphan, was assigned and rate and
+ * then got switched to a valid parent, its rate is eventually within
+ * range.
+ *
+ * FIXME: Even though we update the rate as part of clk_set_parent(), we
+ * don't evaluate whether that new rate is within range and needs to be
+ * adjusted.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long rate;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	clk_hw_set_rate_range(hw, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
+static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = {
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_get_parent),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate),
+	{}
+};
+
+/*
+ * Test suite for a basic mux clock with two parents. The default parent
+ * isn't registered, only the second parent is. By default, the clock
+ * will thus be orphan.
+ *
+ * These tests exercise the behaviour of the consumer API when dealing
+ * with an orphan clock, and how we deal with the transition to a valid
+ * parent.
+ */
+static struct kunit_suite clk_orphan_transparent_multiple_parent_mux_test_suite = {
+	.name = "clk-orphan-transparent-multiple-parent-mux-test",
+	.init = clk_orphan_transparent_multiple_parent_mux_test_init,
+	.exit = clk_orphan_transparent_multiple_parent_mux_test_exit,
+	.test_cases = clk_orphan_transparent_multiple_parent_mux_test_cases,
+};
+
 struct clk_single_parent_ctx {
 	struct clk_dummy_context parent_ctx;
 	struct clk_hw hw;
@@ -1460,6 +1696,7 @@ static struct kunit_suite clk_range_minimize_test_suite = {
 kunit_test_suites(
 	&clk_test_suite,
 	&clk_multiple_parents_mux_test_suite,
+	&clk_orphan_transparent_multiple_parent_mux_test_suite,
 	&clk_orphan_transparent_single_parent_test_suite,
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
Signature · 1 line
2.37.1
#12 lore raw

PATCH v8 12/25 clk: Take into account uncached clocks in clk_set_rate_range()

In reply to Maxime Ripard

clk_set_rate_range() will use the last requested rate for the clock when it calls into the driver set_rate hook.

However, if CLK_GET_RATE_NOCACHE is set on that clock, the last requested rate might not be matching the current rate of the clock. In such a case, let's read out the rate from the hardware and use that in our set_rate instead.

---
 drivers/clk/clk.c      |  6 +++++-
 drivers/clk/clk_test.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
drivers/clk/clk.c +5 -1
index caa2eb640441..53b28e63deae 100644
@@ -2373,6 +2373,10 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 		goto out;
 	}
 
+	rate = clk->core->req_rate;
+	if (clk->core->flags & CLK_GET_RATE_NOCACHE)
+		rate = clk_core_get_rate_recalc(clk->core);
+
 	/*
 	 * Since the boundaries have been changed, let's give the
 	 * opportunity to the provider to adjust the clock rate based on
@@ -2390,7 +2394,7 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 	 * - the determine_rate() callback does not really check for
 	 *   this corner case when determining the rate
 	 */
-	rate = clamp(clk->core->req_rate, min, max);
+	rate = clamp(rate, min, max);
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 	if (ret) {
 		/* rollback the changes */
drivers/clk/clk_test.c +31 -0
index ceed49c5a88b..d3e121f21ae2 100644
@@ -375,9 +375,40 @@ static void clk_test_uncached_set_range(struct kunit *test)
 	clk_put(clk);
 }
 
+/*
+ * Test that for an uncached clock, clk_set_rate_range() will work
+ * properly if the rate has changed in hardware.
+ *
+ * In this case, it means that if the rate wasn't initially in the range
+ * we're trying to set, but got changed at some point into the range
+ * without the kernel knowing about it, its rate shouldn't be affected.
+ */
+static void clk_test_uncached_updated_rate_set_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+
+	/* We change the rate behind the clock framework's back */
+	ctx->rate = DUMMY_CLOCK_RATE_1 + 1000;
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	clk_put(clk);
+}
+
 static struct kunit_case clk_uncached_test_cases[] = {
 	KUNIT_CASE(clk_test_uncached_get_rate),
 	KUNIT_CASE(clk_test_uncached_set_range),
+	KUNIT_CASE(clk_test_uncached_updated_rate_set_range),
 	{}
 };
 
Signature · 1 line
2.37.1
#13 lore raw

PATCH v8 13/25 clk: Set req_rate on reparenting

In reply to Maxime Ripard

If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed.

However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. Let's make sure it's the case for the newly unorphan clock and all its children.

---
 drivers/clk/clk.c      |  18 ++++
 drivers/clk/clk_test.c | 239 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 257 insertions(+)
drivers/clk/clk.c +18 -0
index 53b28e63deae..6f5284f6cab5 100644
@@ -1765,6 +1765,23 @@ static void clk_core_update_orphan_status(struct clk_core *core, bool is_orphan)
 		clk_core_update_orphan_status(child, is_orphan);
 }
 
+/*
+ * Update the orphan rate and req_rate of @core and all its children.
+ */
+static void clk_core_update_orphan_child_rates(struct clk_core *core)
+{
+	struct clk_core *child;
+	unsigned long parent_rate = 0;
+
+	if (core->parent)
+		parent_rate = core->parent->rate;
+
+	core->rate = core->req_rate = clk_recalc(core, parent_rate);
+
+	hlist_for_each_entry(child, &core->children, child_node)
+		clk_core_update_orphan_child_rates(child);
+}
+
 static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
 {
 	bool was_orphan = core->orphan;
@@ -1789,6 +1806,7 @@ static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
 	}
 
 	core->parent = new_parent;
+	clk_core_update_orphan_child_rates(core);
 }
 
 static struct clk_core *__clk_set_parent_before(struct clk_core *core,
drivers/clk/clk_test.c +239 -0

277 lines not on this page · raw message

Signature · 1 line
2.37.1
#14 lore raw

Re: [PATCH v8 13/25] clk: Set req_rate on reparenting

Hello Maxime,

Am Montag, 15. August 2022, 17:41:35 CEST schrieb Maxime Ripard:

Quoted 9 lines

If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed.

However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. Let's make sure it's the case for the newly unorphan clock and all its children.

This works with my basic board DT, but adding an I2C attached audio codec (sound/soc/codecs/tlv320aic32x4-clk.c) I get the following error:

Quoted 39 lines
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:283 in_atomic(): 1, irqs_disabled(): 128, non_block:
0, pid: 217, name: kworker/u8:6 preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
CPU: 3 PID: 217 Comm: kworker/u8:6 Not tainted 6.0.0-rc1-next-20220816+ #521
ac6fe0b093ec56bf12af4f3eda948091742739aa Hardware name: TQ-Systems
i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT)
Workqueue: events_unbound deferred_probe_work_func

Call trace:
 dump_backtrace+0xd4/0x114
 show_stack+0x14/0x4c
 dump_stack_lvl+0x64/0x7c
 dump_stack+0x14/0x2c
 __might_resched+0x124/0x154
 __might_sleep+0x58/0xcc
 mutex_lock+0x20/0x70
 regmap_lock_mutex+0xc/0x1c
 regmap_read+0x38/0x70
 clk_aic32x4_div_recalc_rate+0x34/0x70 [snd_soc_tlv320aic32x4
 4f2256fee3bc49277632fba80c047a2b8a3ad122] clk_recalc+0x44/0xe0
 clk_core_update_orphan_child_rates+0x28/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_reparent+0xa4/0x14c
 __clk_set_parent_before+0x40/0xa0
 clk_core_set_parent_nolock+0x11c/0x27c
 clk_set_parent+0x3c/0x140
 __set_clk_parents+0x114/0x244
 of_clk_set_defaults+0x20/0x50
 platform_probe+0x38/0x100
 call_driver_probe+0x28/0x140
 really_probe+0xc0/0x334
 __driver_probe_device+0x84/0x144
 driver_probe_device+0x38/0x130
 __device_attach_driver+0xc8/0x17c
 bus_for_each_drv+0x74/0xc4
Quoted 8 lines
 __device_attach+0xa8/0x204
 device_initial_probe+0x10/0x1c
 bus_probe_device+0x90/0xa0
 deferred_probe_work_func+0x9c/0xf0
 process_one_work+0x1d0/0x330
 worker_thread+0x68/0x390
 kthread+0xf4/0xfc
 ret_from_fork+0x10/0x20

The audio codec driver provides clocks as well which can't be used in atomic contexts.

Best regards,
Alexander

Quoted 184 lines

g12b Signed-off-by: Maxime Ripard <[email protected]>

---
 drivers/clk/clk.c      |  18 ++++
 drivers/clk/clk_test.c | 239 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 257 insertions(+)
drivers/clk/clk.c +17 -0
index 53b28e63deae..6f5284f6cab5 100644
@@ -1765,6 +1765,23 @@ static void clk_core_update_orphan_status(struct
 clk_core *core, bool is_orphan) clk_core_update_orphan_status(child,
 is_orphan);
 }
 
+/*
+ * Update the orphan rate and req_rate of @core and all its children.
+ */
+static void clk_core_update_orphan_child_rates(struct clk_core *core)
+{
+	struct clk_core *child;
+	unsigned long parent_rate = 0;
+
+	if (core->parent)
+		parent_rate = core->parent->rate;
+
+	core->rate = core->req_rate = clk_recalc(core, parent_rate);
+
+	hlist_for_each_entry(child, &core->children, child_node)
+		clk_core_update_orphan_child_rates(child);
+}
+
 static void clk_reparent(struct clk_core *core, struct clk_core
 *new_parent) {
drivers/clk/clk.c +1 -0
 	bool was_orphan = core->orphan;
@@ -1789,6 +1806,7 @@ static void clk_reparent(struct clk_core *core, struct
 clk_core *new_parent) }
 
 	core->parent = new_parent;
+	clk_core_update_orphan_child_rates(core);
 }
 
 static struct clk_core *__clk_set_parent_before(struct clk_core *core,
drivers/clk/clk_test.c +0 -0
index d3e121f21ae2..d1b1372f7aaa 100644
@@ -594,6 +594,41 @@

clk_test_orphan_transparent_multiple_parent_mux_set_parent(struct kunit *test) clk_put(clk);
}

drivers/clk/clk_test.c +7 -0
+/*
+ * Test that, for a mux that started orphan but got switched to a valid
+ * parent, calling clk_drop_range() on the mux won't affect the parent
+ * rate.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent_drop_range(struc

t kunit *test) +{

drivers/clk/clk_test.c +27 -0
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long parent_rate, new_parent_rate;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, parent_rate, 0);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_drop_range(clk);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_parent_rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, new_parent_rate, 0);
+	KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
 /*
  * Test that, for a mux that started orphan but got switched to a valid
  * parent, the rate of the mux and its new parent are consistent.
@@ -625,6 +660,39 @@

clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate(struct kunit clk_put(clk);
}

drivers/clk/clk_test.c +6 -0
+/*
+ * Test that, for a mux that started orphan but got switched to a valid
+ * parent, calling clk_put() on the mux won't affect the parent rate.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent_put(struct kunit

*test) +{

drivers/clk/clk_test.c +26 -0
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk *clk, *parent;
+	unsigned long parent_rate, new_parent_rate;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	clk = clk_hw_get_clk(&ctx->hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+	parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, parent_rate, 0);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	clk_put(clk);
+
+	new_parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, new_parent_rate, 0);
+	KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
+
+	clk_put(parent);
+}
+
 /*
  * Test that, for a mux that started orphan but got switched to a valid
  * parent, calling clk_set_rate_range() will affect the parent state if
@@ -658,6 +726,43 @@

clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modifi ed(st clk_put(clk);
}

drivers/clk/clk_test.c +7 -0
+/*
+ * Test that, for a mux that started orphan but got switched to a valid
+ * parent, calling clk_set_rate_range() won't affect the parent state if
+ * its rate is within range.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouc

hed(struct kunit *test) +{

drivers/clk/clk_test.c +29 -0
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent;
+	unsigned long parent_rate, new_parent_rate;
+	int ret;
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, parent_rate, 0);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_INIT_RATE - 1000,
+				 DUMMY_CLOCK_INIT_RATE + 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_parent_rate = clk_get_rate(parent);
+	KUNIT_ASSERT_GT(test, new_parent_rate, 0);
+	KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
+
+	clk_put(parent);
+	clk_put(clk);
+}
+
 /*
  * Test that, for a mux whose current parent hasn't been registered yet,
  * calling clk_set_rate_range() will succeed, and will be taken into
@@ -724,8 +829,11 @@

clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_ra te(st static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = { KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_get_parent), KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent),

drivers/clk/clk_test.c

Quoted 1 line
drivers/clk/clk_test.c +1 -0
+	

KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_dro

drivers/clk/clk_test.c

Quoted 4 lines

p_range), KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_r ate),

drivers/clk/clk_test.c +1 -0
+	

KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_put

drivers/clk/clk_test.c

Quoted 4 lines

), KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_r ange_modified),

drivers/clk/clk_test.c +1 -0
+	

KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set

Quoted 151 lines

_range_untouched), KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_round_ rate), KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_set_pa rent_get_rate), {}

drivers/clk/clk_test.c +0 -0
@@ -1021,6 +1129,136 @@ static struct kunit_suite

clk_orphan_transparent_single_parent_test_suite = { .test_cases = clk_orphan_transparent_single_parent_mux_test_cases, };

drivers/clk/clk_test.c +81 -0
+struct clk_single_parent_two_lvl_ctx {
+	struct clk_dummy_context parent_parent_ctx;
+	struct clk_dummy_context parent_ctx;
+	struct clk_hw hw;
+};
+
+static int
+clk_orphan_two_level_root_last_test_init(struct kunit *test)
+{
+	struct clk_single_parent_two_lvl_ctx *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->parent_ctx.hw.init =
+		CLK_HW_INIT("intermediate-parent",
+			    "root-parent",
+			    &clk_dummy_single_parent_ops,
+			    CLK_SET_RATE_PARENT);
+	ret = clk_hw_register(NULL, &ctx->parent_ctx.hw);
+	if (ret)
+		return ret;
+
+	ctx->hw.init =
+		CLK_HW_INIT("test-clk", "intermediate-parent",
+			    &clk_dummy_single_parent_ops,
+			    CLK_SET_RATE_PARENT);
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	ctx->parent_parent_ctx.rate = DUMMY_CLOCK_INIT_RATE;
+	ctx->parent_parent_ctx.hw.init =
+		CLK_HW_INIT_NO_PARENT("root-parent",
+				      &clk_dummy_rate_ops,
+				      0);
+	ret = clk_hw_register(NULL, &ctx->parent_parent_ctx.hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void
+clk_orphan_two_level_root_last_test_exit(struct kunit *test)
+{
+	struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	clk_hw_unregister(&ctx->parent_ctx.hw);
+	clk_hw_unregister(&ctx->parent_parent_ctx.hw);
+}
+
+/*
+ * Test that, for a clock whose parent used to be orphan, clk_get_rate()
+ * will return the proper rate.
+ */
+static void
+clk_orphan_two_level_root_last_test_get_rate(struct kunit *test)
+{
+	struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
+
+	clk_put(clk);
+}
+
+/*
+ * Test that, for a clock whose parent used to be orphan,
+ * clk_set_rate_range() won't affect its rate if it is already within
+ * range.
+ *
+ * See (for Exynos 4210):
+ *

https://lore.kernel.org/linux-clk/366a0232-bb4a-c357-6aa8-636e398e05eb@sams
ung.com/ + */

drivers/clk/clk_test.c +49 -0
+static void
+clk_orphan_two_level_root_last_test_set_range(struct kunit *test)
+{
+	struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_INIT_RATE - 1000,
+				 DUMMY_CLOCK_INIT_RATE + 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
+
+	clk_put(clk);
+}
+
+static struct kunit_case
+clk_orphan_two_level_root_last_test_cases[] = {
+	KUNIT_CASE(clk_orphan_two_level_root_last_test_get_rate),
+	KUNIT_CASE(clk_orphan_two_level_root_last_test_set_range),
+	{}
+};
+
+/*
+ * Test suite for a basic, transparent, clock with a parent that is also
+ * such a clock. The parent's parent is registered last, while the
+ * parent and its child are registered in that order. The intermediate
+ * and leaf clocks will thus be orphan when registered, but the leaf
+ * clock itself will always have its parent and will never be
+ * reparented. Indeed, it's only orphan because its parent is.
+ *
+ * These tests exercise the behaviour of the consumer API when dealing
+ * with an orphan clock, and how we deal with the transition to a valid
+ * parent.
+ */
+static struct kunit_suite
+clk_orphan_two_level_root_last_test_suite = {
+	.name = "clk-orphan-two-level-root-last-test",
+	.init = clk_orphan_two_level_root_last_test_init,
+	.exit = clk_orphan_two_level_root_last_test_exit,
+	.test_cases = clk_orphan_two_level_root_last_test_cases,
+};
+
 /*
  * Test that clk_set_rate_range won't return an error for a valid range
  * and that it will make sure the rate of the clock is within the
@@ -1729,6 +1967,7 @@ kunit_test_suites(
 	&clk_multiple_parents_mux_test_suite,
 	&clk_orphan_transparent_multiple_parent_mux_test_suite,
 	&clk_orphan_transparent_single_parent_test_suite,
+	&clk_orphan_two_level_root_last_test_suite,
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
 	&clk_range_minimize_test_suite,
#15 lore raw

Re: [PATCH v8 13/25] clk: Set req_rate on reparenting

Hi Alexander,

Quoted 63 lines · Alexander Stein

Hello Maxime,

Am Montag, 15. August 2022, 17:41:35 CEST schrieb Maxime Ripard:

Quoted 9 lines

If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed.

However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. Let's make sure it's the case for the newly unorphan clock and all its children.

This works with my basic board DT, but adding an I2C attached audio codec (sound/soc/codecs/tlv320aic32x4-clk.c) I get the following error:

Quoted 39 lines
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:283 in_atomic(): 1, irqs_disabled(): 128, non_block:
0, pid: 217, name: kworker/u8:6 preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
CPU: 3 PID: 217 Comm: kworker/u8:6 Not tainted 6.0.0-rc1-next-20220816+ #521
ac6fe0b093ec56bf12af4f3eda948091742739aa Hardware name: TQ-Systems
i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT)
Workqueue: events_unbound deferred_probe_work_func

Call trace:
 dump_backtrace+0xd4/0x114
 show_stack+0x14/0x4c
 dump_stack_lvl+0x64/0x7c
 dump_stack+0x14/0x2c
 __might_resched+0x124/0x154
 __might_sleep+0x58/0xcc
 mutex_lock+0x20/0x70
 regmap_lock_mutex+0xc/0x1c
 regmap_read+0x38/0x70
 clk_aic32x4_div_recalc_rate+0x34/0x70 [snd_soc_tlv320aic32x4
 4f2256fee3bc49277632fba80c047a2b8a3ad122] clk_recalc+0x44/0xe0
 clk_core_update_orphan_child_rates+0x28/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_reparent+0xa4/0x14c
 __clk_set_parent_before+0x40/0xa0
 clk_core_set_parent_nolock+0x11c/0x27c
 clk_set_parent+0x3c/0x140
 __set_clk_parents+0x114/0x244
 of_clk_set_defaults+0x20/0x50
 platform_probe+0x38/0x100
 call_driver_probe+0x28/0x140
 really_probe+0xc0/0x334
 __driver_probe_device+0x84/0x144
 driver_probe_device+0x38/0x130
 __device_attach_driver+0xc8/0x17c
 bus_for_each_drv+0x74/0xc4
Quoted 8 lines
 __device_attach+0xa8/0x204
 device_initial_probe+0x10/0x1c
 bus_probe_device+0x90/0xa0
 deferred_probe_work_func+0x9c/0xf0
 process_one_work+0x1d0/0x330
 worker_thread+0x68/0x390
 kthread+0xf4/0xfc
 ret_from_fork+0x10/0x20
Quoted 2 lines

The audio codec driver provides clocks as well which can't be used in atomic contexts.

So, this is due to clk_reparent() being called with enable_lock taken in __clk_set_parent_before(), and enable_lock is a spinlock.

The other call sites of clk_reparent() are __clk_set_parent(), that takes that lock too, and clk_core_reparent() that doesn't.

__clk_set_parent() is used exclusively by clk_core_set_parent_nolock() and has the assumption that only the prepare_lock (mutex) is taken.

clk_core_reparent() is used exclusively by clk_hw_reparent(), which is then used by four drivers (clk-stm32mp1.c, stm32/clk-stm32-core.c, clk/tegra/clk-tegra124-emc.c and tegra/clk-tegra210-emc.c)

All but tegra124 use clk_hw_reparent() in their set_parent implementation. The set_parent hook is called in __clk_set_parent() and clk_change_rate(), both times without the enable_lock taken.

tegra210 has it in its set_rate implementation, called only by clk_change_rate(), without the enable_lock taken too.

So I think that if we move the call to clk_core_update_orphan_child_rates() to the clk_reparent() call sites, after the enable_lock has been released if it was taken, we should be safe.

Could you test the following patch?

drivers/clk/clk.c +5 -1
index 5bdfd645f1dc..453e2ff10961 100644
@@ -1941,7 +1941,6 @@ static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
 	}
 
 	core->parent = new_parent;
-	clk_core_update_orphan_child_rates(core);
 }
 
 static struct clk_core *__clk_set_parent_before(struct clk_core *core,
@@ -1987,6 +1986,8 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core,
 	clk_reparent(core, parent);
 	clk_enable_unlock(flags);
 
+	clk_core_update_orphan_child_rates(core);
+
 	return old_parent;
 }
 
@@ -2031,6 +2032,8 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
 		flags = clk_enable_lock();
 		clk_reparent(core, old_parent);
 		clk_enable_unlock(flags);
+
+		clk_core_update_orphan_child_rates(core);
 		__clk_set_parent_after(core, old_parent, parent);
 
 		return ret;
@@ -2654,6 +2657,7 @@ static void clk_core_reparent(struct clk_core *core,
 				  struct clk_core *new_parent)
 {
 	clk_reparent(core, new_parent);
+	clk_core_update_orphan_child_rates(core);
 	__clk_recalc_accuracies(core);
 	__clk_recalc_rates(core, POST_RATE_CHANGE);
 }

Thanks!
Maxime

#16 lore raw

Re: [PATCH v8 13/25] clk: Set req_rate on reparenting

Hello Maxime,

Am Dienstag, 16. August 2022, 11:24:16 CEST schrieb Maxime Ripard:

Quoted 131 lines

* PGP Signed by an unknown key

Hi Alexander,

Quoted 65 lines · Alexander Stein

Hello Maxime,

Am Montag, 15. August 2022, 17:41:35 CEST schrieb Maxime Ripard:

Quoted 9 lines

If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed.

However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. Let's make sure it's the case for the newly unorphan clock and all its children.

This works with my basic board DT, but adding an I2C attached audio codec

(sound/soc/codecs/tlv320aic32x4-clk.c) I get the following error:

Quoted 40 lines
BUG: sleeping function called from invalid context at

kernel/locking/mutex.c:283 in_atomic(): 1, irqs_disabled(): 128, non_block:
0, pid: 217, name: kworker/u8:6 preempt_count: 1, expected: 0 RCU nest depth: 0, expected: 0

CPU: 3 PID: 217 Comm: kworker/u8:6 Not tainted 6.0.0-rc1-next-20220816+
#521 ac6fe0b093ec56bf12af4f3eda948091742739aa Hardware name: TQ-Systems
i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT)
Workqueue: events_unbound deferred_probe_work_func

Call trace:
 dump_backtrace+0xd4/0x114
 show_stack+0x14/0x4c
 dump_stack_lvl+0x64/0x7c
 dump_stack+0x14/0x2c
 __might_resched+0x124/0x154
 __might_sleep+0x58/0xcc
 mutex_lock+0x20/0x70
 regmap_lock_mutex+0xc/0x1c
 regmap_read+0x38/0x70
 clk_aic32x4_div_recalc_rate+0x34/0x70 [snd_soc_tlv320aic32x4
 4f2256fee3bc49277632fba80c047a2b8a3ad122] clk_recalc+0x44/0xe0
 clk_core_update_orphan_child_rates+0x28/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_reparent+0xa4/0x14c
 __clk_set_parent_before+0x40/0xa0
 clk_core_set_parent_nolock+0x11c/0x27c
 clk_set_parent+0x3c/0x140
 __set_clk_parents+0x114/0x244
 of_clk_set_defaults+0x20/0x50
 platform_probe+0x38/0x100
 call_driver_probe+0x28/0x140
 really_probe+0xc0/0x334
 __driver_probe_device+0x84/0x144
 driver_probe_device+0x38/0x130
 __device_attach_driver+0xc8/0x17c
 bus_for_each_drv+0x74/0xc4
Quoted 8 lines
 __device_attach+0xa8/0x204
 device_initial_probe+0x10/0x1c
 bus_probe_device+0x90/0xa0
 deferred_probe_work_func+0x9c/0xf0
 process_one_work+0x1d0/0x330
 worker_thread+0x68/0x390
 kthread+0xf4/0xfc
 ret_from_fork+0x10/0x20
Quoted 2 lines

The audio codec driver provides clocks as well which can't be used in atomic contexts.

So, this is due to clk_reparent() being called with enable_lock taken in __clk_set_parent_before(), and enable_lock is a spinlock.

The other call sites of clk_reparent() are __clk_set_parent(), that takes that lock too, and clk_core_reparent() that doesn't.

__clk_set_parent() is used exclusively by clk_core_set_parent_nolock() and has the assumption that only the prepare_lock (mutex) is taken.

clk_core_reparent() is used exclusively by clk_hw_reparent(), which is then used by four drivers (clk-stm32mp1.c, stm32/clk-stm32-core.c, clk/tegra/clk-tegra124-emc.c and tegra/clk-tegra210-emc.c)

All but tegra124 use clk_hw_reparent() in their set_parent implementation. The set_parent hook is called in __clk_set_parent() and clk_change_rate(), both times without the enable_lock taken.

tegra210 has it in its set_rate implementation, called only by clk_change_rate(), without the enable_lock taken too.

So I think that if we move the call to clk_core_update_orphan_child_rates() to the clk_reparent() call sites, after the enable_lock has been released if it was taken, we should be safe.

Could you test the following patch?

drivers/clk/clk.c +4 -1
index 5bdfd645f1dc..453e2ff10961 100644
@@ -1941,7 +1941,6 @@ static void clk_reparent(struct clk_core *core, struct
 clk_core *new_parent) }
 
 	core->parent = new_parent;
-	clk_core_update_orphan_child_rates(core);
 }
 
 static struct clk_core *__clk_set_parent_before(struct clk_core *core,
@@ -1987,6 +1986,8 @@ static struct clk_core *__clk_set_parent_before(struct
 clk_core *core, clk_reparent(core, parent);
 	clk_enable_unlock(flags);
 
+	clk_core_update_orphan_child_rates(core);
+
 	return old_parent;
 }
 
@@ -2031,6 +2032,8 @@ static int __clk_set_parent(struct clk_core *core,
 struct clk_core *parent, flags = clk_enable_lock();
 		clk_reparent(core, old_parent);
 		clk_enable_unlock(flags);
+
+		clk_core_update_orphan_child_rates(core);
 		__clk_set_parent_after(core, old_parent, parent);
 
 		return ret;

drivers/clk/clk.c static void clk_core_reparent(struct clk_core *core, · line 2663

Quoted 7 lines
drivers/clk/clk.c +1 -0
@@ -2654,6 +2657,7 @@ static void clk_core_reparent(struct clk_core *core,
 				  struct clk_core *new_parent)
 {
 	clk_reparent(core, new_parent);
+	clk_core_update_orphan_child_rates(core);
 	__clk_recalc_accuracies(core);
 	__clk_recalc_rates(core, POST_RATE_CHANGE);
 }

With this patch applied the BUG message is gone and the system boots without any issue.

Thanks
Alexander

#17 lore raw

Re: [PATCH v8 13/25] clk: Set req_rate on reparenting

Quoted 132 lines · Alexander Stein

Hello Maxime,

Am Dienstag, 16. August 2022, 11:24:16 CEST schrieb Maxime Ripard:

Quoted 129 lines

* PGP Signed by an unknown key

Hi Alexander,

Quoted 65 lines · Alexander Stein

Hello Maxime,

Am Montag, 15. August 2022, 17:41:35 CEST schrieb Maxime Ripard:

Quoted 9 lines

If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed.

However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. Let's make sure it's the case for the newly unorphan clock and all its children.

This works with my basic board DT, but adding an I2C attached audio codec

(sound/soc/codecs/tlv320aic32x4-clk.c) I get the following error:

Quoted 40 lines
BUG: sleeping function called from invalid context at

kernel/locking/mutex.c:283 in_atomic(): 1, irqs_disabled(): 128, non_block:
0, pid: 217, name: kworker/u8:6 preempt_count: 1, expected: 0 RCU nest depth: 0, expected: 0

CPU: 3 PID: 217 Comm: kworker/u8:6 Not tainted 6.0.0-rc1-next-20220816+
#521 ac6fe0b093ec56bf12af4f3eda948091742739aa Hardware name: TQ-Systems
i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT)
Workqueue: events_unbound deferred_probe_work_func

Call trace:
 dump_backtrace+0xd4/0x114
 show_stack+0x14/0x4c
 dump_stack_lvl+0x64/0x7c
 dump_stack+0x14/0x2c
 __might_resched+0x124/0x154
 __might_sleep+0x58/0xcc
 mutex_lock+0x20/0x70
 regmap_lock_mutex+0xc/0x1c
 regmap_read+0x38/0x70
 clk_aic32x4_div_recalc_rate+0x34/0x70 [snd_soc_tlv320aic32x4
 4f2256fee3bc49277632fba80c047a2b8a3ad122] clk_recalc+0x44/0xe0
 clk_core_update_orphan_child_rates+0x28/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_core_update_orphan_child_rates+0x48/0x60
 clk_reparent+0xa4/0x14c
 __clk_set_parent_before+0x40/0xa0
 clk_core_set_parent_nolock+0x11c/0x27c
 clk_set_parent+0x3c/0x140
 __set_clk_parents+0x114/0x244
 of_clk_set_defaults+0x20/0x50
 platform_probe+0x38/0x100
 call_driver_probe+0x28/0x140
 really_probe+0xc0/0x334
 __driver_probe_device+0x84/0x144
 driver_probe_device+0x38/0x130
 __device_attach_driver+0xc8/0x17c
 bus_for_each_drv+0x74/0xc4
Quoted 8 lines
 __device_attach+0xa8/0x204
 device_initial_probe+0x10/0x1c
 bus_probe_device+0x90/0xa0
 deferred_probe_work_func+0x9c/0xf0
 process_one_work+0x1d0/0x330
 worker_thread+0x68/0x390
 kthread+0xf4/0xfc
 ret_from_fork+0x10/0x20
Quoted 2 lines

The audio codec driver provides clocks as well which can't be used in atomic contexts.

So, this is due to clk_reparent() being called with enable_lock taken in __clk_set_parent_before(), and enable_lock is a spinlock.

The other call sites of clk_reparent() are __clk_set_parent(), that takes that lock too, and clk_core_reparent() that doesn't.

__clk_set_parent() is used exclusively by clk_core_set_parent_nolock() and has the assumption that only the prepare_lock (mutex) is taken.

clk_core_reparent() is used exclusively by clk_hw_reparent(), which is then used by four drivers (clk-stm32mp1.c, stm32/clk-stm32-core.c, clk/tegra/clk-tegra124-emc.c and tegra/clk-tegra210-emc.c)

All but tegra124 use clk_hw_reparent() in their set_parent implementation. The set_parent hook is called in __clk_set_parent() and clk_change_rate(), both times without the enable_lock taken.

tegra210 has it in its set_rate implementation, called only by clk_change_rate(), without the enable_lock taken too.

So I think that if we move the call to clk_core_update_orphan_child_rates() to the clk_reparent() call sites, after the enable_lock has been released if it was taken, we should be safe.

Could you test the following patch?

drivers/clk/clk.c +4 -1
index 5bdfd645f1dc..453e2ff10961 100644
@@ -1941,7 +1941,6 @@ static void clk_reparent(struct clk_core *core, struct
 clk_core *new_parent) }
 
 	core->parent = new_parent;
-	clk_core_update_orphan_child_rates(core);
 }
 
 static struct clk_core *__clk_set_parent_before(struct clk_core *core,
@@ -1987,6 +1986,8 @@ static struct clk_core *__clk_set_parent_before(struct
 clk_core *core, clk_reparent(core, parent);
 	clk_enable_unlock(flags);
 
+	clk_core_update_orphan_child_rates(core);
+
 	return old_parent;
 }
 
@@ -2031,6 +2032,8 @@ static int __clk_set_parent(struct clk_core *core,
 struct clk_core *parent, flags = clk_enable_lock();
 		clk_reparent(core, old_parent);
 		clk_enable_unlock(flags);
+
+		clk_core_update_orphan_child_rates(core);
 		__clk_set_parent_after(core, old_parent, parent);
 
Quoted 12 lines

drivers/clk/clk.c static void clk_core_reparent(struct clk_core *core, · line 2663

Quoted 9 lines
drivers/clk/clk.c +1 -0
 		return ret;
@@ -2654,6 +2657,7 @@ static void clk_core_reparent(struct clk_core *core,
 				  struct clk_core *new_parent)
 {
 	clk_reparent(core, new_parent);
+	clk_core_update_orphan_child_rates(core);
 	__clk_recalc_accuracies(core);
 	__clk_recalc_rates(core, POST_RATE_CHANGE);
 }

With this patch applied the BUG message is gone and the system boots without any issue.

Thanks for testing :)

I've merged that patch into the original one, updated the commit log and will submit a new version of this series.

Maxime

#18 lore raw

PATCH v8 14/25 clk: Change clk_core_init_rate_req prototype

In reply to Maxime Ripard

The expectation is that a clk_rate_request structure is supposed to be initialized using clk_core_init_rate_req(), yet the rate we want to request still needs to be set by hand.

Let's just pass the rate as a function argument so that callers don't have any extra work to do.

---
 drivers/clk/clk.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/clk/clk.c +6 -4
index 6f5284f6cab5..6670e61edb31 100644
@@ -1380,13 +1380,16 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
 }
 
 static void clk_core_init_rate_req(struct clk_core * const core,
-				   struct clk_rate_request *req)
+				   struct clk_rate_request *req,
+				   unsigned long rate)
 {
 	struct clk_core *parent;
 
 	if (WARN_ON(!core || !req))
 		return;
 
+	req->rate = rate;
+
 	parent = core->parent;
 	if (parent) {
 		req->best_parent_hw = parent->hw;
@@ -1412,7 +1415,7 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 		return 0;
 	}
 
-	clk_core_init_rate_req(core, req);
+	clk_core_init_rate_req(core, req, req->rate);
 
 	if (clk_core_can_round(core))
 		return clk_core_determine_round_nolock(core, req);
@@ -2001,11 +2004,10 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	if (clk_core_can_round(core)) {
 		struct clk_rate_request req;
 
-		req.rate = rate;
 		req.min_rate = min_rate;
 		req.max_rate = max_rate;
 
-		clk_core_init_rate_req(core, &req);
+		clk_core_init_rate_req(core, &req, rate);
 
 		ret = clk_core_determine_round_nolock(core, &req);
 		if (ret < 0)
Signature · 1 line
2.37.1
#19 lore raw

PATCH v8 15/25 clk: Move clk_core_init_rate_req() from clk_core_round_rate_nolock() to its caller

In reply to Maxime Ripard

The clk_rate_request structure is used internally as an argument for the clk_core_determine_round_nolock() and clk_core_round_rate_nolock().

In both cases, the clk_core_init_rate_req() function is used to initialize the clk_rate_request structure.

However, the expectation on who gets to call that function is inconsistent between those two functions. Indeed, clk_core_determine_round_nolock() will assume the structure is properly initialized and will just use it.

On the other hand, clk_core_round_rate_nolock() will call clk_core_init_rate_req() itself, expecting the caller to have filled only a minimal set of parameters (rate, min_rate and max_rate).

If we ignore the calling convention inconsistency, this leads to a second inconsistency for drivers:

* If they get called by the framework through clk_core_round_rate_nolock(), the rate, min_rate and max_rate fields will be filled by the caller, and the best_parent_rate and best_parent_hw fields will get filled by clk_core_init_rate_req().

* If they get called by a driver through __clk_determine_rate (and thus clk_core_round_rate_nolock), only best_parent_rate and best_parent_hw are being explicitly set by the framework. Even though we can reasonably expect rate to be set, only one of the 6 in-tree users explicitly set min_rate and max_rate.

* If they get called by the framework through clk_core_determine_round_nolock(), then we have two callpaths. Either it will be called by clk_core_round_rate_nolock() itself, or it will be called by clk_calc_new_rates(), which will properly initialize rate, min_rate, max_rate itself, and best_parent_rate and best_parent_hw through clk_core_init_rate_req().

Even though the first and third case seems equivalent, they aren't when the clock has CLK_SET_RATE_PARENT. Indeed, in such a case clk_core_round_rate_nolock() will call itself on the current parent clock with the same clk_rate_request structure.

The clk_core_init_rate_req() function will then be called on the parent clock, with the child clk_rate_request pointer and will fill the best_parent_rate and best_parent_hw fields with the parent context.

When the whole recursion stops and the call returns, the initial caller will end up with a clk_rate_request structure with some informations of the child clock (rate, min_rate, max_rate) and some others of the last clock up the tree whose child had CLK_SET_RATE_PARENT (best_parent_hw, best_parent_rate).

In the most common case, best_parent_rate is going to be equal on all the parent clocks so it's not a big deal. However, best_parent_hw is going to point to a clock that never has been a valid parent for that clock which is definitely confusing.

In order to fix the calling inconsistency, let's move the clk_core_init_rate_req() calls to the callers, which will also help a bit with the clk_core_round_rate_nolock() recursion.

---
 drivers/clk/clk.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/clk/clk.c +3 -5
index 6670e61edb31..7a071c567800 100644
@@ -1415,8 +1415,6 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 		return 0;
 	}
 
-	clk_core_init_rate_req(core, req, req->rate);
-
 	if (clk_core_can_round(core))
 		return clk_core_determine_round_nolock(core, req);
 	else if (core->flags & CLK_SET_RATE_PARENT)
@@ -1464,8 +1462,8 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 	int ret;
 	struct clk_rate_request req;
 
+	clk_core_init_rate_req(hw->core, &req, rate);
 	clk_core_get_boundaries(hw->core, &req.min_rate, &req.max_rate);
-	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(hw->core, &req);
 	if (ret)
@@ -1497,8 +1495,8 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
+	clk_core_init_rate_req(clk->core, &req, rate);
 	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
-	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
 
@@ -2206,8 +2204,8 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 	if (cnt < 0)
 		return cnt;
 
+	clk_core_init_rate_req(core, &req, req_rate);
 	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
-	req.rate = req_rate;
 
 	ret = clk_core_round_rate_nolock(core, &req);
 
Signature · 1 line
2.37.1
#20 lore raw

PATCH v8 16/25 clk: Introduce clk_hw_init_rate_request()

In reply to Maxime Ripard

clk-divider instantiates clk_rate_request internally for its round_rate implementations to share the code with its determine_rate implementations.

However, it's missing a few fields (min_rate, max_rate) that would be initialized properly if it was using clk_core_init_rate_req().

Let's create the clk_hw_init_rate_request() function for clock providers to be able to share the code to instation clk_rate_requests with the framework. This will also be useful for some tests introduced in later patches.

---
 drivers/clk/clk-divider.c    | 20 ++++++++++----------
 drivers/clk/clk.c            | 20 ++++++++++++++++++++
 include/linux/clk-provider.h |  6 ++++++
 3 files changed, 36 insertions(+), 10 deletions(-)
drivers/clk/clk-divider.c +10 -10
index f6b2bf558486..a2c2b5203b0a 100644
@@ -386,13 +386,13 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 			       const struct clk_div_table *table,
 			       u8 width, unsigned long flags)
 {
-	struct clk_rate_request req = {
-		.rate = rate,
-		.best_parent_rate = *prate,
-		.best_parent_hw = parent,
-	};
+	struct clk_rate_request req;
 	int ret;
 
+	clk_hw_init_rate_request(hw, &req, rate);
+	req.best_parent_rate = *prate;
+	req.best_parent_hw = parent;
+
 	ret = divider_determine_rate(hw, &req, table, width, flags);
 	if (ret)
 		return ret;
@@ -408,13 +408,13 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 				  const struct clk_div_table *table, u8 width,
 				  unsigned long flags, unsigned int val)
 {
-	struct clk_rate_request req = {
-		.rate = rate,
-		.best_parent_rate = *prate,
-		.best_parent_hw = parent,
-	};
+	struct clk_rate_request req;
 	int ret;
 
+	clk_hw_init_rate_request(hw, &req, rate);
+	req.best_parent_rate = *prate;
+	req.best_parent_hw = parent;
+
 	ret = divider_ro_determine_rate(hw, &req, table, width, flags, val);
 	if (ret)
 		return ret;
drivers/clk/clk.c +20 -0
index 7a071c567800..195f051ee536 100644
@@ -1400,6 +1400,26 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 	}
 }
 
+/**
+ * clk_hw_init_rate_request - Initializes a clk_rate_request
+ * @hw: the clk for which we want to submit a rate request
+ * @req: the clk_rate_request structure we want to initialise
+ * @rate: the rate which is to be requested
+ *
+ * Initializes a clk_rate_request structure to submit to
+ * __clk_determine_rate() or similar functions.
+ */
+void clk_hw_init_rate_request(const struct clk_hw *hw,
+			      struct clk_rate_request *req,
+			      unsigned long rate)
+{
+	if (WARN_ON(!hw || !req))
+		return;
+
+	clk_core_init_rate_req(hw->core, req, rate);
+}
+EXPORT_SYMBOL_GPL(clk_hw_init_rate_request);
+
 static bool clk_core_can_round(struct clk_core * const core)
 {
 	return core->ops->determine_rate || core->ops->round_rate;
include/linux/clk-provider.h +6 -0
index 9a14cfa0d201..d857717aa386 100644
@@ -42,6 +42,8 @@ struct dentry;
  * struct clk_rate_request - Structure encoding the clk constraints that
  * a clock user might require.
  *
+ * Should be initialized by calling clk_hw_init_rate_request().
+ *
  * @rate:		Requested clock rate. This field will be adjusted by
  *			clock drivers according to hardware capabilities.
  * @min_rate:		Minimum rate imposed by clk users.
@@ -60,6 +62,10 @@ struct clk_rate_request {
 	struct clk_hw *best_parent_hw;
 };
 
+void clk_hw_init_rate_request(const struct clk_hw *hw,
+			      struct clk_rate_request *req,
+			      unsigned long rate);
+
 /**
  * struct clk_duty - Struture encoding the duty cycle ratio of a clock
  *
Signature · 1 line
2.37.1
#21 lore raw

PATCH v8 17/25 clk: Add our request boundaries in clk_core_init_rate_req

In reply to Maxime Ripard

The expectation is that a new clk_rate_request is initialized through a call to clk_core_init_rate_req().

However, at the moment it only fills the parent rate and clk_hw pointer, but omits the other fields such as the clock rate boundaries.

Some users of that function will update them after calling it, but most don't.

As we are passed the clk_core pointer, we have access to those boundaries in clk_core_init_rate_req() however, so let's just fill it there and remove it from the few callers that do it right.

---
 drivers/clk/clk.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/clk/clk.c +1 -6
index 195f051ee536..cd5b298f445e 100644
@@ -1389,6 +1389,7 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 		return;
 
 	req->rate = rate;
+	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 
 	parent = core->parent;
 	if (parent) {
@@ -1483,7 +1484,6 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 	struct clk_rate_request req;
 
 	clk_core_init_rate_req(hw->core, &req, rate);
-	clk_core_get_boundaries(hw->core, &req.min_rate, &req.max_rate);
 
 	ret = clk_core_round_rate_nolock(hw->core, &req);
 	if (ret)
@@ -1516,7 +1516,6 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 		clk_core_rate_unprotect(clk->core);
 
 	clk_core_init_rate_req(clk->core, &req, rate);
-	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
 
@@ -2022,9 +2021,6 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	if (clk_core_can_round(core)) {
 		struct clk_rate_request req;
 
-		req.min_rate = min_rate;
-		req.max_rate = max_rate;
-
 		clk_core_init_rate_req(core, &req, rate);
 
 		ret = clk_core_determine_round_nolock(core, &req);
@@ -2225,7 +2221,6 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 		return cnt;
 
 	clk_core_init_rate_req(core, &req, req_rate);
-	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
 
 	ret = clk_core_round_rate_nolock(core, &req);
 
Signature · 1 line
2.37.1
#22 lore raw

PATCH v8 18/25 clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock

In reply to Maxime Ripard

clk_mux_determine_rate_flags() will call into __clk_determine_rate() with a clk_hw pointer, while it has access to the clk_core pointer already.

This leads to back and forth between clk_hw and clk_core, while __clk_determine_rate will only call clk_core_round_rate_nolock() with the clk_core pointer it retrieved from the clk_hw.

Let's simplify things a bit by calling into clk_core_round_rate_nolock directly.

---
 drivers/clk/clk.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/clk/clk.c +10 -3
index cd5b298f445e..ad851afcfdd2 100644
@@ -536,6 +536,9 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 	return now <= rate && now > best;
 }
 
+static int clk_core_round_rate_nolock(struct clk_core *core,
+				      struct clk_rate_request *req);
+
 int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags)
@@ -549,8 +552,12 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	if (core->flags & CLK_SET_RATE_NO_REPARENT) {
 		parent = core->parent;
 		if (core->flags & CLK_SET_RATE_PARENT) {
-			ret = __clk_determine_rate(parent ? parent->hw : NULL,
-						   &parent_req);
+			if (!parent) {
+				req->rate = 0;
+				return 0;
+			}
+
+			ret = clk_core_round_rate_nolock(parent, &parent_req);
 			if (ret)
 				return ret;
 
@@ -573,7 +580,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 
 		if (core->flags & CLK_SET_RATE_PARENT) {
 			parent_req = *req;
-			ret = __clk_determine_rate(parent->hw, &parent_req);
+			ret = clk_core_round_rate_nolock(parent, &parent_req);
 			if (ret)
 				continue;
 		} else {
Signature · 1 line
2.37.1
#23 lore raw

PATCH v8 19/25 clk: Introduce clk_core_has_parent()

In reply to Maxime Ripard

We will need to know if a clk_core pointer has a given parent in other functions, so let's create a clk_core_has_parent() function that clk_has_parent() will call into.

For good measure, let's add some unit tests as well to make sure it works properly.

---
 drivers/clk/clk.c      | 36 ++++++++++++++++++++---------------
 drivers/clk/clk_test.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 15 deletions(-)
drivers/clk/clk.c +21 -15
index ad851afcfdd2..7fc6347e66c6 100644
@@ -539,6 +539,26 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 static int clk_core_round_rate_nolock(struct clk_core *core,
 				      struct clk_rate_request *req);
 
+static bool clk_core_has_parent(struct clk_core *core, const struct clk_core *parent)
+{
+	unsigned int i;
+
+	/* Optimize for the case where the parent is already the parent. */
+	if (core == parent)
+		return true;
+
+	for (i = 0; i < core->num_parents; i++) {
+		struct clk_core *tmp = clk_core_get_parent_by_index(core, i);
+		if (!tmp)
+			continue;
+
+		if (tmp == parent)
+			return true;
+	}
+
+	return false;
+}
+
 int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags)
@@ -2570,25 +2590,11 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
  */
 bool clk_has_parent(struct clk *clk, struct clk *parent)
 {
-	struct clk_core *core, *parent_core;
-	int i;
-
 	/* NULL clocks should be nops, so return success if either is NULL. */
 	if (!clk || !parent)
 		return true;
 
-	core = clk->core;
-	parent_core = parent->core;
-
-	/* Optimize for the case where the parent is already the parent. */
-	if (core->parent == parent_core)
-		return true;
-
-	for (i = 0; i < core->num_parents; i++)
-		if (!strcmp(core->parents[i].name, parent_core->name))
-			return true;
-
-	return false;
+	return clk_core_has_parent(clk->core, parent->core);
 }
 EXPORT_SYMBOL_GPL(clk_has_parent);
 
drivers/clk/clk_test.c +43 -0

63 lines not on this page · raw message

Signature · 1 line
2.37.1
#24 lore raw

PATCH v8 20/25 clk: Constify clk_has_parent()

In reply to Maxime Ripard

clk_has_parent() doesn't modify the clocks being passed, so let's make it const.

---
 drivers/clk/clk.c   | 2 +-
 include/linux/clk.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/clk/clk.c +1 -1
index 7fc6347e66c6..6c0fdf04a229 100644
@@ -2588,7 +2588,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
  *
  * Returns true if @parent is a possible parent for @clk, false otherwise.
  */
-bool clk_has_parent(struct clk *clk, struct clk *parent)
+bool clk_has_parent(const struct clk *clk, const struct clk *parent)
 {
 	/* NULL clocks should be nops, so return success if either is NULL. */
 	if (!clk || !parent)
include/linux/clk.h +1 -1
index c13061cabdfc..1ef013324237 100644
@@ -799,7 +799,7 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
  *
  * Returns true if @parent is a possible parent for @clk, false otherwise.
  */
-bool clk_has_parent(struct clk *clk, struct clk *parent);
+bool clk_has_parent(const struct clk *clk, const struct clk *parent);
 
 /**
  * clk_set_rate_range - set a rate range for a clock source
Signature · 1 line
2.37.1
#25 lore raw

PATCH v8 21/25 clk: Stop forwarding clk_rate_requests to the parent

In reply to Maxime Ripard

If the clock cannot modify its rate and has CLK_SET_RATE_PARENT, clk_mux_determine_rate_flags(), clk_core_round_rate_nolock() and a number of drivers will forward the clk_rate_request to the parent clock.

clk_core_round_rate_nolock() will pass the pointer directly, which means that we pass a clk_rate_request to the parent that has the rate, min_rate and max_rate of the child, and the best_parent_rate and best_parent_hw fields will be relative to the child as well, so will point to our current clock and its rate. The most common case for CLK_SET_RATE_PARENT is that the child and parent clock rates will be equal, so the rate field isn't a worry, but the other fields are.

Similarly, if the parent clock driver ever modifies the best_parent_rate or best_parent_hw, this will be applied to the child once the call to clk_core_round_rate_nolock() is done. best_parent_hw is probably not going to be a valid parent, and best_parent_rate might lead to a parent rate change different to the one that was initially computed.

clk_mux_determine_rate_flags() and the affected drivers will copy the request before forwarding it to the parents, so they won't be affected by the latter issue, but the former is still going to be there and will lead to erroneous data and context being passed to the various clock drivers in the same sub-tree.

Let's create two new functions, clk_core_forward_rate_req() and clk_hw_forward_rate_request() for the framework and the clock providers that will copy a request from a child clock and update the context to match the parent's. We also update the relevant call sites in the framework and drivers to use that new function.

Let's also add a test to make sure we avoid regressions there.

---
 drivers/clk/at91/clk-generated.c  |   5 +-
 drivers/clk/at91/clk-master.c     |   9 +-
 drivers/clk/at91/clk-peripheral.c |   4 +-
 drivers/clk/clk-composite.c       |   6 +-
 drivers/clk/clk.c                 |  84 ++++++++++++--
 drivers/clk/clk_test.c            | 182 ++++++++++++++++++++++++++++++
 include/linux/clk-provider.h      |   5 +
 7 files changed, 279 insertions(+), 16 deletions(-)
drivers/clk/at91/clk-generated.c +3 -2
index d429ba52a719..943ea67bf135 100644
@@ -136,7 +136,6 @@ static int clk_generated_determine_rate(struct clk_hw *hw,
 {
 	struct clk_generated *gck = to_clk_generated(hw);
 	struct clk_hw *parent = NULL;
-	struct clk_rate_request req_parent = *req;
 	long best_rate = -EINVAL;
 	unsigned long min_rate, parent_rate;
 	int best_diff = -1;
@@ -192,7 +191,9 @@ static int clk_generated_determine_rate(struct clk_hw *hw,
 		goto end;
 
 	for (div = 1; div < GENERATED_MAX_DIV + 2; div++) {
-		req_parent.rate = req->rate * div;
+		struct clk_rate_request req_parent;
+
+		clk_hw_forward_rate_request(hw, req, parent, &req_parent, req->rate * div);
 		if (__clk_determine_rate(parent, &req_parent))
 			continue;
 		clk_generated_best_diff(req, parent, req_parent.rate, div,
drivers/clk/at91/clk-master.c +6 -3
index 164e2959c7cf..b7cd1924de52 100644
@@ -581,7 +581,6 @@ static int clk_sama7g5_master_determine_rate(struct clk_hw *hw,
 					     struct clk_rate_request *req)
 {
 	struct clk_master *master = to_clk_master(hw);
-	struct clk_rate_request req_parent = *req;
 	struct clk_hw *parent;
 	long best_rate = LONG_MIN, best_diff = LONG_MIN;
 	unsigned long parent_rate;
@@ -618,11 +617,15 @@ static int clk_sama7g5_master_determine_rate(struct clk_hw *hw,
 		goto end;
 
 	for (div = 0; div < MASTER_PRES_MAX + 1; div++) {
+		struct clk_rate_request req_parent;
+		unsigned long req_rate;
+
 		if (div == MASTER_PRES_MAX)
-			req_parent.rate = req->rate * 3;
+			req_rate = req->rate * 3;
 		else
-			req_parent.rate = req->rate << div;
+			req_rate = req->rate << div;
 
+		clk_hw_forward_rate_request(hw, req, parent, &req_parent, req_rate);
 		if (__clk_determine_rate(parent, &req_parent))
 			continue;
 
drivers/clk/at91/clk-peripheral.c +2 -2
index e14fa5ac734c..5104d4025484 100644
@@ -269,7 +269,6 @@ static int clk_sam9x5_peripheral_determine_rate(struct clk_hw *hw,
 {
 	struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
 	struct clk_hw *parent = clk_hw_get_parent(hw);
-	struct clk_rate_request req_parent = *req;
 	unsigned long parent_rate = clk_hw_get_rate(parent);
 	unsigned long tmp_rate;
 	long best_rate = LONG_MIN;
@@ -302,8 +301,9 @@ static int clk_sam9x5_peripheral_determine_rate(struct clk_hw *hw,
 		goto end;
 
 	for (shift = 0; shift <= PERIPHERAL_MAX_SHIFT; shift++) {
-		req_parent.rate = req->rate << shift;
+		struct clk_rate_request req_parent;
 
+		clk_hw_forward_rate_request(hw, req, parent, &req_parent, req->rate << shift);
 		if (__clk_determine_rate(parent, &req_parent))
 			continue;
 
drivers/clk/clk-composite.c +4 -2
index b9c5f904f535..edfa94641bbf 100644
@@ -85,10 +85,11 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 		req->best_parent_hw = NULL;
 
 		if (clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT) {
-			struct clk_rate_request tmp_req = *req;
+			struct clk_rate_request tmp_req;
 
 			parent = clk_hw_get_parent(mux_hw);
 
+			clk_hw_forward_rate_request(hw, req, parent, &tmp_req, req->rate);
 			ret = clk_composite_determine_rate_for_parent(rate_hw,
 								      &tmp_req,
 								      parent,
@@ -104,12 +105,13 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 		}
 
 		for (i = 0; i < clk_hw_get_num_parents(mux_hw); i++) {
-			struct clk_rate_request tmp_req = *req;
+			struct clk_rate_request tmp_req;
 
 			parent = clk_hw_get_parent_by_index(mux_hw, i);
 			if (!parent)
 				continue;
 
+			clk_hw_forward_rate_request(hw, req, parent, &tmp_req, req->rate);
 			ret = clk_composite_determine_rate_for_parent(rate_hw,
 								      &tmp_req,
 								      parent,
drivers/clk/clk.c +77 -7

149 lines not on this page · raw message

drivers/clk/clk_test.c +182 -0

207 lines not on this page · raw message

include/linux/clk-provider.h +5 -0
index d857717aa386..8bce6c524f29 100644
@@ -65,6 +65,11 @@ struct clk_rate_request {
 void clk_hw_init_rate_request(const struct clk_hw *hw,
 			      struct clk_rate_request *req,
 			      unsigned long rate);
+void clk_hw_forward_rate_request(const struct clk_hw *core,
+				 const struct clk_rate_request *old_req,
+				 const struct clk_hw *parent,
+				 struct clk_rate_request *req,
+				 unsigned long parent_rate);
 
 /**
  * struct clk_duty - Struture encoding the duty cycle ratio of a clock
Signature · 1 line
2.37.1
#26 lore raw

PATCH v8 22/25 clk: Zero the clk_rate_request structure

In reply to Maxime Ripard

In order to make sure we don't carry anything over from an already existing clk_rate_request pointer we would pass to clk_core_init_rate_req(), let's zero the entire structure before initializing it.

---
 drivers/clk/clk.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/clk/clk.c +2 -0
index 75865f07b5e5..77601f435f12 100644
@@ -1446,6 +1446,8 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 	if (WARN_ON(!core || !req))
 		return;
 
+	memset(req, 0, sizeof(*req));
+
 	req->rate = rate;
 	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 
Signature · 1 line
2.37.1
#27 lore raw

PATCH v8 23/25 clk: Introduce the clk_hw_get_rate_range function

In reply to Maxime Ripard

Some clock providers are hand-crafting their clk_rate_request, and need to figure out the current boundaries of their clk_hw to fill it properly.

Let's create such a function for clock providers.

---
 drivers/clk/clk.c            | 16 ++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 2 files changed, 18 insertions(+)
drivers/clk/clk.c +16 -0
index 77601f435f12..5bdfd645f1dc 100644
@@ -683,6 +683,22 @@ static void clk_core_get_boundaries(struct clk_core *core,
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
 
+/*
+ * clk_hw_get_rate_range() - returns the clock rate range for a hw clk
+ * @hw: the hw clk we want to get the range from
+ * @min_rate: pointer to the variable that will hold the minimum
+ * @max_rate: pointer to the variable that will hold the maximum
+ *
+ * Fills the @min_rate and @max_rate variables with the minimum and
+ * maximum that clock can reach.
+ */
+void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
+			   unsigned long *max_rate)
+{
+	clk_core_get_boundaries(hw->core, min_rate, max_rate);
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_rate_range);
+
 static bool clk_core_check_boundaries(struct clk_core *core,
 				      unsigned long min_rate,
 				      unsigned long max_rate)
include/linux/clk-provider.h +2 -0
index 8bce6c524f29..8724a3547a79 100644
@@ -1267,6 +1267,8 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags);
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
+void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
+			   unsigned long *max_rate);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
 
Signature · 1 line
2.37.1
#28 lore raw

PATCH v8 24/25 clk: qcom: clk-rcg2: Take clock boundaries into consideration for gfx3d

In reply to Maxime Ripard

The gfx3d clock is hand-crafting its own clk_rate_request in clk_gfx3d_determine_rate to pass to the parent of that clock.

However, since the clk_rate_request is zero'd at creation, it will have a max_rate of 0 which will break any code depending on the clock boundaries.

That includes the recent commit 948fb0969eae ("clk: Always clamp the rounded rate") which will clamp the rate given to clk_round_rate() to the current clock boundaries.

For the gfx3d clock, it means that since both the min_rate and max_rate fields are set at zero, clk_round_rate() now always return 0.

Let's initialize the min_rate and max_rate fields properly for that clock.

Fixes 948fb0969eae ("clk: Always clamp the rounded rate")
---
 drivers/clk/qcom/clk-rcg2.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/clk/qcom/clk-rcg2.c +9 -0
index 28019edd2a50..ee536b457952 100644
@@ -908,6 +908,15 @@ static int clk_gfx3d_determine_rate(struct clk_hw *hw,
 		req->best_parent_hw = p2;
 	}
 
+	clk_hw_get_rate_range(req->best_parent_hw,
+			      &parent_req.min_rate, &parent_req.max_rate);
+
+	if (req->min_rate > parent_req.min_rate)
+		parent_req.min_rate = req->min_rate;
+
+	if (req->max_rate < parent_req.max_rate)
+		parent_req.max_rate = req->max_rate;
+
 	ret = __clk_determine_rate(req->best_parent_hw, &parent_req);
 	if (ret)
 		return ret;
Signature · 1 line
2.37.1
#29 lore raw

PATCH v8 25/25 clk: tests: Add missing test case for ranges

In reply to Maxime Ripard

Let's add a test on the rate range after a reparenting. This fails for now, but it's worth having it to document the corner cases we don't support yet.

---
 drivers/clk/clk_test.c | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
drivers/clk/clk_test.c +53 -0
index 5a5b7a8baba2..8e0c76ab1a6d 100644
@@ -514,9 +514,62 @@ clk_test_multiple_parents_mux_has_parent(struct kunit *test)
 	clk_put(clk);
 }
 
+/*
+ * Test that for a clock with a multiple parents, if we set a range on
+ * that clock and the parent is changed, its rate after the reparenting
+ * is still within the range we asked for.
+ *
+ * FIXME: clk_set_parent() only does the reparenting but doesn't
+ * reevaluate whether the new clock rate is within its boundaries or
+ * not.
+ */
+static void
+clk_test_multiple_parents_mux_set_range_set_parent_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *parent1, *parent2;
+	unsigned long rate;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	parent1 = clk_hw_get_clk(&ctx->parents_ctx[0].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent1);
+	KUNIT_ASSERT_TRUE(test, clk_is_match(clk_get_parent(clk), parent1));
+
+	parent2 = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent2);
+
+	ret = clk_set_rate(parent1, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate(parent2, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1 - 1000,
+				 DUMMY_CLOCK_RATE_1 + 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_parent(clk, parent2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	clk_put(parent2);
+	clk_put(parent1);
+	clk_put(clk);
+}
+
 static struct kunit_case clk_multiple_parents_mux_test_cases[] = {
 	KUNIT_CASE(clk_test_multiple_parents_mux_get_parent),
 	KUNIT_CASE(clk_test_multiple_parents_mux_has_parent),
+	KUNIT_CASE(clk_test_multiple_parents_mux_set_range_set_parent_get_rate),
 	{}
 };
 
Signature · 1 line
2.37.1