[PATCH 0/2] OMAP3: cpuidle: prevent CORE pwrdm from entering low-power state when DSS active

[PATCH 0/2] OMAP3: cpuidle: prevent CORE pwrdm from entering low-power state when DSS active

[Paul Walmsley]

This series prevents the OMAP3 CPUIdle code from programming the CORE
powerdomain to enter any power state lower than ON when the DSS power domain
is ON.  (The assumption is that in this case, the DSS is active, DMA'ing
lines from RAM and displaying them somewhere.)

Tero, please don't hesitate to comment on the patch from you if I got the
commit message wrong.


- Paul

---

Tero Kristo (1):
      OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

Paul Walmsley (1):
      OMAP3: cpuidle: add more details to the DSS-related CORE power domain state restriction


 arch/arm/mach-omap2/cpuidle34xx.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Paul Walmsley]

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

Prevent the CORE power domain from entering RETENTION or OFF when DSS
is on.  Otherwise, the display FIFO(s) may underflow due to the time
needed for the CORE to wake back up, causing tearing and unnecessary
interrupts.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
[paul at pwsan.com: wrote commit message]
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 0335cd8..d1b7789 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -9,8 +9,9 @@
  * Copyright (C) 2007 Texas Instruments, Inc.
  * Karthik Dasu <karthik-dp@ti.com>
  *
- * Copyright (C) 2006 Nokia Corporation
+ * Copyright (C) 2006, 2011 Nokia Corporation
  * Tony Lindgren <tony@atomide.com>
+ * Tero Kristo <tero.kristo@nokia.com>
  *
  * Copyright (C) 2005 Texas Instruments, Inc.
  * Richard Woodruff <r-woodruff2@ti.com>
@@ -268,6 +269,12 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 		goto select_state;
 	}
 
+	/* If DSS is active, prevent CORE RET/OFF */
+	dss_state = pwrdm_read_pwrst(dss_pd);
+	if (dss_state == PWRDM_POWER_ON &&
+	    core_next_state != PWRDM_POWER_ON)
+		core_next_state = PWRDM_POWER_INACTIVE;
+
 	/*
 	 * Prevent PER off if CORE is not in retention or off as this
 	 * would disable PER wakeups completely.
@@ -288,7 +295,6 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 		iva2_state = pwrdm_read_pwrst(iva2_pd);
 		sgx_state = pwrdm_read_pwrst(sgx_pd);
 		usb_state = pwrdm_read_pwrst(usb_pd);
-		dss_state = pwrdm_read_pwrst(dss_pd);
 
 		if (cam_state > PWRDM_POWER_OFF ||
 		    dss_state > PWRDM_POWER_OFF ||

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> From: Tero Kristo <tero.kristo@nokia.com>
>
> Prevent the CORE power domain from entering RETENTION or OFF when DSS
> is on.  Otherwise, the display FIFO(s) may underflow due to the time
> needed for the CORE to wake back up, causing tearing and unnecessary
> interrupts.
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> [paul at pwsan.com: wrote commit message]
> Signed-off-by: Paul Walmsley <paul@pwsan.com>

This isn't quite ready for merge, and needs a little more testing with
current DSS driver and mainline.

> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 0335cd8..d1b7789 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -9,8 +9,9 @@
>   * Copyright (C) 2007 Texas Instruments, Inc.
>   * Karthik Dasu <karthik-dp@ti.com>
>   *
> - * Copyright (C) 2006 Nokia Corporation
> + * Copyright (C) 2006, 2011 Nokia Corporation
>   * Tony Lindgren <tony@atomide.com>
> + * Tero Kristo <tero.kristo@nokia.com>
>   *
>   * Copyright (C) 2005 Texas Instruments, Inc.
>   * Richard Woodruff <r-woodruff2@ti.com>
> @@ -268,6 +269,12 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>  		goto select_state;
>  	}
>  
> +	/* If DSS is active, prevent CORE RET/OFF */
> +	dss_state = pwrdm_read_pwrst(dss_pd);
> +	if (dss_state == PWRDM_POWER_ON &&
> +	    core_next_state != PWRDM_POWER_ON)
> +		core_next_state = PWRDM_POWER_INACTIVE;
> +

Due to sleepdeps/autodeps, when this code runs, DSS powerdomain is
always on.  The result is that CORE is always set to INACTIVE.

A side effect of this problem is exposing a known issue with PER wakeups
(UART3, GPIO).  Since CORE never goes to retention/off, IO-pad
wakeups are never enabled, so PER wakeups do not work.

I think we need a different way of checking for DSS activity.

Sounds like another usecase for idle notifiers.

Kevin

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Tero.Kristo at nokia.com]

Hi Kevin,

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman at ti.com]
>Sent: 04 March, 2011 01:25
>To: Paul Walmsley
>Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>Kristo Tero (Nokia-MS/Tampere)
>Subject: Re: [PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from
>going to RET or OFF when DSS is on
>
>Paul Walmsley <paul@pwsan.com> writes:
>
>> From: Tero Kristo <tero.kristo@nokia.com>
>>
>> Prevent the CORE power domain from entering RETENTION or OFF when DSS
>> is on.  Otherwise, the display FIFO(s) may underflow due to the time
>> needed for the CORE to wake back up, causing tearing and unnecessary
>> interrupts.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>> [paul at pwsan.com: wrote commit message]
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>
>This isn't quite ready for merge, and needs a little more testing with
>current DSS driver and mainline.
>
>> ---
>>  arch/arm/mach-omap2/cpuidle34xx.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
>omap2/cpuidle34xx.c
>> index 0335cd8..d1b7789 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -9,8 +9,9 @@
>>   * Copyright (C) 2007 Texas Instruments, Inc.
>>   * Karthik Dasu <karthik-dp@ti.com>
>>   *
>> - * Copyright (C) 2006 Nokia Corporation
>> + * Copyright (C) 2006, 2011 Nokia Corporation
>>   * Tony Lindgren <tony@atomide.com>
>> + * Tero Kristo <tero.kristo@nokia.com>
>>   *
>>   * Copyright (C) 2005 Texas Instruments, Inc.
>>   * Richard Woodruff <r-woodruff2@ti.com>
>> @@ -268,6 +269,12 @@ static int omap3_enter_idle_bm(struct
>cpuidle_device *dev,
>>  		goto select_state;
>>  	}
>>
>> +	/* If DSS is active, prevent CORE RET/OFF */
>> +	dss_state = pwrdm_read_pwrst(dss_pd);
>> +	if (dss_state == PWRDM_POWER_ON &&
>> +	    core_next_state != PWRDM_POWER_ON)
>> +		core_next_state = PWRDM_POWER_INACTIVE;
>> +
>
>Due to sleepdeps/autodeps, when this code runs, DSS powerdomain is
>always on.  The result is that CORE is always set to INACTIVE.

Now I recall that someone was asking about a patch similar to this earlier, and had the same issue with DSS sleepdep collision.

What is the reason for having the sleepdep for DSS powerdomain anyway? At least I can't see any reason why the sleepdep for DSS should be set. In my opinion it should be perfectly okay for DSS domain to idle independently of MPU/CORE, as this is going to be better for power consumption also.


>
>A side effect of this problem is exposing a known issue with PER wakeups
>(UART3, GPIO).  Since CORE never goes to retention/off, IO-pad
>wakeups are never enabled, so PER wakeups do not work.
>
>I think we need a different way of checking for DSS activity.
>
>Sounds like another usecase for idle notifiers.
>
>Kevin

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Kevin Hilman]

Hi Tero,

<Tero.Kristo@nokia.com> writes:

[...]

>>>
>>> +	/* If DSS is active, prevent CORE RET/OFF */
>>> +	dss_state = pwrdm_read_pwrst(dss_pd);
>>> +	if (dss_state == PWRDM_POWER_ON &&
>>> +	    core_next_state != PWRDM_POWER_ON)
>>> +		core_next_state = PWRDM_POWER_INACTIVE;
>>> +
>>
>>Due to sleepdeps/autodeps, when this code runs, DSS powerdomain is
>>always on.  The result is that CORE is always set to INACTIVE.
>
> Now I recall that someone was asking about a patch similar to this
> earlier, and had the same issue with DSS sleepdep collision.

>
> What is the reason for having the sleepdep for DSS powerdomain anyway?
> At least I can't see any reason why the sleepdep for DSS should be
> set. In my opinion it should be perfectly okay for DSS domain to idle
> independently of MPU/CORE, as this is going to be better for power
> consumption also.

Agreed, but currently the sleepdeps with MPU are automatically managed
(by clkdm autodeps and hwmod initiator deps.)  Until we have merged a
solution to more selectively enable sleepdeps (or remove them) $SUBJECT
patch cannot be merged.

Kevin

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Tero.Kristo at nokia.com]

>-----Original Message-----
From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner at vger.kernel.org] On Behalf Of ext Kevin Hilman
>Sent: 04 March, 2011 18:56
>To: Kristo Tero (Nokia-MS/Tampere)
>Cc: paul at pwsan.com; linux-omap at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org
>Subject: Re: [PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from
>going to RET or OFF when DSS is on
>
>Hi Tero,
>
><Tero.Kristo@nokia.com> writes:
>
>[...]
>
>>>>
>>>> +	/* If DSS is active, prevent CORE RET/OFF */
>>>> +	dss_state = pwrdm_read_pwrst(dss_pd);
>>>> +	if (dss_state == PWRDM_POWER_ON &&
>>>> +	    core_next_state != PWRDM_POWER_ON)
>>>> +		core_next_state = PWRDM_POWER_INACTIVE;
>>>> +
>>>
>>>Due to sleepdeps/autodeps, when this code runs, DSS powerdomain is
>>>always on.  The result is that CORE is always set to INACTIVE.
>>
>> Now I recall that someone was asking about a patch similar to this
>> earlier, and had the same issue with DSS sleepdep collision.
>
>>
>> What is the reason for having the sleepdep for DSS powerdomain anyway?
>> At least I can't see any reason why the sleepdep for DSS should be
>> set. In my opinion it should be perfectly okay for DSS domain to idle
>> independently of MPU/CORE, as this is going to be better for power
>> consumption also.
>
>Agreed, but currently the sleepdeps with MPU are automatically managed
>(by clkdm autodeps and hwmod initiator deps.)  Until we have merged a
>solution to more selectively enable sleepdeps (or remove them) $SUBJECT
>patch cannot be merged.

Ok I thought this is the case... it would be possible to implement a temporary/permanent solution that uses idle status check instead of pwrdm state check, and prevent core idle if dss is not going to idle. What is the current status with those idlest patches anyway?

-Tero

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Kevin Hilman]

<Tero.Kristo@nokia.com> writes:

[...]

>>>>>
>>>>> +	/* If DSS is active, prevent CORE RET/OFF */
>>>>> +	dss_state = pwrdm_read_pwrst(dss_pd);
>>>>> +	if (dss_state == PWRDM_POWER_ON &&
>>>>> +	    core_next_state != PWRDM_POWER_ON)
>>>>> +		core_next_state = PWRDM_POWER_INACTIVE;
>>>>> +
>>>>
>>>>Due to sleepdeps/autodeps, when this code runs, DSS powerdomain is
>>>>always on.  The result is that CORE is always set to INACTIVE.
>>>
>>> Now I recall that someone was asking about a patch similar to this
>>> earlier, and had the same issue with DSS sleepdep collision.
>>
>>>
>>> What is the reason for having the sleepdep for DSS powerdomain anyway?
>>> At least I can't see any reason why the sleepdep for DSS should be
>>> set. In my opinion it should be perfectly okay for DSS domain to idle
>>> independently of MPU/CORE, as this is going to be better for power
>>> consumption also.
>>
>>Agreed, but currently the sleepdeps with MPU are automatically managed
>>(by clkdm autodeps and hwmod initiator deps.)  Until we have merged a
>>solution to more selectively enable sleepdeps (or remove them) $SUBJECT
>>patch cannot be merged.
>
> Ok I thought this is the case... it would be possible to implement a
> temporary/permanent solution that uses idle status check instead of
> pwrdm state check, and prevent core idle if dss is not going to
> idle. What is the current status with those idlest patches anyway?

Paul will have to answer that one.

Kevin

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Paul Walmsley]

Hi Tero,

On Mon, 7 Mar 2011, Tero.Kristo at nokia.com wrote:

> What is the current status with those idlest patches anyway?

They aren't completed yet, so they won't make it into 2.6.39.  I hope to 
get them in early in 2.6.40.


- Paul

[PATCH 1/2] OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF when DSS is on

[Paul Walmsley]

Hi,

On Fri, 4 Mar 2011, Tero.Kristo at nokia.com wrote:

> What is the reason for having the sleepdep for DSS powerdomain anyway? 
> At least I can't see any reason why the sleepdep for DSS should be set. 
> In my opinion it should be perfectly okay for DSS domain to idle 
> independently of MPU/CORE, as this is going to be better for power 
> consumption also.

Those autodeps are only there because one of the old CDP trees had them in 
there.  Talking with some people at TI, as I understand it, they are only 
useful for working around hardware bugs.  As far as I know, as long as a 
target module has its main clock and interface clock enabled, the PRCM 
should not assert its SIdleReq line to the module, and so the module 
should never enter idle.  

But no one seems to have an authoritative idea of which autodeps are still 
needed, and which are not. Perhaps you, or someone else, might like to try 
seeing which ones can be safely removed?  After applying this patch here:

   http://marc.info/?l=linux-omap&m=129962861605157&w=2

it should be possible to simply set a CLKDM_NO_AUTODEPS flag on a struct 
clockdomain, and then no autodep will be added.

regards

- Paul

[PATCH 2/2] OMAP3: cpuidle: add more details to the DSS-related CORE power domain state restriction

[Paul Walmsley]

Provide some more details about the rationale behind the CORE power domain
state restriction while DSS is enabled -- hopefully, enough to enable others
to implement an approach based on the DSS's required wakeup latency,
which can vary based on quite a few parameters.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Senthilvadivu Guruswamy <svadivu@ti.com>
Cc: Sumit Semwal <sumit.semwal@ti.com>
Cc: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index d1b7789..6bdbb33 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -269,7 +269,33 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 		goto select_state;
 	}
 
-	/* If DSS is active, prevent CORE RET/OFF */
+	/*
+	 * If DSS is active, prevent any CORE power domain transition
+	 * to RETENTION or lower.  This is because we don't currently
+	 * have the infrastructure in the DSS driver or kernel to
+	 * enter CORE RET without causing DSS FIFO underflows.  When
+	 * the DSS FIFO underflows, the screen image can be distorted,
+	 * because the CORE may not be able to come back on-line
+	 * quickly enough to service DSS requests to the SDRAM
+	 * framebuffer.  The DSS may also interrupt the MPU when the
+	 * FIFO underflows, potentially wasting power.
+	 *
+	 * The approach below is easy to implement, but potentially
+	 * wastes power, especially with screens that don't require a
+	 * backlight.  Probably the best long-term way to fix this is
+	 * for the DSS driver to constrain the CORE and SDRAM minimum
+	 * power state with omap_pm_set_max_dev_wakeup_lat(), based on
+	 * the DSS FIFO size(s), watermark levels, drain rate, and
+	 * refill time.  Based on that constraint, the OMAP PM code
+	 * should adjust the CORE maximum power state, CORE DPLL
+	 * autoidle mode, CORE_CLK rate, and voltage scaling to ensure
+	 * the required wakeup latency at the lowest power
+	 * consumption.
+	 *
+	 * (More information on the DSS low-power refresh mode can be
+	 * found in _Using Display Low-Power Refresh on the OMAP3430
+	 * Device_, TI Application Report, SWPA158 - October 2008.)
+	 */
 	dss_state = pwrdm_read_pwrst(dss_pd);
 	if (dss_state == PWRDM_POWER_ON &&
 	    core_next_state != PWRDM_POWER_ON)

[PATCH 0/2] OMAP3: cpuidle: prevent CORE pwrdm from entering low-power state when DSS active

[Tero.Kristo at nokia.com]

>-----Original Message-----
>From: ext Paul Walmsley [mailto:paul at pwsan.com]
>Sent: 02 March, 2011 00:49
>To: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>Kristo Tero (Nokia-MS/Tampere)
>Subject: [PATCH 0/2] OMAP3: cpuidle: prevent CORE pwrdm from entering
>low-power state when DSS active
>
>This series prevents the OMAP3 CPUIdle code from programming the CORE
>powerdomain to enter any power state lower than ON when the DSS power
>domain
>is ON.  (The assumption is that in this case, the DSS is active, DMA'ing
>lines from RAM and displaying them somewhere.)
>
>Tero, please don't hesitate to comment on the patch from you if I got
>the
>commit message wrong.

Hi Paul,

It looks good to me.


>
>
>- Paul
>
>---
>
>Tero Kristo (1):
>      OMAP3: cpuidle: prevent CORE power domain from going to RET or OFF
>when DSS is on
>
>Paul Walmsley (1):
>      OMAP3: cpuidle: add more details to the DSS-related CORE power
>domain state restriction
>
>
> arch/arm/mach-omap2/cpuidle34xx.c |   36
>++++++++++++++++++++++++++++++++++--
> 1 files changed, 34 insertions(+), 2 deletions(-)