Messages in this thread | | | Date | Wed, 3 May 2023 19:53:44 +0800 | Subject | Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate | From | Yang Xiwen <> |
| |
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. > >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(struct kunit *test) > > Please add a comment above each test case like there is for > clk_leaf_mux_set_rate_parent_determine_rate() that describes what is > being tested. > >> +{ >> + struct clk_leaf_mux_ctx *ctx = test->priv; >> + struct clk_hw *hw = &ctx->hw; >> + struct clk *clk = clk_hw_get_clk(hw, NULL); >> + struct clk_rate_request req; >> + unsigned long rate; >> + int ret; >> + >> + rate = clk_get_rate(clk); >> + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); >> + >> + clk_hw_init_rate_request(hw, &req, 0); >> + >> + ret = __clk_determine_rate(hw, &req); >> + KUNIT_ASSERT_EQ(test, ret, -EINVAL); >> + >> + clk_put(clk); >> +} >> + >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case2(struct kunit *test) >> +{ >> + struct clk_leaf_mux_ctx *ctx = test->priv; >> + struct clk_hw *hw = &ctx->hw; >> + struct clk *clk = clk_hw_get_clk(hw, NULL); >> + struct clk_rate_request req; >> + unsigned long rate; >> + int ret; >> + >> + rate = clk_get_rate(clk); >> + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); >> + >> + clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE); >> + >> + ret = __clk_determine_rate(hw, &req); >> + KUNIT_ASSERT_EQ(test, ret, -EINVAL); > > There should be some KUNIT_EXPECT statement in each test.> >> + >> + clk_put(clk); >> +} >> + >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test) >> { >> struct clk_leaf_mux_ctx *ctx = test->priv; >> struct clk_hw *hw = &ctx->hw; >> @@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test) >> clk_put(clk); >> } >> >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case4(struct kunit *test) >> +{ >> + struct clk_leaf_mux_ctx *ctx = test->priv; >> + struct clk_hw *hw = &ctx->hw; >> + struct clk *clk = clk_hw_get_clk(hw, NULL); >> + struct clk_rate_request req; >> + unsigned long rate; >> + int ret; >> + >> + rate = clk_get_rate(clk); >> + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); >> + >> + clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2); >> + >> + ret = __clk_determine_rate(hw, &req); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1); >> + KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1); >> + KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw); >> + >> + clk_put(clk); >> +} >> + >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case5(struct kunit *test) >> +{ >> + struct clk_leaf_mux_ctx *ctx = test->priv; >> + struct clk_hw *hw = &ctx->hw; >> + struct clk *clk = clk_hw_get_clk(hw, NULL); >> + struct clk_rate_request req; >> + unsigned long rate; >> + int ret; >> + >> + rate = clk_get_rate(clk); >> + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); >> + >> + clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2 + 100000); >> + >> + ret = __clk_determine_rate(hw, &req); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2); >> + KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2); >> + KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw); >> + >> + clk_put(clk); >> +} >> + >> +static void clk_leaf_mux_set_rate_parent_determine_rate_case6(struct kunit *test) >> +{ >> + struct clk_leaf_mux_ctx *ctx = test->priv; >> + struct clk_hw *hw = &ctx->hw; >> + struct clk *clk = clk_hw_get_clk(hw, NULL); >> + struct clk_rate_request req; >> + unsigned long rate; >> + int ret; >> + >> + rate = clk_get_rate(clk); >> + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); >> + >> + clk_hw_init_rate_request(hw, &req, ULONG_MAX); >> + >> + ret = __clk_determine_rate(hw, &req); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2); >> + KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2); >> + KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw); >> + >> + clk_put(clk); >> +} >> + >> +/* We test 6 cases here: >> + * 1. The requested rate is 0; >> + * 2. The requested rate is not 0 but lower than any rate that parents could offer; >> + * 3. The requested rate is exactly one of the parents' clock rate; >> + * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer; >> + * 5. The requested rate is larger than all rates that parents could offer; >> + * 6. The requested rate is ULONG_MAX. >> + * >> + * Hopefully they covered all cases. >> + */ > > Please remove this comment and name the cases better. Thanks, I will follow your suggestions and rename them in next version. > >> static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = { >> - KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate), >> + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case1), > > Maybe call it clk_leaf_mux_determine_rate_request_zero? > >> + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2), > > clk_leaf_mux_determine_rate_lower_than_parents_fails > >> + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3), > > clk_leaf_mux_determine_rate_exactly_parent1 > >> + 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?Yes, that's it. For example, 2 parents offer 100MHz and 200MHz, we request 150MHz, and 100MHz should be determined(the highest possible rate lower than or equal to the requested rate). Actually the flag CLK_MUX_ROUND_CLOSEST is still missing in the test cases. I suppose it should be in another patchset. > >> + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5), > > clk_leaf_mux_determine_rate_larger_than_parents > >> + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6), > > clk_leaf_mux_determine_rate_ULONG_MAX_picks_parent1 -- Best regards, Yang Xiwen
| |