On Thu, 30 Jul 2015, Michael Turquette wrote:

Quoting Lee Jones (2015-07-30 04:17:47)

quoted

On Wed, 29 Jul 2015, Michael Turquette wrote:

quoted

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)

quoted

These new API calls will firstly provide a mechanisms to tag a clock as
critical and secondly allow any knowledgeable driver to (un)gate clocks,
even if they are marked as critical.

Suggested-by: Maxime Ripard <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 61c3fc5..486b1da 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on
do? Better yet, what would happen if leave_on did not exist?

quoted

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;

How do we get to this point? clk_enable_critical actually calls
clk_enable, thus incrementing the enable_count. The only time that we
could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is
the case then the drivers need to be fixed. Or better yet some
infrastructure to catch that, now that we have per-user struct clk
cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular,
old clk_disable(foo). But why would a driver do that?

It might be that I am missing the point here, so please feel free to
clue me in.

This check behaves in a very similar to the WARN() above.  It's more
of a fail-safe.  If all drivers are behaving properly, then it
shouldn't ever be true.  If they're not, it prevents an incorrectly
written driver from irrecoverably crippling the system.

Then this check should be replaced with a generic approach that refuses
to honor imbalances anyways. Below are two patches that probably resolve
the issue of badly behaving drivers that cause enable imbalances.

Your patch should make the requirement for this check moot, so it can
probably be removed.

quoted

As I said in the other mail.  We can do without these 3 new wrappers.
We _could_ just write a driver which only calls clk_enable() _after_
it calls clk_disable(), a kind of intentional unbalance and it would
do that same thing.

This naive approach will not work with per-user imbalance tracking.

Steady on.  I said we "_could_", that that I think it's a good idea.

I think it's a bad idea, which is why I wrote this set. ;)

quoted

However, what we're trying to do here is provide
a proper API, so we can see at first glance what the 'knowledgeable'
driver is trying to do and not have someone attempt to submit a 'fix'
which calls clk_enable() or something.

We'll need some type of api for sure for the handoff.

This set will not trigger your new checks.  The clocks will be in
perfect ballance becuase a reference will be taken at start-up.

Again:

start-up:
  clk_prepare_enable()

knowlegable_driver_probe:
  clk_get()

knowlegable_driver_gate_clk:
  clk_disable_critical()

knowlegable_driver_ungate_clk:
  clk_enable_critical()

knowlegable_driver_remove:
  clk_put()

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog