Re: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-30 Thread Tero Kristo
On Tue, 2012-05-29 at 11:31 -0700, Kevin Hilman wrote:
 Tero Kristo t-kri...@ti.com writes:
 
  On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
  +Jean for functional power states
  
  Tero Kristo t-kri...@ti.com writes:
  
   This patch adds device off support to OMAP4 device type.
  
  Description is rather thin for a patch that is doing so much.
  
   OFF mode is disabled by default, 
  
  why?
 
  Good question. For historical reference I guess. The device off works
  pretty nicely with the current kernel already, so it should be possible
  to enable it by default and blame the people who break it.
 
  
   however, there are two ways to enable OFF mode:
   a) In the board file, call omap4_pm_off_mode_enable(1)
   b) Enable OFF mode using the debugfs entry
   echo 1/sys/kernel/debug/pm_debug/enable_off_mode
   (conversely echo '0' will disable it as well).
  
  This part needs to be a separate patch.
  
  But, as stated in the core retention series, I'd like to move away from
  these global flags all together.
  
  The way we manage the disabling of certain states (like off) is already
  clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
  this feature needs to be supported by using constraints on functional
  power states.   Having checks all over the place is getting unwieldy and
  not attractive to maintain.
  
  The combination of constraints and functional power states should make
  this much more manageable.   Until we have that, I'd prefer to keep
  the debugfs enable/disable stuff as separate patches at the end of the
  series used only for testing.
 
  Okay, this sounds like a good plan.
 
  
   Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
   [t-kri...@ti.com: largely re-structured the code]
  
  then the sign-off above from Santosh probably doesn't apply anymore.
  You should change that to a Cc and just mention tht this is based upon
  some original work from Santosh.
 
  Yeah... I am not quite sure where the line goes here as I am modifying
  the patches quite heavily but try to keep credits to the original
  authors... will change this like so.
 
 I guess it's up to you whether you keep Santosh as author.  It all
 depends how much you've changed the original.  But you can use the
 changlog to give credits to Santosh, or state it was a collaboration,
 whatever you like.  You can say stuff like based on an origianl patch
 by..., and/or briefly summarize the changes you made from the original
 verions, etc.
 
  
  First,  some general comments:
  
  There is a lot going on in this patch, so it is hard to follow what all
  is related, and why.  Just a quick glance suggests it needs to be broken
  up into at least a few parts:
 
  What is the merge plan for the func power state stuff? I don't want to
  create new dependencies if unnecessary. Otherwise the split should be
  okay.
  
  - low-level PRM support: new APIs for various off-mode features)
(should probably be done on top of functional power states)
  - powerdomain core support or achievable states
(should probably be done on top of functional power states)
  - IRQ/GIC context save/restore
  - secure RAM save/restore (this has been tightly coupled to the GIC
but it's not obvious why)
 
  This is tightly coupled to GIC because the ROM code has following API
  calls:
 
  - save gic
  - save secure RAM
  - save secure all (gic + RAM + some other mysterious stuff)
 
  It is difficult/impossible to separate these without adding redundant
  code execution (e.g. doing a GIC save from the GIC code + then doing a
  second GIC save with save secure all from core PM code.)
 
 Ok, thanks for clarifying.
 
 That should be explained in the changelog for this patch when it's
 broken up.

Ok will add some comment about this.

 
  - PM debug support to enable/disable off-mode
(for testing only, not for merge)
  
   Signed-off-by: Tero Kristo t-kri...@ti.com
   ---
arch/arm/mach-omap2/omap-mpuss-lowpower.c |   10 -
arch/arm/mach-omap2/omap-wakeupgen.c  |   47 +++-
arch/arm/mach-omap2/pm-debug.c|   17 +--
arch/arm/mach-omap2/pm.h  |   20 +
arch/arm/mach-omap2/pm44xx.c  |   45 +++
arch/arm/mach-omap2/prm44xx.c |   66 
   +
6 files changed, 197 insertions(+), 8 deletions(-)
  
   diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c 
   b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
   index e02c082..7418e7c 100644
   --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
   +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
   @@ -60,6 +60,7 @@
#include prcm44xx.h
#include prm44xx.h
#include prm-regbits-44xx.h
   +#include cm44xx.h

#ifdef CONFIG_SMP

   @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned 
   int power_state)
 * In MPUSS OSWR or device OFF, interrupt controller  contest 
   is lost.

Re: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-29 Thread Kevin Hilman
Tero Kristo t-kri...@ti.com writes:

 On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
 +Jean for functional power states
 
 Tero Kristo t-kri...@ti.com writes:
 
  This patch adds device off support to OMAP4 device type.
 
 Description is rather thin for a patch that is doing so much.
 
  OFF mode is disabled by default, 
 
 why?

 Good question. For historical reference I guess. The device off works
 pretty nicely with the current kernel already, so it should be possible
 to enable it by default and blame the people who break it.

 
  however, there are two ways to enable OFF mode:
  a) In the board file, call omap4_pm_off_mode_enable(1)
  b) Enable OFF mode using the debugfs entry
  echo 1/sys/kernel/debug/pm_debug/enable_off_mode
  (conversely echo '0' will disable it as well).
 
 This part needs to be a separate patch.
 
 But, as stated in the core retention series, I'd like to move away from
 these global flags all together.
 
 The way we manage the disabling of certain states (like off) is already
 clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
 this feature needs to be supported by using constraints on functional
 power states.   Having checks all over the place is getting unwieldy and
 not attractive to maintain.
 
 The combination of constraints and functional power states should make
 this much more manageable.   Until we have that, I'd prefer to keep
 the debugfs enable/disable stuff as separate patches at the end of the
 series used only for testing.

 Okay, this sounds like a good plan.

 
  Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
  [t-kri...@ti.com: largely re-structured the code]
 
 then the sign-off above from Santosh probably doesn't apply anymore.
 You should change that to a Cc and just mention tht this is based upon
 some original work from Santosh.

 Yeah... I am not quite sure where the line goes here as I am modifying
 the patches quite heavily but try to keep credits to the original
 authors... will change this like so.

I guess it's up to you whether you keep Santosh as author.  It all
depends how much you've changed the original.  But you can use the
changlog to give credits to Santosh, or state it was a collaboration,
whatever you like.  You can say stuff like based on an origianl patch
by..., and/or briefly summarize the changes you made from the original
verions, etc.

 
 First,  some general comments:
 
 There is a lot going on in this patch, so it is hard to follow what all
 is related, and why.  Just a quick glance suggests it needs to be broken
 up into at least a few parts:

 What is the merge plan for the func power state stuff? I don't want to
 create new dependencies if unnecessary. Otherwise the split should be
 okay.
 
 - low-level PRM support: new APIs for various off-mode features)
   (should probably be done on top of functional power states)
 - powerdomain core support or achievable states
   (should probably be done on top of functional power states)
 - IRQ/GIC context save/restore
 - secure RAM save/restore (this has been tightly coupled to the GIC
   but it's not obvious why)

 This is tightly coupled to GIC because the ROM code has following API
 calls:

 - save gic
 - save secure RAM
 - save secure all (gic + RAM + some other mysterious stuff)

 It is difficult/impossible to separate these without adding redundant
 code execution (e.g. doing a GIC save from the GIC code + then doing a
 second GIC save with save secure all from core PM code.)

Ok, thanks for clarifying.

That should be explained in the changelog for this patch when it's
broken up.

 - PM debug support to enable/disable off-mode
   (for testing only, not for merge)
 
  Signed-off-by: Tero Kristo t-kri...@ti.com
  ---
   arch/arm/mach-omap2/omap-mpuss-lowpower.c |   10 -
   arch/arm/mach-omap2/omap-wakeupgen.c  |   47 +++-
   arch/arm/mach-omap2/pm-debug.c|   17 +--
   arch/arm/mach-omap2/pm.h  |   20 +
   arch/arm/mach-omap2/pm44xx.c  |   45 +++
   arch/arm/mach-omap2/prm44xx.c |   66 
  +
   6 files changed, 197 insertions(+), 8 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c 
  b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  index e02c082..7418e7c 100644
  --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  @@ -60,6 +60,7 @@
   #include prcm44xx.h
   #include prm44xx.h
   #include prm-regbits-44xx.h
  +#include cm44xx.h
   
   #ifdef CONFIG_SMP
   
  @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned 
  int power_state)
  * In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
  */
 mpuss_clear_prev_logic_pwrst();
  -  if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) 
  +  if (omap4_device_next_state_off())
  +  save_state = 3;
 
 Why?
 
 I don't see why this check is needed in 

Re: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-29 Thread Kevin Hilman
Jean Pihet jean.pi...@newoldbits.com writes:

 Hi Tero, Kevin, Santosh,

 On Mon, May 21, 2012 at 10:48 AM, Tero Kristo t-kri...@ti.com wrote:
 On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
 +Jean for functional power states

 Tero Kristo t-kri...@ti.com writes:

  This patch adds device off support to OMAP4 device type.

 Description is rather thin for a patch that is doing so much.

  OFF mode is disabled by default,

 why?

 Good question. For historical reference I guess. The device off works
 pretty nicely with the current kernel already, so it should be possible
 to enable it by default and blame the people who break it.
 Agree. The next problem is 'who will fix the breakage?'.

  however, there are two ways to enable OFF mode:
  a) In the board file, call omap4_pm_off_mode_enable(1)
  b) Enable OFF mode using the debugfs entry
  echo 1/sys/kernel/debug/pm_debug/enable_off_mode
  (conversely echo '0' will disable it as well).

 This part needs to be a separate patch.

 But, as stated in the core retention series, I'd like to move away from
 these global flags all together.

 The way we manage the disabling of certain states (like off) is already
 clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
 this feature needs to be supported by using constraints on functional
 power states.   Having checks all over the place is getting unwieldy and
 not attractive to maintain.

 The combination of constraints and functional power states should make
 this much more manageable.   Until we have that, I'd prefer to keep
 the debugfs enable/disable stuff as separate patches at the end of the
 series used only for testing.

 Okay, this sounds like a good plan.


  Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
  [t-kri...@ti.com: largely re-structured the code]

 then the sign-off above from Santosh probably doesn't apply anymore.
 You should change that to a Cc and just mention tht this is based upon
 some original work from Santosh.

 Yeah... I am not quite sure where the line goes here as I am modifying
 the patches quite heavily but try to keep credits to the original
 authors... will change this like so.


 First,  some general comments:

 There is a lot going on in this patch, so it is hard to follow what all
 is related, and why.  Just a quick glance suggests it needs to be broken
 up into at least a few parts:

 What is the merge plan for the func power state stuff? I don't want to
 create new dependencies if unnecessary. Otherwise the split should be
 okay.
 That is something I am interested in too ;p
 I did port and test Tero's patches on top of the functional power
 states, which is the logical approach to me. Now given the reviews
 status on both this patch series and the func power states series I am
 not sure which one needs to be ported on top of the other ;-|
 [1] https://gitorious.org/jpihet/linux-omap/commits/omap4_dev_off

 Using the functional power states for the OMAP4 core and device off
 has some advantages in simplifying the code, especially in the
 previous/current/next states checking and programming.

For these reasons, I suggest basing this series on top of the functional
power states series.

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: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-21 Thread Tero Kristo
On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
 +Jean for functional power states
 
 Tero Kristo t-kri...@ti.com writes:
 
  This patch adds device off support to OMAP4 device type.
 
 Description is rather thin for a patch that is doing so much.
 
  OFF mode is disabled by default, 
 
 why?

Good question. For historical reference I guess. The device off works
pretty nicely with the current kernel already, so it should be possible
to enable it by default and blame the people who break it.

 
  however, there are two ways to enable OFF mode:
  a) In the board file, call omap4_pm_off_mode_enable(1)
  b) Enable OFF mode using the debugfs entry
  echo 1/sys/kernel/debug/pm_debug/enable_off_mode
  (conversely echo '0' will disable it as well).
 
 This part needs to be a separate patch.
 
 But, as stated in the core retention series, I'd like to move away from
 these global flags all together.
 
 The way we manage the disabling of certain states (like off) is already
 clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
 this feature needs to be supported by using constraints on functional
 power states.   Having checks all over the place is getting unwieldy and
 not attractive to maintain.
 
 The combination of constraints and functional power states should make
 this much more manageable.   Until we have that, I'd prefer to keep
 the debugfs enable/disable stuff as separate patches at the end of the
 series used only for testing.

Okay, this sounds like a good plan.

 
  Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
  [t-kri...@ti.com: largely re-structured the code]
 
 then the sign-off above from Santosh probably doesn't apply anymore.
 You should change that to a Cc and just mention tht this is based upon
 some original work from Santosh.

Yeah... I am not quite sure where the line goes here as I am modifying
the patches quite heavily but try to keep credits to the original
authors... will change this like so.

 
 First,  some general comments:
 
 There is a lot going on in this patch, so it is hard to follow what all
 is related, and why.  Just a quick glance suggests it needs to be broken
 up into at least a few parts:

What is the merge plan for the func power state stuff? I don't want to
create new dependencies if unnecessary. Otherwise the split should be
okay.

 
 - low-level PRM support: new APIs for various off-mode features)
   (should probably be done on top of functional power states)
 - powerdomain core support or achievable states
   (should probably be done on top of functional power states)
 - IRQ/GIC context save/restore
 - secure RAM save/restore (this has been tightly coupled to the GIC
   but it's not obvious why)

This is tightly coupled to GIC because the ROM code has following API
calls:

- save gic
- save secure RAM
- save secure all (gic + RAM + some other mysterious stuff)

It is difficult/impossible to separate these without adding redundant
code execution (e.g. doing a GIC save from the GIC code + then doing a
second GIC save with save secure all from core PM code.)

 - PM debug support to enable/disable off-mode
   (for testing only, not for merge)
 
  Signed-off-by: Tero Kristo t-kri...@ti.com
  ---
   arch/arm/mach-omap2/omap-mpuss-lowpower.c |   10 -
   arch/arm/mach-omap2/omap-wakeupgen.c  |   47 +++-
   arch/arm/mach-omap2/pm-debug.c|   17 +--
   arch/arm/mach-omap2/pm.h  |   20 +
   arch/arm/mach-omap2/pm44xx.c  |   45 +++
   arch/arm/mach-omap2/prm44xx.c |   66 
  +
   6 files changed, 197 insertions(+), 8 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c 
  b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  index e02c082..7418e7c 100644
  --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  @@ -60,6 +60,7 @@
   #include prcm44xx.h
   #include prm44xx.h
   #include prm-regbits-44xx.h
  +#include cm44xx.h
   
   #ifdef CONFIG_SMP
   
  @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned 
  int power_state)
   * In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
   */
  mpuss_clear_prev_logic_pwrst();
  -   if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) 
  +   if (omap4_device_next_state_off())
  +   save_state = 3;
 
 Why?
 
 I don't see why this check is needed in addition to the mpuss_pd check
 added just below?

mpuss_pd does not go to off, thats why. It goes to OSWR state during
standard device-off. It is possible for it to go to OFF mode, but it is
not recommended.

 
  +   else if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) 
  (pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
  save_state = 2;
  +   else if (pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_OFF)
  +   save_state = 3;
  cpu_clear_prev_logic_pwrst(cpu);
   

Re: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-21 Thread Jean Pihet
Hi Tero, Kevin, Santosh,

On Mon, May 21, 2012 at 10:48 AM, Tero Kristo t-kri...@ti.com wrote:
 On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
 +Jean for functional power states

 Tero Kristo t-kri...@ti.com writes:

  This patch adds device off support to OMAP4 device type.

 Description is rather thin for a patch that is doing so much.

  OFF mode is disabled by default,

 why?

 Good question. For historical reference I guess. The device off works
 pretty nicely with the current kernel already, so it should be possible
 to enable it by default and blame the people who break it.
Agree. The next problem is 'who will fix the breakage?'.

  however, there are two ways to enable OFF mode:
  a) In the board file, call omap4_pm_off_mode_enable(1)
  b) Enable OFF mode using the debugfs entry
  echo 1/sys/kernel/debug/pm_debug/enable_off_mode
  (conversely echo '0' will disable it as well).

 This part needs to be a separate patch.

 But, as stated in the core retention series, I'd like to move away from
 these global flags all together.

 The way we manage the disabling of certain states (like off) is already
 clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
 this feature needs to be supported by using constraints on functional
 power states.   Having checks all over the place is getting unwieldy and
 not attractive to maintain.

 The combination of constraints and functional power states should make
 this much more manageable.   Until we have that, I'd prefer to keep
 the debugfs enable/disable stuff as separate patches at the end of the
 series used only for testing.

 Okay, this sounds like a good plan.


  Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
  [t-kri...@ti.com: largely re-structured the code]

 then the sign-off above from Santosh probably doesn't apply anymore.
 You should change that to a Cc and just mention tht this is based upon
 some original work from Santosh.

 Yeah... I am not quite sure where the line goes here as I am modifying
 the patches quite heavily but try to keep credits to the original
 authors... will change this like so.


 First,  some general comments:

 There is a lot going on in this patch, so it is hard to follow what all
 is related, and why.  Just a quick glance suggests it needs to be broken
 up into at least a few parts:

 What is the merge plan for the func power state stuff? I don't want to
 create new dependencies if unnecessary. Otherwise the split should be
 okay.
That is something I am interested in too ;p
I did port and test Tero's patches on top of the functional power
states, which is the logical approach to me. Now given the reviews
status on both this patch series and the func power states series I am
not sure which one needs to be ported on top of the other ;-|
[1] https://gitorious.org/jpihet/linux-omap/commits/omap4_dev_off

Using the functional power states for the OMAP4 core and device off
has some advantages in simplifying the code, especially in the
previous/current/next states checking and programming.



 - low-level PRM support: new APIs for various off-mode features)
   (should probably be done on top of functional power states)
 - powerdomain core support or achievable states
   (should probably be done on top of functional power states)
 - IRQ/GIC context save/restore
 - secure RAM save/restore (this has been tightly coupled to the GIC
   but it's not obvious why)

 This is tightly coupled to GIC because the ROM code has following API
 calls:

 - save gic
 - save secure RAM
 - save secure all (gic + RAM + some other mysterious stuff)

 It is difficult/impossible to separate these without adding redundant
 code execution (e.g. doing a GIC save from the GIC code + then doing a
 second GIC save with save secure all from core PM code.)

 - PM debug support to enable/disable off-mode
   (for testing only, not for merge)

  Signed-off-by: Tero Kristo t-kri...@ti.com
  ---
   arch/arm/mach-omap2/omap-mpuss-lowpower.c |   10 -
   arch/arm/mach-omap2/omap-wakeupgen.c      |   47 +++-
   arch/arm/mach-omap2/pm-debug.c            |   17 +--
   arch/arm/mach-omap2/pm.h                  |   20 +
   arch/arm/mach-omap2/pm44xx.c              |   45 +++
   arch/arm/mach-omap2/prm44xx.c             |   66 
  +
   6 files changed, 197 insertions(+), 8 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c 
  b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  index e02c082..7418e7c 100644
  --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
  @@ -60,6 +60,7 @@
   #include prcm44xx.h
   #include prm44xx.h
   #include prm-regbits-44xx.h
  +#include cm44xx.h
 
   #ifdef CONFIG_SMP
 
  @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned 
  int power_state)
       * In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
       */
      

Re: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-17 Thread Shilimkar, Santosh
On Thu, May 17, 2012 at 4:06 AM, Kevin Hilman khil...@ti.com wrote:
 +Jean for functional power states

 Tero Kristo t-kri...@ti.com writes:

 This patch adds device off support to OMAP4 device type.

 Description is rather thin for a patch that is doing so much.

 OFF mode is disabled by default,

 why?

 however, there are two ways to enable OFF mode:
 a) In the board file, call omap4_pm_off_mode_enable(1)
 b) Enable OFF mode using the debugfs entry
 echo 1/sys/kernel/debug/pm_debug/enable_off_mode
 (conversely echo '0' will disable it as well).

 This part needs to be a separate patch.

 But, as stated in the core retention series, I'd like to move away from
 these global flags all together.

 The way we manage the disabling of certain states (like off) is already
 clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
 this feature needs to be supported by using constraints on functional
 power states.   Having checks all over the place is getting unwieldy and
 not attractive to maintain.

 The combination of constraints and functional power states should make
 this much more manageable.   Until we have that, I'd prefer to keep
 the debugfs enable/disable stuff as separate patches at the end of the
 series used only for testing.

 Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
 [t-kri...@ti.com: largely re-structured the code]

 then the sign-off above from Santosh probably doesn't apply anymore.
 You should change that to a Cc and just mention tht this is based upon
 some original work from Santosh.

 First,  some general comments:

 There is a lot going on in this patch, so it is hard to follow what all
 is related, and why.  Just a quick glance suggests it needs to be broken
 up into at least a few parts:

 - low-level PRM support: new APIs for various off-mode features)
  (should probably be done on top of functional power states)
 - powerdomain core support or achievable states
  (should probably be done on top of functional power states)
 - IRQ/GIC context save/restore
 - secure RAM save/restore (this has been tightly coupled to the GIC
  but it's not obvious why)
 - PM debug support to enable/disable off-mode
  (for testing only, not for merge)

Further split sounds good to me.

Regards
santosh
--
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: [PATCHv2 03/19] ARM: OMAP4: PM: Add device-off support

2012-05-16 Thread Kevin Hilman
+Jean for functional power states

Tero Kristo t-kri...@ti.com writes:

 This patch adds device off support to OMAP4 device type.

Description is rather thin for a patch that is doing so much.

 OFF mode is disabled by default, 

why?

 however, there are two ways to enable OFF mode:
 a) In the board file, call omap4_pm_off_mode_enable(1)
 b) Enable OFF mode using the debugfs entry
 echo 1/sys/kernel/debug/pm_debug/enable_off_mode
 (conversely echo '0' will disable it as well).

This part needs to be a separate patch.

But, as stated in the core retention series, I'd like to move away from
these global flags all together.

The way we manage the disabling of certain states (like off) is already
clumsy for OMAP3, and it's getting worse with OMAP4.  Basically, I think
this feature needs to be supported by using constraints on functional
power states.   Having checks all over the place is getting unwieldy and
not attractive to maintain.

The combination of constraints and functional power states should make
this much more manageable.   Until we have that, I'd prefer to keep
the debugfs enable/disable stuff as separate patches at the end of the
series used only for testing.

 Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
 [t-kri...@ti.com: largely re-structured the code]

then the sign-off above from Santosh probably doesn't apply anymore.
You should change that to a Cc and just mention tht this is based upon
some original work from Santosh.

First,  some general comments:

There is a lot going on in this patch, so it is hard to follow what all
is related, and why.  Just a quick glance suggests it needs to be broken
up into at least a few parts:

- low-level PRM support: new APIs for various off-mode features)
  (should probably be done on top of functional power states)
- powerdomain core support or achievable states
  (should probably be done on top of functional power states)
- IRQ/GIC context save/restore
- secure RAM save/restore (this has been tightly coupled to the GIC
  but it's not obvious why)
- PM debug support to enable/disable off-mode
  (for testing only, not for merge)

 Signed-off-by: Tero Kristo t-kri...@ti.com
 ---
  arch/arm/mach-omap2/omap-mpuss-lowpower.c |   10 -
  arch/arm/mach-omap2/omap-wakeupgen.c  |   47 +++-
  arch/arm/mach-omap2/pm-debug.c|   17 +--
  arch/arm/mach-omap2/pm.h  |   20 +
  arch/arm/mach-omap2/pm44xx.c  |   45 +++
  arch/arm/mach-omap2/prm44xx.c |   66 
 +
  6 files changed, 197 insertions(+), 8 deletions(-)

 diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c 
 b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
 index e02c082..7418e7c 100644
 --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
 +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
 @@ -60,6 +60,7 @@
  #include prcm44xx.h
  #include prm44xx.h
  #include prm-regbits-44xx.h
 +#include cm44xx.h
  
  #ifdef CONFIG_SMP
  
 @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int 
 power_state)
* In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
*/
   mpuss_clear_prev_logic_pwrst();
 - if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) 
 + if (omap4_device_next_state_off())
 + save_state = 3;

Why?

I don't see why this check is needed in addition to the mpuss_pd check
added just below?

 + else if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) 
   (pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
   save_state = 2;
 + else if (pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_OFF)
 + save_state = 3;
   cpu_clear_prev_logic_pwrst(cpu);
   set_cpu_next_pwrst(cpu, power_state);
 @@ -288,6 +293,9 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int 
 power_state)
   wakeup_cpu = smp_processor_id();
   set_cpu_next_pwrst(wakeup_cpu, PWRDM_POWER_ON);
  
 + if (omap4_device_prev_state_off())
 + omap4_device_clear_prev_off_state();
 +

The 'clear prev off state' is subsequently removed in the last patch
ARM: OMAP4: powerdomain: update mpu / core off counters during device
off.   Why is it needed here?

   pwrdm_post_transition();
  
   return 0;
 diff --git a/arch/arm/mach-omap2/omap-wakeupgen.c 
 b/arch/arm/mach-omap2/omap-wakeupgen.c
 index 42cd7fb..805d08d 100644
 --- a/arch/arm/mach-omap2/omap-wakeupgen.c
 +++ b/arch/arm/mach-omap2/omap-wakeupgen.c
 @@ -32,6 +32,7 @@
  
  #include omap4-sar-layout.h
  #include common.h
 +#include pm.h
  
  #define NR_REG_BANKS 4
  #define MAX_IRQS 128
 @@ -46,6 +47,8 @@ static void __iomem *sar_base;
  static DEFINE_SPINLOCK(wakeupgen_lock);
  static unsigned int irq_target_cpu[NR_IRQS];
  
 +static struct powerdomain *mpuss_pd;
 +
  /*
   * Static helper functions.
   */
 @@ -259,7 +262,7 @@ static void irq_save_context(void)
  /*
   * Clear