diff mbox

[1/4] clk: Introduce 'clk_find_nearest_rate()'

Message ID 1404147396-8041-2-git-send-email-soren.brinkmann@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Soren Brinkmann June 30, 2014, 4:56 p.m. UTC
Introduce a new API function to find the rate a clock can provide which
is closest to a given rate.

clk_round_rate() leaves it to the clock driver how rounding is done.
Commonly implementations round down due to use-cases that have a certain
frequency maximum that must not be exceeded.

The new API call enables use-cases where accuracy is preferred. E.g.
Ethernet clocks.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 drivers/clk/clk.c   | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h |  9 +++++++++
 2 files changed, 66 insertions(+)

Comments

Boris BREZILLON June 30, 2014, 7:27 p.m. UTC | #1
Hello Soren,

On Mon, 30 Jun 2014 09:56:33 -0700
Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:

> Introduce a new API function to find the rate a clock can provide
> which is closest to a given rate.
> 
> clk_round_rate() leaves it to the clock driver how rounding is done.
> Commonly implementations round down due to use-cases that have a
> certain frequency maximum that must not be exceeded.

I had the same concern (how could a driver decide whether it should
round up, down or to the closest value), but had a slightly different
approach in mind.

AFAIU, you're assuming the clock is always a power of two (which is
true most of the time, but some clock implementation might differ,
i.e. a PLL accept any kind of multiplier not necessarily a power of 2).

How about improving the clk_ops interface instead by adding a new method

	long (*round_rate_with_constraint)(struct clk_hw *hw,
					   unsigned long requested_rate,
					   unsigned long *parent_rate,
					   enum clk_round_type type);

with

enum clk_round_type {
	CLK_ROUND_DOWN,
	CLK_ROUND_UP,
	CLK_ROUND_CLOSEST,
};

or just adding these 3 methods:

	long (*round_rate_up)(struct clk_hw *hw,
			      unsigned long requested_rate,
			      unsigned long *parent_rate);

	long (*round_rate_down)(struct clk_hw *hw,
				unsigned long requested_rate,
				unsigned long *parent_rate);

	long (*round_rate_closest)(struct clk_hw *hw,
				   unsigned long requested_rate,
				   unsigned long *parent_rate);

and let the round_rate method implement the default behaviour.

This way you could add 3 functions to the API:

clk_round_closest (or clk_find_nearest_rate as you called it),
clk_round_up and clk_round_down, and let the clk driver implementation
return the appropriate rate.

These are just some thoughts...

Best Regards,

Boris

> 
> The new API call enables use-cases where accuracy is preferred. E.g.
> Ethernet clocks.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
> 
>  drivers/clk/clk.c   | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/clk.h |  9 +++++++++ 2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8b73edef151d..fce1165cd879 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1030,6 +1030,63 @@ long clk_round_rate(struct clk *clk, unsigned
> long rate) EXPORT_SYMBOL_GPL(clk_round_rate);
>  
>  /**
> + * clk_find_nearest_rate - round the given rate for a clk
> + * @clk: the clk for which we are rounding a rate
> + * @rate: the rate which is to be rounded
> + *
> + * Takes in a rate as input and finds the closest rate that the clk
> + * can actually use which is then returned.
> + * Note: This function relies on the clock's clk_round_rate()
> implementation.
> + * For cases clk_round_rate() rounds up, not the closest but the
> rounded up
> + * rate is found.
> + */
> +long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
> +{
> +	long ret, lower, upper;
> +	unsigned long tmp;
> +
> +	clk_prepare_lock();
> +
> +	lower = __clk_round_rate(clk, rate);
> +	if (lower >= rate || lower < 0) {
> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	tmp = rate + (rate - lower) - 1;
> +	if (tmp > LONG_MAX)
> +		upper = LONG_MAX;
> +	else
> +		upper = tmp;
> +
> +	upper = __clk_round_rate(clk, upper);
> +	if (upper <= lower || upper < 0) {
> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	lower = rate + 1;
> +	while (lower < upper) {
> +		long rounded, mid;
> +
> +		mid = lower + ((upper - lower) >> 1);
> +		rounded = __clk_round_rate(clk, mid);
> +		if (rounded < lower)
> +			lower = mid + 1;
> +		else
> +			upper = rounded;
> +	}
> +
> +	ret = upper;
> +
> +unlock:
> +	clk_prepare_unlock();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_find_nearest_rate);
> +
> +/**
>   * __clk_notify - call clk notifier chain
>   * @clk: struct clk * that is changing rate
>   * @msg: clk notifier type (see include/linux/clk.h)
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index fb5e097d8f72..f8b53c515483 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -264,6 +264,15 @@ void devm_clk_put(struct device *dev, struct clk
> *clk); long clk_round_rate(struct clk *clk, unsigned long rate);
>  
>  /**
> + * clk_find_nearest_rate - Find nearest rate to the exact rate a
> clock can provide
> + * @clk: the clk for which we are rounding a rate
> + * @rate: the rate which is to be rounded
> + *
> + * Returns the rate closest to @rate the clock can provide.
> + */
> +long clk_find_nearest_rate(struct clk *clk, unsigned long rate);
> +
> +/**
>   * clk_set_rate - set the clock rate for a clock source
>   * @clk: clock source
>   * @rate: desired clock rate in Hz
Soren Brinkmann July 1, 2014, 12:12 a.m. UTC | #2
Hi Boris,

On Mon, 2014-06-30 at 09:27PM +0200, Boris BREZILLON wrote:
> Hello Soren,
> 
> On Mon, 30 Jun 2014 09:56:33 -0700
> Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> 
> > Introduce a new API function to find the rate a clock can provide
> > which is closest to a given rate.
> > 
> > clk_round_rate() leaves it to the clock driver how rounding is done.
> > Commonly implementations round down due to use-cases that have a
> > certain frequency maximum that must not be exceeded.
> 
> I had the same concern (how could a driver decide whether it should
> round up, down or to the closest value), but had a slightly different
> approach in mind.
> 
> AFAIU, you're assuming the clock is always a power of two (which is
> true most of the time, but some clock implementation might differ,
> i.e. a PLL accept any kind of multiplier not necessarily a power of 2).

No, the idea is to always work. There should not be any such limitation.
Where do you see that?

> 
> How about improving the clk_ops interface instead by adding a new method
> 
> 	long (*round_rate_with_constraint)(struct clk_hw *hw,
> 					   unsigned long requested_rate,
> 					   unsigned long *parent_rate,
> 					   enum clk_round_type type);
> 
> with
> 
> enum clk_round_type {
> 	CLK_ROUND_DOWN,
> 	CLK_ROUND_UP,
> 	CLK_ROUND_CLOSEST,
> };

I thought about that, but the find_nearest() did already exist more or
less and in the end it is not much of a difference, IMHO. If it turns out that
the others are needed as well and somebody implements it, they could be
added as another convenience function like I did, and/or it could be
wrapped in the function you propose here.

> 
> or just adding these 3 methods:
> 
> 	long (*round_rate_up)(struct clk_hw *hw,
> 			      unsigned long requested_rate,
> 			      unsigned long *parent_rate);
> 
> 	long (*round_rate_down)(struct clk_hw *hw,
> 				unsigned long requested_rate,
> 				unsigned long *parent_rate);
> 
> 	long (*round_rate_closest)(struct clk_hw *hw,
> 				   unsigned long requested_rate,
> 				   unsigned long *parent_rate);

That would be quite a change for clock drivers. So far, there are not many
restrictions on round_rate. I think there has already been a discussion
in this direction that went nowhere. https://lkml.org/lkml/2010/7/14/260

> 
> and let the round_rate method implement the default behaviour.

There is no real default. Rounding is not specified for the current API.

> 
> This way you could add 3 functions to the API:
> 
> clk_round_closest (or clk_find_nearest_rate as you called it),
> clk_round_up and clk_round_down, and let the clk driver implementation
> return the appropriate rate.

I'd say the current patch set does kind of align with that, doesn't it?
We can add the find_nearest_rate() (there was a discussion on the name,
ane we decided against round_closest in favor of the current proposal)
now and the other functions could be added later if people find them to
be useful. And if they all get added we can think about consolidating
them if it made sense.

	Thanks,
	Sören
Boris BREZILLON July 1, 2014, 6:32 a.m. UTC | #3
Hi Sören,

On Mon, 30 Jun 2014 17:12:23 -0700
Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:

> Hi Boris,
> 
> On Mon, 2014-06-30 at 09:27PM +0200, Boris BREZILLON wrote:
> > Hello Soren,
> > 
> > On Mon, 30 Jun 2014 09:56:33 -0700
> > Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > 
> > > Introduce a new API function to find the rate a clock can provide
> > > which is closest to a given rate.
> > > 
> > > clk_round_rate() leaves it to the clock driver how rounding is
> > > done. Commonly implementations round down due to use-cases that
> > > have a certain frequency maximum that must not be exceeded.
> > 
> > I had the same concern (how could a driver decide whether it should
> > round up, down or to the closest value), but had a slightly
> > different approach in mind.
> > 
> > AFAIU, you're assuming the clock is always a power of two (which is
> > true most of the time, but some clock implementation might differ,
> > i.e. a PLL accept any kind of multiplier not necessarily a power of
> > 2).
> 
> No, the idea is to always work. There should not be any such
> limitation. Where do you see that?

My bad, I should have read the code more carefully.
BTW, it could help readers if you add some comments to explain how you
are finding the nearest rate.

My main concern with this approach is that you can spend some time
iterating to find the nearest rate where a clk driver would find it
quite quickly (because it knows exactly how the clk works and what are
the possible clk muxing and clk modifiers options).

> 
> > 
> > How about improving the clk_ops interface instead by adding a new
> > method
> > 
> > 	long (*round_rate_with_constraint)(struct clk_hw *hw,
> > 					   unsigned long
> > requested_rate, unsigned long *parent_rate,
> > 					   enum clk_round_type
> > type);
> > 
> > with
> > 
> > enum clk_round_type {
> > 	CLK_ROUND_DOWN,
> > 	CLK_ROUND_UP,
> > 	CLK_ROUND_CLOSEST,
> > };
> 
> I thought about that, but the find_nearest() did already exist more or
> less and in the end it is not much of a difference, IMHO. If it turns
> out that the others are needed as well and somebody implements it,
> they could be added as another convenience function like I did,
> and/or it could be wrapped in the function you propose here.
>
> > 
> > or just adding these 3 methods:
> > 
> > 	long (*round_rate_up)(struct clk_hw *hw,
> > 			      unsigned long requested_rate,
> > 			      unsigned long *parent_rate);
> > 
> > 	long (*round_rate_down)(struct clk_hw *hw,
> > 				unsigned long requested_rate,
> > 				unsigned long *parent_rate);
> > 
> > 	long (*round_rate_closest)(struct clk_hw *hw,
> > 				   unsigned long requested_rate,
> > 				   unsigned long *parent_rate);
> 
> That would be quite a change for clock drivers. So far, there are not
> many restrictions on round_rate. I think there has already been a
> discussion in this direction that went nowhere.
> https://lkml.org/lkml/2010/7/14/260
> 

Not if we keep these (or this, depending on the solution you chose)
functions optional, and return -ENOTSUP, if they're not implemented.

> > 
> > and let the round_rate method implement the default behaviour.
> 
> There is no real default. Rounding is not specified for the current
> API.

What I meant by default behavior is the behavior already implemented by
the clock driver (either round up, down or closest).
This way you do not introduce regressions with existing users, and can
use new methods in new use cases.

> 
> > 
> > This way you could add 3 functions to the API:
> > 
> > clk_round_closest (or clk_find_nearest_rate as you called it),
> > clk_round_up and clk_round_down, and let the clk driver
> > implementation return the appropriate rate.
> 
> I'd say the current patch set does kind of align with that, doesn't
> it? We can add the find_nearest_rate() (there was a discussion on the
> name, ane we decided against round_closest in favor of the current
> proposal) now and the other functions could be added later if people
> find them to be useful. And if they all get added we can think about
> consolidating them if it made sense.

Yes sure.

Best Regards,

Boris
Boris BREZILLON July 1, 2014, 7:18 a.m. UTC | #4
On Tue, 1 Jul 2014 08:32:14 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Hi Sören,
> 
> On Mon, 30 Jun 2014 17:12:23 -0700
> Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> 
> > Hi Boris,
> > 
> > On Mon, 2014-06-30 at 09:27PM +0200, Boris BREZILLON wrote:
> > > Hello Soren,
> > > 
> > > On Mon, 30 Jun 2014 09:56:33 -0700
> > > Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > > 
> > > > Introduce a new API function to find the rate a clock can
> > > > provide which is closest to a given rate.
> > > > 
> > > > clk_round_rate() leaves it to the clock driver how rounding is
> > > > done. Commonly implementations round down due to use-cases that
> > > > have a certain frequency maximum that must not be exceeded.
> > > 
> > > I had the same concern (how could a driver decide whether it
> > > should round up, down or to the closest value), but had a slightly
> > > different approach in mind.
> > > 
> > > AFAIU, you're assuming the clock is always a power of two (which
> > > is true most of the time, but some clock implementation might
> > > differ, i.e. a PLL accept any kind of multiplier not necessarily
> > > a power of 2).
> > 
> > No, the idea is to always work. There should not be any such
> > limitation. Where do you see that?
> 
> My bad, I should have read the code more carefully.
> BTW, it could help readers if you add some comments to explain how you
> are finding the nearest rate.
> 
> My main concern with this approach is that you can spend some time
> iterating to find the nearest rate where a clk driver would find it
> quite quickly (because it knows exactly how the clk works and what are
> the possible clk muxing and clk modifiers options).
> 
> > 
> > > 
> > > How about improving the clk_ops interface instead by adding a new
> > > method
> > > 
> > > 	long (*round_rate_with_constraint)(struct clk_hw *hw,
> > > 					   unsigned long
> > > requested_rate, unsigned long *parent_rate,
> > > 					   enum clk_round_type
> > > type);
> > > 
> > > with
> > > 
> > > enum clk_round_type {
> > > 	CLK_ROUND_DOWN,
> > > 	CLK_ROUND_UP,
> > > 	CLK_ROUND_CLOSEST,
> > > };
> > 
> > I thought about that, but the find_nearest() did already exist more
> > or less and in the end it is not much of a difference, IMHO. If it
> > turns out that the others are needed as well and somebody
> > implements it, they could be added as another convenience function
> > like I did, and/or it could be wrapped in the function you propose
> > here.
> >
> > > 
> > > or just adding these 3 methods:
> > > 
> > > 	long (*round_rate_up)(struct clk_hw *hw,
> > > 			      unsigned long requested_rate,
> > > 			      unsigned long *parent_rate);
> > > 
> > > 	long (*round_rate_down)(struct clk_hw *hw,
> > > 				unsigned long requested_rate,
> > > 				unsigned long *parent_rate);
> > > 
> > > 	long (*round_rate_closest)(struct clk_hw *hw,
> > > 				   unsigned long requested_rate,
> > > 				   unsigned long *parent_rate);
> > 
> > That would be quite a change for clock drivers. So far, there are
> > not many restrictions on round_rate. I think there has already been
> > a discussion in this direction that went nowhere.
> > https://lkml.org/lkml/2010/7/14/260
> > 
> 
> Not if we keep these (or this, depending on the solution you chose)
> functions optional, and return -ENOTSUP, if they're not implemented.

Or even better: fall back to your implementation.

> 
> > > 
> > > and let the round_rate method implement the default behaviour.
> > 
> > There is no real default. Rounding is not specified for the current
> > API.
> 
> What I meant by default behavior is the behavior already implemented
> by the clock driver (either round up, down or closest).
> This way you do not introduce regressions with existing users, and can
> use new methods in new use cases.
> 
> > 
> > > 
> > > This way you could add 3 functions to the API:
> > > 
> > > clk_round_closest (or clk_find_nearest_rate as you called it),
> > > clk_round_up and clk_round_down, and let the clk driver
> > > implementation return the appropriate rate.
> > 
> > I'd say the current patch set does kind of align with that, doesn't
> > it? We can add the find_nearest_rate() (there was a discussion on
> > the name, ane we decided against round_closest in favor of the
> > current proposal) now and the other functions could be added later
> > if people find them to be useful. And if they all get added we can
> > think about consolidating them if it made sense.
> 
> Yes sure.
> 
> Best Regards,
> 
> Boris
>
Uwe Kleine-König July 1, 2014, 8:23 a.m. UTC | #5
On Mon, Jun 30, 2014 at 09:56:33AM -0700, Soren Brinkmann wrote:
> Introduce a new API function to find the rate a clock can provide which
> is closest to a given rate.
> 
> clk_round_rate() leaves it to the clock driver how rounding is done.
> Commonly implementations round down due to use-cases that have a certain
> frequency maximum that must not be exceeded.
> 
> The new API call enables use-cases where accuracy is preferred. E.g.
> Ethernet clocks.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
> 
>  drivers/clk/clk.c   | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h |  9 +++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8b73edef151d..fce1165cd879 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1030,6 +1030,63 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  EXPORT_SYMBOL_GPL(clk_round_rate);
>  
>  /**
> + * clk_find_nearest_rate - round the given rate for a clk
> + * @clk: the clk for which we are rounding a rate
> + * @rate: the rate which is to be rounded
> + *
> + * Takes in a rate as input and finds the closest rate that the clk
> + * can actually use which is then returned.
> + * Note: This function relies on the clock's clk_round_rate() implementation.
> + * For cases clk_round_rate() rounds up, not the closest but the rounded up
> + * rate is found.
> + */
> +long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
> +{
> +	long ret, lower, upper;
> +	unsigned long tmp;
> +
> +	clk_prepare_lock();
> +
> +	lower = __clk_round_rate(clk, rate);
> +	if (lower >= rate || lower < 0) {
> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	tmp = rate + (rate - lower) - 1;
> +	if (tmp > LONG_MAX)
> +		upper = LONG_MAX;
> +	else
> +		upper = tmp;
Consider rate = 0xf0000000, lower = 0x7fffffff (= LONG_MAX). Then tmp =
(unsigned long)0x160000000 = 0x60000000. In this case you pick upper =
0x60000000 while you should use upper = LONG_MAX.

I think you need

-	if (tmp > LONG_MAX)
+	if (tmp > LONG_MAX || tmp < rate)

(and a comment)

> +
> +	upper = __clk_round_rate(clk, upper);
> +	if (upper <= lower || upper < 0) {
Is it an idea to do something like:

		if (IS_ENABLED(CONFIG_CLK_SANITY_CHECKS))
			WARN_ON(upper < lower && upper >= 0);

here?

> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	lower = rate + 1;
> +	while (lower < upper) {
> +		long rounded, mid;
> +
> +		mid = lower + ((upper - lower) >> 1);
> +		rounded = __clk_round_rate(clk, mid);
> +		if (rounded < lower)
> +			lower = mid + 1;
> +		else
> +			upper = rounded;
> +	}
This is broken if you don't assume that __clk_round_rate rounds down.
Consider an implementation that already does round_nearest and clk can
assume the values 0x60000 and 0x85000 (and nothing in between), and rate
= 0x70000. This results in

	lower = 0x60000;
	tmp = 0x7ffff;
	upper = __clk_round_rate(clk, 0x7ffff) = 0x85000

before the loop and the loop then doesn't even terminate.

Best regards
Uwe
Soren Brinkmann July 1, 2014, 5:52 p.m. UTC | #6
Hi Uwe,

On Tue, 2014-07-01 at 10:23AM +0200, Uwe Kleine-König wrote:
> On Mon, Jun 30, 2014 at 09:56:33AM -0700, Soren Brinkmann wrote:
> > Introduce a new API function to find the rate a clock can provide which
> > is closest to a given rate.
> > 
> > clk_round_rate() leaves it to the clock driver how rounding is done.
> > Commonly implementations round down due to use-cases that have a certain
> > frequency maximum that must not be exceeded.
> > 
> > The new API call enables use-cases where accuracy is preferred. E.g.
> > Ethernet clocks.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> > 
> >  drivers/clk/clk.c   | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk.h |  9 +++++++++
> >  2 files changed, 66 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8b73edef151d..fce1165cd879 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1030,6 +1030,63 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
> >  EXPORT_SYMBOL_GPL(clk_round_rate);
> >  
> >  /**
> > + * clk_find_nearest_rate - round the given rate for a clk
> > + * @clk: the clk for which we are rounding a rate
> > + * @rate: the rate which is to be rounded
> > + *
> > + * Takes in a rate as input and finds the closest rate that the clk
> > + * can actually use which is then returned.
> > + * Note: This function relies on the clock's clk_round_rate() implementation.
> > + * For cases clk_round_rate() rounds up, not the closest but the rounded up
> > + * rate is found.
> > + */
> > +long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	long ret, lower, upper;
> > +	unsigned long tmp;
> > +
> > +	clk_prepare_lock();
> > +
> > +	lower = __clk_round_rate(clk, rate);
> > +	if (lower >= rate || lower < 0) {
> > +		ret = lower;
> > +		goto unlock;
> > +	}
> > +
> > +	tmp = rate + (rate - lower) - 1;
> > +	if (tmp > LONG_MAX)
> > +		upper = LONG_MAX;
> > +	else
> > +		upper = tmp;
> Consider rate = 0xf0000000, lower = 0x7fffffff (= LONG_MAX). Then tmp =
> (unsigned long)0x160000000 = 0x60000000. In this case you pick upper =
> 0x60000000 while you should use upper = LONG_MAX.
Strictly speaking yes, but isn't 0xf000000 already an invalid argument?
I kept it as unsigned long to match the round_rate prototype, but as we
discussed before, rates should probably all be long only since the
return type of these functions is signed. Unless I miss something,
requesting a rate > LONG_MAX while we can return a long only should not
be possible. Hence, I was assuming to work with rates in the range up to
LONG_MAX only for now.
But adding a something like 'if (rate > LONG_MAX) return -EINVAL;' would
probably be a good idea in this case.

> 
> I think you need
> 
> -	if (tmp > LONG_MAX)
> +	if (tmp > LONG_MAX || tmp < rate)
I was looking for how to correctly check for arithmetic overflows (since
you pointed out the flaws with my solution using casts), and this check
relies on undefined behavior as well, IIUC.

> 
> (and a comment)
> 
> > +
> > +	upper = __clk_round_rate(clk, upper);
> > +	if (upper <= lower || upper < 0) {
> Is it an idea to do something like:
> 
> 		if (IS_ENABLED(CONFIG_CLK_SANITY_CHECKS))
> 			WARN_ON(upper < lower && upper >= 0);
> 
> here?
Probably a good idea. I assumed that we can - more or less - safely
count on that behavior once we are through those checks above.

> 
> > +		ret = lower;
> > +		goto unlock;
> > +	}
> > +
> > +	lower = rate + 1;
> > +	while (lower < upper) {
> > +		long rounded, mid;
> > +
> > +		mid = lower + ((upper - lower) >> 1);
> > +		rounded = __clk_round_rate(clk, mid);
> > +		if (rounded < lower)
> > +			lower = mid + 1;
> > +		else
> > +			upper = rounded;
> > +	}
> This is broken if you don't assume that __clk_round_rate rounds down.
> Consider an implementation that already does round_nearest and clk can
> assume the values 0x60000 and 0x85000 (and nothing in between), and rate
> = 0x70000. This results in
> 
> 	lower = 0x60000;
> 	tmp = 0x7ffff;
> 	upper = __clk_round_rate(clk, 0x7ffff) = 0x85000
> 
> before the loop and the loop then doesn't even terminate.
Good catch. With rounding being unspecified, there are a lot of
corner cases.
I think this change would fix that corner case, but I'm not sure it
covers all:
 
        tmp = rate + (rate - lower) - 1;
        if (tmp > LONG_MAX)
-               upper = LONG_MAX;
-       else
-               upper = tmp;
+               tmp = LONG_MAX;
 
-       upper = __clk_round_rate(clk, upper);
-       if (upper <= lower || upper < 0) {
+       upper = __clk_round_rate(clk, tmp);
+       if (upper <= lower || upper < 0 || upper > tmp) {
                ret = lower;
                goto unlock;
        }

	Thanks,
	Sören
diff mbox

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8b73edef151d..fce1165cd879 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1030,6 +1030,63 @@  long clk_round_rate(struct clk *clk, unsigned long rate)
 EXPORT_SYMBOL_GPL(clk_round_rate);
 
 /**
+ * clk_find_nearest_rate - round the given rate for a clk
+ * @clk: the clk for which we are rounding a rate
+ * @rate: the rate which is to be rounded
+ *
+ * Takes in a rate as input and finds the closest rate that the clk
+ * can actually use which is then returned.
+ * Note: This function relies on the clock's clk_round_rate() implementation.
+ * For cases clk_round_rate() rounds up, not the closest but the rounded up
+ * rate is found.
+ */
+long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
+{
+	long ret, lower, upper;
+	unsigned long tmp;
+
+	clk_prepare_lock();
+
+	lower = __clk_round_rate(clk, rate);
+	if (lower >= rate || lower < 0) {
+		ret = lower;
+		goto unlock;
+	}
+
+	tmp = rate + (rate - lower) - 1;
+	if (tmp > LONG_MAX)
+		upper = LONG_MAX;
+	else
+		upper = tmp;
+
+	upper = __clk_round_rate(clk, upper);
+	if (upper <= lower || upper < 0) {
+		ret = lower;
+		goto unlock;
+	}
+
+	lower = rate + 1;
+	while (lower < upper) {
+		long rounded, mid;
+
+		mid = lower + ((upper - lower) >> 1);
+		rounded = __clk_round_rate(clk, mid);
+		if (rounded < lower)
+			lower = mid + 1;
+		else
+			upper = rounded;
+	}
+
+	ret = upper;
+
+unlock:
+	clk_prepare_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_find_nearest_rate);
+
+/**
  * __clk_notify - call clk notifier chain
  * @clk: struct clk * that is changing rate
  * @msg: clk notifier type (see include/linux/clk.h)
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e097d8f72..f8b53c515483 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -264,6 +264,15 @@  void devm_clk_put(struct device *dev, struct clk *clk);
 long clk_round_rate(struct clk *clk, unsigned long rate);
 
 /**
+ * clk_find_nearest_rate - Find nearest rate to the exact rate a clock can provide
+ * @clk: the clk for which we are rounding a rate
+ * @rate: the rate which is to be rounded
+ *
+ * Returns the rate closest to @rate the clock can provide.
+ */
+long clk_find_nearest_rate(struct clk *clk, unsigned long rate);
+
+/**
  * clk_set_rate - set the clock rate for a clock source
  * @clk: clock source
  * @rate: desired clock rate in Hz