[PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Paul Walmsley]

Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
clockdomain wakeup and sleep.  This would probably have broken
software-supervised clockdomain wakeup and sleep on OMAP3.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   50 ++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
index a0d68db..f99e65c 100644
--- a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
+++ b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
@@ -241,6 +241,52 @@ static void omap3_clkdm_deny_idle(struct clockdomain *clkdm)
 		_clkdm_del_autodeps(clkdm);
 }
 
+static int omap3xxx_clkdm_clk_enable(struct clockdomain *clkdm)
+{
+	bool hwsup = false;
+
+	if (!clkdm->clktrctrl_mask)
+		return 0;
+
+	hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
+				clkdm->clktrctrl_mask);
+
+	if (hwsup) {
+		/* Disable HW transitions when we are changing deps */
+		_disable_hwsup(clkdm);
+		_clkdm_add_autodeps(clkdm);
+		_enable_hwsup(clkdm);
+	} else {
+		if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
+			omap3_clkdm_wakeup(clkdm);
+	}
+
+	return 0;
+}
+
+static int omap3xxx_clkdm_clk_disable(struct clockdomain *clkdm)
+{
+	bool hwsup = false;
+
+	if (!clkdm->clktrctrl_mask)
+		return 0;
+
+	hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
+				clkdm->clktrctrl_mask);
+
+	if (hwsup) {
+		/* Disable HW transitions when we are changing deps */
+		_disable_hwsup(clkdm);
+		_clkdm_del_autodeps(clkdm);
+		_enable_hwsup(clkdm);
+	} else {
+		if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
+			omap3_clkdm_sleep(clkdm);
+	}
+
+	return 0;
+}
+
 struct clkdm_ops omap2_clkdm_operations = {
 	.clkdm_add_wkdep	= omap2_clkdm_add_wkdep,
 	.clkdm_del_wkdep	= omap2_clkdm_del_wkdep,
@@ -267,6 +313,6 @@ struct clkdm_ops omap3_clkdm_operations = {
 	.clkdm_wakeup		= omap3_clkdm_wakeup,
 	.clkdm_allow_idle	= omap3_clkdm_allow_idle,
 	.clkdm_deny_idle	= omap3_clkdm_deny_idle,
-	.clkdm_clk_enable	= omap2_clkdm_clk_enable,
-	.clkdm_clk_disable	= omap2_clkdm_clk_disable,
+	.clkdm_clk_enable	= omap3xxx_clkdm_clk_enable,
+	.clkdm_clk_disable	= omap3xxx_clkdm_clk_disable,
 };
-- 
1.7.10.4

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Rajendra Nayak]

On Friday 27 July 2012 02:34 AM, Paul Walmsley wrote:
>
> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
> clockdomain wakeup and sleep.  This would probably have broken
> software-supervised clockdomain wakeup and sleep on OMAP3.

Its strange this went unnoticed for so long. Thanks for this fix and
sorry about introducing the bug in the first place.

Acked-by: Rajendra Nayak <rnayak@ti.com>

>
> Signed-off-by: Paul Walmsley<paul@pwsan.com>
> Cc: Rajendra Nayak<rnayak@ti.com>
> Cc: Jon Hunter<jon-hunter@ti.com>
> ---
>   arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   50 ++++++++++++++++++++++++++--
>   1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> index a0d68db..f99e65c 100644
> --- a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> +++ b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> @@ -241,6 +241,52 @@ static void omap3_clkdm_deny_idle(struct clockdomain *clkdm)
>   		_clkdm_del_autodeps(clkdm);
>   }
>
> +static int omap3xxx_clkdm_clk_enable(struct clockdomain *clkdm)
> +{
> +	bool hwsup = false;
> +
> +	if (!clkdm->clktrctrl_mask)
> +		return 0;
> +
> +	hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
> +				clkdm->clktrctrl_mask);
> +
> +	if (hwsup) {
> +		/* Disable HW transitions when we are changing deps */
> +		_disable_hwsup(clkdm);
> +		_clkdm_add_autodeps(clkdm);
> +		_enable_hwsup(clkdm);
> +	} else {
> +		if (clkdm->flags&  CLKDM_CAN_FORCE_WAKEUP)
> +			omap3_clkdm_wakeup(clkdm);
> +	}
> +
> +	return 0;
> +}
> +
> +static int omap3xxx_clkdm_clk_disable(struct clockdomain *clkdm)
> +{
> +	bool hwsup = false;
> +
> +	if (!clkdm->clktrctrl_mask)
> +		return 0;
> +
> +	hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
> +				clkdm->clktrctrl_mask);
> +
> +	if (hwsup) {
> +		/* Disable HW transitions when we are changing deps */
> +		_disable_hwsup(clkdm);
> +		_clkdm_del_autodeps(clkdm);
> +		_enable_hwsup(clkdm);
> +	} else {
> +		if (clkdm->flags&  CLKDM_CAN_FORCE_SLEEP)
> +			omap3_clkdm_sleep(clkdm);
> +	}
> +
> +	return 0;
> +}
> +
>   struct clkdm_ops omap2_clkdm_operations = {
>   	.clkdm_add_wkdep	= omap2_clkdm_add_wkdep,
>   	.clkdm_del_wkdep	= omap2_clkdm_del_wkdep,
> @@ -267,6 +313,6 @@ struct clkdm_ops omap3_clkdm_operations = {
>   	.clkdm_wakeup		= omap3_clkdm_wakeup,
>   	.clkdm_allow_idle	= omap3_clkdm_allow_idle,
>   	.clkdm_deny_idle	= omap3_clkdm_deny_idle,
> -	.clkdm_clk_enable	= omap2_clkdm_clk_enable,
> -	.clkdm_clk_disable	= omap2_clkdm_clk_disable,
> +	.clkdm_clk_enable	= omap3xxx_clkdm_clk_enable,
> +	.clkdm_clk_disable	= omap3xxx_clkdm_clk_disable,
>   };

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Nayak, Rajendra]

Paul,

On Fri, Jul 27, 2012 at 2:34 AM, Paul Walmsley <paul@pwsan.com> wrote:
>
> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
> clockdomain wakeup and sleep.  This would probably have broken
> software-supervised clockdomain wakeup and sleep on OMAP3.

Would something like this be better than duplicating most of
omap2_clkdm_clk_enable/disable?

---
 arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
2012-07-27 13:15:49.238134472 +0530
+++ linux-2.6/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
2012-07-27 13:22:01.607380032 +0530
@@ -162,6 +162,27 @@
                                                clkdm->clktrctrl_mask);
 }

+static void _clkdm_sleep(struct clockdomain *clkdm)
+{
+       if (cpu_is_omap24xx())
+               omap2_cm_set_mod_reg_bits(OMAP24XX_FORCESTATE_MASK,
+                                       clkdm->pwrdm.ptr->prcm_offs,
+                                       OMAP2_PM_PWSTCTRL);
+       else if (cpu_is_omap34xx())
+               omap3xxx_cm_clkdm_force_sleep(clkdm->pwrdm.ptr->prcm_offs,
+                                       clkdm->clktrctrl_mask);
+}
+
+static void _clkdm_wakeup(struct clockdomain *clkdm)
+{
+       if (cpu_is_omap24xx())
+               omap2_cm_clear_mod_reg_bits(OMAP24XX_FORCESTATE_MASK,
+                                       clkdm->pwrdm.ptr->prcm_offs,
+                                       OMAP2_PM_PWSTCTRL);
+       else if (cpu_is_omap34xx())
+               omap3xxx_cm_clkdm_force_wakeup(clkdm->pwrdm.ptr->prcm_offs,
+                                       clkdm->clktrctrl_mask);
+}

 static int omap2_clkdm_clk_enable(struct clockdomain *clkdm)
 {
@@ -180,7 +201,7 @@
                _enable_hwsup(clkdm);
        } else {
                if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
-                       omap2_clkdm_wakeup(clkdm);
+                       _clkdm_wakeup(clkdm);
        }

        return 0;
@@ -203,7 +224,7 @@
                _enable_hwsup(clkdm);
        } else {
                if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
-                       omap2_clkdm_sleep(clkdm);
+                       _clkdm_sleep(clkdm);
        }

>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   50 ++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> index a0d68db..f99e65c 100644
> --- a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> +++ b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> @@ -241,6 +241,52 @@ static void omap3_clkdm_deny_idle(struct clockdomain *clkdm)
>                 _clkdm_del_autodeps(clkdm);
>  }
>
> +static int omap3xxx_clkdm_clk_enable(struct clockdomain *clkdm)
> +{
> +       bool hwsup = false;
> +
> +       if (!clkdm->clktrctrl_mask)
> +               return 0;
> +
> +       hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
> +                               clkdm->clktrctrl_mask);
> +
> +       if (hwsup) {
> +               /* Disable HW transitions when we are changing deps */
> +               _disable_hwsup(clkdm);
> +               _clkdm_add_autodeps(clkdm);
> +               _enable_hwsup(clkdm);
> +       } else {
> +               if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
> +                       omap3_clkdm_wakeup(clkdm);
> +       }
> +
> +       return 0;
> +}
> +
> +static int omap3xxx_clkdm_clk_disable(struct clockdomain *clkdm)
> +{
> +       bool hwsup = false;
> +
> +       if (!clkdm->clktrctrl_mask)
> +               return 0;
> +
> +       hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
> +                               clkdm->clktrctrl_mask);
> +
> +       if (hwsup) {
> +               /* Disable HW transitions when we are changing deps */
> +               _disable_hwsup(clkdm);
> +               _clkdm_del_autodeps(clkdm);
> +               _enable_hwsup(clkdm);
> +       } else {
> +               if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
> +                       omap3_clkdm_sleep(clkdm);
> +       }
> +
> +       return 0;
> +}
> +
>  struct clkdm_ops omap2_clkdm_operations = {
>         .clkdm_add_wkdep        = omap2_clkdm_add_wkdep,
>         .clkdm_del_wkdep        = omap2_clkdm_del_wkdep,
> @@ -267,6 +313,6 @@ struct clkdm_ops omap3_clkdm_operations = {
>         .clkdm_wakeup           = omap3_clkdm_wakeup,
>         .clkdm_allow_idle       = omap3_clkdm_allow_idle,
>         .clkdm_deny_idle        = omap3_clkdm_deny_idle,
> -       .clkdm_clk_enable       = omap2_clkdm_clk_enable,
> -       .clkdm_clk_disable      = omap2_clkdm_clk_disable,
> +       .clkdm_clk_enable       = omap3xxx_clkdm_clk_enable,
> +       .clkdm_clk_disable      = omap3xxx_clkdm_clk_disable,
>  };
> --
> 1.7.10.4
>

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Kevin Hilman]

"Nayak, Rajendra" <rnayak@ti.com> writes:

> Paul,
>
> On Fri, Jul 27, 2012 at 2:34 AM, Paul Walmsley <paul@pwsan.com> wrote:
>>
>> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
>> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
>> clockdomain wakeup and sleep.  This would probably have broken
>> software-supervised clockdomain wakeup and sleep on OMAP3.
>
> Would something like this be better than duplicating most of
> omap2_clkdm_clk_enable/disable?

Not when using cpu_is* checks at runtime.  Maybe if you setup function
pointers at init time?

Kevin

> ---
>  arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> ===================================================================
> --- linux-2.6.orig/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> 2012-07-27 13:15:49.238134472 +0530
> +++ linux-2.6/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
> 2012-07-27 13:22:01.607380032 +0530
> @@ -162,6 +162,27 @@
>                                                 clkdm->clktrctrl_mask);
>  }
>
> +static void _clkdm_sleep(struct clockdomain *clkdm)
> +{
> +       if (cpu_is_omap24xx())
> +               omap2_cm_set_mod_reg_bits(OMAP24XX_FORCESTATE_MASK,
> +                                       clkdm->pwrdm.ptr->prcm_offs,
> +                                       OMAP2_PM_PWSTCTRL);
> +       else if (cpu_is_omap34xx())
> +               omap3xxx_cm_clkdm_force_sleep(clkdm->pwrdm.ptr->prcm_offs,
> +                                       clkdm->clktrctrl_mask);
> +}
> +
> +static void _clkdm_wakeup(struct clockdomain *clkdm)
> +{
> +       if (cpu_is_omap24xx())
> +               omap2_cm_clear_mod_reg_bits(OMAP24XX_FORCESTATE_MASK,
> +                                       clkdm->pwrdm.ptr->prcm_offs,
> +                                       OMAP2_PM_PWSTCTRL);
> +       else if (cpu_is_omap34xx())
> +               omap3xxx_cm_clkdm_force_wakeup(clkdm->pwrdm.ptr->prcm_offs,
> +                                       clkdm->clktrctrl_mask);
> +}
>
>  static int omap2_clkdm_clk_enable(struct clockdomain *clkdm)
>  {
> @@ -180,7 +201,7 @@
>                 _enable_hwsup(clkdm);
>         } else {
>                 if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
> -                       omap2_clkdm_wakeup(clkdm);
> +                       _clkdm_wakeup(clkdm);
>         }
>
>         return 0;
> @@ -203,7 +224,7 @@
>                 _enable_hwsup(clkdm);
>         } else {
>                 if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
> -                       omap2_clkdm_sleep(clkdm);
> +                       _clkdm_sleep(clkdm);
>         }
>
>>
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Jon Hunter <jon-hunter@ti.com>
>> ---
>>  arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |   50 ++++++++++++++++++++++++++--
>>  1 file changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
>> index a0d68db..f99e65c 100644
>> --- a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
>> +++ b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
>> @@ -241,6 +241,52 @@ static void omap3_clkdm_deny_idle(struct clockdomain *clkdm)
>>                 _clkdm_del_autodeps(clkdm);
>>  }
>>
>> +static int omap3xxx_clkdm_clk_enable(struct clockdomain *clkdm)
>> +{
>> +       bool hwsup = false;
>> +
>> +       if (!clkdm->clktrctrl_mask)
>> +               return 0;
>> +
>> +       hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
>> +                               clkdm->clktrctrl_mask);
>> +
>> +       if (hwsup) {
>> +               /* Disable HW transitions when we are changing deps */
>> +               _disable_hwsup(clkdm);
>> +               _clkdm_add_autodeps(clkdm);
>> +               _enable_hwsup(clkdm);
>> +       } else {
>> +               if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
>> +                       omap3_clkdm_wakeup(clkdm);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int omap3xxx_clkdm_clk_disable(struct clockdomain *clkdm)
>> +{
>> +       bool hwsup = false;
>> +
>> +       if (!clkdm->clktrctrl_mask)
>> +               return 0;
>> +
>> +       hwsup = omap2_cm_is_clkdm_in_hwsup(clkdm->pwrdm.ptr->prcm_offs,
>> +                               clkdm->clktrctrl_mask);
>> +
>> +       if (hwsup) {
>> +               /* Disable HW transitions when we are changing deps */
>> +               _disable_hwsup(clkdm);
>> +               _clkdm_del_autodeps(clkdm);
>> +               _enable_hwsup(clkdm);
>> +       } else {
>> +               if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
>> +                       omap3_clkdm_sleep(clkdm);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  struct clkdm_ops omap2_clkdm_operations = {
>>         .clkdm_add_wkdep        = omap2_clkdm_add_wkdep,
>>         .clkdm_del_wkdep        = omap2_clkdm_del_wkdep,
>> @@ -267,6 +313,6 @@ struct clkdm_ops omap3_clkdm_operations = {
>>         .clkdm_wakeup           = omap3_clkdm_wakeup,
>>         .clkdm_allow_idle       = omap3_clkdm_allow_idle,
>>         .clkdm_deny_idle        = omap3_clkdm_deny_idle,
>> -       .clkdm_clk_enable       = omap2_clkdm_clk_enable,
>> -       .clkdm_clk_disable      = omap2_clkdm_clk_disable,
>> +       .clkdm_clk_enable       = omap3xxx_clkdm_clk_enable,
>> +       .clkdm_clk_disable      = omap3xxx_clkdm_clk_disable,
>>  };
>> --
>> 1.7.10.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Paul Walmsley]

Hi

On Fri, 27 Jul 2012, Nayak, Rajendra wrote:

> On Fri, Jul 27, 2012 at 2:34 AM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
> > clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
> > clockdomain wakeup and sleep.  This would probably have broken
> > software-supervised clockdomain wakeup and sleep on OMAP3.
> 
> Would something like this be better than duplicating most of
> omap2_clkdm_clk_enable/disable?

Am working towards getting the cpu_is_*() calls out of there in 
preparation for the move to drivers/.  In fact there are some patches here 
that will be posted shortly to get rid of those clkdm_clk_(enable|disable) 
functions completely.


- Paul

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Jon Hunter]

Hi Paul, Rajendra,

On 07/27/2012 12:43 AM, Rajendra Nayak wrote:
> On Friday 27 July 2012 02:34 AM, Paul Walmsley wrote:
>>
>> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
>> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
>> clockdomain wakeup and sleep.  This would probably have broken
>> software-supervised clockdomain wakeup and sleep on OMAP3.
> 
> Its strange this went unnoticed for so long. Thanks for this fix and
> sorry about introducing the bug in the first place.

Any chance that's because of the following code? I needed to
remove this to get the EMU clock domain to turn off on OMAP3
whilst testing PMU.

Cheers
Jon

commit a0307bd539ecef976793679a1c4ff0d47b22c4bd
Author: Jon Hunter <jon-hunter@ti.com>
Date:   Mon Jul 30 18:04:06 2012 -0500

    ARM: OMAP2/3: Allow HWMOD to disable clock domains
    
    Currently when HWMOD attempts to disable a clock domain on OMAP2/3 devices we
    will return from the function clkdm_hwmod_disable() without actually disabling
    the clock domain. Per the comment this is deliberate because initially HWMOD
    OMAP2/3 devices did not support clock domains. However, clock domains are now
    supported by HWMOD for these devices and so allow HWMOD to disable the clock
    domains.
    
    XXX - Tested on OMAP3430 beagle board, but needs more testing on OMAP2/3

diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
index 011186f..8f7a941 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -1075,10 +1075,6 @@ int clkdm_hwmod_enable(struct clockdomain *clkdm, struct omap_hwmod *oh)
  */
 int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh)
 {
-       /* The clkdm attribute does not exist yet prior OMAP4 */
-       if (cpu_is_omap24xx() || cpu_is_omap34xx())
-               return 0;
-
        /*
         * XXX Rewrite this code to maintain a list of enabled
         * downstream hwmods for debugging purposes?

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Rajendra Nayak]

Hi Jon,

On Tuesday 31 July 2012 10:11 AM, Jon Hunter wrote:
> Hi Paul, Rajendra,
>
> On 07/27/2012 12:43 AM, Rajendra Nayak wrote:
>> On Friday 27 July 2012 02:34 AM, Paul Walmsley wrote:
>>>
>>> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
>>> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
>>> clockdomain wakeup and sleep.  This would probably have broken
>>> software-supervised clockdomain wakeup and sleep on OMAP3.
>>
>> Its strange this went unnoticed for so long. Thanks for this fix and
>> sorry about introducing the bug in the first place.
>
> Any chance that's because of the following code? I needed to
> remove this to get the EMU clock domain to turn off on OMAP3
> whilst testing PMU.

No, this doesn't seem right. We still have clockdomains for omap2/3
controlled using clkdm_clk_enable/disable functions called from
within clk framework, and not clkdm_hwmod_enable/disable from
within hwmod framework.
Besides you removing these checks only in clkdm_hwmod_disable (and
keeping them in clkdm_hwmod_enable) tells me its just hiding some
usecounting issues you were having with clkdm_clk_enable/disable.

Btw, on OMAP2/3 as long as a domain has interface clocks which are
explicitly enabled and autoidled, the clkdm usecount never ends up
going to 0, which is probably what you are hit with too.

regards,
Rajendra

>
> Cheers
> Jon
>
> commit a0307bd539ecef976793679a1c4ff0d47b22c4bd
> Author: Jon Hunter<jon-hunter@ti.com>
> Date:   Mon Jul 30 18:04:06 2012 -0500
>
>      ARM: OMAP2/3: Allow HWMOD to disable clock domains
>
>      Currently when HWMOD attempts to disable a clock domain on OMAP2/3 devices we
>      will return from the function clkdm_hwmod_disable() without actually disabling
>      the clock domain. Per the comment this is deliberate because initially HWMOD
>      OMAP2/3 devices did not support clock domains. However, clock domains are now
>      supported by HWMOD for these devices and so allow HWMOD to disable the clock
>      domains.
>
>      XXX - Tested on OMAP3430 beagle board, but needs more testing on OMAP2/3
>
> diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
> index 011186f..8f7a941 100644
> --- a/arch/arm/mach-omap2/clockdomain.c
> +++ b/arch/arm/mach-omap2/clockdomain.c
> @@ -1075,10 +1075,6 @@ int clkdm_hwmod_enable(struct clockdomain *clkdm, struct omap_hwmod *oh)
>    */
>   int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh)
>   {
> -       /* The clkdm attribute does not exist yet prior OMAP4 */
> -       if (cpu_is_omap24xx() || cpu_is_omap34xx())
> -               return 0;
> -
>          /*
>           * XXX Rewrite this code to maintain a list of enabled
>           * downstream hwmods for debugging purposes?
>

Re: [PATCH] ARM: OMAP3xxx: clockdomain: fix software supervised wakeup/sleep

[Jon Hunter]

Hi Rajendra,

On 07/31/2012 12:41 AM, Rajendra Nayak wrote:
> Hi Jon,
> 
> On Tuesday 31 July 2012 10:11 AM, Jon Hunter wrote:
>> Hi Paul, Rajendra,
>>
>> On 07/27/2012 12:43 AM, Rajendra Nayak wrote:
>>> On Friday 27 July 2012 02:34 AM, Paul Walmsley wrote:
>>>>
>>>> Commit 4da71ae6 ("OMAP: clockdomain: Arch specific funcs for
>>>> clkdm_clk_enable/disable") called the OMAP2xxx-specific functions for
>>>> clockdomain wakeup and sleep.  This would probably have broken
>>>> software-supervised clockdomain wakeup and sleep on OMAP3.
>>>
>>> Its strange this went unnoticed for so long. Thanks for this fix and
>>> sorry about introducing the bug in the first place.
>>
>> Any chance that's because of the following code? I needed to
>> remove this to get the EMU clock domain to turn off on OMAP3
>> whilst testing PMU.
> 
> No, this doesn't seem right. We still have clockdomains for omap2/3
> controlled using clkdm_clk_enable/disable functions called from
> within clk framework, and not clkdm_hwmod_enable/disable from
> within hwmod framework.
> Besides you removing these checks only in clkdm_hwmod_disable (and
> keeping them in clkdm_hwmod_enable) tells me its just hiding some
> usecounting issues you were having with clkdm_clk_enable/disable.

Yes you are right. I was focused in the disable side because the EMU CD
is staying on.

> Btw, on OMAP2/3 as long as a domain has interface clocks which are
> explicitly enabled and autoidled, the clkdm usecount never ends up
> going to 0, which is probably what you are hit with too.

Yes so after further debugging, this is not a problem and not the cause
of my problem either.

Sorry for the noise.

Cheers
Jon