Re: [PATCH v4 02/10] clk: Always clamp the rounded rate

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2022-01-25 06:15:41)
> The current core while setting the min and max rate properly in the
> clk_request structure will not make sure that the requested rate is
> within these boundaries, leaving it to each and every driver to make
> sure it is.

It would be good to describe why. Or decide that it was an oversight and
write that down here.

> 
> Add a clamp call to make sure it's always done, and add a few unit tests
> to make sure we don't have any regression there.

I looked through the per-user constraint patch history on the list but I
couldn't really figure out why it was done this way. I guess we didn't
clamp the rate in the core because we wanted to give the clk providers
all the information, i.e. the rate that was requested and the boundaries
that the consumers have placed on the rate. With the round_rate() clk_op
the providers don't know the min/max because the rate request structure
isn't passed. I think my concern a long time ago was that a consumer
could call clk_round_rate() and get one frequency and then call
clk_set_rate() and get another frequency. We need to make sure that
round_rate and set_rate agree with each other. If we don't do that then
we don't uphold the contract that clk_round_rate() tells the consumer
what rate they'll get if they call clk_set_rate() with the same
frequency.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c      |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c
> index 47a600d590c1..28c718ab82e1 100644
> --- a/drivers/clk/clk-test.c
> +++ b/drivers/clk/clk-test.c
> @@ -203,6 +203,50 @@ static void clk_range_test_set_range_invalid(struct kunit *test)
>                         0);
>  }
>  
> +/*
> + * Test that if our clock has some boundaries and we try to round a rate
> + * lower than the minimum, the returned rate will be within range.
> + */
> +static void clk_range_test_set_range_round_rate_lower(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

The comment says within range but this test says exactly the minimum
rate. Please change it to test that the rate is within rate 1 and rate
2. Also, we should call clk_get_rate() here to make sure the rate is
within the boundaries and matches what clk_round_rate() returned.

> +}
> +
> +/*
> + * Test that if our clock has some boundaries and we try to round a rate
> + * higher than the maximum, the returned rate will be within range.
> + */
> +static void clk_range_test_set_range_round_rate_higher(struct kunit *test)
> +{
> +       struct clk_dummy_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       long rate;
> +
> +       KUNIT_ASSERT_EQ(test,
> +                       clk_set_rate_range(clk,
> +                                          DUMMY_CLOCK_RATE_1,
> +                                          DUMMY_CLOCK_RATE_2),
> +                       0);
> +
> +       rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000);
> +       KUNIT_ASSERT_GT(test, rate, 0);
> +       KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);

Same comment about within range.

> +}
> +
>  /*
>   * Test that if our clock has a rate lower than the minimum set by a
>   * call to clk_set_rate_range(), the rate will be raised to match the
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8de6a22498e7..7bb5ae0fb688 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1330,6 +1330,8 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
>         if (!core)
>                 return 0;
>  
> +       req->rate = clamp(req->rate, req->min_rate, req->max_rate);
> +
>         /*
>          * At this point, core protection will be disabled
>          * - if the provider is not protected at all
> -- 
> 2.34.1
>