[PATCH 00/22] clk: More clock rate fixes and tests

[PATCH 00/22] clk: More clock rate fixes and tests

[Maxime Ripard]

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.

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.

The last patch will probably prove to be controversial, but it can be
left out without affecting the rest of the series. It will affect the
meson clock drivers for the g12b SoC at least. It stems from the
realisation that on the g12b, 4 PLLs (and thus all their children) have
a rate of 0, pretty much forever which feels like a bug considering the
rate 0 is used as an error in most places.

Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
return 0 if the diviser of the PLL is set to 0 in the register, but:

  - pcie_pll_dco has a few registers to initialize set in
    g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
    hook and will instead call it in the enable hook. This looks like a
    bug and could be easily fixed by adding that init hook.

  - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
    the initialisation of their register (g12a_gp0_init_regs and
    g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
    long as set_rate isn't called, that field is going to remain at 0. And
    since all but one users don't have CLK_SET_RATE_PARENT, this is
    still fairly unlikely.

  - hdmi_pll_dco doesn't set the n field in the initialisation either,
    but also doesn't have a set_rate implementation. Thus, if the
    bootloader doesn't set it, this clock and all its children will
    always report a rate of 0, even if the clock is functional.

During the discussion with amlogic clock maintainers, we kind of ended
up on a disagreement of whether returning 0 was ok or not, hence the
expected controversy :)

Let me know what you think,
Maxime

Maxime Ripard (22):
  clk: Drop the rate range on clk_put()
  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: Fix clk_get_parent() documentation
  clk: Set req_rate on reparenting
  clk: Skip set_rate_range if our clock is orphan
  clk: Add our request boundaries in clk_core_init_rate_req
  clk: Change clk_core_init_rate_req prototype
  clk: Introduce clk_hw_init_rate_request()
  clk: Add missing clk_core_init_rate_req calls
  clk: Remove redundant clk_core_init_rate_req() call
  clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
  clk: Introduce clk_core_has_parent()
  clk: Stop forwarding clk_rate_requests to the parent
  clk: Zero the clk_rate_request structure
  clk: Test the clock pointer in clk_hw_get_name()
  clk: Prevent a clock without a rate to register

 drivers/clk/clk.c            |  239 +++++--
 drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
 include/linux/clk-provider.h |    6 +
 include/linux/clk.h          |    5 +-
 4 files changed, 1439 insertions(+), 67 deletions(-)

-- 
2.35.1

[PATCH 01/22] clk: Drop the rate range on clk_put()

[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")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  45 +++++++++++------
 drivers/clk/clk_test.c | 108 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed119182aa1b..4ccf52d31a21 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2332,19 +2332,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;
 
@@ -2357,8 +2353,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);
 
@@ -2402,6 +2396,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;
@@ -4403,9 +4419,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);
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 6731a822f4e3..fd2339cc5898 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -760,9 +760,65 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 	clk_put(user1);
 }
 
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), across multiple users, the core will reevaluate
+ * whether a new rate is needed, including when a user drop its clock.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_range_test_multiple_set_range_rate_put_maximized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   0,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   0,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user2);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user1);
+}
+
 static struct kunit_case clk_range_maximize_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range_rate_maximized),
 	KUNIT_CASE(clk_range_test_multiple_set_range_rate_maximized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_put_maximized),
 	{}
 };
 
@@ -877,9 +933,61 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 	clk_put(user1);
 }
 
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), across multiple users, the core will reevaluate
+ * whether a new rate is needed, including when a user drop its clock.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_range_test_multiple_set_range_rate_put_minimized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   DUMMY_CLOCK_RATE_1,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   DUMMY_CLOCK_RATE_2,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user2);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user1);
+}
+
 static struct kunit_case clk_range_minimize_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range_rate_minimized),
 	KUNIT_CASE(clk_range_test_multiple_set_range_rate_minimized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_put_minimized),
 	{}
 };
 
-- 
2.35.1

[PATCH 02/22] clk: tests: Add test suites description

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index fd2339cc5898..663b3dd388f7 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -250,6 +250,11 @@ static struct kunit_case clk_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests are supposed to exercise the rate API with simple scenarios
+ */
 static struct kunit_suite clk_test_suite = {
 	.name = "clk-test",
 	.init = clk_test_init,
@@ -336,6 +341,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 orphan when
+ * registered, but will no longer be when the tests run.
+ *
+ * These tests are supposed to 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,
@@ -645,6 +658,13 @@ static struct kunit_case clk_range_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests are supposed to 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,
@@ -822,6 +842,14 @@ static struct kunit_case clk_range_maximize_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests are supposed to 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,
@@ -991,6 +1019,14 @@ static struct kunit_case clk_range_minimize_test_cases[] = {
 	{}
 };
 
+/*
+ * Test suite for a basic rate clock, without any parent.
+ *
+ * These tests are supposed to 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,
-- 
2.35.1

[PATCH 03/22] clk: tests: Add reference to the orphan mux bug report

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 663b3dd388f7..e86212be74e2 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -314,6 +314,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/7720158d-10a7-a17b-73a4-a8615c9c6d5c@collabora.com/
  */
 static void clk_test_orphan_transparent_parent_mux_set_range(struct kunit *test)
 {
-- 
2.35.1

[PATCH 04/22] clk: tests: Add tests for uncached clock

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 88 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index e86212be74e2..38a2d6abd28a 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -262,6 +262,91 @@ 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 = hw->clk;
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
+
+	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);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+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 are supposed to 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;
@@ -1042,6 +1127,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");
-- 
2.35.1

[PATCH 05/22] clk: tests: Add tests for single parent mux

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 186 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 177 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 38a2d6abd28a..59661144d7a0 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -352,6 +352,181 @@ 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 = hw->clk;
+	struct clk *parent;
+
+	parent = clk_get_parent(clk);
+	KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parent_ctx.hw.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 = hw->clk;
+	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);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+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 are supposed to 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;
@@ -388,14 +563,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.
@@ -440,7 +607,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,
 };
 
@@ -1128,6 +1295,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");
-- 
2.35.1

[PATCH 06/22] clk: tests: Add tests for mux with multiple parents

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 119 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 59661144d7a0..caf2a28ac4c3 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -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;
@@ -347,6 +380,91 @@ 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 = hw->clk;
+	struct clk *parent;
+
+	parent = clk_get_parent(clk);
+	KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parents_ctx[0].hw.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 are supposed to 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;
@@ -1291,6 +1409,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,
-- 
2.35.1

[PATCH 07/22] clk: tests: Add some tests for orphan with multiple parents

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 173 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index caf2a28ac4c3..f51a9ec3e452 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -465,6 +465,178 @@ 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,
+ * 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 = hw->clk;
+	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);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+/*
+ * 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 = hw->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);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = {
+	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),
+	{}
+};
+
+/*
+ * 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 are supposed to 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;
@@ -1410,6 +1582,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,
-- 
2.35.1

[PATCH 08/22] clk: Take into account uncached clocks in clk_set_rate_range()

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  6 +++++-
 drivers/clk/clk_test.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4ccf52d31a21..80eebf62d1f3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2367,6 +2367,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
@@ -2384,7 +2388,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 */
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index f51a9ec3e452..70fee35c43c6 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -362,9 +362,37 @@ static void clk_test_uncached_set_range(struct kunit *test)
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
 }
 
+/*
+ * 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 = hw->clk;
+	unsigned long rate;
+
+	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);
+}
+
 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),
 	{}
 };
 
-- 
2.35.1

[PATCH 09/22] clk: Fix clk_get_parent() documentation

[Maxime Ripard]

The clk_get_parent() documentation in the header states that it will
return a valid pointer, or an error pointer on failure.

However, the documentation in the source file, and the code itself, will
return also return NULL if there isn't any parent for that clock. Let's
mention it.

An orphan clock should return NULL too, so let's add a test for it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk_test.c | 17 +++++++++++++++++
 include/linux/clk.h    |  5 +++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 70fee35c43c6..0f2d52d3a25d 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -532,6 +532,22 @@ clk_orphan_transparent_multiple_parent_mux_test_exit(struct kunit *test)
 	clk_hw_unregister(&ctx->parents_ctx[1].hw);
 }
 
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet,
+ * 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 = hw->clk;
+	struct clk *parent;
+
+	parent = clk_get_parent(clk);
+	KUNIT_EXPECT_PTR_EQ(test, parent, NULL);
+}
+
 /*
  * 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
@@ -642,6 +658,7 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate(struct kuni
 }
 
 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),
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 39faa54efe88..3b8cf492ff1a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -744,8 +744,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
  * clk_get_parent - get the parent clock source for this clock
  * @clk: clock source
  *
- * Returns struct clk corresponding to parent clock source, or
- * valid IS_ERR() condition containing errno.
+ * Returns struct clk corresponding to parent clock source, a NULL
+ * pointer if it doesn't have a parent, or a valid IS_ERR() condition
+ * containing errno.
  */
 struct clk *clk_get_parent(struct clk *clk);
 
-- 
2.35.1

[PATCH 10/22] clk: Set req_rate on reparenting

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  18 ++++
 drivers/clk/clk_test.c | 231 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 80eebf62d1f3..5b6ec11fcfcd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1759,6 +1759,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;
@@ -1783,6 +1800,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,
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0f2d52d3a25d..fa6e32c460ac 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -575,6 +575,39 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent(struct kunit *test)
 	clk_put(parent);
 }
 
+/*
+ * 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 = hw->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);
+}
+
 /*
  * 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.
@@ -603,6 +636,39 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate(struct kunit
 	KUNIT_EXPECT_EQ(test, parent_rate, rate);
 }
 
+/*
+ * 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
@@ -634,6 +700,41 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified(st
 	clk_put(parent);
 }
 
+/*
+ * 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 = hw->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);
+}
+
 /*
  * 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
@@ -660,8 +761,11 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate(struct kuni
 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),
 	{}
 };
@@ -946,6 +1050,132 @@ 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 = hw->clk;
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
+}
+
+/*
+ * 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 = hw->clk;
+	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);
+}
+
+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 are supposed to 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
@@ -1629,6 +1859,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,
-- 
2.35.1

[PATCH 11/22] clk: Skip set_rate_range if our clock is orphan

[Maxime Ripard]

clk_set_rate_range will now force a clk_set_rate() call to
core->req_rate. However, if our clock is orphan, req_rate will be 0 and
we will thus end up calling a set_rate to 0 on potentially all that
clock subtree.

This can be fairly invasive and result in poor decisions. In such a
case, let's just store the new range but bail out and skip the set_rate.

When that clock is no longer orphan though, we should be enforcing the
new range but we don't. Let's add a skipped test to make sure we don't
forget about that corner case.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  6 +++++
 drivers/clk/clk_test.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5b6ec11fcfcd..80fbec87309e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2389,6 +2389,12 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 	if (clk->core->flags & CLK_GET_RATE_NOCACHE)
 		rate = clk_core_get_rate_recalc(clk->core);
 
+	if (clk->core->orphan && !rate) {
+		pr_warn("%s: clk %s: Clock is orphan and doesn't have a rate!\n",
+			__func__, clk->core->name);
+		goto out;
+	}
+
 	/*
 	 * Since the boundaries have been changed, let's give the
 	 * opportunity to the provider to adjust the clock rate based on
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index fa6e32c460ac..0fffc70f9e2c 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -735,6 +735,26 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouched(s
 	clk_put(parent);
 }
 
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet,
+ * calling clk_set_rate_range() will succeed but won't affect its rate.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_range_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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_get_rate(clk);
+	KUNIT_EXPECT_EQ(test, rate, 0);
+}
+
 /*
  * 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
@@ -758,6 +778,43 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate(struct kuni
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
 }
 
+/*
+ * 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 = hw->clk, *parent;
+	unsigned long rate;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 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_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(parent);
+}
+
 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),
@@ -766,7 +823,9 @@ static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[]
 	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_get_rate),
 	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),
 	{}
 };
 
-- 
2.35.1

[PATCH 12/22] clk: Add our request boundaries in clk_core_init_rate_req

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 80fbec87309e..458c9fcf0349 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1382,6 +1382,8 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 	if (WARN_ON(!core || !req))
 		return;
 
+	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
+
 	parent = core->parent;
 	if (parent) {
 		req->best_parent_hw = parent->hw;
@@ -1456,7 +1458,6 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 	int ret;
 	struct clk_rate_request req;
 
-	clk_core_get_boundaries(hw->core, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(hw->core, &req);
@@ -1489,7 +1490,6 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
-	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
@@ -1996,8 +1996,6 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *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);
 
-- 
2.35.1

[PATCH 13/22] clk: Change clk_core_init_rate_req prototype

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 458c9fcf0349..399080456e45 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1375,13 +1375,15 @@ 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;
 	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 
 	parent = core->parent;
@@ -1409,7 +1411,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);
@@ -1995,9 +1997,7 @@ 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;
-
-		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)
-- 
2.35.1

[PATCH 14/22] clk: Introduce clk_hw_init_rate_request()

[Maxime Ripard]

Some drivers (at91, imx, qcom) use __clk_determine_rate directly, and
thus will need to initialise a clk_rate_request structure.

However, drivers don't have access to the function that the core uses to
initialize that structure which could prove to be useful.

Let's create a function for clock providers that will initialize a
clk_rate_request for a given clk_hw pointer and a rate.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c            | 20 ++++++++++++++++++++
 include/linux/clk-provider.h |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 399080456e45..83dd5f1df0b9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1396,6 +1396,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 and fills a clk_rate_request structure to submit to
+ * __clk_determine_rate or similar functions.
+ */
+void clk_hw_init_rate_request(struct clk_hw * const 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;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c10dc4c659e2..39e4ed301ec5 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -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(struct clk_hw * const hw,
+			      struct clk_rate_request *req,
+			      unsigned long rate);
+
 /**
  * struct clk_duty - Struture encoding the duty cycle ratio of a clock
  *
-- 
2.35.1

[PATCH 15/22] clk: Add missing clk_core_init_rate_req calls

[Maxime Ripard]

Some callers of clk_core_round_rate_nolock() will initialize the
clk_rate_request structure by hand, missing a few parameters that leads
to inconsistencies in what drivers can expect from that structure.

Let's use clk_core_init_rate_req() everywhere to make sure it's built in
a consistent manner.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 83dd5f1df0b9..3a59152b06b8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1480,7 +1480,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 	int ret;
 	struct clk_rate_request req;
 
-	req.rate = rate;
+	clk_core_init_rate_req(hw->core, &req, rate);
 
 	ret = clk_core_round_rate_nolock(hw->core, &req);
 	if (ret)
@@ -1512,7 +1512,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
-	req.rate = rate;
+	clk_core_init_rate_req(clk->core, &req, rate);
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
 
@@ -2216,8 +2216,7 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 	if (cnt < 0)
 		return cnt;
 
-	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
-	req.rate = req_rate;
+	clk_core_init_rate_req(core, &req, req_rate);
 
 	ret = clk_core_round_rate_nolock(core, &req);
 
-- 
2.35.1

[PATCH 16/22] clk: Remove redundant clk_core_init_rate_req() call

[Maxime Ripard]

Since all the users of clk_core_round_rate_nolock() will now properly
initialize, there's no need for it to initialize the request itself.

This is even dangerous, as if the clock cannot change its rate by itself
and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call
itself with the parent clock but the client clk_rate_request structure.

We will then reinitialize the child request with the parent context
(parent, boundaries, etc.), which is an issue if the parent ever changes
its own parent or parent rate.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3a59152b06b8..ccb6e9686fb8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1431,8 +1431,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)
-- 
2.35.1

[PATCH 17/22] clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ccb6e9686fb8..448ead0da1a5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -543,6 +543,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)
@@ -556,8 +559,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;
 
@@ -580,7 +587,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 {
-- 
2.35.1

[PATCH 18/22] clk: Introduce clk_core_has_parent()

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      | 36 +++++++++++++++++++++---------------
 drivers/clk/clk_test.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 448ead0da1a5..2bbeeb2f729e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -546,6 +546,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, 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);
 
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0fffc70f9e2c..0b9088cdb8fe 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -473,8 +473,24 @@ clk_test_multiple_parents_mux_get_parent(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parents_ctx[0].hw.clk));
 }
 
+/*
+ * Test that for a clock with a multiple parents, clk_has_parent()
+ * actually reports all of them as parents.
+ */
+static void
+clk_test_multiple_parents_mux_has_parent(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, ctx->parents_ctx[0].hw.clk));
+	KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, ctx->parents_ctx[1].hw.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),
 	{}
 };
 
@@ -906,6 +922,21 @@ clk_test_single_parent_mux_get_parent(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parent_ctx.hw.clk));
 }
 
+/*
+ * Test that for a clock with a single parent, clk_has_parent() actually
+ * reports it as a parent.
+ */
+static void
+clk_test_single_parent_mux_has_parent(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *parent = ctx->parent_ctx.hw.clk;
+
+	KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, parent));
+}
+
 /*
  * 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,
@@ -1004,6 +1035,7 @@ clk_test_single_parent_mux_set_range_round_rate_child_smaller(struct kunit *test
 
 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_has_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),
-- 
2.35.1

[PATCH 19/22] clk: Stop forwarding clk_rate_requests to the parent

[Maxime Ripard]

If the clock cannot modify its rate and has CLK_SET_RATE_PARENT,
clk_mux_determine_rate_flags() and clk_core_round_rate_nolock() will
call clk_core_round_rate_nolock() with its parent clock but use the
request of the child node either directly (clk_core_round_rate_nolock())
or by copying it (clk_mux_determine_rate_flags()).

Both cases are problematic since the parent will now have a request with
the best parent fields of the child (so pointing to itself) and the
boundaries of the child as well.

clk_core_round_rate_nolock() is even worse since we would directly
modify the caller structure if the parent was ever to modify its own
parent or its parent rate, then returning to the caller a best parent
that isn't a parent of the clock we just called clk_determine_rate()
onto.

Let's create a new function that will create a new request to forward to
the parent, clk_core_forward_rate_req() and update the relevant call
sites to that new function.

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

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  58 ++++++++++++--
 drivers/clk/clk_test.c | 176 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 227 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2bbeeb2f729e..53f5f7434be0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -543,6 +543,10 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 	return now <= rate && now > best;
 }
 
+static void clk_core_init_rate_req(struct clk_core * const core,
+				   struct clk_rate_request *req,
+				   unsigned long rate);
+
 static int clk_core_round_rate_nolock(struct clk_core *core,
 				      struct clk_rate_request *req);
 
@@ -566,6 +570,24 @@ static bool clk_core_has_parent(struct clk_core *core, struct clk_core *parent)
 	return false;
 }
 
+static void
+clk_core_forward_rate_req(struct clk_core * const core,
+			  struct clk_core * const parent,
+			  struct clk_rate_request * const old_req,
+			  struct clk_rate_request *req)
+{
+	if (WARN_ON(!clk_core_has_parent(core, parent)))
+		return;
+
+	clk_core_init_rate_req(parent, req, old_req->rate);
+
+	if (req->min_rate < old_req->min_rate)
+		req->min_rate = old_req->min_rate;
+
+	if (req->max_rate > old_req->max_rate)
+		req->max_rate = old_req->max_rate;
+}
+
 int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags)
@@ -573,17 +595,19 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	struct clk_core *core = hw->core, *parent, *best_parent = NULL;
 	int i, num_parents, ret;
 	unsigned long best = 0;
-	struct clk_rate_request parent_req = *req;
 
 	/* if NO_REPARENT flag set, pass through to current parent */
 	if (core->flags & CLK_SET_RATE_NO_REPARENT) {
 		parent = core->parent;
 		if (core->flags & CLK_SET_RATE_PARENT) {
+			struct clk_rate_request parent_req;
+
 			if (!parent) {
 				req->rate = 0;
 				return 0;
 			}
 
+			clk_core_forward_rate_req(core, parent, req, &parent_req);
 			ret = clk_core_round_rate_nolock(parent, &parent_req);
 			if (ret)
 				return ret;
@@ -601,23 +625,29 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	/* find the parent that can provide the fastest rate <= rate */
 	num_parents = core->num_parents;
 	for (i = 0; i < num_parents; i++) {
+		unsigned long parent_rate;
+
 		parent = clk_core_get_parent_by_index(core, i);
 		if (!parent)
 			continue;
 
 		if (core->flags & CLK_SET_RATE_PARENT) {
-			parent_req = *req;
+			struct clk_rate_request parent_req;
+
+			clk_core_forward_rate_req(core, parent, req, &parent_req);
 			ret = clk_core_round_rate_nolock(parent, &parent_req);
 			if (ret)
 				continue;
+
+			parent_rate = parent_req.rate;
 		} else {
-			parent_req.rate = clk_core_get_rate_nolock(parent);
+			parent_rate = clk_core_get_rate_nolock(parent);
 		}
 
-		if (mux_is_better_rate(req->rate, parent_req.rate,
+		if (mux_is_better_rate(req->rate, parent_rate,
 				       best, flags)) {
 			best_parent = parent;
-			best = parent_req.rate;
+			best = parent_rate;
 		}
 	}
 
@@ -1451,6 +1481,8 @@ static bool clk_core_can_round(struct clk_core * const core)
 static int clk_core_round_rate_nolock(struct clk_core *core,
 				      struct clk_rate_request *req)
 {
+	int ret;
+
 	lockdep_assert_held(&prepare_lock);
 
 	if (!core) {
@@ -1460,8 +1492,20 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 
 	if (clk_core_can_round(core))
 		return clk_core_determine_round_nolock(core, req);
-	else if (core->flags & CLK_SET_RATE_PARENT)
-		return clk_core_round_rate_nolock(core->parent, req);
+
+	if (core->flags & CLK_SET_RATE_PARENT) {
+		struct clk_rate_request parent_req;
+
+		clk_core_forward_rate_req(core, core->parent, req, &parent_req);
+		ret = clk_core_round_rate_nolock(core->parent, &parent_req);
+		if (ret)
+			return ret;
+
+		req->best_parent_rate = parent_req.rate;
+		req->rate = parent_req.rate;
+
+		return 0;
+	}
 
 	req->rate = core->rate;
 	return 0;
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0b9088cdb8fe..23e92f2d8445 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -997,6 +997,34 @@ clk_test_single_parent_mux_set_range_disjoint_parent_last(struct kunit *test)
 	KUNIT_EXPECT_LT(test, ret, 0);
 }
 
+/*
+ * 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 then call
+ * clk_round_rate(), the boundaries of the parent are taken into
+ * account.
+ */
+static void
+clk_test_single_parent_mux_set_range_round_rate_parent_only(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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);
+
+	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);
+}
+
 /*
  * 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
@@ -1033,12 +1061,50 @@ clk_test_single_parent_mux_set_range_round_rate_child_smaller(struct kunit *test
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
 }
 
+/*
+ * Test that for a clock that can't modify its rate and with a single
+ * parent, if we set a range on the child and a more restrictive one on
+ * the parent, 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_parent_smaller(struct kunit *test)
+{
+	struct clk_single_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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 + 1000, DUMMY_CLOCK_RATE_2 - 1000);
+	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_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);
+}
+
 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_has_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),
+	KUNIT_CASE(clk_test_single_parent_mux_set_range_round_rate_parent_only),
+	KUNIT_CASE(clk_test_single_parent_mux_set_range_round_rate_parent_smaller),
 	{}
 };
 
@@ -1945,7 +2011,117 @@ static struct kunit_suite clk_range_minimize_test_suite = {
 	.test_cases = clk_range_minimize_test_cases,
 };
 
+struct clk_leaf_mux_ctx {
+	struct clk_multiple_parent_ctx mux_ctx;
+	struct clk_hw hw;
+};
+
+static int
+clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
+{
+	struct clk_leaf_mux_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->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->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw,
+				      &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_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
+{
+	struct clk_leaf_mux_ctx *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	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, for a clock that will forward any rate request to its
+ * parent, the rate request structure returned by __clk_determine_rate
+ * is sane and will be what we expect.
+ */
+static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
+{
+	struct clk_leaf_mux_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk_rate_request req;
+	unsigned long rate;
+	int ret;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
+}
+
+static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
+	{}
+};
+
+/*
+ * Test suite for a clock whose parent is a mux with multiple parents.
+ * The leaf clock has CLK_SET_RATE_PARENT, and will forward rate
+ * requests to the mux, which will then select which parent is the best
+ * fit for a given rate.
+ *
+ * These tests are supposed to exercise the behaviour of muxes, and the
+ * proper selection of parents.
+  */
+static struct kunit_suite clk_leaf_mux_set_rate_parent_test_suite = {
+	.name = "clk-leaf-mux-set-rate-parent",
+	.init = clk_leaf_mux_set_rate_parent_test_init,
+	.exit = clk_leaf_mux_set_rate_parent_test_exit,
+	.test_cases = clk_leaf_mux_set_rate_parent_test_cases,
+};
+
 kunit_test_suites(
+	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,
 	&clk_multiple_parents_mux_test_suite,
 	&clk_orphan_transparent_multiple_parent_mux_test_suite,
-- 
2.35.1

[PATCH 20/22] clk: Zero the clk_rate_request structure

[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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 53f5f7434be0..3fbd55119215 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1440,6 +1440,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);
 
-- 
2.35.1

[PATCH 21/22] clk: Test the clock pointer in clk_hw_get_name()

[Maxime Ripard]

Unlike __clk_get_name(), clk_hw_get_name() doesn't test wether passed
clk_hw pointer is NULL or not and dereferences it directly. This can
then lead to NULL pointer dereference.

Let's make sure the pointer isn't NULL before dereferencing it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3fbd55119215..8bbb6adeeead 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -269,7 +269,7 @@ EXPORT_SYMBOL_GPL(__clk_get_name);
 
 const char *clk_hw_get_name(const struct clk_hw *hw)
 {
-	return hw->core->name;
+	return !hw ? NULL : hw->core->name;
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_name);
 
-- 
2.35.1

[PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

A rate of 0 for a clock is considered an error, as evidenced by the
documentation of clk_get_rate() and the code of clk_get_rate() and
clk_core_get_rate_nolock().

The main source of that error is if the clock is supposed to have a
parent but is orphan at the moment of the call. This is likely to be
transient and solved later in the life of the system as more clocks are
registered.

The corollary is thus that if a clock is not an orphan, has a parent that
has a rate (so is not an orphan itself either) but returns a rate of 0,
something is wrong in the driver. Let's return an error in such a case.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8bbb6adeeead..e8c55678da85 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
 		rate = 0;
 	core->rate = core->req_rate = rate;
 
+	/*
+	 * If we're not an orphan clock and our parent has a rate, then
+	 * if our rate is 0, something is badly broken in recalc_rate.
+	 */
+	if (!core->orphan && (parent && parent->rate) && !core->rate) {
+		ret = -EINVAL;
+		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
+		goto out;
+	}
+
 	/*
 	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
 	 * don't get accidentally disabled when walking the orphan tree and
-- 
2.35.1

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Jerome Brunet]

On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:

> A rate of 0 for a clock is considered an error, as evidenced by the
> documentation of clk_get_rate() and the code of clk_get_rate() and
> clk_core_get_rate_nolock().
>
> The main source of that error is if the clock is supposed to have a
> parent but is orphan at the moment of the call. This is likely to be
> transient and solved later in the life of the system as more clocks are
> registered.
>
> The corollary is thus that if a clock is not an orphan, has a parent that
> has a rate (so is not an orphan itself either) but returns a rate of 0,
> something is wrong in the driver. Let's return an error in such a case.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8bbb6adeeead..e8c55678da85 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>  		rate = 0;
>  	core->rate = core->req_rate = rate;
>  
> +	/*
> +	 * If we're not an orphan clock and our parent has a rate, then
> +	 * if our rate is 0, something is badly broken in recalc_rate.
> +	 */
> +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
> +		ret = -EINVAL;
> +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> +		goto out;
> +	}
> +

As hinted in the cover letter, I don't really agree with that.

There are situations where we can't compute the rate. Getting invalid
value in the register is one reason.

You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
SoCs would be affected):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
Yes, PLL that have not been previously used (by the ROMCode or the
bootloader) tend to have the value of the divider set to 0 which in
invalid as it would result in a division by zero.

I don't think this is a bug. It is just what the HW is, an unlocked,
uninitialized PLL. There is no problem here and the PLL can remain like
that until it is needed.

IMO, whenever possible we should not put default values in the clocks
which is why I chose to leave it like that.

The PLL being unlocked, it has no rate. It is not set to have any rate.
IMO a returning a rate of 0 is valid here.

>  	/*
>  	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
>  	 * don't get accidentally disabled when walking the orphan tree and

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Fri, Apr 08, 2022 at 11:18:58AM +0200, Jerome Brunet wrote:
> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
> > A rate of 0 for a clock is considered an error, as evidenced by the
> > documentation of clk_get_rate() and the code of clk_get_rate() and
> > clk_core_get_rate_nolock().
> >
> > The main source of that error is if the clock is supposed to have a
> > parent but is orphan at the moment of the call. This is likely to be
> > transient and solved later in the life of the system as more clocks are
> > registered.
> >
> > The corollary is thus that if a clock is not an orphan, has a parent that
> > has a rate (so is not an orphan itself either) but returns a rate of 0,
> > something is wrong in the driver. Let's return an error in such a case.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8bbb6adeeead..e8c55678da85 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> >  		rate = 0;
> >  	core->rate = core->req_rate = rate;
> >  
> > +	/*
> > +	 * If we're not an orphan clock and our parent has a rate, then
> > +	 * if our rate is 0, something is badly broken in recalc_rate.
> > +	 */
> > +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
> > +		ret = -EINVAL;
> > +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> > +		goto out;
> > +	}
> > +
> 
> As hinted in the cover letter, I don't really agree with that.
> 
> There are situations where we can't compute the rate. Getting invalid
> value in the register is one reason.
> 
> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> SoCs would be affected):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> Yes, PLL that have not been previously used (by the ROMCode or the
> bootloader) tend to have the value of the divider set to 0 which in
> invalid as it would result in a division by zero.
> 
> I don't think this is a bug. It is just what the HW is, an unlocked,
> uninitialized PLL. There is no problem here and the PLL can remain like
> that until it is needed.

I think the larger issue is around the semantics of clk_get_rate(), and
especially whether we can call it without a clk_enable(), and whether
returning 0 is fine.

The (clk.h) documentation of clk_get_rate() mentions that "This is only
valid once the clock source has been enabled", and it's fairly
ambiguous. I can see how it could be interpreted as "you need to call
clk_enable() before calling clk_get_rate()", but it can also be
interpreted as "The returned rate will only be valid once clk_enable()
is called".

I think the latter is the proper interpretation though based on what the
drivers are doing, and even the CCF itself will call recalc_rate without
making sure that the clock is enabled (in __clk_core_init() for example).

Then there is the question of whether returning 0 is fine. Again
clk_get_rate() (clk.c) documentation states that "If clk is NULL then
returns 0.". This is indeed returned in case of an error condition (in
clk_get_rate() itself, but also in clk_core_get_rate_nolock()).

All the drivers I could find either assume the rate is valid, or test
whether it's 0 or not (randomly picked, but across completely different
platforms):
https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/armv7m_systick.c#L50
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/armada-8k-cpufreq.c#L74
https://elixir.bootlin.com/linux/latest/source/sound/soc/sti/uniperif_player.c#L194
https://elixir.bootlin.com/linux/latest/source/sound/soc/tegra/tegra20_i2s.c#L278

So my understanding is that the consensus is that clk_get_rate() can be
called even if the clock hasn't been enabled, and that returning 0 is
only meant to be used for errors in general, a NULL pointer according to
the documentation.

That would mean that pcie_pll_dco is buggy because it assumes that
clk_enable() is going to be called before clk_get_rate(), gp0_pll_dco
and hifi_pll_dco because they expect "someone" to call clk_set_rate()
before clk_get_rate(), and hdmi_pll_dco because it will always return 0,
unless the display driver comes around and updates it. If it never does,
or if it's not compiled in, then you're out of luck.

> IMO, whenever possible we should not put default values in the clocks
> which is why I chose to leave it like that.
>
> The PLL being unlocked, it has no rate. It is not set to have any rate.
> IMO a returning a rate of 0 is valid here.

If there's not a sensible default in the hardware already, I don't see
what the big issue is to be honest. You already kind of do that for all
the other PLL parameters with init_regs, and most drivers do that for
various stuff:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-imx6q.c#L917
https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3036.c#L448
https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-h3.c#L1157
https://elixir.bootlin.com/linux/latest/source/drivers/clk/tegra/clk-tegra20.c#L1013

If the driver needs that kind of quirks in order to make the clock
usable in itself, then it just makes sense to do that, especially if
it's to avoid breaking a generic API.

Maxime

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Jerome Brunet]

On Fri 08 Apr 2022 at 12:41, Maxime Ripard <maxime@cerno.tech> wrote:

> [[PGP Signed Part:Undecided]]
> On Fri, Apr 08, 2022 at 11:18:58AM +0200, Jerome Brunet wrote:
>> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
>> > A rate of 0 for a clock is considered an error, as evidenced by the
>> > documentation of clk_get_rate() and the code of clk_get_rate() and
>> > clk_core_get_rate_nolock().
>> >
>> > The main source of that error is if the clock is supposed to have a
>> > parent but is orphan at the moment of the call. This is likely to be
>> > transient and solved later in the life of the system as more clocks are
>> > registered.
>> >
>> > The corollary is thus that if a clock is not an orphan, has a parent that
>> > has a rate (so is not an orphan itself either) but returns a rate of 0,
>> > something is wrong in the driver. Let's return an error in such a case.
>> >
>> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> > ---
>> >  drivers/clk/clk.c | 10 ++++++++++
>> >  1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> > index 8bbb6adeeead..e8c55678da85 100644
>> > --- a/drivers/clk/clk.c
>> > +++ b/drivers/clk/clk.c
>> > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>> >  		rate = 0;
>> >  	core->rate = core->req_rate = rate;
>> >  
>> > +	/*
>> > +	 * If we're not an orphan clock and our parent has a rate, then
>> > +	 * if our rate is 0, something is badly broken in recalc_rate.
>> > +	 */
>> > +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
>> > +		ret = -EINVAL;
>> > +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
>> > +		goto out;
>> > +	}
>> > +
>> 
>> As hinted in the cover letter, I don't really agree with that.
>> 
>> There are situations where we can't compute the rate. Getting invalid
>> value in the register is one reason.
>> 
>> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
>> SoCs would be affected):
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
>> Yes, PLL that have not been previously used (by the ROMCode or the
>> bootloader) tend to have the value of the divider set to 0 which in
>> invalid as it would result in a division by zero.
>> 
>> I don't think this is a bug. It is just what the HW is, an unlocked,
>> uninitialized PLL. There is no problem here and the PLL can remain like
>> that until it is needed.
>
> I think the larger issue is around the semantics of clk_get_rate(), and
> especially whether we can call it without a clk_enable(), and whether
> returning 0 is fine.
>
> The (clk.h) documentation of clk_get_rate() mentions that "This is only
> valid once the clock source has been enabled", and it's fairly
> ambiguous. I can see how it could be interpreted as "you need to call
> clk_enable() before calling clk_get_rate()", but it can also be
> interpreted as "The returned rate will only be valid once clk_enable()
> is called".
>
> I think the latter is the proper interpretation though based on what the
> drivers are doing, and even the CCF itself will call recalc_rate without
> making sure that the clock is enabled (in __clk_core_init() for example).
>
> Then there is the question of whether returning 0 is fine. Again
> clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> returns 0.". This is indeed returned in case of an error condition (in
> clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
>
> All the drivers I could find either assume the rate is valid, or test
> whether it's 0 or not (randomly picked, but across completely different
> platforms):
> https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/armv7m_systick.c#L50
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/armada-8k-cpufreq.c#L74
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sti/uniperif_player.c#L194
> https://elixir.bootlin.com/linux/latest/source/sound/soc/tegra/tegra20_i2s.c#L278
>
> So my understanding is that the consensus is that clk_get_rate() can be
> called even if the clock hasn't been enabled, and that returning 0 is
> only meant to be used for errors in general, a NULL pointer according to
> the documentation.
>
> That would mean that pcie_pll_dco is buggy because it assumes that
> clk_enable()

This one indeed does everything in the enable and I could agree it is
fishy, but since it supports only a single rate I don't think it is a
problem. Even if it had a proper set_rate(), it would not change your
problem since it would still return 0 until some consumer actually needs
its parameter to change.

> is going to be called before clk_get_rate(), gp0_pll_dco
> and hifi_pll_dco because they expect "someone" to call clk_set_rate()
> before clk_get_rate(),

No, they don't expect anything. They will return 0 until they are set
with a an actual rate. I don't think returning 0 should be
problem and it has not been so far.

I understand the ambiguity you mentioned above. If the framework decides
it is clearly forbidden to return 0, we'll change.

Still I don't think it would be wise. What are the alternative if you
can't compute a rate ? return 1 ? This looks aweful to me. At least 0 is
a clear indication that the clock is not in a working state.

> and hdmi_pll_dco because it will always return 0,

It is a read-only clock - whatever we do in CCF, it is not going to
change. CCF has always supported RO clocks.

> unless the display driver comes around and updates it. If it never does,
> or if it's not compiled in, then you're out of luck.

I'm all for managing the display clocks from CCF but it has proved tricky
so far. Maybe it will someday.

Being a read-only clock, the value is what it is and CCF should
deal with it gracefully. It has so far.

If the driver actually managing the clock is not compiled in, then the
clock will never be set, and it should not be a problem either.

>
>> IMO, whenever possible we should not put default values in the clocks
>> which is why I chose to leave it like that.
>>
>> The PLL being unlocked, it has no rate. It is not set to have any rate.
>> IMO a returning a rate of 0 is valid here.
>
> If there's not a sensible default in the hardware already, I don't see
> what the big issue is to be honest. You already kind of do that for all
> the other PLL parameters with init_regs

Those initial parameters are "magic" analog setting which don't have an
impact on the rate setting. The initial rate of the clock is never set
by the clock driver on purpose.

> , and most drivers do that for
> various stuff:
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-imx6q.c#L917
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3036.c#L448
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-h3.c#L1157
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/tegra/clk-tegra20.c#L1013

It is done by other drivers or controllers, yes. It does not make it
right (again, it is just my opinion). Rate should never be set by the
clock driver or the clock controller - Those should just implement what
consumer wants. I would agree it sometimes proves tricky to hold onto
this.

Taking one of the example above:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550

I think it would be better to have an "assigned-clock" on the related
PLL in the USB node of the platform DT. That way the PLL is set when
needed.

If we go down the road of "others are doing it, so why not ?", I think Marek
initial regression report mentioned Exynos too ;) 

>
> If the driver needs that kind of quirks in order to make the clock
> usable in itself, then it just makes sense to do that, especially if
> it's to avoid breaking a generic API.

As it is the clock are usable and it did not break anything so far.

I have no problem updating the drivers if need be. I do have a problem
with the framework changing and requiring the clock driver to set an
initial rate to make it happy.

>
> Maxime
>
> [[End of PGP Signed Part]]

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Marek Szyprowski]

Hi All,

On 08.04.2022 11:18, Jerome Brunet wrote:
> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
>
>> A rate of 0 for a clock is considered an error, as evidenced by the
>> documentation of clk_get_rate() and the code of clk_get_rate() and
>> clk_core_get_rate_nolock().
>>
>> The main source of that error is if the clock is supposed to have a
>> parent but is orphan at the moment of the call. This is likely to be
>> transient and solved later in the life of the system as more clocks are
>> registered.
>>
>> The corollary is thus that if a clock is not an orphan, has a parent that
>> has a rate (so is not an orphan itself either) but returns a rate of 0,
>> something is wrong in the driver. Let's return an error in such a case.
>>
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> ---
>>   drivers/clk/clk.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 8bbb6adeeead..e8c55678da85 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>>   		rate = 0;
>>   	core->rate = core->req_rate = rate;
>>   
>> +	/*
>> +	 * If we're not an orphan clock and our parent has a rate, then
>> +	 * if our rate is 0, something is badly broken in recalc_rate.
>> +	 */
>> +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
>> +		ret = -EINVAL;
>> +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
>> +		goto out;
>> +	}
>> +
> As hinted in the cover letter, I don't really agree with that.
>
> There are situations where we can't compute the rate. Getting invalid
> value in the register is one reason.
>
> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> SoCs would be affected):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> Yes, PLL that have not been previously used (by the ROMCode or the
> bootloader) tend to have the value of the divider set to 0 which in
> invalid as it would result in a division by zero.
>
> I don't think this is a bug. It is just what the HW is, an unlocked,
> uninitialized PLL. There is no problem here and the PLL can remain like
> that until it is needed.
>
> IMO, whenever possible we should not put default values in the clocks
> which is why I chose to leave it like that.
>
> The PLL being unlocked, it has no rate. It is not set to have any rate.
> IMO a returning a rate of 0 is valid here.

I agree that it should be possible to register so called unconfigured 
clock, which doesn't have any rate set yet. It might be used later by 
some drivers, which will do the whole initialization (set rate and or 
parents). I also wonder why the above patch triggers warning on the 
Amlogic Socs, while on Exynos I still have some unused clocks with 0 
rate registered properly.

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

Hi,

On Fri, Apr 08, 2022 at 02:17:55PM +0200, Marek Szyprowski wrote:
> On 08.04.2022 11:18, Jerome Brunet wrote:
> > On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> >> A rate of 0 for a clock is considered an error, as evidenced by the
> >> documentation of clk_get_rate() and the code of clk_get_rate() and
> >> clk_core_get_rate_nolock().
> >>
> >> The main source of that error is if the clock is supposed to have a
> >> parent but is orphan at the moment of the call. This is likely to be
> >> transient and solved later in the life of the system as more clocks are
> >> registered.
> >>
> >> The corollary is thus that if a clock is not an orphan, has a parent that
> >> has a rate (so is not an orphan itself either) but returns a rate of 0,
> >> something is wrong in the driver. Let's return an error in such a case.
> >>
> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >> ---
> >>   drivers/clk/clk.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index 8bbb6adeeead..e8c55678da85 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> >>   		rate = 0;
> >>   	core->rate = core->req_rate = rate;
> >>   
> >> +	/*
> >> +	 * If we're not an orphan clock and our parent has a rate, then
> >> +	 * if our rate is 0, something is badly broken in recalc_rate.
> >> +	 */
> >> +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
> >> +		ret = -EINVAL;
> >> +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> >> +		goto out;
> >> +	}
> >> +
> > As hinted in the cover letter, I don't really agree with that.
> >
> > There are situations where we can't compute the rate. Getting invalid
> > value in the register is one reason.
> >
> > You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> > SoCs would be affected):
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> > Yes, PLL that have not been previously used (by the ROMCode or the
> > bootloader) tend to have the value of the divider set to 0 which in
> > invalid as it would result in a division by zero.
> >
> > I don't think this is a bug. It is just what the HW is, an unlocked,
> > uninitialized PLL. There is no problem here and the PLL can remain like
> > that until it is needed.
> >
> > IMO, whenever possible we should not put default values in the clocks
> > which is why I chose to leave it like that.
> >
> > The PLL being unlocked, it has no rate. It is not set to have any rate.
> > IMO a returning a rate of 0 is valid here.
> 
> I agree that it should be possible to register so called unconfigured 
> clock, which doesn't have any rate set yet. It might be used later by 
> some drivers, which will do the whole initialization (set rate and or 
> parents).

I think we should differentiate between the clock being fine from the
CCF point of view, and from the device point of view. I'm arguing about
the former, but the latter should definitely be up to other drivers to
set it up so that their hardware work.

But in both cases, we should return something valid under the CCF
semantics.

> I also wonder why the above patch triggers warning on the Amlogic
> Socs, while on Exynos I still have some unused clocks with 0 rate
> registered properly.

Most likely the clocks that return a rate of 0 are orphans (ie, their
parent isn't registered yet).

We can't expect all the parents to be registered before their children,
so this is fine, and that patch doesn't break it.

Did you test this series for exynos and meson then? Could you give your
tested-by if you did? :)

Maxime

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Fri, Apr 08, 2022 at 01:24:59PM +0200, Jerome Brunet wrote:
> On Fri 08 Apr 2022 at 12:41, Maxime Ripard <maxime@cerno.tech> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Fri, Apr 08, 2022 at 11:18:58AM +0200, Jerome Brunet wrote:
> >> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
> >> > A rate of 0 for a clock is considered an error, as evidenced by the
> >> > documentation of clk_get_rate() and the code of clk_get_rate() and
> >> > clk_core_get_rate_nolock().
> >> >
> >> > The main source of that error is if the clock is supposed to have a
> >> > parent but is orphan at the moment of the call. This is likely to be
> >> > transient and solved later in the life of the system as more clocks are
> >> > registered.
> >> >
> >> > The corollary is thus that if a clock is not an orphan, has a parent that
> >> > has a rate (so is not an orphan itself either) but returns a rate of 0,
> >> > something is wrong in the driver. Let's return an error in such a case.
> >> >
> >> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >> > ---
> >> >  drivers/clk/clk.c | 10 ++++++++++
> >> >  1 file changed, 10 insertions(+)
> >> >
> >> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> > index 8bbb6adeeead..e8c55678da85 100644
> >> > --- a/drivers/clk/clk.c
> >> > +++ b/drivers/clk/clk.c
> >> > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> >> >  		rate = 0;
> >> >  	core->rate = core->req_rate = rate;
> >> >  
> >> > +	/*
> >> > +	 * If we're not an orphan clock and our parent has a rate, then
> >> > +	 * if our rate is 0, something is badly broken in recalc_rate.
> >> > +	 */
> >> > +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
> >> > +		ret = -EINVAL;
> >> > +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> >> > +		goto out;
> >> > +	}
> >> > +
> >> 
> >> As hinted in the cover letter, I don't really agree with that.
> >> 
> >> There are situations where we can't compute the rate. Getting invalid
> >> value in the register is one reason.
> >> 
> >> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> >> SoCs would be affected):
> >> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> >> Yes, PLL that have not been previously used (by the ROMCode or the
> >> bootloader) tend to have the value of the divider set to 0 which in
> >> invalid as it would result in a division by zero.
> >> 
> >> I don't think this is a bug. It is just what the HW is, an unlocked,
> >> uninitialized PLL. There is no problem here and the PLL can remain like
> >> that until it is needed.
> >
> > I think the larger issue is around the semantics of clk_get_rate(), and
> > especially whether we can call it without a clk_enable(), and whether
> > returning 0 is fine.
> >
> > The (clk.h) documentation of clk_get_rate() mentions that "This is only
> > valid once the clock source has been enabled", and it's fairly
> > ambiguous. I can see how it could be interpreted as "you need to call
> > clk_enable() before calling clk_get_rate()", but it can also be
> > interpreted as "The returned rate will only be valid once clk_enable()
> > is called".
> >
> > I think the latter is the proper interpretation though based on what the
> > drivers are doing, and even the CCF itself will call recalc_rate without
> > making sure that the clock is enabled (in __clk_core_init() for example).
> >
> > Then there is the question of whether returning 0 is fine. Again
> > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > returns 0.". This is indeed returned in case of an error condition (in
> > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> >
> > All the drivers I could find either assume the rate is valid, or test
> > whether it's 0 or not (randomly picked, but across completely different
> > platforms):
> > https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/armv7m_systick.c#L50
> > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/armada-8k-cpufreq.c#L74
> > https://elixir.bootlin.com/linux/latest/source/sound/soc/sti/uniperif_player.c#L194
> > https://elixir.bootlin.com/linux/latest/source/sound/soc/tegra/tegra20_i2s.c#L278
> >
> > So my understanding is that the consensus is that clk_get_rate() can be
> > called even if the clock hasn't been enabled, and that returning 0 is
> > only meant to be used for errors in general, a NULL pointer according to
> > the documentation.
> >
> > That would mean that pcie_pll_dco is buggy because it assumes that
> > clk_enable()
> 
> This one indeed does everything in the enable and I could agree it is
> fishy, but since it supports only a single rate I don't think it is a
> problem. Even if it had a proper set_rate(), it would not change your
> problem since it would still return 0 until some consumer actually needs
> its parameter to change.
> 
> > is going to be called before clk_get_rate(), gp0_pll_dco
> > and hifi_pll_dco because they expect "someone" to call clk_set_rate()
> > before clk_get_rate(),
> 
> No, they don't expect anything. They will return 0 until they are set
> with a an actual rate. I don't think returning 0 should be
> problem and it has not been so far.

So, if I was to rephrase, gp0_pll_dco and hifi_pll_dco expect someone to
call clk_set_rate() before clk_get_rate() to have it return anything
other than 0.

> I understand the ambiguity you mentioned above. If the framework decides
> it is clearly forbidden to return 0, we'll change.
> 
> Still I don't think it would be wise. What are the alternative if you
> can't compute a rate ? return 1 ? This looks aweful to me. At least 0 is
> a clear indication that the clock is not in a working state.

No, the alternative would be to initialize the clock to something
sensible before registering it (or in init).

> > and hdmi_pll_dco because it will always return 0,
> 
> It is a read-only clock - whatever we do in CCF, it is not going to
> change. CCF has always supported RO clocks.

Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
useful in anyway, it should report its current rate. Read-only or not.

> > unless the display driver comes around and updates it. If it never does,
> > or if it's not compiled in, then you're out of luck.
> 
> I'm all for managing the display clocks from CCF but it has proved tricky
> so far. Maybe it will someday.
> 
> Being a read-only clock, the value is what it is and CCF should
> deal with it gracefully. It has so far.
>
> If the driver actually managing the clock is not compiled in, then the
> clock will never be set, and it should not be a problem either.

Again, it depends on what you expect from clk_get_rate(). If it can only
report a rate of 0 on error, then it's definitely a problem.

And it's not because it was done before that it wasn't just as
problematic then.

> >> IMO, whenever possible we should not put default values in the clocks
> >> which is why I chose to leave it like that.
> >>
> >> The PLL being unlocked, it has no rate. It is not set to have any rate.
> >> IMO a returning a rate of 0 is valid here.
> >
> > If there's not a sensible default in the hardware already, I don't see
> > what the big issue is to be honest. You already kind of do that for all
> > the other PLL parameters with init_regs
> 
> Those initial parameters are "magic" analog setting which don't have an
> impact on the rate setting. The initial rate of the clock is never set
> by the clock driver on purpose.

It's still fundamentally the same: whatever is there at boot isn't good
enough, so you change it to have a somewhat sensible default.

> > , and most drivers do that for
> > various stuff:
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-imx6q.c#L917
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3036.c#L448
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-h3.c#L1157
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/tegra/clk-tegra20.c#L1013
> 
> It is done by other drivers or controllers, yes. It does not make it
> right (again, it is just my opinion). Rate should never be set by the
> clock driver or the clock controller - Those should just implement what
> consumer wants. I would agree it sometimes proves tricky to hold onto
> this.
> 
> Taking one of the example above:
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
> 
> I think it would be better to have an "assigned-clock" on the related
> PLL in the USB node of the platform DT. That way the PLL is set when
> needed.

Both are valid. Assigned-clocks is arguably more fragile, but that's not
really the point.

> If we go down the road of "others are doing it, so why not ?", I think Marek
> initial regression report mentioned Exynos too ;) 

Yes, he did mention exynos as well, but the issue was entirely
different.

Exynos had the issue that req_rate wasn't updated whenever a clock
wasn't orphan anymore because it changed parent. It's fixed in patch 10.

Out of the drivers I tested this week (sunxi-ng, exynos, omap3, qcom,
imx6, imx8 and g12b), only meson is in this case.

> > If the driver needs that kind of quirks in order to make the clock
> > usable in itself, then it just makes sense to do that, especially if
> > it's to avoid breaking a generic API.
> 
> As it is the clock are usable and it did not break anything so far.

Anything but the abstraction.

> I have no problem updating the drivers if need be. I do have a problem
> with the framework changing and requiring the clock driver to set an
> initial rate to make it happy.

I mean, the alternative is changing the semantics of clk_get_rate(), and
all the call sites. Fixing one inconsistent driver definitely seems
preferable.

Maxime

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Marek Szyprowski]

Hi Maxime,

On 08.04.2022 14:25, Maxime Ripard wrote:
> On Fri, Apr 08, 2022 at 02:17:55PM +0200, Marek Szyprowski wrote:
>> On 08.04.2022 11:18, Jerome Brunet wrote:
>>> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>>> A rate of 0 for a clock is considered an error, as evidenced by the
>>>> documentation of clk_get_rate() and the code of clk_get_rate() and
>>>> clk_core_get_rate_nolock().
>>>>
>>>> The main source of that error is if the clock is supposed to have a
>>>> parent but is orphan at the moment of the call. This is likely to be
>>>> transient and solved later in the life of the system as more clocks are
>>>> registered.
>>>>
>>>> The corollary is thus that if a clock is not an orphan, has a parent that
>>>> has a rate (so is not an orphan itself either) but returns a rate of 0,
>>>> something is wrong in the driver. Let's return an error in such a case.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>>> ---
>>>>    drivers/clk/clk.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>> index 8bbb6adeeead..e8c55678da85 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>>>>    		rate = 0;
>>>>    	core->rate = core->req_rate = rate;
>>>>    
>>>> +	/*
>>>> +	 * If we're not an orphan clock and our parent has a rate, then
>>>> +	 * if our rate is 0, something is badly broken in recalc_rate.
>>>> +	 */
>>>> +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
>>>> +		ret = -EINVAL;
>>>> +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
>>>> +		goto out;
>>>> +	}
>>>> +
>>> As hinted in the cover letter, I don't really agree with that.
>>>
>>> There are situations where we can't compute the rate. Getting invalid
>>> value in the register is one reason.
>>>
>>> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
>>> SoCs would be affected):
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
>>> Yes, PLL that have not been previously used (by the ROMCode or the
>>> bootloader) tend to have the value of the divider set to 0 which in
>>> invalid as it would result in a division by zero.
>>>
>>> I don't think this is a bug. It is just what the HW is, an unlocked,
>>> uninitialized PLL. There is no problem here and the PLL can remain like
>>> that until it is needed.
>>>
>>> IMO, whenever possible we should not put default values in the clocks
>>> which is why I chose to leave it like that.
>>>
>>> The PLL being unlocked, it has no rate. It is not set to have any rate.
>>> IMO a returning a rate of 0 is valid here.
>> I agree that it should be possible to register so called unconfigured
>> clock, which doesn't have any rate set yet. It might be used later by
>> some drivers, which will do the whole initialization (set rate and or
>> parents).
> I think we should differentiate between the clock being fine from the
> CCF point of view, and from the device point of view. I'm arguing about
> the former, but the latter should definitely be up to other drivers to
> set it up so that their hardware work.
>
> But in both cases, we should return something valid under the CCF
> semantics.
>
>> I also wonder why the above patch triggers warning on the Amlogic
>> Socs, while on Exynos I still have some unused clocks with 0 rate
>> registered properly.
> Most likely the clocks that return a rate of 0 are orphans (ie, their
> parent isn't registered yet).
Right.
> We can't expect all the parents to be registered before their children,
> so this is fine, and that patch doesn't break it.
>
> Did you test this series for exynos and meson then? Could you give your
> tested-by if you did? :)

Yes, I've tested the whole set. All my Exynos based test boards work 
fine. All my Meson based boards fails to boot due to this patch, thus my 
comment. Feel free to add to all but the last patch:

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

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Jerome Brunet]

On Fri 08 Apr 2022 at 14:55, Maxime Ripard <maxime@cerno.tech> wrote:

>> 
>> No, they don't expect anything. They will return 0 until they are set
>> with a an actual rate. I don't think returning 0 should be
>> problem and it has not been so far.
>
> So, if I was to rephrase, gp0_pll_dco and hifi_pll_dco expect someone to
> call clk_set_rate() before clk_get_rate() to have it return anything
> other than 0.

Yes. It has no rate. I won't change until something make it so

>
>> I understand the ambiguity you mentioned above. If the framework decides
>> it is clearly forbidden to return 0, we'll change.
>> 
>> Still I don't think it would be wise. What are the alternative if you
>> can't compute a rate ? return 1 ? This looks aweful to me. At least 0 is
>> a clear indication that the clock is not in a working state.
>
> No, the alternative would be to initialize the clock to something
> sensible before registering it (or in init).

Well, I disagree :/

>
>> > and hdmi_pll_dco because it will always return 0,
>> 
>> It is a read-only clock - whatever we do in CCF, it is not going to
>> change. CCF has always supported RO clocks.
>
> Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
> useful in anyway, it should report its current rate. Read-only or not.
>

It does report its rate, it does not have any.
... and It would pretty weird to initialize a read-only clock.

>> > unless the display driver comes around and updates it. If it never does,
>> > or if it's not compiled in, then you're out of luck.
>> 
>> I'm all for managing the display clocks from CCF but it has proved tricky
>> so far. Maybe it will someday.
>> 
>> Being a read-only clock, the value is what it is and CCF should
>> deal with it gracefully. It has so far.
>>
>> If the driver actually managing the clock is not compiled in, then the
>> clock will never be set, and it should not be a problem either.
>
> Again, it depends on what you expect from clk_get_rate(). If it can only
> report a rate of 0 on error, then it's definitely a problem.

Agreed, it depends on what you expect from clk_get_rate().
Still when something does not oscillate, the rate is 0.

If a driver call clk_get_rate() on an uninitialized, unlocked PLL, I
think returning 0 makes sense.

>
> And it's not because it was done before that it wasn't just as
> problematic then.
>
>> >> IMO, whenever possible we should not put default values in the clocks
>> >> which is why I chose to leave it like that.
>> >>
>> >> The PLL being unlocked, it has no rate. It is not set to have any rate.
>> >> IMO a returning a rate of 0 is valid here.
>> >
>> > If there's not a sensible default in the hardware already, I don't see
>> > what the big issue is to be honest. You already kind of do that for all
>> > the other PLL parameters with init_regs
>> 
>> Those initial parameters are "magic" analog setting which don't have an
>> impact on the rate setting. The initial rate of the clock is never set
>> by the clock driver on purpose.
>
> It's still fundamentally the same: whatever is there at boot isn't good
> enough, so you change it to have a somewhat sensible default.

If you don't need it, no.

>
>> > , and most drivers do that for
>> > various stuff:
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-imx6q.c#L917
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3036.c#L448
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-h3.c#L1157
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/tegra/clk-tegra20.c#L1013
>> 
>> It is done by other drivers or controllers, yes. It does not make it
>> right (again, it is just my opinion). Rate should never be set by the
>> clock driver or the clock controller - Those should just implement what
>> consumer wants. I would agree it sometimes proves tricky to hold onto
>> this.
>> 
>> Taking one of the example above:
>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
>> 
>> I think it would be better to have an "assigned-clock" on the related
>> PLL in the USB node of the platform DT. That way the PLL is set when
>> needed.
>
> Both are valid. Assigned-clocks is arguably more fragile, but that's not
> really the point.
>
>> If we go down the road of "others are doing it, so why not ?", I think Marek
>> initial regression report mentioned Exynos too ;) 
>
> Yes, he did mention exynos as well, but the issue was entirely
> different.
>
> Exynos had the issue that req_rate wasn't updated whenever a clock
> wasn't orphan anymore because it changed parent. It's fixed in patch 10.
>
> Out of the drivers I tested this week (sunxi-ng, exynos, omap3, qcom,
> imx6, imx8 and g12b), only meson is in this case.
>
>> > If the driver needs that kind of quirks in order to make the clock
>> > usable in itself, then it just makes sense to do that, especially if
>> > it's to avoid breaking a generic API.
>> 
>> As it is the clock are usable and it did not break anything so far.
>
> Anything but the abstraction.
>
>> I have no problem updating the drivers if need be. I do have a problem
>> with the framework changing and requiring the clock driver to set an
>> initial rate to make it happy.
>
> I mean, the alternative is changing the semantics of clk_get_rate(), and
> all the call sites. Fixing one inconsistent driver definitely seems
> preferable.
>
> Maxime
>
> [[End of PGP Signed Part]]

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Fri, Apr 08, 2022 at 04:48:08PM +0200, Jerome Brunet wrote:
> >> > and hdmi_pll_dco because it will always return 0,
> >> 
> >> It is a read-only clock - whatever we do in CCF, it is not going to
> >> change. CCF has always supported RO clocks.
> >
> > Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
> > useful in anyway, it should report its current rate. Read-only or not.
> >
> 
> It does report its rate, it does not have any.
> ... and It would pretty weird to initialize a read-only clock.

The KMS driver doesn't seem to have a problem with that.

> >> > unless the display driver comes around and updates it. If it never does,
> >> > or if it's not compiled in, then you're out of luck.
> >> 
> >> I'm all for managing the display clocks from CCF but it has proved tricky
> >> so far. Maybe it will someday.
> >> 
> >> Being a read-only clock, the value is what it is and CCF should
> >> deal with it gracefully. It has so far.
> >>
> >> If the driver actually managing the clock is not compiled in, then the
> >> clock will never be set, and it should not be a problem either.
> >
> > Again, it depends on what you expect from clk_get_rate(). If it can only
> > report a rate of 0 on error, then it's definitely a problem.
> 
> Agreed, it depends on what you expect from clk_get_rate().
> Still when something does not oscillate, the rate is 0.
> 
> If a driver call clk_get_rate() on an uninitialized, unlocked PLL, I
> think returning 0 makes sense.

You're confusing two things: the rate output by the hardware, and what
the CCF needs to return. We're discussing the latter here, a software
construct. It models the hardware, but it doesn't have to be true to the
hardware.

And even the meson driver doesn't follow what you're claiming to the
letter and is inconsistent with what you're saying. Any disabled gate
will also have a hardware rate of 0. Yet, it doesn't return 0 in such a
case. And no one does, because clk_get_rate() isn't supposed to return
the actual rate in hardware at the moment. It's supposed to return what
would be the rate if it was enabled.

> > And it's not because it was done before that it wasn't just as
> > problematic then.
> >
> >> >> IMO, whenever possible we should not put default values in the clocks
> >> >> which is why I chose to leave it like that.
> >> >>
> >> >> The PLL being unlocked, it has no rate. It is not set to have any rate.
> >> >> IMO a returning a rate of 0 is valid here.
> >> >
> >> > If there's not a sensible default in the hardware already, I don't see
> >> > what the big issue is to be honest. You already kind of do that for all
> >> > the other PLL parameters with init_regs
> >> 
> >> Those initial parameters are "magic" analog setting which don't have an
> >> impact on the rate setting. The initial rate of the clock is never set
> >> by the clock driver on purpose.
> >
> > It's still fundamentally the same: whatever is there at boot isn't good
> > enough, so you change it to have a somewhat sensible default.
> 
> If you don't need it, no.

I mean, I provided multiple examples that the meson driver is being
inconsistent with both the CCF documentation and the expected usage of
the CCF consumers. And I suggested solutions to fix this inconsistency
both here and on IRC to Neil and you.

The only argument you provided so far is that you don't like or want it.
I'm sorry you feel this way, but it doesn't change either the consensus
or the documentation that every other driver and consumer is following.
Nor does it provide any solution.

In the grand scheme of things, I don't care, if you want to have your
own conventions a policies, go ahead. Just don't expect me to debug
whatever is going on next time it falls apart.

Maxime

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

Re: [PATCH 00/22] clk: More clock rate fixes and tests

[Yassine Oudjana]

On Friday, April 8th, 2022 at 1:10 PM, Maxime Ripard <maxime@cerno.tech> wrote:
> 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.
>
> 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.
>
> The last patch will probably prove to be controversial, but it can be
> left out without affecting the rest of the series. It will affect the
> meson clock drivers for the g12b SoC at least. It stems from the
> realisation that on the g12b, 4 PLLs (and thus all their children) have
> a rate of 0, pretty much forever which feels like a bug considering the
> rate 0 is used as an error in most places.
>
> Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> return 0 if the diviser of the PLL is set to 0 in the register, but:
>
> - pcie_pll_dco has a few registers to initialize set in
> g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
> hook and will instead call it in the enable hook. This looks like a
> bug and could be easily fixed by adding that init hook.
>
> - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
> the initialisation of their register (g12a_gp0_init_regs and
> g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
> long as set_rate isn't called, that field is going to remain at 0. And
> since all but one users don't have CLK_SET_RATE_PARENT, this is
> still fairly unlikely.
>
> - hdmi_pll_dco doesn't set the n field in the initialisation either,
> but also doesn't have a set_rate implementation. Thus, if the
> bootloader doesn't set it, this clock and all its children will
> always report a rate of 0, even if the clock is functional.
>
> During the discussion with amlogic clock maintainers, we kind of ended
> up on a disagreement of whether returning 0 was ok or not, hence the
> expected controversy :)
>
> Let me know what you think,
> Maxime
>
> Maxime Ripard (22):
> clk: Drop the rate range on clk_put()
> 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: Fix clk_get_parent() documentation
> clk: Set req_rate on reparenting
> clk: Skip set_rate_range if our clock is orphan
> clk: Add our request boundaries in clk_core_init_rate_req
> clk: Change clk_core_init_rate_req prototype
> clk: Introduce clk_hw_init_rate_request()
> clk: Add missing clk_core_init_rate_req calls
> clk: Remove redundant clk_core_init_rate_req() call
> clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
> clk: Introduce clk_core_has_parent()
> clk: Stop forwarding clk_rate_requests to the parent
> clk: Zero the clk_rate_request structure
> clk: Test the clock pointer in clk_hw_get_name()
> clk: Prevent a clock without a rate to register
>
> drivers/clk/clk.c | 239 +++++--
> drivers/clk/clk_test.c | 1256 +++++++++++++++++++++++++++++++++-
> include/linux/clk-provider.h | 6 +
> include/linux/clk.h | 5 +-
> 4 files changed, 1439 insertions(+), 67 deletions(-)
>
> --
> 2.35.1

It appears that this series breaks mmcc-msm8996:

[    8.713810] mmpll2_early: recalc_rate returned a null rate
[    8.713864] mmcc-msm8996: probe of 8c0000.clock-controller failed with error -22

I haven't yet tried to figure out the exact patch that causes
this or how it does. I just wanted to report it as soon as possible.

Re: (EXT) [PATCH 00/22] clk: More clock rate fixes and tests

[Alexander Stein]

Hello Maxime,

Am Freitag, 8. April 2022, 11:10:15 CEST schrieb Maxime Ripard:
> 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.
> 
> 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.
> 
> The last patch will probably prove to be controversial, but it can be
> left out without affecting the rest of the series. It will affect the
> meson clock drivers for the g12b SoC at least. It stems from the
> realisation that on the g12b, 4 PLLs (and thus all their children) have
> a rate of 0, pretty much forever which feels like a bug considering the
> rate 0 is used as an error in most places.
> 
> Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> return 0 if the diviser of the PLL is set to 0 in the register, but:
> 
>   - pcie_pll_dco has a few registers to initialize set in
>     g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
>     hook and will instead call it in the enable hook. This looks like a
>     bug and could be easily fixed by adding that init hook.
> 
>   - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
>     the initialisation of their register (g12a_gp0_init_regs and
>     g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
>     long as set_rate isn't called, that field is going to remain at 0. And
>     since all but one users don't have CLK_SET_RATE_PARENT, this is
>     still fairly unlikely.
> 
>   - hdmi_pll_dco doesn't set the n field in the initialisation either,
>     but also doesn't have a set_rate implementation. Thus, if the
>     bootloader doesn't set it, this clock and all its children will
>     always report a rate of 0, even if the clock is functional.
> 
> During the discussion with amlogic clock maintainers, we kind of ended
> up on a disagreement of whether returning 0 was ok or not, hence the
> expected controversy :)
> 
> Let me know what you think,
> Maxime
> 
> Maxime Ripard (22):
>   clk: Drop the rate range on clk_put()
>   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: Fix clk_get_parent() documentation
>   clk: Set req_rate on reparenting
>   clk: Skip set_rate_range if our clock is orphan
>   clk: Add our request boundaries in clk_core_init_rate_req
>   clk: Change clk_core_init_rate_req prototype
>   clk: Introduce clk_hw_init_rate_request()
>   clk: Add missing clk_core_init_rate_req calls
>   clk: Remove redundant clk_core_init_rate_req() call
>   clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
>   clk: Introduce clk_core_has_parent()
>   clk: Stop forwarding clk_rate_requests to the parent
>   clk: Zero the clk_rate_request structure
>   clk: Test the clock pointer in clk_hw_get_name()
>   clk: Prevent a clock without a rate to register
> 
>  drivers/clk/clk.c            |  239 +++++--
>  drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
>  include/linux/clk-provider.h |    6 +
>  include/linux/clk.h          |    5 +-
>  4 files changed, 1439 insertions(+), 67 deletions(-)

Thanks for another round of patches.
On top my patchset this one does *not* break my imx8mp based setup. Booting is 
as usual without any changes in dmesg.
Given this patch set I suspect the other one on github you sent me on Thursday 
is obsolete now.

Best regards,
Alexander

Re: (EXT) [PATCH 00/22] clk: More clock rate fixes and tests

[Alexander Stein]

Am Montag, 11. April 2022, 08:25:31 CEST schrieb Alexander Stein:
> Hello Maxime,
> 
> Am Freitag, 8. April 2022, 11:10:15 CEST schrieb Maxime Ripard:
> > 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.
> > 
> > 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.
> > 
> > The last patch will probably prove to be controversial, but it can be
> > left out without affecting the rest of the series. It will affect the
> > meson clock drivers for the g12b SoC at least. It stems from the
> > realisation that on the g12b, 4 PLLs (and thus all their children) have
> > a rate of 0, pretty much forever which feels like a bug considering the
> > rate 0 is used as an error in most places.
> > 
> > Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> > 
> > return 0 if the diviser of the PLL is set to 0 in the register, but:
> >   - pcie_pll_dco has a few registers to initialize set in
> >   
> >     g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
> >     hook and will instead call it in the enable hook. This looks like a
> >     bug and could be easily fixed by adding that init hook.
> >   
> >   - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
> >   
> >     the initialisation of their register (g12a_gp0_init_regs and
> >     g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
> >     long as set_rate isn't called, that field is going to remain at 0. And
> >     since all but one users don't have CLK_SET_RATE_PARENT, this is
> >     still fairly unlikely.
> >   
> >   - hdmi_pll_dco doesn't set the n field in the initialisation either,
> >   
> >     but also doesn't have a set_rate implementation. Thus, if the
> >     bootloader doesn't set it, this clock and all its children will
> >     always report a rate of 0, even if the clock is functional.
> > 
> > During the discussion with amlogic clock maintainers, we kind of ended
> > up on a disagreement of whether returning 0 was ok or not, hence the
> > expected controversy :)
> > 
> > Let me know what you think,
> > Maxime
> > 
> > Maxime Ripard (22):
> >   clk: Drop the rate range on clk_put()
> >   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: Fix clk_get_parent() documentation
> >   clk: Set req_rate on reparenting
> >   clk: Skip set_rate_range if our clock is orphan
> >   clk: Add our request boundaries in clk_core_init_rate_req
> >   clk: Change clk_core_init_rate_req prototype
> >   clk: Introduce clk_hw_init_rate_request()
> >   clk: Add missing clk_core_init_rate_req calls
> >   clk: Remove redundant clk_core_init_rate_req() call
> >   clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
> >   clk: Introduce clk_core_has_parent()
> >   clk: Stop forwarding clk_rate_requests to the parent
> >   clk: Zero the clk_rate_request structure
> >   clk: Test the clock pointer in clk_hw_get_name()
> >   clk: Prevent a clock without a rate to register
> >  
> >  drivers/clk/clk.c            |  239 +++++--
> >  drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
> >  include/linux/clk-provider.h |    6 +
> >  include/linux/clk.h          |    5 +-
> >  4 files changed, 1439 insertions(+), 67 deletions(-)
> 
> Thanks for another round of patches.
> On top my patchset this one does *not* break my imx8mp based setup. Booting
> is as usual without any changes in dmesg.
> Given this patch set I suspect the other one on github you sent me on
> Thursday is obsolete now.

I have to correct myself here a bit. While using my current board .dts fpr 
imx8mp works with this patchset, things change if I apply Lucas' HDMI patchset 
[1] and enable HDMI devices. In this case I get the following errors:
> hdmi_pclk: recalc_rate returned a null rate
> fsl-samsung-hdmi-phy 32fdff00.phy: error -EINVAL: failed to register clock
> fsl-samsung-hdmi-phy 32fdff00.phy: register clk failed
> fsl-samsung-hdmi-phy: probe of 32fdff00.phy failed with error -22

The offending patch is the last one 'clk: Prevent a clock without a rate to 
register'.

Best regards,
Alexander

[1] https://patchwork.kernel.org/project/linux-arm-kernel/cover/
20220406160123.1272911-1-l.stach@pengutronix.de/

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Neil Armstrong]

On 08/04/2022 17:36, Maxime Ripard wrote:
> On Fri, Apr 08, 2022 at 04:48:08PM +0200, Jerome Brunet wrote:
>>>>> and hdmi_pll_dco because it will always return 0,
>>>>
>>>> It is a read-only clock - whatever we do in CCF, it is not going to
>>>> change. CCF has always supported RO clocks.
>>>
>>> Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
>>> useful in anyway, it should report its current rate. Read-only or not.
>>>
>>
>> It does report its rate, it does not have any.
>> ... and It would pretty weird to initialize a read-only clock.
> 
> The KMS driver doesn't seem to have a problem with that.
> 
>>>>> unless the display driver comes around and updates it. If it never does,
>>>>> or if it's not compiled in, then you're out of luck.
>>>>
>>>> I'm all for managing the display clocks from CCF but it has proved tricky
>>>> so far. Maybe it will someday.
>>>>
>>>> Being a read-only clock, the value is what it is and CCF should
>>>> deal with it gracefully. It has so far.
>>>>
>>>> If the driver actually managing the clock is not compiled in, then the
>>>> clock will never be set, and it should not be a problem either.
>>>
>>> Again, it depends on what you expect from clk_get_rate(). If it can only
>>> report a rate of 0 on error, then it's definitely a problem.
>>
>> Agreed, it depends on what you expect from clk_get_rate().
>> Still when something does not oscillate, the rate is 0.
>>
>> If a driver call clk_get_rate() on an uninitialized, unlocked PLL, I
>> think returning 0 makes sense.
> 
> You're confusing two things: the rate output by the hardware, and what
> the CCF needs to return. We're discussing the latter here, a software
> construct. It models the hardware, but it doesn't have to be true to the
> hardware.
> 
> And even the meson driver doesn't follow what you're claiming to the
> letter and is inconsistent with what you're saying. Any disabled gate
> will also have a hardware rate of 0. Yet, it doesn't return 0 in such a
> case. And no one does, because clk_get_rate() isn't supposed to return
> the actual rate in hardware at the moment. It's supposed to return what
> would be the rate if it was enabled.

You're pointing a real issue, what should we return as a rate then ?

The rate is not only the rate output by the HW but is also used by the whole
subtree of the PLL to calculate the rates.

If we return a fake or dummy rate instead of 0 (which is the physical reality),
the clock subtree will be populated from a fake rate and if we did set a clock
rate using assigned-clock, eventually this fake rate would match and no set_rate()
would be called on the PLL.

This is why we return the true physical rate, it's the software to adapt in order
to handle the possible HW states.

> 
>>> And it's not because it was done before that it wasn't just as
>>> problematic then.
>>>
>>>>>> IMO, whenever possible we should not put default values in the clocks
>>>>>> which is why I chose to leave it like that.
>>>>>>
>>>>>> The PLL being unlocked, it has no rate. It is not set to have any rate.
>>>>>> IMO a returning a rate of 0 is valid here.
>>>>>
>>>>> If there's not a sensible default in the hardware already, I don't see
>>>>> what the big issue is to be honest. You already kind of do that for all
>>>>> the other PLL parameters with init_regs
>>>>
>>>> Those initial parameters are "magic" analog setting which don't have an
>>>> impact on the rate setting. The initial rate of the clock is never set
>>>> by the clock driver on purpose.
>>>
>>> It's still fundamentally the same: whatever is there at boot isn't good
>>> enough, so you change it to have a somewhat sensible default.
>>
>> If you don't need it, no.
> 
> I mean, I provided multiple examples that the meson driver is being
> inconsistent with both the CCF documentation and the expected usage of
> the CCF consumers. And I suggested solutions to fix this inconsistency
> both here and on IRC to Neil and you.

It's not that we don't like it, we only care the actual HW state is correctly
reported in the clock tree, nothing else.

If a new flag like CLK_INVALID_RATE existed then we would use it.

But so far a zero rate never was problematic, and iMX8mp HDMI PLL and MSM mmpll2
also return a 0 rate as init because it's probably the default PLL state at
HW startup.

> 
> The only argument you provided so far is that you don't like or want it.
> I'm sorry you feel this way, but it doesn't change either the consensus
> or the documentation that every other driver and consumer is following.
> Nor does it provide any solution.
> 
> In the grand scheme of things, I don't care, if you want to have your
> own conventions a policies, go ahead. Just don't expect me to debug
> whatever is going on next time it falls apart.
> 
> Maxime

Neil

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Jerome Brunet]

On Fri 08 Apr 2022 at 17:36, Maxime Ripard <maxime@cerno.tech> wrote:

>
> You're confusing two things: the rate output by the hardware, and what
> the CCF needs to return. We're discussing the latter here, a software
> construct. It models the hardware, but it doesn't have to be true to the
> hardware.
>
> And even the meson driver doesn't follow what you're claiming to the
> letter and is inconsistent with what you're saying. Any disabled gate
> will also have a hardware rate of 0. Yet, it doesn't return 0 in such a
> case. And no one does, because clk_get_rate() isn't supposed to return
> the actual rate in hardware at the moment. It's supposed to return what
> would be the rate if it was enabled.

I don't think I am confused at all.

What rate would you get if you would hit enable on those PLLs with the
invalid setting as they are ? 0 - no lock.

recalc_rate() is about giving the rate of the clock based on its current
parameters. This is exactly what the amlogic PLL driver does. I can't
compute a division by zero, sorry.

The fact that some parameters may be invalid for a clock element is not
specific to any SoC and we have to deal with it.

It is not up to the clock driver to select a random rate to just to make
that valid again, assuming it even can do so  which is not necessarily
the case.

Re: [PATCH 00/22] clk: More clock rate fixes and tests

[Maxime Ripard]

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

Hi Yassine,

On Sun, Apr 10, 2022 at 12:06:55PM +0000, Yassine Oudjana wrote:
> On Friday, April 8th, 2022 at 1:10 PM, Maxime Ripard <maxime@cerno.tech> wrote:
> > 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.
> >
> > 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.
> >
> > The last patch will probably prove to be controversial, but it can be
> > left out without affecting the rest of the series. It will affect the
> > meson clock drivers for the g12b SoC at least. It stems from the
> > realisation that on the g12b, 4 PLLs (and thus all their children) have
> > a rate of 0, pretty much forever which feels like a bug considering the
> > rate 0 is used as an error in most places.
> >
> > Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> > return 0 if the diviser of the PLL is set to 0 in the register, but:
> >
> > - pcie_pll_dco has a few registers to initialize set in
> > g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
> > hook and will instead call it in the enable hook. This looks like a
> > bug and could be easily fixed by adding that init hook.
> >
> > - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
> > the initialisation of their register (g12a_gp0_init_regs and
> > g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
> > long as set_rate isn't called, that field is going to remain at 0. And
> > since all but one users don't have CLK_SET_RATE_PARENT, this is
> > still fairly unlikely.
> >
> > - hdmi_pll_dco doesn't set the n field in the initialisation either,
> > but also doesn't have a set_rate implementation. Thus, if the
> > bootloader doesn't set it, this clock and all its children will
> > always report a rate of 0, even if the clock is functional.
> >
> > During the discussion with amlogic clock maintainers, we kind of ended
> > up on a disagreement of whether returning 0 was ok or not, hence the
> > expected controversy :)
> >
> > Let me know what you think,
> > Maxime
> >
> > Maxime Ripard (22):
> > clk: Drop the rate range on clk_put()
> > 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: Fix clk_get_parent() documentation
> > clk: Set req_rate on reparenting
> > clk: Skip set_rate_range if our clock is orphan
> > clk: Add our request boundaries in clk_core_init_rate_req
> > clk: Change clk_core_init_rate_req prototype
> > clk: Introduce clk_hw_init_rate_request()
> > clk: Add missing clk_core_init_rate_req calls
> > clk: Remove redundant clk_core_init_rate_req() call
> > clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
> > clk: Introduce clk_core_has_parent()
> > clk: Stop forwarding clk_rate_requests to the parent
> > clk: Zero the clk_rate_request structure
> > clk: Test the clock pointer in clk_hw_get_name()
> > clk: Prevent a clock without a rate to register
> >
> > drivers/clk/clk.c | 239 +++++--
> > drivers/clk/clk_test.c | 1256 +++++++++++++++++++++++++++++++++-
> > include/linux/clk-provider.h | 6 +
> > include/linux/clk.h | 5 +-
> > 4 files changed, 1439 insertions(+), 67 deletions(-)
> >
> > --
> > 2.35.1
> 
> It appears that this series breaks mmcc-msm8996:
> 
> [    8.713810] mmpll2_early: recalc_rate returned a null rate
> [    8.713864] mmcc-msm8996: probe of 8c0000.clock-controller failed with error -22
> 
> I haven't yet tried to figure out the exact patch that causes
> this or how it does. I just wanted to report it as soon as possible.

Could you test without the last patch?

Thanks,
Maxime

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

Re: (EXT) [PATCH 00/22] clk: More clock rate fixes and tests

[Maxime Ripard]

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

Hi

On Mon, Apr 11, 2022 at 09:24:22AM +0200, Alexander Stein wrote:
> Am Montag, 11. April 2022, 08:25:31 CEST schrieb Alexander Stein:
> > Hello Maxime,
> > 
> > Am Freitag, 8. April 2022, 11:10:15 CEST schrieb Maxime Ripard:
> > > 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.
> > > 
> > > 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.
> > > 
> > > The last patch will probably prove to be controversial, but it can be
> > > left out without affecting the rest of the series. It will affect the
> > > meson clock drivers for the g12b SoC at least. It stems from the
> > > realisation that on the g12b, 4 PLLs (and thus all their children) have
> > > a rate of 0, pretty much forever which feels like a bug considering the
> > > rate 0 is used as an error in most places.
> > > 
> > > Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> > > 
> > > return 0 if the diviser of the PLL is set to 0 in the register, but:
> > >   - pcie_pll_dco has a few registers to initialize set in
> > >   
> > >     g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
> > >     hook and will instead call it in the enable hook. This looks like a
> > >     bug and could be easily fixed by adding that init hook.
> > >   
> > >   - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
> > >   
> > >     the initialisation of their register (g12a_gp0_init_regs and
> > >     g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
> > >     long as set_rate isn't called, that field is going to remain at 0. And
> > >     since all but one users don't have CLK_SET_RATE_PARENT, this is
> > >     still fairly unlikely.
> > >   
> > >   - hdmi_pll_dco doesn't set the n field in the initialisation either,
> > >   
> > >     but also doesn't have a set_rate implementation. Thus, if the
> > >     bootloader doesn't set it, this clock and all its children will
> > >     always report a rate of 0, even if the clock is functional.
> > > 
> > > During the discussion with amlogic clock maintainers, we kind of ended
> > > up on a disagreement of whether returning 0 was ok or not, hence the
> > > expected controversy :)
> > > 
> > > Let me know what you think,
> > > Maxime
> > > 
> > > Maxime Ripard (22):
> > >   clk: Drop the rate range on clk_put()
> > >   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: Fix clk_get_parent() documentation
> > >   clk: Set req_rate on reparenting
> > >   clk: Skip set_rate_range if our clock is orphan
> > >   clk: Add our request boundaries in clk_core_init_rate_req
> > >   clk: Change clk_core_init_rate_req prototype
> > >   clk: Introduce clk_hw_init_rate_request()
> > >   clk: Add missing clk_core_init_rate_req calls
> > >   clk: Remove redundant clk_core_init_rate_req() call
> > >   clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
> > >   clk: Introduce clk_core_has_parent()
> > >   clk: Stop forwarding clk_rate_requests to the parent
> > >   clk: Zero the clk_rate_request structure
> > >   clk: Test the clock pointer in clk_hw_get_name()
> > >   clk: Prevent a clock without a rate to register
> > >  
> > >  drivers/clk/clk.c            |  239 +++++--
> > >  drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
> > >  include/linux/clk-provider.h |    6 +
> > >  include/linux/clk.h          |    5 +-
> > >  4 files changed, 1439 insertions(+), 67 deletions(-)
> > 
> > Thanks for another round of patches.
> > On top my patchset this one does *not* break my imx8mp based setup. Booting
> > is as usual without any changes in dmesg.
> > Given this patch set I suspect the other one on github you sent me on
> > Thursday is obsolete now.
> 
> I have to correct myself here a bit. While using my current board .dts fpr 
> imx8mp works with this patchset,

Awesome, thanks for testing :)

> things change if I apply Lucas' HDMI patchset
> [1] and enable HDMI devices. In this case I get the following errors:
> > hdmi_pclk: recalc_rate returned a null rate
> > fsl-samsung-hdmi-phy 32fdff00.phy: error -EINVAL: failed to register clock
> > fsl-samsung-hdmi-phy 32fdff00.phy: register clk failed
> > fsl-samsung-hdmi-phy: probe of 32fdff00.phy failed with error -22
> 
> The offending patch is the last one 'clk: Prevent a clock without a rate to 
> register'.

Yeah, I'll comment on that series, thanks!
Maxime

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Mon, Apr 11, 2022 at 09:40:03AM +0200, Neil Armstrong wrote:
> On 08/04/2022 17:36, Maxime Ripard wrote:
> > On Fri, Apr 08, 2022 at 04:48:08PM +0200, Jerome Brunet wrote:
> > > > > > and hdmi_pll_dco because it will always return 0,
> > > > > 
> > > > > It is a read-only clock - whatever we do in CCF, it is not going to
> > > > > change. CCF has always supported RO clocks.
> > > > 
> > > > Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
> > > > useful in anyway, it should report its current rate. Read-only or not.
> > > > 
> > > 
> > > It does report its rate, it does not have any.
> > > ... and It would pretty weird to initialize a read-only clock.
> > 
> > The KMS driver doesn't seem to have a problem with that.
> > 
> > > > > > unless the display driver comes around and updates it. If it never does,
> > > > > > or if it's not compiled in, then you're out of luck.
> > > > > 
> > > > > I'm all for managing the display clocks from CCF but it has proved tricky
> > > > > so far. Maybe it will someday.
> > > > > 
> > > > > Being a read-only clock, the value is what it is and CCF should
> > > > > deal with it gracefully. It has so far.
> > > > > 
> > > > > If the driver actually managing the clock is not compiled in, then the
> > > > > clock will never be set, and it should not be a problem either.
> > > > 
> > > > Again, it depends on what you expect from clk_get_rate(). If it can only
> > > > report a rate of 0 on error, then it's definitely a problem.
> > > 
> > > Agreed, it depends on what you expect from clk_get_rate().
> > > Still when something does not oscillate, the rate is 0.
> > > 
> > > If a driver call clk_get_rate() on an uninitialized, unlocked PLL, I
> > > think returning 0 makes sense.
> > 
> > You're confusing two things: the rate output by the hardware, and what
> > the CCF needs to return. We're discussing the latter here, a software
> > construct. It models the hardware, but it doesn't have to be true to the
> > hardware.
> > 
> > And even the meson driver doesn't follow what you're claiming to the
> > letter and is inconsistent with what you're saying. Any disabled gate
> > will also have a hardware rate of 0. Yet, it doesn't return 0 in such a
> > case. And no one does, because clk_get_rate() isn't supposed to return
> > the actual rate in hardware at the moment. It's supposed to return what
> > would be the rate if it was enabled.
> 
> You're pointing a real issue, what should we return as a rate then ?
> 
> The rate is not only the rate output by the HW but is also used by the
> whole subtree of the PLL to calculate the rates.
> 
> If we return a fake or dummy rate instead of 0 (which is the physical
> reality), the clock subtree will be populated from a fake rate and if
> we did set a clock rate using assigned-clock, eventually this fake
> rate would match and no set_rate() would be called on the PLL.
> 
> This is why we return the true physical rate, it's the software to
> adapt in order to handle the possible HW states.

I'm not sure why you insist so much on the "true physical rate" tbh.
That was never really a thing. It's not for a gate, it's not for a
spread-spectrum clock (which would be even more interesting to report
properly following your criteria). And then you have the clock
accuracies that are throwing it off entirely too.

It just isn't practical, and that's why no-one relies on it.

Or let's turn it the other way around. When you'll have called
clk_enable() on those clocks (but only that), what do you expect the
rate to be? 0? Yet, the "true physical rate" probably changed by then.

If that was a thing, we would also have to:

  * make sure clk_set_rate can't be called on a disabled clock, because
    the true physical rate would never match what is required

  * rate ranges just wouldn't be usable either, because we would have no
    way to check that the true physical rate is actually within that
    range at any given time.

  * All clocks rate wouldn't be cacheable to account for the jitter in
    the system clock source.

  * We would need to use some hardware engine that reads the clock rate
    being output at any given time, to account for spread-spectrum and
    the clock source accuracy.

  * We wouldn't be able to report phases properly, because you can't
    really measure the clock skew either.

I understand that it's convenient for you, but it isn't practical if we
look at it from the framework's PoV.

You could very well initialize the PLLs to have a 1/1 ratio to their
parent, that would probably be sensible and would make the framework
happy.

> > > > And it's not because it was done before that it wasn't just as
> > > > problematic then.
> > > > 
> > > > > > > IMO, whenever possible we should not put default values in the clocks
> > > > > > > which is why I chose to leave it like that.
> > > > > > > 
> > > > > > > The PLL being unlocked, it has no rate. It is not set to have any rate.
> > > > > > > IMO a returning a rate of 0 is valid here.
> > > > > > 
> > > > > > If there's not a sensible default in the hardware already, I don't see
> > > > > > what the big issue is to be honest. You already kind of do that for all
> > > > > > the other PLL parameters with init_regs
> > > > > 
> > > > > Those initial parameters are "magic" analog setting which don't have an
> > > > > impact on the rate setting. The initial rate of the clock is never set
> > > > > by the clock driver on purpose.
> > > > 
> > > > It's still fundamentally the same: whatever is there at boot isn't good
> > > > enough, so you change it to have a somewhat sensible default.
> > > 
> > > If you don't need it, no.
> > 
> > I mean, I provided multiple examples that the meson driver is being
> > inconsistent with both the CCF documentation and the expected usage of
> > the CCF consumers. And I suggested solutions to fix this inconsistency
> > both here and on IRC to Neil and you.
> 
> It's not that we don't like it, we only care the actual HW state is
> correctly reported in the clock tree, nothing else.
>
> If a new flag like CLK_INVALID_RATE existed then we would use it.
> 
> But so far a zero rate never was problematic, and iMX8mp HDMI PLL

That patch isn't upstream.

> and MSM mmpll2 also return a 0 rate as init because it's probably the
> default PLL state at HW startup.

And two wrongs never made a right.

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

Re: [PATCH 14/22] clk: Introduce clk_hw_init_rate_request()

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 02:10:29)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 399080456e45..83dd5f1df0b9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1396,6 +1396,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 and fills a clk_rate_request structure to submit to

s/and fills//

> + * __clk_determine_rate or similar functions.

__clk_determine_rate()

> + */
> +void clk_hw_init_rate_request(struct clk_hw * const hw,

I don't get why it isn't 'const struct clk_hw *hw', but it looks to be
following clk_core_init_rate_req() so that can be figured out later.
Please remove the const from here regardless; it's not doing anything.

> +                             struct clk_rate_request *req,
> +                             unsigned long rate)
> +{
> +       if (WARN_ON(!hw || !req))

Why would you call it without those two items? Another copy/paste from
clk_core_init_rate_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;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index c10dc4c659e2..39e4ed301ec5 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -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().

How is that enforced?

This has only become a problem after commit 948fb0969eae ("clk: Always
clamp the rounded rate") from what I can tell. I guess we can't skip the
clamp when both min/max are zero though because it may be stack junk?
But I looked at all the call sites and either they zero initialize the
whole struct (qcom) or they copy the req from what is passed into the
determine_rate clk_op (others). So we could simply not clamp if both
values are equal to zero and then qcom would be happy, but that has been
fixed by setting the min/max to 0 and max instead. That leaves the other
users, which already copy what is being passed in, i.e. what is done by
clk_core_init_rate_req().

I guess my question is who is going to use this? And if we can't even
enforce that it is used then it feels like we shouldn't add it. Maybe it
can be useful to cleanup the core request initialization logic because
it's sort of spread out but probably not as a clk_hw API.

> + *
>   * @rate:              Requested clock rate. This field will be adjusted by
>   *                     clock drivers according to hardware capabilities.
>   * @min_rate:          Minimum rate imposed by clk users.

Re: [PATCH 15/22] clk: Add missing clk_core_init_rate_req calls

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 02:10:30)
> Some callers of clk_core_round_rate_nolock() will initialize the
> clk_rate_request structure by hand, missing a few parameters that leads

Which parameters?

> to inconsistencies in what drivers can expect from that structure.
> 
> Let's use clk_core_init_rate_req() everywhere to make sure it's built in
> a consistent manner.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 83dd5f1df0b9..3a59152b06b8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1480,7 +1480,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
>         int ret;
>         struct clk_rate_request req;
>  
> -       req.rate = rate;
> +       clk_core_init_rate_req(hw->core, &req, rate);
>  
>         ret = clk_core_round_rate_nolock(hw->core, &req);

clk_core_round_rate_nolock() has a clk_core_init_rate_req() inside it.
This is duplicating that.

>         if (ret)
> @@ -1512,7 +1512,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>         if (clk->exclusive_count)
>                 clk_core_rate_unprotect(clk->core);
>  
> -       req.rate = rate;
> +       clk_core_init_rate_req(clk->core, &req, rate);
>  
>         ret = clk_core_round_rate_nolock(clk->core, &req);

Duplicating again?

>  
> @@ -2216,8 +2216,7 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
>         if (cnt < 0)
>                 return cnt;
>  
> -       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> -       req.rate = req_rate;
> +       clk_core_init_rate_req(core, &req, req_rate);

So we put the boundaries inside clk_core_init_rate_req()? That sounds
like it's required now after we clamp all the time.

>  
>         ret = clk_core_round_rate_nolock(core, &req);

Same.

Re: [PATCH 16/22] clk: Remove redundant clk_core_init_rate_req() call

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 02:10:31)
> Since all the users of clk_core_round_rate_nolock() will now properly
> initialize, there's no need for it to initialize the request itself.

Probably this needs to be combined with the previous patch.

> 
> This is even dangerous, as if the clock cannot change its rate by itself
> and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call
> itself with the parent clock but the client clk_rate_request structure.
> 

I think the next sentence is part of the single sentence paragraph
above.

> We will then reinitialize the child request with the parent context
> (parent, boundaries, etc.), which is an issue if the parent ever changes
> its own parent or parent rate.

The parent of the parent can't be the parent of the child, i.e. itself.
I guess this is only a problem if clk_core_init_rate_req() starts
setting min/max? We want to leave those members unchanged so that the
rate request can flow up through the tree and be modified when rounding
rates from a grandchild. That's why the child req is passed up to the
parent if the child can't round itself. The boundary of the child is
moved to the parent.

Definitely the rate should be clamped through a parent to the
grandparent taking into account any of their constraints. Perhaps the
bug is that __clk_determine_rate() doesn't clamp to boundaries like
clk_hw_round_rate() does by calling clk_core_get_boundaries() and then
mixing in the new requests boundaries.

Re: [PATCH 02/22] clk: tests: Add test suites description

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 02:10:17)
> 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.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Thanks. Mostly nitpicks but otherwise looks good.

> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index fd2339cc5898..663b3dd388f7 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -250,6 +250,11 @@ static struct kunit_case clk_test_cases[] = {
>         {}
>  };
>  
> +/*
> + * Test suite for a basic rate clock, without any parent.
> + *
> + * These tests are supposed to exercise the rate API with simple scenarios

s/are supposed to//

Simplify please.

> + */
>  static struct kunit_suite clk_test_suite = {
>         .name = "clk-test",
>         .init = clk_test_init,
> @@ -336,6 +341,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 orphan when

s/be/be an/

> + * registered, but will no longer be when the tests run.
> + *
> + * These tests are supposed to make sure a clock that used to be orphan

s/are supposed to//

> + * 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,
> @@ -645,6 +658,13 @@ static struct kunit_case clk_range_test_cases[] = {
>         {}
>  };
>  
> +/*
> + * Test suite for a basic rate clock, without any parent.
> + *
> + * These tests are supposed to exercise the rate range API

s/are supposed to//

s/API/API:/


> + * (clk_set_rate_range, clk_set_min_rate, clk_set_max_rate,

Functions have () after them. Drop parenthesis around the list and use
colon instead.

> + * clk_drop_range).
> + */
>  static struct kunit_suite clk_range_test_suite = {
>         .name = "clk-range-test",
>         .init = clk_test_init,
> @@ -822,6 +842,14 @@ static struct kunit_case clk_range_maximize_test_cases[] = {
>         {}
>  };
>  
> +/*
> + * Test suite for a basic rate clock, without any parent.
> + *
> + * These tests are supposed to exercise the rate range API

s/are supposed to//

> + * (clk_set_rate_range, clk_set_min_rate, clk_set_max_rate,

s/(//

> + * clk_drop_range), with a driver that will always try to run at the

s/)//

> + * highest possible rate.
> + */
>  static struct kunit_suite clk_range_maximize_test_suite = {
>         .name = "clk-range-maximize-test",
>         .init = clk_maximize_test_init,
> @@ -991,6 +1019,14 @@ static struct kunit_case clk_range_minimize_test_cases[] = {
>         {}
>  };
>  
> +/*
> + * Test suite for a basic rate clock, without any parent.
> + *
> + * These tests are supposed to exercise the rate range API

s/are supposed to//

> + * (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,

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 02:10:37)
> A rate of 0 for a clock is considered an error, as evidenced by the
> documentation of clk_get_rate() and the code of clk_get_rate() and
> clk_core_get_rate_nolock().
> 
> The main source of that error is if the clock is supposed to have a
> parent but is orphan at the moment of the call. This is likely to be
> transient and solved later in the life of the system as more clocks are
> registered.
> 
> The corollary is thus that if a clock is not an orphan, has a parent that
> has a rate (so is not an orphan itself either) but returns a rate of 0,
> something is wrong in the driver. Let's return an error in such a case.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8bbb6adeeead..e8c55678da85 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>                 rate = 0;
>         core->rate = core->req_rate = rate;
>  
> +       /*
> +        * If we're not an orphan clock and our parent has a rate, then
> +        * if our rate is 0, something is badly broken in recalc_rate.
> +        */
> +       if (!core->orphan && (parent && parent->rate) && !core->rate) {

It's possible that it is an orphan at time of registration, so this
check doesn't even cover the case when it is parented by a later clk
registration. How would we error out when parenting the clk to the
parent if recalc_rate then starts returning 0? It doesn't seem possible
to implement this.

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-08 03:41:27)
> On Fri, Apr 08, 2022 at 11:18:58AM +0200, Jerome Brunet wrote:
> > On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
> > > A rate of 0 for a clock is considered an error, as evidenced by the
> > > documentation of clk_get_rate() and the code of clk_get_rate() and
> > > clk_core_get_rate_nolock().

Where?

> > >
> > > The main source of that error is if the clock is supposed to have a
> > > parent but is orphan at the moment of the call. This is likely to be
> > > transient and solved later in the life of the system as more clocks are
> > > registered.
> > >
> > > The corollary is thus that if a clock is not an orphan, has a parent that
> > > has a rate (so is not an orphan itself either) but returns a rate of 0,
> > > something is wrong in the driver. Let's return an error in such a case.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >  drivers/clk/clk.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 8bbb6adeeead..e8c55678da85 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> > >             rate = 0;
> > >     core->rate = core->req_rate = rate;
> > >  
> > > +   /*
> > > +    * If we're not an orphan clock and our parent has a rate, then
> > > +    * if our rate is 0, something is badly broken in recalc_rate.
> > > +    */
> > > +   if (!core->orphan && (parent && parent->rate) && !core->rate) {
> > > +           ret = -EINVAL;
> > > +           pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> > > +           goto out;
> > > +   }
> > > +
> > 
> > As hinted in the cover letter, I don't really agree with that.
> > 
> > There are situations where we can't compute the rate. Getting invalid
> > value in the register is one reason.
> > 
> > You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> > SoCs would be affected):
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> > Yes, PLL that have not been previously used (by the ROMCode or the
> > bootloader) tend to have the value of the divider set to 0 which in
> > invalid as it would result in a division by zero.
> > 
> > I don't think this is a bug. It is just what the HW is, an unlocked,
> > uninitialized PLL. There is no problem here and the PLL can remain like
> > that until it is needed.
> 
> I think the larger issue is around the semantics of clk_get_rate(), and
> especially whether we can call it without a clk_enable(), and whether
> returning 0 is fine.
> 
> The (clk.h) documentation of clk_get_rate() mentions that "This is only
> valid once the clock source has been enabled", and it's fairly
> ambiguous. I can see how it could be interpreted as "you need to call
> clk_enable() before calling clk_get_rate()", but it can also be
> interpreted as "The returned rate will only be valid once clk_enable()
> is called".

I enjoy the ambiguity! :) This question has come up before and it
doesn't really matter. Drivers can call clk_prepare_enable() if they
want to be sure that clk_get_rate() is meaningful to them, or they can
not. The CCF returns a rate that it gets from calling recalc_rate, which
could be inaccurate for others reasons, either because some driver has
called clk_set_rate() after the clk_get_rate() or because the clk is an
orphan still and clk_get() succeeded, or because the clk_op couldn't
calculate it at the time of caching. Indeed the CCF doesn't try to
recalc the rate after enabling the clk. Maybe we should do that? It
would mean that we have to schedule a work from the enable path to
update the rate accounting outside of any atomic context.

Just thinking out loud, the simpler solution is to probably drop all
rate caching in the CCF and get the frequency on a clk_get_rate() call.
It complicates some of the core though when we check to see if we need
to update clk rates. We could have some middle ground where drivers
indicate that they want to update their cached rate because it's valid
now (either from their enable path or from somewhere else). This may be
nice actually because we could have clk providers call this to force a
recalc down the tree from where they've updated. I think the qcom
DisplayPort phy would want this.

> 
> I think the latter is the proper interpretation though based on what the
> drivers are doing, and even the CCF itself will call recalc_rate without
> making sure that the clock is enabled (in __clk_core_init() for example).
> 
> Then there is the question of whether returning 0 is fine. Again
> clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> returns 0.". This is indeed returned in case of an error condition (in
> clk_get_rate() itself, but also in clk_core_get_rate_nolock()).

A NULL clk isn't an error. We use NULL in the CCF to indicate that it's
an optional clk. Returning 0 from clk_get_rate() is not an error. If
clk_get() returns an error pointer then it's an error. And NULL isn't an
error value per PTR_ERR() (because NULL == 0 when casted, this isn't
golang).

> 
> All the drivers I could find either assume the rate is valid, or test
> whether it's 0 or not (randomly picked, but across completely different
> platforms):
> https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/armv7m_systick.c#L50
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/armada-8k-cpufreq.c#L74
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sti/uniperif_player.c#L194
> https://elixir.bootlin.com/linux/latest/source/sound/soc/tegra/tegra20_i2s.c#L278
> 
> So my understanding is that the consensus is that clk_get_rate() can be
> called even if the clock hasn't been enabled, and that returning 0 is
> only meant to be used for errors in general, a NULL pointer according to
> the documentation.

Again, NULL isn't an invalid clk handle.

> 
> That would mean that pcie_pll_dco is buggy because it assumes that
> clk_enable() is going to be called before clk_get_rate(), gp0_pll_dco
> and hifi_pll_dco because they expect "someone" to call clk_set_rate()
> before clk_get_rate(), and hdmi_pll_dco because it will always return 0,
> unless the display driver comes around and updates it. If it never does,
> or if it's not compiled in, then you're out of luck.
> 

I think this is all fine.

Re: [PATCH 14/22] clk: Introduce clk_hw_init_rate_request()

[Maxime Ripard]

Hi Stephen,

Thanks for your reviews :)

On Fri, Apr 22, 2022 at 08:46:21PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:29)
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 399080456e45..83dd5f1df0b9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1396,6 +1396,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 and fills a clk_rate_request structure to submit to
> 
> s/and fills//
> 
> > + * __clk_determine_rate or similar functions.
> 
> __clk_determine_rate()
> 
> > + */
> > +void clk_hw_init_rate_request(struct clk_hw * const hw,
> 
> I don't get why it isn't 'const struct clk_hw *hw', but it looks to be
> following clk_core_init_rate_req() so that can be figured out later.

We won't modify either the pointer nor the clk_hw itself, so it made
sense to me?

> Please remove the const from here regardless; it's not doing anything.

Ok, I'll remove it.

> > +                             struct clk_rate_request *req,
> > +                             unsigned long rate)
> > +{
> > +       if (WARN_ON(!hw || !req))
> 
> Why would you call it without those two items? Another copy/paste from
> clk_core_init_rate_req()?

I never know what to do here honestly. It's true that calling it with
NULL as a parameter seems dumb, but I could very well see a NULL pointer
stored in a variable and using that variable.

Preventing a NULL pointer dereference in such a case seems sane, but I
couldn't find a consistent policy in the kernel about what to do. I'll
remove it if you don't like it.

> > +               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;
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index c10dc4c659e2..39e4ed301ec5 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -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().
> 
> How is that enforced?

I'm sure we can come up with some coccinelle script if needed, but yeah
it's a pretty loose requirement for now.

> This has only become a problem after commit 948fb0969eae ("clk: Always
> clamp the rounded rate") from what I can tell. I guess we can't skip the
> clamp when both min/max are zero though because it may be stack junk?

Yeah, but it isn't the only issue, really.

Prior to this series, the most glaring issue is that most
CLK_SET_RATE_PARENT handling is just updating the rate, or even just
copying the clk_request_rate and forwarding it to the parent:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L553

In this case, the requested rate is wrong, but the best_parent_hw and
best_parent_rate fields will be the one of the child clock, and thus
will point to the parent itself, not the parent's parent.

at91 has the same issue in multiple places:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-generated.c#L191
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-master.c#L402
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-master.c#L731

So making it available to drivers seems to add some value at least,
whether we require it or not.

> But I looked at all the call sites and either they zero initialize the
> whole struct (qcom) or they copy the req from what is passed into the
> determine_rate clk_op (others). So we could simply not clamp if both
> values are equal to zero and then qcom would be happy, but that has been
> fixed by setting the min/max to 0 and max instead.

Like I said, it's more of a symptom of a larger issue than a bug in
itself. It's not just that the driver doesn't enforce any boundary (so
at the clk_hw level), which would be fine, it's also that it will ignore
any user boundary, and that's where the bug is in my book. And it was
there prior to the patch you mentioned.

I'd be ok to add that kind of check for the time being, but it should at
least come with some kind of warning to report that something fishy is
going on and needs to be addressed.

> That leaves the other users, which already copy what is being passed
> in, i.e. what is done by clk_core_init_rate_req().

> I guess my question is who is going to use this? And if we can't even
> enforce that it is used then it feels like we shouldn't add it. Maybe it
> can be useful to cleanup the core request initialization logic because
> it's sort of spread out but probably not as a clk_hw API.

The main user really will be a unit test I added for
__clk_determine_rate() to make sure the returned clk_request_rate is
sane. All the drivers we discussed so far would probably need to use
clk_core_forward_rate_req() instead.

If we follow what is being done for the rest of the clk_core_*
functions, clk_core_forward_rate_req() isn't available to providers and
isn't exported, to it would be difficult to use as is in the tests.
Could we create a clk_internal.h header or something maybe to export it
only to the tests if you don't want to expose it to drivers?

Maxime

Re: [PATCH 15/22] clk: Add missing clk_core_init_rate_req calls

[Maxime Ripard]

On Fri, Apr 22, 2022 at 08:51:24PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:30)
> > Some callers of clk_core_round_rate_nolock() will initialize the
> > clk_rate_request structure by hand, missing a few parameters that leads
> 
> Which parameters?

min_rate and max_rate

> > to inconsistencies in what drivers can expect from that structure.
> > 
> > Let's use clk_core_init_rate_req() everywhere to make sure it's built in
> > a consistent manner.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 83dd5f1df0b9..3a59152b06b8 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1480,7 +1480,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
> >         int ret;
> >         struct clk_rate_request req;
> >  
> > -       req.rate = rate;
> > +       clk_core_init_rate_req(hw->core, &req, rate);
> >  
> >         ret = clk_core_round_rate_nolock(hw->core, &req);
> 
> clk_core_round_rate_nolock() has a clk_core_init_rate_req() inside it.
> This is duplicating that.

Yeah, and it's an issue whenever we take the recursion path in
clk_core_round_rate_nolock() here:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1412

We just call clk_core_round_rate_nolock() onto our parent clock, with
the child rate request.

clk_core_round_rate_nolock() will then call clk_core_init_rate_req(),
which will set the boundaries (after patch 12) and the best_parent_*
fields. The parent will then possibly modify the rate, its parent rate
(through best_parent_rate), and if it's a mux that allows reparenting
can modify best_parent_hw too.

We then return, and the caller of clk_core_round_rate_nolock() will have
its parent parent rate and clk_hw pointer, and not its parent.

The way I dealt with it is that I'll later remove the
clk_core_init_rate_req() call in clk_core_round_rate_nolock() (patch 17)
to stop updating the *child* request, and in patch 19 will create a new
request for the parent, with the parent details, separating properly the
child context and its parent's.

This isn't just theoretical, patch 19 has a unit test
(clk_leaf_mux_set_rate_parent_determine_rate) that will show how it
affects drivers too.

Let me know if you'd like to address this some other way.

>
> >         if (ret)
> > @@ -1512,7 +1512,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
> >         if (clk->exclusive_count)
> >                 clk_core_rate_unprotect(clk->core);
> >  
> > -       req.rate = rate;
> > +       clk_core_init_rate_req(clk->core, &req, rate);
> >  
> >         ret = clk_core_round_rate_nolock(clk->core, &req);
> 
> Duplicating again?
> 
> >  
> > @@ -2216,8 +2216,7 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> >         if (cnt < 0)
> >                 return cnt;
> >  
> > -       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > -       req.rate = req_rate;
> > +       clk_core_init_rate_req(core, &req, req_rate);
> 
> So we put the boundaries inside clk_core_init_rate_req()? That sounds
> like it's required now after we clamp all the time.

It was already done in a prior patch (patch 12)

Maxime

Re: [PATCH 16/22] clk: Remove redundant clk_core_init_rate_req() call

[Maxime Ripard]

On Fri, Apr 22, 2022 at 09:02:48PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:31)
> > Since all the users of clk_core_round_rate_nolock() will now properly
> > initialize, there's no need for it to initialize the request itself.
> 
> Probably this needs to be combined with the previous patch.
> 
> > 
> > This is even dangerous, as if the clock cannot change its rate by itself
> > and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call
> > itself with the parent clock but the client clk_rate_request structure.
> > 
> 
> I think the next sentence is part of the single sentence paragraph
> above.
> 
> > We will then reinitialize the child request with the parent context
> > (parent, boundaries, etc.), which is an issue if the parent ever changes
> > its own parent or parent rate.
> 
> The parent of the parent can't be the parent of the child, i.e. itself.

I already explained this a bit more in my answer to patch 15.

> I guess this is only a problem if clk_core_init_rate_req() starts
> setting min/max?

The bugs I were seeing were mostly about the parent related infos
(best_parent_rate and best_parent_hw) leaking into the child request.

> We want to leave those members unchanged so that the
> rate request can flow up through the tree and be modified when rounding
> rates from a grandchild. That's why the child req is passed up to the
> parent if the child can't round itself. The boundary of the child is
> moved to the parent.
> 
> Definitely the rate should be clamped through a parent to the
> grandparent taking into account any of their constraints. Perhaps the
> bug is that __clk_determine_rate() doesn't clamp to boundaries like
> clk_hw_round_rate() does by calling clk_core_get_boundaries() and then
> mixing in the new requests boundaries.

That should work fine though, I've added some tests to make sure this is
working properly after a few fixes
(clk_test_single_parent_mux_set_range_round_rate_child_smaller in patch
5, clk_test_single_parent_mux_set_range_round_rate_parent_only and
clk_test_single_parent_mux_set_range_round_rate_parent_smaller in patch 19)

Maxime

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

On Fri, Apr 22, 2022 at 09:12:33PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:37)
> > A rate of 0 for a clock is considered an error, as evidenced by the
> > documentation of clk_get_rate() and the code of clk_get_rate() and
> > clk_core_get_rate_nolock().
> > 
> > The main source of that error is if the clock is supposed to have a
> > parent but is orphan at the moment of the call. This is likely to be
> > transient and solved later in the life of the system as more clocks are
> > registered.
> > 
> > The corollary is thus that if a clock is not an orphan, has a parent that
> > has a rate (so is not an orphan itself either) but returns a rate of 0,
> > something is wrong in the driver. Let's return an error in such a case.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8bbb6adeeead..e8c55678da85 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> >                 rate = 0;
> >         core->rate = core->req_rate = rate;
> >  
> > +       /*
> > +        * If we're not an orphan clock and our parent has a rate, then
> > +        * if our rate is 0, something is badly broken in recalc_rate.
> > +        */
> > +       if (!core->orphan && (parent && parent->rate) && !core->rate) {
> 
> It's possible that it is an orphan at time of registration, so this
> check doesn't even cover the case when it is parented by a later clk
> registration. How would we error out when parenting the clk to the
> parent if recalc_rate then starts returning 0? It doesn't seem possible
> to implement this.

Like I said in my cover letter, this was mostly to spark a discussion :)

Indeed, that case you mentioned wouldn't be covered by this check. I
don't think this patch is reasonable either :)

I mostly wanted to discuss whether you felt like it was something that
was ok or not. If it isn't, I think a good way forward would be to add a
bunch of pr_warn messages to mention that something is fishy and to
clarify the doc.

Hopefully, it will raise both the attention of developers of already
in-tree drivers to fix this, and will prevent new drivers from
introducing more of that behavior.

If the issue persists we could then take stronger measures some time in
the future if needs be?

Maxime

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Fri, Apr 22, 2022 at 09:42:26PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 03:41:27)
> > On Fri, Apr 08, 2022 at 11:18:58AM +0200, Jerome Brunet wrote:
> > > On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > A rate of 0 for a clock is considered an error, as evidenced by the
> > > > documentation of clk_get_rate() and the code of clk_get_rate() and
> > > > clk_core_get_rate_nolock().
> 
> Where?
> 
> > > >
> > > > The main source of that error is if the clock is supposed to have a
> > > > parent but is orphan at the moment of the call. This is likely to be
> > > > transient and solved later in the life of the system as more clocks are
> > > > registered.
> > > >
> > > > The corollary is thus that if a clock is not an orphan, has a parent that
> > > > has a rate (so is not an orphan itself either) but returns a rate of 0,
> > > > something is wrong in the driver. Let's return an error in such a case.
> > > >
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > ---
> > > >  drivers/clk/clk.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 8bbb6adeeead..e8c55678da85 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> > > >             rate = 0;
> > > >     core->rate = core->req_rate = rate;
> > > >  
> > > > +   /*
> > > > +    * If we're not an orphan clock and our parent has a rate, then
> > > > +    * if our rate is 0, something is badly broken in recalc_rate.
> > > > +    */
> > > > +   if (!core->orphan && (parent && parent->rate) && !core->rate) {
> > > > +           ret = -EINVAL;
> > > > +           pr_warn("%s: recalc_rate returned a null rate\n", core->name);
> > > > +           goto out;
> > > > +   }
> > > > +
> > > 
> > > As hinted in the cover letter, I don't really agree with that.
> > > 
> > > There are situations where we can't compute the rate. Getting invalid
> > > value in the register is one reason.
> > > 
> > > You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> > > SoCs would be affected):
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> > > Yes, PLL that have not been previously used (by the ROMCode or the
> > > bootloader) tend to have the value of the divider set to 0 which in
> > > invalid as it would result in a division by zero.
> > > 
> > > I don't think this is a bug. It is just what the HW is, an unlocked,
> > > uninitialized PLL. There is no problem here and the PLL can remain like
> > > that until it is needed.
> > 
> > I think the larger issue is around the semantics of clk_get_rate(), and
> > especially whether we can call it without a clk_enable(), and whether
> > returning 0 is fine.
> > 
> > The (clk.h) documentation of clk_get_rate() mentions that "This is only
> > valid once the clock source has been enabled", and it's fairly
> > ambiguous. I can see how it could be interpreted as "you need to call
> > clk_enable() before calling clk_get_rate()", but it can also be
> > interpreted as "The returned rate will only be valid once clk_enable()
> > is called".
> 
> I enjoy the ambiguity! :) This question has come up before and it
> doesn't really matter. Drivers can call clk_prepare_enable() if they
> want to be sure that clk_get_rate() is meaningful to them, or they can
> not.

Right, but that alone already removes the ambiguity :)

If not calling clk_enable() prior to calling clk_get_rate() is ok, then
we don't *need* to call it, both are valid.

Therefore, the "you need to call clk_enable() before calling
clk_get_rate()" interpretation isn't the right one, right?

> The CCF returns a rate that it gets from calling recalc_rate, which
> could be inaccurate for others reasons, either because some driver has
> called clk_set_rate() after the clk_get_rate()

Right, but I guess that's ok, the CCF never claimed to support atomicity
in any way. That could also be the result of a parent changing its rate,
or a reparenting, etc.

Still, it means that it was the best estimate we could do when
clk_get_rate() was called. It just turned out to be short-lived.

> or because the clk is an orphan still and clk_get() succeeded

In this case, core->parent is NULL, so we could argue that it's the same
case than clk_get_rate(NULL). It makes sense.

> or because the clk_op couldn't calculate it at the time of caching.

I guess this is what the discussion is all about, whether it's actually
something that we should find it acceptable or not.

From what you're saying, it is indeed acceptable and thus clk_get_rate()
can return 0 not only from a NULL clock, but also if it's incapable of
figuring out a rate for that clock. We should thus document that
recalc_rate can indeed return 0 in such a case, and that users should
take this into account and not always expect a valid rate.

Would a documentation for recalc_rate like that:

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. If
the driver cannot figure out a rate for this clock, it must return 0.
Optional, but recommended - if this op is not set then clock rate will
be initialized to 0.

And for clk_get_rate() (in drivers/clk/clk.c):

Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE
flag is set, which means a recalc_rate will be issued. Can be called if
the clock is enabled or not. If clk is NULL, or if an error occurred,
then returns 0.

Work for everyone?

> Indeed the CCF doesn't try to recalc the rate after enabling the clk.
> Maybe we should do that? It would mean that we have to schedule a work
> from the enable path to update the rate accounting outside of any
> atomic context.
> 
> Just thinking out loud, the simpler solution is to probably drop all
> rate caching in the CCF and get the frequency on a clk_get_rate() call.

The above is just enough for me. recalc_rate returning 0 is valid, and
should be taken properly into account.

Longer term I guess it makes sense to drop the rate caching, but I've
introduced enough clock regressions already for a couple of releases :)

> It complicates some of the core though when we check to see if we need
> to update clk rates. We could have some middle ground where drivers
> indicate that they want to update their cached rate because it's valid
> now (either from their enable path or from somewhere else). This may be
> nice actually because we could have clk providers call this to force a
> recalc down the tree from where they've updated. I think the qcom
> DisplayPort phy would want this.

A good workaround for now would be to set CLK_GET_RATE_NOCACHE for those
clocks.

> > 
> > I think the latter is the proper interpretation though based on what the
> > drivers are doing, and even the CCF itself will call recalc_rate without
> > making sure that the clock is enabled (in __clk_core_init() for example).
> > 
> > Then there is the question of whether returning 0 is fine. Again
> > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > returns 0.". This is indeed returned in case of an error condition (in
> > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> 
> A NULL clk isn't an error. We use NULL in the CCF to indicate that it's
> an optional clk. Returning 0 from clk_get_rate() is not an error. If
> clk_get() returns an error pointer then it's an error. And NULL isn't an
> error value per PTR_ERR() (because NULL == 0 when casted, this isn't
> golang).

Ok. So we can either group both the "we couldn't figure out a rate" and
the "our clock is NULL" case as the same case in clk_get_rate(), or we
can convert clk_get_rate() to return signed integers and a negative
error code. The latter seems much more intrusive to me and will probably
lead to a whole bunch of regressions, so I'd be more inclined to stick
to the former. Especially since drivers seem to either don't care or
treat 0 as an error already.

Maxime

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

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-23 02:17:24)
> On Fri, Apr 22, 2022 at 09:42:26PM -0700, Stephen Boyd wrote:
> > I enjoy the ambiguity! :) This question has come up before and it
> > doesn't really matter. Drivers can call clk_prepare_enable() if they
> > want to be sure that clk_get_rate() is meaningful to them, or they can
> > not.
> 
> Right, but that alone already removes the ambiguity :)
> 
> If not calling clk_enable() prior to calling clk_get_rate() is ok, then
> we don't *need* to call it, both are valid.
> 
> Therefore, the "you need to call clk_enable() before calling
> clk_get_rate()" interpretation isn't the right one, right?

It depends. The CCF isn't the only implementation of the clk API, so
some drivers want to maintain API compatibility and call clk_enable()
before getting the rate to make sure the rate is valid. If all other clk
API implementations are removed or made to work without calling
clk_enable() we should be able to update the clk.h documentation.

> 
> > The CCF returns a rate that it gets from calling recalc_rate, which
> > could be inaccurate for others reasons, either because some driver has
> > called clk_set_rate() after the clk_get_rate()
> 
> Right, but I guess that's ok, the CCF never claimed to support atomicity
> in any way. That could also be the result of a parent changing its rate,
> or a reparenting, etc.
> 
> Still, it means that it was the best estimate we could do when
> clk_get_rate() was called. It just turned out to be short-lived.
> 
> > or because the clk is an orphan still and clk_get() succeeded
> 
> In this case, core->parent is NULL, so we could argue that it's the same
> case than clk_get_rate(NULL). It makes sense.
> 
> > or because the clk_op couldn't calculate it at the time of caching.
> 
> I guess this is what the discussion is all about, whether it's actually
> something that we should find it acceptable or not.
> 
> From what you're saying, it is indeed acceptable and thus clk_get_rate()
> can return 0 not only from a NULL clock, but also if it's incapable of
> figuring out a rate for that clock. We should thus document that
> recalc_rate can indeed return 0 in such a case, and that users should
> take this into account and not always expect a valid rate.

I don't think there's anything to update.

> 
> Would a documentation for recalc_rate like that:
> 
> 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. If
> the driver cannot figure out a rate for this clock, it must return 0.

What would the driver return besides 0 if the rate can't be calculated?
UINT_MAX? Does the framework care that the rate couldn't be calculated?
Weakening "must" to "should" or saying it "will most likely return 0" is
better. But again I don't see much point in clarifying here. 0 Hz is a
valid frequency. Maybe we should just say that 0 is valid?

> Optional, but recommended - if this op is not set then clock rate will
> be initialized to 0.
> 
> And for clk_get_rate() (in drivers/clk/clk.c):
> 
> Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE
> flag is set, which means a recalc_rate will be issued. Can be called if
> the clock is enabled or not. If clk is NULL, or if an error occurred,
> then returns 0.

We should remove the clk_get_rate() documentation in clk.c and let the
clk.h documentation be the one that is used. That will clarify that
clk.c implements the API in clk.h. I've been meaning to clean up the
documentation so I'll hack on that tomorrow and see if I can make this
better.

> 
> Work for everyone?
> 
> > Indeed the CCF doesn't try to recalc the rate after enabling the clk.
> > Maybe we should do that? It would mean that we have to schedule a work
> > from the enable path to update the rate accounting outside of any
> > atomic context.
> > 
> > Just thinking out loud, the simpler solution is to probably drop all
> > rate caching in the CCF and get the frequency on a clk_get_rate() call.
> 
> The above is just enough for me. recalc_rate returning 0 is valid, and
> should be taken properly into account.
> 
> Longer term I guess it makes sense to drop the rate caching, but I've
> introduced enough clock regressions already for a couple of releases :)

Ok.

> 
> > It complicates some of the core though when we check to see if we need
> > to update clk rates. We could have some middle ground where drivers
> > indicate that they want to update their cached rate because it's valid
> > now (either from their enable path or from somewhere else). This may be
> > nice actually because we could have clk providers call this to force a
> > recalc down the tree from where they've updated. I think the qcom
> > DisplayPort phy would want this.
> 
> A good workaround for now would be to set CLK_GET_RATE_NOCACHE for those
> clocks.

It's not a good workaround. The clk flag isn't used internally. That's
the problem. It's only used for callers of clk_get_rate(), but we have
rate setting logic where a child doesn't know that the rate should be
recalculated for the parent. It's also not a transitive flag, so if a
clk with the flag set has children we don't care that clk_get_rate() is
called on those children when it should really call up to the parent and
figure out how high to go before calculating the frequency down. This is
where an API that pushes the information to the framework makes sense so
that we can push the update instead of pull it on clk_get_rate().
Anyway, it's a problem for another day.

> 
> > > 
> > > I think the latter is the proper interpretation though based on what the
> > > drivers are doing, and even the CCF itself will call recalc_rate without
> > > making sure that the clock is enabled (in __clk_core_init() for example).
> > > 
> > > Then there is the question of whether returning 0 is fine. Again
> > > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > > returns 0.". This is indeed returned in case of an error condition (in
> > > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> > 
> > A NULL clk isn't an error. We use NULL in the CCF to indicate that it's
> > an optional clk. Returning 0 from clk_get_rate() is not an error. If
> > clk_get() returns an error pointer then it's an error. And NULL isn't an
> > error value per PTR_ERR() (because NULL == 0 when casted, this isn't
> > golang).
> 
> Ok. So we can either group both the "we couldn't figure out a rate" and
> the "our clock is NULL" case as the same case in clk_get_rate(), or we
> can convert clk_get_rate() to return signed integers and a negative
> error code. The latter seems much more intrusive to me and will probably
> lead to a whole bunch of regressions, so I'd be more inclined to stick
> to the former. Especially since drivers seem to either don't care or
> treat 0 as an error already.

Agreed. We can try to work around the "not ready to figure out the rate"
problem by implementing EPROBE_DEFER for clks that are orphans. That
needs some thought around cases where clks have parents that aren't
described in the kernel and we want to reparent them via clk_set_rate()
or clk_set_parent() or their parent is provided by the driver getting
the clk (this latter case is a head spinner). In general indicating this
not fully registered state through clk_get_rate() is wrong though, hence
the decision to make clk_get_rate() always return something valid and
have the "don't know" case be returning zero.

Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

[Maxime Ripard]

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

On Thu, Apr 28, 2022 at 07:08:10PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-23 02:17:24)
> > On Fri, Apr 22, 2022 at 09:42:26PM -0700, Stephen Boyd wrote:
> > > I enjoy the ambiguity! :) This question has come up before and it
> > > doesn't really matter. Drivers can call clk_prepare_enable() if they
> > > want to be sure that clk_get_rate() is meaningful to them, or they can
> > > not.
> > 
> > Right, but that alone already removes the ambiguity :)
> > 
> > If not calling clk_enable() prior to calling clk_get_rate() is ok, then
> > we don't *need* to call it, both are valid.
> > 
> > Therefore, the "you need to call clk_enable() before calling
> > clk_get_rate()" interpretation isn't the right one, right?
> 
> It depends. The CCF isn't the only implementation of the clk API, so
> some drivers want to maintain API compatibility and call clk_enable()
> before getting the rate to make sure the rate is valid. If all other clk
> API implementations are removed or made to work without calling
> clk_enable() we should be able to update the clk.h documentation.
> 
> > 
> > > The CCF returns a rate that it gets from calling recalc_rate, which
> > > could be inaccurate for others reasons, either because some driver has
> > > called clk_set_rate() after the clk_get_rate()
> > 
> > Right, but I guess that's ok, the CCF never claimed to support atomicity
> > in any way. That could also be the result of a parent changing its rate,
> > or a reparenting, etc.
> > 
> > Still, it means that it was the best estimate we could do when
> > clk_get_rate() was called. It just turned out to be short-lived.
> > 
> > > or because the clk is an orphan still and clk_get() succeeded
> > 
> > In this case, core->parent is NULL, so we could argue that it's the same
> > case than clk_get_rate(NULL). It makes sense.
> > 
> > > or because the clk_op couldn't calculate it at the time of caching.
> > 
> > I guess this is what the discussion is all about, whether it's actually
> > something that we should find it acceptable or not.
> > 
> > From what you're saying, it is indeed acceptable and thus clk_get_rate()
> > can return 0 not only from a NULL clock, but also if it's incapable of
> > figuring out a rate for that clock. We should thus document that
> > recalc_rate can indeed return 0 in such a case, and that users should
> > take this into account and not always expect a valid rate.
> 
> I don't think there's anything to update.
> 
> > 
> > Would a documentation for recalc_rate like that:
> > 
> > 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. If
> > the driver cannot figure out a rate for this clock, it must return 0.
> 
> What would the driver return besides 0 if the rate can't be calculated?
> UINT_MAX?

Well, we could expect the driver to never fail, and always be able to
calculate its rate (provided its parent is there). This is what this
patch is doing essentially, and it was working fine on a number of
platforms, so it doesn't seem unfeasible.

Either way, I'm not sure what your pushback is about here. Whether or
not the driver can or cannot return 0 on failure, that should be made
clear in the documentation, and for now it isn't.

If we want to maintain the status quo, then it means that recalc_rate
can return 0 on error, what's the harm in clearly stating that?

> Does the framework care that the rate couldn't be calculated?

I'd say to some extent, yes. The clk_set_rate_range() rework could just
ignore the call to clk_set_rate() if the current rate is 0. That would
have saved us from a couple of regressions.

> Weakening "must" to "should" or saying it "will most likely return 0" is
> better. But again I don't see much point in clarifying here. 0 Hz is a
> valid frequency. Maybe we should just say that 0 is valid?

Ok, so it's about the "must" part.

Yeah, maybe that's the best solution? I don't know, but the current
comment hints a bit at the "0 is an error" part with the reference to a
NULL clk.

> > Optional, but recommended - if this op is not set then clock rate will
> > be initialized to 0.
> > 
> > And for clk_get_rate() (in drivers/clk/clk.c):
> > 
> > Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE
> > flag is set, which means a recalc_rate will be issued. Can be called if
> > the clock is enabled or not. If clk is NULL, or if an error occurred,
> > then returns 0.
> 
> We should remove the clk_get_rate() documentation in clk.c and let the
> clk.h documentation be the one that is used. That will clarify that
> clk.c implements the API in clk.h. I've been meaning to clean up the
> documentation so I'll hack on that tomorrow and see if I can make this
> better.

ok, thanks :)

> > Work for everyone?
> > 
> > > Indeed the CCF doesn't try to recalc the rate after enabling the clk.
> > > Maybe we should do that? It would mean that we have to schedule a work
> > > from the enable path to update the rate accounting outside of any
> > > atomic context.
> > > 
> > > Just thinking out loud, the simpler solution is to probably drop all
> > > rate caching in the CCF and get the frequency on a clk_get_rate() call.
> > 
> > The above is just enough for me. recalc_rate returning 0 is valid, and
> > should be taken properly into account.
> > 
> > Longer term I guess it makes sense to drop the rate caching, but I've
> > introduced enough clock regressions already for a couple of releases :)
> 
> Ok.
> 
> > 
> > > It complicates some of the core though when we check to see if we need
> > > to update clk rates. We could have some middle ground where drivers
> > > indicate that they want to update their cached rate because it's valid
> > > now (either from their enable path or from somewhere else). This may be
> > > nice actually because we could have clk providers call this to force a
> > > recalc down the tree from where they've updated. I think the qcom
> > > DisplayPort phy would want this.
> > 
> > A good workaround for now would be to set CLK_GET_RATE_NOCACHE for those
> > clocks.
> 
> It's not a good workaround. The clk flag isn't used internally. That's
> the problem. It's only used for callers of clk_get_rate(), but we have
> rate setting logic where a child doesn't know that the rate should be
> recalculated for the parent. It's also not a transitive flag, so if a
> clk with the flag set has children we don't care that clk_get_rate() is
> called on those children when it should really call up to the parent and
> figure out how high to go before calculating the frequency down. This is
> where an API that pushes the information to the framework makes sense so
> that we can push the update instead of pull it on clk_get_rate().

Ah, I see what you mean, thanks

> Anyway, it's a problem for another day.

Yeah, I have a can of worms big enough as it is :)

> > > > I think the latter is the proper interpretation though based on what the
> > > > drivers are doing, and even the CCF itself will call recalc_rate without
> > > > making sure that the clock is enabled (in __clk_core_init() for example).
> > > > 
> > > > Then there is the question of whether returning 0 is fine. Again
> > > > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > > > returns 0.". This is indeed returned in case of an error condition (in
> > > > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> > > 
> > > A NULL clk isn't an error. We use NULL in the CCF to indicate that it's
> > > an optional clk. Returning 0 from clk_get_rate() is not an error. If
> > > clk_get() returns an error pointer then it's an error. And NULL isn't an
> > > error value per PTR_ERR() (because NULL == 0 when casted, this isn't
> > > golang).
> > 
> > Ok. So we can either group both the "we couldn't figure out a rate" and
> > the "our clock is NULL" case as the same case in clk_get_rate(), or we
> > can convert clk_get_rate() to return signed integers and a negative
> > error code. The latter seems much more intrusive to me and will probably
> > lead to a whole bunch of regressions, so I'd be more inclined to stick
> > to the former. Especially since drivers seem to either don't care or
> > treat 0 as an error already.
> 
> Agreed. We can try to work around the "not ready to figure out the rate"
> problem by implementing EPROBE_DEFER for clks that are orphans. That
> needs some thought around cases where clks have parents that aren't
> described in the kernel and we want to reparent them via clk_set_rate()
> or clk_set_parent() or their parent is provided by the driver getting
> the clk (this latter case is a head spinner). In general indicating this
> not fully registered state through clk_get_rate() is wrong though, hence
> the decision to make clk_get_rate() always return something valid and
> have the "don't know" case be returning zero.

Ok

maxime

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