Re: [PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

[Brian Masney <bmasney@redhat.com>]

On Fri, Jun 06, 2025 at 10:56:57AM +0200, Maxime Ripard wrote:
> On Wed, May 28, 2025 at 07:16:49PM -0400, Brian Masney wrote:
> > Some of the mock tests care about the relationship between two
> > different rates, and the specific numbers are important, such as for
> > mocking a divider.
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> 
> It's not obvious to me why they are important, actually. The relation
> between the two is, but a divider (and our tests) should work with any
> parent rate, so I guess we can expect it to be opaque.

I agree as well.

> Can you expand on why it's important?

I personally find that having specific numbers in some (but not) of the
tests make the tests clearer that specific functionality within the clk
core is exercised. For example, assume we have a parent that can do any
rate, and two children that are dividers. We could have a test like the
following:

    clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_1);
    clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_2);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_1);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_2);
    /*
     * Make something to figure out what the ideal parent rate should be
     * and test that as well?
     */

So if we set child1 and child2 to 16 MHz and 32 MHz, then that exercises
one path through the clk core. However, it will currently fail if we set
the children to 32 MHz and 48 MHz. I have this working on a WIP branch
and one of my new tests looks similar to:

    clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_32_MHZ);
    clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_48_MHZ);
    // This should test that it's a multiple of 96 MHz
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_96_MHZ);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_32_MHZ);
    KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_48_MHZ);

Based on the work in my WIP branch, I think we need to make some of the
divider only clk tests parameterized, and have a table with various
specific frequencies so that various edge cases within the clk core are
tested by the frequency combinations.

I think that instead of having a list of DUMMY_CLOCK_RATE_XXX_MHZ
defines, a single define like this will suffice:

#define clk_dummy_rate_mhz(rate)      ((rate) * 1000 * 1000)

Brian