[PATCH v3 05/10] clk: add support for clock protection

[jbrunet@baylibre.com (Jerome Brunet)]

On Wed, 2017-08-09 at 12:45 +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 08, 2017 at 07:19:06PM -0700, Stephen Boyd wrote:
> > I also vaguely remember Paul saying that
> > clk_round_rate() could return something and then clk_set_rate()
> > after that would fail because what was calculated during the rate
> > speculation/rounding phase would be different if some other
> > consumer goes and changes some rate high up in the tree.
> 
> That's probably because people tend to get this stuff wrong.??It is
> _not_ supposed to be:
> 
> 	rounded_rate = clk_round_rate(clk, requested_rate);
> 
> 	clk_set_rate(clk, rounded_rate);
> 
> but:
> 
> 	rounded_rate = clk_round_rate(clk, requested_rate);
> 
> 	clk_set_rate(clk, requested_rate);
> 
> The former is wrong for two reasons:
> 
> 1. it's completely wasteful of CPU resources to do all the calculations
> ???that need to be done within clk_set_rate().
> 
> 2. it's racy - there is no guarantee that you'll end up with "rounded_rate"
> 
> The API definition of clk_round_rate() explicitly mentions that it is
> equivalent to clk_set_rate() followed by clk_get_rate() with the
> exception that it doesn't affect the hardware.
> 
> I'm not sure that the clock rate protection API is really the right
> solution - if we're trying to stop others from changing the clock rate,
> that implies we have multiple different threads potentially changing
> the rate at any time.??If a driver does this:
> 
> 	clk_set_rate(clk, foo);
> 	clk_rate_protect(clk);
> 
> what prevents another thread from changing the clock rate between these
> two calls???The only way to do this safely would be something like:
> 
> 	r = clk_round_rate(clk, foo);
> 	while (1) {
> 		err = clk_set_rate(clk, foo);
> 		clk_rate_protect(clk);
> 		if (err < 0)
> 			break;
> 
> 		if (r == clk_get_rate(clk)) /* success */
> 			break;
> 
> 		clk_rate_unprotect(clk);
> 	}
> 
> 	if (err)
> 		failed;

Russell,
I think you have missed one subtle thing, when trying any clock altering
operation, if the consumer is protecting the clock, it will temporarily release 
the protection once, under the prepare_lock (to guarantee safe operation). This
is explained in the cover letter:

"""
With this series there is 3 use-case:
?- the provider is not protected: nothing changes
?- the provider is protected by only 1 consumer (and only once), then only
???this consumer will be able to alter the rate of the clock, as it is the
???only one depending on it.
?- If the provider is protected more than once, or by the provider itself,
???the rate is basically locked and protected against reparenting.
"""

So what you should do if you have to protect the clock is:

clk_rate_protect(clk);
err = clk_set_rate(clk, foo);

[...]
clk_rate_unprotect(clk);

If the clock rate could be changed (protected only once) err = 0;
If the clock rate could not be changed: err = -EBUSY;

if you wish to combine clk_rate_protect() and clk_set_rate(), you may use
clk_set_rate_protect() which is added in another patch of this series.

> 
> That's rather a lot of code to add to every driver, and given the
> number of times I've seen people get the clk_round_rate() vs
> clk_set_rate() thing wrong, I've zero confidence that folk will get
> this right either.
> 
> So, I'd suggest _not_ adding this clk_rate_protect() thing, but
> instead an API that simultaneously sets and protects the rate, so
> driver authors don't have to get involved in details like the above.
>