Meaning of clk_round_rate()?

Meaning of clk_round_rate()?

[Stephen Boyd]

  What is the meaning of clk_round_rate() in the clk API 
(include/linux/clk.h)? The function documentation says "adjust a rate to 
the exact rate a clock can provide". That seems pretty vague. I'm lead 
to believe that it rounds the rate to the closest rate supported. Is 
that correct? Is there some sort of error margin where beyond that it's 
no longer possible to be rounded? 0.5%? 1%?

Assuming it's doing closest matching, I don't see how it's very useful 
in practice. Some users of clk_round_rate() are blindly searching up and 
down in the frequency space until they find a suitable rate (see 
sound/atmel/abdac.c and sound/spi/at73c213.c). These drivers might be 
better served by something like a clk_round_rate_up() and a 
clk_round_rate_down() which would round the rate to the nearest higher 
and lower frequency respectively without requiring complex loops around 
clk_round_rate().

In addition, an up/down rounding approach would make it simpler for 
drivers to find a min/max rate (for example display panels have a max 
frequency they can support).

A similar approach was suggested by David Brownell [1] but nothing came 
of it.

[1] http://article.gmane.org/gmane.linux.ports.arm.kernel/38076

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Meaning of clk_round_rate()?

[Stephen Boyd]

  What is the meaning of clk_round_rate() in the clk API 
(include/linux/clk.h)? The function documentation says "adjust a rate to 
the exact rate a clock can provide". That seems pretty vague. I'm lead 
to believe that it rounds the rate to the closest rate supported. Is 
that correct? Is there some sort of error margin where beyond that it's 
no longer possible to be rounded? 0.5%? 1%?

Assuming it's doing closest matching, I don't see how it's very useful 
in practice. Some users of clk_round_rate() are blindly searching up and 
down in the frequency space until they find a suitable rate (see 
sound/atmel/abdac.c and sound/spi/at73c213.c). These drivers might be 
better served by something like a clk_round_rate_up() and a 
clk_round_rate_down() which would round the rate to the nearest higher 
and lower frequency respectively without requiring complex loops around 
clk_round_rate().

In addition, an up/down rounding approach would make it simpler for 
drivers to find a min/max rate (for example display panels have a max 
frequency they can support).

A similar approach was suggested by David Brownell [1] but nothing came 
of it.

[1] http://article.gmane.org/gmane.linux.ports.arm.kernel/38076

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Meaning of clk_round_rate()?

[Russell King - ARM Linux]

On Wed, Jul 14, 2010 at 11:05:46AM -0700, Stephen Boyd wrote:
>  What is the meaning of clk_round_rate() in the clk API  
> (include/linux/clk.h)? The function documentation says "adjust a rate to  
> the exact rate a clock can provide". That seems pretty vague. I'm lead  
> to believe that it rounds the rate to the closest rate supported. Is  
> that correct? Is there some sort of error margin where beyond that it's  
> no longer possible to be rounded? 0.5%? 1%?

clk_round_rate() returns the clock rate which will be set if you ask
clk_set_rate() to set that rate.  It provides a way to query from
the implementation exactly what rate you'll get if you use clk_set_rate()
with that same argument.

So essentially, clk_set_rate() should be:

static int clk_set_rate(struct clk *clk, unsigned long rate)
{
	rate = clk_round_rate(clk, rate);
	return set_actual_rate(clk, rate);
}

Meaning of clk_round_rate()?

[Russell King - ARM Linux]

On Wed, Jul 14, 2010 at 11:05:46AM -0700, Stephen Boyd wrote:
>  What is the meaning of clk_round_rate() in the clk API  
> (include/linux/clk.h)? The function documentation says "adjust a rate to  
> the exact rate a clock can provide". That seems pretty vague. I'm lead  
> to believe that it rounds the rate to the closest rate supported. Is  
> that correct? Is there some sort of error margin where beyond that it's  
> no longer possible to be rounded? 0.5%? 1%?

clk_round_rate() returns the clock rate which will be set if you ask
clk_set_rate() to set that rate.  It provides a way to query from
the implementation exactly what rate you'll get if you use clk_set_rate()
with that same argument.

So essentially, clk_set_rate() should be:

static int clk_set_rate(struct clk *clk, unsigned long rate)
{
	rate = clk_round_rate(clk, rate);
	return set_actual_rate(clk, rate);
}

Re: Meaning of clk_round_rate()?

[Stephen Boyd]

  On 07/14/2010 01:03 PM, Russell King - ARM Linux wrote:
> clk_round_rate() returns the clock rate which will be set if you ask
> clk_set_rate() to set that rate.  It provides a way to query from
> the implementation exactly what rate you'll get if you use clk_set_rate()
> with that same argument.
>
> So essentially, clk_set_rate() should be:
>
> static int clk_set_rate(struct clk *clk, unsigned long rate)
> {
> 	rate = clk_round_rate(clk, rate);
> 	return set_actual_rate(clk, rate);
> }

 From what I understand, you're saying clk_round_rate() is defined as 
what clk_set_rate() would do, which is call clk_round_rate() and then 
set the rate with whatever is returned by clk_round_rate()? Isn't that a 
recursive definition?

I'll play along though. The use of the function is to determine what the 
rate will be if I call clk_set_rate(), but what is the implementation of 
it suppose to be. I guess now I'm asking what should clk_set_rate() do? 
Round up, down, to the closest value, or just fail if it's not exact.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Meaning of clk_round_rate()?

[Stephen Boyd]

  On 07/14/2010 01:03 PM, Russell King - ARM Linux wrote:
> clk_round_rate() returns the clock rate which will be set if you ask
> clk_set_rate() to set that rate.  It provides a way to query from
> the implementation exactly what rate you'll get if you use clk_set_rate()
> with that same argument.
>
> So essentially, clk_set_rate() should be:
>
> static int clk_set_rate(struct clk *clk, unsigned long rate)
> {
> 	rate = clk_round_rate(clk, rate);
> 	return set_actual_rate(clk, rate);
> }

 From what I understand, you're saying clk_round_rate() is defined as 
what clk_set_rate() would do, which is call clk_round_rate() and then 
set the rate with whatever is returned by clk_round_rate()? Isn't that a 
recursive definition?

I'll play along though. The use of the function is to determine what the 
rate will be if I call clk_set_rate(), but what is the implementation of 
it suppose to be. I guess now I'm asking what should clk_set_rate() do? 
Round up, down, to the closest value, or just fail if it's not exact.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Meaning of clk_round_rate()?

[Saravana Kannan]

Russell King - ARM Linux wrote:
> On Wed, Jul 14, 2010 at 11:05:46AM -0700, Stephen Boyd wrote:
> clk_round_rate() returns the clock rate which will be set if you ask
> clk_set_rate() to set that rate.  It provides a way to query from
> the implementation exactly what rate you'll get if you use clk_set_rate()
> with that same argument.

Fair enough explanation for clk_round_rate(). I guess I should take it 
as "it's up to the specific clock implementation on what it wants to do".

But what about the problem of a clock consumer trying to find a suitable 
frequency amongst the ones provided by a particular clock?

What are your thoughts on adding the following two APIs to linux/clk.h?
clk_round_rate_down/floor()
clk_round_rate_up/ceil()

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Meaning of clk_round_rate()?

[Saravana Kannan]

Russell King - ARM Linux wrote:
> On Wed, Jul 14, 2010 at 11:05:46AM -0700, Stephen Boyd wrote:
> clk_round_rate() returns the clock rate which will be set if you ask
> clk_set_rate() to set that rate.  It provides a way to query from
> the implementation exactly what rate you'll get if you use clk_set_rate()
> with that same argument.

Fair enough explanation for clk_round_rate(). I guess I should take it 
as "it's up to the specific clock implementation on what it wants to do".

But what about the problem of a clock consumer trying to find a suitable 
frequency amongst the ones provided by a particular clock?

What are your thoughts on adding the following two APIs to linux/clk.h?
clk_round_rate_down/floor()
clk_round_rate_up/ceil()

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.