Re: [PATCH v2 2/2] clk: divider: Add some kunit test suites

[Lad, Prabhakar]

Hi Brian,

Thank you for the review.

On Thu, Apr 16, 2026 at 10:35 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>
> Hi Lad,
>
> On Mon, Apr 13, 2026 at 01:49:12PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add KUnit tests to verify clk_divider_bestdiv() returns the maximum
> > achievable rate when clk_round_rate() is called with ULONG_MAX, which
> > is the canonical way to probe the maximum rate a clock can produce.
> >
> > The first test uses a fixed-rate parent driving a table-based divider
> > with no div=1 entry. The second test places a two-input mux between
> > the divider and its root clocks to verify correct parent selection and
> > that the divider loop does not make redundant calls to
> > clk_hw_round_rate() for each remaining table entry after the first
> > overflow.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/clk/Kconfig            |   7 ++
> >  drivers/clk/Makefile           |   1 +
> >  drivers/clk/clk-divider_test.c | 151 +++++++++++++++++++++++++++++++++
> >  3 files changed, 159 insertions(+)
> >  create mode 100644 drivers/clk/clk-divider_test.c
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index cc8743b11bb1..c8f9eaef6f6b 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -573,4 +573,11 @@ config CLK_FD_KUNIT_TEST
> >       help
> >         Kunit test for the clk-fractional-divider type.
> >
> > +config CLK_DIVIDER_KUNIT_TEST
> > +     tristate "KUnit tests for clk divider bestdiv" if !KUNIT_ALL_TESTS
> > +     depends on KUNIT
>
> Since the clk divider calls writel(), you also will need to
> unfortunately add:
>
>     depends on !S390
>
Ok.

> This is already on CLK_GATE_KUNIT_TEST. For the reason why, look at
> commit a6c3da03ead11 ("clk: disable clk gate tests for s390")
>
Thank you for the pointer.

> > +     default KUNIT_ALL_TESTS
> > +     help
> > +       Kunit test for the clk-divider type.
> > +
> >  endif
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index a3e2862ebd7e..0c915c6cf3fa 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -20,6 +20,7 @@ clk-test-y                  := clk_test.o \
> >                                  kunit_clk_assigned_rates_zero_consumer.dtbo.o \
> >                                  kunit_clk_hw_get_dev_of_node.dtbo.o \
> >                                  kunit_clk_parent_data_test.dtbo.o
> > +obj-$(CONFIG_CLK_DIVIDER_KUNIT_TEST) += clk-divider_test.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-divider.o
>
> Swap the order of these two lines above for consistency with the
> clk-fixed-rate and clk-gate tests where the actual implementation is
> first, and then the test.
>
Ok.

> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-factor.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
> > diff --git a/drivers/clk/clk-divider_test.c b/drivers/clk/clk-divider_test.c
> > new file mode 100644
> > index 000000000000..3a5e3adccb2e
> > --- /dev/null
> > +++ b/drivers/clk/clk-divider_test.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit tests for clk_divider_bestdiv()
> > + */
> > +#include <kunit/test.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/limits.h>
> > +#include <linux/units.h>
> > +
> > +#define PARENT_RATE_1GHZ     GIGA
> > +#define PARENT_RATE_2GHZ     (2 * GIGA)
> > +#define PARENT_RATE_4GHZ     (4 * GIGA)
> > +
> > +static u32 fake_reg_a, fake_reg_b;
>
> Right now this limits this to one implementation. Put these in a
> structure and use kunit_kzalloc() so that there can be multiple, and the
> runner can execute them in parallel.
>
Ok.

> > +
> > +static const struct clk_div_table no_div1_table[] = {
> > +     {0, 2},
> > +     {1, 4},
> > +     {2, 8},
> > +     {0, 0},
> > +};
>
> You can pass NULL for the table to simplify this code further. I don't
> see where you are testing anything special related to the table. I think
> you'll need to pass CLK_DIVIDER_ONE_BASED to the flags when you create
> the divider if you use a NULL table.
>
Agreed.

> > +
> > +static void unregister_fixed_rate(void *hw)
> > +{
> > +     clk_hw_unregister_fixed_rate(hw);
> > +}
> > +
> > +static void unregister_divider(void *hw)
> > +{
> > +     clk_hw_unregister_divider(hw);
> > +}
> > +
> > +static void unregister_mux(void *hw)
> > +{
> > +     clk_hw_unregister_mux(hw);
> > +}
> > +
> > +/*
> > + * Test that clk_round_rate(clk, ULONG_MAX) returns the maximum achievable
> > + * rate for a divider clock.
> > + */
> > +static void clk_divider_bestdiv_ulong_max_returns_max_rate(struct kunit *test)
> > +{
> > +     struct clk_hw *parent_hw, *div_hw;
> > +     unsigned long rate;
> > +
> > +     parent_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-parent",
> > +                                            NULL, 0, PARENT_RATE_1GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
> > +     kunit_add_action(test, unregister_fixed_rate, parent_hw);
>
> You can put clk_hw_unregister_fixed_rate() in the call here, and then
> drop unregister_fixed_rate(). There's some cases of this below as well.
>
> Check the return value of kunit_add_action() here and below as well.
>
Or maybe have something like the following (while keeping the wrappers)?

KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test,
unregister_divider, div_hw));


> > +
> > +     fake_reg_a = 0;
> > +     div_hw = clk_hw_register_divider_table(NULL, "bestdiv-div",
> > +                                            "bestdiv-parent",
> > +                                            CLK_SET_RATE_PARENT,
> > +                                            (void __iomem *)&fake_reg_a,
>
> You'll need __force for the cast for sparse as well.
>
Agreed.

> > +                                            0, 2, 0, no_div1_table, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, div_hw);
> > +     kunit_add_action(test, unregister_divider, div_hw);
>
> Same here... you can just put clk_hw_unregister_divider() here and drop
> the function above.
>
> > +
> > +     /*
> > +      * ULONG_MAX is the canonical way to probe the maximum rate a clock
> > +      * can produce. With a parent at 1 GHz and the smallest table divider
> > +      * being 2, the expected maximum is 500 MHz.
> > +      *
> > +      * Before the fix this returned 125 MHz (PARENT_RATE / 8), the
> > +      * minimum rate, because the search loop was bypassed entirely.
>
> The "Before the fix" comment should go in the commit log. The comment in
> the code should describe how the code is right now.
>
Ok.

> > +      */
> > +     rate = clk_hw_round_rate(div_hw, ULONG_MAX);
> > +     KUNIT_EXPECT_EQ(test, rate, PARENT_RATE_1GHZ / 2);
> > +}
> > +
> > +/*
> > + * Test that clk_round_rate(clk, ULONG_MAX) returns the correct maximum rate when
> > + * a mux clock sits between a divider and its parent candidates.
> > + *
> > + * Topology:
> > + *
> > + *   [fixed 4 GHz] --\
> > + *                    +--> [mux CLK_SET_RATE_PARENT] --> [div {2,4,8} CLK_SET_RATE_PARENT]
> > + *   [fixed 2 GHz] --/
> > + *
> > + */
> > +static void clk_divider_bestdiv_mux_ulong_max_returns_max_rate(struct kunit *test)
> > +{
> > +     static const char *const mux_parents[] = {
> > +             "bestdiv-mux-parent-a",
> > +             "bestdiv-mux-parent-b",
> > +     };
> > +     struct clk_hw *parent_a_hw, *parent_b_hw, *mux_hw, *div_hw;
> > +     unsigned long rate;
> > +
> > +     /* Higher-rate parent: the mux should select this for ULONG_MAX. */
> > +     parent_a_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-mux-parent-a",
> > +                                              NULL, 0, PARENT_RATE_4GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_a_hw);
> > +     kunit_add_action(test, unregister_fixed_rate, parent_a_hw);
> > +
> > +     /* Lower-rate parent: should not be selected. */
> > +     parent_b_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-mux-parent-b",
> > +                                              NULL, 0, PARENT_RATE_2GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_b_hw);
> > +     kunit_add_action(test, unregister_fixed_rate, parent_b_hw);
> > +
> > +     /*
> > +      * 1-bit mux register selects between the two parents.
> > +      * CLK_SET_RATE_PARENT allows the divider's rate request to
> > +      * propagate into clk_mux_determine_rate().
> > +      */
> > +     fake_reg_a = 0;
> > +     mux_hw = clk_hw_register_mux(NULL, "bestdiv-mux",
> > +                                  mux_parents, ARRAY_SIZE(mux_parents),
> > +                                  CLK_SET_RATE_PARENT,
> > +                                  (void __iomem *)&fake_reg_a,
> > +                                  0, 1, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mux_hw);
> > +     kunit_add_action(test, unregister_mux, mux_hw);
>
> You can put clk_hw_unregister_mux() here and drop this function above.
>
> > +
> > +     fake_reg_b = 0;
> > +     div_hw = clk_hw_register_divider_table(NULL, "bestdiv-mux-div",
> > +                                            "bestdiv-mux",
> > +                                            CLK_SET_RATE_PARENT,
> > +                                            (void __iomem *)&fake_reg_b,
> > +                                            0, 2, 0, no_div1_table, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, div_hw);
> > +     kunit_add_action(test, unregister_divider, div_hw);
> > +
> > +     /*
> > +      * Expected maximum: mux selects the 4 GHz parent, divider applies
> > +      * the smallest table entry (2): 4 GHz / 2 = 2 GHz.
> > +      */
> > +     rate = clk_hw_round_rate(div_hw, ULONG_MAX);
> > +     KUNIT_EXPECT_EQ(test, rate, PARENT_RATE_4GHZ / 2);
> > +}
> > +
> > +static struct kunit_case clk_divider_bestdiv_test_cases[] = {
> > +     KUNIT_CASE(clk_divider_bestdiv_ulong_max_returns_max_rate),
> > +     KUNIT_CASE(clk_divider_bestdiv_mux_ulong_max_returns_max_rate),
> > +     {}
> > +};
>
> Usually I'd ask for a few other tests for basic functionality to be
> added rather than just testing the maximum. However there's actually
> some stuff broken with the existing dividers and the clk core where a
> clk can change the rate of it's siblings. I have a series to address
> this at:
>
> https://lore.kernel.org/linux-clk/20260327-clk-scaling-v8-0-86cd0aba3c5f@xxxxxxxxxx/
>
> I think the tests that you have are fine.
>
Thanks for the context, that makes sense.

Cheers,
Prabhakar

>
> Stephen: If you have time after the merge window closes I'd appreciate
> it if you could take time to provide feedback about that series.
>
> Puss in Boots please... :)
> https://media2.giphy.com/media/v1.Y2lkPTc5MGI3NjExaXg5cGFhbGdmbTZjdnRkdWs4aXM5d3FlYnFmbTNudWFsZ3Fwc3o5NiZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/qUIm5wu6LAAog/giphy.gif
>
> Brian
>