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

[Russell King - ARM Linux]

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;

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up