[PATCH v2 00/10] clk: add kunit tests and correct rate change bug in the clk core

[PATCH v2 00/10] clk: add kunit tests and correct rate change bug in the clk core

[Brian Masney]

Here's a series that adds various kunit tests to the clk framework,
documents the members of struct clk_core, and adds a fix that does a
better job of preserving the original clock rate when a sibling clock
changes it's rate, and the shared parent's rate.

These tests are centered around inconsistencies and limitations in the
clock framework that may lead to some clocks unknowingly changing rates
during a rate change of their siblings.

The intent of the clock framework is to keep the siblings clock rate
stable during such an operation:

    clk_set_rate(clk, MY_NEW_RATE);

However, it assumes that the sibling can generate that rate in the first
place. In many situations, it can't, and it leads to numerous bugs and
solutions over the years.

https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/
https://lore.kernel.org/linux-kernel/20230807-pll-mipi_set_rate_parent-v6-0-f173239a4b59@oltmanns.dev/
https://lore.kernel.org/all/20241114065759.3341908-1-victor.liu@nxp.com/
https://lore.kernel.org/linux-clk/20241121-ge-ian-debug-imx8-clk-tree-v1-0-0f1b722588fe@bootlin.com/

We intend to fix these issues, but first we need to agree and document
what these shortcomings are. These patches are meant to do that, even
though some will be skipped as they are currently broken.

Special thanks to Maxime Ripard for the guidance and feedback on this
project so far.

Changes in v2:
- Combine my two v1 patch series referenced below into one.
- Patch 1: Newly introduced: clk: add kernel docs for struct clk_core
- Patch 2: Simplfy further and there is no need to call clk_recalc() for
  parts of the tree that didn't request a rate change.
- Patch 5:
  - Enable all of the tests since this particular limitation is
    addressed in the clk core with patch 2.
  - Update the div_div_3 test to ensure the dividers on the clock
    are automatically updated as expected.
- Patch 7: Correct test description of
  clk_rate_change_sibling_test_suite.
- Links to v1:
  clk: preserve original rate when a sibling clk changes it's rate
  https://lore.kernel.org/lkml/20250520192846.9614-1-bmasney@redhat.com/

  v1: clk: test: add tests for inconsistencies and limitations in the framework
  https://lore.kernel.org/lkml/20250407131258.70638-1-bmasney@redhat.com/

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
Brian Masney (10):
      clk: add kernel docs for struct clk_core
      clk: preserve original rate when a sibling clk changes it's rate
      clk: test: introduce a few specific rate constants for mock testing
      clk: test: introduce clk_dummy_div for a mock divider
      clk: test: introduce test suite for sibling rate changes on a divider
      clk: test: introduce clk_dummy_gate for a mock gate
      clk: test: introduce test suite for sibling rate changes on a gate
      clk: test: introduce helper to create a mock mux
      clk: test: introduce test variation for sibling rate changes on a mux
      clk: test: introduce test variation for sibling rate changes on a gate/mux

 drivers/clk/clk.c      |  57 ++++-
 drivers/clk/clk_test.c | 605 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 573 insertions(+), 89 deletions(-)
---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250528-clk-wip-v2-813333ab1059

Best regards,
-- 
Brian Masney <bmasney@redhat.com>

[PATCH v2 01/10] clk: add kernel docs for struct clk_core

[Brian Masney]

Document all of the members of struct clk_core.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0565c87656cf5c557d8259c71b5d2971a7ac87e8..a130eac9072dc7e71f840a0edf51c368650f8386 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -57,6 +57,48 @@ struct clk_parent_map {
 	int			index;
 };
 
+/**
+ * struct clk_core - This structure represents the internal state of a clk
+ * within the kernel's clock tree. Drivers do not interact with this structure
+ * directly. The clk_core is manipulated by the framework to manage clock
+ * operations, parent/child relationships, rate, and other properties.
+ *
+ * @name:              Unique name of the clk for identification.
+ * @ops:               Pointer to hardware-specific operations for this clk.
+ * @hw:                Pointer for traversing from a struct clk to its
+ *                     corresponding hardware-specific structure.
+ * @owner:             Kernel module owning this clk (for reference counting).
+ * @dev:               Device associated with this clk (optional)
+ * @rpm_node:          Node for runtime power management list management.
+ * @of_node:           Device tree node associated with this clk (if applicable)
+ * @parent:            Pointer to the current parent in the clock tree.
+ * @parents:           Array of possible parents (for muxes/selectable parents).
+ * @num_parents:       Number of possible parents
+ * @new_parent_index:  Index of the new parent during parent change.
+ * @rate:              Current clock rate (Hz).
+ * @req_rate:          Requested clock rate (Hz).
+ * @new_rate:          New rate to be set during a rate change operation.
+ * @new_parent:        Pointer to new parent during parent change.
+ * @new_child:         Pointer to new child during reparenting.
+ * @flags:             Clock property and capability flags.
+ * @orphan:            True if this clk is currently orphaned.
+ * @rpm_enabled:       True if runtime power management is enabled for this clk.
+ * @enable_count:      Reference count of enables.
+ * @prepare_count:     Reference count of prepares.
+ * @protect_count:     Protection reference count against disable.
+ * @min_rate:          Minimum supported clock rate (Hz).
+ * @max_rate:          Maximum supported clock rate (Hz).
+ * @accuracy:          Accuracy of the clock rate (Hz).
+ * @phase:             Current phase (degrees or hardware-specific units).
+ * @duty:              Current duty cycle configuration.
+ * @children:          All of the children of this clk.
+ * @child_node:        Node for linking as a child in the parent's list.
+ * @clks:              All of the clk consumers registered.
+ * @notifier_count:    Number of notifiers registered for this clk.
+ * @dentry:            DebugFS entry for this clk.
+ * @debug_node:        DebugFS node for this clk.
+ * @ref:               Reference count for structure lifetime management.
+ */
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;

-- 
2.49.0

[PATCH v2 02/10] clk: preserve original rate when a sibling clk changes it's rate

[Brian Masney]

There are times when the requested rate on a clk cannot be fulfilled
due to the current rate of the parent clk. If CLK_SET_RATE_PARENT is
set, then the parent rate will be adjusted so that the child can
obtain the requested rate.

When the parent rate is adjusted, there's currently an issue where
the rates of the other children are unnecessarily changed. Let's take
the following simplified clk tree as an example:

                     parent
                     24 MHz
                    /      \
              child1        child2
              24 MHz        24 MHz

The child1 and child2 clks are simple divider clocks in this example,
and it doesn't really matter what kind of clk parent is; for simplicity
we can say that the parent can obtain any rate necessary. Now, let's
update the value of child2 with the existing clk code:

- child2 requests 48 MHz. This is incompatible the current parent rate
  of 24 MHz.
- parent is updated to 48 MHz so that child2 can obtain the requested
  rate of 48 MHz by using a divider of 0.
- child1 should stay at 24 MHz, and the divider should be automatically
  changed from 0 to 1.

The current bug in the code sets the rate of child1 to 48 MHz by calling
the clk_op's recalc_rate() with only the new parent rate, which keeps
the clk's divider at 0. Specifically this example occurs in this part of
the call tree:

clk_core_set_rate_nolock(child2, 48_MHZ)
-> clk_calc_new_rates(child2, 48_MHZ)
  # clk has CLK_SET_RATE_PARENT set, so clk_calc_new_rates() is invoked
  # via the following block:
  # if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
  #     best_parent_rate != parent->rate)
  #      top = clk_calc_new_rates(parent, best_parent_rate);
  -> clk_calc_new_rates(parent, 48_MHZ)
    -> clk_calc_subtree(parent, 48_MHZ, ...)
      -> clk_recalc(child1, 48_MHZ)
         # BOOM! This is where the bug occurs. This invokes the
         # clk_op's recalc_rate() with the new parent rate of 48 MHz,
         # and the original divider of 0 is kept intact, so child1's
         # rate is changed from 24 MHz to 48 MHz by the clk core.

When the clk core requests rate changes, the struct clk_core contains
a new_rate field that contains the rate that the clk is changed to by
clk_change_rate().

When a parent changes it's rate, only ensure that the section of the
clk tree where the rate change request propagated up is changed. All
other sibling nodes should try to keep the same rate (or close to it)
that was previously set. We avoid this by not initially calling the
clk_op's recalc_rate() for parts of the subtree that haven't been
modified.

Once the new_rate fields are populated with the correct values,
eventually clk_change_rate() is called on the parent, and the parent
will invoke clk_change_rate() for all of the children with the expected
rates stored in the new_rate fields. This will invoke the clk_op's
set_rate() on each of the children.

This doesn't fix all of the issues where a clk can unknowingly change
the rate of it's siblings, or put them in an invalid state, however
this is a relatively small change that can fix some issues. A correct
change that includes coordination with the other children in the
subtree, and works across the various types of clks will involve a
much more elaborate patch set.

This change was tested with kunit tests, and also boot tested on a
Lenovo Thinkpad x13s laptop.

Fixes: b2476490ef11 ("clk: introduce the common clock framework")
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a130eac9072dc7e71f840a0edf51c368650f8386..65408899a4ae8674e78494d77ff07fa658f7d3b0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -78,6 +78,10 @@ struct clk_parent_map {
  * @rate:              Current clock rate (Hz).
  * @req_rate:          Requested clock rate (Hz).
  * @new_rate:          New rate to be set during a rate change operation.
+ * @rate_directly_changed: Clocks have the ability to change the rate of it's
+ *                     parent in order to satisfy a rate change request. This
+ *                     flag indicates that the rate change request initiated
+ *                     with this particular node.
  * @new_parent:        Pointer to new parent during parent change.
  * @new_child:         Pointer to new child during reparenting.
  * @flags:             Clock property and capability flags.
@@ -114,6 +118,7 @@ struct clk_core {
 	unsigned long		rate;
 	unsigned long		req_rate;
 	unsigned long		new_rate;
+	bool			rate_directly_changed;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
 	unsigned long		flags;
@@ -2306,7 +2311,11 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 		new_parent->new_child = core;
 
 	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, new_rate);
+		if (child->rate_directly_changed)
+			child->new_rate = clk_recalc(child, new_rate);
+		else
+			child->new_rate = child->rate;
+
 		clk_calc_subtree(child, child->new_rate, NULL, 0);
 	}
 }
@@ -2579,6 +2588,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;
 
+	core->rate_directly_changed = true;
+
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
@@ -2601,6 +2612,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	/* change the rates */
 	clk_change_rate(top);
 
+	core->rate_directly_changed = false;
+
 	core->req_rate = req_rate;
 err:
 	clk_pm_runtime_put(core);

-- 
2.49.0

[PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

[Brian Masney]

Some of the mock tests care about the relationship between two
different rates, and the specific numbers are important, such as for
mocking a divider.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index f08feeaa3750bc86859294650de298762dea690a..1b34d54ec9c610ffa3e91b06f5a5180e0395e26f 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -24,6 +24,10 @@ static const struct clk_ops empty_clk_ops = { };
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
 #define DUMMY_CLOCK_RATE_1	(142 * 1000 * 1000)
 #define DUMMY_CLOCK_RATE_2	(242 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_6_MHZ	(6 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_16_MHZ	(16 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_24_MHZ	(24 * 1000 * 1000)
+#define DUMMY_CLOCK_RATE_48_MHZ	(48 * 1000 * 1000)
 
 struct clk_dummy_context {
 	struct clk_hw hw;

-- 
2.49.0

[PATCH v2 04/10] clk: test: introduce clk_dummy_div for a mock divider

[Brian Masney]

This is used to mock up a divider in the clk kunit tests.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 1b34d54ec9c610ffa3e91b06f5a5180e0395e26f..4908fb9c0c46e34063ecf696e49b48510da44538 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -140,6 +140,47 @@ static const struct clk_ops clk_dummy_single_parent_ops = {
 	.get_parent = clk_dummy_single_get_parent,
 };
 
+// 4 ought to be enough for anybody
+#define CLK_DUMMY_DIV_WIDTH 4
+
+struct clk_dummy_div {
+	struct clk_hw hw;
+	unsigned int div;
+};
+
+static unsigned long clk_dummy_div_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	struct clk_dummy_div *div = container_of(hw, struct clk_dummy_div, hw);
+
+	return divider_recalc_rate(hw, parent_rate, div->div, NULL,
+				   CLK_DIVIDER_ROUND_CLOSEST, CLK_DUMMY_DIV_WIDTH);
+}
+
+static long clk_dummy_div_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *parent_rate)
+{
+	return divider_round_rate(hw, rate, parent_rate, NULL,
+				  CLK_DUMMY_DIV_WIDTH, CLK_DIVIDER_ROUND_CLOSEST);
+}
+
+static int clk_dummy_div_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long parent_rate)
+{
+	struct clk_dummy_div *div = container_of(hw, struct clk_dummy_div, hw);
+
+	div->div = divider_get_val(rate, parent_rate, NULL, CLK_DUMMY_DIV_WIDTH,
+				   CLK_DIVIDER_ROUND_CLOSEST);
+
+	return 0;
+}
+
+static const struct clk_ops clk_dummy_div_ops = {
+	.recalc_rate = clk_dummy_div_recalc_rate,
+	.round_rate = clk_dummy_div_round_rate,
+	.set_rate = clk_dummy_div_set_rate,
+};
+
 struct clk_multiple_parent_ctx {
 	struct clk_dummy_context parents_ctx[2];
 	struct clk_hw hw;

-- 
2.49.0

[PATCH v2 05/10] clk: test: introduce test suite for sibling rate changes on a divider

[Brian Masney]

Introduce a test suite that creates a parent with two divider-only
children, and ensure that changing the rate of one child does not
affect the rate of the sibling.

Some of the tests are disabled until the relevant issue(s) are fixed in
the clk core.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 4908fb9c0c46e34063ecf696e49b48510da44538..35f516fd71a2e33ca19a0512bd2db02111ea644c 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -653,6 +653,144 @@ clk_multiple_parents_mux_test_suite = {
 	.test_cases = clk_multiple_parents_mux_test_cases,
 };
 
+struct clk_rate_change_sibling_div_div_context {
+	struct clk_dummy_context parent;
+	struct clk_dummy_div child1, child2;
+	struct clk *parent_clk, *child1_clk, *child2_clk;
+};
+
+static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
+	ctx->parent.rate = DUMMY_CLOCK_RATE_24_MHZ;
+	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
+	if (ret)
+		return ret;
+
+	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw,
+					     &clk_dummy_div_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
+	if (ret)
+		return ret;
+
+	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw,
+					     &clk_dummy_div_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
+	if (ret)
+		return ret;
+
+	ctx->parent_clk = clk_hw_get_clk(&ctx->parent.hw, NULL);
+	ctx->child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
+	ctx->child2_clk = clk_hw_get_clk(&ctx->child2.hw, NULL);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_24_MHZ);
+
+	return 0;
+}
+
+static void clk_rate_change_sibling_div_div_test_exit(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+
+	clk_put(ctx->parent_clk);
+	clk_put(ctx->child1_clk);
+	clk_put(ctx->child2_clk);
+}
+
+/*
+ * Test that, for a parent with two divider-only children and one requests a
+ * rate compatible with the existing parent rate, the parent and sibling rates
+ * are not affected.
+ */
+static void clk_test_rate_change_sibling_div_div_1(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_6_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_24_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_6_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+}
+
+/*
+ * Test that, for a parent with two divider-only children and one requests a
+ * rate incompatible with the existing parent rate, the sibling rate is not
+ * affected.
+ */
+static void clk_test_rate_change_sibling_div_div_2(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+}
+
+/*
+ * Test that, for a parent with two divider-only children that request rates
+ * incompatible with the existing parent rate, both children end up with the
+ * requested rates. The internal divider for child1 will be changed from 0 to 2.
+ */
+static void clk_test_rate_change_sibling_div_div_3(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	/* Validate initial child rates and divider states. */
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_24_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 0);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 0);
+
+	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_16_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_16_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 2);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 0);
+}
+
+static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
+	KUNIT_CASE(clk_test_rate_change_sibling_div_div_1),
+	KUNIT_CASE(clk_test_rate_change_sibling_div_div_2),
+	KUNIT_CASE(clk_test_rate_change_sibling_div_div_3),
+	{}
+};
+
+/*
+ * Test suite that creates a parent with two divider-only children, and
+ * ensures that changing the rate of one child does not affect the rate
+ * of the other child.
+ */
+static struct kunit_suite clk_rate_change_sibling_div_div_test_suite = {
+	.name = "clk-rate-change-sibling-div-div",
+	.init = clk_rate_change_sibling_div_div_test_init,
+	.exit = clk_rate_change_sibling_div_div_test_exit,
+	.test_cases = clk_rate_change_sibling_div_div_cases,
+};
+
 static int
 clk_orphan_transparent_multiple_parent_mux_test_init(struct kunit *test)
 {
@@ -3445,6 +3583,7 @@ kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,
 	&clk_multiple_parents_mux_test_suite,
+	&clk_rate_change_sibling_div_div_test_suite,
 	&clk_mux_no_reparent_test_suite,
 	&clk_mux_notifier_test_suite,
 	&clk_orphan_transparent_multiple_parent_mux_test_suite,

-- 
2.49.0

[PATCH v2 06/10] clk: test: introduce clk_dummy_gate for a mock gate

[Brian Masney]

This is used to mock up a gate in the clk kunit tests.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 35f516fd71a2e33ca19a0512bd2db02111ea644c..c2337527873d3241e7b0a38f67ecaa13535bcc71 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -181,6 +181,40 @@ static const struct clk_ops clk_dummy_div_ops = {
 	.set_rate = clk_dummy_div_set_rate,
 };
 
+struct clk_dummy_gate {
+	struct clk_hw hw;
+	bool enabled;
+};
+
+static int clk_dummy_gate_enable(struct clk_hw *hw)
+{
+	struct clk_dummy_gate *gate = container_of(hw, struct clk_dummy_gate, hw);
+
+	gate->enabled = true;
+
+	return 0;
+}
+
+static void clk_dummy_gate_disable(struct clk_hw *hw)
+{
+	struct clk_dummy_gate *gate = container_of(hw, struct clk_dummy_gate, hw);
+
+	gate->enabled = false;
+}
+
+static int clk_dummy_gate_is_enabled(struct clk_hw *hw)
+{
+	struct clk_dummy_gate *gate = container_of(hw, struct clk_dummy_gate, hw);
+
+	return gate->enabled;
+}
+
+static const struct clk_ops clk_dummy_gate_ops = {
+	.enable = clk_dummy_gate_enable,
+	.disable = clk_dummy_gate_disable,
+	.is_enabled = clk_dummy_gate_is_enabled,
+};
+
 struct clk_multiple_parent_ctx {
 	struct clk_dummy_context parents_ctx[2];
 	struct clk_hw hw;

-- 
2.49.0

[PATCH v2 07/10] clk: test: introduce test suite for sibling rate changes on a gate

[Brian Masney]

Introduce a test suite that creates a parent with two children: a
divider and a gate. Ensure that changing the rate of one child does
not affect the rate of the gate.

Some of the tests are disabled until the relevant issue(s) are fixed in
the clk core. This is also implemented as a parameterized kunit test
since additional test variations will be added.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index c2337527873d3241e7b0a38f67ecaa13535bcc71..1440eb3c41def8c549f92c0e95b2a472f3bdb4a7 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -825,6 +825,161 @@ static struct kunit_suite clk_rate_change_sibling_div_div_test_suite = {
 	.test_cases = clk_rate_change_sibling_div_div_cases,
 };
 
+struct clk_test_rate_change_sibling_clk_ctx {
+	struct clk *parent_clk, *child1_clk, *child2_clk;
+};
+
+static void
+clk_test_rate_change_sibling_clk_ctx_put(struct clk_test_rate_change_sibling_clk_ctx *clk_ctx)
+{
+	clk_put(clk_ctx->parent_clk);
+	clk_put(clk_ctx->child1_clk);
+	clk_put(clk_ctx->child2_clk);
+}
+
+struct clk_rate_change_sibling_div_gate_sibling_context {
+	struct clk_dummy_context parent;
+	struct clk_dummy_div child1;
+	struct clk_dummy_gate child2;
+	struct clk_test_rate_change_sibling_clk_ctx clk_ctx;
+};
+
+static struct clk_test_rate_change_sibling_clk_ctx *
+clk_rate_change_sibling_div_gate_test_init(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_gate_sibling_context *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+	test->priv = ctx;
+
+	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
+	ctx->parent.rate = DUMMY_CLOCK_RATE_24_MHZ;
+	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw,
+					     &clk_dummy_div_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw,
+					     &clk_dummy_gate_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->clk_ctx.parent_clk = clk_hw_get_clk(&ctx->parent.hw, NULL);
+	ctx->clk_ctx.child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
+	ctx->clk_ctx.child2_clk = clk_hw_get_clk(&ctx->child2.hw, NULL);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->clk_ctx.parent_clk),
+			DUMMY_CLOCK_RATE_24_MHZ);
+
+	return &ctx->clk_ctx;
+}
+
+struct clk_test_rate_change_sibling_test_case {
+	const char *desc;
+	struct clk_test_rate_change_sibling_clk_ctx *(*init)(struct kunit *test);
+};
+
+static struct clk_test_rate_change_sibling_test_case clk_test_rate_change_sibling_test_cases[] = {
+	{
+		.desc = "div_gate",
+		.init = clk_rate_change_sibling_div_gate_test_init,
+	},
+};
+
+KUNIT_ARRAY_PARAM_DESC(clk_test_rate_change_sibling_test_case,
+		       clk_test_rate_change_sibling_test_cases, desc);
+
+/*
+ * Test that, for a parent with two children and one requests a rate change that
+ * requires a change to the parent rate, the sibling rates are not affected.
+ */
+static void clk_test_rate_change_sibling_1(struct kunit *test)
+{
+	struct clk_test_rate_change_sibling_test_case *testcase =
+		(struct clk_test_rate_change_sibling_test_case *) test->param_value;
+	struct clk_test_rate_change_sibling_clk_ctx *ctx;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	ctx = testcase->init(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+
+	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+
+	clk_test_rate_change_sibling_clk_ctx_put(ctx);
+}
+
+/*
+ * Test that, for a parent with two children where one requests an exclusive
+ * rate and the other requests a rate change that requires a change to the
+ * parent rate, the sibling rates are not affected.
+ */
+static void clk_test_rate_change_sibling_2(struct kunit *test)
+{
+	struct clk_test_rate_change_sibling_test_case *testcase =
+		(struct clk_test_rate_change_sibling_test_case *)(test->param_value);
+	struct clk_test_rate_change_sibling_clk_ctx *ctx;
+	int ret;
+
+	ctx = testcase->init(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ret = clk_rate_exclusive_get(ctx->child2_clk);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+
+	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_24_MHZ);
+	/* child1 is rounded to the closest supported rate */
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_24_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
+
+	clk_rate_exclusive_put(ctx->child2_clk);
+
+	clk_test_rate_change_sibling_clk_ctx_put(ctx);
+}
+
+
+static struct kunit_case clk_rate_change_sibling_cases[] = {
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_1,
+			 clk_test_rate_change_sibling_test_case_gen_params),
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_2,
+			 clk_test_rate_change_sibling_test_case_gen_params),
+	{}
+};
+
+/*
+ * Test suite that creates a parent with two children, where the children can
+ * be combinations of a divider, gate, and a mux. Ensure that changing the rate
+ * of one child does affect the rate of the other child.
+ */
+static struct kunit_suite clk_rate_change_sibling_test_suite = {
+	.name = "clk-rate-change-sibling",
+	.test_cases = clk_rate_change_sibling_cases,
+};
+
 static int
 clk_orphan_transparent_multiple_parent_mux_test_init(struct kunit *test)
 {
@@ -3618,6 +3773,7 @@ kunit_test_suites(
 	&clk_test_suite,
 	&clk_multiple_parents_mux_test_suite,
 	&clk_rate_change_sibling_div_div_test_suite,
+	&clk_rate_change_sibling_test_suite,
 	&clk_mux_no_reparent_test_suite,
 	&clk_mux_notifier_test_suite,
 	&clk_orphan_transparent_multiple_parent_mux_test_suite,

-- 
2.49.0

[PATCH v2 08/10] clk: test: introduce helper to create a mock mux

[Brian Masney]

Introduce a helper to create a mock mux to reduce code duplication.
This also changes it so that the relevant clk_hws are registered with
the kunit framework.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 141 ++++++++++++++++++-------------------------------
 1 file changed, 52 insertions(+), 89 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 1440eb3c41def8c549f92c0e95b2a472f3bdb4a7..147935975969f8da4a9365c0fac6ffe37e310933 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -538,45 +538,64 @@ static struct kunit_suite clk_uncached_test_suite = {
 	.test_cases = clk_uncached_test_cases,
 };
 
-static int
-clk_multiple_parents_mux_test_init(struct kunit *test)
-{
-	struct clk_multiple_parent_ctx *ctx;
-	const char *parents[2] = { "parent-0", "parent-1"};
+static int clk_init_multiple_parent_ctx(struct kunit *test,
+					struct clk_multiple_parent_ctx *ctx,
+					const char *parent0_name,
+					unsigned long parent0_rate,
+					const char *parent1_name,
+					unsigned long parent1_rate,
+					const char *mux_name, int mux_flags,
+					const struct clk_ops *mux_ops)
+{
+	const struct clk_hw *parents[2];
 	int ret;
 
-	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
-	test->priv = ctx;
-
-	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
+	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT(parent0_name,
 							    &clk_dummy_rate_ops,
 							    0);
-	ctx->parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
+	ctx->parents_ctx[0].rate = parent0_rate;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->parents_ctx[0].hw);
 	if (ret)
 		return ret;
 
-	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
+	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT(parent1_name,
 							    &clk_dummy_rate_ops,
 							    0);
-	ctx->parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
+	ctx->parents_ctx[1].rate = parent1_rate;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->parents_ctx[1].hw);
 	if (ret)
 		return ret;
 
-	ctx->current_parent = 0;
-	ctx->hw.init = CLK_HW_INIT_PARENTS("test-mux", parents,
-					   &clk_multiple_parents_mux_ops,
-					   CLK_SET_RATE_PARENT);
+	parents[0] = &ctx->parents_ctx[0].hw;
+	parents[1] = &ctx->parents_ctx[1].hw;
+	ctx->hw.init = CLK_HW_INIT_PARENTS_HW(mux_name, parents,
+					      mux_ops, mux_flags);
 	ret = clk_hw_register_kunit(test, NULL, &ctx->hw);
 	if (ret)
 		return ret;
 
+	ctx->current_parent = 0;
+
 	return 0;
 }
 
+static int
+clk_multiple_parents_mux_test_init(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	return clk_init_multiple_parent_ctx(test, ctx,
+					    "parent-0", DUMMY_CLOCK_RATE_1,
+					    "parent-1", DUMMY_CLOCK_RATE_2,
+					    "test-mux", CLK_SET_RATE_PARENT,
+					    &clk_multiple_parents_mux_ops);
+}
+
 /*
  * Test that for a clock with multiple parents, clk_get_parent()
  * actually returns the current one.
@@ -2536,7 +2555,6 @@ static int
 clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 {
 	struct clk_leaf_mux_ctx *ctx;
-	const char *top_parents[2] = { "parent-0", "parent-1" };
 	int ret;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
@@ -2544,27 +2562,11 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 		return -ENOMEM;
 	test->priv = ctx;
 
-	ctx->mux_ctx.parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
-								    &clk_dummy_rate_ops,
-								    0);
-	ctx->mux_ctx.parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[0].hw);
-	if (ret)
-		return ret;
-
-	ctx->mux_ctx.parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
-								    &clk_dummy_rate_ops,
-								    0);
-	ctx->mux_ctx.parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[1].hw);
-	if (ret)
-		return ret;
-
-	ctx->mux_ctx.current_parent = 0;
-	ctx->mux_ctx.hw.init = CLK_HW_INIT_PARENTS("test-mux", top_parents,
-						   &clk_multiple_parents_mux_ops,
-						   0);
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.hw);
+	ret = clk_init_multiple_parent_ctx(test, &ctx->mux_ctx,
+					   "parent-0", DUMMY_CLOCK_RATE_1,
+					   "parent-1", DUMMY_CLOCK_RATE_2,
+					   "test-mux", 0,
+					   &clk_multiple_parents_mux_ops);
 	if (ret)
 		return ret;
 
@@ -2752,7 +2754,6 @@ static int clk_mux_notifier_callback(struct notifier_block *nb,
 static int clk_mux_notifier_test_init(struct kunit *test)
 {
 	struct clk_mux_notifier_ctx *ctx;
-	const char *top_parents[2] = { "parent-0", "parent-1" };
 	int ret;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
@@ -2763,27 +2764,11 @@ static int clk_mux_notifier_test_init(struct kunit *test)
 	init_waitqueue_head(&ctx->pre_rate_change.wq);
 	init_waitqueue_head(&ctx->post_rate_change.wq);
 
-	ctx->mux_ctx.parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
-								    &clk_dummy_rate_ops,
-								    0);
-	ctx->mux_ctx.parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[0].hw);
-	if (ret)
-		return ret;
-
-	ctx->mux_ctx.parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
-								    &clk_dummy_rate_ops,
-								    0);
-	ctx->mux_ctx.parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.parents_ctx[1].hw);
-	if (ret)
-		return ret;
-
-	ctx->mux_ctx.current_parent = 0;
-	ctx->mux_ctx.hw.init = CLK_HW_INIT_PARENTS("test-mux", top_parents,
-						   &clk_multiple_parents_mux_ops,
-						   0);
-	ret = clk_hw_register(NULL, &ctx->mux_ctx.hw);
+	ret = clk_init_multiple_parent_ctx(test, &ctx->mux_ctx,
+					   "parent-0", DUMMY_CLOCK_RATE_1,
+					   "parent-1", DUMMY_CLOCK_RATE_2,
+					   "test-mux", 0,
+					   &clk_multiple_parents_mux_ops);
 	if (ret)
 		return ret;
 
@@ -2866,39 +2851,17 @@ static int
 clk_mux_no_reparent_test_init(struct kunit *test)
 {
 	struct clk_multiple_parent_ctx *ctx;
-	const char *parents[2] = { "parent-0", "parent-1"};
-	int ret;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 	test->priv = ctx;
 
-	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
-							    &clk_dummy_rate_ops,
-							    0);
-	ctx->parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
-	ret = clk_hw_register(NULL, &ctx->parents_ctx[0].hw);
-	if (ret)
-		return ret;
-
-	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
-							    &clk_dummy_rate_ops,
-							    0);
-	ctx->parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
-	ret = clk_hw_register(NULL, &ctx->parents_ctx[1].hw);
-	if (ret)
-		return ret;
-
-	ctx->current_parent = 0;
-	ctx->hw.init = CLK_HW_INIT_PARENTS("test-mux", parents,
-					   &clk_multiple_parents_no_reparent_mux_ops,
-					   0);
-	ret = clk_hw_register(NULL, &ctx->hw);
-	if (ret)
-		return ret;
-
-	return 0;
+	return clk_init_multiple_parent_ctx(test, ctx,
+					    "parent-0", DUMMY_CLOCK_RATE_1,
+					    "parent-1", DUMMY_CLOCK_RATE_2,
+					    "test-mux", 0,
+					    &clk_multiple_parents_no_reparent_mux_ops);
 }
 
 static void

-- 
2.49.0

[PATCH v2 09/10] clk: test: introduce test variation for sibling rate changes on a mux

[Brian Masney]

Introduce a test variation that creates a parent with two children: a
divider and a mux. Ensure that changing the rate of the divider does not
affect the rate of the mux.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 147935975969f8da4a9365c0fac6ffe37e310933..b5cf0de16abd1e098368a67626fff9044f7a1a6a 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -904,6 +904,48 @@ clk_rate_change_sibling_div_gate_test_init(struct kunit *test)
 	return &ctx->clk_ctx;
 }
 
+struct clk_rate_change_sibling_div_mux_sibling_context {
+	struct clk_dummy_div child1;
+	struct clk_multiple_parent_ctx child2_mux;
+	struct clk_test_rate_change_sibling_clk_ctx clk_ctx;
+};
+
+static struct clk_test_rate_change_sibling_clk_ctx *
+clk_rate_change_sibling_div_mux_test_init(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_mux_sibling_context *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+	test->priv = ctx;
+
+	ret = clk_init_multiple_parent_ctx(test, &ctx->child2_mux,
+					   "parent0", DUMMY_CLOCK_RATE_24_MHZ,
+					   "parent1", DUMMY_CLOCK_RATE_48_MHZ,
+					   "child2", CLK_SET_RATE_NO_REPARENT,
+					   &clk_multiple_parents_mux_ops);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->child1.hw.init = CLK_HW_INIT_HW("child1",
+					     &ctx->child2_mux.parents_ctx[0].hw,
+					     &clk_dummy_div_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ctx->clk_ctx.parent_clk = clk_hw_get_clk(&ctx->child2_mux.parents_ctx[0].hw, NULL);
+	ctx->clk_ctx.child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
+	ctx->clk_ctx.child2_clk = clk_hw_get_clk(&ctx->child2_mux.hw, NULL);
+
+	return &ctx->clk_ctx;
+}
+
 struct clk_test_rate_change_sibling_test_case {
 	const char *desc;
 	struct clk_test_rate_change_sibling_clk_ctx *(*init)(struct kunit *test);
@@ -914,6 +956,10 @@ static struct clk_test_rate_change_sibling_test_case clk_test_rate_change_siblin
 		.desc = "div_gate",
 		.init = clk_rate_change_sibling_div_gate_test_init,
 	},
+	{
+		.desc = "div_mux",
+		.init = clk_rate_change_sibling_div_mux_test_init,
+	},
 };
 
 KUNIT_ARRAY_PARAM_DESC(clk_test_rate_change_sibling_test_case,

-- 
2.49.0

[PATCH v2 10/10] clk: test: introduce test variation for sibling rate changes on a gate/mux

[Brian Masney]

Introduce a test variation that creates a parent with two children: a
gate and a mux. Ensure that changing the rate of the gate does not
affect the rate of the mux.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index b5cf0de16abd1e098368a67626fff9044f7a1a6a..c5b856e224f057b011b095d998ca3016b6a041a8 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -946,6 +946,48 @@ clk_rate_change_sibling_div_mux_test_init(struct kunit *test)
 	return &ctx->clk_ctx;
 }
 
+struct clk_rate_change_sibling_gate_mux_sibling_context {
+	struct clk_dummy_gate child1;
+	struct clk_multiple_parent_ctx child2_mux;
+	struct clk_test_rate_change_sibling_clk_ctx clk_ctx;
+};
+
+static struct clk_test_rate_change_sibling_clk_ctx *
+clk_rate_change_sibling_gate_mux_test_init(struct kunit *test)
+{
+	struct clk_rate_change_sibling_gate_mux_sibling_context *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+	test->priv = ctx;
+
+	ret = clk_init_multiple_parent_ctx(test, &ctx->child2_mux,
+					   "parent0", DUMMY_CLOCK_RATE_24_MHZ,
+					   "parent1", DUMMY_CLOCK_RATE_48_MHZ,
+					   "child2", CLK_SET_RATE_NO_REPARENT,
+					   &clk_multiple_parents_mux_ops);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->child1.hw.init = CLK_HW_INIT_HW("child1",
+					     &ctx->child2_mux.parents_ctx[0].hw,
+					     &clk_dummy_gate_ops,
+					     CLK_SET_RATE_PARENT);
+	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ctx->clk_ctx.parent_clk = clk_hw_get_clk(&ctx->child2_mux.parents_ctx[0].hw, NULL);
+	ctx->clk_ctx.child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
+	ctx->clk_ctx.child2_clk = clk_hw_get_clk(&ctx->child2_mux.hw, NULL);
+
+	return &ctx->clk_ctx;
+}
+
 struct clk_test_rate_change_sibling_test_case {
 	const char *desc;
 	struct clk_test_rate_change_sibling_clk_ctx *(*init)(struct kunit *test);
@@ -960,6 +1002,10 @@ static struct clk_test_rate_change_sibling_test_case clk_test_rate_change_siblin
 		.desc = "div_mux",
 		.init = clk_rate_change_sibling_div_mux_test_init,
 	},
+	{
+		.desc = "gate_mux",
+		.init = clk_rate_change_sibling_gate_mux_test_init,
+	},
 };
 
 KUNIT_ARRAY_PARAM_DESC(clk_test_rate_change_sibling_test_case,

-- 
2.49.0

Re: [PATCH v2 06/10] clk: test: introduce clk_dummy_gate for a mock gate

[Maxime Ripard]

On Wed, 28 May 2025 19:16:52 -0400, Brian Masney wrote:
> This is used to mock up a gate in the clk kunit tests.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v2 02/10] clk: preserve original rate when a sibling clk changes it's rate

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:48PM -0400, Brian Masney wrote:
> There are times when the requested rate on a clk cannot be fulfilled
> due to the current rate of the parent clk. If CLK_SET_RATE_PARENT is
> set, then the parent rate will be adjusted so that the child can
> obtain the requested rate.
> 
> When the parent rate is adjusted, there's currently an issue where
> the rates of the other children are unnecessarily changed. Let's take
> the following simplified clk tree as an example:
> 
>                      parent
>                      24 MHz
>                     /      \
>               child1        child2
>               24 MHz        24 MHz
> 
> The child1 and child2 clks are simple divider clocks in this example,
> and it doesn't really matter what kind of clk parent is; for simplicity
> we can say that the parent can obtain any rate necessary. Now, let's
> update the value of child2 with the existing clk code:
> 
> - child2 requests 48 MHz. This is incompatible the current parent rate
>   of 24 MHz.
> - parent is updated to 48 MHz so that child2 can obtain the requested
>   rate of 48 MHz by using a divider of 0.

If we're talking about theory, a divider of 0 doesn't sound great :)

> - child1 should stay at 24 MHz, and the divider should be automatically
>   changed from 0 to 1.
> 
> The current bug in the code sets the rate of child1 to 48 MHz by calling
> the clk_op's recalc_rate() with only the new parent rate, which keeps
> the clk's divider at 0. Specifically this example occurs in this part of
> the call tree:
> 
> clk_core_set_rate_nolock(child2, 48_MHZ)
> -> clk_calc_new_rates(child2, 48_MHZ)
>   # clk has CLK_SET_RATE_PARENT set, so clk_calc_new_rates() is invoked
>   # via the following block:
>   # if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
>   #     best_parent_rate != parent->rate)
>   #      top = clk_calc_new_rates(parent, best_parent_rate);
>   -> clk_calc_new_rates(parent, 48_MHZ)
>     -> clk_calc_subtree(parent, 48_MHZ, ...)
>       -> clk_recalc(child1, 48_MHZ)
>          # BOOM! This is where the bug occurs. This invokes the
>          # clk_op's recalc_rate() with the new parent rate of 48 MHz,
>          # and the original divider of 0 is kept intact, so child1's
>          # rate is changed from 24 MHz to 48 MHz by the clk core.
> 
> When the clk core requests rate changes, the struct clk_core contains
> a new_rate field that contains the rate that the clk is changed to by
> clk_change_rate().

I'm not sure we should frame the problem as "the core calls clk_recalc",
but rather that it doesn't call round_rate or determine_rate on the new
parent rate, so the clock doesn't have the chance to update its
dividers.

If we were doing the latter, calling clk_recalc after would be ok (even
though a bit redundant).

And calling either will cause other challenges, since they can also
reparent or change the parent rate.

I think I'd still mention that it's a temporary solution because we
can't use them yet.

> When a parent changes it's rate, only ensure that the section of the
> clk tree where the rate change request propagated up is changed. All
> other sibling nodes should try to keep the same rate (or close to it)
> that was previously set. We avoid this by not initially calling the
> clk_op's recalc_rate() for parts of the subtree that haven't been
> modified.
> 
> Once the new_rate fields are populated with the correct values,
> eventually clk_change_rate() is called on the parent, and the parent
> will invoke clk_change_rate() for all of the children with the expected
> rates stored in the new_rate fields. This will invoke the clk_op's
> set_rate() on each of the children.
> 
> This doesn't fix all of the issues where a clk can unknowingly change
> the rate of it's siblings, or put them in an invalid state, however
> this is a relatively small change that can fix some issues. A correct
> change that includes coordination with the other children in the
> subtree, and works across the various types of clks will involve a
> much more elaborate patch set.
> 
> This change was tested with kunit tests, and also boot tested on a
> Lenovo Thinkpad x13s laptop.
> 
> Fixes: b2476490ef11 ("clk: introduce the common clock framework")
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a130eac9072dc7e71f840a0edf51c368650f8386..65408899a4ae8674e78494d77ff07fa658f7d3b0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -78,6 +78,10 @@ struct clk_parent_map {
>   * @rate:              Current clock rate (Hz).
>   * @req_rate:          Requested clock rate (Hz).
>   * @new_rate:          New rate to be set during a rate change operation.
> + * @rate_directly_changed: Clocks have the ability to change the rate of it's
> + *                     parent in order to satisfy a rate change request. This
> + *                     flag indicates that the rate change request initiated
> + *                     with this particular node.
>   * @new_parent:        Pointer to new parent during parent change.
>   * @new_child:         Pointer to new child during reparenting.
>   * @flags:             Clock property and capability flags.
> @@ -114,6 +118,7 @@ struct clk_core {
>  	unsigned long		rate;
>  	unsigned long		req_rate;
>  	unsigned long		new_rate;
> +	bool			rate_directly_changed;

clk_core already owns a lot of transient data, and I'm not sure adding
more is a good idea.

What if we were to pass the initiating clock to all these functions, and
test if we are indeed this clock instead?

Maxime

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

Re: [PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:49PM -0400, Brian Masney wrote:
> Some of the mock tests care about the relationship between two
> different rates, and the specific numbers are important, such as for
> mocking a divider.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>

It's not obvious to me why they are important, actually. The relation
between the two is, but a divider (and our tests) should work with any
parent rate, so I guess we can expect it to be opaque.

Can you expand on why it's important?

Maxime

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

Re: [PATCH v2 04/10] clk: test: introduce clk_dummy_div for a mock divider

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:50PM -0400, Brian Masney wrote:
> This is used to mock up a divider in the clk kunit tests.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 1b34d54ec9c610ffa3e91b06f5a5180e0395e26f..4908fb9c0c46e34063ecf696e49b48510da44538 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -140,6 +140,47 @@ static const struct clk_ops clk_dummy_single_parent_ops = {
>  	.get_parent = clk_dummy_single_get_parent,
>  };
>  
> +// 4 ought to be enough for anybody
> +#define CLK_DUMMY_DIV_WIDTH 4
> +
> +struct clk_dummy_div {
> +	struct clk_hw hw;
> +	unsigned int div;
> +};
> +
> +static unsigned long clk_dummy_div_recalc_rate(struct clk_hw *hw,
> +					       unsigned long parent_rate)
> +{
> +	struct clk_dummy_div *div = container_of(hw, struct clk_dummy_div, hw);
> +
> +	return divider_recalc_rate(hw, parent_rate, div->div, NULL,
> +				   CLK_DIVIDER_ROUND_CLOSEST, CLK_DUMMY_DIV_WIDTH);
> +}
> +
> +static long clk_dummy_div_round_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long *parent_rate)
> +{
> +	return divider_round_rate(hw, rate, parent_rate, NULL,
> +				  CLK_DUMMY_DIV_WIDTH, CLK_DIVIDER_ROUND_CLOSEST);
> +}
> +
> +static int clk_dummy_div_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long parent_rate)
> +{
> +	struct clk_dummy_div *div = container_of(hw, struct clk_dummy_div, hw);
> +
> +	div->div = divider_get_val(rate, parent_rate, NULL, CLK_DUMMY_DIV_WIDTH,
> +				   CLK_DIVIDER_ROUND_CLOSEST);
> +
> +	return 0;
> +}

I wonder if we should use CLK_DIVIDER_ONE_BASED too, that way we would
catch any improperly initialized structure.

Maxime

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

Re: [PATCH v2 01/10] clk: add kernel docs for struct clk_core

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:47PM -0400, Brian Masney wrote:
> Document all of the members of struct clk_core.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0565c87656cf5c557d8259c71b5d2971a7ac87e8..a130eac9072dc7e71f840a0edf51c368650f8386 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -57,6 +57,48 @@ struct clk_parent_map {
>  	int			index;
>  };
>  
> +/**
> + * struct clk_core - This structure represents the internal state of a clk
> + * within the kernel's clock tree. Drivers do not interact with this structure
> + * directly. The clk_core is manipulated by the framework to manage clock
> + * operations, parent/child relationships, rate, and other properties.
> + *
> + * @name:              Unique name of the clk for identification.
> + * @ops:               Pointer to hardware-specific operations for this clk.
> + * @hw:                Pointer for traversing from a struct clk to its
> + *                     corresponding hardware-specific structure.
> + * @owner:             Kernel module owning this clk (for reference counting).
> + * @dev:               Device associated with this clk (optional)
> + * @rpm_node:          Node for runtime power management list management.
> + * @of_node:           Device tree node associated with this clk (if applicable)
> + * @parent:            Pointer to the current parent in the clock tree.
> + * @parents:           Array of possible parents (for muxes/selectable parents).
> + * @num_parents:       Number of possible parents
> + * @new_parent_index:  Index of the new parent during parent change.
> + * @rate:              Current clock rate (Hz).

I think we should define what current means here. clk_core->rate is
effectively a cached value of what the hardware has been programmed with.

It's initialized by reading the value at boot time, and will be updated
every time an operation affecting the rate will be performed.

Clocks the CLK_GET_RATE_NOCACHE flag however should not use this value,
as its rate is expected to change behind the kernel's back (because the
firmware might change it, for example).

Also, if the clock is orphan, it's set to 0 and updated when (and if)
its parent is later loaded, so its content is only ever valid if
clk_core->orphan is false.

> + * @req_rate:          Requested clock rate (Hz).

and clk_core->req_rate is the last rate requested by a call to
clk_set_rate. It's initialized to clk_core->rate. It's also updated to
clk_core->rate every time the clock is reparented, and when we're doing
the orphan -> !orphan transition.

> + * @new_rate:          New rate to be set during a rate change operation.
> + * @new_parent:        Pointer to new parent during parent change.
> + * @new_child:         Pointer to new child during reparenting.
> + * @flags:             Clock property and capability flags.
> + * @orphan:            True if this clk is currently orphaned.
> + * @rpm_enabled:       True if runtime power management is enabled for this clk.
> + * @enable_count:      Reference count of enables.
> + * @prepare_count:     Reference count of prepares.
> + * @protect_count:     Protection reference count against disable.
> + * @min_rate:          Minimum supported clock rate (Hz).
> + * @max_rate:          Maximum supported clock rate (Hz).
> + * @accuracy:          Accuracy of the clock rate (Hz).

The unit is parts per billion

> + * @phase:             Current phase (degrees or hardware-specific units).

It's degrees, not hardware specific units.

Maxime

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

Re: [PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

[Brian Masney]

On Fri, Jun 06, 2025 at 10:56:57AM +0200, Maxime Ripard wrote:
> On Wed, May 28, 2025 at 07:16:49PM -0400, Brian Masney wrote:
> > Some of the mock tests care about the relationship between two
> > different rates, and the specific numbers are important, such as for
> > mocking a divider.
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> 
> It's not obvious to me why they are important, actually. The relation
> between the two is, but a divider (and our tests) should work with any
> parent rate, so I guess we can expect it to be opaque.

I agree as well.

> Can you expand on why it's important?

I personally find that having specific numbers in some (but not) of the
tests make the tests clearer that specific functionality within the clk
core is exercised. For example, assume we have a parent that can do any
rate, and two children that are dividers. We could have a test like the
following:

    clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_1);
    clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_2);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_1);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_2);
    /*
     * Make something to figure out what the ideal parent rate should be
     * and test that as well?
     */

So if we set child1 and child2 to 16 MHz and 32 MHz, then that exercises
one path through the clk core. However, it will currently fail if we set
the children to 32 MHz and 48 MHz. I have this working on a WIP branch
and one of my new tests looks similar to:

    clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_32_MHZ);
    clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_48_MHZ);
    // This should test that it's a multiple of 96 MHz
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_96_MHZ);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_32_MHZ);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_48_MHZ);

Based on the work in my WIP branch, I think we need to make some of the
divider only clk tests parameterized, and have a table with various
specific frequencies so that various edge cases within the clk core are
tested by the frequency combinations.

I think that instead of having a list of DUMMY_CLOCK_RATE_XXX_MHZ
defines, a single define like this will suffice:

#define clk_dummy_rate_mhz(rate)      ((rate) * 1000 * 1000)

Brian

Re: [PATCH v2 05/10] clk: test: introduce test suite for sibling rate changes on a divider

[Maxime Ripard]

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

Hi,

On Wed, May 28, 2025 at 07:16:51PM -0400, Brian Masney wrote:
> Introduce a test suite that creates a parent with two divider-only
> children, and ensure that changing the rate of one child does not
> affect the rate of the sibling.
> 
> Some of the tests are disabled until the relevant issue(s) are fixed in
> the clk core.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 139 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 4908fb9c0c46e34063ecf696e49b48510da44538..35f516fd71a2e33ca19a0512bd2db02111ea644c 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -653,6 +653,144 @@ clk_multiple_parents_mux_test_suite = {
>  	.test_cases = clk_multiple_parents_mux_test_cases,
>  };
>  
> +struct clk_rate_change_sibling_div_div_context {
> +	struct clk_dummy_context parent;
> +	struct clk_dummy_div child1, child2;
> +	struct clk *parent_clk, *child1_clk, *child2_clk;
> +};
> +
> +static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> +{
> +	struct clk_rate_change_sibling_div_div_context *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	test->priv = ctx;
> +
> +	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
> +	ctx->parent.rate = DUMMY_CLOCK_RATE_24_MHZ;
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
> +	if (ret)
> +		return ret;
> +
> +	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw,
> +					     &clk_dummy_div_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
> +	if (ret)
> +		return ret;
> +
> +	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw,
> +					     &clk_dummy_div_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
> +	if (ret)
> +		return ret;
> +
> +	ctx->parent_clk = clk_hw_get_clk(&ctx->parent.hw, NULL);
> +	ctx->child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
> +	ctx->child2_clk = clk_hw_get_clk(&ctx->child2.hw, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_24_MHZ);
> +
> +	return 0;
> +}

We should probably document that we're using CLK_SET_RATE_PARENT on the
dividers, because it'll affect the outcome of the tests.

The rest looks good to me, but is still dependant on some earlier
discussions being solved.

Maxime

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

Re: [PATCH v2 07/10] clk: test: introduce test suite for sibling rate changes on a gate

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:53PM -0400, Brian Masney wrote:
> Introduce a test suite that creates a parent with two children: a
> divider and a gate. Ensure that changing the rate of one child does
> not affect the rate of the gate.
> 
> Some of the tests are disabled until the relevant issue(s) are fixed in
> the clk core. This is also implemented as a parameterized kunit test
> since additional test variations will be added.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 156 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index c2337527873d3241e7b0a38f67ecaa13535bcc71..1440eb3c41def8c549f92c0e95b2a472f3bdb4a7 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -825,6 +825,161 @@ static struct kunit_suite clk_rate_change_sibling_div_div_test_suite = {
>  	.test_cases = clk_rate_change_sibling_div_div_cases,
>  };
>  
> +struct clk_test_rate_change_sibling_clk_ctx {
> +	struct clk *parent_clk, *child1_clk, *child2_clk;
> +};
> +
> +static void
> +clk_test_rate_change_sibling_clk_ctx_put(struct clk_test_rate_change_sibling_clk_ctx *clk_ctx)
> +{
> +	clk_put(clk_ctx->parent_clk);
> +	clk_put(clk_ctx->child1_clk);
> +	clk_put(clk_ctx->child2_clk);
> +}
> +
> +struct clk_rate_change_sibling_div_gate_sibling_context {
> +	struct clk_dummy_context parent;
> +	struct clk_dummy_div child1;
> +	struct clk_dummy_gate child2;
> +	struct clk_test_rate_change_sibling_clk_ctx clk_ctx;
> +};
> +
> +static struct clk_test_rate_change_sibling_clk_ctx *
> +clk_rate_change_sibling_div_gate_test_init(struct kunit *test)
> +{
> +	struct clk_rate_change_sibling_div_gate_sibling_context *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return ERR_PTR(-ENOMEM);
> +	test->priv = ctx;
> +
> +	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
> +	ctx->parent.rate = DUMMY_CLOCK_RATE_24_MHZ;
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw,
> +					     &clk_dummy_div_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw,
> +					     &clk_dummy_gate_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->clk_ctx.parent_clk = clk_hw_get_clk(&ctx->parent.hw, NULL);
> +	ctx->clk_ctx.child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
> +	ctx->clk_ctx.child2_clk = clk_hw_get_clk(&ctx->child2.hw, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->clk_ctx.parent_clk),
> +			DUMMY_CLOCK_RATE_24_MHZ);

EXPECT is for the expected output of the test. It looks to me that
you're are here checking if the test is properly setup, which would be
an assertion.

> +	return &ctx->clk_ctx;
> +}
> +
> +struct clk_test_rate_change_sibling_test_case {
> +	const char *desc;
> +	struct clk_test_rate_change_sibling_clk_ctx *(*init)(struct kunit *test);
> +};
> +
> +static struct clk_test_rate_change_sibling_test_case clk_test_rate_change_sibling_test_cases[] = {
> +	{
> +		.desc = "div_gate",
> +		.init = clk_rate_change_sibling_div_gate_test_init,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(clk_test_rate_change_sibling_test_case,
> +		       clk_test_rate_change_sibling_test_cases, desc);

I'm not sure making them parameterized is a good idea (yet), I tend to
think that the more straightforward the tests are the better. That can
indeed lead to repetitions, but it's also much easier to debug once we
get a test failure.

> +
> +/*
> + * Test that, for a parent with two children and one requests a rate change that
> + * requires a change to the parent rate, the sibling rates are not affected.
> + */
> +static void clk_test_rate_change_sibling_1(struct kunit *test)
> +{
> +	struct clk_test_rate_change_sibling_test_case *testcase =
> +		(struct clk_test_rate_change_sibling_test_case *) test->param_value;
> +	struct clk_test_rate_change_sibling_clk_ctx *ctx;
> +	int ret;
> +
> +	kunit_skip(test, "This needs to be fixed in the core.");
> +
> +	ctx = testcase->init(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
> +
> +	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);

And also, we wouldn't have the same expectations between a gate like
here, and a mux (that can reparent), so sharing the code isn't going to
be trivial.

> +	clk_test_rate_change_sibling_clk_ctx_put(ctx);

This won't be run if you hit any KUNIT_ASSERT_*() conditions. We should
probably create a kunit-managed clk_hw_get() variant so we don't have to
deal with this.

Maxime

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

Re: [PATCH v2 08/10] clk: test: introduce helper to create a mock mux

[Maxime Ripard]

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

On Wed, May 28, 2025 at 07:16:54PM -0400, Brian Masney wrote:
> Introduce a helper to create a mock mux to reduce code duplication.
> This also changes it so that the relevant clk_hws are registered with
> the kunit framework.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 141 ++++++++++++++++++-------------------------------
>  1 file changed, 52 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 1440eb3c41def8c549f92c0e95b2a472f3bdb4a7..147935975969f8da4a9365c0fac6ffe37e310933 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -538,45 +538,64 @@ static struct kunit_suite clk_uncached_test_suite = {
>  	.test_cases = clk_uncached_test_cases,
>  };
>  
> -static int
> -clk_multiple_parents_mux_test_init(struct kunit *test)
> -{
> -	struct clk_multiple_parent_ctx *ctx;
> -	const char *parents[2] = { "parent-0", "parent-1"};
> +static int clk_init_multiple_parent_ctx(struct kunit *test,
> +					struct clk_multiple_parent_ctx *ctx,
> +					const char *parent0_name,
> +					unsigned long parent0_rate,
> +					const char *parent1_name,
> +					unsigned long parent1_rate,
> +					const char *mux_name, int mux_flags,
> +					const struct clk_ops *mux_ops)
> +{
> +	const struct clk_hw *parents[2];
>  	int ret;
>  
> -	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> -	if (!ctx)
> -		return -ENOMEM;
> -	test->priv = ctx;
> -
> -	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
> +	ctx->parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT(parent0_name,
>  							    &clk_dummy_rate_ops,
>  							    0);
> -	ctx->parents_ctx[0].rate = DUMMY_CLOCK_RATE_1;
> +	ctx->parents_ctx[0].rate = parent0_rate;
>  	ret = clk_hw_register_kunit(test, NULL, &ctx->parents_ctx[0].hw);
>  	if (ret)
>  		return ret;
>  
> -	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT("parent-1",
> +	ctx->parents_ctx[1].hw.init = CLK_HW_INIT_NO_PARENT(parent1_name,
>  							    &clk_dummy_rate_ops,
>  							    0);
> -	ctx->parents_ctx[1].rate = DUMMY_CLOCK_RATE_2;
> +	ctx->parents_ctx[1].rate = parent1_rate;
>  	ret = clk_hw_register_kunit(test, NULL, &ctx->parents_ctx[1].hw);
>  	if (ret)
>  		return ret;
>  
> -	ctx->current_parent = 0;
> -	ctx->hw.init = CLK_HW_INIT_PARENTS("test-mux", parents,
> -					   &clk_multiple_parents_mux_ops,
> -					   CLK_SET_RATE_PARENT);
> +	parents[0] = &ctx->parents_ctx[0].hw;
> +	parents[1] = &ctx->parents_ctx[1].hw;
> +	ctx->hw.init = CLK_HW_INIT_PARENTS_HW(mux_name, parents,
> +					      mux_ops, mux_flags);
>  	ret = clk_hw_register_kunit(test, NULL, &ctx->hw);
>  	if (ret)
>  		return ret;
>  
> +	ctx->current_parent = 0;
> +
>  	return 0;
>  }

In this patch too, I'm wondering if we're not making it more complex
than it needs to be. I can see how small variations (like the parent
flags, or ops, or...) will all sound reasonable, but will turn this
allegedly simple function into a large, hard-to-parse, one

Maxime

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

Re: [PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

[Maxime Ripard]

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

On Fri, Jun 06, 2025 at 10:28:19AM -0400, Brian Masney wrote:
> On Fri, Jun 06, 2025 at 10:56:57AM +0200, Maxime Ripard wrote:
> > On Wed, May 28, 2025 at 07:16:49PM -0400, Brian Masney wrote:
> > > Some of the mock tests care about the relationship between two
> > > different rates, and the specific numbers are important, such as for
> > > mocking a divider.
> > > 
> > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > 
> > It's not obvious to me why they are important, actually. The relation
> > between the two is, but a divider (and our tests) should work with any
> > parent rate, so I guess we can expect it to be opaque.
> 
> I agree as well.
> 
> > Can you expand on why it's important?
> 
> I personally find that having specific numbers in some (but not) of the
> tests make the tests clearer that specific functionality within the clk
> core is exercised. For example, assume we have a parent that can do any
> rate, and two children that are dividers. We could have a test like the
> following:
> 
>     clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_1);
>     clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_2);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_1);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_2);
>     /*
>      * Make something to figure out what the ideal parent rate should be
>      * and test that as well?
>      */
> 
> So if we set child1 and child2 to 16 MHz and 32 MHz, then that exercises
> one path through the clk core. However, it will currently fail if we set
> the children to 32 MHz and 48 MHz. I have this working on a WIP branch
> and one of my new tests looks similar to:
> 
>     clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_32_MHZ);
>     clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_48_MHZ);
>     // This should test that it's a multiple of 96 MHz
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_96_MHZ);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_32_MHZ);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_48_MHZ);
> 
> Based on the work in my WIP branch, I think we need to make some of the
> divider only clk tests parameterized, and have a table with various
> specific frequencies so that various edge cases within the clk core are
> tested by the frequency combinations.
> 
> I think that instead of having a list of DUMMY_CLOCK_RATE_XXX_MHZ
> defines, a single define like this will suffice:
> 
> #define clk_dummy_rate_mhz(rate)      ((rate) * 1000 * 1000)

So, my main worry is that I'm concerned that some tests will only pass
because (or thanks to) the rates we've chosen, and not because they are
actually passing. Kind of like what happened with your earlier patch to
change the rate clk_recalc was called with.

I have the feeling our tests should have caught it, and maybe it's also
because we're missing some coverage, but didn't because we picked those
particular rates it worked by accident.

Maybe we should start using parent_rate / X in our assertions instead of
defines, or come up with better assertions?

Maxime

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