[RFC 0/2] CLK_SET_RATE_GATE and protection against changes

[RFC 0/2] CLK_SET_RATE_GATE and protection against changes

[Jerome Brunet]

Hi,

I have been playing with CLK_SET_RATE_GATE, with the goal of protecting a
parent clock against rate change if the clock is already used. I did not
get the expected result.  Before explaining more my use case, I'd like to
point out that the patches attached to this RFC are just here to illustrate
what I understand of the problem and start a discussion. By no means I
expect them to be merged as it is.

I'm working on the audio support on Amlogic's meson platforms [0]. In CCF,
I have an i2s master clock and spdif clock. Both can take any of 3 PLLs as
parent. These 3 PLLs have the capabilities, and I have set the
"CLK_SET_RATE_GATE" on each of them.

After boot, here is what we have:
name                       | en_cnt | prepare_cnt | rate      |
===============================================================
fixed_pll                    2        2            2000000000
   mpll2                     0        0                     0
   mpll1                     0        0             346956926
   mpll0                     0        0             274510132

Here the audio clocks don't appear because of an undocumented boot value of
the mux in front of them Please ignore this.

Starting i2s playback:
fixed_pll                    3        3            2000000000
   mpll2                     0        0                     0
   mpll1                     0        0             346956926
   mpll0                     1        1             491495425
      cts_amclk_sel          1        1             491495425
         cts_amclk_div       1        1              12287386
            cts_amclk        1        1              12287386

Here the i2s master is attached to the mpll0 and pll is set to (roughly)
500Mhz. So far so good.

Now starting the spdif playback, I expected the mpll0 to be protected and
.round_rate/.determine_rate to return the current rate of mpll0. By going
through the possible parents, I expected CCF not to select mpll0 as the
best parent. Well, guess what happened ;)

Starting spdif playback
fixed_pll                    3        3            2000000000
    mpll2                    0        0                     0
    mpll1                    0        0             346956926
    mpll0                    2        2              36863870
       cts_mclk_i958_sel     1        1              36863870
          cts_mclk_i958_div  1        1               6143979
             cts_mclk_i958   1        1               6143979
       cts_amclk_sel         1        1              36863870
          cts_amclk_div      1        1                921597
             cts_amclk       1        1                921597

mpll0 got selected anyway, the rate changed and the i2s master clock
wrecked.

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.

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.  The 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.  Because of
this change, mpll0 is no longer the best parent out there.

fixed_pll                    3        3            2000000000
   mpll2                     0        0                     0
   mpll1                     0        0              36863870
      cts_mclk_i958_sel      0        0              36863870
         cts_mclk_i958_div   0        0               6143979
            cts_mclk_i958    0        0               6143979
   mpll0                     1        1             491495425
      cts_amclk_sel          1        1             491495425
         cts_amclk_div       1        1              12287386
            cts_amclk        1        1              12287386

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.  Finally, i2s gets scheduled again, enables its
clock but the rate of the selected parent has changed behind our back.  I
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.

If you read that far, thanks a lot !

Does any of what I just wrote make sense ?  Do you have any suggestion ?

Patch 2 is merely a suggestion. As explained in the patch, I think the
wording is bit confusing.

[0] : https://github.com/jeromebrunet/linux/commits/amlogic/wip/audio

Jerome Brunet (2):
  clk: fix CLK_SET_RATE_GATE on parent clocks
  clk: use enable_count to check if clk is busy

 drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

-- 
2.9.3

[RFC 1/2] clk: fix CLK_SET_RATE_GATE on parent clocks

[Jerome Brunet]

CLK_SET_RATE_GATE flag will only prevent a consumer from directly changing
the rate on the clock (if the clock is the leaf when calling clk_set_rate).
However the clock rate can be changed without being gated if it is a parent
of the leaf. In addition, other child clocks depending on the rate of
parent clock might not appreciate.

To address this issue, if the clock is busy, this patch stops the tree walk
while calculating the new rates, and return the current rate as if it was
fixed clock.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0fb39fe217d1..6fe2ea81a9af 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -172,6 +172,14 @@ static bool clk_core_is_enabled(struct clk_core *core)
 	return core->ops->is_enabled(core->hw);
 }
 
+static bool clk_core_rate_can_change(struct clk_core *core)
+{
+	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
+		return false;
+
+	return true;
+}
+
 /***    helper functions   ***/
 
 const char *__clk_get_name(const struct clk *clk)
@@ -833,11 +841,32 @@ static int clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
+static int clk_core_round_rate_query(struct clk_core *core,
+				     struct clk_rate_request *req)
+{
+	long rate;
+
+	if (!clk_core_rate_can_change(core)) {
+		req->rate = core->rate;
+	} else if (core->ops->determine_rate) {
+		return core->ops->determine_rate(core->hw, req);
+	} else if (core->ops->round_rate) {
+		rate = core->ops->round_rate(core->hw, req->rate,
+					     &req->best_parent_rate);
+		if (rate < 0)
+			return rate;
+
+		req->rate = rate;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
 static int clk_core_round_rate_nolock(struct clk_core *core,
 				      struct clk_rate_request *req)
 {
 	struct clk_core *parent;
-	long rate;
 
 	lockdep_assert_held(&prepare_lock);
 
@@ -853,15 +882,8 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 		req->best_parent_rate = 0;
 	}
 
-	if (core->ops->determine_rate) {
-		return core->ops->determine_rate(core->hw, req);
-	} else if (core->ops->round_rate) {
-		rate = core->ops->round_rate(core->hw, req->rate,
-					     &req->best_parent_rate);
-		if (rate < 0)
-			return rate;
-
-		req->rate = rate;
+	if (core->ops->determine_rate || core->ops->round_rate) {
+		return clk_core_round_rate_query(core, req);
 	} else if (core->flags & CLK_SET_RATE_PARENT) {
 		return clk_core_round_rate_nolock(parent, req);
 	} else {
@@ -1353,8 +1375,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 
 	clk_core_get_boundaries(core, &min_rate, &max_rate);
 
-	/* find the closest rate and parent clk/rate */
-	if (core->ops->determine_rate) {
+	if (core->ops->determine_rate || core->ops->round_rate) {
 		struct clk_rate_request req;
 
 		req.rate = rate;
@@ -1368,22 +1389,17 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 			req.best_parent_rate = 0;
 		}
 
-		ret = core->ops->determine_rate(core->hw, &req);
+		ret = clk_core_round_rate_query(core, &req);
 		if (ret < 0)
 			return NULL;
 
 		best_parent_rate = req.best_parent_rate;
 		new_rate = req.rate;
 		parent = req.best_parent_hw ? req.best_parent_hw->core : NULL;
-	} else if (core->ops->round_rate) {
-		ret = core->ops->round_rate(core->hw, rate,
-					    &best_parent_rate);
-		if (ret < 0)
-			return NULL;
 
-		new_rate = ret;
 		if (new_rate < min_rate || new_rate > max_rate)
 			return NULL;
+
 	} else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
 		/* pass-through clock without adjustable parent */
 		core->new_rate = core->rate;
@@ -1571,7 +1587,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
 
-	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
+	if (!clk_core_rate_can_change(core))
 		return -EBUSY;
 
 	/* calculate new rates and get the topmost changed clock */
-- 
2.9.3

[RFC 2/2] clk: use enable_count to check if clk is busy

[Jerome Brunet]

When CLK_SET_RATE_GATE is set, we use the prepare_count to determine if a
clock is busy. A consumer could have just prepared his clock and not be
able to set it, eventhough the clock is still disabled (gated).

Might be the expected behavior, but the wording seems a bit misleading

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 6fe2ea81a9af..602f369bd1eb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -174,7 +174,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
 
 static bool clk_core_rate_can_change(struct clk_core *core)
 {
-	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
+	if ((core->flags & CLK_SET_RATE_GATE) && core->enable_count)
 		return false;
 
 	return true;
-- 
2.9.3

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

[Stephen Boyd]

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 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.  The 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.  Because of
> this change, mpll0 is no longer the best parent out there.
> 
> fixed_pll                    3        3            2000000000
>    mpll2                     0        0                     0
>    mpll1                     0        0              36863870
>       cts_mclk_i958_sel      0        0              36863870
>          cts_mclk_i958_div   0        0               6143979
>             cts_mclk_i958    0        0               6143979
>    mpll0                     1        1             491495425
>       cts_amclk_sel          1        1             491495425
>          cts_amclk_div       1        1              12287386
>             cts_amclk        1        1              12287386
> 
> 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.  Finally, i2s gets scheduled again, enables its
> clock but the rate of the selected parent has changed behind our back.  I
> 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.

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

It may also work to use clk_set_rate_range() to "lock" the rate
of a clk to specifically what frequency you want. 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.

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

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

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

[Jerome Brunet]

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.

> > 
> > 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.  The
> > 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.  Because of
> > this change, mpll0 is no longer the best parent out there.
> > 
> > fixed_pll                    3        3            2000000000
> >    mpll2                     0        0                     0
> >    mpll1                     0        0              36863870
> >       cts_mclk_i958_sel      0        0              36863870
> >          cts_mclk_i958_div   0        0               6143979
> >             cts_mclk_i958    0        0               6143979
> >    mpll0                     1        1             491495425
> >       cts_amclk_sel          1        1             491495425
> >          cts_amclk_div       1        1              12287386
> >             cts_amclk        1        1              12287386
> > 
> > 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.  Finally, i2s gets scheduled again, enables
> > its
> > clock but the rate of the selected parent has changed behind our
> > back.  I
> > 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
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 ?

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

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

The problem I see with this approach is the case where 2 consumers
prepare the clock w/o being able to set the rate => 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/
> 

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

[Michael Turquette]

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

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

[Jerome Brunet]

On Thu, 2017-03-09 at 14:23 -0800, Michael Turquette wrote:

[...]
> 
> 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 ;-)
> 

What would be the actual effect of deleting the flag altogether ? I see 2 cases:
1) Clock rate set through the "CLK_RATE_PARENT" mechanism: For these one,
removing the flag changes absolutely nothing.
2) Clock which are the initial clock of the clk_set_rate call: with the flag,
and w/o being unprepared first, the call would fail. Since this is the clock
directly targeted, there is good chance all consumer explicitly call unprepare.
The flag only act a reminder to the developer while writing the code.

If we remove this flag, the greatest risk I see here with clocks really needing
this, is more about future development and the lack of warning.

> > 
> > > > 
> > > > 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.  Finally, i2s gets scheduled again, enables
> > > > its
> > > > clock but the rate of the selected parent has changed behind our
> > > > back.  I
> > > > 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? ;-)

Sure we could try to be smart and find the best solution for all consumer.
That's a classic ressource allocation problem i suppose. For the moment I'd like
to be *sure* to have satisfied one... and try make the best out of what's
available for the other.

Let's make it safer, then make it smarter :) 

> 
> > 
> > > 
> > > 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.
> 
Ohh, thanks for the explanation. 

> > 
> > >  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().
> 
Even better

> 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 == 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()
> 
Sounds like a plan ;)
Stephen and you probably know the framework better than anyone but if you are to
busy, I could have go at 0) and make an RFC later on ?

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

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

[Michael Turquette]

Quoting Jerome Brunet (2017-03-11 10:18:23)
> On Thu, 2017-03-09 at 14:23 -0800, Michael Turquette wrote:
> =

> [...]
> > =

> > 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 ;-)
> > =

> =

> What would be the actual effect of deleting the flag altogether ? I see 2=
 cases:
> 1) Clock rate set through the "CLK_RATE_PARENT" mechanism: For these one,
> removing the flag changes absolutely nothing.
> 2) Clock which are the initial clock of the clk_set_rate call: with the f=
lag,
> and w/o being unprepared first, the call would fail. Since this is the cl=
ock
> directly targeted, there is good chance all consumer explicitly call unpr=
epare.
> The flag only act a reminder to the developer while writing the code.

I'm not sure about 2). It's possible that some consumer drivers do not
realize that the clock they consume has this flag set. The flag is a
property of the clock provider, and the consumer driver may or may not
know about it.

Also the purpose of the flag is ambiguous. Are we trying to prevent
downstream peripheral devices from glitching? If so the new
clk_rate_{un}lock() api is better for that and can replace such usage. I
think this is 80% of current users*.

If we're trying to prevent a problem with the clock controller itself
(e.g. some clock controller hardware requires gating clocks to change
the rate) then this flag still makes sense. This is 20% of the current
users.*

* Statistics are fictitious and are a bad application of the Pareto
  Principle

> =

> If we remove this flag, the greatest risk I see here with clocks really n=
eeding
> this, is more about future development and the lack of warning.
> =

> > > =

> > > > > =

> > > > > 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 aga=
in, 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" whi=
le
> > > 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? ;-)
> =

> Sure we could try to be smart and find the best solution for all consumer.
> That's a classic ressource allocation problem i suppose. For the moment I=
'd like
> to be *sure* to have satisfied one... and try make the best out of what's
> available for the other.
> =

> Let's make it safer, then make it smarter :) =

> =

> > =

> > > =

> > > > =

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

> Ohh, thanks for the explanation. =

> =

> > > =

> > > > =C2=A0I 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().
> > =

> Even better
> =

> > 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
> > =C2=A0=C2=A0=C2=A0clk_core->rate_lock_count. Add clk_set_rate_lock() he=
lper
> > 1) Repeal and replace CLK_SET_RATE_GATE users with clk_set_rate_lock()
> > =

> Sounds like a plan ;)
> Stephen and you probably know the framework better than anyone but if you=
 are to
> busy, I could have go at 0) and make an RFC later on ?

Go for it! I'm happy to review your implementation.

Best regards,
Mike

> =

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

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

[Stephen Boyd]

On 03/09, Michael Turquette wrote:
> 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 ;-)

I know for sure that qcom uses this flag incorrectly because I
added it. On some of the older platforms we can't support rate
changing in a glitch free manner so we really need to disable the
clk across the rate switch.

We used to forcibly disable the clk and then change the rate and
then forcibly enable whatever we forcibly disabled all inside the
clk ops under a register spinlock. That was before we converted
to CCF. We may want to keep doing that, because it's confusing
from a consumer perspective to know if a clk needs to be disabled
or not when changing the rate (more on this point later).

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

Great!

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

I'm not really thinking ccr here, just that hardware designers
typically figure out some static configuration of the clk tree
that doesn't lead to these sorts of problems. The problems
typically happen when we start using the hardware in ways the
designers never thought, and then we get into these problems
where we have to dynamically calculate the clk tree
configuration.

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

I lost you here. We should be handling constraints in the
framework, so min == max should make it impossible to change the
rate of that clk if it would be affected. The rate propagation up
the tree is supposed to happen so that we know what range of
acceptable rates is possible on parent clks.

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

Agreed. But that sort of knowledge shouldn't always be put on the
consumer of the clk to know and indicate to the framework.
Sometimes providers have a glitch free mux that can handle rate
changes in a glitch free manner. So providers should be
indicating they can't perform glitch free rate changes through
some flag or they should ensure they can handle things glitch
free via coordinated clk rates.

I would say drivers don't really care to say they can or can't
handle glitches downstream either. Drivers most likely _always_
want it to be glitch free when they call clk_set_rate(), so we
should make it that way in the core framework by figuring out if
something downstream of whatever changes rate will glitch and
taking the right action to avoid that. I've only ever seen this
as disabling clks downstream of the rate changing source, but
maybe there are other solutions. Probably we can put this
information into the rate request structure and act upon it in
the core.

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