[PATCH 0/2] clk: More Range Fixes

[PATCH 0/2] clk: More Range Fixes

[Maxime Ripard]

Hi,

Here are patches that might address the issues reported by Marek and Mark,
according to Stephen's intuition :)

Either way, it's still fixing and adding test coverage for a regression
introduced by my recent work.

If it's easier for you to test, this series is also pushed here:
https://github.com/mripard/linux/tree/rpi/clk-fixes-again

Maxime

To: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Maxime Ripard (2):
      clk: Update req_rate on __clk_recalc_rates()
      clk: tests: Add tests for notifiers

 drivers/clk/clk.c      |  39 ++++---------
 drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 167 insertions(+), 28 deletions(-)
---
base-commit: cd9fd78f5c11b5e165d9317ef11e613f4aef4dd1
change-id: 20221010-rpi-clk-fixes-again-a95180ef0108

Best regards,
-- 
Maxime Ripard <maxime@cerno.tech>

[PATCH 1/2] clk: Update req_rate on __clk_recalc_rates()

[Maxime Ripard]

Commit cb1b1dd96241 ("clk: Set req_rate on reparenting") introduced a
new function, clk_core_update_orphan_child_rates(), that updates the
req_rate field on reparenting.

It turns out that that function will interfere with the clock notifying
done by __clk_recalc_rates(). This ends up reporting the new rate in
both the old_rate and new_rate fields of struct clk_notifier_data.

Since clk_core_update_orphan_child_rates() is basically
__clk_recalc_rates() without the notifiers, and with the req_rate field
update, we can drop clk_core_update_orphan_child_rates() entirely, and
make __clk_recalc_rates() update req_rate.

However, __clk_recalc_rates() is being called in several code paths:
when retrieving a rate (most likely through clk_get_rate()), when changing
parents (through clk_set_rate() or clk_hw_reparent()), or when updating
the orphan status (through clk_core_reparent_orphans_nolock(), called at
registration).

Updating req_rate on reparenting or initialisation makes sense, but we
shouldn't do it on clk_get_rate(). Thus an extra flag has been added to
update or not req_rate depending on the context.

Fixes: cb1b1dd96241 ("clk: Set req_rate on reparenting")
Link: https://lore.kernel.org/linux-clk/0acc7217-762c-7c0d-45a0-55c384824ce4@samsung.com/
Link: https://lore.kernel.org/linux-clk/Y0QNSx+ZgqKSvPOC@sirena.org.uk/
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reported-by: Mark Brown <broonie@kernel.org>
Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dee5f39bfa90..c3c3f8c07258 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1759,6 +1759,7 @@ static unsigned long clk_recalc(struct clk_core *core,
 /**
  * __clk_recalc_rates
  * @core: first clk in the subtree
+ * @update_req: Whether req_rate should be updated with the new rate
  * @msg: notification type (see include/linux/clk.h)
  *
  * Walks the subtree of clks starting with clk and recalculates rates as it
@@ -1768,7 +1769,8 @@ static unsigned long clk_recalc(struct clk_core *core,
  * clk_recalc_rates also propagates the POST_RATE_CHANGE notification,
  * if necessary.
  */
-static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
+static void __clk_recalc_rates(struct clk_core *core, bool update_req,
+			       unsigned long msg)
 {
 	unsigned long old_rate;
 	unsigned long parent_rate = 0;
@@ -1782,6 +1784,8 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
 		parent_rate = core->parent->rate;
 
 	core->rate = clk_recalc(core, parent_rate);
+	if (update_req)
+		core->req_rate = core->rate;
 
 	/*
 	 * ignore NOTIFY_STOP and NOTIFY_BAD return values for POST_RATE_CHANGE
@@ -1791,13 +1795,13 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
 		__clk_notify(core, msg, old_rate, core->rate);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		__clk_recalc_rates(child, msg);
+		__clk_recalc_rates(child, update_req, msg);
 }
 
 static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
 {
 	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
-		__clk_recalc_rates(core, 0);
+		__clk_recalc_rates(core, false, 0);
 
 	return clk_core_get_rate_nolock(core);
 }
@@ -1900,23 +1904,6 @@ 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;
@@ -1986,8 +1973,6 @@ 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;
 }
 
@@ -2033,7 +2018,6 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
 		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;
@@ -2657,9 +2641,8 @@ 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);
+	__clk_recalc_rates(core, true, POST_RATE_CHANGE);
 }
 
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
@@ -2743,9 +2726,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 
 	/* propagate rate an accuracy recalculation accordingly */
 	if (ret) {
-		__clk_recalc_rates(core, ABORT_RATE_CHANGE);
+		__clk_recalc_rates(core, true, ABORT_RATE_CHANGE);
 	} else {
-		__clk_recalc_rates(core, POST_RATE_CHANGE);
+		__clk_recalc_rates(core, true, POST_RATE_CHANGE);
 		__clk_recalc_accuracies(core);
 	}
 
@@ -3642,7 +3625,7 @@ static void clk_core_reparent_orphans_nolock(void)
 			__clk_set_parent_before(orphan, parent);
 			__clk_set_parent_after(orphan, parent, NULL);
 			__clk_recalc_accuracies(orphan);
-			__clk_recalc_rates(orphan, 0);
+			__clk_recalc_rates(orphan, true, 0);
 
 			/*
 			 * __clk_init_parent() will set the initial req_rate to

-- 
b4 0.11.0-dev-7da52

[PATCH 2/2] clk: tests: Add tests for notifiers

[Maxime Ripard]

We're recently encountered a regression due to the rates reported
through the clk_notifier_data being off when changing parents.

Let's add a test suite and a test to make sure that we do get notified
and with the proper rates.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 509256c5567a..f9a5c2964c65 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2239,10 +2239,166 @@ static struct kunit_suite clk_leaf_mux_set_rate_parent_test_suite = {
 	.test_cases = clk_leaf_mux_set_rate_parent_test_cases,
 };
 
+struct clk_mux_notifier_rate_change {
+	bool done;
+	unsigned long old_rate;
+	unsigned long new_rate;
+	wait_queue_head_t wq;
+};
+
+struct clk_mux_notifier_ctx {
+	struct clk_multiple_parent_ctx mux_ctx;
+	struct clk *clk;
+	struct notifier_block clk_nb;
+	struct clk_mux_notifier_rate_change pre_rate_change;
+	struct clk_mux_notifier_rate_change post_rate_change;
+};
+
+#define NOTIFIER_TIMEOUT_MS 100
+
+static int clk_mux_notifier_callback(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct clk_notifier_data *clk_data = data;
+	struct clk_mux_notifier_ctx *ctx = container_of(nb,
+							struct clk_mux_notifier_ctx,
+							clk_nb);
+
+	if (action & PRE_RATE_CHANGE) {
+		ctx->pre_rate_change.old_rate = clk_data->old_rate;
+		ctx->pre_rate_change.new_rate = clk_data->new_rate;
+		ctx->pre_rate_change.done = true;
+		wake_up_interruptible(&ctx->pre_rate_change.wq);
+	}
+
+	if (action & POST_RATE_CHANGE) {
+		ctx->post_rate_change.old_rate = clk_data->old_rate;
+		ctx->post_rate_change.new_rate = clk_data->new_rate;
+		ctx->post_rate_change.done = true;
+		wake_up_interruptible(&ctx->post_rate_change.wq);
+	}
+
+	return 0;
+}
+
+static int clk_mux_notifier_test_init(struct kunit *test)
+{
+	struct clk_mux_notifier_ctx *ctx;
+	const char *top_parents[2] = { "parent-0", "parent-1" };
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+	ctx->clk_nb.notifier_call = clk_mux_notifier_callback;
+	init_waitqueue_head(&ctx->pre_rate_change.wq);
+	init_waitqueue_head(&ctx->post_rate_change.wq);
+
+	ctx->mux_ctx.parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
+								    &clk_dummy_rate_ops,
+								    0);
+	ctx->mux_ctx.parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
+	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[0].hw);
+	if (ret)
+		return ret;
+
+	ctx->mux_ctx.parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
+								    &clk_dummy_rate_ops,
+								    0);
+	ctx->mux_ctx.parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
+	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[1].hw);
+	if (ret)
+		return ret;
+
+	ctx->mux_ctx.current_parent = 0;
+	ctx->mux_ctx.hw.init = CLK_HW_INIT_PARENTS("test-mux", top_parents,
+						   &clk_multiple_parents_mux_ops,
+						   0);
+	ret = clk_hw_register(NULL, &ctx->mux_ctx.hw);
+	if (ret)
+		return ret;
+
+	ctx->clk = clk_hw_get_clk(&ctx->mux_ctx.hw, NULL);
+	ret = clk_notifier_register(ctx->clk, &ctx->clk_nb);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void clk_mux_notifier_test_exit(struct kunit *test)
+{
+	struct clk_mux_notifier_ctx *ctx = test->priv;
+	struct clk *clk = ctx->clk;
+
+	clk_notifier_unregister(clk, &ctx->clk_nb);
+	clk_put(clk);
+
+	clk_hw_unregister(&ctx->mux_ctx.hw);
+	clk_hw_unregister(&ctx->mux_ctx.parents_ctx[0].hw);
+	clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw);
+}
+
+/*
+ * Test that if the we have a notifier registered on a mux, the core
+ * will notify us when we switch to another parent, and with the proper
+ * old and new rates.
+ */
+static void clk_mux_notifier_set_parent_test(struct kunit *test)
+{
+	struct clk_mux_notifier_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->mux_ctx.hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+	struct clk *new_parent = clk_hw_get_clk(&ctx->mux_ctx.parents_ctx[1].hw, NULL);
+	int ret;
+
+	ret = clk_set_parent(clk, new_parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(ctx->pre_rate_change.wq,
+					       ctx->pre_rate_change.done,
+					       msecs_to_jiffies(NOTIFIER_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, ctx->pre_rate_change.old_rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_EQ(test, ctx->pre_rate_change.new_rate, DUMMY_CLOCK_RATE_2);
+
+	ret = wait_event_interruptible_timeout(ctx->post_rate_change.wq,
+					       ctx->post_rate_change.done,
+					       msecs_to_jiffies(NOTIFIER_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, ctx->post_rate_change.old_rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_EQ(test, ctx->post_rate_change.new_rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(new_parent);
+	clk_put(clk);
+}
+
+static struct kunit_case clk_mux_notifier_test_cases[] = {
+	KUNIT_CASE(clk_mux_notifier_set_parent_test),
+	{}
+};
+
+/*
+ * Test suite for a mux with multiple parents, and a notifier registered
+ * on the mux.
+ *
+ * These tests exercise the behaviour of notifiers.
+ */
+static struct kunit_suite clk_mux_notifier_test_suite = {
+	.name = "clk-mux-notifier",
+	.init = clk_mux_notifier_test_init,
+	.exit = clk_mux_notifier_test_exit,
+	.test_cases = clk_mux_notifier_test_cases,
+};
+
 kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,
 	&clk_multiple_parents_mux_test_suite,
+	&clk_mux_notifier_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,

-- 
b4 0.11.0-dev-7da52

Re: [PATCH 0/2] clk: More Range Fixes

[Marek Szyprowski]

Hi Maxime,

On 10.10.2022 16:47, Maxime Ripard wrote:
> Here are patches that might address the issues reported by Marek and Mark,
> according to Stephen's intuition :)
>
> Either way, it's still fixing and adding test coverage for a regression
> introduced by my recent work.
>
> If it's easier for you to test, this series is also pushed here:
> https://protect2.fireeye.com/v1/url?k=e4440f7a-bbdf3663-e4458435-000babff3793-bdf960866cc2289b&q=1&e=179d765b-8652-488e-977d-8f404e45f6c8&u=https%3A%2F%2Fgithub.com%2Fmripard%2Flinux%2Ftree%2Frpi%2Fclk-fixes-again
>
> Maxime
>
> To: Michael Turquette <mturquette@baylibre.com>
> To: Stephen Boyd <sboyd@kernel.org>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

It looks it fixes the issue I've reported here: 
https://lore.kernel.org/all/8cdd1927-da38-c23e-fa75-384694724b1c@samsung.com/

All my Amlogic Meson G12A/B based boards work fine after applying those 
fixes.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Maxime Ripard (2):
>        clk: Update req_rate on __clk_recalc_rates()
>        clk: tests: Add tests for notifiers
>
>   drivers/clk/clk.c      |  39 ++++---------
>   drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 167 insertions(+), 28 deletions(-)
> ---
> base-commit: cd9fd78f5c11b5e165d9317ef11e613f4aef4dd1
> change-id: 20221010-rpi-clk-fixes-again-a95180ef0108
>
> Best regards,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/2] clk: tests: Add tests for notifiers

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-10 07:47:39)
> We're recently encountered a regression due to the rates reported
> through the clk_notifier_data being off when changing parents.
> 
> Let's add a test suite and a test to make sure that we do get notified
> and with the proper rates.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied to clk-next

Re: [PATCH 1/2] clk: Update req_rate on __clk_recalc_rates()

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-10 07:47:38)
> Commit cb1b1dd96241 ("clk: Set req_rate on reparenting") introduced a
> new function, clk_core_update_orphan_child_rates(), that updates the
> req_rate field on reparenting.
> 
> It turns out that that function will interfere with the clock notifying
> done by __clk_recalc_rates(). This ends up reporting the new rate in
> both the old_rate and new_rate fields of struct clk_notifier_data.
> 
> Since clk_core_update_orphan_child_rates() is basically
> __clk_recalc_rates() without the notifiers, and with the req_rate field
> update, we can drop clk_core_update_orphan_child_rates() entirely, and
> make __clk_recalc_rates() update req_rate.
> 
> However, __clk_recalc_rates() is being called in several code paths:
> when retrieving a rate (most likely through clk_get_rate()), when changing
> parents (through clk_set_rate() or clk_hw_reparent()), or when updating
> the orphan status (through clk_core_reparent_orphans_nolock(), called at
> registration).
> 
> Updating req_rate on reparenting or initialisation makes sense, but we
> shouldn't do it on clk_get_rate(). Thus an extra flag has been added to
> update or not req_rate depending on the context.
> 
> Fixes: cb1b1dd96241 ("clk: Set req_rate on reparenting")
> Link: https://lore.kernel.org/linux-clk/0acc7217-762c-7c0d-45a0-55c384824ce4@samsung.com/
> Link: https://lore.kernel.org/linux-clk/Y0QNSx+ZgqKSvPOC@sirena.org.uk/
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reported-by: Mark Brown <broonie@kernel.org>
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied to clk-next