[PATCH v6 00/12] clk: Improve clock range handling

[PATCH v6 00/12] 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 v5:
  - Changed clk_hw_create_clk for clk_hw_get_clk since the former isn't
    exported to modules.
  - Added fix for clk_hw_get_clk

Changes from v4:
  - Rename the test file
  - Move all the tests to the first patch, and fix them up as fixes are done
  - Improved the test conditions
  - Added more tests
  - Improved commit messages
  - Fixed a regression where two disjoints clock ranges would now be accepted

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 (12):
  clk: Fix clk_hw_get_clk() when dev is NULL
  clk: Introduce Kunit Tests for the framework
  clk: Enforce that disjoints limits are invalid
  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.c                 |  76 ++-
 drivers/clk/clk_test.c            | 790 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  13 -
 drivers/gpu/drm/vc4/vc4_kms.c     |  11 +
 include/linux/clk.h               |  11 +
 9 files changed, 980 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk_test.c

-- 
2.35.1

[PATCH v6 00/12] 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 v5:
  - Changed clk_hw_create_clk for clk_hw_get_clk since the former isn't
    exported to modules.
  - Added fix for clk_hw_get_clk

Changes from v4:
  - Rename the test file
  - Move all the tests to the first patch, and fix them up as fixes are done
  - Improved the test conditions
  - Added more tests
  - Improved commit messages
  - Fixed a regression where two disjoints clock ranges would now be accepted

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 (12):
  clk: Fix clk_hw_get_clk() when dev is NULL
  clk: Introduce Kunit Tests for the framework
  clk: Enforce that disjoints limits are invalid
  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.c                 |  76 ++-
 drivers/clk/clk_test.c            | 790 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  13 -
 drivers/gpu/drm/vc4/vc4_kms.c     |  11 +
 include/linux/clk.h               |  11 +
 9 files changed, 980 insertions(+), 55 deletions(-)
 create mode 100644 drivers/clk/clk_test.c

-- 
2.35.1

[PATCH v6 01/12] clk: Fix clk_hw_get_clk() when dev is NULL

[Maxime Ripard]

Any registered clk_core structure can have a NULL pointer in its dev
field. While never actually documented, this is evidenced by the wide
usage of clk_register and clk_hw_register with a NULL device pointer,
and the fact that the core of_clk_hw_register() function also passes a
NULL device pointer.

A call to clk_hw_get_clk() on a clk_hw struct whose clk_core is in that
case will result in a NULL pointer derefence when it calls dev_name() on
that NULL device pointer.

Add a test for this case and use NULL as the dev_id if the device
pointer is NULL.

Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8de6a22498e7..fff5edb89d6d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3773,8 +3773,9 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id)
 {
 	struct device *dev = hw->core->dev;
+	const char *name = dev ? dev_name(dev) : NULL;
 
-	return clk_hw_create_clk(dev, hw, dev_name(dev), con_id);
+	return clk_hw_create_clk(dev, hw, name, con_id);
 }
 EXPORT_SYMBOL(clk_hw_get_clk);
 
-- 
2.35.1

[PATCH v6 01/12] clk: Fix clk_hw_get_clk() when dev is NULL

[Maxime Ripard]

Any registered clk_core structure can have a NULL pointer in its dev
field. While never actually documented, this is evidenced by the wide
usage of clk_register and clk_hw_register with a NULL device pointer,
and the fact that the core of_clk_hw_register() function also passes a
NULL device pointer.

A call to clk_hw_get_clk() on a clk_hw struct whose clk_core is in that
case will result in a NULL pointer derefence when it calls dev_name() on
that NULL device pointer.

Add a test for this case and use NULL as the dev_id if the device
pointer is NULL.

Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8de6a22498e7..fff5edb89d6d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3773,8 +3773,9 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id)
 {
 	struct device *dev = hw->core->dev;
+	const char *name = dev ? dev_name(dev) : NULL;
 
-	return clk_hw_create_clk(dev, hw, dev_name(dev), con_id);
+	return clk_hw_create_clk(dev, hw, name, con_id);
 }
 EXPORT_SYMBOL(clk_hw_get_clk);
 
-- 
2.35.1

[PATCH v6 02/12] 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   | 786 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 795 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..8f9b1daba411 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..0ca6cd391c8e
--- /dev/null
+++ b/drivers/clk/clk_test.c
@@ -0,0 +1,786 @@
+// 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>
+
+/* Needed for clk_hw_get_clk() */
+#include "clk.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_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)
+{
+	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_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_determine_rate,
+	.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;
+	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_rate_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;
+
+	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);
+}
+
+/*
+ * Test that clk_round_rate and clk_set_rate are consitent and will
+ * return the same frequency.
+ */
+static void clk_test_round_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 rounded_rate, set_rate;
+
+	rounded_rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_GT(test, rounded_rate, 0);
+	KUNIT_EXPECT_EQ(test, rounded_rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	set_rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, set_rate, 0);
+	KUNIT_EXPECT_EQ(test, rounded_rate, set_rate);
+}
+
+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),
+	KUNIT_CASE(clk_test_round_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 users can't set multiple, disjoints, range that would be
+ * impossible to meet.
+ */
+static void clk_range_test_multiple_disjoints_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *user1, *user2;
+
+	user1 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1, 1000, 2000),
+			0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate_range(user2, 3000, 4000),
+			0);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * lower than the minimum, the returned rate won't be affected by the
+ * boundaries.
+ */
+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 - 1000);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to set a rate
+ * lower than the minimum, we'll get an error.
+ */
+static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	KUNIT_ASSERT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round and
+ * set a rate lower than the minimum, the values won't be consistent
+ * between clk_round_rate() and clk_set_rate().
+ */
+static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rounded;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rounded, 0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * higher than the maximum, the returned rate won't be affected by the
+ * boundaries.
+ */
+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 + 1000);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to set a rate
+ * lower than the maximum, we'll get an error.
+ */
+static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	KUNIT_ASSERT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round and
+ * set a rate higher than the maximum, the values won't be consistent
+ * between clk_round_rate() and clk_set_rate().
+ */
+static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rounded;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_GT(test, rounded, 0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+}
+
+/*
+ * 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_multiple_disjoints_range),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_set_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_set_round_rate_consistent_lower),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_higher),
+	KUNIT_CASE(clk_range_test_set_range_set_rate_higher),
+	KUNIT_CASE(clk_range_test_set_range_set_round_rate_consistent_higher),
+	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,
+};
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range();
+ * - and we have a round_rate ops that always return the maximum
+ *   frequency allowed;
+ *
+ * The clock will run at the minimum of all maximum boundaries
+ * requested, even if those boundaries aren't there anymore.
+ */
+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 - 1000);
+}
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range(), across
+ *   multiple users;
+ * - and we have a round_rate ops that always return the maximum
+ *   frequency allowed;
+ *
+ * The clock will run at the minimum of all maximum boundaries
+ * requested, even if those boundaries aren't there anymore.
+ */
+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_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   0,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   0,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	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_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()
+ * - and we have a round_rate ops that always return the minimum
+ *   frequency allowed;
+ *
+ * The clock will run at the maximum of all minimum boundaries
+ * requested, even if those boundaries aren't there anymore.
+*/
+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 + 1000);
+}
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range(), across
+ *   multiple users;
+ * - and we have a round_rate ops that always return the minimum
+ *   frequency allowed;
+ *
+ * The clock will run at the maximum of all minimum boundaries
+ * requested, even if those boundaries aren't there anymore.
+*/
+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_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   DUMMY_CLOCK_RATE_1,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   DUMMY_CLOCK_RATE_2,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	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_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");
-- 
2.35.1

[PATCH v6 02/12] 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   | 786 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 795 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..8f9b1daba411 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..0ca6cd391c8e
--- /dev/null
+++ b/drivers/clk/clk_test.c
@@ -0,0 +1,786 @@
+// 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>
+
+/* Needed for clk_hw_get_clk() */
+#include "clk.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_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)
+{
+	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_rate_ops = {
+	.recalc_rate = clk_dummy_recalc_rate,
+	.determine_rate = clk_dummy_determine_rate,
+	.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;
+	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_rate_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;
+
+	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);
+}
+
+/*
+ * Test that clk_round_rate and clk_set_rate are consitent and will
+ * return the same frequency.
+ */
+static void clk_test_round_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 rounded_rate, set_rate;
+
+	rounded_rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_GT(test, rounded_rate, 0);
+	KUNIT_EXPECT_EQ(test, rounded_rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1),
+			0);
+
+	set_rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, set_rate, 0);
+	KUNIT_EXPECT_EQ(test, rounded_rate, set_rate);
+}
+
+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),
+	KUNIT_CASE(clk_test_round_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 users can't set multiple, disjoints, range that would be
+ * impossible to meet.
+ */
+static void clk_range_test_multiple_disjoints_range(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *user1, *user2;
+
+	user1 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1, 1000, 2000),
+			0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate_range(user2, 3000, 4000),
+			0);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * lower than the minimum, the returned rate won't be affected by the
+ * boundaries.
+ */
+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 - 1000);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to set a rate
+ * lower than the minimum, we'll get an error.
+ */
+static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	KUNIT_ASSERT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round and
+ * set a rate lower than the minimum, the values won't be consistent
+ * between clk_round_rate() and clk_set_rate().
+ */
+static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rounded;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
+	KUNIT_ASSERT_GT(test, rounded, 0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
+			0);
+
+	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round a rate
+ * higher than the maximum, the returned rate won't be affected by the
+ * boundaries.
+ */
+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 + 1000);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to set a rate
+ * lower than the maximum, we'll get an error.
+ */
+static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	KUNIT_ASSERT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+}
+
+/*
+ * Test that if our clock has some boundaries and we try to round and
+ * set a rate higher than the maximum, the values won't be consistent
+ * between clk_round_rate() and clk_set_rate().
+ */
+static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	long rounded;
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(clk,
+					   DUMMY_CLOCK_RATE_1,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
+	KUNIT_ASSERT_GT(test, rounded, 0);
+
+	KUNIT_EXPECT_LT(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+}
+
+/*
+ * 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_multiple_disjoints_range),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_set_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_set_round_rate_consistent_lower),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_higher),
+	KUNIT_CASE(clk_range_test_set_range_set_rate_higher),
+	KUNIT_CASE(clk_range_test_set_range_set_round_rate_consistent_higher),
+	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,
+};
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range();
+ * - and we have a round_rate ops that always return the maximum
+ *   frequency allowed;
+ *
+ * The clock will run at the minimum of all maximum boundaries
+ * requested, even if those boundaries aren't there anymore.
+ */
+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 - 1000);
+}
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range(), across
+ *   multiple users;
+ * - and we have a round_rate ops that always return the maximum
+ *   frequency allowed;
+ *
+ * The clock will run at the minimum of all maximum boundaries
+ * requested, even if those boundaries aren't there anymore.
+ */
+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_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+			0);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   0,
+					   DUMMY_CLOCK_RATE_2),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   0,
+					   DUMMY_CLOCK_RATE_1),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	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_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()
+ * - and we have a round_rate ops that always return the minimum
+ *   frequency allowed;
+ *
+ * The clock will run at the maximum of all minimum boundaries
+ * requested, even if those boundaries aren't there anymore.
+*/
+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 + 1000);
+}
+
+/*
+ * Test that if:
+ * - we have several subsequent calls to clk_set_rate_range(), across
+ *   multiple users;
+ * - and we have a round_rate ops that always return the minimum
+ *   frequency allowed;
+ *
+ * The clock will run at the maximum of all minimum boundaries
+ * requested, even if those boundaries aren't there anymore.
+*/
+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_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user1,
+					   DUMMY_CLOCK_RATE_1,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	KUNIT_ASSERT_EQ(test,
+			clk_set_rate_range(user2,
+					   DUMMY_CLOCK_RATE_2,
+					   ULONG_MAX),
+			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+	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_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");
-- 
2.35.1

[PATCH v6 03/12] clk: Enforce that disjoints limits are invalid

[Maxime Ripard]

If we were to have two users of the same clock, doing something like:

clk_set_rate_range(user1, 1000, 2000);
clk_set_rate_range(user2, 3000, 4000);

The second call would fail with -EINVAL, preventing from getting in a
situation where we end up with impossible limits.

However, this is never explicitly checked against and enforced, and
works by relying on an undocumented behaviour of clk_set_rate().

Indeed, on the first clk_set_rate_range will make sure the current clock
rate is within the new range, so it will be between 1000 and 2000Hz. On
the second clk_set_rate_range(), it will consider (rightfully), that our
current clock is outside of the 3000-4000Hz range, and will call
clk_core_set_rate_nolock() to set it to 3000Hz.

clk_core_set_rate_nolock() will then call clk_calc_new_rates() that will
eventually check that our rate 3000Hz rate is outside the min 3000Hz max
2000Hz range, will bail out, the error will propagate and we'll
eventually return -EINVAL.

This solely relies on the fact that clk_calc_new_rates(), and in
particular clk_core_determine_round_nolock(), won't modify the new rate
allowing the error to be reported. That assumption won't be true for all
drivers, and most importantly we'll break that assumption in a later
patch.

It can also be argued that we shouldn't even reach the point where we're
calling clk_core_set_rate_nolock().

Let's make an explicit check for disjoints range before we're doing
anything.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fff5edb89d6d..112911138a7b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -632,6 +632,24 @@ static void clk_core_get_boundaries(struct clk_core *core,
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
 
+static bool clk_core_check_boundaries(struct clk_core *core,
+				      unsigned long min_rate,
+				      unsigned long max_rate)
+{
+	struct clk *user;
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (min_rate > core->max_rate || max_rate < core->min_rate)
+		return false;
+
+	hlist_for_each_entry(user, &core->clks, clks_node)
+		if (min_rate > user->max_rate || max_rate < user->min_rate)
+			return false;
+
+	return true;
+}
+
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate)
 {
@@ -2348,6 +2366,11 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk->min_rate = min;
 	clk->max_rate = max;
 
+	if (!clk_core_check_boundaries(clk->core, min, max)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	rate = clk_core_get_rate_nolock(clk->core);
 	if (rate < min || rate > max) {
 		/*
@@ -2376,6 +2399,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		}
 	}
 
+out:
 	if (clk->exclusive_count)
 		clk_core_rate_protect(clk->core);
 
-- 
2.35.1

[PATCH v6 03/12] clk: Enforce that disjoints limits are invalid

[Maxime Ripard]

If we were to have two users of the same clock, doing something like:

clk_set_rate_range(user1, 1000, 2000);
clk_set_rate_range(user2, 3000, 4000);

The second call would fail with -EINVAL, preventing from getting in a
situation where we end up with impossible limits.

However, this is never explicitly checked against and enforced, and
works by relying on an undocumented behaviour of clk_set_rate().

Indeed, on the first clk_set_rate_range will make sure the current clock
rate is within the new range, so it will be between 1000 and 2000Hz. On
the second clk_set_rate_range(), it will consider (rightfully), that our
current clock is outside of the 3000-4000Hz range, and will call
clk_core_set_rate_nolock() to set it to 3000Hz.

clk_core_set_rate_nolock() will then call clk_calc_new_rates() that will
eventually check that our rate 3000Hz rate is outside the min 3000Hz max
2000Hz range, will bail out, the error will propagate and we'll
eventually return -EINVAL.

This solely relies on the fact that clk_calc_new_rates(), and in
particular clk_core_determine_round_nolock(), won't modify the new rate
allowing the error to be reported. That assumption won't be true for all
drivers, and most importantly we'll break that assumption in a later
patch.

It can also be argued that we shouldn't even reach the point where we're
calling clk_core_set_rate_nolock().

Let's make an explicit check for disjoints range before we're doing
anything.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fff5edb89d6d..112911138a7b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -632,6 +632,24 @@ static void clk_core_get_boundaries(struct clk_core *core,
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
 
+static bool clk_core_check_boundaries(struct clk_core *core,
+				      unsigned long min_rate,
+				      unsigned long max_rate)
+{
+	struct clk *user;
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (min_rate > core->max_rate || max_rate < core->min_rate)
+		return false;
+
+	hlist_for_each_entry(user, &core->clks, clks_node)
+		if (min_rate > user->max_rate || max_rate < user->min_rate)
+			return false;
+
+	return true;
+}
+
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate)
 {
@@ -2348,6 +2366,11 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk->min_rate = min;
 	clk->max_rate = max;
 
+	if (!clk_core_check_boundaries(clk->core, min, max)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	rate = clk_core_get_rate_nolock(clk->core);
 	if (rate < min || rate > max) {
 		/*
@@ -2376,6 +2399,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		}
 	}
 
+out:
 	if (clk->exclusive_count)
 		clk_core_rate_protect(clk->core);
 
-- 
2.35.1

[PATCH v6 04/12] 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.

It's not clear if this was on purpose or not, but this introduces some
inconsistencies within the API.

For example, a user setting a range and then calling clk_round_rate()
with a value outside of that range will get the same value back
(ignoring any driver adjustements), effectively ignoring the range that
was just set.

Another one, arguably worse, is that it also makes clk_round_rate() and
clk_set_rate() behave differently if there's a range and the rate being
used for both is outside that range. As we have seen, the rate will be
returned unchanged by clk_round_rate(), but clk_set_rate() will error
out returning -EINVAL.

Let's make sure the framework will always clamp the rate to the current
range found on the clock, which will fix both these inconsistencies.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 112911138a7b..6c4e10209568 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1348,6 +1348,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
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0ca6cd391c8e..5a740f301f67 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -309,8 +309,7 @@ static void clk_range_test_multiple_disjoints_range(struct kunit *test)
 
 /*
  * Test that if our clock has some boundaries and we try to round a rate
- * lower than the minimum, the returned rate won't be affected by the
- * boundaries.
+ * lower than the minimum, the returned rate will be within range.
  */
 static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 {
@@ -327,18 +326,19 @@ static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 
 	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 - 1000);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to set a rate
- * lower than the minimum, we'll get an error.
+ * higher than the maximum, the new rate will be within range.
  */
 static void clk_range_test_set_range_set_rate_lower(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,
@@ -346,15 +346,20 @@ static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
 					   DUMMY_CLOCK_RATE_2),
 			0);
 
-	KUNIT_ASSERT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
 			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round and
- * set a rate lower than the minimum, the values won't be consistent
- * between clk_round_rate() and clk_set_rate().
+ * set a rate lower than the minimum, the rate returned by
+ * clk_round_rate() will be consistent with the new rate set by
+ * clk_set_rate().
  */
 static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kunit *test)
 {
@@ -372,17 +377,16 @@ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kuni
 	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
 	KUNIT_ASSERT_GT(test, rounded, 0);
 
-	KUNIT_EXPECT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
 			0);
 
-	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round a rate
- * higher than the maximum, the returned rate won't be affected by the
- * boundaries.
+ * higher than the maximum, the returned rate will be within range.
  */
 static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 {
@@ -399,18 +403,19 @@ static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 
 	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 + 1000);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to set a rate
- * lower than the maximum, we'll get an error.
+ * higher than the maximum, the new rate will be within range.
  */
 static void clk_range_test_set_range_set_rate_higher(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,
@@ -418,15 +423,20 @@ static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
 					   DUMMY_CLOCK_RATE_2),
 			0);
 
-	KUNIT_ASSERT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
 			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round and
- * set a rate higher than the maximum, the values won't be consistent
- * between clk_round_rate() and clk_set_rate().
+ * set a rate higher than the maximum, the rate returned by
+ * clk_round_rate() will be consistent with the new rate set by
+ * clk_set_rate().
  */
 static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kunit *test)
 {
@@ -444,11 +454,11 @@ static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kun
 	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
 	KUNIT_ASSERT_GT(test, rounded, 0);
 
-	KUNIT_EXPECT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
 			0);
 
-	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
 }
 
 /*
-- 
2.35.1

[PATCH v6 04/12] 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.

It's not clear if this was on purpose or not, but this introduces some
inconsistencies within the API.

For example, a user setting a range and then calling clk_round_rate()
with a value outside of that range will get the same value back
(ignoring any driver adjustements), effectively ignoring the range that
was just set.

Another one, arguably worse, is that it also makes clk_round_rate() and
clk_set_rate() behave differently if there's a range and the rate being
used for both is outside that range. As we have seen, the rate will be
returned unchanged by clk_round_rate(), but clk_set_rate() will error
out returning -EINVAL.

Let's make sure the framework will always clamp the rate to the current
range found on the clock, which will fix both these inconsistencies.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 112911138a7b..6c4e10209568 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1348,6 +1348,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
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0ca6cd391c8e..5a740f301f67 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -309,8 +309,7 @@ static void clk_range_test_multiple_disjoints_range(struct kunit *test)
 
 /*
  * Test that if our clock has some boundaries and we try to round a rate
- * lower than the minimum, the returned rate won't be affected by the
- * boundaries.
+ * lower than the minimum, the returned rate will be within range.
  */
 static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 {
@@ -327,18 +326,19 @@ static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
 
 	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 - 1000);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to set a rate
- * lower than the minimum, we'll get an error.
+ * higher than the maximum, the new rate will be within range.
  */
 static void clk_range_test_set_range_set_rate_lower(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,
@@ -346,15 +346,20 @@ static void clk_range_test_set_range_set_rate_lower(struct kunit *test)
 					   DUMMY_CLOCK_RATE_2),
 			0);
 
-	KUNIT_ASSERT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
 			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round and
- * set a rate lower than the minimum, the values won't be consistent
- * between clk_round_rate() and clk_set_rate().
+ * set a rate lower than the minimum, the rate returned by
+ * clk_round_rate() will be consistent with the new rate set by
+ * clk_set_rate().
  */
 static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kunit *test)
 {
@@ -372,17 +377,16 @@ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kuni
 	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
 	KUNIT_ASSERT_GT(test, rounded, 0);
 
-	KUNIT_EXPECT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000),
 			0);
 
-	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round a rate
- * higher than the maximum, the returned rate won't be affected by the
- * boundaries.
+ * higher than the maximum, the returned rate will be within range.
  */
 static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 {
@@ -399,18 +403,19 @@ static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
 
 	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 + 1000);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to set a rate
- * lower than the maximum, we'll get an error.
+ * higher than the maximum, the new rate will be within range.
  */
 static void clk_range_test_set_range_set_rate_higher(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,
@@ -418,15 +423,20 @@ static void clk_range_test_set_range_set_rate_higher(struct kunit *test)
 					   DUMMY_CLOCK_RATE_2),
 			0);
 
-	KUNIT_ASSERT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
 			0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2);
 }
 
 /*
  * Test that if our clock has some boundaries and we try to round and
- * set a rate higher than the maximum, the values won't be consistent
- * between clk_round_rate() and clk_set_rate().
+ * set a rate higher than the maximum, the rate returned by
+ * clk_round_rate() will be consistent with the new rate set by
+ * clk_set_rate().
  */
 static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kunit *test)
 {
@@ -444,11 +454,11 @@ static void clk_range_test_set_range_set_round_rate_consistent_higher(struct kun
 	rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
 	KUNIT_ASSERT_GT(test, rounded, 0);
 
-	KUNIT_EXPECT_LT(test,
+	KUNIT_ASSERT_EQ(test,
 			clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
 			0);
 
-	KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk));
+	KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk));
 }
 
 /*
-- 
2.35.1

[PATCH v6 05/12] 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.

Since it's running under the condition that the rate is either lower
than the minimum, or higher than the 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 6c4e10209568..c15ee5070f52 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2388,11 +2388,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.35.1

[PATCH v6 05/12] 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.

Since it's running under the condition that the rate is either lower
than the minimum, or higher than the 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 6c4e10209568..c15ee5070f52 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2388,11 +2388,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.35.1

[PATCH v6 06/12] 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.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      | 45 ++++++++++++++++----------------
 drivers/clk/clk_test.c | 58 +++++++++++++++++++-----------------------
 2 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c15ee5070f52..9bc8bf434b94 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2373,28 +2373,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		goto out;
 	}
 
-	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;
 	}
 
 out:
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 5a740f301f67..60c206d1bcb0 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -544,13 +544,12 @@ static struct kunit_suite clk_range_test_suite = {
 };
 
 /*
- * Test that if:
- * - we have several subsequent calls to clk_set_rate_range();
- * - and we have a round_rate ops that always return the maximum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the minimum of all maximum boundaries
- * requested, even if those boundaries aren't there anymore.
+ * 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)
 {
@@ -591,18 +590,16 @@ static void clk_range_test_set_range_rate_maximized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+	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;
- * - and we have a round_rate ops that always return the maximum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the minimum of all maximum boundaries
- * requested, even if those boundaries aren't there anymore.
+ * 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)
 {
@@ -648,7 +645,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
 	clk_put(user2);
 	clk_put(user1);
@@ -668,14 +665,13 @@ static struct kunit_suite clk_range_maximize_test_suite = {
 };
 
 /*
- * Test that if:
- * - we have several subsequent calls to clk_set_rate_range()
- * - and we have a round_rate ops that always return the minimum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the maximum of all minimum boundaries
- * requested, even if those boundaries aren't there anymore.
-*/
+ * 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;
@@ -715,19 +711,17 @@ static void clk_range_test_set_range_rate_minimized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+	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;
- * - and we have a round_rate ops that always return the minimum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the maximum of all minimum boundaries
- * requested, even if those boundaries aren't there anymore.
-*/
+ * 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;
@@ -768,7 +762,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
 	clk_put(user2);
 	clk_put(user1);
-- 
2.35.1

[PATCH v6 06/12] 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.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      | 45 ++++++++++++++++----------------
 drivers/clk/clk_test.c | 58 +++++++++++++++++++-----------------------
 2 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c15ee5070f52..9bc8bf434b94 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2373,28 +2373,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		goto out;
 	}
 
-	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;
 	}
 
 out:
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 5a740f301f67..60c206d1bcb0 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -544,13 +544,12 @@ static struct kunit_suite clk_range_test_suite = {
 };
 
 /*
- * Test that if:
- * - we have several subsequent calls to clk_set_rate_range();
- * - and we have a round_rate ops that always return the maximum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the minimum of all maximum boundaries
- * requested, even if those boundaries aren't there anymore.
+ * 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)
 {
@@ -591,18 +590,16 @@ static void clk_range_test_set_range_rate_maximized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000);
+	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;
- * - and we have a round_rate ops that always return the maximum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the minimum of all maximum boundaries
- * requested, even if those boundaries aren't there anymore.
+ * 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)
 {
@@ -648,7 +645,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
 
 	clk_put(user2);
 	clk_put(user1);
@@ -668,14 +665,13 @@ static struct kunit_suite clk_range_maximize_test_suite = {
 };
 
 /*
- * Test that if:
- * - we have several subsequent calls to clk_set_rate_range()
- * - and we have a round_rate ops that always return the minimum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the maximum of all minimum boundaries
- * requested, even if those boundaries aren't there anymore.
-*/
+ * 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;
@@ -715,19 +711,17 @@ static void clk_range_test_set_range_rate_minimized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000);
+	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;
- * - and we have a round_rate ops that always return the minimum
- *   frequency allowed;
+ * 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.
  *
- * The clock will run at the maximum of all minimum boundaries
- * requested, even if those boundaries aren't there anymore.
-*/
+ * 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;
@@ -768,7 +762,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_GT(test, rate, 0);
-	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
 	clk_put(user2);
 	clk_put(user1);
-- 
2.35.1

[PATCH v6 07/12] 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 60c206d1bcb0..490d364642b6 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -640,7 +640,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);
@@ -757,7 +757,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..39faa54efe88 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -986,6 +986,17 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
 	clk_bulk_unprepare(num_clks, clks);
 }
 
+/**
+ * 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);
+}
+
 /**
  * clk_get_optional - lookup and obtain a reference to an optional clock
  *		      producer.
-- 
2.35.1

[PATCH v6 07/12] 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 60c206d1bcb0..490d364642b6 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -640,7 +640,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);
@@ -757,7 +757,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..39faa54efe88 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -986,6 +986,17 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
 	clk_bulk_unprepare(num_clks, clks);
 }
 
+/**
+ * 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);
+}
+
 /**
  * clk_get_optional - lookup and obtain a reference to an optional clock
  *		      producer.
-- 
2.35.1

[PATCH v6 08/12] 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.35.1

[PATCH v6 08/12] 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.35.1

[PATCH v6 09/12] 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.35.1

[PATCH v6 09/12] 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.35.1

[PATCH v6 10/12] 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.35.1

[PATCH v6 10/12] 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.35.1

[PATCH v6 11/12] 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.35.1

[PATCH v6 11/12] 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.35.1

[PATCH v6 12/12] 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 92b1530aa17b..21aff3ad96cf 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2576,19 +2576,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.35.1

[PATCH v6 12/12] 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 92b1530aa17b..21aff3ad96cf 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2576,19 +2576,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.35.1

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> 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>

Tested-by: Daniel Latypov <dlatypov@google.com>

Looks good to me on the KUnit side.
Two small nits below.

FYI, I computed the incremental coverage for this series, i.e.:
1) applied the full series
2) computed the absolute coverage

$  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
--make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
--kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
$ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6

3) intersected that with the total diff

Incremental coverage for 3/9 files in --diff_file
Total incremental: 99.29% coverage (281/283 lines)
  drivers/clk/clk.c: 84.62% coverage (11/13 lines)
  drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
  include/linux/clk.h: 100.00% coverage (1/1 lines)

Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
+       if (ret) {
+               /* rollback the changes */
+               clk->min_rate = old_min; <- 2397
+               clk->max_rate = old_max; <- 2398

These are from before and were just moved around.

Note: this filters out code that wasn't compiled in and wasn't executable.
So that's why it's missing clk-raspberrypi.c and friends and it says
clk.c had 13 changed lines (since most of the lines are comments).

>
> ---
>  drivers/clk/.kunitconfig |   1 +
>  drivers/clk/Kconfig      |   7 +
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk_test.c   | 786 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 795 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..8f9b1daba411 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..0ca6cd391c8e
> --- /dev/null
> +++ b/drivers/clk/clk_test.c
> @@ -0,0 +1,786 @@
> +// 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>

Very nit:
Is this #include <linux/slab.h> necessary?
I removed it and added a check that its header guard is not defined:

+#ifdef _LINUX_SLAB_H
+#error "Included slab.h indirectly"
+#endif

The test still compiled, so afaik, nothing here needs it.

>
> +
> +/* Needed for clk_hw_get_clk() */
> +#include "clk.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_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)
> +{
> +       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_rate_ops = {
> +       .recalc_rate = clk_dummy_recalc_rate,
> +       .determine_rate = clk_dummy_determine_rate,
> +       .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;
> +       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_rate_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;
> +
> +       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);

nit:
  KUNIT_ASSERT_GT(test, rate, 0);

Looks like we updated the others below but forgot this one.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> 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>

Tested-by: Daniel Latypov <dlatypov@google.com>

Looks good to me on the KUnit side.
Two small nits below.

FYI, I computed the incremental coverage for this series, i.e.:
1) applied the full series
2) computed the absolute coverage

$  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
--make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
--kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
$ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6

3) intersected that with the total diff

Incremental coverage for 3/9 files in --diff_file
Total incremental: 99.29% coverage (281/283 lines)
  drivers/clk/clk.c: 84.62% coverage (11/13 lines)
  drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
  include/linux/clk.h: 100.00% coverage (1/1 lines)

Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
+       if (ret) {
+               /* rollback the changes */
+               clk->min_rate = old_min; <- 2397
+               clk->max_rate = old_max; <- 2398

These are from before and were just moved around.

Note: this filters out code that wasn't compiled in and wasn't executable.
So that's why it's missing clk-raspberrypi.c and friends and it says
clk.c had 13 changed lines (since most of the lines are comments).

>
> ---
>  drivers/clk/.kunitconfig |   1 +
>  drivers/clk/Kconfig      |   7 +
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk_test.c   | 786 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 795 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..8f9b1daba411 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..0ca6cd391c8e
> --- /dev/null
> +++ b/drivers/clk/clk_test.c
> @@ -0,0 +1,786 @@
> +// 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>

Very nit:
Is this #include <linux/slab.h> necessary?
I removed it and added a check that its header guard is not defined:

+#ifdef _LINUX_SLAB_H
+#error "Included slab.h indirectly"
+#endif

The test still compiled, so afaik, nothing here needs it.

>
> +
> +/* Needed for clk_hw_get_clk() */
> +#include "clk.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_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)
> +{
> +       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_rate_ops = {
> +       .recalc_rate = clk_dummy_recalc_rate,
> +       .determine_rate = clk_dummy_determine_rate,
> +       .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;
> +       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_rate_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;
> +
> +       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);

nit:
  KUNIT_ASSERT_GT(test, rate, 0);

Looks like we updated the others below but forgot this one.

Re: [PATCH v6 05/12] clk: Use clamp instead of open-coding our own

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-23 02:55:53)
> 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.

s/will//

> 
> Since it's running under the condition that the rate is either lower
> than the minimum, or higher than the 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 6c4e10209568..c15ee5070f52 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2388,11 +2388,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) {

Re: [PATCH v6 05/12] clk: Use clamp instead of open-coding our own

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-23 02:55:53)
> 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.

s/will//

> 
> Since it's running under the condition that the rate is either lower
> than the minimum, or higher than the 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 6c4e10209568..c15ee5070f52 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2388,11 +2388,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) {

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Daniel Latypov (2022-02-23 14:50:59)
> On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > 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>
> 
> Tested-by: Daniel Latypov <dlatypov@google.com>
> 
> Looks good to me on the KUnit side.
> Two small nits below.
> 
> FYI, I computed the incremental coverage for this series, i.e.:
> 1) applied the full series
> 2) computed the absolute coverage
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6

This is cool. Thanks! Is it possible to add some 'coverage' command to
kunit so we don't have to recall this invocation?

> 
> 3) intersected that with the total diff

This would also be cool to do automatically with a revision range.

> 
> Incremental coverage for 3/9 files in --diff_file
> Total incremental: 99.29% coverage (281/283 lines)
>   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
>   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
>   include/linux/clk.h: 100.00% coverage (1/1 lines)
> 
> Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> +       if (ret) {
> +               /* rollback the changes */
> +               clk->min_rate = old_min; <- 2397
> +               clk->max_rate = old_max; <- 2398
> 
> These are from before and were just moved around.

We could trigger a failure in the provider when the rate is set, and
then we could call round_rate() again and make sure the boundaries from
before are maintained.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Daniel Latypov (2022-02-23 14:50:59)
> On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > 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>
> 
> Tested-by: Daniel Latypov <dlatypov@google.com>
> 
> Looks good to me on the KUnit side.
> Two small nits below.
> 
> FYI, I computed the incremental coverage for this series, i.e.:
> 1) applied the full series
> 2) computed the absolute coverage
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6

This is cool. Thanks! Is it possible to add some 'coverage' command to
kunit so we don't have to recall this invocation?

> 
> 3) intersected that with the total diff

This would also be cool to do automatically with a revision range.

> 
> Incremental coverage for 3/9 files in --diff_file
> Total incremental: 99.29% coverage (281/283 lines)
>   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
>   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
>   include/linux/clk.h: 100.00% coverage (1/1 lines)
> 
> Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> +       if (ret) {
> +               /* rollback the changes */
> +               clk->min_rate = old_min; <- 2397
> +               clk->max_rate = old_max; <- 2398
> 
> These are from before and were just moved around.

We could trigger a failure in the provider when the rate is set, and
then we could call round_rate() again and make sure the boundaries from
before are maintained.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Thu, Feb 24, 2022 at 2:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Latypov (2022-02-23 14:50:59)
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > 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>
> >
> > Tested-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks good to me on the KUnit side.
> > Two small nits below.
> >
> > FYI, I computed the incremental coverage for this series, i.e.:
> > 1) applied the full series
> > 2) computed the absolute coverage
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6
>
> This is cool. Thanks! Is it possible to add some 'coverage' command to
> kunit so we don't have to recall this invocation?

This is documented at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
It also includes pointers on how to use lcov to process the .gcda files.
I wrote it before --kconfig_add existed, so it just looks a bit different.

The main blockers to directly supporting this in kunit.py are
1.) this only works on UML
2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
3.) in kernels older than 5.14, this requires some more hacks to get
working. So for the large portion of us stuck dealing with somewhat
older kernels, we'd have to do stuff manually anyway.

For #1, we'd need different kconfig options and kunit.py's QEMU would
need some sort of userspace (busybox should be sufficient).
For #2, I don't recall what the precise issues were anymore. But I
think there were some more issues in gcc 8 or 9... :(

>
> >
> > 3) intersected that with the total diff
>
> This would also be cool to do automatically with a revision range.

Hmm, can you elaborate?
I assume you mean other revision ranges beyond this patch set?

My script roughly looks like

$ incremental_cli --diff_file=a.diff --lcov_file=coverage.info
I can generate a.diff in any way I want.

For these numbers I did
$ git diff clk-next/clk-next > a.diff
But I could do
$ git diff HEAD~3 > a.diff
or anything else.

Just need to make sure the endpoint of the range is the one we
generated coverage at so the line #s match up.

So if you have some specific requests, I can get those generated as well.

I would share my incremental_cli script, but it depends on some
internal code (an LCOV parser).
I don't quite understand how to use lcov's --diff option, but maybe it
supports this?
I just saw "to merge coverage data from different source code levels"
in the man page and figured it wasn't relevant.

>
> >
> > Incremental coverage for 3/9 files in --diff_file
> > Total incremental: 99.29% coverage (281/283 lines)
> >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> >
> > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > +       if (ret) {
> > +               /* rollback the changes */
> > +               clk->min_rate = old_min; <- 2397
> > +               clk->max_rate = old_max; <- 2398
> >
> > These are from before and were just moved around.
>
> We could trigger a failure in the provider when the rate is set, and
> then we could call round_rate() again and make sure the boundaries from
> before are maintained.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Thu, Feb 24, 2022 at 2:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Latypov (2022-02-23 14:50:59)
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > 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>
> >
> > Tested-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks good to me on the KUnit side.
> > Two small nits below.
> >
> > FYI, I computed the incremental coverage for this series, i.e.:
> > 1) applied the full series
> > 2) computed the absolute coverage
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6
>
> This is cool. Thanks! Is it possible to add some 'coverage' command to
> kunit so we don't have to recall this invocation?

This is documented at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
It also includes pointers on how to use lcov to process the .gcda files.
I wrote it before --kconfig_add existed, so it just looks a bit different.

The main blockers to directly supporting this in kunit.py are
1.) this only works on UML
2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
3.) in kernels older than 5.14, this requires some more hacks to get
working. So for the large portion of us stuck dealing with somewhat
older kernels, we'd have to do stuff manually anyway.

For #1, we'd need different kconfig options and kunit.py's QEMU would
need some sort of userspace (busybox should be sufficient).
For #2, I don't recall what the precise issues were anymore. But I
think there were some more issues in gcc 8 or 9... :(

>
> >
> > 3) intersected that with the total diff
>
> This would also be cool to do automatically with a revision range.

Hmm, can you elaborate?
I assume you mean other revision ranges beyond this patch set?

My script roughly looks like

$ incremental_cli --diff_file=a.diff --lcov_file=coverage.info
I can generate a.diff in any way I want.

For these numbers I did
$ git diff clk-next/clk-next > a.diff
But I could do
$ git diff HEAD~3 > a.diff
or anything else.

Just need to make sure the endpoint of the range is the one we
generated coverage at so the line #s match up.

So if you have some specific requests, I can get those generated as well.

I would share my incremental_cli script, but it depends on some
internal code (an LCOV parser).
I don't quite understand how to use lcov's --diff option, but maybe it
supports this?
I just saw "to merge coverage data from different source code levels"
in the man page and figured it wasn't relevant.

>
> >
> > Incremental coverage for 3/9 files in --diff_file
> > Total incremental: 99.29% coverage (281/283 lines)
> >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> >
> > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > +       if (ret) {
> > +               /* rollback the changes */
> > +               clk->min_rate = old_min; <- 2397
> > +               clk->max_rate = old_max; <- 2398
> >
> > These are from before and were just moved around.
>
> We could trigger a failure in the provider when the rate is set, and
> then we could call round_rate() again and make sure the boundaries from
> before are maintained.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Daniel,

On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > 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>
> 
> Tested-by: Daniel Latypov <dlatypov@google.com>
> 
> Looks good to me on the KUnit side.
> Two small nits below.
> 
> FYI, I computed the incremental coverage for this series, i.e.:
> 1) applied the full series
> 2) computed the absolute coverage
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y

I built a docker container based on ubuntu 18.04 to have gcc6 and
python3.7, but this doesn't seem to be working, I'm not entirely sure why:

[13:11:22] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".

Thanks,
Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Daniel,

On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > 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>
> 
> Tested-by: Daniel Latypov <dlatypov@google.com>
> 
> Looks good to me on the KUnit side.
> Two small nits below.
> 
> FYI, I computed the incremental coverage for this series, i.e.:
> 1) applied the full series
> 2) computed the absolute coverage
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y

I built a docker container based on ubuntu 18.04 to have gcc6 and
python3.7, but this doesn't seem to be working, I'm not entirely sure why:

[13:11:22] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".

Thanks,
Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Stephen,

On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> Quoting Daniel Latypov (2022-02-23 14:50:59)
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > Incremental coverage for 3/9 files in --diff_file
> > Total incremental: 99.29% coverage (281/283 lines)
> >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > 
> > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > +       if (ret) {
> > +               /* rollback the changes */
> > +               clk->min_rate = old_min; <- 2397
> > +               clk->max_rate = old_max; <- 2398
> > 
> > These are from before and were just moved around.
> 
> We could trigger a failure in the provider when the rate is set, and
> then we could call round_rate() again and make sure the boundaries from
> before are maintained.

I tried to do that, and it turns out we can't, since we ignore the
set_rate return code:

https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107

We could make determine_rate fail, but then clk_round_rate would fail as
well and wouldn't allow us to test whether the boundaries are still in
place.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Stephen,

On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> Quoting Daniel Latypov (2022-02-23 14:50:59)
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > Incremental coverage for 3/9 files in --diff_file
> > Total incremental: 99.29% coverage (281/283 lines)
> >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > 
> > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > +       if (ret) {
> > +               /* rollback the changes */
> > +               clk->min_rate = old_min; <- 2397
> > +               clk->max_rate = old_max; <- 2398
> > 
> > These are from before and were just moved around.
> 
> We could trigger a failure in the provider when the rate is set, and
> then we could call round_rate() again and make sure the boundaries from
> before are maintained.

I tried to do that, and it turns out we can't, since we ignore the
set_rate return code:

https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107

We could make determine_rate fail, but then clk_round_rate would fail as
well and wouldn't allow us to test whether the boundaries are still in
place.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Daniel,
>
> On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > 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>
> >
> > Tested-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks good to me on the KUnit side.
> > Two small nits below.
> >
> > FYI, I computed the incremental coverage for this series, i.e.:
> > 1) applied the full series
> > 2) computed the absolute coverage
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
>
> I built a docker container based on ubuntu 18.04 to have gcc6 and
> python3.7, but this doesn't seem to be working, I'm not entirely sure why:
>
> [13:11:22] Configuring KUnit Kernel ...
> Regenerating .config ...
> Populating config with:
> $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> This is probably due to unsatisfied dependencies.
> Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".

Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
Need to add 3 config options in total for coverage.

If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
get the error message you get:

$  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
--make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
--kconfig_add=CONFIG_GCOV=y
...
Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
Note: many Kconfig options aren't available on UML. You can try
running on a different architecture with something like
"--arch=x86_64".

>
> Thanks,
> Maxime

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Daniel,
>
> On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > 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>
> >
> > Tested-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks good to me on the KUnit side.
> > Two small nits below.
> >
> > FYI, I computed the incremental coverage for this series, i.e.:
> > 1) applied the full series
> > 2) computed the absolute coverage
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
>
> I built a docker container based on ubuntu 18.04 to have gcc6 and
> python3.7, but this doesn't seem to be working, I'm not entirely sure why:
>
> [13:11:22] Configuring KUnit Kernel ...
> Regenerating .config ...
> Populating config with:
> $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> This is probably due to unsatisfied dependencies.
> Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".

Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
Need to add 3 config options in total for coverage.

If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
get the error message you get:

$  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
--make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
--kconfig_add=CONFIG_GCOV=y
...
Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
Note: many Kconfig options aren't available on UML. You can try
running on a different architecture with something like
"--arch=x86_64".

>
> Thanks,
> Maxime

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-25 06:26:06)
> Hi Stephen,
> 
> On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > Incremental coverage for 3/9 files in --diff_file
> > > Total incremental: 99.29% coverage (281/283 lines)
> > >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> > >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> > >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > > 
> > > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > > +       if (ret) {
> > > +               /* rollback the changes */
> > > +               clk->min_rate = old_min; <- 2397
> > > +               clk->max_rate = old_max; <- 2398
> > > 
> > > These are from before and were just moved around.
> > 
> > We could trigger a failure in the provider when the rate is set, and
> > then we could call round_rate() again and make sure the boundaries from
> > before are maintained.
> 
> I tried to do that, and it turns out we can't, since we ignore the
> set_rate return code:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107
> 
> We could make determine_rate fail, but then clk_round_rate would fail as
> well and wouldn't allow us to test whether the boundaries are still in
> place.
> 

The test could still do it at a high level right? And when/if we decide
to bubble up the set_rate failure then we would be testing these lines.
Seems like a good idea to implement it with a TODO note that clk.c is
ignoring the set_rate clk_op returning failure.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Maxime Ripard (2022-02-25 06:26:06)
> Hi Stephen,
> 
> On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > Incremental coverage for 3/9 files in --diff_file
> > > Total incremental: 99.29% coverage (281/283 lines)
> > >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> > >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> > >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > > 
> > > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > > +       if (ret) {
> > > +               /* rollback the changes */
> > > +               clk->min_rate = old_min; <- 2397
> > > +               clk->max_rate = old_max; <- 2398
> > > 
> > > These are from before and were just moved around.
> > 
> > We could trigger a failure in the provider when the rate is set, and
> > then we could call round_rate() again and make sure the boundaries from
> > before are maintained.
> 
> I tried to do that, and it turns out we can't, since we ignore the
> set_rate return code:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107
> 
> We could make determine_rate fail, but then clk_round_rate would fail as
> well and wouldn't allow us to test whether the boundaries are still in
> place.
> 

The test could still do it at a high level right? And when/if we decide
to bubble up the set_rate failure then we would be testing these lines.
Seems like a good idea to implement it with a TODO note that clk.c is
ignoring the set_rate clk_op returning failure.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Daniel,
> >
> > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > 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>
> > >
> > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > >
> > > Looks good to me on the KUnit side.
> > > Two small nits below.
> > >
> > > FYI, I computed the incremental coverage for this series, i.e.:
> > > 1) applied the full series
> > > 2) computed the absolute coverage
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> >
> > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> >
> > [13:11:22] Configuring KUnit Kernel ...
> > Regenerating .config ...
> > Populating config with:
> > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > This is probably due to unsatisfied dependencies.
> > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> 
> Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> Need to add 3 config options in total for coverage.
> 
> If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> get the error message you get:
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> --kconfig_add=CONFIG_GCOV=y
> ...
> Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> Note: many Kconfig options aren't available on UML. You can try
> running on a different architecture with something like
> "--arch=x86_64".

It looks to me that it's more that DEBUG_INFO isn't enabled.

If I'm running

./tools/testing/kunit/kunit.py config --kunitconfig=drivers/clk
    --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
    --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y

DEBUG_INFO isn't selected and I end up with DEBUG_INFO_NONE.
DEBUG_KERNEL is enabled though.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Daniel,
> >
> > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > 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>
> > >
> > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > >
> > > Looks good to me on the KUnit side.
> > > Two small nits below.
> > >
> > > FYI, I computed the incremental coverage for this series, i.e.:
> > > 1) applied the full series
> > > 2) computed the absolute coverage
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> >
> > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> >
> > [13:11:22] Configuring KUnit Kernel ...
> > Regenerating .config ...
> > Populating config with:
> > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > This is probably due to unsatisfied dependencies.
> > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> 
> Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> Need to add 3 config options in total for coverage.
> 
> If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> get the error message you get:
> 
> $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> --kconfig_add=CONFIG_GCOV=y
> ...
> Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> Note: many Kconfig options aren't available on UML. You can try
> running on a different architecture with something like
> "--arch=x86_64".

It looks to me that it's more that DEBUG_INFO isn't enabled.

If I'm running

./tools/testing/kunit/kunit.py config --kunitconfig=drivers/clk
    --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
    --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y

DEBUG_INFO isn't selected and I end up with DEBUG_INFO_NONE.
DEBUG_KERNEL is enabled though.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

On Fri, Feb 25, 2022 at 02:44:04PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-25 06:26:06)
> > Hi Stephen,
> > 
> > On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> > > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > Incremental coverage for 3/9 files in --diff_file
> > > > Total incremental: 99.29% coverage (281/283 lines)
> > > >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> > > >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> > > >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > > > 
> > > > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > > > +       if (ret) {
> > > > +               /* rollback the changes */
> > > > +               clk->min_rate = old_min; <- 2397
> > > > +               clk->max_rate = old_max; <- 2398
> > > > 
> > > > These are from before and were just moved around.
> > > 
> > > We could trigger a failure in the provider when the rate is set, and
> > > then we could call round_rate() again and make sure the boundaries from
> > > before are maintained.
> > 
> > I tried to do that, and it turns out we can't, since we ignore the
> > set_rate return code:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107
> > 
> > We could make determine_rate fail, but then clk_round_rate would fail as
> > well and wouldn't allow us to test whether the boundaries are still in
> > place.
> > 
> 
> The test could still do it at a high level right? And when/if we decide
> to bubble up the set_rate failure then we would be testing these lines.
> Seems like a good idea to implement it with a TODO note that clk.c is
> ignoring the set_rate clk_op returning failure.

I'm sorry, but I don't get what I need to implement here

The trivial test for this would be to have a driver with set_rate that
returns some error, and then:

KUNIT_ASSERT_LT(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);
KUNIT_EXPECT_LT(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 - 1000);

clk_set_rate_range will never return an error code though, and the
round_rate tests won't work either because the set_rate error was not
propagated, and therefore the boundaries won't be reverted either. So
not only the test will fail, but it will also not increase the coverage.

It's really not clear to me what you expect here. Or we should just
create it but skip it all the time with a FIXME? But then again, it
doesn't increase the coverage, so I'm not sure why it's holding off that
series.

Honestly, I'm getting a bit frustrated by all this. This started as a
small fix, and now we keep moving the goalposts, with more and more
expectations and fixes for things that have nothing related to the
original series. And we're now arguing about gaining a few percent of
code coverage on some code that without this series has a 0% percent
coverage.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

On Fri, Feb 25, 2022 at 02:44:04PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-25 06:26:06)
> > Hi Stephen,
> > 
> > On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> > > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > Incremental coverage for 3/9 files in --diff_file
> > > > Total incremental: 99.29% coverage (281/283 lines)
> > > >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> > > >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> > > >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > > > 
> > > > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > > > +       if (ret) {
> > > > +               /* rollback the changes */
> > > > +               clk->min_rate = old_min; <- 2397
> > > > +               clk->max_rate = old_max; <- 2398
> > > > 
> > > > These are from before and were just moved around.
> > > 
> > > We could trigger a failure in the provider when the rate is set, and
> > > then we could call round_rate() again and make sure the boundaries from
> > > before are maintained.
> > 
> > I tried to do that, and it turns out we can't, since we ignore the
> > set_rate return code:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107
> > 
> > We could make determine_rate fail, but then clk_round_rate would fail as
> > well and wouldn't allow us to test whether the boundaries are still in
> > place.
> > 
> 
> The test could still do it at a high level right? And when/if we decide
> to bubble up the set_rate failure then we would be testing these lines.
> Seems like a good idea to implement it with a TODO note that clk.c is
> ignoring the set_rate clk_op returning failure.

I'm sorry, but I don't get what I need to implement here

The trivial test for this would be to have a driver with set_rate that
returns some error, and then:

KUNIT_ASSERT_LT(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);
KUNIT_EXPECT_LT(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 - 1000);

clk_set_rate_range will never return an error code though, and the
round_rate tests won't work either because the set_rate error was not
propagated, and therefore the boundaries won't be reverted either. So
not only the test will fail, but it will also not increase the coverage.

It's really not clear to me what you expect here. Or we should just
create it but skip it all the time with a FIXME? But then again, it
doesn't increase the coverage, so I'm not sure why it's holding off that
series.

Honestly, I'm getting a bit frustrated by all this. This started as a
small fix, and now we keep moving the goalposts, with more and more
expectations and fixes for things that have nothing related to the
original series. And we're now arguing about gaining a few percent of
code coverage on some code that without this series has a 0% percent
coverage.

Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Daniel Latypov (2022-02-24 15:21:57)
> On Thu, Feb 24, 2022 at 2:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > 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>
> > >
> > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > >
> > > Looks good to me on the KUnit side.
> > > Two small nits below.
> > >
> > > FYI, I computed the incremental coverage for this series, i.e.:
> > > 1) applied the full series
> > > 2) computed the absolute coverage
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6
> >
> > This is cool. Thanks! Is it possible to add some 'coverage' command to
> > kunit so we don't have to recall this invocation?
> 
> This is documented at
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
> It also includes pointers on how to use lcov to process the .gcda files.
> I wrote it before --kconfig_add existed, so it just looks a bit different.
> 
> The main blockers to directly supporting this in kunit.py are
> 1.) this only works on UML
> 2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
> 3.) in kernels older than 5.14, this requires some more hacks to get
> working. So for the large portion of us stuck dealing with somewhat
> older kernels, we'd have to do stuff manually anyway.
> 
> For #1, we'd need different kconfig options and kunit.py's QEMU would
> need some sort of userspace (busybox should be sufficient).
> For #2, I don't recall what the precise issues were anymore. But I
> think there were some more issues in gcc 8 or 9... :(
> 
> >
> > >
> > > 3) intersected that with the total diff
> >
> > This would also be cool to do automatically with a revision range.
> 
> Hmm, can you elaborate?
> I assume you mean other revision ranges beyond this patch set?

I mean somehow to tell kunit.py that I want incremental coverage
information for a git revision range so that I can say something like

	kunit.py incremental HEAD~3..HEAD

and have it tell me the line coverage.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Stephen Boyd]

Quoting Daniel Latypov (2022-02-24 15:21:57)
> On Thu, Feb 24, 2022 at 2:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > 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>
> > >
> > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > >
> > > Looks good to me on the KUnit side.
> > > Two small nits below.
> > >
> > > FYI, I computed the incremental coverage for this series, i.e.:
> > > 1) applied the full series
> > > 2) computed the absolute coverage
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > $ lcov -t "clk_tests" -o coverage.info -c -d .kunit/ --gcov-tool=/usr/bin/gcov-6
> >
> > This is cool. Thanks! Is it possible to add some 'coverage' command to
> > kunit so we don't have to recall this invocation?
> 
> This is documented at
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
> It also includes pointers on how to use lcov to process the .gcda files.
> I wrote it before --kconfig_add existed, so it just looks a bit different.
> 
> The main blockers to directly supporting this in kunit.py are
> 1.) this only works on UML
> 2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
> 3.) in kernels older than 5.14, this requires some more hacks to get
> working. So for the large portion of us stuck dealing with somewhat
> older kernels, we'd have to do stuff manually anyway.
> 
> For #1, we'd need different kconfig options and kunit.py's QEMU would
> need some sort of userspace (busybox should be sufficient).
> For #2, I don't recall what the precise issues were anymore. But I
> think there were some more issues in gcc 8 or 9... :(
> 
> >
> > >
> > > 3) intersected that with the total diff
> >
> > This would also be cool to do automatically with a revision range.
> 
> Hmm, can you elaborate?
> I assume you mean other revision ranges beyond this patch set?

I mean somehow to tell kunit.py that I want incremental coverage
information for a git revision range so that I can say something like

	kunit.py incremental HEAD~3..HEAD

and have it tell me the line coverage.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > 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>
> > > >
> > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > >
> > > > Looks good to me on the KUnit side.
> > > > Two small nits below.
> > > >
> > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > 1) applied the full series
> > > > 2) computed the absolute coverage
> > > >
> > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > >
> > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > >
> > > [13:11:22] Configuring KUnit Kernel ...
> > > Regenerating .config ...
> > > Populating config with:
> > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > This is probably due to unsatisfied dependencies.
> > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >
> > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > Need to add 3 config options in total for coverage.
> >
> > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > get the error message you get:
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > --kconfig_add=CONFIG_GCOV=y
> > ...
> > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > Note: many Kconfig options aren't available on UML. You can try
> > running on a different architecture with something like
> > "--arch=x86_64".
>
> It looks to me that it's more that DEBUG_INFO isn't enabled.

Sorry for the very delayed response.
I was largely getting internet over mobile data around when this email
came in and didn't want to try and download docker images over that.

It looks like that there was another change that is now merged into
Linus' tree that causes this.

I found that adding this helped (thanks David Gow)
  --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT

Running against --kunitconfig=lib/kunit, my final coverage result is

Overall coverage rate:
  lines......: 13.6% (18004 of 132055 lines)
  functions..: 15.7% (1885 of 12010 functions)

Can you give that a shot and see if it works?

Daniel

>
> If I'm running
>
> ./tools/testing/kunit/kunit.py config --kunitconfig=drivers/clk
>     --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
>     --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
>
> DEBUG_INFO isn't selected and I end up with DEBUG_INFO_NONE.
> DEBUG_KERNEL is enabled though.
>
> Maxime

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > 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>
> > > >
> > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > >
> > > > Looks good to me on the KUnit side.
> > > > Two small nits below.
> > > >
> > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > 1) applied the full series
> > > > 2) computed the absolute coverage
> > > >
> > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > >
> > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > >
> > > [13:11:22] Configuring KUnit Kernel ...
> > > Regenerating .config ...
> > > Populating config with:
> > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > This is probably due to unsatisfied dependencies.
> > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >
> > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > Need to add 3 config options in total for coverage.
> >
> > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > get the error message you get:
> >
> > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > --kconfig_add=CONFIG_GCOV=y
> > ...
> > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > Note: many Kconfig options aren't available on UML. You can try
> > running on a different architecture with something like
> > "--arch=x86_64".
>
> It looks to me that it's more that DEBUG_INFO isn't enabled.

Sorry for the very delayed response.
I was largely getting internet over mobile data around when this email
came in and didn't want to try and download docker images over that.

It looks like that there was another change that is now merged into
Linus' tree that causes this.

I found that adding this helped (thanks David Gow)
  --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT

Running against --kunitconfig=lib/kunit, my final coverage result is

Overall coverage rate:
  lines......: 13.6% (18004 of 132055 lines)
  functions..: 15.7% (1885 of 12010 functions)

Can you give that a shot and see if it works?

Daniel

>
> If I'm running
>
> ./tools/testing/kunit/kunit.py config --kunitconfig=drivers/clk
>     --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
>     --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
>
> DEBUG_INFO isn't selected and I end up with DEBUG_INFO_NONE.
> DEBUG_KERNEL is enabled though.
>
> Maxime

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Fri, Mar 25, 2022 at 4:19 PM Stephen Boyd <sboyd@kernel.org> wrote:
>

<snip>

> > >
> > > This is cool. Thanks! Is it possible to add some 'coverage' command to
> > > kunit so we don't have to recall this invocation?
> >
> > This is documented at
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
> > It also includes pointers on how to use lcov to process the .gcda files.
> > I wrote it before --kconfig_add existed, so it just looks a bit different.
> >
> > The main blockers to directly supporting this in kunit.py are
> > 1.) this only works on UML
> > 2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
> > 3.) in kernels older than 5.14, this requires some more hacks to get
> > working. So for the large portion of us stuck dealing with somewhat
> > older kernels, we'd have to do stuff manually anyway.
> >
> > For #1, we'd need different kconfig options and kunit.py's QEMU would
> > need some sort of userspace (busybox should be sufficient).
> > For #2, I don't recall what the precise issues were anymore. But I
> > think there were some more issues in gcc 8 or 9... :(
> >
> > >
> > > >
> > > > 3) intersected that with the total diff
> > >
> > > This would also be cool to do automatically with a revision range.
> >
> > Hmm, can you elaborate?
> > I assume you mean other revision ranges beyond this patch set?
>
> I mean somehow to tell kunit.py that I want incremental coverage
> information for a git revision range so that I can say something like
>
>         kunit.py incremental HEAD~3..HEAD
>
> and have it tell me the line coverage.

Yes, this is doable.

The steps I did were
1. generate coverage.info file per steps above
2. git diff HEAD~ > /tmp/inc.diff
3. <my personal script> --info=coverage.info --diff=/tmp/inc.diff

We'd just change step #2 to be `git diff HEAD~3..HEAD > /tmp/inc.diff`

As mentioned upthread, unfortunately my personal script in step #3
isn't open source or open sourceable atm.
I wrote it using some internal company code for parsing LCOV .info
files out of expediency, but there's nothing too complicated about it.
Just need to find what lines were "added" and intersect that w/ the
coverage data.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Fri, Mar 25, 2022 at 4:19 PM Stephen Boyd <sboyd@kernel.org> wrote:
>

<snip>

> > >
> > > This is cool. Thanks! Is it possible to add some 'coverage' command to
> > > kunit so we don't have to recall this invocation?
> >
> > This is documented at
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml
> > It also includes pointers on how to use lcov to process the .gcda files.
> > I wrote it before --kconfig_add existed, so it just looks a bit different.
> >
> > The main blockers to directly supporting this in kunit.py are
> > 1.) this only works on UML
> > 2.) it needs gcc-6 or lower (and the kernel's min version is 5.1, iirc)...
> > 3.) in kernels older than 5.14, this requires some more hacks to get
> > working. So for the large portion of us stuck dealing with somewhat
> > older kernels, we'd have to do stuff manually anyway.
> >
> > For #1, we'd need different kconfig options and kunit.py's QEMU would
> > need some sort of userspace (busybox should be sufficient).
> > For #2, I don't recall what the precise issues were anymore. But I
> > think there were some more issues in gcc 8 or 9... :(
> >
> > >
> > > >
> > > > 3) intersected that with the total diff
> > >
> > > This would also be cool to do automatically with a revision range.
> >
> > Hmm, can you elaborate?
> > I assume you mean other revision ranges beyond this patch set?
>
> I mean somehow to tell kunit.py that I want incremental coverage
> information for a git revision range so that I can say something like
>
>         kunit.py incremental HEAD~3..HEAD
>
> and have it tell me the line coverage.

Yes, this is doable.

The steps I did were
1. generate coverage.info file per steps above
2. git diff HEAD~ > /tmp/inc.diff
3. <my personal script> --info=coverage.info --diff=/tmp/inc.diff

We'd just change step #2 to be `git diff HEAD~3..HEAD > /tmp/inc.diff`

As mentioned upthread, unfortunately my personal script in step #3
isn't open source or open sourceable atm.
I wrote it using some internal company code for parsing LCOV .info
files out of expediency, but there's nothing too complicated about it.
Just need to find what lines were "added" and intersect that w/ the
coverage data.

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi,

On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > 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>
> > > > >
> > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > >
> > > > > Looks good to me on the KUnit side.
> > > > > Two small nits below.
> > > > >
> > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > 1) applied the full series
> > > > > 2) computed the absolute coverage
> > > > >
> > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > >
> > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > >
> > > > [13:11:22] Configuring KUnit Kernel ...
> > > > Regenerating .config ...
> > > > Populating config with:
> > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > This is probably due to unsatisfied dependencies.
> > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > >
> > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > Need to add 3 config options in total for coverage.
> > >
> > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > get the error message you get:
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > --kconfig_add=CONFIG_GCOV=y
> > > ...
> > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > Note: many Kconfig options aren't available on UML. You can try
> > > running on a different architecture with something like
> > > "--arch=x86_64".
> >
> > It looks to me that it's more that DEBUG_INFO isn't enabled.
> 
> Sorry for the very delayed response.
> I was largely getting internet over mobile data around when this email
> came in and didn't want to try and download docker images over that.
> 
> It looks like that there was another change that is now merged into
> Linus' tree that causes this.
> 
> I found that adding this helped (thanks David Gow)
>   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> 
> Running against --kunitconfig=lib/kunit, my final coverage result is
> 
> Overall coverage rate:
>   lines......: 13.6% (18004 of 132055 lines)
>   functions..: 15.7% (1885 of 12010 functions)
> 
> Can you give that a shot and see if it works?

It does fix the configuration issue, but I'm not able to run the tests either:

[07:53:51] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um olddefconfig O=/home/max/out
[07:53:53] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um olddefconfig O=/home/max/out
Building with:
$ make ARCH=um --jobs=16 O=/home/max/out
[07:54:09] Starting KUnit Kernel (1/1)...
[07:54:09] ============================================================
[07:54:09] [ERROR] Test : invalid KTAP input!
[07:54:09] ============================================================
[07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
[07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running


I've tried to remove all the coverage from the equation, and I get the
same issue if I only run kunit run from inside the container, but it
works fine outside. So I guess it's my setup that is broken. Is there
some way to debug what could be going wrong there?

Thanks!
Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi,

On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > 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>
> > > > >
> > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > >
> > > > > Looks good to me on the KUnit side.
> > > > > Two small nits below.
> > > > >
> > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > 1) applied the full series
> > > > > 2) computed the absolute coverage
> > > > >
> > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > >
> > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > >
> > > > [13:11:22] Configuring KUnit Kernel ...
> > > > Regenerating .config ...
> > > > Populating config with:
> > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > This is probably due to unsatisfied dependencies.
> > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > >
> > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > Need to add 3 config options in total for coverage.
> > >
> > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > get the error message you get:
> > >
> > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > --kconfig_add=CONFIG_GCOV=y
> > > ...
> > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > Note: many Kconfig options aren't available on UML. You can try
> > > running on a different architecture with something like
> > > "--arch=x86_64".
> >
> > It looks to me that it's more that DEBUG_INFO isn't enabled.
> 
> Sorry for the very delayed response.
> I was largely getting internet over mobile data around when this email
> came in and didn't want to try and download docker images over that.
> 
> It looks like that there was another change that is now merged into
> Linus' tree that causes this.
> 
> I found that adding this helped (thanks David Gow)
>   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> 
> Running against --kunitconfig=lib/kunit, my final coverage result is
> 
> Overall coverage rate:
>   lines......: 13.6% (18004 of 132055 lines)
>   functions..: 15.7% (1885 of 12010 functions)
> 
> Can you give that a shot and see if it works?

It does fix the configuration issue, but I'm not able to run the tests either:

[07:53:51] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um olddefconfig O=/home/max/out
[07:53:53] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um olddefconfig O=/home/max/out
Building with:
$ make ARCH=um --jobs=16 O=/home/max/out
[07:54:09] Starting KUnit Kernel (1/1)...
[07:54:09] ============================================================
[07:54:09] [ERROR] Test : invalid KTAP input!
[07:54:09] ============================================================
[07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
[07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running


I've tried to remove all the coverage from the equation, and I get the
same issue if I only run kunit run from inside the container, but it
works fine outside. So I guess it's my setup that is broken. Is there
some way to debug what could be going wrong there?

Thanks!
Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Mon, Mar 28, 2022 at 2:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> > On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > Hi Daniel,
> > > > >
> > > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > >
> > > > > > > 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>
> > > > > >
> > > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > > >
> > > > > > Looks good to me on the KUnit side.
> > > > > > Two small nits below.
> > > > > >
> > > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > > 1) applied the full series
> > > > > > 2) computed the absolute coverage
> > > > > >
> > > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > > >
> > > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > > >
> > > > > [13:11:22] Configuring KUnit Kernel ...
> > > > > Regenerating .config ...
> > > > > Populating config with:
> > > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > > This is probably due to unsatisfied dependencies.
> > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > > >
> > > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > > Need to add 3 config options in total for coverage.
> > > >
> > > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > > get the error message you get:
> > > >
> > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > > --kconfig_add=CONFIG_GCOV=y
> > > > ...
> > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > Note: many Kconfig options aren't available on UML. You can try
> > > > running on a different architecture with something like
> > > > "--arch=x86_64".
> > >
> > > It looks to me that it's more that DEBUG_INFO isn't enabled.
> >
> > Sorry for the very delayed response.
> > I was largely getting internet over mobile data around when this email
> > came in and didn't want to try and download docker images over that.
> >
> > It looks like that there was another change that is now merged into
> > Linus' tree that causes this.
> >
> > I found that adding this helped (thanks David Gow)
> >   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> >
> > Running against --kunitconfig=lib/kunit, my final coverage result is
> >
> > Overall coverage rate:
> >   lines......: 13.6% (18004 of 132055 lines)
> >   functions..: 15.7% (1885 of 12010 functions)
> >
> > Can you give that a shot and see if it works?
>
> It does fix the configuration issue, but I'm not able to run the tests either:
>
> [07:53:51] Configuring KUnit Kernel ...
> Generating .config ...
> Populating config with:
> $ make ARCH=um olddefconfig O=/home/max/out
> [07:53:53] Building KUnit Kernel ...
> Populating config with:
> $ make ARCH=um olddefconfig O=/home/max/out
> Building with:
> $ make ARCH=um --jobs=16 O=/home/max/out
> [07:54:09] Starting KUnit Kernel (1/1)...
> [07:54:09] ============================================================
> [07:54:09] [ERROR] Test : invalid KTAP input!
> [07:54:09] ============================================================
> [07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
> [07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running
>
>
> I've tried to remove all the coverage from the equation, and I get the
> same issue if I only run kunit run from inside the container, but it
> works fine outside. So I guess it's my setup that is broken. Is there
> some way to debug what could be going wrong there?

kunit.py is failing to find any test results from the raw kernel dmesg output.
That is stored in $BUILD_DIR/test.log, so /home/max/out/test.log.
(You can also have kunit.py print this out instead with `kunit.py run
--raw_output`)

It looks like it's specifically not finding the (K)TAP header.

Here's a snippet of what you'd expect to see:
<bunch of boot stuff>
printk: console [mc-1] enabled
TAP version 14
1..9
    # Subtest: clk-test
    1..4
    ok 1 - clk_test_get_rate
    ok 2 - clk_test_set_get_rate
<more kunit output in similar format>

Here's the failing lines in kunit_parser.py
   805  def parse_run_tests(kernel_output: Iterable[str]) -> Test:
   817          lines = extract_tap_lines(kernel_output)   <= lines
after "K?TAP version"
   818          test = Test()
   819          if not lines:
   820                  test.add_error('invalid KTAP input!')  <= this error msg
   821                  test.status = TestStatus.FAILURE_TO_PARSE_TESTS

Normally the issue people run into is that KUnit prints a header but
has no tests.
That's a different error than what you see here.

It seems like we're either not running this func
   206  static void kunit_exec_run_tests(struct suite_set *suite_set)
   207  {
   208          struct kunit_suite * const * const *suites;
   209
   210          kunit_print_tap_header(suite_set); <= not hitting this line?
   211
   212          for (suites = suite_set->start; suites <
suite_set->end; suites++)
   213                  __kunit_test_suites_init(*suites);
   214  }

or maybe the output got mangled somehow?

Daniel

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Daniel Latypov]

On Mon, Mar 28, 2022 at 2:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> > On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > Hi Daniel,
> > > > >
> > > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > >
> > > > > > > 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>
> > > > > >
> > > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > > >
> > > > > > Looks good to me on the KUnit side.
> > > > > > Two small nits below.
> > > > > >
> > > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > > 1) applied the full series
> > > > > > 2) computed the absolute coverage
> > > > > >
> > > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > > >
> > > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > > >
> > > > > [13:11:22] Configuring KUnit Kernel ...
> > > > > Regenerating .config ...
> > > > > Populating config with:
> > > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > > This is probably due to unsatisfied dependencies.
> > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > > >
> > > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > > Need to add 3 config options in total for coverage.
> > > >
> > > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > > get the error message you get:
> > > >
> > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > > --kconfig_add=CONFIG_GCOV=y
> > > > ...
> > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > Note: many Kconfig options aren't available on UML. You can try
> > > > running on a different architecture with something like
> > > > "--arch=x86_64".
> > >
> > > It looks to me that it's more that DEBUG_INFO isn't enabled.
> >
> > Sorry for the very delayed response.
> > I was largely getting internet over mobile data around when this email
> > came in and didn't want to try and download docker images over that.
> >
> > It looks like that there was another change that is now merged into
> > Linus' tree that causes this.
> >
> > I found that adding this helped (thanks David Gow)
> >   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> >
> > Running against --kunitconfig=lib/kunit, my final coverage result is
> >
> > Overall coverage rate:
> >   lines......: 13.6% (18004 of 132055 lines)
> >   functions..: 15.7% (1885 of 12010 functions)
> >
> > Can you give that a shot and see if it works?
>
> It does fix the configuration issue, but I'm not able to run the tests either:
>
> [07:53:51] Configuring KUnit Kernel ...
> Generating .config ...
> Populating config with:
> $ make ARCH=um olddefconfig O=/home/max/out
> [07:53:53] Building KUnit Kernel ...
> Populating config with:
> $ make ARCH=um olddefconfig O=/home/max/out
> Building with:
> $ make ARCH=um --jobs=16 O=/home/max/out
> [07:54:09] Starting KUnit Kernel (1/1)...
> [07:54:09] ============================================================
> [07:54:09] [ERROR] Test : invalid KTAP input!
> [07:54:09] ============================================================
> [07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
> [07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running
>
>
> I've tried to remove all the coverage from the equation, and I get the
> same issue if I only run kunit run from inside the container, but it
> works fine outside. So I guess it's my setup that is broken. Is there
> some way to debug what could be going wrong there?

kunit.py is failing to find any test results from the raw kernel dmesg output.
That is stored in $BUILD_DIR/test.log, so /home/max/out/test.log.
(You can also have kunit.py print this out instead with `kunit.py run
--raw_output`)

It looks like it's specifically not finding the (K)TAP header.

Here's a snippet of what you'd expect to see:
<bunch of boot stuff>
printk: console [mc-1] enabled
TAP version 14
1..9
    # Subtest: clk-test
    1..4
    ok 1 - clk_test_get_rate
    ok 2 - clk_test_set_get_rate
<more kunit output in similar format>

Here's the failing lines in kunit_parser.py
   805  def parse_run_tests(kernel_output: Iterable[str]) -> Test:
   817          lines = extract_tap_lines(kernel_output)   <= lines
after "K?TAP version"
   818          test = Test()
   819          if not lines:
   820                  test.add_error('invalid KTAP input!')  <= this error msg
   821                  test.status = TestStatus.FAILURE_TO_PARSE_TESTS

Normally the issue people run into is that KUnit prints a header but
has no tests.
That's a different error than what you see here.

It seems like we're either not running this func
   206  static void kunit_exec_run_tests(struct suite_set *suite_set)
   207  {
   208          struct kunit_suite * const * const *suites;
   209
   210          kunit_print_tap_header(suite_set); <= not hitting this line?
   211
   212          for (suites = suite_set->start; suites <
suite_set->end; suites++)
   213                  __kunit_test_suites_init(*suites);
   214  }

or maybe the output got mangled somehow?

Daniel

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Daniel,

On Mon, Mar 28, 2022 at 02:36:25PM -0500, Daniel Latypov wrote:
> On Mon, Mar 28, 2022 at 2:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> > > On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > Hi Daniel,
> > > > > >
> > > > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > >
> > > > > > > > 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>
> > > > > > >
> > > > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > > > >
> > > > > > > Looks good to me on the KUnit side.
> > > > > > > Two small nits below.
> > > > > > >
> > > > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > > > 1) applied the full series
> > > > > > > 2) computed the absolute coverage
> > > > > > >
> > > > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > > > >
> > > > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > > > >
> > > > > > [13:11:22] Configuring KUnit Kernel ...
> > > > > > Regenerating .config ...
> > > > > > Populating config with:
> > > > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > > > This is probably due to unsatisfied dependencies.
> > > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > > > >
> > > > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > > > Need to add 3 config options in total for coverage.
> > > > >
> > > > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > > > get the error message you get:
> > > > >
> > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > > > --kconfig_add=CONFIG_GCOV=y
> > > > > ...
> > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > Note: many Kconfig options aren't available on UML. You can try
> > > > > running on a different architecture with something like
> > > > > "--arch=x86_64".
> > > >
> > > > It looks to me that it's more that DEBUG_INFO isn't enabled.
> > >
> > > Sorry for the very delayed response.
> > > I was largely getting internet over mobile data around when this email
> > > came in and didn't want to try and download docker images over that.
> > >
> > > It looks like that there was another change that is now merged into
> > > Linus' tree that causes this.
> > >
> > > I found that adding this helped (thanks David Gow)
> > >   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> > >
> > > Running against --kunitconfig=lib/kunit, my final coverage result is
> > >
> > > Overall coverage rate:
> > >   lines......: 13.6% (18004 of 132055 lines)
> > >   functions..: 15.7% (1885 of 12010 functions)
> > >
> > > Can you give that a shot and see if it works?
> >
> > It does fix the configuration issue, but I'm not able to run the tests either:
> >
> > [07:53:51] Configuring KUnit Kernel ...
> > Generating .config ...
> > Populating config with:
> > $ make ARCH=um olddefconfig O=/home/max/out
> > [07:53:53] Building KUnit Kernel ...
> > Populating config with:
> > $ make ARCH=um olddefconfig O=/home/max/out
> > Building with:
> > $ make ARCH=um --jobs=16 O=/home/max/out
> > [07:54:09] Starting KUnit Kernel (1/1)...
> > [07:54:09] ============================================================
> > [07:54:09] [ERROR] Test : invalid KTAP input!
> > [07:54:09] ============================================================
> > [07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
> > [07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running
> >
> >
> > I've tried to remove all the coverage from the equation, and I get the
> > same issue if I only run kunit run from inside the container, but it
> > works fine outside. So I guess it's my setup that is broken. Is there
> > some way to debug what could be going wrong there?
> 
> kunit.py is failing to find any test results from the raw kernel dmesg output.
> That is stored in $BUILD_DIR/test.log, so /home/max/out/test.log.
> (You can also have kunit.py print this out instead with `kunit.py run
> --raw_output`)

I was missing CAP_SYS_PTRACE in my container, once set it works just fine, thanks!
Maxime

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

Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard]

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

Hi Daniel,

On Mon, Mar 28, 2022 at 02:36:25PM -0500, Daniel Latypov wrote:
> On Mon, Mar 28, 2022 at 2:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote:
> > > On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote:
> > > > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > Hi Daniel,
> > > > > >
> > > > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote:
> > > > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > >
> > > > > > > > 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>
> > > > > > >
> > > > > > > Tested-by: Daniel Latypov <dlatypov@google.com>
> > > > > > >
> > > > > > > Looks good to me on the KUnit side.
> > > > > > > Two small nits below.
> > > > > > >
> > > > > > > FYI, I computed the incremental coverage for this series, i.e.:
> > > > > > > 1) applied the full series
> > > > > > > 2) computed the absolute coverage
> > > > > > >
> > > > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_KERNEL=y
> > > > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y
> > > > > >
> > > > > > I built a docker container based on ubuntu 18.04 to have gcc6 and
> > > > > > python3.7, but this doesn't seem to be working, I'm not entirely sure why:
> > > > > >
> > > > > > [13:11:22] Configuring KUnit Kernel ...
> > > > > > Regenerating .config ...
> > > > > > Populating config with:
> > > > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit
> > > > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> > > > > > This is probably due to unsatisfied dependencies.
> > > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > > Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> > > > >
> > > > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y?
> > > > > Need to add 3 config options in total for coverage.
> > > > >
> > > > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I
> > > > > get the error message you get:
> > > > >
> > > > > $  ./tools/testing/kunit/kunit.py run  --kunitconfig=drivers/clk
> > > > > --make_options=CC=/usr/bin/gcc-6  --kconfig_add=CONFIG_DEBUG_INFO=y
> > > > > --kconfig_add=CONFIG_GCOV=y
> > > > > ...
> > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y
> > > > > Note: many Kconfig options aren't available on UML. You can try
> > > > > running on a different architecture with something like
> > > > > "--arch=x86_64".
> > > >
> > > > It looks to me that it's more that DEBUG_INFO isn't enabled.
> > >
> > > Sorry for the very delayed response.
> > > I was largely getting internet over mobile data around when this email
> > > came in and didn't want to try and download docker images over that.
> > >
> > > It looks like that there was another change that is now merged into
> > > Linus' tree that causes this.
> > >
> > > I found that adding this helped (thanks David Gow)
> > >   --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
> > >
> > > Running against --kunitconfig=lib/kunit, my final coverage result is
> > >
> > > Overall coverage rate:
> > >   lines......: 13.6% (18004 of 132055 lines)
> > >   functions..: 15.7% (1885 of 12010 functions)
> > >
> > > Can you give that a shot and see if it works?
> >
> > It does fix the configuration issue, but I'm not able to run the tests either:
> >
> > [07:53:51] Configuring KUnit Kernel ...
> > Generating .config ...
> > Populating config with:
> > $ make ARCH=um olddefconfig O=/home/max/out
> > [07:53:53] Building KUnit Kernel ...
> > Populating config with:
> > $ make ARCH=um olddefconfig O=/home/max/out
> > Building with:
> > $ make ARCH=um --jobs=16 O=/home/max/out
> > [07:54:09] Starting KUnit Kernel (1/1)...
> > [07:54:09] ============================================================
> > [07:54:09] [ERROR] Test : invalid KTAP input!
> > [07:54:09] ============================================================
> > [07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, Errors: 1
> > [07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s building, 0.003s running
> >
> >
> > I've tried to remove all the coverage from the equation, and I get the
> > same issue if I only run kunit run from inside the container, but it
> > works fine outside. So I guess it's my setup that is broken. Is there
> > some way to debug what could be going wrong there?
> 
> kunit.py is failing to find any test results from the raw kernel dmesg output.
> That is stored in $BUILD_DIR/test.log, so /home/max/out/test.log.
> (You can also have kunit.py print this out instead with `kunit.py run
> --raw_output`)

I was missing CAP_SYS_PTRACE in my container, once set it works just fine, thanks!
Maxime

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