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

[PATCH v4 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 v3:
  - Renamed the test file and Kconfig option
  - Add option to .kunitconfig
  - Switch to kunit_kzalloc
  - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant
  - Test directly relevant calls instead of going through a temporary variable
  - Switch to more precise KUNIT_ASSERT_* macros where relevant

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: Introduce Kunit Tests for the framework
  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/.kunitconfig          |   1 +
 drivers/clk/Kconfig               |   7 +
 drivers/clk/Makefile              |   1 +
 drivers/clk/bcm/clk-raspberrypi.c | 125 +++++-
 drivers/clk/clk-test.c            | 621 ++++++++++++++++++++++++++++++
 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 +
 9 files changed, 786 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk-test.c

-- 
2.34.1

[PATCH v4 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 v3:
  - Renamed the test file and Kconfig option
  - Add option to .kunitconfig
  - Switch to kunit_kzalloc
  - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant
  - Test directly relevant calls instead of going through a temporary variable
  - Switch to more precise KUNIT_ASSERT_* macros where relevant

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: Introduce Kunit Tests for the framework
  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/.kunitconfig          |   1 +
 drivers/clk/Kconfig               |   7 +
 drivers/clk/Makefile              |   1 +
 drivers/clk/bcm/clk-raspberrypi.c | 125 +++++-
 drivers/clk/clk-test.c            | 621 ++++++++++++++++++++++++++++++
 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 +
 9 files changed, 786 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk-test.c

-- 
2.34.1

[PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[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/.kunitconfig |   1 +
 drivers/clk/Kconfig      |   7 +
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-test.c   | 285 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 294 insertions(+)
 create mode 100644 drivers/clk/clk-test.c

diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig
index 3754fdb9485a..cdbc7d7deba9 100644
--- a/drivers/clk/.kunitconfig
+++ b/drivers/clk/.kunitconfig
@@ -1,3 +1,4 @@
 CONFIG_KUNIT=y
 CONFIG_COMMON_CLK=y
+CONFIG_CLK_KUNIT_TEST=y
 CONFIG_CLK_GATE_KUNIT_TEST=y
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3cdf33470a75..2ef6eca297ff 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -429,6 +429,13 @@ source "drivers/clk/xilinx/Kconfig"
 source "drivers/clk/zynqmp/Kconfig"
 
 # Kunit test cases
+config CLK_KUNIT_TEST
+	tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Kunit tests for the common clock framework.
+
 config CLK_GATE_KUNIT_TEST
 	tristate "Basic gate type Kunit test" if !KUNIT_ALL_TESTS
 	depends on KUNIT
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 6a98291350b6..2664aaab8068 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_KUNIT_TEST)	+= clk-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-test.c b/drivers/clk/clk-test.c
new file mode 100644
index 000000000000..47a600d590c1
--- /dev/null
+++ b/drivers/clk/clk-test.c
@@ -0,0 +1,285 @@
+// 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_context {
+	struct clk_hw hw;
+	unsigned long rate;
+};
+
+static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw,
+					   unsigned long parent_rate)
+{
+	struct clk_dummy_context *ctx =
+		container_of(hw, struct clk_dummy_context, hw);
+
+	return ctx->rate;
+}
+
+static int clk_dummy_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_set_rate(struct clk_hw *hw,
+			      unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct clk_dummy_context *ctx =
+		container_of(hw, struct clk_dummy_context, hw);
+
+	ctx->rate = rate;
+	return 0;
+}
+
+static const struct clk_ops clk_dummy_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_determine_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
+static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
+{
+	struct clk_dummy_context *ctx;
+	struct clk_init_data init = { };
+	int ret;
+
+	ctx = kunit_kzalloc(test, 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_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_ops);
+}
+
+static void clk_test_exit(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+}
+
+/*
+ * Test that the actual rate matches what is returned by clk_get_rate()
+ */
+static void clk_test_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_EXPECT_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_test_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_test_set_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_test_cases[] = {
+	KUNIT_CASE(clk_test_get_rate),
+	KUNIT_CASE(clk_test_set_get_rate),
+	KUNIT_CASE(clk_test_set_set_get_rate),
+	{}
+};
+
+static struct kunit_suite clk_test_suite = {
+	.name = "clk-test",
+	.init = clk_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_test_cases,
+};
+
+/*
+ * Test that clk_set_rate_range won't return an error for a valid range.
+ */
+static void clk_range_test_set_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+/*
+ * Test that calling clk_set_rate_range with a minimum rate higher than
+ * the maximum rate returns an error.
+ */
+static void clk_range_test_set_range_invalid(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1 + 1000,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+}
+
+/*
+ * 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_range_test_set_range_get_rate_raised(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_range_test_set_range_get_rate_lowered(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_range_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range),
+	KUNIT_CASE(clk_range_test_set_range_invalid),
+	KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
+	KUNIT_CASE(clk_range_test_set_range_get_rate_lowered),
+	{}
+};
+
+static struct kunit_suite clk_range_test_suite = {
+	.name = "clk-range-test",
+	.init = clk_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_range_test_cases,
+};
+
+kunit_test_suites(
+	&clk_test_suite,
+	&clk_range_test_suite,
+);
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

[PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[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/.kunitconfig |   1 +
 drivers/clk/Kconfig      |   7 +
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-test.c   | 285 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 294 insertions(+)
 create mode 100644 drivers/clk/clk-test.c

diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig
index 3754fdb9485a..cdbc7d7deba9 100644
--- a/drivers/clk/.kunitconfig
+++ b/drivers/clk/.kunitconfig
@@ -1,3 +1,4 @@
 CONFIG_KUNIT=y
 CONFIG_COMMON_CLK=y
+CONFIG_CLK_KUNIT_TEST=y
 CONFIG_CLK_GATE_KUNIT_TEST=y
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3cdf33470a75..2ef6eca297ff 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -429,6 +429,13 @@ source "drivers/clk/xilinx/Kconfig"
 source "drivers/clk/zynqmp/Kconfig"
 
 # Kunit test cases
+config CLK_KUNIT_TEST
+	tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Kunit tests for the common clock framework.
+
 config CLK_GATE_KUNIT_TEST
 	tristate "Basic gate type Kunit test" if !KUNIT_ALL_TESTS
 	depends on KUNIT
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 6a98291350b6..2664aaab8068 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_KUNIT_TEST)	+= clk-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-test.c b/drivers/clk/clk-test.c
new file mode 100644
index 000000000000..47a600d590c1
--- /dev/null
+++ b/drivers/clk/clk-test.c
@@ -0,0 +1,285 @@
+// 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_context {
+	struct clk_hw hw;
+	unsigned long rate;
+};
+
+static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw,
+					   unsigned long parent_rate)
+{
+	struct clk_dummy_context *ctx =
+		container_of(hw, struct clk_dummy_context, hw);
+
+	return ctx->rate;
+}
+
+static int clk_dummy_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_set_rate(struct clk_hw *hw,
+			      unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct clk_dummy_context *ctx =
+		container_of(hw, struct clk_dummy_context, hw);
+
+	ctx->rate = rate;
+	return 0;
+}
+
+static const struct clk_ops clk_dummy_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_determine_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
+static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
+{
+	struct clk_dummy_context *ctx;
+	struct clk_init_data init = { };
+	int ret;
+
+	ctx = kunit_kzalloc(test, 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_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_ops);
+}
+
+static void clk_test_exit(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+
+	clk_hw_unregister(&ctx->hw);
+}
+
+/*
+ * Test that the actual rate matches what is returned by clk_get_rate()
+ */
+static void clk_test_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_TRUE(test, rate > 0);
+	KUNIT_EXPECT_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_test_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_test_set_set_get_rate(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_test_cases[] = {
+	KUNIT_CASE(clk_test_get_rate),
+	KUNIT_CASE(clk_test_set_get_rate),
+	KUNIT_CASE(clk_test_set_set_get_rate),
+	{}
+};
+
+static struct kunit_suite clk_test_suite = {
+	.name = "clk-test",
+	.init = clk_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_test_cases,
+};
+
+/*
+ * Test that clk_set_rate_range won't return an error for a valid range.
+ */
+static void clk_range_test_set_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+/*
+ * Test that calling clk_set_rate_range with a minimum rate higher than
+ * the maximum rate returns an error.
+ */
+static void clk_range_test_set_range_invalid(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1 + 1000,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+}
+
+/*
+ * 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_range_test_set_range_get_rate_raised(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_range_test_set_range_get_rate_lowered(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+}
+
+static struct kunit_case clk_range_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range),
+	KUNIT_CASE(clk_range_test_set_range_invalid),
+	KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
+	KUNIT_CASE(clk_range_test_set_range_get_rate_lowered),
+	{}
+};
+
+static struct kunit_suite clk_range_test_suite = {
+	.name = "clk-range-test",
+	.init = clk_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_range_test_cases,
+};
+
+kunit_test_suites(
+	&clk_test_suite,
+	&clk_range_test_suite,
+);
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:40)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6a98291350b6..2664aaab8068 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_KUNIT_TEST)   += clk-test.o

The file name should be clk_test.c with an underscore.

>  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-test.c b/drivers/clk/clk-test.c
> new file mode 100644
> index 000000000000..47a600d590c1
> --- /dev/null
> +++ b/drivers/clk/clk-test.c
> @@ -0,0 +1,285 @@
> +// 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_context {
> +       struct clk_hw hw;
> +       unsigned long rate;
> +};
> +
> +static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw,
> +                                          unsigned long parent_rate)
> +{
> +       struct clk_dummy_context *ctx =
> +               container_of(hw, struct clk_dummy_context, hw);
> +
> +       return ctx->rate;
> +}
> +
> +static int clk_dummy_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_set_rate(struct clk_hw *hw,
> +                             unsigned long rate,
> +                             unsigned long parent_rate)
> +{
> +       struct clk_dummy_context *ctx =
> +               container_of(hw, struct clk_dummy_context, hw);
> +
> +       ctx->rate = rate;
> +       return 0;
> +}
> +
> +static const struct clk_ops clk_dummy_ops = {

Maybe clk_dummy_rate_ops? So we don't mix it up with other dummy ops in
this file testing things that aren't rates.

> +       .recalc_rate = clk_dummy_recalc_rate,
> +       .determine_rate = clk_dummy_determine_rate,
> +       .set_rate = clk_dummy_set_rate,
> +};
> +
> +static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
> +{
> +       struct clk_dummy_context *ctx;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ctx = kunit_kzalloc(test, 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_test_init(struct kunit *test)
> +{
> +       return clk_test_init_with_ops(test, &clk_dummy_ops);
> +}
> +
> +static void clk_test_exit(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +
> +       clk_hw_unregister(&ctx->hw);
> +}
> +
> +/*
> + * Test that the actual rate matches what is returned by clk_get_rate()
> + */
> +static void clk_test_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_EXPECT_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_test_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * 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_test_set_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_test_cases[] = {
> +       KUNIT_CASE(clk_test_get_rate),
> +       KUNIT_CASE(clk_test_set_get_rate),
> +       KUNIT_CASE(clk_test_set_set_get_rate),
> +       {}
> +};
> +
> +static struct kunit_suite clk_test_suite = {
> +       .name = "clk-test",
> +       .init = clk_test_init,
> +       .exit = clk_test_exit,
> +       .test_cases = clk_test_cases,
> +};
> +
> +/*
> + * Test that clk_set_rate_range won't return an error for a valid range.
> + */
> +static void clk_range_test_set_range(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
> +       KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +/*
> + * Test that calling clk_set_rate_range with a minimum rate higher than
> + * the maximum rate returns an error.
> + */
> +static void clk_range_test_set_range_invalid(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +
> +       KUNIT_EXPECT_LT(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1 + 1000,
> +                                          DUMMY_CLOCK_RATE_1),
> +                       0);
> +}
> +
> +/*
> + * 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_range_test_set_range_get_rate_raised(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * 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_range_test_set_range_get_rate_lowered(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_range_test_cases[] = {
> +       KUNIT_CASE(clk_range_test_set_range),
> +       KUNIT_CASE(clk_range_test_set_range_invalid),
> +       KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
> +       KUNIT_CASE(clk_range_test_set_range_get_rate_lowered),

Can you add a test case for round_rate matching what set_rate did, i.e.
calling clk_round_rate() and then clk_set_rate() followed by
clk_get_rate() with the same argument for round and set rate leads to
the same frequency?

It would also be good to add a test that tries to set the clk rate with
clk_set_rate() after a range has been set that is outside the acceptable
range and verify that it fails, and one that tries to set it within the
range and make sure it succeeds (and changes it to be exactly what was
set). Similarly, a call to set two disjoint ranges and verify that the
call that tries to set the second disjoint range fails. We want to test
the failure paths as well, to make sure we don't start causing them to
pass, unless it's expected. This patch could also contain the failure
scenario you're experiencing and mark it as expecting to fail. Then the
patch that fixes it in the core could mark the test as expecting to
pass, which may help us understand more easily what exactly changed
instead of having to figure that out after the fact by reading the
entire test.

> +       {}
> +};
> +
> +static struct kunit_suite clk_range_test_suite = {
> +       .name = "clk-range-test",
> +       .init = clk_test_init,
> +       .exit = clk_test_exit,
> +       .test_cases = clk_range_test_cases,
> +};

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:40)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6a98291350b6..2664aaab8068 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_KUNIT_TEST)   += clk-test.o

The file name should be clk_test.c with an underscore.

>  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-test.c b/drivers/clk/clk-test.c
> new file mode 100644
> index 000000000000..47a600d590c1
> --- /dev/null
> +++ b/drivers/clk/clk-test.c
> @@ -0,0 +1,285 @@
> +// 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_context {
> +       struct clk_hw hw;
> +       unsigned long rate;
> +};
> +
> +static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw,
> +                                          unsigned long parent_rate)
> +{
> +       struct clk_dummy_context *ctx =
> +               container_of(hw, struct clk_dummy_context, hw);
> +
> +       return ctx->rate;
> +}
> +
> +static int clk_dummy_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_set_rate(struct clk_hw *hw,
> +                             unsigned long rate,
> +                             unsigned long parent_rate)
> +{
> +       struct clk_dummy_context *ctx =
> +               container_of(hw, struct clk_dummy_context, hw);
> +
> +       ctx->rate = rate;
> +       return 0;
> +}
> +
> +static const struct clk_ops clk_dummy_ops = {

Maybe clk_dummy_rate_ops? So we don't mix it up with other dummy ops in
this file testing things that aren't rates.

> +       .recalc_rate = clk_dummy_recalc_rate,
> +       .determine_rate = clk_dummy_determine_rate,
> +       .set_rate = clk_dummy_set_rate,
> +};
> +
> +static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
> +{
> +       struct clk_dummy_context *ctx;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ctx = kunit_kzalloc(test, 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_test_init(struct kunit *test)
> +{
> +       return clk_test_init_with_ops(test, &clk_dummy_ops);
> +}
> +
> +static void clk_test_exit(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +
> +       clk_hw_unregister(&ctx->hw);
> +}
> +
> +/*
> + * Test that the actual rate matches what is returned by clk_get_rate()
> + */
> +static void clk_test_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_EXPECT_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_test_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * 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_test_set_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_test_cases[] = {
> +       KUNIT_CASE(clk_test_get_rate),
> +       KUNIT_CASE(clk_test_set_get_rate),
> +       KUNIT_CASE(clk_test_set_set_get_rate),
> +       {}
> +};
> +
> +static struct kunit_suite clk_test_suite = {
> +       .name = "clk-test",
> +       .init = clk_test_init,
> +       .exit = clk_test_exit,
> +       .test_cases = clk_test_cases,
> +};
> +
> +/*
> + * Test that clk_set_rate_range won't return an error for a valid range.
> + */
> +static void clk_range_test_set_range(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
> +       KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +/*
> + * Test that calling clk_set_rate_range with a minimum rate higher than
> + * the maximum rate returns an error.
> + */
> +static void clk_range_test_set_range_invalid(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +
> +       KUNIT_EXPECT_LT(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1 + 1000,
> +                                          DUMMY_CLOCK_RATE_1),
> +                       0);
> +}
> +
> +/*
> + * 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_range_test_set_range_get_rate_raised(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * 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_range_test_set_range_get_rate_lowered(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
> +                       0);
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_range_test_cases[] = {
> +       KUNIT_CASE(clk_range_test_set_range),
> +       KUNIT_CASE(clk_range_test_set_range_invalid),
> +       KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
> +       KUNIT_CASE(clk_range_test_set_range_get_rate_lowered),

Can you add a test case for round_rate matching what set_rate did, i.e.
calling clk_round_rate() and then clk_set_rate() followed by
clk_get_rate() with the same argument for round and set rate leads to
the same frequency?

It would also be good to add a test that tries to set the clk rate with
clk_set_rate() after a range has been set that is outside the acceptable
range and verify that it fails, and one that tries to set it within the
range and make sure it succeeds (and changes it to be exactly what was
set). Similarly, a call to set two disjoint ranges and verify that the
call that tries to set the second disjoint range fails. We want to test
the failure paths as well, to make sure we don't start causing them to
pass, unless it's expected. This patch could also contain the failure
scenario you're experiencing and mark it as expecting to fail. Then the
patch that fixes it in the core could mark the test as expecting to
pass, which may help us understand more easily what exactly changed
instead of having to figure that out after the fact by reading the
entire test.

> +       {}
> +};
> +
> +static struct kunit_suite clk_range_test_suite = {
> +       .name = "clk-range-test",
> +       .init = clk_test_init,
> +       .exit = clk_test_exit,
> +       .test_cases = clk_range_test_cases,
> +};

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Stephen,

Thanks for your review

On Fri, Feb 18, 2022 at 06:20:46PM -0800, Stephen Boyd wrote:
> It would also be good to add a test that tries to set the clk rate with
> clk_set_rate() after a range has been set that is outside the acceptable
> range and verify that it fails, and one that tries to set it within the
> range and make sure it succeeds (and changes it to be exactly what was
> set).

Do we expect it to fail though?

If we do:

clk_set_range_range(clk, 1000, 2000);
clk_set_rate(3000);

The current behaviour is that the rate is going to be rounded to 2000,
but it doesn't fail.

Or is it what you meant by fail? ie, that the return code is 0, but the
rate isn't what we asked for?

> Similarly, a call to set two disjoint ranges and verify that the call
> that tries to set the second disjoint range fails.

Ok

> We want to test the failure paths as well, to make sure we don't start
> causing them to pass, unless it's expected.

Do you have any other failure condition you want to test? I already
tried to come up with those I could think of, but I clearly missed some
if you said that :)

> This patch could also contain the failure scenario you're experiencing
> and mark it as expecting to fail. Then the patch that fixes it in the
> core could mark the test as expecting to pass, which may help us
> understand more easily what exactly changed instead of having to
> figure that out after the fact by reading the entire test.>

Ok

Maxime

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

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Stephen,

Thanks for your review

On Fri, Feb 18, 2022 at 06:20:46PM -0800, Stephen Boyd wrote:
> It would also be good to add a test that tries to set the clk rate with
> clk_set_rate() after a range has been set that is outside the acceptable
> range and verify that it fails, and one that tries to set it within the
> range and make sure it succeeds (and changes it to be exactly what was
> set).

Do we expect it to fail though?

If we do:

clk_set_range_range(clk, 1000, 2000);
clk_set_rate(3000);

The current behaviour is that the rate is going to be rounded to 2000,
but it doesn't fail.

Or is it what you meant by fail? ie, that the return code is 0, but the
rate isn't what we asked for?

> Similarly, a call to set two disjoint ranges and verify that the call
> that tries to set the second disjoint range fails.

Ok

> We want to test the failure paths as well, to make sure we don't start
> causing them to pass, unless it's expected.

Do you have any other failure condition you want to test? I already
tried to come up with those I could think of, but I clearly missed some
if you said that :)

> This patch could also contain the failure scenario you're experiencing
> and mark it as expecting to fail. Then the patch that fixes it in the
> core could mark the test as expecting to pass, which may help us
> understand more easily what exactly changed instead of having to
> figure that out after the fact by reading the entire test.>

Ok

Maxime

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

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 07:12:59)
> Hi Stephen,
> 
> Thanks for your review
> 
> On Fri, Feb 18, 2022 at 06:20:46PM -0800, Stephen Boyd wrote:
> > It would also be good to add a test that tries to set the clk rate with
> > clk_set_rate() after a range has been set that is outside the acceptable
> > range and verify that it fails, and one that tries to set it within the
> > range and make sure it succeeds (and changes it to be exactly what was
> > set).
> 
> Do we expect it to fail though?
> 
> If we do:
> 
> clk_set_range_range(clk, 1000, 2000);
> clk_set_rate(3000);
> 
> The current behaviour is that the rate is going to be rounded to 2000,
> but it doesn't fail.
> 
> Or is it what you meant by fail? ie, that the return code is 0, but the
> rate isn't what we asked for?

Yeah sorry for not being clear. I meant that it would be constrained to
the range from before.

> 
> > We want to test the failure paths as well, to make sure we don't start
> > causing them to pass, unless it's expected.
> 
> Do you have any other failure condition you want to test? I already
> tried to come up with those I could think of, but I clearly missed some
> if you said that :)

Not really! :)

Re: [PATCH v4 01/10] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 07:12:59)
> Hi Stephen,
> 
> Thanks for your review
> 
> On Fri, Feb 18, 2022 at 06:20:46PM -0800, Stephen Boyd wrote:
> > It would also be good to add a test that tries to set the clk rate with
> > clk_set_rate() after a range has been set that is outside the acceptable
> > range and verify that it fails, and one that tries to set it within the
> > range and make sure it succeeds (and changes it to be exactly what was
> > set).
> 
> Do we expect it to fail though?
> 
> If we do:
> 
> clk_set_range_range(clk, 1000, 2000);
> clk_set_rate(3000);
> 
> The current behaviour is that the rate is going to be rounded to 2000,
> but it doesn't fail.
> 
> Or is it what you meant by fail? ie, that the return code is 0, but the
> rate isn't what we asked for?

Yeah sorry for not being clear. I meant that it would be constrained to
the range from before.

> 
> > We want to test the failure paths as well, to make sure we don't start
> > causing them to pass, unless it's expected.
> 
> Do you have any other failure condition you want to test? I already
> tried to come up with those I could think of, but I clearly missed some
> if you said that :)

Not really! :)

[PATCH v4 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-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c      |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index 47a600d590c1..28c718ab82e1 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-test.c
@@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
 			0);
 }
 
+/*
+ * 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_range_test_set_range_round_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_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_range_test_set_range_round_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_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
@@ -266,6 +310,8 @@ static void clk_range_test_set_range_get_rate_lowered(struct kunit *test)
 static struct kunit_case clk_range_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range),
 	KUNIT_CASE(clk_range_test_set_range_invalid),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_higher),
 	KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
 	KUNIT_CASE(clk_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 v4 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-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c      |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index 47a600d590c1..28c718ab82e1 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-test.c
@@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
 			0);
 }
 
+/*
+ * 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_range_test_set_range_round_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_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_range_test_set_range_round_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_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
@@ -266,6 +310,8 @@ static void clk_range_test_set_range_get_rate_lowered(struct kunit *test)
 static struct kunit_case clk_range_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range),
 	KUNIT_CASE(clk_range_test_set_range_invalid),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_higher),
 	KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
 	KUNIT_CASE(clk_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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:41)
> 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.

It would be good to describe why. Or decide that it was an oversight and
write that down here.

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

I looked through the per-user constraint patch history on the list but I
couldn't really figure out why it was done this way. I guess we didn't
clamp the rate in the core because we wanted to give the clk providers
all the information, i.e. the rate that was requested and the boundaries
that the consumers have placed on the rate. With the round_rate() clk_op
the providers don't know the min/max because the rate request structure
isn't passed. I think my concern a long time ago was that a consumer
could call clk_round_rate() and get one frequency and then call
clk_set_rate() and get another frequency. We need to make sure that
round_rate and set_rate agree with each other. If we don't do that then
we don't uphold the contract that clk_round_rate() tells the consumer
what rate they'll get if they call clk_set_rate() with the same
frequency.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c      |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
> index 47a600d590c1..28c718ab82e1 100644
> --- a/drivers/clk/clk-test.c
> +++ b/drivers/clk/clk-test.c
> @@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
>                         0);
>  }
>  
> +/*
> + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

The comment says within range but this test says exactly the minimum
rate. Please change it to test that the rate is within rate 1 and rate
2. Also, we should call clk_get_rate() here to make sure the rate is
within the boundaries and matches what clk_round_rate() returned.

> +}
> +
> +/*
> + * 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_range_test_set_range_round_rate_higher(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);

Same comment about within range.

> +}
> +
>  /*
>   * 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
> 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
>

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:41)
> 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.

It would be good to describe why. Or decide that it was an oversight and
write that down here.

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

I looked through the per-user constraint patch history on the list but I
couldn't really figure out why it was done this way. I guess we didn't
clamp the rate in the core because we wanted to give the clk providers
all the information, i.e. the rate that was requested and the boundaries
that the consumers have placed on the rate. With the round_rate() clk_op
the providers don't know the min/max because the rate request structure
isn't passed. I think my concern a long time ago was that a consumer
could call clk_round_rate() and get one frequency and then call
clk_set_rate() and get another frequency. We need to make sure that
round_rate and set_rate agree with each other. If we don't do that then
we don't uphold the contract that clk_round_rate() tells the consumer
what rate they'll get if they call clk_set_rate() with the same
frequency.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c      |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
> index 47a600d590c1..28c718ab82e1 100644
> --- a/drivers/clk/clk-test.c
> +++ b/drivers/clk/clk-test.c
> @@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
>                         0);
>  }
>  
> +/*
> + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

The comment says within range but this test says exactly the minimum
rate. Please change it to test that the rate is within rate 1 and rate
2. Also, we should call clk_get_rate() here to make sure the rate is
within the boundaries and matches what clk_round_rate() returned.

> +}
> +
> +/*
> + * 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_range_test_set_range_round_rate_higher(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);

Same comment about within range.

> +}
> +
>  /*
>   * 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
> 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
>

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

[Maxime Ripard]

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

Hi,

On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-01-25 06:15:41)
> > 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.
> 
> It would be good to describe why. Or decide that it was an oversight and
> write that down here.
> 
> > 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.
> 
> I looked through the per-user constraint patch history on the list but I
> couldn't really figure out why it was done this way. I guess we didn't
> clamp the rate in the core because we wanted to give the clk providers
> all the information, i.e. the rate that was requested and the boundaries
> that the consumers have placed on the rate.

I'm not really sure we should really leave it to the users, something like:

clk_set_range_rate(clk, 1000, 2000);
clk_set_rate(clk, 500);
clk_get_rate(clk) # == 500

Is definitely weird, and would break the least surprise :)

We shouldn't leave that to drivers, especially since close to none of
them handle this properly.

> With the round_rate() clk_op the providers don't know the min/max
> because the rate request structure isn't passed. I think my concern a
> long time ago was that a consumer could call clk_round_rate() and get
> one frequency and then call clk_set_rate() and get another frequency.

I'm not sure I follow you there.

The function affected is clk_core_determine_round_nolock(), which is
called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
turn, they will be part of clk(_hw_)_round_clock for the former, and
clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.

I don't see how you can get a discrepancy between clk_round_rate() and
clk_set_rate().

And yeah, it's true that the round_rate op won't have the min and max
passed to them, but i'd consider this an argument for doing this check
here, since you don't have that option at all for those clocks.

> We need to make sure that round_rate and set_rate agree with each
> other. If we don't do that then we don't uphold the contract that
> clk_round_rate() tells the consumer what rate they'll get if they call
> clk_set_rate() with the same frequency.
> 
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/clk.c      |  2 ++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
> > index 47a600d590c1..28c718ab82e1 100644
> > --- a/drivers/clk/clk-test.c
> > +++ b/drivers/clk/clk-test.c
> > @@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
> >                         0);
> >  }
> >  
> > +/*
> > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > +{
> > +       struct clk_dummy_context *ctx = test->priv;
> > +       struct clk_hw *hw = &ctx->hw;
> > +       struct clk *clk = hw->clk;
> > +       long rate;
> > +
> > +       KUNIT_ASSERT_EQ(test,
> > +                       clk_set_rate_range(clk,
> > +                                          DUMMY_CLOCK_RATE_1,
> > +                                          DUMMY_CLOCK_RATE_2),
> > +                       0);
> > +
> > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > +       KUNIT_ASSERT_GT(test, rate, 0);
> > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> 
> The comment says within range but this test says exactly the minimum
> rate. Please change it to test that the rate is within rate 1 and rate
> 2. Also, we should call clk_get_rate() here to make sure the rate is
> within the boundaries and matches what clk_round_rate() returned.

Ok

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

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

[Maxime Ripard]

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

Hi,

On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-01-25 06:15:41)
> > 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.
> 
> It would be good to describe why. Or decide that it was an oversight and
> write that down here.
> 
> > 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.
> 
> I looked through the per-user constraint patch history on the list but I
> couldn't really figure out why it was done this way. I guess we didn't
> clamp the rate in the core because we wanted to give the clk providers
> all the information, i.e. the rate that was requested and the boundaries
> that the consumers have placed on the rate.

I'm not really sure we should really leave it to the users, something like:

clk_set_range_rate(clk, 1000, 2000);
clk_set_rate(clk, 500);
clk_get_rate(clk) # == 500

Is definitely weird, and would break the least surprise :)

We shouldn't leave that to drivers, especially since close to none of
them handle this properly.

> With the round_rate() clk_op the providers don't know the min/max
> because the rate request structure isn't passed. I think my concern a
> long time ago was that a consumer could call clk_round_rate() and get
> one frequency and then call clk_set_rate() and get another frequency.

I'm not sure I follow you there.

The function affected is clk_core_determine_round_nolock(), which is
called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
turn, they will be part of clk(_hw_)_round_clock for the former, and
clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.

I don't see how you can get a discrepancy between clk_round_rate() and
clk_set_rate().

And yeah, it's true that the round_rate op won't have the min and max
passed to them, but i'd consider this an argument for doing this check
here, since you don't have that option at all for those clocks.

> We need to make sure that round_rate and set_rate agree with each
> other. If we don't do that then we don't uphold the contract that
> clk_round_rate() tells the consumer what rate they'll get if they call
> clk_set_rate() with the same frequency.
> 
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/clk.c      |  2 ++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
> > index 47a600d590c1..28c718ab82e1 100644
> > --- a/drivers/clk/clk-test.c
> > +++ b/drivers/clk/clk-test.c
> > @@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
> >                         0);
> >  }
> >  
> > +/*
> > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > +{
> > +       struct clk_dummy_context *ctx = test->priv;
> > +       struct clk_hw *hw = &ctx->hw;
> > +       struct clk *clk = hw->clk;
> > +       long rate;
> > +
> > +       KUNIT_ASSERT_EQ(test,
> > +                       clk_set_rate_range(clk,
> > +                                          DUMMY_CLOCK_RATE_1,
> > +                                          DUMMY_CLOCK_RATE_2),
> > +                       0);
> > +
> > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > +       KUNIT_ASSERT_GT(test, rate, 0);
> > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> 
> The comment says within range but this test says exactly the minimum
> rate. Please change it to test that the rate is within rate 1 and rate
> 2. Also, we should call clk_get_rate() here to make sure the rate is
> within the boundaries and matches what clk_round_rate() returned.

Ok

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

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

[Maxime Ripard]

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

Hi again,

On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > +/*
> > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > +{
> > > +       struct clk_dummy_context *ctx = test->priv;
> > > +       struct clk_hw *hw = &ctx->hw;
> > > +       struct clk *clk = hw->clk;
> > > +       long rate;
> > > +
> > > +       KUNIT_ASSERT_EQ(test,
> > > +                       clk_set_rate_range(clk,
> > > +                                          DUMMY_CLOCK_RATE_1,
> > > +                                          DUMMY_CLOCK_RATE_2),
> > > +                       0);
> > > +
> > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > 
> > The comment says within range but this test says exactly the minimum
> > rate. Please change it to test that the rate is within rate 1 and rate
> > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > within the boundaries and matches what clk_round_rate() returned.
> 
> Ok

Actually, that doesn't work. Calling clk_round_rate() won't affect the
clock rate, so the rate returned by clk_get_rate() won't match what
clk_round_rate() will return.

Maxime

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

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

[Maxime Ripard]

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

Hi again,

On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > +/*
> > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > +{
> > > +       struct clk_dummy_context *ctx = test->priv;
> > > +       struct clk_hw *hw = &ctx->hw;
> > > +       struct clk *clk = hw->clk;
> > > +       long rate;
> > > +
> > > +       KUNIT_ASSERT_EQ(test,
> > > +                       clk_set_rate_range(clk,
> > > +                                          DUMMY_CLOCK_RATE_1,
> > > +                                          DUMMY_CLOCK_RATE_2),
> > > +                       0);
> > > +
> > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > 
> > The comment says within range but this test says exactly the minimum
> > rate. Please change it to test that the rate is within rate 1 and rate
> > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > within the boundaries and matches what clk_round_rate() returned.
> 
> Ok

Actually, that doesn't work. Calling clk_round_rate() won't affect the
clock rate, so the rate returned by clk_get_rate() won't match what
clk_round_rate() will return.

Maxime

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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:43:23)
> Hi again,
> 
> On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > +/*
> > > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > > +{
> > > > +       struct clk_dummy_context *ctx = test->priv;
> > > > +       struct clk_hw *hw = &ctx->hw;
> > > > +       struct clk *clk = hw->clk;
> > > > +       long rate;
> > > > +
> > > > +       KUNIT_ASSERT_EQ(test,
> > > > +                       clk_set_rate_range(clk,
> > > > +                                          DUMMY_CLOCK_RATE_1,
> > > > +                                          DUMMY_CLOCK_RATE_2),
> > > > +                       0);
> > > > +
> > > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > > 
> > > The comment says within range but this test says exactly the minimum
> > > rate. Please change it to test that the rate is within rate 1 and rate
> > > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > > within the boundaries and matches what clk_round_rate() returned.
> > 
> > Ok
> 
> Actually, that doesn't work. Calling clk_round_rate() won't affect the
> clock rate, so the rate returned by clk_get_rate() won't match what
> clk_round_rate() will return.

Huh? This is asking "what rate will I get if I call clk_set_rate() with
DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
2. It should round that up to some value (and we should enforce that it
is inclusive or exclusive). I think I missed that this is
clk_round_rate().

Either way, the clk provider implementation could say that if you call
clk_set_rate() with a frequency below the minimum that it lies somewhere
between the rate 1 and rate 2. The expectation should only check that it
is within the range and not exactly the minimum because we're not
testing the clk provider implementation of the rounding here, just that
the constraints are satisfied and the rate is within range. That's my
understanding of the comment above the function and the function name.

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:43:23)
> Hi again,
> 
> On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > +/*
> > > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > > +{
> > > > +       struct clk_dummy_context *ctx = test->priv;
> > > > +       struct clk_hw *hw = &ctx->hw;
> > > > +       struct clk *clk = hw->clk;
> > > > +       long rate;
> > > > +
> > > > +       KUNIT_ASSERT_EQ(test,
> > > > +                       clk_set_rate_range(clk,
> > > > +                                          DUMMY_CLOCK_RATE_1,
> > > > +                                          DUMMY_CLOCK_RATE_2),
> > > > +                       0);
> > > > +
> > > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > > 
> > > The comment says within range but this test says exactly the minimum
> > > rate. Please change it to test that the rate is within rate 1 and rate
> > > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > > within the boundaries and matches what clk_round_rate() returned.
> > 
> > Ok
> 
> Actually, that doesn't work. Calling clk_round_rate() won't affect the
> clock rate, so the rate returned by clk_get_rate() won't match what
> clk_round_rate() will return.

Huh? This is asking "what rate will I get if I call clk_set_rate() with
DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
2. It should round that up to some value (and we should enforce that it
is inclusive or exclusive). I think I missed that this is
clk_round_rate().

Either way, the clk provider implementation could say that if you call
clk_set_rate() with a frequency below the minimum that it lies somewhere
between the rate 1 and rate 2. The expectation should only check that it
is within the range and not exactly the minimum because we're not
testing the clk provider implementation of the rounding here, just that
the constraints are satisfied and the rate is within range. That's my
understanding of the comment above the function and the function name.

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

[Maxime Ripard]

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

Hi,

On Thu, Feb 24, 2022 at 02:39:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:43:23)
> > Hi again,
> > 
> > On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > > +/*
> > > > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > > > +{
> > > > > +       struct clk_dummy_context *ctx = test->priv;
> > > > > +       struct clk_hw *hw = &ctx->hw;
> > > > > +       struct clk *clk = hw->clk;
> > > > > +       long rate;
> > > > > +
> > > > > +       KUNIT_ASSERT_EQ(test,
> > > > > +                       clk_set_rate_range(clk,
> > > > > +                                          DUMMY_CLOCK_RATE_1,
> > > > > +                                          DUMMY_CLOCK_RATE_2),
> > > > > +                       0);
> > > > > +
> > > > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > > > 
> > > > The comment says within range but this test says exactly the minimum
> > > > rate. Please change it to test that the rate is within rate 1 and rate
> > > > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > > > within the boundaries and matches what clk_round_rate() returned.
> > > 
> > > Ok
> > 
> > Actually, that doesn't work. Calling clk_round_rate() won't affect the
> > clock rate, so the rate returned by clk_get_rate() won't match what
> > clk_round_rate() will return.
> 
> Huh? This is asking "what rate will I get if I call clk_set_rate() with
> DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
> 2. It should round that up to some value (and we should enforce that it
> is inclusive or exclusive). I think I missed that this is
> clk_round_rate().
> 
> Either way, the clk provider implementation could say that if you call
> clk_set_rate() with a frequency below the minimum that it lies somewhere
> between the rate 1 and rate 2. The expectation should only check that it
> is within the range and not exactly the minimum because we're not
> testing the clk provider implementation of the rounding here, just that
> the constraints are satisfied and the rate is within range. That's my
> understanding of the comment above the function and the function name.

You're right, that has been addressed in the last version I sent already

Maxime

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

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

[Maxime Ripard]

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

Hi,

On Thu, Feb 24, 2022 at 02:39:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:43:23)
> > Hi again,
> > 
> > On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > > +/*
> > > > > + * 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_range_test_set_range_round_rate_lower(struct kunit *test)
> > > > > +{
> > > > > +       struct clk_dummy_context *ctx = test->priv;
> > > > > +       struct clk_hw *hw = &ctx->hw;
> > > > > +       struct clk *clk = hw->clk;
> > > > > +       long rate;
> > > > > +
> > > > > +       KUNIT_ASSERT_EQ(test,
> > > > > +                       clk_set_rate_range(clk,
> > > > > +                                          DUMMY_CLOCK_RATE_1,
> > > > > +                                          DUMMY_CLOCK_RATE_2),
> > > > > +                       0);
> > > > > +
> > > > > +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> > > > > +       KUNIT_ASSERT_GT(test, rate, 0);
> > > > > +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> > > > 
> > > > The comment says within range but this test says exactly the minimum
> > > > rate. Please change it to test that the rate is within rate 1 and rate
> > > > 2. Also, we should call clk_get_rate() here to make sure the rate is
> > > > within the boundaries and matches what clk_round_rate() returned.
> > > 
> > > Ok
> > 
> > Actually, that doesn't work. Calling clk_round_rate() won't affect the
> > clock rate, so the rate returned by clk_get_rate() won't match what
> > clk_round_rate() will return.
> 
> Huh? This is asking "what rate will I get if I call clk_set_rate() with
> DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
> 2. It should round that up to some value (and we should enforce that it
> is inclusive or exclusive). I think I missed that this is
> clk_round_rate().
> 
> Either way, the clk provider implementation could say that if you call
> clk_set_rate() with a frequency below the minimum that it lies somewhere
> between the rate 1 and rate 2. The expectation should only check that it
> is within the range and not exactly the minimum because we're not
> testing the clk provider implementation of the rounding here, just that
> the constraints are satisfied and the rate is within range. That's my
> understanding of the comment above the function and the function name.

You're right, that has been addressed in the last version I sent already

Maxime

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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:18:21)
> Hi,
> 
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > 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.
> > 
> > It would be good to describe why. Or decide that it was an oversight and
> > write that down here.
> > 
> > > 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.
> > 
> > I looked through the per-user constraint patch history on the list but I
> > couldn't really figure out why it was done this way. I guess we didn't
> > clamp the rate in the core because we wanted to give the clk providers
> > all the information, i.e. the rate that was requested and the boundaries
> > that the consumers have placed on the rate.
> 
> I'm not really sure we should really leave it to the users, something like:
> 
> clk_set_range_rate(clk, 1000, 2000);
> clk_set_rate(clk, 500);
> clk_get_rate(clk) # == 500
> 
> Is definitely weird, and would break the least surprise :)
> 
> We shouldn't leave that to drivers, especially since close to none of
> them handle this properly.

Ok.

> 
> > With the round_rate() clk_op the providers don't know the min/max
> > because the rate request structure isn't passed. I think my concern a
> > long time ago was that a consumer could call clk_round_rate() and get
> > one frequency and then call clk_set_rate() and get another frequency.
> 
> I'm not sure I follow you there.
> 
> The function affected is clk_core_determine_round_nolock(), which is
> called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> turn, they will be part of clk(_hw_)_round_clock for the former, and
> clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> 
> I don't see how you can get a discrepancy between clk_round_rate() and
> clk_set_rate().
> 
> And yeah, it's true that the round_rate op won't have the min and max
> passed to them, but i'd consider this an argument for doing this check
> here, since you don't have that option at all for those clocks.

When the range setting API was introduced the rounding logic and the
rate setting logic didn't use the same code paths. It looks like that
code got consolidated now though so we should be fine.

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:18:21)
> Hi,
> 
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > 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.
> > 
> > It would be good to describe why. Or decide that it was an oversight and
> > write that down here.
> > 
> > > 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.
> > 
> > I looked through the per-user constraint patch history on the list but I
> > couldn't really figure out why it was done this way. I guess we didn't
> > clamp the rate in the core because we wanted to give the clk providers
> > all the information, i.e. the rate that was requested and the boundaries
> > that the consumers have placed on the rate.
> 
> I'm not really sure we should really leave it to the users, something like:
> 
> clk_set_range_rate(clk, 1000, 2000);
> clk_set_rate(clk, 500);
> clk_get_rate(clk) # == 500
> 
> Is definitely weird, and would break the least surprise :)
> 
> We shouldn't leave that to drivers, especially since close to none of
> them handle this properly.

Ok.

> 
> > With the round_rate() clk_op the providers don't know the min/max
> > because the rate request structure isn't passed. I think my concern a
> > long time ago was that a consumer could call clk_round_rate() and get
> > one frequency and then call clk_set_rate() and get another frequency.
> 
> I'm not sure I follow you there.
> 
> The function affected is clk_core_determine_round_nolock(), which is
> called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> turn, they will be part of clk(_hw_)_round_clock for the former, and
> clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> 
> I don't see how you can get a discrepancy between clk_round_rate() and
> clk_set_rate().
> 
> And yeah, it's true that the round_rate op won't have the min and max
> passed to them, but i'd consider this an argument for doing this check
> here, since you don't have that option at all for those clocks.

When the range setting API was introduced the rounding logic and the
rate setting logic didn't use the same code paths. It looks like that
code got consolidated now though so we should be fine.

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

[Maxime Ripard]

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

Hi,

On Thu, Feb 24, 2022 at 02:32:37PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:18:21)
> > Hi,
> > 
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > 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.
> > > 
> > > It would be good to describe why. Or decide that it was an oversight and
> > > write that down here.
> > > 
> > > > 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.
> > > 
> > > I looked through the per-user constraint patch history on the list but I
> > > couldn't really figure out why it was done this way. I guess we didn't
> > > clamp the rate in the core because we wanted to give the clk providers
> > > all the information, i.e. the rate that was requested and the boundaries
> > > that the consumers have placed on the rate.
> > 
> > I'm not really sure we should really leave it to the users, something like:
> > 
> > clk_set_range_rate(clk, 1000, 2000);
> > clk_set_rate(clk, 500);
> > clk_get_rate(clk) # == 500
> > 
> > Is definitely weird, and would break the least surprise :)
> > 
> > We shouldn't leave that to drivers, especially since close to none of
> > them handle this properly.
> 
> Ok.
> 
> > > With the round_rate() clk_op the providers don't know the min/max
> > > because the rate request structure isn't passed. I think my concern a
> > > long time ago was that a consumer could call clk_round_rate() and get
> > > one frequency and then call clk_set_rate() and get another frequency.
> > 
> > I'm not sure I follow you there.
> > 
> > The function affected is clk_core_determine_round_nolock(), which is
> > called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> > turn, they will be part of clk(_hw_)_round_clock for the former, and
> > clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> > 
> > I don't see how you can get a discrepancy between clk_round_rate() and
> > clk_set_rate().
> > 
> > And yeah, it's true that the round_rate op won't have the min and max
> > passed to them, but i'd consider this an argument for doing this check
> > here, since you don't have that option at all for those clocks.
> 
> When the range setting API was introduced the rounding logic and the
> rate setting logic didn't use the same code paths. It looks like that
> code got consolidated now though so we should be fine.

Actually, there was a discrepancy. If you are doing, before this patch
series:

clk_set_range_rate(clk, 1000, 2000)
clk_round_rate(500);

Unless the driver was involved, the returned rate would be 500.

Now, if you call clk_set_rate(500), it will return -EINVAL, hitting the
check here:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1973

If the driver was looking at the min and max and clamping the rate, you
would get clk_round_rate() == 1000 and clk_set_rate() would succeed,
with the rate set to 1000.

This seems like an abstraction leakage to me.

This patch fixes that discrepancy, but in the last version I sent, I
added a test that would check that once you have a range in place, then
clk_round_rate and clk_set_rate/clk_get_rate would return the same
value.

Maxime

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

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

[Maxime Ripard]

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

Hi,

On Thu, Feb 24, 2022 at 02:32:37PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:18:21)
> > Hi,
> > 
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > 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.
> > > 
> > > It would be good to describe why. Or decide that it was an oversight and
> > > write that down here.
> > > 
> > > > 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.
> > > 
> > > I looked through the per-user constraint patch history on the list but I
> > > couldn't really figure out why it was done this way. I guess we didn't
> > > clamp the rate in the core because we wanted to give the clk providers
> > > all the information, i.e. the rate that was requested and the boundaries
> > > that the consumers have placed on the rate.
> > 
> > I'm not really sure we should really leave it to the users, something like:
> > 
> > clk_set_range_rate(clk, 1000, 2000);
> > clk_set_rate(clk, 500);
> > clk_get_rate(clk) # == 500
> > 
> > Is definitely weird, and would break the least surprise :)
> > 
> > We shouldn't leave that to drivers, especially since close to none of
> > them handle this properly.
> 
> Ok.
> 
> > > With the round_rate() clk_op the providers don't know the min/max
> > > because the rate request structure isn't passed. I think my concern a
> > > long time ago was that a consumer could call clk_round_rate() and get
> > > one frequency and then call clk_set_rate() and get another frequency.
> > 
> > I'm not sure I follow you there.
> > 
> > The function affected is clk_core_determine_round_nolock(), which is
> > called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> > turn, they will be part of clk(_hw_)_round_clock for the former, and
> > clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> > 
> > I don't see how you can get a discrepancy between clk_round_rate() and
> > clk_set_rate().
> > 
> > And yeah, it's true that the round_rate op won't have the min and max
> > passed to them, but i'd consider this an argument for doing this check
> > here, since you don't have that option at all for those clocks.
> 
> When the range setting API was introduced the rounding logic and the
> rate setting logic didn't use the same code paths. It looks like that
> code got consolidated now though so we should be fine.

Actually, there was a discrepancy. If you are doing, before this patch
series:

clk_set_range_rate(clk, 1000, 2000)
clk_round_rate(500);

Unless the driver was involved, the returned rate would be 500.

Now, if you call clk_set_rate(500), it will return -EINVAL, hitting the
check here:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1973

If the driver was looking at the min and max and clamping the rate, you
would get clk_round_rate() == 1000 and clk_set_rate() would succeed,
with the rate set to 1000.

This seems like an abstraction leakage to me.

This patch fixes that discrepancy, but in the last version I sent, I
added a test that would check that once you have a range in place, then
clk_round_rate and clk_set_rate/clk_get_rate would return the same
value.

Maxime

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

[PATCH v4 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 v4 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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:42)
> 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);

This isn't equivalent. The else arm is taken if rate >= min and rate is
set to max, whereas clamp() will leave the rate unchanged if rate >= min
&& rate < max.

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:42)
> 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);

This isn't equivalent. The else arm is taken if rate >= min and rate is
set to max, whereas clamp() will leave the rate unchanged if rate >= min
&& rate < max.

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

[Maxime Ripard]

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

On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-01-25 06:15:42)
> > 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);
> 
> This isn't equivalent. The else arm is taken if rate >= min and rate is
> set to max, whereas clamp() will leave the rate unchanged if rate >= min
> && rate < max.

This can't happen, since we're in an if block that is (rate < min ||
rate > max), so at this point if rate is not less than min, it is
greater than rate. Thus, it's equivalent to clamp.

Still, the commit message could be better, I'll rephrase it.

Maxime

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

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

[Maxime Ripard]

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

On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-01-25 06:15:42)
> > 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);
> 
> This isn't equivalent. The else arm is taken if rate >= min and rate is
> set to max, whereas clamp() will leave the rate unchanged if rate >= min
> && rate < max.

This can't happen, since we're in an if block that is (rate < min ||
rate > max), so at this point if rate is not less than min, it is
greater than rate. Thus, it's equivalent to clamp.

Still, the commit message could be better, I'll rephrase it.

Maxime

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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:30:01)
> On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:42)
> > > 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);
> > 
> > This isn't equivalent. The else arm is taken if rate >= min and rate is
> > set to max, whereas clamp() will leave the rate unchanged if rate >= min
> > && rate < max.
> 
> This can't happen, since we're in an if block that is (rate < min ||
> rate > max), so at this point if rate is not less than min, it is
> greater than rate. Thus, it's equivalent to clamp.
> 
> Still, the commit message could be better, I'll rephrase it.

Perfect! Should probably add a comment above the clamp as well just in
case someone decides to move it out of that if block.

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-21 08:30:01)
> On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:42)
> > > 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);
> > 
> > This isn't equivalent. The else arm is taken if rate >= min and rate is
> > set to max, whereas clamp() will leave the rate unchanged if rate >= min
> > && rate < max.
> 
> This can't happen, since we're in an if block that is (rate < min ||
> rate > max), so at this point if rate is not less than min, it is
> greater than rate. Thus, it's equivalent to clamp.
> 
> Still, the commit message could be better, I'll rephrase it.

Perfect! Should probably add a comment above the clamp as well just in
case someone decides to move it out of that if block.

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

[Maxime Ripard]

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

On Thu, Feb 24, 2022 at 02:44:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:30:01)
> > On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:42)
> > > > 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);
> > > 
> > > This isn't equivalent. The else arm is taken if rate >= min and rate is
> > > set to max, whereas clamp() will leave the rate unchanged if rate >= min
> > > && rate < max.
> > 
> > This can't happen, since we're in an if block that is (rate < min ||
> > rate > max), so at this point if rate is not less than min, it is
> > greater than rate. Thus, it's equivalent to clamp.
> > 
> > Still, the commit message could be better, I'll rephrase it.
> 
> Perfect! Should probably add a comment above the clamp as well just in
> case someone decides to move it out of that if block.

The last version has a better commit message. We're actually moving that
out of the if in the next patch.

I'm not sure we really need a comment for this though:

The existing code does:

if (rate < min || rate > max)
    if (rate < min)
        rate = min;
    else
        rate = max;

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max.

With this patch, the code does:

if (rate < min || rate > max)
    rate = clamp(rate, min, max)

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max. It's
equivalent.

After the next patch, the code will do:
rate = clamp(rate, min, max)

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max. They are
all equivalent.

Maxime

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

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

[Maxime Ripard]

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

On Thu, Feb 24, 2022 at 02:44:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:30:01)
> > On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:42)
> > > > 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);
> > > 
> > > This isn't equivalent. The else arm is taken if rate >= min and rate is
> > > set to max, whereas clamp() will leave the rate unchanged if rate >= min
> > > && rate < max.
> > 
> > This can't happen, since we're in an if block that is (rate < min ||
> > rate > max), so at this point if rate is not less than min, it is
> > greater than rate. Thus, it's equivalent to clamp.
> > 
> > Still, the commit message could be better, I'll rephrase it.
> 
> Perfect! Should probably add a comment above the clamp as well just in
> case someone decides to move it out of that if block.

The last version has a better commit message. We're actually moving that
out of the if in the next patch.

I'm not sure we really need a comment for this though:

The existing code does:

if (rate < min || rate > max)
    if (rate < min)
        rate = min;
    else
        rate = max;

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max.

With this patch, the code does:

if (rate < min || rate > max)
    rate = clamp(rate, min, max)

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max. It's
equivalent.

After the next patch, the code will do:
rate = clamp(rate, min, max)

So if the rate is below min, it's set to min, if it's between min and
max, it's unaffected, and if it's above max, it's set to max. They are
all equivalent.

Maxime

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

[PATCH v4 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-test.c | 290 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c      |  45 +++----
 2 files changed, 313 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index 28c718ab82e1..cb749515837b 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-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_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int clk_dummy_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_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_set_rate(struct clk_hw *hw,
 			      unsigned long rate,
 			      unsigned long parent_rate)
@@ -50,6 +79,18 @@ static const struct clk_ops clk_dummy_ops = {
 	.set_rate = clk_dummy_set_rate,
 };
 
+static const struct clk_ops clk_dummy_maximize_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_maximize_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
+static const struct clk_ops clk_dummy_minimize_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_minimize_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
 static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
 {
 	struct clk_dummy_context *ctx;
@@ -78,6 +119,16 @@ static int clk_test_init(struct kunit *test)
 	return clk_test_init_with_ops(test, &clk_dummy_ops);
 }
 
+static int clk_maximize_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_maximize_rate_ops);
+}
+
+static int clk_minimize_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_minimize_rate_ops);
+}
+
 static void clk_test_exit(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
@@ -324,8 +375,247 @@ static struct kunit_suite clk_range_test_suite = {
 	.test_cases = clk_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_range_test_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2 - 1000),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_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_range_test_multiple_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   0,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   0,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2, 0, ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_range_maximize_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range_rate_maximized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_maximized),
+	{}
+};
+
+static struct kunit_suite clk_range_maximize_test_suite = {
+	.name = "clk-range-maximize-test",
+	.init = clk_maximize_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_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_range_test_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1 + 1000,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_range_test_multiple_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   DUMMY_CLOCK_RATE_1,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   DUMMY_CLOCK_RATE_2,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2, 0, ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_range_minimize_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range_rate_minimized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_minimized),
+	{}
+};
+
+static struct kunit_suite clk_range_minimize_test_suite = {
+	.name = "clk-range-minimize-test",
+	.init = clk_minimize_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_range_minimize_test_cases,
+};
+
 kunit_test_suites(
 	&clk_test_suite,
 	&clk_range_test_suite,
+	&clk_range_maximize_test_suite,
+	&clk_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 v4 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-test.c | 290 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c      |  45 +++----
 2 files changed, 313 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index 28c718ab82e1..cb749515837b 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-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_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int clk_dummy_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_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_set_rate(struct clk_hw *hw,
 			      unsigned long rate,
 			      unsigned long parent_rate)
@@ -50,6 +79,18 @@ static const struct clk_ops clk_dummy_ops = {
 	.set_rate = clk_dummy_set_rate,
 };
 
+static const struct clk_ops clk_dummy_maximize_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_maximize_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
+static const struct clk_ops clk_dummy_minimize_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_minimize_rate,
+	.set_rate = clk_dummy_set_rate,
+};
+
 static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
 {
 	struct clk_dummy_context *ctx;
@@ -78,6 +119,16 @@ static int clk_test_init(struct kunit *test)
 	return clk_test_init_with_ops(test, &clk_dummy_ops);
 }
 
+static int clk_maximize_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_maximize_rate_ops);
+}
+
+static int clk_minimize_test_init(struct kunit *test)
+{
+	return clk_test_init_with_ops(test, &clk_dummy_minimize_rate_ops);
+}
+
 static void clk_test_exit(struct kunit *test)
 {
 	struct clk_dummy_context *ctx = test->priv;
@@ -324,8 +375,247 @@ static struct kunit_suite clk_range_test_suite = {
 	.test_cases = clk_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_range_test_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2 - 1000),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_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_range_test_multiple_set_range_rate_maximized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   0,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   0,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2, 0, ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_range_maximize_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range_rate_maximized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_maximized),
+	{}
+};
+
+static struct kunit_suite clk_range_maximize_test_suite = {
+	.name = "clk-range-maximize-test",
+	.init = clk_maximize_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_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_range_test_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1 + 1000,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+}
+
+/*
+ * 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_range_test_multiple_set_range_rate_minimized(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	struct clk *user1, *user2;
+	unsigned long rate;
+
+	user1 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_create_clk(NULL, hw, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   DUMMY_CLOCK_RATE_1,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   DUMMY_CLOCK_RATE_2,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2, 0, ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+static struct kunit_case clk_range_minimize_test_cases[] = {
+	KUNIT_CASE(clk_range_test_set_range_rate_minimized),
+	KUNIT_CASE(clk_range_test_multiple_set_range_rate_minimized),
+	{}
+};
+
+static struct kunit_suite clk_range_minimize_test_suite = {
+	.name = "clk-range-minimize-test",
+	.init = clk_minimize_test_init,
+	.exit = clk_test_exit,
+	.test_cases = clk_range_minimize_test_cases,
+};
+
 kunit_test_suites(
 	&clk_test_suite,
 	&clk_range_test_suite,
+	&clk_range_maximize_test_suite,
+	&clk_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 v4 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-test.c |  4 ++--
 include/linux/clk.h    | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index cb749515837b..d1ce8879eb97 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-test.c
@@ -471,7 +471,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
 	KUNIT_ASSERT_EQ(test,
-			clk_set_rate_range(user2, 0, ULONG_MAX),
+			clk_drop_range(user2),
 			0);
 
 	rate = clk_get_rate(clk);
@@ -588,7 +588,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
 	KUNIT_ASSERT_EQ(test,
-			clk_set_rate_range(user2, 0, ULONG_MAX),
+			clk_drop_range(user2),
 			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 v4 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-test.c |  4 ++--
 include/linux/clk.h    | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
index cb749515837b..d1ce8879eb97 100644
--- a/drivers/clk/clk-test.c
+++ b/drivers/clk/clk-test.c
@@ -471,7 +471,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
 	KUNIT_ASSERT_EQ(test,
-			clk_set_rate_range(user2, 0, ULONG_MAX),
+			clk_drop_range(user2),
 			0);
 
 	rate = clk_get_rate(clk);
@@ -588,7 +588,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
 	KUNIT_ASSERT_EQ(test,
-			clk_set_rate_range(user2, 0, ULONG_MAX),
+			clk_drop_range(user2),
 			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

Re: [PATCH v4 05/10] clk: Add clk_drop_range

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:44)
> 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);
> +}

Please move this above clk_get_optional() as this is the "clk_get" zone
of this file.

> +
>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk *of_clk_get(struct device_node *np, int index);

Re: [PATCH v4 05/10] clk: Add clk_drop_range

[Stephen Boyd]

Quoting Maxime Ripard (2022-01-25 06:15:44)
> 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);
> +}

Please move this above clk_get_optional() as this is the "clk_get" zone
of this file.

> +
>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk *of_clk_get(struct device_node *np, int index);

[PATCH v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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

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

[Maxime Ripard]

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

Hi Stephen,

On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> 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.

As far as I know, this should address any concern you had with the
previous iterations.

Is there something else you'd like to see fixed/improved?

Maxime

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

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

[Maxime Ripard]

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

Hi Stephen,

On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> 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.

As far as I know, this should address any concern you had with the
previous iterations.

Is there something else you'd like to see fixed/improved?

Maxime

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

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-10 02:19:16)
> Hi Stephen,
> 
> On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> > 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.
> 
> As far as I know, this should address any concern you had with the
> previous iterations.
> 
> Is there something else you'd like to see fixed/improved?

Looks much improved. Some minor nits and requests for more test cases. I
hope we can merge it next week or so. I'll be on the lookout for the
next round.

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-10 02:19:16)
> Hi Stephen,
> 
> On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> > 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.
> 
> As far as I know, this should address any concern you had with the
> previous iterations.
> 
> Is there something else you'd like to see fixed/improved?

Looks much improved. Some minor nits and requests for more test cases. I
hope we can merge it next week or so. I'll be on the lookout for the
next round.

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Laurent Pinchart]

Hi Maxime and Stephen,

We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
(see [1]) which is a perfect candidate for this API, as it needs a
minimum rate for the VPU clock. Any chance we can get this series merged
? :-)

[1] https://lore.kernel.org/linux-media/20220208155027.891055-1-jeanmichel.hautbois@ideasonboard.com

On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> 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 v3:
>   - Renamed the test file and Kconfig option
>   - Add option to .kunitconfig
>   - Switch to kunit_kzalloc
>   - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant
>   - Test directly relevant calls instead of going through a temporary variable
>   - Switch to more precise KUNIT_ASSERT_* macros where relevant
> 
> 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: Introduce Kunit Tests for the framework
>   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/.kunitconfig          |   1 +
>  drivers/clk/Kconfig               |   7 +
>  drivers/clk/Makefile              |   1 +
>  drivers/clk/bcm/clk-raspberrypi.c | 125 +++++-
>  drivers/clk/clk-test.c            | 621 ++++++++++++++++++++++++++++++
>  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 +
>  9 files changed, 786 insertions(+), 55 deletions(-)
>  create mode 100644 drivers/clk/clk-test.c

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Laurent Pinchart]

Hi Maxime and Stephen,

We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
(see [1]) which is a perfect candidate for this API, as it needs a
minimum rate for the VPU clock. Any chance we can get this series merged
? :-)

[1] https://lore.kernel.org/linux-media/20220208155027.891055-1-jeanmichel.hautbois@ideasonboard.com

On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote:
> 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 v3:
>   - Renamed the test file and Kconfig option
>   - Add option to .kunitconfig
>   - Switch to kunit_kzalloc
>   - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant
>   - Test directly relevant calls instead of going through a temporary variable
>   - Switch to more precise KUNIT_ASSERT_* macros where relevant
> 
> 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: Introduce Kunit Tests for the framework
>   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/.kunitconfig          |   1 +
>  drivers/clk/Kconfig               |   7 +
>  drivers/clk/Makefile              |   1 +
>  drivers/clk/bcm/clk-raspberrypi.c | 125 +++++-
>  drivers/clk/clk-test.c            | 621 ++++++++++++++++++++++++++++++
>  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 +
>  9 files changed, 786 insertions(+), 55 deletions(-)
>  create mode 100644 drivers/clk/clk-test.c

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Stephen Boyd]

Quoting Laurent Pinchart (2022-02-14 01:45:56)
> Hi Maxime and Stephen,
> 
> We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
> (see [1]) which is a perfect candidate for this API, as it needs a
> minimum rate for the VPU clock. Any chance we can get this series merged
> ? :-)

The rate range API already exists, but it's busted. I can see you like
the patch series, can you provide any Reviewed-by or Tested-by tags?

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Stephen Boyd]

Quoting Laurent Pinchart (2022-02-14 01:45:56)
> Hi Maxime and Stephen,
> 
> We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
> (see [1]) which is a perfect candidate for this API, as it needs a
> minimum rate for the VPU clock. Any chance we can get this series merged
> ? :-)

The rate range API already exists, but it's busted. I can see you like
the patch series, can you provide any Reviewed-by or Tested-by tags?

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Laurent Pinchart]

Hi Stephen,

(CC'ing Jean-Michel)

On Fri, Feb 18, 2022 at 06:24:08PM -0800, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2022-02-14 01:45:56)
> > Hi Maxime and Stephen,
> > 
> > We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
> > (see [1]) which is a perfect candidate for this API, as it needs a
> > minimum rate for the VPU clock. Any chance we can get this series merged
> > ? :-)
> 
> The rate range API already exists, but it's busted. I can see you like
> the patch series, can you provide any Reviewed-by or Tested-by tags?

Jean-Michel, as you're working on upstreaming the RPi Unicam driver
which is our use case for this API, could you test this patch series ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/10] clk: Improve clock range handling

[Laurent Pinchart]

Hi Stephen,

(CC'ing Jean-Michel)

On Fri, Feb 18, 2022 at 06:24:08PM -0800, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2022-02-14 01:45:56)
> > Hi Maxime and Stephen,
> > 
> > We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver
> > (see [1]) which is a perfect candidate for this API, as it needs a
> > minimum rate for the VPU clock. Any chance we can get this series merged
> > ? :-)
> 
> The rate range API already exists, but it's busted. I can see you like
> the patch series, can you provide any Reviewed-by or Tested-by tags?

Jean-Michel, as you're working on upstreaming the RPi Unicam driver
which is our use case for this API, could you test this patch series ?

-- 
Regards,

Laurent Pinchart