[PATCH][RFC] drivers: sh: late disabling of clocks

[PATCH][RFC] drivers: sh: late disabling of clocks

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

This RFC patch kills two birds with one stone:

1) Fix existing issue where in-use clocks without software
   reference are disabled during boot. One example of this
   is the handling of the Mackerel serial console output.

2) Make sure so far unused clocks actually get turned off 

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/sh/clk/core.c |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

--- 0001/drivers/sh/clk/core.c
+++ work/drivers/sh/clk/core.c	2011-06-09 14:13:02.000000000 +0900
@@ -34,6 +34,9 @@ static LIST_HEAD(clock_list);
 static DEFINE_SPINLOCK(clock_lock);
 static DEFINE_MUTEX(clock_list_sem);
 
+/* clock disable operations are not passed on to hardware during boot */
+static int allow_disable;
+
 void clk_rate_table_build(struct clk *clk,
 			  struct cpufreq_frequency_table *freq_table,
 			  int nr_freqs,
@@ -228,7 +231,7 @@ static void __clk_disable(struct clk *cl
 		return;
 
 	if (!(--clk->usecount)) {
-		if (likely(clk->ops && clk->ops->disable))
+		if (likely(allow_disable && clk->ops && clk->ops->disable))
 			clk->ops->disable(clk);
 		if (likely(clk->parent))
 			__clk_disable(clk->parent);
@@ -747,3 +750,24 @@ err_out:
 	return err;
 }
 late_initcall(clk_debugfs_init);
+
+static int __init clk_late_init(void)
+{
+	unsigned long flags;
+	struct clk *clk;
+
+	/* from now on allow clock disable operations */
+	allow_disable = 1;
+
+	/* disable all clocks with zero use count */
+	mutex_lock(&clock_list_sem);
+	list_for_each_entry(clk, &clock_list, node) {
+		spin_lock_irqsave(&clock_lock, flags);
+		if (!clk->usecount && clk->ops && clk->ops->disable)
+			clk->ops->disable(clk);
+		spin_unlock_irqrestore(&clock_lock, flags);
+	}
+	mutex_unlock(&clock_list_sem);
+	return 0;
+}
+late_initcall(clk_late_init);

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Simon Horman]

On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> This RFC patch kills two birds with one stone:
> 
> 1) Fix existing issue where in-use clocks without software
>    reference are disabled during boot. One example of this
>    is the handling of the Mackerel serial console output.
> 
> 2) Make sure so far unused clocks actually get turned off 

Is access to allow_disable SMP-safe?
In particular, does the the access in clk_late_init()
need to be guarded by clock_lock as calls to __clk_disable() are?

> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  drivers/sh/clk/core.c |   26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> --- 0001/drivers/sh/clk/core.c
> +++ work/drivers/sh/clk/core.c	2011-06-09 14:13:02.000000000 +0900
> @@ -34,6 +34,9 @@ static LIST_HEAD(clock_list);
>  static DEFINE_SPINLOCK(clock_lock);
>  static DEFINE_MUTEX(clock_list_sem);
>  
> +/* clock disable operations are not passed on to hardware during boot */
> +static int allow_disable;
> +
>  void clk_rate_table_build(struct clk *clk,
>  			  struct cpufreq_frequency_table *freq_table,
>  			  int nr_freqs,
> @@ -228,7 +231,7 @@ static void __clk_disable(struct clk *cl
>  		return;
>  
>  	if (!(--clk->usecount)) {
> -		if (likely(clk->ops && clk->ops->disable))
> +		if (likely(allow_disable && clk->ops && clk->ops->disable))
>  			clk->ops->disable(clk);
>  		if (likely(clk->parent))
>  			__clk_disable(clk->parent);
> @@ -747,3 +750,24 @@ err_out:
>  	return err;
>  }
>  late_initcall(clk_debugfs_init);
> +
> +static int __init clk_late_init(void)
> +{
> +	unsigned long flags;
> +	struct clk *clk;
> +
> +	/* from now on allow clock disable operations */
> +	allow_disable = 1;
> +
> +	/* disable all clocks with zero use count */
> +	mutex_lock(&clock_list_sem);
> +	list_for_each_entry(clk, &clock_list, node) {
> +		spin_lock_irqsave(&clock_lock, flags);
> +		if (!clk->usecount && clk->ops && clk->ops->disable)
> +			clk->ops->disable(clk);
> +		spin_unlock_irqrestore(&clock_lock, flags);
> +	}
> +	mutex_unlock(&clock_list_sem);
> +	return 0;
> +}
> +late_initcall(clk_late_init);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Simon Horman]

On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> > 
> > This RFC patch kills two birds with one stone:
> > 
> > 1) Fix existing issue where in-use clocks without software
> >    reference are disabled during boot. One example of this
> >    is the handling of the Mackerel serial console output.
> > 
> > 2) Make sure so far unused clocks actually get turned off 
> 
> Is access to allow_disable SMP-safe?
> In particular, does the the access in clk_late_init()
> need to be guarded by clock_lock as calls to __clk_disable() are?

On reflection I think that the problem is a bit deeper.

I think allow_disable is set to 1 prematurely, as it indicates
to __clk_disable() that calling clk->ops->disable(clk) is allowed,
even if the iteration of the loop in clk_late_init() is not complete. 

My proposed re-working of clk_late_init() is below.
Completely untested.

> 
> > Signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> > 
> >  drivers/sh/clk/core.c |   26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > --- 0001/drivers/sh/clk/core.c
> > +++ work/drivers/sh/clk/core.c	2011-06-09 14:13:02.000000000 +0900
> > @@ -34,6 +34,9 @@ static LIST_HEAD(clock_list);
> >  static DEFINE_SPINLOCK(clock_lock);
> >  static DEFINE_MUTEX(clock_list_sem);
> >  
> > +/* clock disable operations are not passed on to hardware during boot */
> > +static int allow_disable;
> > +
> >  void clk_rate_table_build(struct clk *clk,
> >  			  struct cpufreq_frequency_table *freq_table,
> >  			  int nr_freqs,
> > @@ -228,7 +231,7 @@ static void __clk_disable(struct clk *cl
> >  		return;
> >  
> >  	if (!(--clk->usecount)) {
> > -		if (likely(clk->ops && clk->ops->disable))
> > +		if (likely(allow_disable && clk->ops && clk->ops->disable))
> >  			clk->ops->disable(clk);
> >  		if (likely(clk->parent))
> >  			__clk_disable(clk->parent);
> > @@ -747,3 +750,24 @@ err_out:
> >  	return err;
> >  }
> >  late_initcall(clk_debugfs_init);
> > +
> > +static int __init clk_late_init(void)
> > +{
> > +	unsigned long flags;
> > +	struct clk *clk;
> > +
> > +	/* from now on allow clock disable operations */
> > +	allow_disable = 1;
> > +
> > +	/* disable all clocks with zero use count */
> > +	mutex_lock(&clock_list_sem);
> > +	list_for_each_entry(clk, &clock_list, node) {
> > +		spin_lock_irqsave(&clock_lock, flags);
> > +		if (!clk->usecount && clk->ops && clk->ops->disable)
> > +			clk->ops->disable(clk);
> > +		spin_unlock_irqrestore(&clock_lock, flags);
> > +	}
> > +	mutex_unlock(&clock_list_sem);
> > +	return 0;
> > +}

static int __init clk_late_init(void)
{
	unsigned long flags;
	struct clk *clk;

	mutex_lock(&clock_list_sem);
	spin_lock_irqsave(&clock_lock, flags);

	/* disable all clocks with zero use count */
	list_for_each_entry(clk, &clock_list, node)
		if (!clk->usecount && clk->ops && clk->ops->disable)
			clk->ops->disable(clk);

	/* from now on allow clock disable operations */
	allow_disable = 1;

	spin_unlock_irqrestore(&clock_lock, flags);
	mutex_unlock(&clock_list_sem);

	return 0;
}


> > +late_initcall(clk_late_init);

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Magnus Damm]

On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
>> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
>> > From: Magnus Damm <damm@opensource.se>
>> >
>> > This RFC patch kills two birds with one stone:
>> >
>> > 1) Fix existing issue where in-use clocks without software
>> >    reference are disabled during boot. One example of this
>> >    is the handling of the Mackerel serial console output.
>> >
>> > 2) Make sure so far unused clocks actually get turned off
>>
>> Is access to allow_disable SMP-safe?
>> In particular, does the the access in clk_late_init()
>> need to be guarded by clock_lock as calls to __clk_disable() are?
>
> On reflection I think that the problem is a bit deeper.
>
> I think allow_disable is set to 1 prematurely, as it indicates
> to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> even if the iteration of the loop in clk_late_init() is not complete.
>
> My proposed re-working of clk_late_init() is below.
> Completely untested.

Right, I was thinking something along those lines too. The spinlock
should make sure we're serialized.

Thanks,

/ magnus

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Simon Horman]

On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> >> > From: Magnus Damm <damm@opensource.se>
> >> >
> >> > This RFC patch kills two birds with one stone:
> >> >
> >> > 1) Fix existing issue where in-use clocks without software
> >> >    reference are disabled during boot. One example of this
> >> >    is the handling of the Mackerel serial console output.
> >> >
> >> > 2) Make sure so far unused clocks actually get turned off
> >>
> >> Is access to allow_disable SMP-safe?
> >> In particular, does the the access in clk_late_init()
> >> need to be guarded by clock_lock as calls to __clk_disable() are?
> >
> > On reflection I think that the problem is a bit deeper.
> >
> > I think allow_disable is set to 1 prematurely, as it indicates
> > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> > even if the iteration of the loop in clk_late_init() is not complete.
> >
> > My proposed re-working of clk_late_init() is below.
> > Completely untested.
> 
> Right, I was thinking something along those lines too. The spinlock
> should make sure we're serialized.

Great. Feel free to take my ideas verbatim if it helps.

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Simon Horman]

On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> > >> > From: Magnus Damm <damm@opensource.se>
> > >> >
> > >> > This RFC patch kills two birds with one stone:
> > >> >
> > >> > 1) Fix existing issue where in-use clocks without software
> > >> >    reference are disabled during boot. One example of this
> > >> >    is the handling of the Mackerel serial console output.
> > >> >
> > >> > 2) Make sure so far unused clocks actually get turned off
> > >>
> > >> Is access to allow_disable SMP-safe?
> > >> In particular, does the the access in clk_late_init()
> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
> > >
> > > On reflection I think that the problem is a bit deeper.
> > >
> > > I think allow_disable is set to 1 prematurely, as it indicates
> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> > > even if the iteration of the loop in clk_late_init() is not complete.
> > >
> > > My proposed re-working of clk_late_init() is below.
> > > Completely untested.
> > 
> > Right, I was thinking something along those lines too. The spinlock
> > should make sure we're serialized.
> 
> Great. Feel free to take my ideas verbatim if it helps.

Hi Magnus,

have you had time to look into this?

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Magnus Damm]

On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
>> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
>> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
>> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
>> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
>> > >> > From: Magnus Damm <damm@opensource.se>
>> > >> >
>> > >> > This RFC patch kills two birds with one stone:
>> > >> >
>> > >> > 1) Fix existing issue where in-use clocks without software
>> > >> >    reference are disabled during boot. One example of this
>> > >> >    is the handling of the Mackerel serial console output.
>> > >> >
>> > >> > 2) Make sure so far unused clocks actually get turned off
>> > >>
>> > >> Is access to allow_disable SMP-safe?
>> > >> In particular, does the the access in clk_late_init()
>> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
>> > >
>> > > On reflection I think that the problem is a bit deeper.
>> > >
>> > > I think allow_disable is set to 1 prematurely, as it indicates
>> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
>> > > even if the iteration of the loop in clk_late_init() is not complete.
>> > >
>> > > My proposed re-working of clk_late_init() is below.
>> > > Completely untested.
>> >
>> > Right, I was thinking something along those lines too. The spinlock
>> > should make sure we're serialized.
>>
>> Great. Feel free to take my ideas verbatim if it helps.
>
> Hi Magnus,
>
> have you had time to look into this?

I have not updated this patch yet, mainly due to lack of feedback. Not
so much due to lack of feedback from you - I appreciate your ideas and
I agree that your rework of the ordering is correct. I'm however still
not sure if other people agree with my approach of delaying the
disable operations until a certain point in time.

Thanks,

/ magnus

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Simon Horman]

On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> >> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> >> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> >> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> >> > >> > From: Magnus Damm <damm@opensource.se>
> >> > >> >
> >> > >> > This RFC patch kills two birds with one stone:
> >> > >> >
> >> > >> > 1) Fix existing issue where in-use clocks without software
> >> > >> >    reference are disabled during boot. One example of this
> >> > >> >    is the handling of the Mackerel serial console output.
> >> > >> >
> >> > >> > 2) Make sure so far unused clocks actually get turned off
> >> > >>
> >> > >> Is access to allow_disable SMP-safe?
> >> > >> In particular, does the the access in clk_late_init()
> >> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
> >> > >
> >> > > On reflection I think that the problem is a bit deeper.
> >> > >
> >> > > I think allow_disable is set to 1 prematurely, as it indicates
> >> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> >> > > even if the iteration of the loop in clk_late_init() is not complete.
> >> > >
> >> > > My proposed re-working of clk_late_init() is below.
> >> > > Completely untested.
> >> >
> >> > Right, I was thinking something along those lines too. The spinlock
> >> > should make sure we're serialized.
> >>
> >> Great. Feel free to take my ideas verbatim if it helps.
> >
> > Hi Magnus,
> >
> > have you had time to look into this?
> 
> I have not updated this patch yet, mainly due to lack of feedback. Not
> so much due to lack of feedback from you - I appreciate your ideas and
> I agree that your rework of the ordering is correct. I'm however still
> not sure if other people agree with my approach of delaying the
> disable operations until a certain point in time.

Understood.

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Paul Mundt]

On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> > Right, I was thinking something along those lines too. The spinlock
> >> > should make sure we're serialized.
> >>
> >> Great. Feel free to take my ideas verbatim if it helps.
> >
> > Hi Magnus,
> >
> > have you had time to look into this?
> 
> I have not updated this patch yet, mainly due to lack of feedback. Not
> so much due to lack of feedback from you - I appreciate your ideas and
> I agree that your rework of the ordering is correct. I'm however still
> not sure if other people agree with my approach of delaying the
> disable operations until a certain point in time.
> 
Except that's not really how it works. I don't have the time or
inclination to go through and provide feedback for every single patch
that crosses my inbox. If something has been posted for awhile and has
feedback, then it's expected that the feedback is addressed and an
updated version is posted. If no other action happens within an arbitrary
but not too long period of time, I'll then roll it in to one of my trees
for testing (assuming there are no glaring issues with the concept or
implementation prior to integration/testing). You've posted multiple
versions of the same patches in the past upon receiving feedback, so it's
a bit perplexing as to why this time you've simply opted to throw the
patch over the fence and do nothing.

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Magnus Damm]

On Tue, Jun 21, 2011 at 2:27 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
>> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
>> >> > Right, I was thinking something along those lines too. The spinlock
>> >> > should make sure we're serialized.
>> >>
>> >> Great. Feel free to take my ideas verbatim if it helps.
>> >
>> > Hi Magnus,
>> >
>> > have you had time to look into this?
>>
>> I have not updated this patch yet, mainly due to lack of feedback. Not
>> so much due to lack of feedback from you - I appreciate your ideas and
>> I agree that your rework of the ordering is correct. I'm however still
>> not sure if other people agree with my approach of delaying the
>> disable operations until a certain point in time.
>>
> Except that's not really how it works. I don't have the time or
> inclination to go through and provide feedback for every single patch
> that crosses my inbox. If something has been posted for awhile and has
> feedback, then it's expected that the feedback is addressed and an
> updated version is posted. If no other action happens within an arbitrary
> but not too long period of time, I'll then roll it in to one of my trees
> for testing (assuming there are no glaring issues with the concept or
> implementation prior to integration/testing). You've posted multiple
> versions of the same patches in the past upon receiving feedback, so it's
> a bit perplexing as to why this time you've simply opted to throw the
> patch over the fence and do nothing.

I believe I received some feedback through face-to-face conversations
earlier, and that feedback was not all that positive. I decided to
post the code anyway, and Simon kindly provided detailed feedback via
email about internal ordering. The main idea behind the patch is not
really changed, so in that area there still is silence.

I'll resend a new version in a little while. Thanks.

/ magnus

Re: [PATCH][RFC] drivers: sh: late disabling of clocks

[Paul Mundt]

On Tue, Jun 21, 2011 at 02:48:44PM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 2:27 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> >> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> >> > Right, I was thinking something along those lines too. The spinlock
> >> >> > should make sure we're serialized.
> >> >>
> >> >> Great. Feel free to take my ideas verbatim if it helps.
> >> >
> >> > Hi Magnus,
> >> >
> >> > have you had time to look into this?
> >>
> >> I have not updated this patch yet, mainly due to lack of feedback. Not
> >> so much due to lack of feedback from you - I appreciate your ideas and
> >> I agree that your rework of the ordering is correct. I'm however still
> >> not sure if other people agree with my approach of delaying the
> >> disable operations until a certain point in time.
> >>
> > Except that's not really how it works. I don't have the time or
> > inclination to go through and provide feedback for every single patch
> > that crosses my inbox. If something has been posted for awhile and has
> > feedback, then it's expected that the feedback is addressed and an
> > updated version is posted. If no other action happens within an arbitrary
> > but not too long period of time, I'll then roll it in to one of my trees
> > for testing (assuming there are no glaring issues with the concept or
> > implementation prior to integration/testing). You've posted multiple
> > versions of the same patches in the past upon receiving feedback, so it's
> > a bit perplexing as to why this time you've simply opted to throw the
> > patch over the fence and do nothing.
> 
> I believe I received some feedback through face-to-face conversations
> earlier, and that feedback was not all that positive. I decided to
> post the code anyway, and Simon kindly provided detailed feedback via
> email about internal ordering. The main idea behind the patch is not
> really changed, so in that area there still is silence.
> 
> I'll resend a new version in a little while. Thanks.
> 
I don't have any particular problems with the approach at least, and at
this point it's really just the locking stuff pointed out by Simon that
is keeping it from being applied. It's obviously somewhat of a hack and
I'm certainly not thrilled by it, but the fact we're forced in to this
position and are entering with undefined state is certainly not your
fault, nor something we can really do much about. We also have half a
dozen syscall ABIs for no good reason because of hardware designer
muppetry, such is life.

Having said that, it might need a bit of fiddling in terms of deciding on
how to enable it or present it to the user, but those are fairly
superficial details that can be worked out once an updated version is
posted.