On Tue, 2012-05-29 at 11:31 -0700, Kevin Hilman wrote:

Tero Kristo [off-list ref] writes:

quoted

On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:

quoted

+Jean for functional power states

Tero Kristo [off-list ref] writes:

quoted

This patch adds device off support to OMAP4 device type.

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

quoted

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.

quoted

quoted

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.

quoted

quoted

Signed-off-by: Santosh Shilimkar <redacted>
[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.

quoted

quoted

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.

quoted

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

quoted

quoted

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

quoted

Signed-off-by: Tero Kristo <redacted>
---
 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.

quoted

quoted

quoted

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

quoted

quoted

quoted

 	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.

quoted

quoted

+	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

quoted

quoted

quoted

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

quoted

quoted

quoted

+		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