Hello S?ren,

On Thu, May 22, 2014 at 11:03:00AM -0700, S?ren Brinkmann wrote:

On Wed, 2014-05-21 at 01:33PM -0700, Mike Turquette wrote:

quoted

Quoting Uwe Kleine-K?nig (2014-05-21 11:23:08)

quoted

Hello S?ren,

On Wed, May 21, 2014 at 08:58:10AM -0700, S?ren Brinkmann wrote:

quoted

On Wed, 2014-05-21 at 09:34AM +0200, Uwe Kleine-K?nig wrote:

quoted

On Tue, May 20, 2014 at 02:48:20PM -0700, S?ren Brinkmann wrote:

quoted

On Tue, 2014-05-20 at 10:48AM -0700, Stephen Boyd wrote:

quoted

On 05/20/14 09:01, S?ren Brinkmann wrote:

quoted

quoted

quoted

quoted

quoted

+{
+ unsigned long lower, upper, cur, lower_last, upper_last;
+
+ lower = clk_round_rate(clk, rate);
+ if (lower >= rate)
+         return lower;

Is the >-case worth a warning?

No, it's correct behavior. If you request a rate that is way lower than what the
clock can generate, returning something larger is perfectly valid, IMHO.
Which reveals one problem in this whole discussion. The API does not
require clk_round_rate() to round down. It is actually an implementation
choice that had been made for clk-divider.

I'm sure it's more than an implementation choice for clk-divider. But I
don't find any respective documentation (but I didn't try hard).

A similar discussion - without final conclusion:
https://lkml.org/lkml/2010/7/14/260

Please call this new API something like clk_find_nearest_rate() or
something. clk_round_rate() is supposed to return the rate that will be
set if you call clk_set_rate() with the same arguments. It's up to the
implementation to decide if that means rounding the rate up or down or
to the nearest value.

Sounds good to me. Are there any cases of clocks that round up? I think
that case would not be handled correctly. But I also don't see a use
case for such an implementation.

I don't really care which semantic (i.e. round up, round down or round
closest) is picked, but I'd vote that all should pick up the same. I
think the least surprising definition is to choose rounding down and add
the function that is under discussion here to get a nearest match.

So I suggest:

    - if round_rate is given a rate that is smaller than the
      smallest available rate, return 0
    - add WARN_ONCE to round_rate and set_rate if they return with a
      rate bigger than requested

Why do you think 0 is always valid? I think for a clock that can
generate 40, 70, 120, clk_round_rate(20) should return 40.

I didn't say it's a valid value. It just makes the it possible to check
for clk_round_rate(clk, rate) <= rate.

I grepped a bit around and found da850_round_armrate which implements a
round_rate callback returning the best match.
omap1_clk_round_rate_ckctl_arm can return a value < 0.
s3c2412_roundrate_usbsrc can return values that are bigger than
requested. (I wonder if that is a bug though.)

quoted

quoted

    - change the return values to unsigned long

Yep, I agree, this should happen.

And we're using 0 as error value? e.g. for the case where
omap1_clk_round_rate_ckctl_arm returns -EIO now?

No. clk_round_rate returns long for a reason, which is that we can
provide an error code to the caller. From include/linux/clk.h:

/**
 * clk_round_rate - adjust a rate to the exact rate a clock can provide
 * @clk: clock source
 * @rate: desired clock rate in Hz
 *
 * Returns rounded clock rate in Hz, or negative errno.
 */

This has the unfortunate side effect that the max value we can return
safely is 2147483647 (~2GHz). So another issue here is converting clock
rates to 64-bit values.

So, let's assume
 - a clock does either of these
   - round down
   - round nearest
   - round up (is there any such case? I don't see a use-case for this)
 - or return an error

I think my latest try handles such cases, with the limitation of
for a clock that rounds up, the up-rounded value is found instead of the
nearest.


static long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
{
	long ret;
	unsigned long lower, upper;

	clk_prepare_lock();

	lower = __clk_round_rate(clk, rate);

this is CCF specific while I don't see a need for it. (But yes, a
lock-less clk_find_nearest_rate is of course racy.)

	if (lower >= rate || (long)lower < 0) {

If you made lower and upper a signed long, you could drop the casting
here. BTW, why does __clk_round_rate return an unsigned long??
There seem to be several more type mismatches in that area.
Maybe we should add a waring if rate is > LONG_MAX?

(And ISTR that the C standard doesn't specify what the result of
(long)lower is given that lower is of type unsigned long and holding a
value > LONG_MAX.)

		ret = lower;
		goto unlock;
	}

	upper = rate + (rate - lower) - 1;
	if (upper > LONG_MAX)
		upper = LONG_MAX;

	upper = __clk_round_rate(clk, upper);
	if (upper <= lower || (long)upper < 0) {
		ret = lower;
		goto unlock;
	}

	lower = rate + 1;
	while (lower < upper) {
		unsigned 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;
}

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