clk: implement clock rate protection mechanism

drivers/clk/clk.c +2 -1

index fc58c52a26b4..f5c371532509 100644
@@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_has_parent);
 
-static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
+static int clk_core_set_parent_nolock(struct clk_core *core,
+				      struct clk_core *parent)
 {
 	int ret = 0;
 	int p_index = 0;

drivers/clk/clk.c +4 -6

+		return -ENOSYS;
 
 	/* check that we are allowed to re-parent if the clock is in use */
-	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
+		return -EBUSY;
 
 	/* try finding the new parent index */
 	if (parent) {

@@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		if (p_index < 0) {
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
-			ret = p_index;
-			goto out;
+			return p_index;
 		}
 		p_rate = parent->rate;
 	}

drivers/clk/clk.c +0 -0

index f5c371532509..dceaf0ff23db 100644

drivers/clk/clk.c +11 -0

+
+	trace_clk_set_phase(core, degrees);
+
+	if (core->ops->set_phase)
+		ret = core->ops->set_phase(core->hw, degrees);
+
+	trace_clk_set_phase_complete(core, degrees);
+
+	return ret;
+}
+
 /**
  * clk_set_phase - adjust the phase shift of a clock signal
  * @clk: clock signal source

Rework the way the callbacks round_rate and determine_rate are called. The goal is to do this at a single point and make it easier to add conditions before calling them.

This rework is done to ease the integration of "protected" clock functionality.

Signed-off-by Jerome Brunet <[email protected]>

---
 drivers/clk/clk.c | 78 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

drivers/clk/clk.c +0 -0

index dceaf0ff23db..8cc4672414be 100644
@@ -833,16 +833,34 @@ static int clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);

drivers/clk/clk.c +7 -1

+	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;
 
-	if (!core)

drivers/clk/clk.c +21 -13

+				   struct clk_rate_request *req)
+{
+	struct clk_core *parent;
+
+	if (WARN_ON(!core || !req))
+		return;
 
 	parent = core->parent;
 	if (parent) {

@@ -852,22 +870,24 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 		req->best_parent_hw = NULL;
 		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;
+static int clk_core_round_rate_nolock(struct clk_core *core,
+				      struct clk_rate_request *req)
+{
+	lockdep_assert_held(&prepare_lock);
 
-		req->rate = rate;
-	} else if (core->flags & CLK_SET_RATE_PARENT) {
-		return clk_core_round_rate_nolock(parent, req);
-	} else {
-		req->rate = core->rate;
-	}
+	if (!core)
+		return 0;
+
+	clk_core_init_rate_req(core, req);
+
+	if (core->ops->determine_rate || core->ops->round_rate)
+		return clk_core_determine_round(core, req);
+	else if (core->flags & CLK_SET_RATE_PARENT)
+		return clk_core_round_rate_nolock(core->parent, req);
 
+	req->rate = core->rate;
 	return 0;
 }
 

drivers/clk/clk.c +3 -8

 		struct clk_rate_request req;
 
 		req.rate = rate;
 		req.min_rate = min_rate;
 		req.max_rate = max_rate;
-		if (parent) {
-			req.best_parent_hw = parent->hw;
-			req.best_parent_rate = parent->rate;
-		} else {
-			req.best_parent_hw = NULL;
-			req.best_parent_rate = 0;
-		}
 
-		ret = core->ops->determine_rate(core->hw, &req);
+		clk_core_init_rate_req(core, &req);
+
+		ret = clk_core_determine_round(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;

The current implementation of clk_core_set_rate_nolock bails out early if

the requested rate is exactly the same as the one set. It should bail out if the request would not result in rate a change. This important when

rate is not exactly what is requested, which is fairly common with PLLs.

Ex: provider able to give any rate with steps of 100Hz
- 1st consumer request 48000Hz and gets it.
- 2nd consumer request 48010Hz as well. If we were to perform the usual mechanism, we would get 48000Hz as well. The clock would not change so there is no point performing any checks to make sure the clock can change, we know it won't.

This is important to prepare the addition of the clock protection mechanism

Signed-off-by Jerome Brunet <[email protected]>

---
 drivers/clk/clk.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

drivers/clk/clk.c +0 -0

index 8cc4672414be..163cb9832f10 100644
@@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
 		clk_change_rate(core->new_child);

Quoted 1 line · Jerome Brunet

The current implementation of clk_core_set_rate_nolock bails out early if

Add () to functions please.

Quoted 2 lines

the requested rate is exactly the same as the one set. It should bail out if the request would not result in rate a change.??This important when

s/in rate a change/in a rate change/
s/This important/This is important/

Quoted 1 line

rate is not exactly what is requested, which is fairly common with PLLs.

s/rate/the rate/

Quoted 7 lines

Ex: provider able to give any rate with steps of 100Hz
?- 1st consumer request 48000Hz and gets it.
?- 2nd consumer request 48010Hz as well. If we were to perform the usual ???mechanism, we would get 48000Hz as well. The clock would not change so ???there is no point performing any checks to make sure the clock can ???change, we know it won't.

I think Peter reported a similar problem as to what you're describing. Can you further explain a problem situation that this patch is fixing based on that thread (I can find the link if needed)?
Some of this series if fixing rate constraints actually, so please Cc him on future patch sets.

Quoted 22 lines

This is important to prepare the addition of the clock protection mechanism

Signed-off-by Jerome Brunet <[email protected]>

---
?drivers/clk/clk.c | 23 +++++++++++++++++++++--
?1 file changed, 21 insertions(+), 2 deletions(-)

drivers/clk/clk.c +0 -0

index 8cc4672414be..163cb9832f10 100644
@@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
 ?		clk_change_rate(core->new_child);
 ?}

?

drivers/clk/clk.c +2 -0

+static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
+						?????unsigned long

req_rate)
+{

drivers/clk/clk.c static void clk_change_rate(struct clk_core *core) · line 1580

Quoted 5 lines

drivers/clk/clk.c +5 -0

+	int ret;
+	struct clk_rate_request req;
+
+	if (!core)
+		return 0;

This is impossible because the only call site checks for core being NULL already.

drivers/clk/clk.c static void clk_change_rate(struct clk_core *core) · line 1587

Quoted 7 lines

drivers/clk/clk.c +7 -0

+
+	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
+	req.rate = req_rate;
+
+	ret = clk_core_round_rate_nolock(core, &req);
+
+	return ret ? 0 : req.rate;

What if the return value is negative?

We should bail the set rate
call instead of continuing on?

Quoted 10 lines · Stephen Boyd

Quoted 7 lines · Jerome Brunet

Ex: provider able to give any rate with steps of 100Hz
?- 1st consumer request 48000Hz and gets it.
?- 2nd consumer request 48010Hz as well. If we were to perform the usual ???mechanism, we would get 48000Hz as well. The clock would not change so ???there is no point performing any checks to make sure the clock can ???change, we know it won't.

Quoted 6 lines

I think Peter reported a similar problem as to what you're describing. Can you further explain a problem situation that this patch is fixing based on that thread (I can find the link if needed)?
Some of this series if fixing rate constraints actually, so please Cc him on future patch sets.

I had not seen the thread you referring to.
I assume Peter is Peter De Schrijver and the thread is:

http://lkml.kernel.org/r/1490103807-21821-1-git-send-email-pdeschrijver at nvidia.c om

Peter is right, There is indeed a problem when the current rate is out of the requested range.

I'm not sure about the proposed solution though. Even the rate set by set_rate_range() should go trough the bail-out mechanism because of the use-case I have explained here.

After spending some time on it, I realized that patch 7 is useless, as the call to clk_core_set_rate_nolock() with core->req_rate is a no-op and will never fail.

We could request the rate to be changed to nearest rate range limit (here is a candidate fix doing that [0]

But then we get to a more fundamental issue of the rate range mechanism.

Take the example of Peter:
* You had 500Mhz and you set a range of [100-300] MHz
* The logical thing to do would be to request the clock rate to change to 300MHz, right ?
* Hw capabilities being what they are, the rate is unfortunately rounded to 301Mhz.
* In the end, you are out of the range and the call fails.

That is when the clock only implement round_rate(). If it implement determine_rate(), it could take the range into account when rounding the rate. However, I doubt many clock drivers are taking care of this corner case, if any.

The clean way to address this would be to have all clock drivers use determine_rate() and make sure they all that the case into account, and delete the round_rate() ... not happening tomorrow.

The consistent way would be to systematically fail if the current rate is out of the requested range ... a bit rude maybe.

The proposed patch [0] does it's best to change the rate, but may fail as explained above ...

For now, I have dropped patch 7 and pushed patch [0] at the end of the queue. Since It is really related to the clock protect mechanism, we should probably discuss this in another thread.

[0]: https://github.com/jeromebrunet/linux/commit/235e477f346a9e8d115dda257f9f73
834151bd7f

Quoted 22 lines

Quoted 22 lines

This is important to prepare the addition of the clock protection mechanism

Signed-off-by Jerome Brunet <[email protected]>

---
?drivers/clk/clk.c | 23 +++++++++++++++++++++--
?1 file changed, 21 insertions(+), 2 deletions(-)

drivers/clk/clk.c +0 -0

index 8cc4672414be..163cb9832f10 100644
@@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
 ?		clk_change_rate(core->new_child);
 ?}

?

drivers/clk/clk.c +2 -0

+static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
+						?????unsigned long

req_rate)
+{

Quoted 9 lines

drivers/clk/clk.c static void clk_change_rate(struct clk_core *core) · line 1580

Quoted 5 lines

drivers/clk/clk.c +5 -0

+	int ret;
+	struct clk_rate_request req;
+
+	if (!core)
+		return 0;

This is impossible because the only call site checks for core being NULL already.

This more or less like the previous comments on lockdep_assert. Most (should be all) "_no_lock" function check the prepare_lock and the core pointer. Even when it is not strictly necessary, I think we should be consistent
about it.

In the unlikely event this function is used some place else, it would avoid bad surprise

So?if it is OK with you, I would prefer to keep this check and add the check of the prepare lock. Maybe I could go over the other "_nolock" functions in clk.c to verify they are all doing it ? What do you think ?

Quoted 10 lines

drivers/clk/clk.c static void clk_change_rate(struct clk_core *core) · line 1587

Quoted 8 lines

drivers/clk/clk.c +7 -0

+
+	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
+	req.rate = req_rate;
+
+	ret = clk_core_round_rate_nolock(core, &req);
+
+	return ret ? 0 : req.rate;

What if the return value is negative?

Here we are trying to return a rate, so unsigned long.
I think (and I remember discussing it with Mike sometimes ago) that a 0 clock rate quite often represent an error.?

Quoted 2 lines

We should bail the set rate
call instead of continuing on?

I don't think this for this particular function to decide. It should try compute what the rate would be. It's up to the calling function to decide what do with this 0 (error)

To answer your question, I think that if we can't figure out the what the rate would be, we should not error right away and just let the regular process happen (no bail-out) ... If there is a problem, it will error anyway

Quoted 1 line

The patch adds clk_protect and clk_unprotect to the CCF API. These functions allow a consumer to inform the system that the rate of clock is critical to for its operations and it can't tolerate other consumers

changing the rate or introducing glitches while the clock is protected.

drivers/clk/clk.c +24 -0

index 163cb9832f10..d688b8f59a59 100644
+
+/**
+ * clk_rate_unprotect - unprotect the rate of a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect completes a critical section during which the clock
+ * consumer cannot tolerate any change to the clock rate. If no other clock
+ * consumers have protected clocks in the parent chain, then calls to this
+ * function will allow the clocks in the parent chain to change rates
+ * freely.
+ *
+ * Unlike the clk_set_rate_range method, which allows the rate to change
+ * within a given range, protected clocks cannot have their rate changed,
+ * either directly or indirectly due to changes further up the parent chain
+ * of clocks.
+ *
+ * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
+ * to this function may sleep, and do not return error status.
+ */
+void clk_rate_unprotect(struct clk *clk)
+{
+	if (!clk)
+		return;
+

drivers/clk/clk.c +5 -0

+	clk->protect_count--;
+out:
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_unprotect);

drivers/clk/clk.c +1 -0

+

@@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_lock();
 

include/linux/clk.h +1 -0

index 91bd464f4c9b..b60c36f2e6b0 100644
@@ -331,6 +331,30 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
  */
 struct clk *devm_get_clk_from_child(struct device *dev,
 				    struct device_node *np, const char *con_id);
+/**

Quoted 3 lines · Jerome Brunet

The patch adds clk_protect and clk_unprotect to the CCF API. These functions allow a consumer to inform the system that the rate of clock is critical to for its operations and it can't tolerate other consumers

s/for//

Quoted 40 lines

changing the rate or introducing glitches while the clock is protected.

drivers/clk/clk.c +28 -0

index 163cb9832f10..d688b8f59a59 100644
+
+/**
+ * clk_rate_unprotect - unprotect the rate of a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect completes a critical section during which the clock
+ * consumer cannot tolerate any change to the clock rate. If no other clock
+ * consumers have protected clocks in the parent chain, then calls to this
+ * function will allow the clocks in the parent chain to change rates
+ * freely.
+ *
+ * Unlike the clk_set_rate_range method, which allows the rate to change
+ * within a given range, protected clocks cannot have their rate changed,
+ * either directly or indirectly due to changes further up the parent chain
+ * of clocks.
+ *
+ * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
+ * to this function may sleep, and do not return error status.
+ */
+void clk_rate_unprotect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+
+	/*
+	?* if there is something wrong with this consumer protect count,

stop

drivers/clk/clk.c +4 -0

+	?* here before messing with the provider
+	?*/
+	if (WARN_ON(clk->protect_count <= 0))
+		goto out;

drivers/clk/clk.c

Quoted 2 lines

drivers/clk/clk.c +2 -0

+
+	clk_core_rate_unprotect(clk->core);

Can we make this stuff non-recursive? I know that this is basically a copy paste of prepare/unprepare code and recursion is nice and elegant, but we really don't need to do it when we could have a loop that's the same and doesn't blow up our stack frame usage. I'll send a patch for prepare/enable so you get the idea.

Quoted 6 lines

drivers/clk/clk.c +5 -0

+	clk->protect_count--;
+out:
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_unprotect);

[..]

Quoted 10 lines

drivers/clk/clk.c +1 -0

+

@@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)

?
? clk_prepare_lock();
?

drivers/clk/clk.c +2 -0

+	/*
+	?* Before calling clk_put, all calls to clk_rate_protect from a

given

drivers/clk/clk.c +1 -0

+	?* user must be balanced with calls to clk_rate_unprotect and by

that

drivers/clk/clk.c void __clk_put(struct clk *clk) · line 3146

Quoted 7 lines

drivers/clk/clk.c +7 -0

+	?* same user
+	?*/
+	WARN_ON(clk->protect_count);
+
+	/* We voiced our concern, let's sanitize the situation */
+	for (; clk->protect_count; clk->protect_count--)
+		clk_core_rate_unprotect(clk->core);

Does this do anything different than:

	clk->core->protect_count -= clk->protect_count;
	clk->protect_count = 1;

	clk_core_rate_unprotect(clk->core);

Just seems better to not do a loop here.

Quoted 22 lines

include/linux/clk.h +0 -0

index 91bd464f4c9b..b60c36f2e6b0 100644
@@ -331,6 +331,30 @@ struct clk *devm_clk_get(struct device *dev, const char
 *id);

? */
?struct clk *devm_get_clk_from_child(struct device *dev,
? ????struct device_node *np, const char
*con_id);
+/**
+ * clk_rate_protect - inform the system when the clock rate must be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.

Quoted 2 lines

+ */
+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?

I'm tempted to say that we could do this rate locking stuff with clk_set_rate_range(), but with more thought that doesn't seem possible because there's a subtle difference. The range API is willing to accept a range of frequencies, and calling clk_set_rate_range() with some exact frequency should fail if that exact frequency can't be met. With this API and the subsequent clk_set_rate_protect() API we're willing to accept that the rate we call clk_set_rate_protect() with could be different than the rate we actually get.

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.

Quoted 9 lines

include/linux/clk.h +6 -0

+
+/**
+ * clk_rate_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting

the
+ * clock rate can now deal with other consumer altering the clock source rate

other consumers

Quoted 3 lines

+ *
+ * The caller must balance the number of rate_protect and rate_unprotect calls.

Please say clk_rate_protect() and clk_rate_unprotect() here.

Quoted 11 lines · Stephen Boyd

drivers/clk/clk.c EXPORT_SYMBOL_GPL(__clk_mux_determine_rate_closest); · line 529

Quoted 4 lines · Jerome Brunet

drivers/clk/clk.c +4 -0

+	if (WARN_ON(clk->protect_count <= 0))
+		goto out;
+
+	clk_core_rate_unprotect(clk->core);

Can we make this stuff non-recursive? I know that this is basically a copy paste of prepare/unprepare code and recursion is nice and elegant, but we really don't need to do it when we could have a loop that's the same and doesn't blow up our stack frame usage. I'll send a patch for prepare/enable so you get the idea.

I think we should not be doing the same thing differently inside the framework. If enable and disable are recursive, protect should be too. The stack can handle enable and prepare, why would it not handle protect ?

Of course, If your patch to "stack-ify" enable and prepare merges first, I'll copy the mechanism. The important thing for me is keep things consistent.

On a more general note, your idea is no light change ...
Are the trees really that tall that it is becoming a problem ?
I see in your patch comment that there is still a few question to be answered, right ?

Could we take this one step at time and keep the recursion for now? When the whole CCF moves to a stack approach, I'll be happy to help.

Quoted 29 lines

Quoted 7 lines

drivers/clk/clk.c +5 -0

+	clk->protect_count--;
+out:
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_unprotect);

[..]

Quoted 10 lines

(quoted diff) +1 -0

+

@@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)

?
? clk_prepare_lock();
?

(quoted diff) +2 -0

+	/*
+	?* Before calling clk_put, all calls to clk_rate_protect from a

given

(quoted diff) +1 -0

+	?* user must be balanced with calls to clk_rate_unprotect and by

that

drivers/clk/clk.c void __clk_put(struct clk *clk) · line 3146

Quoted 7 lines

drivers/clk/clk.c +7 -0

+	?* same user
+	?*/
+	WARN_ON(clk->protect_count);
+
+	/* We voiced our concern, let's sanitize the situation */
+	for (; clk->protect_count; clk->protect_count--)
+		clk_core_rate_unprotect(clk->core);

Does this do anything different than:

Quoted 2 lines

	clk->core->protect_count -= clk->protect_count;
	clk->protect_count = 1;

this looks really hacky !

Quoted 3 lines

	clk_core_rate_unprotect(clk->core);

Just seems better to not do a loop here.

A loop a easy to understand.
This code should actually never run, so readability looks more important to me than optimisation.
If you really insist on this, I'll yield but I'm not a big fan.

Quoted 23 lines

Quoted 23 lines

include/linux/clk.h +0 -0

index 91bd464f4c9b..b60c36f2e6b0 100644
@@ -331,6 +331,30 @@ struct clk *devm_clk_get(struct device *dev, const char
 *id);

? */
?struct clk *devm_get_clk_from_child(struct device *dev,
? ????struct device_node *np, const char
*con_id);
+/**
+ * clk_rate_protect - inform the system when the clock rate must be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.

Quoted 8 lines

Quoted 2 lines

+ */
+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 at baylibre.com [1]: https://github.com/jeromebrunet/linux/commits/amlogic/wip/audio-clk-lock

Quoted 13 lines

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.

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.

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.

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 6 lines

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 at baylibre.com [1]: https://github.com/jeromebrunet/linux/commits/amlogic/wip/audio-clk-lock

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.

Quoted 21 lines

Quoted 14 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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,

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.

Quoted 7 lines

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

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?

Signature · 3 lines

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

Hi Stephen,

Quoting Stephen Boyd (2017-08-03 17:18:36)

Quoted 13 lines · Jerome Brunet

Quoted 12 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 2 lines

Quoted 2 lines

[0]: http://lkml.kernel.org/r/20170302173835.18313-1-jbrunet at baylibre.com [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.

Quoted 6 lines

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:

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

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.

Quoted 4 lines

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.

Quoted 9 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

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.

Quoted 5 lines · Stephen Boyd

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.

That's probably because people tend to get this stuff wrong.??It is _not_ supposed to be:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, rounded_rate);

but:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, requested_rate);

The former is wrong for two reasons:

1. it's completely wasteful of CPU resources to do all the calculations ???that need to be done within clk_set_rate().

2. it's racy - there is no guarantee that you'll end up with "rounded_rate"

The API definition of clk_round_rate() explicitly mentions that it is equivalent to clk_set_rate() followed by clk_get_rate() with the exception that it doesn't affect the hardware.

I'm not sure that the clock rate protection API is really the right solution - if we're trying to stop others from changing the clock rate, that implies we have multiple different threads potentially changing the rate at any time.??If a driver does this:

	clk_set_rate(clk, foo);
	clk_rate_protect(clk);

what prevents another thread from changing the clock rate between these two calls???The only way to do this safely would be something like:

	r = clk_round_rate(clk, foo);
	while (1) {
		err = clk_set_rate(clk, foo);
		clk_rate_protect(clk);
		if (err < 0)
			break;

		if (r == clk_get_rate(clk)) /* success */
			break;

		clk_rate_unprotect(clk);
	}

	if (err)
		failed;

That's rather a lot of code to add to every driver, and given the number of times I've seen people get the clk_round_rate() vs clk_set_rate() thing wrong, I've zero confidence that folk will get this right either.

So, I'd suggest _not_ adding this clk_rate_protect() thing, but instead an API that simultaneously sets and protects the rate, so driver authors don't have to get involved in details like the above.

Quoted 42 lines · Russell King - ARM Linux

Quoted 5 lines · Stephen Boyd

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.

That's probably because people tend to get this stuff wrong.??It is _not_ supposed to be:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, rounded_rate);

but:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, requested_rate);

The former is wrong for two reasons:

1. it's completely wasteful of CPU resources to do all the calculations ???that need to be done within clk_set_rate().

2. it's racy - there is no guarantee that you'll end up with "rounded_rate"

The API definition of clk_round_rate() explicitly mentions that it is equivalent to clk_set_rate() followed by clk_get_rate() with the exception that it doesn't affect the hardware.

I'm not sure that the clock rate protection API is really the right solution - if we're trying to stop others from changing the clock rate, that implies we have multiple different threads potentially changing the rate at any time.??If a driver does this:

	clk_set_rate(clk, foo);
	clk_rate_protect(clk);

what prevents another thread from changing the clock rate between these two calls???The only way to do this safely would be something like:

Quoted 15 lines

	r = clk_round_rate(clk, foo);
	while (1) {
		err = clk_set_rate(clk, foo);
		clk_rate_protect(clk);
		if (err < 0)
			break;

		if (r == clk_get_rate(clk)) /* success */
			break;

		clk_rate_unprotect(clk);
	}

	if (err)
		failed;

Russell,
I think you have missed one subtle thing, when trying any clock altering operation, if the consumer is protecting the clock, it will temporarily release the protection once, under the prepare_lock (to guarantee safe operation). This is explained in the cover letter:

"""
With this series there is 3 use-case:
?- the provider is not protected: nothing changes
?- the provider is protected by only 1 consumer (and only once), then only ???this consumer will be able to alter the rate of the clock, as it is the ???only one depending on it.
?- If the provider is protected more than once, or by the provider itself, ???the rate is basically locked and protected against reparenting. """

So what you should do if you have to protect the clock is:

clk_rate_protect(clk);
err = clk_set_rate(clk, foo);

[...]
clk_rate_unprotect(clk);

Quoted 80 lines · Jerome Brunet

Quoted 43 lines · Russell King - ARM Linux

Quoted 5 lines · Stephen Boyd

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.

That's probably because people tend to get this stuff wrong.??It is _not_ supposed to be:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, rounded_rate);

but:

	rounded_rate = clk_round_rate(clk, requested_rate);

	clk_set_rate(clk, requested_rate);

The former is wrong for two reasons:

1. it's completely wasteful of CPU resources to do all the calculations ???that need to be done within clk_set_rate().

2. it's racy - there is no guarantee that you'll end up with "rounded_rate"

The API definition of clk_round_rate() explicitly mentions that it is equivalent to clk_set_rate() followed by clk_get_rate() with the exception that it doesn't affect the hardware.

I'm not sure that the clock rate protection API is really the right solution - if we're trying to stop others from changing the clock rate, that implies we have multiple different threads potentially changing the rate at any time.??If a driver does this:

	clk_set_rate(clk, foo);
	clk_rate_protect(clk);

what prevents another thread from changing the clock rate between these two calls???The only way to do this safely would be something like:

Quoted 15 lines

	r = clk_round_rate(clk, foo);
	while (1) {
		err = clk_set_rate(clk, foo);
		clk_rate_protect(clk);
		if (err < 0)
			break;

		if (r == clk_get_rate(clk)) /* success */
			break;

		clk_rate_unprotect(clk);
	}

	if (err)
		failed;

Russell,
I think you have missed one subtle thing, when trying any clock altering operation, if the consumer is protecting the clock, it will temporarily release?
the protection once, under the prepare_lock (to guarantee safe operation). This
is explained in the cover letter:

"""
With this series there is 3 use-case:
?- the provider is not protected: nothing changes
?- the provider is protected by only 1 consumer (and only once), then only ???this consumer will be able to alter the rate of the clock, as it is the ???only one depending on it.
?- If the provider is protected more than once, or by the provider itself, ???the rate is basically locked and protected against reparenting. """

So what you should do if you have to protect the clock is:

Quoted 5 lines

clk_rate_protect(clk);
err = clk_set_rate(clk, foo);

[...]
clk_rate_unprotect(clk);

So here you drop the protection, which means anyone can alter the clock again.

Either that or "clk_rate_unprotect" is inappropriately named and doesn't do what it says it does.

Quoted 77 lines · Russell King - ARM Linux

Quoted 42 lines · Stephen Boyd

Quoted 5 lines

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.

That's probably because people tend to get this stuff wrong.??It is _not_ supposed to be:

    rounded_rate = clk_round_rate(clk, requested_rate);

    clk_set_rate(clk, rounded_rate);

but:

    rounded_rate = clk_round_rate(clk, requested_rate);

    clk_set_rate(clk, requested_rate);

The former is wrong for two reasons:

1. it's completely wasteful of CPU resources to do all the calculations ???that need to be done within clk_set_rate().

2. it's racy - there is no guarantee that you'll end up with "rounded_rate"

The API definition of clk_round_rate() explicitly mentions that it is equivalent to clk_set_rate() followed by clk_get_rate() with the exception that it doesn't affect the hardware.

I'm not sure that the clock rate protection API is really the right solution - if we're trying to stop others from changing the clock rate, that implies we have multiple different threads potentially changing the rate at any time.??If a driver does this:

    clk_set_rate(clk, foo);
    clk_rate_protect(clk);

what prevents another thread from changing the clock rate between these two calls???The only way to do this safely would be something like:

Quoted 15 lines

    r = clk_round_rate(clk, foo);
    while (1) {
            err = clk_set_rate(clk, foo);
            clk_rate_protect(clk);
            if (err < 0)
                    break;

            if (r == clk_get_rate(clk)) /* success */
                    break;

            clk_rate_unprotect(clk);
    }

    if (err)
            failed;

Russell,
I think you have missed one subtle thing, when trying any clock altering operation, if the consumer is protecting the clock, it will temporarily release the protection once, under the prepare_lock (to guarantee safe operation). This is explained in the cover letter:

"""
With this series there is 3 use-case:
?- the provider is not protected: nothing changes
?- the provider is protected by only 1 consumer (and only once), then only ???this consumer will be able to alter the rate of the clock, as it is the ???only one depending on it.
?- If the provider is protected more than once, or by the provider itself, ???the rate is basically locked and protected against reparenting. """

So what you should do if you have to protect the clock is:

Quoted 5 lines

clk_rate_protect(clk);
err = clk_set_rate(clk, foo);

[...]
clk_rate_unprotect(clk);

So here you drop the protection, which means anyone can alter the clock again.

Either that or "clk_rate_unprotect" is inappropriately named and doesn't do what it says it does.

Signature · 3 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

I'm not sure that the clock rate protection API is really the right solution - if we're trying to stop others from changing the clock rate, that implies we have multiple different threads potentially changing the rate at any time. If a driver does this:

        clk_set_rate(clk, foo);
        clk_rate_protect(clk);

what prevents another thread from changing the clock rate between these two calls? The only way to do this safely would be something like:

        r = clk_round_rate(clk, foo);
        while (1) {
                err = clk_set_rate(clk, foo);
                clk_rate_protect(clk);
                if (err < 0)
                        break;

                if (r == clk_get_rate(clk)) /* success */
                        break;

                clk_rate_unprotect(clk);
        }

        if (err)
                failed;

That's rather a lot of code to add to every driver, and given the number of times I've seen people get the clk_round_rate() vs clk_set_rate() thing wrong, I've zero confidence that folk will get this right either.

So, I'd suggest _not_ adding this clk_rate_protect() thing, but instead an API that simultaneously sets and protects the rate, so driver authors don't have to get involved in details like the above.

Signature · 4 lines

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Quoted 23 lines · Michael Turquette

Hi Stephen,

Quoting Stephen Boyd (2017-08-03 17:18:36)

Quoted 15 lines · Jerome Brunet

Quoted 14 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 4 lines

Quoted 4 lines

[0]: http://lkml.kernel.org/r/20170302173835.18313-1-jbrunet at baylibre.co m
[1]: https://github.com/jeromebrunet/linux/commits/amlogic/wip/audio-clk
-lock

Quoted 4 lines

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.

Quoted 13 lines

Quoted 6 lines

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:

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 48 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 26 lines

Quoted 13 lines

Quoted 4 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

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.

Hi Stephen,

Quoting Stephen Boyd (2017-08-03 17:18:36)

Quoted 15 lines · Jerome Brunet

Quoted 14 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 3 lines

Quoted 3 lines

[0]: http://lkml.kernel.org/r/20170302173835.18313-1-jbrunet at 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.

Quoted 5 lines

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.

Quoted 23 lines

Quoted 23 lines

Quoted 14 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 4 lines

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

Quoted 144 lines

Hi Stephen,

Quoting Stephen Boyd (2017-08-03 17:18:36)

Quoted 15 lines · Jerome Brunet

Quoted 14 lines

Quoted 7 lines

Quoted 1 line

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

Quoted 3 lines

Quoted 3 lines

[0]: http://lkml.kernel.org/r/20170302173835.18313-1-jbrunet at 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.

Quoted 5 lines

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.

Quoted 23 lines

Quoted 23 lines

Quoted 14 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 21 lines

Quoted 6 lines

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

Quoted 4 lines

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

drivers/clk/clk.c +9 -0

+ * same clock provider.
+ *
+ * The protection is not applied if setting the rate failed.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate_protect(struct clk *clk, unsigned long rate)
+{
+	int ret;

drivers/clk/clk.c +12 -0

+	if (!ret) {
+		clk_core_rate_protect(clk->core);
+		clk->protect_count++;
+	}
+
+	clk_prepare_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate_protect);
+
+/**
  * clk_set_rate_range - set a rate range for a clock source
  * @clk: clock source
  * @min: desired minimum clock rate in Hz, inclusive

Signed-off-by Jerome Brunet <[email protected]>

---
 drivers/clk/clk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/clk/clk.c +8 -0

index d91236e70a04..253f1d3f0971 100644
@@ -1864,6 +1864,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate_protect);
 int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 {
 	int ret = 0;
+	unsigned int old_min, old_max;
 
 	if (!clk)
 		return 0;

@@ -1881,9 +1882,16 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		clk_core_rate_unprotect(clk->core);
 
 	if (min != clk->min_rate || max != clk->max_rate) {
+		old_min = clk->min_rate;
+		old_max = clk->max_rate;
 		clk->min_rate = min;
 		clk->max_rate = max;
 		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+		if (ret) {
+			/* undo changes */
+			clk->min_rate = old_min;
+			clk->max_rate = old_max;
+		}
 	}
 
 	if (clk->protect_count)

Quoted 1 line · Jerome Brunet

Signed-off-by Jerome Brunet <[email protected]>

There should be some commit text here describing the problem. Also a Fixes: tag and please put this earlier in the series.

Quoted 37 lines

---
?drivers/clk/clk.c | 8 ++++++++
?1 file changed, 8 insertions(+)

drivers/clk/clk.c +0 -0

index d91236e70a04..253f1d3f0971 100644
@@ -1864,6 +1864,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate_protect);

?int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
?{
? int ret = 0;

drivers/clk/clk.c +1 -0

+	unsigned int old_min, old_max;

?
? if (!clk)
? return 0;

drivers/clk/clk.c +0 -0

@@ -1881,9 +1882,16 @@ int clk_set_rate_range(struct clk *clk, unsigned long
 min, unsigned long max)
 ?		clk_core_rate_unprotect(clk->core);

?
? if (min != clk->min_rate || max != clk->max_rate) {

drivers/clk/clk.c +2 -0

+		old_min = clk->min_rate;
+		old_max = clk->max_rate;

? clk->min_rate = min;
? clk->max_rate = max;
? ret = clk_core_set_rate_nolock(clk->core, clk->core-

Quoted 1 line

req_rate);

drivers/clk/clk.c +5 -0

+		if (ret) {
+			/* undo changes */
+			clk->min_rate = old_min;
+			clk->max_rate = old_max;
+		}

? }
?
? if (clk->protect_count)

clk_summary debugfs entry was already well over the traditional 80 characters per line limit but it grew even larger with the addition of clock protection.

Signed-off-by Jerome Brunet <[email protected]>

---

ENOSYS is special and should only be used for incorrect syscall number. It does not seem to be the case here.

Reported by checkpatch.pl while working on clock protection.

Signed-off-by Jerome Brunet <[email protected]>

The goal of this patchset is to provide a way for consumers to inform the system that they depend on the rate of the clock source and can't tolerate other consumers changing the rate or causing glitches.

Quoted 3 lines · Jerome Brunet

The goal of this patchset is to provide a way for consumers to inform the system that they depend on the rate of the clock source and can't tolerate other consumers changing the rate or causing glitches.

Just for context: chat kind of consumers are these?

i.e. the consumers that can't tolerate a clock rate change?

I understand it if it is a hardware limitation (like the block would crash if it glitches or changes rate).

On the other hand, in the kernel we have things like in arch/arm/kernel/smp_twd.c we use a notifier to deal with a changing clock rate.

Just want to be sure that you're not working around something that can be dealt with using rate change notifiers.

(OK maybe a stupid question, I assume this is not the case, but anyways, had to ask.)

Yours,
Linus Walleij

On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:

When the rate is critical to perform a particular task. My example is the audio and i2s output. You can't tolerate glitches during the playback, the end user would complain (longer description in the original RFC)

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you in CC.

ux500 uses this flag several time, I'd like make sure people are not relying on its broken implementation.

Quoted 6 lines · Jerome Brunet

On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:
When the rate is critical to perform a particular task. My example is the audio
and i2s output. You can't tolerate glitches during the playback, the end user
would complain (longer description in the original RFC)

I see. Thanks for your detailed explanation!

Quoted 4 lines

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you in
CC.

Quoted 3 lines

ux500 uses this flag several time, I'd like make sure people are not relying on
its broken implementation.

Ux500 audio is broken, but I'm fixing it a little at a time...

Yours,
Linus Walleij

Quoted 14 lines · Linus Walleij

Quoted 6 lines · Jerome Brunet

On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:
When the rate is critical to perform a particular task. My example is the audio
and i2s output. You can't tolerate glitches during the playback, the end user
would complain (longer description in the original RFC)

I see. Thanks for your detailed explanation!

Quoted 4 lines

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you in
CC.

Quoted 5 lines

Quoted 3 lines

ux500 uses this flag several time, I'd like make sure people are not relying on
its broken implementation.

Ux500 audio is broken, but I'm fixing it a little at a time...

No problem with Ux500 audio, don't worry :)
Audio is just one application among others.

The concern regarding ux500 is that the clock controller declares clocks using the CLK_SET_RATE_GATE flag (like qcom, at91 and several other)

here is the definition:
#define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */

My interpretation it that, as long as clock is enabled, rate can't change.

The implementation of this flag is currently broken:
* If you call set_rate on directly on the clock while it is enabled, you will get -EBUSY, as expected
* If you call set_rate on its parent, rate will change, changing the rate of the child clock as well ...

With this patchset applied, calling set_rate on the parent would also return -EBUSY, enforcing the "rate can't change while enabled" property.

To build confidence that this won't be causing regression, I'd like to check that platform using this flag are no relying on the broken behavior.

I've included the clock maintainers of at91, qcom, and ux500 (you) in this thread because they are the heaviest users of this flag, so the more likely to report a problem.

If you could apply this series and just do things as usual, It'd be awesome !

+Quentin

Quoted 41 lines · On Tue, 20 Jun 2017 14:32:30 +0200 Jerome Brunet <[email protected]> wrote:

Quoted 14 lines · Linus Walleij

Quoted 6 lines · Jerome Brunet

On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:
When the rate is critical to perform a particular task. My example is the audio
and i2s output. You can't tolerate glitches during the playback, the end user
would complain (longer description in the original RFC)

I see. Thanks for your detailed explanation!

Quoted 4 lines

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you in
CC.

Quoted 5 lines

Quoted 3 lines

ux500 uses this flag several time, I'd like make sure people are not relying on
its broken implementation.

Ux500 audio is broken, but I'm fixing it a little at a time...

No problem with Ux500 audio, don't worry :)
Audio is just one application among others.

The concern regarding ux500 is that the clock controller declares clocks using the CLK_SET_RATE_GATE flag (like qcom, at91 and several other)

here is the definition:
#define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */

My interpretation it that, as long as clock is enabled, rate can't change.

The implementation of this flag is currently broken:
* If you call set_rate on directly on the clock while it is enabled, you will get -EBUSY, as expected
* If you call set_rate on its parent, rate will change, changing the rate of the child clock as well ...

With this patchset applied, calling set_rate on the parent would also return -EBUSY, enforcing the "rate can't change while enabled" property.

Quoted 6 lines

To build confidence that this won't be causing regression, I'd like to check that platform using this flag are no relying on the broken behavior.

I've included the clock maintainers of at91, qcom, and ux500 (you) in this thread because they are the heaviest users of this flag, so the more likely to report a problem.

This flag in at91 clk drivers really means that rate cannot be changed while the clk is enabled. We have other clock where this is not a problem (at least, nothing in mentioned in the datasheet).

Quoted 2 lines

If you could apply this series and just do things as usual, It'd be awesome !

Actually, this is a feature I was pushing for a while back [1], because I had the same problem (one user messing up with a PLL while others were relying on its rate). I'm glad someone finally had time to provide a solution for this problem.

Quentin tested the series, and I guess he'll soon add his Tested-by. Note that this series does not address all problems. For example, when several drivers are setting a rate on different clks that take the same parent, the first one to set the rate and enable the clk wins.

He has an hack-ish solution for our audio-pll case preventing non audio users to mess up with the audio PLL. It'd be great to have a generic solution, though I don't know how this can be solved without advanced description of the clk-rate setting policy.

[1]https://patchwork.kernel.org/patch/6204221/

Hi Jerome,

Quoted 88 lines · Boris Brezillon

+Quentin

Quoted 51 lines · On Tue, 20 Jun 2017 14:32:30 +0200 Jerome Brunet <[email protected]> wrote:

Quoted 18 lines · Linus Walleij

On Tue, Jun 20, 2017 at 12:50 PM, Jerome Brunet <[email protected]> wrote:??

Quoted 8 lines · Linus Walleij

When the rate is critical to perform a particular task. My example is the
audio
and i2s output. You can't tolerate glitches during the playback, the end
user
would complain (longer description in the original RFC)??

I see. Thanks for your detailed explanation!
??

Quoted 5 lines

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you
in
CC.

Quoted 6 lines

Quoted 4 lines

ux500 uses this flag several time, I'd like make sure people are not relying
on
its broken implementation.??

Ux500 audio is broken, but I'm fixing it a little at a time...??

No problem with Ux500 audio, don't worry :)
Audio is just one application among others.

The concern regarding ux500 is that the clock controller declares clocks using
the CLK_SET_RATE_GATE flag (like qcom, at91 and several other)

here is the definition:
#define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate
change */

My interpretation it that, as long as clock is enabled, rate can't change.

The implementation of this flag is currently broken:
* If you call set_rate on directly on the clock while it is enabled, you will
get -EBUSY, as expected
* If you call set_rate on its parent, rate will change, changing the rate of the
child clock as well ...

With this patchset applied, calling set_rate on the parent would also return
-EBUSY, enforcing the "rate can't change while enabled" property.

Quoted 8 lines

To build confidence that this won't be causing regression, I'd like to check
that platform using this flag are no relying on the broken behavior.

I've included the clock maintainers of at91, qcom, and ux500 (you) in this thread because they are the heaviest users of this flag, so the more likely to
report a problem.

This flag in at91 clk drivers really means that rate cannot be changed while the clk is enabled. We have other clock where this is not a problem (at least, nothing in mentioned in the datasheet).

Quoted 3 lines

If you could apply this series and just do things as usual, It'd be awesome !

Actually, this is a feature I was pushing for a while back [1], because I had the same problem (one user messing up with a PLL while others were relying on its rate). I'm glad someone finally had time to provide a solution for this problem.

Quentin tested the series, and I guess he'll soon add his Tested-by. Note that this series does not address all problems. For example, when several drivers are setting a rate on different clks that take the same parent, the first one to set the rate and enable the clk wins.

He has an hack-ish solution for our audio-pll case preventing non audio users to mess up with the audio PLL. It'd be great to have a generic solution, though I don't know how this can be solved without advanced description of the clk-rate setting policy.

Quoted 2 lines

[1]https://patchwork.kernel.org/patch/6204221/

I've tested the patch series and it seems good to me.

Tested-by Quentin Schulz <[email protected]>

Our hackish solution for the moment is to deny clock children to take the audio-pll clock as parent except if it's classd (the audio IP). That way, we take care of the driver probing "order" (i.e., the first driver to enable the clock will lock the rate, but maybe that rate isn't the one we want for a more critical driver) but it is definitely not a neat solution.

Thanks,
Quentin

To build confidence that this won't be causing regression, I'd like to check that platform using this flag are no relying on the broken behavior.

I've included the clock maintainers of at91, qcom, and ux500 (you) in this thread because they are the heaviest users of this flag, so the more likely to report a problem.

Quoted 16 lines · Linus Walleij

Quoted 6 lines · Jerome Brunet

On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:
When the rate is critical to perform a particular task. My example is the audio
and i2s output. You can't tolerate glitches during the playback, the end user
would complain (longer description in the original RFC)

I see. Thanks for your detailed explanation!

Quoted 5 lines

It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put you
in
CC.

Quoted 6 lines

Quoted 4 lines

ux500 uses this flag several time, I'd like make sure people are not relying
on
its broken implementation.

Ux500 audio is broken, but I'm fixing it a little at a time...

No problem with Ux500 audio, don't worry :)
Audio is just one application among others.

The concern regarding ux500 is that the clock controller declares clocks using the CLK_SET_RATE_GATE flag (like qcom, at91 and several other)

here is the definition:
#define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */

My interpretation it that, as long as clock is enabled, rate can't change.

The implementation of this flag is currently broken:
* If you call set_rate on directly on the clock while it is enabled, you will get -EBUSY, as expected
* If you call set_rate on its parent, rate will change, changing the rate of the
child clock as well ...

With this patchset applied, calling set_rate on the parent would also return -EBUSY, enforcing the "rate can't change while enabled" property.

To build confidence that this won't be causing regression, I'd like to check that platform using this flag are no relying on the broken behavior.

I've included the clock maintainers of at91, qcom, and ux500 (you) in this thread because they are the heaviest users of this flag, so the more likely to report a problem.

If you could apply this series and just do things as usual, It'd be awesome !

I'll also seek the help of the kci guys, see if it is possible to give the series spin on it.

Quoted 3 lines

Yours,
Linus Walleij

I managed to have a run on kci - based on v4.12-rc6:
https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-10-ge
a373ddef830/

There was no build regression but kci did find one boot regression on qcom platforms:
* qcom-apq8064-cm-qs600
* qcom-apq8064-ifc6410

it seems the problem is coming from the clock used by the mmc driver (drivers/mmc/host/mmci.c)

the driver does following sequence:
* get_clk
* prepare_enable
* get_rate
* set_rate
* ...

with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has CLK_SET_RATE_PARENT flag so it will transmit the request to its parent.
The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.

This can't work with sequence above. The flag was clearly bypassed before, so I think it would be best to remove CLK_SET_RATE_GATE from the SDCx_src clocks

Stephen, would you agree ?

There was an issue with the chip board as well but this particular board has not been very reliable in this lab lately. It has failed on several other job.

Quoted 30 lines · Jerome Brunet

I managed to have a run on kci - based on v4.12-rc6:
https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-1
0-ge
a373ddef830/

There was no build regression but kci did find one boot regression on qcom platforms:
* qcom-apq8064-cm-qs600
* qcom-apq8064-ifc6410

it seems the problem is coming from the clock used by the mmc driver (drivers/mmc/host/mmci.c)

the driver does following sequence:
* get_clk
* prepare_enable
* get_rate
* set_rate
* ...

with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has CLK_SET_RATE_PARENT
flag so it will transmit the request to its parent.
The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.

This can't work with sequence above. The flag was clearly bypassed before, so I
think it would be best to remove CLK_SET_RATE_GATE from the SDCx_src clocks

Quoted 6 lines

Stephen, would you agree ?

There was an issue with the chip board as well but this particular board has not
been very reliable in this lab lately. It has failed on several other job.

Yes the flag was never working before and we've been ignoring the subtle problems that it causes to change the rate while the clk is enabled. On these older SoCs, all the clks marked with CLK_SET_RATE_GATE need to implement something to forcibly disable all child clks across the rate change. That is the hardware recommended procedure.

So something like

get_clk
prepare_enable
get_rate
set_rate
call ->set_rate() op
forcibly disable children of rcgs actually change the rate of rcg forcibly enable all children that were disabled

and have that happen under one big spinlock local to the qcom driver I suppose so that enable/disable can't leak into the rate change. Fun! I'll make an attempt at implementing the right solution tomorrow/tonight. Of course, we can remove the flag from qcom drivers if it's blocking this series.

Quoted 38 lines · Stephen Boyd

Quoted 36 lines · Jerome Brunet

I managed to have a run on kci - based on v4.12-rc6:
https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-1
0-ge
a373ddef830/

There was no build regression but kci did find one boot regression on qcom platforms:
* qcom-apq8064-cm-qs600
* qcom-apq8064-ifc6410

it seems the problem is coming from the clock used by the mmc driver (drivers/mmc/host/mmci.c)

the driver does following sequence:
* get_clk
* prepare_enable
* get_rate
* set_rate
* ...

with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has CLK_SET_RATE_PARENT
flag so it will transmit the request to its parent.
The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.

This can't work with sequence above. The flag was clearly bypassed before, so I
think it would be best to remove CLK_SET_RATE_GATE from the SDCx_src clocks

Stephen, would you agree ?

There was an issue with the chip board as well but this particular board has not
been very reliable in this lab lately. It has failed on several other job.

Quoted 6 lines

Yes the flag was never working before and we've been ignoring the subtle problems that it causes to change the rate while the clk is enabled. On these older SoCs, all the clks marked with CLK_SET_RATE_GATE need to implement something to forcibly disable all child clks across the rate change. That is the hardware recommended procedure.

Seems that HW has been doing fine w/o recommended procedure so far ;)

Quoted 13 lines

So something like

get_clk
prepare_enable
get_rate
set_rate
call ->set_rate() op
forcibly disable children of rcgs actually change the rate of rcg forcibly enable all children that were disabled

Quoted 6 lines

and have that happen under one big spinlock local to the qcom driver I suppose so that enable/disable can't leak into the rate change. Fun! I'll make an attempt at implementing the right solution tomorrow/tonight. Of course, we can remove the flag from qcom drivers if it's blocking this series.

Yeah, it is probably blocking the series.

In the following mail, I've only replied where I wanted to discuss a bit more. For everything else, please consider the comments agreed and the modifications done as requested.

Quoted 4 lines

In the following mail, I've only replied where I wanted to discuss a bit more. For everything else, please consider the comments agreed and the modifications done as requested.

Ok. Good that we can move forward in some way so this is not a blocker. I've almost completed writing the qcom code to force clks on and off so it will be resolved one way or the other soon.

Signature · 3 lines

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

Quoting Stephen Boyd (2017-07-27 15:44:59)

Quoted 8 lines · Jerome Brunet

Quoted 6 lines

In the following mail, I've only replied where I wanted to discuss a bit more.
For everything else, please consider the comments agreed and the modifications
done as requested.

Quoted 3 lines

Ok. Good that we can move forward in some way so this is not a blocker. I've almost completed writing the qcom code to force clks on and off so it will be resolved one way or the other soon.

Stephen, are all of the existential issues are sufficiently resolved?

Jerome, are you going to submit a v4?

Thanks,
Mike

Quoted 4 lines

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

This Patchset is related the RFC [0] and the discussion around CLK_SET_RATE_GATE available here [1]

This patchset is based on clk-next.

The goal of this patchset is to provide a way for consumers to inform the system that they depend on the rate of the clock source and can't tolerate other consumers changing the rate or causing glitches.

With this series there is 3 use-case:
?- the provider is not protected: nothing changes
?- the provider is protected by only 1 consumer (and only once), then only ???this consumer will be able to alter the rate of the clock, as it is the ???only one depending on it.
?- If the provider is protected more than once, or by the provider itself, ???the rate is basically locked and protected against reparenting.

The last patch provide the same functionnality for providers themself, fixing CLK_SET_RATE_GATE flag (enforce clock gating along the tree)

qcom, at91 and ux500 are the heaviest users of this flag. If anybody having one these platform could try this series, it would help build confidence that the platform relying on CLK_SET_RATE_GATE (even if broken) don't get any regression.

Changes since RFC:
?- s/clk_protect/clk_rate_protect
?- Request rework around core_nolock function
?- Add clk_set_rate_protect
?- Reword clk_rate_protect and clk_unprotect documentation ?- Add few comments to explain the code
?- Add 2 last patches to fix users of CLK_SET_RATE_GATE

Changes since v1:
?- Add patch 4: Check if the rate would actually change before continuing, a ???possibly in clk_set_rate.

Changes since v2: [2]
?- Fix issues reported by Adriana Reus (Thanks !)
?- Dropped patch "clk: move CLK_SET_RATE_GATE protection from prepare ???to enable". This was broken as the protect count, like the prepare_count ???should only be accessed under the prepare_lock.
?- No change around bail-out code. Mike wanted to ponder a bit more on it. ?- Patch 1 still included, could not find the clk-next-protect Mike was ???referring to.

This was tested with the audio use case mentioned in [1]

[0]: https://lkml.kernel.org/r/20170321183330.26722-1-jbrunet at baylibre.com [1]: https://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resonan ce
[2]: https://lkml.kernel.org/r/20170521215958.19743-1-jbrunet at baylibre.com

Jerome Brunet (10):
? clk: take the prepare lock out of clk_core_set_parent ? clk: add clk_core_set_phase_nolock function
? clk: rework calls to round and determine rate callbacks ? clk: use round rate to bail out early in set_rate ? clk: add support for clock protection
? clk: add clk_set_rate_protect
? clk: rollback set_rate_range changes on failure ? clk: cosmetic changes to clk_summary debugfs entry ? clk: fix incorrect usage of ENOSYS
? clk: fix CLK_SET_RATE_GATE with clock rate protection

?drivers/clk/clk.c????????????| 428 ++++++++++++++++++++++++++++++++++++----
---
?include/linux/clk-provider.h |???1 + ?include/linux/clk.h??????????|??43 +++++
?3 files changed, 402 insertions(+), 70 deletions(-)