Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1189932 > unrolled thread

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework Lee Jones <lee.jones@linaro.org> - 2015-07-22 15:30 +0200
    Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable,  disable} framework Michael Turquette <mturquette@linaro.org> - 2015-07-30 03:30 +0200
      Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable,  disable} framework Lee Jones <lee.jones@linaro.org> - 2015-07-30 11:30 +0200
    Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable,  disable} framework Lee Jones <lee.jones@linaro.org> - 2015-07-30 12:00 +0200
    Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable,  disable} framework Lee Jones <lee.jones@linaro.org> - 2015-07-30 13:20 +0200

#1189932 — [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

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 <maxime.ripard@free-electrons.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 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;
+
 	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;
+	clk_disable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_critical);
+
 static int clk_core_enable(struct clk_core *clk)
 {
 	int ret = 0;
@@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_enable_critical(struct clk *clk)
+{
+	if (clk->core->critical.enabled)
+		clk->core->critical.leave_on = true;
+
+	return clk_enable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_enable_critical);
+
 static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
 						unsigned long rate,
 						unsigned long min_rate,
@@ -2482,6 +2518,15 @@ fail_out:
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
+void clk_init_critical(struct clk *clk)
+{
+	struct critical *critical = &clk->core->critical;
+
+	critical->enabled = true;
+	critical->leave_on = true;
+}
+EXPORT_SYMBOL_GPL(clk_init_critical);
+
 /*
  * Free memory allocated for a clock.
  * Caller must hold prepare_lock.
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5591ea7..15ef8c9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);
 
+void clk_init_critical(struct clk *clk);
+
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
 struct clk_hw *__clk_get_hw(struct clk *clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8381bbf..9807f3b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_enable_critical - inform the system when the clock source should be
+ * 			 running, even if clock is critical.
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_enable_critical(struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_critical - inform the system when the clock source is no
+ * 			  longer required, even if clock is critical.
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the clock source is shared between
+ * multiple drivers, clk_enable_critical() calls must be balanced
+ * by the same number of clk_disable_critical() calls for the clock
+ * source to be disabled.
+ */
+void clk_disable_critical(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1195662 — Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

Quoting Lee Jones (2015-07-27 01:53:38)
> On Mon, 27 Jul 2015, Maxime Ripard wrote:
> 
> > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > 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 <maxime.ripard@free-electrons.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  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...
> > 
> > >     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.

Regards,
Mike

> 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1195849 — Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

On Wed, 29 Jul 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-07-27 01:53:38)
> > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > 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 <maxime.ripard@free-electrons.com>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  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...
> > > 
> > > >     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.

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

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.

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

> > 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1195877 — Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

On Wed, 29 Jul 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-07-28 06:00:55)
> > On Tue, 28 Jul 2015, Maxime Ripard wrote:
> > 
> > > On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> > > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > > 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 <maxime.ripard@free-electrons.com>
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  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...
> > > > > 
> > > > > >         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.
> > > 
> > > Indeed.
> > > 
> > > > I don't think we can use reference counting, because we'd need as
> > > > many critical clock owners as there are critical clocks.
> > > 
> > > Which we can have if we replace the call to clk_prepare_enable you add
> > > in your fourth patch in __set_critical_clocks.
> > 
> > What should it be replaced with?
> > 
> > > > 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().
> > > 
> > > I don't think this addresses the issue, if you just throw more
> > > customers at it, the issue remain with your implementation.
> > > 
> > > If you have three customers that used the critical API, and if on of
> > > these calls clk_disable_critical, you're losing leave_on.
> > 
> > That's the idea.  See my point above, the one you replied "Indeed"
> > to.  So when a driver uses clk_disable_critical() it's saying, "I know
> > why this clock is a critical clock, and I know that nothing terrible
> > will happen if I disable it, as I have that covered".  So then if it's
> > not the last user to call clk_disable(), the last one out the door
> > will be allowed to finally gate the clock, regardless whether it's
> > critical aware or not.
> > 
> > Then, when we come to enable the clock again, the critical aware user
> > then re-marks the clock as leave_on, so not critical un-aware user can
> > take the final reference and disable the clock.
> > 
> > > Which means that if there's one of the two users left that calls
> > > clk_disable on it, the clock will actually be disabled, which is
> > > clearly not what we want to do, as we have still a user that want the
> > > clock to be enabled.
> > 
> > That's not what happens (at least it shouldn't if I've coded it up
> > right).  The API _still_ requires all of the users to give-up their
> > reference.
> > 
> > > It would be much more robust to have another count for the critical
> > > stuff, initialised to one by the __set_critical_clocks function.
> > 
> > If I understand you correctly, we already have a count.  We use the
> > original reference count.  No need for one of our own.
> > 
> > Using your RAM Clock (Clock 4) as an example
> > --------------------------------------------
> > 
> > Early start-up:
> >   Clock 4 is marked as critical and a reference is taken (ref == 1)
> > 
> > Driver probe:
> >   SPI enables Clock 4 (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> > 
> > Suspend (without RAM driver's permission):
> >   SPI disables Clock 4 (ref == 2)
> >   I2C disables Clock 4 (ref == 1)
> >   /*
> >    * Clock won't be gated because:
> >    *   .leave_on is True - can't dec final reference
> 
> I am clearly missing the point. The clock won't be gated because the
> enable_count is still 1! What does .leave_on do here?

The point of _this_ (the extended) part of the API is so that the
clock _can_ be turned off.  Without the possibility to disable
.leave_on and the logic with accompanies it (i.e.
clk_disable_critical()) the clock will _never_ be gated.

> >    */
> > 
> > Suspend (with RAM driver's permission):
> >   /* Order is unimportant */
> >   SPI disables Clock 4 (ref == 2)
> >   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
> >   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
> >   /*
> >    * Clock will be gated because:
> >    *   .leave_on is False, so (ref == 0)
> 
> Again, .leave_on does nothing new here. We gate the clock because the
> reference count is 0.

It's the fact that .leave_on has been disabled in
clk_disable_critical() that allows the final reference to be taken.

> >    */
> > 
> > Resume:
> >   /* Order is unimportant */
> >   SPI enables Clock 4 (ref == 1)
> >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> 
> Same again. As soon as RAM calls clk_enable_critical the ref count goes
> up. .leave_on does nothing as far as I can tell. The all works because
> of the reference counting, which already exists before this patch
> series.

So fundamentally you're right in what you say.  All you really need to
disable a critical clock is write a knowledgeable driver, which is
intentionally unbalanced i.e. just calls clk_disable().  All this
extended API really does is makes the process more official and
ensures that an unintentionally unbalanced driver doesn't bugger up
the running platform.  We could also add a new WARN() to say that said
driver is unbalanced, as it just tried to turn off a critical clock.

What do you think is best?

-- 
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1195915 — Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

On Wed, 29 Jul 2015, Michael Turquette wrote:

> Hi Lee,
> 
> + linux-clk ml
> 
> Quoting Lee Jones (2015-07-22 06:04:13)
> > 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 <maxime.ripard@free-electrons.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  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?
> 
> > + */
> > +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.

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

> > +
> >         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;
> > +       clk_disable(clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_disable_critical);
> > +
> >  static int clk_core_enable(struct clk_core *clk)
> >  {
> >         int ret = 0;
> > @@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_enable);
> >  
> > +int clk_enable_critical(struct clk *clk)
> > +{
> > +       if (clk->core->critical.enabled)
> > +               clk->core->critical.leave_on = true;
> > +
> > +       return clk_enable(clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_enable_critical);
> > +
> >  static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
> >                                                 unsigned long rate,
> >                                                 unsigned long min_rate,
> > @@ -2482,6 +2518,15 @@ fail_out:
> >  }
> >  EXPORT_SYMBOL_GPL(clk_register);
> >  
> > +void clk_init_critical(struct clk *clk)
> > +{
> > +       struct critical *critical = &clk->core->critical;
> > +
> > +       critical->enabled = true;
> > +       critical->leave_on = true;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_init_critical);
> > +
> >  /*
> >   * Free memory allocated for a clock.
> >   * Caller must hold prepare_lock.
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 5591ea7..15ef8c9 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
> >  void clk_unregister(struct clk *clk);
> >  void devm_clk_unregister(struct device *dev, struct clk *clk);
> >  
> > +void clk_init_critical(struct clk *clk);
> > +
> >  /* helper functions */
> >  const char *__clk_get_name(struct clk *clk);
> >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 8381bbf..9807f3b 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_enable_critical - inform the system when the clock source should be
> > + *                      running, even if clock is critical.
> > + * @clk: clock source
> > + *
> > + * If the clock can not be enabled/disabled, this should return success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int clk_enable_critical(struct clk *clk);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer required.
> >   * @clk: clock source
> >   *
> > @@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_disable_critical - inform the system when the clock source is no
> > + *                       longer required, even if clock is critical.
> > + * @clk: clock source
> > + *
> > + * Inform the system that a clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the clock source is shared between
> > + * multiple drivers, clk_enable_critical() calls must be balanced
> > + * by the same number of clk_disable_critical() calls for the clock
> > + * source to be disabled.
> > + */
> > +void clk_disable_critical(struct clk *clk);
> > +
> > +/**
> >   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> >   *               This is only valid once the clock source has been enabled.
> >   * @clk: clock source

-- 
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web