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

[Jerome Brunet]

On Tue, 2017-08-08 at 19:19 -0700, Stephen Boyd wrote:
> On 08/08, 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.co
> > > > m
> > > > [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.
> 
> I seem to recall this being about something with how OMAP could
> change rates only high up in the tree and that would trickle down
> to multiple different clks that all had to use that rate and then
> divide it themselves to achieve the rate they actually wanted. Is
> that right? 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.
> 
> > 
> > > 
> > > 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."
> 
> This sounds like we're fixing the CLK_SET_RATE_GATE flag. I'd
> prefer we also describe how adding that flag to some clk doesn't
> solve the problem. I'm all more commit text when introducing this
> API. Please don't go super verbose, but at least describe the
> real problem.

Yes we are, with another patch later in this series.
CLK_SET_RATE_GATE is a particular case of clock protection, where the clock rate
is protected by provider (kind of the clock protecting itself) instead of being
protected by the consumer.

> 
> > 
> > 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:
> 
> I'm not really concerned about rate range handling this problem
> here. Let's try to untangle the two in this discussion please.
> 
> > 
> > > 
> > > > 
> > > > 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.
> 
> Ok. Can we get details on where we can't fix it in the provider
> layer? I'm fine to ignore the PLL sharing thing, if we can
> clearly describe a real situation where a clk consumer needs to
> lock in a rate, and that isn't the only consumer of a clk that is
> changing that rate or causing a glitch by changing some other
> rate up the tree. So far we haven't come across this case,
> presumably because clk providers are working around the problem
> by hardcoding parents that can change rate during
> determine_rate(), or blocking rate changes going up the tree for
> certain clks, or the hardware isn't designed in a way that
> something is shared while two devices are active, or maybe nobody
> has complained (likely!).
> 
> I'm all for consumers locking in rates in a critical section,
> just that I haven't seen a need for it so far. I've mostly seen
> that hardware doesn't design it this way, or that there's some
> sort of hardware plan to make sure that desired frequencies can
> be achieved when there's some shared resource. And the glitching
> thing hasn't been a problem because consumers are the only ones
> causing the glitch when they call clk_set_rate() and so far they
> have only called clk_set_rate() when their hardware is able to
> handle the glitch that comes down (e.g. audio turns off speaker
> output, or device is held in reset).
> 
> TL;DR: Please distract me from the PLL sharing thing with the
> real problem that's being solved.

Well, the problem is not sharing "per-se" but constraints resolution.
When walking the tree, CCF check what each clock can provide and decide what the
best setting is to provide the clock down the line.

The fact the several clocks may share/depend on the same parent is another
constraint in the tree. When clock can't change or glitch, it adds constraints
along the tree. This is what clock_protect() does.

A use-case of this, without "clock sharing" is the one I already explained is
this thread:

Say you have 2 clocks fed by a divider (SET_RATE_PARENT of course)
# clock #1 is a PLL locked to 48000kHz
# clock #2 is another clock (whatever)

clock #2 request a new rate which may change the parent divider...

With the current CCF code, the divider would change and just screw up the PLL
(which would end up with another rate)
Say, we could automatically relock the PLL at the initial rate (with clk_range
for example), we would still get a glitch.
This would not happen with clock protect, clock #2 would have to work with what
the divider provides at the moment ... or "determine" a better suited parent if
available.

So what it solves is quite simple: 
* Once consumer protect a clock it has the guarantee from CCF that no-one will
be messing with what comes out of the said clock.
* For CCF, it provides a more accurate view of the clock tree constraints when
trying to determine a new clock rate.

> 
> > 
> > > 
> > > > 
> > > > 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.
> > 
> 
> No need to remove the API. This is a good description of a case
> where a consumer would just call clk_rate_protect() (or perhaps
> clk_dont_glitch()) without caring what the rate is. I was missing
> this part of the argument because I got hung up on the critical
> section part. Maybe this can be added to the function
> documentation so we don't forget that some consumers don't set a
> rate but still care to make sure it doesn't glitch for them.

Sure, no problem.

>