[PATCH 0/4] Introduce clk_find_nearest_rate()

[PATCH 0/4] Introduce clk_find_nearest_rate()

[Soren Brinkmann]

On Zynq I encountered issues due to rounding here and there. Often the
issue would have been resolved by rounding towards the requested
frequency. Unfortunately, the CCF does not specify  the behavior of
clk_round_rate() in terms of rounding, making this proposed API
call useful for certain use-cases.

An RFC of this series has been discussed here:
https://lkml.org/lkml/2014/5/14/694

Mike: IIRC, you wanted to fix the return type of __clk_round_rate(). Do
you have any patches for that already? I couldn't find anything in
clk-next.

Patch 1 adds the new API call.
2 and 3 apply the new API.
And 4 is a minor fixup of the Zynq DT. The frequencies in the Zynq OPPs
had been specified taking this rounding issues into account, which
is no longer required with this patchset.

	Thanks,
	Sören


Soren Brinkmann (4):
  clk: Introduce 'clk_find_nearest_rate()'
  cpufreq: cpu0: Use clk_find_nearest_rate()
  net: macb: Use clk_find_nearest_rate() API
  ARM: zynq: dt: Use properly rounded frequencies in OPPs

 arch/arm/boot/dts/zynq-7000.dtsi    |  4 +--
 drivers/clk/clk.c                   | 57 +++++++++++++++++++++++++++++++++++++
 drivers/cpufreq/cpufreq-cpu0.c      |  3 +-
 drivers/net/ethernet/cadence/macb.c |  2 +-
 include/linux/clk.h                 |  9 ++++++
 5 files changed, 71 insertions(+), 4 deletions(-)

-- 
2.0.1.1.gfbfc394

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Soren Brinkmann]

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;
+
+	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
-- 
2.0.1.1.gfbfc394

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

[Boris BREZILLON]

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



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Sören Brinkmann]

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

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

[Boris BREZILLON]

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, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris BREZILLON]

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
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sören Brinkmann]

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

[PATCH 2/4] cpufreq: cpu0: Use clk_find_nearest_rate()

[Soren Brinkmann]

Round clock frequencies to the nearest possible frequency. Since the
OPPs as specified in DT and the CCF use different a resolution for clock
frequencies, the clk_round_rate() API may return unexpected results, due
to not mandating how rounding has to happen. The clk_find_nearest_rate()
API mitigates such issues and finds the appropriate frequency for
an OPP.

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

 drivers/cpufreq/cpufreq-cpu0.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index ee1ae303a07c..1650581f85b6 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -42,7 +42,8 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
 	long freq_Hz, freq_exact;
 	int ret;
 
-	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
+	freq_Hz = clk_find_nearest_rate(cpu_clk,
+			freq_table[index].frequency * 1000);
 	if (freq_Hz <= 0)
 		freq_Hz = freq_table[index].frequency * 1000;
 
-- 
2.0.1.1.gfbfc394

[PATCH 3/4] net: macb: Use clk_find_nearest_rate() API

[Soren Brinkmann]

The Ethernet clock has to match the specified frequencies as accurately
as possible. clk_round_rate() does not specify how rounding is
implemented. Hence use clk_find_nearest_rate().

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

 drivers/net/ethernet/cadence/macb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e9daa072ebb4..7b7f5eb1b341 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -223,7 +223,7 @@ static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
 		return;
 	}
 
-	rate_rounded = clk_round_rate(clk, rate);
+	rate_rounded = clk_find_nearest_rate(clk, rate);
 	if (rate_rounded < 0)
 		return;
 
-- 
2.0.1.1.gfbfc394

[PATCH 4/4] ARM: zynq: dt: Use properly rounded frequencies in OPPs

[Soren Brinkmann]

Due to the clk_find_nearest_rate() API, OPPs can be specified
using proper rounding, now.

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

---
 arch/arm/boot/dts/zynq-7000.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 760bbc463c5b..b8e39ed5b132 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -29,8 +29,8 @@
 			operating-points = <
 				/* kHz    uV */
 				666667  1000000
-				333334  1000000
-				222223  1000000
+				333333  1000000
+				222222  1000000
 			>;
 		};
 
-- 
2.0.1.1.gfbfc394