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.0Hi,
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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.