[v4,1/7] clk: kona: allow nested ccu_write_enable() requests

Message ID 1401483188-5395-2-git-send-email-elder@linaro.org
State New
Headers show

Commit Message

Alex Elder May 30, 2014, 8:53 p.m.
Use a counter rather than a Boolean to track whether write access to
a CCU has been enabled or not.  This will allow more than one of
these requests to be nested.

Note that __ccu_write_enable() and __ccu_write_disable() calls all
come in pairs, and they are always surrounded immediately by calls
to ccu_lock() and ccu_unlock().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-kona.c | 14 ++++----------
 drivers/clk/bcm/clk-kona.h |  2 +-
 2 files changed, 5 insertions(+), 11 deletions(-)

Comments

Mike Turquette May 30, 2014, 11:28 p.m. | #1
Quoting Alex Elder (2014-05-30 13:53:02)
> Use a counter rather than a Boolean to track whether write access to
> a CCU has been enabled or not.  This will allow more than one of
> these requests to be nested.
> 
> Note that __ccu_write_enable() and __ccu_write_disable() calls all
> come in pairs, and they are always surrounded immediately by calls
> to ccu_lock() and ccu_unlock().
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
>  drivers/clk/bcm/clk-kona.h |  2 +-
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index 95af2e6..ee8e988 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
>   */
>  static inline void __ccu_write_enable(struct ccu_data *ccu)

Per Documentation/CodingStyle, chapter 15, "the inline disease", it
might be best to not inline these functions.

>  {
> -       if (ccu->write_enabled) {
> -               pr_err("%s: access already enabled for %s\n", __func__,
> -                       ccu->name);
> -               return;
> -       }
> -       ccu->write_enabled = true;
> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> +       if (!ccu->write_enabled++)
> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>  }
>  
>  static inline void __ccu_write_disable(struct ccu_data *ccu)
> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
>                         ccu->name);
>                 return;
>         }
> -
> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> -       ccu->write_enabled = false;
> +       if (!--ccu->write_enabled)
> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);

What happens if calls to __ccu_write_enable and __ccu_write_disable are
unbalanced? It would be better to catch that case and throw a WARN:

	if (WARN_ON(ccu->write_enabled == 0))
		return;

	if (--ccu->write_enabled > 0)
		return;

	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);

>  }
>  
>  /*
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index 2537b30..e9a8466 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -478,7 +478,7 @@ struct ccu_policy {
>  struct ccu_data {
>         void __iomem *base;     /* base of mapped address space */
>         spinlock_t lock;        /* serialization lock */
> -       bool write_enabled;     /* write access is currently enabled */
> +       u32 write_enabled;      /* write access enable count */

Why u32? An unsigned int will do just nicely here.

Regards,
Mike

>         struct ccu_policy policy;
>         struct list_head links; /* for ccu_list */
>         struct device_node *node;
> -- 
> 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/
Alex Elder May 31, 2014, 3:46 a.m. | #2
On 05/30/2014 06:28 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-30 13:53:02)
>> Use a counter rather than a Boolean to track whether write access to
>> a CCU has been enabled or not.  This will allow more than one of
>> these requests to be nested.
>>
>> Note that __ccu_write_enable() and __ccu_write_disable() calls all
>> come in pairs, and they are always surrounded immediately by calls
>> to ccu_lock() and ccu_unlock().
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
>>  drivers/clk/bcm/clk-kona.h |  2 +-
>>  2 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>> index 95af2e6..ee8e988 100644
>> --- a/drivers/clk/bcm/clk-kona.c
>> +++ b/drivers/clk/bcm/clk-kona.c
>> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
>>   */
>>  static inline void __ccu_write_enable(struct ccu_data *ccu)
> 
> Per Documentation/CodingStyle, chapter 15, "the inline disease", it
> might be best to not inline these functions.

This was not intentional.  I normally only inline things
defined in header files, and maybe this is an artifact of
having been in a header at one time.  I don't know, I'll get
rid of the inline.

> 
>>  {
>> -       if (ccu->write_enabled) {
>> -               pr_err("%s: access already enabled for %s\n", __func__,
>> -                       ccu->name);
>> -               return;
>> -       }
>> -       ccu->write_enabled = true;
>> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>> +       if (!ccu->write_enabled++)
>> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>>  }
>>  
>>  static inline void __ccu_write_disable(struct ccu_data *ccu)
>> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
>>                         ccu->name);
>>                 return;
>>         }
>> -
>> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
>> -       ccu->write_enabled = false;
>> +       if (!--ccu->write_enabled)
>> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> 
> What happens if calls to __ccu_write_enable and __ccu_write_disable are
> unbalanced? It would be better to catch that case and throw a WARN:

You can't see it in the diff, but that's what happens
(well, it's a pr_err(), not a WARN()).   I think a WARN()
is probably right in this case though.

> 	if (WARN_ON(ccu->write_enabled == 0))
> 		return;
> 
> 	if (--ccu->write_enabled > 0)
> 		return;
> 
> 	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> 
>>  }
>>  
>>  /*
>> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
>> index 2537b30..e9a8466 100644
>> --- a/drivers/clk/bcm/clk-kona.h
>> +++ b/drivers/clk/bcm/clk-kona.h
>> @@ -478,7 +478,7 @@ struct ccu_policy {
>>  struct ccu_data {
>>         void __iomem *base;     /* base of mapped address space */
>>         spinlock_t lock;        /* serialization lock */
>> -       bool write_enabled;     /* write access is currently enabled */
>> +       u32 write_enabled;      /* write access enable count */
> 
> Why u32? An unsigned int will do just nicely here.

That's a preference of mine.  I almost always favor
using u32, etc. because they are compact, and explicit
about the size and signedness.  I "know" an int is 32
bits, but I still prefer being explicit.

I'll interpret this as a preference on your part for
unsigned int, and I have no problem making that change.

					-Alex

> Regards,
> Mike
> 
>>         struct ccu_policy policy;
>>         struct list_head links; /* for ccu_list */
>>         struct device_node *node;
>> -- 
>> 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/
Mike Turquette June 2, 2014, 9:05 p.m. | #3
Quoting Alex Elder (2014-05-30 20:46:46)
> On 05/30/2014 06:28 PM, Mike Turquette wrote:
> > Quoting Alex Elder (2014-05-30 13:53:02)
> >> Use a counter rather than a Boolean to track whether write access to
> >> a CCU has been enabled or not.  This will allow more than one of
> >> these requests to be nested.
> >>
> >> Note that __ccu_write_enable() and __ccu_write_disable() calls all
> >> come in pairs, and they are always surrounded immediately by calls
> >> to ccu_lock() and ccu_unlock().
> >>
> >> Signed-off-by: Alex Elder <elder@linaro.org>
> >> ---
> >>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
> >>  drivers/clk/bcm/clk-kona.h |  2 +-
> >>  2 files changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> >> index 95af2e6..ee8e988 100644
> >> --- a/drivers/clk/bcm/clk-kona.c
> >> +++ b/drivers/clk/bcm/clk-kona.c
> >> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
> >>   */
> >>  static inline void __ccu_write_enable(struct ccu_data *ccu)
> > 
> > Per Documentation/CodingStyle, chapter 15, "the inline disease", it
> > might be best to not inline these functions.
> 
> This was not intentional.  I normally only inline things
> defined in header files, and maybe this is an artifact of
> having been in a header at one time.  I don't know, I'll get
> rid of the inline.
> 
> > 
> >>  {
> >> -       if (ccu->write_enabled) {
> >> -               pr_err("%s: access already enabled for %s\n", __func__,
> >> -                       ccu->name);
> >> -               return;
> >> -       }
> >> -       ccu->write_enabled = true;
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >> +       if (!ccu->write_enabled++)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >>  }
> >>  
> >>  static inline void __ccu_write_disable(struct ccu_data *ccu)
> >> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
> >>                         ccu->name);
> >>                 return;
> >>         }
> >> -
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> >> -       ccu->write_enabled = false;
> >> +       if (!--ccu->write_enabled)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> > What happens if calls to __ccu_write_enable and __ccu_write_disable are
> > unbalanced? It would be better to catch that case and throw a WARN:
> 
> You can't see it in the diff, but that's what happens
> (well, it's a pr_err(), not a WARN()).   I think a WARN()
> is probably right in this case though.
> 
> >       if (WARN_ON(ccu->write_enabled == 0))
> >               return;
> > 
> >       if (--ccu->write_enabled > 0)
> >               return;
> > 
> >       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> >>  }
> >>  
> >>  /*
> >> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> >> index 2537b30..e9a8466 100644
> >> --- a/drivers/clk/bcm/clk-kona.h
> >> +++ b/drivers/clk/bcm/clk-kona.h
> >> @@ -478,7 +478,7 @@ struct ccu_policy {
> >>  struct ccu_data {
> >>         void __iomem *base;     /* base of mapped address space */
> >>         spinlock_t lock;        /* serialization lock */
> >> -       bool write_enabled;     /* write access is currently enabled */
> >> +       u32 write_enabled;      /* write access enable count */
> > 
> > Why u32? An unsigned int will do just nicely here.
> 
> That's a preference of mine.  I almost always favor
> using u32, etc. because they are compact, and explicit
> about the size and signedness.  I "know" an int is 32
> bits, but I still prefer being explicit.
> 
> I'll interpret this as a preference on your part for
> unsigned int, and I have no problem making that change.

It's not a big deal, I was just curious why. Feel free to use whatever
solution you prefer here.

Regards,
Mike

> 
>                                         -Alex
> 
> > Regards,
> > Mike
> > 
> >>         struct ccu_policy policy;
> >>         struct list_head links; /* for ccu_list */
> >>         struct device_node *node;
> >> -- 
> >> 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/

Patch

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index 95af2e6..ee8e988 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -170,13 +170,8 @@  static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
  */
 static inline void __ccu_write_enable(struct ccu_data *ccu)
 {
-	if (ccu->write_enabled) {
-		pr_err("%s: access already enabled for %s\n", __func__,
-			ccu->name);
-		return;
-	}
-	ccu->write_enabled = true;
-	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
+	if (!ccu->write_enabled++)
+		__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
 }
 
 static inline void __ccu_write_disable(struct ccu_data *ccu)
@@ -186,9 +181,8 @@  static inline void __ccu_write_disable(struct ccu_data *ccu)
 			ccu->name);
 		return;
 	}
-
-	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
-	ccu->write_enabled = false;
+	if (!--ccu->write_enabled)
+		__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
 }
 
 /*
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 2537b30..e9a8466 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -478,7 +478,7 @@  struct ccu_policy {
 struct ccu_data {
 	void __iomem *base;	/* base of mapped address space */
 	spinlock_t lock;	/* serialization lock */
-	bool write_enabled;	/* write access is currently enabled */
+	u32 write_enabled;	/* write access enable count */
 	struct ccu_policy policy;
 	struct list_head links;	/* for ccu_list */
 	struct device_node *node;