clk: implement clock locking mechanism

drivers/clk/clk.c +1 -4

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

drivers/clk/clk.c +0 -0

 }
 EXPORT_SYMBOL_GPL(clk_set_parent);

=

drivers/clk/clk.c +1 -1

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

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

-- =

2.9.3
=20

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 19:33:24)

Quoted 16 lines

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 <[email protected]>

---
?drivers/clk/clk.c | 38 ++++++++++++++++++++------------------ ?1 file changed, 20 insertions(+), 18 deletions(-)

drivers/clk/clk.c +0 -0

index 67201f67a14a..e77f03a47da6 100644

drivers/clk/clk.c static int clk_core_set_parent(struct clk_core · line 1794

Quoted 1 line

drivers/clk/clk.c +0 -0

@@ -1794,23 +1794,16 @@ static int clk_core_set_parent(struct clk_core
 *core, struct clk_core *parent)

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

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

Quoted 53 lines

drivers/clk/clk.c +0 -0

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

drivers/clk/clk.c +1 -2

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

????????}

drivers/clk/clk.c +0 -0

@@ -1829,7 +1821,7 @@ static int clk_core_set_parent(struct clk_core *core,
 struct clk_core *parent)

?
????????/* abort if a driver objects */ ????????if (ret & NOTIFY_STOP_MASK) -???????????????goto out; +???????????????return ret;
?
????????/* do the re-parent */
????????ret = __clk_set_parent(core, parent, p_index);

drivers/clk/clk.c +0 -0

@@ -1842,7 +1834,16 @@ static int clk_core_set_parent(struct clk_core *core,
 struct clk_core *parent)
 ????????????????__clk_recalc_accuracies(core);

Quoted 4 lines

drivers/clk/clk.c +0 -0

 ????????}

?
-out:
+???????return ret;

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

drivers/clk/clk.c static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)

Quoted 10 lines

drivers/clk/clk.c +9 -0

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

Above line should be nolock.

Quoted 11 lines

????????clk_prepare_unlock();
?
????????return ret;

drivers/clk/clk.c +0 -0

@@ -1870,7 +1871,8 @@ int clk_set_parent(struct clk *clk, struct clk
 *parent)
 ????????if (!clk)
 ????????????????return 0;

?
-???????return clk_core_set_parent(clk->core, parent ? parent->core : NULL); +???????return clk_core_set_parent_lock(clk->core, +???????????????????????????????????????parent ? parent->core : NULL);

We can avoid this line by using the nolock pattern.

Basic premise of the patch is OK though.

Regards,
Mike

Quoted 15 lines

?}
?EXPORT_SYMBOL_GPL(clk_set_parent);
?

drivers/clk/clk.c +0 -0

@@ -2720,7 +2722,7 @@ void clk_unregister(struct clk *clk)

????????????????/* Reparent all children to the orphan list. */ ????????????????hlist_for_each_entry_safe(child, t, &clk->core->children, ??????????????????????????????????????????child_node) -???????????????????????clk_core_set_parent(child, NULL); +???????????????????????clk_core_set_parent_lock(child, NULL); ????????}
?
????????hlist_del_init(&clk->core->child_node);
--?
2.9.3

drivers/clk/clk.c +16 -0

+{
+       int ret =3D -EINVAL;
+
+       if (!core)
+               return 0;
+
+       trace_clk_set_phase(clk->core, degrees);
+
+       if (core->ops->set_phase)
+               ret =3D 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

drivers/clk/clk.c +1 -1

@@ -1898,7 +1915,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
  */
 int clk_set_phase(struct clk *clk, int degrees)
 {
-       int ret =3D -EINVAL;
+       int ret;
 =
 
        if (!clk)

                return 0;

drivers/clk/clk.c +1 -11

@@ -1909,17 +1926,7 @@ int clk_set_phase(struct clk *clk, int degrees)
                degrees +=3D 360;
 =
 
        clk_prepare_lock();
-
-       trace_clk_set_phase(clk->core, degrees);
-
-       if (clk->core->ops->set_phase)
-               ret =3D clk->core->ops->set_phase(clk->core->hw, degrees);
-
-       trace_clk_set_phase_complete(clk->core, degrees);
-
-       if (!ret)
-               clk->core->phase =3D degrees;
-
+       ret =3D clk_core_set_phase(clk->core, degrees);
        clk_prepare_unlock();
 =

        return ret;

-- =

2.9.3
=20

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

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

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

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

drivers/clk/clk.c +44 -34

index 57982a06dbce..fa77a1841e0f 100644
@@ -833,16 +833,34 @@ static int clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
-static int clk_core_round_rate_nolock(struct clk_core *core,
-                                     struct clk_rate_request *req)
+static int clk_core_determine_round(struct clk_core *core,
+                                   struct clk_rate_request *req)
 {
-       struct clk_core *parent;
        long rate;
 
-       lockdep_assert_held(&prepare_lock);
+       if (core->ops->determine_rate) {
+               return core->ops->determine_rate(core->hw, req);
+       } else if (core->ops->round_rate) {
+               rate = core->ops->round_rate(core->hw, req->rate,
+                                            &req->best_parent_rate);
+               if (rate < 0)
+                       return rate;
 
-       if (!core)
-               return 0;
+               req->rate = rate;
+       } else {
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+static void clk_core_init_rate_req(struct clk_core *core,
+                                  struct clk_rate_request *req)
+{
+       struct clk_core *parent;
+
+       if (WARN_ON(!core || !req))
+               return;
 
        parent = core->parent;
        if (parent) {

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

@@ -1354,36 +1374,26 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
        clk_core_get_boundaries(core, &min_rate, &max_rate);
 
        /* find the closest rate and parent clk/rate */
-       if (core->ops->determine_rate) {
+       if (core->ops->determine_rate || core->ops->round_rate) {
                struct clk_rate_request req;
 
                req.rate = rate;
                req.min_rate = min_rate;
                req.max_rate = max_rate;
-               if (parent) {
-                       req.best_parent_hw = parent->hw;
-                       req.best_parent_rate = parent->rate;
-               } else {
-                       req.best_parent_hw = NULL;
-                       req.best_parent_rate = 0;
-               }
 
-               ret = core->ops->determine_rate(core->hw, &req);
+               clk_core_init_rate_req(core, req);
+
+               ret = clk_core_determine_round(core, &req);
                if (ret < 0)
                        return NULL;
 
                best_parent_rate = req.best_parent_rate;
                new_rate = req.rate;
                parent = req.best_parent_hw ? req.best_parent_hw->core : NULL;
-       } else if (core->ops->round_rate) {
-               ret = core->ops->round_rate(core->hw, rate,
-                                           &best_parent_rate);
-               if (ret < 0)
-                       return NULL;
 
-               new_rate = ret;
                if (new_rate < min_rate || new_rate > max_rate)
                        return NULL;
+
        } else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
                /* pass-through clock without adjustable parent */
                core->new_rate = core->rate;

Signature · 1 line

2.9.3

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 <[email protected]>

---
 drivers/clk/clk.c            | 177 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 +++++++
 3 files changed, 197 insertions(+), 10 deletions(-)

drivers/clk/clk.c +26 -0

index fa77a1841e0f..69db8cc15063 100644
@@ -60,6 +60,7 @@ struct clk_core {
        bool                    orphan;
        unsigned int            enable_count;
        unsigned int            prepare_count;
+       unsigned int            protect_count;
        unsigned long           min_rate;
        unsigned long           max_rate;
        unsigned long           accuracy;

@@ -84,6 +85,7 @@ struct clk {
        const char *con_id;
        unsigned long min_rate;
        unsigned long max_rate;
+       unsigned long protect_ucount;
        struct hlist_node clks_node;
 };
 

@@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
        return core->ops->is_prepared(core->hw);
 }
 
+static bool clk_core_is_protected(struct clk_core *core)
+{
+       return core->protect_count;
+}
+
 static bool clk_core_is_enabled(struct clk_core *core)
 {
        /*

@@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
        return clk_core_is_prepared(hw->core);
 }
 
+bool clk_hw_is_protected(const struct clk_hw *hw)
+{
+       return clk_core_is_protected(hw->core);
+}
+
 bool clk_hw_is_enabled(const struct clk_hw *hw)
 {
        return clk_core_is_enabled(hw->core);

@@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+static void clk_core_unprotect(struct clk_core *core)
+{
+       lockdep_assert_held(&prepare_lock);
+
+       if (!core)
+               return;
+
+       if (WARN_ON(core->protect_count == 0))
+               return;
+
+       if (--core->protect_count > 0)
+               return;
+
+       clk_core_unprotect(core->parent);

drivers/clk/clk.c +37 -0

+ */
+void clk_unprotect(struct clk *clk)
+{
+       if (!clk)
+               return;
+
+       clk_prepare_lock();
+
+       if (WARN_ON(clk->protect_ucount <= 0)) {
+               /*
+                * There is something wrong with this consumer protect count.
+                * Stop here before messing with the provider
+                */
+               clk_prepare_unlock();
+               return;
+       }
+
+       clk_core_unprotect(clk->core);
+       clk->protect_ucount--;
+       clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_unprotect);
+
+static void clk_core_protect(struct clk_core *core)
+{
+       lockdep_assert_held(&prepare_lock);
+
+       if (!core)
+               return;
+
+       if (core->protect_count == 0)
+               clk_core_protect(core->parent);
+
+       core->protect_count++;
+}
+
+/**

drivers/clk/clk.c +12 -0

+void clk_protect(struct clk *clk)
+{
+       if (!clk)
+               return;
+
+       clk_prepare_lock();
+       clk_core_protect(clk->core);
+       clk->protect_ucount++;
+       clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_protect);
+
 static void clk_core_disable(struct clk_core *core)
 {
        lockdep_assert_held(&enable_lock);

drivers/clk/clk.c +66 -8

+
        ret = clk_core_set_rate_nolock(clk->core, rate);
 
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 
        clk_prepare_lock();
 
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
        if (min != clk->min_rate || max != clk->max_rate) {
                clk->min_rate = min;
                clk->max_rate = max;
                ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
        }
 
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
        if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
                return -EBUSY;
 
+       if (clk_core_is_protected(core))
+               return -EBUSY;
+
        /* try finding the new parent index */
        if (parent) {
                p_index = clk_fetch_parent_index(core, parent);

@@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core *core,
  */
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
+       int ret;
+
        if (!clk)
                return 0;
 
-       return clk_core_set_parent_lock(clk->core,
-                                       parent ? parent->core : NULL);
+       clk_prepare_lock();
+
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
+       ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
+       clk_prepare_unlock();
+
+       return ret;
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 

@@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core, int degrees)
        if (!core)
                return 0;
 
-       trace_clk_set_phase(clk->core, degrees);
+       if (clk_core_is_protected(core))
+               return -EBUSY;
+
+       trace_clk_set_phase(core, degrees);
 
        if (core->ops->set_phase)
                ret = core->ops->set_phase(core->hw, degrees);

@@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
                degrees += 360;
 
        clk_prepare_lock();
+
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
        ret = clk_core_set_phase(clk->core, degrees);
+
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
        if (!c)
                return;
 
-       seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
+       seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
                   level * 3 + 1, "",
                   30 - level * 3, c->name,
-                  c->enable_count, c->prepare_count, clk_core_get_rate(c),
-                  clk_core_get_accuracy(c), clk_core_get_phase(c));
+                  c->enable_count, c->prepare_count, c->protect_count,
+                  clk_core_get_rate(c), clk_core_get_accuracy(c),
+                  clk_core_get_phase(c));
 }
 
 static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,

@@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
        struct clk_core *c;
        struct hlist_head **lists = (struct hlist_head **)s->private;
 
-       seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
-       seq_puts(s, "----------------------------------------------------------------------------------------\n");
+       seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
+       seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
 
        clk_prepare_lock();
 

@@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
        seq_printf(s, "\"%s\": { ", c->name);
        seq_printf(s, "\"enable_count\": %d,", c->enable_count);
        seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
+       seq_printf(s, "\"protect_count\": %d,", c->protect_count);
        seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
        seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
        seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));

@@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
        if (!d)
                goto err_out;
 
+       d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
+                       (u32 *)&core->protect_count);
+       if (!d)
+               goto err_out;
+
        d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
                        (u32 *)&core->notifier_count);
        if (!d)

@@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
        if (clk->core->prepare_count)
                pr_warn("%s: unregistering prepared clock: %s\n",
                                        __func__, clk->core->name);
+
+       if (clk->core->protect_count)
+               pr_warn("%s: unregistering protected clock: %s\n",
+                                       __func__, clk->core->name);
+
        kref_put(&clk->core->ref, __clk_release);
 unlock:
        clk_prepare_unlock();

@@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
 
        clk_prepare_lock();
 
+       /* Protect count not balanced: warn and sanitize */
+       if (clk->protect_ucount) {
+               pr_warn("%s: releasing protected clock: %s\n",
+                                       __func__, clk->core->name);
+
+               for (; clk->protect_ucount; clk->protect_ucount--)
+                       clk_core_unprotect(clk->core);
+       }
+
        hlist_del(&clk->clks_node);
        if (clk->min_rate > clk->core->req_rate ||
            clk->max_rate < clk->core->req_rate)

include/linux/clk-provider.h +1 -0

index a428aec36ace..705a158d9b8f 100644
@@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 unsigned long __clk_get_flags(struct clk *clk);
 unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_protected(const struct clk_hw *hw);
 bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
 struct clk *__clk_lookup(const char *name);

include/linux/clk.h +29 -0

index e9d36b3e49de..90b72ead4411 100644
@@ -265,6 +265,30 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
  */
 struct clk *devm_get_clk_from_child(struct device *dev,
                                    struct device_node *np, const char *con_id);
+/**
+ * clk_protect - inform the system when the clock source should be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_protect(struct clk *clk);
+
+/**
+ * clk_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting the
+ * clock source can now deal with other consumer altering the clock source.
+ *
+ * The caller must balance the number of protect and unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprotect(struct clk *clk);
 
 /**
  * clk_enable - inform the system when the clock source should be running.

@@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+
+static inline void clk_protect(struct clk *clk) {}
+
+static inline void clk_unprotect(struct clk *clk) {}
+
 static inline int clk_enable(struct clk *clk)
 {
        return 0;

Signature · 1 line

2.9.3

Hi Jerome,

Quoting Jerome Brunet (2017-03-21 11:33:27)

Quoted 82 lines

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 <[email protected]>

---
?drivers/clk/clk.c????????????| 177
++++++++++++++++++++++++++++++++++++++++---
?include/linux/clk-provider.h |???1 + ?include/linux/clk.h??????????|??29 +++++++ ?3 files changed, 197 insertions(+), 10 deletions(-)

drivers/clk/clk.c +2 -0

index fa77a1841e0f..69db8cc15063 100644
@@ -60,6 +60,7 @@ struct clk_core {
 ????????bool????????????????????orphan;
 ????????unsigned int????????????enable_count;
 ????????unsigned int????????????prepare_count;
+???????unsigned int????????????protect_count;
 ????????unsigned long???????????min_rate;
 ????????unsigned long???????????max_rate;
 ????????unsigned long???????????accuracy;

@@ -84,6 +85,7 @@ struct clk {
 ????????const char *con_id;
 ????????unsigned long min_rate;
 ????????unsigned long max_rate;
+???????unsigned long protect_ucount;
 ????????struct hlist_node clks_node;
 ?};

?

drivers/clk/clk.c +0 -0

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

?

drivers/clk/clk.c +5 -0

+static bool clk_core_is_protected(struct clk_core *core)
+{
+???????return core->protect_count;
+}
+

?static bool clk_core_is_enabled(struct clk_core *core) ?{
????????/*

drivers/clk/clk.c +0 -0

@@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
 ????????return clk_core_is_prepared(hw->core);
 ?}

?

drivers/clk/clk.c +5 -0

+bool clk_hw_is_protected(const struct clk_hw *hw)
+{
+???????return clk_core_is_protected(hw->core);
+}
+

?bool clk_hw_is_enabled(const struct clk_hw *hw)
?{
????????return clk_core_is_enabled(hw->core);

drivers/clk/clk.c +0 -0

@@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
 ?}
 ?EXPORT_SYMBOL_GPL(clk_prepare);

?

drivers/clk/clk.c +21 -0

+static void clk_core_unprotect(struct clk_core *core)
+{
+???????lockdep_assert_held(&prepare_lock);
+
+???????if (!core)
+???????????????return;
+
+???????if (WARN_ON(core->protect_count == 0))
+???????????????return;
+
+???????if (--core->protect_count > 0)
+???????????????return;
+
+???????clk_core_unprotect(core->parent);
+}
+
+/**
+ * clk_unprotect - unprotect a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect shall be used when a consumer no longer depends on the

Quoted 5 lines

clock
+ * rate and can tolerate glitches. As with clk_unprepare and clk_enable, calls
+ * to clk_unprotect must be balanced with clk_protect.
+ * clk_protect may sleep

Maybe:

"""
clk_unprotect completes a critical section during which the clock consumer cannot tolerate any change to the clock rate. If no other clock consumers have protected clocks in the parent chain, then calls to this function will allow the clocks in the parent chain to change rates freely.

Unlike the clk_set_rate_range method, which allows the rate to change within a given range, protected clocks cannot have their rate changed, either directly or indirectly due to changes further up the parent chain of clocks.

Calls to clk_unprotect must be balanced with calls to clk_protect. Calls to this function may sleep, and do not return error status. """

Quoted 43 lines

+ */

drivers/clk/clk.c +10 -0

+void clk_unprotect(struct clk *clk)
+{
+???????if (!clk)
+???????????????return;
+
+???????clk_prepare_lock();
+
+???????if (WARN_ON(clk->protect_ucount <= 0)) {
+???????????????/*
+????????????????* There is something wrong with this consumer protect

count.
+????????????????* Stop here before messing with the provider +????????????????*/ +???????????????clk_prepare_unlock(); +???????????????return;
+???????}

drivers/clk/clk.c +25 -0

+
+???????clk_core_unprotect(clk->core);
+???????clk->protect_ucount--;
+???????clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_unprotect);
+
+static void clk_core_protect(struct clk_core *core)
+{
+???????lockdep_assert_held(&prepare_lock);
+
+???????if (!core)
+???????????????return;
+
+???????if (core->protect_count == 0)
+???????????????clk_core_protect(core->parent);
+
+???????core->protect_count++;
+}
+
+/**
+ * clk_protect - protect a clock source
+ * @clk: the clk being protected
+ *
+ * clk_protect can be used when a consumer depends on the clock rate and

Quoted 8 lines

can't
+ * tolerate any glitches. The consumer protecting the clock can still make
+ * adjustment to clock, if it is the only one protecting the clock. Other
+ * consumers can still use the clock but won't be able to adjust the rate or
+ * reparent the clock while it is protected.
+ * clk_protect may sleep.
+ */

Maybe:

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

Unlike the clk_set_rate_range method, which allows the rate to change within a given range, protected clocks cannot have their rate changed, either directly or indirectly due to changes further up the parent chain of clocks.

Calls to clk_protect should be balanced with calls to clk_unprotect. Calls to this function may sleep, and do not return error status. """

Quoted 20 lines

drivers/clk/clk.c +12 -0

+void clk_protect(struct clk *clk)
+{
+???????if (!clk)
+???????????????return;
+
+???????clk_prepare_lock();
+???????clk_core_protect(clk->core);
+???????clk->protect_ucount++;
+???????clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_protect);
+

?static void clk_core_disable(struct clk_core *core) ?{ ????????lockdep_assert_held(&enable_lock);

drivers/clk/clk.c +0 -0

@@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core
 *core,
 ?{
 ????????long rate;

Quoted 4 lines

?
-???????if (core->ops->determine_rate) {
+???????if (clk_core_is_protected(core)) {
+???????????????req->rate = core->rate;

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

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

	clk_protect(clk);

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

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

	ret = clk_set_rate(clk, 5678);

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

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

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

Quoted 9 lines

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

drivers/clk/clk.c +0 -0

@@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct
 clk_core *core,
 ????????????????req.min_rate = min_rate;
 ????????????????req.max_rate = max_rate;

Quoted 3 lines

? -???????????????clk_core_init_rate_req(core, req); +???????????????clk_core_init_rate_req(core, &req);

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

Quoted 7 lines

?
????????????????ret = clk_core_determine_round(core, &req); ????????????????if (ret < 0)

drivers/clk/clk.c +0 -0

@@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)

????????/* prevent racing with updates to the clock topology */ ????????clk_prepare_lock();

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

Quoted 4 lines

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

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

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

if clk_is_protected(clk) AND it is only protect by THIS clk:

Best regards,
Mike

Quoted 251 lines

+
????????ret = clk_core_set_rate_nolock(clk->core, rate); ?
+???????if (clk->protect_ucount) +???????????????clk_core_protect(clk->core);
+
????????clk_prepare_unlock();
?
????????return ret;

drivers/clk/clk.c +0 -0

@@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
 long min, unsigned long max)

?
????????clk_prepare_lock();
?
+???????if (clk->protect_ucount) +???????????????clk_core_unprotect(clk->core);
+
????????if (min != clk->min_rate || max != clk->max_rate) { ????????????????clk->min_rate = min; ????????????????clk->max_rate = max;
????????????????ret = clk_core_set_rate_nolock(clk->core, clk->core-

Quoted 1 line

req_rate);

????????}
?
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+
????????clk_prepare_unlock();
?
????????return ret;

drivers/clk/clk.c +0 -0

@@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core,
 struct clk_core *parent)
 ????????if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
 ????????????????return -EBUSY;

?
+???????if (clk_core_is_protected(core)) +???????????????return -EBUSY;
+
????????/* try finding the new parent index */ ????????if (parent) {
????????????????p_index = clk_fetch_parent_index(core, parent);

drivers/clk/clk.c +0 -0

@@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core
 *core,

? */
?int clk_set_parent(struct clk *clk, struct clk *parent)
?{
+???????int ret;
+
????????if (!clk)
????????????????return 0;
?
-???????return clk_core_set_parent_lock(clk->core, -???????????????????????????????????????parent ? parent->core : NULL); +???????clk_prepare_lock();

drivers/clk/clk.c +12 -0

+
+???????if (clk->protect_ucount)
+???????????????clk_core_unprotect(clk->core);
+
+???????ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+
+???????clk_prepare_unlock();
+
+???????return ret;

?}
?EXPORT_SYMBOL_GPL(clk_set_parent);
?

drivers/clk/clk.c +0 -0

@@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core,
 int degrees)
 ????????if (!core)
 ????????????????return 0;

?
-???????trace_clk_set_phase(clk->core, degrees); +???????if (clk_core_is_protected(core)) +???????????????return -EBUSY;
+
+???????trace_clk_set_phase(core, degrees);
?
????????if (core->ops->set_phase) ????????????????ret = core->ops->set_phase(core->hw, degrees);

drivers/clk/clk.c +0 -0

@@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
 ????????????????degrees += 360;

?
????????clk_prepare_lock();

drivers/clk/clk.c +4 -0

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

????????ret = clk_core_set_phase(clk->core, degrees);

drivers/clk/clk.c +4 -0

+
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+

????????clk_prepare_unlock();
?
????????return ret;

drivers/clk/clk.c +0 -0

@@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s,
 struct clk_core *c,
 ????????if (!c)
 ????????????????return;

?
-???????seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n", +???????seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n", ???????????????????level * 3 + 1, "",
???????????????????30 - level * 3, c->name, -??????????????????c->enable_count, c->prepare_count, clk_core_get_rate(c), -??????????????????clk_core_get_accuracy(c), clk_core_get_phase(c)); +??????????????????c->enable_count, c->prepare_count, c->protect_count, +??????????????????clk_core_get_rate(c), clk_core_get_accuracy(c), +??????????????????clk_core_get_phase(c));
?}
?
?static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,

drivers/clk/clk.c +0 -0

@@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void
 *data)
 ????????struct clk_core *c;
 ????????struct hlist_head **lists = (struct hlist_head **)s->private;

?
-???????seq_puts(s, "???clock?????????????????????????enable_cnt??prepare_cnt????????rate???accu racy???phase\n");
-???????seq_puts(s, "------------------------------------------------------- ---------------------------------\n");
+???????seq_puts(s, "???clock?????????????????????????enable_cnt??prepare_cnt??protect_cnt?????? ??rate???accuracy???phase\n");
+???????seq_puts(s, "------------------------------------------------------- ---------------------------------------------\n");
?
????????clk_prepare_lock();
?

drivers/clk/clk.c +1 -0

@@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct
 clk_core *c, int level)
 ????????seq_printf(s, "\"%s\": { ", c->name);
 ????????seq_printf(s, "\"enable_count\": %d,", c->enable_count);
 ????????seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
+???????seq_printf(s, "\"protect_count\": %d,", c->protect_count);
 ????????seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
 ????????seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));

????????seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));

drivers/clk/clk.c +0 -0

@@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core
 *core, struct dentry *pdentry)
 ????????if (!d)
 ????????????????goto err_out;

?
+???????d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry, +???????????????????????(u32 *)&core->protect_count); +???????if (!d)
+???????????????goto err_out;
+
????????d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry, ????????????????????????(u32 *)&core->notifier_count); ????????if (!d)

drivers/clk/clk.c +5 -0

@@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
 ????????if (clk->core->prepare_count)
 ????????????????pr_warn("%s: unregistering prepared clock: %s\n",
 ????????????????????????????????????????__func__, clk->core->name);
+
+???????if (clk->core->protect_count)
+???????????????pr_warn("%s: unregistering protected clock: %s\n",
+???????????????????????????????????????__func__, clk->core->name);
+
 ????????kref_put(&clk->core->ref, __clk_release);

?unlock:
????????clk_prepare_unlock();

drivers/clk/clk.c +0 -0

@@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)

?
????????clk_prepare_lock();
?
+???????/* Protect count not balanced: warn and sanitize */ +???????if (clk->protect_ucount) { +???????????????pr_warn("%s: releasing protected clock: %s\n", +???????????????????????????????????????__func__, clk->core->name);

drivers/clk/clk.c +5 -0

+
+???????????????for (; clk->protect_ucount; clk->protect_ucount--)
+???????????????????????clk_core_unprotect(clk->core);
+???????}
+

????????hlist_del(&clk->clks_node); ????????if (clk->min_rate > clk->core->req_rate || ????????????clk->max_rate < clk->core->req_rate)

include/linux/clk-provider.h +1 -0

index a428aec36ace..705a158d9b8f 100644
@@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 ?unsigned long __clk_get_flags(struct clk *clk);
 ?unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 ?bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_protected(const struct clk_hw *hw);
 ?bool clk_hw_is_enabled(const struct clk_hw *hw);
 ?bool __clk_is_enabled(struct clk *clk);
 ?struct clk *__clk_lookup(const char *name);

include/linux/clk.h +0 -0

index e9d36b3e49de..90b72ead4411 100644
@@ -265,6 +265,30 @@ struct clk *devm_clk_get(struct device *dev, const char
 *id);

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

include/linux/clk.h +7 -0

+void clk_protect(struct clk *clk);
+
+/**
+ * clk_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting

the
+ * clock source can now deal with other consumer altering the clock source.
+ *
+ * The caller must balance the number of protect and unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprotect(struct clk *clk);
?
?/**
? * clk_enable - inform the system when the clock source should be running.

include/linux/clk.h +0 -0

@@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}

?
?static inline void devm_clk_put(struct device *dev, struct clk *clk) {} ?

include/linux/clk.h +5 -0

+
+static inline void clk_protect(struct clk *clk) {}
+
+static inline void clk_unprotect(struct clk *clk) {}
+

?static inline int clk_enable(struct clk *clk)
?{
????????return 0;
--?
2.9.3

Quoted 1 line

Quoting Jerome Brunet (2017-03-21 11:33:27)

Quoted 26 lines

Quoted 8 lines

(quoted diff) +0 -0

@@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core
 *core,
 ?{
 ????????long rate;

?
-???????if (core->ops->determine_rate) {
+???????if (clk_core_is_protected(core)) {
+???????????????req->rate = core->rate;

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

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

      clk_protect(clk);

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

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

      ret = clk_set_rate(clk, 5678);

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

Quoted 7 lines

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

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

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

Quoted 7 lines

Quoted 2 lines

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

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

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

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

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

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

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

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

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

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

It this what you meant by :

Quoted 1 line

if clk_is_protected(clk) AND it is only protect by THIS clk:

?

Cheers
Jerome

Quoted 256 lines

Best regards,
Mike

Quoted 251 lines

+
????????ret = clk_core_set_rate_nolock(clk->core, rate); ?
+???????if (clk->protect_ucount) +???????????????clk_core_protect(clk->core);
+
????????clk_prepare_unlock();
?
????????return ret;

(quoted diff) +0 -0

@@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned
 long min, unsigned long max)

?
????????clk_prepare_lock();
?
+???????if (clk->protect_ucount) +???????????????clk_core_unprotect(clk->core);
+
????????if (min != clk->min_rate || max != clk->max_rate) { ????????????????clk->min_rate = min; ????????????????clk->max_rate = max;
????????????????ret = clk_core_set_rate_nolock(clk->core, clk->core-

Quoted 1 line

req_rate);

????????}
?
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+
????????clk_prepare_unlock();
?
????????return ret;

(quoted diff) +0 -0

@@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core,
 struct clk_core *parent)
 ????????if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
 ????????????????return -EBUSY;

?
+???????if (clk_core_is_protected(core)) +???????????????return -EBUSY;
+
????????/* try finding the new parent index */ ????????if (parent) {
????????????????p_index = clk_fetch_parent_index(core, parent);

(quoted diff) +0 -0

@@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core
 *core,

? */
?int clk_set_parent(struct clk *clk, struct clk *parent)
?{
+???????int ret;
+
????????if (!clk)
????????????????return 0;
?
-???????return clk_core_set_parent_lock(clk->core, -???????????????????????????????????????parent ? parent->core : NULL); +???????clk_prepare_lock();

(quoted diff) +12 -0

+
+???????if (clk->protect_ucount)
+???????????????clk_core_unprotect(clk->core);
+
+???????ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+
+???????clk_prepare_unlock();
+
+???????return ret;

?}
?EXPORT_SYMBOL_GPL(clk_set_parent);
?

(quoted diff) +0 -0

@@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core,
 int degrees)
 ????????if (!core)
 ????????????????return 0;

?
-???????trace_clk_set_phase(clk->core, degrees); +???????if (clk_core_is_protected(core)) +???????????????return -EBUSY;
+
+???????trace_clk_set_phase(core, degrees);
?
????????if (core->ops->set_phase) ????????????????ret = core->ops->set_phase(core->hw, degrees);

(quoted diff) +0 -0

@@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
 ????????????????degrees += 360;

?
????????clk_prepare_lock();

drivers/clk/clk.c +4 -0

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

????????ret = clk_core_set_phase(clk->core, degrees);

drivers/clk/clk.c +4 -0

+
+???????if (clk->protect_ucount)
+???????????????clk_core_protect(clk->core);
+

????????clk_prepare_unlock();
?
????????return ret;

(quoted diff) +0 -0

@@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s,
 struct clk_core *c,
 ????????if (!c)
 ????????????????return;

?
-???????seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n", +???????seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n", ???????????????????level * 3 + 1, "",
???????????????????30 - level * 3, c->name, -??????????????????c->enable_count, c->prepare_count, clk_core_get_rate(c), -??????????????????clk_core_get_accuracy(c), clk_core_get_phase(c)); +??????????????????c->enable_count, c->prepare_count, c->protect_count, +??????????????????clk_core_get_rate(c), clk_core_get_accuracy(c), +??????????????????clk_core_get_phase(c));
?}
?
?static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,

(quoted diff) +0 -0

@@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void
 *data)
 ????????struct clk_core *c;
 ????????struct hlist_head **lists = (struct hlist_head **)s->private;

?
-???????seq_puts(s, "???clock?????????????????????????enable_cnt??prepare_cnt????????rate???accu racy???phase\n");
-???????seq_puts(s, "------------------------------------------------------- ---------------------------------\n");
+???????seq_puts(s, "???clock?????????????????????????enable_cnt??prepare_cnt??protect_cnt?????? ??rate???accuracy???phase\n");
+???????seq_puts(s, "------------------------------------------------------- ---------------------------------------------\n");
?
????????clk_prepare_lock();
?

(quoted diff) +1 -0

@@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct
 clk_core *c, int level)
 ????????seq_printf(s, "\"%s\": { ", c->name);
 ????????seq_printf(s, "\"enable_count\": %d,", c->enable_count);
 ????????seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
+???????seq_printf(s, "\"protect_count\": %d,", c->protect_count);
 ????????seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
 ????????seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));

????????seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));

(quoted diff) +0 -0

@@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core
 *core, struct dentry *pdentry)
 ????????if (!d)
 ????????????????goto err_out;

?
+???????d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry, +???????????????????????(u32 *)&core->protect_count); +???????if (!d)
+???????????????goto err_out;
+
????????d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry, ????????????????????????(u32 *)&core->notifier_count); ????????if (!d)

(quoted diff) +5 -0

@@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
 ????????if (clk->core->prepare_count)
 ????????????????pr_warn("%s: unregistering prepared clock: %s\n",
 ????????????????????????????????????????__func__, clk->core->name);
+
+???????if (clk->core->protect_count)
+???????????????pr_warn("%s: unregistering protected clock: %s\n",
+???????????????????????????????????????__func__, clk->core->name);
+
 ????????kref_put(&clk->core->ref, __clk_release);

?unlock:
????????clk_prepare_unlock();

(quoted diff) +0 -0

@@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)

?
????????clk_prepare_lock();
?
+???????/* Protect count not balanced: warn and sanitize */ +???????if (clk->protect_ucount) { +???????????????pr_warn("%s: releasing protected clock: %s\n", +???????????????????????????????????????__func__, clk->core->name);

(quoted diff) +5 -0

+
+???????????????for (; clk->protect_ucount; clk->protect_ucount--)
+???????????????????????clk_core_unprotect(clk->core);
+???????}
+

????????hlist_del(&clk->clks_node); ????????if (clk->min_rate > clk->core->req_rate || ????????????clk->max_rate < clk->core->req_rate)

include/linux/clk-provider.h +1 -0

index a428aec36ace..705a158d9b8f 100644
@@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 ?unsigned long __clk_get_flags(struct clk *clk);
 ?unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 ?bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_protected(const struct clk_hw *hw);
 ?bool clk_hw_is_enabled(const struct clk_hw *hw);
 ?bool __clk_is_enabled(struct clk *clk);
 ?struct clk *__clk_lookup(const char *name);

include/linux/clk.h +0 -0

index e9d36b3e49de..90b72ead4411 100644
@@ -265,6 +265,30 @@ struct clk *devm_clk_get(struct device *dev, const char
 *id);

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

include/linux/clk.h +7 -0

+void clk_protect(struct clk *clk);
+
+/**
+ * clk_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting

the
+ * clock source can now deal with other consumer altering the clock source.
+ *
+ * The caller must balance the number of protect and unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprotect(struct clk *clk);
?
?/**
? * clk_enable - inform the system when the clock source should be running.

include/linux/clk.h +0 -0

@@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}

?
?static inline void devm_clk_put(struct device *dev, struct clk *clk) {} ?

include/linux/clk.h +5 -0

+
+static inline void clk_protect(struct clk *clk) {}
+
+static inline void clk_unprotect(struct clk *clk) {}
+

?static inline int clk_enable(struct clk *clk)
?{
????????return 0;
--?
2.9.3

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 <[email protected]>

---
 drivers/clk/clk.c            | 177 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |   1 +
 include/linux/clk.h          |  29 +++++++
 3 files changed, 197 insertions(+), 10 deletions(-)

drivers/clk/clk.c +167 -10

index fa77a1841e0f..69db8cc15063 100644
@@ -60,6 +60,7 @@ struct clk_core {
        bool                    orphan;
        unsigned int            enable_count;
        unsigned int            prepare_count;
+       unsigned int            protect_count;
        unsigned long           min_rate;
        unsigned long           max_rate;
        unsigned long           accuracy;

@@ -84,6 +85,7 @@ struct clk {
        const char *con_id;
        unsigned long min_rate;
        unsigned long max_rate;
+       unsigned long protect_ucount;
        struct hlist_node clks_node;
 };
 

@@ -160,6 +162,11 @@ static bool clk_core_is_prepared(struct clk_core *core)
        return core->ops->is_prepared(core->hw);
 }
 
+static bool clk_core_is_protected(struct clk_core *core)
+{
+       return core->protect_count;
+}
+
 static bool clk_core_is_enabled(struct clk_core *core)
 {
        /*

@@ -328,6 +335,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
        return clk_core_is_prepared(hw->core);
 }
 
+bool clk_hw_is_protected(const struct clk_hw *hw)
+{
+       return clk_core_is_protected(hw->core);
+}
+
 bool clk_hw_is_enabled(const struct clk_hw *hw)
 {
        return clk_core_is_enabled(hw->core);

@@ -584,6 +596,89 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+static void clk_core_unprotect(struct clk_core *core)
+{
+       lockdep_assert_held(&prepare_lock);
+
+       if (!core)
+               return;
+
+       if (WARN_ON(core->protect_count == 0))
+               return;
+
+       if (--core->protect_count > 0)
+               return;
+
+       clk_core_unprotect(core->parent);
+}
+
+/**
+ * clk_unprotect - unprotect a clock source
+ * @clk: the clk being unprotected
+ *
+ * clk_unprotect shall be used when a consumer no longer depends on the clock
+ * rate and can tolerate glitches. As with clk_unprepare and clk_enable, calls
+ * to clk_unprotect must be balanced with clk_protect.
+ * clk_protect may sleep
+ */
+void clk_unprotect(struct clk *clk)
+{
+       if (!clk)
+               return;
+
+       clk_prepare_lock();
+
+       if (WARN_ON(clk->protect_ucount <= 0)) {
+               /*
+                * There is something wrong with this consumer protect count.
+                * Stop here before messing with the provider
+                */
+               clk_prepare_unlock();
+               return;
+       }
+
+       clk_core_unprotect(clk->core);
+       clk->protect_ucount--;
+       clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_unprotect);
+
+static void clk_core_protect(struct clk_core *core)
+{
+       lockdep_assert_held(&prepare_lock);
+
+       if (!core)
+               return;
+
+       if (core->protect_count == 0)
+               clk_core_protect(core->parent);
+
+       core->protect_count++;
+}
+
+/**
+ * clk_protect - protect a clock source
+ * @clk: the clk being protected
+ *
+ * clk_protect can be used when a consumer depends on the clock rate and can't
+ * tolerate any glitches. The consumer protecting the clock can still make
+ * adjustment to clock, if it is the only one protecting the clock. Other
+ * consumers can still use the clock but won't be able to adjust the rate or
+ * reparent the clock while it is protected.
+ * clk_protect may sleep.
+ */
+void clk_protect(struct clk *clk)
+{
+       if (!clk)
+               return;
+
+       clk_prepare_lock();
+       clk_core_protect(clk->core);
+       clk->protect_ucount++;
+       clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_protect);
+
 static void clk_core_disable(struct clk_core *core)
 {
        lockdep_assert_held(&enable_lock);

@@ -838,7 +933,9 @@ static int clk_core_determine_round(struct clk_core *core,
 {
        long rate;
 
-       if (core->ops->determine_rate) {
+       if (clk_core_is_protected(core)) {
+               req->rate = core->rate;
+       } else if (core->ops->determine_rate) {
                return core->ops->determine_rate(core->hw, req);
        } else if (core->ops->round_rate) {
                rate = core->ops->round_rate(core->hw, req->rate,

@@ -1381,7 +1478,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
                req.min_rate = min_rate;
                req.max_rate = max_rate;
 
-               clk_core_init_rate_req(core, req);
+               clk_core_init_rate_req(core, &req);
 
                ret = clk_core_determine_round(core, &req);
                if (ret < 0)

@@ -1637,8 +1734,14 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
        /* prevent racing with updates to the clock topology */
        clk_prepare_lock();
 
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
        ret = clk_core_set_rate_nolock(clk->core, rate);
 
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -1669,12 +1772,18 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 
        clk_prepare_lock();
 
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
        if (min != clk->min_rate || max != clk->max_rate) {
                clk->min_rate = min;
                clk->max_rate = max;
                ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
        }
 
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -1815,6 +1924,9 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
        if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count)
                return -EBUSY;
 
+       if (clk_core_is_protected(core))
+               return -EBUSY;
+
        /* try finding the new parent index */
        if (parent) {
                p_index = clk_fetch_parent_index(core, parent);

@@ -1878,11 +1990,24 @@ static int clk_core_set_parent_lock(struct clk_core *core,
  */
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
+       int ret;
+
        if (!clk)
                return 0;
 
-       return clk_core_set_parent_lock(clk->core,
-                                       parent ? parent->core : NULL);
+       clk_prepare_lock();
+
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
+       ret = clk_core_set_parent(clk->core, parent ? parent->core : NULL);
+
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
+       clk_prepare_unlock();
+
+       return ret;
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 

@@ -1893,7 +2018,10 @@ static int clk_core_set_phase(struct clk_core *core, int degrees)
        if (!core)
                return 0;
 
-       trace_clk_set_phase(clk->core, degrees);
+       if (clk_core_is_protected(core))
+               return -EBUSY;
+
+       trace_clk_set_phase(core, degrees);
 
        if (core->ops->set_phase)
                ret = core->ops->set_phase(core->hw, degrees);

@@ -1936,7 +2064,15 @@ int clk_set_phase(struct clk *clk, int degrees)
                degrees += 360;
 
        clk_prepare_lock();
+
+       if (clk->protect_ucount)
+               clk_core_unprotect(clk->core);
+
        ret = clk_core_set_phase(clk->core, degrees);
+
+       if (clk->protect_ucount)
+               clk_core_protect(clk->core);
+
        clk_prepare_unlock();
 
        return ret;

@@ -2023,11 +2159,12 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
        if (!c)
                return;
 
-       seq_printf(s, "%*s%-*s %11d %12d %11lu %10lu %-3d\n",
+       seq_printf(s, "%*s%-*s %11d %12d %12d %11lu %10lu %-3d\n",
                   level * 3 + 1, "",
                   30 - level * 3, c->name,
-                  c->enable_count, c->prepare_count, clk_core_get_rate(c),
-                  clk_core_get_accuracy(c), clk_core_get_phase(c));
+                  c->enable_count, c->prepare_count, c->protect_count,
+                  clk_core_get_rate(c), clk_core_get_accuracy(c),
+                  clk_core_get_phase(c));
 }
 
 static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,

@@ -2049,8 +2186,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
        struct clk_core *c;
        struct hlist_head **lists = (struct hlist_head **)s->private;
 
-       seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
-       seq_puts(s, "----------------------------------------------------------------------------------------\n");
+       seq_puts(s, "   clock                         enable_cnt  prepare_cnt  protect_cnt        rate   accuracy   phase\n");
+       seq_puts(s, "----------------------------------------------------------------------------------------------------\n");
 
        clk_prepare_lock();
 

@@ -2085,6 +2222,7 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
        seq_printf(s, "\"%s\": { ", c->name);
        seq_printf(s, "\"enable_count\": %d,", c->enable_count);
        seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
+       seq_printf(s, "\"protect_count\": %d,", c->protect_count);
        seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
        seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
        seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));

@@ -2191,6 +2329,11 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
        if (!d)
                goto err_out;
 
+       d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
+                       (u32 *)&core->protect_count);
+       if (!d)
+               goto err_out;
+
        d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
                        (u32 *)&core->notifier_count);
        if (!d)

@@ -2747,6 +2890,11 @@ void clk_unregister(struct clk *clk)
        if (clk->core->prepare_count)
                pr_warn("%s: unregistering prepared clock: %s\n",
                                        __func__, clk->core->name);
+
+       if (clk->core->protect_count)
+               pr_warn("%s: unregistering protected clock: %s\n",
+                                       __func__, clk->core->name);
+
        kref_put(&clk->core->ref, __clk_release);
 unlock:
        clk_prepare_unlock();

@@ -2905,6 +3053,15 @@ void __clk_put(struct clk *clk)
 
        clk_prepare_lock();
 
+       /* Protect count not balanced: warn and sanitize */
+       if (clk->protect_ucount) {
+               pr_warn("%s: releasing protected clock: %s\n",
+                                       __func__, clk->core->name);
+
+               for (; clk->protect_ucount; clk->protect_ucount--)
+                       clk_core_unprotect(clk->core);
+       }
+
        hlist_del(&clk->clks_node);
        if (clk->min_rate > clk->core->req_rate ||
            clk->max_rate < clk->core->req_rate)

include/linux/clk-provider.h +1 -0

index a428aec36ace..705a158d9b8f 100644
@@ -739,6 +739,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 unsigned long __clk_get_flags(struct clk *clk);
 unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_protected(const struct clk_hw *hw);
 bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
 struct clk *__clk_lookup(const char *name);

include/linux/clk.h +29 -0

index e9d36b3e49de..90b72ead4411 100644
@@ -265,6 +265,30 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
  */
 struct clk *devm_get_clk_from_child(struct device *dev,
                                    struct device_node *np, const char *con_id);
+/**
+ * clk_protect - inform the system when the clock source should be protected.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer protecting the clock
+ * depends on the rate of the clock source and can't tolerate any glitches
+ * introduced by further clock rate change or re-parenting of the clock source.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_protect(struct clk *clk);
+
+/**
+ * clk_unprotect - release the protection of the clock source.
+ * @clk: clock source
+ *
+ * This function informs the system that the consumer previously protecting the
+ * clock source can now deal with other consumer altering the clock source.
+ *
+ * The caller must balance the number of protect and unprotect calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprotect(struct clk *clk);
 
 /**
  * clk_enable - inform the system when the clock source should be running.

@@ -460,6 +484,11 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+
+static inline void clk_protect(struct clk *clk) {}
+
+static inline void clk_unprotect(struct clk *clk) {}
+
 static inline int clk_enable(struct clk *clk)
 {
        return 0;

Signature · 1 line

2.9.3

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

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

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

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

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

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

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

In function altering the clock, if the consumer is protecting the clock, the protection is temporarily release under the prepare_lock. This ensure that:
* the clock is still protect from another consumer because of the prepare_lock
* the rate set on a protected clock cannot change between set_rate and later enable
* only a consumer protecting a clock can do this temporary protection removal (less chance of people messing with the refcount)

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

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

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

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

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

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

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

Bonus:

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

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

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

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

Signature · 1 line

2.9.3

Hi J?r?me,

Quoting Jerome Brunet (2017-03-21 11:33:23)

Quoted 7 lines

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

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

Quoted 8 lines

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

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

I prefer clk_lock_rate and clk_unlock_rate.

Quoted 4 lines

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

Quoted 8 lines

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

Similarly, s/protect/lock_rate/g

Quoted 10 lines

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

Quoted 9 lines

In function altering the clock, if the consumer is protecting the clock, the protection is temporarily release under the prepare_lock. This ensure that:
* the clock is still protect from another consumer because of the ? prepare_lock
* the rate set on a protected clock cannot change between set_rate and ? later enable
* only a consumer protecting a clock can do this temporary protection ? removal (less chance of people messing with the refcount)

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

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

Quoted 3 lines

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

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

Quoted 5 lines

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

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

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

Quoted 28 lines

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

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

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

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

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

Bonus:

Quoted 4 lines

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

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

Regards,
Mike

Quoted 22 lines

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

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

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

--?
2.9.3

This RFC patchset is related to the discussion around CLK_SET_RATE_GATE a=

clk_lock which available here [0]

=

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

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

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

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

Inside the clock framework, 2 new count values are introduced to keep tra=

of the protection:
* core "protect_count": this reference count value works in the same way =

prepare and enable count, and track whether the clock is protected or not. This is the value that will checked to allow operation which may cause glitches or change the rate of clock source.
* clk consumer "protect_ucount": this allows to track if the consumer is protecting the clock.
=

Requiring the consumer to unprotect its clock before changing it would ha=

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

to use the consumer protect count.
=

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

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

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

One caveat is the following case:
N+1 consumers protect their clocks. These clocks come from N possible providers. We should able to satisfy at least N consumers before exhausti=

the resources. In the particular case where all the consumers call "protect" before having a chance to call "set_rate", the last 2 consumers will remains stuck on the last provider w/o being able to set the rate on it. This means that in situation where there is 2 consumers on 1 providers, they will compete for the clock, and may end up in situation where both protect the clock and none can set the rate they need. This c=

be solved if, when the rate fails to be set, the consumer release the protection. Then the second consumer will be able to satisfy its request.

=

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

change.
=

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

Patch 7 fix a warning reported by checkpatch.pl. Apparently, ENOSYS is us=

incorrectly.
=

Bonus:
=

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

[0]: http://lkml.kernel.org/r/148942423440.82235.17188153691656009029@res=

=

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

drivers/clk/clk.c | 313 ++++++++++++++++++++++++++++++++++---=

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

=

-- =

2.9.3
=20

Hi Jerome,

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

Quoting Jerome Brunet (2017-03-21 19:33:23)

Quoted 2 lines

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

If we merge this solution then we may want to convert some of those CLK_SET_RATE_GATE users and

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

Quoted 54 lines

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

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

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

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

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

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

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

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

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

Quoted 11 lines

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

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

Quoted 2 lines

"the last 2 consumers
will remains stuck on the last provider w/o being able to set the rate"

?if setting the rate failes

Best regards,
Mike

Quoted 39 lines

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

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

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

Bonus:

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

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

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

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

--?
2.9.3

Quoted 11 lines

Hi Jerome,

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

Quoting Jerome Brunet (2017-03-21 19:33:23)

Quoted 2 lines

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

If we merge this solution then we may want to convert some of those CLK_SET_RATE_GATE users and

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

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

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

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

Quoted 4 lines

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

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

Quoted 66 lines

Quoted 54 lines

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

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

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

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

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

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

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

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

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

Quoted 11 lines

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

Quoted 5 lines

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

set_rate_lock would solve the problem for?

Quoted 2 lines

Quoted 2 lines

"the last 2 consumers
will remains stuck on the last provider w/o being able to set the rate"

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

Quoted 1 line

?if setting the rate failes

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

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

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

Which one has failed ?

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

Quoted 43 lines

Best regards,
Mike

Quoted 39 lines

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

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

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

Bonus:

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

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

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

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

--?
2.9.3