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

[Tero Kristo]

On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
> +Jean for functional power states
> 
> Tero Kristo <t-kristo at 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 at 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.

> 
> 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-kristo at 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);
> >  	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.)

> 
> >  	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.

> 
> > +
> > +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

> 
> > +		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.)

> 
> >  static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> >  {
> >  	struct power_state *pwrst;
> > @@ -115,6 +136,30 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> >  	return omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> >  }
> >  
> > +void omap4_pm_off_mode_enable(int enable)
> > +{
> > +	u32 next_state = PWRDM_POWER_RET;
> > +	u32 next_logic_state = PWRDM_POWER_ON;
> > +	struct power_state *pwrst;
> > +
> > +	if (enable) {
> > +		next_state = PWRDM_POWER_OFF;
> > +		next_logic_state = PWRDM_POWER_OFF;
> > +	}
> > +
> > +	omap4_device_set_state_off(enable);
> > +
> > +	list_for_each_entry(pwrst, &pwrst_list, node) {
> > +		pwrst->next_state =
> > +			get_achievable_state(pwrst->pwrdm->pwrsts, next_state);
> > +		pwrst->next_logic_state =
> > +			get_achievable_state(pwrst->pwrdm->pwrsts_logic_ret,
> > +				next_logic_state);
> > +		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> > +		pwrdm_set_logic_retst(pwrst->pwrdm, pwrst->next_logic_state);
> > +	}
> > +}
> > +
> >  void omap4_pm_oswr_mode_enable(int enable)
> >  {
> >  	u32 next_logic_state;
> > diff --git a/arch/arm/mach-omap2/prm44xx.c b/arch/arm/mach-omap2/prm44xx.c
> > index 09d84de..86c6c6b 100644
> > --- a/arch/arm/mach-omap2/prm44xx.c
> > +++ b/arch/arm/mach-omap2/prm44xx.c
> > @@ -289,6 +289,72 @@ static void __init omap44xx_prm_enable_io_wakeup(void)
> >  			OMAP4_PRM_IO_PMCTRL_OFFSET);
> >  }
> >  
> > +/**
> > + * omap4_device_set_state_off() - setup device off state
> > + * @enable:	set to off or not.
> > + *
> > + * When Device OFF is enabled, Device is allowed to perform
> > + * transition to off mode as soon as all power domains in MPU, IVA
> > + * and CORE voltage are in OFF or OSWR state (open switch retention)
> > + */
> > +void omap4_device_set_state_off(u8 enable)
> > +{
> > +	if (enable)
> > +		omap4_prminst_write_inst_reg(0x1 <<
> > +				OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> > +				OMAP4430_PRM_PARTITION,
> > +				OMAP4430_PRM_DEVICE_INST,
> > +				OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> > +	else
> > +		omap4_prminst_write_inst_reg(0x0 <<
> > +				OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> > +				OMAP4430_PRM_PARTITION,
> > +				OMAP4430_PRM_DEVICE_INST,
> > +				OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> > +}
> 
> minor: a more compact, and more readable (so it's obvious at first
> glance that the same register is being written) would be:
> 
> void omap4_device_set_state_off(u8 enable)
> {
>         u8 val = enable ? 0x1 : 0x0;
> 
> 	omap4_prminst_write_inst_reg(val <<
> 				OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> 				OMAP4430_PRM_PARTITION,
> 				OMAP4430_PRM_DEVICE_INST,
> 				OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> }

True.

> 
> > +/**
> > + * omap4_device_prev_state_off:
> > + * returns true if the device hit OFF mode
> > + * This is API to check whether OMAP is waking up from device OFF mode.
> > + * There is no other status bit available for SW to read whether last state
> > + * entered was device OFF. To work around this, CORE PD, RFF context state
> > + * is used which is lost only when we hit device OFF state
> > + */
> > +bool omap4_device_prev_state_off(void)
> > +{
> > +	u32 reg;
> 
> minor: reg makes me think the result of the read is a register address,
> not a value.  How about s/reg/val/.

Yea, good point.

> 
> > +	reg = omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
> > +				OMAP4430_PRM_CORE_INST,
> > +				OMAP4_RM_L3_1_L3_1_CONTEXT_OFFSET)
> > +		& OMAP4430_LOSTCONTEXT_RFF_MASK;
> > +
> > +	return reg ? true : false;
> > +}
> > +
> > +void omap4_device_clear_prev_off_state(void)
> > +{
> > +	omap4_prminst_write_inst_reg(OMAP4430_LOSTCONTEXT_RFF_MASK |
> > +				OMAP4430_LOSTCONTEXT_DFF_MASK,
> > +				OMAP4430_PRM_PARTITION,
> > +				OMAP4430_PRM_CORE_INST,
> > +				OMAP4_RM_L3_1_L3_1_CONTEXT_OFFSET);
> > +}
> > +
> > +/**
> > + * omap4_device_next_state_off:
> > + * returns true if the device next state is OFF
> > + * This is API to check whether OMAP is programmed for device OFF
> > + */
> > +bool omap4_device_next_state_off(void)
> > +{
> > +	return omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
> > +			OMAP4430_PRM_DEVICE_INST,
> > +			OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET)
> > +			& OMAP4430_DEVICE_OFF_ENABLE_MASK ? true : false;
> 
> minor: when using these long register read helpers, IMO readability
> improves if this is of the form:
> 
>        val = <read register>;
>        val &= <mask>;
>        return val ? true : false;

True.

> 
> > +}
> > +
> >  static int __init omap4xxx_prcm_init(void)
> >  {
> >  	if (cpu_is_omap44xx()) {
> 
> Kevin
>