summaryrefslogtreecommitdiff
path: root/drivers/clk
diff options
context:
space:
mode:
authorMaxime Ripard <maxime@cerno.tech>2022-08-16 13:25:18 +0200
committerStephen Boyd <sboyd@kernel.org>2022-09-15 09:31:09 -0700
commitcb1b1dd96241f37ea41d241946d5153c48141cd5 (patch)
treea13346389da096ba710c88177b752a416bdd9e2f /drivers/clk
parent3afb07231d603d51dca6a5d5e16d9d8f422f9b5f (diff)
clk: Set req_rate on reparenting
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. The natural solution to this would be to update core->rate and core->req_rate in clk_reparent() by calling clk_recalc(). However, this doesn't work in all cases. Indeed, clk_recalc() is called by __clk_set_parent_before(), __clk_set_parent() and clk_core_reparent(). Both __clk_set_parent_before() and __clk_set_parent will call clk_recalc() with the enable_lock taken through a call to clk_enable_lock(), the underlying locking primitive being a spinlock. clk_recalc() calls the backing driver .recalc_rate hook, and that implementation might sleep if the underlying device uses a bus with accesses that might sleep, such as i2c. In such a situation, we would end up sleeping while holding a spinlock, and thus in an atomic section. In order to work around this, we can move the core->rate and core->req_rate update to the clk_recalc() calling sites, after the enable_lock has been released if it was taken. The only situation that could still be problematic is the clk_core_reparent() -> clk_reparent() case that doesn't have any locking. clk_core_reparent() is itself called by clk_hw_reparent(), which is then called by 4 drivers: * clk-stm32mp1.c, stm32/clk-stm32-core.c and tegra/clk-tegra210-emc.c use it in their set_parent implementation. The set_parent hook is only called by __clk_set_parent() and clk_change_rate(), both of them calling it without the enable_lock taken. * clk/tegra/clk-tegra124-emc.c calls it as part of its set_rate implementation. set_rate is only called by clk_change_rate(), again without the enable_lock taken. In both cases we can't end up in a situation where the clk_hw_reparent() caller would hold a spinlock, so it seems like this is a good workaround. Let's also add some unit tests to make sure we cover the original bug. Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> # imx8mp Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> # exynos4210, meson g12b Signed-off-by: Maxime Ripard <maxime@cerno.tech> Link: https://lore.kernel.org/r/20220816112530.1837489-14-maxime@cerno.tech Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
Diffstat (limited to 'drivers/clk')
-rw-r--r--drivers/clk/clk.c22
-rw-r--r--drivers/clk/clk_test.c239
2 files changed, 261 insertions, 0 deletions
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 53b28e63deae..91bb1ea0e147 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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;
@@ -1834,6 +1851,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;
}
@@ -1878,6 +1897,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;
@@ -2506,6 +2527,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);
}
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index d3e121f21ae2..d1b1372f7aaa 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -596,6 +596,41 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent(struct kunit *test)
/*
* 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(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, 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.
*/
static void
@@ -627,6 +662,39 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate(struct kunit
/*
* 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)
+{
+ 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
* its rate is out of range.
*/
@@ -659,6 +727,43 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified(st
}
/*
+ * 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_untouched(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, 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
* account when rounding a rate.
@@ -724,8 +829,11 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(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),
+ KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_drop_range),
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate),
+ KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_put),
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified),
+ KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_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_parent_get_rate),
{}
@@ -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,
};
+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@samsung.com/
+ */
+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,