[PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Brian Masney]

The Common Clock Framework is expected to keep a clock’s rate stable
after setting a new rate with:

    clk_set_rate(clk, NEW_RATE);

Clock consumers do not know about the clock hierarchy, sibling clocks,
or the type of clocks involved. However, several longstanding issues
affect how rate changes propagate through the clock tree when
CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:

- A clock in some cases can unknowingly change a sibling clock's rate.
  More details about this particular case are documented at:
  https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/

- No negotiation is done with the sibling clocks, so an inappropriate
  or less than ideal parent rate can be selected.

A selection of some real world examples of where this shows up is at
[1]. DRM needs to run at precise clock rates, and this issue shows up
there, however will also show up in other subsystems that require
precise clock rates, such as sound.

An unknown subset of existing boards are unknowingly dependent on the
existing behavior, so it's risky to change the way the rate negotiation
logic is done in the clk core.

This series adds support for v1 and v2 rate negotiation logic to the clk
core. When a child determines that a parent rate change needs to occur
when the v2 logic is used, the parent negotiates with all nodes in that
part of the clk subtree and picks the first rate that's acceptable to
all nodes.

Kunit tests are introduced to illustrate the problem, and are updated
later in the series to illustrate that the v2 negotiation logic works
as expected, while keeping compatibility with v1.

I marked this as a RFC since Stephen asked me in a video call to not
add a new member to struct clk_core, however I don't see how to do this
any other way.

- The clk core doesn’t, and shouldn’t, know about the internal state the
  various clk providers.
- Child clks shouldn’t have to know the internal state of the parent clks.
- Currently this information is not exposed in any way to the clk core.

Changes since v3:
https://lore.kernel.org/r/20250812-clk-tests-docs-v3-0-054aed58dcd3@redhat.com
- Update clk_core struct members (Maxime)
- Add v2 rate negotiation logic and additional kunit tests
- Drop clk_dummy_rate_mhz() in kunit tests; use HZ_PER_MHZ

[1] 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/

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
Brian Masney (12):
      clk: add kernel docs for struct clk_core
      clk: test: convert constants to use HZ_PER_MHZ
      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
      clk: add support for v2 rate negotiation
      clk: test: introduce negotiate_rates() op for clk_dummy and clk_dummy_div
      clk: test: update divider kunit tests for v1 and v2 rate negotiation

 Documentation/admin-guide/kernel-parameters.txt |  15 +
 Documentation/driver-api/clk.rst                |   3 +
 drivers/clk/clk.c                               | 201 ++++++-
 drivers/clk/clk_test.c                          | 694 ++++++++++++++++++++----
 include/linux/clk-provider.h                    |   7 +
 include/linux/clk.h                             |  20 +
 6 files changed, 835 insertions(+), 105 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250806-clk-tests-docs-3c398759e998

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

[PATCH RFC v4 01/12] clk: add kernel docs for struct clk_core

[Brian Masney]

Document all of the members of struct clk_core.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..018dd5a32ecbf166718da3eda851f51fdfdd2088 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -57,6 +57,64 @@ 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. This is
+ *                     also used when a clk's rate is changed.
+ * @rate:              Current clock rate (Hz). This 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 affects the rate.
+ *                     Clocks with the CLK_GET_RATE_NOCACHE flag 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:          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. This is also
+ *                     used when a clk's rate is changed.
+ * @new_child:         Pointer to new child during reparenting. This is also
+ *                     used when a clk's rate is changed.
+ * @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 (parts per billion).
+ * @phase:             Current phase (degrees).
+ * @duty:              Current duty cycle configuration (percent).
+ * @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.51.0

[PATCH RFC v4 02/12] clk: test: convert constants to use HZ_PER_MHZ

[Brian Masney]

Convert the DUMMY_CLOCK_* constants over to use HZ_PER_MHZ.

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

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a268d7b5d4cb28ec1f029f828c31107f8e130556..372dd289a7ba148a0725ea0643342ccda7196216 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -7,6 +7,7 @@
 #include <linux/clk/clk-conf.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/units.h>
 
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
@@ -21,9 +22,9 @@
 
 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_INIT_RATE		(42 * HZ_PER_MHZ)
+#define DUMMY_CLOCK_RATE_1		(142 * HZ_PER_MHZ)
+#define DUMMY_CLOCK_RATE_2		(242 * HZ_PER_MHZ)
 
 struct clk_dummy_context {
 	struct clk_hw hw;

-- 
2.51.0

[PATCH RFC v4 03/12] 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 372dd289a7ba148a0725ea0643342ccda7196216..e1f72fcede1df1d486744b171728231ec8fa8836 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -137,6 +137,50 @@ 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
+#define CLK_DUMMY_DIV_FLAGS (CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST)
+
+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_DUMMY_DIV_FLAGS, CLK_DUMMY_DIV_WIDTH);
+}
+
+static int clk_dummy_div_determine_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && req->best_parent_rate < req->rate)
+		return -EINVAL;
+
+	return divider_determine_rate(hw, req, NULL, CLK_DUMMY_DIV_WIDTH, CLK_DUMMY_DIV_FLAGS);
+}
+
+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_DUMMY_DIV_FLAGS);
+
+	return 0;
+}
+
+static const struct clk_ops clk_dummy_div_ops = {
+	.recalc_rate = clk_dummy_div_recalc_rate,
+	.determine_rate = clk_dummy_div_determine_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.51.0

[PATCH RFC v4 04/12] 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 underlying issue is fixed in the clk core.

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

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index e1f72fcede1df1d486744b171728231ec8fa8836..1aca266f9922beb7d81124c07d21b2a3d700dc5c 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -653,6 +653,150 @@ 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 = 24 * HZ_PER_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);
+	ctx->child1.div = 1;
+	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);
+	ctx->child2.div = 1;
+	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), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_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 with CLK_SET_RATE_PARENT set
+ * 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, 6 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 6 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 4);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
+}
+
+/*
+ * Test that, for a parent with two divider-only children with CLK_SET_RATE_PARENT
+ * set and one requests a rate incompatible with the existing parent rate, the
+ * sibling rate is not affected. The requested child rate picks a parent rate
+ * that's compatible with both children.
+ */
+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;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	ret = clk_set_rate(ctx->child1_clk, 48 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 48 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 48 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 1);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+}
+
+/*
+ * Test that, for a parent with two divider-only children with CLK_SET_RATE_PARENT
+ * set and one requests a rate incompatible with the existing parent rate, the
+ * sibling rate is not affected. The requested child rates require a parent rate
+ * that neither child would initially pick.
+ */
+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;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_rate(ctx->child2_clk, 48 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 96 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 32 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 3);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 48 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+}
+
+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)
 {
@@ -3593,6 +3737,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.51.0

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

Reviewed-by: Maxime Ripard <mripard@kernel.org>
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 1aca266f9922beb7d81124c07d21b2a3d700dc5c..e798ee0591b5db6a7728eda20dcab167245a9834 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.51.0

[PATCH RFC v4 06/12] 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 | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index e798ee0591b5db6a7728eda20dcab167245a9834..5fb908a8c764f3c3d2c744022bd61e6307c890c2 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -831,6 +831,160 @@ 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 = 24 * HZ_PER_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);
+	ctx->child1.div = 1;
+	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), 24 * HZ_PER_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 with CLK_SET_RATE_PARENT set 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), 24 * HZ_PER_MHZ);
+
+	ret = clk_set_rate(ctx->child1_clk, 48 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), 48 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 48 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_MHZ);
+
+	clk_test_rate_change_sibling_clk_ctx_put(ctx);
+}
+
+/*
+ * Test that, for a parent with two children with CLK_SET_RATE_PARENT set 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), 24 * HZ_PER_MHZ);
+
+	ret = clk_set_rate(ctx->child1_clk, 48 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), 24 * HZ_PER_MHZ);
+	/* child1 is rounded to the closest supported rate */
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_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)
 {
@@ -3772,6 +3926,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.51.0

[PATCH RFC v4 07/12] 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 5fb908a8c764f3c3d2c744022bd61e6307c890c2..a330de8bd8dc2cdda558d364a3c6d87a26791c8d 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.
@@ -2541,7 +2560,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);
@@ -2549,27 +2567,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;
 
@@ -2757,7 +2759,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);
@@ -2768,27 +2769,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;
 
@@ -2871,39 +2856,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.51.0

[PATCH RFC v4 08/12] 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a330de8bd8dc2cdda558d364a3c6d87a26791c8d..0344f3f62251728e15af277ea0d143dc1f40fd94 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -908,6 +908,47 @@ 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", 24 * HZ_PER_MHZ,
+					   "parent1", 48 * HZ_PER_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);
+	ctx->child1.div = 1;
+	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);
@@ -918,6 +959,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.51.0

[PATCH RFC v4 09/12] 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 0344f3f62251728e15af277ea0d143dc1f40fd94..32defaf1972c28224108c32aef1e74796aae8bc0 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -949,6 +949,46 @@ 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", 24 * HZ_PER_MHZ,
+					   "parent1", 48 * HZ_PER_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);
@@ -963,6 +1003,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.51.0

[PATCH RFC v4 10/12] clk: add support for v2 rate negotiation

[Brian Masney]

The Common Clock Framework is expected to keep a clock’s rate stable
after setting a new rate with:

    clk_set_rate(clk, NEW_RATE);

Clock consumers do not know about the clock hierarchy, sibling clocks,
or the type of clocks involved. However, several longstanding issues
affect how rate changes propagate through the clock tree when
CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:

- A clock in some cases can unknowingly change a sibling clock's rate.
  More details about this particular case are documented at the Link
  tag below.

- No negotiation is done with the sibling clocks, so an inappropriate
  or less than ideal parent rate can be selected.

An unknown subset of existing boards are unknowingly dependent on the
existing behavior, so it's risky to change the way the rate negotiation
logic is done in the clk core.

Add support for v1 and v2 rate negotiation logic to the clk core. When
a child determines that a parent rate change needs to occur when the
v2 logic is used, the parent negotiates with all nodes in that part of
the clk subtree and picks the first rate that's acceptable to all nodes.

The v2 logic is used when the negotiate_rates clk op is defined
on the parent, or current clk code. Otherwise, the existing v1 logic
will be used. Additionally, the clk_v2_rate_negotiation kernel parameter
is introduced to help with debugging when you suspect your board is
unknowingly dependent the legacy behavior.

Link: https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  15 +++
 Documentation/driver-api/clk.rst                |   3 +
 drivers/clk/clk.c                               | 143 +++++++++++++++++++++---
 include/linux/clk-provider.h                    |   7 ++
 include/linux/clk.h                             |  20 ++++
 5 files changed, 174 insertions(+), 14 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 747a55abf4946bb9efe320f0f62fdcd1560b0a71..3b8534811792165d0468616c8cbb3b78f3591472 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -695,6 +695,21 @@
 			platform with proper driver support.  For more
 			information, see Documentation/driver-api/clk.rst.
 
+	clk_v2_rate_negotiation
+			[CLK]
+			Use the v2 common clk rate negotiation logic. In some
+			cases when a child needs to change the rate of it's
+			parent clk, the rate of a sibling clk can be
+			unexpectedly changed with the v1 rate negotiation logic,
+			and some platforms are unknowingly dependent on this
+			behavior.
+
+			This defaults to 1 where the v2 interface is used when
+			supported by the clk providers. If it is 0, then the v1
+			interface will be used. This is useful for debugging when
+			you suspect your board is unknowingly dependent the
+			legacy behavior.
+
 	clock=		[BUGS=X86-32, HW] gettimeofday clocksource override.
 			[Deprecated]
 			Forces specified clocksource (if available) to be used
diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index 93bab5336dfda06069eea700d2830089bf3bce03..c46ee62ba5bd1bf0c66ca282c582963b9ea55580 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -75,6 +75,9 @@ the operations defined in clk-provider.h::
 		void		(*disable)(struct clk_hw *hw);
 		int		(*is_enabled)(struct clk_hw *hw);
 		void		(*disable_unused)(struct clk_hw *hw);
+		bool            (*negotiate_rates)(struct clk_hw *hw,
+					           struct clk_rate_request *req,
+					           bool (*check_rate)(struct clk_core *, unsigned long));
 		unsigned long	(*recalc_rate)(struct clk_hw *hw,
 						unsigned long parent_rate);
 		long		(*round_rate)(struct clk_hw *hw,
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 018dd5a32ecbf166718da3eda851f51fdfdd2088..0cab15f0d7c3d45ff38c1d9971f29d95ac402d41 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -6,6 +6,7 @@
  * Standard functionality for the common clock API.  See Documentation/driver-api/clk.rst
  */
 
+#include <kunit/visibility.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clk/clk-conf.h>
@@ -92,6 +93,8 @@ struct clk_parent_map {
  *                     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.
+ * @validate_rate:     Temporary rate that's used when various clk nodes are
+ *                     negotiating a rate with the v2 rate negotiation logic.
  * @new_parent:        Pointer to new parent during parent change. This is also
  *                     used when a clk's rate is changed.
  * @new_child:         Pointer to new child during reparenting. This is also
@@ -130,6 +133,7 @@ struct clk_core {
 	unsigned long		rate;
 	unsigned long		req_rate;
 	unsigned long		new_rate;
+	unsigned long		validate_rate;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
 	unsigned long		flags;
@@ -411,6 +415,44 @@ static bool clk_core_is_enabled(struct clk_core *core)
 
 /***    helper functions   ***/
 
+static bool clk_v2_rate_negotiation = 1;
+static int __init clk_v2_rate_negotiation_setup(char *str)
+{
+	unsigned long enabled;
+
+	if (!kstrtoul(str, 0, &enabled))
+		clk_v2_rate_negotiation = enabled ? 1 : 0;
+
+	return 1;
+}
+__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
+
+static int clk_core_use_v2_rate_negotiation(struct clk_core *core)
+{
+	bool has_v2_ops = core->ops->negotiate_rates ||
+		(core->parent && core->parent->ops->negotiate_rates);
+
+	return has_v2_ops && clk_v2_rate_negotiation;
+}
+
+void clk_enable_v1_rate_negotiation(void)
+{
+	clk_v2_rate_negotiation = false;
+}
+EXPORT_SYMBOL_IF_KUNIT(clk_enable_v1_rate_negotiation);
+
+void clk_enable_v2_rate_negotiation(void)
+{
+	clk_v2_rate_negotiation = true;
+}
+EXPORT_SYMBOL_IF_KUNIT(clk_enable_v2_rate_negotiation);
+
+int clk_use_v2_rate_negotiation(struct clk *clk)
+{
+	return clk_core_use_v2_rate_negotiation(clk->core);
+}
+EXPORT_SYMBOL_IF_KUNIT(clk_use_v2_rate_negotiation);
+
 const char *__clk_get_name(const struct clk *clk)
 {
 	return !clk ? NULL : clk->core->name;
@@ -2321,7 +2363,8 @@ static int __clk_speculate_rates(struct clk_core *core,
 }
 
 static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
-			     struct clk_core *new_parent, u8 p_index)
+			     struct clk_core *new_parent, u8 p_index,
+			     struct clk_core *initiating_clk)
 {
 	struct clk_core *child;
 
@@ -2334,8 +2377,66 @@ 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);
-		clk_calc_subtree(child, child->new_rate, NULL, 0);
+		if (child == initiating_clk || !clk_core_use_v2_rate_negotiation(child))
+			child->new_rate = clk_recalc(child, new_rate);
+		else
+			child->new_rate = child->rate;
+
+		clk_calc_subtree(child, child->new_rate, NULL, 0, initiating_clk);
+	}
+}
+
+static bool clk_check_rate(struct clk_core *core, unsigned long validate_rate)
+{
+	struct clk_rate_request req;
+	struct clk_hw *orig_parent;
+	struct clk_core *child;
+	int ret, old_flags;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		clk_core_init_rate_req(child, &req, child->new_rate);
+		req.best_parent_rate = validate_rate;
+		orig_parent = req.best_parent_hw;
+
+		/*
+		 * Round the existing child rate based on the new proposed
+		 * parent rate. Don't allow the parent rate to be changed.
+		 */
+		old_flags = child->flags;
+		child->flags &= ~CLK_SET_RATE_PARENT;
+		ret = clk_core_determine_round_nolock(child, &req);
+		child->flags = old_flags;
+
+		if (ret < 0)
+			return false;
+
+		if (WARN_ON_ONCE(req.best_parent_rate != validate_rate))
+			return false;
+
+		// FIXME - muxes not supported at the moment
+		if (req.best_parent_hw != orig_parent)
+			return false;
+
+		if (req.rate < child->new_rate || req.rate > child->new_rate)
+			return false;
+
+		// Check the child's children (if present)
+		if (!clk_check_rate(child, req.rate))
+			return false;
+	}
+
+	core->validate_rate = validate_rate;
+
+	return true;
+}
+
+static void clk_accept_rate_negotiations(struct clk_core *core)
+{
+	struct clk_core *child;
+
+	core->new_rate = core->validate_rate;
+	hlist_for_each_entry(child, &core->children, child_node) {
+		clk_accept_rate_negotiations(child);
 	}
 }
 
@@ -2344,11 +2445,13 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
  * changed.
  */
 static struct clk_core *clk_calc_new_rates(struct clk_core *core,
-					   unsigned long rate)
+					   unsigned long rate,
+					   struct clk_core *initiating_clk)
 {
 	struct clk_core *top = core;
 	struct clk_core *old_parent, *parent;
 	unsigned long best_parent_rate = 0;
+	bool fine_tune_rates = false;
 	unsigned long new_rate;
 	unsigned long min_rate;
 	unsigned long max_rate;
@@ -2392,7 +2495,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		return NULL;
 	} else {
 		/* pass-through clock with adjustable parent */
-		top = clk_calc_new_rates(parent, rate);
+		top = clk_calc_new_rates(parent, rate, initiating_clk);
 		new_rate = parent->new_rate;
 		goto out;
 	}
@@ -2416,11 +2519,23 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	}
 
 	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
-	    best_parent_rate != parent->rate)
-		top = clk_calc_new_rates(parent, best_parent_rate);
+	    best_parent_rate != parent->rate) {
+		top = clk_calc_new_rates(parent, best_parent_rate, initiating_clk);
+		fine_tune_rates = true;
+	}
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	clk_calc_subtree(core, new_rate, parent, p_index, initiating_clk);
+
+	if (fine_tune_rates && clk_core_use_v2_rate_negotiation(top)) {
+		struct clk_rate_request req;
+
+		clk_core_init_rate_req(top, &req, top->new_rate);
+		if (!top->ops->negotiate_rates(top->hw, &req, clk_check_rate))
+			return NULL;
+
+		clk_accept_rate_negotiations(top);
+	}
 
 	return top;
 }
@@ -2468,7 +2583,7 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
  */
-static void clk_change_rate(struct clk_core *core)
+static void clk_change_rate(struct clk_core *core, struct clk_core *initiating_clk)
 {
 	struct clk_core *child;
 	struct hlist_node *tmp;
@@ -2537,7 +2652,7 @@ static void clk_change_rate(struct clk_core *core)
 		__clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
 
 	if (core->flags & CLK_RECALC_NEW_RATES)
-		(void)clk_calc_new_rates(core, core->new_rate);
+		(void)clk_calc_new_rates(core, core->new_rate, initiating_clk);
 
 	/*
 	 * Use safe iteration, as change_rate can actually swap parents
@@ -2547,12 +2662,12 @@ static void clk_change_rate(struct clk_core *core)
 		/* Skip children who will be reparented to another clock */
 		if (child->new_parent && child->new_parent != core)
 			continue;
-		clk_change_rate(child);
+		clk_change_rate(child, initiating_clk);
 	}
 
 	/* handle the new child who might not be in core->children yet */
 	if (core->new_child)
-		clk_change_rate(core->new_child);
+		clk_change_rate(core->new_child, initiating_clk);
 
 	clk_pm_runtime_put(core);
 }
@@ -2608,7 +2723,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EBUSY;
 
 	/* calculate new rates and get the topmost changed clock */
-	top = clk_calc_new_rates(core, req_rate);
+	top = clk_calc_new_rates(core, req_rate, core);
 	if (!top)
 		return -EINVAL;
 
@@ -2627,7 +2742,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	}
 
 	/* change the rates */
-	clk_change_rate(top);
+	clk_change_rate(top, core);
 
 	core->req_rate = req_rate;
 err:
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 630705a47129453c241f1b1755f2c2f2a7ed8f77..9041b17ba99ef16a01e9f4d749e2e4b601a94b93 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -129,6 +129,10 @@ struct clk_duty {
  * @restore_context: Restore the context of the clock after a restoration
  *		of power.
  *
+ * @negotiate_rates: When a child clk requests a new rate that requires a rate
+ *		change from the parent, this negotiates a new parent rate that's
+ *		acceptable to all of the children.
+ *
  * @recalc_rate: Recalculate the rate of this clock, by querying hardware. The
  *		parent rate is an input parameter.  It is up to the caller to
  *		ensure that the prepare_mutex is held across this call. If the
@@ -242,6 +246,9 @@ struct clk_ops {
 	void		(*disable_unused)(struct clk_hw *hw);
 	int		(*save_context)(struct clk_hw *hw);
 	void		(*restore_context)(struct clk_hw *hw);
+	bool		(*negotiate_rates)(struct clk_hw *hw,
+					   struct clk_rate_request *req,
+					   bool (*check_rate)(struct clk_core *, unsigned long));
 	unsigned long	(*recalc_rate)(struct clk_hw *hw,
 					unsigned long parent_rate);
 	long		(*round_rate)(struct clk_hw *hw, unsigned long rate,
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b607482ca77e987b9344c38f25ebb5c8d35c1d39..e58f6c91aceaa329b4af90d3924ee63d47ecd68c 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -91,6 +91,26 @@ struct clk_bulk_data {
 
 #ifdef CONFIG_COMMON_CLK
 
+void clk_enable_v1_rate_negotiation(void);
+
+void clk_enable_v2_rate_negotiation(void);
+
+/**
+ * clk_use_v2_rate_negotiation - Use the v2 common clk rate negotiation logic.
+ * In some cases when a child needs to change the rate of it's parent clk, the
+ * rate of a sibling clk can be unexpectedly changed by the v1 negotiation
+ * logic. Some platforms are unknowingly dependent on this behavior.
+ *
+ * This function is only exported for the kunit tests, and this is not to be
+ * used outside of the clk core.
+ *
+ * @clk: clock to check
+ *
+ * Returns 1 if the v2 negotiation logic is used, or 0 if the v1 negotiation
+ * logic is used.
+ */
+int clk_use_v2_rate_negotiation(struct clk *clk);
+
 /**
  * clk_notifier_register - register a clock rate-change notifier callback
  * @clk: clock whose rate we are interested in

-- 
2.51.0

[PATCH RFC v4 11/12] clk: test: introduce negotiate_rates() op for clk_dummy and clk_dummy_div

[Brian Masney]

This is needed for the v2 rate negotiation code where the parent works
with all of it's children to find the best suitable rate.

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

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 32defaf1972c28224108c32aef1e74796aae8bc0..7c4d1a50a7dd0bfb66e021ba314a9a9709813d97 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -29,6 +29,7 @@ static const struct clk_ops empty_clk_ops = { };
 struct clk_dummy_context {
 	struct clk_hw hw;
 	unsigned long rate;
+	unsigned long negotiate_step_size;
 };
 
 static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw,
@@ -97,10 +98,31 @@ static u8 clk_dummy_single_get_parent(struct clk_hw *hw)
 	return 0;
 }
 
+static bool clk_dummy_negotiate_rates(struct clk_hw *hw,
+				      struct clk_rate_request *req,
+				      bool (*check_rate)(struct clk_core *, unsigned long))
+{
+	struct clk_dummy_context *ctx =
+		container_of(hw, struct clk_dummy_context, hw);
+
+	if (WARN_ON_ONCE(!ctx->negotiate_step_size))
+		return false;
+
+	for (unsigned long rate = req->min_rate;
+	     rate <= req->max_rate;
+	     rate += ctx->negotiate_step_size) {
+		if (check_rate(req->core, rate))
+			return true;
+	}
+
+	return false;
+}
+
 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,
+	.negotiate_rates = clk_dummy_negotiate_rates,
 };
 
 static const struct clk_ops clk_dummy_maximize_rate_ops = {
@@ -175,10 +197,28 @@ static int clk_dummy_div_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static bool clk_dummy_div_negotiate_rates(struct clk_hw *hw,
+					  struct clk_rate_request *req,
+					  bool (*check_rate)(struct clk_core *, unsigned long))
+{
+	unsigned long rate;
+
+	for (int i = 0; i < BIT(CLK_DUMMY_DIV_WIDTH + 1); i++) {
+		rate = divider_recalc_rate(hw, req->best_parent_rate, i, NULL,
+					   CLK_DIVIDER_ROUND_CLOSEST,
+					   CLK_DUMMY_DIV_WIDTH);
+		if (check_rate(req->core, rate))
+			return true;
+	}
+
+	return false;
+}
+
 static const struct clk_ops clk_dummy_div_ops = {
 	.recalc_rate = clk_dummy_div_recalc_rate,
 	.determine_rate = clk_dummy_div_determine_rate,
 	.set_rate = clk_dummy_div_set_rate,
+	.negotiate_rates = clk_dummy_div_negotiate_rates,
 };
 
 struct clk_dummy_gate {

-- 
2.51.0

[PATCH RFC v4 12/12] clk: test: update divider kunit tests for v1 and v2 rate negotiation

[Brian Masney]

Update the divider kunit tests to verify that the v1 and v2 rate
negotiation logic is working as expected.

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

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 7c4d1a50a7dd0bfb66e021ba314a9a9709813d97..87af60d0782274c9faacf7729ed95bf04dfd4860 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -763,6 +763,7 @@ static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 	test->priv = ctx;
 
 	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
+	ctx->parent.negotiate_step_size = 1 * HZ_PER_MHZ;
 	ctx->parent.rate = 24 * HZ_PER_MHZ;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
 	if (ret)
@@ -793,6 +794,20 @@ static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 	return 0;
 }
 
+static int clk_rate_change_sibling_div_div_v1_test_init(struct kunit *test)
+{
+	clk_enable_v1_rate_negotiation();
+
+	return clk_rate_change_sibling_div_div_test_init(test);
+}
+
+static int clk_rate_change_sibling_div_div_v2_test_init(struct kunit *test)
+{
+	clk_enable_v2_rate_negotiation();
+
+	return clk_rate_change_sibling_div_div_test_init(test);
+}
+
 static void clk_rate_change_sibling_div_div_test_exit(struct kunit *test)
 {
 	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
@@ -833,16 +848,21 @@ 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;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	ret = clk_set_rate(ctx->child1_clk, 48 * HZ_PER_MHZ);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 48 * HZ_PER_MHZ);
 	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 48 * HZ_PER_MHZ);
 	KUNIT_EXPECT_EQ(test, ctx->child1.div, 1);
-	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_MHZ);
-	KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+
+	if (clk_use_v2_rate_negotiation(ctx->child1_clk)) {
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 24 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+	} else {
+		// Legacy behavior in v1 logic where sibling clks are expectedly changed.
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 48 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
+	}
 }
 
 /*
@@ -856,19 +876,26 @@ 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;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	ret = clk_set_rate(ctx->child2_clk, 48 * HZ_PER_MHZ);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 96 * HZ_PER_MHZ);
-	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 32 * HZ_PER_MHZ);
-	KUNIT_EXPECT_EQ(test, ctx->child1.div, 3);
-	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 48 * HZ_PER_MHZ);
-	KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+	if (clk_use_v2_rate_negotiation(ctx->child1_clk)) {
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 96 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 32 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child1.div, 3);
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 48 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child2.div, 2);
+	} else {
+		// Legacy behavior in v1 logic where sibling clks are expectedly changed.
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 48 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 48 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child1.div, 1);
+		KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 48 * HZ_PER_MHZ);
+		KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
+	}
 }
 
 static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
@@ -879,13 +906,25 @@ static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
 };
 
 /*
- * 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.
+ * Test suite with v1 rate negotiation logic 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_v1_test_suite = {
+	.name = "clk-rate-change-sibling-div-div-v1",
+	.init = clk_rate_change_sibling_div_div_v1_test_init,
+	.exit = clk_rate_change_sibling_div_div_test_exit,
+	.test_cases = clk_rate_change_sibling_div_div_cases,
+};
+
+/*
+ * Test suite with v2 rate negotiation logic 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,
+static struct kunit_suite clk_rate_change_sibling_div_div_v2_test_suite = {
+	.name = "clk-rate-change-sibling-div-div-v2",
+	.init = clk_rate_change_sibling_div_div_v2_test_init,
 	.exit = clk_rate_change_sibling_div_div_test_exit,
 	.test_cases = clk_rate_change_sibling_div_div_cases,
 };
@@ -4017,7 +4056,8 @@ 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_rate_change_sibling_div_div_v1_test_suite,
+	&clk_rate_change_sibling_div_div_v2_test_suite,
 	&clk_rate_change_sibling_test_suite,
 	&clk_mux_no_reparent_test_suite,
 	&clk_mux_notifier_test_suite,
@@ -4033,4 +4073,5 @@ kunit_test_suites(
 	&clk_uncached_test_suite,
 );
 MODULE_DESCRIPTION("Kunit tests for clk framework");
+MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
 MODULE_LICENSE("GPL v2");

-- 
2.51.0

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Brian Masney]

On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> I marked this as a RFC since Stephen asked me in a video call to not
> add a new member to struct clk_core, however I don't see how to do this
> any other way.

Correction: Stephen asked me to not add a new member to struct clk_ops.

Brian

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Maxime Ripard]

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

Hi Brian,

On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> The Common Clock Framework is expected to keep a clock’s rate stable
> after setting a new rate with:
> 
>     clk_set_rate(clk, NEW_RATE);
> 
> Clock consumers do not know about the clock hierarchy, sibling clocks,
> or the type of clocks involved. However, several longstanding issues
> affect how rate changes propagate through the clock tree when
> CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> 
> - A clock in some cases can unknowingly change a sibling clock's rate.
>   More details about this particular case are documented at:
>   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> 
> - No negotiation is done with the sibling clocks, so an inappropriate
>   or less than ideal parent rate can be selected.
> 
> A selection of some real world examples of where this shows up is at
> [1]. DRM needs to run at precise clock rates, and this issue shows up
> there, however will also show up in other subsystems that require
> precise clock rates, such as sound.
> 
> An unknown subset of existing boards are unknowingly dependent on the
> existing behavior, so it's risky to change the way the rate negotiation
> logic is done in the clk core.
> 
> This series adds support for v1 and v2 rate negotiation logic to the clk
> core. When a child determines that a parent rate change needs to occur
> when the v2 logic is used, the parent negotiates with all nodes in that
> part of the clk subtree and picks the first rate that's acceptable to
> all nodes.
> 
> Kunit tests are introduced to illustrate the problem, and are updated
> later in the series to illustrate that the v2 negotiation logic works
> as expected, while keeping compatibility with v1.
> 
> I marked this as a RFC since Stephen asked me in a video call to not
> add a new member to struct clk_core, however I don't see how to do this
> any other way.
> 
> - The clk core doesn’t, and shouldn’t, know about the internal state the
>   various clk providers.
> - Child clks shouldn’t have to know the internal state of the parent clks.
> - Currently this information is not exposed in any way to the clk core.

I recall from that video call that Stephen asked:

- to indeed not introduce a new op
- to evaluate the change from top to bottom, but to set it bottom to top
- to evaluate the rate by letting child clocks expose an array of the
  parent rates they would like, and to intersect all of them to figure
  out the best parent rate.

It looks like you followed none of these suggestions, so explaining why
you couldn't implement them would be a great first step.

Also, you sent an RFC, on what would you like a comment exactly?

Maxime

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

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Brian Masney]

On Thu, Sep 25, 2025 at 02:14:14PM +0200, Maxime Ripard wrote:
> On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> > The Common Clock Framework is expected to keep a clock’s rate stable
> > after setting a new rate with:
> > 
> >     clk_set_rate(clk, NEW_RATE);
> > 
> > Clock consumers do not know about the clock hierarchy, sibling clocks,
> > or the type of clocks involved. However, several longstanding issues
> > affect how rate changes propagate through the clock tree when
> > CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> > 
> > - A clock in some cases can unknowingly change a sibling clock's rate.
> >   More details about this particular case are documented at:
> >   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> > 
> > - No negotiation is done with the sibling clocks, so an inappropriate
> >   or less than ideal parent rate can be selected.
> > 
> > A selection of some real world examples of where this shows up is at
> > [1]. DRM needs to run at precise clock rates, and this issue shows up
> > there, however will also show up in other subsystems that require
> > precise clock rates, such as sound.
> > 
> > An unknown subset of existing boards are unknowingly dependent on the
> > existing behavior, so it's risky to change the way the rate negotiation
> > logic is done in the clk core.
> > 
> > This series adds support for v1 and v2 rate negotiation logic to the clk
> > core. When a child determines that a parent rate change needs to occur
> > when the v2 logic is used, the parent negotiates with all nodes in that
> > part of the clk subtree and picks the first rate that's acceptable to
> > all nodes.
> > 
> > Kunit tests are introduced to illustrate the problem, and are updated
> > later in the series to illustrate that the v2 negotiation logic works
> > as expected, while keeping compatibility with v1.
> > 
> > I marked this as a RFC since Stephen asked me in a video call to not
> > add a new member to struct clk_core, however I don't see how to do this
> > any other way.
> > 
> > - The clk core doesn’t, and shouldn’t, know about the internal state the
> >   various clk providers.
> > - Child clks shouldn’t have to know the internal state of the parent clks.
> > - Currently this information is not exposed in any way to the clk core.
> 
> I recall from that video call that Stephen asked:
> 
> - to indeed not introduce a new op
> - to evaluate the change from top to bottom, but to set it bottom to top
> - to evaluate the rate by letting child clocks expose an array of the
>   parent rates they would like, and to intersect all of them to figure
>   out the best parent rate.
> 
> It looks like you followed none of these suggestions, so explaining why
> you couldn't implement them would be a great first step.
> 
> Also, you sent an RFC, on what would you like a comment exactly?

Stephen asked me to not introduce a new clk op, however I don't see a
clean way to do this any other way. Personally, I think that we need a
new clk op for this use case for the reasons I outlined on the cover
letter. I am open for suggestions about alternative ways, and will
gladly make modifications. This is why I marked this series as RFC.
Patch 10 in this series is the main change of note here.

Additionally, the presence of the new op is a convenient way to also
signal to the clk core that these providers can use the v2 negotiation
logic. Otherwise, we'll need to introduce a flag somewhere else if we
want to support a v1/v2 negotiation logic across the clk tree.

As for 2), I negotiate the rate change from the top down. The new_rate
is propagated in the same manner as what's done today in the clk core
when a parent rate change occurs. I let it reuse the existing rate
change code that's currently in the clk core to minimize the change
that was introduced.

Regarding the clock table in 3), it could be done with what's there,
but there's the issue of the new clk_op. I posted this to get feedback
about that since I think we should settle on the core changes.

Brian

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Maxime Ripard]

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

On Thu, Sep 25, 2025 at 10:20:24AM -0400, Brian Masney wrote:
> On Thu, Sep 25, 2025 at 02:14:14PM +0200, Maxime Ripard wrote:
> > On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> > > The Common Clock Framework is expected to keep a clock’s rate stable
> > > after setting a new rate with:
> > > 
> > >     clk_set_rate(clk, NEW_RATE);
> > > 
> > > Clock consumers do not know about the clock hierarchy, sibling clocks,
> > > or the type of clocks involved. However, several longstanding issues
> > > affect how rate changes propagate through the clock tree when
> > > CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> > > 
> > > - A clock in some cases can unknowingly change a sibling clock's rate.
> > >   More details about this particular case are documented at:
> > >   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> > > 
> > > - No negotiation is done with the sibling clocks, so an inappropriate
> > >   or less than ideal parent rate can be selected.
> > > 
> > > A selection of some real world examples of where this shows up is at
> > > [1]. DRM needs to run at precise clock rates, and this issue shows up
> > > there, however will also show up in other subsystems that require
> > > precise clock rates, such as sound.
> > > 
> > > An unknown subset of existing boards are unknowingly dependent on the
> > > existing behavior, so it's risky to change the way the rate negotiation
> > > logic is done in the clk core.
> > > 
> > > This series adds support for v1 and v2 rate negotiation logic to the clk
> > > core. When a child determines that a parent rate change needs to occur
> > > when the v2 logic is used, the parent negotiates with all nodes in that
> > > part of the clk subtree and picks the first rate that's acceptable to
> > > all nodes.
> > > 
> > > Kunit tests are introduced to illustrate the problem, and are updated
> > > later in the series to illustrate that the v2 negotiation logic works
> > > as expected, while keeping compatibility with v1.
> > > 
> > > I marked this as a RFC since Stephen asked me in a video call to not
> > > add a new member to struct clk_core, however I don't see how to do this
> > > any other way.
> > > 
> > > - The clk core doesn’t, and shouldn’t, know about the internal state the
> > >   various clk providers.
> > > - Child clks shouldn’t have to know the internal state of the parent clks.
> > > - Currently this information is not exposed in any way to the clk core.
> > 
> > I recall from that video call that Stephen asked:
> > 
> > - to indeed not introduce a new op
> > - to evaluate the change from top to bottom, but to set it bottom to top
> > - to evaluate the rate by letting child clocks expose an array of the
> >   parent rates they would like, and to intersect all of them to figure
> >   out the best parent rate.
> > 
> > It looks like you followed none of these suggestions, so explaining why
> > you couldn't implement them would be a great first step.
> > 
> > Also, you sent an RFC, on what would you like a comment exactly?
> 
> Stephen asked me to not introduce a new clk op, however I don't see a
> clean way to do this any other way. Personally, I think that we need a
> new clk op for this use case for the reasons I outlined on the cover
> letter.

So his suggestion was to base the whole logic on clk_ops.determine_rate.
You're saying that it would violate parent/child abstraction. Can you
explain why you think that is the case, because it's really not obvious
to me.

Additionally, and assuming that we indeed need something similar to your
suggestion, determinate_rate takes a pointer to struct clk_rate_request.
Why did you choose to create a new op instead of adding the check_rate
pointer to clk_rate_request?

> I am open for suggestions about alternative ways, and will gladly make
> modifications. This is why I marked this series as RFC. Patch 10 in
> this series is the main change of note here.
> 
> Additionally, the presence of the new op is a convenient way to also
> signal to the clk core that these providers can use the v2 negotiation
> logic. Otherwise, we'll need to introduce a flag somewhere else if we
> want to support a v1/v2 negotiation logic across the clk tree.
> 
> As for 2), I negotiate the rate change from the top down. The new_rate
> is propagated in the same manner as what's done today in the clk core
> when a parent rate change occurs. I let it reuse the existing rate
> change code that's currently in the clk core to minimize the change
> that was introduced.
> 
> Regarding the clock table in 3), it could be done with what's there,
> but there's the issue of the new clk_op. I posted this to get feedback
> about that since I think we should settle on the core changes.

Again, why can't we add a pointer to that array in clk_rate_request?

Maxime

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

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Brian Masney]

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

Hi Maxime (and Stephen),

On Tue, Sep 30, 2025 at 01:28:52PM +0200, Maxime Ripard wrote:
> On Thu, Sep 25, 2025 at 10:20:24AM -0400, Brian Masney wrote:
> > On Thu, Sep 25, 2025 at 02:14:14PM +0200, Maxime Ripard wrote:
> > > On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> > > > The Common Clock Framework is expected to keep a clock’s rate stable
> > > > after setting a new rate with:
> > > > 
> > > >     clk_set_rate(clk, NEW_RATE);
> > > > 
> > > > Clock consumers do not know about the clock hierarchy, sibling clocks,
> > > > or the type of clocks involved. However, several longstanding issues
> > > > affect how rate changes propagate through the clock tree when
> > > > CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> > > > 
> > > > - A clock in some cases can unknowingly change a sibling clock's rate.
> > > >   More details about this particular case are documented at:
> > > >   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> > > > 
> > > > - No negotiation is done with the sibling clocks, so an inappropriate
> > > >   or less than ideal parent rate can be selected.
> > > > 
> > > > A selection of some real world examples of where this shows up is at
> > > > [1]. DRM needs to run at precise clock rates, and this issue shows up
> > > > there, however will also show up in other subsystems that require
> > > > precise clock rates, such as sound.
> > > > 
> > > > An unknown subset of existing boards are unknowingly dependent on the
> > > > existing behavior, so it's risky to change the way the rate negotiation
> > > > logic is done in the clk core.
> > > > 
> > > > This series adds support for v1 and v2 rate negotiation logic to the clk
> > > > core. When a child determines that a parent rate change needs to occur
> > > > when the v2 logic is used, the parent negotiates with all nodes in that
> > > > part of the clk subtree and picks the first rate that's acceptable to
> > > > all nodes.
> > > > 
> > > > Kunit tests are introduced to illustrate the problem, and are updated
> > > > later in the series to illustrate that the v2 negotiation logic works
> > > > as expected, while keeping compatibility with v1.
> > > > 
> > > > I marked this as a RFC since Stephen asked me in a video call to not
> > > > add a new member to struct clk_core, however I don't see how to do this
> > > > any other way.
> > > > 
> > > > - The clk core doesn’t, and shouldn’t, know about the internal state the
> > > >   various clk providers.
> > > > - Child clks shouldn’t have to know the internal state of the parent clks.
> > > > - Currently this information is not exposed in any way to the clk core.
> > > 
> > > I recall from that video call that Stephen asked:
> > > 
> > > - to indeed not introduce a new op
> > > - to evaluate the change from top to bottom, but to set it bottom to top
> > > - to evaluate the rate by letting child clocks expose an array of the
> > >   parent rates they would like, and to intersect all of them to figure
> > >   out the best parent rate.
> > > 
> > > It looks like you followed none of these suggestions, so explaining why
> > > you couldn't implement them would be a great first step.
> > > 
> > > Also, you sent an RFC, on what would you like a comment exactly?
> > 
> > Stephen asked me to not introduce a new clk op, however I don't see a
> > clean way to do this any other way. Personally, I think that we need a
> > new clk op for this use case for the reasons I outlined on the cover
> > letter.
> 
> So his suggestion was to base the whole logic on clk_ops.determine_rate.
> You're saying that it would violate parent/child abstraction. Can you
> explain why you think that is the case, because it's really not obvious
> to me.
> 
> Additionally, and assuming that we indeed need something similar to your
> suggestion, determinate_rate takes a pointer to struct clk_rate_request.
> Why did you choose to create a new op instead of adding the check_rate
> pointer to clk_rate_request?

Sorry about the delayed response. I've been busy with other projects at
work.

I attached a patch that puts the negotiate_rates member on struct
clk_rate_request instead of struct clk_ops. In order to get this to
work, it also required adding it to struct clk_core and
struct clk_init_data as well. I made this so that this patch applies
on top of this series.

I think the clk_rate_request approach is very ugly, and adding it to
struct clk_ops like I have it in this series is the way to go.

I'm giving a talk at Linux Plumbers in Tokyo next month:

    Fixing Clock Tree Propagation in the Common Clk Framework 
    https://lpc.events/event/19/contributions/2152/

Stephen will be there as well, and hopefully we can reach consensus
about an acceptable approach to fix this.

My round_rate to determine_rate conversion will drop one member from
struct clk_ops, so maybe that'll help, and we can have a trade?
There's currently only one outstanding patch series remaining in
drivers/phy that's blocking me from posting what I have to remove
round_rate from the clk core:

https://lore.kernel.org/linux-clk/20251106-phy-clk-route-rate-v2-resend-v1-0-e2058963bfb1@redhat.com/T/

I'll bug Vinod on email again so that we can hopefully get that in for
v6.19. (No response on IRC yesterday.) If not, I see that he's giving a
talk at Plumbers and I'll bug him in person after his talk.

Brian

[-- Attachment #2: 0001-HACK-clk-move-negotiate_rates-member-from-struct-clk.patch --]
[-- Type: text/plain, Size: 9282 bytes --]

From 469c29a4d14a83155a280df4679a43ad4af2e1b4 Mon Sep 17 00:00:00 2001
From: Brian Masney <bmasney@redhat.com>
Date: Fri, 14 Nov 2025 18:45:06 -0500
Subject: [PATCH] HACK: clk: move negotiate_rates member from struct clk_ops to
 struct clk_rate_request
Content-type: text/plain

Demonstrate one way to move the negotiate_rates member from struct
clk_ops to struct clk_rate_request. Personally I think it's much cleaner
to have this on struct clk_ops.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 Documentation/driver-api/clk.rst |  8 +++++---
 drivers/clk/clk.c                | 11 ++++++++---
 drivers/clk/clk_test.c           | 32 +++++++++++++++++++++++++-------
 include/linux/clk-provider.h     | 19 ++++++++++++-------
 4 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index c46ee62ba5bd..47ed511432db 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -75,9 +75,6 @@ the operations defined in clk-provider.h::
 		void		(*disable)(struct clk_hw *hw);
 		int		(*is_enabled)(struct clk_hw *hw);
 		void		(*disable_unused)(struct clk_hw *hw);
-		bool            (*negotiate_rates)(struct clk_hw *hw,
-					           struct clk_rate_request *req,
-					           bool (*check_rate)(struct clk_core *, unsigned long));
 		unsigned long	(*recalc_rate)(struct clk_hw *hw,
 						unsigned long parent_rate);
 		long		(*round_rate)(struct clk_hw *hw,
@@ -103,6 +100,11 @@ the operations defined in clk-provider.h::
 					      struct dentry *dentry);
 	};
 
+Note: The negotiate_rates callback is not part of struct clk_ops. Instead, it
+is specified in struct clk_init_data during clock registration and is copied
+to struct clk_rate_request for use during rate negotiations. It is invoked
+through the rate request structure rather than through ops.
+
 Hardware clk implementations
 ============================
 
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 926b7d4b9ab8..8014eb719266 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -130,6 +130,9 @@ struct clk_parent_map {
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;
+	bool			(*negotiate_rates)(struct clk_hw *hw,
+						   struct clk_rate_request *req,
+						   bool (*check_rate)(struct clk_core *, unsigned long));
 	struct clk_hw		*hw;
 	struct module		*owner;
 	struct device		*dev;
@@ -427,8 +430,8 @@ static bool clk_core_is_enabled(struct clk_core *core)
 
 static int clk_core_use_v2_rate_negotiation(struct clk_core *core)
 {
-	bool has_v2_ops = core->ops->negotiate_rates ||
-	                  (core->parent && core->parent->ops->negotiate_rates);
+	bool has_v2_ops = core->negotiate_rates ||
+		(core->parent && core->parent->negotiate_rates);
 
 	return has_v2_ops && modparam_clk_v2_rate_negotiation;
 }
@@ -1713,6 +1716,7 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 
 	req->core = core;
 	req->rate = rate;
+	req->negotiate_rates = core->negotiate_rates;
 	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 
 	parent = core->parent;
@@ -2504,7 +2508,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		struct clk_rate_request req;
 
 		clk_core_init_rate_req(top, &req, top->new_rate);
-		if (!top->ops->negotiate_rates(top->hw, &req, clk_check_rate))
+		if (!req.negotiate_rates || !req.negotiate_rates(top->hw, &req, clk_check_rate))
 			return NULL;
 
 		clk_accept_rate_negotiations(top);
@@ -4511,6 +4515,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 		goto fail_ops;
 	}
 	core->ops = init->ops;
+	core->negotiate_rates = init->negotiate_rates;
 
 	core->dev = dev;
 	clk_pm_runtime_init(core);
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index ff53b8bdf872..be2a49d2e446 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -122,7 +122,6 @@ 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,
-	.negotiate_rates = clk_dummy_negotiate_rates,
 };
 
 static const struct clk_ops clk_dummy_maximize_rate_ops = {
@@ -218,7 +217,6 @@ static const struct clk_ops clk_dummy_div_ops = {
 	.recalc_rate = clk_dummy_div_recalc_rate,
 	.determine_rate = clk_dummy_div_determine_rate,
 	.set_rate = clk_dummy_div_set_rate,
-	.negotiate_rates = clk_dummy_div_negotiate_rates,
 };
 
 struct clk_dummy_gate {
@@ -755,6 +753,26 @@ struct clk_rate_change_sibling_div_div_context {
 static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 {
 	struct clk_rate_change_sibling_div_div_context *ctx;
+	struct clk_init_data parent_init = {
+		.name = "parent",
+		.ops = &clk_dummy_rate_ops,
+		.negotiate_rates = clk_dummy_negotiate_rates,
+		.flags = 0,
+	};
+	struct clk_init_data child1_init = {
+		.name = "child1",
+		.ops = &clk_dummy_div_ops,
+		.negotiate_rates = clk_dummy_div_negotiate_rates,
+		.flags = CLK_SET_RATE_PARENT,
+		.num_parents = 1,
+	};
+	struct clk_init_data child2_init = {
+		.name = "child2",
+		.ops = &clk_dummy_div_ops,
+		.negotiate_rates = clk_dummy_div_negotiate_rates,
+		.flags = CLK_SET_RATE_PARENT,
+		.num_parents = 1,
+	};
 	int ret;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
@@ -762,22 +780,22 @@ static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 		return -ENOMEM;
 	test->priv = ctx;
 
-	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
+	ctx->parent.hw.init = &parent_init;
 	ctx->parent.negotiate_step_size = 1 * HZ_PER_MHZ;
 	ctx->parent.rate = 24 * HZ_PER_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);
+	child1_init.parent_hws = (const struct clk_hw*[]) { &ctx->parent.hw };
+	ctx->child1.hw.init = &child1_init;
 	ctx->child1.div = 1;
 	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);
+	child2_init.parent_hws = (const struct clk_hw*[]) { &ctx->parent.hw };
+	ctx->child2.hw.init = &child2_init;
 	ctx->child2.div = 1;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
 	if (ret)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 9041b17ba99e..bc162afc8cd8 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -53,6 +53,9 @@ struct dentry;
  *			requested constraints.
  * @best_parent_hw:	The most appropriate parent clock that fulfills the
  *			requested constraints.
+ * @negotiate_rates:	When a child clk requests a new rate that requires a rate
+ *			change from the parent, this negotiates a new parent rate that's
+ *			acceptable to all of the children.
  *
  */
 struct clk_rate_request {
@@ -62,6 +65,9 @@ struct clk_rate_request {
 	unsigned long max_rate;
 	unsigned long best_parent_rate;
 	struct clk_hw *best_parent_hw;
+	bool (*negotiate_rates)(struct clk_hw *hw,
+				struct clk_rate_request *req,
+				bool (*check_rate)(struct clk_core *, unsigned long));
 };
 
 void clk_hw_init_rate_request(const struct clk_hw *hw,
@@ -129,10 +135,6 @@ struct clk_duty {
  * @restore_context: Restore the context of the clock after a restoration
  *		of power.
  *
- * @negotiate_rates: When a child clk requests a new rate that requires a rate
- *		change from the parent, this negotiates a new parent rate that's
- *		acceptable to all of the children.
- *
  * @recalc_rate: Recalculate the rate of this clock, by querying hardware. The
  *		parent rate is an input parameter.  It is up to the caller to
  *		ensure that the prepare_mutex is held across this call. If the
@@ -246,9 +248,6 @@ struct clk_ops {
 	void		(*disable_unused)(struct clk_hw *hw);
 	int		(*save_context)(struct clk_hw *hw);
 	void		(*restore_context)(struct clk_hw *hw);
-	bool		(*negotiate_rates)(struct clk_hw *hw,
-					   struct clk_rate_request *req,
-					   bool (*check_rate)(struct clk_core *, unsigned long));
 	unsigned long	(*recalc_rate)(struct clk_hw *hw,
 					unsigned long parent_rate);
 	long		(*round_rate)(struct clk_hw *hw, unsigned long rate,
@@ -295,6 +294,9 @@ struct clk_parent_data {
  *
  * @name: clock name
  * @ops: operations this clock supports
+ * @negotiate_rates: When a child clk requests a new rate that requires a rate
+ *		change from the parent, this negotiates a new parent rate that's
+ *		acceptable to all of the children.
  * @parent_names: array of string names for all possible parents
  * @parent_data: array of parent data for all possible parents (when some
  *               parents are external to the clk controller)
@@ -306,6 +308,9 @@ struct clk_parent_data {
 struct clk_init_data {
 	const char		*name;
 	const struct clk_ops	*ops;
+	bool			(*negotiate_rates)(struct clk_hw *hw,
+						   struct clk_rate_request *req,
+						   bool (*check_rate)(struct clk_core *, unsigned long));
 	/* Only one of the following three should be assigned */
 	const char		* const *parent_names;
 	const struct clk_parent_data	*parent_data;
-- 
2.51.1

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Maxime Ripard]

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

On Fri, Nov 14, 2025 at 07:22:55PM -0500, Brian Masney wrote:
> Hi Maxime (and Stephen),
> 
> On Tue, Sep 30, 2025 at 01:28:52PM +0200, Maxime Ripard wrote:
> > On Thu, Sep 25, 2025 at 10:20:24AM -0400, Brian Masney wrote:
> > > On Thu, Sep 25, 2025 at 02:14:14PM +0200, Maxime Ripard wrote:
> > > > On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> > > > > The Common Clock Framework is expected to keep a clock’s rate stable
> > > > > after setting a new rate with:
> > > > > 
> > > > >     clk_set_rate(clk, NEW_RATE);
> > > > > 
> > > > > Clock consumers do not know about the clock hierarchy, sibling clocks,
> > > > > or the type of clocks involved. However, several longstanding issues
> > > > > affect how rate changes propagate through the clock tree when
> > > > > CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> > > > > 
> > > > > - A clock in some cases can unknowingly change a sibling clock's rate.
> > > > >   More details about this particular case are documented at:
> > > > >   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> > > > > 
> > > > > - No negotiation is done with the sibling clocks, so an inappropriate
> > > > >   or less than ideal parent rate can be selected.
> > > > > 
> > > > > A selection of some real world examples of where this shows up is at
> > > > > [1]. DRM needs to run at precise clock rates, and this issue shows up
> > > > > there, however will also show up in other subsystems that require
> > > > > precise clock rates, such as sound.
> > > > > 
> > > > > An unknown subset of existing boards are unknowingly dependent on the
> > > > > existing behavior, so it's risky to change the way the rate negotiation
> > > > > logic is done in the clk core.
> > > > > 
> > > > > This series adds support for v1 and v2 rate negotiation logic to the clk
> > > > > core. When a child determines that a parent rate change needs to occur
> > > > > when the v2 logic is used, the parent negotiates with all nodes in that
> > > > > part of the clk subtree and picks the first rate that's acceptable to
> > > > > all nodes.
> > > > > 
> > > > > Kunit tests are introduced to illustrate the problem, and are updated
> > > > > later in the series to illustrate that the v2 negotiation logic works
> > > > > as expected, while keeping compatibility with v1.
> > > > > 
> > > > > I marked this as a RFC since Stephen asked me in a video call to not
> > > > > add a new member to struct clk_core, however I don't see how to do this
> > > > > any other way.
> > > > > 
> > > > > - The clk core doesn’t, and shouldn’t, know about the internal state the
> > > > >   various clk providers.
> > > > > - Child clks shouldn’t have to know the internal state of the parent clks.
> > > > > - Currently this information is not exposed in any way to the clk core.
> > > > 
> > > > I recall from that video call that Stephen asked:
> > > > 
> > > > - to indeed not introduce a new op
> > > > - to evaluate the change from top to bottom, but to set it bottom to top
> > > > - to evaluate the rate by letting child clocks expose an array of the
> > > >   parent rates they would like, and to intersect all of them to figure
> > > >   out the best parent rate.
> > > > 
> > > > It looks like you followed none of these suggestions, so explaining why
> > > > you couldn't implement them would be a great first step.
> > > > 
> > > > Also, you sent an RFC, on what would you like a comment exactly?
> > > 
> > > Stephen asked me to not introduce a new clk op, however I don't see a
> > > clean way to do this any other way. Personally, I think that we need a
> > > new clk op for this use case for the reasons I outlined on the cover
> > > letter.
> > 
> > So his suggestion was to base the whole logic on clk_ops.determine_rate.
> > You're saying that it would violate parent/child abstraction. Can you
> > explain why you think that is the case, because it's really not obvious
> > to me.
> > 
> > Additionally, and assuming that we indeed need something similar to your
> > suggestion, determinate_rate takes a pointer to struct clk_rate_request.
> > Why did you choose to create a new op instead of adding the check_rate
> > pointer to clk_rate_request?
> 
> Sorry about the delayed response. I've been busy with other projects at
> work.
> 
> I attached a patch that puts the negotiate_rates member on struct
> clk_rate_request instead of struct clk_ops. In order to get this to
> work, it also required adding it to struct clk_core and
> struct clk_init_data as well. I made this so that this patch applies
> on top of this series.
> 
> I think the clk_rate_request approach is very ugly, and adding it to
> struct clk_ops like I have it in this series is the way to go.

I think I'm still confused about why we need negociate_rates in the
first place.

IIRC, Stephen's suggestion was something along the lines of "let's make
determine_rate optionally return a list of possible parent rates in
clk_rate_request and make the core find the intersection".

Why do we need to involve the driver in such a scenario, and using
negociate_rate in particular?

> I'm giving a talk at Linux Plumbers in Tokyo next month:
> 
>     Fixing Clock Tree Propagation in the Common Clk Framework 
>     https://lpc.events/event/19/contributions/2152/
> 
> Stephen will be there as well, and hopefully we can reach consensus
> about an acceptable approach to fix this.

Yeah, discussing it at plumbers would probably be a good idea, and maybe
try to record / transcribe the meeting so we can have the minutes too
somewhere?
Maxime

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

Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Brian Masney]

On Fri, Nov 21, 2025 at 11:09 AM Maxime Ripard <mripard@kernel.org> wrote:
> > I'm giving a talk at Linux Plumbers in Tokyo next month:
> >
> >     Fixing Clock Tree Propagation in the Common Clk Framework
> >     https://lpc.events/event/19/contributions/2152/
> >
> > Stephen will be there as well, and hopefully we can reach consensus
> > about an acceptable approach to fix this.
>
> Yeah, discussing it at plumbers would probably be a good idea, and maybe
> try to record / transcribe the meeting so we can have the minutes too
> somewhere?

The talk will be recorded, plus I'm sure there will be discussion
after my talk. I'll post a summary to this thread with the next steps
after Plumbers.

Brian

Follow up from Linux Plumbers about my clk rate change talk (was Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests)

[Brian Masney]

Hi all,

On Fri, Nov 21, 2025 at 03:35:10PM -0500, Brian Masney wrote:
> On Fri, Nov 21, 2025 at 11:09 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > I'm giving a talk at Linux Plumbers in Tokyo next month:
> > >
> > >     Fixing Clock Tree Propagation in the Common Clk Framework
> > >     https://lpc.events/event/19/contributions/2152/
> > >
> > > Stephen will be there as well, and hopefully we can reach consensus
> > > about an acceptable approach to fix this.
> >
> > Yeah, discussing it at plumbers would probably be a good idea, and maybe
> > try to record / transcribe the meeting so we can have the minutes too
> > somewhere?
> 
> The talk will be recorded, plus I'm sure there will be discussion
> after my talk. I'll post a summary to this thread with the next steps
> after Plumbers.

I presented a talk at Linux Plumbers in Tokyo last week about this patch
set:

    Fixing Clock Tree Propagation in the Common Clk Framework
    https://www.youtube.com/watch?v=R8TytDzlcFs
    https://lpc.events/event/19/contributions/2152/
    My slide deck is on the second link. I didn't present all of the
    slides.

In summary, I made the scope of this patch set too big, and this is an
intractable problem the way I currently have this setup. Some boards
are currently unknowingly dependent on the existing behavior, and we
need to keep things the way they currently are. We can detect when rates
are unknowingly changed by a sibling, and log a warning, however some
systems are configured to panic the system on warning, so we don't want
to go that route.

Most clock rates are fine to change with the existing behavior. It's
just some clocks that are dependent on a precise clock rate, such as DRM
and sound, that are affected by this.

The way forward on this is to leave the existing behavior in the clock
core, and fix this in the clock providers themselves. When a rate change
occurs, it can walk down that portion of the subtree inside the clk
provider via some new helpers that will be added to the clk core, and
ensure that the clocks that need precise rates will have their rates,
and their parent (if needed), updated as necessary. An array of rate
changes can be added to struct clk_rate_request, and the clk core can
update the clocks in that order. So it'll be up to the clk providers to
ensure that the array is populated in the correct order.

I'm going to get this working first in kunit, and I'll post a new
version of this patch set with these changes. Once that's done, I'll
work with Maxime and some other folks to find a board that has this
problem, and I'll ensure my new clk patch set is able to fix the issue.

Thank you to everyone that attended and provided feedback. Please reply
here if I missed something.


Separately, I talked to Stephen about ways that I can help him with clk
maintenance. Here's some information from my notes:

- I converted from round_rate to determine_rate() across most of the
  kernel tree in over 200 patches. However, the only remaining patch
  set is to the phy subsystem. The patches have received Reviewed-by's,
  however I haven't been able to get the phy maintainer to pick up the
  patches. Stephen mentioned he can pick them up. There was a merge
  conflict against the latest linux-next, so I posted a new version that
  addresses the merge conflict.

  https://lore.kernel.org/linux-clk/20251212-phy-clk-round-rate-v3-0-beae3962f767@redhat.com/T/#t

  Here's the patch set that actually removes round_rate() from the clk
  core.

  https://lore.kernel.org/linux-clk/20251212-clk-remove-round-rate-v1-0-5c3d5f3edc78@redhat.com/

  We still occasionally get people that try to add new round_rate
  implementations. I try to catch them when the patches are posted,
  however I miss some across the tree and will post a patch when it hits
  linux-next. 

  Someone two days ago posted a patch that adds a new round_rate(), so
  it'd ideally be nice to get my two patch sets above into linux-next to
  put a stop to this.

  https://lore.kernel.org/linux-clk/20251216-dr1v90-cru-v3-3-52cc938d1db0@pigmoral.tech/

  I commented on that patch to drop the round_rate.

- There are maybe a dozen or so determine_rate() implementations where
  it just returns 0, and it lets the firmware deal with setting the
  appropriate rate. Stephen suggested that we add a new flag to the
  clk core so that we can drop these empty determine_rate()
  implementations. We didn't talk about a name, but at the moment I'm
  leaning towards CLK_IS_FW_MANAGED. If that's set, then the clk core
  will not allow determine_rate to be set. I'm open to other
  suggestions for a name.

- I will continue to help review changes to the clk core. As for the
  clk providers themselves, I don't have access to the data sheets, and
  I can't really review those in detail. However, I think it would be
  nice if we add some extra clk-specific checks that we can run against
  patches. Look for common issues that come up in review. Look for cases
  where some of the helpers in clk-provider.h can possibly be used.
  Chen-Yu Tsai pointed out that's may not be entirely accurate in all
  cases, but we can at least warn about it.

  Another case I thought of is if someone posts a patch set where the
  clocks referenced in the dt bindings don't match what's actually in
  the clock provider itself.

  Make this as a clk-specific check script, and ideally see if we can
  hook that script into checkpatch.pl for clk-specific patches. My
  perl days are long over though, and I'd like to make the clk check
  script in python.

  Anyways, with a pre flight check like this, I could help review clk
  drivers themselves to look for anything else that's out of the
  ordinary.

It was good to meet so many people in person at Japan.

Brian