Message-ID: <5500EA26.7090007@codeaurora.org>
Date:	Wed, 11 Mar 2015 18:21:42 -0700
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
CC:	Mike Turquette <mturquette@...aro.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Sören Brinkmann <soren.brinkmann@...inx.com>,
	kernel@...gutronix.de
Subject: Re: [PATCH 0/3] clk: divider: three exactness fixes (and a rant)

On 03/09/15 16:34, Uwe Kleine-König wrote:
> Hello Stephen,
>
> On Mon, Mar 09, 2015 at 03:40:29PM -0700, Stephen Boyd wrote:
>> On 03/09/15 14:58, Uwe Kleine-König wrote:
>>> If you see
>>>
>>> 	round_rate(110) = 108
>>>
>>> it would be fortunate to know if you get 108 because the next available
>>> greater rate is > 112 or because the implementation rounds down always
>>> (which would mean that 111 is possible, too). For the "easy" consumers
>>> this probably doesn't matter much, but if you do things that affects
>>> a considerable part of the clock tree, you really want to know more
>>> about the behaviour of round_rate to effectively work with its results.
>>>
>>> So yes, please let us pick ceiling for round_rate (i.e. a fixed policy
>>> for all clks) and then it should even be possible to make
>>> clk_set_rate_range a generic function that doesn't need the min and max
>>> members in the clk struct and the respective parameters to
>>> determine_rate.
>>>
>>> What should a clock that can only provide 100 Hz return on
>>>
>>> 	clk_round_rate(clk, 60);
>>>
>>> ? 0? -ESOMETHING (for SOMETHING = ...?)?
>>>
>> Do you have any real world use cases, or is this just all theoretical?
> The question about clk_round_rate(fixed_clk_100hz, 60) is an
> implementation detail that we must handle after agreeing that
> clk_round_rate should always round down. I faced that when I tried to
> implement this rounding requirement for dividers.
>
>> At least in Philipp's panel case we can discuss how to make an API that
>> works properly. These other examples are either completely theoretical
>> or taken out of context and so it's unclear how they matter in practice.
> We can stick to Philipp's panel case if you want. Philipp wants to find
> a rate between 100 Hz and 120 Hz and likes 110 Hz most. 

Why does Philipp like 110Hz the most? Where is the desire for that rate
coming from?

> And the lower
> abs(1 / 110 - 1 / r) the better. 

Similarly, where is this requirement coming from? Some datasheet? Or is
it just some arbitrary decision we've made that may not hold true for
all consumers?

> Let's assume the clk is provided by a
> fixed clk with 10000 Hz that goes through two 4bit-dividers. (So no,
> that's not a real world use case, but I imagine that something like that
> can occur and should definitely be possible to handle.) Something
> similar happens if you have for example an i2c bus device that has a
> built-in divider. For the lowest consumer the situation is easy most of
> the time: It wants a certain frequency to update a panel, or it wants at
> most 100 kHz on the i2c bus. But already for the divider one step up
> the clk tree it's not that easy any more.
>
>> Ideally I'd like an API to exist that doesn't require going back and
>> forth with the framework (i.e. it's "atomic" and doesn't require calling
>> clk_round_rate() in a loop) and that allows consumers to properly
> Why is calling "clk_round_rate() in a loop" bad? In some situations you
> won't be able to do something different.

The problem is open coding that loop all over the kernel in slightly
different ways. Plus it doesn't even work for some platforms where the
parent for a clock could change between the call to clk_round_rate() and
clk_set_rate() and then clk_set_rate() would do something completely
different. It would be nice if clk_round_rate() was considered "stable"
but unfortunately on these platforms it isn't. This is the "atomic"
problem I mentioned.

>
>> express what they want. Right now we have a way to say min/max and a
>> typical rate is in the works. If we need to declare some sort of clock
>> provider rounding policy then we've failed to provide an API that
>> properly expresses all the requirements that the consumer has. It
> I think you don't want a way to express: "I want a frequency that I
> can divide down to 110 Hz with a divider picked from [1 ... 16].".
> And even if we somehow manage to create something like that in a sane
> way, I think the only reliable and maintainable way to get there is to
> not ask all clock types to implement this functionality.

No we don't want anything like consumers asking for a rate with a set of
division ratios.

> That is, I want the complexity at a single place in the framework and
> only ask easy things from the clk type implementors. A .round_rate
> callback is easy for most clk types. .determine_rate a bit less and it
> already promotes boilerplate because each implementation has to check
> for min_rate and max_rate. And .determine_rate as it is today doesn't
> even support the typical value yet.

Now we seem to be talking about framework internal details. We needed to
push the min/max (and typical if/when we add it) into the
.determine_rate ops because .determine_rate is 1) the replacement for
.round_rate and 2) may need to query a number of parent clocks to figure
out what rate is supported and make sure it fits within the constraints
it requires. To consolidate all this logic into one place on top of
.round_rate would completely miss the 'mux' capabilities of clocks. We
could have added min/max to .round_rate but that would have been hugely
invasive so it was deemed easier to fix up the handful of
.determine_rate ops for this new feature.

I don't see how .round_rate or .determine_rate is any harder to
implement than the other but I'm probably biased. Most likely we should
make the arguments to .determine_rate into a struct so that it doesn't
keep requiring a flag day to change parameters passed there. We can
probably allow .determine_rate to return values outside min/max too and
check it in the framework when we get the rate back if that's a concern.
I don't see a way around the min/max check if an op calls
__clk_determine_rate() on its parent though.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists