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

[Brian Masney]

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

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

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

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

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

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

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

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

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

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