[PATCHv3 0/6] Idle status patches revisited again

[PATCHv3 0/6] Idle status patches revisited again

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Improvements / fixes compared to previous version:

- Added pwrdm next_state cache initialization to patch 1
- Fixed changelog for patch 1
- Added FCLK checks to patch 5 (now checks both idlest and fclk)
- Added more info to changelog for patch 5

-Tero

[PATCHv3 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Currently only ON, RET and OFF are supported, and ON is arguably broken as it
allows the powerdomain to enter INACTIVE state unless idle is prevented.
Now, pwrdm code prevents idle if ON is selected, and also adds support for
INACTIVE. This simplifies the control needed inside sleep code.

This patch also requires caching of next power state inside powerdomain code,
as INACTIVE is not directly supported by hardware. Next powerstate is thus
now stored for each powerdomain in next_state.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/powerdomain.c             |   32 ++++++++++++++++++++----
 arch/arm/mach-omap2/powerdomains34xx.h        |   26 ++++++++++----------
 arch/arm/plat-omap/include/plat/powerdomain.h |    4 +++
 3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 26b3f3e..a08d596 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -110,8 +110,8 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
 static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 {
 
-	int prev;
-	int state;
+	u8 prev;
+	u8 state;
 
 	if (pwrdm == NULL)
 		return -EINVAL;
@@ -218,7 +218,9 @@ int pwrdm_register(struct powerdomain *pwrdm)
 
 	pr_debug("powerdomain: registered %s\n", pwrdm->name);
 	ret = 0;
-
+	pwrdm->next_state =
+		prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
+			OMAP_POWERSTATE_MASK);
 pr_unlock:
 	write_unlock_irqrestore(&pwrdm_rwlock, flags);
 
@@ -699,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm)
  */
 int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
 {
+	u8 prg_pwrst;
+
 	if (!pwrdm)
 		return -EINVAL;
 
+	if (pwrdm->next_state == pwrst)
+		return 0;
+
 	if (!(pwrdm->pwrsts & (1 << pwrst)))
 		return -EINVAL;
 
 	pr_debug("powerdomain: setting next powerstate for %s to %0x\n",
 		 pwrdm->name, pwrst);
 
+	/* INACTIVE is reserved, so we program pwrdm as ON */
+	if (pwrst == PWRDM_POWER_INACTIVE)
+		prg_pwrst = PWRDM_POWER_ON;
+	else
+		prg_pwrst = pwrst;
+
 	prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
-			     (pwrst << OMAP_POWERSTATE_SHIFT),
+			     (prg_pwrst << OMAP_POWERSTATE_SHIFT),
 			     pwrdm->prcm_offs, PM_PWSTCTRL);
 
+	/* If next state is ON, prevent idle */
+	if (pwrst == PWRDM_POWER_ON)
+		omap2_clkdm_deny_idle(pwrdm->pwrdm_clkdms[0]);
+	else
+		omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
+
+	pwrdm->next_state = pwrst;
+
 	return 0;
 }
 
@@ -728,8 +749,7 @@ int pwrdm_read_next_pwrst(struct powerdomain *pwrdm)
 	if (!pwrdm)
 		return -EINVAL;
 
-	return prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
-					OMAP_POWERSTATE_MASK);
+	return pwrdm->next_state;
 }
 
 /**
diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
index 588f7e0..f50a3f2 100644
--- a/arch/arm/mach-omap2/powerdomains34xx.h
+++ b/arch/arm/mach-omap2/powerdomains34xx.h
@@ -165,7 +165,7 @@ static struct powerdomain iva2_pwrdm = {
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
 	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_IVA2_SHIFT,
 	.wkdep_srcs	  = iva2_wkdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRSTS_OFF_RET,
 	.banks		  = 4,
 	.pwrsts_mem_ret	  = {
@@ -188,7 +188,7 @@ static struct powerdomain mpu_34xx_pwrdm = {
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
 	.dep_bit	  = OMAP3430_EN_MPU_SHIFT,
 	.wkdep_srcs	  = mpu_34xx_wkdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRSTS_OFF_RET,
 	.flags		  = PWRDM_HAS_MPU_QUIRK,
 	.banks		  = 1,
@@ -207,7 +207,7 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 |
 					   CHIP_IS_OMAP3430ES2 |
 					   CHIP_IS_OMAP3430ES3_0),
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
 	.banks		  = 2,
 	.pwrsts_mem_ret	  = {
@@ -215,8 +215,8 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
 		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
 	},
 	.pwrsts_mem_on	  = {
-		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
-		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
+		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
+		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
 	},
 };
 
@@ -225,7 +225,7 @@ static struct powerdomain core_34xx_es3_1_pwrdm = {
 	.name		  = "core_pwrdm",
 	.prcm_offs	  = CORE_MOD,
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES3_1),
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
 	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBTLL only */
 	.banks		  = 2,
@@ -234,8 +234,8 @@ static struct powerdomain core_34xx_es3_1_pwrdm = {
 		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
 	},
 	.pwrsts_mem_on	  = {
-		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
-		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
+		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
+		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
 	},
 };
 
@@ -247,7 +247,7 @@ static struct powerdomain dss_pwrdm = {
 	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_DSS_SHIFT,
 	.wkdep_srcs	  = cam_dss_wkdeps,
 	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRDM_POWER_RET,
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
@@ -287,7 +287,7 @@ static struct powerdomain cam_pwrdm = {
 	.prcm_offs	  = OMAP3430_CAM_MOD,
 	.wkdep_srcs	  = cam_dss_wkdeps,
 	.sleepdep_srcs	  = cam_gfx_sleepdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRDM_POWER_RET,
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
@@ -305,7 +305,7 @@ static struct powerdomain per_pwrdm = {
 	.dep_bit	  = OMAP3430_EN_PER_SHIFT,
 	.wkdep_srcs	  = per_usbhost_wkdeps,
 	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRSTS_OFF_RET,
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
@@ -327,7 +327,7 @@ static struct powerdomain neon_pwrdm = {
 	.prcm_offs	  = OMAP3430_NEON_MOD,
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
 	.wkdep_srcs	  = neon_wkdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRDM_POWER_RET,
 };
 
@@ -337,7 +337,7 @@ static struct powerdomain usbhost_pwrdm = {
 	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
 	.wkdep_srcs	  = per_usbhost_wkdeps,
 	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
 	.pwrsts_logic_ret = PWRDM_POWER_RET,
 	/*
 	 * REVISIT: Enabling usb host save and restore mechanism seems to
diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
index 0b96005..159e4ad 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -39,6 +39,9 @@
 
 #define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_RET_INA_ON	(PWRSTS_OFF_RET_ON | \
+				 (1 << PWRDM_POWER_INACTIVE))
+
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware save-and-restore support */
@@ -123,6 +126,7 @@ struct powerdomain {
 	struct list_head node;
 
 	int state;
+	int next_state;
 	unsigned state_counter[PWRDM_MAX_PWRSTS];
 
 #ifdef CONFIG_PM_DEBUG
-- 
1.5.4.3

[PATCHv3 2/6] OMAP3: PM: Added support for INACTIVE and ON states for powerdomains

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Previously omap_sram_idle() did not know about the difference between ON and
INACTIVE states, which complicated the state handling in these cases. Now,
the following changes are done in the idle logic:

- Check for IO-chain arming is changed to reflect desired state (RET)
- UART clocks will be disabled if we attempt to enter INACTIVE (this allows
  the state change to actually happen)

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a5335f9..7cc3293 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -386,6 +386,7 @@ void omap_sram_idle(void)
 	mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 	switch (mpu_next_state) {
 	case PWRDM_POWER_ON:
+	case PWRDM_POWER_INACTIVE:
 	case PWRDM_POWER_RET:
 		/* No need to save context */
 		save_state = 0;
@@ -452,9 +453,11 @@ void omap_sram_idle(void)
 						OMAP3430_GR_MOD,
 						OMAP3_PRM_VOLTCTRL_OFFSET);
 		}
-		/* Enable IO-PAD and IO-CHAIN wakeups */
-		prm_set_mod_reg_bits(OMAP3430_EN_IO, WKUP_MOD, PM_WKEN);
-		omap3_enable_io_chain();
+		if (core_next_state <= PWRDM_POWER_RET) {
+			/* Enable IO-PAD and IO-CHAIN wakeups */
+			prm_set_mod_reg_bits(OMAP3430_EN_IO, WKUP_MOD, PM_WKEN);
+			omap3_enable_io_chain();
+		}
 	}
 	omap3_intc_prepare_idle();
 
@@ -555,15 +558,13 @@ void omap_sram_idle(void)
 	}
 
 	/* Disable IO-PAD and IO-CHAIN wakeup */
-	if (core_next_state < PWRDM_POWER_ON) {
+	if (core_next_state <= PWRDM_POWER_RET) {
 		prm_clear_mod_reg_bits(OMAP3430_EN_IO, WKUP_MOD, PM_WKEN);
 		omap3_disable_io_chain();
 	}
 
 
 	pwrdm_post_transition();
-
-	omap2_clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
 }
 
 int omap3_can_sleep(void)
@@ -611,7 +612,6 @@ int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 	}
 
 	if (sleep_switch) {
-		omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
 		pwrdm_wait_transition(pwrdm);
 		pwrdm_state_switch(pwrdm);
 	}
-- 
1.5.4.3

[PATCHv3 3/6] OMAP3: CPUidle: Fixed support for ON / INACTIVE states

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

New powerdomain code support for INACTIVE state removes the need to control
clockdomains directly from cpuidle. Also, cpuidle state definitions can now
directly support ON / INACTIVE simplifying the implementation.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   32 ++++----------------------------
 1 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 1cfa5a6..4a81ef1 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -91,20 +91,6 @@ static int omap3_idle_bm_check(void)
 	return 0;
 }
 
-static int _cpuidle_allow_idle(struct powerdomain *pwrdm,
-				struct clockdomain *clkdm)
-{
-	omap2_clkdm_allow_idle(clkdm);
-	return 0;
-}
-
-static int _cpuidle_deny_idle(struct powerdomain *pwrdm,
-				struct clockdomain *clkdm)
-{
-	omap2_clkdm_deny_idle(clkdm);
-	return 0;
-}
-
 /**
  * omap3_enter_idle - Programs OMAP3 to enter the specified state
  * @dev: cpuidle device
@@ -141,19 +127,9 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 	if (omap_irq_pending() || need_resched())
 		goto return_sleep_time;
 
-	if (cx->type == OMAP3_STATE_C1) {
-		pwrdm_for_each_clkdm(mpu_pd, _cpuidle_deny_idle);
-		pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle);
-	}
-
 	/* Execute ARM wfi */
 	omap_sram_idle();
 
-	if (cx->type == OMAP3_STATE_C1) {
-		pwrdm_for_each_clkdm(mpu_pd, _cpuidle_allow_idle);
-		pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
-	}
-
 return_sleep_time:
 	getnstimeofday(&ts_postidle);
 	ts_idle = timespec_sub(ts_postidle, ts_preidle);
@@ -246,8 +222,8 @@ void omap_init_power_states(void)
 			cpuidle_params_table[OMAP3_STATE_C2].wake_latency;
 	omap3_power_states[OMAP3_STATE_C2].threshold =
 			cpuidle_params_table[OMAP3_STATE_C2].threshold;
-	omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_ON;
-	omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_ON;
+	omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_INACTIVE;
+	omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_INACTIVE;
 	omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID;
 
 	/* C3 . MPU CSWR + Core inactive */
@@ -261,7 +237,7 @@ void omap_init_power_states(void)
 	omap3_power_states[OMAP3_STATE_C3].threshold =
 			cpuidle_params_table[OMAP3_STATE_C3].threshold;
 	omap3_power_states[OMAP3_STATE_C3].mpu_state = PWRDM_POWER_RET;
-	omap3_power_states[OMAP3_STATE_C3].core_state = PWRDM_POWER_ON;
+	omap3_power_states[OMAP3_STATE_C3].core_state = PWRDM_POWER_INACTIVE;
 	omap3_power_states[OMAP3_STATE_C3].flags = CPUIDLE_FLAG_TIME_VALID |
 				CPUIDLE_FLAG_CHECK_BM;
 
@@ -276,7 +252,7 @@ void omap_init_power_states(void)
 	omap3_power_states[OMAP3_STATE_C4].threshold =
 			cpuidle_params_table[OMAP3_STATE_C4].threshold;
 	omap3_power_states[OMAP3_STATE_C4].mpu_state = PWRDM_POWER_OFF;
-	omap3_power_states[OMAP3_STATE_C4].core_state = PWRDM_POWER_ON;
+	omap3_power_states[OMAP3_STATE_C4].core_state = PWRDM_POWER_INACTIVE;
 	omap3_power_states[OMAP3_STATE_C4].flags = CPUIDLE_FLAG_TIME_VALID |
 				CPUIDLE_FLAG_CHECK_BM;
 
-- 
1.5.4.3

[PATCHv3 4/6] OMAP3: PM: Removed pwrdm state hacking from omap_sram_idle

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Following hacks will be moved inside cpuidle in subsequent patch:

- CAM domain prevents idle completely
- PER should not go OFF if core remains active

This simplifies the design and allows cpuidle to keep better track of which
power states system will actually enter.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   18 ++----------------
 1 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 7cc3293..549be95 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -93,7 +93,6 @@ static int (*_omap_save_secure_sram)(u32 *addr);
 
 static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
 static struct powerdomain *core_pwrdm, *per_pwrdm;
-static struct powerdomain *cam_pwrdm;
 
 static struct prm_setup_vc prm_setup = {
 	.clksetup = 0xff,
@@ -373,7 +372,6 @@ void omap_sram_idle(void)
 	int core_next_state = PWRDM_POWER_ON;
 	int core_prev_state, per_prev_state;
 	u32 sdrc_pwr = 0;
-	int per_state_modified = 0;
 
 	if (!_omap_sram_idle)
 		return;
@@ -411,20 +409,11 @@ void omap_sram_idle(void)
 	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
 	if (per_next_state < PWRDM_POWER_ON) {
 		omap2_gpio_prepare_for_idle(per_next_state);
-		if (per_next_state == PWRDM_POWER_OFF) {
-			if (core_next_state == PWRDM_POWER_ON) {
-				per_next_state = PWRDM_POWER_RET;
-				pwrdm_set_next_pwrst(per_pwrdm, per_next_state);
-				per_state_modified = 1;
-			} else
-				omap3_per_save_context();
-		}
+		if (per_next_state == PWRDM_POWER_OFF)
+			omap3_per_save_context();
 		omap_uart_prepare_idle(2);
 	}
 
-	if (pwrdm_read_pwrst(cam_pwrdm) == PWRDM_POWER_ON)
-		omap2_clkdm_deny_idle(mpu_pwrdm->pwrdm_clkdms[0]);
-
 	/*
 	 * Disable smartreflex before entering WFI.
 	 * Only needed if we are going to enter retention or off.
@@ -553,8 +542,6 @@ void omap_sram_idle(void)
 		}
 		omap2_gpio_resume_after_idle();
 		omap_uart_resume_idle(2);
-		if (per_state_modified)
-			pwrdm_set_next_pwrst(per_pwrdm, PWRDM_POWER_OFF);
 	}
 
 	/* Disable IO-PAD and IO-CHAIN wakeup */
@@ -1171,7 +1158,6 @@ static int __init omap3_pm_init(void)
 	neon_pwrdm = pwrdm_lookup("neon_pwrdm");
 	per_pwrdm = pwrdm_lookup("per_pwrdm");
 	core_pwrdm = pwrdm_lookup("core_pwrdm");
-	cam_pwrdm = pwrdm_lookup("cam_pwrdm");
 
 	omap_push_sram_idle();
 #ifdef CONFIG_SUSPEND
-- 
1.5.4.3

[PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

pwrdm_can_idle(pwrdm) will check if the specified powerdomain can enter
idle. This is done by checking all clockdomains under the powerdomain
if they can idle also.

omap2_clkdm_can_idle(clkdm) will check if the specified clockdomain can
enter idle. This checks the functional clocks and idle status bits of the
domain according to following rules:
1) get inverse of idlest and mask against idle_def.mask
  * this gives a bitmask with non-idle bits high
2) bitwise OR active functional clocks from the domain to the result
  * in some cases FCLK can be active but idlest still shows module in idle
3) disable bits defined in idle_def.mask
  * some bits should be ignored, like UART clocks which are dynamically
    switched inside sleep loop

These calls can be used e.g. inside cpuidle to decide which power states
core and mpu should enter during idle, as there are certain dependencies
between wakeup capabilities and reset logic.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/clockdomain.c             |   27 ++++++++++++
 arch/arm/mach-omap2/clockdomains.h            |   54 +++++++++++++++++++++++++
 arch/arm/mach-omap2/powerdomain.c             |   25 +++++++++++
 arch/arm/plat-omap/include/plat/clockdomain.h |   17 ++++++++
 arch/arm/plat-omap/include/plat/powerdomain.h |    1 +
 5 files changed, 124 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
index dd285f0..f42111a 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct clockdomain *clkdm)
 	return 0;
 }
 
+
+/**
+ * omap2_clkdm_can_idle - check if clockdomain has any active clocks or not
+ * @clkdm: struct clockdomain *
+ *
+ * Checks if the clockdomain has any active clock or not, i.e. whether it
+ * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if unable to idle;
+ * 1 if can idle.
+ */
+int omap2_clkdm_can_idle(struct clockdomain *clkdm)
+{
+	int i;
+
+	if (!clkdm)
+		return -EINVAL;
+
+	for (i = 0; i < clkdm->clk_reg_amt; i++)
+		if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
+				CM_IDLEST + 4 * i) &
+				clkdm->idle_def[i].mask) |
+				cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
+				CM_FCLKEN + 4 * i)) &
+				~clkdm->idle_def[i].ignore)
+			return 0;
+	return 1;
+}
+
 /**
  * omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
  * @clkdm: struct clockdomain *
diff --git a/arch/arm/mach-omap2/clockdomains.h b/arch/arm/mach-omap2/clockdomains.h
index c4ee076..2c1f2eb 100644
--- a/arch/arm/mach-omap2/clockdomains.h
+++ b/arch/arm/mach-omap2/clockdomains.h
@@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.mask = 0x1,
+		},
+	},
 };
 
 static struct clockdomain gfx_3430es1_clkdm = {
@@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.mask = 0x1,
+		},
+	},
 };
 
 /*
@@ -206,6 +218,23 @@ static struct clockdomain core_l3_34xx_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP,
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.clk_reg_amt	= 3,
+	.idle_def	= {
+		[0] = {
+			.ignore = OMAP3430_ST_UART2_MASK |
+				OMAP3430_ST_UART1_MASK |
+				OMAP3430_ST_SDRC_MASK |
+				OMAP3430_ST_OMAPCTRL_MASK |
+				OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,
+			.mask = 0xffffffff,
+		},
+		[1] = {
+			.mask = 0x1f,
+		},
+		[2] = {
+			.mask = 0x4,
+		},
+	},
 };
 
 static struct clockdomain core_l4_34xx_clkdm = {
@@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.mask = 0x1,
+		},
+	},
 };
 
 static struct clockdomain cam_clkdm = {
@@ -230,6 +265,12 @@ static struct clockdomain cam_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_CAM_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.mask = 0x1,
+		},
+	},
 };
 
 static struct clockdomain usbhost_clkdm = {
@@ -238,6 +279,12 @@ static struct clockdomain usbhost_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.mask = 0x1,
+		},
+	},
 };
 
 static struct clockdomain per_clkdm = {
@@ -246,6 +293,13 @@ static struct clockdomain per_clkdm = {
 	.flags		= CLKDM_CAN_HWSUP_SWSUP,
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.clk_reg_amt	= 1,
+	.idle_def	= {
+		[0] = {
+			.ignore = OMAP3430_ST_UART3_MASK,
+			.mask = 0x1fff,
+		},
+	},
 };
 
 /*
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index a08d596..46090bc 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1219,6 +1219,31 @@ int pwrdm_wait_transition(struct powerdomain *pwrdm)
 	return 0;
 }
 
+/**
+ * pwrdm_can_idle - check if the powerdomain can enter idle
+ * @pwrdm: struct powerdomain * the powerdomain to check status of
+ *
+ * Checks all associated clockdomains if they can idle or not.
+ * Returns 1 if the powerdomain can idle, 0 if not.
+ */
+int pwrdm_can_idle(struct powerdomain *pwrdm)
+{
+	unsigned long flags;
+	int i;
+	int ret = 1;
+
+	read_lock_irqsave(&pwrdm_rwlock, flags);
+
+	for (i = 0; i < PWRDM_MAX_CLKDMS; i++)
+		if (pwrdm->pwrdm_clkdms[i] &&
+		    !omap2_clkdm_can_idle(pwrdm->pwrdm_clkdms[i]))
+			ret = 0;
+
+	read_unlock_irqrestore(&pwrdm_rwlock, flags);
+
+	return ret;
+}
+
 int pwrdm_state_switch(struct powerdomain *pwrdm)
 {
 	return _pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
diff --git a/arch/arm/plat-omap/include/plat/clockdomain.h b/arch/arm/plat-omap/include/plat/clockdomain.h
index eb73482..71cbc5c 100644
--- a/arch/arm/plat-omap/include/plat/clockdomain.h
+++ b/arch/arm/plat-omap/include/plat/clockdomain.h
@@ -30,6 +30,12 @@
 #define CLKDM_CAN_SWSUP		(CLKDM_CAN_FORCE_SLEEP | CLKDM_CAN_FORCE_WAKEUP)
 #define CLKDM_CAN_HWSUP_SWSUP	(CLKDM_CAN_SWSUP | CLKDM_CAN_HWSUP)
 
+/*
+ * Maximum number of FCLK register masks that can be associated with a
+ * clockdomain. CORE powerdomain on OMAP3 is the worst case
+ */
+#define CLKDM_MAX_CLK_REGS			3
+
 /* OMAP24XX CM_CLKSTCTRL_*.AUTOSTATE_* register bit values */
 #define OMAP24XX_CLKSTCTRL_DISABLE_AUTO		0x0
 #define OMAP24XX_CLKSTCTRL_ENABLE_AUTO		0x1
@@ -61,6 +67,11 @@ struct clkdm_pwrdm_autodep {
 
 };
 
+struct clkdm_idle_def {
+	u32 ignore;
+	u32 mask;
+};
+
 struct clockdomain {
 
 	/* Clockdomain name */
@@ -83,6 +94,10 @@ struct clockdomain {
 	/* OMAP chip types that this clockdomain is valid on */
 	const struct omap_chip_id omap_chip;
 
+	/* For idle checks */
+	u8 clk_reg_amt;
+	struct clkdm_idle_def idle_def[CLKDM_MAX_CLK_REGS];
+
 	/* Usecount tracking */
 	atomic_t usecount;
 
@@ -108,4 +123,6 @@ int omap2_clkdm_sleep(struct clockdomain *clkdm);
 int omap2_clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk);
 int omap2_clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk);
 
+int omap2_clkdm_can_idle(struct clockdomain *clkdm);
+
 #endif
diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
index 159e4ad..42f5f88 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -182,6 +182,7 @@ int pwrdm_disable_hdwr_sar(struct powerdomain *pwrdm);
 bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 
 int pwrdm_wait_transition(struct powerdomain *pwrdm);
+int pwrdm_can_idle(struct powerdomain *pwrdm);
 
 int pwrdm_state_switch(struct powerdomain *pwrdm);
 int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
-- 
1.5.4.3

[PATCHv3 6/6] OMAP3: CPUidle: Added peripheral pwrdm checks into bm check

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Following checks are made (and their reasoning):

- If CAM domain is active, prevent idle completely
  * CAM pwrdm does not have HW wakeup capability
- If PER is likely to remain on, prevent PER off
  * Saves on unnecessary context save/restore
- If CORE domain is active, prevent PER off-mode
  * PER off in this case would prevent wakeups from PER completely
- Only allow CORE off, if all peripheral domains are off
  * CORE off will cause a chipwide reset

Also, enabled CHECK_BM flag for C2, as this is needed for the camera case.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |  118 ++++++++++++++++++++++++++++++++++--
 1 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 4a81ef1..dad64a9 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -58,7 +58,8 @@ struct omap3_processor_cx {
 
 struct omap3_processor_cx omap3_power_states[OMAP3_MAX_STATES];
 struct omap3_processor_cx current_cx_state;
-struct powerdomain *mpu_pd, *core_pd;
+struct powerdomain *mpu_pd, *core_pd, *per_pd, *iva2_pd;
+struct powerdomain *sgx_pd, *usb_pd, *cam_pd, *dss_pd;
 
 /*
  * The latencies/thresholds for various C states have
@@ -92,6 +93,22 @@ static int omap3_idle_bm_check(void)
 }
 
 /**
+ * pwrdm_get_idle_state - Get the power state a pwrdm will enter during idle
+ * @pwrdm: powerdomain to check state for
+ *
+ * Checks if the powerdomain can enter idle or not, if yes, will return
+ * the programmed target state for the domain. Otherwise will indicate
+ * that the domain will stay on.
+ * Returns the power state the pwrdm will enter.
+ */
+static int pwrdm_get_idle_state(struct powerdomain *pwrdm)
+{
+	if (pwrdm_can_idle(pwrdm))
+		return pwrdm_read_next_pwrst(pwrdm);
+	return PWRDM_POWER_ON;
+}
+
+/**
  * omap3_enter_idle - Programs OMAP3 to enter the specified state
  * @dev: cpuidle device
  * @state: The target state to be programmed
@@ -153,14 +170,94 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 			       struct cpuidle_state *state)
 {
 	struct cpuidle_state *new_state = state;
-
-	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
-		BUG_ON(!dev->safe_state);
-		new_state = dev->safe_state;
+	u32 per_state = 0, saved_per_state = 0, cam_state, usb_state;
+	u32 iva2_state, sgx_state, dss_state, new_core_state;
+	struct omap3_processor_cx *cx;
+	int ret;
+
+	if (state->flags & CPUIDLE_FLAG_CHECK_BM) {
+		if (omap3_idle_bm_check()) {
+			BUG_ON(!dev->safe_state);
+			new_state = dev->safe_state;
+			goto select_state;
+		}
+		cx = cpuidle_get_statedata(state);
+		new_core_state = cx->core_state;
+
+		/* Check if CORE is active, if yes, fallback to inactive */
+		if (!pwrdm_can_idle(core_pd))
+			new_core_state = PWRDM_POWER_INACTIVE;
+
+		/*
+		 * Prevent idle completely if CAM is active.
+		 * CAM does not have wakeup capability in OMAP3.
+		 */
+		cam_state = pwrdm_get_idle_state(cam_pd);
+		if (cam_state == PWRDM_POWER_ON) {
+			new_state = dev->safe_state;
+			goto select_state;
+		}
+
+		/*
+		 * Check if PER can idle or not. If we are not likely
+		 * to idle, deny PER off. This prevents unnecessary
+		 * context save/restore.
+		 */
+		saved_per_state = pwrdm_read_next_pwrst(per_pd);
+		if (pwrdm_can_idle(per_pd)) {
+			per_state = saved_per_state;
+			/*
+			 * Prevent PER off if CORE is active as this
+			 * would disable PER wakeups completely
+			 */
+			if (per_state == PWRDM_POWER_OFF &&
+			    new_core_state > PWRDM_POWER_RET)
+				per_state = PWRDM_POWER_RET;
+
+		} else if (saved_per_state == PWRDM_POWER_OFF)
+			per_state = PWRDM_POWER_RET;
+
+		/*
+		 * If we are attempting CORE off, check if any other
+		 * powerdomains are at retention or higher. CORE off causes
+		 * chipwide reset which would reset these domains also.
+		 */
+		if (new_core_state == PWRDM_POWER_OFF) {
+			dss_state = pwrdm_get_idle_state(dss_pd);
+			iva2_state = pwrdm_get_idle_state(iva2_pd);
+			sgx_state = pwrdm_get_idle_state(sgx_pd);
+			usb_state = pwrdm_get_idle_state(usb_pd);
+
+			if (cam_state > PWRDM_POWER_OFF ||
+			    dss_state > PWRDM_POWER_OFF ||
+			    iva2_state > PWRDM_POWER_OFF ||
+			    per_state > PWRDM_POWER_OFF ||
+			    sgx_state > PWRDM_POWER_OFF ||
+			    usb_state > PWRDM_POWER_OFF)
+				new_core_state = PWRDM_POWER_RET;
+		}
+
+		/* Fallback to new target core state */
+		while (cx->core_state > new_core_state) {
+			state--;
+			cx = cpuidle_get_statedata(state);
+		}
+		new_state = state;
+
+		/* Are we changing PER target state? */
+		if (per_state != saved_per_state)
+			pwrdm_set_next_pwrst(per_pd, per_state);
 	}
 
+select_state:
 	dev->last_state = new_state;
-	return omap3_enter_idle(dev, new_state);
+	ret = omap3_enter_idle(dev, new_state);
+
+	/* Restore potentially tampered PER state */
+	if (per_state != saved_per_state)
+		pwrdm_set_next_pwrst(per_pd, saved_per_state);
+
+	return ret;
 }
 
 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
@@ -224,7 +321,8 @@ void omap_init_power_states(void)
 			cpuidle_params_table[OMAP3_STATE_C2].threshold;
 	omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_INACTIVE;
 	omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_INACTIVE;
-	omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID;
+	omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
+				CPUIDLE_FLAG_CHECK_BM;
 
 	/* C3 . MPU CSWR + Core inactive */
 	omap3_power_states[OMAP3_STATE_C3].valid =
@@ -322,6 +420,12 @@ int __init omap3_idle_init(void)
 
 	mpu_pd = pwrdm_lookup("mpu_pwrdm");
 	core_pd = pwrdm_lookup("core_pwrdm");
+	per_pd = pwrdm_lookup("per_pwrdm");
+	iva2_pd = pwrdm_lookup("iva2_pwrdm");
+	sgx_pd = pwrdm_lookup("sgx_pwrdm");
+	usb_pd = pwrdm_lookup("usbhost_pwrdm");
+	cam_pd = pwrdm_lookup("cam_pwrdm");
+	dss_pd = pwrdm_lookup("dss_pwrdm");
 
 	omap_init_power_states();
 	cpuidle_register_driver(&omap3_idle_driver);
-- 
1.5.4.3

Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle

[Kevin Hilman]

Tero Kristo <tero.kristo@nokia.com> writes:

> From: Tero Kristo <tero.kristo@nokia.com>
>
> pwrdm_can_idle(pwrdm) will check if the specified powerdomain can enter
> idle. This is done by checking all clockdomains under the powerdomain
> if they can idle also.
>
> omap2_clkdm_can_idle(clkdm) will check if the specified clockdomain can
> enter idle. This checks the functional clocks and idle status bits of the
> domain according to following rules:
> 1) get inverse of idlest and mask against idle_def.mask
>   * this gives a bitmask with non-idle bits high
> 2) bitwise OR active functional clocks from the domain to the result
>   * in some cases FCLK can be active but idlest still shows module in idle
> 3) disable bits defined in idle_def.mask

disable?  it looks like they're just being ignored.

>   * some bits should be ignored, like UART clocks which are dynamically
>     switched inside sleep loop
>
> These calls can be used e.g. inside cpuidle to decide which power states
> core and mpu should enter during idle, as there are certain dependencies
> between wakeup capabilities and reset logic.
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>

Thanks for adding the IDLEST checks, I think that will help.

In the .mask field, instead of using a hard-coded mask, probably using
the existing bitfields headers would be a bit more readable, and help
ensure correctness.

Hopefully there's a way to auto-generate these for OMAP4.

> ---
>  arch/arm/mach-omap2/clockdomain.c             |   27 ++++++++++++
>  arch/arm/mach-omap2/clockdomains.h            |   54 +++++++++++++++++++++++++
>  arch/arm/mach-omap2/powerdomain.c             |   25 +++++++++++
>  arch/arm/plat-omap/include/plat/clockdomain.h |   17 ++++++++
>  arch/arm/plat-omap/include/plat/powerdomain.h |    1 +
>  5 files changed, 124 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
> index dd285f0..f42111a 100644
> --- a/arch/arm/mach-omap2/clockdomain.c
> +++ b/arch/arm/mach-omap2/clockdomain.c
> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct clockdomain *clkdm)
>  	return 0;
>  }
>  
> +
> +/**
> + * omap2_clkdm_can_idle - check if clockdomain has any active clocks or not
> + * @clkdm: struct clockdomain *
> + *
> + * Checks if the clockdomain has any active clock or not, i.e. whether it
> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if unable to idle;
> + * 1 if can idle.
> + */
> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
> +{
> +	int i;
> +
> +	if (!clkdm)
> +		return -EINVAL;
> +
> +	for (i = 0; i < clkdm->clk_reg_amt; i++)
> +		if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
> +				CM_IDLEST + 4 * i) &
> +				clkdm->idle_def[i].mask) |
> +				cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
> +				CM_FCLKEN + 4 * i)) &
> +				~clkdm->idle_def[i].ignore)

This has some readability/indent issues.  

Also, could check fclk first, if active fclks, no reason to check
idlest.  How about something like this (untested, but I *think* I kept
the same logic):

	for (i = 0; i < clkdm->clk_reg_amt; i++) {
		u32 idlest, fclk;

		fclk = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
				       CM_FCLKEN + 4 * i);
		if (fclk & ~clkdm->idle_def[i].ignore)
			return 0;

		idlest = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
					 CM_IDLEST + 4 * i);
		if (~idlest & clkdm->idle_def[i].mask)
			return 0;
	}

> +			return 0;
> +	return 1;
> +}
> +
>  /**
>   * omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
>   * @clkdm: struct clockdomain *
> diff --git a/arch/arm/mach-omap2/clockdomains.h b/arch/arm/mach-omap2/clockdomains.h
> index c4ee076..2c1f2eb 100644
> --- a/arch/arm/mach-omap2/clockdomains.h
> +++ b/arch/arm/mach-omap2/clockdomains.h
> @@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.mask = 0x1,

Use OMAP3430_CM_FCLKEN_IVA2_EN_IVA2_MASK

> +		},
> +	},
>  };
>  
>  static struct clockdomain gfx_3430es1_clkdm = {
> @@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.mask = 0x1,

Use OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK

And here's a a good reason for using the pre-defined bitmasks.  According to
the #defines (and the TRM) The single EN bit in SGX is actually in bit 1, not bit 0)

> +		},
> +	},
>  };
>  
>  /*
> @@ -206,6 +218,23 @@ static struct clockdomain core_l3_34xx_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP,
>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.clk_reg_amt	= 3,
> +	.idle_def	= {
> +		[0] = {
> +			.ignore = OMAP3430_ST_UART2_MASK |
> +				OMAP3430_ST_UART1_MASK |
> +				OMAP3430_ST_SDRC_MASK |
> +				OMAP3430_ST_OMAPCTRL_MASK |
> +				OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,

Hmm, why ignore OTG mask?  I think we need some comments as to why all
of these are ignored.  You mention the UARTs in the changelog, but not
the others.

> +			.mask = 0xffffffff,

There are a lot of reserved bits here that I'm not sure we should be
checking.

> +		},
> +		[1] = {
> +			.mask = 0x1f,
> +		},
> +		[2] = {
> +			.mask = 0x4,
> +		},
> +	},
>  };
>  
>  static struct clockdomain core_l4_34xx_clkdm = {
> @@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.mask = 0x1,
> +		},
> +	},
>  };
>  
>  static struct clockdomain cam_clkdm = {
> @@ -230,6 +265,12 @@ static struct clockdomain cam_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_CAM_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.mask = 0x1,
> +		},
> +	},
>  };
>  
>  static struct clockdomain usbhost_clkdm = {
> @@ -238,6 +279,12 @@ static struct clockdomain usbhost_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.mask = 0x1,
> +		},
> +	},
>  };
>  
>  static struct clockdomain per_clkdm = {
> @@ -246,6 +293,13 @@ static struct clockdomain per_clkdm = {
>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.clk_reg_amt	= 1,
> +	.idle_def	= {
> +		[0] = {
> +			.ignore = OMAP3430_ST_UART3_MASK,
> +			.mask = 0x1fff,
> +		},
> +	},
>  };
>  
>  /*
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index a08d596..46090bc 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -1219,6 +1219,31 @@ int pwrdm_wait_transition(struct powerdomain *pwrdm)
>  	return 0;
>  }
>  
> +/**
> + * pwrdm_can_idle - check if the powerdomain can enter idle
> + * @pwrdm: struct powerdomain * the powerdomain to check status of
> + *
> + * Checks all associated clockdomains if they can idle or not.
> + * Returns 1 if the powerdomain can idle, 0 if not.
> + */
> +int pwrdm_can_idle(struct powerdomain *pwrdm)
> +{
> +	unsigned long flags;
> +	int i;
> +	int ret = 1;
> +
> +	read_lock_irqsave(&pwrdm_rwlock, flags);
> +
> +	for (i = 0; i < PWRDM_MAX_CLKDMS; i++)
> +		if (pwrdm->pwrdm_clkdms[i] &&
> +		    !omap2_clkdm_can_idle(pwrdm->pwrdm_clkdms[i]))
> +			ret = 0;
> +
> +	read_unlock_irqrestore(&pwrdm_rwlock, flags);
> +
> +	return ret;
> +}
> +
>  int pwrdm_state_switch(struct powerdomain *pwrdm)
>  {
>  	return _pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
> diff --git a/arch/arm/plat-omap/include/plat/clockdomain.h b/arch/arm/plat-omap/include/plat/clockdomain.h
> index eb73482..71cbc5c 100644
> --- a/arch/arm/plat-omap/include/plat/clockdomain.h
> +++ b/arch/arm/plat-omap/include/plat/clockdomain.h
> @@ -30,6 +30,12 @@
>  #define CLKDM_CAN_SWSUP		(CLKDM_CAN_FORCE_SLEEP | CLKDM_CAN_FORCE_WAKEUP)
>  #define CLKDM_CAN_HWSUP_SWSUP	(CLKDM_CAN_SWSUP | CLKDM_CAN_HWSUP)
>  
> +/*
> + * Maximum number of FCLK register masks that can be associated with a
> + * clockdomain. CORE powerdomain on OMAP3 is the worst case
> + */
> +#define CLKDM_MAX_CLK_REGS			3
> +
>  /* OMAP24XX CM_CLKSTCTRL_*.AUTOSTATE_* register bit values */
v>  #define OMAP24XX_CLKSTCTRL_DISABLE_AUTO		0x0
>  #define OMAP24XX_CLKSTCTRL_ENABLE_AUTO		0x1
> @@ -61,6 +67,11 @@ struct clkdm_pwrdm_autodep {
>  
>  };
>  
> +struct clkdm_idle_def {
> +	u32 ignore;

I think fclk_ignore is a better name

> +	u32 mask;

and idlest_mask here.

> +};
> +
>  struct clockdomain {
>  
>  	/* Clockdomain name */
> @@ -83,6 +94,10 @@ struct clockdomain {
>  	/* OMAP chip types that this clockdomain is valid on */
>  	const struct omap_chip_id omap_chip;
>  
> +	/* For idle checks */
> +	u8 clk_reg_amt;

I think clk_reg_num is a better name here.

> +	struct clkdm_idle_def idle_def[CLKDM_MAX_CLK_REGS];
> +
>  	/* Usecount tracking */
>  	atomic_t usecount;
>  
> @@ -108,4 +123,6 @@ int omap2_clkdm_sleep(struct clockdomain *clkdm);
>  int omap2_clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk);
>  int omap2_clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk);
>  
> +int omap2_clkdm_can_idle(struct clockdomain *clkdm);
> +
>  #endif
> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
> index 159e4ad..42f5f88 100644
> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
> @@ -182,6 +182,7 @@ int pwrdm_disable_hdwr_sar(struct powerdomain *pwrdm);
>  bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
>  
>  int pwrdm_wait_transition(struct powerdomain *pwrdm);
> +int pwrdm_can_idle(struct powerdomain *pwrdm);
>  
>  int pwrdm_state_switch(struct powerdomain *pwrdm);
>  int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
> -- 
> 1.5.4.3

RE: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle

[Tero.Kristo]

 

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>Sent: 18 January, 2010 20:41
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for 
>checking if pwrdm/clkdm can idle
>
>Tero Kristo <tero.kristo@nokia.com> writes:
>
>> From: Tero Kristo <tero.kristo@nokia.com>
>>
>> pwrdm_can_idle(pwrdm) will check if the specified 
>powerdomain can enter
>> idle. This is done by checking all clockdomains under the powerdomain
>> if they can idle also.
>>
>> omap2_clkdm_can_idle(clkdm) will check if the specified 
>clockdomain can
>> enter idle. This checks the functional clocks and idle 
>status bits of the
>> domain according to following rules:
>> 1) get inverse of idlest and mask against idle_def.mask
>>   * this gives a bitmask with non-idle bits high
>> 2) bitwise OR active functional clocks from the domain to the result
>>   * in some cases FCLK can be active but idlest still shows 
>module in idle
>> 3) disable bits defined in idle_def.mask
>
>disable?  it looks like they're just being ignored.

Oh yes, just a small typo here in patch description. This should indeed say that they are ignored, as the bit-field also is named as ignore.

>
>>   * some bits should be ignored, like UART clocks which are 
>dynamically
>>     switched inside sleep loop
>>
>> These calls can be used e.g. inside cpuidle to decide which 
>power states
>> core and mpu should enter during idle, as there are certain 
>dependencies
>> between wakeup capabilities and reset logic.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>
>Thanks for adding the IDLEST checks, I think that will help.

Yes, and this is actually required for the secure side clock status handling, this information is not available anywhere else, thus it is impossible to get the idle status from the clock framework usecounts.

>
>In the .mask field, instead of using a hard-coded mask, probably using
>the existing bitfields headers would be a bit more readable, and help
>ensure correctness.

Well, the bitfield definitions for those do not exist at the moment. The idea of the mask is just to get all bits in that are needed, e.g. PER domain does not have GPIO bits in the mask, as these appear to always be in a state that they are accessible (i.e. IDLEST shows accessible for them.)

I can write all IDLEST bits into the masks (as was done before) if you like this approach better. This will generate rather long definitions though, and the probability of errors in those is actually higher than in this version (I just write a number of low bits to one.)

>
>Hopefully there's a way to auto-generate these for OMAP4.
>
>> ---
>>  arch/arm/mach-omap2/clockdomain.c             |   27 ++++++++++++
>>  arch/arm/mach-omap2/clockdomains.h            |   54 
>+++++++++++++++++++++++++
>>  arch/arm/mach-omap2/powerdomain.c             |   25 +++++++++++
>>  arch/arm/plat-omap/include/plat/clockdomain.h |   17 ++++++++
>>  arch/arm/plat-omap/include/plat/powerdomain.h |    1 +
>>  5 files changed, 124 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clockdomain.c 
>b/arch/arm/mach-omap2/clockdomain.c
>> index dd285f0..f42111a 100644
>> --- a/arch/arm/mach-omap2/clockdomain.c
>> +++ b/arch/arm/mach-omap2/clockdomain.c
>> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct 
>clockdomain *clkdm)
>>  	return 0;
>>  }
>>  
>> +
>> +/**
>> + * omap2_clkdm_can_idle - check if clockdomain has any 
>active clocks or not
>> + * @clkdm: struct clockdomain *
>> + *
>> + * Checks if the clockdomain has any active clock or not, 
>i.e. whether it
>> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if 
>unable to idle;
>> + * 1 if can idle.
>> + */
>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
>> +{
>> +	int i;
>> +
>> +	if (!clkdm)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < clkdm->clk_reg_amt; i++)
>> +		if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>> +				CM_IDLEST + 4 * i) &
>> +				clkdm->idle_def[i].mask) |
>> +				
>cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>> +				CM_FCLKEN + 4 * i)) &
>> +				~clkdm->idle_def[i].ignore)
>
>This has some readability/indent issues.  
>
>Also, could check fclk first, if active fclks, no reason to check
>idlest.  How about something like this (untested, but I *think* I kept
>the same logic):
>
>	for (i = 0; i < clkdm->clk_reg_amt; i++) {
>		u32 idlest, fclk;
>
>		fclk = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>				       CM_FCLKEN + 4 * i);
>		if (fclk & ~clkdm->idle_def[i].ignore)
>			return 0;
>
>		idlest = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>					 CM_IDLEST + 4 * i);
>		if (~idlest & clkdm->idle_def[i].mask)
>			return 0;
>	}
>
>> +			return 0;
>> +	return 1;
>> +}
>> +
>>  /**
>>   * omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
>>   * @clkdm: struct clockdomain *
>> diff --git a/arch/arm/mach-omap2/clockdomains.h 
>b/arch/arm/mach-omap2/clockdomains.h
>> index c4ee076..2c1f2eb 100644
>> --- a/arch/arm/mach-omap2/clockdomains.h
>> +++ b/arch/arm/mach-omap2/clockdomains.h
>> @@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.mask = 0x1,
>
>Use OMAP3430_CM_FCLKEN_IVA2_EN_IVA2_MASK
>
>> +		},
>> +	},
>>  };
>>  
>>  static struct clockdomain gfx_3430es1_clkdm = {
>> @@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.mask = 0x1,
>
>Use OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK
>
>And here's a a good reason for using the pre-defined bitmasks. 
> According to
>the #defines (and the TRM) The single EN bit in SGX is 
>actually in bit 1, not bit 0)
>
>> +		},
>> +	},
>>  };
>>  
>>  /*
>> @@ -206,6 +218,23 @@ static struct clockdomain core_l3_34xx_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP,
>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.clk_reg_amt	= 3,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.ignore = OMAP3430_ST_UART2_MASK |
>> +				OMAP3430_ST_UART1_MASK |
>> +				OMAP3430_ST_SDRC_MASK |
>> +				OMAP3430_ST_OMAPCTRL_MASK |
>> +				OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,
>
>Hmm, why ignore OTG mask?  I think we need some comments as to why all
>of these are ignored.  You mention the UARTs in the changelog, but not
>the others.
>
>> +			.mask = 0xffffffff,
>
>There are a lot of reserved bits here that I'm not sure we should be
>checking.
>
>> +		},
>> +		[1] = {
>> +			.mask = 0x1f,
>> +		},
>> +		[2] = {
>> +			.mask = 0x4,
>> +		},
>> +	},
>>  };
>>  
>>  static struct clockdomain core_l4_34xx_clkdm = {
>> @@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.mask = 0x1,
>> +		},
>> +	},
>>  };
>>  
>>  static struct clockdomain cam_clkdm = {
>> @@ -230,6 +265,12 @@ static struct clockdomain cam_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_CAM_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.mask = 0x1,
>> +		},
>> +	},
>>  };
>>  
>>  static struct clockdomain usbhost_clkdm = {
>> @@ -238,6 +279,12 @@ static struct clockdomain usbhost_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.mask = 0x1,
>> +		},
>> +	},
>>  };
>>  
>>  static struct clockdomain per_clkdm = {
>> @@ -246,6 +293,13 @@ static struct clockdomain per_clkdm = {
>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.clk_reg_amt	= 1,
>> +	.idle_def	= {
>> +		[0] = {
>> +			.ignore = OMAP3430_ST_UART3_MASK,
>> +			.mask = 0x1fff,
>> +		},
>> +	},
>>  };
>>  
>>  /*
>> diff --git a/arch/arm/mach-omap2/powerdomain.c 
>b/arch/arm/mach-omap2/powerdomain.c
>> index a08d596..46090bc 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -1219,6 +1219,31 @@ int pwrdm_wait_transition(struct 
>powerdomain *pwrdm)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * pwrdm_can_idle - check if the powerdomain can enter idle
>> + * @pwrdm: struct powerdomain * the powerdomain to check status of
>> + *
>> + * Checks all associated clockdomains if they can idle or not.
>> + * Returns 1 if the powerdomain can idle, 0 if not.
>> + */
>> +int pwrdm_can_idle(struct powerdomain *pwrdm)
>> +{
>> +	unsigned long flags;
>> +	int i;
>> +	int ret = 1;
>> +
>> +	read_lock_irqsave(&pwrdm_rwlock, flags);
>> +
>> +	for (i = 0; i < PWRDM_MAX_CLKDMS; i++)
>> +		if (pwrdm->pwrdm_clkdms[i] &&
>> +		    !omap2_clkdm_can_idle(pwrdm->pwrdm_clkdms[i]))
>> +			ret = 0;
>> +
>> +	read_unlock_irqrestore(&pwrdm_rwlock, flags);
>> +
>> +	return ret;
>> +}
>> +
>>  int pwrdm_state_switch(struct powerdomain *pwrdm)
>>  {
>>  	return _pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
>> diff --git a/arch/arm/plat-omap/include/plat/clockdomain.h 
>b/arch/arm/plat-omap/include/plat/clockdomain.h
>> index eb73482..71cbc5c 100644
>> --- a/arch/arm/plat-omap/include/plat/clockdomain.h
>> +++ b/arch/arm/plat-omap/include/plat/clockdomain.h
>> @@ -30,6 +30,12 @@
>>  #define CLKDM_CAN_SWSUP		(CLKDM_CAN_FORCE_SLEEP 
>| CLKDM_CAN_FORCE_WAKEUP)
>>  #define CLKDM_CAN_HWSUP_SWSUP	(CLKDM_CAN_SWSUP | 
>CLKDM_CAN_HWSUP)
>>  
>> +/*
>> + * Maximum number of FCLK register masks that can be 
>associated with a
>> + * clockdomain. CORE powerdomain on OMAP3 is the worst case
>> + */
>> +#define CLKDM_MAX_CLK_REGS			3
>> +
>>  /* OMAP24XX CM_CLKSTCTRL_*.AUTOSTATE_* register bit values */
>v>  #define OMAP24XX_CLKSTCTRL_DISABLE_AUTO		0x0
>>  #define OMAP24XX_CLKSTCTRL_ENABLE_AUTO		0x1
>> @@ -61,6 +67,11 @@ struct clkdm_pwrdm_autodep {
>>  
>>  };
>>  
>> +struct clkdm_idle_def {
>> +	u32 ignore;
>
>I think fclk_ignore is a better name
>
>> +	u32 mask;
>
>and idlest_mask here.
>
>> +};
>> +
>>  struct clockdomain {
>>  
>>  	/* Clockdomain name */
>> @@ -83,6 +94,10 @@ struct clockdomain {
>>  	/* OMAP chip types that this clockdomain is valid on */
>>  	const struct omap_chip_id omap_chip;
>>  
>> +	/* For idle checks */
>> +	u8 clk_reg_amt;
>
>I think clk_reg_num is a better name here.
>
>> +	struct clkdm_idle_def idle_def[CLKDM_MAX_CLK_REGS];
>> +
>>  	/* Usecount tracking */
>>  	atomic_t usecount;
>>  
>> @@ -108,4 +123,6 @@ int omap2_clkdm_sleep(struct clockdomain *clkdm);
>>  int omap2_clkdm_clk_enable(struct clockdomain *clkdm, 
>struct clk *clk);
>>  int omap2_clkdm_clk_disable(struct clockdomain *clkdm, 
>struct clk *clk);
>>  
>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm);
>> +
>>  #endif
>> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
>b/arch/arm/plat-omap/include/plat/powerdomain.h
>> index 159e4ad..42f5f88 100644
>> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
>> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>> @@ -182,6 +182,7 @@ int pwrdm_disable_hdwr_sar(struct 
>powerdomain *pwrdm);
>>  bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
>>  
>>  int pwrdm_wait_transition(struct powerdomain *pwrdm);
>> +int pwrdm_can_idle(struct powerdomain *pwrdm);
>>  
>>  int pwrdm_state_switch(struct powerdomain *pwrdm);
>>  int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
>> -- 
>> 1.5.4.3
>

Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle

[Kevin Hilman]

<Tero.Kristo@nokia.com> writes:

>  
>
>>-----Original Message-----
>>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>Sent: 18 January, 2010 20:41
>>To: Kristo Tero (Nokia-D/Tampere)
>>Cc: linux-omap@vger.kernel.org
>>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for 
>>checking if pwrdm/clkdm can idle
>>
>>Tero Kristo <tero.kristo@nokia.com> writes:
>>
>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>
>>> pwrdm_can_idle(pwrdm) will check if the specified 
>>powerdomain can enter
>>> idle. This is done by checking all clockdomains under the powerdomain
>>> if they can idle also.
>>>
>>> omap2_clkdm_can_idle(clkdm) will check if the specified 
>>clockdomain can
>>> enter idle. This checks the functional clocks and idle 
>>status bits of the
>>> domain according to following rules:
>>> 1) get inverse of idlest and mask against idle_def.mask
>>>   * this gives a bitmask with non-idle bits high
>>> 2) bitwise OR active functional clocks from the domain to the result
>>>   * in some cases FCLK can be active but idlest still shows 
>>module in idle
>>> 3) disable bits defined in idle_def.mask
>>
>>disable?  it looks like they're just being ignored.
>
> Oh yes, just a small typo here in patch description. This should indeed say that they are ignored, as the bit-field also is named as ignore.
>
>>
>>>   * some bits should be ignored, like UART clocks which are 
>>dynamically
>>>     switched inside sleep loop
>>>
>>> These calls can be used e.g. inside cpuidle to decide which 
>>power states
>>> core and mpu should enter during idle, as there are certain 
>>dependencies
>>> between wakeup capabilities and reset logic.
>>>
>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>
>>Thanks for adding the IDLEST checks, I think that will help.
>
> Yes, and this is actually required for the secure side clock status
> handling, this information is not available anywhere else, thus it
> is impossible to get the idle status from the clock framework
> usecounts.
>
>>
>>In the .mask field, instead of using a hard-coded mask, probably using
>>the existing bitfields headers would be a bit more readable, and help
>>ensure correctness.
>
> Well, the bitfield definitions for those do not exist at the
> moment. 

We could use the EN bit definitions for the CM_FCLKEN* since they're
the same for IDLEST.

> The idea of the mask is just to get all bits in that are
> needed, e.g. PER domain does not have GPIO bits in the mask, as
> these appear to always be in a state that they are accessible
> (i.e. IDLEST shows accessible for them.)

And I think if we used the #defines here, it would be much clearer
what bits are included and which are left out.

Some comments as to which ones are needed or not would be helpful too.

> I can write all IDLEST bits into the masks (as was done before) if
> you like this approach better. This will generate rather long
> definitions though, and the probability of errors in those is
> actually higher than in this version (I just write a number of low
> bits to one.)

I guess you didn't see the rest of my comments further down in the
original review.  There was at least one error already in the SGX
since it's bit is not bit 0 but but 1.  Using the existing #defines
would've solved that.

On that note, please check out the other inline comments in the
original review.

I should've mentioned that there were further comments inline in the
original review.  Sorry for the confusion.

Kevin

>>
>>Hopefully there's a way to auto-generate these for OMAP4.
>>
>>> ---
>>>  arch/arm/mach-omap2/clockdomain.c             |   27 ++++++++++++
>>>  arch/arm/mach-omap2/clockdomains.h            |   54 
>>+++++++++++++++++++++++++
>>>  arch/arm/mach-omap2/powerdomain.c             |   25 +++++++++++
>>>  arch/arm/plat-omap/include/plat/clockdomain.h |   17 ++++++++
>>>  arch/arm/plat-omap/include/plat/powerdomain.h |    1 +
>>>  5 files changed, 124 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/clockdomain.c 
>>b/arch/arm/mach-omap2/clockdomain.c
>>> index dd285f0..f42111a 100644
>>> --- a/arch/arm/mach-omap2/clockdomain.c
>>> +++ b/arch/arm/mach-omap2/clockdomain.c
>>> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct 
>>clockdomain *clkdm)
>>>  	return 0;
>>>  }
>>>  
>>> +
>>> +/**
>>> + * omap2_clkdm_can_idle - check if clockdomain has any 
>>active clocks or not
>>> + * @clkdm: struct clockdomain *
>>> + *
>>> + * Checks if the clockdomain has any active clock or not, 
>>i.e. whether it
>>> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if 
>>unable to idle;
>>> + * 1 if can idle.
>>> + */
>>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
>>> +{
>>> +	int i;
>>> +
>>> +	if (!clkdm)
>>> +		return -EINVAL;
>>> +
>>> +	for (i = 0; i < clkdm->clk_reg_amt; i++)
>>> +		if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>> +				CM_IDLEST + 4 * i) &
>>> +				clkdm->idle_def[i].mask) |
>>> +				
>>cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>> +				CM_FCLKEN + 4 * i)) &
>>> +				~clkdm->idle_def[i].ignore)
>>
>>This has some readability/indent issues.  
>>
>>Also, could check fclk first, if active fclks, no reason to check
>>idlest.  How about something like this (untested, but I *think* I kept
>>the same logic):
>>
>>	for (i = 0; i < clkdm->clk_reg_amt; i++) {
>>		u32 idlest, fclk;
>>
>>		fclk = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>				       CM_FCLKEN + 4 * i);
>>		if (fclk & ~clkdm->idle_def[i].ignore)
>>			return 0;
>>
>>		idlest = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>					 CM_IDLEST + 4 * i);
>>		if (~idlest & clkdm->idle_def[i].mask)
>>			return 0;
>>	}
>>
>>> +			return 0;
>>> +	return 1;
>>> +}
>>> +
>>>  /**
>>>   * omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
>>>   * @clkdm: struct clockdomain *
>>> diff --git a/arch/arm/mach-omap2/clockdomains.h 
>>b/arch/arm/mach-omap2/clockdomains.h
>>> index c4ee076..2c1f2eb 100644
>>> --- a/arch/arm/mach-omap2/clockdomains.h
>>> +++ b/arch/arm/mach-omap2/clockdomains.h
>>> @@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.mask = 0x1,
>>
>>Use OMAP3430_CM_FCLKEN_IVA2_EN_IVA2_MASK
>>
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  static struct clockdomain gfx_3430es1_clkdm = {
>>> @@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.mask = 0x1,
>>
>>Use OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK
>>
>>And here's a a good reason for using the pre-defined bitmasks. 
>> According to
>>the #defines (and the TRM) The single EN bit in SGX is 
>>actually in bit 1, not bit 0)
>>
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  /*
>>> @@ -206,6 +218,23 @@ static struct clockdomain core_l3_34xx_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP,
>>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>> +	.clk_reg_amt	= 3,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.ignore = OMAP3430_ST_UART2_MASK |
>>> +				OMAP3430_ST_UART1_MASK |
>>> +				OMAP3430_ST_SDRC_MASK |
>>> +				OMAP3430_ST_OMAPCTRL_MASK |
>>> +				OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,
>>
>>Hmm, why ignore OTG mask?  I think we need some comments as to why all
>>of these are ignored.  You mention the UARTs in the changelog, but not
>>the others.
>>
>>> +			.mask = 0xffffffff,
>>
>>There are a lot of reserved bits here that I'm not sure we should be
>>checking.
>>
>>> +		},
>>> +		[1] = {
>>> +			.mask = 0x1f,
>>> +		},
>>> +		[2] = {
>>> +			.mask = 0x4,
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  static struct clockdomain core_l4_34xx_clkdm = {
>>> @@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.mask = 0x1,
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  static struct clockdomain cam_clkdm = {
>>> @@ -230,6 +265,12 @@ static struct clockdomain cam_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_CAM_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.mask = 0x1,
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  static struct clockdomain usbhost_clkdm = {
>>> @@ -238,6 +279,12 @@ static struct clockdomain usbhost_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.mask = 0x1,
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  static struct clockdomain per_clkdm = {
>>> @@ -246,6 +293,13 @@ static struct clockdomain per_clkdm = {
>>>  	.flags		= CLKDM_CAN_HWSUP_SWSUP,
>>>  	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
>>>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>> +	.clk_reg_amt	= 1,
>>> +	.idle_def	= {
>>> +		[0] = {
>>> +			.ignore = OMAP3430_ST_UART3_MASK,
>>> +			.mask = 0x1fff,
>>> +		},
>>> +	},
>>>  };
>>>  
>>>  /*
>>> diff --git a/arch/arm/mach-omap2/powerdomain.c 
>>b/arch/arm/mach-omap2/powerdomain.c
>>> index a08d596..46090bc 100644
>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>> @@ -1219,6 +1219,31 @@ int pwrdm_wait_transition(struct 
>>powerdomain *pwrdm)
>>>  	return 0;
>>>  }
>>>  
>>> +/**
>>> + * pwrdm_can_idle - check if the powerdomain can enter idle
>>> + * @pwrdm: struct powerdomain * the powerdomain to check status of
>>> + *
>>> + * Checks all associated clockdomains if they can idle or not.
>>> + * Returns 1 if the powerdomain can idle, 0 if not.
>>> + */
>>> +int pwrdm_can_idle(struct powerdomain *pwrdm)
>>> +{
>>> +	unsigned long flags;
>>> +	int i;
>>> +	int ret = 1;
>>> +
>>> +	read_lock_irqsave(&pwrdm_rwlock, flags);
>>> +
>>> +	for (i = 0; i < PWRDM_MAX_CLKDMS; i++)
>>> +		if (pwrdm->pwrdm_clkdms[i] &&
>>> +		    !omap2_clkdm_can_idle(pwrdm->pwrdm_clkdms[i]))
>>> +			ret = 0;
>>> +
>>> +	read_unlock_irqrestore(&pwrdm_rwlock, flags);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  int pwrdm_state_switch(struct powerdomain *pwrdm)
>>>  {
>>>  	return _pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
>>> diff --git a/arch/arm/plat-omap/include/plat/clockdomain.h 
>>b/arch/arm/plat-omap/include/plat/clockdomain.h
>>> index eb73482..71cbc5c 100644
>>> --- a/arch/arm/plat-omap/include/plat/clockdomain.h
>>> +++ b/arch/arm/plat-omap/include/plat/clockdomain.h
>>> @@ -30,6 +30,12 @@
>>>  #define CLKDM_CAN_SWSUP		(CLKDM_CAN_FORCE_SLEEP 
>>| CLKDM_CAN_FORCE_WAKEUP)
>>>  #define CLKDM_CAN_HWSUP_SWSUP	(CLKDM_CAN_SWSUP | 
>>CLKDM_CAN_HWSUP)
>>>  
>>> +/*
>>> + * Maximum number of FCLK register masks that can be 
>>associated with a
>>> + * clockdomain. CORE powerdomain on OMAP3 is the worst case
>>> + */
>>> +#define CLKDM_MAX_CLK_REGS			3
>>> +
>>>  /* OMAP24XX CM_CLKSTCTRL_*.AUTOSTATE_* register bit values */
>>v>  #define OMAP24XX_CLKSTCTRL_DISABLE_AUTO		0x0
>>>  #define OMAP24XX_CLKSTCTRL_ENABLE_AUTO		0x1
>>> @@ -61,6 +67,11 @@ struct clkdm_pwrdm_autodep {
>>>  
>>>  };
>>>  
>>> +struct clkdm_idle_def {
>>> +	u32 ignore;
>>
>>I think fclk_ignore is a better name
>>
>>> +	u32 mask;
>>
>>and idlest_mask here.
>>
>>> +};
>>> +
>>>  struct clockdomain {
>>>  
>>>  	/* Clockdomain name */
>>> @@ -83,6 +94,10 @@ struct clockdomain {
>>>  	/* OMAP chip types that this clockdomain is valid on */
>>>  	const struct omap_chip_id omap_chip;
>>>  
>>> +	/* For idle checks */
>>> +	u8 clk_reg_amt;
>>
>>I think clk_reg_num is a better name here.
>>
>>> +	struct clkdm_idle_def idle_def[CLKDM_MAX_CLK_REGS];
>>> +
>>>  	/* Usecount tracking */
>>>  	atomic_t usecount;
>>>  
>>> @@ -108,4 +123,6 @@ int omap2_clkdm_sleep(struct clockdomain *clkdm);
>>>  int omap2_clkdm_clk_enable(struct clockdomain *clkdm, 
>>struct clk *clk);
>>>  int omap2_clkdm_clk_disable(struct clockdomain *clkdm, 
>>struct clk *clk);
>>>  
>>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm);
>>> +
>>>  #endif
>>> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
>>b/arch/arm/plat-omap/include/plat/powerdomain.h
>>> index 159e4ad..42f5f88 100644
>>> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
>>> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>>> @@ -182,6 +182,7 @@ int pwrdm_disable_hdwr_sar(struct 
>>powerdomain *pwrdm);
>>>  bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
>>>  
>>>  int pwrdm_wait_transition(struct powerdomain *pwrdm);
>>> +int pwrdm_can_idle(struct powerdomain *pwrdm);
>>>  
>>>  int pwrdm_state_switch(struct powerdomain *pwrdm);
>>>  int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
>>> -- 
>>> 1.5.4.3
>>

RE: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle

[Tero.Kristo]

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: 20 January, 2010 02:48
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for
>checking if pwrdm/clkdm can idle
>
><Tero.Kristo@nokia.com> writes:
>
>>
>>
>>>-----Original Message-----
>>>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>>Sent: 18 January, 2010 20:41
>>>To: Kristo Tero (Nokia-D/Tampere)
>>>Cc: linux-omap@vger.kernel.org
>>>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for
>>>checking if pwrdm/clkdm can idle
>>>
>>>Tero Kristo <tero.kristo@nokia.com> writes:
>>>
>>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>>
>>>> pwrdm_can_idle(pwrdm) will check if the specified
>>>powerdomain can enter
>>>> idle. This is done by checking all clockdomains under the
>powerdomain
>>>> if they can idle also.
>>>>
>>>> omap2_clkdm_can_idle(clkdm) will check if the specified
>>>clockdomain can
>>>> enter idle. This checks the functional clocks and idle
>>>status bits of the
>>>> domain according to following rules:
>>>> 1) get inverse of idlest and mask against idle_def.mask
>>>>   * this gives a bitmask with non-idle bits high
>>>> 2) bitwise OR active functional clocks from the domain to
>the result
>>>>   * in some cases FCLK can be active but idlest still shows
>>>module in idle
>>>> 3) disable bits defined in idle_def.mask
>>>
>>>disable?  it looks like they're just being ignored.
>>
>> Oh yes, just a small typo here in patch description. This
>should indeed say that they are ignored, as the bit-field also
>is named as ignore.
>>
>>>
>>>>   * some bits should be ignored, like UART clocks which are
>>>dynamically
>>>>     switched inside sleep loop
>>>>
>>>> These calls can be used e.g. inside cpuidle to decide which
>>>power states
>>>> core and mpu should enter during idle, as there are certain
>>>dependencies
>>>> between wakeup capabilities and reset logic.
>>>>
>>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>>
>>>Thanks for adding the IDLEST checks, I think that will help.
>>
>> Yes, and this is actually required for the secure side clock status
>> handling, this information is not available anywhere else, thus it
>> is impossible to get the idle status from the clock framework
>> usecounts.
>>
>>>
>>>In the .mask field, instead of using a hard-coded mask,
>probably using
>>>the existing bitfields headers would be a bit more readable, and help
>>>ensure correctness.
>>
>> Well, the bitfield definitions for those do not exist at the
>> moment.
>
>We could use the EN bit definitions for the CM_FCLKEN* since they're
>the same for IDLEST.

Some bits do not exist as EN bit definitions, but they do exist as ST. E.g secure clocks do not have EN bit, and some CORE domain EN bits are not definied either as they do not exist. I will update the masks in the code according to this, and just use ST bits.

>
>> The idea of the mask is just to get all bits in that are
>> needed, e.g. PER domain does not have GPIO bits in the mask, as
>> these appear to always be in a state that they are accessible
>> (i.e. IDLEST shows accessible for them.)
>
>And I think if we used the #defines here, it would be much clearer
>what bits are included and which are left out.
>
>Some comments as to which ones are needed or not would be helpful too.

Ok, I'll inline the comments with the masks.

>
>> I can write all IDLEST bits into the masks (as was done before) if
>> you like this approach better. This will generate rather long
>> definitions though, and the probability of errors in those is
>> actually higher than in this version (I just write a number of low
>> bits to one.)
>
>I guess you didn't see the rest of my comments further down in the
>original review.  There was at least one error already in the SGX
>since it's bit is not bit 0 but but 1.  Using the existing #defines
>would've solved that.

True, I will update the masks according to this.

>
>On that note, please check out the other inline comments in the
>original review.
>
>I should've mentioned that there were further comments inline in the
>original review.  Sorry for the confusion.

Yeah, sorry missed those, will comment now.

>
>Kevin
>
>>>
>>>Hopefully there's a way to auto-generate these for OMAP4.
>>>
>>>> ---
>>>>  arch/arm/mach-omap2/clockdomain.c             |   27 ++++++++++++
>>>>  arch/arm/mach-omap2/clockdomains.h            |   54
>>>+++++++++++++++++++++++++
>>>>  arch/arm/mach-omap2/powerdomain.c             |   25 +++++++++++
>>>>  arch/arm/plat-omap/include/plat/clockdomain.h |   17 ++++++++
>>>>  arch/arm/plat-omap/include/plat/powerdomain.h |    1 +
>>>>  5 files changed, 124 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/clockdomain.c
>>>b/arch/arm/mach-omap2/clockdomain.c
>>>> index dd285f0..f42111a 100644
>>>> --- a/arch/arm/mach-omap2/clockdomain.c
>>>> +++ b/arch/arm/mach-omap2/clockdomain.c
>>>> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct
>>>clockdomain *clkdm)
>>>>    return 0;
>>>>  }
>>>>
>>>> +
>>>> +/**
>>>> + * omap2_clkdm_can_idle - check if clockdomain has any
>>>active clocks or not
>>>> + * @clkdm: struct clockdomain *
>>>> + *
>>>> + * Checks if the clockdomain has any active clock or not,
>>>i.e. whether it
>>>> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if
>>>unable to idle;
>>>> + * 1 if can idle.
>>>> + */
>>>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
>>>> +{
>>>> +  int i;
>>>> +
>>>> +  if (!clkdm)
>>>> +          return -EINVAL;
>>>> +
>>>> +  for (i = 0; i < clkdm->clk_reg_amt; i++)
>>>> +          if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>>> +                          CM_IDLEST + 4 * i) &
>>>> +                          clkdm->idle_def[i].mask) |
>>>> +
>>>cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>>> +                          CM_FCLKEN + 4 * i)) &
>>>> +                          ~clkdm->idle_def[i].ignore)
>>>
>>>This has some readability/indent issues.
>>>
>>>Also, could check fclk first, if active fclks, no reason to check
>>>idlest.  How about something like this (untested, but I
>*think* I kept
>>>the same logic):
>>>
>>>     for (i = 0; i < clkdm->clk_reg_amt; i++) {
>>>             u32 idlest, fclk;
>>>
>>>             fclk = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>>                                    CM_FCLKEN + 4 * i);
>>>             if (fclk & ~clkdm->idle_def[i].ignore)
>>>                     return 0;
>>>
>>>             idlest = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>>>                                      CM_IDLEST + 4 * i);
>>>             if (~idlest & clkdm->idle_def[i].mask)
>>>                     return 0;
>>>     }

Ok, I'll rewrite the code to something like this. Early exit by just checking fclk especially is a good change.


>>>
>>>> +                  return 0;
>>>> +  return 1;
>>>> +}
>>>> +
>>>>  /**
>>>>   * omap2_clkdm_allow_idle - enable hwsup idle transitions
>for clkdm
>>>>   * @clkdm: struct clockdomain *
>>>> diff --git a/arch/arm/mach-omap2/clockdomains.h
>>>b/arch/arm/mach-omap2/clockdomains.h
>>>> index c4ee076..2c1f2eb 100644
>>>> --- a/arch/arm/mach-omap2/clockdomains.h
>>>> +++ b/arch/arm/mach-omap2/clockdomains.h
>>>> @@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .mask = 0x1,
>>>
>>>Use OMAP3430_CM_FCLKEN_IVA2_EN_IVA2_MASK

Or ST mask actually, these are against IDLEST bits anyway.

>>>
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  static struct clockdomain gfx_3430es1_clkdm = {
>>>> @@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .mask = 0x1,
>>>
>>>Use OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK
>>>
>>>And here's a a good reason for using the pre-defined bitmasks.
>>> According to
>>>the #defines (and the TRM) The single EN bit in SGX is
>>>actually in bit 1, not bit 0)

Yeah, nice catch. Though, this does not generate any bugs in case of SGX, as the FCLK check should always work properly. ;)

>>>
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  /*
>>>> @@ -206,6 +218,23 @@ static struct clockdomain
>core_l3_34xx_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP,
>>>>    .clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>> +  .clk_reg_amt    = 3,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .ignore = OMAP3430_ST_UART2_MASK |
>>>> +                          OMAP3430_ST_UART1_MASK |
>>>> +                          OMAP3430_ST_SDRC_MASK |
>>>> +                          OMAP3430_ST_OMAPCTRL_MASK |
>>>> +                          OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,
>>>
>>>Hmm, why ignore OTG mask?  I think we need some comments as
>to why all
>>>of these are ignored.  You mention the UARTs in the
>changelog, but not
>>>the others.

I'll add inline comment for these. Reasons here though as a reply:

UARTs -> handled by PM idle loop
SDRC -> idled automatically by HW
OMAPCTRL -> idled automatically by HW
HSOTGUSB_IDLE_MASK -> appears to be always accessible, side effect of the OTG autoidle fix I think


>>>
>>>> +                  .mask = 0xffffffff,
>>>
>>>There are a lot of reserved bits here that I'm not sure we should be
>>>checking.

I'll just put all the CORE ST bits here.

>>>
>>>> +          },
>>>> +          [1] = {
>>>> +                  .mask = 0x1f,
>>>> +          },
>>>> +          [2] = {
>>>> +                  .mask = 0x4,
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  static struct clockdomain core_l4_34xx_clkdm = {
>>>> @@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .mask = 0x1,
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  static struct clockdomain cam_clkdm = {
>>>> @@ -230,6 +265,12 @@ static struct clockdomain cam_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430_CLKTRCTRL_CAM_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .mask = 0x1,
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  static struct clockdomain usbhost_clkdm = {
>>>> @@ -238,6 +279,12 @@ static struct clockdomain usbhost_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .mask = 0x1,
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  static struct clockdomain per_clkdm = {
>>>> @@ -246,6 +293,13 @@ static struct clockdomain per_clkdm = {
>>>>    .flags          = CLKDM_CAN_HWSUP_SWSUP,
>>>>    .clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
>>>>    .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>> +  .clk_reg_amt    = 1,
>>>> +  .idle_def       = {
>>>> +          [0] = {
>>>> +                  .ignore = OMAP3430_ST_UART3_MASK,
>>>> +                  .mask = 0x1fff,
>>>> +          },
>>>> +  },
>>>>  };
>>>>
>>>>  /*
>>>> diff --git a/arch/arm/mach-omap2/powerdomain.c
>>>b/arch/arm/mach-omap2/powerdomain.c
>>>> index a08d596..46090bc 100644
>>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>>> @@ -1219,6 +1219,31 @@ int pwrdm_wait_transition(struct
>>>powerdomain *pwrdm)
>>>>    return 0;
>>>>  }
>>>>
>>>> +/**
>>>> + * pwrdm_can_idle - check if the powerdomain can enter idle
>>>> + * @pwrdm: struct powerdomain * the powerdomain to check status of
>>>> + *
>>>> + * Checks all associated clockdomains if they can idle or not.
>>>> + * Returns 1 if the powerdomain can idle, 0 if not.
>>>> + */
>>>> +int pwrdm_can_idle(struct powerdomain *pwrdm)
>>>> +{
>>>> +  unsigned long flags;
>>>> +  int i;
>>>> +  int ret = 1;
>>>> +
>>>> +  read_lock_irqsave(&pwrdm_rwlock, flags);
>>>> +
>>>> +  for (i = 0; i < PWRDM_MAX_CLKDMS; i++)
>>>> +          if (pwrdm->pwrdm_clkdms[i] &&
>>>> +              !omap2_clkdm_can_idle(pwrdm->pwrdm_clkdms[i]))
>>>> +                  ret = 0;
>>>> +
>>>> +  read_unlock_irqrestore(&pwrdm_rwlock, flags);
>>>> +
>>>> +  return ret;
>>>> +}
>>>> +
>>>>  int pwrdm_state_switch(struct powerdomain *pwrdm)
>>>>  {
>>>>    return _pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
>>>> diff --git a/arch/arm/plat-omap/include/plat/clockdomain.h
>>>b/arch/arm/plat-omap/include/plat/clockdomain.h
>>>> index eb73482..71cbc5c 100644
>>>> --- a/arch/arm/plat-omap/include/plat/clockdomain.h
>>>> +++ b/arch/arm/plat-omap/include/plat/clockdomain.h
>>>> @@ -30,6 +30,12 @@
>>>>  #define CLKDM_CAN_SWSUP           (CLKDM_CAN_FORCE_SLEEP
>>>| CLKDM_CAN_FORCE_WAKEUP)
>>>>  #define CLKDM_CAN_HWSUP_SWSUP     (CLKDM_CAN_SWSUP |
>>>CLKDM_CAN_HWSUP)
>>>>
>>>> +/*
>>>> + * Maximum number of FCLK register masks that can be
>>>associated with a
>>>> + * clockdomain. CORE powerdomain on OMAP3 is the worst case
>>>> + */
>>>> +#define CLKDM_MAX_CLK_REGS                        3
>>>> +
>>>>  /* OMAP24XX CM_CLKSTCTRL_*.AUTOSTATE_* register bit values */
>>>v>  #define OMAP24XX_CLKSTCTRL_DISABLE_AUTO          0x0
>>>>  #define OMAP24XX_CLKSTCTRL_ENABLE_AUTO            0x1
>>>> @@ -61,6 +67,11 @@ struct clkdm_pwrdm_autodep {
>>>>
>>>>  };
>>>>
>>>> +struct clkdm_idle_def {
>>>> +  u32 ignore;
>>>
>>>I think fclk_ignore is a better name
>>>
>>>> +  u32 mask;
>>>
>>>and idlest_mask here.

Ok, will change.

>>>
>>>> +};
>>>> +
>>>>  struct clockdomain {
>>>>
>>>>    /* Clockdomain name */
>>>> @@ -83,6 +94,10 @@ struct clockdomain {
>>>>    /* OMAP chip types that this clockdomain is valid on */
>>>>    const struct omap_chip_id omap_chip;
>>>>
>>>> +  /* For idle checks */
>>>> +  u8 clk_reg_amt;
>>>
>>>I think clk_reg_num is a better name here.

Ok.

>>>
>>>> +  struct clkdm_idle_def idle_def[CLKDM_MAX_CLK_REGS];
>>>> +
>>>>    /* Usecount tracking */
>>>>    atomic_t usecount;
>>>>
>>>> @@ -108,4 +123,6 @@ int omap2_clkdm_sleep(struct
>clockdomain *clkdm);
>>>>  int omap2_clkdm_clk_enable(struct clockdomain *clkdm,
>>>struct clk *clk);
>>>>  int omap2_clkdm_clk_disable(struct clockdomain *clkdm,
>>>struct clk *clk);
>>>>
>>>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm);
>>>> +
>>>>  #endif
>>>> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h
>>>b/arch/arm/plat-omap/include/plat/powerdomain.h
>>>> index 159e4ad..42f5f88 100644
>>>> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
>>>> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>>>> @@ -182,6 +182,7 @@ int pwrdm_disable_hdwr_sar(struct
>>>powerdomain *pwrdm);
>>>>  bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
>>>>
>>>>  int pwrdm_wait_transition(struct powerdomain *pwrdm);
>>>> +int pwrdm_can_idle(struct powerdomain *pwrdm);
>>>>
>>>>  int pwrdm_state_switch(struct powerdomain *pwrdm);
>>>>  int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
>>>> --
>>>> 1.5.4.3
>>>
>

Re: [PATCHv3 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level

[Paul Walmsley]

Hi Tero,

I regret the delay in reviewing.  I haven't kept up on the comments on 
this thread, so if I've asked a question that you've already answered, 
please feel free to point me to the response.

A few comments/questions:

On Fri, 15 Jan 2010, Tero Kristo wrote:

> From: Tero Kristo <tero.kristo@nokia.com>
> 
> Currently only ON, RET and OFF are supported, and ON is arguably broken as it
> allows the powerdomain to enter INACTIVE state unless idle is prevented.
> Now, pwrdm code prevents idle if ON is selected, and also adds support for
> INACTIVE. This simplifies the control needed inside sleep code.
> 
> This patch also requires caching of next power state inside powerdomain code,
> as INACTIVE is not directly supported by hardware.

The idea seems like a good one, and a simplification for the PM code.  I'm 
a little worried that this patch mixes hardware-programmable powerdomain 
states with logical powerdomain states.  I wonder if we would be better 
off separating, for example, the logic of putting a powerdomain into 
INACTIVE state and any other logical powerdomain management, from the 
physical details of how the chip is programmed.  Just looking for your 
comments on this, not necessarily any changes in your patches in this 
regard.

> Next powerstate is thus now stored for each powerdomain in next_state.
> 
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c             |   32 ++++++++++++++++++++----
>  arch/arm/mach-omap2/powerdomains34xx.h        |   26 ++++++++++----------
>  arch/arm/plat-omap/include/plat/powerdomain.h |    4 +++
>  3 files changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 26b3f3e..a08d596 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -110,8 +110,8 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
>  static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>  {
>  
> -	int prev;
> -	int state;
> +	u8 prev;
> +	u8 state;
>  
>  	if (pwrdm == NULL)
>  		return -EINVAL;
> @@ -218,7 +218,9 @@ int pwrdm_register(struct powerdomain *pwrdm)
>  
>  	pr_debug("powerdomain: registered %s\n", pwrdm->name);
>  	ret = 0;
> -
> +	pwrdm->next_state =
> +		prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
> +			OMAP_POWERSTATE_MASK);
>  pr_unlock:
>  	write_unlock_irqrestore(&pwrdm_rwlock, flags);
>  
> @@ -699,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm)
>   */
>  int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>  {
> +	u8 prg_pwrst;
> +
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> +	if (pwrdm->next_state == pwrst)
> +		return 0;
> +
>  	if (!(pwrdm->pwrsts & (1 << pwrst)))
>  		return -EINVAL;
>  
>  	pr_debug("powerdomain: setting next powerstate for %s to %0x\n",
>  		 pwrdm->name, pwrst);
>  
> +	/* INACTIVE is reserved, so we program pwrdm as ON */
> +	if (pwrst == PWRDM_POWER_INACTIVE)
> +		prg_pwrst = PWRDM_POWER_ON;
> +	else
> +		prg_pwrst = pwrst;
> +
>  	prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
> -			     (pwrst << OMAP_POWERSTATE_SHIFT),
> +			     (prg_pwrst << OMAP_POWERSTATE_SHIFT),
>  			     pwrdm->prcm_offs, PM_PWSTCTRL);
>  
> +	/* If next state is ON, prevent idle */
> +	if (pwrst == PWRDM_POWER_ON)
> +		omap2_clkdm_deny_idle(pwrdm->pwrdm_clkdms[0]);
> +	else
> +		omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);

Looks like this will force clockdomains into hardware-supervised mode, 
even if they were originally programmed into software-supervised mode.
Please fix this so clockdomains that were originally programmed into 
software-supervised mode aren't inadvertently switched.

> +
> +	pwrdm->next_state = pwrst;
> +
>  	return 0;
>  }
>  
> @@ -728,8 +749,7 @@ int pwrdm_read_next_pwrst(struct powerdomain *pwrdm)
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	return prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
> -					OMAP_POWERSTATE_MASK);
> +	return pwrdm->next_state;
>  }
>  
>  /**
> diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
> index 588f7e0..f50a3f2 100644
> --- a/arch/arm/mach-omap2/powerdomains34xx.h
> +++ b/arch/arm/mach-omap2/powerdomains34xx.h
> @@ -165,7 +165,7 @@ static struct powerdomain iva2_pwrdm = {
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>  	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_IVA2_SHIFT,
>  	.wkdep_srcs	  = iva2_wkdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>  	.banks		  = 4,
>  	.pwrsts_mem_ret	  = {
> @@ -188,7 +188,7 @@ static struct powerdomain mpu_34xx_pwrdm = {
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>  	.dep_bit	  = OMAP3430_EN_MPU_SHIFT,
>  	.wkdep_srcs	  = mpu_34xx_wkdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>  	.flags		  = PWRDM_HAS_MPU_QUIRK,
>  	.banks		  = 1,
> @@ -207,7 +207,7 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 |
>  					   CHIP_IS_OMAP3430ES2 |
>  					   CHIP_IS_OMAP3430ES3_0),
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
>  	.banks		  = 2,
>  	.pwrsts_mem_ret	  = {
> @@ -215,8 +215,8 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
>  		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
>  	},
>  	.pwrsts_mem_on	  = {
> -		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
> -		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
> +		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
> +		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */

What's the thinking behind changing these memory bank states?  Will some 
code try to call pwrdm_set_mem_onst() with pwrst = INACTIVE, and if so, 
what should it do?  Similarly, if we add a pwrdm_read_mem_onst(), under 
what conditions should it return pwrst = INACTIVE?  Will we also need 
next_state variables for all of the banks?

>  	},
>  };
>  
> @@ -225,7 +225,7 @@ static struct powerdomain core_34xx_es3_1_pwrdm = {
>  	.name		  = "core_pwrdm",
>  	.prcm_offs	  = CORE_MOD,
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES3_1),
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
>  	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBTLL only */
>  	.banks		  = 2,
> @@ -234,8 +234,8 @@ static struct powerdomain core_34xx_es3_1_pwrdm = {
>  		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
>  	},
>  	.pwrsts_mem_on	  = {
> -		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
> -		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
> +		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
> +		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
>  	},
>  };
>  
> @@ -247,7 +247,7 @@ static struct powerdomain dss_pwrdm = {
>  	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_DSS_SHIFT,
>  	.wkdep_srcs	  = cam_dss_wkdeps,
>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>  	.banks		  = 1,
>  	.pwrsts_mem_ret	  = {
> @@ -287,7 +287,7 @@ static struct powerdomain cam_pwrdm = {
>  	.prcm_offs	  = OMAP3430_CAM_MOD,
>  	.wkdep_srcs	  = cam_dss_wkdeps,
>  	.sleepdep_srcs	  = cam_gfx_sleepdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>  	.banks		  = 1,
>  	.pwrsts_mem_ret	  = {
> @@ -305,7 +305,7 @@ static struct powerdomain per_pwrdm = {
>  	.dep_bit	  = OMAP3430_EN_PER_SHIFT,
>  	.wkdep_srcs	  = per_usbhost_wkdeps,
>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>  	.banks		  = 1,
>  	.pwrsts_mem_ret	  = {
> @@ -327,7 +327,7 @@ static struct powerdomain neon_pwrdm = {
>  	.prcm_offs	  = OMAP3430_NEON_MOD,
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>  	.wkdep_srcs	  = neon_wkdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>  };
>  
> @@ -337,7 +337,7 @@ static struct powerdomain usbhost_pwrdm = {
>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>  	.wkdep_srcs	  = per_usbhost_wkdeps,
>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>  	/*
>  	 * REVISIT: Enabling usb host save and restore mechanism seems to
> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
> index 0b96005..159e4ad 100644
> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
> @@ -39,6 +39,9 @@
>  
>  #define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>  
> +#define PWRSTS_OFF_RET_INA_ON	(PWRSTS_OFF_RET_ON | \
> +				 (1 << PWRDM_POWER_INACTIVE))
> +
>  
>  /* Powerdomain flags */
>  #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware save-and-restore support */
> @@ -123,6 +126,7 @@ struct powerdomain {
>  	struct list_head node;
>  
>  	int state;
> +	int next_state;
>  	unsigned state_counter[PWRDM_MAX_PWRSTS];
>  
>  #ifdef CONFIG_PM_DEBUG
> -- 
> 1.5.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

regards,

- Paul

RE: [PATCHv3 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level

[Tero.Kristo]

Hi Paul,

Thanks for comments, my responses inline below.

>-----Original Message-----
>From: ext Paul Walmsley [mailto:paul@pwsan.com] 
>Sent: 21 January, 2010 06:35
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
>Subject: Re: [PATCHv3 1/6] OMAP: Powerdomains: Add support for 
>INACTIVE state on pwrdm level
>
>Hi Tero,
>
>I regret the delay in reviewing.  I haven't kept up on the comments on 
>this thread, so if I've asked a question that you've already answered, 
>please feel free to point me to the response.
>
>A few comments/questions:
>
>On Fri, 15 Jan 2010, Tero Kristo wrote:
>
>> From: Tero Kristo <tero.kristo@nokia.com>
>> 
>> Currently only ON, RET and OFF are supported, and ON is 
>arguably broken as it
>> allows the powerdomain to enter INACTIVE state unless idle 
>is prevented.
>> Now, pwrdm code prevents idle if ON is selected, and also 
>adds support for
>> INACTIVE. This simplifies the control needed inside sleep code.
>> 
>> This patch also requires caching of next power state inside 
>powerdomain code,
>> as INACTIVE is not directly supported by hardware.
>
>The idea seems like a good one, and a simplification for the 
>PM code.  I'm 
>a little worried that this patch mixes hardware-programmable 
>powerdomain 
>states with logical powerdomain states.  I wonder if we would 
>be better 
>off separating, for example, the logic of putting a powerdomain into 
>INACTIVE state and any other logical powerdomain management, from the 
>physical details of how the chip is programmed.  Just looking for your 
>comments on this, not necessarily any changes in your patches in this 
>regard.
>
>> Next powerstate is thus now stored for each powerdomain in 
>next_state.
>> 
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>> ---
>>  arch/arm/mach-omap2/powerdomain.c             |   32 
>++++++++++++++++++++----
>>  arch/arm/mach-omap2/powerdomains34xx.h        |   26 
>++++++++++----------
>>  arch/arm/plat-omap/include/plat/powerdomain.h |    4 +++
>>  3 files changed, 43 insertions(+), 19 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/powerdomain.c 
>b/arch/arm/mach-omap2/powerdomain.c
>> index 26b3f3e..a08d596 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -110,8 +110,8 @@ static struct powerdomain 
>*_pwrdm_deps_lookup(struct powerdomain *pwrdm,
>>  static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>>  {
>>  
>> -	int prev;
>> -	int state;
>> +	u8 prev;
>> +	u8 state;
>>  
>>  	if (pwrdm == NULL)
>>  		return -EINVAL;
>> @@ -218,7 +218,9 @@ int pwrdm_register(struct powerdomain *pwrdm)
>>  
>>  	pr_debug("powerdomain: registered %s\n", pwrdm->name);
>>  	ret = 0;
>> -
>> +	pwrdm->next_state =
>> +		prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
>> +			OMAP_POWERSTATE_MASK);
>>  pr_unlock:
>>  	write_unlock_irqrestore(&pwrdm_rwlock, flags);
>>  
>> @@ -699,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct 
>powerdomain *pwrdm)
>>   */
>>  int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>>  {
>> +	u8 prg_pwrst;
>> +
>>  	if (!pwrdm)
>>  		return -EINVAL;
>>  
>> +	if (pwrdm->next_state == pwrst)
>> +		return 0;
>> +
>>  	if (!(pwrdm->pwrsts & (1 << pwrst)))
>>  		return -EINVAL;
>>  
>>  	pr_debug("powerdomain: setting next powerstate for %s to %0x\n",
>>  		 pwrdm->name, pwrst);
>>  
>> +	/* INACTIVE is reserved, so we program pwrdm as ON */
>> +	if (pwrst == PWRDM_POWER_INACTIVE)
>> +		prg_pwrst = PWRDM_POWER_ON;
>> +	else
>> +		prg_pwrst = pwrst;
>> +
>>  	prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
>> -			     (pwrst << OMAP_POWERSTATE_SHIFT),
>> +			     (prg_pwrst << OMAP_POWERSTATE_SHIFT),
>>  			     pwrdm->prcm_offs, PM_PWSTCTRL);
>>  
>> +	/* If next state is ON, prevent idle */
>> +	if (pwrst == PWRDM_POWER_ON)
>> +		omap2_clkdm_deny_idle(pwrdm->pwrdm_clkdms[0]);
>> +	else
>> +		omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
>
>Looks like this will force clockdomains into hardware-supervised mode, 
>even if they were originally programmed into software-supervised mode.
>Please fix this so clockdomains that were originally programmed into 
>software-supervised mode aren't inadvertently switched.

I think the change here would be to only deny/allow idle if we are in hwsup mode. Probably needs caching of the setup also. I'll do the change for this.

>
>> +
>> +	pwrdm->next_state = pwrst;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -728,8 +749,7 @@ int pwrdm_read_next_pwrst(struct 
>powerdomain *pwrdm)
>>  	if (!pwrdm)
>>  		return -EINVAL;
>>  
>> -	return prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL,
>> -					OMAP_POWERSTATE_MASK);
>> +	return pwrdm->next_state;
>>  }
>>  
>>  /**
>> diff --git a/arch/arm/mach-omap2/powerdomains34xx.h 
>b/arch/arm/mach-omap2/powerdomains34xx.h
>> index 588f7e0..f50a3f2 100644
>> --- a/arch/arm/mach-omap2/powerdomains34xx.h
>> +++ b/arch/arm/mach-omap2/powerdomains34xx.h
>> @@ -165,7 +165,7 @@ static struct powerdomain iva2_pwrdm = {
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>  	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_IVA2_SHIFT,
>>  	.wkdep_srcs	  = iva2_wkdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>>  	.banks		  = 4,
>>  	.pwrsts_mem_ret	  = {
>> @@ -188,7 +188,7 @@ static struct powerdomain mpu_34xx_pwrdm = {
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>  	.dep_bit	  = OMAP3430_EN_MPU_SHIFT,
>>  	.wkdep_srcs	  = mpu_34xx_wkdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>>  	.flags		  = PWRDM_HAS_MPU_QUIRK,
>>  	.banks		  = 1,
>> @@ -207,7 +207,7 @@ static struct powerdomain 
>core_34xx_pre_es3_1_pwrdm = {
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 |
>>  					   CHIP_IS_OMAP3430ES2 |
>>  					   CHIP_IS_OMAP3430ES3_0),
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
>>  	.banks		  = 2,
>>  	.pwrsts_mem_ret	  = {
>> @@ -215,8 +215,8 @@ static struct powerdomain 
>core_34xx_pre_es3_1_pwrdm = {
>>  		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
>>  	},
>>  	.pwrsts_mem_on	  = {
>> -		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
>> -		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
>> +		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
>> +		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
>
>What's the thinking behind changing these memory bank states?  
>Will some 
>code try to call pwrdm_set_mem_onst() with pwrst = INACTIVE, 
>and if so, 
>what should it do?  Similarly, if we add a 
>pwrdm_read_mem_onst(), under 
>what conditions should it return pwrst = INACTIVE?  Will we also need 
>next_state variables for all of the banks?

Did some checking on the code + TRM, and true, these memory bank states should not be altered, HW only supports certain subset of states. I will remove these pieces from the patch. This was just some bogus logic of mine thinking also these needed to be changed as I added the support for INA.


>
>>  	},
>>  };
>>  
>> @@ -225,7 +225,7 @@ static struct powerdomain 
>core_34xx_es3_1_pwrdm = {
>>  	.name		  = "core_pwrdm",
>>  	.prcm_offs	  = CORE_MOD,
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES3_1),
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.dep_bit	  = OMAP3430_EN_CORE_SHIFT,
>>  	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBTLL only */
>>  	.banks		  = 2,
>> @@ -234,8 +234,8 @@ static struct powerdomain 
>core_34xx_es3_1_pwrdm = {
>>  		[1] = PWRSTS_OFF_RET,	 /* MEM2RETSTATE */
>>  	},
>>  	.pwrsts_mem_on	  = {
>> -		[0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
>> -		[1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
>> +		[0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
>> +		[1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
>>  	},
>>  };
>>  
>> @@ -247,7 +247,7 @@ static struct powerdomain dss_pwrdm = {
>>  	.dep_bit	  = OMAP3430_PM_WKDEP_MPU_EN_DSS_SHIFT,
>>  	.wkdep_srcs	  = cam_dss_wkdeps,
>>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>>  	.banks		  = 1,
>>  	.pwrsts_mem_ret	  = {
>> @@ -287,7 +287,7 @@ static struct powerdomain cam_pwrdm = {
>>  	.prcm_offs	  = OMAP3430_CAM_MOD,
>>  	.wkdep_srcs	  = cam_dss_wkdeps,
>>  	.sleepdep_srcs	  = cam_gfx_sleepdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>>  	.banks		  = 1,
>>  	.pwrsts_mem_ret	  = {
>> @@ -305,7 +305,7 @@ static struct powerdomain per_pwrdm = {
>>  	.dep_bit	  = OMAP3430_EN_PER_SHIFT,
>>  	.wkdep_srcs	  = per_usbhost_wkdeps,
>>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRSTS_OFF_RET,
>>  	.banks		  = 1,
>>  	.pwrsts_mem_ret	  = {
>> @@ -327,7 +327,7 @@ static struct powerdomain neon_pwrdm = {
>>  	.prcm_offs	  = OMAP3430_NEON_MOD,
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>  	.wkdep_srcs	  = neon_wkdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>>  };
>>  
>> @@ -337,7 +337,7 @@ static struct powerdomain usbhost_pwrdm = {
>>  	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
>>  	.wkdep_srcs	  = per_usbhost_wkdeps,
>>  	.sleepdep_srcs	  = dss_per_usbhost_sleepdeps,
>> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
>> +	.pwrsts		  = PWRSTS_OFF_RET_INA_ON,
>>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>>  	/*
>>  	 * REVISIT: Enabling usb host save and restore 
>mechanism seems to
>> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
>b/arch/arm/plat-omap/include/plat/powerdomain.h
>> index 0b96005..159e4ad 100644
>> --- a/arch/arm/plat-omap/include/plat/powerdomain.h
>> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>> @@ -39,6 +39,9 @@
>>  
>>  #define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>>  
>> +#define PWRSTS_OFF_RET_INA_ON	(PWRSTS_OFF_RET_ON | \
>> +				 (1 << PWRDM_POWER_INACTIVE))
>> +
>>  
>>  /* Powerdomain flags */
>>  #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware 
>save-and-restore support */
>> @@ -123,6 +126,7 @@ struct powerdomain {
>>  	struct list_head node;
>>  
>>  	int state;
>> +	int next_state;
>>  	unsigned state_counter[PWRDM_MAX_PWRSTS];
>>  
>>  #ifdef CONFIG_PM_DEBUG
>> -- 
>> 1.5.4.3
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe 
>linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>
>regards,
>
>- Paul
>