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

[t-kristo@ti.com (Tero Kristo)]

On Tue, 2012-05-29 at 11:31 -0700, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
> >> +Jean for functional power states
> >> 
> >> Tero Kristo <t-kristo@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.shilimkar@ti.com>
> >> > [t-kristo at 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-kristo@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.
> 
> What is confusing is that the check below specifically checks for
> mpuss_pd == off.

Yes, I guess I'll just drop that check out. Or add an error if someone
actually tries to use it.

> 
> >> 
> >> > +	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?
> >
> > That patch is moving the clear part to the lost_context_rff func, it is
> > probably not that clear. Should be easier to follow the logic once I
> > re-structure the code a bit more (separate the prm support funcs out to
> > their own set etc.)
> 
> Yes, thanks.
> 
> >> 
> >> >  	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 WakeupGen SAR backup status.
> >> >   */
> >> > -void irq_sar_clear(void)
> >> > +static void irq_sar_clear(void)
> >> >  {
> >> >  	u32 val;
> >> >  	val = __raw_readl(sar_base + SAR_BACKUP_STATUS_OFFSET);
> >> > @@ -271,7 +274,7 @@ void irq_sar_clear(void)
> >> >   * Save GIC and Wakeupgen interrupt context using secure API
> >> >   * for HS/EMU devices.
> >> >   */
> >> > -static void irq_save_secure_context(void)
> >> > +static void irq_save_secure_gic(void)
> >> >  {
> >> >  	u32 ret;
> >> >  	ret = omap_secure_dispatcher(OMAP4_HAL_SAVEGIC_INDEX,
> >> > @@ -282,6 +285,40 @@ static void irq_save_secure_context(void)
> >> >  }
> >> >  #endif
> >> >  
> >> > +static void save_secure_ram(void)
> >> > +{
> >> > +	u32 ret;
> >> 
> >> CodingStyle nit: blank line needed here (and in multiple other places)
> >
> > Okay.
> >
> >> 
> >> > +	ret = omap_secure_dispatcher(OMAP4_HAL_SAVESECURERAM_INDEX,
> >> > +				FLAG_START_CRITICAL,
> >> > +				1, omap_secure_ram_mempool_base(),
> >> > +				0, 0, 0);
> >> > +	if (ret != API_HAL_RET_VALUE_OK)
> >> > +		pr_err("Secure ram context save failed\n");
> >> > +}
> >> > +
> >> > +static void save_secure_all(void)
> >> > +{
> >> > +	u32 ret;
> >> > +	ret = omap_secure_dispatcher(OMAP4_HAL_SAVEALL_INDEX,
> >> > +				FLAG_START_CRITICAL,
> >> > +				1, omap_secure_ram_mempool_base(),
> >> > +				0, 0, 0);
> >> > +	if (ret != API_HAL_RET_VALUE_OK)
> >> > +		pr_err("Secure all context save failed\n");
> >> > +}
> >> 
> >> This secure mode save/restore seems misplaced in the wakeupgen driver.
> >> Seems to me like it belongs in omap-secure.c.
> >
> > Can't really move it there, as described above. Unless we want to save
> > GIC context twice.
> 
> OK
> 
> >> 
> >> > +
> >> > +static void irq_save_secure_context(void)
> >> > +{
> >> > +	if (omap4_device_next_state_off()) {
> >> > +		save_secure_all();
> >> > +	} else if (pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_OFF) {
> >> 
> >> Why is this check needed?  This is called from the CPU_CLUSTER_PM_ENTER
> >> notifier, which AFAICT, is only called when the cluster is going off.
> >
> > This is checking against the different MPU PD states we can go into. If:
> > - MPU OSWR => GIC save needed only
> > - MPU OFF => GIC + secure RAM save needed (not recommended to be used
> > though)
> > - MPU OSWR / OFF + device-off => save all needed
> 
> OK, more detailed changelog and descriptive kerneldoc for this function
> would certainly help.

Yep will add.

> 
> >> 
> >> > +		irq_save_secure_gic();
> >> > +		save_secure_ram();
> >> > +	} else {
> >> > +		irq_save_secure_gic();
> >> > +	}
> >> > +}
> >> > +
> >> >  #ifdef CONFIG_HOTPLUG_CPU
> >> >  static int __cpuinit irq_cpu_hotplug_notify(struct notifier_block *self,
> >> >  					 unsigned long action, void *hcpu)
> >> > @@ -388,5 +425,11 @@ int __init omap_wakeupgen_init(void)
> >> >  	irq_hotplug_init();
> >> >  	irq_pm_init();
> >> >  
> >> > +	mpuss_pd = pwrdm_lookup("mpu_pwrdm");
> >> > +	if (!mpuss_pd) {
> >> > +		pr_err("wakeupgen: unable to get mpu_pwrdm\n");
> >> > +		return -EINVAL;
> >> > +	}
> >> > +
> >> >  	return 0;
> >> >  }
> >> > diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> >> > index d9a8e42..d8cf5e5 100644
> >> > --- a/arch/arm/mach-omap2/pm-debug.c
> >> > +++ b/arch/arm/mach-omap2/pm-debug.c
> >> > @@ -40,6 +40,7 @@
> >> >  
> >> >  u32 enable_off_mode;
> >> >  static u32 enable_oswr_mode;
> >> > +static void (*off_mode_enable_func) (int);
> >> >  
> >> >  #ifdef CONFIG_DEBUG_FS
> >> >  #include <linux/debugfs.h>
> >> > @@ -249,7 +250,8 @@ static int option_set(void *data, u64 val)
> >> >  		else
> >> >  			omap_pm_disable_off_mode();
> >> >  
> >> > -		omap3_pm_off_mode_enable(val);
> >> > +		if (off_mode_enable_func)
> >> > +			off_mode_enable_func(val);
> >> >  	}
> >> >  
> >> >  	if (option == &enable_oswr_mode)
> >> > @@ -278,16 +280,21 @@ static int __init pm_dbg_init(void)
> >> >  
> >> >  	pwrdm_for_each(pwrdms_setup, (void *)d);
> >> >  
> >> > -	if (cpu_is_omap34xx())
> >> > -		(void) debugfs_create_file("enable_off_mode",
> >> > -			S_IRUGO | S_IWUSR, d, &enable_off_mode,
> >> > -			&pm_dbg_option_fops);
> >> > +	(void) debugfs_create_file("enable_off_mode",
> >> > +		S_IRUGO | S_IWUSR, d, &enable_off_mode,
> >> > +		&pm_dbg_option_fops);
> >> >  
> >> >  	if (cpu_is_omap44xx())
> >> >  		(void) debugfs_create_file("enable_oswr_mode",
> >> >  			S_IRUGO | S_IWUSR, d, &enable_oswr_mode,
> >> >  			&pm_dbg_option_fops);
> >> >  
> >> > +	if (cpu_is_omap34xx())
> >> > +		off_mode_enable_func = omap3_pm_off_mode_enable;
> >> > +
> >> > +	if (cpu_is_omap44xx())
> >> > +		off_mode_enable_func = omap4_pm_off_mode_enable;
> >> > +
> >> >  	pm_dbg_init_done = 1;
> >> >  
> >> >  	return 0;
> >> > diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> >> > index c36ab63..d95f8c5 100644
> >> > --- a/arch/arm/mach-omap2/pm.h
> >> > +++ b/arch/arm/mach-omap2/pm.h
> >> > @@ -18,6 +18,7 @@
> >> >  extern void *omap3_secure_ram_storage;
> >> >  extern void omap3_pm_off_mode_enable(int);
> >> >  extern void omap4_pm_oswr_mode_enable(int);
> >> > +extern void omap4_pm_off_mode_enable(int);
> >> >  extern void omap_sram_idle(void);
> >> >  extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
> >> >  extern int omap3_idle_init(void);
> >> > @@ -25,6 +26,25 @@ extern int omap4_idle_init(void);
> >> >  extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
> >> >  extern int (*omap_pm_suspend)(void);
> >> >  
> >> > +#ifdef CONFIG_PM
> >> > +extern void omap4_device_set_state_off(u8 enable);
> >> > +extern bool omap4_device_prev_state_off(void);
> >> > +extern bool omap4_device_next_state_off(void);
> >> > +extern void omap4_device_clear_prev_off_state(void);
> >> > +#else
> >> > +static inline void omap4_device_set_state_off(u8 enable)
> >> > +{
> >> > +}
> >> > +static inline bool omap4_device_prev_state_off(void)
> >> > +{
> >> > +	return false;
> >> > +}
> >> > +static inline bool omap4_device_next_state_off(void)
> >> > +{
> >> > +	return false;
> >> > +}
> >> > +#endif
> >> > +
> >> >  #if defined(CONFIG_PM_OPP)
> >> >  extern int omap3_opp_init(void);
> >> >  extern int omap4_opp_init(void);
> >> > diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
> >> > index 07ac0d3..8f0ec56 100644
> >> > --- a/arch/arm/mach-omap2/pm44xx.c
> >> > +++ b/arch/arm/mach-omap2/pm44xx.c
> >> > @@ -87,6 +87,27 @@ static int omap4_pm_suspend(void)
> >> >  }
> >> >  #endif /* CONFIG_SUSPEND */
> >> >  
> >> > +/**
> >> > + * get_achievable_state() - Provide achievable state
> >> > + * @available_states:	what states are available
> >> > + * @req_min_state:	what state is the minimum we'd like to hit
> >> > + *
> >> > + * Power domains have varied capabilities. When attempting a low power
> >> > + * state such as OFF/RET, a specific min requested state may not be
> >> > + * supported on the power domain, in which case, the next higher power
> >> > + * state which is supported is returned. This is because a combination
> >> > + * of system power states where the parent PD's state is not in line
> >> > + * with expectation can result in system instabilities.
> >> > + */
> >> > +static inline u8 get_achievable_state(u8 available_states, u8 req_min_state)
> >> > +{
> >> > +	u16 mask = 0xFF << req_min_state;
> >> > +
> >> > +	if (available_states & mask)
> >> > +		return __ffs(available_states & mask);
> >> > +	return PWRDM_POWER_ON;
> >> > +}
> >> 
> >> This feature needs to be generalized (preferably on top of functional
> >> power states) because we have the same need on various AM3xxx SoCs which
> >> don't support RET/OFF.  Mark Greer has started on this, but I think it
> >> needs to be part of the functional power states work.
> >> 
> >> It also needs to be part of the powerdomain layer, IMO.
> >
> > So, are you saying this whole set should be re-based on top of the
> > functional power states stuff? If yes, that can be done (Jean actually
> > already did some work on this and got it working I think.)
> 
> Yes please.

Okay, next version will be based on top of func power states.

-Tero