[PATCHv2] PM : cpuidle - Update statistics for correct state

[PATCHv2] PM : cpuidle - Update statistics for correct state

[Sanjeev Premi]

When 'enable_off_mode' is 0, and (mpu_state < PWRDM_POWER_RET)
the local variables mpu_state and core_state are modified; but
the usage count for the original state selected by the governor
are updated.

This patch updates the 'last_state' in the cpuidle driver to ensure
that statistics for the correct state are updated.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 62fbb2e..b138abd 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -76,23 +76,32 @@ 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;
-	u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
-
-	current_cx_state = *cx;
+	u32 mpu_state, core_state;
 
 	/* Used to keep track of the total time in idle */
 	getnstimeofday(&ts_preidle);
 
-	local_irq_disable();
-	local_fiq_disable();
-
+	/*
+	 * Adjust the idle state (if required).
+	 * Also, ensure that usage statistics of correct state are updated.
+	 */
 	if (!enable_off_mode) {
-		if (mpu_state < PWRDM_POWER_RET)
-			mpu_state = PWRDM_POWER_RET;
-		if (core_state < PWRDM_POWER_RET)
-			core_state = PWRDM_POWER_RET;
+		if (cx->type > OMAP3_STATE_C4) {
+			state = &(dev->states[OMAP3_STATE_C4 - 1]);
+			dev->last_state = state ;
+
+			cx = cpuidle_get_statedata(state);
+		}
 	}
 
+	current_cx_state = *cx;
+
+	mpu_state = cx->mpu_state;
+	core_state = cx->core_state;
+
+	local_irq_disable();
+	local_fiq_disable();
+
 	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
 	pwrdm_set_next_pwrst(core_pd, core_state);
 
-- 
1.5.6

Re: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Högander Jouni]

ext Sanjeev Premi <premi@ti.com> writes:

> When 'enable_off_mode' is 0, and (mpu_state < PWRDM_POWER_RET)
> the local variables mpu_state and core_state are modified; but
> the usage count for the original state selected by the governor
> are updated.
>
> This patch updates the 'last_state' in the cpuidle driver to ensure
> that statistics for the correct state are updated.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 62fbb2e..b138abd 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -76,23 +76,32 @@ 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;
> -	u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
> -
> -	current_cx_state = *cx;
> +	u32 mpu_state, core_state;
>  
>  	/* Used to keep track of the total time in idle */
>  	getnstimeofday(&ts_preidle);
>  
> -	local_irq_disable();
> -	local_fiq_disable();
> -
> +	/*
> +	 * Adjust the idle state (if required).
> +	 * Also, ensure that usage statistics of correct state are updated.
> +	 */
>  	if (!enable_off_mode) {
> -		if (mpu_state < PWRDM_POWER_RET)
> -			mpu_state = PWRDM_POWER_RET;
> -		if (core_state < PWRDM_POWER_RET)
> -			core_state = PWRDM_POWER_RET;
> +		if (cx->type > OMAP3_STATE_C4) {
> +			state = &(dev->states[OMAP3_STATE_C4 - 1]);
> +			dev->last_state = state ;
> +
> +			cx = cpuidle_get_statedata(state);

There is still C3 where OFF is used for MPU. This needs to be taken
into account.

> +		}
>  	}
>  
> +	current_cx_state = *cx;
> +
> +	mpu_state = cx->mpu_state;
> +	core_state = cx->core_state;
> +
> +	local_irq_disable();
> +	local_fiq_disable();
> +
>  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>  	pwrdm_set_next_pwrst(core_pd, core_state);
>  
> -- 
> 1.5.6
>
> --
> 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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Premi, Sanjeev]

> -----Original Message-----
> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Monday, March 09, 2009 3:38 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
> correct state
> 
> ext Sanjeev Premi <premi@ti.com> writes:
> 
> > When 'enable_off_mode' is 0, and (mpu_state < PWRDM_POWER_RET) the 
> > local variables mpu_state and core_state are modified; but 
> the usage 
> > count for the original state selected by the governor are updated.
> >
> > This patch updates the 'last_state' in the cpuidle driver to ensure 
> > that statistics for the correct state are updated.
> >
> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> > ---
> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
> +++++++++++++++++++----------
> >  1 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> > b/arch/arm/mach-omap2/cpuidle34xx.c
> > index 62fbb2e..b138abd 100644
> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> > @@ -76,23 +76,32 @@ 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;
> > -	u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
> > -
> > -	current_cx_state = *cx;
> > +	u32 mpu_state, core_state;
> >  
> >  	/* Used to keep track of the total time in idle */
> >  	getnstimeofday(&ts_preidle);
> >  
> > -	local_irq_disable();
> > -	local_fiq_disable();
> > -
> > +	/*
> > +	 * Adjust the idle state (if required).
> > +	 * Also, ensure that usage statistics of correct state 
> are updated.
> > +	 */
> >  	if (!enable_off_mode) {
> > -		if (mpu_state < PWRDM_POWER_RET)
> > -			mpu_state = PWRDM_POWER_RET;
> > -		if (core_state < PWRDM_POWER_RET)
> > -			core_state = PWRDM_POWER_RET;
> > +		if (cx->type > OMAP3_STATE_C4) {
> > +			state = &(dev->states[OMAP3_STATE_C4 - 1]);
> > +			dev->last_state = state ;
> > +
> > +			cx = cpuidle_get_statedata(state);
> 
> There is still C3 where OFF is used for MPU. This needs to be 
> taken into account.

[sp] Thanks. Good catch!
     I wasn't happy doing the "OMAP3_STATEn - 1"; but could not find a better way.
     It should be C2 as defined now.

     On another note, would it make sense to swap the definitions for C3 and C4.
     C3 : MPU CSWR + CORE CSWR
     C4 : MPU OFF + CORE Actove

> 
> > +		}
> >  	}
> >  
> > +	current_cx_state = *cx;
> > +
> > +	mpu_state = cx->mpu_state;
> > +	core_state = cx->core_state;
> > +
> > +	local_irq_disable();
> > +	local_fiq_disable();
> > +
> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
> >  	pwrdm_set_next_pwrst(core_pd, core_state);
> >  
> > --
> > 1.5.6
> >
> > --
> > 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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Högander Jouni]

"ext Premi, Sanjeev" <premi@ti.com> writes:

>> -----Original Message-----
>> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Monday, March 09, 2009 3:38 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
>> correct state
>> 
>> ext Sanjeev Premi <premi@ti.com> writes:
>> 
>> > When 'enable_off_mode' is 0, and (mpu_state < PWRDM_POWER_RET) the 
>> > local variables mpu_state and core_state are modified; but 
>> the usage 
>> > count for the original state selected by the governor are updated.
>> >
>> > This patch updates the 'last_state' in the cpuidle driver to ensure 
>> > that statistics for the correct state are updated.
>> >
>> > Signed-off-by: Sanjeev Premi <premi@ti.com>
>> > ---
>> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
>> +++++++++++++++++++----------
>> >  1 files changed, 19 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
>> > b/arch/arm/mach-omap2/cpuidle34xx.c
>> > index 62fbb2e..b138abd 100644
>> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> > @@ -76,23 +76,32 @@ 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;
>> > -	u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
>> > -
>> > -	current_cx_state = *cx;
>> > +	u32 mpu_state, core_state;
>> >  
>> >  	/* Used to keep track of the total time in idle */
>> >  	getnstimeofday(&ts_preidle);
>> >  
>> > -	local_irq_disable();
>> > -	local_fiq_disable();
>> > -
>> > +	/*
>> > +	 * Adjust the idle state (if required).
>> > +	 * Also, ensure that usage statistics of correct state 
>> are updated.
>> > +	 */
>> >  	if (!enable_off_mode) {
>> > -		if (mpu_state < PWRDM_POWER_RET)
>> > -			mpu_state = PWRDM_POWER_RET;
>> > -		if (core_state < PWRDM_POWER_RET)
>> > -			core_state = PWRDM_POWER_RET;
>> > +		if (cx->type > OMAP3_STATE_C4) {
>> > +			state = &(dev->states[OMAP3_STATE_C4 - 1]);
>> > +			dev->last_state = state ;
>> > +
>> > +			cx = cpuidle_get_statedata(state);
>> 
>> There is still C3 where OFF is used for MPU. This needs to be 
>> taken into account.
>
> [sp] Thanks. Good catch!
>      I wasn't happy doing the "OMAP3_STATEn - 1"; but could not find a better way.
>      It should be C2 as defined now.

This means C4 is not used if off mode is not enabled? I think this is
not wanted. Would it be possible to remove "OFF" C states when
enable_off_mode is written to 0 and add them back when 1 written?

>
>      On another note, would it make sense to swap the definitions for C3 and C4.
>      C3 : MPU CSWR + CORE CSWR
>      C4 : MPU OFF + CORE Actove

No it doesn't. They are organized by latency.

One grounding for current implementation is that enable_off_mode is
more or less testing interface. In final solution it might be even
removed. Adjusting states directly still shows guite accurate
information on used C-states.

>
>> 
>> > +		}
>> >  	}
>> >  
>> > +	current_cx_state = *cx;
>> > +
>> > +	mpu_state = cx->mpu_state;
>> > +	core_state = cx->core_state;
>> > +
>> > +	local_irq_disable();
>> > +	local_fiq_disable();
>> > +
>> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>> >  	pwrdm_set_next_pwrst(core_pd, core_state);
>> >  
>> > --
>> > 1.5.6
>> >
>> > --
>> > 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
>> 
>> 

-- 
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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Monday, March 09, 2009 4:07 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
> correct state
> 
> "ext Premi, Sanjeev" <premi@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
> >> Sent: Monday, March 09, 2009 3:38 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics 
> for correct 
> >> state
> >> 
> >> ext Sanjeev Premi <premi@ti.com> writes:
> >> 
> >> > When 'enable_off_mode' is 0, and (mpu_state < 
> PWRDM_POWER_RET) the 
> >> > local variables mpu_state and core_state are modified; but
> >> the usage
> >> > count for the original state selected by the governor 
> are updated.
> >> >
> >> > This patch updates the 'last_state' in the cpuidle 
> driver to ensure 
> >> > that statistics for the correct state are updated.
> >> >
> >> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> >> > ---
> >> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
> >> +++++++++++++++++++----------
> >> >  1 files changed, 19 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> > b/arch/arm/mach-omap2/cpuidle34xx.c
> >> > index 62fbb2e..b138abd 100644
> >> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> > @@ -76,23 +76,32 @@ 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;
> >> > -	u32 mpu_state = cx->mpu_state, core_state = 
> cx->core_state;
> >> > -
> >> > -	current_cx_state = *cx;
> >> > +	u32 mpu_state, core_state;
> >> >  
> >> >  	/* Used to keep track of the total time in idle */
> >> >  	getnstimeofday(&ts_preidle);
> >> >  
> >> > -	local_irq_disable();
> >> > -	local_fiq_disable();
> >> > -
> >> > +	/*
> >> > +	 * Adjust the idle state (if required).
> >> > +	 * Also, ensure that usage statistics of correct state
> >> are updated.
> >> > +	 */
> >> >  	if (!enable_off_mode) {
> >> > -		if (mpu_state < PWRDM_POWER_RET)
> >> > -			mpu_state = PWRDM_POWER_RET;
> >> > -		if (core_state < PWRDM_POWER_RET)
> >> > -			core_state = PWRDM_POWER_RET;
> >> > +		if (cx->type > OMAP3_STATE_C4) {
> >> > +			state = 
> &(dev->states[OMAP3_STATE_C4 - 1]);
> >> > +			dev->last_state = state ;
> >> > +
> >> > +			cx = cpuidle_get_statedata(state);
> >> 
> >> There is still C3 where OFF is used for MPU. This needs to 
> be taken 
> >> into account.
> >
> > [sp] Thanks. Good catch!
> >      I wasn't happy doing the "OMAP3_STATEn - 1"; but could 
> not find a better way.
> >      It should be C2 as defined now.
> 
> This means C4 is not used if off mode is not enabled? I think 
> this is not wanted. Would it be possible to remove "OFF" C 
> states when enable_off_mode is written to 0 and add them back 
> when 1 written?

[sp] That should be possible. We could use the 'valid' field
     for the purpose.

> >
> >      On another note, would it make sense to swap the 
> definitions for C3 and C4.
> >      C3 : MPU CSWR + CORE CSWR
> >      C4 : MPU OFF + CORE Actove
> 
> No it doesn't. They are organized by latency.

[sp] Okay. That was a loud thinking from my side :)
> 
> One grounding for current implementation is that 
> enable_off_mode is more or less testing interface. In final 
> solution it might be even removed. Adjusting states directly 
> still shows guite accurate information on used C-states.
> 
> >
> >> 
> >> > +		}
> >> >  	}
> >> >  
> >> > +	current_cx_state = *cx;
> >> > +
> >> > +	mpu_state = cx->mpu_state;
> >> > +	core_state = cx->core_state;
> >> > +
> >> > +	local_irq_disable();
> >> > +	local_fiq_disable();
> >> > +
> >> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
> >> >  	pwrdm_set_next_pwrst(core_pd, core_state);
> >> >  
> >> > --
> >> > 1.5.6
> >> >
> >> > --
> >> > 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
> >> 
> >> 
> 
> --
> 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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Kevin Hilman]

"Premi, Sanjeev" <premi@ti.com> writes:

>  
>
>> -----Original Message-----
>> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Monday, March 09, 2009 4:07 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
>> correct state
>> 
>> "ext Premi, Sanjeev" <premi@ti.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
>> >> Sent: Monday, March 09, 2009 3:38 PM
>> >> To: Premi, Sanjeev
>> >> Cc: linux-omap@vger.kernel.org
>> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics 
>> for correct 
>> >> state
>> >> 
>> >> ext Sanjeev Premi <premi@ti.com> writes:
>> >> 
>> >> > When 'enable_off_mode' is 0, and (mpu_state < 
>> PWRDM_POWER_RET) the 
>> >> > local variables mpu_state and core_state are modified; but
>> >> the usage
>> >> > count for the original state selected by the governor 
>> are updated.
>> >> >
>> >> > This patch updates the 'last_state' in the cpuidle 
>> driver to ensure 
>> >> > that statistics for the correct state are updated.
>> >> >
>> >> > Signed-off-by: Sanjeev Premi <premi@ti.com>
>> >> > ---
>> >> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
>> >> +++++++++++++++++++----------
>> >> >  1 files changed, 19 insertions(+), 10 deletions(-)
>> >> >
>> >> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> > b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> > index 62fbb2e..b138abd 100644
>> >> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> > @@ -76,23 +76,32 @@ 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;
>> >> > -	u32 mpu_state = cx->mpu_state, core_state = 
>> cx->core_state;
>> >> > -
>> >> > -	current_cx_state = *cx;
>> >> > +	u32 mpu_state, core_state;
>> >> >  
>> >> >  	/* Used to keep track of the total time in idle */
>> >> >  	getnstimeofday(&ts_preidle);
>> >> >  
>> >> > -	local_irq_disable();
>> >> > -	local_fiq_disable();
>> >> > -
>> >> > +	/*
>> >> > +	 * Adjust the idle state (if required).
>> >> > +	 * Also, ensure that usage statistics of correct state
>> >> are updated.
>> >> > +	 */
>> >> >  	if (!enable_off_mode) {
>> >> > -		if (mpu_state < PWRDM_POWER_RET)
>> >> > -			mpu_state = PWRDM_POWER_RET;
>> >> > -		if (core_state < PWRDM_POWER_RET)
>> >> > -			core_state = PWRDM_POWER_RET;
>> >> > +		if (cx->type > OMAP3_STATE_C4) {
>> >> > +			state = 
>> &(dev->states[OMAP3_STATE_C4 - 1]);
>> >> > +			dev->last_state = state ;
>> >> > +
>> >> > +			cx = cpuidle_get_statedata(state);
>> >> 
>> >> There is still C3 where OFF is used for MPU. This needs to 
>> be taken 
>> >> into account.
>> >
>> > [sp] Thanks. Good catch!
>> >      I wasn't happy doing the "OMAP3_STATEn - 1"; but could 
>> not find a better way.
>> >      It should be C2 as defined now.
>> 
>> This means C4 is not used if off mode is not enabled? I think 
>> this is not wanted. Would it be possible to remove "OFF" C 
>> states when enable_off_mode is written to 0 and add them back 
>> when 1 written?
>
> [sp] That should be possible. We could use the 'valid' field
>      for the purpose.

I would gladly take a patch which uses the 'valid' field for this
and then the enter hook whould drop to the next lower valid state
if the state requested is not valid.

Kevin


>
>> >
>> >      On another note, would it make sense to swap the 
>> definitions for C3 and C4.
>> >      C3 : MPU CSWR + CORE CSWR
>> >      C4 : MPU OFF + CORE Actove
>> 
>> No it doesn't. They are organized by latency.
>
> [sp] Okay. That was a loud thinking from my side :)
>> 
>> One grounding for current implementation is that 
>> enable_off_mode is more or less testing interface. In final 
>> solution it might be even removed. Adjusting states directly 
>> still shows guite accurate information on used C-states.
>> 
>> >
>> >> 
>> >> > +		}
>> >> >  	}
>> >> >  
>> >> > +	current_cx_state = *cx;
>> >> > +
>> >> > +	mpu_state = cx->mpu_state;
>> >> > +	core_state = cx->core_state;
>> >> > +
>> >> > +	local_irq_disable();
>> >> > +	local_fiq_disable();
>> >> > +
>> >> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>> >> >  	pwrdm_set_next_pwrst(core_pd, core_state);
>> >> >  
>> >> > --
>> >> > 1.5.6
>> >> >
>> >> > --
>> >> > 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
>> >> 
>> >> 
>> 
>> --
>> 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
--
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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Premi, Sanjeev]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Monday, March 09, 2009 11:31 PM
> To: Premi, Sanjeev
> Cc: Högander Jouni; linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
> correct state
> 
> "Premi, Sanjeev" <premi@ti.com> writes:
> 
> >  
> >
> >> -----Original Message-----
> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
> >> Sent: Monday, March 09, 2009 4:07 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics 
> for correct 
> >> state
> >> 
> >> "ext Premi, Sanjeev" <premi@ti.com> writes:
> >> 
> >> >> -----Original Message-----
> >> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
> >> >> Sent: Monday, March 09, 2009 3:38 PM
> >> >> To: Premi, Sanjeev
> >> >> Cc: linux-omap@vger.kernel.org
> >> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics
> >> for correct
> >> >> state
> >> >> 
> >> >> ext Sanjeev Premi <premi@ti.com> writes:
> >> >> 
> >> >> > When 'enable_off_mode' is 0, and (mpu_state <
> >> PWRDM_POWER_RET) the
> >> >> > local variables mpu_state and core_state are modified; but
> >> >> the usage
> >> >> > count for the original state selected by the governor
> >> are updated.
> >> >> >
> >> >> > This patch updates the 'last_state' in the cpuidle
> >> driver to ensure
> >> >> > that statistics for the correct state are updated.
> >> >> >
> >> >> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> >> >> > ---
> >> >> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
> >> >> +++++++++++++++++++----------
> >> >> >  1 files changed, 19 insertions(+), 10 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > index 62fbb2e..b138abd 100644
> >> >> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > @@ -76,23 +76,32 @@ 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;
> >> >> > -	u32 mpu_state = cx->mpu_state, core_state = 
> >> cx->core_state;
> >> >> > -
> >> >> > -	current_cx_state = *cx;
> >> >> > +	u32 mpu_state, core_state;
> >> >> >  
> >> >> >  	/* Used to keep track of the total time in idle */
> >> >> >  	getnstimeofday(&ts_preidle);
> >> >> >  
> >> >> > -	local_irq_disable();
> >> >> > -	local_fiq_disable();
> >> >> > -
> >> >> > +	/*
> >> >> > +	 * Adjust the idle state (if required).
> >> >> > +	 * Also, ensure that usage statistics of correct state
> >> >> are updated.
> >> >> > +	 */
> >> >> >  	if (!enable_off_mode) {
> >> >> > -		if (mpu_state < PWRDM_POWER_RET)
> >> >> > -			mpu_state = PWRDM_POWER_RET;
> >> >> > -		if (core_state < PWRDM_POWER_RET)
> >> >> > -			core_state = PWRDM_POWER_RET;
> >> >> > +		if (cx->type > OMAP3_STATE_C4) {
> >> >> > +			state =
> >> &(dev->states[OMAP3_STATE_C4 - 1]);
> >> >> > +			dev->last_state = state ;
> >> >> > +
> >> >> > +			cx = cpuidle_get_statedata(state);
> >> >> 
> >> >> There is still C3 where OFF is used for MPU. This needs to
> >> be taken
> >> >> into account.
> >> >
> >> > [sp] Thanks. Good catch!
> >> >      I wasn't happy doing the "OMAP3_STATEn - 1"; but could
> >> not find a better way.
> >> >      It should be C2 as defined now.
> >> 
> >> This means C4 is not used if off mode is not enabled? I 
> think this is 
> >> not wanted. Would it be possible to remove "OFF" C states when 
> >> enable_off_mode is written to 0 and add them back when 1 written?
> >
> > [sp] That should be possible. We could use the 'valid' field
> >      for the purpose.
> 
> I would gladly take a patch which uses the 'valid' field for 
> this and then the enter hook whould drop to the next lower 
> valid state if the state requested is not valid.
> 
> Kevin

[sp] I have not yet tested it (working offline for sometime).
     But, wanted to share the changes for an early review.

     Initially, I was trying see if the CPUIDLE framework could
     use ".valid" as an additional argument in cpuidle_state.
     But, may be that's for later...

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx
index c25158c..9493553 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -88,16 +88,19 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
                goto return_sleep_time;

        /*
+        * Check if the chosen idle state is valid.
+        * If no, drop down to a lower valid state. Expects the lowest
+        * state will always be active.
         */
+       if (!cx->valid) {
+               for (idx = (cx->type - 1); idx == 1; idx--) {
+                       if (omap3_power_states[idx].valid)
+                               break;
                }
+               state = &(dev->states[idx]);
+               dev->last_state = state ;
+
+               cx = cpuidle_get_statedata(state);
        }

        current_cx_state = *cx;
@@ -150,6 +153,26 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
        return omap3_enter_idle(dev, new_state);
 }

+/**
+ * omap3_toggle_off_states - Enable / Disable validity of idle states
+ * @flag: Enable/ Disable support for OFF mode
+ *
+ * Called as result of change to "enable_off_mode".
+ */
+void omap3_toggle_off_states(unsigned short flag)
+{
+       if (flag) {
+               omap3_power_states[OMAP3_STATE_C3].valid = 1;
+               omap3_power_states[OMAP3_STATE_C5].valid = 1;
+               omap3_power_states[OMAP3_STATE_C6].valid = 1;
+       }
+       else {
+               omap3_power_states[OMAP3_STATE_C3].valid = 0;
+               omap3_power_states[OMAP3_STATE_C5].valid = 0;
+               omap3_power_states[OMAP3_STATE_C6].valid = 0;
+       }
+}
+
 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);

 /* omap3_init_power_states - Initialises the OMAP3 specific C states.
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 61c6dfb..6785850 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -47,6 +47,8 @@ atomic_t sleep_block = ATOMIC_INIT(0);
 static int vdd1_locked;
 static int vdd2_locked;

+extern void omap3_toggle_off_states(unsigned short);
+
 static ssize_t idle_show(struct kobject *, struct kobj_attribute *, char *);
 static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
                          const char *buf, size_t n);
@@ -112,6 +114,8 @@ static ssize_t idle_store(struct kobject *kobj, struct kobj_
        } else if (attr == &enable_off_mode_attr) {
                enable_off_mode = value;
                omap3_pm_off_mode_enable(enable_off_mode);
+               if (cpu_is_omap34xx())
+                       omap3_toggle_off_states(value);
        } else if (attr == &voltage_off_while_idle_attr) {
                voltage_off_while_idle = value;
                if (voltage_off_while_idle)

> 
> 
> >
> >> >
> >> >      On another note, would it make sense to swap the
> >> definitions for C3 and C4.
> >> >      C3 : MPU CSWR + CORE CSWR
> >> >      C4 : MPU OFF + CORE Actove
> >> 
> >> No it doesn't. They are organized by latency.
> >
> > [sp] Okay. That was a loud thinking from my side :)
> >> 
> >> One grounding for current implementation is that 
> enable_off_mode is 
> >> more or less testing interface. In final solution it might be even 
> >> removed. Adjusting states directly still shows guite accurate 
> >> information on used C-states.
> >> 
> >> >
> >> >> 
> >> >> > +		}
> >> >> >  	}
> >> >> >  
> >> >> > +	current_cx_state = *cx;
> >> >> > +
> >> >> > +	mpu_state = cx->mpu_state;
> >> >> > +	core_state = cx->core_state;
> >> >> > +
> >> >> > +	local_irq_disable();
> >> >> > +	local_fiq_disable();
> >> >> > +
> >> >> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
> >> >> >  	pwrdm_set_next_pwrst(core_pd, core_state);
> >> >> >  
> >> >> > --
> >> >> > 1.5.6
> >> >> >
> >> >> > --
> >> >> > 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
> >> >> 
> >> >> 
> >> 
> >> --
> >> 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
> 
> --
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: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Kevin Hilman]

"Premi, Sanjeev" <premi@ti.com> writes:

>> 
>> I would gladly take a patch which uses the 'valid' field for 
>> this and then the enter hook whould drop to the next lower 
>> valid state if the state requested is not valid.
>> 
>> Kevin
>
> [sp] I have not yet tested it (working offline for sometime).
>      But, wanted to share the changes for an early review.

Thanks, some comments below.

>      Initially, I was trying see if the CPUIDLE framework could
>      use ".valid" as an additional argument in cpuidle_state.
>      But, may be that's for later...

Yeah, I looked at that too, but it currently doesn't have a concept
of valid states, so for now I recommend we implement that in the
OMAP-specific code as you have done.

> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx
> index c25158c..9493553 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -88,16 +88,19 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>                 goto return_sleep_time;
>
>         /*
> +        * Check if the chosen idle state is valid.
> +        * If no, drop down to a lower valid state. Expects the lowest
> +        * state will always be active.
>          */
> +       if (!cx->valid) {
> +               for (idx = (cx->type - 1); idx == 1; idx--) {
                                             ^^^^^^^^

I think you mean idx >= 1 here.

Also, while you're working on this, could you fix this up so the
omap3_power_states[] array is zero based insted of 1-based, it would
make this code and the other code walking this array easier to follow.

That means defining OMAP_STATE_C1 = 0 and so on.

> +                       if (omap3_power_states[idx].valid)
> +                               break;
>                 }
> +               state = &(dev->states[idx]);
> +               dev->last_state = state ;
> +
> +               cx = cpuidle_get_statedata(state);
>         }
>
>         current_cx_state = *cx;
> @@ -150,6 +153,26 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>         return omap3_enter_idle(dev, new_state);
>  }
>
> +/**
> + * omap3_toggle_off_states - Enable / Disable validity of idle states
> + * @flag: Enable/ Disable support for OFF mode
> + *
> + * Called as result of change to "enable_off_mode".
> + */
> +void omap3_toggle_off_states(unsigned short flag)
> +{

How about calling this omap3_cpuidle_update_states() and taking
no arguments.  Rather than the 'flag' argument, internally it
just checks the global 'enable_off_mode.'

This allows for potential further expansion down the road of other
reasons to update CPUidle valid states.  For example, we've talked
about updating the CPUidle state latencies on the fly depending on
various other chip settings.

> +       if (flag) {
> +               omap3_power_states[OMAP3_STATE_C3].valid = 1;
> +               omap3_power_states[OMAP3_STATE_C5].valid = 1;
> +               omap3_power_states[OMAP3_STATE_C6].valid = 1;
> +       }
> +       else {
> +               omap3_power_states[OMAP3_STATE_C3].valid = 0;
> +               omap3_power_states[OMAP3_STATE_C5].valid = 0;
> +               omap3_power_states[OMAP3_STATE_C6].valid = 0;
> +       }
> +}
> +

Rather than set set specific OMAP3_STATE_Cx values, it would be better
to just walk the array, and check for [mpu|core]_state = PWRDM_POWER_OFF.
If the state has either set, then update the valid flag.

>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>
>  /* omap3_init_power_states - Initialises the OMAP3 specific C states.
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 61c6dfb..6785850 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -47,6 +47,8 @@ atomic_t sleep_block = ATOMIC_INIT(0);
>  static int vdd1_locked;
>  static int vdd2_locked;
>
> +extern void omap3_toggle_off_states(unsigned short);
> +

Move the definition into pm.h as omap3_cpuidle_update_states(void) as
described above.

>  static ssize_t idle_show(struct kobject *, struct kobj_attribute *, char *);
>  static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
>                           const char *buf, size_t n);
> @@ -112,6 +114,8 @@ static ssize_t idle_store(struct kobject *kobj, struct kobj_
>         } else if (attr == &enable_off_mode_attr) {
>                 enable_off_mode = value;
>                 omap3_pm_off_mode_enable(enable_off_mode);
> +               if (cpu_is_omap34xx())
> +                       omap3_toggle_off_states(value);

Then, don't modify pm.c, rather just call omap3_cpuidle_update_states() from
omap3_pm_off_mode_enable().

Kevin

RE: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Thursday, March 12, 2009 5:31 AM
> To: Premi, Sanjeev
> Cc: Högander Jouni; linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
> correct state
> 
> "Premi, Sanjeev" <premi@ti.com> writes:
> 
> >> 
> >> I would gladly take a patch which uses the 'valid' field 
> for this and 
> >> then the enter hook whould drop to the next lower valid 
> state if the 
> >> state requested is not valid.
> >> 
> >> Kevin
> >
> > [sp] I have not yet tested it (working offline for sometime).
> >      But, wanted to share the changes for an early review.
> 
> Thanks, some comments below.
> 
> >      Initially, I was trying see if the CPUIDLE framework could
> >      use ".valid" as an additional argument in cpuidle_state.
> >      But, may be that's for later...
> 
> Yeah, I looked at that too, but it currently doesn't have a 
> concept of valid states, so for now I recommend we implement 
> that in the OMAP-specific code as you have done.
> 
> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> > b/arch/arm/mach-omap2/cpuidle34xx index c25158c..9493553 100644
> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> > @@ -88,16 +88,19 @@ static int omap3_enter_idle(struct 
> cpuidle_device *dev,
> >                 goto return_sleep_time;
> >
> >         /*
> > +        * Check if the chosen idle state is valid.
> > +        * If no, drop down to a lower valid state. Expects 
> the lowest
> > +        * state will always be active.
> >          */
> > +       if (!cx->valid) {
> > +               for (idx = (cx->type - 1); idx == 1; idx--) {
>                                              ^^^^^^^^
> 
> I think you mean idx >= 1 here.

[sp] Yes. A typo. Still haven't tried this patch myself.

> 
> Also, while you're working on this, could you fix this up so 
> the omap3_power_states[] array is zero based insted of 
> 1-based, it would make this code and the other code walking 
> this array easier to follow.
> 
> That means defining OMAP_STATE_C1 = 0 and so on.

[sp] Definitely.

> 
> > +                       if (omap3_power_states[idx].valid)
> > +                               break;
> >                 }
> > +               state = &(dev->states[idx]);
> > +               dev->last_state = state ;
> > +
> > +               cx = cpuidle_get_statedata(state);
> >         }
> >
> >         current_cx_state = *cx;
> > @@ -150,6 +153,26 @@ static int omap3_enter_idle_bm(struct 
> cpuidle_device *dev,
> >         return omap3_enter_idle(dev, new_state);  }
> >
> > +/**
> > + * omap3_toggle_off_states - Enable / Disable validity of 
> idle states
> > + * @flag: Enable/ Disable support for OFF mode
> > + *
> > + * Called as result of change to "enable_off_mode".
> > + */
> > +void omap3_toggle_off_states(unsigned short flag) {
> 
> How about calling this omap3_cpuidle_update_states() and 
> taking no arguments.  Rather than the 'flag' argument, 
> internally it just checks the global 'enable_off_mode.'
> 
> This allows for potential further expansion down the road of 
> other reasons to update CPUidle valid states.  For example, 
> we've talked about updating the CPUidle state latencies on 
> the fly depending on various other chip settings.
> 
> > +       if (flag) {
> > +               omap3_power_states[OMAP3_STATE_C3].valid = 1;
> > +               omap3_power_states[OMAP3_STATE_C5].valid = 1;
> > +               omap3_power_states[OMAP3_STATE_C6].valid = 1;
> > +       }
> > +       else {
> > +               omap3_power_states[OMAP3_STATE_C3].valid = 0;
> > +               omap3_power_states[OMAP3_STATE_C5].valid = 0;
> > +               omap3_power_states[OMAP3_STATE_C6].valid = 0;
> > +       }
> > +}
> > +
> 
> Rather than set set specific OMAP3_STATE_Cx values, it would 
> be better to just walk the array, and check for 
> [mpu|core]_state = PWRDM_POWER_OFF.
> If the state has either set, then update the valid flag.
> 
> >  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> >
> >  /* omap3_init_power_states - Initialises the OMAP3 
> specific C states.
> > diff --git a/arch/arm/mach-omap2/pm.c 
> b/arch/arm/mach-omap2/pm.c index 
> > 61c6dfb..6785850 100644
> > --- a/arch/arm/mach-omap2/pm.c
> > +++ b/arch/arm/mach-omap2/pm.c
> > @@ -47,6 +47,8 @@ atomic_t sleep_block = ATOMIC_INIT(0);  
> static int 
> > vdd1_locked;  static int vdd2_locked;
> >
> > +extern void omap3_toggle_off_states(unsigned short);
> > +
> 
> Move the definition into pm.h as 
> omap3_cpuidle_update_states(void) as described above.
> 
> >  static ssize_t idle_show(struct kobject *, struct 
> kobj_attribute *, 
> > char *);  static ssize_t idle_store(struct kobject *k, 
> struct kobj_attribute *,
> >                           const char *buf, size_t n); @@ 
> -112,6 +114,8 
> > @@ static ssize_t idle_store(struct kobject *kobj, struct kobj_
> >         } else if (attr == &enable_off_mode_attr) {
> >                 enable_off_mode = value;
> >                 omap3_pm_off_mode_enable(enable_off_mode);
> > +               if (cpu_is_omap34xx())
> > +                       omap3_toggle_off_states(value);
> 
> Then, don't modify pm.c, rather just call 
> omap3_cpuidle_update_states() from omap3_pm_off_mode_enable().
> 
> Kevin
> 
> --
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