Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Stephen Boyd]

Quoting Yang Xiwen (2023-05-03 11:44:45)
> On 5/3/2023 11:08 AM, Stephen Boyd wrote:
> > Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
> >> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> >> index f9a5c2964c65d..4f7f9a964637a 100644
> >> --- a/drivers/clk/clk_test.c
> >> +++ b/drivers/clk/clk_test.c
> >> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
> >>   * parent, the rate request structure returned by __clk_determine_rate
> >>   * is sane and will be what we expect.
> >>   */
> >> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
> > 
> > Just leave this one alone and put the other test cases right after it.
> > Don't rename it and also move it lower down. It makes the diff hard to
> > read.
> I don't quite understand. In this patch, I renamed it to case3. Here I
> think you'd like it to remain as-is. But I think the comments below said
> I should rename it to clk_leaf_mux_determine_rate_exactly_parent1()?
> Actually what this test has done should be included in this series of
> test cases as one of them.

You can rename it, but don't move it lower in the file.

[...]
> >> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
> > 
> > I'm not sure I understand what is being tested in this case. Are we
> > testing that __clk_determine_rate() with a rate between parent0 and
> > parent1 picks parent1?
> I think I have to speak more about how these tests are arranged.

The order that tests run in should not be relied upon. The tests should
be hermetic.

> Imagine
> that there is a segment starting at 0, ending at ULONG_MAX. Now add two
> points on it, assuming DUMMY_CLOCK_RATE_1(142MHz) and
> DUMMY_CLOCK_RATE_2(242MHz). We split the segment into 3 segments, and we
> have 4 endpoints in total. They are: 0, 0-142MHz, 142MHz, 142MHz-242MHz,
> 242MHz-ULONG_MAX, ULONG_MAX. Ideally, we need to test all these 7 cases.
> For those 4 points, the tests should be straightforward. But for the 3
> segments, we can only extract a random point on it to test. Besides, I
> think requesting for 142MHz or 242MHz has no big difference, so I
> omitted one of them. The original
> clk_leaf_mux_set_rate_parent_determine_rate() is considered to be one of
> the cases here, for which I think should be absorbed and renamed.

Got it. Adding various tests is OK. Hard-coding a rate, instead of doing
math makes it easier to read. Dividing by 2 threw me off because I was
trying to figure out why it was important.

I'll note that you're basically testing the rounding implementation of
the clk's determine_rate clk_op though, which may be different from what
this test suite is testing. The test suite talks about making sure the
struct clk_rate_request is correct. Maybe the suite should be renamed to
"clk-leaf-mux-determine-rate-request" because it doesn't call set_rate
at all.

If you want to add tests that check the rounding behavior then maybe add
a test for clk-mux.c called clk-mux_test.c and follow the
clk-gate_test.c approach to fake the register. Then also add tests for
the exported functions in there like clk_mux_val_to_index() and
clk_mux_determine_rate_flags(), etc. The mux determine rate API is in
clk.c, but that's mostly because it is so generic it can be used by any
mux type of clk hardware.

Please Cc Maxime on these patches too, because the lines you're
modifying were authored by Maxime.

> 
> And as said in the previous email, the situation here would become even
> more complicated if CLK_MUX_ROUND_CLOSEST is true. I guess it should be
> in another patchset.

Yes, that's different, and probably should be implemented as a direct
call to __clk_mux_determine_rate_closest() instead.