Messages in this thread | | | From | Stephen Boyd <> | Subject | Re: [PATCH 0/1] clk: Meson8/8b/8m2: fix the mali clock flags | Date | Mon, 23 Dec 2019 19:36:35 -0800 |
| |
Quoting Jerome Brunet (2019-12-16 11:17:21) > > On Mon 16 Dec 2019 at 18:50, Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting Jerome Brunet (2019-12-16 01:13:31) > >> > >> *updated last* which crucial to your use case. > >> > >> I just wonder if this crucial part something CCF guarantee and you can > >> rely on it ... or if it might break in the future. > >> > >> Stephen, any thoughts on this ? > > > > We have problems with the order in which we call the set_rate clk_op. > > Sometimes clk providers want us to call from leaf to root but instead we > > call from root to leaf because of implementation reasons. Controlling > > the order in which clk operations are done is an unsolved problem. But > > yes, in the future I'd like to see us introduce the vaporware that is > > coordinated clk rates that would allow clk providers to decide what this > > order should be, instead of having to do this "root-to-leaf" update. > > Doing so would help us with the clk dividers that have some parent > > changing rate that causes the downstream device to be overclocked while > > we change the parent before the divider. > > > > If there are more assumptions like this about how the CCF is implemented > > then we'll have to be extra careful to not disturb the "normal" order of > > operations when introducing something that allows clk providers to > > modify it. > > I understand that CCR would, in theory, allow to define that sort of > details. Still defining (and documenting) the default behavior would be > nice. > > So the question is: > * Can we rely set_rate() doing a root-to-leaf update until CCR comes > around ? > * If not, for use cases like the one described by Martin, I guess we > are stuck with the notifier ? Or would you have something else to > propose ?
I suppose we should just state that clk_set_rate() should do a root-to-leaf update. It's not like anyone is interested in changing this behavior. The notifier is not ideal. I've wanted to add a new clk_op that would cover some amount of the notifier users by having a 'pre_set_rate' clk op that can mux the clk over to something safe or setup a divider to something that is known to be safe and work. Then we can avoid having to register for a notifier just to do something right before the root-to-leaf update happens.
> > > > > Also, isn't CLK_SET_RATE_GATE broken in the case that clk_set_rate() > > isn't called on that particular clk? I seem to recall that the flag only > > matters when it's applied to the "leaf" or entry point into the CCF from > > a consumer API. > > It did but not anymore > > > I've wanted to fix that but never gotten around to it. > > I fixed that already :P > CLK_SET_RATE_GATE is a special case of clock protect. The clock is > protecting itself so it is going down through the tree. >
Ahaha ok. As you can see I'm trying to forget clock protect ;-)
> > > The whole flag sort of irks me because I don't understand what consumers > > are supposed to do when this flag is set on a clk. How do they discover > > it? > > Actually (ATM) the consumer is not even aware of it. If a clock with > CLK_SET_RATE_GATE is enabled, it will return the current rate to > .round_rate() and .set_rate() ... as if it was fixed.
And then when the clk is disabled it will magically "unstick" and start to accept the same rate request again?
> > > They're supposed to "just know" and turn off the clk first and then > > call clk_set_rate()? > > ATM, yes ... if CCF cannot switch to another "unlocked" subtree (the > case here) > > > Why can't the framework do this all in the clk_set_rate() call? > > When there is multiple consumers the behavior would become a bit > difficult to predict and drivers may have troubles anticipating that, > maybe, the clock is locked.
Fun times!
| |