On Thu, 30 Jul 2015, Michael Turquette wrote:

Quoting Lee Jones (2015-07-30 02:21:39)

quoted

On Wed, 29 Jul 2015, Michael Turquette wrote:

quoted

Quoting Lee Jones (2015-07-27 01:53:38)

quoted

On Mon, 27 Jul 2015, Maxime Ripard wrote:

quoted

On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:

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

I think it should be handled by a separate counting. Otherwise, if you
have two users that marked the clock as critical, and then one of them
disable it...

quoted

    if (--clk->enable_count > 0)
            return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+void clk_disable_critical(struct clk *clk)
+{
+   clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

I thought about both of these points, which is why I came up with this
strategy.

Any device which uses the *_critical() API should a) have knowledge of
what happens when a particular critical clock is gated and b) have
thought about the consequences.

If this statement above is true then I fail to see the need for a new
api. A driver which has a really great idea of when it is safe or unsafe
to gate a clock should call clk_prepare_enable at probe and then only
call clk_disable_unprepare once it is safe to do so.

The existing bookkeeping in the clock framework will do the rest.

I think you are viewing this particular API back-to-front.  The idea
is to mark all of the critical clocks at start-up by taking a
reference.  Then, if there are no knowledgable drivers who wish to
turn the clock off, the CCF will leave the clock ungated becuase the
reference count will always be >0.

Right. So I'll ask the same question here that I asked in the other
patch: is there ever a case where a clock consumer driver would want to
call clk_enable_critical? It seems to me that in you usage of it, that
call would only ever be made by the core framework code (e.g. clk-conf.c
or perhaps somewhere in drivers/clk/clk.c).

Yes, _after_ it has called clk_disable_critical(), when it has
finished fiddling with it.  clk_enable_critical() simply resets the
clock back to an enabled/critical state (how the knowledgeable driver
found it).

quoted

The clk_{disable,enable}_critical() calls are to be used by
knowledgable drivers to say "[disable] I know what I'm doing and it's
okay for this clock to be turned off" and "[enable] right I'm done
with this clock now, let's turn it back on and mark it back as
critical, so no one else can turn it off".

OK, so this almost answers my question above. You have a driver that may
finish using a clock for a while (ie, rmmod knowledgeable_driver), and
you want it (critically) enabled again. Is this a real use case? Who
would come along and disable this clock later on? If the driver is to be
re-loaded then I would suggest never unloading it in the first place.

This has nothing to do with modules.  I believe the knowledgeable
consumer should only gate the clock (steal a reference) when the clock
is actually gated.  The rest of the time the framework will have it
marked as critical "do not turn me off".

(Oh and bear in mind when answering my question above that imbalanced
clk_enable/clk_disable calls will not succeed thanks to the vaporware
patch that I have in my local tree)

They won't be imbalanced, because clk_enable() would have been called
first during start-up (__set_critical_clocks()).

If you have a second knowledgeable_driver_2 that shares that clock and
can be trusted to manage it (critically) then I would need to see that
example, as it doesn't feel like a real use to me.

Nor me, that's why this impementation doesn't handle that use-case,
however Maxime thinks it is one, so we can solve that with reference
counting.

quoted

To put things simply, the knowledgable driver will _not_ be enabling
the clock in the first place.  The first interaction it has with it is
to gate it.  Then, once it's done, it will enable it again and mark it
back up as critical.a

I like the first part. Makes sense and fills a real need. I am fully
on-board with a provider-handoff-to-consumer solution. It is all the
weird stuff that comes after it that I'm unsure of.

I don't think there should be hand-off.  I think the
{enable,disable}_critical() should "momentarily" take the last
reference, then put everything back as it found it when it's finished
disabling the clock.

quoted

Still confused ... let's go on another Q in one of your other emails
for another way of putting it.

quoted

quoted

I don't think we can use reference
counting, because we'd need as many critical clock owners as there are
critical clocks.  Cast your mind back to the reasons for this critical
clock API.  One of the most important intentions of this API is the
requirement mitigation for each of the critical clocks to have an owner
(driver).

With regards to your second point, that's what 'critical.enabled'
is for.  Take a look at clk_enable_critical().

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html