[PATCH v3 00/10] clk: Improve clock range handling

[PATCH v3 00/10] clk: Improve clock range handling

[Maxime Ripard]

Hi,

This is a follow-up of the discussion here:
https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/

and here:
https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/

While the initial proposal implemented a new API to temporarily raise and lower
clock rates based on consumer workloads, Stephen suggested an
alternative approach implemented here.

The main issue that needed to be addressed in our case was that in a
situation where we would have multiple calls to clk_set_rate_range, we
would end up with a clock at the maximum of the minimums being set. This
would be expected, but the issue was that if one of the users was to
relax or drop its requirements, the rate would be left unchanged, even
though the ideal rate would have changed.

So something like

clk_set_rate(user1_clk, 1000);
clk_set_min_rate(user1_clk, 2000);
clk_set_min_rate(user2_clk, 3000);
clk_set_min_rate(user2_clk, 1000);

Would leave the clock running at 3000Hz, while the minimum would now be
2000Hz.

This was mostly due to the fact that the core only triggers a rate
change in clk_set_rate_range() if the current rate is outside of the
boundaries, but not if it's within the new boundaries.

That series changes that and will trigger a rate change on every call,
with the former rate being tried again. This way, providers have a
chance to follow whatever policy they see fit for a given clock each
time the boundaries change.

This series also implements some kunit tests, first to test a few rate
related functions in the CCF, and then extends it to make sure that
behaviour has some test coverage.

Let me know what you think
Maxime

Changes from v2:
  - Rebased on current next
  - Rewrote the whole thing according to Stephen reviews
  - Implemented some kunit tests

Changes from v1:
  - Return NULL in clk_request_start if clk pointer is NULL
  - Test for clk_req pointer in clk_request_done
  - Add another user in vc4
  - Rebased on top of v5.15-rc1

Maxime Ripard (10):
  clk: Add Kunit tests for rate
  clk: Always clamp the rounded rate
  clk: Use clamp instead of open-coding our own
  clk: Always set the rate on clk_set_range_rate
  clk: Add clk_drop_range
  clk: bcm: rpi: Add variant structure
  clk: bcm: rpi: Set a default minimum rate
  clk: bcm: rpi: Run some clocks at the minimum rate allowed
  drm/vc4: Add logging and comments
  drm/vc4: hdmi: Remove clock rate initialization

 drivers/clk/Kconfig               |   7 +
 drivers/clk/Makefile              |   1 +
 drivers/clk/bcm/clk-raspberrypi.c | 125 ++++++-
 drivers/clk/clk-rate-test.c       | 603 ++++++++++++++++++++++++++++++
 drivers/clk/clk.c                 |  51 ++-
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  13 -
 drivers/gpu/drm/vc4/vc4_kms.c     |  11 +
 include/linux/clk.h               |  11 +
 8 files changed, 767 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk-rate-test.c

-- 
2.34.1

[PATCH v3 00/10] clk: Improve clock range handling

[Maxime Ripard]

Hi,

This is a follow-up of the discussion here:
https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/

and here:
https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/

While the initial proposal implemented a new API to temporarily raise and lower
clock rates based on consumer workloads, Stephen suggested an
alternative approach implemented here.

The main issue that needed to be addressed in our case was that in a
situation where we would have multiple calls to clk_set_rate_range, we
would end up with a clock at the maximum of the minimums being set. This
would be expected, but the issue was that if one of the users was to
relax or drop its requirements, the rate would be left unchanged, even
though the ideal rate would have changed.

So something like

clk_set_rate(user1_clk, 1000);
clk_set_min_rate(user1_clk, 2000);
clk_set_min_rate(user2_clk, 3000);
clk_set_min_rate(user2_clk, 1000);

Would leave the clock running at 3000Hz, while the minimum would now be
2000Hz.

This was mostly due to the fact that the core only triggers a rate
change in clk_set_rate_range() if the current rate is outside of the
boundaries, but not if it's within the new boundaries.

That series changes that and will trigger a rate change on every call,
with the former rate being tried again. This way, providers have a
chance to follow whatever policy they see fit for a given clock each
time the boundaries change.

This series also implements some kunit tests, first to test a few rate
related functions in the CCF, and then extends it to make sure that
behaviour has some test coverage.

Let me know what you think
Maxime

Changes from v2:
  - Rebased on current next
  - Rewrote the whole thing according to Stephen reviews
  - Implemented some kunit tests

Changes from v1:
  - Return NULL in clk_request_start if clk pointer is NULL
  - Test for clk_req pointer in clk_request_done
  - Add another user in vc4
  - Rebased on top of v5.15-rc1

Maxime Ripard (10):
  clk: Add Kunit tests for rate
  clk: Always clamp the rounded rate
  clk: Use clamp instead of open-coding our own
  clk: Always set the rate on clk_set_range_rate
  clk: Add clk_drop_range
  clk: bcm: rpi: Add variant structure
  clk: bcm: rpi: Set a default minimum rate
  clk: bcm: rpi: Run some clocks at the minimum rate allowed
  drm/vc4: Add logging and comments
  drm/vc4: hdmi: Remove clock rate initialization

 drivers/clk/Kconfig               |   7 +
 drivers/clk/Makefile              |   1 +
 drivers/clk/bcm/clk-raspberrypi.c | 125 ++++++-
 drivers/clk/clk-rate-test.c       | 603 ++++++++++++++++++++++++++++++
 drivers/clk/clk.c                 |  51 ++-
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  13 -
 drivers/gpu/drm/vc4/vc4_kms.c     |  11 +
 include/linux/clk.h               |  11 +
 8 files changed, 767 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk-rate-test.c

-- 
2.34.1

[PATCH v3 01/10] clk: Add Kunit tests for rate

[Maxime Ripard]

Let's test various parts of the rate-related clock API with the kunit
testing framework.

Cc: kunit-dev@googlegroups.com
Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/Kconfig         |   7 +
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/clk/clk-rate-test.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index f5807d190ba2..7ae48a91f738 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -436,4 +436,11 @@ config CLK_GATE_TEST
 	help
 	  Kunit test for the basic clk gate type.
 
+config CLK_RATE_TEST
+	tristate "Basic Core Rate Kunit Tests"
+	depends on KUNIT
+	default KUNIT
+	help
+	  Kunit test for the basic clock rate management.
+
 endif
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b940c6d35922..0238a595167a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,6 +2,7 @@
 # common clock types
 obj-$(CONFIG_HAVE_CLK)		+= clk-devres.o clk-bulk.o clkdev.o
 obj-$(CONFIG_COMMON_CLK)	+= clk.o
+obj-$(CONFIG_CLK_RATE_TEST)	+= clk-rate-test.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-divider.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
new file mode 100644
index 000000000000..f2d3df791b5a
--- /dev/null
+++ b/drivers/clk/clk-rate-test.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kunit test for clk rate management
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+
+#include <kunit/test.h>
+
+#define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_1	(142 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_2	(242 * 1000 * 1000)
+
+struct clk_dummy_rate_context {
+	struct clk_hw hw;
+	unsigned long rate;
+};
+
+static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct clk_dummy_rate_context *ctx =
+		container_of(hw, struct clk_dummy_rate_context, hw);
+
+	return ctx->rate;
+}
+
+static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
+					 struct clk_rate_request *req)
+{
+	/* Just return the same rate without modifying it */
+	return 0;
+}
+
+static int clk_dummy_rate_set_rate(struct clk_hw *hw,
+				   unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct clk_dummy_rate_context *ctx =
+		container_of(hw, struct clk_dummy_rate_context, hw);
+
+	ctx->rate = rate;
+	return 0;
+}
+
+const static struct clk_ops clk_dummy_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_determine_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
+static int clk_rate_test_init_with_ops(struct kunit *test,
+				       const struct clk_ops *ops)
+{
+	struct clk_dummy_rate_context *ctx;
+	struct clk_init_data init = { };
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	ctx->rate = DUMMY_CLOCK_INIT_RATE;
+	test->priv = ctx;
+
+	init.name = "test_dummy_rate";
+	init.ops = ops;
+	ctx->hw.init = &init;
+
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int clk_rate_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
+}
+
+static void clk_rate_test_exit(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	kfree(ctx);
+}
+
+/*
+ * Test that the actual rate matches what is returned by clk_get_rate()
+ */
+static void clk_rate_test_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, ctx->rate);
+}
+
+/*
+ * Test that, after a call to clk_set_rate(), the rate returned by
+ * clk_get_rate() matches.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_test_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that, after several calls to clk_set_rate(), the rate returned
+ * by clk_get_rate() matches the last one.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_test_set_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_rate_test_cases[] = {
+	KUNIT_CASE(clk_rate_test_get_rate),
+	KUNIT_CASE(clk_rate_test_set_get_rate),
+	KUNIT_CASE(clk_rate_test_set_set_get_rate),
+	{}
+};
+
+static struct kunit_suite clk_rate_test_suite = {
+	.name = "clk-rate-test",
+	.init = clk_rate_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_test_cases,
+};
+
+/*
+ * Test that clk_set_rate_range won't return an error for a valid range.
+ */
+static void clk_rate_range_test_set_range(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	int ret;
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+}
+
+/*
+ * Test that calling clk_set_rate_range with a minimum rate higher than
+ * the maximum rate returns an error.
+ */
+static void clk_rate_range_test_set_range_invalid(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	int ret;
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1 + 1000,
+				 DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+}
+
+/*
+ * Test that if our clock has a rate lower than the minimum set by a
+ * call to clk_set_rate_range(), the rate will be raised to match the
+ * new minimum.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that if our clock has a rate higher than the maximum set by a
+ * call to clk_set_rate_range(), the rate will be lowered to match the
+ * new maximum.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_rate_range_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range),
+	KUNIT_CASE(clk_rate_range_test_set_range_invalid),
+	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised),
+	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_test_suite = {
+	.name = "clk-rate-range-test",
+	.init = clk_rate_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_test_cases,
+};
+
+kunit_test_suites(
+	&clk_rate_test_suite,
+	&clk_rate_range_test_suite
+);
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

[PATCH v3 01/10] clk: Add Kunit tests for rate

[Maxime Ripard]

Let's test various parts of the rate-related clock API with the kunit
testing framework.

Cc: kunit-dev@googlegroups.com
Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/Kconfig         |   7 +
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/clk/clk-rate-test.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index f5807d190ba2..7ae48a91f738 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -436,4 +436,11 @@ config CLK_GATE_TEST
 	help
 	  Kunit test for the basic clk gate type.
 
+config CLK_RATE_TEST
+	tristate "Basic Core Rate Kunit Tests"
+	depends on KUNIT
+	default KUNIT
+	help
+	  Kunit test for the basic clock rate management.
+
 endif
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b940c6d35922..0238a595167a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,6 +2,7 @@
 # common clock types
 obj-$(CONFIG_HAVE_CLK)		+= clk-devres.o clk-bulk.o clkdev.o
 obj-$(CONFIG_COMMON_CLK)	+= clk.o
+obj-$(CONFIG_CLK_RATE_TEST)	+= clk-rate-test.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-divider.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
new file mode 100644
index 000000000000..f2d3df791b5a
--- /dev/null
+++ b/drivers/clk/clk-rate-test.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kunit test for clk rate management
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+
+#include <kunit/test.h>
+
+#define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_1	(142 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_2	(242 * 1000 * 1000)
+
+struct clk_dummy_rate_context {
+	struct clk_hw hw;
+	unsigned long rate;
+};
+
+static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct clk_dummy_rate_context *ctx =
+		container_of(hw, struct clk_dummy_rate_context, hw);
+
+	return ctx->rate;
+}
+
+static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
+					 struct clk_rate_request *req)
+{
+	/* Just return the same rate without modifying it */
+	return 0;
+}
+
+static int clk_dummy_rate_set_rate(struct clk_hw *hw,
+				   unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct clk_dummy_rate_context *ctx =
+		container_of(hw, struct clk_dummy_rate_context, hw);
+
+	ctx->rate = rate;
+	return 0;
+}
+
+const static struct clk_ops clk_dummy_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_determine_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
+static int clk_rate_test_init_with_ops(struct kunit *test,
+				       const struct clk_ops *ops)
+{
+	struct clk_dummy_rate_context *ctx;
+	struct clk_init_data init = { };
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	ctx->rate = DUMMY_CLOCK_INIT_RATE;
+	test->priv = ctx;
+
+	init.name = "test_dummy_rate";
+	init.ops = ops;
+	ctx->hw.init = &init;
+
+	ret = clk_hw_register(NULL, &ctx->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int clk_rate_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
+}
+
+static void clk_rate_test_exit(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+	kfree(ctx);
+}
+
+/*
+ * Test that the actual rate matches what is returned by clk_get_rate()
+ */
+static void clk_rate_test_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, ctx->rate);
+}
+
+/*
+ * Test that, after a call to clk_set_rate(), the rate returned by
+ * clk_get_rate() matches.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_test_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that, after several calls to clk_set_rate(), the rate returned
+ * by clk_get_rate() matches the last one.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_test_set_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_rate_test_cases[] = {
+	KUNIT_CASE(clk_rate_test_get_rate),
+	KUNIT_CASE(clk_rate_test_set_get_rate),
+	KUNIT_CASE(clk_rate_test_set_set_get_rate),
+	{}
+};
+
+static struct kunit_suite clk_rate_test_suite = {
+	.name = "clk-rate-test",
+	.init = clk_rate_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_test_cases,
+};
+
+/*
+ * Test that clk_set_rate_range won't return an error for a valid range.
+ */
+static void clk_rate_range_test_set_range(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	int ret;
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+}
+
+/*
+ * Test that calling clk_set_rate_range with a minimum rate higher than
+ * the maximum rate returns an error.
+ */
+static void clk_rate_range_test_set_range_invalid(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	int ret;
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1 + 1000,
+				 DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+}
+
+/*
+ * Test that if our clock has a rate lower than the minimum set by a
+ * call to clk_set_rate_range(), the rate will be raised to match the
+ * new minimum.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that if our clock has a rate higher than the maximum set by a
+ * call to clk_set_rate_range(), the rate will be lowered to match the
+ * new maximum.
+ *
+ * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
+ * modify the requested rate, which is our case in clk_dummy_rate_ops.
+ */
+static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_rate_range_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range),
+	KUNIT_CASE(clk_rate_range_test_set_range_invalid),
+	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised),
+	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_test_suite = {
+	.name = "clk-rate-range-test",
+	.init = clk_rate_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_test_cases,
+};
+
+kunit_test_suites(
+	&clk_rate_test_suite,
+	&clk_rate_range_test_suite
+);
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-20 06:34:08)
> Let's test various parts of the rate-related clock API with the kunit
> testing framework.
> 
> Cc: kunit-dev@googlegroups.com
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

This is great! Thanks for doing this.

>  drivers/clk/Kconfig         |   7 +
>  drivers/clk/Makefile        |   1 +
>  drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/clk/clk-rate-test.c

I was thinking this would be more generic so that one file tests clk.c
and all the code in there, but I guess there may be config dependencies
like CONFIG_OF that we may want to extract out and depend on
differently. I'm not sure how kunit will handle testing different paths
depending on build configuration so this approach may head off future
problems. If it doesn't then we can always slam the test together.

> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index f5807d190ba2..7ae48a91f738 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -436,4 +436,11 @@ config CLK_GATE_TEST
>         help
>           Kunit test for the basic clk gate type.
>  
> +config CLK_RATE_TEST

See v3 here[1] and have it follow that.

> +       tristate "Basic Core Rate Kunit Tests"
> +       depends on KUNIT
> +       default KUNIT
> +       help
> +         Kunit test for the basic clock rate management.
> +
>  endif
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index b940c6d35922..0238a595167a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -2,6 +2,7 @@
>  # common clock types
>  obj-$(CONFIG_HAVE_CLK)         += clk-devres.o clk-bulk.o clkdev.o
>  obj-$(CONFIG_COMMON_CLK)       += clk.o
> +obj-$(CONFIG_CLK_RATE_TEST)    += clk-rate-test.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
> diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
> new file mode 100644
> index 000000000000..f2d3df791b5a
> --- /dev/null
> +++ b/drivers/clk/clk-rate-test.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for clk rate management
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +
> +#include <kunit/test.h>
> +
> +#define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_1     (142 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_2     (242 * 1000 * 1000)
> +
> +struct clk_dummy_rate_context {
> +       struct clk_hw hw;
> +       unsigned long rate;
> +};
> +
> +static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       return ctx->rate;
> +}
> +
> +static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
> +                                        struct clk_rate_request *req)
> +{
> +       /* Just return the same rate without modifying it */
> +       return 0;
> +}
> +
> +static int clk_dummy_rate_set_rate(struct clk_hw *hw,
> +                                  unsigned long rate,
> +                                  unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       ctx->rate = rate;
> +       return 0;
> +}
> +
> +const static struct clk_ops clk_dummy_rate_ops = {

static const?

> +       .recalc_rate = clk_dummy_rate_recalc_rate,
> +       .determine_rate = clk_dummy_rate_determine_rate,
> +       .set_rate = clk_dummy_rate_set_rate,
> +};
> +
> +static int clk_rate_test_init_with_ops(struct kunit *test,
> +                                      const struct clk_ops *ops)
> +{
> +       struct clk_dummy_rate_context *ctx;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

Use kunit_kzalloc() here

> +       if (!ctx)
> +               return -ENOMEM;
> +       ctx->rate = DUMMY_CLOCK_INIT_RATE;
> +       test->priv = ctx;
> +
> +       init.name = "test_dummy_rate";
> +       init.ops = ops;
> +       ctx->hw.init = &init;
> +
> +       ret = clk_hw_register(NULL, &ctx->hw);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int clk_rate_test_init(struct kunit *test)
> +{
> +       return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
> +}
> +
> +static void clk_rate_test_exit(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +
> +       clk_hw_unregister(&ctx->hw);
> +       kfree(ctx);

And drop the kfree as it is now test managed.

> +}
> +
> +/*
> + * Test that the actual rate matches what is returned by clk_get_rate()
> + */
> +static void clk_rate_test_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_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_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, ctx->rate);

These should be KUNIT_EXPECT_*() as we don't want to stop the test if
the rate is wrong, we want to check that the rate is what we expected it
to be. Assertions are about making sure things are sane and if not we
should stop testing, whereas expectations are about testing the code. A
test must have an EXPECT while it can have an ASSERT.

Maybe kunit should check that there was an EXPECT on return from the
test. Daniel?

> +}
> +
> +/*
> + * Test that, after a call to clk_set_rate(), the rate returned by
> + * clk_get_rate() matches.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

I'd like to throw the ret check directly into KUNIT_ASSERT_EQ() here.

	KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1), 0);

so we can easily see if something goes wrong that the set rate failed.
Good use of assert here, we don't want to continue with the test unless
the set rate actually worked.

> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * Test that, after several calls to clk_set_rate(), the rate returned
> + * by clk_get_rate() matches the last one.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_rate_test_cases[] = {
> +       KUNIT_CASE(clk_rate_test_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_set_get_rate),
> +       {}
> +};
> +
> +static struct kunit_suite clk_rate_test_suite = {
> +       .name = "clk-rate-test",
> +       .init = clk_rate_test_init,
> +       .exit = clk_rate_test_exit,
> +       .test_cases = clk_rate_test_cases,
> +};
> +
> +/*
> + * Test that clk_set_rate_range won't return an error for a valid range.
> + */
> +static void clk_rate_range_test_set_range(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1,
> +                                DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

Also make sure that the rate is within the bounds of rate_1 and rate_2?

> +}
> +
> +/*
> + * Test that calling clk_set_rate_range with a minimum rate higher than
> + * the maximum rate returns an error.
> + */
> +static void clk_rate_range_test_set_range_invalid(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1 + 1000,
> +                                DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);

Let's not check for a specific error, but a negative value instead. I'd
rather not encode particular error values unless we need them to be
particular.

> +}
> +
> +/*
> + * Test that if our clock has a rate lower than the minimum set by a
> + * call to clk_set_rate_range(), the rate will be raised to match the
> + * new minimum.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);

KUNIT_EXPECT_LE(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_2);

Or just drop it entirely as DUMMY_CLOCK_RATE_1 is greater than 0 and
it's tested next.

> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

KUNIT_EXPECT_EQ(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);

> +}
> +

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-20 06:34:08)
> Let's test various parts of the rate-related clock API with the kunit
> testing framework.
> 
> Cc: kunit-dev@googlegroups.com
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

This is great! Thanks for doing this.

>  drivers/clk/Kconfig         |   7 +
>  drivers/clk/Makefile        |   1 +
>  drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/clk/clk-rate-test.c

I was thinking this would be more generic so that one file tests clk.c
and all the code in there, but I guess there may be config dependencies
like CONFIG_OF that we may want to extract out and depend on
differently. I'm not sure how kunit will handle testing different paths
depending on build configuration so this approach may head off future
problems. If it doesn't then we can always slam the test together.

> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index f5807d190ba2..7ae48a91f738 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -436,4 +436,11 @@ config CLK_GATE_TEST
>         help
>           Kunit test for the basic clk gate type.
>  
> +config CLK_RATE_TEST

See v3 here[1] and have it follow that.

> +       tristate "Basic Core Rate Kunit Tests"
> +       depends on KUNIT
> +       default KUNIT
> +       help
> +         Kunit test for the basic clock rate management.
> +
>  endif
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index b940c6d35922..0238a595167a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -2,6 +2,7 @@
>  # common clock types
>  obj-$(CONFIG_HAVE_CLK)         += clk-devres.o clk-bulk.o clkdev.o
>  obj-$(CONFIG_COMMON_CLK)       += clk.o
> +obj-$(CONFIG_CLK_RATE_TEST)    += clk-rate-test.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
> diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
> new file mode 100644
> index 000000000000..f2d3df791b5a
> --- /dev/null
> +++ b/drivers/clk/clk-rate-test.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for clk rate management
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +
> +#include <kunit/test.h>
> +
> +#define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_1     (142 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_2     (242 * 1000 * 1000)
> +
> +struct clk_dummy_rate_context {
> +       struct clk_hw hw;
> +       unsigned long rate;
> +};
> +
> +static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       return ctx->rate;
> +}
> +
> +static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
> +                                        struct clk_rate_request *req)
> +{
> +       /* Just return the same rate without modifying it */
> +       return 0;
> +}
> +
> +static int clk_dummy_rate_set_rate(struct clk_hw *hw,
> +                                  unsigned long rate,
> +                                  unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       ctx->rate = rate;
> +       return 0;
> +}
> +
> +const static struct clk_ops clk_dummy_rate_ops = {

static const?

> +       .recalc_rate = clk_dummy_rate_recalc_rate,
> +       .determine_rate = clk_dummy_rate_determine_rate,
> +       .set_rate = clk_dummy_rate_set_rate,
> +};
> +
> +static int clk_rate_test_init_with_ops(struct kunit *test,
> +                                      const struct clk_ops *ops)
> +{
> +       struct clk_dummy_rate_context *ctx;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

Use kunit_kzalloc() here

> +       if (!ctx)
> +               return -ENOMEM;
> +       ctx->rate = DUMMY_CLOCK_INIT_RATE;
> +       test->priv = ctx;
> +
> +       init.name = "test_dummy_rate";
> +       init.ops = ops;
> +       ctx->hw.init = &init;
> +
> +       ret = clk_hw_register(NULL, &ctx->hw);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int clk_rate_test_init(struct kunit *test)
> +{
> +       return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
> +}
> +
> +static void clk_rate_test_exit(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +
> +       clk_hw_unregister(&ctx->hw);
> +       kfree(ctx);

And drop the kfree as it is now test managed.

> +}
> +
> +/*
> + * Test that the actual rate matches what is returned by clk_get_rate()
> + */
> +static void clk_rate_test_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_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_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, ctx->rate);

These should be KUNIT_EXPECT_*() as we don't want to stop the test if
the rate is wrong, we want to check that the rate is what we expected it
to be. Assertions are about making sure things are sane and if not we
should stop testing, whereas expectations are about testing the code. A
test must have an EXPECT while it can have an ASSERT.

Maybe kunit should check that there was an EXPECT on return from the
test. Daniel?

> +}
> +
> +/*
> + * Test that, after a call to clk_set_rate(), the rate returned by
> + * clk_get_rate() matches.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

I'd like to throw the ret check directly into KUNIT_ASSERT_EQ() here.

	KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1), 0);

so we can easily see if something goes wrong that the set rate failed.
Good use of assert here, we don't want to continue with the test unless
the set rate actually worked.

> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * Test that, after several calls to clk_set_rate(), the rate returned
> + * by clk_get_rate() matches the last one.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_rate_test_cases[] = {
> +       KUNIT_CASE(clk_rate_test_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_set_get_rate),
> +       {}
> +};
> +
> +static struct kunit_suite clk_rate_test_suite = {
> +       .name = "clk-rate-test",
> +       .init = clk_rate_test_init,
> +       .exit = clk_rate_test_exit,
> +       .test_cases = clk_rate_test_cases,
> +};
> +
> +/*
> + * Test that clk_set_rate_range won't return an error for a valid range.
> + */
> +static void clk_rate_range_test_set_range(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1,
> +                                DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

Also make sure that the rate is within the bounds of rate_1 and rate_2?

> +}
> +
> +/*
> + * Test that calling clk_set_rate_range with a minimum rate higher than
> + * the maximum rate returns an error.
> + */
> +static void clk_rate_range_test_set_range_invalid(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1 + 1000,
> +                                DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);

Let's not check for a specific error, but a negative value instead. I'd
rather not encode particular error values unless we need them to be
particular.

> +}
> +
> +/*
> + * Test that if our clock has a rate lower than the minimum set by a
> + * call to clk_set_rate_range(), the rate will be raised to match the
> + * new minimum.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);

KUNIT_EXPECT_LE(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_2);

Or just drop it entirely as DUMMY_CLOCK_RATE_1 is greater than 0 and
it's tested next.

> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

KUNIT_EXPECT_EQ(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);

> +}
> +

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Daniel Latypov]

On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> I was thinking this would be more generic so that one file tests clk.c
> and all the code in there, but I guess there may be config dependencies
> like CONFIG_OF that we may want to extract out and depend on
> differently. I'm not sure how kunit will handle testing different paths
> depending on build configuration so this approach may head off future
> problems. If it doesn't then we can always slam the test together.

KUnit doesn't have hard technical limitations in this regard.

You could have something like this

static void my_optional_kunit_test(struct kunit *test)
{
#ifdef CONFIG_OPTIONAL_FEATURE

# else
  kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
#endif
}

I think it's just a matter of what's least confusing to users.

> > +}
> > +
> > +/*
> > + * Test that the actual rate matches what is returned by clk_get_rate()
> > + */
> > +static void clk_rate_test_get_rate(struct kunit *test)
> > +{
> > +       struct clk_dummy_rate_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_TRUE(test, rate > 0);

KUNIT_EXPECT_GT(test, rate, 0);

> > +       KUNIT_ASSERT_EQ(test, rate, ctx->rate);
>
> These should be KUNIT_EXPECT_*() as we don't want to stop the test if
> the rate is wrong, we want to check that the rate is what we expected it
> to be. Assertions are about making sure things are sane and if not we
> should stop testing, whereas expectations are about testing the code. A
> test must have an EXPECT while it can have an ASSERT.
>
> Maybe kunit should check that there was an EXPECT on return from the
> test. Daniel?

Sorry, I'm not sure I understand the question.

Are you saying you want kunit to flag cases like
  static void empty_test(struct kunit *) {}
?

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Daniel Latypov]

On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> I was thinking this would be more generic so that one file tests clk.c
> and all the code in there, but I guess there may be config dependencies
> like CONFIG_OF that we may want to extract out and depend on
> differently. I'm not sure how kunit will handle testing different paths
> depending on build configuration so this approach may head off future
> problems. If it doesn't then we can always slam the test together.

KUnit doesn't have hard technical limitations in this regard.

You could have something like this

static void my_optional_kunit_test(struct kunit *test)
{
#ifdef CONFIG_OPTIONAL_FEATURE

# else
  kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
#endif
}

I think it's just a matter of what's least confusing to users.

> > +}
> > +
> > +/*
> > + * Test that the actual rate matches what is returned by clk_get_rate()
> > + */
> > +static void clk_rate_test_get_rate(struct kunit *test)
> > +{
> > +       struct clk_dummy_rate_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_TRUE(test, rate > 0);

KUNIT_EXPECT_GT(test, rate, 0);

> > +       KUNIT_ASSERT_EQ(test, rate, ctx->rate);
>
> These should be KUNIT_EXPECT_*() as we don't want to stop the test if
> the rate is wrong, we want to check that the rate is what we expected it
> to be. Assertions are about making sure things are sane and if not we
> should stop testing, whereas expectations are about testing the code. A
> test must have an EXPECT while it can have an ASSERT.
>
> Maybe kunit should check that there was an EXPECT on return from the
> test. Daniel?

Sorry, I'm not sure I understand the question.

Are you saying you want kunit to flag cases like
  static void empty_test(struct kunit *) {}
?

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Daniel Latypov (2022-01-20 13:56:39)
> On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > I was thinking this would be more generic so that one file tests clk.c
> > and all the code in there, but I guess there may be config dependencies
> > like CONFIG_OF that we may want to extract out and depend on
> > differently. I'm not sure how kunit will handle testing different paths
> > depending on build configuration so this approach may head off future
> > problems. If it doesn't then we can always slam the test together.
> 
> KUnit doesn't have hard technical limitations in this regard.
> 
> You could have something like this
> 
> static void my_optional_kunit_test(struct kunit *test)
> {
> #ifdef CONFIG_OPTIONAL_FEATURE
> 
> # else
>   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> #endif
> }
> 
> I think it's just a matter of what's least confusing to users.

Ok, I see. Is there some way to have multiple configs checked into the
tree so we can test different kernel configuration code paths? This
discussion isn't really relevant to this patch so we can take this up in
another thread if you like.

> 
> >
> > Maybe kunit should check that there was an EXPECT on return from the
> > test. Daniel?
> 
> Sorry, I'm not sure I understand the question.
> 
> Are you saying you want kunit to flag cases like
>   static void empty_test(struct kunit *) {}
> ?

Yes. I'd like kunit to enforce that all tests have at least one
EXPECT_*() in them.

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Daniel Latypov (2022-01-20 13:56:39)
> On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > I was thinking this would be more generic so that one file tests clk.c
> > and all the code in there, but I guess there may be config dependencies
> > like CONFIG_OF that we may want to extract out and depend on
> > differently. I'm not sure how kunit will handle testing different paths
> > depending on build configuration so this approach may head off future
> > problems. If it doesn't then we can always slam the test together.
> 
> KUnit doesn't have hard technical limitations in this regard.
> 
> You could have something like this
> 
> static void my_optional_kunit_test(struct kunit *test)
> {
> #ifdef CONFIG_OPTIONAL_FEATURE
> 
> # else
>   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> #endif
> }
> 
> I think it's just a matter of what's least confusing to users.

Ok, I see. Is there some way to have multiple configs checked into the
tree so we can test different kernel configuration code paths? This
discussion isn't really relevant to this patch so we can take this up in
another thread if you like.

> 
> >
> > Maybe kunit should check that there was an EXPECT on return from the
> > test. Daniel?
> 
> Sorry, I'm not sure I understand the question.
> 
> Are you saying you want kunit to flag cases like
>   static void empty_test(struct kunit *) {}
> ?

Yes. I'd like kunit to enforce that all tests have at least one
EXPECT_*() in them.

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Daniel Latypov]

On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Latypov (2022-01-20 13:56:39)
> > On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > KUnit doesn't have hard technical limitations in this regard.
> >
> > You could have something like this
> >
> > static void my_optional_kunit_test(struct kunit *test)
> > {
> > #ifdef CONFIG_OPTIONAL_FEATURE
> >
> > # else
> >   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> > #endif
> > }
> >
> > I think it's just a matter of what's least confusing to users.
>
> Ok, I see. Is there some way to have multiple configs checked into the
> tree so we can test different kernel configuration code paths? This

Multiple kunitconfigs?
There's no restrictions on those

$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar

The first one will assume drivers/clk/.kunitconfig.
But there's no reason you need to have a file called that.
One could just have multiple standalone kunitconfigs, named however they like.

--kunitconfig is new enough (5.12+) that there's no real conventions yet.

Another option is
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \
   --kconfig_add=CONFIG_RARELY_USED=y

This is another case where we can do whatever is least confusing.

> discussion isn't really relevant to this patch so we can take this up in
> another thread if you like.
>
> >
> > >
> > > Maybe kunit should check that there was an EXPECT on return from the
> > > test. Daniel?
> >
> > Sorry, I'm not sure I understand the question.
> >
> > Are you saying you want kunit to flag cases like
> >   static void empty_test(struct kunit *) {}
> > ?
>
> Yes. I'd like kunit to enforce that all tests have at least one
> EXPECT_*() in them.

I totally understand the rationale.
It's a bit misleading to say PASSED if no expectation/assertion passed.
One might want a NO_STATUS (or maybe SKIPPED) result instead.

But other unit test frameworks act the way KUnit does here, so there's
an argument for consistency with others so users don't have to have a
whole new mental model.

Some examples below for reference.
All of these output something like
  test result: ok. 1 passed; ...

E.g. in Python
import unittest

class ExampleTest(unittest.TestCase):
  def test_foo(self):
    pass

if __name__ == '__main__':
  unittest.main()

In Golang:
package example

import "testing"

func TestFoo(t *testing.T) {}

In C++ using Googletest:
#include "gtest/gtest.h"

TEST(Suite, Case) { }

In Rust:
#[cfg(test)]
mod tests {
    #[test]
    fn test_empty() {}
}

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Daniel Latypov]

On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Latypov (2022-01-20 13:56:39)
> > On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > KUnit doesn't have hard technical limitations in this regard.
> >
> > You could have something like this
> >
> > static void my_optional_kunit_test(struct kunit *test)
> > {
> > #ifdef CONFIG_OPTIONAL_FEATURE
> >
> > # else
> >   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> > #endif
> > }
> >
> > I think it's just a matter of what's least confusing to users.
>
> Ok, I see. Is there some way to have multiple configs checked into the
> tree so we can test different kernel configuration code paths? This

Multiple kunitconfigs?
There's no restrictions on those

$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar

The first one will assume drivers/clk/.kunitconfig.
But there's no reason you need to have a file called that.
One could just have multiple standalone kunitconfigs, named however they like.

--kunitconfig is new enough (5.12+) that there's no real conventions yet.

Another option is
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \
   --kconfig_add=CONFIG_RARELY_USED=y

This is another case where we can do whatever is least confusing.

> discussion isn't really relevant to this patch so we can take this up in
> another thread if you like.
>
> >
> > >
> > > Maybe kunit should check that there was an EXPECT on return from the
> > > test. Daniel?
> >
> > Sorry, I'm not sure I understand the question.
> >
> > Are you saying you want kunit to flag cases like
> >   static void empty_test(struct kunit *) {}
> > ?
>
> Yes. I'd like kunit to enforce that all tests have at least one
> EXPECT_*() in them.

I totally understand the rationale.
It's a bit misleading to say PASSED if no expectation/assertion passed.
One might want a NO_STATUS (or maybe SKIPPED) result instead.

But other unit test frameworks act the way KUnit does here, so there's
an argument for consistency with others so users don't have to have a
whole new mental model.

Some examples below for reference.
All of these output something like
  test result: ok. 1 passed; ...

E.g. in Python
import unittest

class ExampleTest(unittest.TestCase):
  def test_foo(self):
    pass

if __name__ == '__main__':
  unittest.main()

In Golang:
package example

import "testing"

func TestFoo(t *testing.T) {}

In C++ using Googletest:
#include "gtest/gtest.h"

TEST(Suite, Case) { }

In Rust:
#[cfg(test)]
mod tests {
    #[test]
    fn test_empty() {}
}

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Daniel Latypov (2022-01-20 21:25:03)
> On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Latypov (2022-01-20 13:56:39)
> > > On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > KUnit doesn't have hard technical limitations in this regard.
> > >
> > > You could have something like this
> > >
> > > static void my_optional_kunit_test(struct kunit *test)
> > > {
> > > #ifdef CONFIG_OPTIONAL_FEATURE
> > >
> > > # else
> > >   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> > > #endif
> > > }
> > >
> > > I think it's just a matter of what's least confusing to users.
> >
> > Ok, I see. Is there some way to have multiple configs checked into the
> > tree so we can test different kernel configuration code paths? This
> 
> Multiple kunitconfigs?
> There's no restrictions on those
> 
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar
> 
> The first one will assume drivers/clk/.kunitconfig.
> But there's no reason you need to have a file called that.
> One could just have multiple standalone kunitconfigs, named however they like.
> 
> --kunitconfig is new enough (5.12+) that there's no real conventions yet.
> 
> Another option is
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \
>    --kconfig_add=CONFIG_RARELY_USED=y
> 
> This is another case where we can do whatever is least confusing.

Got it, thanks.

> 
> > discussion isn't really relevant to this patch so we can take this up in
> > another thread if you like.
> >
> > >
> > > >
> > > > Maybe kunit should check that there was an EXPECT on return from the
> > > > test. Daniel?
> > >
> > > Sorry, I'm not sure I understand the question.
> > >
> > > Are you saying you want kunit to flag cases like
> > >   static void empty_test(struct kunit *) {}
> > > ?
> >
> > Yes. I'd like kunit to enforce that all tests have at least one
> > EXPECT_*() in them.
> 
> I totally understand the rationale.
> It's a bit misleading to say PASSED if no expectation/assertion passed.
> One might want a NO_STATUS (or maybe SKIPPED) result instead.
> 
> But other unit test frameworks act the way KUnit does here, so there's
> an argument for consistency with others so users don't have to have a
> whole new mental model.

Ok if other test frameworks don't care then there's nothing to do.

Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd]

Quoting Daniel Latypov (2022-01-20 21:25:03)
> On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Latypov (2022-01-20 13:56:39)
> > > On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > KUnit doesn't have hard technical limitations in this regard.
> > >
> > > You could have something like this
> > >
> > > static void my_optional_kunit_test(struct kunit *test)
> > > {
> > > #ifdef CONFIG_OPTIONAL_FEATURE
> > >
> > > # else
> > >   kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled");
> > > #endif
> > > }
> > >
> > > I think it's just a matter of what's least confusing to users.
> >
> > Ok, I see. Is there some way to have multiple configs checked into the
> > tree so we can test different kernel configuration code paths? This
> 
> Multiple kunitconfigs?
> There's no restrictions on those
> 
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar
> 
> The first one will assume drivers/clk/.kunitconfig.
> But there's no reason you need to have a file called that.
> One could just have multiple standalone kunitconfigs, named however they like.
> 
> --kunitconfig is new enough (5.12+) that there's no real conventions yet.
> 
> Another option is
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \
>    --kconfig_add=CONFIG_RARELY_USED=y
> 
> This is another case where we can do whatever is least confusing.

Got it, thanks.

> 
> > discussion isn't really relevant to this patch so we can take this up in
> > another thread if you like.
> >
> > >
> > > >
> > > > Maybe kunit should check that there was an EXPECT on return from the
> > > > test. Daniel?
> > >
> > > Sorry, I'm not sure I understand the question.
> > >
> > > Are you saying you want kunit to flag cases like
> > >   static void empty_test(struct kunit *) {}
> > > ?
> >
> > Yes. I'd like kunit to enforce that all tests have at least one
> > EXPECT_*() in them.
> 
> I totally understand the rationale.
> It's a bit misleading to say PASSED if no expectation/assertion passed.
> One might want a NO_STATUS (or maybe SKIPPED) result instead.
> 
> But other unit test frameworks act the way KUnit does here, so there's
> an argument for consistency with others so users don't have to have a
> whole new mental model.

Ok if other test frameworks don't care then there's nothing to do.

[PATCH v3 02/10] clk: Always clamp the rounded rate

[Maxime Ripard]

The current core while setting the min and max rate properly in the
clk_request structure will not make sure that the requested rate is
within these boundaries, leaving it to each and every driver to make
sure it is.

Add a clamp call to make sure it's always done, and add a few unit tests
to make sure we don't have any regression there.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c | 46 +++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c           |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index f2d3df791b5a..a13b02702d20 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -198,6 +198,50 @@ static void clk_rate_range_test_set_range_invalid(struct kunit *test)
 	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
 }
 
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * lower than the minimum, the returned rate will be within range.
+ */
+static void clk_rate_range_test_set_range_round_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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_FALSE(test, rate < 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * higher than the maximum, the returned rate will be within range.
+ */
+static void clk_rate_range_test_set_range_round_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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_2 + 1000);
+	KUNIT_ASSERT_FALSE(test, rate < 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
 /*
  * Test that if our clock has a rate lower than the minimum set by a
  * call to clk_set_rate_range(), the rate will be raised to match the
@@ -259,6 +303,8 @@ static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test)
 static struct kunit_case clk_rate_range_test_cases[] = {
 	KUNIT_CASE(clk_rate_range_test_set_range),
 	KUNIT_CASE(clk_rate_range_test_set_range_invalid),
+	KUNIT_CASE(clk_rate_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_rate_range_test_set_range_round_rate_higher),
 	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised),
 	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered),
 	{}
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8de6a22498e7..7bb5ae0fb688 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1330,6 +1330,8 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
 	if (!core)
 		return 0;
 
+	req->rate = clamp(req->rate, req->min_rate, req->max_rate);
+
 	/*
 	 * At this point, core protection will be disabled
 	 * - if the provider is not protected at all
-- 
2.34.1

[PATCH v3 02/10] clk: Always clamp the rounded rate

[Maxime Ripard]

The current core while setting the min and max rate properly in the
clk_request structure will not make sure that the requested rate is
within these boundaries, leaving it to each and every driver to make
sure it is.

Add a clamp call to make sure it's always done, and add a few unit tests
to make sure we don't have any regression there.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c | 46 +++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c           |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index f2d3df791b5a..a13b02702d20 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -198,6 +198,50 @@ static void clk_rate_range_test_set_range_invalid(struct kunit *test)
 	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
 }
 
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * lower than the minimum, the returned rate will be within range.
+ */
+static void clk_rate_range_test_set_range_round_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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_FALSE(test, rate < 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * higher than the maximum, the returned rate will be within range.
+ */
+static void clk_rate_range_test_set_range_round_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	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_2 + 1000);
+	KUNIT_ASSERT_FALSE(test, rate < 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
 /*
  * Test that if our clock has a rate lower than the minimum set by a
  * call to clk_set_rate_range(), the rate will be raised to match the
@@ -259,6 +303,8 @@ static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test)
 static struct kunit_case clk_rate_range_test_cases[] = {
 	KUNIT_CASE(clk_rate_range_test_set_range),
 	KUNIT_CASE(clk_rate_range_test_set_range_invalid),
+	KUNIT_CASE(clk_rate_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_rate_range_test_set_range_round_rate_higher),
 	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised),
 	KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered),
 	{}
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8de6a22498e7..7bb5ae0fb688 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1330,6 +1330,8 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
 	if (!core)
 		return 0;
 
+	req->rate = clamp(req->rate, req->min_rate, req->max_rate);
+
 	/*
 	 * At this point, core protection will be disabled
 	 * - if the provider is not protected at all
-- 
2.34.1

[PATCH v3 03/10] clk: Use clamp instead of open-coding our own

[Maxime Ripard]

The code in clk_set_rate_range() will, if the current rate is outside of
the new range, will force it to the minimum or maximum. This is
equivalent to using clamp, while being less readable. Let's switch to
using clamp instead.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7bb5ae0fb688..150d1bc0985b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		 *   this corner case when determining the rate
 		 */
 
-		if (rate < min)
-			rate = min;
-		else
-			rate = max;
-
+		rate = clamp(clk->core->req_rate, min, max);
 		ret = clk_core_set_rate_nolock(clk->core, rate);
 		if (ret) {
 			/* rollback the changes */
-- 
2.34.1

[PATCH v3 03/10] clk: Use clamp instead of open-coding our own

[Maxime Ripard]

The code in clk_set_rate_range() will, if the current rate is outside of
the new range, will force it to the minimum or maximum. This is
equivalent to using clamp, while being less readable. Let's switch to
using clamp instead.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7bb5ae0fb688..150d1bc0985b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		 *   this corner case when determining the rate
 		 */
 
-		if (rate < min)
-			rate = min;
-		else
-			rate = max;
-
+		rate = clamp(clk->core->req_rate, min, max);
 		ret = clk_core_set_rate_nolock(clk->core, rate);
 		if (ret) {
 			/* rollback the changes */
-- 
2.34.1

[PATCH v3 04/10] clk: Always set the rate on clk_set_range_rate

[Maxime Ripard]

When we change a clock minimum or maximum using clk_set_rate_range(),
clk_set_min_rate() or clk_set_max_rate(), the current code will only
trigger a new rate change if the rate is outside of the new boundaries.

However, a clock driver might want to always keep the clock rate to
one of its boundary, for example the minimum to keep the power
consumption as low as possible.

Since they don't always get called though, clock providers don't have the
opportunity to implement this behaviour.

Let's trigger a clk_set_rate() on the previous requested rate every time
clk_set_rate_range() is called. That way, providers that care about the
new boundaries have a chance to adjust the rate, while providers that
don't care about those new boundaries will return the same rate than
before, which will be ignored by clk_set_rate() and won't result in a
new rate change.

Also add a few new test cases to make sure that behaviour is not broken
in the future.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c | 281 +++++++++++++++++++++++++++++++++++-
 drivers/clk/clk.c           |  45 +++---
 2 files changed, 303 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index a13b02702d20..baf0ea315322 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -6,6 +6,9 @@
 #include <linux/clk-provider.h>
 #include <linux/slab.h>
 
+/* Needed for clk_hw_create_clk() */
+#include "clk.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
@@ -33,6 +36,32 @@ static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int clk_dummy_rate_maximize_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	/*
+	 * If there's a maximum set, always run the clock at the maximum
+	 * allowed.
+	 */
+	if (req->max_rate < ULONG_MAX)
+		req->rate = req->max_rate;
+
+	return 0;
+}
+
+static int clk_dummy_rate_minimize_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	/*
+	 * If there's a minimum set, always run the clock at the minimum
+	 * allowed.
+	 */
+	if (req->min_rate > 0)
+		req->rate = req->min_rate;
+
+	return 0;
+}
+
 static int clk_dummy_rate_set_rate(struct clk_hw *hw,
 				   unsigned long rate,
 				   unsigned long parent_rate)
@@ -50,6 +79,18 @@ const static struct clk_ops clk_dummy_rate_ops = {
 	.set_rate = clk_dummy_rate_set_rate,
 };
 
+const static struct clk_ops clk_dummy_maximize_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_maximize_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
+const static struct clk_ops clk_dummy_minimize_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_minimize_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
 static int clk_rate_test_init_with_ops(struct kunit *test,
 				       const struct clk_ops *ops)
 {
@@ -79,6 +120,16 @@ static int clk_rate_test_init(struct kunit *test)
 	return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
 }
 
+static int clk_rate_maximize_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_maximize_rate_ops);
+}
+
+static int clk_rate_minimize_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_minimize_rate_ops);
+}
+
 static void clk_rate_test_exit(struct kunit *test)
 {
 	struct clk_dummy_rate_context *ctx = test->priv;
@@ -317,8 +368,236 @@ static struct kunit_suite clk_rate_range_test_suite = {
 	.test_cases = clk_rate_range_test_cases,
 };
 
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), the core will reevaluate whether a new rate is
+ * needed each and every time.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_rate_range_test_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1,
+				 DUMMY_CLOCK_RATE_2 - 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+/*
+ * 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 each and every time.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+	int ret;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user1));
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user2));
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(user1,
+				 0,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(user2,
+				 0,
+				 DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_rate_range_maximize_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range_rate_maximized),
+	KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_maximized),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_maximize_test_suite = {
+	.name = "clk-rate-range-maximize-test",
+	.init = clk_rate_maximize_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_maximize_test_cases,
+};
+
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), the core will reevaluate whether a new rate is
+ * needed each and every time.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_rate_range_test_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1 + 1000,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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 each and every time.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+	int ret;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user1));
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user2));
+
+	ret = clk_set_rate_range(user1,
+				 DUMMY_CLOCK_RATE_1,
+				 ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(user2,
+				 DUMMY_CLOCK_RATE_2,
+				 ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_rate_range_minimize_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range_rate_minimized),
+	KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_minimized),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_minimize_test_suite = {
+	.name = "clk-rate-range-minimize-test",
+	.init = clk_rate_minimize_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_minimize_test_cases,
+};
+
 kunit_test_suites(
 	&clk_rate_test_suite,
-	&clk_rate_range_test_suite
+	&clk_rate_range_test_suite,
+	&clk_rate_range_maximize_test_suite,
+	&clk_rate_range_minimize_test_suite
 );
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 150d1bc0985b..718cab23f706 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2350,28 +2350,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk->min_rate = min;
 	clk->max_rate = max;
 
-	rate = clk_core_get_rate_nolock(clk->core);
-	if (rate < min || rate > max) {
-		/*
-		 * FIXME:
-		 * We are in bit of trouble here, current rate is outside the
-		 * the requested range. We are going try to request appropriate
-		 * range boundary but there is a catch. It may fail for the
-		 * usual reason (clock broken, clock protected, etc) but also
-		 * because:
-		 * - round_rate() was not favorable and fell on the wrong
-		 *   side of the boundary
-		 * - the determine_rate() callback does not really check for
-		 *   this corner case when determining the rate
-		 */
-
-		rate = clamp(clk->core->req_rate, min, max);
-		ret = clk_core_set_rate_nolock(clk->core, rate);
-		if (ret) {
-			/* rollback the changes */
-			clk->min_rate = old_min;
-			clk->max_rate = old_max;
-		}
+	/*
+	 * Since the boundaries have been changed, let's give the
+	 * opportunity to the provider to adjust the clock rate based on
+	 * the new boundaries.
+	 *
+	 * We also need to handle the case where the clock is currently
+	 * outside of the boundaries. Clamping the last requested rate
+	 * to the current minimum and maximum will also handle this.
+	 *
+	 * FIXME:
+	 * There is a catch. It may fail for the usual reason (clock
+	 * broken, clock protected, etc) but also because:
+	 * - round_rate() was not favorable and fell on the wrong
+	 *   side of the boundary
+	 * - the determine_rate() callback does not really check for
+	 *   this corner case when determining the rate
+	 */
+	rate = clamp(clk->core->req_rate, min, max);
+	ret = clk_core_set_rate_nolock(clk->core, rate);
+	if (ret) {
+		/* rollback the changes */
+		clk->min_rate = old_min;
+		clk->max_rate = old_max;
 	}
 
 	if (clk->exclusive_count)
-- 
2.34.1

[PATCH v3 04/10] clk: Always set the rate on clk_set_range_rate

[Maxime Ripard]

When we change a clock minimum or maximum using clk_set_rate_range(),
clk_set_min_rate() or clk_set_max_rate(), the current code will only
trigger a new rate change if the rate is outside of the new boundaries.

However, a clock driver might want to always keep the clock rate to
one of its boundary, for example the minimum to keep the power
consumption as low as possible.

Since they don't always get called though, clock providers don't have the
opportunity to implement this behaviour.

Let's trigger a clk_set_rate() on the previous requested rate every time
clk_set_rate_range() is called. That way, providers that care about the
new boundaries have a chance to adjust the rate, while providers that
don't care about those new boundaries will return the same rate than
before, which will be ignored by clk_set_rate() and won't result in a
new rate change.

Also add a few new test cases to make sure that behaviour is not broken
in the future.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c | 281 +++++++++++++++++++++++++++++++++++-
 drivers/clk/clk.c           |  45 +++---
 2 files changed, 303 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index a13b02702d20..baf0ea315322 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -6,6 +6,9 @@
 #include <linux/clk-provider.h>
 #include <linux/slab.h>
 
+/* Needed for clk_hw_create_clk() */
+#include "clk.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
@@ -33,6 +36,32 @@ static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int clk_dummy_rate_maximize_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	/*
+	 * If there's a maximum set, always run the clock at the maximum
+	 * allowed.
+	 */
+	if (req->max_rate < ULONG_MAX)
+		req->rate = req->max_rate;
+
+	return 0;
+}
+
+static int clk_dummy_rate_minimize_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	/*
+	 * If there's a minimum set, always run the clock at the minimum
+	 * allowed.
+	 */
+	if (req->min_rate > 0)
+		req->rate = req->min_rate;
+
+	return 0;
+}
+
 static int clk_dummy_rate_set_rate(struct clk_hw *hw,
 				   unsigned long rate,
 				   unsigned long parent_rate)
@@ -50,6 +79,18 @@ const static struct clk_ops clk_dummy_rate_ops = {
 	.set_rate = clk_dummy_rate_set_rate,
 };
 
+const static struct clk_ops clk_dummy_maximize_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_maximize_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
+const static struct clk_ops clk_dummy_minimize_rate_ops = {
+	.recalc_rate = clk_dummy_rate_recalc_rate,
+	.determine_rate = clk_dummy_rate_minimize_rate,
+	.set_rate = clk_dummy_rate_set_rate,
+};
+
 static int clk_rate_test_init_with_ops(struct kunit *test,
 				       const struct clk_ops *ops)
 {
@@ -79,6 +120,16 @@ static int clk_rate_test_init(struct kunit *test)
 	return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
 }
 
+static int clk_rate_maximize_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_maximize_rate_ops);
+}
+
+static int clk_rate_minimize_test_init(struct kunit *test)
+{
+	return clk_rate_test_init_with_ops(test, &clk_dummy_minimize_rate_ops);
+}
+
 static void clk_rate_test_exit(struct kunit *test)
 {
 	struct clk_dummy_rate_context *ctx = test->priv;
@@ -317,8 +368,236 @@ static struct kunit_suite clk_rate_range_test_suite = {
 	.test_cases = clk_rate_range_test_cases,
 };
 
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), the core will reevaluate whether a new rate is
+ * needed each and every time.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_rate_range_test_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1,
+				 DUMMY_CLOCK_RATE_2 - 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+/*
+ * 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 each and every time.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+	int ret;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user1));
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user2));
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate_range(user1,
+				 0,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(user2,
+				 0,
+				 DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_rate_range_maximize_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range_rate_maximized),
+	KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_maximized),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_maximize_test_suite = {
+	.name = "clk-rate-range-maximize-test",
+	.init = clk_rate_maximize_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_maximize_test_cases,
+};
+
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), the core will reevaluate whether a new rate is
+ * needed each and every time.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_rate_range_test_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 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_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(clk,
+				 DUMMY_CLOCK_RATE_1 + 1000,
+				 DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	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_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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 each and every time.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_rate_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+	int ret;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user1));
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(user2));
+
+	ret = clk_set_rate_range(user1,
+				 DUMMY_CLOCK_RATE_1,
+				 ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	ret = clk_set_rate_range(user2,
+				 DUMMY_CLOCK_RATE_2,
+				 ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_rate_range_minimize_test_cases[] = {
+	KUNIT_CASE(clk_rate_range_test_set_range_rate_minimized),
+	KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_minimized),
+	{}
+};
+
+static struct kunit_suite clk_rate_range_minimize_test_suite = {
+	.name = "clk-rate-range-minimize-test",
+	.init = clk_rate_minimize_test_init,
+	.exit = clk_rate_test_exit,
+	.test_cases = clk_rate_range_minimize_test_cases,
+};
+
 kunit_test_suites(
 	&clk_rate_test_suite,
-	&clk_rate_range_test_suite
+	&clk_rate_range_test_suite,
+	&clk_rate_range_maximize_test_suite,
+	&clk_rate_range_minimize_test_suite
 );
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 150d1bc0985b..718cab23f706 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2350,28 +2350,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk->min_rate = min;
 	clk->max_rate = max;
 
-	rate = clk_core_get_rate_nolock(clk->core);
-	if (rate < min || rate > max) {
-		/*
-		 * FIXME:
-		 * We are in bit of trouble here, current rate is outside the
-		 * the requested range. We are going try to request appropriate
-		 * range boundary but there is a catch. It may fail for the
-		 * usual reason (clock broken, clock protected, etc) but also
-		 * because:
-		 * - round_rate() was not favorable and fell on the wrong
-		 *   side of the boundary
-		 * - the determine_rate() callback does not really check for
-		 *   this corner case when determining the rate
-		 */
-
-		rate = clamp(clk->core->req_rate, min, max);
-		ret = clk_core_set_rate_nolock(clk->core, rate);
-		if (ret) {
-			/* rollback the changes */
-			clk->min_rate = old_min;
-			clk->max_rate = old_max;
-		}
+	/*
+	 * Since the boundaries have been changed, let's give the
+	 * opportunity to the provider to adjust the clock rate based on
+	 * the new boundaries.
+	 *
+	 * We also need to handle the case where the clock is currently
+	 * outside of the boundaries. Clamping the last requested rate
+	 * to the current minimum and maximum will also handle this.
+	 *
+	 * FIXME:
+	 * There is a catch. It may fail for the usual reason (clock
+	 * broken, clock protected, etc) but also because:
+	 * - round_rate() was not favorable and fell on the wrong
+	 *   side of the boundary
+	 * - the determine_rate() callback does not really check for
+	 *   this corner case when determining the rate
+	 */
+	rate = clamp(clk->core->req_rate, min, max);
+	ret = clk_core_set_rate_nolock(clk->core, rate);
+	if (ret) {
+		/* rollback the changes */
+		clk->min_rate = old_min;
+		clk->max_rate = old_max;
 	}
 
 	if (clk->exclusive_count)
-- 
2.34.1

[PATCH v3 05/10] clk: Add clk_drop_range

[Maxime Ripard]

In order to reset the range on a clock, we need to call
clk_set_rate_range with a minimum of 0 and a maximum of ULONG_MAX. Since
it's fairly inconvenient, let's introduce a clk_drop_range() function
that will do just this.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c |  4 ++--
 include/linux/clk.h         | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index baf0ea315322..9beed87f663c 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -458,7 +458,7 @@ static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit *
 	KUNIT_ASSERT_TRUE(test, rate > 0);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
-	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	ret = clk_drop_range(user2);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	rate = clk_get_rate(clk);
@@ -570,7 +570,7 @@ static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit *
 	KUNIT_ASSERT_TRUE(test, rate > 0);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
-	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	ret = clk_drop_range(user2);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	rate = clk_get_rate(clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..f365dac7be17 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -1005,6 +1005,17 @@ static inline struct clk *clk_get_optional(struct device *dev, const char *id)
 	return clk;
 }
 
+/**
+ * clk_drop_range - Reset any range set on that clock
+ * @clk: clock source
+ *
+ * Returns success (0) or negative errno.
+ */
+static inline int clk_drop_range(struct clk *clk)
+{
+	return clk_set_rate_range(clk, 0, ULONG_MAX);
+}
+
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
-- 
2.34.1

[PATCH v3 05/10] clk: Add clk_drop_range

[Maxime Ripard]

In order to reset the range on a clock, we need to call
clk_set_rate_range with a minimum of 0 and a maximum of ULONG_MAX. Since
it's fairly inconvenient, let's introduce a clk_drop_range() function
that will do just this.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-rate-test.c |  4 ++--
 include/linux/clk.h         | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
index baf0ea315322..9beed87f663c 100644
--- a/drivers/clk/clk-rate-test.c
+++ b/drivers/clk/clk-rate-test.c
@@ -458,7 +458,7 @@ static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit *
 	KUNIT_ASSERT_TRUE(test, rate > 0);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
-	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	ret = clk_drop_range(user2);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	rate = clk_get_rate(clk);
@@ -570,7 +570,7 @@ static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit *
 	KUNIT_ASSERT_TRUE(test, rate > 0);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
-	ret = clk_set_rate_range(user2, 0, ULONG_MAX);
+	ret = clk_drop_range(user2);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	rate = clk_get_rate(clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..f365dac7be17 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -1005,6 +1005,17 @@ static inline struct clk *clk_get_optional(struct device *dev, const char *id)
 	return clk;
 }
 
+/**
+ * clk_drop_range - Reset any range set on that clock
+ * @clk: clock source
+ *
+ * Returns success (0) or negative errno.
+ */
+static inline int clk_drop_range(struct clk *clk)
+{
+	return clk_set_rate_range(clk, 0, ULONG_MAX);
+}
+
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
-- 
2.34.1

[PATCH v3 06/10] clk: bcm: rpi: Add variant structure

[Maxime Ripard]

We only export a bunch of firmware clocks, and some of them require
special treatment.

This has been do so far using some tests on the clock id in various
places, but this is fairly hard to extend and doesn't scale very well.

Since we'll need some more cases in the next patches, let's switch to a
variant structure that defines the behaviour we need to have for a given
clock.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index dd3b71eafabf..f7185d421085 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -56,6 +56,8 @@ static char *rpi_firmware_clk_names[] = {
 #define RPI_FIRMWARE_STATE_ENABLE_BIT	BIT(0)
 #define RPI_FIRMWARE_STATE_WAIT_BIT	BIT(1)
 
+struct raspberrypi_clk_variant;
+
 struct raspberrypi_clk {
 	struct device *dev;
 	struct rpi_firmware *firmware;
@@ -66,10 +68,36 @@ struct raspberrypi_clk_data {
 	struct clk_hw hw;
 
 	unsigned int id;
+	struct raspberrypi_clk_variant *variant;
 
 	struct raspberrypi_clk *rpi;
 };
 
+struct raspberrypi_clk_variant {
+	bool		export;
+	char		*clkdev;
+};
+
+static struct raspberrypi_clk_variant
+raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
+	[RPI_FIRMWARE_ARM_CLK_ID] = {
+		.export = true,
+		.clkdev = "cpu0",
+	},
+	[RPI_FIRMWARE_CORE_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_M2MC_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_V3D_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = {
+		.export = true,
+	},
+};
+
 /*
  * Structure of the message passed to Raspberry Pi's firmware in order to
  * change clock rates. The 'disable_turbo' option is only available to the ARM
@@ -183,7 +211,8 @@ static const struct clk_ops raspberrypi_firmware_clk_ops = {
 
 static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 					       unsigned int parent,
-					       unsigned int id)
+					       unsigned int id,
+					       struct raspberrypi_clk_variant *variant)
 {
 	struct raspberrypi_clk_data *data;
 	struct clk_init_data init = {};
@@ -195,6 +224,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 		return ERR_PTR(-ENOMEM);
 	data->rpi = rpi;
 	data->id = id;
+	data->variant = variant;
 
 	init.name = devm_kasprintf(rpi->dev, GFP_KERNEL,
 				   "fw-clk-%s",
@@ -228,9 +258,9 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 
 	clk_hw_set_rate_range(&data->hw, min_rate, max_rate);
 
-	if (id == RPI_FIRMWARE_ARM_CLK_ID) {
+	if (variant->clkdev) {
 		ret = devm_clk_hw_register_clkdev(rpi->dev, &data->hw,
-						  NULL, "cpu0");
+						  NULL, variant->clkdev);
 		if (ret) {
 			dev_err(rpi->dev, "Failed to initialize clkdev\n");
 			return ERR_PTR(ret);
@@ -264,27 +294,27 @@ static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
 		return ret;
 
 	while (clks->id) {
-		struct clk_hw *hw;
+		struct raspberrypi_clk_variant *variant;
+
+		if (clks->id > RPI_FIRMWARE_NUM_CLK_ID) {
+			dev_err(rpi->dev, "Unknown clock id: %u", clks->id);
+			return -EINVAL;
+		}
+
+		variant = &raspberrypi_clk_variants[clks->id];
+		if (variant->export) {
+			struct clk_hw *hw;
 
-		switch (clks->id) {
-		case RPI_FIRMWARE_ARM_CLK_ID:
-		case RPI_FIRMWARE_CORE_CLK_ID:
-		case RPI_FIRMWARE_M2MC_CLK_ID:
-		case RPI_FIRMWARE_V3D_CLK_ID:
-		case RPI_FIRMWARE_PIXEL_BVB_CLK_ID:
 			hw = raspberrypi_clk_register(rpi, clks->parent,
-						      clks->id);
+						      clks->id, variant);
 			if (IS_ERR(hw))
 				return PTR_ERR(hw);
 
 			data->hws[clks->id] = hw;
 			data->num = clks->id + 1;
-			fallthrough;
-
-		default:
-			clks++;
-			break;
 		}
+
+		clks++;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH v3 06/10] clk: bcm: rpi: Add variant structure

[Maxime Ripard]

We only export a bunch of firmware clocks, and some of them require
special treatment.

This has been do so far using some tests on the clock id in various
places, but this is fairly hard to extend and doesn't scale very well.

Since we'll need some more cases in the next patches, let's switch to a
variant structure that defines the behaviour we need to have for a given
clock.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index dd3b71eafabf..f7185d421085 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -56,6 +56,8 @@ static char *rpi_firmware_clk_names[] = {
 #define RPI_FIRMWARE_STATE_ENABLE_BIT	BIT(0)
 #define RPI_FIRMWARE_STATE_WAIT_BIT	BIT(1)
 
+struct raspberrypi_clk_variant;
+
 struct raspberrypi_clk {
 	struct device *dev;
 	struct rpi_firmware *firmware;
@@ -66,10 +68,36 @@ struct raspberrypi_clk_data {
 	struct clk_hw hw;
 
 	unsigned int id;
+	struct raspberrypi_clk_variant *variant;
 
 	struct raspberrypi_clk *rpi;
 };
 
+struct raspberrypi_clk_variant {
+	bool		export;
+	char		*clkdev;
+};
+
+static struct raspberrypi_clk_variant
+raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
+	[RPI_FIRMWARE_ARM_CLK_ID] = {
+		.export = true,
+		.clkdev = "cpu0",
+	},
+	[RPI_FIRMWARE_CORE_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_M2MC_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_V3D_CLK_ID] = {
+		.export = true,
+	},
+	[RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = {
+		.export = true,
+	},
+};
+
 /*
  * Structure of the message passed to Raspberry Pi's firmware in order to
  * change clock rates. The 'disable_turbo' option is only available to the ARM
@@ -183,7 +211,8 @@ static const struct clk_ops raspberrypi_firmware_clk_ops = {
 
 static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 					       unsigned int parent,
-					       unsigned int id)
+					       unsigned int id,
+					       struct raspberrypi_clk_variant *variant)
 {
 	struct raspberrypi_clk_data *data;
 	struct clk_init_data init = {};
@@ -195,6 +224,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 		return ERR_PTR(-ENOMEM);
 	data->rpi = rpi;
 	data->id = id;
+	data->variant = variant;
 
 	init.name = devm_kasprintf(rpi->dev, GFP_KERNEL,
 				   "fw-clk-%s",
@@ -228,9 +258,9 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 
 	clk_hw_set_rate_range(&data->hw, min_rate, max_rate);
 
-	if (id == RPI_FIRMWARE_ARM_CLK_ID) {
+	if (variant->clkdev) {
 		ret = devm_clk_hw_register_clkdev(rpi->dev, &data->hw,
-						  NULL, "cpu0");
+						  NULL, variant->clkdev);
 		if (ret) {
 			dev_err(rpi->dev, "Failed to initialize clkdev\n");
 			return ERR_PTR(ret);
@@ -264,27 +294,27 @@ static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
 		return ret;
 
 	while (clks->id) {
-		struct clk_hw *hw;
+		struct raspberrypi_clk_variant *variant;
+
+		if (clks->id > RPI_FIRMWARE_NUM_CLK_ID) {
+			dev_err(rpi->dev, "Unknown clock id: %u", clks->id);
+			return -EINVAL;
+		}
+
+		variant = &raspberrypi_clk_variants[clks->id];
+		if (variant->export) {
+			struct clk_hw *hw;
 
-		switch (clks->id) {
-		case RPI_FIRMWARE_ARM_CLK_ID:
-		case RPI_FIRMWARE_CORE_CLK_ID:
-		case RPI_FIRMWARE_M2MC_CLK_ID:
-		case RPI_FIRMWARE_V3D_CLK_ID:
-		case RPI_FIRMWARE_PIXEL_BVB_CLK_ID:
 			hw = raspberrypi_clk_register(rpi, clks->parent,
-						      clks->id);
+						      clks->id, variant);
 			if (IS_ERR(hw))
 				return PTR_ERR(hw);
 
 			data->hws[clks->id] = hw;
 			data->num = clks->id + 1;
-			fallthrough;
-
-		default:
-			clks++;
-			break;
 		}
+
+		clks++;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH v3 07/10] clk: bcm: rpi: Set a default minimum rate

[Maxime Ripard]

The M2MC clock provides the state machine clock for both HDMI
controllers.

However, if no HDMI monitor is plugged in at boot, its clock rate will
be left at 0 by the firmware and will make any register access end up in
a CPU stall, even though the clock was enabled.

We had some code in the HDMI controller to deal with this before, but it
makes more sense to have it in the clock driver. Move it there.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index f7185d421085..c879f2e9a4a7 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -76,6 +76,7 @@ struct raspberrypi_clk_data {
 struct raspberrypi_clk_variant {
 	bool		export;
 	char		*clkdev;
+	unsigned long	min_rate;
 };
 
 static struct raspberrypi_clk_variant
@@ -89,6 +90,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
+
+		/*
+		 * If we boot without any cable connected to any of the
+		 * HDMI connector, the firmware will skip the HSM
+		 * initialization and leave it with a rate of 0,
+		 * resulting in a bus lockup when we're accessing the
+		 * registers even if it's enabled.
+		 *
+		 * Let's put a sensible default so that we don't end up
+		 * in this situation.
+		 */
+		.min_rate = 120000000,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -267,6 +280,19 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 		}
 	}
 
+	if (variant->min_rate) {
+		unsigned long rate;
+
+		clk_hw_set_rate_range(&data->hw, variant->min_rate, max_rate);
+
+		rate = raspberrypi_fw_get_rate(&data->hw, 0);
+		if (rate < variant->min_rate) {
+			ret = raspberrypi_fw_set_rate(&data->hw, variant->min_rate, 0);
+			if (ret)
+				return ERR_PTR(ret);
+		}
+	}
+
 	return &data->hw;
 }
 
-- 
2.34.1

[PATCH v3 07/10] clk: bcm: rpi: Set a default minimum rate

[Maxime Ripard]

The M2MC clock provides the state machine clock for both HDMI
controllers.

However, if no HDMI monitor is plugged in at boot, its clock rate will
be left at 0 by the firmware and will make any register access end up in
a CPU stall, even though the clock was enabled.

We had some code in the HDMI controller to deal with this before, but it
makes more sense to have it in the clock driver. Move it there.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index f7185d421085..c879f2e9a4a7 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -76,6 +76,7 @@ struct raspberrypi_clk_data {
 struct raspberrypi_clk_variant {
 	bool		export;
 	char		*clkdev;
+	unsigned long	min_rate;
 };
 
 static struct raspberrypi_clk_variant
@@ -89,6 +90,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
+
+		/*
+		 * If we boot without any cable connected to any of the
+		 * HDMI connector, the firmware will skip the HSM
+		 * initialization and leave it with a rate of 0,
+		 * resulting in a bus lockup when we're accessing the
+		 * registers even if it's enabled.
+		 *
+		 * Let's put a sensible default so that we don't end up
+		 * in this situation.
+		 */
+		.min_rate = 120000000,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -267,6 +280,19 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 		}
 	}
 
+	if (variant->min_rate) {
+		unsigned long rate;
+
+		clk_hw_set_rate_range(&data->hw, variant->min_rate, max_rate);
+
+		rate = raspberrypi_fw_get_rate(&data->hw, 0);
+		if (rate < variant->min_rate) {
+			ret = raspberrypi_fw_set_rate(&data->hw, variant->min_rate, 0);
+			if (ret)
+				return ERR_PTR(ret);
+		}
+	}
+
 	return &data->hw;
 }
 
-- 
2.34.1

[PATCH v3 08/10] clk: bcm: rpi: Run some clocks at the minimum rate allowed

[Maxime Ripard]

The core clock and M2MC clocks are shared between some devices (Unicam
controllers and the HVS, and the HDMI controllers, respectively) that
will have various, varying, requirements depending on their current work
load.

Since those loads can require a fairly high clock rate in extreme
conditions (up to ~600MHz), we can end up running those clocks at their
maximum frequency even though we no longer require such a high rate.

Fortunately, those devices don't require an exact rate but a minimum
rate, and all the drivers are using clk_set_min_rate. Thus, we can just
rely on the fact that the clk_request minimum (which is the aggregated
minimum of all the clock users) is what we want at all times.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index c879f2e9a4a7..9d09621549b9 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -77,6 +77,7 @@ struct raspberrypi_clk_variant {
 	bool		export;
 	char		*clkdev;
 	unsigned long	min_rate;
+	bool		minimize;
 };
 
 static struct raspberrypi_clk_variant
@@ -87,6 +88,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	},
 	[RPI_FIRMWARE_CORE_CLK_ID] = {
 		.export = true,
+
+		/*
+		 * The clock is shared between the HVS and the CSI
+		 * controllers, on the BCM2711 and will change depending
+		 * on the pixels composited on the HVS and the capture
+		 * resolution on Unicam.
+		 *
+		 * Since the rate can get quite large, and we need to
+		 * coordinate between both driver instances, let's
+		 * always use the minimum the drivers will let us.
+		 */
+		.minimize = true,
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
@@ -102,6 +115,16 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * in this situation.
 		 */
 		.min_rate = 120000000,
+
+		/*
+		 * The clock is shared between the two HDMI controllers
+		 * on the BCM2711 and will change depending on the
+		 * resolution output on each. Since the rate can get
+		 * quite large, and we need to coordinate between both
+		 * driver instances, let's always use the minimum the
+		 * drivers will let us.
+		 */
+		.minimize = true,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -206,12 +229,26 @@ static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate,
 static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
 					      struct clk_rate_request *req)
 {
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
+	struct raspberrypi_clk_variant *variant = data->variant;
+
 	/*
 	 * The firmware will do the rounding but that isn't part of
 	 * the interface with the firmware, so we just do our best
 	 * here.
 	 */
+
 	req->rate = clamp(req->rate, req->min_rate, req->max_rate);
+
+	/*
+	 * We want to aggressively reduce the clock rate here, so let's
+	 * just ignore the requested rate and return the bare minimum
+	 * rate we can get away with.
+	 */
+	if (variant->minimize && req->min_rate > 0)
+		req->rate = req->min_rate;
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH v3 08/10] clk: bcm: rpi: Run some clocks at the minimum rate allowed

[Maxime Ripard]

The core clock and M2MC clocks are shared between some devices (Unicam
controllers and the HVS, and the HDMI controllers, respectively) that
will have various, varying, requirements depending on their current work
load.

Since those loads can require a fairly high clock rate in extreme
conditions (up to ~600MHz), we can end up running those clocks at their
maximum frequency even though we no longer require such a high rate.

Fortunately, those devices don't require an exact rate but a minimum
rate, and all the drivers are using clk_set_min_rate. Thus, we can just
rely on the fact that the clk_request minimum (which is the aggregated
minimum of all the clock users) is what we want at all times.

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

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index c879f2e9a4a7..9d09621549b9 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -77,6 +77,7 @@ struct raspberrypi_clk_variant {
 	bool		export;
 	char		*clkdev;
 	unsigned long	min_rate;
+	bool		minimize;
 };
 
 static struct raspberrypi_clk_variant
@@ -87,6 +88,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	},
 	[RPI_FIRMWARE_CORE_CLK_ID] = {
 		.export = true,
+
+		/*
+		 * The clock is shared between the HVS and the CSI
+		 * controllers, on the BCM2711 and will change depending
+		 * on the pixels composited on the HVS and the capture
+		 * resolution on Unicam.
+		 *
+		 * Since the rate can get quite large, and we need to
+		 * coordinate between both driver instances, let's
+		 * always use the minimum the drivers will let us.
+		 */
+		.minimize = true,
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
@@ -102,6 +115,16 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * in this situation.
 		 */
 		.min_rate = 120000000,
+
+		/*
+		 * The clock is shared between the two HDMI controllers
+		 * on the BCM2711 and will change depending on the
+		 * resolution output on each. Since the rate can get
+		 * quite large, and we need to coordinate between both
+		 * driver instances, let's always use the minimum the
+		 * drivers will let us.
+		 */
+		.minimize = true,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -206,12 +229,26 @@ static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate,
 static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
 					      struct clk_rate_request *req)
 {
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
+	struct raspberrypi_clk_variant *variant = data->variant;
+
 	/*
 	 * The firmware will do the rounding but that isn't part of
 	 * the interface with the firmware, so we just do our best
 	 * here.
 	 */
+
 	req->rate = clamp(req->rate, req->min_rate, req->max_rate);
+
+	/*
+	 * We want to aggressively reduce the clock rate here, so let's
+	 * just ignore the requested rate and return the bare minimum
+	 * rate we can get away with.
+	 */
+	if (variant->minimize && req->min_rate > 0)
+		req->rate = req->min_rate;
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH v3 09/10] drm/vc4: Add logging and comments

[Maxime Ripard]

The HVS core clock isn't really obvious, so let's add a bunch more
comments and some logging for easier debugging.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 24de29bc1cda..6fe03fc17d73 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -389,8 +389,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 						500000000,
 						new_hvs_state->core_clock_rate);
 
+		drm_dbg(dev, "Raising the core clock at %lu Hz\n", core_rate);
+
+		/*
+		 * Do a temporary request on the core clock during the
+		 * modeset.
+		 */
 		clk_set_min_rate(hvs->core_clk, core_rate);
 	}
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	vc4_ctm_commit(vc4, state);
@@ -416,6 +423,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		drm_dbg(dev, "Running the core clock at %lu Hz\n",
 			new_hvs_state->core_clock_rate);
 
+		/*
+		 * Request a clock rate based on the current HVS
+		 * requirements.
+		 */
 		clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate);
 	}
 }
-- 
2.34.1

[PATCH v3 09/10] drm/vc4: Add logging and comments

[Maxime Ripard]

The HVS core clock isn't really obvious, so let's add a bunch more
comments and some logging for easier debugging.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 24de29bc1cda..6fe03fc17d73 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -389,8 +389,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 						500000000,
 						new_hvs_state->core_clock_rate);
 
+		drm_dbg(dev, "Raising the core clock at %lu Hz\n", core_rate);
+
+		/*
+		 * Do a temporary request on the core clock during the
+		 * modeset.
+		 */
 		clk_set_min_rate(hvs->core_clk, core_rate);
 	}
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	vc4_ctm_commit(vc4, state);
@@ -416,6 +423,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		drm_dbg(dev, "Running the core clock at %lu Hz\n",
 			new_hvs_state->core_clock_rate);
 
+		/*
+		 * Request a clock rate based on the current HVS
+		 * requirements.
+		 */
 		clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate);
 	}
 }
-- 
2.34.1

[PATCH v3 10/10] drm/vc4: hdmi: Remove clock rate initialization

[Maxime Ripard]

Now that the clock driver makes sure we never end up with a rate of 0,
the HDMI driver doesn't need to care anymore.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 053fbaf765ca..43aced269082 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2543,19 +2543,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			vc4_hdmi->disable_4kp60 = true;
 	}
 
-	/*
-	 * If we boot without any cable connected to the HDMI connector,
-	 * the firmware will skip the HSM initialization and leave it
-	 * with a rate of 0, resulting in a bus lockup when we're
-	 * accessing the registers even if it's enabled.
-	 *
-	 * Let's put a sensible default at runtime_resume so that we
-	 * don't end up in this situation.
-	 */
-	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
-	if (ret)
-		goto err_put_ddc;
-
 	/*
 	 * We need to have the device powered up at this point to call
 	 * our reset hook and for the CEC init.
-- 
2.34.1

[PATCH v3 10/10] drm/vc4: hdmi: Remove clock rate initialization

[Maxime Ripard]

Now that the clock driver makes sure we never end up with a rate of 0,
the HDMI driver doesn't need to care anymore.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 053fbaf765ca..43aced269082 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2543,19 +2543,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			vc4_hdmi->disable_4kp60 = true;
 	}
 
-	/*
-	 * If we boot without any cable connected to the HDMI connector,
-	 * the firmware will skip the HSM initialization and leave it
-	 * with a rate of 0, resulting in a bus lockup when we're
-	 * accessing the registers even if it's enabled.
-	 *
-	 * Let's put a sensible default at runtime_resume so that we
-	 * don't end up in this situation.
-	 */
-	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
-	if (ret)
-		goto err_put_ddc;
-
 	/*
 	 * We need to have the device powered up at this point to call
 	 * our reset hook and for the CEC init.
-- 
2.34.1