diff mbox series

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

Message ID 20220125141549.747889-3-maxime@cerno.tech (mailing list archive)
State New, archived
Headers show
Series clk: Improve clock range handling | expand

Commit Message

Maxime Ripard Jan. 25, 2022, 2:15 p.m. UTC
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.

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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c      |  2 ++
 2 files changed, 48 insertions(+)

Comments

Stephen Boyd Feb. 18, 2022, 11:15 p.m. UTC | #1
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
>
Maxime Ripard Feb. 21, 2022, 4:18 p.m. UTC | #2
Hi,

On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> 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.

I'm not really sure we should really leave it to the users, something like:

clk_set_range_rate(clk, 1000, 2000);
clk_set_rate(clk, 500);
clk_get_rate(clk) # == 500

Is definitely weird, and would break the least surprise :)

We shouldn't leave that to drivers, especially since close to none of
them handle this properly.

> 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.

I'm not sure I follow you there.

The function affected is clk_core_determine_round_nolock(), which is
called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
turn, they will be part of clk(_hw_)_round_clock for the former, and
clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.

I don't see how you can get a discrepancy between clk_round_rate() and
clk_set_rate().

And yeah, it's true that the round_rate op won't have the min and max
passed to them, but i'd consider this an argument for doing this check
here, since you don't have that option at all for those clocks.

> 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.

Ok
Maxime Ripard Feb. 21, 2022, 4:43 p.m. UTC | #3
Hi again,

On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > +/*
> > > + * 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.
> 
> Ok

Actually, that doesn't work. Calling clk_round_rate() won't affect the
clock rate, so the rate returned by clk_get_rate() won't match what
clk_round_rate() will return.

Maxime
Stephen Boyd Feb. 24, 2022, 10:32 p.m. UTC | #4
Quoting Maxime Ripard (2022-02-21 08:18:21)
> Hi,
> 
> On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > 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.
> 
> I'm not really sure we should really leave it to the users, something like:
> 
> clk_set_range_rate(clk, 1000, 2000);
> clk_set_rate(clk, 500);
> clk_get_rate(clk) # == 500
> 
> Is definitely weird, and would break the least surprise :)
> 
> We shouldn't leave that to drivers, especially since close to none of
> them handle this properly.

Ok.

> 
> > 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.
> 
> I'm not sure I follow you there.
> 
> The function affected is clk_core_determine_round_nolock(), which is
> called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> turn, they will be part of clk(_hw_)_round_clock for the former, and
> clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> 
> I don't see how you can get a discrepancy between clk_round_rate() and
> clk_set_rate().
> 
> And yeah, it's true that the round_rate op won't have the min and max
> passed to them, but i'd consider this an argument for doing this check
> here, since you don't have that option at all for those clocks.

When the range setting API was introduced the rounding logic and the
rate setting logic didn't use the same code paths. It looks like that
code got consolidated now though so we should be fine.
Stephen Boyd Feb. 24, 2022, 10:39 p.m. UTC | #5
Quoting Maxime Ripard (2022-02-21 08:43:23)
> Hi again,
> 
> On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > +/*
> > > > + * 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.
> > 
> > Ok
> 
> Actually, that doesn't work. Calling clk_round_rate() won't affect the
> clock rate, so the rate returned by clk_get_rate() won't match what
> clk_round_rate() will return.

Huh? This is asking "what rate will I get if I call clk_set_rate() with
DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
2. It should round that up to some value (and we should enforce that it
is inclusive or exclusive). I think I missed that this is
clk_round_rate().

Either way, the clk provider implementation could say that if you call
clk_set_rate() with a frequency below the minimum that it lies somewhere
between the rate 1 and rate 2. The expectation should only check that it
is within the range and not exactly the minimum because we're not
testing the clk provider implementation of the rounding here, just that
the constraints are satisfied and the rate is within range. That's my
understanding of the comment above the function and the function name.
Maxime Ripard Feb. 25, 2022, 9:35 a.m. UTC | #6
Hi,

On Thu, Feb 24, 2022 at 02:39:20PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:43:23)
> > Hi again,
> > 
> > On Mon, Feb 21, 2022 at 05:18:21PM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > > Quoting Maxime Ripard (2022-01-25 06:15:41)
> > > > > +/*
> > > > > + * 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.
> > > 
> > > Ok
> > 
> > Actually, that doesn't work. Calling clk_round_rate() won't affect the
> > clock rate, so the rate returned by clk_get_rate() won't match what
> > clk_round_rate() will return.
> 
> Huh? This is asking "what rate will I get if I call clk_set_rate() with
> DUMMY_CLOCK_RATE_1 - 1000 after setting the range to be rate 1 and rate
> 2. It should round that up to some value (and we should enforce that it
> is inclusive or exclusive). I think I missed that this is
> clk_round_rate().
> 
> Either way, the clk provider implementation could say that if you call
> clk_set_rate() with a frequency below the minimum that it lies somewhere
> between the rate 1 and rate 2. The expectation should only check that it
> is within the range and not exactly the minimum because we're not
> testing the clk provider implementation of the rounding here, just that
> the constraints are satisfied and the rate is within range. That's my
> understanding of the comment above the function and the function name.

You're right, that has been addressed in the last version I sent already

Maxime
Maxime Ripard Feb. 25, 2022, 9:45 a.m. UTC | #7
Hi,

On Thu, Feb 24, 2022 at 02:32:37PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-21 08:18:21)
> > Hi,
> > 
> > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote:
> > > 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.
> > 
> > I'm not really sure we should really leave it to the users, something like:
> > 
> > clk_set_range_rate(clk, 1000, 2000);
> > clk_set_rate(clk, 500);
> > clk_get_rate(clk) # == 500
> > 
> > Is definitely weird, and would break the least surprise :)
> > 
> > We shouldn't leave that to drivers, especially since close to none of
> > them handle this properly.
> 
> Ok.
> 
> > > 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.
> > 
> > I'm not sure I follow you there.
> > 
> > The function affected is clk_core_determine_round_nolock(), which is
> > called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In
> > turn, they will be part of clk(_hw_)_round_clock for the former, and
> > clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter.
> > 
> > I don't see how you can get a discrepancy between clk_round_rate() and
> > clk_set_rate().
> > 
> > And yeah, it's true that the round_rate op won't have the min and max
> > passed to them, but i'd consider this an argument for doing this check
> > here, since you don't have that option at all for those clocks.
> 
> When the range setting API was introduced the rounding logic and the
> rate setting logic didn't use the same code paths. It looks like that
> code got consolidated now though so we should be fine.

Actually, there was a discrepancy. If you are doing, before this patch
series:

clk_set_range_rate(clk, 1000, 2000)
clk_round_rate(500);

Unless the driver was involved, the returned rate would be 500.

Now, if you call clk_set_rate(500), it will return -EINVAL, hitting the
check here:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1973

If the driver was looking at the min and max and clamping the rate, you
would get clk_round_rate() == 1000 and clk_set_rate() would succeed,
with the rate set to 1000.

This seems like an abstraction leakage to me.

This patch fixes that discrepancy, but in the last version I sent, I
added a test that would check that once you have a range in place, then
clk_round_rate and clk_set_rate/clk_get_rate would return the same
value.

Maxime
diff mbox series

Patch

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);
+}
+
+/*
+ * 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);
+}
+
 /*
  * 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
@@ -266,6 +310,8 @@  static void clk_range_test_set_range_get_rate_lowered(struct kunit *test)
 static struct kunit_case clk_range_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range),
 	KUNIT_CASE(clk_range_test_set_range_invalid),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
+	KUNIT_CASE(clk_range_test_set_range_round_rate_higher),
 	KUNIT_CASE(clk_range_test_set_range_get_rate_raised),
 	KUNIT_CASE(clk_range_test_set_range_get_rate_lowered),
 	{}
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