Re: [RFC 0/2] CLK_SET_RATE_GATE and protection against changes

[Michael Turquette <mturquette@baylibre.com>]

Quoting Jerome Brunet (2017-03-07 08:00:36)
> On Tue, 2017-03-07 at 06:38 -0800, Stephen Boyd wrote:
> > On 03/02, Jerome Brunet wrote:
> > > =

> > > I tried to understand what happened but my understanding of CCF is
> > > limited,
> > > if the following is complete nonsense, feel free to (gently) mock
> > > me.
> > > CLK_SET_RATE_GATE only prevent rate change when the clock is busy
> > > and we
> > > through clk_core_set_rate_nolock. So if we call clk_set_rate
> > > directly on
> > > clock with CLK_SET_RATE_GATE, while another clock uses it, it shall
> > > fail. However if we reach this clock by walking up the clock tree,
> > > everything seems to be as if this flag did not exist. I think this
> > > explains
> > > why mpll0 was selected has best parent and updated.
> > =

> > Right. My understanding is that this is the desired behavior of
> > this flag. At least, this is what I recall when speaking with
> > Mike about this a year or two ago.
> > =

> > A few months ago, Jiada Wang reported a similar problem[1] and
> > I've never merged it because of the concern it will break
> > something due to the flag behavior changing. Perhaps the way
> > forward here is to add a new flag for this different behavior and
> > let drivers opt-in to it.
> > =

> =

> In this previous thread, you mentioned the idea of deleting the flag.
> While a bit radical, I kind of like it. The name, as it is, is
> misleading. The flag does not really enforce gating the clock to change
> the rate.
> Changing the behavior of the flag is also too agressive I guess.
> =

> What about renaming the current flag to CLK_SET_RATE_GATE_LEAF (or
> anything else) and clearly mark it as obsolete in the header with a bit
> of explanation ?
> =

> We could keep that old behavior around while providers ask themself
> whether they really need to gate to change rate or not.

Adding a new flag is safer, but we might start accruing more and more
technical debt with deprecated flags versus the new ones. We have some
of this already with .round_rate vs .determine_rate and some other
stuff.

Since -rc1 juuuuust came out, maybe we could try merging it and see what
happens?

Also, if we can come up with a better solution that covers all the use
cases, I would be fine to delete the flag altogether and cover the
existing users. I count only a handful:

wm831x, qcom, at91, sirf, acpi-lpss, axi-clkgen, cs2000, bcm, stm32,
h8300, imx, microchip, ux500, and the mediatek drm drivers.

OK, maybe more than handful ;-)

> =

> > > =

> > > In patch 1, I try in intercept the calls to .round_rate and
> > > .determine_rate
> > > and just return the current rate of the clock when it is busy.=C2=A0=
=C2=A0The
> > > way the
> > > clock remains usable with the consumer can deal with the current
> > > but the
> > > rate won't change for the consumer already using the
> > > clock.=C2=A0=C2=A0Because of
> > > this change, mpll0 is no longer the best parent out there.
> > > =

> > > fixed_pll=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A03=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A03=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A02000000000
> > > =C2=A0=C2=A0=C2=A0mpll2=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00
> > > =C2=A0=C2=A0=C2=A0mpll1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A036863870
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cts_mclk_i958_sel=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A036863870
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cts_mclk_i958_d=
iv=C2=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A06143979
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0cts_mclk_i958=C2=A0=C2=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A00=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A06143979
> > > =C2=A0=C2=A0=C2=A0mpll0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0491495425
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cts_amclk_sel=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0491495425
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cts_amclk_div=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A012287386
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0cts_amclk=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A01=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A012287386
> > > =

> > > This is the result I expected :) However, the situation is still
> > > not ideal
> > > as I think using CLK_SET_RATE_GATE to protect against rate changes
> > > in such
> > > case is subject to race condition.
> > > =

> > > Suppose that I start both playbacks at the same time, i2s sets its
> > > rate but
> > > get descheduled before enabling the clock. Then spdif get
> > > scheduled, set
> > > the rate on the same pll (it can as the prepare/enable count is
> > > still 0)
> > > and enables the clock.=C2=A0=C2=A0Finally, i2s gets scheduled again, =
enables
> > > its
> > > clock but the rate of the selected parent has changed behind our
> > > back.=C2=A0=C2=A0I
> > > don't really know how to solve this one. I was thinking of another
> > > counter
> > > (like owner_count) but we already have 2 of those, there must be
> > > something
> > > smarter we can do about it... I guess.
> > =

> > Solving this problem is never fun. One "solution" is to use clk
> > notifiers to block rate changes that are undesirable. Overall,
> > that isn't really great though because we are using notifiers,
> > and it doesn't allow us to resolve what the rate changes should
> > do. Instead, we can just say yes or no.
> =

> Wouldn't it be too late anyway ? The goal would be to force the second

It's too late to select another parent, but not too late for any
affected drivers to clean up their work and pause operations while the
clock rate change happens.

> consumer to switch a better suited parent. if we just notify "no" while
> changing the rate, set_rate will return a error I suppose ? Even if
> could have chosen another parent and be successful ? Something needs to
> happen in round_rate/determine_rate, doesn't it ?

If the goal of the notifiers was to select a different parent, then
you'd be correct. However the goal of the notifiers is to allow
downstream drivers a chance to survive an otherwise catastrophic clock
rate change. Imagine a system with only a single PLL that clocks
everything and you'll see why I designed them this way.

On the other hand we could just fail the second clk_set_rate operation
but that's not very nice is it? ;-)

> =

> > =

> > Do the hardware designers have a frequency plan in mind when
> > designing the hardware so that we would know the PLLs they
> > planned to use for particular clks? Or is the whole thing
> > completely open ended and they expect software to figure out the
> > configuration of the clk tree at runtime based on what
> > frequencies are required on the different leaf clks (i2s/spdif).

Maybe they have a plan. If so they haven't told us yet ;-)

Are you thinking about the ccr stuff here for discrete clock
plan/operating points?

> > =

> > It may also work to use clk_set_rate_range() to "lock" the rate
> > of a clk to specifically what frequency you want.
> =

> Would any other driver be prevented from calling clk_set_rate_range as
> well, if you have the kind race condition I mentioned ?

Ranges are tracked on a per-consumer basis and all consumers are taken
into account when rates are being changed. The he-who-writes-last-wins
problem doesn't exist with rate ranges.

> =

> >  I haven't
> > thought that through completely, but it may work enough to make
> > sure the rate can't change while still allowing other clks to get
> > rates they want by searching the tree for another source.
> =

> For this use case, I have to admit I was probably abusing the
> CLK_SET_RATE_GATE to fit my use case ;)
> =

> The clock does not need to be gated for the rate to change, but the
> rate can't change if a consumer depends on it.
> =

> Here another idea: yet another flag (CLK_SET_RATE_PROTECT).
> It would require the clock to be prepared to be allowed to set the
> rate. if prepare_count > 1, it would return the current rate, acting as
> fixed clock. This allows determine_rate to switch to better parent if
> available, or try to make the best out of what is available.

So I think this idea is getting somewhere, but it should not be a flag.
Instead we could have a clk_lock_rate() and clk_unlock_rate() function,
and even a nice helper named clk_set_rate_lock() that wraps
clk_set_rate() and clk_lock_rate().

In order to make it easy to track whether parents of a clock are allowed
to be changed by a sibling we should introduce a rate_lock_count member to
struct clk_core and incremented it up the parent chain exactly how we do
already for prepare_count and enable_count. Any time rate_lock_count > 0
then we cannot change that clock rate.

I admit that clk_lock_rate() could be satisfied by using a range where
min =3D=3D max, but the problem there is that we do not propagate rate
clocks up the parent chain to make it easy to figure out of rate changes
are acceptable.

Additionally the "range" semantics say nothing about whether a
downstream peripheral will glitch during a clock rate change during an
operation, which was the idea behind CLK_SET_RATE_GATE, but that flag
doesn't handle the sibling-blows-up-everything corner case.

So I propose the following:

0) introduce clk_lock_rate() and clk_unlock_rate() along with struct
   clk_core->rate_lock_count. Add clk_set_rate_lock() helper
1) Repeal and replace CLK_SET_RATE_GATE users with clk_set_rate_lock()

The range stuff still makes sense and there are valid use cases where we
would want to specify a range as well as lock the frequency (e.g. a
glitchy downstream peripheral that tolerates a frequency band).

We'll have to take care to migrate the rate_lock_count in .set_parent,
but this should be easier versus the enable_count stuff that requires
holding both the spinlock and the mutex.

Thoughts?

Regards,
Mike

> =

> The problem I see with this approach is the case where 2 consumers
> prepare the clock w/o being able to set the rate =3D> no consumer is
> satisfied by the clock is locked. Could it be solved by adding a
> "prepare_set_rate" to the CCF api ?
> =

> > =

> > [1] https://patchwork.kernel.org/patch/9222903/
> > [2] https://patchwork.kernel.org/patch/9295171/
> >=20