RE: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

2010-03-09 Thread Tero.Kristo 
 

-Original Message-
From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] 
Sent: 08 March, 2010 19:16
To: Kristo Tero (Nokia-D/Tampere)
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for 
suspending to INACTIVE state

tero.kri...@nokia.com writes:

[...]

 True, ancient info there. OFF for example has been supported 
for ages already.



 +  if (state != PWRDM_POWER_INACTIVE)
 +  while (!(pwrdm-pwrsts  (1  state))) {
 +  if (state == PWRDM_POWER_OFF)
 +  return ret;
 +  state--;
 +  }

I think all powerdomains can be inactive right?

 Yes.

I think it would be cleaner to just have all the pwrdm-pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

 Yeah, something like this was done previously, however Paul did not
 like the idea of changing the generic powerdomain code too much so I
 dropped it completely. It is now done only via the support functions
 in patch #1, and only done for the powerdomains that actually need
 it for the cpuidle (mpu/core/neon.) It would be possible to add
 support for the rest of the powerdomains also, but I decided to drop
 this in favor of getting the patch set in.

I'm not proposing changing any of the other powerdomain code.  Just
changing the PWRSTS_* defines, essentially so that INACTIVE is
a valid state.

That will eliminate the need for a special check for inactive in this
patch.

This is a chicken-egg problem. If you alter the PWRSTS_* defines, you need to 
change implementation of pwrdm_set_next_pwrst() as it would accept INACTIVE 
also, which is not supported by the code right now.


Kevin


diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
 #define PWRDM_MAX_PWRSTS4
 
 /* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON   ((1  PWRDM_POWER_OFF) | \
+#define PWRSTS_ON   ((1  PWRDM_POWER_INACTIVE) | \
  (1  PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_ON   ((1  PWRDM_POWER_OFF) 
| PWRSTS_ON)
+
 #define PWRSTS_OFF_RET  ((1  PWRDM_POWER_OFF) | \
  (1  PWRDM_POWER_RET))
 
-#define PWRSTS_RET_ON   ((1  PWRDM_POWER_RET) | \
- (1  PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON   (PWRSTS_OFF_RET | (1  PWRDM_POWER_ON))
+#define PWRSTS_RET_ON   ((1  PWRDM_POWER_RET) 
| PWRSTS_ON)
 
+#define PWRSTS_OFF_RET_ON   (PWRSTS_OFF_RET | PWRSTS_ON)
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR  (1  0) /* hardware 
save-and-restore support */

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

2010-03-09 Thread Kevin Hilman 
tero.kri...@nokia.com writes:

  

-Original Message-
From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] 
Sent: 08 March, 2010 19:16
To: Kristo Tero (Nokia-D/Tampere)
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for 
suspending to INACTIVE state

tero.kri...@nokia.com writes:

[...]

 True, ancient info there. OFF for example has been supported 
for ages already.



 + if (state != PWRDM_POWER_INACTIVE)
 + while (!(pwrdm-pwrsts  (1  state))) {
 + if (state == PWRDM_POWER_OFF)
 + return ret;
 + state--;
 + }

I think all powerdomains can be inactive right?

 Yes.

I think it would be cleaner to just have all the pwrdm-pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

 Yeah, something like this was done previously, however Paul did not
 like the idea of changing the generic powerdomain code too much so I
 dropped it completely. It is now done only via the support functions
 in patch #1, and only done for the powerdomains that actually need
 it for the cpuidle (mpu/core/neon.) It would be possible to add
 support for the rest of the powerdomains also, but I decided to drop
 this in favor of getting the patch set in.

I'm not proposing changing any of the other powerdomain code.  Just
changing the PWRSTS_* defines, essentially so that INACTIVE is
a valid state.

That will eliminate the need for a special check for inactive in this
patch.

 This is a chicken-egg problem. If you alter the PWRSTS_* defines,
 you need to change implementation of pwrdm_set_next_pwrst() as it
 would accept INACTIVE also, which is not supported by the code right
 now.

OK, I see the chicken-egg problem now.  

You're original version is ok with me.

Thanks,

Kevin


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

2010-03-08 Thread Kevin Hilman 
tero.kri...@nokia.com writes:

[...]

 True, ancient info there. OFF for example has been supported for ages already.



 +   if (state != PWRDM_POWER_INACTIVE)
 +   while (!(pwrdm-pwrsts  (1  state))) {
 +   if (state == PWRDM_POWER_OFF)
 +   return ret;
 +   state--;
 +   }

I think all powerdomains can be inactive right?

 Yes.

I think it would be cleaner to just have all the pwrdm-pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

 Yeah, something like this was done previously, however Paul did not
 like the idea of changing the generic powerdomain code too much so I
 dropped it completely. It is now done only via the support functions
 in patch #1, and only done for the powerdomains that actually need
 it for the cpuidle (mpu/core/neon.) It would be possible to add
 support for the rest of the powerdomains also, but I decided to drop
 this in favor of getting the patch set in.

I'm not proposing changing any of the other powerdomain code.  Just
changing the PWRSTS_* defines, essentially so that INACTIVE is
a valid state.

That will eliminate the need for a special check for inactive in this
patch.

Kevin


diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
 #define PWRDM_MAX_PWRSTS 4
 
 /* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON((1  PWRDM_POWER_OFF) | \
+#define PWRSTS_ON((1  PWRDM_POWER_INACTIVE) | \
   (1  PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_ON((1  PWRDM_POWER_OFF) | PWRSTS_ON)
+
 #define PWRSTS_OFF_RET   ((1  PWRDM_POWER_OFF) | \
   (1  PWRDM_POWER_RET))
 
-#define PWRSTS_RET_ON((1  PWRDM_POWER_RET) | \
-  (1  PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON(PWRSTS_OFF_RET | (1  PWRDM_POWER_ON))
+#define PWRSTS_RET_ON((1  PWRDM_POWER_RET) | PWRSTS_ON)
 
+#define PWRSTS_OFF_RET_ON(PWRSTS_OFF_RET | PWRSTS_ON)
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR   (1  0) /* hardware 
save-and-restore support */

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

2010-03-02 Thread Tero.Kristo 
 

-Original Message-
From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] 
Sent: 02 March, 2010 01:43
To: Kristo Tero (Nokia-D/Tampere)
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for 
suspending to INACTIVE state

Tero Kristo tero.kri...@nokia.com writes:

 From: Tero Kristo tero.kri...@nokia.com

 With the new support functions this is now possible. 
Suspending to INACTIVE
 is useful for testing purposes.

 Signed-off-by: Tero Kristo tero.kri...@nokia.com
 ---
  arch/arm/mach-omap2/pm34xx.c |   11 ++-
  1 files changed, 6 insertions(+), 5 deletions(-)

 diff --git a/arch/arm/mach-omap2/pm34xx.c 
b/arch/arm/mach-omap2/pm34xx.c
 index cdbedcf..41d6cc3 100644
 --- a/arch/arm/mach-omap2/pm34xx.c
 +++ b/arch/arm/mach-omap2/pm34xx.c
 @@ -633,11 +633,12 @@ int set_pwrdm_state(struct powerdomain 
*pwrdm, u32 state)
  if (pwrdm == NULL || IS_ERR(pwrdm))
  return -EINVAL;
  
 -while (!(pwrdm-pwrsts  (1  state))) {
 -if (state == PWRDM_POWER_OFF)
 -return ret;
 -state--;
 -}

The comment above set_pwrdm_state() says only ON  RET are supported.
Please update that comment as well.

True, ancient info there. OFF for example has been supported for ages already.



 +if (state != PWRDM_POWER_INACTIVE)
 +while (!(pwrdm-pwrsts  (1  state))) {
 +if (state == PWRDM_POWER_OFF)
 +return ret;
 +state--;
 +}

I think all powerdomains can be inactive right?

Yes.

I think it would be cleaner to just have all the pwrdm-pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

Yeah, something like this was done previously, however Paul did not like the 
idea of changing the generic powerdomain code too much so I dropped it 
completely. It is now done only via the support functions in patch #1, and only 
done for the powerdomains that actually need it for the cpuidle 
(mpu/core/neon.) It would be possible to add support for the rest of the 
powerdomains also, but I decided to drop this in favor of getting the patch set 
in.


Kevin

diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
 #define PWRDM_MAX_PWRSTS  4
 
 /* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON ((1  PWRDM_POWER_OFF) | \
+#define PWRSTS_ON ((1  PWRDM_POWER_INACTIVE) | \
(1  PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_ON ((1  PWRDM_POWER_OFF) | PWRSTS_ON)
+
 #define PWRSTS_OFF_RET((1  PWRDM_POWER_OFF) | \
(1  PWRDM_POWER_RET))
 
-#define PWRSTS_RET_ON ((1  PWRDM_POWER_RET) | \
-   (1  PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | (1  PWRDM_POWER_ON))
+#define PWRSTS_RET_ON ((1  PWRDM_POWER_RET) | PWRSTS_ON)
 
+#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON)
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR(1  0) /* hardware 
save-and-restore support */
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

2010-03-01 Thread Kevin Hilman 
Tero Kristo tero.kri...@nokia.com writes:

 From: Tero Kristo tero.kri...@nokia.com

 With the new support functions this is now possible. Suspending to INACTIVE
 is useful for testing purposes.

 Signed-off-by: Tero Kristo tero.kri...@nokia.com
 ---
  arch/arm/mach-omap2/pm34xx.c |   11 ++-
  1 files changed, 6 insertions(+), 5 deletions(-)

 diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
 index cdbedcf..41d6cc3 100644
 --- a/arch/arm/mach-omap2/pm34xx.c
 +++ b/arch/arm/mach-omap2/pm34xx.c
 @@ -633,11 +633,12 @@ int set_pwrdm_state(struct powerdomain *pwrdm, u32 
 state)
   if (pwrdm == NULL || IS_ERR(pwrdm))
   return -EINVAL;
  
 - while (!(pwrdm-pwrsts  (1  state))) {
 - if (state == PWRDM_POWER_OFF)
 - return ret;
 - state--;
 - }

The comment above set_pwrdm_state() says only ON  RET are supported.
Please update that comment as well.


 + if (state != PWRDM_POWER_INACTIVE)
 + while (!(pwrdm-pwrsts  (1  state))) {
 + if (state == PWRDM_POWER_OFF)
 + return ret;
 + state--;
 + }

I think all powerdomains can be inactive right?

I think it would be cleaner to just have all the pwrdm-pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

Kevin

diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
 #define PWRDM_MAX_PWRSTS   4
 
 /* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON  ((1  PWRDM_POWER_OFF) | \
+#define PWRSTS_ON  ((1  PWRDM_POWER_INACTIVE) | \
 (1  PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_ON  ((1  PWRDM_POWER_OFF) | PWRSTS_ON)
+
 #define PWRSTS_OFF_RET ((1  PWRDM_POWER_OFF) | \
 (1  PWRDM_POWER_RET))
 
-#define PWRSTS_RET_ON  ((1  PWRDM_POWER_RET) | \
-(1  PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON  (PWRSTS_OFF_RET | (1  PWRDM_POWER_ON))
+#define PWRSTS_RET_ON  ((1  PWRDM_POWER_RET) | PWRSTS_ON)
 
+#define PWRSTS_OFF_RET_ON  (PWRSTS_OFF_RET | PWRSTS_ON)
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR (1  0) /* hardware save-and-restore support */
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html