[PATCH v3 04/10] clk: use round rate to bail out early in set_rate

[jbrunet@baylibre.com (Jerome Brunet)]

On Tue, 2017-07-11 at 19:00 -0700, Stephen Boyd wrote:
> On 06/12, Jerome Brunet wrote:
> > The current implementation of clk_core_set_rate_nolock bails out early if
> 
> Add () to functions please.
> 
> > the requested rate is exactly the same as the one set. It should bail out
> > if the request would not result in rate a change.??This important when
> 
> s/in rate a change/in a rate change/
> s/This important/This is important/
> 
> > rate is not exactly what is requested, which is fairly common with PLLs.
> 
> s/rate/the rate/
> 
> > 
> > Ex: provider able to give any rate with steps of 100Hz
> > ?- 1st consumer request 48000Hz and gets it.
> > ?- 2nd consumer request 48010Hz as well. If we were to perform the usual
> > ???mechanism, we would get 48000Hz as well. The clock would not change so
> > ???there is no point performing any checks to make sure the clock can
> > ???change, we know it won't.
> 
> I think Peter reported a similar problem as to what you're
> describing. Can you further explain a problem situation that this
> patch is fixing based on that thread (I can find the link if
> needed)?
> Some of this series if fixing rate constraints actually,
> so please Cc him on future patch sets.

I had not seen the thread you referring to.
I assume Peter is Peter De Schrijver and the thread is:

http://lkml.kernel.org/r/1490103807-21821-1-git-send-email-pdeschrijver at nvidia.c
om

Peter is right, There is indeed a problem when the current rate is out of the
requested range.

I'm not sure about the proposed solution though. Even the rate set by
set_rate_range() should go trough the bail-out mechanism because of the use-case 
I have explained here.

After spending some time on it, I realized that patch 7 is useless, as the call
to clk_core_set_rate_nolock() with core->req_rate is a no-op and will never
fail.

We could request the rate to be changed to nearest rate range limit (here is a
candidate fix doing that [0]

But then we get to a more fundamental issue of the rate range mechanism.

Take the example of Peter:
* You had 500Mhz and you set a range of [100-300] MHz
* The logical thing to do would be to request the clock rate to change to
300MHz, right ?
* Hw capabilities being what they are, the rate is unfortunately rounded to
301Mhz.
* In the end, you are out of the range and the call fails.

That is when the clock only implement round_rate(). If it implement
determine_rate(), it could take the range into account when rounding the rate.
However, I doubt many clock drivers are taking care of this corner case, if any.

The clean way to address this would be to have all clock drivers use
determine_rate() and make sure they all that the case into account, and delete
the round_rate() ... not happening tomorrow.

The consistent way would be to systematically fail if the current rate is out of
the requested range ... a bit rude maybe.

The proposed patch [0] does it's best to change the rate, but may fail as
explained above ... 

For now, I have dropped patch 7 and pushed patch [0] at the end of the queue.
Since It is really related to the clock protect mechanism, we should probably
discuss this in another thread. 

[0]: https://github.com/jeromebrunet/linux/commit/235e477f346a9e8d115dda257f9f73
834151bd7f

> 
> > 
> > This is important to prepare the addition of the clock protection
> > mechanism
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > ?drivers/clk/clk.c | 23 +++++++++++++++++++++--
> > ?1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8cc4672414be..163cb9832f10 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
> > ?		clk_change_rate(core->new_child);
> > ?}
> > ?
> > +static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> > +						?????unsigned long
> > req_rate)
> > +{
> > +	int ret;
> > +	struct clk_rate_request req;
> > +
> > +	if (!core)
> > +		return 0;
> 
> This is impossible because the only call site checks for core
> being NULL already.
> 

This more or less like the previous comments on lockdep_assert.
Most (should be all) "_no_lock" function check the prepare_lock and the core
pointer. Even when it is not strictly necessary, I think we should be consistent
 about it.

In the unlikely event this function is used some place else, it would avoid bad
surprise

So?if it is OK with you, I would prefer to keep this check and add the check of
the prepare lock. Maybe I could go over the other "_nolock" functions in clk.c
to verify they are all doing it ? What do you think ?

> > +
> > +	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > +	req.rate = req_rate;
> > +
> > +	ret = clk_core_round_rate_nolock(core, &req);
> > +
> > +	return ret ? 0 : req.rate;
> 
> What if the return value is negative?

Here we are trying to return a rate, so unsigned long.
I think (and I remember discussing it with Mike sometimes ago) that a 0 clock
rate quite often represent an error.?


> We should bail the set rate
> call instead of continuing on?

I don't think this for this particular function to decide. It should try compute
what the rate would be. It's up to the calling function to decide what do with
this 0 (error)

To answer your question, I think that if we can't figure out the what the rate
would be, we should not error right away and just let the regular process happen
(no bail-out) ... If there is a problem, it will error anyway 

>