[PATCH v2 00/11] clk: implement clock rate protection mechanism

[PATCH v2 00/11] clk: implement clock rate protection mechanism

[Jerome Brunet]

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

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

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

The last 2 patches provide the same functionnality for providers themself
by fixing CLK_SET_RATE_GATE (enforce clock gating along the tree)

qcom, at91 and ux500 are the heaviest users of this flag. If anybody having
one these platform could try this series, I would help build confidence
that they are relying on CLK_SET_RATE_GATE being broken.

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

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

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

[0]: http://lkml.kernel.org/r/20170321183330.26722-1-jbrunet@baylibre.com
[1]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029@resonance

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

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

-- 
2.9.4

[PATCH v2 00/11] clk: implement clock rate protection mechanism

[Jerome Brunet]

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

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

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

The last 2 patches provide the same functionnality for providers themself
by fixing CLK_SET_RATE_GATE (enforce clock gating along the tree)

qcom, at91 and ux500 are the heaviest users of this flag. If anybody having
one these platform could try this series, I would help build confidence
that they are relying on CLK_SET_RATE_GATE being broken.

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

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

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

[0]: http://lkml.kernel.org/r/20170321183330.26722-1-jbrunet at baylibre.com
[1]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029 at resonance

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

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

-- 
2.9.4

[PATCH v2 01/11] 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 | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fc58c52a26b4..f5c371532509 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_has_parent);
 
-static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
+static int clk_core_set_parent_nolock(struct clk_core *core,
+				      struct clk_core *parent)
 {
 	int ret = 0;
 	int p_index = 0;
@@ -1796,23 +1797,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) {
@@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		if (p_index < 0) {
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
-			ret = p_index;
-			goto out;
+			return p_index;
 		}
 		p_rate = parent->rate;
 	}
@@ -1831,7 +1824,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);
@@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		__clk_recalc_accuracies(core);
 	}
 
-out:
-	clk_prepare_unlock();
-
 	return ret;
 }
 
@@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
  */
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
+	int ret;
+
 	if (!clk)
 		return 0;
 
-	return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+	clk_prepare_lock();
+	ret = clk_core_set_parent_nolock(clk->core,
+					 parent ? parent->core : NULL);
+	clk_prepare_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
@@ -2753,7 +2750,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_nolock(child, NULL);
 	}
 
 	hlist_del_init(&clk->core->child_node);
-- 
2.9.4

[PATCH v2 01/11] 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 | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fc58c52a26b4..f5c371532509 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_has_parent);
 
-static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
+static int clk_core_set_parent_nolock(struct clk_core *core,
+				      struct clk_core *parent)
 {
 	int ret = 0;
 	int p_index = 0;
@@ -1796,23 +1797,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) {
@@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		if (p_index < 0) {
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
-			ret = p_index;
-			goto out;
+			return p_index;
 		}
 		p_rate = parent->rate;
 	}
@@ -1831,7 +1824,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);
@@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		__clk_recalc_accuracies(core);
 	}
 
-out:
-	clk_prepare_unlock();
-
 	return ret;
 }
 
@@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
  */
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
+	int ret;
+
 	if (!clk)
 		return 0;
 
-	return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+	clk_prepare_lock();
+	ret = clk_core_set_parent_nolock(clk->core,
+					 parent ? parent->core : NULL);
+	clk_prepare_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
@@ -2753,7 +2750,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_nolock(child, NULL);
 	}
 
 	hlist_del_init(&clk->core->child_node);
-- 
2.9.4

[PATCH v2 02/11] clk: add clk_core_set_phase_nolock 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 f5c371532509..6031fada37f9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
+static int clk_core_set_phase_nolock(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
@@ -1895,7 +1912,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;
@@ -1906,17 +1923,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_nolock(clk->core, degrees);
 	clk_prepare_unlock();
 
 	return ret;
-- 
2.9.4

[PATCH v2 02/11] clk: add clk_core_set_phase_nolock 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 f5c371532509..6031fada37f9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
+static int clk_core_set_phase_nolock(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
@@ -1895,7 +1912,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;
@@ -1906,17 +1923,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_nolock(clk->core, degrees);
 	clk_prepare_unlock();
 
 	return ret;
-- 
2.9.4

[PATCH v2 03/11] 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 6031fada37f9..100f72472e10 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;
 }
 
@@ -1356,36 +1376,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.4

[PATCH v2 03/11] 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 6031fada37f9..100f72472e10 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;
 }
 
@@ -1356,36 +1376,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.4

[PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Jerome Brunet]

The current implementation of clk_core_set_rate_nolock bails out early if
the requested rate is exactly the same as the one set. It should bail out
if the request would not result in rate a change.  This important when rate
is not exactly what is requested, which is fairly common with PLLs.

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

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

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 100f72472e10..1a8c0d013238 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
 		clk_change_rate(core->new_child);
 }
 
+static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
+						     unsigned long req_rate)
+{
+	int ret;
+	struct clk_rate_request req;
+
+	if (!core)
+		return 0;
+
+	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
+	req.rate = req_rate;
+
+	ret = clk_core_round_rate_nolock(core, &req);
+
+	return ret ? 0 : req.rate;
+}
+
 static int clk_core_set_rate_nolock(struct clk_core *core,
 				    unsigned long req_rate)
 {
 	struct clk_core *top, *fail_clk;
-	unsigned long rate = req_rate;
+	unsigned long rate;
 
 	if (!core)
 		return 0;
 
+	rate = clk_core_req_round_rate_nolock(core, req_rate);
+
 	/* bail early if nothing to do */
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
@@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EBUSY;
 
 	/* calculate new rates and get the topmost changed clock */
-	top = clk_calc_new_rates(core, rate);
+	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
 		return -EINVAL;
 
-- 
2.9.4

[PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Jerome Brunet]

The current implementation of clk_core_set_rate_nolock bails out early if
the requested rate is exactly the same as the one set. It should bail out
if the request would not result in rate a change.  This important when rate
is not exactly what is requested, which is fairly common with PLLs.

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

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

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 100f72472e10..1a8c0d013238 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
 		clk_change_rate(core->new_child);
 }
 
+static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
+						     unsigned long req_rate)
+{
+	int ret;
+	struct clk_rate_request req;
+
+	if (!core)
+		return 0;
+
+	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
+	req.rate = req_rate;
+
+	ret = clk_core_round_rate_nolock(core, &req);
+
+	return ret ? 0 : req.rate;
+}
+
 static int clk_core_set_rate_nolock(struct clk_core *core,
 				    unsigned long req_rate)
 {
 	struct clk_core *top, *fail_clk;
-	unsigned long rate = req_rate;
+	unsigned long rate;
 
 	if (!core)
 		return 0;
 
+	rate = clk_core_req_round_rate_nolock(core, req_rate);
+
 	/* bail early if nothing to do */
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
@@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EBUSY;
 
 	/* calculate new rates and get the topmost changed clock */
-	top = clk_calc_new_rates(core, rate);
+	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
 		return -EINVAL;
 
-- 
2.9.4

[PATCH v2 05/11] 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            | 207 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 ++++++
 3 files changed, 230 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1a8c0d013238..a0524e3bfaca 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_count;
 	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_rate_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_rate_is_protected(const struct clk_hw *hw)
+{
+	return clk_core_rate_is_protected(hw->core);
+}
+
 bool clk_hw_is_enabled(const struct clk_hw *hw)
 {
 	return clk_core_is_enabled(hw->core);
@@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+static void clk_core_rate_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_rate_unprotect(core->parent);
+}
+
+/**
+ * clk_rate_unprotect - unprotect the rate of a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect completes a critical section during which the clock
+ * consumer cannot tolerate any change to the clock rate. If no other clock
+ * consumers have protected clocks in the parent chain, then calls to this
+ * function will allow the clocks in the parent chain to change rates
+ * freely.
+ *
+ * Unlike the clk_set_rate_range method, which allows the rate to change
+ * within a given range, protected clocks cannot have their rate changed,
+ * either directly or indirectly due to changes further up the parent chain
+ * of clocks.
+ *
+ * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
+ * to this function may sleep, and do not return error status.
+ */
+void clk_rate_unprotect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+
+	/*
+	 * if there is something wrong with this consumer protect count, stop
+	 * here before messing with the provider
+	 */
+	if (WARN_ON(clk->protect_count <= 0))
+		goto out;
+
+	clk_core_rate_unprotect(clk->core);
+	clk->protect_count--;
+out:
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_unprotect);
+
+static void clk_core_rate_protect(struct clk_core *core)
+{
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	if (core->protect_count == 0)
+		clk_core_rate_protect(core->parent);
+
+	core->protect_count++;
+}
+
+/**
+ * clk_rate_protect - protect a clock source
+ * @clk: the clk being protected
+ *
+ * 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_rate_protect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+	clk_core_rate_protect(clk->core);
+	clk->protect_count++;
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_protect);
+
 static void clk_core_disable(struct clk_core *core)
 {
 	lockdep_assert_held(&enable_lock);
@@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core *core,
 {
 	long rate;
 
-	if (core->ops->determine_rate) {
+	/*
+	 * At this point, core protection will be disabled if
+	 * - if the provider is not protected at all
+	 * - if the calling consumer is the only one protecting the
+	 *   provider (and only once)
+	 */
+	if (clk_core_rate_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,
@@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
 	clk_prepare_lock();
 
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	if (ret)
@@ -1575,15 +1698,24 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 {
 	int ret;
 	struct clk_rate_request req;
+	unsigned int cnt = core->protect_count;
 
 	if (!core)
 		return 0;
 
+	/* simulate what the rate would be if it could be freely set */
+	while (core->protect_count)
+		clk_core_rate_unprotect(core);
+
 	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
 	req.rate = req_rate;
 
 	ret = clk_core_round_rate_nolock(core, &req);
 
+	/* restore the protection */
+	while (core->protect_count < cnt)
+		clk_core_rate_protect(core);
+
 	return ret ? 0 : req.rate;
 }
 
@@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
 
+	/* fail on a direct rate set of a protected provider */
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
 	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
 		return -EBUSY;
 
@@ -1658,8 +1794,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_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 
 	clk_prepare_lock();
 
+	if (clk->protect_count)
+		clk_core_rate_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_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
 		return -EBUSY;
 
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
 	/* try finding the new parent index */
 	if (parent) {
 		p_index = clk_fetch_parent_index(core, parent);
@@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 		return 0;
 
 	clk_prepare_lock();
+
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_parent_nolock(clk->core,
 					 parent ? parent->core : NULL);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
 	if (!core)
 		return 0;
 
-	trace_clk_set_phase(clk->core, degrees);
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
+	trace_clk_set_phase(core, degrees);
 
 	if (core->ops->set_phase)
 		ret = core->ops->set_phase(core->hw, degrees);
@@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
 		degrees += 360;
 
 	clk_prepare_lock();
+
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_phase_nolock(clk->core, degrees);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -2039,11 +2209,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,
@@ -2065,8 +2236,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();
 
@@ -2101,6 +2272,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));
@@ -2231,6 +2403,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)
@@ -2794,6 +2971,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();
@@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_lock();
 
+	/*
+	 * Before calling clk_put, all calls to clk_rate_protect from a given
+	 * user must be balanced with calls to clk_rate_unprotect and by that
+	 * same user
+	 */
+	WARN_ON(clk->protect_count);
+
+	/* We voiced our concern, let's sanitize the situation */
+	for (; clk->protect_count; clk->protect_count--)
+		clk_core_rate_unprotect(clk->core);
+
 	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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 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_rate_protect - inform the system when the clock rate must be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_rate_protect(struct clk *clk);
+
+/**
+ * clk_rate_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting the
+ * clock rate can now deal with other consumer altering the clock source rate
+ *
+ * The caller must balance the number of rate_protect and rate_unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_rate_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.4

[PATCH v2 05/11] 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            | 207 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 ++++++
 3 files changed, 230 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1a8c0d013238..a0524e3bfaca 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_count;
 	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_rate_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_rate_is_protected(const struct clk_hw *hw)
+{
+	return clk_core_rate_is_protected(hw->core);
+}
+
 bool clk_hw_is_enabled(const struct clk_hw *hw)
 {
 	return clk_core_is_enabled(hw->core);
@@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+static void clk_core_rate_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_rate_unprotect(core->parent);
+}
+
+/**
+ * clk_rate_unprotect - unprotect the rate of a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect completes a critical section during which the clock
+ * consumer cannot tolerate any change to the clock rate. If no other clock
+ * consumers have protected clocks in the parent chain, then calls to this
+ * function will allow the clocks in the parent chain to change rates
+ * freely.
+ *
+ * Unlike the clk_set_rate_range method, which allows the rate to change
+ * within a given range, protected clocks cannot have their rate changed,
+ * either directly or indirectly due to changes further up the parent chain
+ * of clocks.
+ *
+ * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
+ * to this function may sleep, and do not return error status.
+ */
+void clk_rate_unprotect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+
+	/*
+	 * if there is something wrong with this consumer protect count, stop
+	 * here before messing with the provider
+	 */
+	if (WARN_ON(clk->protect_count <= 0))
+		goto out;
+
+	clk_core_rate_unprotect(clk->core);
+	clk->protect_count--;
+out:
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_unprotect);
+
+static void clk_core_rate_protect(struct clk_core *core)
+{
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	if (core->protect_count == 0)
+		clk_core_rate_protect(core->parent);
+
+	core->protect_count++;
+}
+
+/**
+ * clk_rate_protect - protect a clock source
+ * @clk: the clk being protected
+ *
+ * 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_rate_protect(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	clk_prepare_lock();
+	clk_core_rate_protect(clk->core);
+	clk->protect_count++;
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_protect);
+
 static void clk_core_disable(struct clk_core *core)
 {
 	lockdep_assert_held(&enable_lock);
@@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core *core,
 {
 	long rate;
 
-	if (core->ops->determine_rate) {
+	/*
+	 * At this point, core protection will be disabled if
+	 * - if the provider is not protected at all
+	 * - if the calling consumer is the only one protecting the
+	 *   provider (and only once)
+	 */
+	if (clk_core_rate_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,
@@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
 	clk_prepare_lock();
 
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	if (ret)
@@ -1575,15 +1698,24 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 {
 	int ret;
 	struct clk_rate_request req;
+	unsigned int cnt = core->protect_count;
 
 	if (!core)
 		return 0;
 
+	/* simulate what the rate would be if it could be freely set */
+	while (core->protect_count)
+		clk_core_rate_unprotect(core);
+
 	clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
 	req.rate = req_rate;
 
 	ret = clk_core_round_rate_nolock(core, &req);
 
+	/* restore the protection */
+	while (core->protect_count < cnt)
+		clk_core_rate_protect(core);
+
 	return ret ? 0 : req.rate;
 }
 
@@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
 
+	/* fail on a direct rate set of a protected provider */
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
 	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
 		return -EBUSY;
 
@@ -1658,8 +1794,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_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 
 	clk_prepare_lock();
 
+	if (clk->protect_count)
+		clk_core_rate_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_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
 		return -EBUSY;
 
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
 	/* try finding the new parent index */
 	if (parent) {
 		p_index = clk_fetch_parent_index(core, parent);
@@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 		return 0;
 
 	clk_prepare_lock();
+
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_parent_nolock(clk->core,
 					 parent ? parent->core : NULL);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
 	if (!core)
 		return 0;
 
-	trace_clk_set_phase(clk->core, degrees);
+	if (clk_core_rate_is_protected(core))
+		return -EBUSY;
+
+	trace_clk_set_phase(core, degrees);
 
 	if (core->ops->set_phase)
 		ret = core->ops->set_phase(core->hw, degrees);
@@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
 		degrees += 360;
 
 	clk_prepare_lock();
+
+	if (clk->protect_count)
+		clk_core_rate_unprotect(clk->core);
+
 	ret = clk_core_set_phase_nolock(clk->core, degrees);
+
+	if (clk->protect_count)
+		clk_core_rate_protect(clk->core);
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -2039,11 +2209,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,
@@ -2065,8 +2236,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();
 
@@ -2101,6 +2272,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));
@@ -2231,6 +2403,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)
@@ -2794,6 +2971,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();
@@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_lock();
 
+	/*
+	 * Before calling clk_put, all calls to clk_rate_protect from a given
+	 * user must be balanced with calls to clk_rate_unprotect and by that
+	 * same user
+	 */
+	WARN_ON(clk->protect_count);
+
+	/* We voiced our concern, let's sanitize the situation */
+	for (; clk->protect_count; clk->protect_count--)
+		clk_core_rate_unprotect(clk->core);
+
 	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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 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_rate_protect - inform the system when the clock rate must be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_rate_protect(struct clk *clk);
+
+/**
+ * clk_rate_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting the
+ * clock rate can now deal with other consumer altering the clock source rate
+ *
+ * The caller must balance the number of rate_protect and rate_unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_rate_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.4

[PATCH v2 06/11] clk: add clk_set_rate_protect

[Jerome Brunet]

clk_set_rate_protect is a combination of clk_set_rate and clk_rate_protect
within a critical section. In case where several protecting consumers
compete to set the rate of the same provider, it provides a way to make
sure that at least one of them will be satisfied before the resource is
locked.

This is to avoid the unlikely situation where several consumers protect a
clock provider and none actually get a rate it can work with.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h | 14 ++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a0524e3bfaca..33ec990b2e97 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1809,6 +1809,51 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 EXPORT_SYMBOL_GPL(clk_set_rate);
 
 /**
+ * clk_set_rate_protect - specify a new rate and protect it
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * This is a combination of clk_set_rate and clk_rate_protect within
+ * a critical section
+ *
+ * This can be used initially to ensure that at least 1 consumers is
+ * statisfied when several protecting consummers are competing for the
+ * same clock provider.
+ *
+ * The protection is not applied if setting the rate failed.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate_protect(struct clk *clk, unsigned long rate)
+{
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	/* prevent racing with updates to the clock topology */
+	clk_prepare_lock();
+
+	/*
+	 * The temporary protection removal is not here, on purpose
+	 * This function is meant to be used in instead of clk_rate_protect,
+	 * so before the consumer code path protect the clock provider
+	 */
+
+	ret = clk_core_set_rate_nolock(clk->core, rate);
+
+	if (!ret) {
+		clk_core_rate_protect(clk->core);
+		clk->protect_count++;
+	}
+
+	clk_prepare_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate_protect);
+
+/**
  * clk_set_rate_range - set a rate range for a clock source
  * @clk: clock source
  * @min: desired minimum clock rate in Hz, inclusive
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 85d73e02df40..d3c299d23ae7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -388,6 +388,15 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
 int clk_set_rate(struct clk *clk, unsigned long rate);
 
 /**
+ * clk_set_rate_protect - set and protect the clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_rate_protect(struct clk *clk, unsigned long rate);
+
+/**
  * clk_has_parent - check if a clock is a possible parent for another
  * @clk: clock source
  * @parent: parent clock source
@@ -506,6 +515,11 @@ static inline int clk_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static inline int clk_set_rate_protect(struct clk *clk, unsigned long rate)
+{
+	return 0;
+}
+
 static inline long clk_round_rate(struct clk *clk, unsigned long rate)
 {
 	return 0;
-- 
2.9.4

[PATCH v2 06/11] clk: add clk_set_rate_protect

[Jerome Brunet]

clk_set_rate_protect is a combination of clk_set_rate and clk_rate_protect
within a critical section. In case where several protecting consumers
compete to set the rate of the same provider, it provides a way to make
sure that at least one of them will be satisfied before the resource is
locked.

This is to avoid the unlikely situation where several consumers protect a
clock provider and none actually get a rate it can work with.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h | 14 ++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a0524e3bfaca..33ec990b2e97 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1809,6 +1809,51 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 EXPORT_SYMBOL_GPL(clk_set_rate);
 
 /**
+ * clk_set_rate_protect - specify a new rate and protect it
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * This is a combination of clk_set_rate and clk_rate_protect within
+ * a critical section
+ *
+ * This can be used initially to ensure that at least 1 consumers is
+ * statisfied when several protecting consummers are competing for the
+ * same clock provider.
+ *
+ * The protection is not applied if setting the rate failed.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate_protect(struct clk *clk, unsigned long rate)
+{
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	/* prevent racing with updates to the clock topology */
+	clk_prepare_lock();
+
+	/*
+	 * The temporary protection removal is not here, on purpose
+	 * This function is meant to be used in instead of clk_rate_protect,
+	 * so before the consumer code path protect the clock provider
+	 */
+
+	ret = clk_core_set_rate_nolock(clk->core, rate);
+
+	if (!ret) {
+		clk_core_rate_protect(clk->core);
+		clk->protect_count++;
+	}
+
+	clk_prepare_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate_protect);
+
+/**
  * clk_set_rate_range - set a rate range for a clock source
  * @clk: clock source
  * @min: desired minimum clock rate in Hz, inclusive
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 85d73e02df40..d3c299d23ae7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -388,6 +388,15 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
 int clk_set_rate(struct clk *clk, unsigned long rate);
 
 /**
+ * clk_set_rate_protect - set and protect the clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_rate_protect(struct clk *clk, unsigned long rate);
+
+/**
  * clk_has_parent - check if a clock is a possible parent for another
  * @clk: clock source
  * @parent: parent clock source
@@ -506,6 +515,11 @@ static inline int clk_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static inline int clk_set_rate_protect(struct clk *clk, unsigned long rate)
+{
+	return 0;
+}
+
 static inline long clk_round_rate(struct clk *clk, unsigned long rate)
 {
 	return 0;
-- 
2.9.4

[PATCH v2 07/11] 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 33ec990b2e97..71b0480a152b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1864,6 +1864,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate_protect);
 int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 {
 	int ret = 0;
+	unsigned int old_min, old_max;
 
 	if (!clk)
 		return 0;
@@ -1881,9 +1882,16 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		clk_core_rate_unprotect(clk->core);
 
 	if (min != clk->min_rate || max != clk->max_rate) {
+		old_min = clk->min_rate;
+		old_max = clk->max_rate;
 		clk->min_rate = min;
 		clk->max_rate = max;
 		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+		if (ret) {
+			/* undo changes */
+			clk->min_rate = old_min;
+			clk->max_rate = old_max;
+		}
 	}
 
 	if (clk->protect_count)
-- 
2.9.4

[PATCH v2 07/11] 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 33ec990b2e97..71b0480a152b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1864,6 +1864,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate_protect);
 int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 {
 	int ret = 0;
+	unsigned int old_min, old_max;
 
 	if (!clk)
 		return 0;
@@ -1881,9 +1882,16 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		clk_core_rate_unprotect(clk->core);
 
 	if (min != clk->min_rate || max != clk->max_rate) {
+		old_min = clk->min_rate;
+		old_max = clk->max_rate;
 		clk->min_rate = min;
 		clk->max_rate = max;
 		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+		if (ret) {
+			/* undo changes */
+			clk->min_rate = old_min;
+			clk->max_rate = old_max;
+		}
 	}
 
 	if (clk->protect_count)
-- 
2.9.4

[PATCH v2 08/11] 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 71b0480a152b..955dae044f11 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2262,7 +2262,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,
@@ -2289,8 +2289,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.4

[PATCH v2 08/11] 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 71b0480a152b..955dae044f11 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2262,7 +2262,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,
@@ -2289,8 +2289,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.4

[PATCH v2 09/11] 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 955dae044f11..01306191133c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2032,7 +2032,7 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 
 	/* 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.4

[PATCH v2 09/11] 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 955dae044f11..01306191133c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2032,7 +2032,7 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 
 	/* 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.4

[PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
clock tree

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 01306191133c..6ee5fc59cf1f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_unprotect(core);
+
 	if (--core->prepare_count > 0)
 		return;
 
@@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
 
 	core->prepare_count++;
 
+	/*
+	 * CLK_SET_RATE_GATE is a special case of clock protection
+	 * Instead of a consumer protection, the provider is protecting
+	 * itself when prepared
+	 */
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_protect(core);
+
 	return 0;
 }
 
@@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;
 
-	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
-		return -EBUSY;
-
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
-- 
2.9.4

[PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
clock tree

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 01306191133c..6ee5fc59cf1f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_unprotect(core);
+
 	if (--core->prepare_count > 0)
 		return;
 
@@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
 
 	core->prepare_count++;
 
+	/*
+	 * CLK_SET_RATE_GATE is a special case of clock protection
+	 * Instead of a consumer protection, the provider is protecting
+	 * itself when prepared
+	 */
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_protect(core);
+
 	return 0;
 }
 
@@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;
 
-	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
-		return -EBUSY;
-
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
-- 
2.9.4

[PATCH v2 11/11] clk: move CLK_SET_RATE_GATE protection from prepare to enable

[Jerome Brunet]

CLK_SET_RATE_GATE name suggest that the rate can be set when the provider
is gated (disabled). With the current implementation, the rate can only be
set when the provider is unprepared, while it should be allowed to set a
prepared and disable provider.
Fix this by moving the rate protection mechanism in the enable/disable
core functions

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 6ee5fc59cf1f..e6e5048ce186 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -491,9 +491,6 @@ static void clk_core_unprepare(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
-	if (core->flags & CLK_SET_RATE_GATE)
-		clk_core_rate_unprotect(core);
-
 	if (--core->prepare_count > 0)
 		return;
 
@@ -564,14 +561,6 @@ static int clk_core_prepare(struct clk_core *core)
 
 	core->prepare_count++;
 
-	/*
-	 * CLK_SET_RATE_GATE is a special case of clock protection
-	 * Instead of a consumer protection, the provider is protecting
-	 * itself when prepared
-	 */
-	if (core->flags & CLK_SET_RATE_GATE)
-		clk_core_rate_protect(core);
-
 	return 0;
 }
 
@@ -716,6 +705,9 @@ static void clk_core_disable(struct clk_core *core)
 	if (WARN_ON(core->enable_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_unprotect(core);
+
 	if (--core->enable_count > 0)
 		return;
 
@@ -791,6 +783,15 @@ static int clk_core_enable(struct clk_core *core)
 	}
 
 	core->enable_count++;
+
+	/*
+	 * CLK_SET_RATE_GATE is a special case of clock protection
+	 * Instead of a consumer protection, the provider is protecting
+	 * itself when enabled
+	 */
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_protect(core);
+
 	return 0;
 }
 
-- 
2.9.4

[PATCH v2 11/11] clk: move CLK_SET_RATE_GATE protection from prepare to enable

[Jerome Brunet]

CLK_SET_RATE_GATE name suggest that the rate can be set when the provider
is gated (disabled). With the current implementation, the rate can only be
set when the provider is unprepared, while it should be allowed to set a
prepared and disable provider.
Fix this by moving the rate protection mechanism in the enable/disable
core functions

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 6ee5fc59cf1f..e6e5048ce186 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -491,9 +491,6 @@ static void clk_core_unprepare(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
-	if (core->flags & CLK_SET_RATE_GATE)
-		clk_core_rate_unprotect(core);
-
 	if (--core->prepare_count > 0)
 		return;
 
@@ -564,14 +561,6 @@ static int clk_core_prepare(struct clk_core *core)
 
 	core->prepare_count++;
 
-	/*
-	 * CLK_SET_RATE_GATE is a special case of clock protection
-	 * Instead of a consumer protection, the provider is protecting
-	 * itself when prepared
-	 */
-	if (core->flags & CLK_SET_RATE_GATE)
-		clk_core_rate_protect(core);
-
 	return 0;
 }
 
@@ -716,6 +705,9 @@ static void clk_core_disable(struct clk_core *core)
 	if (WARN_ON(core->enable_count == 1 && core->flags & CLK_IS_CRITICAL))
 		return;
 
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_unprotect(core);
+
 	if (--core->enable_count > 0)
 		return;
 
@@ -791,6 +783,15 @@ static int clk_core_enable(struct clk_core *core)
 	}
 
 	core->enable_count++;
+
+	/*
+	 * CLK_SET_RATE_GATE is a special case of clock protection
+	 * Instead of a consumer protection, the provider is protecting
+	 * itself when enabled
+	 */
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_protect(core);
+
 	return 0;
 }
 
-- 
2.9.4

Re: [PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Adriana Reus]

On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 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 f5c371532509..6031fada37f9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>
> +static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
> +{
> +       int ret = -EINVAL;
> +
> +       if (!core)
> +               return 0;
> +
> +       trace_clk_set_phase(clk->core, degrees);
 ^ trace_clk_set_phase(core, degrees)
> +
> +       if (core->ops->set_phase)
> +               ret = core->ops->set_phase(core->hw, degrees);
> +
> +       trace_clk_set_phase_complete(core, degrees);
> +
> +       return ret;
> +}
> +
>  /**
>   * clk_set_phase - adjust the phase shift of a clock signal
>   * @clk: clock signal source
> @@ -1895,7 +1912,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;
> @@ -1906,17 +1923,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_nolock(clk->core, degrees);
>         clk_prepare_unlock();
>
>         return ret;
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Adriana Reus]

On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 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 f5c371532509..6031fada37f9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>
> +static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
> +{
> +       int ret = -EINVAL;
> +
> +       if (!core)
> +               return 0;
> +
> +       trace_clk_set_phase(clk->core, degrees);
 ^ trace_clk_set_phase(core, degrees)
> +
> +       if (core->ops->set_phase)
> +               ret = core->ops->set_phase(core->hw, degrees);
> +
> +       trace_clk_set_phase_complete(core, degrees);
> +
> +       return ret;
> +}
> +
>  /**
>   * clk_set_phase - adjust the phase shift of a clock signal
>   * @clk: clock signal source
> @@ -1895,7 +1912,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;
> @@ -1906,17 +1923,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_nolock(clk->core, degrees);
>         clk_prepare_unlock();
>
>         return ret;
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Jerome Brunet]

On Tue, 2017-05-23 at 12:35 +0300, Adriana Reus wrote:
> On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > 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 f5c371532509..6031fada37f9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk
> > *parent)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_set_parent);
> > 
> > +static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
> > +{
> > +       int ret = -EINVAL;
> > +
> > +       if (!core)
> > +               return 0;
> > +
> > +       trace_clk_set_phase(clk->core, degrees);
> 
>  ^ trace_clk_set_phase(core, degrees)

Shame ... Once again this is a poor use of 'git add --patch'.
This particular diff ended up in patch 5.

Thanks a lot for catching it!

> > +
> > +       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
> > @@ -1895,7 +1912,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;
> > @@ -1906,17 +1923,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_nolock(clk->core, degrees);
> >         clk_prepare_unlock();
> > 
> >         return ret;
> > --
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Jerome Brunet]

On Tue, 2017-05-23 at 12:35 +0300, Adriana Reus wrote:
> On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > 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 f5c371532509..6031fada37f9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk
> > *parent)
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > 
> > +static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
> > +{
> > +???????int ret = -EINVAL;
> > +
> > +???????if (!core)
> > +???????????????return 0;
> > +
> > +???????trace_clk_set_phase(clk->core, degrees);
> 
> ?^ trace_clk_set_phase(core, degrees)

Shame ... Once again this is a poor use of 'git add --patch'.
This particular diff ended up in patch 5.

Thanks a lot for catching it!

> > +
> > +???????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
> > @@ -1895,7 +1912,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;
> > @@ -1906,17 +1923,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_nolock(clk->core, degrees);
> > ????????clk_prepare_unlock();
> > 
> > ????????return ret;
> > --
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at??http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Adriana Reus]

On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> clock tree
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 01306191133c..6ee5fc59cf1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
>         if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
>                 return;
>
> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_unprotect(core);
^ function call before declaration (unless i missed something when applying),
  gets fixed in following patch, but makes this one not compile standalone.

drivers/clk/clk.c: In function 'clk_core_unprepare':
drivers/clk/clk.c:495:3: error: implicit declaration of function
'clk_core_rate_unprotect' [-Werror=implicit-function-declaration]
   clk_core_rate_unprotect(core);

> +
>         if (--core->prepare_count > 0)
>                 return;
>
> @@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
>
>         core->prepare_count++;
>
> +       /*
> +        * CLK_SET_RATE_GATE is a special case of clock protection
> +        * Instead of a consumer protection, the provider is protecting
> +        * itself when prepared
> +        */
> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_protect(core);
^ same here;
Note: maybe have a quick check that each patch compiles individually
(if you haven't already), I did not check them all.
> +
>         return 0;
>  }
>
> @@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>         if (clk_core_rate_is_protected(core))
>                 return -EBUSY;
>
> -       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -               return -EBUSY;
> -
>         /* calculate new rates and get the topmost changed clock */
>         top = clk_calc_new_rates(core, req_rate);
>         if (!top)
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Adriana Reus]

On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> clock tree
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 01306191133c..6ee5fc59cf1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
>         if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
>                 return;
>
> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_unprotect(core);
^ function call before declaration (unless i missed something when applying),
  gets fixed in following patch, but makes this one not compile standalone.

drivers/clk/clk.c: In function 'clk_core_unprepare':
drivers/clk/clk.c:495:3: error: implicit declaration of function
'clk_core_rate_unprotect' [-Werror=implicit-function-declaration]
   clk_core_rate_unprotect(core);

> +
>         if (--core->prepare_count > 0)
>                 return;
>
> @@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
>
>         core->prepare_count++;
>
> +       /*
> +        * CLK_SET_RATE_GATE is a special case of clock protection
> +        * Instead of a consumer protection, the provider is protecting
> +        * itself when prepared
> +        */
> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_protect(core);
^ same here;
Note: maybe have a quick check that each patch compiles individually
(if you haven't already), I did not check them all.
> +
>         return 0;
>  }
>
> @@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>         if (clk_core_rate_is_protected(core))
>                 return -EBUSY;
>
> -       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -               return -EBUSY;
> -
>         /* calculate new rates and get the topmost changed clock */
>         top = clk_calc_new_rates(core, req_rate);
>         if (!top)
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

On Tue, 2017-05-23 at 16:42 +0300, Adriana Reus wrote:
> On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> > clock tree
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> >  drivers/clk/clk.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 01306191133c..6ee5fc59cf1f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
> >         if (WARN_ON(core->prepare_count == 1 && core->flags &
> > CLK_IS_CRITICAL))
> >                 return;
> > 
> > +       if (core->flags & CLK_SET_RATE_GATE)
> > +               clk_core_rate_unprotect(core);
> 
> ^ function call before declaration (unless i missed something when applying),
>   gets fixed in following patch, but makes this one not compile standalone.
> 

You didn't miss anything, I did. It is now fixed


> drivers/clk/clk.c: In function 'clk_core_unprepare':
> drivers/clk/clk.c:495:3: error: implicit declaration of function
> 'clk_core_rate_unprotect' [-Werror=implicit-function-declaration]
>    clk_core_rate_unprotect(core);
> 
> > +
> >         if (--core->prepare_count > 0)
> >                 return;
> > 
> > @@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
> > 
> >         core->prepare_count++;
> > 
> > +       /*
> > +        * CLK_SET_RATE_GATE is a special case of clock protection
> > +        * Instead of a consumer protection, the provider is protecting
> > +        * itself when prepared
> > +        */
> > +       if (core->flags & CLK_SET_RATE_GATE)
> > +               clk_core_rate_protect(core);
> 
> ^ same here;
> Note: maybe have a quick check that each patch compiles individually
> (if you haven't already), I did not check them all.

I should have. Now it is done and it is OK (with our remarks fixed, of course)
Thanks a lot for pointing this out.

> > +
> >         return 0;
> >  }
> > 
> > @@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> >         if (clk_core_rate_is_protected(core))
> >                 return -EBUSY;
> > 
> > -       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> > -               return -EBUSY;
> > -
> >         /* calculate new rates and get the topmost changed clock */
> >         top = clk_calc_new_rates(core, req_rate);
> >         if (!top)
> > --
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 10/11] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

On Tue, 2017-05-23 at 16:42 +0300, Adriana Reus wrote:
> On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> > clock tree
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > ?drivers/clk/clk.c | 14 +++++++++++---
> > ?1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 01306191133c..6ee5fc59cf1f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -491,6 +491,9 @@ static void clk_core_unprepare(struct clk_core *core)
> > ????????if (WARN_ON(core->prepare_count == 1 && core->flags &
> > CLK_IS_CRITICAL))
> > ????????????????return;
> > 
> > +???????if (core->flags & CLK_SET_RATE_GATE)
> > +???????????????clk_core_rate_unprotect(core);
> 
> ^ function call before declaration (unless i missed something when applying),
> ? gets fixed in following patch, but makes this one not compile standalone.
> 

You didn't miss anything, I did. It is now fixed


> drivers/clk/clk.c: In function 'clk_core_unprepare':
> drivers/clk/clk.c:495:3: error: implicit declaration of function
> 'clk_core_rate_unprotect' [-Werror=implicit-function-declaration]
> ???clk_core_rate_unprotect(core);
> 
> > +
> > ????????if (--core->prepare_count > 0)
> > ????????????????return;
> > 
> > @@ -561,6 +564,14 @@ static int clk_core_prepare(struct clk_core *core)
> > 
> > ????????core->prepare_count++;
> > 
> > +???????/*
> > +????????* CLK_SET_RATE_GATE is a special case of clock protection
> > +????????* Instead of a consumer protection, the provider is protecting
> > +????????* itself when prepared
> > +????????*/
> > +???????if (core->flags & CLK_SET_RATE_GATE)
> > +???????????????clk_core_rate_protect(core);
> 
> ^ same here;
> Note: maybe have a quick check that each patch compiles individually
> (if you haven't already), I did not check them all.

I should have. Now it is done and it is OK (with our remarks fixed, of course)
Thanks a lot for pointing this out.

> > +
> > ????????return 0;
> > ?}
> > 
> > @@ -1738,9 +1749,6 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> > ????????if (clk_core_rate_is_protected(core))
> > ????????????????return -EBUSY;
> > 
> > -???????if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> > -???????????????return -EBUSY;
> > -
> > ????????/* calculate new rates and get the topmost changed clock */
> > ????????top = clk_calc_new_rates(core, req_rate);
> > ????????if (!top)
> > --
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at??http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/11] clk: take the prepare lock out of clk_core_set_parent

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:48)
> =

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

Applied to clk-next-protect for testing.

Regards,
Mike

> ---
>  drivers/clk/clk.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> =

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fc58c52a26b4..f5c371532509 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk *pa=
rent)
>  }
>  EXPORT_SYMBOL_GPL(clk_has_parent);
>  =

> -static int clk_core_set_parent(struct clk_core *core, struct clk_core *p=
arent)
> +static int clk_core_set_parent_nolock(struct clk_core *core,
> +                                     struct clk_core *parent)
>  {
>         int ret =3D 0;
>         int p_index =3D 0;
> @@ -1796,23 +1797,16 @@ static int clk_core_set_parent(struct clk_core *c=
ore, struct clk_core *parent)
>         if (!core)
>                 return 0;
>  =

> -       /* prevent racing with updates to the clock topology */
> -       clk_prepare_lock();
> -
>         if (core->parent =3D=3D parent)
> -               goto out;
> +               return 0;
>  =

>         /* verify ops for for multi-parent clks */
> -       if ((core->num_parents > 1) && (!core->ops->set_parent)) {
> -               ret =3D -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 =3D -EBUSY;
> -               goto out;
> -       }
> +       if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
> +               return -EBUSY;
>  =

>         /* try finding the new parent index */
>         if (parent) {
> @@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *cor=
e, 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->nam=
e);
> -                       ret =3D p_index;
> -                       goto out;
> +                       return p_index;
>                 }
>                 p_rate =3D parent->rate;
>         }
> @@ -1831,7 +1824,7 @@ static int clk_core_set_parent(struct clk_core *cor=
e, struct clk_core *parent)
>  =

>         /* abort if a driver objects */
>         if (ret & NOTIFY_STOP_MASK)
> -               goto out;
> +               return ret;
>  =

>         /* do the re-parent */
>         ret =3D __clk_set_parent(core, parent, p_index);
> @@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *cor=
e, struct clk_core *parent)
>                 __clk_recalc_accuracies(core);
>         }
>  =

> -out:
> -       clk_prepare_unlock();
> -
>         return ret;
>  }
>  =

> @@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core *c=
ore, struct clk_core *parent)
>   */
>  int clk_set_parent(struct clk *clk, struct clk *parent)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  =

> -       return clk_core_set_parent(clk->core, parent ? parent->core : NUL=
L);
> +       clk_prepare_lock();
> +       ret =3D clk_core_set_parent_nolock(clk->core,
> +                                        parent ? parent->core : NULL);
> +       clk_prepare_unlock();
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  =

> @@ -2753,7 +2750,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_nolock(child, NULL);
>         }
>  =

>         hlist_del_init(&clk->core->child_node);
> -- =

> 2.9.4
>=20

[PATCH v2 01/11] clk: take the prepare lock out of clk_core_set_parent

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:48)
> 
> 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>

Applied to clk-next-protect for testing.

Regards,
Mike

> ---
>  drivers/clk/clk.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fc58c52a26b4..f5c371532509 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_has_parent);
>  
> -static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
> +static int clk_core_set_parent_nolock(struct clk_core *core,
> +                                     struct clk_core *parent)
>  {
>         int ret = 0;
>         int p_index = 0;
> @@ -1796,23 +1797,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) {
> @@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>                 if (p_index < 0) {
>                         pr_debug("%s: clk %s can not be parent of clk %s\n",
>                                         __func__, parent->name, core->name);
> -                       ret = p_index;
> -                       goto out;
> +                       return p_index;
>                 }
>                 p_rate = parent->rate;
>         }
> @@ -1831,7 +1824,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);
> @@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>                 __clk_recalc_accuracies(core);
>         }
>  
> -out:
> -       clk_prepare_unlock();
> -
>         return ret;
>  }
>  
> @@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
>   */
>  int clk_set_parent(struct clk *clk, struct clk *parent)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> +       clk_prepare_lock();
> +       ret = clk_core_set_parent_nolock(clk->core,
> +                                        parent ? parent->core : NULL);
> +       clk_prepare_unlock();
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> @@ -2753,7 +2750,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_nolock(child, NULL);
>         }
>  
>         hlist_del_init(&clk->core->child_node);
> -- 
> 2.9.4
> 

Re: [PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Michael Turquette]

Quoting Jerome Brunet (2017-05-23 02:48:48)
> On Tue, 2017-05-23 at 12:35 +0300, Adriana Reus wrote:
> > On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> =
wrote:
> > > 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>
> > > ---
> > > =C2=A0drivers/clk/clk.c | 31 +++++++++++++++++++------------
> > > =C2=A01 file changed, 19 insertions(+), 12 deletions(-)
> > > =

> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index f5c371532509..6031fada37f9 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk
> > > *parent)
> > > =C2=A0}
> > > =C2=A0EXPORT_SYMBOL_GPL(clk_set_parent);
> > > =

> > > +static int clk_core_set_phase_nolock(struct clk_core *core, int degr=
ees)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret =3D -EINVAL;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!core)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_clk_set_phase(clk->c=
ore, degrees);
> > =

> > =C2=A0^ trace_clk_set_phase(core, degrees)
> =

> Shame ... Once again this is a poor use of 'git add --patch'.
> This particular diff ended up in patch 5.
> =

> Thanks a lot for catching it!

Patch looks good to me overall. Can you reply here with V3? I'll apply
it to clk-next-protect for testing.

Regards,
Mike

> =

> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (core->ops->set_phase)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ret =3D core->ops->set_phase(core->hw, degrees);
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_clk_set_phase_comple=
te(core, degrees);
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret;
> > > +}
> > > +
> > > =C2=A0/**
> > > =C2=A0 * clk_set_phase - adjust the phase shift of a clock signal
> > > =C2=A0 * @clk: clock signal source
> > > @@ -1895,7 +1912,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
> > > =C2=A0 */
> > > =C2=A0int clk_set_phase(struct clk *clk, int degrees)
> > > =C2=A0{
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret =3D -EINVAL;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret;
> > > =

> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!clk)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> > > @@ -1906,17 +1923,7 @@ int clk_set_phase(struct clk *clk, int degrees)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0degrees +=3D 360;
> > > =

> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0clk_prepare_lock();
> > > -
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_clk_set_phase(clk->c=
ore, degrees);
> > > -
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (clk->core->ops->set_ph=
ase)
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ret =3D clk->core->ops->set_phase(clk->core->hw, de=
grees);
> > > -
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_clk_set_phase_comple=
te(clk->core, degrees);
> > > -
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!ret)
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0clk->core->phase =3D degrees;
> > > -
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D clk_core_set_phase=
_nolock(clk->core, degrees);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0clk_prepare_unlock();
> > > =

> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret;
> > > --
> > > 2.9.4
> > > =

> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-clk" =
in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at=C2=A0=C2=A0http://vger.kernel.org/majordomo-in=
fo.html
>=20

[PATCH v2 02/11] clk: add clk_core_set_phase_nolock function

[Michael Turquette]

Quoting Jerome Brunet (2017-05-23 02:48:48)
> On Tue, 2017-05-23 at 12:35 +0300, Adriana Reus wrote:
> > On Mon, May 22, 2017 at 12:59 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > 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 f5c371532509..6031fada37f9 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -1873,6 +1873,23 @@ int clk_set_parent(struct clk *clk, struct clk
> > > *parent)
> > > ?}
> > > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > > 
> > > +static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
> > > +{
> > > +???????int ret = -EINVAL;
> > > +
> > > +???????if (!core)
> > > +???????????????return 0;
> > > +
> > > +???????trace_clk_set_phase(clk->core, degrees);
> > 
> > ?^ trace_clk_set_phase(core, degrees)
> 
> Shame ... Once again this is a poor use of 'git add --patch'.
> This particular diff ended up in patch 5.
> 
> Thanks a lot for catching it!

Patch looks good to me overall. Can you reply here with V3? I'll apply
it to clk-next-protect for testing.

Regards,
Mike

> 
> > > +
> > > +???????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
> > > @@ -1895,7 +1912,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;
> > > @@ -1906,17 +1923,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_nolock(clk->core, degrees);
> > > ????????clk_prepare_unlock();
> > > 
> > > ????????return ret;
> > > --
> > > 2.9.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > > the body of a message to majordomo at vger.kernel.org
> > > More majordomo info at??http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 03/11] clk: rework calls to round and determine rate callbacks

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:50)
> 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>

Looks good to me. I wonder if we should rename __clk_determine_rate to
clk_hw_determine_rate to bring it in line with clk_hw_round_rate? Only
two users of that clk provider function...

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 6031fada37f9..100f72472e10 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 =3D core->ops->round_rate(core->hw, req->rate,
> +                                            &req->best_parent_rate);
> +               if (rate < 0)
> +                       return rate;
>  =

> -       if (!core)
> -               return 0;
> +               req->rate =3D 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 =3D core->parent;
>         if (parent) {
> @@ -852,22 +870,24 @@ static int clk_core_round_rate_nolock(struct clk_co=
re *core,
>                 req->best_parent_hw =3D NULL;
>                 req->best_parent_rate =3D 0;
>         }
> +}
>  =

> -       if (core->ops->determine_rate) {
> -               return core->ops->determine_rate(core->hw, req);
> -       } else if (core->ops->round_rate) {
> -               rate =3D 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 =3D rate;
> -       } else if (core->flags & CLK_SET_RATE_PARENT) {
> -               return clk_core_round_rate_nolock(parent, req);
> -       } else {
> -               req->rate =3D 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 =3D core->rate;
>         return 0;
>  }
>  =

> @@ -1356,36 +1376,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 =3D rate;
>                 req.min_rate =3D min_rate;
>                 req.max_rate =3D max_rate;
> -               if (parent) {
> -                       req.best_parent_hw =3D parent->hw;
> -                       req.best_parent_rate =3D parent->rate;
> -               } else {
> -                       req.best_parent_hw =3D NULL;
> -                       req.best_parent_rate =3D 0;
> -               }
>  =

> -               ret =3D core->ops->determine_rate(core->hw, &req);
> +               clk_core_init_rate_req(core, &req);
> +
> +               ret =3D clk_core_determine_round(core, &req);
>                 if (ret < 0)
>                         return NULL;
>  =

>                 best_parent_rate =3D req.best_parent_rate;
>                 new_rate =3D req.rate;
>                 parent =3D req.best_parent_hw ? req.best_parent_hw->core =
: NULL;
> -       } else if (core->ops->round_rate) {
> -               ret =3D core->ops->round_rate(core->hw, rate,
> -                                           &best_parent_rate);
> -               if (ret < 0)
> -                       return NULL;
>  =

> -               new_rate =3D 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 =3D core->rate;
> -- =

> 2.9.4
>=20

[PATCH v2 03/11] clk: rework calls to round and determine rate callbacks

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:50)
> 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>

Looks good to me. I wonder if we should rename __clk_determine_rate to
clk_hw_determine_rate to bring it in line with clk_hw_round_rate? Only
two users of that clk provider function...

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 6031fada37f9..100f72472e10 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;
>  }
>  
> @@ -1356,36 +1376,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.4
> 

Re: [PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:51)
> The current implementation of clk_core_set_rate_nolock bails out early if
> the requested rate is exactly the same as the one set. It should bail out
> if the request would not result in rate a change.  This important when ra=
te
> is not exactly what is requested, which is fairly common with PLLs.
> =

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

> This is important to prepare the addition of the clock protection mechani=
sm

Why is this change necessary for the rate_protect feature? I don't see a
major problem with it, but not sure I want to change the expected
behavior unless it is required.

Thanks,
Mike

> =

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

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 100f72472e10..1a8c0d013238 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
>                 clk_change_rate(core->new_child);
>  }
>  =

> +static unsigned long clk_core_req_round_rate_nolock(struct clk_core *cor=
e,
> +                                                    unsigned long req_ra=
te)
> +{
> +       int ret;
> +       struct clk_rate_request req;
> +
> +       if (!core)
> +               return 0;
> +
> +       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> +       req.rate =3D req_rate;
> +
> +       ret =3D clk_core_round_rate_nolock(core, &req);
> +
> +       return ret ? 0 : req.rate;
> +}
> +
>  static int clk_core_set_rate_nolock(struct clk_core *core,
>                                     unsigned long req_rate)
>  {
>         struct clk_core *top, *fail_clk;
> -       unsigned long rate =3D req_rate;
> +       unsigned long rate;
>  =

>         if (!core)
>                 return 0;
>  =

> +       rate =3D clk_core_req_round_rate_nolock(core, req_rate);
> +
>         /* bail early if nothing to do */
>         if (rate =3D=3D clk_core_get_rate_nolock(core))
>                 return 0;
> @@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core=
 *core,
>                 return -EBUSY;
>  =

>         /* calculate new rates and get the topmost changed clock */
> -       top =3D clk_calc_new_rates(core, rate);
> +       top =3D clk_calc_new_rates(core, req_rate);
>         if (!top)
>                 return -EINVAL;
>  =

> -- =

> 2.9.4
>=20

[PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:51)
> The current implementation of clk_core_set_rate_nolock bails out early if
> the requested rate is exactly the same as the one set. It should bail out
> if the request would not result in rate a change.  This important when rate
> is not exactly what is requested, which is fairly common with PLLs.
> 
> Ex: provider able to give any rate with steps of 100Hz
>  - 1st consumer request 48000Hz and gets it.
>  - 2nd consumer request 48010Hz as well. If we were to perform the usual
>    mechanism, we would get 48000Hz as well. The clock would not change so
>    there is no point performing any checks to make sure the clock can
>    change, we know it won't.
> 
> This is important to prepare the addition of the clock protection mechanism

Why is this change necessary for the rate_protect feature? I don't see a
major problem with it, but not sure I want to change the expected
behavior unless it is required.

Thanks,
Mike

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 100f72472e10..1a8c0d013238 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
>                 clk_change_rate(core->new_child);
>  }
>  
> +static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> +                                                    unsigned long req_rate)
> +{
> +       int ret;
> +       struct clk_rate_request req;
> +
> +       if (!core)
> +               return 0;
> +
> +       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> +       req.rate = req_rate;
> +
> +       ret = clk_core_round_rate_nolock(core, &req);
> +
> +       return ret ? 0 : req.rate;
> +}
> +
>  static int clk_core_set_rate_nolock(struct clk_core *core,
>                                     unsigned long req_rate)
>  {
>         struct clk_core *top, *fail_clk;
> -       unsigned long rate = req_rate;
> +       unsigned long rate;
>  
>         if (!core)
>                 return 0;
>  
> +       rate = clk_core_req_round_rate_nolock(core, req_rate);
> +
>         /* bail early if nothing to do */
>         if (rate == clk_core_get_rate_nolock(core))
>                 return 0;
> @@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>                 return -EBUSY;
>  
>         /* calculate new rates and get the topmost changed clock */
> -       top = clk_calc_new_rates(core, rate);
> +       top = clk_calc_new_rates(core, req_rate);
>         if (!top)
>                 return -EINVAL;
>  
> -- 
> 2.9.4
> 

Re: [PATCH v2 05/11] clk: add support for clock protection

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:52)
> 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            | 207 +++++++++++++++++++++++++++++++++++++=
++++--
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 ++++++
>  3 files changed, 230 insertions(+), 7 deletions(-)
> =

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1a8c0d013238..a0524e3bfaca 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_count;
>         struct hlist_node clks_node;
>  };
>  =

> @@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *co=
re)
>         return core->ops->is_prepared(core->hw);
>  }
>  =

> +static bool clk_core_rate_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_rate_is_protected(const struct clk_hw *hw)
> +{
> +       return clk_core_rate_is_protected(hw->core);
> +}
> +
>  bool clk_hw_is_enabled(const struct clk_hw *hw)
>  {
>         return clk_core_is_enabled(hw->core);
> @@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_prepare);
>  =

> +static void clk_core_rate_unprotect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (WARN_ON(core->protect_count =3D=3D 0))
> +               return;
> +
> +       if (--core->protect_count > 0)
> +               return;
> +
> +       clk_core_rate_unprotect(core->parent);
> +}
> +
> +/**
> + * clk_rate_unprotect - unprotect the rate of a clock source
> + * @clk: the clk being unprotected
> + *
> + * clk_unprotect completes a critical section during which the clock
> + * consumer cannot tolerate any change to the clock rate. If no other cl=
ock
> + * consumers have protected clocks in the parent chain, then calls to th=
is
> + * 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 ch=
ain
> + * of clocks.
> + *
> + * Calls to clk_unprotect must be balanced with calls to clk_protect. Ca=
lls
> + * to this function may sleep, and do not return error status.
> + */
> +void clk_rate_unprotect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +
> +       /*
> +        * if there is something wrong with this consumer protect count, =
stop
> +        * here before messing with the provider
> +        */
> +       if (WARN_ON(clk->protect_count <=3D 0))
> +               goto out;
> +
> +       clk_core_rate_unprotect(clk->core);
> +       clk->protect_count--;
> +out:
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_rate_unprotect);
> +
> +static void clk_core_rate_protect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (core->protect_count =3D=3D 0)
> +               clk_core_rate_protect(core->parent);
> +
> +       core->protect_count++;
> +}
> +
> +/**
> + * clk_rate_protect - protect a clock source
> + * @clk: the clk being protected
> + *
> + * clk_protect begins a critical section during which the clock consumer
> + * cannot tolerate any change to the clock rate. This results in all clo=
cks
> + * 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 ch=
ain
> + * 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_rate_protect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +       clk_core_rate_protect(clk->core);
> +       clk->protect_count++;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_rate_protect);
> +
>  static void clk_core_disable(struct clk_core *core)
>  {
>         lockdep_assert_held(&enable_lock);
> @@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core =
*core,
>  {
>         long rate;
>  =

> -       if (core->ops->determine_rate) {
> +       /*
> +        * At this point, core protection will be disabled if
> +        * - if the provider is not protected at all
> +        * - if the calling consumer is the only one protecting the
> +        *   provider (and only once)
> +        */
> +       if (clk_core_rate_is_protected(core)) {
> +               req->rate =3D core->rate;
> +       } else if (core->ops->determine_rate) {
>                 return core->ops->determine_rate(core->hw, req);
>         } else if (core->ops->round_rate) {
>                 rate =3D core->ops->round_rate(core->hw, req->rate,
> @@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long=
 rate)
>  =

>         clk_prepare_lock();
>  =

> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
>         req.rate =3D rate;
>  =

>         ret =3D clk_core_round_rate_nolock(clk->core, &req);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  =

>         if (ret)
> @@ -1575,15 +1698,24 @@ static unsigned long clk_core_req_round_rate_nolo=
ck(struct clk_core *core,

There are way too many round_rate and determine_rate functions.

Here is a list of round_rate and determine_rate "helpers":

static int clk_core_determine_round(struct clk_core *core,
                                    struct clk_rate_request *req)
(called by clk_core_round_rate_nolock and clk_calc_new_rates)

static int clk_core_round_rate_nolock(struct clk_core *core,
                                      struct clk_rate_request *req)
(called in several places)

int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
(called by a few provider drivers. Should probably be renamed as I
mentioned in my previous response)

unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
(used a whole lot)

static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
                                                     unsigned long req_rate)
(only called by clk_core_set_rate_nolock. Can we replace with one of the
options above? Probably clk_core_round_rate_nolock)

>  {
>         int ret;
>         struct clk_rate_request req;
> +       unsigned int cnt =3D core->protect_count;
>  =

>         if (!core)
>                 return 0;
>  =

> +       /* simulate what the rate would be if it could be freely set */
> +       while (core->protect_count)
> +               clk_core_rate_unprotect(core);

Gross. Why do this here and not in similar functions like
clk_core_round_rate_nolock?

> +
>         clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
>         req.rate =3D req_rate;
>  =

>         ret =3D clk_core_round_rate_nolock(core, &req);
>  =

> +       /* restore the protection */
> +       while (core->protect_count < cnt)
> +               clk_core_rate_protect(core);
> +
>         return ret ? 0 : req.rate;
>  }
>  =

> @@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_cor=
e *core,
>         if (rate =3D=3D clk_core_get_rate_nolock(core))
>                 return 0;
>  =

> +       /* fail on a direct rate set of a protected provider */
> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;

Again, why bother unrolling the protected clocks in
clk_core_req_round_rate_nolock if any protection will cause the
operation to fail here?

Best regards,
Mike

> +
>         if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
>                 return -EBUSY;
>  =

> @@ -1658,8 +1794,14 @@ int clk_set_rate(struct clk *clk, unsigned long ra=
te)
>         /* prevent racing with updates to the clock topology */
>         clk_prepare_lock();
>  =

> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret =3D clk_core_set_rate_nolock(clk->core, rate);
>  =

> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  =

>         return ret;
> @@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned =
long min, unsigned long max)
>  =

>         clk_prepare_lock();
>  =

> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         if (min !=3D clk->min_rate || max !=3D clk->max_rate) {
>                 clk->min_rate =3D min;
>                 clk->max_rate =3D max;
>                 ret =3D clk_core_set_rate_nolock(clk->core, clk->core->re=
q_rate);
>         }
>  =

> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  =

>         return ret;
> @@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_co=
re *core,
>         if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
>                 return -EBUSY;
>  =

> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;
> +
>         /* try finding the new parent index */
>         if (parent) {
>                 p_index =3D clk_fetch_parent_index(core, parent);
> @@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk *pa=
rent)
>                 return 0;
>  =

>         clk_prepare_lock();
> +
> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret =3D clk_core_set_parent_nolock(clk->core,
>                                          parent ? parent->core : NULL);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  =

>         return ret;
> @@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_co=
re *core, int degrees)
>         if (!core)
>                 return 0;
>  =

> -       trace_clk_set_phase(clk->core, degrees);
> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;
> +
> +       trace_clk_set_phase(core, degrees);
>  =

>         if (core->ops->set_phase)
>                 ret =3D core->ops->set_phase(core->hw, degrees);
> @@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
>                 degrees +=3D 360;
>  =

>         clk_prepare_lock();
> +
> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret =3D clk_core_set_phase_nolock(clk->core, degrees);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  =

>         return ret;
> @@ -2039,11 +2209,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,
> @@ -2065,8 +2236,8 @@ static int clk_summary_show(struct seq_file *s, voi=
d *data)
>         struct clk_core *c;
>         struct hlist_head **lists =3D (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();
>  =

> @@ -2101,6 +2272,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));
> @@ -2231,6 +2403,11 @@ static int clk_debug_create_one(struct clk_core *c=
ore, struct dentry *pdentry)
>         if (!d)
>                 goto err_out;
>  =

> +       d =3D debugfs_create_u32("clk_protect_count", S_IRUGO, core->dent=
ry,
> +                       (u32 *)&core->protect_count);
> +       if (!d)
> +               goto err_out;
> +
>         d =3D debugfs_create_u32("clk_notifier_count", S_IRUGO, core->den=
try,
>                         (u32 *)&core->notifier_count);
>         if (!d)
> @@ -2794,6 +2971,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();
> @@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
>  =

>         clk_prepare_lock();
>  =

> +       /*
> +        * Before calling clk_put, all calls to clk_rate_protect from a g=
iven
> +        * user must be balanced with calls to clk_rate_unprotect and by =
that
> +        * same user
> +        */
> +       WARN_ON(clk->protect_count);
> +
> +       /* We voiced our concern, let's sanitize the situation */
> +       for (; clk->protect_count; clk->protect_count--)
> +               clk_core_rate_unprotect(clk->core);
> +
>         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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -265,6 +265,30 @@ struct clk *devm_clk_get(struct device *dev, const c=
har *id);
>   */
>  struct clk *devm_get_clk_from_child(struct device *dev,
>                                     struct device_node *np, const char *c=
on_id);
> +/**
> + * clk_rate_protect - inform the system when the clock rate must be prot=
ected.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer protecting the clo=
ck
> + * depends on the rate of the clock source and can't tolerate any glitch=
es
> + * introduced by further clock rate change or re-parenting of the clock =
source.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_rate_protect(struct clk *clk);
> +
> +/**
> + * clk_rate_unprotect - release the protection of the clock source.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer previously protect=
ing the
> + * clock rate can now deal with other consumer altering the clock source=
 rate
> + *
> + * The caller must balance the number of rate_protect and rate_unprotect=
 calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_rate_unprotect(struct clk *clk);
>  =

>  /**
>   * clk_enable - inform the system when the clock source should be runnin=
g.
> @@ -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.4
> =

> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 05/11] clk: add support for clock protection

[Michael Turquette]

Quoting Jerome Brunet (2017-05-21 14:59:52)
> 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            | 207 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/clk-provider.h |   1 +
>  include/linux/clk.h          |  29 ++++++
>  3 files changed, 230 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1a8c0d013238..a0524e3bfaca 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_count;
>         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_rate_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_rate_is_protected(const struct clk_hw *hw)
> +{
> +       return clk_core_rate_is_protected(hw->core);
> +}
> +
>  bool clk_hw_is_enabled(const struct clk_hw *hw)
>  {
>         return clk_core_is_enabled(hw->core);
> @@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_prepare);
>  
> +static void clk_core_rate_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_rate_unprotect(core->parent);
> +}
> +
> +/**
> + * clk_rate_unprotect - unprotect the rate of a clock source
> + * @clk: the clk being unprotected
> + *
> + * clk_unprotect completes a critical section during which the clock
> + * consumer cannot tolerate any change to the clock rate. If no other clock
> + * consumers have protected clocks in the parent chain, then calls to this
> + * function will allow the clocks in the parent chain to change rates
> + * freely.
> + *
> + * Unlike the clk_set_rate_range method, which allows the rate to change
> + * within a given range, protected clocks cannot have their rate changed,
> + * either directly or indirectly due to changes further up the parent chain
> + * of clocks.
> + *
> + * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
> + * to this function may sleep, and do not return error status.
> + */
> +void clk_rate_unprotect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +
> +       /*
> +        * if there is something wrong with this consumer protect count, stop
> +        * here before messing with the provider
> +        */
> +       if (WARN_ON(clk->protect_count <= 0))
> +               goto out;
> +
> +       clk_core_rate_unprotect(clk->core);
> +       clk->protect_count--;
> +out:
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_rate_unprotect);
> +
> +static void clk_core_rate_protect(struct clk_core *core)
> +{
> +       lockdep_assert_held(&prepare_lock);
> +
> +       if (!core)
> +               return;
> +
> +       if (core->protect_count == 0)
> +               clk_core_rate_protect(core->parent);
> +
> +       core->protect_count++;
> +}
> +
> +/**
> + * clk_rate_protect - protect a clock source
> + * @clk: the clk being protected
> + *
> + * 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_rate_protect(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       clk_prepare_lock();
> +       clk_core_rate_protect(clk->core);
> +       clk->protect_count++;
> +       clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_rate_protect);
> +
>  static void clk_core_disable(struct clk_core *core)
>  {
>         lockdep_assert_held(&enable_lock);
> @@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core *core,
>  {
>         long rate;
>  
> -       if (core->ops->determine_rate) {
> +       /*
> +        * At this point, core protection will be disabled if
> +        * - if the provider is not protected at all
> +        * - if the calling consumer is the only one protecting the
> +        *   provider (and only once)
> +        */
> +       if (clk_core_rate_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,
> @@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  
>         clk_prepare_lock();
>  
> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
>         req.rate = rate;
>  
>         ret = clk_core_round_rate_nolock(clk->core, &req);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         if (ret)
> @@ -1575,15 +1698,24 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,

There are way too many round_rate and determine_rate functions.

Here is a list of round_rate and determine_rate "helpers":

static int clk_core_determine_round(struct clk_core *core,
                                    struct clk_rate_request *req)
(called by clk_core_round_rate_nolock and clk_calc_new_rates)

static int clk_core_round_rate_nolock(struct clk_core *core,
                                      struct clk_rate_request *req)
(called in several places)

int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
(called by a few provider drivers. Should probably be renamed as I
mentioned in my previous response)

unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
(used a whole lot)

static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
                                                     unsigned long req_rate)
(only called by clk_core_set_rate_nolock. Can we replace with one of the
options above? Probably clk_core_round_rate_nolock)

>  {
>         int ret;
>         struct clk_rate_request req;
> +       unsigned int cnt = core->protect_count;
>  
>         if (!core)
>                 return 0;
>  
> +       /* simulate what the rate would be if it could be freely set */
> +       while (core->protect_count)
> +               clk_core_rate_unprotect(core);

Gross. Why do this here and not in similar functions like
clk_core_round_rate_nolock?

> +
>         clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
>         req.rate = req_rate;
>  
>         ret = clk_core_round_rate_nolock(core, &req);
>  
> +       /* restore the protection */
> +       while (core->protect_count < cnt)
> +               clk_core_rate_protect(core);
> +
>         return ret ? 0 : req.rate;
>  }
>  
> @@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>         if (rate == clk_core_get_rate_nolock(core))
>                 return 0;
>  
> +       /* fail on a direct rate set of a protected provider */
> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;

Again, why bother unrolling the protected clocks in
clk_core_req_round_rate_nolock if any protection will cause the
operation to fail here?

Best regards,
Mike

> +
>         if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
>                 return -EBUSY;
>  
> @@ -1658,8 +1794,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_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret = clk_core_set_rate_nolock(clk->core, rate);
>  
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>  
>         clk_prepare_lock();
>  
> +       if (clk->protect_count)
> +               clk_core_rate_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_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
>         if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
>                 return -EBUSY;
>  
> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;
> +
>         /* try finding the new parent index */
>         if (parent) {
>                 p_index = clk_fetch_parent_index(core, parent);
> @@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>                 return 0;
>  
>         clk_prepare_lock();
> +
> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret = clk_core_set_parent_nolock(clk->core,
>                                          parent ? parent->core : NULL);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
>         if (!core)
>                 return 0;
>  
> -       trace_clk_set_phase(clk->core, degrees);
> +       if (clk_core_rate_is_protected(core))
> +               return -EBUSY;
> +
> +       trace_clk_set_phase(core, degrees);
>  
>         if (core->ops->set_phase)
>                 ret = core->ops->set_phase(core->hw, degrees);
> @@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
>                 degrees += 360;
>  
>         clk_prepare_lock();
> +
> +       if (clk->protect_count)
> +               clk_core_rate_unprotect(clk->core);
> +
>         ret = clk_core_set_phase_nolock(clk->core, degrees);
> +
> +       if (clk->protect_count)
> +               clk_core_rate_protect(clk->core);
> +
>         clk_prepare_unlock();
>  
>         return ret;
> @@ -2039,11 +2209,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,
> @@ -2065,8 +2236,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();
>  
> @@ -2101,6 +2272,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));
> @@ -2231,6 +2403,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)
> @@ -2794,6 +2971,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();
> @@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
>  
>         clk_prepare_lock();
>  
> +       /*
> +        * Before calling clk_put, all calls to clk_rate_protect from a given
> +        * user must be balanced with calls to clk_rate_unprotect and by that
> +        * same user
> +        */
> +       WARN_ON(clk->protect_count);
> +
> +       /* We voiced our concern, let's sanitize the situation */
> +       for (; clk->protect_count; clk->protect_count--)
> +               clk_core_rate_unprotect(clk->core);
> +
>         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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 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_rate_protect - inform the system when the clock rate must be protected.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer protecting the clock
> + * depends on the rate of the clock source and can't tolerate any glitches
> + * introduced by further clock rate change or re-parenting of the clock source.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_rate_protect(struct clk *clk);
> +
> +/**
> + * clk_rate_unprotect - release the protection of the clock source.
> + * @clk: clock source
> + *
> + * This function informs the system that the consumer previously protecting the
> + * clock rate can now deal with other consumer altering the clock source rate
> + *
> + * The caller must balance the number of rate_protect and rate_unprotect calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_rate_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.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Jerome Brunet]

On Thu, 2017-05-25 at 13:20 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:51)
> > The current implementation of clk_core_set_rate_nolock bails out early if
> > the requested rate is exactly the same as the one set. It should bail out
> > if the request would not result in rate a change.  This important when rate
> > is not exactly what is requested, which is fairly common with PLLs.
> > 
> > Ex: provider able to give any rate with steps of 100Hz
> >  - 1st consumer request 48000Hz and gets it.
> >  - 2nd consumer request 48010Hz as well. If we were to perform the usual
> >    mechanism, we would get 48000Hz as well. The clock would not change so
> >    there is no point performing any checks to make sure the clock can
> >    change, we know it won't.
> > 
> > This is important to prepare the addition of the clock protection mechanism
> 
> Why is this change necessary for the rate_protect feature? I don't see a
> major problem with it, but not sure I want to change the expected
> behavior unless it is required.

It isn't strictly required for the clock protection to work but it allows the
consumer to get a coherent feedback from set_rate.

Without this, we may end up in a situation where the set_rate return with -EBUSY
while the rate of the clock is actually the best possible we could have wished
for (like in the example: we ask for 48010 and the rate set in 48000)

With this patch, If a consumer gets EBUSY, it knows the rate set is not optimal.
It could have been closer to what is requested if it wasn't busy doing something
else.

If the rate is (already) optimally set, as requested by the consumer, I think we
should not return an error here.

I don't think this changes the behavior of the function, the rate set will be
same anyway, it just won't return an error in a particular case.

If you firmly oppose this idea, I suppose It can be dropped but I think it could
be a valuable addition.

> 
> Thanks,
> Mike
> 
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> >  drivers/clk/clk.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 100f72472e10..1a8c0d013238 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
> >                 clk_change_rate(core->new_child);
> >  }
> >  
> > +static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> > +                                                    unsigned long req_rate)
> > +{
> > +       int ret;
> > +       struct clk_rate_request req;
> > +
> > +       if (!core)
> > +               return 0;
> > +
> > +       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > +       req.rate = req_rate;
> > +
> > +       ret = clk_core_round_rate_nolock(core, &req);
> > +
> > +       return ret ? 0 : req.rate;
> > +}
> > +
> >  static int clk_core_set_rate_nolock(struct clk_core *core,
> >                                     unsigned long req_rate)
> >  {
> >         struct clk_core *top, *fail_clk;
> > -       unsigned long rate = req_rate;
> > +       unsigned long rate;
> >  
> >         if (!core)
> >                 return 0;
> >  
> > +       rate = clk_core_req_round_rate_nolock(core, req_rate);
> > +
> >         /* bail early if nothing to do */
> >         if (rate == clk_core_get_rate_nolock(core))
> >                 return 0;
> > @@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> >                 return -EBUSY;
> >  
> >         /* calculate new rates and get the topmost changed clock */
> > -       top = clk_calc_new_rates(core, rate);
> > +       top = clk_calc_new_rates(core, req_rate);
> >         if (!top)
> >                 return -EINVAL;
> >  
> > -- 
> > 2.9.4
> > 

[PATCH v2 04/11] clk: use round rate to bail out early in set_rate

[Jerome Brunet]

On Thu, 2017-05-25 at 13:20 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:51)
> > The current implementation of clk_core_set_rate_nolock bails out early if
> > the requested rate is exactly the same as the one set. It should bail out
> > if the request would not result in rate a change.??This important when rate
> > is not exactly what is requested, which is fairly common with PLLs.
> > 
> > Ex: provider able to give any rate with steps of 100Hz
> > ?- 1st consumer request 48000Hz and gets it.
> > ?- 2nd consumer request 48010Hz as well. If we were to perform the usual
> > ???mechanism, we would get 48000Hz as well. The clock would not change so
> > ???there is no point performing any checks to make sure the clock can
> > ???change, we know it won't.
> > 
> > This is important to prepare the addition of the clock protection mechanism
> 
> Why is this change necessary for the rate_protect feature? I don't see a
> major problem with it, but not sure I want to change the expected
> behavior unless it is required.

It isn't strictly required for the clock protection to work but it allows the
consumer to get a coherent feedback from set_rate.

Without this, we may end up in a situation where the set_rate return with -EBUSY
while the rate of the clock is actually the best possible we could have wished
for (like in the example: we ask for 48010 and the rate set in 48000)

With this patch, If a consumer gets EBUSY, it knows the rate set is not optimal.
It could have been closer to what is requested if it wasn't busy doing something
else.

If the rate is (already) optimally set, as requested by the consumer, I think we
should not return an error here.

I don't think this changes the behavior of the function, the rate set will be
same anyway, it just won't return an error in a particular case.

If you firmly oppose this idea, I suppose It can be dropped but I think it could
be a valuable addition.

> 
> Thanks,
> Mike
> 
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > ?drivers/clk/clk.c | 23 +++++++++++++++++++++--
> > ?1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 100f72472e10..1a8c0d013238 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1570,15 +1570,34 @@ static void clk_change_rate(struct clk_core *core)
> > ????????????????clk_change_rate(core->new_child);
> > ?}
> > ?
> > +static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> > +????????????????????????????????????????????????????unsigned long req_rate)
> > +{
> > +???????int ret;
> > +???????struct clk_rate_request req;
> > +
> > +???????if (!core)
> > +???????????????return 0;
> > +
> > +???????clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > +???????req.rate = req_rate;
> > +
> > +???????ret = clk_core_round_rate_nolock(core, &req);
> > +
> > +???????return ret ? 0 : req.rate;
> > +}
> > +
> > ?static int clk_core_set_rate_nolock(struct clk_core *core,
> > ????????????????????????????????????unsigned long req_rate)
> > ?{
> > ????????struct clk_core *top, *fail_clk;
> > -???????unsigned long rate = req_rate;
> > +???????unsigned long rate;
> > ?
> > ????????if (!core)
> > ????????????????return 0;
> > ?
> > +???????rate = clk_core_req_round_rate_nolock(core, req_rate);
> > +
> > ????????/* bail early if nothing to do */
> > ????????if (rate == clk_core_get_rate_nolock(core))
> > ????????????????return 0;
> > @@ -1587,7 +1606,7 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> > ????????????????return -EBUSY;
> > ?
> > ????????/* calculate new rates and get the topmost changed clock */
> > -???????top = clk_calc_new_rates(core, rate);
> > +???????top = clk_calc_new_rates(core, req_rate);
> > ????????if (!top)
> > ????????????????return -EINVAL;
> > ?
> > --?
> > 2.9.4
> > 

Re: [PATCH v2 05/11] clk: add support for clock protection

[Jerome Brunet]

On Thu, 2017-05-25 at 13:58 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:52)
> > 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            | 207
> > +++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/clk-provider.h |   1 +
> >  include/linux/clk.h          |  29 ++++++
> >  3 files changed, 230 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 1a8c0d013238..a0524e3bfaca 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_count;
> >         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_rate_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_rate_is_protected(const struct clk_hw *hw)
> > +{
> > +       return clk_core_rate_is_protected(hw->core);
> > +}
> > +
> >  bool clk_hw_is_enabled(const struct clk_hw *hw)
> >  {
> >         return clk_core_is_enabled(hw->core);
> > @@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_prepare);
> >  
> > +static void clk_core_rate_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_rate_unprotect(core->parent);
> > +}
> > +
> > +/**
> > + * clk_rate_unprotect - unprotect the rate of a clock source
> > + * @clk: the clk being unprotected
> > + *
> > + * clk_unprotect completes a critical section during which the clock
> > + * consumer cannot tolerate any change to the clock rate. If no other clock
> > + * consumers have protected clocks in the parent chain, then calls to this
> > + * function will allow the clocks in the parent chain to change rates
> > + * freely.
> > + *
> > + * Unlike the clk_set_rate_range method, which allows the rate to change
> > + * within a given range, protected clocks cannot have their rate changed,
> > + * either directly or indirectly due to changes further up the parent chain
> > + * of clocks.
> > + *
> > + * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
> > + * to this function may sleep, and do not return error status.
> > + */
> > +void clk_rate_unprotect(struct clk *clk)
> > +{
> > +       if (!clk)
> > +               return;
> > +
> > +       clk_prepare_lock();
> > +
> > +       /*
> > +        * if there is something wrong with this consumer protect count,
> > stop
> > +        * here before messing with the provider
> > +        */
> > +       if (WARN_ON(clk->protect_count <= 0))
> > +               goto out;
> > +
> > +       clk_core_rate_unprotect(clk->core);
> > +       clk->protect_count--;
> > +out:
> > +       clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_rate_unprotect);
> > +
> > +static void clk_core_rate_protect(struct clk_core *core)
> > +{
> > +       lockdep_assert_held(&prepare_lock);
> > +
> > +       if (!core)
> > +               return;
> > +
> > +       if (core->protect_count == 0)
> > +               clk_core_rate_protect(core->parent);
> > +
> > +       core->protect_count++;
> > +}
> > +
> > +/**
> > + * clk_rate_protect - protect a clock source
> > + * @clk: the clk being protected
> > + *
> > + * 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_rate_protect(struct clk *clk)
> > +{
> > +       if (!clk)
> > +               return;
> > +
> > +       clk_prepare_lock();
> > +       clk_core_rate_protect(clk->core);
> > +       clk->protect_count++;
> > +       clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_rate_protect);
> > +
> >  static void clk_core_disable(struct clk_core *core)
> >  {
> >         lockdep_assert_held(&enable_lock);
> > @@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core
> > *core,
> >  {
> >         long rate;
> >  
> > -       if (core->ops->determine_rate) {
> > +       /*
> > +        * At this point, core protection will be disabled if
> > +        * - if the provider is not protected at all
> > +        * - if the calling consumer is the only one protecting the
> > +        *   provider (and only once)
> > +        */
> > +       if (clk_core_rate_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,
> > @@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long
> > rate)
> >  
> >         clk_prepare_lock();
> >  
> > +       if (clk->protect_count)
> > +               clk_core_rate_unprotect(clk->core);
> > +
> >         clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
> >         req.rate = rate;
> >  
> >         ret = clk_core_round_rate_nolock(clk->core, &req);
> > +
> > +       if (clk->protect_count)
> > +               clk_core_rate_protect(clk->core);
> > +
> >         clk_prepare_unlock();
> >  
> >         if (ret)
> > @@ -1575,15 +1698,24 @@ static unsigned long
> > clk_core_req_round_rate_nolock(struct clk_core *core,
> 
> There are way too many round_rate and determine_rate functions.
> 
> Here is a list of round_rate and determine_rate "helpers":
> 
> static int clk_core_determine_round(struct clk_core *core,
>                                     struct clk_rate_request *req)
> (called by clk_core_round_rate_nolock and clk_calc_new_rates)
> 
> static int clk_core_round_rate_nolock(struct clk_core *core,
>                                       struct clk_rate_request *req)
> (called in several places)
> 
> int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> (called by a few provider drivers. Should probably be renamed as I
> mentioned in my previous response)
> 
> unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
> (used a whole lot)
> 
> static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
>                                                      unsigned long req_rate)
> (only called by clk_core_set_rate_nolock. Can we replace with one of the
> options above? Probably clk_core_round_rate_nolock)

Agreed, we could clean this a bit I suppose

> 
> >  {
> >         int ret;
> >         struct clk_rate_request req;
> > +       unsigned int cnt = core->protect_count;
> >  
> >         if (!core)
> >                 return 0;
> >  
> > +       /* simulate what the rate would be if it could be freely set */
> > +       while (core->protect_count)
> > +               clk_core_rate_unprotect(core);
> 
> Gross. Why do this here and not in similar functions like
> clk_core_round_rate_nolock?

I agree it isn't very nice but there is a good reason for this.
When a provider is protected, all you are going to get from round_rate is the
current rate - which is the expected result.

At this point, we are trying to figure out if the request of the consumer would
require the provider to change its settings, whether it is protected or not. 

In order to get that, we need to unroll the protection applied to this provider
along the tree. This why I needed a specific helper, otherwise there is no
reason to unroll the protection.

If parent providers are still protected by siblings after that, it is a
constraints on the clock tree and it is fine.

> 
> > +
> >         clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> >         req.rate = req_rate;
> >  
> >         ret = clk_core_round_rate_nolock(core, &req);
> >  
> > +       /* restore the protection */
> > +       while (core->protect_count < cnt)
> > +               clk_core_rate_protect(core);
> > +
> >         return ret ? 0 : req.rate;
> >  }
> >  
> > @@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> >         if (rate == clk_core_get_rate_nolock(core))
> >                 return 0;
> >  
> > +       /* fail on a direct rate set of a protected provider */
> > +       if (clk_core_rate_is_protected(core))
> > +               return -EBUSY;
> 
> Again, why bother unrolling the protected clocks in
> clk_core_req_round_rate_nolock if any protection will cause the
> operation to fail here?

If we don't unroll the protection on a protected clock:
(round_rate(requested_rate) == clk_core_get_rate_nolock(core)) would always be
true and we would never get to this check. 

We should get to this point only if the request requires a change of the
settings. If the clock is protected, we should tell the consumer its request
can't be (optimally) satisfied. 

All this sounds like obscur corner case but I think it may happen more often
than we think:

Imagine the 2 consumers requesting the same rate from a provider (ex: 48000) and
 the provider only approximately match it (ex 48010):

1 - Consumer #1 request 48000
2 - Rate is set to 48010 and set rate successfully returns
3 - Consumer #2 request 48000 as well
     -> Should it get different result than consumer #1 ?

As it is, requested rate is different from the rate set and set_rate reports
-EBUSY

With the proposed change, set_rate is able to figure out that we are already on
the best setting and report success to the consumer. From the consumer point of
view, I think this is more coherent.

> 
> Best regards,
> Mike
> 
> > +
> >         if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> >                 return -EBUSY;
> >  
> > @@ -1658,8 +1794,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_count)
> > +               clk_core_rate_unprotect(clk->core);
> > +
> >         ret = clk_core_set_rate_nolock(clk->core, rate);
> >  
> > +       if (clk->protect_count)
> > +               clk_core_rate_protect(clk->core);
> > +
> >         clk_prepare_unlock();
> >  
> >         return ret;
> > @@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
> > long min, unsigned long max)
> >  
> >         clk_prepare_lock();
> >  
> > +       if (clk->protect_count)
> > +               clk_core_rate_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_count)
> > +               clk_core_rate_protect(clk->core);
> > +
> >         clk_prepare_unlock();
> >  
> >         return ret;
> > @@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_core
> > *core,
> >         if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
> >                 return -EBUSY;
> >  
> > +       if (clk_core_rate_is_protected(core))
> > +               return -EBUSY;
> > +
> >         /* try finding the new parent index */
> >         if (parent) {
> >                 p_index = clk_fetch_parent_index(core, parent);
> > @@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk
> > *parent)
> >                 return 0;
> >  
> >         clk_prepare_lock();
> > +
> > +       if (clk->protect_count)
> > +               clk_core_rate_unprotect(clk->core);
> > +
> >         ret = clk_core_set_parent_nolock(clk->core,
> >                                          parent ? parent->core : NULL);
> > +
> > +       if (clk->protect_count)
> > +               clk_core_rate_protect(clk->core);
> > +
> >         clk_prepare_unlock();
> >  
> >         return ret;
> > @@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_core
> > *core, int degrees)
> >         if (!core)
> >                 return 0;
> >  
> > -       trace_clk_set_phase(clk->core, degrees);
> > +       if (clk_core_rate_is_protected(core))
> > +               return -EBUSY;
> > +
> > +       trace_clk_set_phase(core, degrees);
> >  
> >         if (core->ops->set_phase)
> >                 ret = core->ops->set_phase(core->hw, degrees);
> > @@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
> >                 degrees += 360;
> >  
> >         clk_prepare_lock();
> > +
> > +       if (clk->protect_count)
> > +               clk_core_rate_unprotect(clk->core);
> > +
> >         ret = clk_core_set_phase_nolock(clk->core, degrees);
> > +
> > +       if (clk->protect_count)
> > +               clk_core_rate_protect(clk->core);
> > +
> >         clk_prepare_unlock();
> >  
> >         return ret;
> > @@ -2039,11 +2209,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,
> > @@ -2065,8 +2236,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();
> >  
> > @@ -2101,6 +2272,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));
> > @@ -2231,6 +2403,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)
> > @@ -2794,6 +2971,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();
> > @@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
> >  
> >         clk_prepare_lock();
> >  
> > +       /*
> > +        * Before calling clk_put, all calls to clk_rate_protect from a
> > given
> > +        * user must be balanced with calls to clk_rate_unprotect and by
> > that
> > +        * same user
> > +        */
> > +       WARN_ON(clk->protect_count);
> > +
> > +       /* We voiced our concern, let's sanitize the situation */
> > +       for (; clk->protect_count; clk->protect_count--)
> > +               clk_core_rate_unprotect(clk->core);
> > +
> >         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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 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_rate_protect - inform the system when the clock rate must be
> > protected.
> > + * @clk: clock source
> > + *
> > + * This function informs the system that the consumer protecting the clock
> > + * depends on the rate of the clock source and can't tolerate any glitches
> > + * introduced by further clock rate change or re-parenting of the clock
> > source.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_rate_protect(struct clk *clk);
> > +
> > +/**
> > + * clk_rate_unprotect - release the protection of the clock source.
> > + * @clk: clock source
> > + *
> > + * This function informs the system that the consumer previously protecting
> > the
> > + * clock rate can now deal with other consumer altering the clock source
> > rate
> > + *
> > + * The caller must balance the number of rate_protect and rate_unprotect
> > calls.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_rate_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.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 05/11] clk: add support for clock protection

[Jerome Brunet]

On Thu, 2017-05-25 at 13:58 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:52)
> > 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????????????| 207
> > +++++++++++++++++++++++++++++++++++++++++--
> > ?include/linux/clk-provider.h |???1 +
> > ?include/linux/clk.h??????????|??29 ++++++
> > ?3 files changed, 230 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 1a8c0d013238..a0524e3bfaca 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_count;
> > ????????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_rate_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_rate_is_protected(const struct clk_hw *hw)
> > +{
> > +???????return clk_core_rate_is_protected(hw->core);
> > +}
> > +
> > ?bool clk_hw_is_enabled(const struct clk_hw *hw)
> > ?{
> > ????????return clk_core_is_enabled(hw->core);
> > @@ -584,6 +596,102 @@ int clk_prepare(struct clk *clk)
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_prepare);
> > ?
> > +static void clk_core_rate_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_rate_unprotect(core->parent);
> > +}
> > +
> > +/**
> > + * clk_rate_unprotect - unprotect the rate of a clock source
> > + * @clk: the clk being unprotected
> > + *
> > + * clk_unprotect completes a critical section during which the clock
> > + * consumer cannot tolerate any change to the clock rate. If no other clock
> > + * consumers have protected clocks in the parent chain, then calls to this
> > + * function will allow the clocks in the parent chain to change rates
> > + * freely.
> > + *
> > + * Unlike the clk_set_rate_range method, which allows the rate to change
> > + * within a given range, protected clocks cannot have their rate changed,
> > + * either directly or indirectly due to changes further up the parent chain
> > + * of clocks.
> > + *
> > + * Calls to clk_unprotect must be balanced with calls to clk_protect. Calls
> > + * to this function may sleep, and do not return error status.
> > + */
> > +void clk_rate_unprotect(struct clk *clk)
> > +{
> > +???????if (!clk)
> > +???????????????return;
> > +
> > +???????clk_prepare_lock();
> > +
> > +???????/*
> > +????????* if there is something wrong with this consumer protect count,
> > stop
> > +????????* here before messing with the provider
> > +????????*/
> > +???????if (WARN_ON(clk->protect_count <= 0))
> > +???????????????goto out;
> > +
> > +???????clk_core_rate_unprotect(clk->core);
> > +???????clk->protect_count--;
> > +out:
> > +???????clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_rate_unprotect);
> > +
> > +static void clk_core_rate_protect(struct clk_core *core)
> > +{
> > +???????lockdep_assert_held(&prepare_lock);
> > +
> > +???????if (!core)
> > +???????????????return;
> > +
> > +???????if (core->protect_count == 0)
> > +???????????????clk_core_rate_protect(core->parent);
> > +
> > +???????core->protect_count++;
> > +}
> > +
> > +/**
> > + * clk_rate_protect - protect a clock source
> > + * @clk: the clk being protected
> > + *
> > + * 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_rate_protect(struct clk *clk)
> > +{
> > +???????if (!clk)
> > +???????????????return;
> > +
> > +???????clk_prepare_lock();
> > +???????clk_core_rate_protect(clk->core);
> > +???????clk->protect_count++;
> > +???????clk_prepare_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(clk_rate_protect);
> > +
> > ?static void clk_core_disable(struct clk_core *core)
> > ?{
> > ????????lockdep_assert_held(&enable_lock);
> > @@ -838,7 +946,15 @@ static int clk_core_determine_round(struct clk_core
> > *core,
> > ?{
> > ????????long rate;
> > ?
> > -???????if (core->ops->determine_rate) {
> > +???????/*
> > +????????* At this point, core protection will be disabled if
> > +????????* - if the provider is not protected at all
> > +????????* - if the calling consumer is the only one protecting the
> > +????????*???provider (and only once)
> > +????????*/
> > +???????if (clk_core_rate_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,
> > @@ -944,10 +1060,17 @@ long clk_round_rate(struct clk *clk, unsigned long
> > rate)
> > ?
> > ????????clk_prepare_lock();
> > ?
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_unprotect(clk->core);
> > +
> > ????????clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
> > ????????req.rate = rate;
> > ?
> > ????????ret = clk_core_round_rate_nolock(clk->core, &req);
> > +
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????if (ret)
> > @@ -1575,15 +1698,24 @@ static unsigned long
> > clk_core_req_round_rate_nolock(struct clk_core *core,
> 
> There are way too many round_rate and determine_rate functions.
> 
> Here is a list of round_rate and determine_rate "helpers":
> 
> static int clk_core_determine_round(struct clk_core *core,
> ????????????????????????????????????struct clk_rate_request *req)
> (called by clk_core_round_rate_nolock and clk_calc_new_rates)
> 
> static int clk_core_round_rate_nolock(struct clk_core *core,
> ??????????????????????????????????????struct clk_rate_request *req)
> (called in several places)
> 
> int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> (called by a few provider drivers. Should probably be renamed as I
> mentioned in my previous response)
> 
> unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
> (used a whole lot)
> 
> static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> ?????????????????????????????????????????????????????unsigned long req_rate)
> (only called by clk_core_set_rate_nolock. Can we replace with one of the
> options above? Probably clk_core_round_rate_nolock)

Agreed, we could clean this a bit I suppose

> 
> > ?{
> > ????????int ret;
> > ????????struct clk_rate_request req;
> > +???????unsigned int cnt = core->protect_count;
> > ?
> > ????????if (!core)
> > ????????????????return 0;
> > ?
> > +???????/* simulate what the rate would be if it could be freely set */
> > +???????while (core->protect_count)
> > +???????????????clk_core_rate_unprotect(core);
> 
> Gross. Why do this here and not in similar functions like
> clk_core_round_rate_nolock?

I agree it isn't very nice but there is a good reason for this.
When a provider is protected, all you are going to get from round_rate is the
current rate - which is the expected result.

At this point, we are trying to figure out if the request of the consumer would
require the provider to change its settings, whether it is protected or not.?

In order to get that, we need to unroll the protection applied to this provider
along the tree. This why I needed a specific helper, otherwise there is no
reason to unroll the protection.

If parent providers are still protected by siblings after that, it is a
constraints on the clock tree and it is fine.

> 
> > +
> > ????????clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > ????????req.rate = req_rate;
> > ?
> > ????????ret = clk_core_round_rate_nolock(core, &req);
> > ?
> > +???????/* restore the protection */
> > +???????while (core->protect_count < cnt)
> > +???????????????clk_core_rate_protect(core);
> > +
> > ????????return ret ? 0 : req.rate;
> > ?}
> > ?
> > @@ -1602,6 +1734,10 @@ static int clk_core_set_rate_nolock(struct clk_core
> > *core,
> > ????????if (rate == clk_core_get_rate_nolock(core))
> > ????????????????return 0;
> > ?
> > +???????/* fail on a direct rate set of a protected provider */
> > +???????if (clk_core_rate_is_protected(core))
> > +???????????????return -EBUSY;
> 
> Again, why bother unrolling the protected clocks in
> clk_core_req_round_rate_nolock if any protection will cause the
> operation to fail here?

If we don't unroll the protection on a protected clock:
(round_rate(requested_rate) == clk_core_get_rate_nolock(core)) would always be
true and we would never get to this check.?

We should get to this point only if the request requires a change of the
settings. If the clock is protected, we should tell the consumer its request
can't be (optimally) satisfied. 

All this sounds like obscur corner case but I think it may happen more often
than we think:

Imagine the 2 consumers requesting the same rate from a provider (ex: 48000) and
 the provider only approximately match it (ex 48010):

1 - Consumer #1 request 48000
2 - Rate is set to 48010 and set rate successfully returns
3 - Consumer #2 request 48000 as well
     -> Should it get different result than consumer #1 ?

As it is, requested rate is different from the rate set and set_rate reports
-EBUSY

With the proposed change, set_rate is able to figure out that we are already on
the best setting and report success to the consumer. From the consumer point of
view, I think this is more coherent.

> 
> Best regards,
> Mike
> 
> > +
> > ????????if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> > ????????????????return -EBUSY;
> > ?
> > @@ -1658,8 +1794,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_count)
> > +???????????????clk_core_rate_unprotect(clk->core);
> > +
> > ????????ret = clk_core_set_rate_nolock(clk->core, rate);
> > ?
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1690,12 +1832,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
> > long min, unsigned long max)
> > ?
> > ????????clk_prepare_lock();
> > ?
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_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_count)
> > +???????????????clk_core_rate_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1837,6 +1985,9 @@ static int clk_core_set_parent_nolock(struct clk_core
> > *core,
> > ????????if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
> > ????????????????return -EBUSY;
> > ?
> > +???????if (clk_core_rate_is_protected(core))
> > +???????????????return -EBUSY;
> > +
> > ????????/* try finding the new parent index */
> > ????????if (parent) {
> > ????????????????p_index = clk_fetch_parent_index(core, parent);
> > @@ -1894,8 +2045,16 @@ int clk_set_parent(struct clk *clk, struct clk
> > *parent)
> > ????????????????return 0;
> > ?
> > ????????clk_prepare_lock();
> > +
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_unprotect(clk->core);
> > +
> > ????????ret = clk_core_set_parent_nolock(clk->core,
> > ?????????????????????????????????????????parent ? parent->core : NULL);
> > +
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -1909,7 +2068,10 @@ static int clk_core_set_phase_nolock(struct clk_core
> > *core, int degrees)
> > ????????if (!core)
> > ????????????????return 0;
> > ?
> > -???????trace_clk_set_phase(clk->core, degrees);
> > +???????if (clk_core_rate_is_protected(core))
> > +???????????????return -EBUSY;
> > +
> > +???????trace_clk_set_phase(core, degrees);
> > ?
> > ????????if (core->ops->set_phase)
> > ????????????????ret = core->ops->set_phase(core->hw, degrees);
> > @@ -1952,7 +2114,15 @@ int clk_set_phase(struct clk *clk, int degrees)
> > ????????????????degrees += 360;
> > ?
> > ????????clk_prepare_lock();
> > +
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_unprotect(clk->core);
> > +
> > ????????ret = clk_core_set_phase_nolock(clk->core, degrees);
> > +
> > +???????if (clk->protect_count)
> > +???????????????clk_core_rate_protect(clk->core);
> > +
> > ????????clk_prepare_unlock();
> > ?
> > ????????return ret;
> > @@ -2039,11 +2209,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,
> > @@ -2065,8 +2236,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();
> > ?
> > @@ -2101,6 +2272,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));
> > @@ -2231,6 +2403,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)
> > @@ -2794,6 +2971,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();
> > @@ -2952,6 +3134,17 @@ void __clk_put(struct clk *clk)
> > ?
> > ????????clk_prepare_lock();
> > ?
> > +???????/*
> > +????????* Before calling clk_put, all calls to clk_rate_protect from a
> > given
> > +????????* user must be balanced with calls to clk_rate_unprotect and by
> > that
> > +????????* same user
> > +????????*/
> > +???????WARN_ON(clk->protect_count);
> > +
> > +???????/* We voiced our concern, let's sanitize the situation */
> > +???????for (; clk->protect_count; clk->protect_count--)
> > +???????????????clk_core_rate_unprotect(clk->core);
> > +
> > ????????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..ebd7df5f375f 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_rate_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 024cd07870d0..85d73e02df40 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_rate_protect - inform the system when the clock rate must be
> > protected.
> > + * @clk: clock source
> > + *
> > + * This function informs the system that the consumer protecting the clock
> > + * depends on the rate of the clock source and can't tolerate any glitches
> > + * introduced by further clock rate change or re-parenting of the clock
> > source.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_rate_protect(struct clk *clk);
> > +
> > +/**
> > + * clk_rate_unprotect - release the protection of the clock source.
> > + * @clk: clock source
> > + *
> > + * This function informs the system that the consumer previously protecting
> > the
> > + * clock rate can now deal with other consumer altering the clock source
> > rate
> > + *
> > + * The caller must balance the number of rate_protect and rate_unprotect
> > calls.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_rate_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.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at??http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/11] clk: move CLK_SET_RATE_GATE protection from prepare to enable

[Jerome Brunet]

On Sun, 2017-05-21 at 23:59 +0200, Jerome Brunet wrote:
> CLK_SET_RATE_GATE name suggest that the rate can be set when the provider
> is gated (disabled). With the current implementation, the rate can only be
> set when the provider is unprepared, while it should be allowed to set a
> prepared and disable provider.
> Fix this by moving the rate protection mechanism in the enable/disable
> core functions
> 

I'll drop this patch in the v3.
The protection count should only be touch while holding the prepare_lock.
This was disaster waiting to happen ... :(

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 6ee5fc59cf1f..e6e5048ce186 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -491,9 +491,6 @@ static void clk_core_unprepare(struct clk_core *core)
>  	if (WARN_ON(core->prepare_count == 1 && core->flags &
> CLK_IS_CRITICAL))
>  		return;
>  
> -	if (core->flags & CLK_SET_RATE_GATE)
> -		clk_core_rate_unprotect(core);
> -
>  	if (--core->prepare_count > 0)
>  		return;
>  
> @@ -564,14 +561,6 @@ static int clk_core_prepare(struct clk_core *core)
>  
>  	core->prepare_count++;
>  
> -	/*
> -	 * CLK_SET_RATE_GATE is a special case of clock protection
> -	 * Instead of a consumer protection, the provider is protecting
> -	 * itself when prepared
> -	 */
> -	if (core->flags & CLK_SET_RATE_GATE)
> -		clk_core_rate_protect(core);
> -
>  	return 0;
>  }
>  
> @@ -716,6 +705,9 @@ static void clk_core_disable(struct clk_core *core)
>  	if (WARN_ON(core->enable_count == 1 && core->flags &
> CLK_IS_CRITICAL))
>  		return;
>  
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_unprotect(core);
> +
>  	if (--core->enable_count > 0)
>  		return;
>  
> @@ -791,6 +783,15 @@ static int clk_core_enable(struct clk_core *core)
>  	}
>  
>  	core->enable_count++;
> +
> +	/*
> +	 * CLK_SET_RATE_GATE is a special case of clock protection
> +	 * Instead of a consumer protection, the provider is protecting
> +	 * itself when enabled
> +	 */
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_protect(core);
> +
>  	return 0;
>  }
>  

[PATCH v2 11/11] clk: move CLK_SET_RATE_GATE protection from prepare to enable

[Jerome Brunet]

On Sun, 2017-05-21 at 23:59 +0200, Jerome Brunet wrote:
> CLK_SET_RATE_GATE name suggest that the rate can be set when the provider
> is gated (disabled). With the current implementation, the rate can only be
> set when the provider is unprepared, while it should be allowed to set a
> prepared and disable provider.
> Fix this by moving the rate protection mechanism in the enable/disable
> core functions
> 

I'll drop this patch in the v3.
The protection count should only be touch while holding the prepare_lock.
This was disaster waiting to happen ... :(

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> ?drivers/clk/clk.c | 23 ++++++++++++-----------
> ?1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 6ee5fc59cf1f..e6e5048ce186 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -491,9 +491,6 @@ static void clk_core_unprepare(struct clk_core *core)
> ?	if (WARN_ON(core->prepare_count == 1 && core->flags &
> CLK_IS_CRITICAL))
> ?		return;
> ?
> -	if (core->flags & CLK_SET_RATE_GATE)
> -		clk_core_rate_unprotect(core);
> -
> ?	if (--core->prepare_count > 0)
> ?		return;
> ?
> @@ -564,14 +561,6 @@ static int clk_core_prepare(struct clk_core *core)
> ?
> ?	core->prepare_count++;
> ?
> -	/*
> -	?* CLK_SET_RATE_GATE is a special case of clock protection
> -	?* Instead of a consumer protection, the provider is protecting
> -	?* itself when prepared
> -	?*/
> -	if (core->flags & CLK_SET_RATE_GATE)
> -		clk_core_rate_protect(core);
> -
> ?	return 0;
> ?}
> ?
> @@ -716,6 +705,9 @@ static void clk_core_disable(struct clk_core *core)
> ?	if (WARN_ON(core->enable_count == 1 && core->flags &
> CLK_IS_CRITICAL))
> ?		return;
> ?
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_unprotect(core);
> +
> ?	if (--core->enable_count > 0)
> ?		return;
> ?
> @@ -791,6 +783,15 @@ static int clk_core_enable(struct clk_core *core)
> ?	}
> ?
> ?	core->enable_count++;
> +
> +	/*
> +	?* CLK_SET_RATE_GATE is a special case of clock protection
> +	?* Instead of a consumer protection, the provider is protecting
> +	?* itself when enabled
> +	?*/
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_protect(core);
> +
> ?	return 0;
> ?}
> ?

Re: [PATCH v2 01/11] clk: take the prepare lock out of clk_core_set_parent

[Jerome Brunet]

On Thu, 2017-05-25 at 11:54 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:48)
> > 
> > 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>
> 
> Applied to clk-next-protect for testing.
> 

Noted, I'll base the v3 on top of this branch

> Regards,
> Mike
> 
> > ---
> >  drivers/clk/clk.c | 39 ++++++++++++++++++---------------------
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index fc58c52a26b4..f5c371532509 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk
> > *parent)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_has_parent);
> >  
> > -static int clk_core_set_parent(struct clk_core *core, struct clk_core
> > *parent)
> > +static int clk_core_set_parent_nolock(struct clk_core *core,
> > +                                     struct clk_core *parent)
> >  {
> >         int ret = 0;
> >         int p_index = 0;
> > @@ -1796,23 +1797,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) {
> > @@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> >                 if (p_index < 0) {
> >                         pr_debug("%s: clk %s can not be parent of clk %s\n",
> >                                         __func__, parent->name, core->name);
> > -                       ret = p_index;
> > -                       goto out;
> > +                       return p_index;
> >                 }
> >                 p_rate = parent->rate;
> >         }
> > @@ -1831,7 +1824,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);
> > @@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> >                 __clk_recalc_accuracies(core);
> >         }
> >  
> > -out:
> > -       clk_prepare_unlock();
> > -
> >         return ret;
> >  }
> >  
> > @@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core
> > *core, struct clk_core *parent)
> >   */
> >  int clk_set_parent(struct clk *clk, struct clk *parent)
> >  {
> > +       int ret;
> > +
> >         if (!clk)
> >                 return 0;
> >  
> > -       return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> > +       clk_prepare_lock();
> > +       ret = clk_core_set_parent_nolock(clk->core,
> > +                                        parent ? parent->core : NULL);
> > +       clk_prepare_unlock();
> > +
> > +       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(clk_set_parent);
> >  
> > @@ -2753,7 +2750,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_nolock(child, NULL);
> >         }
> >  
> >         hlist_del_init(&clk->core->child_node);
> > -- 
> > 2.9.4
> > 

[PATCH v2 01/11] clk: take the prepare lock out of clk_core_set_parent

[Jerome Brunet]

On Thu, 2017-05-25 at 11:54 -0700, Michael Turquette wrote:
> Quoting Jerome Brunet (2017-05-21 14:59:48)
> > 
> > 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>
> 
> Applied to clk-next-protect for testing.
> 

Noted, I'll base the v3 on top of this branch

> Regards,
> Mike
> 
> > ---
> > ?drivers/clk/clk.c | 39 ++++++++++++++++++---------------------
> > ?1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index fc58c52a26b4..f5c371532509 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1787,7 +1787,8 @@ bool clk_has_parent(struct clk *clk, struct clk
> > *parent)
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_has_parent);
> > ?
> > -static int clk_core_set_parent(struct clk_core *core, struct clk_core
> > *parent)
> > +static int clk_core_set_parent_nolock(struct clk_core *core,
> > +?????????????????????????????????????struct clk_core *parent)
> > ?{
> > ????????int ret = 0;
> > ????????int p_index = 0;
> > @@ -1796,23 +1797,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) {
> > @@ -1820,8 +1814,7 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> > ????????????????if (p_index < 0) {
> > ????????????????????????pr_debug("%s: clk %s can not be parent of clk %s\n",
> > ????????????????????????????????????????__func__, parent->name, core->name);
> > -???????????????????????ret = p_index;
> > -???????????????????????goto out;
> > +???????????????????????return p_index;
> > ????????????????}
> > ????????????????p_rate = parent->rate;
> > ????????}
> > @@ -1831,7 +1824,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);
> > @@ -1844,9 +1837,6 @@ static int clk_core_set_parent(struct clk_core *core,
> > struct clk_core *parent)
> > ????????????????__clk_recalc_accuracies(core);
> > ????????}
> > ?
> > -out:
> > -???????clk_prepare_unlock();
> > -
> > ????????return ret;
> > ?}
> > ?
> > @@ -1869,10 +1859,17 @@ static int clk_core_set_parent(struct clk_core
> > *core, struct clk_core *parent)
> > ? */
> > ?int clk_set_parent(struct clk *clk, struct clk *parent)
> > ?{
> > +???????int ret;
> > +
> > ????????if (!clk)
> > ????????????????return 0;
> > ?
> > -???????return clk_core_set_parent(clk->core, parent ? parent->core : NULL);
> > +???????clk_prepare_lock();
> > +???????ret = clk_core_set_parent_nolock(clk->core,
> > +????????????????????????????????????????parent ? parent->core : NULL);
> > +???????clk_prepare_unlock();
> > +
> > +???????return ret;
> > ?}
> > ?EXPORT_SYMBOL_GPL(clk_set_parent);
> > ?
> > @@ -2753,7 +2750,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_nolock(child, NULL);
> > ????????}
> > ?
> > ????????hlist_del_init(&clk->core->child_node);
> > --?
> > 2.9.4
> >