[RFC 0/7] clk: implement clock locking mechanism

[RFC 0/7] clk: implement clock locking mechanism

[Jerome Brunet]

This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
clk_lock which available here [0]

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.

Patches 1 to 3 are just rework to ease the integration of the locking
mechanism. They should not introduce any functional differences.

Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
clk_protect and clk_unprotect (The "lock" word is already used lot in
clk.c. Using clk_lock and clk_unlock would have been very messy. If you
found "protect" to be a poor choice, I'm happy to sed the whole thing in
future version)

These calls can happen at anytime during the life of the clk. As for
prepare and enable, the calls must be balanced.

Inside the clock framework, 2 new count values are introduced to keep track
of the protection:
* core "protect_count": this reference count value works in the same way as
  prepare and enable count, and track whether the clock is protected or
  not. This is the value that will checked to allow operation which may
  cause glitches or change the rate of clock source.
* clk consumer "protect_ucount": this allows to track if the consumer is
  protecting the clock.

Requiring the consumer to unprotect its clock before changing it would have
been very annoying for the consumer. It would also be unsafe, as it would
still be possible for another consumer to change the rate between the call
to set_rate and protect. This needs to happen in a critical section.  A
solution would be to add "set_rate_protect", but we would need to do the
same for the set_parent and set_phase (and the possible future
functions). It does not scale well.  The solution proposed in this patch is
to use the consumer protect count.

In function altering the clock, if the consumer is protecting the clock,
the protection is temporarily release under the prepare_lock. This ensure
that:
* the clock is still protect from another consumer because of the
  prepare_lock
* the rate set on a protected clock cannot change between set_rate and
  later enable
* only a consumer protecting a clock can do this temporary protection
  removal (less chance of people messing with the refcount)
* if more than one consumer protect the clock, it remains protected.
  Because the protection is removed for the calling consumer, it gives
  it a chance to jump to another provider, if available.

This protection mechanism assume that "protecting" consumer knows what it
is doing when it comes to setting the rate, and does not expect to be
protected against itself.

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

One caveat is the following case:
N+1 consumers protect their clocks. These clocks come from N possible
providers. We should able to satisfy at least N consumers before exhausting
the resources.  In the particular case where all the consumers call
"protect" before having a chance to call "set_rate", the last 2 consumers
will remains stuck on the last provider w/o being able to set the rate on
it. This means that in situation where there is 2 consumers on 1
providers, they will compete for the clock, and may end up in situation
where both protect the clock and none can set the rate they need.  This can
be solved if, when the rate fails to be set, the consumer release the
protection. Then the second consumer will be able to satisfy its request.

Patch 5 is a small change on set_rate_range to restore the old range on
failure. It don't use set_rate_range in any driver so I could not test this
change.

Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
less wide after adding protect count in it.

Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
incorrectly.

Bonus:

With this patchset, we should probably change the use the flags
"CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
them. Another solution would be to have them automatically gate the clock
before performing the related operation.  What is your view on this ?

[0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resonance

Jerome Brunet (7):
  clk: take the prepare lock out of clk_core_set_parent
  clk: add set_phase core function
  clk: rework calls to round and determine rate callbacks
  clk: add support for clock protection
  clk: rollback set_rate_range changes on failure
  clk: cosmetic changes to clk_summary debugfs entry
  clk: fix incorrect usage of ENOSYS

 drivers/clk/clk.c            | 313 ++++++++++++++++++++++++++++++++++---------
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 ++++
 3 files changed, 279 insertions(+), 64 deletions(-)

-- 
2.9.3

[RFC 1/7] clk: take the prepare lock out of clk_core_set_parent

[Jerome Brunet]

Rework set_parent core function so it can be called when the prepare lock
is already held by the caller.

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

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 67201f67a14a..e77f03a47da6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1794,23 +1794,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	if (!core)
 		return 0;
 
-	/* prevent racing with updates to the clock topology */
-	clk_prepare_lock();
-
 	if (core->parent == parent)
-		goto out;
+		return 0;
 
 	/* verify ops for for multi-parent clks */
-	if ((core->num_parents > 1) && (!core->ops->set_parent)) {
-		ret = -ENOSYS;
-		goto out;
-	}
+	if ((core->num_parents > 1) && (!core->ops->set_parent))
+		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) {
@@ -1818,8 +1811,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;
 	}
@@ -1829,7 +1821,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 
 	/* abort if a driver objects */
 	if (ret & NOTIFY_STOP_MASK)
-		goto out;
+		return ret;
 
 	/* do the re-parent */
 	ret = __clk_set_parent(core, parent, p_index);
@@ -1842,7 +1834,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		__clk_recalc_accuracies(core);
 	}
 
-out:
+	return ret;
+}
+
+static int clk_core_set_parent_lock(struct clk_core *core,
+				    struct clk_core *parent)
+{
+	int ret;
+
+	clk_prepare_lock();
+	ret = clk_core_set_parent(core, parent);
 	clk_prepare_unlock();
 
 	return ret;
@@ -1870,7 +1871,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (!clk)
 		return 0;
 
-	return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+	return clk_core_set_parent_lock(clk->core,
+					parent ? parent->core : NULL);
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
@@ -2720,7 +2722,7 @@ void clk_unregister(struct clk *clk)
 		/* Reparent all children to the orphan list. */
 		hlist_for_each_entry_safe(child, t, &clk->core->children,
 					  child_node)
-			clk_core_set_parent(child, NULL);
+			clk_core_set_parent_lock(child, NULL);
 	}
 
 	hlist_del_init(&clk->core->child_node);
-- 
2.9.3

[RFC 2/7] clk: add set_phase core function

[Jerome Brunet]

Create a core function for set_phase, as it is done for set_rate and
set_parent.

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

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e77f03a47da6..57982a06dbce 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1876,6 +1876,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
+static int clk_core_set_phase(struct clk_core *core, int degrees)
+{
+	int ret = -EINVAL;
+
+	if (!core)
+		return 0;
+
+	trace_clk_set_phase(clk->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
@@ -1898,7 +1915,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
  */
 int clk_set_phase(struct clk *clk, int degrees)
 {
-	int ret = -EINVAL;
+	int ret;
 
 	if (!clk)
 		return 0;
@@ -1909,17 +1926,7 @@ int clk_set_phase(struct clk *clk, int degrees)
 		degrees += 360;
 
 	clk_prepare_lock();
-
-	trace_clk_set_phase(clk->core, degrees);
-
-	if (clk->core->ops->set_phase)
-		ret = clk->core->ops->set_phase(clk->core->hw, degrees);
-
-	trace_clk_set_phase_complete(clk->core, degrees);
-
-	if (!ret)
-		clk->core->phase = degrees;
-
+	ret = clk_core_set_phase(clk->core, degrees);
 	clk_prepare_unlock();
 
 	return ret;
-- 
2.9.3

[RFC 3/7] clk: rework calls to round and determine rate callbacks

[Jerome Brunet]

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 <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 78 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 57982a06dbce..fa77a1841e0f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -833,16 +833,34 @@ static int clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
-static int clk_core_round_rate_nolock(struct clk_core *core,
-				      struct clk_rate_request *req)
+static int clk_core_determine_round(struct clk_core *core,
+				    struct clk_rate_request *req)
 {
-	struct clk_core *parent;
 	long rate;
 
-	lockdep_assert_held(&prepare_lock);
+	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)
-		return 0;
+		req->rate = rate;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void clk_core_init_rate_req(struct clk_core *core,
+				   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;
 }
 
@@ -1354,36 +1374,26 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	clk_core_get_boundaries(core, &min_rate, &max_rate);
 
 	/* find the closest rate and parent clk/rate */
-	if (core->ops->determine_rate) {
+	if (core->ops->determine_rate || core->ops->round_rate) {
 		struct clk_rate_request req;
 
 		req.rate = rate;
 		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;
-	} else if (core->ops->round_rate) {
-		ret = core->ops->round_rate(core->hw, rate,
-					    &best_parent_rate);
-		if (ret < 0)
-			return NULL;
 
-		new_rate = ret;
 		if (new_rate < min_rate || new_rate > max_rate)
 			return NULL;
+
 	} else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
 		/* pass-through clock without adjustable parent */
 		core->new_rate = core->rate;
-- 
2.9.3

[RFC 4/7] clk: add support for clock protection

[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
changing the rate or introducing glitches while the clock is protected.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c            | 177 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 +++++++
 3 files changed, 197 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fa77a1841e0f..69db8cc15063 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -60,6 +60,7 @@ struct clk_core {
 	bool			orphan;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
+	unsigned int		protect_count;
 	unsigned long		min_rate;
 	unsigned long		max_rate;
 	unsigned long		accuracy;
@@ -84,6 +85,7 @@ struct clk {
 	const char *con_id;
 	unsigned long min_rate;
 	unsigned long max_rate;
+	unsigned long protect_ucount;
 	struct hlist_node clks_node;
 };
 
@@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
 	return core->ops->is_prepared(core->hw);
 }
 
+static bool clk_core_is_protected(struct clk_core *core)
+{
+	return core->protect_count;
+}
+
 static bool clk_core_is_enabled(struct clk_core *core)
 {
 	/*
@@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
 	return clk_core_is_prepared(hw->core);
 }
 
+bool clk_hw_is_protected(const struct clk_hw *hw)
+{
+	return clk_core_is_protected(hw->core);
+}
+
 bool clk_hw_is_enabled(const struct clk_hw *hw)
 {
 	return clk_core_is_enabled(hw->core);
@@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+static void clk_core_unprotect(struct clk_core *core)
+{
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	if (WARN_ON(core->protect_count == 0))
+		return;
+
+	if (--core->protect_count > 0)
+		return;
+
+	clk_core_unprotect(core->parent);
+}
+
+/**
+ * clk_unprotect - unprotect a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect shall be used when a consumer no longer depends on the clock
+ * rate and can tolerate glitches. As with clk_unprepare and clk_enable, calls
+ * to clk_unprotect must be balanced with clk_protect.
+ * clk_protect may sleep
+ */
+void clk_unprotect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+
+	if (WARN_ON(clk->protect_ucount <= 0)) {
+		/*
+		 * There is something wrong with this consumer protect count.
+		 * Stop here before messing with the provider
+		 */
+		clk_prepare_unlock();
+		return;
+	}
+
+	clk_core_unprotect(clk->core);
+	clk->protect_ucount--;
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_unprotect);
+
+static void clk_core_protect(struct clk_core *core)
+{
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	if (core->protect_count == 0)
+		clk_core_protect(core->parent);
+
+	core->protect_count++;
+}
+
+/**
+ * clk_protect - protect a clock source
+ * @clk: the clk being protected
+ *
+ * clk_protect can be used when a consumer depends on the clock rate and can't
+ * tolerate any glitches. The consumer protecting the clock can still make
+ * adjustment to clock, if it is the only one protecting the clock. Other
+ * consumers can still use the clock but won't be able to adjust the rate or
+ * reparent the clock while it is protected.
+ * clk_protect may sleep.
+ */
+void clk_protect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+	clk_core_protect(clk->core);
+	clk->protect_ucount++;
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_protect);
+
 static void clk_core_disable(struct clk_core *core)
 {
 	lockdep_assert_held(&enable_lock);
@@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core *core,
 {
 	long rate;
 
-	if (core->ops->determine_rate) {
+	if (clk_core_is_protected(core)) {
+		req->rate = core->rate;
+	} else if (core->ops->determine_rate) {
 		return core->ops->determine_rate(core->hw, req);
 	} else if (core->ops->round_rate) {
 		rate = core->ops->round_rate(core->hw, req->rate,
@@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		req.min_rate = min_rate;
 		req.max_rate = max_rate;
 
-		clk_core_init_rate_req(core, req);
+		clk_core_init_rate_req(core, &req);
 
 		ret = clk_core_determine_round(core, &req);
 		if (ret < 0)
@@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	if (clk->protect_ucount)
+		clk_core_unprotect(clk->core);
+
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 
+	if (clk->protect_ucount)
+		clk_core_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 
 	clk_prepare_lock();
 
+	if (clk->protect_ucount)
+		clk_core_unprotect(clk->core);
+
 	if (min != clk->min_rate || 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 (clk->protect_ucount)
+		clk_core_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
 		return -EBUSY;
 
+	if (clk_core_is_protected(core))
+		return -EBUSY;
+
 	/* try finding the new parent index */
 	if (parent) {
 		p_index = clk_fetch_parent_index(core, parent);
@@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core *core,
  */
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
+	int ret;
+
 	if (!clk)
 		return 0;
 
-	return clk_core_set_parent_lock(clk->core,
-					parent ? parent->core : NULL);
+	clk_prepare_lock();
+
+	if (clk->protect_ucount)
+		clk_core_unprotect(clk->core);
+
+	ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+
+	if (clk->protect_ucount)
+		clk_core_protect(clk->core);
+
+	clk_prepare_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
@@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core, int degrees)
 	if (!core)
 		return 0;
 
-	trace_clk_set_phase(clk->core, degrees);
+	if (clk_core_is_protected(core))
+		return -EBUSY;
+
+	trace_clk_set_phase(core, degrees);
 
 	if (core->ops->set_phase)
 		ret = core->ops->set_phase(core->hw, degrees);
@@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
 		degrees += 360;
 
 	clk_prepare_lock();
+
+	if (clk->protect_ucount)
+		clk_core_unprotect(clk->core);
+
 	ret = clk_core_set_phase(clk->core, degrees);
+
+	if (clk->protect_ucount)
+		clk_core_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
 	if (!c)
 		return;
 
-	seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
+	seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
 		   level * 3 + 1, "",
 		   30 - level * 3, c->name,
-		   c->enable_count, c->prepare_count, clk_core_get_rate(c),
-		   clk_core_get_accuracy(c), clk_core_get_phase(c));
+		   c->enable_count, c->prepare_count, c->protect_count,
+		   clk_core_get_rate(c), clk_core_get_accuracy(c),
+		   clk_core_get_phase(c));
 }
 
 static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
@@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
 	struct clk_core *c;
 	struct hlist_head **lists = (struct hlist_head **)s->private;
 
-	seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
-	seq_puts(s, "----------------------------------------------------------------------------------------\n");
+	seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
+	seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
 
 	clk_prepare_lock();
 
@@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
 	seq_printf(s, "\"%s\": { ", c->name);
 	seq_printf(s, "\"enable_count\": %d,", c->enable_count);
 	seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
+	seq_printf(s, "\"protect_count\": %d,", c->protect_count);
 	seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
 	seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
 	seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
@@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 	if (!d)
 		goto err_out;
 
+	d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
+			(u32 *)&core->protect_count);
+	if (!d)
+		goto err_out;
+
 	d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
 			(u32 *)&core->notifier_count);
 	if (!d)
@@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
 	if (clk->core->prepare_count)
 		pr_warn("%s: unregistering prepared clock: %s\n",
 					__func__, clk->core->name);
+
+	if (clk->core->protect_count)
+		pr_warn("%s: unregistering protected clock: %s\n",
+					__func__, clk->core->name);
+
 	kref_put(&clk->core->ref, __clk_release);
 unlock:
 	clk_prepare_unlock();
@@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_lock();
 
+	/* Protect count not balanced: warn and sanitize */
+	if (clk->protect_ucount) {
+		pr_warn("%s: releasing protected clock: %s\n",
+					__func__, clk->core->name);
+
+		for (; clk->protect_ucount; clk->protect_ucount--)
+			clk_core_unprotect(clk->core);
+	}
+
 	hlist_del(&clk->clks_node);
 	if (clk->min_rate > clk->core->req_rate ||
 	    clk->max_rate < clk->core->req_rate)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a428aec36ace..705a158d9b8f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 unsigned long __clk_get_flags(struct clk *clk);
 unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_protected(const struct clk_hw *hw);
 bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
 struct clk *__clk_lookup(const char *name);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9d36b3e49de..90b72ead4411 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -265,6 +265,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_protect - inform the system when the clock source should 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.
+ */
+void clk_protect(struct clk *clk);
+
+/**
+ * clk_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting the
+ * clock source can now deal with other consumer altering the clock source.
+ *
+ * The caller must balance the number of protect and unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprotect(struct clk *clk);
 
 /**
  * clk_enable - inform the system when the clock source should be running.
@@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+
+static inline void clk_protect(struct clk *clk) {}
+
+static inline void clk_unprotect(struct clk *clk) {}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
-- 
2.9.3

[RFC 5/7] clk: rollback set_rate_range changes on failure

[Jerome Brunet]

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 69db8cc15063..50798ef618d1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1759,6 +1759,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate);
 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;
@@ -1776,9 +1777,16 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		clk_core_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_ucount)
-- 
2.9.3

[RFC 6/7] clk: cosmetic changes to clk_summary debugfs entry

[Jerome Brunet]

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 <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 50798ef618d1..ec6d57e97bac 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2167,7 +2167,7 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
 	if (!c)
 		return;
 
-	seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
+	seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %-3d\n",
 		   level * 3 + 1, "",
 		   30 - level * 3, c->name,
 		   c->enable_count, c->prepare_count, c->protect_count,
@@ -2194,8 +2194,9 @@ static int clk_summary_show(struct seq_file *s, void *data)
 	struct clk_core *c;
 	struct hlist_head **lists = (struct hlist_head **)s->private;
 
-	seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
-	seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
+	seq_puts(s, "                                 enable  prepare  protect                               \n");
+	seq_puts(s, "   clock                          count    count    count        rate   accuracy   phase\n");
+	seq_puts(s, "----------------------------------------------------------------------------------------\n");
 
 	clk_prepare_lock();
 
-- 
2.9.3

[RFC 7/7] clk: fix incorrect usage of ENOSYS

[Jerome Brunet]

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 <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ec6d57e97bac..377d5301348b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1926,7 +1926,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 
 	/* verify ops for for multi-parent clks */
 	if ((core->num_parents > 1) && (!core->ops->set_parent))
-		return -ENOSYS;
+		return -EPERM;
 
 	/* check that we are allowed to re-parent if the clock is in use */
 	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
-- 
2.9.3

[RFC 0/7] clk: implement clock locking mechanism

[Michael Turquette]

Hi J?r?me,

Quoting Jerome Brunet (2017-03-21 11:33:23)
> This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
> clk_lock which available here [0]
> 
> 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.
> 
> Patches 1 to 3 are just rework to ease the integration of the locking
> mechanism. They should not introduce any functional differences.
> 
> Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
> clk_protect and clk_unprotect (The "lock" word is already used lot in
> clk.c. Using clk_lock and clk_unlock would have been very messy. If you
> found "protect" to be a poor choice, I'm happy to sed the whole thing in
> future version)

I prefer clk_lock_rate and clk_unlock_rate.

> 
> These calls can happen at anytime during the life of the clk. As for
> prepare and enable, the calls must be balanced.
> 
> Inside the clock framework, 2 new count values are introduced to keep track
> of the protection:
> * core "protect_count": this reference count value works in the same way as
>   prepare and enable count, and track whether the clock is protected or
>   not. This is the value that will checked to allow operation which may
>   cause glitches or change the rate of clock source.
> * clk consumer "protect_ucount": this allows to track if the consumer is
>   protecting the clock.

Similarly, s/protect/lock_rate/g

> 
> Requiring the consumer to unprotect its clock before changing it would have
> been very annoying for the consumer. It would also be unsafe, as it would
> still be possible for another consumer to change the rate between the call
> to set_rate and protect. This needs to happen in a critical section.  A
> solution would be to add "set_rate_protect", but we would need to do the
> same for the set_parent and set_phase (and the possible future
> functions). It does not scale well.  The solution proposed in this patch is
> to use the consumer protect count.
> 
> In function altering the clock, if the consumer is protecting the clock,
> the protection is temporarily release under the prepare_lock. This ensure
> that:
> * the clock is still protect from another consumer because of the
>   prepare_lock
> * the rate set on a protected clock cannot change between set_rate and
>   later enable
> * only a consumer protecting a clock can do this temporary protection
>   removal (less chance of people messing with the refcount)

It should be impossible for consumer_A to remove the lock refcount set
by consumer_B using something like these patches I wrote for
prepare_count and enable_count:

https://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette at baylibre.com

> * if more than one consumer protect the clock, it remains protected.
>   Because the protection is removed for the calling consumer, it gives
>   it a chance to jump to another provider, if available.

Hmm, that's a bit confusing. I guess you mean, "if the rate a parent
clock is locked, and the child clock has multiple parents to choose
from, then the child may select a different parent with an unlocked
rate"?

> 
> This protection mechanism assume that "protecting" consumer knows what it
> is doing when it comes to setting the rate, and does not expect to be
> protected against itself.

Is this always true? Are there drivers where one code path requires a
stable rate (due to glitches) while another code path might try to
change the rate?

I can't think of any, but it's a big assumption to make. Anyone else
have thoughts on it?

> 
> This was tested with the audio use case mentioned in [0]
> 
> One caveat is the following case:
> N+1 consumers protect their clocks. These clocks come from N possible
> providers. We should able to satisfy at least N consumers before exhausting
> the resources.  In the particular case where all the consumers call
> "protect" before having a chance to call "set_rate", the last 2 consumers
> will remains stuck on the last provider w/o being able to set the rate on
> it. This means that in situation where there is 2 consumers on 1
> providers, they will compete for the clock, and may end up in situation
> where both protect the clock and none can set the rate they need.  This can
> be solved if, when the rate fails to be set, the consumer release the
> protection. Then the second consumer will be able to satisfy its request.
> 
> Patch 5 is a small change on set_rate_range to restore the old range on
> failure. It don't use set_rate_range in any driver so I could not test this
> change.
> 
> Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
> less wide after adding protect count in it.
> 
> Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
> incorrectly.
> 
> Bonus:
> 
> With this patchset, we should probably change the use the flags
> "CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
> them. Another solution would be to have them automatically gate the clock
> before performing the related operation.  What is your view on this ?

We'll need to ask the users of these flags. It would be nice to convert
those folks over to use clk_lock_rate() instead of those flags.

Regards,
Mike

> 
> [0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resonance
> 
> Jerome Brunet (7):
>   clk: take the prepare lock out of clk_core_set_parent
>   clk: add set_phase core function
>   clk: rework calls to round and determine rate callbacks
>   clk: add support for clock protection
>   clk: rollback set_rate_range changes on failure
>   clk: cosmetic changes to clk_summary debugfs entry
>   clk: fix incorrect usage of ENOSYS
> 
>  drivers/clk/clk.c            | 313 ++++++++++++++++++++++++++++++++++---------
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 ++++
>  3 files changed, 279 insertions(+), 64 deletions(-)
> 
> -- 
> 2.9.3
> 

[RFC 0/7] clk: implement clock locking mechanism

[Jerome Brunet]

On Tue, 2017-03-21 at 17:07 -0700, Michael Turquette wrote:
> Hi J?r?me,
> 
> Quoting Jerome Brunet (2017-03-21 11:33:23)
> > This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
> > clk_lock which available here [0]
> > 
> > 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.
> > 
> > Patches 1 to 3 are just rework to ease the integration of the locking
> > mechanism. They should not introduce any functional differences.
> > 
> > Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
> > clk_protect and clk_unprotect (The "lock" word is already used lot in
> > clk.c. Using clk_lock and clk_unlock would have been very messy. If you
> > found "protect" to be a poor choice, I'm happy to sed the whole thing in
> > future version)
> 
> I prefer clk_lock_rate and clk_unlock_rate.
> 
> > 
> > These calls can happen at anytime during the life of the clk. As for
> > prepare and enable, the calls must be balanced.
> > 
> > Inside the clock framework, 2 new count values are introduced to keep track
> > of the protection:
> > * core "protect_count": this reference count value works in the same way as
> > ? prepare and enable count, and track whether the clock is protected or
> > ? not. This is the value that will checked to allow operation which may
> > ? cause glitches or change the rate of clock source.
> > * clk consumer "protect_ucount": this allows to track if the consumer is
> > ? protecting the clock.
> 
> Similarly, s/protect/lock_rate/g
> 
> > 
> > Requiring the consumer to unprotect its clock before changing it would have
> > been very annoying for the consumer. It would also be unsafe, as it would
> > still be possible for another consumer to change the rate between the call
> > to set_rate and protect. This needs to happen in a critical section.??A
> > solution would be to add "set_rate_protect", but we would need to do the
> > same for the set_parent and set_phase (and the possible future
> > functions). It does not scale well.??The solution proposed in this patch is
> > to use the consumer protect count.
> > 
> > In function altering the clock, if the consumer is protecting the clock,
> > the protection is temporarily release under the prepare_lock. This ensure
> > that:
> > * the clock is still protect from another consumer because of the
> > ? prepare_lock
> > * the rate set on a protected clock cannot change between set_rate and
> > ? later enable
> > * only a consumer protecting a clock can do this temporary protection
> > ? removal (less chance of people messing with the refcount)
> 
> It should be impossible for consumer_A to remove the lock refcount set
> by consumer_B using something like these patches I wrote for
> prepare_count and enable_count:
> 
> https://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette at baylibr
> e.com
> 
Looks like what I have done here, except that this (core_count behind the
consumer count) is an added bonus.  The initial reason for it was to allow
consumer protecting the clock to do the temporary release of the protection
under the prepare_lock, so the consumer does not lock itself out.

> > * if more than one consumer protect the clock, it remains protected.
> > ? Because the protection is removed for the calling consumer, it gives
> > ? it a chance to jump to another provider, if available.
> 
> Hmm, that's a bit confusing. I guess you mean, "if the rate a parent
> clock is locked, and the child clock has multiple parents to choose
> from, then the child may select a different parent with an unlocked
> rate"?

Could not have said it better, apparently :)

> 
> > 
> > This protection mechanism assume that "protecting" consumer knows what it
> > is doing when it comes to setting the rate, and does not expect to be
> > protected against itself.
> 
> Is this always true? Are there drivers where one code path requires a
> stable rate (due to glitches) while another code path might try to
> change the rate?

For this particular case, each path can call "protect", protecting against each
other.

> 
> I can't think of any, but it's a big assumption to make. Anyone else
> have thoughts on it?

This mechanism I'm proposing is not a locking mechanism, but a way for a
consumer to inform the system "I heavily depends on this clock rate, it is
critical to my task, I can't tolerate anyone *else* from messing with it during
this specific period"

It effectively give the consumer the guarantee that the rate set under the
protection is the one it will get as long as it hold the protection

> 
> > 
> > This was tested with the audio use case mentioned in [0]
> > 
> > One caveat is the following case:
> > N+1 consumers protect their clocks. These clocks come from N possible
> > providers. We should able to satisfy at least N consumers before exhausting
> > the resources.??In the particular case where all the consumers call
> > "protect" before having a chance to call "set_rate", the last 2 consumers
> > will remains stuck on the last provider w/o being able to set the rate on
> > it. This means that in situation where there is 2 consumers on 1
> > providers, they will compete for the clock, and may end up in situation
> > where both protect the clock and none can set the rate they need.??This can
> > be solved if, when the rate fails to be set, the consumer release the
> > protection. Then the second consumer will be able to satisfy its request.
> > 
> > Patch 5 is a small change on set_rate_range to restore the old range on
> > failure. It don't use set_rate_range in any driver so I could not test this
> > change.
> > 
> > Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
> > less wide after adding protect count in it.
> > 
> > Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
> > incorrectly.
> > 
> > Bonus:
> > 
> > With this patchset, we should probably change the use the flags
> > "CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
> > them. Another solution would be to have them automatically gate the clock
> > before performing the related operation.??What is your view on this ?
> 
> We'll need to ask the users of these flags. It would be nice to convert
> those folks over to use clk_lock_rate() instead of those flags.
> 

These flag have a very different purpose in the end.
These flags *should* mean that "to perform this operation, the clock should be
gated" In a way, it is the provider protecting itself against all the consumers
(1).
The api proposed here allow consumers to protect against other consumers (2).

I think these are different things. Some tried to do (2) using these flags (like
 me) and probably failed (like me).

I think these flags are still useful (if fixed - enforcing the clock to be gated
to perform the operation) to implement (1) use-cases.

> Regards,
> Mike
> 
> > 
> > [0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resona
> > nce
> > 
> > Jerome Brunet (7):
> > ? clk: take the prepare lock out of clk_core_set_parent
> > ? clk: add set_phase core function
> > ? clk: rework calls to round and determine rate callbacks
> > ? clk: add support for clock protection
> > ? clk: rollback set_rate_range changes on failure
> > ? clk: cosmetic changes to clk_summary debugfs entry
> > ? clk: fix incorrect usage of ENOSYS
> > 
> > ?drivers/clk/clk.c????????????| 313 ++++++++++++++++++++++++++++++++++----
> > -----
> > ?include/linux/clk-provider.h |???1 +
> > ?include/linux/clk.h??????????|??29 ++++
> > ?3 files changed, 279 insertions(+), 64 deletions(-)
> > 
> > --?
> > 2.9.3
> > 

[RFC 3/7] clk: rework calls to round and determine rate callbacks

[Michael Turquette]

Quoting Jerome Brunet (2017-03-21 11:33:26)
> 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 <jbrunet@baylibre.com>

This was a bit ugly to read in unified format, but looks OKish to me.
This code can be fragile so some testing would be good.

Regards,
Mike

> ---
>  drivers/clk/clk.c | 78 +++++++++++++++++++++++++++++++------------------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 57982a06dbce..fa77a1841e0f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -833,16 +833,34 @@ static int clk_disable_unused(void)
>  }
>  late_initcall_sync(clk_disable_unused);
>  
> -static int clk_core_round_rate_nolock(struct clk_core *core,
> -                                     struct clk_rate_request *req)
> +static int clk_core_determine_round(struct clk_core *core,
> +                                   struct clk_rate_request *req)
>  {
> -       struct clk_core *parent;
>         long rate;
>  
> -       lockdep_assert_held(&prepare_lock);
> +       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)
> -               return 0;
> +               req->rate = rate;
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static void clk_core_init_rate_req(struct clk_core *core,
> +                                  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;
>  }
>  
> @@ -1354,36 +1374,26 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>         clk_core_get_boundaries(core, &min_rate, &max_rate);
>  
>         /* find the closest rate and parent clk/rate */
> -       if (core->ops->determine_rate) {
> +       if (core->ops->determine_rate || core->ops->round_rate) {
>                 struct clk_rate_request req;
>  
>                 req.rate = rate;
>                 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;
> -       } else if (core->ops->round_rate) {
> -               ret = core->ops->round_rate(core->hw, rate,
> -                                           &best_parent_rate);
> -               if (ret < 0)
> -                       return NULL;
>  
> -               new_rate = ret;
>                 if (new_rate < min_rate || new_rate > max_rate)
>                         return NULL;
> +
>         } else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
>                 /* pass-through clock without adjustable parent */
>                 core->new_rate = core->rate;
> -- 
> 2.9.3
> 

[RFC 1/7] clk: take the prepare lock out of clk_core_set_parent

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 19:33:24)
> Rework set_parent core function so it can be called when the prepare lock
> is already held by the caller.
> 
> This rework is done to ease the integration of the "protected" clock
> functionality.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 67201f67a14a..e77f03a47da6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1794,23 +1794,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)

Nitpick: rename clk_core_set_parent to clk_core_st_parent_nolock. Drop
the clk_core_set_parent_lock name below and just call it
clk_core_set_parent.

This matches all the other nolock patterns in clk.c.

>         if (!core)
>                 return 0;
>  
> -       /* prevent racing with updates to the clock topology */
> -       clk_prepare_lock();
> -
>         if (core->parent == parent)
> -               goto out;
> +               return 0;
>  
>         /* verify ops for for multi-parent clks */
> -       if ((core->num_parents > 1) && (!core->ops->set_parent)) {
> -               ret = -ENOSYS;
> -               goto out;
> -       }
> +       if ((core->num_parents > 1) && (!core->ops->set_parent))
> +               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) {
> @@ -1818,8 +1811,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;
>         }
> @@ -1829,7 +1821,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>  
>         /* abort if a driver objects */
>         if (ret & NOTIFY_STOP_MASK)
> -               goto out;
> +               return ret;
>  
>         /* do the re-parent */
>         ret = __clk_set_parent(core, parent, p_index);
> @@ -1842,7 +1834,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>                 __clk_recalc_accuracies(core);
>         }
>  
> -out:
> +       return ret;

I do not understand the benefit of the code churn above where gotos are
replaced with returns. This should be a few line patch but it is much
more than that due to this churn.

> +}
> +
> +static int clk_core_set_parent_lock(struct clk_core *core,
> +                                   struct clk_core *parent)
> +{
> +       int ret;
> +
> +       clk_prepare_lock();
> +       ret = clk_core_set_parent(core, parent);

Above line should be nolock.

>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1870,7 +1871,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> +       return clk_core_set_parent_lock(clk->core,
> +                                       parent ? parent->core : NULL);

We can avoid this line by using the nolock pattern.

Basic premise of the patch is OK though.

Regards,
Mike

>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> @@ -2720,7 +2722,7 @@ void clk_unregister(struct clk *clk)
>                 /* Reparent all children to the orphan list. */
>                 hlist_for_each_entry_safe(child, t, &clk->core->children,
>                                           child_node)
> -                       clk_core_set_parent(child, NULL);
> +                       clk_core_set_parent_lock(child, NULL);
>         }
>  
>         hlist_del_init(&clk->core->child_node);
> -- 
> 2.9.3
> 

[RFC 0/7] clk: implement clock locking mechanism

[Michael Turquette]

Hi Jerome,

Thanks for sending this series. The concept is one we've been talking
about for years, and your approach makes sense. Some comments below.

Quoting Jerome Brunet (2017-03-21 19:33:23)
> This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
> clk_lock which available here [0]

If we merge this solution then we may want to convert some of those
CLK_SET_RATE_GATE users and potentially some of the rate-range users
that set min/max to the same value over to this new api.

> 
> 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.
> 
> Patches 1 to 3 are just rework to ease the integration of the locking
> mechanism. They should not introduce any functional differences.
> 
> Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
> clk_protect and clk_unprotect (The "lock" word is already used lot in
> clk.c. Using clk_lock and clk_unlock would have been very messy. If you
> found "protect" to be a poor choice, I'm happy to sed the whole thing in
> future version)
> 
> These calls can happen at anytime during the life of the clk. As for
> prepare and enable, the calls must be balanced.
> 
> Inside the clock framework, 2 new count values are introduced to keep track
> of the protection:
> * core "protect_count": this reference count value works in the same way as
>   prepare and enable count, and track whether the clock is protected or
>   not. This is the value that will checked to allow operation which may
>   cause glitches or change the rate of clock source.
> * clk consumer "protect_ucount": this allows to track if the consumer is
>   protecting the clock.
> 
> Requiring the consumer to unprotect its clock before changing it would have
> been very annoying for the consumer. It would also be unsafe, as it would
> still be possible for another consumer to change the rate between the call
> to set_rate and protect. This needs to happen in a critical section.  A
> solution would be to add "set_rate_protect", but we would need to do the
> same for the set_parent and set_phase (and the possible future
> functions). It does not scale well.  The solution proposed in this patch is
> to use the consumer protect count.
> 
> In function altering the clock, if the consumer is protecting the clock,
> the protection is temporarily release under the prepare_lock. This ensure
> that:
> * the clock is still protect from another consumer because of the
>   prepare_lock
> * the rate set on a protected clock cannot change between set_rate and
>   later enable
> * only a consumer protecting a clock can do this temporary protection
>   removal (less chance of people messing with the refcount)
> * if more than one consumer protect the clock, it remains protected.
>   Because the protection is removed for the calling consumer, it gives
>   it a chance to jump to another provider, if available.
> 
> This protection mechanism assume that "protecting" consumer knows what it
> is doing when it comes to setting the rate, and does not expect to be
> protected against itself.
> 
> This was tested with the audio use case mentioned in [0]
> 
> One caveat is the following case:
> N+1 consumers protect their clocks. These clocks come from N possible
> providers. We should able to satisfy at least N consumers before exhausting
> the resources.  In the particular case where all the consumers call
> "protect" before having a chance to call "set_rate", the last 2 consumers
> will remains stuck on the last provider w/o being able to set the rate on
> it. This means that in situation where there is 2 consumers on 1
> providers, they will compete for the clock, and may end up in situation
> where both protect the clock and none can set the rate they need.  This can
> be solved if, when the rate fails to be set, the consumer release the
> protection. Then the second consumer will be able to satisfy its request.

This situation can be handled a bit more gracefully if clk_set_rate_lock
both returns an error code if setting the rate failes AND it releases
the rate lock in that case. At least that helps for the case of
initializing rates during .probe(). Automatically dropping the lock
might complicate things in other cases though...

Best regards,
Mike

> 
> Patch 5 is a small change on set_rate_range to restore the old range on
> failure. It don't use set_rate_range in any driver so I could not test this
> change.
> 
> Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
> less wide after adding protect count in it.
> 
> Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
> incorrectly.
> 
> Bonus:
> 
> With this patchset, we should probably change the use the flags
> "CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
> them. Another solution would be to have them automatically gate the clock
> before performing the related operation.  What is your view on this ?
> 
> [0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resonance
> 
> Jerome Brunet (7):
>   clk: take the prepare lock out of clk_core_set_parent
>   clk: add set_phase core function
>   clk: rework calls to round and determine rate callbacks
>   clk: add support for clock protection
>   clk: rollback set_rate_range changes on failure
>   clk: cosmetic changes to clk_summary debugfs entry
>   clk: fix incorrect usage of ENOSYS
> 
>  drivers/clk/clk.c            | 313 ++++++++++++++++++++++++++++++++++---------
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 ++++
>  3 files changed, 279 insertions(+), 64 deletions(-)
> 
> -- 
> 2.9.3
> 

[RFC 2/7] clk: add set_phase core function

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 19:33:25)
> Create a core function for set_phase, as it is done for set_rate and
> set_parent.
> 
> This rework is done to ease the integration of "protected" clock
> functionality.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e77f03a47da6..57982a06dbce 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1876,6 +1876,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> +static int clk_core_set_phase(struct clk_core *core, int degrees)

s/clk_core_set_phase/clk_core_set_phase_nolock/

Otherwise patch looks OK to me.

Regards,
Mike

> +{
> +       int ret = -EINVAL;
> +
> +       if (!core)
> +               return 0;
> +
> +       trace_clk_set_phase(clk->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
> @@ -1898,7 +1915,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
>   */
>  int clk_set_phase(struct clk *clk, int degrees)
>  {
> -       int ret = -EINVAL;
> +       int ret;
>  
>         if (!clk)
>                 return 0;
> @@ -1909,17 +1926,7 @@ int clk_set_phase(struct clk *clk, int degrees)
>                 degrees += 360;
>  
>         clk_prepare_lock();
> -
> -       trace_clk_set_phase(clk->core, degrees);
> -
> -       if (clk->core->ops->set_phase)
> -               ret = clk->core->ops->set_phase(clk->core->hw, degrees);
> -
> -       trace_clk_set_phase_complete(clk->core, degrees);
> -
> -       if (!ret)
> -               clk->core->phase = degrees;
> -
> +       ret = clk_core_set_phase(clk->core, degrees);
>         clk_prepare_unlock();
>  
>         return ret;
> -- 
> 2.9.3
> 

[RFC 4/7] clk: add support for clock protection

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 11:33:27)
> 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.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c            | 177 ++++++++++++++++++++++++++++++++++++++++---
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 +++++++
>  3 files changed, 197 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fa77a1841e0f..69db8cc15063 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -60,6 +60,7 @@ struct clk_core {
>         bool                    orphan;
>         unsigned int            enable_count;
>         unsigned int            prepare_count;
> +       unsigned int            protect_count;
>         unsigned long           min_rate;
>         unsigned long           max_rate;
>         unsigned long           accuracy;
> @@ -84,6 +85,7 @@ struct clk {
>         const char *con_id;
>         unsigned long min_rate;
>         unsigned long max_rate;
> +       unsigned long protect_ucount;
>         struct hlist_node clks_node;
>  };
>  
> @@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
>         return core->ops->is_prepared(core->hw);
>  }
>  
> +static bool clk_core_is_protected(struct clk_core *core)
> +{
> +       return core->protect_count;
> +}
> +
>  static bool clk_core_is_enabled(struct clk_core *core)
>  {
>         /*
> @@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
>         return clk_core_is_prepared(hw->core);
>  }
>  
> +bool clk_hw_is_protected(const struct clk_hw *hw)
> +{
> +       return clk_core_is_protected(hw->core);
> +}
> +
>  bool clk_hw_is_enabled(const struct clk_hw *hw)
>  {
>         return clk_core_is_enabled(hw->core);
> @@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_prepare);
>  
> +static void clk_core_unprotect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (WARN_ON(core->protect_count == 0))
> +               return;
> +
> +       if (--core->protect_count > 0)
> +               return;
> +
> +       clk_core_unprotect(core->parent);
> +}
> +
> +/**
> + * clk_unprotect - unprotect a clock source
> + * @clk: the clk being unprotected
> + *
> + * clk_unprotect shall be used when a consumer no longer depends on the clock
> + * rate and can tolerate glitches. As with clk_unprepare and clk_enable, calls
> + * to clk_unprotect must be balanced with clk_protect.
> + * clk_protect may sleep

Maybe:

"""
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_unprotect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +
> +       if (WARN_ON(clk->protect_ucount <= 0)) {
> +               /*
> +                * There is something wrong with this consumer protect count.
> +                * Stop here before messing with the provider
> +                */
> +               clk_prepare_unlock();
> +               return;
> +       }
> +
> +       clk_core_unprotect(clk->core);
> +       clk->protect_ucount--;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_unprotect);
> +
> +static void clk_core_protect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (core->protect_count == 0)
> +               clk_core_protect(core->parent);
> +
> +       core->protect_count++;
> +}
> +
> +/**
> + * clk_protect - protect a clock source
> + * @clk: the clk being protected
> + *
> + * clk_protect can be used when a consumer depends on the clock rate and can't
> + * tolerate any glitches. The consumer protecting the clock can still make
> + * adjustment to clock, if it is the only one protecting the clock. Other
> + * consumers can still use the clock but won't be able to adjust the rate or
> + * reparent the clock while it is protected.
> + * clk_protect may sleep.
> + */

Maybe:

"""
clk_protect begins a critical section during which the clock consumer
cannot tolerate any change to the clock rate. This results in all clocks
up the parent chain to also be rate-protected.

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_protect should be balanced with calls to clk_unprotect.
Calls to this function may sleep, and do not return error status.
"""

> +void clk_protect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +       clk_core_protect(clk->core);
> +       clk->protect_ucount++;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_protect);
> +
>  static void clk_core_disable(struct clk_core *core)
>  {
>         lockdep_assert_held(&enable_lock);
> @@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core *core,
>  {
>         long rate;
>  
> -       if (core->ops->determine_rate) {
> +       if (clk_core_is_protected(core)) {
> +               req->rate = core->rate;

Hmm, in CCF we basically re-use round_rate/determine_rate from within
calls to clk_set_rate. The point is that clk_set_rate should set the
same rate that is returned from either of the other two functions.

The change above breaks that subtle assumption, as a clk consumer can
now call:

	clk_protect(clk);

	rate = clk_get_rate(clk);		// rate is 1234;

	rate = clk_round_rate(clk, 5678);	// rate is STILL 1234

	ret = clk_set_rate(clk, 5678);

	rate = clk_get_rate(clk);		// rate is 5678

Is my understanding correct? If so then we might consider adding some
more complex knowledge to clk_round_rate. Something like:

	if clk_is_protected(clk) AND it is only protect by THIS clk:
		round the rate
	else:
		return core->rate

> +       } else if (core->ops->determine_rate) {
>                 return core->ops->determine_rate(core->hw, req);
>         } else if (core->ops->round_rate) {
>                 rate = core->ops->round_rate(core->hw, req->rate,
> @@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                 req.min_rate = min_rate;
>                 req.max_rate = max_rate;
>  
> -               clk_core_init_rate_req(core, req);
> +               clk_core_init_rate_req(core, &req);

Why isn't this change in patch #3? Seems like a bug introduced in patch
#3 and fixed here in patch #4.

>  
>                 ret = clk_core_determine_round(core, &req);
>                 if (ret < 0)
> @@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>         /* prevent racing with updates to the clock topology */
>         clk_prepare_lock();

The fact that a user can protect a clk AND change its rate is very
subtle. Please update the kerneldoc for clk_set_rate, clk_set_parent and
any other affected apis.

>  
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);

What happens here if two different users have both protected this same
clk? Looks like we ignore the second user?

In other words, what happens if clk->protect_ucount == 2?


Best regards,
Mike

> +
>         ret = clk_core_set_rate_nolock(clk->core, rate);
>  
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>  
>         clk_prepare_lock();
>  
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
>         if (min != clk->min_rate || 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 (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>         if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
>                 return -EBUSY;
>  
> +       if (clk_core_is_protected(core))
> +               return -EBUSY;
> +
>         /* try finding the new parent index */
>         if (parent) {
>                 p_index = clk_fetch_parent_index(core, parent);
> @@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core *core,
>   */
>  int clk_set_parent(struct clk *clk, struct clk *parent)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_set_parent_lock(clk->core,
> -                                       parent ? parent->core : NULL);
> +       clk_prepare_lock();
> +
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
> +       ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> +
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
> +       clk_prepare_unlock();
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> @@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core, int degrees)
>         if (!core)
>                 return 0;
>  
> -       trace_clk_set_phase(clk->core, degrees);
> +       if (clk_core_is_protected(core))
> +               return -EBUSY;
> +
> +       trace_clk_set_phase(core, degrees);
>  
>         if (core->ops->set_phase)
>                 ret = core->ops->set_phase(core->hw, degrees);
> @@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
>                 degrees += 360;
>  
>         clk_prepare_lock();
> +
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
>         ret = clk_core_set_phase(clk->core, degrees);
> +
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
>         if (!c)
>                 return;
>  
> -       seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
> +       seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
>                    level * 3 + 1, "",
>                    30 - level * 3, c->name,
> -                  c->enable_count, c->prepare_count, clk_core_get_rate(c),
> -                  clk_core_get_accuracy(c), clk_core_get_phase(c));
> +                  c->enable_count, c->prepare_count, c->protect_count,
> +                  clk_core_get_rate(c), clk_core_get_accuracy(c),
> +                  clk_core_get_phase(c));
>  }
>  
>  static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
> @@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
>         struct clk_core *c;
>         struct hlist_head **lists = (struct hlist_head **)s->private;
>  
> -       seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
> -       seq_puts(s, "----------------------------------------------------------------------------------------\n");
> +       seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
> +       seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
>  
>         clk_prepare_lock();
>  
> @@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
>         seq_printf(s, "\"%s\": { ", c->name);
>         seq_printf(s, "\"enable_count\": %d,", c->enable_count);
>         seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
> +       seq_printf(s, "\"protect_count\": %d,", c->protect_count);
>         seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
>         seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
>         seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
> @@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
>         if (!d)
>                 goto err_out;
>  
> +       d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
> +                       (u32 *)&core->protect_count);
> +       if (!d)
> +               goto err_out;
> +
>         d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
>                         (u32 *)&core->notifier_count);
>         if (!d)
> @@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
>         if (clk->core->prepare_count)
>                 pr_warn("%s: unregistering prepared clock: %s\n",
>                                         __func__, clk->core->name);
> +
> +       if (clk->core->protect_count)
> +               pr_warn("%s: unregistering protected clock: %s\n",
> +                                       __func__, clk->core->name);
> +
>         kref_put(&clk->core->ref, __clk_release);
>  unlock:
>         clk_prepare_unlock();
> @@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
>  
>         clk_prepare_lock();
>  
> +       /* Protect count not balanced: warn and sanitize */
> +       if (clk->protect_ucount) {
> +               pr_warn("%s: releasing protected clock: %s\n",
> +                                       __func__, clk->core->name);
> +
> +               for (; clk->protect_ucount; clk->protect_ucount--)
> +                       clk_core_unprotect(clk->core);
> +       }
> +
>         hlist_del(&clk->clks_node);
>         if (clk->min_rate > clk->core->req_rate ||
>             clk->max_rate < clk->core->req_rate)
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index a428aec36ace..705a158d9b8f 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
>  unsigned long __clk_get_flags(struct clk *clk);
>  unsigned long clk_hw_get_flags(const struct clk_hw *hw);
>  bool clk_hw_is_prepared(const struct clk_hw *hw);
> +bool clk_hw_is_protected(const struct clk_hw *hw);
>  bool clk_hw_is_enabled(const struct clk_hw *hw);
>  bool __clk_is_enabled(struct clk *clk);
>  struct clk *__clk_lookup(const char *name);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index e9d36b3e49de..90b72ead4411 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -265,6 +265,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_protect - inform the system when the clock source should 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.
> + */
> +void clk_protect(struct clk *clk);
> +
> +/**
> + * clk_unprotect - release the protection of the clock source.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer previously protecting the
> + * clock source can now deal with other consumer altering the clock source.
> + *
> + * The caller must balance the number of protect and unprotect calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_unprotect(struct clk *clk);
>  
>  /**
>   * clk_enable - inform the system when the clock source should be running.
> @@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
>  
>  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
>  
> +
> +static inline void clk_protect(struct clk *clk) {}
> +
> +static inline void clk_unprotect(struct clk *clk) {}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>         return 0;
> -- 
> 2.9.3
> 

[RFC 4/7] clk: add support for clock protection

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 11:33:27)
> 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.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

I haven't reviewed this yet, but changes to clk.h should also keep
Russell King in Cc.

Best regards,
Mike

> ---
>  drivers/clk/clk.c            | 177 ++++++++++++++++++++++++++++++++++++++++---
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 +++++++
>  3 files changed, 197 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fa77a1841e0f..69db8cc15063 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -60,6 +60,7 @@ struct clk_core {
>         bool                    orphan;
>         unsigned int            enable_count;
>         unsigned int            prepare_count;
> +       unsigned int            protect_count;
>         unsigned long           min_rate;
>         unsigned long           max_rate;
>         unsigned long           accuracy;
> @@ -84,6 +85,7 @@ struct clk {
>         const char *con_id;
>         unsigned long min_rate;
>         unsigned long max_rate;
> +       unsigned long protect_ucount;
>         struct hlist_node clks_node;
>  };
>  
> @@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
>         return core->ops->is_prepared(core->hw);
>  }
>  
> +static bool clk_core_is_protected(struct clk_core *core)
> +{
> +       return core->protect_count;
> +}
> +
>  static bool clk_core_is_enabled(struct clk_core *core)
>  {
>         /*
> @@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
>         return clk_core_is_prepared(hw->core);
>  }
>  
> +bool clk_hw_is_protected(const struct clk_hw *hw)
> +{
> +       return clk_core_is_protected(hw->core);
> +}
> +
>  bool clk_hw_is_enabled(const struct clk_hw *hw)
>  {
>         return clk_core_is_enabled(hw->core);
> @@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_prepare);
>  
> +static void clk_core_unprotect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (WARN_ON(core->protect_count == 0))
> +               return;
> +
> +       if (--core->protect_count > 0)
> +               return;
> +
> +       clk_core_unprotect(core->parent);
> +}
> +
> +/**
> + * clk_unprotect - unprotect a clock source
> + * @clk: the clk being unprotected
> + *
> + * clk_unprotect shall be used when a consumer no longer depends on the clock
> + * rate and can tolerate glitches. As with clk_unprepare and clk_enable, calls
> + * to clk_unprotect must be balanced with clk_protect.
> + * clk_protect may sleep
> + */
> +void clk_unprotect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +
> +       if (WARN_ON(clk->protect_ucount <= 0)) {
> +               /*
> +                * There is something wrong with this consumer protect count.
> +                * Stop here before messing with the provider
> +                */
> +               clk_prepare_unlock();
> +               return;
> +       }
> +
> +       clk_core_unprotect(clk->core);
> +       clk->protect_ucount--;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_unprotect);
> +
> +static void clk_core_protect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (core->protect_count == 0)
> +               clk_core_protect(core->parent);
> +
> +       core->protect_count++;
> +}
> +
> +/**
> + * clk_protect - protect a clock source
> + * @clk: the clk being protected
> + *
> + * clk_protect can be used when a consumer depends on the clock rate and can't
> + * tolerate any glitches. The consumer protecting the clock can still make
> + * adjustment to clock, if it is the only one protecting the clock. Other
> + * consumers can still use the clock but won't be able to adjust the rate or
> + * reparent the clock while it is protected.
> + * clk_protect may sleep.
> + */
> +void clk_protect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +       clk_core_protect(clk->core);
> +       clk->protect_ucount++;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_protect);
> +
>  static void clk_core_disable(struct clk_core *core)
>  {
>         lockdep_assert_held(&enable_lock);
> @@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core *core,
>  {
>         long rate;
>  
> -       if (core->ops->determine_rate) {
> +       if (clk_core_is_protected(core)) {
> +               req->rate = core->rate;
> +       } else if (core->ops->determine_rate) {
>                 return core->ops->determine_rate(core->hw, req);
>         } else if (core->ops->round_rate) {
>                 rate = core->ops->round_rate(core->hw, req->rate,
> @@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                 req.min_rate = min_rate;
>                 req.max_rate = max_rate;
>  
> -               clk_core_init_rate_req(core, req);
> +               clk_core_init_rate_req(core, &req);
>  
>                 ret = clk_core_determine_round(core, &req);
>                 if (ret < 0)
> @@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>         /* prevent racing with updates to the clock topology */
>         clk_prepare_lock();
>  
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
>         ret = clk_core_set_rate_nolock(clk->core, rate);
>  
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>  
>         clk_prepare_lock();
>  
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
>         if (min != clk->min_rate || 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 (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>         if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
>                 return -EBUSY;
>  
> +       if (clk_core_is_protected(core))
> +               return -EBUSY;
> +
>         /* try finding the new parent index */
>         if (parent) {
>                 p_index = clk_fetch_parent_index(core, parent);
> @@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core *core,
>   */
>  int clk_set_parent(struct clk *clk, struct clk *parent)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_set_parent_lock(clk->core,
> -                                       parent ? parent->core : NULL);
> +       clk_prepare_lock();
> +
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
> +       ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> +
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
> +       clk_prepare_unlock();
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> @@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core, int degrees)
>         if (!core)
>                 return 0;
>  
> -       trace_clk_set_phase(clk->core, degrees);
> +       if (clk_core_is_protected(core))
> +               return -EBUSY;
> +
> +       trace_clk_set_phase(core, degrees);
>  
>         if (core->ops->set_phase)
>                 ret = core->ops->set_phase(core->hw, degrees);
> @@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
>                 degrees += 360;
>  
>         clk_prepare_lock();
> +
> +       if (clk->protect_ucount)
> +               clk_core_unprotect(clk->core);
> +
>         ret = clk_core_set_phase(clk->core, degrees);
> +
> +       if (clk->protect_ucount)
> +               clk_core_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
>         if (!c)
>                 return;
>  
> -       seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
> +       seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
>                    level * 3 + 1, "",
>                    30 - level * 3, c->name,
> -                  c->enable_count, c->prepare_count, clk_core_get_rate(c),
> -                  clk_core_get_accuracy(c), clk_core_get_phase(c));
> +                  c->enable_count, c->prepare_count, c->protect_count,
> +                  clk_core_get_rate(c), clk_core_get_accuracy(c),
> +                  clk_core_get_phase(c));
>  }
>  
>  static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
> @@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
>         struct clk_core *c;
>         struct hlist_head **lists = (struct hlist_head **)s->private;
>  
> -       seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
> -       seq_puts(s, "----------------------------------------------------------------------------------------\n");
> +       seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
> +       seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
>  
>         clk_prepare_lock();
>  
> @@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
>         seq_printf(s, "\"%s\": { ", c->name);
>         seq_printf(s, "\"enable_count\": %d,", c->enable_count);
>         seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
> +       seq_printf(s, "\"protect_count\": %d,", c->protect_count);
>         seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
>         seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
>         seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
> @@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
>         if (!d)
>                 goto err_out;
>  
> +       d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
> +                       (u32 *)&core->protect_count);
> +       if (!d)
> +               goto err_out;
> +
>         d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
>                         (u32 *)&core->notifier_count);
>         if (!d)
> @@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
>         if (clk->core->prepare_count)
>                 pr_warn("%s: unregistering prepared clock: %s\n",
>                                         __func__, clk->core->name);
> +
> +       if (clk->core->protect_count)
> +               pr_warn("%s: unregistering protected clock: %s\n",
> +                                       __func__, clk->core->name);
> +
>         kref_put(&clk->core->ref, __clk_release);
>  unlock:
>         clk_prepare_unlock();
> @@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
>  
>         clk_prepare_lock();
>  
> +       /* Protect count not balanced: warn and sanitize */
> +       if (clk->protect_ucount) {
> +               pr_warn("%s: releasing protected clock: %s\n",
> +                                       __func__, clk->core->name);
> +
> +               for (; clk->protect_ucount; clk->protect_ucount--)
> +                       clk_core_unprotect(clk->core);
> +       }
> +
>         hlist_del(&clk->clks_node);
>         if (clk->min_rate > clk->core->req_rate ||
>             clk->max_rate < clk->core->req_rate)
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index a428aec36ace..705a158d9b8f 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
>  unsigned long __clk_get_flags(struct clk *clk);
>  unsigned long clk_hw_get_flags(const struct clk_hw *hw);
>  bool clk_hw_is_prepared(const struct clk_hw *hw);
> +bool clk_hw_is_protected(const struct clk_hw *hw);
>  bool clk_hw_is_enabled(const struct clk_hw *hw);
>  bool __clk_is_enabled(struct clk *clk);
>  struct clk *__clk_lookup(const char *name);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index e9d36b3e49de..90b72ead4411 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -265,6 +265,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_protect - inform the system when the clock source should 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.
> + */
> +void clk_protect(struct clk *clk);
> +
> +/**
> + * clk_unprotect - release the protection of the clock source.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer previously protecting the
> + * clock source can now deal with other consumer altering the clock source.
> + *
> + * The caller must balance the number of protect and unprotect calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_unprotect(struct clk *clk);
>  
>  /**
>   * clk_enable - inform the system when the clock source should be running.
> @@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
>  
>  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
>  
> +
> +static inline void clk_protect(struct clk *clk) {}
> +
> +static inline void clk_unprotect(struct clk *clk) {}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>         return 0;
> -- 
> 2.9.3
> 

[RFC 1/7] clk: take the prepare lock out of clk_core_set_parent

[Jerome Brunet]

On Sat, 2017-04-15 at 22:09 +0200, Michael Turquette wrote:
> Hi Jerome,
> 
> Quoting Jerome Brunet (2017-03-21 19:33:24)
> > Rework set_parent core function so it can be called when the prepare lock
> > is already held by the caller.
> > 
> > This rework is done to ease the integration of the "protected" clock
> > functionality.
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > ?drivers/clk/clk.c | 38 ++++++++++++++++++++------------------
> > ?1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 67201f67a14a..e77f03a47da6 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1794,23 +1794,16 @@ static int clk_core_set_parent(struct clk_core
> > *core, struct clk_core *parent)
> 
> Nitpick: rename clk_core_set_parent to clk_core_st_parent_nolock. Drop
> the clk_core_set_parent_lock name below and just call it
> clk_core_set_parent.
> 
> This matches all the other nolock patterns in clk.c.

Indeed, I should tidy up the naming regarding the "_lock" and "_no_lock"
This applies to the rest of the patches.

> 
> > ????????if (!core)
> > ????????????????return 0;
> > ?
> > -???????/* prevent racing with updates to the clock topology */
> > -???????clk_prepare_lock();
> > -
> > ????????if (core->parent == parent)
> > -???????????????goto out;
> > +???????????????return 0;
> > ?
> > ????????/* verify ops for for multi-parent clks */
> > -???????if ((core->num_parents > 1) && (!core->ops->set_parent)) {
> > -???????????????ret = -ENOSYS;
> > -???????????????goto out;
> > -???????}
> > +???????if ((core->num_parents > 1) && (!core->ops->set_parent))
> > +???????????????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) {
> > @@ -1818,8 +1811,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;
> > ????????}
> > @@ -1829,7 +1821,7 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> > ?
> > ????????/* abort if a driver objects */
> > ????????if (ret & NOTIFY_STOP_MASK)
> > -???????????????goto out;
> > +???????????????return ret;
> > ?
> > ????????/* do the re-parent */
> > ????????ret = __clk_set_parent(core, parent, p_index);
> > @@ -1842,7 +1834,16 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> > ????????????????__clk_recalc_accuracies(core);
> > ????????}
> > ?
> > -out:
> > +???????return ret;
> 
> I do not understand the benefit of the code churn above where gotos are
> replaced with returns. This should be a few line patch but it is much
> more than that due to this churn.

I understand, the unified diff is far from being easy to read.
Gotos and labels were useful to centralize the call to clk_prepare_unlock()
on exit (including error case). With this patch, the prepare lock is no longer 
managed in this function (but one level up)

All you would have under the "out" label is "return ret;".
In such case, I think we should call directly return and get rid of the gotos.

Do you agree ? 

> 
> > +}
> > +
> > +static int clk_core_set_parent_lock(struct clk_core *core,
> > +???????????????????????????????????struct clk_core *parent)
> > +{
> > +???????int ret;
> > +
> > +???????clk_prepare_lock();
> > +???????ret = clk_core_set_parent(core, parent);
> 
> Above line should be nolock.
> 
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1870,7 +1871,8 @@ int clk_set_parent(struct clk *clk, struct clk
> > *parent)
> > ????????if (!clk)
> > ????????????????return 0;
> > ?
> > -???????return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> > +???????return clk_core_set_parent_lock(clk->core,
> > +???????????????????????????????????????parent ? parent->core : NULL);
> 
> We can avoid this line by using the nolock pattern.
> 
> Basic premise of the patch is OK though.
> 
> Regards,
> Mike
> 
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > ?
> > @@ -2720,7 +2722,7 @@ void clk_unregister(struct clk *clk)
> > ????????????????/* Reparent all children to the orphan list. */
> > ????????????????hlist_for_each_entry_safe(child, t, &clk->core->children,
> > ??????????????????????????????????????????child_node)
> > -???????????????????????clk_core_set_parent(child, NULL);
> > +???????????????????????clk_core_set_parent_lock(child, NULL);
> > ????????}
> > ?
> > ????????hlist_del_init(&clk->core->child_node);
> > --?
> > 2.9.3
> > 

[RFC 4/7] clk: add support for clock protection

[Jerome Brunet]

On Thu, 2017-05-11 at 12:05 -0700, Michael Turquette wrote:
> Hi Jerome,
> 
> Quoting Jerome Brunet (2017-03-21 11:33:27)
> > 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.
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > ?drivers/clk/clk.c????????????| 177
> > ++++++++++++++++++++++++++++++++++++++++---
> > ?include/linux/clk-provider.h |???1 +
> > ?include/linux/clk.h??????????|??29 +++++++
> > ?3 files changed, 197 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index fa77a1841e0f..69db8cc15063 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -60,6 +60,7 @@ struct clk_core {
> > ????????bool????????????????????orphan;
> > ????????unsigned int????????????enable_count;
> > ????????unsigned int????????????prepare_count;
> > +???????unsigned int????????????protect_count;
> > ????????unsigned long???????????min_rate;
> > ????????unsigned long???????????max_rate;
> > ????????unsigned long???????????accuracy;
> > @@ -84,6 +85,7 @@ struct clk {
> > ????????const char *con_id;
> > ????????unsigned long min_rate;
> > ????????unsigned long max_rate;
> > +???????unsigned long protect_ucount;
> > ????????struct hlist_node clks_node;
> > ?};
> > ?
> > @@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
> > ????????return core->ops->is_prepared(core->hw);
> > ?}
> > ?
> > +static bool clk_core_is_protected(struct clk_core *core)
> > +{
> > +???????return core->protect_count;
> > +}
> > +
> > ?static bool clk_core_is_enabled(struct clk_core *core)
> > ?{
> > ????????/*
> > @@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
> > ????????return clk_core_is_prepared(hw->core);
> > ?}
> > ?
> > +bool clk_hw_is_protected(const struct clk_hw *hw)
> > +{
> > +???????return clk_core_is_protected(hw->core);
> > +}
> > +
> > ?bool clk_hw_is_enabled(const struct clk_hw *hw)
> > ?{
> > ????????return clk_core_is_enabled(hw->core);
> > @@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_prepare);
> > ?
> > +static void clk_core_unprotect(struct clk_core *core)
> > +{
> > +???????lockdep_assert_held(&prepare_lock);
> > +
> > +???????if (!core)
> > +???????????????return;
> > +
> > +???????if (WARN_ON(core->protect_count == 0))
> > +???????????????return;
> > +
> > +???????if (--core->protect_count > 0)
> > +???????????????return;
> > +
> > +???????clk_core_unprotect(core->parent);
> > +}
> > +
> > +/**
> > + * clk_unprotect - unprotect a clock source
> > + * @clk: the clk being unprotected
> > + *
> > + * clk_unprotect shall be used when a consumer no longer depends on the
> > clock
> > + * rate and can tolerate glitches. As with clk_unprepare and clk_enable,
> > calls
> > + * to clk_unprotect must be balanced with clk_protect.
> > + * clk_protect may sleep
> 
> Maybe:
> 
> """
> 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.
> """

Like it ! Thx !

> 
> > + */
> > +void clk_unprotect(struct clk *clk)
> > +{
> > +???????if (!clk)
> > +???????????????return;
> > +
> > +???????clk_prepare_lock();
> > +
> > +???????if (WARN_ON(clk->protect_ucount <= 0)) {
> > +???????????????/*
> > +????????????????* There is something wrong with this consumer protect
> > count.
> > +????????????????* Stop here before messing with the provider
> > +????????????????*/
> > +???????????????clk_prepare_unlock();
> > +???????????????return;
> > +???????}
> > +
> > +???????clk_core_unprotect(clk->core);
> > +???????clk->protect_ucount--;
> > +???????clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_unprotect);
> > +
> > +static void clk_core_protect(struct clk_core *core)
> > +{
> > +???????lockdep_assert_held(&prepare_lock);
> > +
> > +???????if (!core)
> > +???????????????return;
> > +
> > +???????if (core->protect_count == 0)
> > +???????????????clk_core_protect(core->parent);
> > +
> > +???????core->protect_count++;
> > +}
> > +
> > +/**
> > + * clk_protect - protect a clock source
> > + * @clk: the clk being protected
> > + *
> > + * clk_protect can be used when a consumer depends on the clock rate and
> > can't
> > + * tolerate any glitches. The consumer protecting the clock can still make
> > + * adjustment to clock, if it is the only one protecting the clock. Other
> > + * consumers can still use the clock but won't be able to adjust the rate
> > or
> > + * reparent the clock while it is protected.
> > + * clk_protect may sleep.
> > + */
> 
> Maybe:
> 
> """
> clk_protect begins a critical section during which the clock consumer
> cannot tolerate any change to the clock rate. This results in all clocks
> up the parent chain to also be rate-protected.
> 
> 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_protect should be balanced with calls to clk_unprotect.
> Calls to this function may sleep, and do not return error status.
> """

+1

> 
> > +void clk_protect(struct clk *clk)
> > +{
> > +???????if (!clk)
> > +???????????????return;
> > +
> > +???????clk_prepare_lock();
> > +???????clk_core_protect(clk->core);
> > +???????clk->protect_ucount++;
> > +???????clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_protect);
> > +
> > ?static void clk_core_disable(struct clk_core *core)
> > ?{
> > ????????lockdep_assert_held(&enable_lock);
> > @@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core
> > *core,
> > ?{
> > ????????long rate;
> > ?
> > -???????if (core->ops->determine_rate) {
> > +???????if (clk_core_is_protected(core)) {
> > +???????????????req->rate = core->rate;
> 
> Hmm, in CCF we basically re-use round_rate/determine_rate from within
> calls to clk_set_rate. The point is that clk_set_rate should set the
> same rate that is returned from either of the other two functions.
> 
> The change above breaks that subtle assumption, as a clk consumer can
> now call:
> 
> 	clk_protect(clk);
> 
> 	rate = clk_get_rate(clk);		// rate is 1234;
> 
> 	rate = clk_round_rate(clk, 5678);	// rate is STILL 1234
> 
> 	ret = clk_set_rate(clk, 5678);
> 
> 	rate = clk_get_rate(clk);		// rate is 5678
> 
> Is my understanding correct? If so then we might consider adding some
> more complex knowledge to clk_round_rate. Something like:
> 
> 	if clk_is_protected(clk) AND it is only protect by THIS clk:
> 		round the rate
> 	else:
> 		return core->rate

I may be wrong but I think it is already the way you want it. Please see below
[0]

> 
> > +???????} else if (core->ops->determine_rate) {
> > ????????????????return core->ops->determine_rate(core->hw, req);
> > ????????} else if (core->ops->round_rate) {
> > ????????????????rate = core->ops->round_rate(core->hw, req->rate,
> > @@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct
> > clk_core *core,
> > ????????????????req.min_rate = min_rate;
> > ????????????????req.max_rate = max_rate;
> > ?
> > -???????????????clk_core_init_rate_req(core, req);
> > +???????????????clk_core_init_rate_req(core, &req);
> 
> Why isn't this change in patch #3? Seems like a bug introduced in patch
> #3 and fixed here in patch #4.

Indeed, I messed up while formatting the patches :(

> 
> > ?
> > ????????????????ret = clk_core_determine_round(core, &req);
> > ????????????????if (ret < 0)
> > @@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> > ????????/* prevent racing with updates to the clock topology */
> > ????????clk_prepare_lock();
> 
> The fact that a user can protect a clk AND change its rate is very
> subtle. Please update the kerneldoc for clk_set_rate, clk_set_parent and
> any other affected apis.

Will do

> 
> > ?
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_unprotect(clk->core);
> 
> What happens here if two different users have both protected this same
> clk? Looks like we ignore the second user?
> 
> In other words, what happens if clk->protect_ucount == 2?

We don't really care what is the value of the consumer count, what is important
is whether it is null or not. This is linked to the point [0] above. Maybe it is
not clear but here is how it (should) goes :

Every time clock_rate_protect is called the consumer *and* core count is
incremented. (consumer_count =< core_count)

When an altering action is tried (set_rate, set_parent, ...), if the consumer
count is not null, we call clk_core_unprotect which mean that the core_count is
temporarily decremented by 1. It is safe to do so because we are holding the
prepare_lock.

When we get to 
+???????if (clk_core_is_protected(core)) {
+???????????????req->rate = core->rate;

Either the clock was only protected by the calling consumer, *and only once*,
then the protection has been temporarily removed, test is false and the usual
code will be executed, calling round/determine rate.

If the test is true, it means the clock is still protected. This happens if 
* the clock is protected and we come from an unprotected consumer
* the clock is protected by more than one consumer
* the clock is protected more than one time by the same consumer - when there is
multiple code path protecting the rate in a device driver, as previously
discussed. That's why clk_core_unprotect decrements by 1 and not by the
consumer_count.

In such case, the clock is still protected, I think it make sense to return what
is already set and not round/determine again. The clock was already
rounded/determined when it wasn't protected.

It this what you meant by :
> if clk_is_protected(clk) AND it is only protect by THIS clk:
?

Cheers
Jerome

> 
> 
> Best regards,
> Mike
> 
> > +
> > ????????ret = clk_core_set_rate_nolock(clk->core, rate);
> > ?
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
> > long min, unsigned long max)
> > ?
> > ????????clk_prepare_lock();
> > ?
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_unprotect(clk->core);
> > +
> > ????????if (min != clk->min_rate || 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 (clk->protect_ucount)
> > +???????????????clk_core_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> > ????????if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
> > ????????????????return -EBUSY;
> > ?
> > +???????if (clk_core_is_protected(core))
> > +???????????????return -EBUSY;
> > +
> > ????????/* try finding the new parent index */
> > ????????if (parent) {
> > ????????????????p_index = clk_fetch_parent_index(core, parent);
> > @@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core
> > *core,
> > ? */
> > ?int clk_set_parent(struct clk *clk, struct clk *parent)
> > ?{
> > +???????int ret;
> > +
> > ????????if (!clk)
> > ????????????????return 0;
> > ?
> > -???????return clk_core_set_parent_lock(clk->core,
> > -???????????????????????????????????????parent ? parent->core : NULL);
> > +???????clk_prepare_lock();
> > +
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_unprotect(clk->core);
> > +
> > +???????ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> > +
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_protect(clk->core);
> > +
> > +???????clk_prepare_unlock();
> > +
> > +???????return ret;
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > ?
> > @@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core,
> > int degrees)
> > ????????if (!core)
> > ????????????????return 0;
> > ?
> > -???????trace_clk_set_phase(clk->core, degrees);
> > +???????if (clk_core_is_protected(core))
> > +???????????????return -EBUSY;
> > +
> > +???????trace_clk_set_phase(core, degrees);
> > ?
> > ????????if (core->ops->set_phase)
> > ????????????????ret = core->ops->set_phase(core->hw, degrees);
> > @@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
> > ????????????????degrees += 360;
> > ?
> > ????????clk_prepare_lock();
> > +
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_unprotect(clk->core);
> > +
> > ????????ret = clk_core_set_phase(clk->core, degrees);
> > +
> > +???????if (clk->protect_ucount)
> > +???????????????clk_core_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s,
> > struct clk_core *c,
> > ????????if (!c)
> > ????????????????return;
> > ?
> > -???????seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
> > +???????seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
> > ???????????????????level * 3 + 1, "",
> > ???????????????????30 - level * 3, c->name,
> > -??????????????????c->enable_count, c->prepare_count, clk_core_get_rate(c),
> > -??????????????????clk_core_get_accuracy(c), clk_core_get_phase(c));
> > +??????????????????c->enable_count, c->prepare_count, c->protect_count,
> > +??????????????????clk_core_get_rate(c), clk_core_get_accuracy(c),
> > +??????????????????clk_core_get_phase(c));
> > ?}
> > ?
> > ?static void clk_summary_show_subtree(struct seq_file *s, struct clk_core
> > *c,
> > @@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void
> > *data)
> > ????????struct clk_core *c;
> > ????????struct hlist_head **lists = (struct hlist_head **)s->private;
> > ?
> > -???????seq_puts(s,
> > "???clock?????????????????????????enable_cnt??prepare_cnt????????rate???accu
> > racy???phase\n");
> > -???????seq_puts(s, "-------------------------------------------------------
> > ---------------------------------\n");
> > +???????seq_puts(s,
> > "???clock?????????????????????????enable_cnt??prepare_cnt??protect_cnt??????
> > ??rate???accuracy???phase\n");
> > +???????seq_puts(s, "-------------------------------------------------------
> > ---------------------------------------------\n");
> > ?
> > ????????clk_prepare_lock();
> > ?
> > @@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct
> > clk_core *c, int level)
> > ????????seq_printf(s, "\"%s\": { ", c->name);
> > ????????seq_printf(s, "\"enable_count\": %d,", c->enable_count);
> > ????????seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
> > +???????seq_printf(s, "\"protect_count\": %d,", c->protect_count);
> > ????????seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
> > ????????seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
> > ????????seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
> > @@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core
> > *core, struct dentry *pdentry)
> > ????????if (!d)
> > ????????????????goto err_out;
> > ?
> > +???????d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
> > +???????????????????????(u32 *)&core->protect_count);
> > +???????if (!d)
> > +???????????????goto err_out;
> > +
> > ????????d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
> > ????????????????????????(u32 *)&core->notifier_count);
> > ????????if (!d)
> > @@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
> > ????????if (clk->core->prepare_count)
> > ????????????????pr_warn("%s: unregistering prepared clock: %s\n",
> > ????????????????????????????????????????__func__, clk->core->name);
> > +
> > +???????if (clk->core->protect_count)
> > +???????????????pr_warn("%s: unregistering protected clock: %s\n",
> > +???????????????????????????????????????__func__, clk->core->name);
> > +
> > ????????kref_put(&clk->core->ref, __clk_release);
> > ?unlock:
> > ????????clk_prepare_unlock();
> > @@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
> > ?
> > ????????clk_prepare_lock();
> > ?
> > +???????/* Protect count not balanced: warn and sanitize */
> > +???????if (clk->protect_ucount) {
> > +???????????????pr_warn("%s: releasing protected clock: %s\n",
> > +???????????????????????????????????????__func__, clk->core->name);
> > +
> > +???????????????for (; clk->protect_ucount; clk->protect_ucount--)
> > +???????????????????????clk_core_unprotect(clk->core);
> > +???????}
> > +
> > ????????hlist_del(&clk->clks_node);
> > ????????if (clk->min_rate > clk->core->req_rate ||
> > ????????????clk->max_rate < clk->core->req_rate)
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index a428aec36ace..705a158d9b8f 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
> > ?unsigned long __clk_get_flags(struct clk *clk);
> > ?unsigned long clk_hw_get_flags(const struct clk_hw *hw);
> > ?bool clk_hw_is_prepared(const struct clk_hw *hw);
> > +bool clk_hw_is_protected(const struct clk_hw *hw);
> > ?bool clk_hw_is_enabled(const struct clk_hw *hw);
> > ?bool __clk_is_enabled(struct clk *clk);
> > ?struct clk *__clk_lookup(const char *name);
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index e9d36b3e49de..90b72ead4411 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -265,6 +265,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_protect - inform the system when the clock source should 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.
> > + */
> > +void clk_protect(struct clk *clk);
> > +
> > +/**
> > + * clk_unprotect - release the protection of the clock source.
> > + * @clk: clock source
> > + *
> > + * This function informs the system that the consumer previously protecting
> > the
> > + * clock source can now deal with other consumer altering the clock source.
> > + *
> > + * The caller must balance the number of protect and unprotect calls.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_unprotect(struct clk *clk);
> > ?
> > ?/**
> > ? * clk_enable - inform the system when the clock source should be running.
> > @@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
> > ?
> > ?static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
> > ?
> > +
> > +static inline void clk_protect(struct clk *clk) {}
> > +
> > +static inline void clk_unprotect(struct clk *clk) {}
> > +
> > ?static inline int clk_enable(struct clk *clk)
> > ?{
> > ????????return 0;
> > --?
> > 2.9.3
> > 

[RFC 0/7] clk: implement clock locking mechanism

[Jerome Brunet]

On Sat, 2017-04-15 at 21:50 +0200, Michael Turquette wrote:
> Hi Jerome,
> 
> Thanks for sending this series. The concept is one we've been talking
> about for years, and your approach makes sense. Some comments below.
> 
> Quoting Jerome Brunet (2017-03-21 19:33:23)
> > This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
> > clk_lock which available here [0]
> 
> If we merge this solution then we may want to convert some of those
> CLK_SET_RATE_GATE users and 

It would be nice, but this would be on a case by case basis and would require
the help of the platform maintainers. I think there two kind of users of
CLK_SET_RATE_GATE at this moment:

1) The clock must be gated - disabled - to change the rate safely:
clk_protect_rate won't help here, it does not enforce such thing. We should
provide another fix for this because CLK_SET_RATE_GATE does really enforce this
constraint along the clock tree either

2) The one (like me) trying to abuse the enable_count to get some protection:
This never worked, and these drivers had no protection until now. If they really
need protection they can start using clk_protect_rate.

What I mean with this two point is: even if the intent is the same, there is
real functional difference between CLK_SET_RATE_GATE and clk_protect_rate. We
will have to be careful ...

> potentially some of the rate-range users
> that set min/max to the same value over to this new api.
> 

This case is easier, if they use min=max, yes for sure.

> > 
> > 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.
> > 
> > Patches 1 to 3 are just rework to ease the integration of the locking
> > mechanism. They should not introduce any functional differences.
> > 
> > Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
> > clk_protect and clk_unprotect (The "lock" word is already used lot in
> > clk.c. Using clk_lock and clk_unlock would have been very messy. If you
> > found "protect" to be a poor choice, I'm happy to sed the whole thing in
> > future version)
> > 
> > These calls can happen at anytime during the life of the clk. As for
> > prepare and enable, the calls must be balanced.
> > 
> > Inside the clock framework, 2 new count values are introduced to keep track
> > of the protection:
> > * core "protect_count": this reference count value works in the same way as
> > ? prepare and enable count, and track whether the clock is protected or
> > ? not. This is the value that will checked to allow operation which may
> > ? cause glitches or change the rate of clock source.
> > * clk consumer "protect_ucount": this allows to track if the consumer is
> > ? protecting the clock.
> > 
> > Requiring the consumer to unprotect its clock before changing it would have
> > been very annoying for the consumer. It would also be unsafe, as it would
> > still be possible for another consumer to change the rate between the call
> > to set_rate and protect. This needs to happen in a critical section.??A
> > solution would be to add "set_rate_protect", but we would need to do the
> > same for the set_parent and set_phase (and the possible future
> > functions). It does not scale well.??The solution proposed in this patch is
> > to use the consumer protect count.
> > 
> > In function altering the clock, if the consumer is protecting the clock,
> > the protection is temporarily release under the prepare_lock. This ensure
> > that:
> > * the clock is still protect from another consumer because of the
> > ? prepare_lock
> > * the rate set on a protected clock cannot change between set_rate and
> > ? later enable
> > * only a consumer protecting a clock can do this temporary protection
> > ? removal (less chance of people messing with the refcount)
> > * if more than one consumer protect the clock, it remains protected.
> > ? Because the protection is removed for the calling consumer, it gives
> > ? it a chance to jump to another provider, if available.
> > 
> > This protection mechanism assume that "protecting" consumer knows what it
> > is doing when it comes to setting the rate, and does not expect to be
> > protected against itself.
> > 
> > This was tested with the audio use case mentioned in [0]
> > 
> > One caveat is the following case:
> > N+1 consumers protect their clocks. These clocks come from N possible
> > providers. We should able to satisfy at least N consumers before exhausting
> > the resources.??In the particular case where all the consumers call
> > "protect" before having a chance to call "set_rate", the last 2 consumers
> > will remains stuck on the last provider w/o being able to set the rate on
> > it. This means that in situation where there is 2 consumers on 1
> > providers, they will compete for the clock, and may end up in situation
> > where both protect the clock and none can set the rate they need.??This can
> > be solved if, when the rate fails to be set, the consumer release the
> > protection. Then the second consumer will be able to satisfy its request.
> 
> This situation can be handled a bit more gracefully if clk_set_rate_lock
> both returns an error code if setting the rate failes AND it releases
> the rate lock in that case. At least that helps for the case of
> initializing rates during .probe(). Automatically dropping the lock
> might complicate things in other cases though...

set_rate_lock would solve the problem for?
> > "the last 2 consumers
> > will remains stuck on the last provider w/o being able to set the rate"

With set_rate_lock, only the last one won't be satisfied, which is fine I
suppose.

> ?if setting the rate failes

Setting aside this RFC, when can we consider a that setting the rate failed ?
CCF always give the best it can, according to a set of constraints (possible
parents, range of the parents, ...) but does not return an error.

Clock protect is just one more constraint added to the equation, right ?

set_rate having failed depends on the accuracy you need.
For exemple You ask for 100Mhz out of :
* a PLL: you get 98 MHz
* a very slow clock: you get 10Hz

Which one has failed ?

Thanks for taking time to review this RFC Mike !
Cheers
Jerome

> 
> Best regards,
> Mike
> 
> > 
> > Patch 5 is a small change on set_rate_range to restore the old range on
> > failure. It don't use set_rate_range in any driver so I could not test this
> > change.
> > 
> > Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
> > less wide after adding protect count in it.
> > 
> > Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
> > incorrectly.
> > 
> > Bonus:
> > 
> > With this patchset, we should probably change the use the flags
> > "CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
> > them. Another solution would be to have them automatically gate the clock
> > before performing the related operation.??What is your view on this ?
> > 
> > [0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resona
> > nce
> > 
> > Jerome Brunet (7):
> > ? clk: take the prepare lock out of clk_core_set_parent
> > ? clk: add set_phase core function
> > ? clk: rework calls to round and determine rate callbacks
> > ? clk: add support for clock protection
> > ? clk: rollback set_rate_range changes on failure
> > ? clk: cosmetic changes to clk_summary debugfs entry
> > ? clk: fix incorrect usage of ENOSYS
> > 
> > ?drivers/clk/clk.c????????????| 313 ++++++++++++++++++++++++++++++++++----
> > -----
> > ?include/linux/clk-provider.h |???1 +
> > ?include/linux/clk.h??????????|??29 ++++
> > ?3 files changed, 279 insertions(+), 64 deletions(-)
> > 
> > --?
> > 2.9.3
> > 

[RFC 4/7] clk: add support for clock protection

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-05-12 06:08:02)
> On Thu, 2017-05-11 at 12:05 -0700, Michael Turquette wrote:
> > Quoting Jerome Brunet (2017-03-21 11:33:27)
<snip>
> > > @@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core
> > > *core,
> > > ?{
> > > ????????long rate;
> > > ?
> > > -???????if (core->ops->determine_rate) {
> > > +???????if (clk_core_is_protected(core)) {
> > > +???????????????req->rate = core->rate;
> > 
> > Hmm, in CCF we basically re-use round_rate/determine_rate from within
> > calls to clk_set_rate. The point is that clk_set_rate should set the
> > same rate that is returned from either of the other two functions.
> > 
> > The change above breaks that subtle assumption, as a clk consumer can
> > now call:
> > 
> >       clk_protect(clk);
> > 
> >       rate = clk_get_rate(clk);               // rate is 1234;
> > 
> >       rate = clk_round_rate(clk, 5678);       // rate is STILL 1234
> > 
> >       ret = clk_set_rate(clk, 5678);
> > 
> >       rate = clk_get_rate(clk);               // rate is 5678
> > 
> > Is my understanding correct? If so then we might consider adding some
> > more complex knowledge to clk_round_rate. Something like:
> > 
> >       if clk_is_protected(clk) AND it is only protect by THIS clk:
> >               round the rate
> >       else:
> >               return core->rate
> 
> I may be wrong but I think it is already the way you want it. Please see below
> [0]
<snip>
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_unprotect(clk->core);
> > 
> > What happens here if two different users have both protected this same
> > clk? Looks like we ignore the second user?
> > 
> > In other words, what happens if clk->protect_ucount == 2?
> 
> We don't really care what is the value of the consumer count, what is important
> is whether it is null or not. This is linked to the point [0] above. Maybe it is
> not clear but here is how it (should) goes :
> 
> Every time clock_rate_protect is called the consumer *and* core count is
> incremented. (consumer_count =< core_count)
> 
> When an altering action is tried (set_rate, set_parent, ...), if the consumer
> count is not null, we call clk_core_unprotect which mean that the core_count is
> temporarily decremented by 1. It is safe to do so because we are holding the
> prepare_lock.
> 
> When we get to 
> +???????if (clk_core_is_protected(core)) {
> +???????????????req->rate = core->rate;
> 
> Either the clock was only protected by the calling consumer, *and only once*,
> then the protection has been temporarily removed, test is false and the usual
> code will be executed, calling round/determine rate.
> 
> If the test is true, it means the clock is still protected. This happens if 
> * the clock is protected and we come from an unprotected consumer
> * the clock is protected by more than one consumer
> * the clock is protected more than one time by the same consumer - when there is
> multiple code path protecting the rate in a device driver, as previously
> discussed. That's why clk_core_unprotect decrements by 1 and not by the
> consumer_count.
> 
> In such case, the clock is still protected, I think it make sense to return what
> is already set and not round/determine again. The clock was already
> rounded/determined when it wasn't protected.
> 
> It this what you meant by :
> > if clk_is_protected(clk) AND it is only protect by THIS clk:
> ?

Yes, you've answered my question and I'm happy with that. When reviewing
the code before I missed the subtle point that we test for
clk->protect_ucount but then only modify core->protect_count. I think a
one-line comment above those conditional statements would be nice.

I do have a doubt about how clk_round_rate works now. Is it correct that
we do not ever decrement the protect count when a protecting consumer
calls clk_round_rate?

It seems to me that a clk consumer should be able to call clk_protect()
and then clk_round_rate() and get a real rounded rate instead req->rate
= core->rate, assuming that the core protect count is decremented to
zero.

In other words, can you make clk_round_rate work like clk_set_rate and
clk_set_parent?

Regards,
Mike

> 
> Cheers
> Jerome
> 
> > 
> > 
> > Best regards,
> > Mike
> > 
> > > +
> > > ????????ret = clk_core_set_rate_nolock(clk->core, rate);
> > > ?
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_protect(clk->core);
> > > +
> > > ????????clk_prepare_unlock();
> > > ?
> > > ????????return ret;
> > > @@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
> > > long min, unsigned long max)
> > > ?
> > > ????????clk_prepare_lock();
> > > ?
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_unprotect(clk->core);
> > > +
> > > ????????if (min != clk->min_rate || 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 (clk->protect_ucount)
> > > +???????????????clk_core_protect(clk->core);
> > > +
> > > ????????clk_prepare_unlock();
> > > ?
> > > ????????return ret;
> > > @@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core,
> > > struct clk_core *parent)
> > > ????????if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
> > > ????????????????return -EBUSY;
> > > ?
> > > +???????if (clk_core_is_protected(core))
> > > +???????????????return -EBUSY;
> > > +
> > > ????????/* try finding the new parent index */
> > > ????????if (parent) {
> > > ????????????????p_index = clk_fetch_parent_index(core, parent);
> > > @@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core
> > > *core,
> > > ? */
> > > ?int clk_set_parent(struct clk *clk, struct clk *parent)
> > > ?{
> > > +???????int ret;
> > > +
> > > ????????if (!clk)
> > > ????????????????return 0;
> > > ?
> > > -???????return clk_core_set_parent_lock(clk->core,
> > > -???????????????????????????????????????parent ? parent->core : NULL);
> > > +???????clk_prepare_lock();
> > > +
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_unprotect(clk->core);
> > > +
> > > +???????ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> > > +
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_protect(clk->core);
> > > +
> > > +???????clk_prepare_unlock();
> > > +
> > > +???????return ret;
> > > ?}
> > > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > > ?
> > > @@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core,
> > > int degrees)
> > > ????????if (!core)
> > > ????????????????return 0;
> > > ?
> > > -???????trace_clk_set_phase(clk->core, degrees);
> > > +???????if (clk_core_is_protected(core))
> > > +???????????????return -EBUSY;
> > > +
> > > +???????trace_clk_set_phase(core, degrees);
> > > ?
> > > ????????if (core->ops->set_phase)
> > > ????????????????ret = core->ops->set_phase(core->hw, degrees);
> > > @@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
> > > ????????????????degrees += 360;
> > > ?
> > > ????????clk_prepare_lock();
> > > +
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_unprotect(clk->core);
> > > +
> > > ????????ret = clk_core_set_phase(clk->core, degrees);
> > > +
> > > +???????if (clk->protect_ucount)
> > > +???????????????clk_core_protect(clk->core);
> > > +
> > > ????????clk_prepare_unlock();
> > > ?
> > > ????????return ret;
> > > @@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s,
> > > struct clk_core *c,
> > > ????????if (!c)
> > > ????????????????return;
> > > ?
> > > -???????seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
> > > +???????seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
> > > ???????????????????level * 3 + 1, "",
> > > ???????????????????30 - level * 3, c->name,
> > > -??????????????????c->enable_count, c->prepare_count, clk_core_get_rate(c),
> > > -??????????????????clk_core_get_accuracy(c), clk_core_get_phase(c));
> > > +??????????????????c->enable_count, c->prepare_count, c->protect_count,
> > > +??????????????????clk_core_get_rate(c), clk_core_get_accuracy(c),
> > > +??????????????????clk_core_get_phase(c));
> > > ?}
> > > ?
> > > ?static void clk_summary_show_subtree(struct seq_file *s, struct clk_core
> > > *c,
> > > @@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void
> > > *data)
> > > ????????struct clk_core *c;
> > > ????????struct hlist_head **lists = (struct hlist_head **)s->private;
> > > ?
> > > -???????seq_puts(s,
> > > "???clock?????????????????????????enable_cnt??prepare_cnt????????rate???accu
> > > racy???phase\n");
> > > -???????seq_puts(s, "-------------------------------------------------------
> > > ---------------------------------\n");
> > > +???????seq_puts(s,
> > > "???clock?????????????????????????enable_cnt??prepare_cnt??protect_cnt??????
> > > ??rate???accuracy???phase\n");
> > > +???????seq_puts(s, "-------------------------------------------------------
> > > ---------------------------------------------\n");
> > > ?
> > > ????????clk_prepare_lock();
> > > ?
> > > @@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct
> > > clk_core *c, int level)
> > > ????????seq_printf(s, "\"%s\": { ", c->name);
> > > ????????seq_printf(s, "\"enable_count\": %d,", c->enable_count);
> > > ????????seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
> > > +???????seq_printf(s, "\"protect_count\": %d,", c->protect_count);
> > > ????????seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
> > > ????????seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
> > > ????????seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
> > > @@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core
> > > *core, struct dentry *pdentry)
> > > ????????if (!d)
> > > ????????????????goto err_out;
> > > ?
> > > +???????d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
> > > +???????????????????????(u32 *)&core->protect_count);
> > > +???????if (!d)
> > > +???????????????goto err_out;
> > > +
> > > ????????d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
> > > ????????????????????????(u32 *)&core->notifier_count);
> > > ????????if (!d)
> > > @@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
> > > ????????if (clk->core->prepare_count)
> > > ????????????????pr_warn("%s: unregistering prepared clock: %s\n",
> > > ????????????????????????????????????????__func__, clk->core->name);
> > > +
> > > +???????if (clk->core->protect_count)
> > > +???????????????pr_warn("%s: unregistering protected clock: %s\n",
> > > +???????????????????????????????????????__func__, clk->core->name);
> > > +
> > > ????????kref_put(&clk->core->ref, __clk_release);
> > > ?unlock:
> > > ????????clk_prepare_unlock();
> > > @@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
> > > ?
> > > ????????clk_prepare_lock();
> > > ?
> > > +???????/* Protect count not balanced: warn and sanitize */
> > > +???????if (clk->protect_ucount) {
> > > +???????????????pr_warn("%s: releasing protected clock: %s\n",
> > > +???????????????????????????????????????__func__, clk->core->name);
> > > +
> > > +???????????????for (; clk->protect_ucount; clk->protect_ucount--)
> > > +???????????????????????clk_core_unprotect(clk->core);
> > > +???????}
> > > +
> > > ????????hlist_del(&clk->clks_node);
> > > ????????if (clk->min_rate > clk->core->req_rate ||
> > > ????????????clk->max_rate < clk->core->req_rate)
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index a428aec36ace..705a158d9b8f 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
> > > ?unsigned long __clk_get_flags(struct clk *clk);
> > > ?unsigned long clk_hw_get_flags(const struct clk_hw *hw);
> > > ?bool clk_hw_is_prepared(const struct clk_hw *hw);
> > > +bool clk_hw_is_protected(const struct clk_hw *hw);
> > > ?bool clk_hw_is_enabled(const struct clk_hw *hw);
> > > ?bool __clk_is_enabled(struct clk *clk);
> > > ?struct clk *__clk_lookup(const char *name);
> > > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > > index e9d36b3e49de..90b72ead4411 100644
> > > --- a/include/linux/clk.h
> > > +++ b/include/linux/clk.h
> > > @@ -265,6 +265,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_protect - inform the system when the clock source should 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.
> > > + */
> > > +void clk_protect(struct clk *clk);
> > > +
> > > +/**
> > > + * clk_unprotect - release the protection of the clock source.
> > > + * @clk: clock source
> > > + *
> > > + * This function informs the system that the consumer previously protecting
> > > the
> > > + * clock source can now deal with other consumer altering the clock source.
> > > + *
> > > + * The caller must balance the number of protect and unprotect calls.
> > > + *
> > > + * Must not be called from within atomic context.
> > > + */
> > > +void clk_unprotect(struct clk *clk);
> > > ?
> > > ?/**
> > > ? * clk_enable - inform the system when the clock source should be running.
> > > @@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
> > > ?
> > > ?static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
> > > ?
> > > +
> > > +static inline void clk_protect(struct clk *clk) {}
> > > +
> > > +static inline void clk_unprotect(struct clk *clk) {}
> > > +
> > > ?static inline int clk_enable(struct clk *clk)
> > > ?{
> > > ????????return 0;
> > > --?
> > > 2.9.3
> > > 

[RFC 0/7] clk: implement clock locking mechanism

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-05-12 07:04:59)
> On Sat, 2017-04-15 at 21:50 +0200, Michael Turquette wrote:
> > Hi Jerome,
> > 
> > Thanks for sending this series. The concept is one we've been talking
> > about for years, and your approach makes sense. Some comments below.
> > 
> > Quoting Jerome Brunet (2017-03-21 19:33:23)
> > > This RFC patchset is related to the discussion around CLK_SET_RATE_GATE and
> > > clk_lock which available here [0]
> > 
> > If we merge this solution then we may want to convert some of those
> > CLK_SET_RATE_GATE users and 
> 
> It would be nice, but this would be on a case by case basis and would require
> the help of the platform maintainers. I think there two kind of users of
> CLK_SET_RATE_GATE at this moment:
> 
> 1) The clock must be gated - disabled - to change the rate safely:
> clk_protect_rate won't help here, it does not enforce such thing. We should
> provide another fix for this because CLK_SET_RATE_GATE does really enforce this
> constraint along the clock tree either
> 
> 2) The one (like me) trying to abuse the enable_count to get some protection:
> This never worked, and these drivers had no protection until now. If they really
> need protection they can start using clk_protect_rate.
> 
> What I mean with this two point is: even if the intent is the same, there is
> real functional difference between CLK_SET_RATE_GATE and clk_protect_rate. We
> will have to be careful ...

Agreed. I was only referring to the #2 category above, which I suspect
is a significant portion of them.

> 
> > potentially some of the rate-range users
> > that set min/max to the same value over to this new api.
> > 
> 
> This case is easier, if they use min=max, yes for sure.
> 
> > > 
> > > 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.
> > > 
> > > Patches 1 to 3 are just rework to ease the integration of the locking
> > > mechanism. They should not introduce any functional differences.
> > > 
> > > Patch 4 is the important bit. It introduce 2 new functions to the CCF API:
> > > clk_protect and clk_unprotect (The "lock" word is already used lot in
> > > clk.c. Using clk_lock and clk_unlock would have been very messy. If you
> > > found "protect" to be a poor choice, I'm happy to sed the whole thing in
> > > future version)
> > > 
> > > These calls can happen at anytime during the life of the clk. As for
> > > prepare and enable, the calls must be balanced.
> > > 
> > > Inside the clock framework, 2 new count values are introduced to keep track
> > > of the protection:
> > > * core "protect_count": this reference count value works in the same way as
> > > ? prepare and enable count, and track whether the clock is protected or
> > > ? not. This is the value that will checked to allow operation which may
> > > ? cause glitches or change the rate of clock source.
> > > * clk consumer "protect_ucount": this allows to track if the consumer is
> > > ? protecting the clock.
> > > 
> > > Requiring the consumer to unprotect its clock before changing it would have
> > > been very annoying for the consumer. It would also be unsafe, as it would
> > > still be possible for another consumer to change the rate between the call
> > > to set_rate and protect. This needs to happen in a critical section.??A
> > > solution would be to add "set_rate_protect", but we would need to do the
> > > same for the set_parent and set_phase (and the possible future
> > > functions). It does not scale well.??The solution proposed in this patch is
> > > to use the consumer protect count.
> > > 
> > > In function altering the clock, if the consumer is protecting the clock,
> > > the protection is temporarily release under the prepare_lock. This ensure
> > > that:
> > > * the clock is still protect from another consumer because of the
> > > ? prepare_lock
> > > * the rate set on a protected clock cannot change between set_rate and
> > > ? later enable
> > > * only a consumer protecting a clock can do this temporary protection
> > > ? removal (less chance of people messing with the refcount)
> > > * if more than one consumer protect the clock, it remains protected.
> > > ? Because the protection is removed for the calling consumer, it gives
> > > ? it a chance to jump to another provider, if available.
> > > 
> > > This protection mechanism assume that "protecting" consumer knows what it
> > > is doing when it comes to setting the rate, and does not expect to be
> > > protected against itself.
> > > 
> > > This was tested with the audio use case mentioned in [0]
> > > 
> > > One caveat is the following case:
> > > N+1 consumers protect their clocks. These clocks come from N possible
> > > providers. We should able to satisfy at least N consumers before exhausting
> > > the resources.??In the particular case where all the consumers call
> > > "protect" before having a chance to call "set_rate", the last 2 consumers
> > > will remains stuck on the last provider w/o being able to set the rate on
> > > it. This means that in situation where there is 2 consumers on 1
> > > providers, they will compete for the clock, and may end up in situation
> > > where both protect the clock and none can set the rate they need.??This can
> > > be solved if, when the rate fails to be set, the consumer release the
> > > protection. Then the second consumer will be able to satisfy its request.
> > 
> > This situation can be handled a bit more gracefully if clk_set_rate_lock
> > both returns an error code if setting the rate failes AND it releases
> > the rate lock in that case. At least that helps for the case of
> > initializing rates during .probe(). Automatically dropping the lock
> > might complicate things in other cases though...
> 
> set_rate_lock would solve the problem for?
> > > "the last 2 consumers
> > > will remains stuck on the last provider w/o being able to set the rate"
> 
> With set_rate_lock, only the last one won't be satisfied, which is fine I
> suppose.
> 
> > ?if setting the rate failes
> 
> Setting aside this RFC, when can we consider a that setting the rate failed ?
> CCF always give the best it can, according to a set of constraints (possible
> parents, range of the parents, ...) but does not return an error.

It should return errors if the .set_rate operation fails. That is a
long-standing bug.

I agree that rate selection via .determine_rate is not really a
pass/fail sort of thing.

> 
> Clock protect is just one more constraint added to the equation, right ?
> 
> set_rate having failed depends on the accuracy you need.
> For exemple You ask for 100Mhz out of :
> * a PLL: you get 98 MHz
> * a very slow clock: you get 10Hz
> 
> Which one has failed ?

Again, picking the rate is not the problem. I'm more concerned with PLLs
that will not lock and a timeout is hit in the .set_rate callback from
the PLL clock provider.

Regards,
Mike

> 
> Thanks for taking time to review this RFC Mike !
> Cheers
> Jerome
> 
> > 
> > Best regards,
> > Mike
> > 
> > > 
> > > Patch 5 is a small change on set_rate_range to restore the old range on
> > > failure. It don't use set_rate_range in any driver so I could not test this
> > > change.
> > > 
> > > Patch 6 is just a cosmetic change to clk_summary debugfs entry to make it
> > > less wide after adding protect count in it.
> > > 
> > > Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is used
> > > incorrectly.
> > > 
> > > Bonus:
> > > 
> > > With this patchset, we should probably change the use the flags
> > > "CLK_SET_RATE_GATE" and "CLK_SET_PARENT_GATE" We discussed removing
> > > them. Another solution would be to have them automatically gate the clock
> > > before performing the related operation.??What is your view on this ?
> > > 
> > > [0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resona
> > > nce
> > > 
> > > Jerome Brunet (7):
> > > ? clk: take the prepare lock out of clk_core_set_parent
> > > ? clk: add set_phase core function
> > > ? clk: rework calls to round and determine rate callbacks
> > > ? clk: add support for clock protection
> > > ? clk: rollback set_rate_range changes on failure
> > > ? clk: cosmetic changes to clk_summary debugfs entry
> > > ? clk: fix incorrect usage of ENOSYS
> > > 
> > > ?drivers/clk/clk.c????????????| 313 ++++++++++++++++++++++++++++++++++----
> > > -----
> > > ?include/linux/clk-provider.h |???1 +
> > > ?include/linux/clk.h??????????|??29 ++++
> > > ?3 files changed, 279 insertions(+), 64 deletions(-)
> > > 
> > > --?
> > > 2.9.3
> > >