[PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Tero Kristo]

On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many other
parts of the chip will be reset.  If those parts of the chip are busy,
the reset will disrupt them, causing unpredictable and generally
undesirable results.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   40 +++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index f3e043f..d31b858 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -61,7 +61,7 @@ struct omap3_processor_cx {
 struct omap3_processor_cx omap3_power_states[OMAP3_MAX_STATES];
 struct omap3_processor_cx current_cx_state;
 struct powerdomain *mpu_pd, *core_pd, *per_pd;
-struct powerdomain *cam_pd;
+struct powerdomain *cam_pd, *dss_pd, *iva2_pd, *sgx_pd, *usb_pd;
 
 /*
  * The latencies/thresholds for various C states have
@@ -235,7 +235,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 {
 	struct cpuidle_state *new_state = next_valid_state(dev, state);
 	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
-	u32 cam_state;
+	u32 cam_state, dss_state, iva2_state, sgx_state, usb_state;
 	struct omap3_processor_cx *cx;
 	int ret;
 
@@ -256,6 +256,8 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 	 *        its own code.
 	 */
 
+	/* XXX Add CORE-active check here */
+
 	/*
 	 * Prevent idle completely if CAM is active.
 	 * CAM does not have wakeup capability in OMAP3.
@@ -275,6 +277,36 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 	    (core_next_state > PWRDM_POWER_RET))
 		per_next_state = PWRDM_POWER_RET;
 
+	/* XXX Add prevent-PER-off check here */
+
+	/*
+	 * If we are attempting CORE off, check if any other powerdomains
+	 * are at retention or higher. CORE off causes chipwide reset which
+	 * would reset these domains also.
+	 */
+	if (core_next_state == PWRDM_POWER_OFF) {
+		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 ||
+		    iva2_state > PWRDM_POWER_OFF ||
+		    per_next_state > PWRDM_POWER_OFF ||
+		    sgx_state > PWRDM_POWER_OFF ||
+		    usb_state > PWRDM_POWER_OFF)
+			core_next_state = PWRDM_POWER_RET;
+	}
+
+	/* Fallback to new target core/mpu state */
+	while (cx->core_state < core_next_state) {
+		state--;
+		cx = cpuidle_get_statedata(state);
+	}
+
+	new_state = state;
+
 	/* Are we changing PER target state? */
 	if (per_next_state != per_saved_state)
 		pwrdm_set_next_pwrst(per_pd, per_next_state);
@@ -489,6 +521,10 @@ int __init omap3_idle_init(void)
 	core_pd = pwrdm_lookup("core_pwrdm");
 	per_pd = pwrdm_lookup("per_pwrdm");
 	cam_pd = pwrdm_lookup("cam_pwrdm");
+	dss_pd = pwrdm_lookup("dss_pwrdm");
+	iva2_pd = pwrdm_lookup("iva2_pwrdm");
+	sgx_pd = pwrdm_lookup("sgx_pwrdm");
+	usb_pd = pwrdm_lookup("usbhost_pwrdm");
 
 	omap_init_power_states();
 	cpuidle_register_driver(&omap3_idle_driver);
-- 
1.7.1

Re: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Paul Walmsley]

On Tue, 18 Jan 2011, Tero Kristo wrote:

> On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many other
> parts of the chip will be reset.  If those parts of the chip are busy,
> the reset will disrupt them, causing unpredictable and generally
> undesirable results.
> 
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> Cc: Paul Walmsley <paul@pwsan.com>

Thanks Tero.

Reviewed-by: Paul Walmsley <paul@pwsan.com>

Kevin, do you want to take this one?


- Paul

RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Vishwanath Sripathy]

Tero,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Tero Kristo
> Sent: Tuesday, January 18, 2011 3:18 PM
> To: linux-omap@vger.kernel.org
> Cc: Paul Walmsley; Kevin Hilman
> Subject: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing
> so would reset an active clockdomain
>
> On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many other
> parts of the chip will be reset.  If those parts of the chip are busy,
> the reset will disrupt them, causing unpredictable and generally
> undesirable results.
If some parts of the chip are busy, then how can Core domain enter off
state? The necessary condition for Core to enter low power state is that
all the clock domains (including DSS, CAM, IVA, USB, PER etc) should have
idled. Doesn't it mean that all the modules have idled and asserted
idleack when Core is entering off state?

Vishwa
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   40
> +++++++++++++++++++++++++++++++++++-
>  1 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
> omap2/cpuidle34xx.c
> index f3e043f..d31b858 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -61,7 +61,7 @@ struct omap3_processor_cx {
>  struct omap3_processor_cx
> omap3_power_states[OMAP3_MAX_STATES];
>  struct omap3_processor_cx current_cx_state;
>  struct powerdomain *mpu_pd, *core_pd, *per_pd;
> -struct powerdomain *cam_pd;
> +struct powerdomain *cam_pd, *dss_pd, *iva2_pd, *sgx_pd, *usb_pd;
>
>  /*
>   * The latencies/thresholds for various C states have
> @@ -235,7 +235,7 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
>  {
>  	struct cpuidle_state *new_state = next_valid_state(dev, state);
>  	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
> -	u32 cam_state;
> +	u32 cam_state, dss_state, iva2_state, sgx_state, usb_state;
>  	struct omap3_processor_cx *cx;
>  	int ret;
>
> @@ -256,6 +256,8 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
>  	 *        its own code.
>  	 */
>
> +	/* XXX Add CORE-active check here */
> +
>  	/*
>  	 * Prevent idle completely if CAM is active.
>  	 * CAM does not have wakeup capability in OMAP3.
> @@ -275,6 +277,36 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
>  	    (core_next_state > PWRDM_POWER_RET))
>  		per_next_state = PWRDM_POWER_RET;
>
> +	/* XXX Add prevent-PER-off check here */
> +
> +	/*
> +	 * If we are attempting CORE off, check if any other
> powerdomains
> +	 * are at retention or higher. CORE off causes chipwide reset
> which
> +	 * would reset these domains also.
> +	 */
> +	if (core_next_state == PWRDM_POWER_OFF) {
> +		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 ||
> +		    iva2_state > PWRDM_POWER_OFF ||
> +		    per_next_state > PWRDM_POWER_OFF ||
> +		    sgx_state > PWRDM_POWER_OFF ||
> +		    usb_state > PWRDM_POWER_OFF)
> +			core_next_state = PWRDM_POWER_RET;
> +	}
> +
> +	/* Fallback to new target core/mpu state */
> +	while (cx->core_state < core_next_state) {
> +		state--;
> +		cx = cpuidle_get_statedata(state);
> +	}
> +
> +	new_state = state;
> +
>  	/* Are we changing PER target state? */
>  	if (per_next_state != per_saved_state)
>  		pwrdm_set_next_pwrst(per_pd, per_next_state);
> @@ -489,6 +521,10 @@ int __init omap3_idle_init(void)
>  	core_pd = pwrdm_lookup("core_pwrdm");
>  	per_pd = pwrdm_lookup("per_pwrdm");
>  	cam_pd = pwrdm_lookup("cam_pwrdm");
> +	dss_pd = pwrdm_lookup("dss_pwrdm");
> +	iva2_pd = pwrdm_lookup("iva2_pwrdm");
> +	sgx_pd = pwrdm_lookup("sgx_pwrdm");
> +	usb_pd = pwrdm_lookup("usbhost_pwrdm");
>
>  	omap_init_power_states();
>  	cpuidle_register_driver(&omap3_idle_driver);
> --
> 1.7.1
>
> --
> 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: prevent CORE from going off if doing so would reset an active clockdomain

[Vishwanath Sripathy]

Tero,

> -----Original Message-----
> From: Vishwanath Sripathy [mailto:vishwanath.bs@ti.com]
> Sent: Wednesday, January 19, 2011 10:09 AM
> To: Tero Kristo; linux-omap@vger.kernel.org
> Cc: Paul Walmsley; Kevin Hilman
> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> doing so would reset an active clockdomain
>
> Tero,
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Tero Kristo
> > Sent: Tuesday, January 18, 2011 3:18 PM
> > To: linux-omap@vger.kernel.org
> > Cc: Paul Walmsley; Kevin Hilman
> > Subject: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> doing
> > so would reset an active clockdomain
> >
> > On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many
> other
> > parts of the chip will be reset.  If those parts of the chip are busy,
> > the reset will disrupt them, causing unpredictable and generally
> > undesirable results.
> If some parts of the chip are busy, then how can Core domain enter off
> state? The necessary condition for Core to enter low power state is that
> all the clock domains (including DSS, CAM, IVA, USB, PER etc) should
> have
> idled. Doesn't it mean that all the modules have idled and asserted
> idleack when Core is entering off state?
Besides these, Core off should reset the modules which are only in Core
domain. It should not impact other power domains. Also Core domain modules
which are reset will restore their context when Core comes out of off
mode. So why are you saying that "If those parts of the chip are busy,
the reset will disrupt them, causing unpredictable and generally
undesirable results."?

Vishwa
>
> Vishwa
> >
> > Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> > Cc: Kevin Hilman <khilman@deeprootsystems.com>
> > ---
> >  arch/arm/mach-omap2/cpuidle34xx.c |   40
> > +++++++++++++++++++++++++++++++++++-
> >  1 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
> > omap2/cpuidle34xx.c
> > index f3e043f..d31b858 100644
> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> > @@ -61,7 +61,7 @@ struct omap3_processor_cx {
> >  struct omap3_processor_cx
> > omap3_power_states[OMAP3_MAX_STATES];
> >  struct omap3_processor_cx current_cx_state;
> >  struct powerdomain *mpu_pd, *core_pd, *per_pd;
> > -struct powerdomain *cam_pd;
> > +struct powerdomain *cam_pd, *dss_pd, *iva2_pd, *sgx_pd,
> *usb_pd;
> >
> >  /*
> >   * The latencies/thresholds for various C states have
> > @@ -235,7 +235,7 @@ static int omap3_enter_idle_bm(struct
> > cpuidle_device *dev,
> >  {
> >  	struct cpuidle_state *new_state = next_valid_state(dev, state);
> >  	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
> > -	u32 cam_state;
> > +	u32 cam_state, dss_state, iva2_state, sgx_state, usb_state;
> >  	struct omap3_processor_cx *cx;
> >  	int ret;
> >
> > @@ -256,6 +256,8 @@ static int omap3_enter_idle_bm(struct
> > cpuidle_device *dev,
> >  	 *        its own code.
> >  	 */
> >
> > +	/* XXX Add CORE-active check here */
> > +
> >  	/*
> >  	 * Prevent idle completely if CAM is active.
> >  	 * CAM does not have wakeup capability in OMAP3.
> > @@ -275,6 +277,36 @@ static int omap3_enter_idle_bm(struct
> > cpuidle_device *dev,
> >  	    (core_next_state > PWRDM_POWER_RET))
> >  		per_next_state = PWRDM_POWER_RET;
> >
> > +	/* XXX Add prevent-PER-off check here */
> > +
> > +	/*
> > +	 * If we are attempting CORE off, check if any other
> > powerdomains
> > +	 * are at retention or higher. CORE off causes chipwide reset
> > which
> > +	 * would reset these domains also.
> > +	 */
> > +	if (core_next_state == PWRDM_POWER_OFF) {
> > +		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 ||
> > +		    iva2_state > PWRDM_POWER_OFF ||
> > +		    per_next_state > PWRDM_POWER_OFF ||
> > +		    sgx_state > PWRDM_POWER_OFF ||
> > +		    usb_state > PWRDM_POWER_OFF)
> > +			core_next_state = PWRDM_POWER_RET;
> > +	}
> > +
> > +	/* Fallback to new target core/mpu state */
> > +	while (cx->core_state < core_next_state) {
> > +		state--;
> > +		cx = cpuidle_get_statedata(state);
> > +	}
> > +
> > +	new_state = state;
> > +
> >  	/* Are we changing PER target state? */
> >  	if (per_next_state != per_saved_state)
> >  		pwrdm_set_next_pwrst(per_pd, per_next_state);
> > @@ -489,6 +521,10 @@ int __init omap3_idle_init(void)
> >  	core_pd = pwrdm_lookup("core_pwrdm");
> >  	per_pd = pwrdm_lookup("per_pwrdm");
> >  	cam_pd = pwrdm_lookup("cam_pwrdm");
> > +	dss_pd = pwrdm_lookup("dss_pwrdm");
> > +	iva2_pd = pwrdm_lookup("iva2_pwrdm");
> > +	sgx_pd = pwrdm_lookup("sgx_pwrdm");
> > +	usb_pd = pwrdm_lookup("usbhost_pwrdm");
> >
> >  	omap_init_power_states();
> >  	cpuidle_register_driver(&omap3_idle_driver);
> > --
> > 1.7.1
> >
> > --
> > 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: prevent CORE from going off if doing so would reset an active clockdomain

[Tero.Kristo]

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner@vger.kernel.org] On Behalf Of ext Vishwanath Sripathy
>Sent: 19 January, 2011 06:39
>To: Kristo Tero (Nokia-MS/Tampere); linux-omap@vger.kernel.org
>Cc: Paul Walmsley; Kevin Hilman
>Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>doing so would reset an active clockdomain
>
>Tero,
>
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Tero Kristo
>> Sent: Tuesday, January 18, 2011 3:18 PM
>> To: linux-omap@vger.kernel.org
>> Cc: Paul Walmsley; Kevin Hilman
>> Subject: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing
>> so would reset an active clockdomain
>>
>> On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many other
>> parts of the chip will be reset.  If those parts of the chip are busy,
>> the reset will disrupt them, causing unpredictable and generally
>> undesirable results.
>If some parts of the chip are busy, then how can Core domain enter off
>state? The necessary condition for Core to enter low power state is that
>all the clock domains (including DSS, CAM, IVA, USB, PER etc) should
>have
>idled. Doesn't it mean that all the modules have idled and asserted
>idleack when Core is entering off state?

This can happen e.g. when some powerdomain has entered RET state. We have faced this issue at least with IVA2 because it has its own power management.

>
>Vishwa
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>>  arch/arm/mach-omap2/cpuidle34xx.c |   40
>> +++++++++++++++++++++++++++++++++++-
>>  1 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
>> omap2/cpuidle34xx.c
>> index f3e043f..d31b858 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -61,7 +61,7 @@ struct omap3_processor_cx {
>>  struct omap3_processor_cx
>> omap3_power_states[OMAP3_MAX_STATES];
>>  struct omap3_processor_cx current_cx_state;
>>  struct powerdomain *mpu_pd, *core_pd, *per_pd;
>> -struct powerdomain *cam_pd;
>> +struct powerdomain *cam_pd, *dss_pd, *iva2_pd, *sgx_pd, *usb_pd;
>>
>>  /*
>>   * The latencies/thresholds for various C states have
>> @@ -235,7 +235,7 @@ static int omap3_enter_idle_bm(struct
>> cpuidle_device *dev,
>>  {
>>  	struct cpuidle_state *new_state = next_valid_state(dev, state);
>>  	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
>> -	u32 cam_state;
>> +	u32 cam_state, dss_state, iva2_state, sgx_state, usb_state;
>>  	struct omap3_processor_cx *cx;
>>  	int ret;
>>
>> @@ -256,6 +256,8 @@ static int omap3_enter_idle_bm(struct
>> cpuidle_device *dev,
>>  	 *        its own code.
>>  	 */
>>
>> +	/* XXX Add CORE-active check here */
>> +
>>  	/*
>>  	 * Prevent idle completely if CAM is active.
>>  	 * CAM does not have wakeup capability in OMAP3.
>> @@ -275,6 +277,36 @@ static int omap3_enter_idle_bm(struct
>> cpuidle_device *dev,
>>  	    (core_next_state > PWRDM_POWER_RET))
>>  		per_next_state = PWRDM_POWER_RET;
>>
>> +	/* XXX Add prevent-PER-off check here */
>> +
>> +	/*
>> +	 * If we are attempting CORE off, check if any other
>> powerdomains
>> +	 * are at retention or higher. CORE off causes chipwide reset
>> which
>> +	 * would reset these domains also.
>> +	 */
>> +	if (core_next_state == PWRDM_POWER_OFF) {
>> +		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 ||
>> +		    iva2_state > PWRDM_POWER_OFF ||
>> +		    per_next_state > PWRDM_POWER_OFF ||
>> +		    sgx_state > PWRDM_POWER_OFF ||
>> +		    usb_state > PWRDM_POWER_OFF)
>> +			core_next_state = PWRDM_POWER_RET;
>> +	}
>> +
>> +	/* Fallback to new target core/mpu state */
>> +	while (cx->core_state < core_next_state) {
>> +		state--;
>> +		cx = cpuidle_get_statedata(state);
>> +	}
>> +
>> +	new_state = state;
>> +
>>  	/* Are we changing PER target state? */
>>  	if (per_next_state != per_saved_state)
>>  		pwrdm_set_next_pwrst(per_pd, per_next_state);
>> @@ -489,6 +521,10 @@ int __init omap3_idle_init(void)
>>  	core_pd = pwrdm_lookup("core_pwrdm");
>>  	per_pd = pwrdm_lookup("per_pwrdm");
>>  	cam_pd = pwrdm_lookup("cam_pwrdm");
>> +	dss_pd = pwrdm_lookup("dss_pwrdm");
>> +	iva2_pd = pwrdm_lookup("iva2_pwrdm");
>> +	sgx_pd = pwrdm_lookup("sgx_pwrdm");
>> +	usb_pd = pwrdm_lookup("usbhost_pwrdm");
>>
>>  	omap_init_power_states();
>>  	cpuidle_register_driver(&omap3_idle_driver);
>> --
>> 1.7.1
>>
>> --
>> 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
>--
>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: prevent CORE from going off if doing so would reset an active clockdomain

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Tero.Kristo@nokia.com
> Sent: Wednesday, January 19, 2011 1:52 PM
> To: vishwanath.bs@ti.com; linux-omap@vger.kernel.org
> Cc: paul@pwsan.com; khilman@deeprootsystems.com
> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> doing so would reset an active clockdomain
>
>
>
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >owner@vger.kernel.org] On Behalf Of ext Vishwanath Sripathy
> >Sent: 19 January, 2011 06:39
> >To: Kristo Tero (Nokia-MS/Tampere); linux-omap@vger.kernel.org
> >Cc: Paul Walmsley; Kevin Hilman
> >Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> >doing so would reset an active clockdomain
> >
> >Tero,
> >
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] On Behalf Of Tero Kristo
> >> Sent: Tuesday, January 18, 2011 3:18 PM
> >> To: linux-omap@vger.kernel.org
> >> Cc: Paul Walmsley; Kevin Hilman
> >> Subject: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> doing
> >> so would reset an active clockdomain
> >>
> >> On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many
> other
> >> parts of the chip will be reset.  If those parts of the chip are
> busy,
> >> the reset will disrupt them, causing unpredictable and generally
> >> undesirable results.
> >If some parts of the chip are busy, then how can Core domain enter
> off
> >state? The necessary condition for Core to enter low power state is
> that
> >all the clock domains (including DSS, CAM, IVA, USB, PER etc)
> should
> >have
> >idled. Doesn't it mean that all the modules have idled and asserted
> >idleack when Core is entering off state?
>
> This can happen e.g. when some powerdomain has entered RET state. We
> have faced this issue at least with IVA2 because it has its own
> power management.
>
IVA2 PD managed directly instead of linux side PM frameworks is one
problem. If the IVA2 hits retention, and if there is no dependency
on core PD, it should be still fine for core to enter OFF. I hope
it's not the case where some modules from CORE PD are used by IVA
side software ( may be SDMA, GPIO etc) and as part of CORE OFF
you loose context of these which leads to the issue.

Regards,
Santosh

RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Tero.Kristo]

>-----Original Message-----
>From: ext Vishwanath Sripathy [mailto:vishwanath.bs@ti.com]
>Sent: 19 January, 2011 08:06
>To: Kristo Tero (Nokia-MS/Tampere); linux-omap@vger.kernel.org
>Cc: Paul Walmsley; Kevin Hilman
>Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>doing so would reset an active clockdomain
>
>Tero,
>
>> -----Original Message-----
>> From: Vishwanath Sripathy [mailto:vishwanath.bs@ti.com]
>> Sent: Wednesday, January 19, 2011 10:09 AM
>> To: Tero Kristo; linux-omap@vger.kernel.org
>> Cc: Paul Walmsley; Kevin Hilman
>> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>> doing so would reset an active clockdomain
>>
>> Tero,
>>
>> > -----Original Message-----
>> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> > owner@vger.kernel.org] On Behalf Of Tero Kristo
>> > Sent: Tuesday, January 18, 2011 3:18 PM
>> > To: linux-omap@vger.kernel.org
>> > Cc: Paul Walmsley; Kevin Hilman
>> > Subject: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>> doing
>> > so would reset an active clockdomain
>> >
>> > On OMAP3 SoCs, if the CORE powerdomain enters off-mode, many
>> other
>> > parts of the chip will be reset.  If those parts of the chip are
>busy,
>> > the reset will disrupt them, causing unpredictable and generally
>> > undesirable results.
>> If some parts of the chip are busy, then how can Core domain enter off
>> state? The necessary condition for Core to enter low power state is
>that
>> all the clock domains (including DSS, CAM, IVA, USB, PER etc) should
>> have
>> idled. Doesn't it mean that all the modules have idled and asserted
>> idleack when Core is entering off state?
>Besides these, Core off should reset the modules which are only in Core
>domain. It should not impact other power domains. Also Core domain
>modules
>which are reset will restore their context when Core comes out of off
>mode. So why are you saying that "If those parts of the chip are busy,
>the reset will disrupt them, causing unpredictable and generally
>undesirable results."?

Core off issues reset to peripheral domains when it wakes up, this is somehow (badly) visible in TRM (look for COREDOMAINWKUP_RST.) When this reset happens, the peripheral domain shows its reset status as being high, but the powerdomain itself has not entered off (previous state can be e.g. RET), thus its context will not be restored.

Also, another justification for this patch is to prevent unnecessary core-off when the device can't really idle at all => prevents unnecessary overhead inside omap_sram_idle path.

>
>Vishwa
>>
>> Vishwa
>> >
>> > Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>> > Cc: Paul Walmsley <paul@pwsan.com>
>> > Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> > ---
>> >  arch/arm/mach-omap2/cpuidle34xx.c |   40
>> > +++++++++++++++++++++++++++++++++++-
>> >  1 files changed, 38 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
>> > omap2/cpuidle34xx.c
>> > index f3e043f..d31b858 100644
>> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> > @@ -61,7 +61,7 @@ struct omap3_processor_cx {
>> >  struct omap3_processor_cx
>> > omap3_power_states[OMAP3_MAX_STATES];
>> >  struct omap3_processor_cx current_cx_state;
>> >  struct powerdomain *mpu_pd, *core_pd, *per_pd;
>> > -struct powerdomain *cam_pd;
>> > +struct powerdomain *cam_pd, *dss_pd, *iva2_pd, *sgx_pd,
>> *usb_pd;
>> >
>> >  /*
>> >   * The latencies/thresholds for various C states have
>> > @@ -235,7 +235,7 @@ static int omap3_enter_idle_bm(struct
>> > cpuidle_device *dev,
>> >  {
>> >  	struct cpuidle_state *new_state = next_valid_state(dev, state);
>> >  	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
>> > -	u32 cam_state;
>> > +	u32 cam_state, dss_state, iva2_state, sgx_state, usb_state;
>> >  	struct omap3_processor_cx *cx;
>> >  	int ret;
>> >
>> > @@ -256,6 +256,8 @@ static int omap3_enter_idle_bm(struct
>> > cpuidle_device *dev,
>> >  	 *        its own code.
>> >  	 */
>> >
>> > +	/* XXX Add CORE-active check here */
>> > +
>> >  	/*
>> >  	 * Prevent idle completely if CAM is active.
>> >  	 * CAM does not have wakeup capability in OMAP3.
>> > @@ -275,6 +277,36 @@ static int omap3_enter_idle_bm(struct
>> > cpuidle_device *dev,
>> >  	    (core_next_state > PWRDM_POWER_RET))
>> >  		per_next_state = PWRDM_POWER_RET;
>> >
>> > +	/* XXX Add prevent-PER-off check here */
>> > +
>> > +	/*
>> > +	 * If we are attempting CORE off, check if any other
>> > powerdomains
>> > +	 * are at retention or higher. CORE off causes chipwide reset
>> > which
>> > +	 * would reset these domains also.
>> > +	 */
>> > +	if (core_next_state == PWRDM_POWER_OFF) {
>> > +		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 ||
>> > +		    iva2_state > PWRDM_POWER_OFF ||
>> > +		    per_next_state > PWRDM_POWER_OFF ||
>> > +		    sgx_state > PWRDM_POWER_OFF ||
>> > +		    usb_state > PWRDM_POWER_OFF)
>> > +			core_next_state = PWRDM_POWER_RET;
>> > +	}
>> > +
>> > +	/* Fallback to new target core/mpu state */
>> > +	while (cx->core_state < core_next_state) {
>> > +		state--;
>> > +		cx = cpuidle_get_statedata(state);
>> > +	}
>> > +
>> > +	new_state = state;
>> > +
>> >  	/* Are we changing PER target state? */
>> >  	if (per_next_state != per_saved_state)
>> >  		pwrdm_set_next_pwrst(per_pd, per_next_state);
>> > @@ -489,6 +521,10 @@ int __init omap3_idle_init(void)
>> >  	core_pd = pwrdm_lookup("core_pwrdm");
>> >  	per_pd = pwrdm_lookup("per_pwrdm");
>> >  	cam_pd = pwrdm_lookup("cam_pwrdm");
>> > +	dss_pd = pwrdm_lookup("dss_pwrdm");
>> > +	iva2_pd = pwrdm_lookup("iva2_pwrdm");
>> > +	sgx_pd = pwrdm_lookup("sgx_pwrdm");
>> > +	usb_pd = pwrdm_lookup("usbhost_pwrdm");
>> >
>> >  	omap_init_power_states();
>> >  	cpuidle_register_driver(&omap3_idle_driver);
>> > --
>> > 1.7.1
>> >
>> > --
>> > 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: prevent CORE from going off if doing so would reset an active clockdomain

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Tero.Kristo@nokia.com
> Sent: Wednesday, January 19, 2011 2:09 PM
> To: vishwanath.bs@ti.com; linux-omap@vger.kernel.org
> Cc: paul@pwsan.com; khilman@deeprootsystems.com
> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
> doing so would reset an active clockdomain
>
[...]

> >> If some parts of the chip are busy, then how can Core domain
> enter off
> >> state? The necessary condition for Core to enter low power state
> is
> >that
> >> all the clock domains (including DSS, CAM, IVA, USB, PER etc)
> should
> >> have
> >> idled. Doesn't it mean that all the modules have idled and
> asserted
> >> idleack when Core is entering off state?
> >Besides these, Core off should reset the modules which are only in
> Core
> >domain. It should not impact other power domains. Also Core domain
> >modules
> >which are reset will restore their context when Core comes out of
> off
> >mode. So why are you saying that "If those parts of the chip are
> busy,
> >the reset will disrupt them, causing unpredictable and generally
> >undesirable results."?
>
> Core off issues reset to peripheral domains when it wakes up, this
> is somehow (badly) visible in TRM (look for COREDOMAINWKUP_RST.)
> When this reset happens, the peripheral domain shows its reset
> status as being high, but the powerdomain itself has not entered off
> (previous state can be e.g. RET), thus its context will not be
> restored.
>
Now its clear. Reseting other independent clockdomains is
certainly bad from CORE PD OFF to ON behavior .

Please add this additional information to change log.

Regards,
Santosh

RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Paul Walmsley]

On Wed, 19 Jan 2011, Vishwanath Sripathy wrote:

> If some parts of the chip are busy, then how can Core domain enter off
> state? The necessary condition for Core to enter low power state is that
> all the clock domains (including DSS, CAM, IVA, USB, PER etc) should have
> idled. Doesn't it mean that all the modules have idled and asserted
> idleack when Core is entering off state?

Some modules can operate autonomously from the rest of the system.  For 
example, McBSP or DSS.  They can indicate that they are idle or in standby 
while their FIFO(s) are draining or filling.  At this point, the CORE 
clockdomains can potentially go inactive, even though the module is still 
operating autonomously.

Once the module's FIFO watermark has been reached, the modules can 
deassert their SIdleAck or deassert MStandby and wake the system back up.


- Paul

Re: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Cousson, Benoit]

On 1/19/2011 3:03 AM, Shilimkar, Santosh wrote:
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Tero.Kristo@nokia.com
>> Sent: Wednesday, January 19, 2011 2:09 PM
>> To: vishwanath.bs@ti.com; linux-omap@vger.kernel.org
>> Cc: paul@pwsan.com; khilman@deeprootsystems.com
>> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>> doing so would reset an active clockdomain
>>
> [...]
>
>>>> If some parts of the chip are busy, then how can Core domain
>> enter off
>>>> state? The necessary condition for Core to enter low power state
>> is
>>> that
>>>> all the clock domains (including DSS, CAM, IVA, USB, PER etc)
>> should
>>>> have
>>>> idled. Doesn't it mean that all the modules have idled and
>> asserted
>>>> idleack when Core is entering off state?
>>> Besides these, Core off should reset the modules which are only in
>> Core
>>> domain. It should not impact other power domains. Also Core domain
>>> modules
>>> which are reset will restore their context when Core comes out of
>> off
>>> mode. So why are you saying that "If those parts of the chip are
>> busy,
>>> the reset will disrupt them, causing unpredictable and generally
>>> undesirable results."?
>>
>> Core off issues reset to peripheral domains when it wakes up, this
>> is somehow (badly) visible in TRM (look for COREDOMAINWKUP_RST.)
>> When this reset happens, the peripheral domain shows its reset
>> status as being high, but the powerdomain itself has not entered off
>> (previous state can be e.g. RET), thus its context will not be
>> restored.

That's for that reason that CORE OFF with any other power domain active 
is not a supported configuration from the system point of view.
And for that very same reason it was removed on OMAP4 to avoid the OMAP3 
confusion. Only CORE OSWR is supported on OMAP4.
CORE OFF should be set only if device OFF is targeted, all the other 
combinations are not valid.
Wakeup from CORE off will force a reset in order to ensure that the MPU 
and thus the ROM code is executed in order to deal with firewall config.

Regards,
Benoit

RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if doing so would reset an active clockdomain

[Tero.Kristo]

>-----Original Message-----
>From: ext Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
>Sent: 19 January, 2011 11:03
>To: Kristo Tero (Nokia-MS/Tampere); Vishwanath Sripathy; linux-
>omap@vger.kernel.org
>Cc: paul@pwsan.com; khilman@deeprootsystems.com
>Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>doing so would reset an active clockdomain
>
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Tero.Kristo@nokia.com
>> Sent: Wednesday, January 19, 2011 2:09 PM
>> To: vishwanath.bs@ti.com; linux-omap@vger.kernel.org
>> Cc: paul@pwsan.com; khilman@deeprootsystems.com
>> Subject: RE: [PATCH] OMAP3: CPUIdle: prevent CORE from going off if
>> doing so would reset an active clockdomain
>>
>[...]
>
>> >> If some parts of the chip are busy, then how can Core domain
>> enter off
>> >> state? The necessary condition for Core to enter low power state
>> is
>> >that
>> >> all the clock domains (including DSS, CAM, IVA, USB, PER etc)
>> should
>> >> have
>> >> idled. Doesn't it mean that all the modules have idled and
>> asserted
>> >> idleack when Core is entering off state?
>> >Besides these, Core off should reset the modules which are only in
>> Core
>> >domain. It should not impact other power domains. Also Core domain
>> >modules
>> >which are reset will restore their context when Core comes out of
>> off
>> >mode. So why are you saying that "If those parts of the chip are
>> busy,
>> >the reset will disrupt them, causing unpredictable and generally
>> >undesirable results."?
>>
>> Core off issues reset to peripheral domains when it wakes up, this
>> is somehow (badly) visible in TRM (look for COREDOMAINWKUP_RST.)
>> When this reset happens, the peripheral domain shows its reset
>> status as being high, but the powerdomain itself has not entered off
>> (previous state can be e.g. RET), thus its context will not be
>> restored.
>>
>Now its clear. Reseting other independent clockdomains is
>certainly bad from CORE PD OFF to ON behavior .
>
>Please add this additional information to change log.

I'll update the patch desc and resend it in a bit.

-Tero