| Message ID | 1236455149-17017-1-git-send-email-premi@ti.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kevin Hilman |
| Headers | show |
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
> -----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
"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 >> >>
> -----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
"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
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);
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(-)