Re: [PATCH 0/3] clk: divider: three exactness fixes (and a rant)

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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. And the lower
abs(1 / 110 - 1 / r) the better. 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.

> 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.
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.

> probably means we're missing some key parameter that consumers know but
> we don't accept. Maybe some more concrete examples will help clarify
> what this is.

Best regards
Uwe

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