Re: [PATCH v2 07/10] clk: test: introduce test suite for sibling rate changes on a gate

[Maxime Ripard <mripard@kernel.org>]

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

On Wed, May 28, 2025 at 07:16:53PM -0400, Brian Masney wrote:
> Introduce a test suite that creates a parent with two children: a
> divider and a gate. Ensure that changing the rate of one child does
> not affect the rate of the gate.
> 
> Some of the tests are disabled until the relevant issue(s) are fixed in
> the clk core. This is also implemented as a parameterized kunit test
> since additional test variations will be added.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk_test.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 156 insertions(+)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index c2337527873d3241e7b0a38f67ecaa13535bcc71..1440eb3c41def8c549f92c0e95b2a472f3bdb4a7 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -825,6 +825,161 @@ static struct kunit_suite clk_rate_change_sibling_div_div_test_suite = {
>  	.test_cases = clk_rate_change_sibling_div_div_cases,
>  };
>  
> +struct clk_test_rate_change_sibling_clk_ctx {
> +	struct clk *parent_clk, *child1_clk, *child2_clk;
> +};
> +
> +static void
> +clk_test_rate_change_sibling_clk_ctx_put(struct clk_test_rate_change_sibling_clk_ctx *clk_ctx)
> +{
> +	clk_put(clk_ctx->parent_clk);
> +	clk_put(clk_ctx->child1_clk);
> +	clk_put(clk_ctx->child2_clk);
> +}
> +
> +struct clk_rate_change_sibling_div_gate_sibling_context {
> +	struct clk_dummy_context parent;
> +	struct clk_dummy_div child1;
> +	struct clk_dummy_gate child2;
> +	struct clk_test_rate_change_sibling_clk_ctx clk_ctx;
> +};
> +
> +static struct clk_test_rate_change_sibling_clk_ctx *
> +clk_rate_change_sibling_div_gate_test_init(struct kunit *test)
> +{
> +	struct clk_rate_change_sibling_div_gate_sibling_context *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return ERR_PTR(-ENOMEM);
> +	test->priv = ctx;
> +
> +	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
> +	ctx->parent.rate = DUMMY_CLOCK_RATE_24_MHZ;
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw,
> +					     &clk_dummy_div_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw,
> +					     &clk_dummy_gate_ops,
> +					     CLK_SET_RATE_PARENT);
> +	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ctx->clk_ctx.parent_clk = clk_hw_get_clk(&ctx->parent.hw, NULL);
> +	ctx->clk_ctx.child1_clk = clk_hw_get_clk(&ctx->child1.hw, NULL);
> +	ctx->clk_ctx.child2_clk = clk_hw_get_clk(&ctx->child2.hw, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->clk_ctx.parent_clk),
> +			DUMMY_CLOCK_RATE_24_MHZ);

EXPECT is for the expected output of the test. It looks to me that
you're are here checking if the test is properly setup, which would be
an assertion.

> +	return &ctx->clk_ctx;
> +}
> +
> +struct clk_test_rate_change_sibling_test_case {
> +	const char *desc;
> +	struct clk_test_rate_change_sibling_clk_ctx *(*init)(struct kunit *test);
> +};
> +
> +static struct clk_test_rate_change_sibling_test_case clk_test_rate_change_sibling_test_cases[] = {
> +	{
> +		.desc = "div_gate",
> +		.init = clk_rate_change_sibling_div_gate_test_init,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(clk_test_rate_change_sibling_test_case,
> +		       clk_test_rate_change_sibling_test_cases, desc);

I'm not sure making them parameterized is a good idea (yet), I tend to
think that the more straightforward the tests are the better. That can
indeed lead to repetitions, but it's also much easier to debug once we
get a test failure.

> +
> +/*
> + * Test that, for a parent with two children and one requests a rate change that
> + * requires a change to the parent rate, the sibling rates are not affected.
> + */
> +static void clk_test_rate_change_sibling_1(struct kunit *test)
> +{
> +	struct clk_test_rate_change_sibling_test_case *testcase =
> +		(struct clk_test_rate_change_sibling_test_case *) test->param_value;
> +	struct clk_test_rate_change_sibling_clk_ctx *ctx;
> +	int ret;
> +
> +	kunit_skip(test, "This needs to be fixed in the core.");
> +
> +	ctx = testcase->init(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
> +
> +	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_GE(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_48_MHZ);
> +	KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);

And also, we wouldn't have the same expectations between a gate like
here, and a mux (that can reparent), so sharing the code isn't going to
be trivial.

> +	clk_test_rate_change_sibling_clk_ctx_put(ctx);

This won't be run if you hit any KUNIT_ASSERT_*() conditions. We should
probably create a kunit-managed clk_hw_get() variant so we don't have to
deal with this.

Maxime

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