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

[Michael Turquette]

Hi Stephen,

Quoting Stephen Boyd (2017-08-03 17:18:36)
> On 07/26, Jerome Brunet wrote:
> > > > +void clk_rate_protect(struct clk *clk);
> > > 
> > > Is there any plan to use this clk_rate_protect() API? It seems
> > > inherently racy for a clk consumer to call clk_set_rate() and
> > > then this clk_rate_protect() API after that to lock the rate in.
> > > How about we leave this out of the consumer API until a user
> > > needs it?
> > 
> > Having this API available is whole reason I've been working on this for so long.
> > By the now, you may have forgot but I explained the use-case in first RFC [0]
> > Here is an example (wip) of usage [1]
> > 
> > [0]: http://lkml.kernel.org/r/20170302173835.18313-1-jbrunet@baylibre.com
> > [1]: https://github.com/jeromebrunet/linux/commits/amlogic/wip/audio-clk-lock

Indeed, something like rate protection or "lock rate" has been discussed
since the birth of CCF. I remember a whiteboarding session between you,
Paul W. and myself about it probably in 2012. Peter might have been
there too.

> 
> If we're forgetting why something is introduced then it means the
> commit text is missing information. Please clearly describe the
> need for the API in the commit text for the patch that introduces
> it.

My $0.02 is that the "pick an unused PLL" thing is a benefit of this new
api that is internal to the clock controller driver, and is not the
driving force behind the series. If a simple, easy to understand
justification for this patch series is needed, might I suggest something
like the following for the next commit log/coverletter:

"Some clock consumers require that a clock rate must not deviate from
its selected frequency. There can be several reasons for this, not least
of which is that some hardware may not be able to handle or recover from
a glitch caused by changing the clock rate while the hardware is in
operation. The ability to lock a clock's rate, and release that lock, is
a fundamental clock rate control primitive. It's absence is a bug that
is fixed by this patch series."

That's the short and sweet version. If more verbosity is needed as to
why rate_range doesn't need this, there are some good reasons:

1) simplicity: some consumers don't care about their rate, but do
care that they rate doesn't change. clk_rate_{un}protect is much simpler
than forcing those consumers to call clk_get_rate, then cache that value
for future use and then call clk_set_rate_range.

2) expressiveness / debug: trying to find out why a clock rate is locked
searching through every use of clk_set_rate_range is sort of lame,
especially if variables are used to pass in min/max instead of
hard-coded values. It's way way easier to just grep for
clk_rate_protect.

> 
> > 
> > > 
> > > Finally, When does a consumer want the rate of a clk to change
> > > after they call clk_set_rate() on it? I would guess that very few
> > > consumers would be willing to accept that. Which begs the
> > > question, if anyone will keep calling clk_set_rate() after this
> > > API (and the clk_set_rate_protect() API) is added. It almost
> > > seems like we would want it to be opt-out, instead of opt-in, so
> > > that consumers would call clk_set_rate() and expect it to be a
> > > stable clk rate after that, and they would call
> > > clk_set_rate_trample_on_me() or something properly named when
> > > they don't care what the rate is after they call the API.
> > > 
> > 
> > Indeed, we generally don't want our rate to change, but:
> > - This is mostly for leaf clocks, the internal path would generally not care, as
> > long as the leaf are happy.
> > - Even a leaf may be open (be able to deal with) to small glitches, pll relock,
> > re parenting
> > 
> > Actually, if all the clock could not tolerate any glitches, there would have
> > been a lot of complains about CCF by now, it does not prevent glitches at all.
> 
> Well some devices handle glitches in the hardware, so the details
> of glitch free rate changes are hidden from clk consumers, and
> the software in general, on those devices.

On the hardware that I am familiar with, the problem of glitches lies
not in the clock hardware, but with the downstream peripheral logic / ip
block that consumes that clock signal. So it seems to me that having a
consumer api for locking the rate makes perfect sense.

> 
> > 
> > If you go over the initial RFC, the point is also for the CCF to favor other
> > (unused parent) when several (sometimes with the same capabilities) are
> > available. I think this is also a fairly common use case. That's something
> > rate_range won't do either, as far as I understood.
> 
> Fair enough, but why do we want consumers to need to know that
> there are sometimes unused parents that aren't getting chosen for
> a particular frequency? I see this as exposing the internals of
> the clk tree to consumers when they shouldn't need to care. Of
> course, sometimes clk consumers really do care about internals,

Right, sometimes they do, and we need to strike a balance. I think that
this api has been needed for some time. It very likely could have been
included in the initial version of the CCF that was merged years back if
I hadn't been trying very hard to stick only to the existing clk.h.

clk_set_rate has always been a "last write wins" api, across a shared
resource, and we've always known that this is not a great situation.
This patch series does a good job of solving that issue, in conjunction
with the existing range stuff.

> for example if some PLL is used for a display controller and it's
> also routed out of the chip on the display phy pins to encode
> data or something. Then perhaps we really want to use one
> particular PLL instead of a generic one that may also be a parent
> of the display controller clk. Making sure clk_set_rate() doesn't
> trample on clks deep in the tree seems different than this
> though.
> 
> Going back to your RFC series cover letter though, I see three
> PLLs (mppl0,1,2) and two users of the PLLs (amclk and mclk_i958).
> Is anything else using these PLLs in the system? Why are we doing
> all this stuff instead of hard-coding the parents for these clks
> to be different PLLs? If we want it to be flexible we could
> assign parents to the cts_amclk_sel and cts_mclk_i958_sel to be
> different PLLs in DT via assigned clock parents. Or it could just
> be hard-coded in the clk driver during probe.
> 
> If there's really sharing going on, and you can't hardcode the
> parents, please indicate why that's the case in the commit text.
> I don't want to introduce another consumer API just because we
> didn't want to restrict the available parents for a couple clks.

I think the PLL sharing thing is a big distraction. Giving consumers the
ability to guarantee that their rates are locked in using a simple
critical section call just makes sense to me. If it helps with other
cases then yay.

> 
> > 
> > Last but not least, it allows consumer to set the rate in a sort of critical
> > section and have the guarantee that nobody will be able to change the rate
> > between the clk_set_rate() call and prepare_enable(). That's something we don't
> > have at the moment.
> > 
> 
> Right, but clk_rate_protect() doesn't close the critical section
> between clk_set_rate() and clk_rate_protect() if another
> consumers changes the frequency, or if that consumer changes the
> frequency of some parent of the clk. This is why I'm asking what

Right, clk_set_rate_protect does this in the next patch in this series.

> the use of this API is for. Shouldn't we want consumers to use
> clk_set_rate_protect() so they can be sure they got the rate they
> wanted, instead of hoping that something else hasn't come in
> between the set_rate and the protect calls and changed the
> frequency?

We can remove clk_rate_protect if you really want, but there is always
the case that a consumer driver does not set the rate, but needs to
guarantee that the rate will not change during operation.

Any driver that must both set the rate AND guarantee it will not change
must use clk_set_rate_protect.

Regards,
Mike

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project