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

[Jerome Brunet]

On Tue, 2017-08-08 at 15:37 -0700, Michael Turquette wrote:
> 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-l
> > > ock
> 
> 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.

It guarantee that what is being protected, won't be broken ... for sure it helps
with sharing issues.

> 
> > 
> > > 
> > > 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.

Hum, I disagree here. For sure, if you call clk_set_rate() before
clk_rate_protect() you have not guarantee at all, but you are doing it wrong.
The way to use this it to call clk_rate_protect() then clk_set_rate().

As explained earlier, as long as the clock is protected only once, the
protecting consumer is still able to change the rate, because it is the only one
depending on clock.

This is a good fit for driver which heavily depends on rate being but need to
change the rate during during their operations. It is not necessary to unprotect
the clock before calling clk_set_rate().

clk_set_rate_protect() is only introduce to help driver which could get in the
situation where 2 consumers protect the clock w/o getting a chance to set the
rate, exhausting  the ressources. To re-set the rate, clk_rate_unprotect() must
be called before calling clk_set_rate_protect() again.

> 
> > 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