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

[jbrunet@baylibre.com (Jerome Brunet)]

On Wed, 2017-08-09 at 14:40 +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 09, 2017 at 03:34:48PM +0200, Jerome Brunet wrote:
> > 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);
> 
> So here you drop the protection, which means anyone can alter the clock
> again.
> 

That's just an example. The rate is set after clk_set_rate() if no other
consumer depends on the clock.

I just added clk_rate_unprotect() here to illustrate that the calls should be
balanced, as documented.

> Either that or "clk_rate_unprotect" is inappropriately named and doesn't
> do what it says it does.
>