| Message ID | 20230421-clk-v3-2-9ff79e7e7fed@outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: fix corner case of clk_mux_determine_rate_flags() | expand |
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. > 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? > + 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
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
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. > >> +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. > >> 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? I think I have to speak more about how these tests are arranged. 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. 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. > >> + 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
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.
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) +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(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, 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); + + 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. + */ 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), + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2), + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3), + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4), + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5), + KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6), {} };