RFC: A better clock rounding API?

RFC: A better clock rounding API?

[Saravana Kannan]

I'm mostly familiar with ARM.  So I will limit the discussion to ARM 
boards/machines.  But I think the points raised in this email would 
apply for most architectures.

I'm sending this to the ARM mailing list first to see if I can get a 
consensus within the ARM community before I propose this to the wider 
audience in LKML.

The meaning of clk_round_rate() is very ambiguous since the nature of 
the rounding is architecture and platform specific.  In the case of ARM, 
it's up to each ARM mach's clock driver to determine how it wants to 
round the rate.

To quote Russel King from an email about a month ago:
"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 when someone writes a device driver for a device that's external to 
the SoC or is integrated into more than one SoC, they currently have the 
following options to deal with clock rates differences:

1. Use clk_round_rate() to get clock rates and test their driver on the 
one/few board(s) they have at hand and hope it works on boards using 
different SoCs.

2. Add each and every needed clock rate (low power rate, high 
performance rate, handshake rate, etc) as fields to their platform data 
and have it populated in every board file that uses the device.

3. Do a search of the frequency space of the clock by making several 
clk_round_rate() calls at various intervals between their minimum and 
maximum acceptable rates and hope to find one of the supported rates. 
If clk_round_rate() does a +/- N percentage rounding and the interval is 
larger, even this searching might not find an existing rate that's 
supported between the driver's min and max acceptable rates.

IMHO, each of these options have short comings that could be alleviated 
by adding a more definitive "rounding" API.  Also, considering that it's 
the consumer of each clock that knows best what amount of rounding and 
in which direction is acceptable, IMHO the current approach of hiding 
the rounding inside the clock drivers seems counter intuitive.

I would like to propose the addition of either:

long clk_find_rate(struct clk *clk,
			unsigned long min_rate,
			long max_rate);

or

long clk_find_rate(struct clk *clk,
			unsigned long start_rate,
			long end_rate);

The advantage of the 2nd suggestion is that it allows a driver to 
specify which end of a frequency range it prefers.  But I'm not sure how 
important of an advantage that is.  So, proposing both and having the 
community decide which one, if any, is acceptable.

If the clk_find_rate() API is available, the driver developer wouldn't 
have to worry about figuring out a way for the clk_set_rate() to work on 
different platforms/machs/SoCs. If a platform/mach/SoC can provide a 
clock rate that's acceptable to their hardware and software 
requirements, then they can be assured to find it without having to jump 
through hoops or having a driver not work when it could have.

Does the addition of one of the suggested APIs sound reasonable? If not, 
can someone explain what the right/better solution is? If the addition 
of a new API is reasonable, what's the community preference between the 
two suggestion? If I submit a patch that will add one of the APIs is it 
likely to be accepted?

Thanks for your time.

-Sarav

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

RFC: A better clock rounding API?

[Saravana Kannan]

Btw, if we are going to pass a rate range from the driver, we might as 
well make it clk_set_rate_range(). By passing the range, we rule out the 
possibility of any catastrophic rates -- so clk_find_rate() seems less 
useful.

Thanks,
Sarav
P.S: murphy++;
Found a correction just after I sent the email but not on any of the 
several iterations of proof reading that preceded it.


Saravana Kannan wrote:
> I'm mostly familiar with ARM.  So I will limit the discussion to ARM 
> boards/machines.  But I think the points raised in this email would 
> apply for most architectures.
> 
> I'm sending this to the ARM mailing list first to see if I can get a 
> consensus within the ARM community before I propose this to the wider 
> audience in LKML.
> 
> The meaning of clk_round_rate() is very ambiguous since the nature of 
> the rounding is architecture and platform specific.  In the case of ARM, 
> it's up to each ARM mach's clock driver to determine how it wants to 
> round the rate.
> 
> To quote Russel King from an email about a month ago:
> "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 when someone writes a device driver for a device that's external to 
> the SoC or is integrated into more than one SoC, they currently have the 
> following options to deal with clock rates differences:
> 
> 1. Use clk_round_rate() to get clock rates and test their driver on the 
> one/few board(s) they have at hand and hope it works on boards using 
> different SoCs.
> 
> 2. Add each and every needed clock rate (low power rate, high 
> performance rate, handshake rate, etc) as fields to their platform data 
> and have it populated in every board file that uses the device.
> 
> 3. Do a search of the frequency space of the clock by making several 
> clk_round_rate() calls at various intervals between their minimum and 
> maximum acceptable rates and hope to find one of the supported rates. 
> If clk_round_rate() does a +/- N percentage rounding and the interval is 
> larger, even this searching might not find an existing rate that's 
> supported between the driver's min and max acceptable rates.
> 
> IMHO, each of these options have short comings that could be alleviated 
> by adding a more definitive "rounding" API.  Also, considering that it's 
> the consumer of each clock that knows best what amount of rounding and 
> in which direction is acceptable, IMHO the current approach of hiding 
> the rounding inside the clock drivers seems counter intuitive.
> 
> I would like to propose the addition of either:
> 
> long clk_find_rate(struct clk *clk,
> 			unsigned long min_rate,
> 			long max_rate);
> 
> or
> 
> long clk_find_rate(struct clk *clk,
> 			unsigned long start_rate,
> 			long end_rate);
> 
> The advantage of the 2nd suggestion is that it allows a driver to 
> specify which end of a frequency range it prefers.  But I'm not sure how 
> important of an advantage that is.  So, proposing both and having the 
> community decide which one, if any, is acceptable.
> 
> If the clk_find_rate() API is available, the driver developer wouldn't 
> have to worry about figuring out a way for the clk_set_rate() to work on 
> different platforms/machs/SoCs. If a platform/mach/SoC can provide a 
> clock rate that's acceptable to their hardware and software 
> requirements, then they can be assured to find it without having to jump 
> through hoops or having a driver not work when it could have.
> 
> Does the addition of one of the suggested APIs sound reasonable? If not, 
> can someone explain what the right/better solution is? If the addition 
> of a new API is reasonable, what's the community preference between the 
> two suggestion? If I submit a patch that will add one of the APIs is it 
> likely to be accepted?
> 
> Thanks for your time.
> 
> -Sarav
> 


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

RFC: A better clock rounding API?

[Rob Herring]

Saravana,

On Wed, Aug 18, 2010 at 12:39 AM, Saravana Kannan
<skannan@codeaurora.org> wrote:
> I'm mostly familiar with ARM. ?So I will limit the discussion to ARM
> boards/machines. ?But I think the points raised in this email would apply
> for most architectures.
>
> I'm sending this to the ARM mailing list first to see if I can get a
> consensus within the ARM community before I propose this to the wider
> audience in LKML.
>
> The meaning of clk_round_rate() is very ambiguous since the nature of the
> rounding is architecture and platform specific. ?In the case of ARM, it's up
> to each ARM mach's clock driver to determine how it wants to round the rate.
>
> To quote Russel King from an email about a month ago:
> "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 when someone writes a device driver for a device that's external to the
> SoC or is integrated into more than one SoC, they currently have the
> following options to deal with clock rates differences:
>
> 1. Use clk_round_rate() to get clock rates and test their driver on the
> one/few board(s) they have at hand and hope it works on boards using
> different SoCs.
>
> 2. Add each and every needed clock rate (low power rate, high performance
> rate, handshake rate, etc) as fields to their platform data and have it
> populated in every board file that uses the device.
>
> 3. Do a search of the frequency space of the clock by making several
> clk_round_rate() calls at various intervals between their minimum and
> maximum acceptable rates and hope to find one of the supported rates. If
> clk_round_rate() does a +/- N percentage rounding and the interval is
> larger, even this searching might not find an existing rate that's supported
> between the driver's min and max acceptable rates.
>
> IMHO, each of these options have short comings that could be alleviated by
> adding a more definitive "rounding" API. ?Also, considering that it's the
> consumer of each clock that knows best what amount of rounding and in which
> direction is acceptable, IMHO the current approach of hiding the rounding
> inside the clock drivers seems counter intuitive.
>
> I would like to propose the addition of either:
>
> long clk_find_rate(struct clk *clk,
> ? ? ? ? ? ? ? ? ? ? ? ?unsigned long min_rate,
> ? ? ? ? ? ? ? ? ? ? ? ?long max_rate);
>
> or
>
> long clk_find_rate(struct clk *clk,
> ? ? ? ? ? ? ? ? ? ? ? ?unsigned long start_rate,
> ? ? ? ? ? ? ? ? ? ? ? ?long end_rate);
>
> The advantage of the 2nd suggestion is that it allows a driver to specify
> which end of a frequency range it prefers. ?But I'm not sure how important
> of an advantage that is. ?So, proposing both and having the community decide
> which one, if any, is acceptable.
>
> If the clk_find_rate() API is available, the driver developer wouldn't have
> to worry about figuring out a way for the clk_set_rate() to work on
> different platforms/machs/SoCs. If a platform/mach/SoC can provide a clock
> rate that's acceptable to their hardware and software requirements, then
> they can be assured to find it without having to jump through hoops or
> having a driver not work when it could have.
>
> Does the addition of one of the suggested APIs sound reasonable? If not, can
> someone explain what the right/better solution is? If the addition of a new
> API is reasonable, what's the community preference between the two
> suggestion? If I submit a patch that will add one of the APIs is it likely
> to be accepted?
>
> Thanks for your time.
>

This is still ambiguous. If there are multiple valid frequencies in
the range, is the preferred rate returned the lowest rate or highest
rate in the range? For something like an SD bus clock you would want
the maximum rate within the range. For something like a LCD pixel
clock or CMOS sensor input clock, you typically only need to be
greater than a certain minimum freq and for power reasons you want it
to be closest to the minimum.

Perhaps adding a flag to clk_round_rate to indicate to round up, down,
or smallest delta would be sufficient.

Rob

RFC: A better clock rounding API?

[Saravana Kannan]

Rob,

Rob Herring wrote:
> Saravana,
> 
> On Wed, Aug 18, 2010 at 12:39 AM, Saravana Kannan
> <skannan@codeaurora.org> wrote:
>> I'm mostly familiar with ARM.  So I will limit the discussion to ARM
>> boards/machines.  But I think the points raised in this email would apply
>> for most architectures.
>>
>> I'm sending this to the ARM mailing list first to see if I can get a
>> consensus within the ARM community before I propose this to the wider
>> audience in LKML.
>>
>> The meaning of clk_round_rate() is very ambiguous since the nature of the
>> rounding is architecture and platform specific.  In the case of ARM, it's up
>> to each ARM mach's clock driver to determine how it wants to round the rate.
>>
>> To quote Russel King from an email about a month ago:
>> "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 when someone writes a device driver for a device that's external to the
>> SoC or is integrated into more than one SoC, they currently have the
>> following options to deal with clock rates differences:
>>
>> 1. Use clk_round_rate() to get clock rates and test their driver on the
>> one/few board(s) they have at hand and hope it works on boards using
>> different SoCs.
>>
>> 2. Add each and every needed clock rate (low power rate, high performance
>> rate, handshake rate, etc) as fields to their platform data and have it
>> populated in every board file that uses the device.
>>
>> 3. Do a search of the frequency space of the clock by making several
>> clk_round_rate() calls at various intervals between their minimum and
>> maximum acceptable rates and hope to find one of the supported rates. If
>> clk_round_rate() does a +/- N percentage rounding and the interval is
>> larger, even this searching might not find an existing rate that's supported
>> between the driver's min and max acceptable rates.
>>
>> IMHO, each of these options have short comings that could be alleviated by
>> adding a more definitive "rounding" API.  Also, considering that it's the
>> consumer of each clock that knows best what amount of rounding and in which
>> direction is acceptable, IMHO the current approach of hiding the rounding
>> inside the clock drivers seems counter intuitive.
>>
>> I would like to propose the addition of either:
>>
>> long clk_find_rate(struct clk *clk,
>>                        unsigned long min_rate,
>>                        long max_rate);
>>
>> or
>>
>> long clk_find_rate(struct clk *clk,
>>                        unsigned long start_rate,
>>                        long end_rate);
>>
>> The advantage of the 2nd suggestion is that it allows a driver to specify
>> which end of a frequency range it prefers.  But I'm not sure how important
>> of an advantage that is.  So, proposing both and having the community decide
>> which one, if any, is acceptable.
>>
>> If the clk_find_rate() API is available, the driver developer wouldn't have
>> to worry about figuring out a way for the clk_set_rate() to work on
>> different platforms/machs/SoCs. If a platform/mach/SoC can provide a clock
>> rate that's acceptable to their hardware and software requirements, then
>> they can be assured to find it without having to jump through hoops or
>> having a driver not work when it could have.
>>
>> Does the addition of one of the suggested APIs sound reasonable? If not, can
>> someone explain what the right/better solution is? If the addition of a new
>> API is reasonable, what's the community preference between the two
>> suggestion? If I submit a patch that will add one of the APIs is it likely
>> to be accepted?
>>
>> Thanks for your time.
>>
> 
> This is still ambiguous. If there are multiple valid frequencies in
> the range, is the preferred rate returned the lowest rate or highest
> rate in the range? For something like an SD bus clock you would want
> the maximum rate within the range. For something like a LCD pixel
> clock or CMOS sensor input clock, you typically only need to be
> greater than a certain minimum freq and for power reasons you want it
> to be closest to the minimum.

Thanks for the feed back. This ambiguity is what my alternate proposal 
handles. To repeat the 2nd option:

long clk_find_rate(struct clk *clk,
                         unsigned long start_rate,
                         long end_rate);

With the above API, the clock driver tries to find a freq between 
start_rate and end_rate, but starts the search from "start_rate".

So, if you prefer lower freqs, you call it as:
clk_find_rate(clk, 100000, 150000);

and if you prefer higher freqs, you call it as:
clk_find_rate(clk, 150000, 100000);

That should take care of your concerns, right?

-Saravana

RFC: A better clock rounding API?

[Rob Herring]

Saravana,

On Fri, Aug 20, 2010 at 5:01 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> Rob,
>
>> This is still ambiguous. If there are multiple valid frequencies in
>> the range, is the preferred rate returned the lowest rate or highest
>> rate in the range? For something like an SD bus clock you would want
>> the maximum rate within the range. For something like a LCD pixel
>> clock or CMOS sensor input clock, you typically only need to be
>> greater than a certain minimum freq and for power reasons you want it
>> to be closest to the minimum.
>
> Thanks for the feed back. This ambiguity is what my alternate proposal
> handles. To repeat the 2nd option:
>
> long clk_find_rate(struct clk *clk,
> ? ? ? ? ? ? ? ? ? ? ? ?unsigned long start_rate,
> ? ? ? ? ? ? ? ? ? ? ? ?long end_rate);
>
> With the above API, the clock driver tries to find a freq between start_rate
> and end_rate, but starts the search from "start_rate".
>
> So, if you prefer lower freqs, you call it as:
> clk_find_rate(clk, 100000, 150000);
>
> and if you prefer higher freqs, you call it as:
> clk_find_rate(clk, 150000, 100000);
>
> That should take care of your concerns, right?
>
> -Saravana

Okay, that does address my concern, but I think that the use of the
function is not completely obvious without explanation and subject to
coding mistakes. Also, considering clk_round_rate would still be
ambiguous and with efforts to unify struct clk, perhaps it makes more
sense to fix all users and implementations of clk_round_rate rather
than add a new function which will duplicate some portion of
clk_round_rate implementations.

Rob

RFC: A better clock rounding API?

[Saravana Kannan]

Rob,

Rob Herring wrote:
> Saravana,
> 
> On Fri, Aug 20, 2010 at 5:01 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> Rob,
>>
>>> This is still ambiguous. If there are multiple valid frequencies in
>>> the range, is the preferred rate returned the lowest rate or highest
>>> rate in the range? For something like an SD bus clock you would want
>>> the maximum rate within the range. For something like a LCD pixel
>>> clock or CMOS sensor input clock, you typically only need to be
>>> greater than a certain minimum freq and for power reasons you want it
>>> to be closest to the minimum.
>> Thanks for the feed back. This ambiguity is what my alternate proposal
>> handles. To repeat the 2nd option:
>>
>> long clk_find_rate(struct clk *clk,
>>                        unsigned long start_rate,
>>                        long end_rate);
>>
>> With the above API, the clock driver tries to find a freq between start_rate
>> and end_rate, but starts the search from "start_rate".
>>
>> So, if you prefer lower freqs, you call it as:
>> clk_find_rate(clk, 100000, 150000);
>>
>> and if you prefer higher freqs, you call it as:
>> clk_find_rate(clk, 150000, 100000);
>>
>> That should take care of your concerns, right?
>>
>> -Saravana
> 
> Okay, that does address my concern, but I think that the use of the
> function is not completely obvious without explanation
We can resolve this by making sure the inline documentation in clk.h is 
good.

> and subject to
> coding mistakes.
I think this is true for any API. If an API is called with wrong 
parameters without understanding it, then it's not going to work.

> Also, considering clk_round_rate would still be
> ambiguous and with efforts to unify struct clk, perhaps it makes more
> sense to fix all users and implementations of clk_round_rate rather
> than add a new function which will duplicate some portion of
> clk_round_rate implementations.

clk_round_rate() in inherently broken because it doesn't provide the 
clock driver with enough information (consumer's freq tolerance) to make 
the right decision. I don't think there is a right way to fix it with 
the existing parameters.

To address your concern, we could mark clk_round_rate() as deprecated 
and remove it when everyone has converted their device drivers to use 
clk_find_freq().

Anyway, I think we are putting the cart before the horse here. I'm 
waiting for Russell King and possibly others to first agree to the 
proposed API before deciding what we are going to do with the users of 
the old API.

Russell and other active clock developers,

Any comments?

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.