[PATCH v6 0/7] clk: add support for v1 / v2 clock rate negotiation and kunit tests

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

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

This series introduces kunit tests to illustrate the current behavior in
the clk core. As discussed at Linux Plumbers Conference 2025 in Tokyo,
it was suggested to move the negotiation logic into the clk providers
themselves so that only the clks with this issue can have their rate
preserved, and add some common helpers to the clk core.

How to test on real hardware
============================

I am interested in getting feedback about testing this patch on real
hardware, particularly for scenarios where CLK_SET_RATE_PARENT needs
to be removed like this:
https://lore.kernel.org/all/20260303115550.9279-1-mitltlatltl@gmail.com/

1) First ensure that your board uses divider_determine_rate() or
   divider_ro_determine_rate() from drivers/clk/clk-divider.c.

2) Boot the kernel with this patch set. There should be no change to the
   existing behavior, and everything should still function as is.

3) Now boot the kernel with the parameter clk_v2_rate_negotiation.
   Trigger the scenario where the clks get in a bad state, and it should
   not occur with this series.

Changes since v5:
https://lore.kernel.org/r/20260306-clk-scaling-v5-0-d21b84ee6f27@redhat.com
- Dropped all of the helpers, and complexity to the clk core. To solve
  this problem, we don't need to chain rate requests together.
- Add more unit tests
- Convert clk-divider.c so that there's a real user of this new API.
- This series no longer has the problem where large numbers of boards are
  moved over at once to the new v2 negotiation logic.

Changes since v4:
https://lore.kernel.org/linux-clk/20250923-clk-tests-docs-v4-0-9205cb3d3cba@redhat.com/
- Reworked based on feedback at Linux Plumbers [2] as described in two
  paragraphs above.
- Dropped gate and mux tests.

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/

[2] Fixing Clock Tree Propagation in the Common Clk Framework
    https://www.youtube.com/watch?v=R8TytDzlcFs
    https://lpc.events/event/19/contributions/2152/

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
Brian Masney (7):
      clk: test: introduce clk_dummy_div for a mock divider
      clk: test: introduce test suite for sibling rate changes on a divider
      clk: introduce new helper clk_hw_get_children_lcm() to calculate LCM of all child rates
      clk: test: introduce additional test case showing sibling clock rate change
      clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks
      clk: divider: enable optional support for v2 rate negotiation
      clk: test: introduce additional test case showing v2 rate change + LCM parent

 drivers/clk/clk-divider.c    |  28 +++-
 drivers/clk/clk.c            | 118 +++++++++++++++--
 drivers/clk/clk_test.c       | 300 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |   5 +
 4 files changed, 436 insertions(+), 15 deletions(-)
---
base-commit: 6a2abc8018be1ce9a03470ebb1c526f814903452
change-id: 20260305-clk-scaling-b3b63cae7624

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

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

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
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 a268d7b5d4cb28ec1f029f828c31107f8e130556..88e35f4419c958983578750356a97c0a45effb55 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -136,6 +136,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.53.0

[PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Brian Masney]

Introduce a kunit test suite that demonstrates the current behavior
of how a clock can unknowingly change the rate of it's siblings. Some
boards are unknowingly dependent on this behavior, and per discussions
at the 2025 Linux Plumbers Conference in Tokyo, we can't break the
existing behavior. So let's add kunit tests with the current behavior
so that we can be made aware if that functionality changes in the
future.

The tests in this commit use the following simplified clk tree with
the initial state:

                     parent
                     24 MHz
                    /      \
              child1        child2
              24 MHz        24 MHz

child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
set, and the parent is capable of achieving any rate.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 88e35f4419c958983578750356a97c0a45effb55..325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862 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"
@@ -652,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;
+};
+
+struct clk_rate_change_sibling_div_div_test_param {
+	const char *desc;
+	const struct clk_ops *ops;
+	unsigned int extra_child_flags;
+};
+
+static const struct clk_rate_change_sibling_div_div_test_param
+clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
+	{
+		.desc = "regular_ops",
+		.ops = &clk_dummy_div_ops,
+		.extra_child_flags = 0,
+	},
+};
+
+KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
+		       clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
+
+static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
+{
+	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
+	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,
+					     param->ops,
+					     CLK_SET_RATE_PARENT | param->extra_child_flags);
+	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,
+					     param->ops,
+					     CLK_SET_RATE_PARENT | param->extra_child_flags);
+	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 also affected. This preserves existing behavior in the clk
+ * core that some drivers may be unknowingly dependent on.
+ */
+static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/*
+	 * The last sibling rate change is the one that was successful, and
+	 * wins. The parent, and two children are all changed to 32 MHz. This
+	 * keeps the long-standing behavior of the clk core that some drivers
+	 * may be unknowingly dependent on.
+	 */
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), 32 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), 32 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child1.div, 1);
+	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), 32 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
+}
+
+static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_1,
+			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_2_v1,
+			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
+	{}
+};
+
+/*
+ * Test suite that creates a parent with two divider-only children, and
+ * documents the behavior of what happens to the sibling clock when one child
+ * changes its rate.
+ */
+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)
 {
@@ -3592,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.53.0

[PATCH v6 3/7] clk: introduce new helper clk_hw_get_children_lcm() to calculate LCM of all child rates

[Brian Masney]

Introduce a new helper that recursively walks through all children and
their descendants, calculating the lowest common multiple (LCM) of their
rates. For the requesting child, it uses the requested rate; for other
children, it uses their current rate. This is useful for determining
what parent rate can satisfy all children through simple integer
dividers.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c            | 48 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 47093cda9df32223c1120c3710261296027c4cd3..051fe755e3bf1b0a06db254b92f8a02889456db9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
+#include <linux/lcm.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -838,6 +839,53 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 }
 EXPORT_SYMBOL_GPL(clk_hw_set_rate_range);
 
+/**
+ * clk_hw_get_children_lcm - Calculate LCM of all children's rates recursively
+ * @hw: The parent clock hardware
+ * @requesting_hw: The child clock that is requesting a rate change (can be NULL)
+ * @requesting_rate: The target rate for the requesting clock
+ *
+ * This helper recursively walks through all children and their descendants,
+ * calculating the lowest common multiple (LCM) of their rates. For the
+ * requesting child, it uses the requested rate; for other children, it uses their
+ * current rate. This is useful for determining what parent rate can satisfy all
+ * children through simple integer dividers.
+ *
+ * Returns: The LCM of all non-zero rates found in the subtree, or 0 if no valid rates.
+ */
+unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesting_hw,
+				      unsigned long requesting_rate)
+{
+	unsigned long lcm_rate = 0;
+	unsigned long child_rate;
+	struct clk_core *child;
+
+	hlist_for_each_entry(child, &hw->core->children, child_node) {
+		/* Use requesting rate for the requesting child, current rate for others */
+		if (child->hw == requesting_hw)
+			child_rate = requesting_rate;
+		else
+			child_rate = clk_hw_get_rate(child->hw);
+
+		if (child_rate == 0)
+			continue;
+
+		if (lcm_rate == 0)
+			lcm_rate = child_rate;
+		else
+			lcm_rate = lcm(lcm_rate, child_rate);
+
+		/* Recursively get LCM of this child's children */
+		child_rate = clk_hw_get_children_lcm(child->hw, requesting_hw,
+						     requesting_rate);
+		if (child_rate > 0)
+			lcm_rate = lcm(lcm_rate, child_rate);
+	}
+
+	return lcm_rate;
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
+
 /*
  * __clk_mux_determine_rate - clk_ops::determine_rate implementation for a mux type clk
  * @hw: mux type clk to determine rate on
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 630705a47129453c241f1b1755f2c2f2a7ed8f77..2699b9759e13d0c1f0b54f4fa4f7f7bdd42e8dde 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1430,6 +1430,8 @@ void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
 			   unsigned long *max_rate);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
+unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesting_hw,
+				      unsigned long requesting_rate);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {

-- 
2.53.0

[PATCH v6 4/7] clk: test: introduce additional test case showing sibling clock rate change

[Brian Masney]

Add a test case where the parent clk rate is set to a rate that's
acceptable to both children, however the sibling clk rate is affected.

The tests in this commit use the following simplified clk tree with
the initial state:

                     parent
                     24 MHz
                    /      \
              child1        child2
              24 MHz        24 MHz

child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
set, and the parent is capable of achieving any rate.

child1 requests 32 MHz, and the tree should end up with the state:

                     parent
                     96 MHz
                    /      \
              child1        child2
              32 MHz        24 MHz

However, child2 ends up with it's parent rate due to the way the clk
core currently calculates handles rate changes.

                     parent
                     96 MHz
                    /      \
              child1        child2
              32 MHz        96 MHz
                            ^^^^^^
                            Incorrect. Should be 24 MHz.

Let's document this behavior with a kunit test since some boards are
unknowingly dependent on this behavior.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862..40bc01a0259d8d49ca4c1983b6c10a3684a95f0b 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -181,6 +181,26 @@ static const struct clk_ops clk_dummy_div_ops = {
 	.set_rate = clk_dummy_div_set_rate,
 };
 
+static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
+					    struct clk_rate_request *req)
+{
+	struct clk_hw *parent_hw = clk_hw_get_parent(hw);
+
+	if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && req->best_parent_rate < req->rate)
+		return -EINVAL;
+
+	req->best_parent_rate = clk_hw_get_children_lcm(parent_hw, hw, req->rate);
+	req->best_parent_hw = parent_hw;
+
+	return divider_determine_rate(hw, req, NULL, CLK_DUMMY_DIV_WIDTH, CLK_DUMMY_DIV_FLAGS);
+}
+
+static const struct clk_ops clk_dummy_div_lcm_ops = {
+	.recalc_rate = clk_dummy_div_recalc_rate,
+	.determine_rate = clk_dummy_div_lcm_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;
@@ -677,6 +697,18 @@ clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
 KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
 		       clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
 
+static const struct clk_rate_change_sibling_div_div_test_param
+clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
+	{
+		.desc = "lcm_ops_v1",
+		.ops = &clk_dummy_div_lcm_ops,
+		.extra_child_flags = 0,
+	},
+};
+
+KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
+		       clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
+
 static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 {
 	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
@@ -777,11 +809,40 @@ static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
 	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 also affected. This preserves existing behavior in the clk
+ * core that some drivers may be unknowingly dependent on.
+ */
+static void clk_test_rate_change_sibling_div_div_3_v1(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/*
+	 * With LCM-based coordinated rate changes, the parent should be at
+	 * 96 MHz (LCM of 32 and 24), child1 at 32 MHz (div=3), and child2
+	 * at 24 MHz (div=4). However, the clk core by default will clobber
+	 * the sibling clk rate, so the sibling gets the parent rate.
+	 */
+	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), 96 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
+}
+
 static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
 	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_1,
 			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
 	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_2_v1,
 			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_3_v1,
+			 clk_rate_change_sibling_div_div_test_lcm_ops_v1_gen_params),
 	{}
 };
 

-- 
2.53.0

[PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Brian Masney]

As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
can overwrite a sibling clk with the parent rate. Clocks that are used
for some subsystems like DRM and sound are particularly sensitive to
this issue.

I consider this to be a logic bug in the clk subsystem, however this
functionality has existed since the clk core was introduced with
commit b2476490ef11 ("clk: introduce the common clock framework"),
and some boards are unknowingly dependent on this behavior.

Let's add support for a v2 rate negotiation logic that addresses the
logic error. Clks can opt into this new behavior by adding the flag
CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
parameter clk_v2_rate_negotiation.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
 include/linux/clk-provider.h |  3 ++
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
 
+static bool clk_v2_rate_negotiation_enabled;
+static int __init clk_v2_rate_negotiation_setup(char *__unused)
+{
+	clk_v2_rate_negotiation_enabled = true;
+	return 1;
+}
+__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
+
+/**
+ * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
+ * @core: The clock core to check
+ *
+ * This function recursively checks if the clk or any of its descendants have
+ * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
+ * globally by adding clk_v2_rate_negotiation to the kernel command line.
+ *
+ * Returns: true if the v2 logic should be used; false otherwise
+ */
+bool clk_has_v2_rate_negotiation(const struct clk_core *core)
+{
+	struct clk_core *child;
+
+	if (clk_v2_rate_negotiation_enabled)
+		return true;
+
+	if (core->flags & CLK_V2_RATE_NEGOTIATION)
+		return true;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		if (clk_has_v2_rate_negotiation(child))
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
+
 /*
  * __clk_mux_determine_rate - clk_ops::determine_rate implementation for a mux type clk
  * @hw: mux type clk to determine rate on
@@ -2293,7 +2330,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;
 
@@ -2306,8 +2344,12 @@ 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_has_v2_rate_negotiation(child))
+			child->new_rate = child->rate;
+		else
+			child->new_rate = clk_recalc(child, new_rate);
+
+		clk_calc_subtree(child, child->new_rate, NULL, 0, initiating_clk);
 	}
 }
 
@@ -2316,7 +2358,8 @@ 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;
@@ -2364,7 +2407,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;
 	}
@@ -2389,10 +2432,10 @@ 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);
+		top = clk_calc_new_rates(parent, best_parent_rate, initiating_clk);
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	clk_calc_subtree(core, new_rate, parent, p_index, initiating_clk);
 
 	return top;
 }
@@ -2440,7 +2483,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;
@@ -2509,7 +2552,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
@@ -2519,12 +2562,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);
 }
@@ -2580,7 +2623,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;
 
@@ -2599,7 +2642,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:
@@ -3586,6 +3629,7 @@ static const struct {
 	ENTRY(CLK_IS_CRITICAL),
 	ENTRY(CLK_OPS_PARENT_ENABLE),
 	ENTRY(CLK_DUTY_CYCLE_PARENT),
+	ENTRY(CLK_V2_RATE_NEGOTIATION),
 #undef ENTRY
 };
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2699b9759e13d0c1f0b54f4fa4f7f7bdd42e8dde..e0fc0bd347e5920e999ac96dbed9fc247f9443fa 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -32,6 +32,8 @@
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
 /* duty cycle call may be forwarded to the parent clock */
 #define CLK_DUTY_CYCLE_PARENT	BIT(13)
+/* clock participates in v2 rate negotiation */
+#define CLK_V2_RATE_NEGOTIATION	BIT(14)
 
 struct clk;
 struct clk_hw;
@@ -1432,6 +1434,7 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
 unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesting_hw,
 				      unsigned long requesting_rate);
+bool clk_has_v2_rate_negotiation(const struct clk_core *core);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {

-- 
2.53.0

[PATCH v6 6/7] clk: divider: enable optional support for v2 rate negotiation

[Brian Masney]

If the v2 rate negotiation logic is enabled in this portion of the clk
subtree, then use the Lowest Common Multiple (LCM) of all of the child
rates to determine what the parent rate should be. Make this change
for clk_divider_bestdiv (used by clk_divider_determine_rate), and
divider_ro_determine_rate.

Note that the v2 rate negotiation logic is disabled by default, unless
a clk in this portion of the subtree has the flag
CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
parameter clk_v2_rate_negotiation.

This change was tested on a Thinkpad x13s laptop. Some clks used
this new code, however this needs to be tested on more real systems.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk-divider.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 45e7ebde4a8b4d6572aa9d867a6f12f6caf8aae4..17646a06420e099b5fa3ec99d92175356c65afa0 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -315,6 +315,19 @@ static int clk_divider_bestdiv(struct clk_hw *hw, struct clk_hw *parent,
 		return bestdiv;
 	}
 
+	if (clk_has_v2_rate_negotiation(parent->core)) {
+		unsigned long lcm_rate;
+
+		lcm_rate = clk_hw_get_children_lcm(parent, hw, rate);
+		if (lcm_rate > 0) {
+			*best_parent_rate = lcm_rate;
+			bestdiv = _div_round(table, lcm_rate, rate, flags);
+			bestdiv = bestdiv == 0 ? 1 : bestdiv;
+			bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv;
+			return bestdiv;
+		}
+	}
+
 	/*
 	 * The maximum divider we can use without overflowing
 	 * unsigned long in rate * i below
@@ -377,8 +390,19 @@ int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
 		if (!req->best_parent_hw)
 			return -EINVAL;
 
-		req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
-							  req->rate * div);
+		if (clk_has_v2_rate_negotiation(req->best_parent_hw->core)) {
+			unsigned long lcm_rate;
+
+			lcm_rate = clk_hw_get_children_lcm(req->best_parent_hw, hw, req->rate);
+			if (lcm_rate > 0)
+				req->best_parent_rate = lcm_rate;
+			else
+				req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
+									  req->rate * div);
+		} else {
+			req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
+								  req->rate * div);
+		}
 	}
 
 	req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);

-- 
2.53.0

[PATCH v6 7/7] clk: test: introduce additional test case showing v2 rate change + LCM parent

[Brian Masney]

Add a test case that uses the clk divider with the v2 rate negotiation
logic, plus the Lowest Common Multiple (LCM) to calculate the optimal
parent rate. The test ensures that the parent clk rate is set to a rate
that's acceptable to both children, and the sibling clock is not
affected.

The test in this commit use the following simplified clk tree with
the initial state:

                     parent
                     24 MHz
                    /      \
              child1        child2
              24 MHz        24 MHz

child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
set, and the parent is capable of achieving any rate.

child1 requests 32 MHz, and the tree ends up with the correct state:

                     parent
                     96 MHz
                    /      \
              child1        child2
              32 MHz        24 MHz
              div=3          div=4

Additionally, add a note about why clk_dummy_div_lcm_ops + friends are
still needed.

Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 40bc01a0259d8d49ca4c1983b6c10a3684a95f0b..00511afe5faa8b2631633ab796ece1bfe6944fb5 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -181,6 +181,14 @@ static const struct clk_ops clk_dummy_div_ops = {
 	.set_rate = clk_dummy_div_set_rate,
 };
 
+/*
+ * drivers/clk/clk-divider.c has support for v2 rate negotiation, and setting
+ * the parent based on the LCM, however we need to be able to test just setting
+ * the parent rate based on the LCM, and not set the v2 rate negotiation flag.
+ * This is to demonstrate existing functionality in the clk core, and ensure it
+ * stays the same. That's why we need to keep clk_dummy_div_lcm_ops, and can't
+ * just use clk_dummy_div_ops.
+ */
 static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
 					    struct clk_rate_request *req)
 {
@@ -709,6 +717,18 @@ clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
 KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
 		       clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
 
+static const struct clk_rate_change_sibling_div_div_test_param
+clk_rate_change_sibling_div_div_test_regular_ops_v2_params[] = {
+	{
+		.desc = "regular_ops_v2",
+		.ops = &clk_dummy_div_ops,
+		.extra_child_flags = CLK_V2_RATE_NEGOTIATION,
+	},
+};
+
+KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops_v2,
+		       clk_rate_change_sibling_div_div_test_regular_ops_v2_params, desc)
+
 static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 {
 	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
@@ -836,6 +856,31 @@ static void clk_test_rate_change_sibling_div_div_3_v1(struct kunit *test)
 	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, and maintains it's rate.
+ */
+static void clk_test_rate_change_sibling_div_div_4_v2(struct kunit *test)
+{
+	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
+	int ret;
+
+	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/*
+	 * With LCM-based parent + v2 rate changes, the parent should be at
+	 * 96 MHz (LCM of 32 and 24), child1 at 32 MHz (div=3), and child2
+	 * at 24 MHz (div=4).
+	 */
+	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), 24 * HZ_PER_MHZ);
+	KUNIT_EXPECT_EQ(test, ctx->child2.div, 4);
+}
+
 static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
 	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_1,
 			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
@@ -843,6 +888,10 @@ static struct kunit_case clk_rate_change_sibling_div_div_cases[] = {
 			 clk_rate_change_sibling_div_div_test_regular_ops_gen_params),
 	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_3_v1,
 			 clk_rate_change_sibling_div_div_test_lcm_ops_v1_gen_params),
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_1,
+			 clk_rate_change_sibling_div_div_test_regular_ops_v2_gen_params),
+	KUNIT_CASE_PARAM(clk_test_rate_change_sibling_div_div_4_v2,
+			 clk_rate_change_sibling_div_div_test_regular_ops_v2_gen_params),
 	{}
 };
 

-- 
2.53.0

Re: [PATCH v6 1/7] clk: test: introduce clk_dummy_div for a mock divider

[Maxime Ripard]

On Fri, 13 Mar 2026 12:43:08 -0400, Brian Masney wrote:
> This is used to mock up a divider in the clk kunit tests.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Maxime Ripard]

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

Hi,

On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> Introduce a kunit test suite that demonstrates the current behavior
> of how a clock can unknowingly change the rate of it's siblings. Some
> boards are unknowingly dependent on this behavior, and per discussions
> at the 2025 Linux Plumbers Conference in Tokyo, we can't break the
> existing behavior. So let's add kunit tests with the current behavior
> so that we can be made aware if that functionality changes in the
> future.
> 
> The tests in this commit use the following simplified clk tree with
> the initial state:
> 
>                      parent
>                      24 MHz
>                     /      \
>               child1        child2
>               24 MHz        24 MHz
> 
> child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> set, and the parent is capable of achieving any rate.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 88e35f4419c958983578750356a97c0a45effb55..325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862 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"
> @@ -652,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;
> +};
> +
> +struct clk_rate_change_sibling_div_div_test_param {
> +	const char *desc;
> +	const struct clk_ops *ops;
> +	unsigned int extra_child_flags;
> +};
> +
> +static const struct clk_rate_change_sibling_div_div_test_param
> +clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
> +	{
> +		.desc = "regular_ops",
> +		.ops = &clk_dummy_div_ops,
> +		.extra_child_flags = 0,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
> +		       clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
> +
> +static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> +{
> +	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> +	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,
> +					     param->ops,
> +					     CLK_SET_RATE_PARENT | param->extra_child_flags);
> +	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,
> +					     param->ops,
> +					     CLK_SET_RATE_PARENT | param->extra_child_flags);
> +	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);

I think we should move those expectations (assertions, really) to the
drivers. It will make it much clearer what the individual test relies on
and why it makes sense.

> +	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);
> +}

That's not something the clock framework guarantees at all.
divider_determine_rate does, but I'm not even sure it's something it
guarantees. It's not documented anywhere at least.

Plenty of drivers do not work that way though and will just forward
their rate request to the parent if CLK_SET_RATE_PARENT is set. Maybe
that's a problem of its own, idk.

Anyway, what I'm trying to say at least is that, at least, we shouldn't
frame it as a guarantee the framework provides, because it's really not
the case.

> +/*
> + * 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 also affected. This preserves existing behavior in the clk
> + * core that some drivers may be unknowingly dependent on.
> + */
> +static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
> +{
> +	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
> +	int ret;
> +
> +	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

Going back to my comment about the init assertions, here for example
this whole test only makes sense if the original rate wasn't equal to
32MHz, but it's not obvious if it is.

Maxime

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

Re: [PATCH v6 3/7] clk: introduce new helper clk_hw_get_children_lcm() to calculate LCM of all child rates

[Maxime Ripard]

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

Hi,

On Fri, Mar 13, 2026 at 12:43:10PM -0400, Brian Masney wrote:
> Introduce a new helper that recursively walks through all children and
> their descendants, calculating the lowest common multiple (LCM) of their
> rates. For the requesting child, it uses the requested rate; for other
> children, it uses their current rate. This is useful for determining
> what parent rate can satisfy all children through simple integer
> dividers.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>

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

Somewhat related: we probably want to do the same with the highest
common divider for multipliers.

Maxime

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

Re: [PATCH v6 4/7] clk: test: introduce additional test case showing sibling clock rate change

[Maxime Ripard]

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

On Fri, Mar 13, 2026 at 12:43:11PM -0400, Brian Masney wrote:
> Add a test case where the parent clk rate is set to a rate that's
> acceptable to both children, however the sibling clk rate is affected.
> 
> The tests in this commit use the following simplified clk tree with
> the initial state:
> 
>                      parent
>                      24 MHz
>                     /      \
>               child1        child2
>               24 MHz        24 MHz
> 
> child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> set, and the parent is capable of achieving any rate.
> 
> child1 requests 32 MHz, and the tree should end up with the state:
> 
>                      parent
>                      96 MHz
>                     /      \
>               child1        child2
>               32 MHz        24 MHz
> 
> However, child2 ends up with it's parent rate due to the way the clk
> core currently calculates handles rate changes.
> 
>                      parent
>                      96 MHz
>                     /      \
>               child1        child2
>               32 MHz        96 MHz
>                             ^^^^^^
>                             Incorrect. Should be 24 MHz.
> 
> Let's document this behavior with a kunit test since some boards are
> unknowingly dependent on this behavior.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862..40bc01a0259d8d49ca4c1983b6c10a3684a95f0b 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -181,6 +181,26 @@ static const struct clk_ops clk_dummy_div_ops = {
>  	.set_rate = clk_dummy_div_set_rate,
>  };
>  
> +static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
> +					    struct clk_rate_request *req)
> +{
> +	struct clk_hw *parent_hw = clk_hw_get_parent(hw);
> +
> +	if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && req->best_parent_rate < req->rate)
> +		return -EINVAL;
> +
> +	req->best_parent_rate = clk_hw_get_children_lcm(parent_hw, hw, req->rate);
> +	req->best_parent_hw = parent_hw;
> +
> +	return divider_determine_rate(hw, req, NULL, CLK_DUMMY_DIV_WIDTH, CLK_DUMMY_DIV_FLAGS);
> +}
> +
> +static const struct clk_ops clk_dummy_div_lcm_ops = {
> +	.recalc_rate = clk_dummy_div_recalc_rate,
> +	.determine_rate = clk_dummy_div_lcm_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;
> @@ -677,6 +697,18 @@ clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
>  KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
>  		       clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
>  
> +static const struct clk_rate_change_sibling_div_div_test_param
> +clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
> +	{
> +		.desc = "lcm_ops_v1",
> +		.ops = &clk_dummy_div_lcm_ops,
> +		.extra_child_flags = 0,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
> +		       clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
> +
>  static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
>  {
>  	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> @@ -777,11 +809,40 @@ static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
>  	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 also affected. This preserves existing behavior in the clk
> + * core that some drivers may be unknowingly dependent on.
> + */

This description is identical to the one for
clk_test_rate_change_sibling_div_div_2_v1(), so it's not clear to me
what the difference between those two tests are.

Maxime

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

Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Maxime Ripard]

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

On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> can overwrite a sibling clk with the parent rate. Clocks that are used
> for some subsystems like DRM and sound are particularly sensitive to
> this issue.
> 
> I consider this to be a logic bug in the clk subsystem, however this
> functionality has existed since the clk core was introduced with
> commit b2476490ef11 ("clk: introduce the common clock framework"),
> and some boards are unknowingly dependent on this behavior.
> 
> Let's add support for a v2 rate negotiation logic that addresses the
> logic error. Clks can opt into this new behavior by adding the flag
> CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> parameter clk_v2_rate_negotiation.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
>  include/linux/clk-provider.h |  3 ++
>  2 files changed, 60 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
>  
> +static bool clk_v2_rate_negotiation_enabled;
> +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> +{
> +	clk_v2_rate_negotiation_enabled = true;
> +	return 1;
> +}
> +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> +
> +/**
> + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> + * @core: The clock core to check
> + *
> + * This function recursively checks if the clk or any of its descendants have
> + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> + *
> + * Returns: true if the v2 logic should be used; false otherwise
> + */
> +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> +{
> +	struct clk_core *child;
> +
> +	if (clk_v2_rate_negotiation_enabled)
> +		return true;
> +
> +	if (core->flags & CLK_V2_RATE_NEGOTIATION)
> +		return true;
> +
> +	hlist_for_each_entry(child, &core->children, child_node) {
> +		if (clk_has_v2_rate_negotiation(child))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> +

Do we really need to export it? I'd expect it to be abstracted away for
consumers and providers?

Maxime

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

Re: [PATCH v6 6/7] clk: divider: enable optional support for v2 rate negotiation

[Maxime Ripard]

On Fri, 13 Mar 2026 12:43:13 -0400, Brian Masney wrote:
> If the v2 rate negotiation logic is enabled in this portion of the clk
> subtree, then use the Lowest Common Multiple (LCM) of all of the child
> rates to determine what the parent rate should be. Make this change
> for clk_divider_bestdiv (used by clk_divider_determine_rate), and
> divider_ro_determine_rate.
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH v6 7/7] clk: test: introduce additional test case showing v2 rate change + LCM parent

[Maxime Ripard]

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

On Fri, Mar 13, 2026 at 12:43:14PM -0400, Brian Masney wrote:
> Add a test case that uses the clk divider with the v2 rate negotiation
> logic, plus the Lowest Common Multiple (LCM) to calculate the optimal
> parent rate. The test ensures that the parent clk rate is set to a rate
> that's acceptable to both children, and the sibling clock is not
> affected.
> 
> The test in this commit use the following simplified clk tree with
> the initial state:
> 
>                      parent
>                      24 MHz
>                     /      \
>               child1        child2
>               24 MHz        24 MHz
> 
> child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> set, and the parent is capable of achieving any rate.
> 
> child1 requests 32 MHz, and the tree ends up with the correct state:
> 
>                      parent
>                      96 MHz
>                     /      \
>               child1        child2
>               32 MHz        24 MHz
>               div=3          div=4
> 
> Additionally, add a note about why clk_dummy_div_lcm_ops + friends are
> still needed.
> 
> Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 40bc01a0259d8d49ca4c1983b6c10a3684a95f0b..00511afe5faa8b2631633ab796ece1bfe6944fb5 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -181,6 +181,14 @@ static const struct clk_ops clk_dummy_div_ops = {
>  	.set_rate = clk_dummy_div_set_rate,
>  };
>  
> +/*
> + * drivers/clk/clk-divider.c has support for v2 rate negotiation, and setting
> + * the parent based on the LCM, however we need to be able to test just setting
> + * the parent rate based on the LCM, and not set the v2 rate negotiation flag.
> + * This is to demonstrate existing functionality in the clk core, and ensure it
> + * stays the same. That's why we need to keep clk_dummy_div_lcm_ops, and can't
> + * just use clk_dummy_div_ops.
> + */
>  static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
>  					    struct clk_rate_request *req)
>  {
> @@ -709,6 +717,18 @@ clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
>  KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
>  		       clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
>  
> +static const struct clk_rate_change_sibling_div_div_test_param
> +clk_rate_change_sibling_div_div_test_regular_ops_v2_params[] = {
> +	{
> +		.desc = "regular_ops_v2",
> +		.ops = &clk_dummy_div_ops,
> +		.extra_child_flags = CLK_V2_RATE_NEGOTIATION,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops_v2,
> +		       clk_rate_change_sibling_div_div_test_regular_ops_v2_params, desc)
> +
>  static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
>  {
>  	const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> @@ -836,6 +856,31 @@ static void clk_test_rate_change_sibling_div_div_3_v1(struct kunit *test)
>  	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, and maintains it's rate.
> + */

I assume that's only true if the v2 negociation is enabled?

> +static void clk_test_rate_change_sibling_div_div_4_v2(struct kunit *test)
> +{
> +	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
> +	int ret;
> +
> +	ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

If so, we should have an assertion here

Maxime
>

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

Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Brian Masney]

On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> > As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> > can overwrite a sibling clk with the parent rate. Clocks that are used
> > for some subsystems like DRM and sound are particularly sensitive to
> > this issue.
> >
> > I consider this to be a logic bug in the clk subsystem, however this
> > functionality has existed since the clk core was introduced with
> > commit b2476490ef11 ("clk: introduce the common clock framework"),
> > and some boards are unknowingly dependent on this behavior.
> >
> > Let's add support for a v2 rate negotiation logic that addresses the
> > logic error. Clks can opt into this new behavior by adding the flag
> > CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> > parameter clk_v2_rate_negotiation.
> >
> > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > Link: https://lpc.events/event/19/contributions/2152/
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
> >  include/linux/clk-provider.h |  3 ++
> >  2 files changed, 60 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
> >  }
> >  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
> >
> > +static bool clk_v2_rate_negotiation_enabled;
> > +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> > +{
> > +     clk_v2_rate_negotiation_enabled = true;
> > +     return 1;
> > +}
> > +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> > +
> > +/**
> > + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> > + * @core: The clock core to check
> > + *
> > + * This function recursively checks if the clk or any of its descendants have
> > + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> > + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> > + *
> > + * Returns: true if the v2 logic should be used; false otherwise
> > + */
> > +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> > +{
> > +     struct clk_core *child;
> > +
> > +     if (clk_v2_rate_negotiation_enabled)
> > +             return true;
> > +
> > +     if (core->flags & CLK_V2_RATE_NEGOTIATION)
> > +             return true;
> > +
> > +     hlist_for_each_entry(child, &core->children, child_node) {
> > +             if (clk_has_v2_rate_negotiation(child))
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> > +
>
> Do we really need to export it? I'd expect it to be abstracted away for
> consumers and providers?

This is abstracted away for the consumers, however the provider needs
to be aware if it wants to support the v2 logic. Patch 6 of this
series that adds support to clk-divider.c uses this export. Well
technically clk-divider.c is built with CONFIG_COMMON_CLK, however
other clk providers that want to use v2 logic will need this export.

The way I see it is that there are provider-specific things that need
to change if the v2 logic is used. For instance, the current behavior
of clk-divider.c is that when CLK_V2_RATE_NEGOTIATION is set, the
parent rate is just set to the new desired child rate, without taking
into account any of the sibling rates. We need to keep this behavior
for the v1 logic for existing boards. Moving this to the clk core
won't work since it doesn't know what kind of clock this is. Maybe
some need to compute the LCM, others may need the GCD, some may need
something else possibly. Some of the existing providers will need to
change to support this.

Now if we decided to not support the v1 logic, and just go all on on
v2 logic everywhere, then we won't need this export, and can just
update the providers.

Brian

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Brian Masney]

On Thu, Mar 19, 2026 at 5:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> > Introduce a kunit test suite that demonstrates the current behavior
> > of how a clock can unknowingly change the rate of it's siblings. Some
> > boards are unknowingly dependent on this behavior, and per discussions
> > at the 2025 Linux Plumbers Conference in Tokyo, we can't break the
> > existing behavior. So let's add kunit tests with the current behavior
> > so that we can be made aware if that functionality changes in the
> > future.
> >
> > The tests in this commit use the following simplified clk tree with
> > the initial state:
> >
> >                      parent
> >                      24 MHz
> >                     /      \
> >               child1        child2
> >               24 MHz        24 MHz
> >
> > child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> > set, and the parent is capable of achieving any rate.
> >
> > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > Link: https://lpc.events/event/19/contributions/2152/
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/clk/clk_test.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >
> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index 88e35f4419c958983578750356a97c0a45effb55..325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862 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"
> > @@ -652,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;
> > +};
> > +
> > +struct clk_rate_change_sibling_div_div_test_param {
> > +     const char *desc;
> > +     const struct clk_ops *ops;
> > +     unsigned int extra_child_flags;
> > +};
> > +
> > +static const struct clk_rate_change_sibling_div_div_test_param
> > +clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
> > +     {
> > +             .desc = "regular_ops",
> > +             .ops = &clk_dummy_div_ops,
> > +             .extra_child_flags = 0,
> > +     },
> > +};
> > +
> > +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
> > +                    clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
> > +
> > +static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> > +{
> > +     const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> > +     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,
> > +                                          param->ops,
> > +                                          CLK_SET_RATE_PARENT | param->extra_child_flags);
> > +     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,
> > +                                          param->ops,
> > +                                          CLK_SET_RATE_PARENT | param->extra_child_flags);
> > +     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);
>
> I think we should move those expectations (assertions, really) to the
> drivers. It will make it much clearer what the individual test relies on
> and why it makes sense.

Agreed. I will do that in the next version.

>
> > +     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);
> > +}
>
> That's not something the clock framework guarantees at all.
> divider_determine_rate does, but I'm not even sure it's something it
> guarantees. It's not documented anywhere at least.
>
> Plenty of drivers do not work that way though and will just forward
> their rate request to the parent if CLK_SET_RATE_PARENT is set. Maybe
> that's a problem of its own, idk.
>
> Anyway, what I'm trying to say at least is that, at least, we shouldn't
> frame it as a guarantee the framework provides, because it's really not
> the case.

I see what you are saying, however these are divider tests, and this
is the way that clk-divider works. I want to demonstrate that the clk
core is being called, and that ultimately the correct dividers are
computed. For example, on patch 7 of this series:

- Parent, child1 and child2 all start out at 24 MHz.
- child1 requests 32 MHz.
- Parent is changed to 96 MHz, child1 at 32 MHz, child2 stays at 24 MHz.

Child2 keeps the same rate, however the tests show that the clk is
actually updated since the divider is changed from 1 to 4 after this
operation. This is to simulate what would be programmed into a
register for real hardware.

I can drop the expects for the dividers if you really want in the next
version. Personally, I see value since these are divider-specific
tests.

Brian




>
> > +/*
> > + * 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 also affected. This preserves existing behavior in the clk
> > + * core that some drivers may be unknowingly dependent on.
> > + */
> > +static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
> > +{
> > +     struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
> > +     int ret;
> > +
> > +     ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
>
> Going back to my comment about the init assertions, here for example
> this whole test only makes sense if the original rate wasn't equal to
> 32MHz, but it's not obvious if it is.
>
> Maxime

Re: [PATCH v6 7/7] clk: test: introduce additional test case showing v2 rate change + LCM parent

[Brian Masney]

On Thu, Mar 19, 2026 at 5:43 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Mar 13, 2026 at 12:43:14PM -0400, Brian Masney wrote:
> > Add a test case that uses the clk divider with the v2 rate negotiation
> > logic, plus the Lowest Common Multiple (LCM) to calculate the optimal
> > parent rate. The test ensures that the parent clk rate is set to a rate
> > that's acceptable to both children, and the sibling clock is not
> > affected.
> >
> > The test in this commit use the following simplified clk tree with
> > the initial state:
> >
> >                      parent
> >                      24 MHz
> >                     /      \
> >               child1        child2
> >               24 MHz        24 MHz
> >
> > child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> > set, and the parent is capable of achieving any rate.
> >
> > child1 requests 32 MHz, and the tree ends up with the correct state:
> >
> >                      parent
> >                      96 MHz
> >                     /      \
> >               child1        child2
> >               32 MHz        24 MHz
> >               div=3          div=4
> >
> > Additionally, add a note about why clk_dummy_div_lcm_ops + friends are
> > still needed.
> >
> > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > Link: https://lpc.events/event/19/contributions/2152/
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/clk/clk_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index 40bc01a0259d8d49ca4c1983b6c10a3684a95f0b..00511afe5faa8b2631633ab796ece1bfe6944fb5 100644
> > --- a/drivers/clk/clk_test.c
> > +++ b/drivers/clk/clk_test.c
> > @@ -181,6 +181,14 @@ static const struct clk_ops clk_dummy_div_ops = {
> >       .set_rate = clk_dummy_div_set_rate,
> >  };
> >
> > +/*
> > + * drivers/clk/clk-divider.c has support for v2 rate negotiation, and setting
> > + * the parent based on the LCM, however we need to be able to test just setting
> > + * the parent rate based on the LCM, and not set the v2 rate negotiation flag.
> > + * This is to demonstrate existing functionality in the clk core, and ensure it
> > + * stays the same. That's why we need to keep clk_dummy_div_lcm_ops, and can't
> > + * just use clk_dummy_div_ops.
> > + */
> >  static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
> >                                           struct clk_rate_request *req)
> >  {
> > @@ -709,6 +717,18 @@ clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
> >  KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
> >                      clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
> >
> > +static const struct clk_rate_change_sibling_div_div_test_param
> > +clk_rate_change_sibling_div_div_test_regular_ops_v2_params[] = {
> > +     {
> > +             .desc = "regular_ops_v2",
> > +             .ops = &clk_dummy_div_ops,
> > +             .extra_child_flags = CLK_V2_RATE_NEGOTIATION,
> > +     },
> > +};
> > +
> > +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops_v2,
> > +                    clk_rate_change_sibling_div_div_test_regular_ops_v2_params, desc)
> > +
> >  static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> >  {
> >       const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> > @@ -836,6 +856,31 @@ static void clk_test_rate_change_sibling_div_div_3_v1(struct kunit *test)
> >       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, and maintains it's rate.
> > + */
>
> I assume that's only true if the v2 negociation is enabled?

Correct.

> > +static void clk_test_rate_change_sibling_div_div_4_v2(struct kunit *test)
> > +{
> > +     struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
> > +     int ret;
> > +
> > +     ret = clk_set_rate(ctx->child1_clk, 32 * HZ_PER_MHZ);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
>
> If so, we should have an assertion here

OK, I'll add that in the next version, and update the description.
I'll also add a similar assertion to the other tests.

Brian

Re: [PATCH v6 4/7] clk: test: introduce additional test case showing sibling clock rate change

[Brian Masney]

On Thu, Mar 19, 2026 at 5:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Mar 13, 2026 at 12:43:11PM -0400, Brian Masney wrote:
> > Add a test case where the parent clk rate is set to a rate that's
> > acceptable to both children, however the sibling clk rate is affected.
> >
> > The tests in this commit use the following simplified clk tree with
> > the initial state:
> >
> >                      parent
> >                      24 MHz
> >                     /      \
> >               child1        child2
> >               24 MHz        24 MHz
> >
> > child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> > set, and the parent is capable of achieving any rate.
> >
> > child1 requests 32 MHz, and the tree should end up with the state:
> >
> >                      parent
> >                      96 MHz
> >                     /      \
> >               child1        child2
> >               32 MHz        24 MHz
> >
> > However, child2 ends up with it's parent rate due to the way the clk
> > core currently calculates handles rate changes.
> >
> >                      parent
> >                      96 MHz
> >                     /      \
> >               child1        child2
> >               32 MHz        96 MHz
> >                             ^^^^^^
> >                             Incorrect. Should be 24 MHz.
> >
> > Let's document this behavior with a kunit test since some boards are
> > unknowingly dependent on this behavior.
> >
> > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > Link: https://lpc.events/event/19/contributions/2152/
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/clk/clk_test.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index 325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862..40bc01a0259d8d49ca4c1983b6c10a3684a95f0b 100644
> > --- a/drivers/clk/clk_test.c
> > +++ b/drivers/clk/clk_test.c
> > @@ -181,6 +181,26 @@ static const struct clk_ops clk_dummy_div_ops = {
> >       .set_rate = clk_dummy_div_set_rate,
> >  };
> >
> > +static int clk_dummy_div_lcm_determine_rate(struct clk_hw *hw,
> > +                                         struct clk_rate_request *req)
> > +{
> > +     struct clk_hw *parent_hw = clk_hw_get_parent(hw);
> > +
> > +     if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && req->best_parent_rate < req->rate)
> > +             return -EINVAL;
> > +
> > +     req->best_parent_rate = clk_hw_get_children_lcm(parent_hw, hw, req->rate);
> > +     req->best_parent_hw = parent_hw;
> > +
> > +     return divider_determine_rate(hw, req, NULL, CLK_DUMMY_DIV_WIDTH, CLK_DUMMY_DIV_FLAGS);
> > +}
> > +
> > +static const struct clk_ops clk_dummy_div_lcm_ops = {
> > +     .recalc_rate = clk_dummy_div_recalc_rate,
> > +     .determine_rate = clk_dummy_div_lcm_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;
> > @@ -677,6 +697,18 @@ clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
> >  KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
> >                      clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
> >
> > +static const struct clk_rate_change_sibling_div_div_test_param
> > +clk_rate_change_sibling_div_div_test_lcm_ops_v1_params[] = {
> > +     {
> > +             .desc = "lcm_ops_v1",
> > +             .ops = &clk_dummy_div_lcm_ops,
> > +             .extra_child_flags = 0,
> > +     },
> > +};
> > +
> > +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_lcm_ops_v1,
> > +                    clk_rate_change_sibling_div_div_test_lcm_ops_v1_params, desc)
> > +
> >  static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> >  {
> >       const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> > @@ -777,11 +809,40 @@ static void clk_test_rate_change_sibling_div_div_2_v1(struct kunit *test)
> >       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 also affected. This preserves existing behavior in the clk
> > + * core that some drivers may be unknowingly dependent on.
> > + */
>
> This description is identical to the one for
> clk_test_rate_change_sibling_div_div_2_v1(), so it's not clear to me
> what the difference between those two tests are.

I put this test here to demonstrate that there are actually two
separate items that need to be addressed in order to fix the clk
scaling issue. So basically in the series:

- Patch 2 demonstrates the current behavior of the clk core with
today's behavior with the v1 scaling.
- Patches 3/4 (This patch): introduce LCM for the parent rate, and a
test showing the current behavior with this one change. This
demonstrates that this change alone is not enough since the sibling
rate is squashed and set to the parent rate.
- Patches 5/6/7: add the v2 negotiation flag, along with a test
showing how the two changes together give the desired behavior.

I'll update the test description here.

Also, thank you for all of the feedback.

Brian


Brian

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Maxime Ripard]

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

On Thu, Mar 19, 2026 at 07:08:07AM -0400, Brian Masney wrote:
> On Thu, Mar 19, 2026 at 5:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> > > Introduce a kunit test suite that demonstrates the current behavior
> > > of how a clock can unknowingly change the rate of it's siblings. Some
> > > boards are unknowingly dependent on this behavior, and per discussions
> > > at the 2025 Linux Plumbers Conference in Tokyo, we can't break the
> > > existing behavior. So let's add kunit tests with the current behavior
> > > so that we can be made aware if that functionality changes in the
> > > future.
> > >
> > > The tests in this commit use the following simplified clk tree with
> > > the initial state:
> > >
> > >                      parent
> > >                      24 MHz
> > >                     /      \
> > >               child1        child2
> > >               24 MHz        24 MHz
> > >
> > > child1 and child2 both divider-only clocks that have CLK_SET_RATE_PARENT
> > > set, and the parent is capable of achieving any rate.
> > >
> > > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > > Link: https://lpc.events/event/19/contributions/2152/
> > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > ---
> > >  drivers/clk/clk_test.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > > index 88e35f4419c958983578750356a97c0a45effb55..325da7c84ab2ecdcf6b7a023ce4c2c4ef2d49862 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"
> > > @@ -652,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;
> > > +};
> > > +
> > > +struct clk_rate_change_sibling_div_div_test_param {
> > > +     const char *desc;
> > > +     const struct clk_ops *ops;
> > > +     unsigned int extra_child_flags;
> > > +};
> > > +
> > > +static const struct clk_rate_change_sibling_div_div_test_param
> > > +clk_rate_change_sibling_div_div_test_regular_ops_params[] = {
> > > +     {
> > > +             .desc = "regular_ops",
> > > +             .ops = &clk_dummy_div_ops,
> > > +             .extra_child_flags = 0,
> > > +     },
> > > +};
> > > +
> > > +KUNIT_ARRAY_PARAM_DESC(clk_rate_change_sibling_div_div_test_regular_ops,
> > > +                    clk_rate_change_sibling_div_div_test_regular_ops_params, desc)
> > > +
> > > +static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
> > > +{
> > > +     const struct clk_rate_change_sibling_div_div_test_param *param = test->param_value;
> > > +     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,
> > > +                                          param->ops,
> > > +                                          CLK_SET_RATE_PARENT | param->extra_child_flags);
> > > +     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,
> > > +                                          param->ops,
> > > +                                          CLK_SET_RATE_PARENT | param->extra_child_flags);
> > > +     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);
> >
> > I think we should move those expectations (assertions, really) to the
> > drivers. It will make it much clearer what the individual test relies on
> > and why it makes sense.
> 
> Agreed. I will do that in the next version.
> 
> >
> > > +     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);
> > > +}
> >
> > That's not something the clock framework guarantees at all.
> > divider_determine_rate does, but I'm not even sure it's something it
> > guarantees. It's not documented anywhere at least.
> >
> > Plenty of drivers do not work that way though and will just forward
> > their rate request to the parent if CLK_SET_RATE_PARENT is set. Maybe
> > that's a problem of its own, idk.
> >
> > Anyway, what I'm trying to say at least is that, at least, we shouldn't
> > frame it as a guarantee the framework provides, because it's really not
> > the case.
> 
> I see what you are saying, however these are divider tests, and this
> is the way that clk-divider works.

Yes, this is an undocumented behaviour of *clk-divider*. clk-divider is
not the only divider implementation. If anything, it's the reference
implementation, but that's pretty much it.

So when you say:

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

And

> I want to demonstrate that the clk core is being called, and that
> ultimately the correct dividers are computed.

This is only true for one implementation, and so far has been considered
an implementation detail. It's not something you can generalize.

And to make my point clearer, I wasn't saying this test shouldn't be
there, I was saying we shouldn't do and document that generalization.

> For example, on patch 7 of this series:
> 
> - Parent, child1 and child2 all start out at 24 MHz.
> - child1 requests 32 MHz.
> - Parent is changed to 96 MHz, child1 at 32 MHz, child2 stays at 24 MHz.
> 
> Child2 keeps the same rate, however the tests show that the clk is
> actually updated since the divider is changed from 1 to 4 after this
> operation. This is to simulate what would be programmed into a
> register for real hardware.
> 
> I can drop the expects for the dividers if you really want in the next
> version. Personally, I see value since these are divider-specific
> tests.

Not really, these tests are clk-divider tests, nothing more.

Maxime

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

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Brian Masney]

On Fri, Mar 20, 2026 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Thu, Mar 19, 2026 at 07:08:07AM -0400, Brian Masney wrote:
> > On Thu, Mar 19, 2026 at 5:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> > > Anyway, what I'm trying to say at least is that, at least, we shouldn't
> > > frame it as a guarantee the framework provides, because it's really not
> > > the case.
> >
> > I see what you are saying, however these are divider tests, and this
> > is the way that clk-divider works.
>
> Yes, this is an undocumented behaviour of *clk-divider*. clk-divider is
> not the only divider implementation. If anything, it's the reference
> implementation, but that's pretty much it.
>
> So when you say:
>
> > +/*
> > + * 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.
> > + */
>
> And
>
> > I want to demonstrate that the clk core is being called, and that
> > ultimately the correct dividers are computed.
>
> This is only true for one implementation, and so far has been considered
> an implementation detail. It's not something you can generalize.
>
> And to make my point clearer, I wasn't saying this test shouldn't be
> there, I was saying we shouldn't do and document that generalization.
>
> > For example, on patch 7 of this series:
> >
> > - Parent, child1 and child2 all start out at 24 MHz.
> > - child1 requests 32 MHz.
> > - Parent is changed to 96 MHz, child1 at 32 MHz, child2 stays at 24 MHz.
> >
> > Child2 keeps the same rate, however the tests show that the clk is
> > actually updated since the divider is changed from 1 to 4 after this
> > operation. This is to simulate what would be programmed into a
> > register for real hardware.
> >
> > I can drop the expects for the dividers if you really want in the next
> > version. Personally, I see value since these are divider-specific
> > tests.
>
> Not really, these tests are clk-divider tests, nothing more.

OK, I'll drop the checks for the actual dividers in the next version.

Brian

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Maxime Ripard]

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

On Fri, Mar 20, 2026 at 09:08:29AM -0400, Brian Masney wrote:
> On Fri, Mar 20, 2026 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Thu, Mar 19, 2026 at 07:08:07AM -0400, Brian Masney wrote:
> > > On Thu, Mar 19, 2026 at 5:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > > On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> > > > Anyway, what I'm trying to say at least is that, at least, we shouldn't
> > > > frame it as a guarantee the framework provides, because it's really not
> > > > the case.
> > >
> > > I see what you are saying, however these are divider tests, and this
> > > is the way that clk-divider works.
> >
> > Yes, this is an undocumented behaviour of *clk-divider*. clk-divider is
> > not the only divider implementation. If anything, it's the reference
> > implementation, but that's pretty much it.
> >
> > So when you say:
> >
> > > +/*
> > > + * 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.
> > > + */
> >
> > And
> >
> > > I want to demonstrate that the clk core is being called, and that
> > > ultimately the correct dividers are computed.
> >
> > This is only true for one implementation, and so far has been considered
> > an implementation detail. It's not something you can generalize.
> >
> > And to make my point clearer, I wasn't saying this test shouldn't be
> > there, I was saying we shouldn't do and document that generalization.
> >
> > > For example, on patch 7 of this series:
> > >
> > > - Parent, child1 and child2 all start out at 24 MHz.
> > > - child1 requests 32 MHz.
> > > - Parent is changed to 96 MHz, child1 at 32 MHz, child2 stays at 24 MHz.
> > >
> > > Child2 keeps the same rate, however the tests show that the clk is
> > > actually updated since the divider is changed from 1 to 4 after this
> > > operation. This is to simulate what would be programmed into a
> > > register for real hardware.
> > >
> > > I can drop the expects for the dividers if you really want in the next
> > > version. Personally, I see value since these are divider-specific
> > > tests.
> >
> > Not really, these tests are clk-divider tests, nothing more.
> 
> OK, I'll drop the checks for the actual dividers in the next version.

It really wasn't the point I was trying to make. It's fine to have that
test as a clk-divider test, but we should document it as just that,
nothing more.

Maxime

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

Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Maxime Ripard]

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

On Thu, Mar 19, 2026 at 06:35:56AM -0400, Brian Masney wrote:
> On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> > > As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> > > can overwrite a sibling clk with the parent rate. Clocks that are used
> > > for some subsystems like DRM and sound are particularly sensitive to
> > > this issue.
> > >
> > > I consider this to be a logic bug in the clk subsystem, however this
> > > functionality has existed since the clk core was introduced with
> > > commit b2476490ef11 ("clk: introduce the common clock framework"),
> > > and some boards are unknowingly dependent on this behavior.
> > >
> > > Let's add support for a v2 rate negotiation logic that addresses the
> > > logic error. Clks can opt into this new behavior by adding the flag
> > > CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> > > parameter clk_v2_rate_negotiation.
> > >
> > > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > > Link: https://lpc.events/event/19/contributions/2152/
> > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > ---
> > >  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
> > >  include/linux/clk-provider.h |  3 ++
> > >  2 files changed, 60 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
> > >
> > > +static bool clk_v2_rate_negotiation_enabled;
> > > +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> > > +{
> > > +     clk_v2_rate_negotiation_enabled = true;
> > > +     return 1;
> > > +}
> > > +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> > > +
> > > +/**
> > > + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> > > + * @core: The clock core to check
> > > + *
> > > + * This function recursively checks if the clk or any of its descendants have
> > > + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> > > + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> > > + *
> > > + * Returns: true if the v2 logic should be used; false otherwise
> > > + */
> > > +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> > > +{
> > > +     struct clk_core *child;
> > > +
> > > +     if (clk_v2_rate_negotiation_enabled)
> > > +             return true;
> > > +
> > > +     if (core->flags & CLK_V2_RATE_NEGOTIATION)
> > > +             return true;
> > > +
> > > +     hlist_for_each_entry(child, &core->children, child_node) {
> > > +             if (clk_has_v2_rate_negotiation(child))
> > > +                     return true;
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> > > +
> >
> > Do we really need to export it? I'd expect it to be abstracted away for
> > consumers and providers?
> 
> This is abstracted away for the consumers, however the provider needs
> to be aware if it wants to support the v2 logic. Patch 6 of this
> series that adds support to clk-divider.c uses this export. Well
> technically clk-divider.c is built with CONFIG_COMMON_CLK, however
> other clk providers that want to use v2 logic will need this export.
> 
> The way I see it is that there are provider-specific things that need
> to change if the v2 logic is used. For instance, the current behavior
> of clk-divider.c is that when CLK_V2_RATE_NEGOTIATION is set, the
> parent rate is just set to the new desired child rate, without taking
> into account any of the sibling rates. We need to keep this behavior
> for the v1 logic for existing boards. Moving this to the clk core
> won't work since it doesn't know what kind of clock this is. Maybe
> some need to compute the LCM, others may need the GCD, some may need
> something else possibly. Some of the existing providers will need to
> change to support this.
> 
> Now if we decided to not support the v1 logic, and just go all on on
> v2 logic everywhere, then we won't need this export, and can just
> update the providers.

Yeah... If we take a step back, in your previous version, we were
reworking the entire rate propagation logic, which was potentially
pretty hard to test and not easy to do a partial test and opt-in for.

With your current work, you have a clock flag that does the opt-in on a
clock by clock basis which makes it much easier to enable, and also
wouldn't create any unforeseen side-effects.

So I'm not sure we need the module parameter now.

Maxime

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

Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Maxime Ripard]

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

On Fri, Mar 20, 2026 at 03:31:41PM +0100, Maxime Ripard wrote:
> On Thu, Mar 19, 2026 at 06:35:56AM -0400, Brian Masney wrote:
> > On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> > > > As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> > > > can overwrite a sibling clk with the parent rate. Clocks that are used
> > > > for some subsystems like DRM and sound are particularly sensitive to
> > > > this issue.
> > > >
> > > > I consider this to be a logic bug in the clk subsystem, however this
> > > > functionality has existed since the clk core was introduced with
> > > > commit b2476490ef11 ("clk: introduce the common clock framework"),
> > > > and some boards are unknowingly dependent on this behavior.
> > > >
> > > > Let's add support for a v2 rate negotiation logic that addresses the
> > > > logic error. Clks can opt into this new behavior by adding the flag
> > > > CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> > > > parameter clk_v2_rate_negotiation.
> > > >
> > > > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > > > Link: https://lpc.events/event/19/contributions/2152/
> > > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > > ---
> > > >  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
> > > >  include/linux/clk-provider.h |  3 ++
> > > >  2 files changed, 60 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
> > > >
> > > > +static bool clk_v2_rate_negotiation_enabled;
> > > > +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> > > > +{
> > > > +     clk_v2_rate_negotiation_enabled = true;
> > > > +     return 1;
> > > > +}
> > > > +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> > > > +
> > > > +/**
> > > > + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> > > > + * @core: The clock core to check
> > > > + *
> > > > + * This function recursively checks if the clk or any of its descendants have
> > > > + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> > > > + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> > > > + *
> > > > + * Returns: true if the v2 logic should be used; false otherwise
> > > > + */
> > > > +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> > > > +{
> > > > +     struct clk_core *child;
> > > > +
> > > > +     if (clk_v2_rate_negotiation_enabled)
> > > > +             return true;
> > > > +
> > > > +     if (core->flags & CLK_V2_RATE_NEGOTIATION)
> > > > +             return true;
> > > > +
> > > > +     hlist_for_each_entry(child, &core->children, child_node) {
> > > > +             if (clk_has_v2_rate_negotiation(child))
> > > > +                     return true;
> > > > +     }
> > > > +
> > > > +     return false;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> > > > +
> > >
> > > Do we really need to export it? I'd expect it to be abstracted away for
> > > consumers and providers?
> > 
> > This is abstracted away for the consumers, however the provider needs
> > to be aware if it wants to support the v2 logic. Patch 6 of this
> > series that adds support to clk-divider.c uses this export. Well
> > technically clk-divider.c is built with CONFIG_COMMON_CLK, however
> > other clk providers that want to use v2 logic will need this export.
> > 
> > The way I see it is that there are provider-specific things that need
> > to change if the v2 logic is used. For instance, the current behavior
> > of clk-divider.c is that when CLK_V2_RATE_NEGOTIATION is set, the
> > parent rate is just set to the new desired child rate, without taking
> > into account any of the sibling rates. We need to keep this behavior
> > for the v1 logic for existing boards. Moving this to the clk core
> > won't work since it doesn't know what kind of clock this is. Maybe
> > some need to compute the LCM, others may need the GCD, some may need
> > something else possibly. Some of the existing providers will need to
> > change to support this.
> > 
> > Now if we decided to not support the v1 logic, and just go all on on
> > v2 logic everywhere, then we won't need this export, and can just
> > update the providers.
> 
> Yeah... If we take a step back, in your previous version, we were
> reworking the entire rate propagation logic, which was potentially
> pretty hard to test and not easy to do a partial test and opt-in for.
> 
> With your current work, you have a clock flag that does the opt-in on a
> clock by clock basis which makes it much easier to enable, and also
> wouldn't create any unforeseen side-effects.
> 
> So I'm not sure we need the module parameter now.

I'd even think it makes it *harder* to test since now you have to test
two combinations for each clock you convert instead of one (with one
being unlikely to happen). It's something that would be easily
overlooked, and	would receive less real-world testing as well.

Maxime

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

Re: [PATCH v6 2/7] clk: test: introduce test suite for sibling rate changes on a divider

[Brian Masney]

On Fri, Mar 20, 2026 at 10:29 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Mar 20, 2026 at 09:08:29AM -0400, Brian Masney wrote:
> > On Fri, Mar 20, 2026 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Thu, Mar 19, 2026 at 07:08:07AM -0400, Brian Masney wrote:
> > > > On Thu, Mar 19, 2026 at 5:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > > > On Fri, Mar 13, 2026 at 12:43:09PM -0400, Brian Masney wrote:
> > > > > Anyway, what I'm trying to say at least is that, at least, we shouldn't
> > > > > frame it as a guarantee the framework provides, because it's really not
> > > > > the case.
> > > >
> > > > I see what you are saying, however these are divider tests, and this
> > > > is the way that clk-divider works.
> > >
> > > Yes, this is an undocumented behaviour of *clk-divider*. clk-divider is
> > > not the only divider implementation. If anything, it's the reference
> > > implementation, but that's pretty much it.
> > >
> > > So when you say:
> > >
> > > > +/*
> > > > + * 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.
> > > > + */
> > >
> > > And
> > >
> > > > I want to demonstrate that the clk core is being called, and that
> > > > ultimately the correct dividers are computed.
> > >
> > > This is only true for one implementation, and so far has been considered
> > > an implementation detail. It's not something you can generalize.
> > >
> > > And to make my point clearer, I wasn't saying this test shouldn't be
> > > there, I was saying we shouldn't do and document that generalization.
> > >
> > > > For example, on patch 7 of this series:
> > > >
> > > > - Parent, child1 and child2 all start out at 24 MHz.
> > > > - child1 requests 32 MHz.
> > > > - Parent is changed to 96 MHz, child1 at 32 MHz, child2 stays at 24 MHz.
> > > >
> > > > Child2 keeps the same rate, however the tests show that the clk is
> > > > actually updated since the divider is changed from 1 to 4 after this
> > > > operation. This is to simulate what would be programmed into a
> > > > register for real hardware.
> > > >
> > > > I can drop the expects for the dividers if you really want in the next
> > > > version. Personally, I see value since these are divider-specific
> > > > tests.
> > >
> > > Not really, these tests are clk-divider tests, nothing more.
> >
> > OK, I'll drop the checks for the actual dividers in the next version.
>
> It really wasn't the point I was trying to make. It's fine to have that
> test as a clk-divider test, but we should document it as just that,
> nothing more.

I can move them out of clk_test.c and create a new clk-divider_test.c
file, and make it clear that these are tests against that specific
implementation of clk-divider.c. Would that work? There's already a
clk-fixed-rate_test.c file in tree.

Brian

Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Brian Masney]

On Fri, Mar 20, 2026 at 03:33:36PM +0100, Maxime Ripard wrote:
> On Fri, Mar 20, 2026 at 03:31:41PM +0100, Maxime Ripard wrote:
> > On Thu, Mar 19, 2026 at 06:35:56AM -0400, Brian Masney wrote:
> > > On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > > On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> > > > > As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> > > > > can overwrite a sibling clk with the parent rate. Clocks that are used
> > > > > for some subsystems like DRM and sound are particularly sensitive to
> > > > > this issue.
> > > > >
> > > > > I consider this to be a logic bug in the clk subsystem, however this
> > > > > functionality has existed since the clk core was introduced with
> > > > > commit b2476490ef11 ("clk: introduce the common clock framework"),
> > > > > and some boards are unknowingly dependent on this behavior.
> > > > >
> > > > > Let's add support for a v2 rate negotiation logic that addresses the
> > > > > logic error. Clks can opt into this new behavior by adding the flag
> > > > > CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> > > > > parameter clk_v2_rate_negotiation.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@redhat.com/
> > > > > Link: https://lpc.events/event/19/contributions/2152/
> > > > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > > > ---
> > > > >  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
> > > > >  include/linux/clk-provider.h |  3 ++
> > > > >  2 files changed, 60 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
> > > > >
> > > > > +static bool clk_v2_rate_negotiation_enabled;
> > > > > +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> > > > > +{
> > > > > +     clk_v2_rate_negotiation_enabled = true;
> > > > > +     return 1;
> > > > > +}
> > > > > +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> > > > > +
> > > > > +/**
> > > > > + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> > > > > + * @core: The clock core to check
> > > > > + *
> > > > > + * This function recursively checks if the clk or any of its descendants have
> > > > > + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> > > > > + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> > > > > + *
> > > > > + * Returns: true if the v2 logic should be used; false otherwise
> > > > > + */
> > > > > +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> > > > > +{
> > > > > +     struct clk_core *child;
> > > > > +
> > > > > +     if (clk_v2_rate_negotiation_enabled)
> > > > > +             return true;
> > > > > +
> > > > > +     if (core->flags & CLK_V2_RATE_NEGOTIATION)
> > > > > +             return true;
> > > > > +
> > > > > +     hlist_for_each_entry(child, &core->children, child_node) {
> > > > > +             if (clk_has_v2_rate_negotiation(child))
> > > > > +                     return true;
> > > > > +     }
> > > > > +
> > > > > +     return false;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> > > > > +
> > > >
> > > > Do we really need to export it? I'd expect it to be abstracted away for
> > > > consumers and providers?
> > > 
> > > This is abstracted away for the consumers, however the provider needs
> > > to be aware if it wants to support the v2 logic. Patch 6 of this
> > > series that adds support to clk-divider.c uses this export. Well
> > > technically clk-divider.c is built with CONFIG_COMMON_CLK, however
> > > other clk providers that want to use v2 logic will need this export.
> > > 
> > > The way I see it is that there are provider-specific things that need
> > > to change if the v2 logic is used. For instance, the current behavior
> > > of clk-divider.c is that when CLK_V2_RATE_NEGOTIATION is set, the
> > > parent rate is just set to the new desired child rate, without taking
> > > into account any of the sibling rates. We need to keep this behavior
> > > for the v1 logic for existing boards. Moving this to the clk core
> > > won't work since it doesn't know what kind of clock this is. Maybe
> > > some need to compute the LCM, others may need the GCD, some may need
> > > something else possibly. Some of the existing providers will need to
> > > change to support this.
> > > 
> > > Now if we decided to not support the v1 logic, and just go all on on
> > > v2 logic everywhere, then we won't need this export, and can just
> > > update the providers.
> > 
> > Yeah... If we take a step back, in your previous version, we were
> > reworking the entire rate propagation logic, which was potentially
> > pretty hard to test and not easy to do a partial test and opt-in for.
> > 
> > With your current work, you have a clock flag that does the opt-in on a
> > clock by clock basis which makes it much easier to enable, and also
> > wouldn't create any unforeseen side-effects.
> > 
> > So I'm not sure we need the module parameter now.
> 
> I'd even think it makes it *harder* to test since now you have to test
> two combinations for each clock you convert instead of one (with one
> being unlikely to happen). It's something that would be easily
> overlooked, and	would receive less real-world testing as well.

That's a good point. The only reason I included the module parameter was
for testing purposes. If a board happens to use clk-divider, and has the
clk scaling issue, then we could ask them to boot the kernel with this
parameter to see if it addresses the problem.

I'll omit the kernel parameter from the next version. If we want someone
to test this, we can just have them add the clk flag to the relevant
part(s) of the clk tree.

Brian