[RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Jouni Hogander]

With this patch cpuidle boot problem with cpuidle doesn't exist. Also hang on wake-up seems to
disappear (haven't seen this far).

Main points in this patch are:
1. Add wkdep between neon and mpu
2. Add wkdep between per and core
3. Deny hwsup mode before writing next pwrst state
3. Make sure that order in idle loop is such that clocks are _really_
   enabled before accessing registers (serial & gpio).

This patch is meant to be applied on top of "OMAP3 CPUidle patches"
patch set and "OMAP3 CPUidle patches - fixes" patch sent by Rajendra
Nayak.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   82 ++--------------------------
 arch/arm/mach-omap2/pm34xx.c      |  109 +++++++++++++++++++++++++++++++++++--
 2 files changed, 108 insertions(+), 83 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 6440515..c14152f 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -404,8 +404,6 @@ void omap3_save_core_ctx(void)
 	omap_save_gpmc_ctx();
 	/* Save the system control module context, padconf already save above*/
 	omap_save_control_ctx();
-	omap_save_uart_ctx(0);
-	omap_save_uart_ctx(1);
 }
 
 void omap3_restore_core_ctx(void)
@@ -416,11 +414,10 @@ void omap3_restore_core_ctx(void)
 	omap_restore_gpmc_ctx();
 	/* Restore the interrupt controller context */
 	omap_restore_intc_ctx();
-	omap_restore_uart_ctx(0);
-	omap_restore_uart_ctx(1);
 	padconf_saved = 0;
 }
 
+int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
 /* omap3_enter_idle - Programs OMAP3 to enter the specified state.
  * returns the total time during which the system was idle.
  */
@@ -429,8 +426,7 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 {
 	struct omap3_processor_cx *cx = cpuidle_get_statedata(state);
 	struct timespec ts_preidle, ts_postidle, ts_idle;
-	struct powerdomain *mpu_pd, *core_pd, *per_pd, *neon_pd;
-	int per_pwrst, neon_pwrst;
+	struct powerdomain *mpu_pd, *core_pd;
 
 	current_cx_state = *cx;
 
@@ -450,49 +446,9 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 
 	mpu_pd = pwrdm_lookup("mpu_pwrdm");
 	core_pd = pwrdm_lookup("core_pwrdm");
-	per_pd = pwrdm_lookup("per_pwrdm");
-	neon_pd = pwrdm_lookup("neon_pwrdm");
-
-	/* Reset previous power state registers */
-	pwrdm_clear_all_prev_pwrst(mpu_pd);
-	pwrdm_clear_all_prev_pwrst(neon_pd);
-	pwrdm_clear_all_prev_pwrst(core_pd);
-	pwrdm_clear_all_prev_pwrst(per_pd);
-
-	per_pwrst = pwrdm_read_pwrst(per_pd);
-	neon_pwrst = pwrdm_read_pwrst(neon_pd);
-
-	/* Do this before any changes to PRCM registers */
-	if (cx->core_state == PWRDM_POWER_OFF)
-		omap3_save_prcm_ctx();
-
-	/* Program MPU to target state */
-	if (cx->mpu_state < PWRDM_POWER_ON) {
-		pwrdm_set_next_pwrst(neon_pd, cx->mpu_state);
-		pwrdm_set_next_pwrst(mpu_pd, cx->mpu_state);
-	}
 
-	/* Program CORE and PER to target state */
-	if (cx->core_state < PWRDM_POWER_ON) {
-		omap2_gpio_prepare_for_retention();
-		if (clocks_off_while_idle) {
-			omap3_save_per_ctx();
-			per_gpio_clk_disable();
-			omap_save_uart_ctx(2);
-			omap_serial_enable_clocks(0, 2);
-		}
-		if (cx->core_state == PWRDM_POWER_OFF)
-			omap3_save_core_ctx();
-		/* Disable UART1/UART2 clocks here. Done using direct register
-		* writes as using clock f/w calls results in a hang in prcm_
-		* interrupt_handler trying to clear WKST for CORE
-		*/
-		cm_clear_mod_reg_bits(0x6000, CORE_MOD, CM_ICLKEN1);
-		cm_clear_mod_reg_bits(0x6000, CORE_MOD, CM_FCLKEN1);
-		pwrdm_set_next_pwrst(core_pd, cx->core_state);
-	}
-
-	*(scratchpad_restore_addr) = restore_pointer_address;
+        set_pwrdm_state(mpu_pd, cx->mpu_state);
+	set_pwrdm_state(core_pd, cx->core_state);
 
 	if (omap_irq_pending())
 		goto return_sleep_time;
@@ -500,34 +456,6 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 	/* Execute ARM wfi */
 	omap_sram_idle();
 
-	*(scratchpad_restore_addr) = 0x0;
-
-	if (cx->core_state < PWRDM_POWER_ON) {
-		cm_set_mod_reg_bits(0x6000, CORE_MOD, CM_ICLKEN1);
-		cm_set_mod_reg_bits(0x6000, CORE_MOD, CM_FCLKEN1);
-		if ((cx->core_state == PWRDM_POWER_OFF)
-		&& (pwrdm_read_prev_pwrst(core_pd) == PWRDM_POWER_OFF)) {
-			omap3_restore_core_ctx();
-			omap3_restore_prcm_ctx();
-			omap3_restore_sram_ctx();
-		}
-		pwrdm_set_next_pwrst(core_pd, PWRDM_POWER_ON);
-		if (clocks_off_while_idle) {
-			omap_serial_enable_clocks(1, 2);
-			omap_restore_uart_ctx(2);
-			per_gpio_clk_enable();
-			omap3_restore_per_ctx();
-		}
-		omap2_gpio_resume_after_retention();
-	}
-
-	pr_debug("MPU prev st:%x,NEON prev st:%x\n",
-				pwrdm_read_prev_pwrst(mpu_pd),
-				pwrdm_read_prev_pwrst(neon_pd));
-	pr_debug("PER prev st:%x,CORE prev st:%x\n",
-				pwrdm_read_prev_pwrst(per_pd),
-				pwrdm_read_prev_pwrst(core_pd));
-
 return_sleep_time:
 	getnstimeofday(&ts_postidle);
 	ts_idle = timespec_sub(ts_postidle, ts_preidle);
@@ -617,7 +545,7 @@ void omap_init_power_states(void)
 	omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_RET;
 	omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_ON;
 	omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
-						CPUIDLE_FLAG_BALANCED;
+                                               CPUIDLE_FLAG_BALANCED;
 
 	/* C3 . MPU OFF + Core active */
 	omap3_power_states[OMAP3_STATE_C3].valid = 1;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index dc7bb90..1f9c58a 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -65,6 +65,20 @@ void (*_omap_sram_idle)(u32 *addr, int save_state, int disable_clocks);
 static void (*saved_idle)(void);
 
 static struct powerdomain *mpu_pwrdm;
+static struct powerdomain *neon_pwrdm, * per_pwrdm;
+static struct powerdomain *core_pwrdm;
+
+void omap3_save_per_ctx(void);
+void omap3_restore_per_ctx(void);
+void omap3_save_core_ctx(void);
+void omap3_restore_core_ctx(void);
+void omap3_save_sram_ctx(void);
+void omap3_restore_sram_ctx(void);
+void omap3_save_prcm_ctx(void);
+void omap3_restore_prcm_ctx(void);
+void omap_save_uart_ctx(int unum);
+void omap_restore_uart_ctx(int unum);
+int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
 
 /* XXX This is for gpio fclk hack. Will be removed as gpio driver
  * handles fcks correctly */
@@ -224,11 +238,19 @@ void omap_sram_idle(void)
 	/* save_state = 1 => Only L1 and logic lost */
 	/* save_state = 2 => Only L2 lost */
 	/* save_state = 3 => L1, L2 and logic lost */
-	int save_state = 0, mpu_next_state;
+	int save_state = 0;
+	int mpu_next_state = PWRDM_POWER_ON, core_next_state = PWRDM_POWER_ON,
+		per_next_state = PWRDM_POWER_ON;
+	int mpu_prev_state, core_prev_state, per_prev_state;
 
 	if (!_omap_sram_idle)
 		return;
 
+	pwrdm_clear_all_prev_pwrst(mpu_pwrdm);
+	pwrdm_clear_all_prev_pwrst(neon_pwrdm);
+	pwrdm_clear_all_prev_pwrst(core_pwrdm);
+	pwrdm_clear_all_prev_pwrst(per_pwrdm);
+
 	mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 	switch (mpu_next_state) {
 	case PWRDM_POWER_ON:
@@ -245,10 +267,79 @@ void omap_sram_idle(void)
 		return;
 	}
 
-	_omap_sram_idle(context_mem, save_state, clocks_off_while_idle);
-	/* Restore table entry modified during MMU restoration */
+	/* NEON control */
+	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
+		set_pwrdm_state(neon_pwrdm, mpu_next_state);
+
+	/* CORE & PER */
+	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
+	if (core_next_state < PWRDM_POWER_ON) {
+		omap2_gpio_prepare_for_retention();
+		/* PER changes only with core */
+		per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
+		if (per_next_state < PWRDM_POWER_ON) {
+			if (per_next_state == PWRDM_POWER_OFF) {
+				omap3_save_per_ctx();
+				omap_save_uart_ctx(2);
+			}
+			if (clocks_off_while_idle) {
+				per_gpio_clk_disable();
+				omap_serial_enable_clocks(0, 2);
+			}
+		}
+		if (core_next_state == PWRDM_POWER_OFF) {
+			omap3_save_core_ctx();
+			omap3_save_prcm_ctx();
+			omap_save_uart_ctx(0);
+			omap_save_uart_ctx(1);
+		}
+		if (clocks_off_while_idle) {
+			omap_serial_enable_clocks(0, 0);
+			omap_serial_enable_clocks(0, 1);
+		}
+		if (core_next_state == PWRDM_POWER_OFF)
+			omap3_save_prcm_ctx();
+	}
+
+	*(scratchpad_restore_addr) = restore_pointer_address;
+
+	_omap_sram_idle(context_mem, save_state, 1);
+
+	*(scratchpad_restore_addr) = 0x0;
+
 	if (pwrdm_read_prev_pwrst(mpu_pwrdm) == 0x0)
 		restore_table_entry();
+	
+	if (core_next_state < PWRDM_POWER_ON) {
+		core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm);
+		if (core_prev_state == PWRDM_POWER_OFF)
+			omap3_restore_prcm_ctx();
+		if (clocks_off_while_idle) {
+			omap_serial_enable_clocks(1, 0);
+			omap_serial_enable_clocks(1, 1);
+		}
+		if (core_prev_state == PWRDM_POWER_OFF) {
+			omap3_restore_core_ctx();
+			omap3_restore_sram_ctx();
+			omap_restore_uart_ctx(0);
+			omap_restore_uart_ctx(1);
+		}
+		if (per_next_state < PWRDM_POWER_ON) {
+			if (clocks_off_while_idle) {
+				per_gpio_clk_enable();
+				/* This would be actually more effective */
+				omap_serial_enable_clocks(1, 2);
+			}
+			per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
+			if (per_prev_state == PWRDM_POWER_OFF) {
+				omap3_restore_per_ctx();
+				/* This would be actually more effective */
+				omap_restore_uart_ctx(2);
+			}
+
+		}
+		omap2_gpio_resume_after_retention();
+	}
 }
 
 static int omap3_fclks_active(void)
@@ -318,7 +409,7 @@ static int _clkdm_allow_idle(struct powerdomain *pwrdm,
 /* This sets pwrdm state (other than mpu & core. Currently only ON &
  * RET are supported. Function is assuming that clkdm doesn't have
  * hw_sup mode enabled. */
-static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
+int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 {
 	u32 cur_state;
 	int ret = 0;
@@ -657,8 +748,8 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
 	if (!pwrst)
 		return -ENOMEM;
 	pwrst->pwrdm = pwrdm;
-	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm"))
-		pwrst->next_state = PWRDM_POWER_RET;
+	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
+		pwrst->next_state = PWRDM_POWER_ON;
 	else
 		pwrst->next_state = PWRDM_POWER_OFF;
 	list_add(&pwrst->node, &pwrst_list);
@@ -708,6 +799,9 @@ int __init omap3_pm_init(void)
 		printk(KERN_ERR "Failed to get mpu_pwrdm\n");
 		goto err2;
 	}
+	neon_pwrdm = pwrdm_lookup("neon_pwrdm");
+	per_pwrdm = pwrdm_lookup("per_pwrdm");
+	core_pwrdm = pwrdm_lookup("core_pwrdm");
 
 	omap_push_sram_idle();
 	suspend_set_ops(&omap_pm_ops);
@@ -723,6 +817,9 @@ int __init omap3_pm_init(void)
 		gpio_fcks[i-1] = clk_get(NULL, clk_name);
 	}
 
+	pwrdm_add_wkdep(neon_pwrdm, mpu_pwrdm);
+	pwrdm_add_wkdep(per_pwrdm, core_pwrdm);
+
 err1:
 	return ret;
 err2:
-- 
1.5.5

[RFC] OMAP3: CPUIDLE & PM: Modifications and fixes. Suspend part.

[Jouni Hogander]

With this suspend also works.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   32 ++++++--------------------------
 1 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 1f9c58a..9f73e5c 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -503,29 +503,8 @@ static int omap3_pm_suspend(void)
 			goto restore;
 	}
 
-	omap2_gpio_prepare_for_retention();
-
-	if (clocks_off_while_idle) {
-		omap_serial_enable_clocks(0, 0);
-		omap_serial_enable_clocks(0, 1);
-		omap_serial_enable_clocks(0, 2);
-		/* XXX This is for gpio fclk hack. Will be removed as
-		 * gpio driver * handles fcks correctly */
-		per_gpio_clk_disable();
-	}
-
 	omap_sram_idle();
 
-	if (clocks_off_while_idle) {
-		omap_serial_enable_clocks(1, 0);
-		omap_serial_enable_clocks(1, 1);
-		omap_serial_enable_clocks(1, 2);
-		/* XXX This is for gpio fclk hack. Will be removed as
-		 * gpio driver * handles fcks correctly */
-		per_gpio_clk_enable();
-	}
-
-	omap2_gpio_resume_after_retention();
 restore:
 	/* Restore next_pwrsts */
 	list_for_each_entry(pwrst, &pwrst_list, node) {
@@ -748,16 +727,17 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
 	if (!pwrst)
 		return -ENOMEM;
 	pwrst->pwrdm = pwrdm;
-	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
-		pwrst->next_state = PWRDM_POWER_ON;
-	else
-		pwrst->next_state = PWRDM_POWER_OFF;
+	pwrst->next_state = PWRDM_POWER_OFF;
 	list_add(&pwrst->node, &pwrst_list);
 
 	if (pwrdm_has_hdwr_sar(pwrdm))
 		pwrdm_enable_hdwr_sar(pwrdm);
 
-	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
+	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
+	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
+		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
+	else
+		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
 }
 
 void omap_push_sram_idle()
-- 
1.5.5

[PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Jouni Hogander]

This patch fixes problems with uart usage. omap3_enter_idle_bm was
select C5 and C6 states even if there was "bus activity.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
 arch/arm/mach-omap2/pm34xx.c      |    2 +-
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index c14152f..a636edb 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 			       struct cpuidle_state *state)
 {
 	struct cpuidle_state *new_state = NULL;
-	int i, j;
-
-	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
-
-		/* Find current state in list */
-		for (i = 0; i < OMAP3_MAX_STATES; i++)
-			if (state == &dev->states[i])
-				break;
-		BUG_ON(i == OMAP3_MAX_STATES);
-
-		/* Back up to non 'CHECK_BM' state */
-		for (j = i - 1;  j > 0; j--) {
-			struct cpuidle_state *s = &dev->states[j];
+	int i;
 
+	if (omap3_idle_bm_check()) {
+		for (i = 0; i < OMAP3_MAX_STATES; i++) {
+			struct cpuidle_state *s = &dev->states[i];
 			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
 				new_state = s;
 				break;
 			}
 		}
-
+		BUG_ON(i == OMAP3_MAX_STATES);
 		pr_debug("%s: Bus activity: Entering %s (instead of %s)\n",
-			__FUNCTION__, new_state->name, state->name);
+			 __FUNCTION__, new_state->name, state->name);
 	}
 
-	return omap3_enter_idle(dev, new_state ? : state);
+	return omap3_enter_idle(dev, new_state ? new_state : state);
 }
 
 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 9f73e5c..ca0600a 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
 		pwrdm_enable_hdwr_sar(pwrdm);
 
 	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
-	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
+	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
 		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
 	else
 		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
-- 
1.5.5

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Rajendra Nayak]

 
> -----Original Message-----
> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
> Sent: Wednesday, July 09, 2008 12:00 PM
> To: linux-omap@vger.kernel.org; rnayak@ti.com
> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> 
> This patch fixes problems with uart usage. omap3_enter_idle_bm was
> select C5 and C6 states even if there was "bus activity.

Am not very clear on what was the issue with the previous logic that it would 
attempt a C5/C6 even with bus activity.

> 
> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index c14152f..a636edb 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
> cpuidle_device *dev,
>  			       struct cpuidle_state *state)
>  {
>  	struct cpuidle_state *new_state = NULL;
> -	int i, j;
> -
> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
> omap3_idle_bm_check()) {
> -
> -		/* Find current state in list */
> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
> -			if (state == &dev->states[i])
> -				break;
> -		BUG_ON(i == OMAP3_MAX_STATES);
> -
> -		/* Back up to non 'CHECK_BM' state */
> -		for (j = i - 1;  j > 0; j--) {
> -			struct cpuidle_state *s = &dev->states[j];
> +	int i;
>  
> +	if (omap3_idle_bm_check()) {
> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
> +			struct cpuidle_state *s = &dev->states[i];

So now a C0 is attempted every time there is bus activity?

>  			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
>  				new_state = s;
>  				break;
>  			}
>  		}
> -
> +		BUG_ON(i == OMAP3_MAX_STATES);
>  		pr_debug("%s: Bus activity: Entering %s 
> (instead of %s)\n",
> -			__FUNCTION__, new_state->name, state->name);
> +			 __FUNCTION__, new_state->name, state->name);
>  	}
>  
> -	return omap3_enter_idle(dev, new_state ? : state);
> +	return omap3_enter_idle(dev, new_state ? new_state : state);
>  }
>  
>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> diff --git a/arch/arm/mach-omap2/pm34xx.c 
> b/arch/arm/mach-omap2/pm34xx.c
> index 9f73e5c..ca0600a 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
> powerdomain *pwrdm)
>  		pwrdm_enable_hdwr_sar(pwrdm);
>  
>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
>  		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
>  	else
>  		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> -- 
> 1.5.5
> 
> 

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

"ext Rajendra Nayak" <rnayak@ti.com> writes:

>  
>> -----Original Message-----
>> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
>> Sent: Wednesday, July 09, 2008 12:00 PM
>> To: linux-omap@vger.kernel.org; rnayak@ti.com
>> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
>> 
>> This patch fixes problems with uart usage. omap3_enter_idle_bm was
>> select C5 and C6 states even if there was "bus activity.
>
> Am not very clear on what was the issue with the previous logic that it would 
> attempt a C5/C6 even with bus activity.

If you try it you will notice the difference;) What is the meaning of
this bm check if C5/C6 is used even when there is "acitivity". Anyway if
you want to have bm check code in your cpuidle driver you need to
rewrite the check code. I have seen that omap3_can_sleep partially as
a workaround. Just to make sure PM doesn't break any driver as long as
they are not all configured properly what comes to interconnect. Just
like what happened here as cpuidle was trying to enter C5/C6.

>
>> 
>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>> ---
>>  arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
>>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
>>  2 files changed, 8 insertions(+), 17 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
>> b/arch/arm/mach-omap2/cpuidle34xx.c
>> index c14152f..a636edb 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
>> cpuidle_device *dev,
>>  			       struct cpuidle_state *state)
>>  {
>>  	struct cpuidle_state *new_state = NULL;
>> -	int i, j;
>> -
>> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
>> omap3_idle_bm_check()) {
>> -
>> -		/* Find current state in list */
>> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
>> -			if (state == &dev->states[i])
>> -				break;
>> -		BUG_ON(i == OMAP3_MAX_STATES);
>> -
>> -		/* Back up to non 'CHECK_BM' state */
>> -		for (j = i - 1;  j > 0; j--) {
>> -			struct cpuidle_state *s = &dev->states[j];
>> +	int i;
>>  
>> +	if (omap3_idle_bm_check()) {
>> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
>> +			struct cpuidle_state *s = &dev->states[i];
>
> So now a C0 is attempted every time there is bus activity?

Yes you are right here. This code is not very effective. It should
select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.

>
>>  			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
>>  				new_state = s;
>>  				break;
>>  			}
>>  		}
>> -
>> +		BUG_ON(i == OMAP3_MAX_STATES);
>>  		pr_debug("%s: Bus activity: Entering %s 
>> (instead of %s)\n",
>> -			__FUNCTION__, new_state->name, state->name);
>> +			 __FUNCTION__, new_state->name, state->name);
>>  	}
>>  
>> -	return omap3_enter_idle(dev, new_state ? : state);
>> +	return omap3_enter_idle(dev, new_state ? new_state : state);
>>  }
>>  
>>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> diff --git a/arch/arm/mach-omap2/pm34xx.c 
>> b/arch/arm/mach-omap2/pm34xx.c
>> index 9f73e5c..ca0600a 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
>> powerdomain *pwrdm)
>>  		pwrdm_enable_hdwr_sar(pwrdm);
>>  
>>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
>> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
>> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
>> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
>>  		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
>>  	else
>>  		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>> -- 
>> 1.5.5
>> 
>> 
>
>
>

-- 
Jouni Högander

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Rajendra Nayak]

> -----Original Message-----
> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Wednesday, July 09, 2008 12:29 PM
> To: ext Rajendra Nayak
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> 
> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> 
> >  
> >> -----Original Message-----
> >> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
> >> Sent: Wednesday, July 09, 2008 12:00 PM
> >> To: linux-omap@vger.kernel.org; rnayak@ti.com
> >> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> >> 
> >> This patch fixes problems with uart usage. omap3_enter_idle_bm was
> >> select C5 and C6 states even if there was "bus activity.
> >
> > Am not very clear on what was the issue with the previous 
> logic that it would 
> > attempt a C5/C6 even with bus activity.
> 
> If you try it you will notice the difference;) What is the meaning of
> this bm check if C5/C6 is used even when there is 
> "acitivity".

No, What I meant to say was that the previous logic I thought was good enough to
*prevent* C5/C6 or any other state with CPUIDLE_FLAG_CHECK_BM flag set, while there is 
bus activity and then go ahead and select the highest possible state with 
CPUIDLE_FLAG_CHECK_BM _not_ set.

 Anyway if
> you want to have bm check code in your cpuidle driver you need to
> rewrite the check code. I have seen that omap3_can_sleep partially as
> a workaround. Just to make sure PM doesn't break any driver as long as
> they are not all configured properly what comes to interconnect. Just
> like what happened here as cpuidle was trying to enter C5/C6.
> 
> >
> >> 
> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> >> ---
> >>  arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
> >>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
> >>  2 files changed, 8 insertions(+), 17 deletions(-)
> >> 
> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> index c14152f..a636edb 100644
> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
> >> cpuidle_device *dev,
> >>  			       struct cpuidle_state *state)
> >>  {
> >>  	struct cpuidle_state *new_state = NULL;
> >> -	int i, j;
> >> -
> >> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
> >> omap3_idle_bm_check()) {
> >> -
> >> -		/* Find current state in list */
> >> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
> >> -			if (state == &dev->states[i])
> >> -				break;
> >> -		BUG_ON(i == OMAP3_MAX_STATES);
> >> -
> >> -		/* Back up to non 'CHECK_BM' state */
> >> -		for (j = i - 1;  j > 0; j--) {
> >> -			struct cpuidle_state *s = &dev->states[j];
> >> +	int i;
> >>  
> >> +	if (omap3_idle_bm_check()) {
> >> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
> >> +			struct cpuidle_state *s = &dev->states[i];
> >
> > So now a C0 is attempted every time there is bus activity?
> 
> Yes you are right here. This code is not very effective. It should
> select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.
> 
> >
> >>  			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
> >>  				new_state = s;
> >>  				break;
> >>  			}
> >>  		}
> >> -
> >> +		BUG_ON(i == OMAP3_MAX_STATES);
> >>  		pr_debug("%s: Bus activity: Entering %s 
> >> (instead of %s)\n",
> >> -			__FUNCTION__, new_state->name, state->name);
> >> +			 __FUNCTION__, new_state->name, state->name);
> >>  	}
> >>  
> >> -	return omap3_enter_idle(dev, new_state ? : state);
> >> +	return omap3_enter_idle(dev, new_state ? new_state : state);
> >>  }
> >>  
> >>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> >> diff --git a/arch/arm/mach-omap2/pm34xx.c 
> >> b/arch/arm/mach-omap2/pm34xx.c
> >> index 9f73e5c..ca0600a 100644
> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
> >> powerdomain *pwrdm)
> >>  		pwrdm_enable_hdwr_sar(pwrdm);
> >>  
> >>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
> >> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
> >> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
> >> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
> >>  		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
> >>  	else
> >>  		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> >> -- 
> >> 1.5.5
> >> 
> >> 
> >
> >
> >
> 
> -- 
> Jouni Högander
> 
> 

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

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

"ext Rajendra Nayak" <rnayak@ti.com> writes:

>> -----Original Message-----
>> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Wednesday, July 09, 2008 12:29 PM
>> To: ext Rajendra Nayak
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
>> 
>> "ext Rajendra Nayak" <rnayak@ti.com> writes:
>> 
>> >  
>> >> -----Original Message-----
>> >> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
>> >> Sent: Wednesday, July 09, 2008 12:00 PM
>> >> To: linux-omap@vger.kernel.org; rnayak@ti.com
>> >> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
>> >> 
>> >> This patch fixes problems with uart usage. omap3_enter_idle_bm was
>> >> select C5 and C6 states even if there was "bus activity.
>> >
>> > Am not very clear on what was the issue with the previous 
>> logic that it would 
>> > attempt a C5/C6 even with bus activity.
>> 
>> If you try it you will notice the difference;) What is the meaning of
>> this bm check if C5/C6 is used even when there is 
>> "acitivity".
>
> No, What I meant to say was that the previous logic I thought was good enough to
> *prevent* C5/C6 or any other state with CPUIDLE_FLAG_CHECK_BM flag set, while there is 
> bus activity and then go ahead and select the highest possible state with 
> CPUIDLE_FLAG_CHECK_BM _not_ set.

Yes you are right. Current implementation is ok, maybe this change?:

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index c14152f..7c97c7d 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -484,7 +484,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
                BUG_ON(i == OMAP3_MAX_STATES);
 
                /* Back up to non 'CHECK_BM' state */
-               for (j = i - 1;  j > 0; j--) {
+               for (j = i - 1;  j >= 0; j--) {
                        struct cpuidle_state *s = &dev->states[j];
 
                        if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {


This caused my misunderstanding. I tried to mark all states except C0
with bm check flag and omap3_enter_idle_bm was selecting C6 even when
there was "activity".

Anyway serial console seems to work very slow if mpu is entering
wfi. This can be checked by marking C1 and C2 with bm check flag.

>
>  Anyway if
>> you want to have bm check code in your cpuidle driver you need to
>> rewrite the check code. I have seen that omap3_can_sleep partially as
>> a workaround. Just to make sure PM doesn't break any driver as long as
>> they are not all configured properly what comes to interconnect. Just
>> like what happened here as cpuidle was trying to enter C5/C6.
>> 
>> >
>> >> 
>> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>> >> ---
>> >>  arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
>> >>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
>> >>  2 files changed, 8 insertions(+), 17 deletions(-)
>> >> 
>> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
>> >> b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> index c14152f..a636edb 100644
>> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
>> >> cpuidle_device *dev,
>> >>  			       struct cpuidle_state *state)
>> >>  {
>> >>  	struct cpuidle_state *new_state = NULL;
>> >> -	int i, j;
>> >> -
>> >> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
>> >> omap3_idle_bm_check()) {
>> >> -
>> >> -		/* Find current state in list */
>> >> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
>> >> -			if (state == &dev->states[i])
>> >> -				break;
>> >> -		BUG_ON(i == OMAP3_MAX_STATES);
>> >> -
>> >> -		/* Back up to non 'CHECK_BM' state */
>> >> -		for (j = i - 1;  j > 0; j--) {
>> >> -			struct cpuidle_state *s = &dev->states[j];
>> >> +	int i;
>> >>  
>> >> +	if (omap3_idle_bm_check()) {
>> >> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
>> >> +			struct cpuidle_state *s = &dev->states[i];
>> >
>> > So now a C0 is attempted every time there is bus activity?
>> 
>> Yes you are right here. This code is not very effective. It should
>> select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.
>> 
>> >
>> >>  			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
>> >>  				new_state = s;
>> >>  				break;
>> >>  			}
>> >>  		}
>> >> -
>> >> +		BUG_ON(i == OMAP3_MAX_STATES);
>> >>  		pr_debug("%s: Bus activity: Entering %s 
>> >> (instead of %s)\n",
>> >> -			__FUNCTION__, new_state->name, state->name);
>> >> +			 __FUNCTION__, new_state->name, state->name);
>> >>  	}
>> >>  
>> >> -	return omap3_enter_idle(dev, new_state ? : state);
>> >> +	return omap3_enter_idle(dev, new_state ? new_state : state);
>> >>  }
>> >>  
>> >>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> >> diff --git a/arch/arm/mach-omap2/pm34xx.c 
>> >> b/arch/arm/mach-omap2/pm34xx.c
>> >> index 9f73e5c..ca0600a 100644
>> >> --- a/arch/arm/mach-omap2/pm34xx.c
>> >> +++ b/arch/arm/mach-omap2/pm34xx.c
>> >> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
>> >> powerdomain *pwrdm)
>> >>  		pwrdm_enable_hdwr_sar(pwrdm);
>> >>  
>> >>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
>> >> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
>> >> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
>> >> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
>> >>  		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
>> >>  	else
>> >>  		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>> >> -- 
>> >> 1.5.5
>> >> 
>> >> 
>> >
>> >
>> >
>> 
>> -- 
>> Jouni Högander
>> 
>> 
>
> --
> 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
>
>

-- 
Jouni Högander

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

[RFC] OMAP3: CPUIDLE & PM: Fix slow serial-console

[Jouni Hogander]

This patch fixes slowness on serial console. This patch superseeds
"OMAP3: CPUIDLE & PM: check_bm fix".
---
 arch/arm/mach-omap2/cpuidle34xx.c |    3 ++-
 arch/arm/mach-omap2/pm34xx.c      |    2 --
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index c14152f..b227d9b 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -30,6 +30,7 @@
 #include <asm/arch/gpio.h>
 #include <asm/arch/gpmc.h>
 #include <asm/arch/control.h>
+#include <asm/arch/common.h>
 #include <linux/sched.h>
 #include "cpuidle34xx.h"
 #include "cm.h"
@@ -430,7 +431,7 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 
 	current_cx_state = *cx;
 
-	if (cx->type == OMAP3_STATE_C0) {
+	if (cx->type == OMAP3_STATE_C0 || !omap_serial_can_sleep()) {
 		/* Do nothing for C0, not even a wfi */
 		return 0;
 	}
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 9f73e5c..9bb0fba 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -385,8 +385,6 @@ int omap3_can_sleep(void)
 		return 0;
 	if (atomic_read(&sleep_block) > 0)
 		return 0;
-	if (!omap_serial_can_sleep())
-		return 0;
 	return 1;
 }
 
-- 
1.5.5

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

Hi, Rajendra

I think we have now all the pieces here to get stable cpuidle +
pm_idle + suspend + off mode with minimal configuration on SDP
board. Could you Rajendra now gather these together and prepare new
patch set in sensible format?

"ext Rajendra Nayak" <rnayak@ti.com> writes:

>> -----Original Message-----
>> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Wednesday, July 09, 2008 12:29 PM
>> To: ext Rajendra Nayak
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
>> 
>> "ext Rajendra Nayak" <rnayak@ti.com> writes:
>> 
>> >  
>> >> -----Original Message-----
>> >> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
>> >> Sent: Wednesday, July 09, 2008 12:00 PM
>> >> To: linux-omap@vger.kernel.org; rnayak@ti.com
>> >> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
>> >> 
>> >> This patch fixes problems with uart usage. omap3_enter_idle_bm was
>> >> select C5 and C6 states even if there was "bus activity.
>> >
>> > Am not very clear on what was the issue with the previous 
>> logic that it would 
>> > attempt a C5/C6 even with bus activity.
>> 
>> If you try it you will notice the difference;) What is the meaning of
>> this bm check if C5/C6 is used even when there is 
>> "acitivity".
>
> No, What I meant to say was that the previous logic I thought was good enough to
> *prevent* C5/C6 or any other state with CPUIDLE_FLAG_CHECK_BM flag set, while there is 
> bus activity and then go ahead and select the highest possible state with 
> CPUIDLE_FLAG_CHECK_BM _not_ set.
>
>  Anyway if
>> you want to have bm check code in your cpuidle driver you need to
>> rewrite the check code. I have seen that omap3_can_sleep partially as
>> a workaround. Just to make sure PM doesn't break any driver as long as
>> they are not all configured properly what comes to interconnect. Just
>> like what happened here as cpuidle was trying to enter C5/C6.
>> 
>> >
>> >> 
>> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>> >> ---
>> >>  arch/arm/mach-omap2/cpuidle34xx.c |   23 +++++++----------------
>> >>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
>> >>  2 files changed, 8 insertions(+), 17 deletions(-)
>> >> 
>> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
>> >> b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> index c14152f..a636edb 100644
>> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
>> >> cpuidle_device *dev,
>> >>  			       struct cpuidle_state *state)
>> >>  {
>> >>  	struct cpuidle_state *new_state = NULL;
>> >> -	int i, j;
>> >> -
>> >> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
>> >> omap3_idle_bm_check()) {
>> >> -
>> >> -		/* Find current state in list */
>> >> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
>> >> -			if (state == &dev->states[i])
>> >> -				break;
>> >> -		BUG_ON(i == OMAP3_MAX_STATES);
>> >> -
>> >> -		/* Back up to non 'CHECK_BM' state */
>> >> -		for (j = i - 1;  j > 0; j--) {
>> >> -			struct cpuidle_state *s = &dev->states[j];
>> >> +	int i;
>> >>  
>> >> +	if (omap3_idle_bm_check()) {
>> >> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
>> >> +			struct cpuidle_state *s = &dev->states[i];
>> >
>> > So now a C0 is attempted every time there is bus activity?
>> 
>> Yes you are right here. This code is not very effective. It should
>> select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.
>> 
>> >
>> >>  			if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
>> >>  				new_state = s;
>> >>  				break;
>> >>  			}
>> >>  		}
>> >> -
>> >> +		BUG_ON(i == OMAP3_MAX_STATES);
>> >>  		pr_debug("%s: Bus activity: Entering %s 
>> >> (instead of %s)\n",
>> >> -			__FUNCTION__, new_state->name, state->name);
>> >> +			 __FUNCTION__, new_state->name, state->name);
>> >>  	}
>> >>  
>> >> -	return omap3_enter_idle(dev, new_state ? : state);
>> >> +	return omap3_enter_idle(dev, new_state ? new_state : state);
>> >>  }
>> >>  
>> >>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> >> diff --git a/arch/arm/mach-omap2/pm34xx.c 
>> >> b/arch/arm/mach-omap2/pm34xx.c
>> >> index 9f73e5c..ca0600a 100644
>> >> --- a/arch/arm/mach-omap2/pm34xx.c
>> >> +++ b/arch/arm/mach-omap2/pm34xx.c
>> >> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
>> >> powerdomain *pwrdm)
>> >>  		pwrdm_enable_hdwr_sar(pwrdm);
>> >>  
>> >>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
>> >> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
>> >> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
>> >> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
>> >>  		return set_pwrdm_state(pwrst->pwrdm, PWRDM_POWER_ON);
>> >>  	else
>> >>  		return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>> >> -- 
>> >> 1.5.5
>> >> 
>> >> 
>> >
>> >
>> >
>> 
>> -- 
>> Jouni Högander
>> 
>> 
>
> --
> 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
>
>

-- 
Jouni Högander

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Rajendra Nayak]

 

> -----Original Message-----
> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Wednesday, July 09, 2008 2:05 PM
> To: ext Rajendra Nayak
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> 
> Hi, Rajendra
> 
> I think we have now all the pieces here to get stable cpuidle +
> pm_idle + suspend + off mode with minimal configuration on SDP
> board. Could you Rajendra now gather these together and prepare new
> patch set in sensible format?

Yes, I will do that. Will re-send the complete patch set with all the fixes in place.

> 
> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> >> Sent: Wednesday, July 09, 2008 12:29 PM
> >> To: ext Rajendra Nayak
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> >> 
> >> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> >> 
> >> >  
> >> >> -----Original Message-----
> >> >> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
> >> >> Sent: Wednesday, July 09, 2008 12:00 PM
> >> >> To: linux-omap@vger.kernel.org; rnayak@ti.com
> >> >> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> >> >> 
> >> >> This patch fixes problems with uart usage. 
> omap3_enter_idle_bm was
> >> >> select C5 and C6 states even if there was "bus activity.
> >> >
> >> > Am not very clear on what was the issue with the previous 
> >> logic that it would 
> >> > attempt a C5/C6 even with bus activity.
> >> 
> >> If you try it you will notice the difference;) What is the 
> meaning of
> >> this bm check if C5/C6 is used even when there is 
> >> "acitivity".
> >
> > No, What I meant to say was that the previous logic I 
> thought was good enough to
> > *prevent* C5/C6 or any other state with 
> CPUIDLE_FLAG_CHECK_BM flag set, while there is 
> > bus activity and then go ahead and select the highest 
> possible state with 
> > CPUIDLE_FLAG_CHECK_BM _not_ set.
> >
> >  Anyway if
> >> you want to have bm check code in your cpuidle driver you need to
> >> rewrite the check code. I have seen that omap3_can_sleep 
> partially as
> >> a workaround. Just to make sure PM doesn't break any 
> driver as long as
> >> they are not all configured properly what comes to 
> interconnect. Just
> >> like what happened here as cpuidle was trying to enter C5/C6.
> >> 
> >> >
> >> >> 
> >> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> >> >> ---
> >> >>  arch/arm/mach-omap2/cpuidle34xx.c |   23 
> +++++++----------------
> >> >>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
> >> >>  2 files changed, 8 insertions(+), 17 deletions(-)
> >> >> 
> >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> >> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> index c14152f..a636edb 100644
> >> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
> >> >> cpuidle_device *dev,
> >> >>  			       struct cpuidle_state *state)
> >> >>  {
> >> >>  	struct cpuidle_state *new_state = NULL;
> >> >> -	int i, j;
> >> >> -
> >> >> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
> >> >> omap3_idle_bm_check()) {
> >> >> -
> >> >> -		/* Find current state in list */
> >> >> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
> >> >> -			if (state == &dev->states[i])
> >> >> -				break;
> >> >> -		BUG_ON(i == OMAP3_MAX_STATES);
> >> >> -
> >> >> -		/* Back up to non 'CHECK_BM' state */
> >> >> -		for (j = i - 1;  j > 0; j--) {
> >> >> -			struct cpuidle_state *s = 
> &dev->states[j];
> >> >> +	int i;
> >> >>  
> >> >> +	if (omap3_idle_bm_check()) {
> >> >> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
> >> >> +			struct cpuidle_state *s = 
> &dev->states[i];
> >> >
> >> > So now a C0 is attempted every time there is bus activity?
> >> 
> >> Yes you are right here. This code is not very effective. It should
> >> select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.
> >> 
> >> >
> >> >>  			if (!(s->flags & 
> CPUIDLE_FLAG_CHECK_BM)) {
> >> >>  				new_state = s;
> >> >>  				break;
> >> >>  			}
> >> >>  		}
> >> >> -
> >> >> +		BUG_ON(i == OMAP3_MAX_STATES);
> >> >>  		pr_debug("%s: Bus activity: Entering %s 
> >> >> (instead of %s)\n",
> >> >> -			__FUNCTION__, new_state->name, 
> state->name);
> >> >> +			 __FUNCTION__, new_state->name, 
> state->name);
> >> >>  	}
> >> >>  
> >> >> -	return omap3_enter_idle(dev, new_state ? : state);
> >> >> +	return omap3_enter_idle(dev, new_state ? 
> new_state : state);
> >> >>  }
> >> >>  
> >> >>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> >> >> diff --git a/arch/arm/mach-omap2/pm34xx.c 
> >> >> b/arch/arm/mach-omap2/pm34xx.c
> >> >> index 9f73e5c..ca0600a 100644
> >> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> >> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
> >> >> powerdomain *pwrdm)
> >> >>  		pwrdm_enable_hdwr_sar(pwrdm);
> >> >>  
> >> >>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
> >> >> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
> >> >> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
> >> >> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
> >> >>  		return set_pwrdm_state(pwrst->pwrdm, 
> PWRDM_POWER_ON);
> >> >>  	else
> >> >>  		return set_pwrdm_state(pwrst->pwrdm, 
> pwrst->next_state);
> >> >> -- 
> >> >> 1.5.5
> >> >> 
> >> >> 
> >> >
> >> >
> >> >
> >> 
> >> -- 
> >> Jouni Högander
> >> 
> >> 
> >
> > --
> > 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
> >
> >
> 
> -- 
> Jouni Högander
> 
> 

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Woodruff, Richard]

Hi,

> > I think we have now all the pieces here to get stable cpuidle +
> > pm_idle + suspend + off mode with minimal configuration on SDP
> > board. Could you Rajendra now gather these together and prepare new
> > patch set in sensible format?
>
> Yes, I will do that. Will re-send the complete patch set with all the
> fixes in place.

I'll add a couple notes based on recent changes in internal code.

Code has moved to using the safe_idle method like reference Intel driver code uses.
        - This removes for loop backing up to 1st non-BM state and makes idle path faster.
        - The biggest power-performance gain comes by making sure a INACTIVE state is hit.  If there is BM activity chances are MPU will be needed anyway and going to OFF probably isn't a win.  Getting 98% savings is ok.

Slowness on serial is there anytime you hit INACTIVE if you don't have the UART-NO-IDLE-ON-TX workaround in tree.  This is not in Tony's tree yet and probably should be if the UART is bothering you.  Choosing a non-WFI state penalty like your patch did is too expensive from a power standpoint for the effect.

Below patches for comment.

Regards,
Richard W.

--- arch/arm/mach-omap2/pm_idle_34xx.c@@/LINUX-GIT-2.6K_INT_FLOAT_4.0   2008-07-02 11:39:50.000000000 -0500
+++ arch/arm/mach-omap2/pm_idle_34xx.c  2008-07-04 11:54:15.000000000 -0500
@@ -706,32 +706,15 @@ return_sleep_time:
 static int omap3_enter_idle_bm(struct cpuidle_device *dev,
                                struct cpuidle_state *state)
 {
-       struct cpuidle_state *new_state = NULL;
-       int i, j;
-
        if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
-
-               /* Find current state in list */
-               for (i = 0; i < OMAP3_MAX_STATES; i++)
-                       if (state == &dev->states[i])
-                               break;
-               BUG_ON(i == OMAP3_MAX_STATES);
-
-               /* Back up to non 'CHECK_BM' state */
-               for (j = i - 1;  j > 0; j--) {
-                       struct cpuidle_state *s = &dev->states[j];
-
-                       if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
-                               new_state = s;
-                               break;
-                       }
+               if (dev->safe_state)
+                       return dev->safe_state->enter(dev, dev->safe_state);
+               else {
+                       omap_sram_idle();
+                       return 0;
                }
-
-               pr_debug("%s: Bus activity: Entering %s (instead of %s)\n",
-                       __FUNCTION__, new_state->name, state->name);
        }
-
-       return omap3_enter_idle(dev, new_state ? : state);
+       return omap3_enter_idle(dev, state);
 }

 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
@@ -766,8 +749,8 @@ void omap_init_power_states(void)
        omap3_power_states[OMAP3_STATE_C2].core_state = PRCM_CORE_ACTIVE;
        omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
                        CPUIDLE_FLAG_BALANCED;
-       omap3_power_states[OMAP3_STATE_C3].valid = 1;

+       omap3_power_states[OMAP3_STATE_C3].valid = 1;
        omap3_power_states[OMAP3_STATE_C3].type = OMAP3_STATE_C3;
        omap3_power_states[OMAP3_STATE_C3].sleep_latency = 1500;
        omap3_power_states[OMAP3_STATE_C3].wakeup_latency = 1800;
@@ -849,6 +832,8 @@ int omap3_idle_init(void)
                state->flags = cx->flags;
                state->enter = (state->flags & CPUIDLE_FLAG_CHECK_BM) ?
                        omap3_enter_idle_bm : omap3_enter_idle;
+               if (cx->type == OMAP3_STATE_C1)
+                       dev->safe_state = state;
                sprintf(state->name, "C%d", count+1);
                count++;
        }


diff -purN img/2.6_kernel/drivers/serial/8250.c 2.6_kernel/drivers/serial/8250.c
--- img/2.6_kernel/drivers/serial/8250.c        2008-05-09 18:50:53.000000000 -0500
+++ 2.6_kernel/drivers/serial/8250.c    2008-06-03 19:44:24.000000000 -0500
@@ -1221,6 +1221,14 @@ static inline void __stop_tx(struct uart
                p->ier &= ~UART_IER_THRI;
                serial_out(p, UART_IER, p->ier);
        }
+#ifdef CONFIG_OMAP3_PM
+       {
+               /* Advertise partial idle to save power. RX side can do async wake */
+               unsigned int tmp;
+               tmp = (serial_in(p, UART_OMAP_SYSC) & 0x7) | (2 << 3);
+               serial_out(p, UART_OMAP_SYSC, tmp); /* smart-idle */
+       }
+#endif
 }

 static void serial8250_stop_tx(struct uart_port *port)
@@ -1268,6 +1276,15 @@ static void serial8250_start_tx(struct u
                up->acr &= ~UART_ACR_TXDIS;
                serial_icr_write(up, UART_ACR, up->acr);
        }
+#ifdef CONFIG_OMAP3_PM
+       {
+               /* Don't advertise partial idle else TX irqs will not be seen */
+               /* Alternative is to set kernel timer at fifo drain rate */
+               unsigned int tmp;
+               tmp = (serial_in(up, UART_OMAP_SYSC) & 0x7) | (1 << 3);
+               serial_out(up, UART_OMAP_SYSC, tmp); /* no-idle */
+       }
+#endif
 }

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

Hi,

"ext Woodruff, Richard" <r-woodruff2@ti.com> writes:

> Hi,
>
>> > I think we have now all the pieces here to get stable cpuidle +
>> > pm_idle + suspend + off mode with minimal configuration on SDP
>> > board. Could you Rajendra now gather these together and prepare new
>> > patch set in sensible format?
>>
>> Yes, I will do that. Will re-send the complete patch set with all the
>> fixes in place.
>
> I'll add a couple notes based on recent changes in internal code.
>
> Code has moved to using the safe_idle method like reference Intel driver code uses.
>         - This removes for loop backing up to 1st non-BM state and makes idle path faster.
>         - The biggest power-performance gain comes by making sure a
> INACTIVE state is hit.  If there is BM activity chances are MPU will
> be needed anyway and going to OFF probably isn't a win.  Getting 98%
> savings is ok.

Impact of this change depends a lot on what is happening on the
system. HW triggered dma transfer comes to my mind first. In that case
mpu could be kept in OFF state while core is active. Changing bm
check this way would cause mpu to be kept in INACTIVE state. Any
analysis of the impact?

>
> Slowness on serial is there anytime you hit INACTIVE if you don't
> have the UART-NO-IDLE-ON-TX workaround in tree.  This is not in
> Tony's tree yet and probably should be if the UART is bothering you.
> Choosing a non-WFI state penalty like your patch did is too
> expensive from a power standpoint for the effect.

Well I think there is very similiar workaround in workaround patch set
(see omap_serial_can_sleep). Now non-WFI state is selected if there
was activity in last 5 seconds. So get just hands off from the
keyboard for 5 seconds and C state selection continues normally after
that. I think 5 seconds without powersave from last character entered
to serial-console is not too expensive. Benefit of working
serial-console is bigger I think.

>
> Below patches for comment.
>
> Regards,
> Richard W.
>
> --- arch/arm/mach-omap2/pm_idle_34xx.c@@/LINUX-GIT-2.6K_INT_FLOAT_4.0   2008-07-02 11:39:50.000000000 -0500
> +++ arch/arm/mach-omap2/pm_idle_34xx.c  2008-07-04 11:54:15.000000000 -0500
> @@ -706,32 +706,15 @@ return_sleep_time:
>  static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>                                 struct cpuidle_state *state)
>  {
> -       struct cpuidle_state *new_state = NULL;
> -       int i, j;
> -
>         if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
> -
> -               /* Find current state in list */
> -               for (i = 0; i < OMAP3_MAX_STATES; i++)
> -                       if (state == &dev->states[i])
> -                               break;
> -               BUG_ON(i == OMAP3_MAX_STATES);
> -
> -               /* Back up to non 'CHECK_BM' state */
> -               for (j = i - 1;  j > 0; j--) {
> -                       struct cpuidle_state *s = &dev->states[j];
> -
> -                       if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
> -                               new_state = s;
> -                               break;
> -                       }
> +               if (dev->safe_state)
> +                       return dev->safe_state->enter(dev, dev->safe_state);
> +               else {
> +                       omap_sram_idle();
> +                       return 0;
>                 }

How about adding that old loop into this else statement? Calling
omap_sram_idle strictly here has an effect that last selected state is
used (next states are not written). Also switching between safe_state
mechanism and loop would be very easy.

> -
> -               pr_debug("%s: Bus activity: Entering %s (instead of %s)\n",
> -                       __FUNCTION__, new_state->name, state->name);
>         }
> -
> -       return omap3_enter_idle(dev, new_state ? : state);

And then if previous comment having old loop in else statement is done
then this should be left in it's old format.
 
> +       return omap3_enter_idle(dev, state);
>  }
>
>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> @@ -766,8 +749,8 @@ void omap_init_power_states(void)
>         omap3_power_states[OMAP3_STATE_C2].core_state = PRCM_CORE_ACTIVE;
>         omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
>                         CPUIDLE_FLAG_BALANCED;
> -       omap3_power_states[OMAP3_STATE_C3].valid = 1;
>
> +       omap3_power_states[OMAP3_STATE_C3].valid = 1;
>         omap3_power_states[OMAP3_STATE_C3].type = OMAP3_STATE_C3;
>         omap3_power_states[OMAP3_STATE_C3].sleep_latency = 1500;
>         omap3_power_states[OMAP3_STATE_C3].wakeup_latency = 1800;
> @@ -849,6 +832,8 @@ int omap3_idle_init(void)
>                 state->flags = cx->flags;
>                 state->enter = (state->flags & CPUIDLE_FLAG_CHECK_BM) ?
>                         omap3_enter_idle_bm : omap3_enter_idle;
> +               if (cx->type == OMAP3_STATE_C1)
> +                       dev->safe_state = state;
>                 sprintf(state->name, "C%d", count+1);
>                 count++;
>         }
>
>
> diff -purN img/2.6_kernel/drivers/serial/8250.c 2.6_kernel/drivers/serial/8250.c
> --- img/2.6_kernel/drivers/serial/8250.c        2008-05-09 18:50:53.000000000 -0500
> +++ 2.6_kernel/drivers/serial/8250.c    2008-06-03 19:44:24.000000000 -0500
> @@ -1221,6 +1221,14 @@ static inline void __stop_tx(struct uart
>                 p->ier &= ~UART_IER_THRI;
>                 serial_out(p, UART_IER, p->ier);
>         }
> +#ifdef CONFIG_OMAP3_PM
> +       {
> +               /* Advertise partial idle to save power. RX side can do async wake */
> +               unsigned int tmp;
> +               tmp = (serial_in(p, UART_OMAP_SYSC) & 0x7) | (2 << 3);
> +               serial_out(p, UART_OMAP_SYSC, tmp); /* smart-idle */
> +       }
> +#endif
>  }
>
>  static void serial8250_stop_tx(struct uart_port *port)
> @@ -1268,6 +1276,15 @@ static void serial8250_start_tx(struct u
>                 up->acr &= ~UART_ACR_TXDIS;
>                 serial_icr_write(up, UART_ACR, up->acr);
>         }
> +#ifdef CONFIG_OMAP3_PM
> +       {
> +               /* Don't advertise partial idle else TX irqs will not be seen */
> +               /* Alternative is to set kernel timer at fifo drain rate */
> +               unsigned int tmp;
> +               tmp = (serial_in(up, UART_OMAP_SYSC) & 0x7) | (1 << 3);
> +               serial_out(up, UART_OMAP_SYSC, tmp); /* no-idle */
> +       }
> +#endif

I tried this quickly. This doesn't work alone. At least if we want
working serial-console. Problem with this is that when entering char
to console to wake-up omap. First character is lost and then no "echo"
happens and serial8250_start_tx is not called. I think this will need
some timeout anyway which is started when uart rx|iopad wakeup
happens.

>  }
>
>
>

-- 
Jouni Högander

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Woodruff, Richard]

Hi,

> >  static void serial8250_stop_tx(struct uart_port *port)
> > @@ -1268,6 +1276,15 @@ static void serial8250_start_tx(struct u
> >                 up->acr &= ~UART_ACR_TXDIS;
> >                 serial_icr_write(up, UART_ACR, up->acr);
> >         }
> > +#ifdef CONFIG_OMAP3_PM
> > +       {
> > +               /* Don't advertise partial idle else TX irqs will
> not be seen */
> > +               /* Alternative is to set kernel timer at fifo drain
> rate */
> > +               unsigned int tmp;
> > +               tmp = (serial_in(up, UART_OMAP_SYSC) & 0x7) | (1 <<
> 3);
> > +               serial_out(up, UART_OMAP_SYSC, tmp); /* no-idle */
> > +       }
> > +#endif
>
> I tried this quickly. This doesn't work alone. At least if we want
> working serial-console. Problem with this is that when entering char
> to console to wake-up omap. First character is lost and then no "echo"
> happens and serial8250_start_tx is not called. I think this will need
> some timeout anyway which is started when uart rx|iopad wakeup
> happens.

Yes that is true.

In reference code it is dealt with and rationalized.  This was an issue last year in the old code and will be this year in this newer code.

* CDP code employs an activity check which can gate idle.  The disruption was bothersome and gave a bad impression (even if its just a debug port).  It also interfered at times with existing functional tests which depended on console (either getting all characters or reasonable performance).

The current check will: On activity raise a cpuidle bus master activity failure for some number of seconds.  This allows normal typing for extended periods.  It does this by marking UART function IRQs with a time stamp and it checks internal state to make sure RX/TX engine is not busy or has queued data waiting.

This activity assertion will gate the usage of C states where its F-CLOCK is cut.  At the same time its natural wake up events are enabled (along with the above hack as the tx events are not currenly hooked into the wakeup logic).

When OFF/RET mode is selected IO pad is enabled for the port wakeup.

** The end effect is typing and input/output are good.  However, if you stop interacting for greater than the time out you will loose your 1st character.  This is unavoidable as the machine doesn't re-start fast enough to not loose the start bit (wakeup/DPLL relock).

It doesn't take much code to do this today.  But with out it the console is not very useable when PM is enabled _AND_ being effective.  As I mentioned initially, having the UART problem in a sense is a good milestone as it shows you are starting to hit the big power states very often.

Regards,
Richard W.

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

"ext Woodruff, Richard" <r-woodruff2@ti.com> writes:

> Hi,
>
>> >  static void serial8250_stop_tx(struct uart_port *port)
>> > @@ -1268,6 +1276,15 @@ static void serial8250_start_tx(struct u
>> >                 up->acr &= ~UART_ACR_TXDIS;
>> >                 serial_icr_write(up, UART_ACR, up->acr);
>> >         }
>> > +#ifdef CONFIG_OMAP3_PM
>> > +       {
>> > +               /* Don't advertise partial idle else TX irqs will
>> not be seen */
>> > +               /* Alternative is to set kernel timer at fifo drain
>> rate */
>> > +               unsigned int tmp;
>> > +               tmp = (serial_in(up, UART_OMAP_SYSC) & 0x7) | (1 <<
>> 3);
>> > +               serial_out(up, UART_OMAP_SYSC, tmp); /* no-idle */
>> > +       }
>> > +#endif
>>
>> I tried this quickly. This doesn't work alone. At least if we want
>> working serial-console. Problem with this is that when entering char
>> to console to wake-up omap. First character is lost and then no "echo"
>> happens and serial8250_start_tx is not called. I think this will need
>> some timeout anyway which is started when uart rx|iopad wakeup
>> happens.
>
> Yes that is true.
>
> In reference code it is dealt with and rationalized.  This was an issue last year in the old code and will be this year in this newer code.
>
> * CDP code employs an activity check which can gate idle.  The disruption was bothersome and gave a bad impression (even if its just a debug port).  It also interfered at times with existing functional tests which depended on console (either getting all characters or reasonable performance).
>
> The current check will: On activity raise a cpuidle bus master
> activity failure for some number of seconds.  This allows normal
> typing for extended periods.  It does this by marking UART function
> IRQs with a time stamp and it checks internal state to make sure
> RX/TX engine is not busy or has queued data waiting.

Isn't this exactly what is done in "Added sleep support to UART" patch
in workaround patch set?

>
> This activity assertion will gate the usage of C states where its F-CLOCK is cut.  At the same time its natural wake up events are enabled (along with the above hack as the tx events are not currenly hooked into the wakeup logic).
>
> When OFF/RET mode is selected IO pad is enabled for the port wakeup.

I have seen this in CDP reference code. Is there some specific reason
why this is enabled dynamically in code?

>
> ** The end effect is typing and input/output are good.  However, if you stop interacting for greater than the time out you will loose your 1st character.  This is unavoidable as the machine doesn't re-start fast enough to not loose the start bit (wakeup/DPLL relock).
>
> It doesn't take much code to do this today.  But with out it the console is not very useable when PM is enabled _AND_ being effective.  As I mentioned initially, having the UART problem in a sense is a good milestone as it shows you are starting to hit the big power states very often.
>
> Regards,
> Richard W.

-- 
Jouni Högander

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Woodruff, Richard]

Hi,

> Impact of this change depends a lot on what is happening on the
> system. HW triggered dma transfer comes to my mind first. In that case
> mpu could be kept in OFF state while core is active. Changing bm
> check this way would cause mpu to be kept in INACTIVE state. Any
> analysis of the impact?

Using C1 as a safe state will keep MPU up when CORE is INACTIVE.  You can use C2 or C3 just the same.  I found C1 to give best performance-per-power tradeoff in light tests.

Which state is best depends on activity of the system.  Unless you are sleeping for a good amount of time the pay back from RET/OFF is not as high.  Probably if you have a DMA or other going on the MPU will need to be active to service a completion in the near future.

There is a crossover point where calling OFF will burn more power than it will save because of context save and restore.

Anyway basic thought was BM activity failure probably means an interrupt sooner or later so take the biggest power drop and retain performance.  More characterization with end use cases would provide the best answer.

> >
> > Slowness on serial is there anytime you hit INACTIVE if you don't
> > have the UART-NO-IDLE-ON-TX workaround in tree.  This is not in
> > Tony's tree yet and probably should be if the UART is bothering you.
> > Choosing a non-WFI state penalty like your patch did is too
> > expensive from a power standpoint for the effect.
>
> Well I think there is very similiar workaround in workaround patch set
> (see omap_serial_can_sleep). Now non-WFI state is selected if there
> was activity in last 5 seconds. So get just hands off from the
> keyboard for 5 seconds and C state selection continues normally after
> that. I think 5 seconds without powersave from last character entered
> to serial-console is not too expensive. Benefit of working
> serial-console is bigger I think.

Maybe I should have commented in order (previous mail).

The omap_serial_can_sleep is similar to what I described before as being done to complete the work around.  We added this type of work around a couple years back on 2420 and adapted it for 3430 last year.

Probably the difference is if you use the UART SMART-FORCE method you can allow all C-States which don’t fall in the BM check.  If you abort the way you proposed you don't hit any idle states at all.

The major goal for _active_ use cases is to have the chip in INACTIVE state.  RET/OFF give most benefit for prolonged chip idle periods.

Code development (and test) will be a bit more true to end device performance if you allow a UART and your system hits INACTIVE.  You now hit and know your performance issues for active driver states before removing UART.  And you can debug them with the UART.

> > --- arch/arm/mach-omap2/pm_idle_34xx.c@@/LINUX-GIT-
> 2.6K_INT_FLOAT_4.0   2008-07-02 11:39:50.000000000 -0500
> > +++ arch/arm/mach-omap2/pm_idle_34xx.c  2008-07-04
> 11:54:15.000000000 -0500
> > @@ -706,32 +706,15 @@ return_sleep_time:
> >  static int omap3_enter_idle_bm(struct cpuidle_device *dev,
> >                                 struct cpuidle_state *state)
> >  {
> > -       struct cpuidle_state *new_state = NULL;
> > -       int i, j;
> > -
> >         if ((state->flags & CPUIDLE_FLAG_CHECK_BM) &&
> omap3_idle_bm_check()) {
> > -
> > -               /* Find current state in list */
> > -               for (i = 0; i < OMAP3_MAX_STATES; i++)
> > -                       if (state == &dev->states[i])
> > -                               break;
> > -               BUG_ON(i == OMAP3_MAX_STATES);
> > -
> > -               /* Back up to non 'CHECK_BM' state */
> > -               for (j = i - 1;  j > 0; j--) {
> > -                       struct cpuidle_state *s = &dev->states[j];
> > -
> > -                       if (!(s->flags & CPUIDLE_FLAG_CHECK_BM)) {
> > -                               new_state = s;
> > -                               break;
> > -                       }
> > +               if (dev->safe_state)
> > +                       return dev->safe_state->enter(dev, dev-
> >safe_state);
> > +               else {
> > +                       omap_sram_idle();
> > +                       return 0;
> >                 }
>
> How about adding that old loop into this else statement? Calling
> omap_sram_idle strictly here has an effect that last selected state is
> used (next states are not written). Also switching between safe_state
> mechanism and loop would be very easy.

Yes that probably is a safest thing.

Initially the thought was the non-safe state would never be called.  As the Intel code thought it necessary it seemed fine to do something similar.

I do agree that if it ever gets called there might be side effects.

Regards,
Richard W.

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Woodruff, Richard]

> > The current check will: On activity raise a cpuidle bus master
> > activity failure for some number of seconds.  This allows normal
> > typing for extended periods.  It does this by marking UART function
> > IRQs with a time stamp and it checks internal state to make sure
> > RX/TX engine is not busy or has queued data waiting.
>
> Isn't this exactly what is done in "Added sleep support to UART" patch
> in workaround patch set?

See last mail.  It probably is.  I assumed that was derived from our code which has been available for a long time before.  I didn't actually look at it very closely with that assumption.

> > This activity assertion will gate the usage of C states where its F-
> CLOCK is cut.  At the same time its natural wake up events are enabled
> (along with the above hack as the tx events are not currenly hooked
> into the wakeup logic).
> >
> > When OFF/RET mode is selected IO pad is enabled for the port wakeup.
>
> I have seen this in CDP reference code. Is there some specific reason
> why this is enabled dynamically in code?

Not that I am aware.

Do you think there is a need to toggle it?  Today only the global IOPAD enable is toggled (which is necessary for pad state latching).

Regards,
Richard W.

Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Högander Jouni]

"ext Woodruff, Richard" <r-woodruff2@ti.com> writes:

>> > The current check will: On activity raise a cpuidle bus master
>> > activity failure for some number of seconds.  This allows normal
>> > typing for extended periods.  It does this by marking UART function
>> > IRQs with a time stamp and it checks internal state to make sure
>> > RX/TX engine is not busy or has queued data waiting.
>>
>> Isn't this exactly what is done in "Added sleep support to UART" patch
>> in workaround patch set?
>
> See last mail.  It probably is.  I assumed that was derived from our code which has been available for a long time before.  I didn't actually look at it very closely with that assumption.
>
>> > This activity assertion will gate the usage of C states where its F-
>> CLOCK is cut.  At the same time its natural wake up events are enabled
>> (along with the above hack as the tx events are not currenly hooked
>> into the wakeup logic).
>> >
>> > When OFF/RET mode is selected IO pad is enabled for the port wakeup.
>>
>> I have seen this in CDP reference code. Is there some specific reason
>> why this is enabled dynamically in code?
>
> Not that I am aware.
>
> Do you think there is a need to toggle it?  Today only the global
> IOPAD enable is toggled (which is necessary for pad state latching).

No, I might have seen this global IOPAD enable. This is valuable
information. Currently in linux-omap setting IOPAD enable bit is done
on initialization. We haven't noticed any problems this far, but this
feature haven't been used too much. So probably we have been just
lucky?

-- 
Jouni Högander

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

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Woodruff, Richard]

> >> When OFF/RET mode is selected IO pad is enabled for the port
> wakeup.
> >>
> >> I have seen this in CDP reference code. Is there some specific
> reason
> >> why this is enabled dynamically in code?
> >
> > Not that I am aware.
> >
> > Do you think there is a need to toggle it?  Today only the global
> > IOPAD enable is toggled (which is necessary for pad state latching).
>
> No, I might have seen this global IOPAD enable. This is valuable
> information. Currently in linux-omap setting IOPAD enable bit is done
> on initialization. We haven't noticed any problems this far, but this
> feature haven't been used too much. So probably we have been just
> lucky?

The TRM does highlight this procedure of set-before-wake and clear-after-wake in several spots.  Search on PM_WKEN_WKUP.

I think I may have not worded that completely right.  To clarify, it was my understanding that the wakeup event generated by a pad event wouldn't be cleared with out the toggle.

After you wake up you can read pad status to find out which ball was responsible for the wakeup event. After your done inspecting them, use the toggle to clear it.

Each wakeup capable ball at MUX register has a status bit you read to see if it was the one which woke you up (if you care).  Unlike say an IRQ register status register there is no way to clear this at that interface.  Note there is no selectable polarity for that pin, so status reflects a change from latched state at sleep.

What is clearly written somewhere is the wake up daisy chain is enabled by turning it on, and disabled and _reset_ at turn off time.

The actual state latching happens at time of transition to sleep. The current level is captured, current padconfs saved into wakeup domain scratch memory and, wakeup armed.  On event it all unwinds.

Regards,
Richard W.




Regards,
Richard W.

RE: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.

[Rajendra Nayak]

Hi Jouni,

> I think we have now all the pieces here to get stable cpuidle +
> pm_idle + suspend + off mode with minimal configuration on SDP
> board. Could you Rajendra now gather these together and prepare new
> patch set in sensible format?

I was working on a refreshed patch set for CPUidle which would include all
the fixes discussed so far. 
Was wondering if all the workaround patches are now in mainline?
Would you be sending anymore updated patches for those, as I see still some 
pieces missing in the mainline.

regards,
Rajendra

> -----Original Message-----
> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Wednesday, July 09, 2008 2:05 PM
> To: ext Rajendra Nayak
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> 
> Hi, Rajendra
> 
> I think we have now all the pieces here to get stable cpuidle +
> pm_idle + suspend + off mode with minimal configuration on SDP
> board. Could you Rajendra now gather these together and prepare new
> patch set in sensible format?
> 
> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> >> Sent: Wednesday, July 09, 2008 12:29 PM
> >> To: ext Rajendra Nayak
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> >> 
> >> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> >> 
> >> >  
> >> >> -----Original Message-----
> >> >> From: Jouni Hogander [mailto:jouni.hogander@nokia.com] 
> >> >> Sent: Wednesday, July 09, 2008 12:00 PM
> >> >> To: linux-omap@vger.kernel.org; rnayak@ti.com
> >> >> Subject: [PATCH] OMAP3: CPUIDLE & PM: check_bm fix.
> >> >> 
> >> >> This patch fixes problems with uart usage. 
> omap3_enter_idle_bm was
> >> >> select C5 and C6 states even if there was "bus activity.
> >> >
> >> > Am not very clear on what was the issue with the previous 
> >> logic that it would 
> >> > attempt a C5/C6 even with bus activity.
> >> 
> >> If you try it you will notice the difference;) What is the 
> meaning of
> >> this bm check if C5/C6 is used even when there is 
> >> "acitivity".
> >
> > No, What I meant to say was that the previous logic I 
> thought was good enough to
> > *prevent* C5/C6 or any other state with 
> CPUIDLE_FLAG_CHECK_BM flag set, while there is 
> > bus activity and then go ahead and select the highest 
> possible state with 
> > CPUIDLE_FLAG_CHECK_BM _not_ set.
> >
> >  Anyway if
> >> you want to have bm check code in your cpuidle driver you need to
> >> rewrite the check code. I have seen that omap3_can_sleep 
> partially as
> >> a workaround. Just to make sure PM doesn't break any 
> driver as long as
> >> they are not all configured properly what comes to 
> interconnect. Just
> >> like what happened here as cpuidle was trying to enter C5/C6.
> >> 
> >> >
> >> >> 
> >> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> >> >> ---
> >> >>  arch/arm/mach-omap2/cpuidle34xx.c |   23 
> +++++++----------------
> >> >>  arch/arm/mach-omap2/pm34xx.c      |    2 +-
> >> >>  2 files changed, 8 insertions(+), 17 deletions(-)
> >> >> 
> >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> >> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> index c14152f..a636edb 100644
> >> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> @@ -473,31 +473,22 @@ static int omap3_enter_idle_bm(struct 
> >> >> cpuidle_device *dev,
> >> >>  			       struct cpuidle_state *state)
> >> >>  {
> >> >>  	struct cpuidle_state *new_state = NULL;
> >> >> -	int i, j;
> >> >> -
> >> >> -	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && 
> >> >> omap3_idle_bm_check()) {
> >> >> -
> >> >> -		/* Find current state in list */
> >> >> -		for (i = 0; i < OMAP3_MAX_STATES; i++)
> >> >> -			if (state == &dev->states[i])
> >> >> -				break;
> >> >> -		BUG_ON(i == OMAP3_MAX_STATES);
> >> >> -
> >> >> -		/* Back up to non 'CHECK_BM' state */
> >> >> -		for (j = i - 1;  j > 0; j--) {
> >> >> -			struct cpuidle_state *s = 
> &dev->states[j];
> >> >> +	int i;
> >> >>  
> >> >> +	if (omap3_idle_bm_check()) {
> >> >> +		for (i = 0; i < OMAP3_MAX_STATES; i++) {
> >> >> +			struct cpuidle_state *s = 
> &dev->states[i];
> >> >
> >> > So now a C0 is attempted every time there is bus activity?
> >> 
> >> Yes you are right here. This code is not very effective. It should
> >> select deepest sleep state without CPUIDLE_FLAG_CHECK_BM.
> >> 
> >> >
> >> >>  			if (!(s->flags & 
> CPUIDLE_FLAG_CHECK_BM)) {
> >> >>  				new_state = s;
> >> >>  				break;
> >> >>  			}
> >> >>  		}
> >> >> -
> >> >> +		BUG_ON(i == OMAP3_MAX_STATES);
> >> >>  		pr_debug("%s: Bus activity: Entering %s 
> >> >> (instead of %s)\n",
> >> >> -			__FUNCTION__, new_state->name, 
> state->name);
> >> >> +			 __FUNCTION__, new_state->name, 
> state->name);
> >> >>  	}
> >> >>  
> >> >> -	return omap3_enter_idle(dev, new_state ? : state);
> >> >> +	return omap3_enter_idle(dev, new_state ? 
> new_state : state);
> >> >>  }
> >> >>  
> >> >>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> >> >> diff --git a/arch/arm/mach-omap2/pm34xx.c 
> >> >> b/arch/arm/mach-omap2/pm34xx.c
> >> >> index 9f73e5c..ca0600a 100644
> >> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> >> @@ -734,7 +734,7 @@ static int __init pwrdms_setup(struct 
> >> >> powerdomain *pwrdm)
> >> >>  		pwrdm_enable_hdwr_sar(pwrdm);
> >> >>  
> >> >>  	if (!strcmp(pwrst->pwrdm->name, "core_pwrdm") || 
> >> >> !strcmp(pwrst->pwrdm->name, "mpu_pwrdm") ||
> >> >> -	    !strcmp(pwrst->pwrdm->name, "mpu_pwrdm"))
> >> >> +	    !strcmp(pwrst->pwrdm->name, "neon_pwrdm"))
> >> >>  		return set_pwrdm_state(pwrst->pwrdm, 
> PWRDM_POWER_ON);
> >> >>  	else
> >> >>  		return set_pwrdm_state(pwrst->pwrdm, 
> pwrst->next_state);
> >> >> -- 
> >> >> 1.5.5
> >> >> 
> >> >> 
> >> >
> >> >
> >> >
> >> 
> >> -- 
> >> Jouni Högander
> >> 
> >> 
> >
> > --
> > 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
> >
> >
> 
> -- 
> Jouni Högander
> 
> 

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

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Paul Walmsley]

Hello Jouni,

a couple of questions about these patches....

On Tue, 8 Jul 2008, Jouni Hogander wrote:

> With this patch cpuidle boot problem with cpuidle doesn't exist. Also hang on wake-up seems to
> disappear (haven't seen this far).
> 
> Main points in this patch are:
> 1. Add wkdep between neon and mpu
> 2. Add wkdep between per and core

Could you explain a little further why PER would have a wakeup dependency 
on CORE?  Is this something that we should only enable under certain 
conditions, e.g., latency requirements for a device in PER?  

> 3. Deny hwsup mode before writing next pwrst state

I missed this part of the patch - could you point me at that section?


regards,

- Paul

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Högander Jouni]

"ext Paul Walmsley" <paul@booyaka.com> writes:

> Hello Jouni,
>
> a couple of questions about these patches....
>
> On Tue, 8 Jul 2008, Jouni Hogander wrote:
>
>> With this patch cpuidle boot problem with cpuidle doesn't exist. Also hang on wake-up seems to
>> disappear (haven't seen this far).
>> 
>> Main points in this patch are:
>> 1. Add wkdep between neon and mpu
>> 2. Add wkdep between per and core
>
> Could you explain a little further why PER would have a wakeup dependency 
> on CORE?  Is this something that we should only enable under certain 
> conditions, e.g., latency requirements for a device in PER?

This is done to make sure we don't loose any gpio interrupts: GPIO
wakeup/interrupt doesn't work for GPIOs in PER domain if PER is not
active.

>
>> 3. Deny hwsup mode before writing next pwrst state
>
> I missed this part of the patch - could you point me at that
> section?

Patch changes code to use set_pwrdm_state from pm34xx.c instead of
pwrdm_set_next_pwrst from powerdomain code. That function contains
denying hwsup mode.

>
>
> regards,
>
> - Paul
> --
> 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
>
>

-- 
Jouni Högander

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

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1161 bytes --]

Hello Jouni,

On Mon, 11 Aug 2008, Högander Jouni wrote:

> "ext Paul Walmsley" <paul@booyaka.com> writes:
>
> > Could you explain a little further why PER would have a wakeup dependency 
> > on CORE?  Is this something that we should only enable under certain 
> > conditions, e.g., latency requirements for a device in PER?
> 
> This is done to make sure we don't loose any gpio interrupts: GPIO
> wakeup/interrupt doesn't work for GPIOs in PER domain if PER is not
> active.

I'm probably misunderstanding something, but ... wouldn't it better to 
just keep PER powerdomain ON all the time when PER GPIOs are enabled for 
interrupts?  It seems possible for PER to go to retention or OFF even with 
the CORE wkdep in place, which would result in a period of time where the 
interrupts would be missed, no?

> >> 3. Deny hwsup mode before writing next pwrst state
> >
> > I missed this part of the patch - could you point me at that
> > section?
> 
> Patch changes code to use set_pwrdm_state from pm34xx.c instead of
> pwrdm_set_next_pwrst from powerdomain code. That function contains
> denying hwsup mode.

Ah, okay.


- Paul

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Högander Jouni]

"ext Paul Walmsley" <paul@booyaka.com> writes:

> Hello Jouni,
>
> On Mon, 11 Aug 2008, Högander Jouni wrote:
>
>> "ext Paul Walmsley" <paul@booyaka.com> writes:
>>
>> > Could you explain a little further why PER would have a wakeup dependency 
>> > on CORE?  Is this something that we should only enable under certain 
>> > conditions, e.g., latency requirements for a device in PER?
>> 
>> This is done to make sure we don't loose any gpio interrupts: GPIO
>> wakeup/interrupt doesn't work for GPIOs in PER domain if PER is not
>> active.
>
> I'm probably misunderstanding something, but ... wouldn't it better to 
> just keep PER powerdomain ON all the time when PER GPIOs are enabled for 
> interrupts?  It seems possible for PER to go to retention or OFF even with 
> the CORE wkdep in place, which would result in a period of time where the 
> interrupts would be missed, no?

No, it won't, PER goes sleep state only if CORE goes
too. There is a hardware sleepdep between PER and CORE, thanks to
Rajendra for pointing this out some time ago. This way there are all
the time some wakeup mechanism available for PER gpios
(gpio/iopad). Leaving PER ON would increase consumption.

>
>> >> 3. Deny hwsup mode before writing next pwrst state
>> >
>> > I missed this part of the patch - could you point me at that
>> > section?
>> 
>> Patch changes code to use set_pwrdm_state from pm34xx.c instead of
>> pwrdm_set_next_pwrst from powerdomain code. That function contains
>> denying hwsup mode.
>
> Ah, okay.
>
>
> - Paul

-- 
Jouni Högander

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

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1638 bytes --]

Hello Jouni, 

On Tue, 12 Aug 2008, Högander Jouni wrote:

> "ext Paul Walmsley" <paul@booyaka.com> writes:
> 
> > On Mon, 11 Aug 2008, Högander Jouni wrote:
> >
> >> "ext Paul Walmsley" <paul@booyaka.com> writes:
> >>
> >> > Could you explain a little further why PER would have a wakeup dependency 
> >> > on CORE?  Is this something that we should only enable under certain 
> >> > conditions, e.g., latency requirements for a device in PER?
> >> 
> >> This is done to make sure we don't loose any gpio interrupts: GPIO
> >> wakeup/interrupt doesn't work for GPIOs in PER domain if PER is not
> >> active.
> >
> > I'm probably misunderstanding something, but ... wouldn't it better to 
> > just keep PER powerdomain ON all the time when PER GPIOs are enabled for 
> > interrupts?  It seems possible for PER to go to retention or OFF even with 
> > the CORE wkdep in place, which would result in a period of time where the 
> > interrupts would be missed, no?
> 
> No, it won't, PER goes sleep state only if CORE goes
> too. There is a hardware sleepdep between PER and CORE, thanks to
> Rajendra for pointing this out some time ago. This way there are all
> the time some wakeup mechanism available for PER gpios
> (gpio/iopad). Leaving PER ON would increase consumption.

Ah, I see.

Do you think we should add a mechanism for the CORE->PER wkdep to be 
dynamically added and removed, based on whether GPIO2-6 balls are enabled 
in IO pad wakeup?  Conceivably a board could just use GPIO1 (in WKUP), and 
PER would not need to be awakened along with CORE in those instances, 
correct?


- Paul

Re: [RFC] OMAP3: CPUIDLE & PM: Modifications and fixes

[Högander Jouni]

"ext Paul Walmsley" <paul@booyaka.com> writes:

> Hello Jouni, 
>
> On Tue, 12 Aug 2008, Högander Jouni wrote:
>
>> "ext Paul Walmsley" <paul@booyaka.com> writes:
>> 
>> > On Mon, 11 Aug 2008, Högander Jouni wrote:
>> >
>> >> "ext Paul Walmsley" <paul@booyaka.com> writes:
>> >>
>> >> > Could you explain a little further why PER would have a wakeup dependency 
>> >> > on CORE?  Is this something that we should only enable under certain 
>> >> > conditions, e.g., latency requirements for a device in PER?
>> >> 
>> >> This is done to make sure we don't loose any gpio interrupts: GPIO
>> >> wakeup/interrupt doesn't work for GPIOs in PER domain if PER is not
>> >> active.
>> >
>> > I'm probably misunderstanding something, but ... wouldn't it better to 
>> > just keep PER powerdomain ON all the time when PER GPIOs are enabled for 
>> > interrupts?  It seems possible for PER to go to retention or OFF even with 
>> > the CORE wkdep in place, which would result in a period of time where the 
>> > interrupts would be missed, no?
>> 
>> No, it won't, PER goes sleep state only if CORE goes
>> too. There is a hardware sleepdep between PER and CORE, thanks to
>> Rajendra for pointing this out some time ago. This way there are all
>> the time some wakeup mechanism available for PER gpios
>> (gpio/iopad). Leaving PER ON would increase consumption.
>
> Ah, I see.
>
> Do you think we should add a mechanism for the CORE->PER wkdep to be 
> dynamically added and removed, based on whether GPIO2-6 balls are enabled 
> in IO pad wakeup?  Conceivably a board could just use GPIO1 (in WKUP), and 
> PER would not need to be awakened along with CORE in those instances, 
> correct?

If gpio irq is enabled also iopad wakeup is enabled for related pad
and wkdep is added. Same thing should be done also for core gpios,
because not all of them are capable to generate wakeup when core is
off. Problem with this is how to know in kernel side to which ball
each gpio is connected. To my understanding muxing is mostly/commonly
done in bootloader. Any idea to this? There seems to be also
enable/disable_irq_wake, where this logic could be connected.

>
>
> - Paul

-- 
Jouni Högander

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