* Re: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-16 16:33 [PATCH] PM: Fixed clockdomain state control for OMAP3 Tero Kristo
@ 2008-09-19 14:08 ` Högander Jouni
2008-09-23 13:02 ` Paul Walmsley
2008-09-23 13:22 ` Paul Walmsley
2 siblings, 0 replies; 7+ messages in thread
From: Högander Jouni @ 2008-09-19 14:08 UTC (permalink / raw)
To: ext Tero Kristo; +Cc: linux-omap, ext Rajendra Nayak
"ext Tero Kristo" <tero.kristo@nokia.com> writes:
> Hardware supervised control for clockdomain power state transitions now
> enabled in omap3_pm_init(). Also fixed set_pwrdm_state() to allow state
> changes between sleep states (i.e. RET<->OFF.)
Yes you are right here. Denying idle doesn't make sure that related
pwrdm is woken up.
Rajendra, after this patch is applied, it is probably enough if you set
just next state for core and mpu domains in cpuidle. No need to call
set_pwrdm_state from pm34xx.c.
Acked-by: Jouni Högander <jouni.hogander@nokia.com>
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> ---
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++++++++++++-------------------
> 1 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 01b637a..2bccdab 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -227,28 +227,13 @@ static int omap3_can_sleep(void)
> return 1;
> }
>
> -/* _clkdm_deny_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_deny_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_deny_idle(clkdm);
> - return 0;
> -}
> -
> -/* _clkdm_allow_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_allow_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_allow_idle(clkdm);
> - return 0;
> -}
> -
> /* This sets pwrdm state (other than mpu & core. Currently only ON &
> * RET are supported. Function is assuming that clkdm doesn't have
> * hw_sup mode enabled. */
> static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> {
> u32 cur_state;
> + int sleep_switch = 0;
> int ret = 0;
>
> if (pwrdm == NULL || IS_ERR(pwrdm))
> @@ -259,8 +244,12 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> if (cur_state == state)
> return ret;
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_deny_idle);
> -
> + if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
> + omap2_clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
> + sleep_switch = 1;
> + pwrdm_wait_transition(pwrdm);
> + }
> +
> ret = pwrdm_set_next_pwrst(pwrdm, state);
> if (ret) {
> printk(KERN_ERR "Unable to set state of powerdomain: %s\n",
> @@ -268,7 +257,10 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> goto err;
> }
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_allow_idle);
> + if (sleep_switch) {
> + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
> + pwrdm_wait_transition(pwrdm);
> + }
>
> err:
> return ret;
> @@ -540,6 +532,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
> return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> }
>
> +static int __init clkdms_setup(struct clockdomain *clkdm)
> +{
> + omap2_clkdm_allow_idle(clkdm);
> + return 0;
> +}
> +
> int __init omap3_pm_init(void)
> {
> struct power_state *pwrst;
> @@ -566,6 +564,8 @@ int __init omap3_pm_init(void)
> goto err2;
> }
>
> + (void) clkdm_for_each(clkdms_setup);
> +
> mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
> if (mpu_pwrdm == NULL) {
> printk(KERN_ERR "Failed to get mpu_pwrdm\n");
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-16 16:33 [PATCH] PM: Fixed clockdomain state control for OMAP3 Tero Kristo
2008-09-19 14:08 ` Högander Jouni
@ 2008-09-23 13:02 ` Paul Walmsley
2008-09-23 13:16 ` Tero.Kristo
2008-09-23 13:22 ` Paul Walmsley
2 siblings, 1 reply; 7+ messages in thread
From: Paul Walmsley @ 2008-09-23 13:02 UTC (permalink / raw)
To: Tero Kristo; +Cc: linux-omap
Hello Tero,
a few comments.
On Tue, 16 Sep 2008, Tero Kristo wrote:
> Hardware supervised control for clockdomain power state transitions now
> enabled in omap3_pm_init(). Also fixed set_pwrdm_state() to allow state
> changes between sleep states (i.e. RET<->OFF.)
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> ---
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++++++++++++-------------------
> 1 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 01b637a..2bccdab 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -227,28 +227,13 @@ static int omap3_can_sleep(void)
> return 1;
> }
>
> -/* _clkdm_deny_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_deny_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_deny_idle(clkdm);
> - return 0;
> -}
> -
> -/* _clkdm_allow_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_allow_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_allow_idle(clkdm);
> - return 0;
> -}
> -
> /* This sets pwrdm state (other than mpu & core. Currently only ON &
> * RET are supported. Function is assuming that clkdm doesn't have
> * hw_sup mode enabled. */
> static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> {
> u32 cur_state;
> + int sleep_switch = 0;
> int ret = 0;
>
> if (pwrdm == NULL || IS_ERR(pwrdm))
> @@ -259,8 +244,12 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> if (cur_state == state)
> return ret;
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_deny_idle);
> -
> + if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
> + omap2_clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
In general, shouldn't this happen for each clkdm in the pwrdm? i.e.,
you should probably create some _clkdm_wakeup() (similar to
_clkdm_allow_idle() ) and call that via pwrdm_for_each_clkdm().
This may not have any functional effect at present with OMAP3, but
avoids the assumption that each powerdomain will have only one
software-controllable clockdomain (and that it will always appear at the 0
element in the array).
> + sleep_switch = 1;
> + pwrdm_wait_transition(pwrdm);
> + }
> +
> ret = pwrdm_set_next_pwrst(pwrdm, state);
> if (ret) {
> printk(KERN_ERR "Unable to set state of powerdomain: %s\n",
> @@ -268,7 +257,10 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> goto err;
> }
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_allow_idle);
> + if (sleep_switch) {
> + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
Same comment as previous - shouldn't _clkdm_allow_idle() be kept, and
called for each clkdm in the pwrdm, instead?
> + pwrdm_wait_transition(pwrdm);
> + }
>
> err:
> return ret;
> @@ -540,6 +532,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
> return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> }
>
> +static int __init clkdms_setup(struct clockdomain *clkdm)
> +{
> + omap2_clkdm_allow_idle(clkdm);
> + return 0;
> +}
> +
> int __init omap3_pm_init(void)
> {
> struct power_state *pwrst;
> @@ -566,6 +564,8 @@ int __init omap3_pm_init(void)
> goto err2;
> }
>
> + (void) clkdm_for_each(clkdms_setup);
> +
> mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
> if (mpu_pwrdm == NULL) {
> printk(KERN_ERR "Failed to get mpu_pwrdm\n");
> --
> 1.5.4.3
>
> --
> 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
>
- Paul
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-23 13:02 ` Paul Walmsley
@ 2008-09-23 13:16 ` Tero.Kristo
2008-09-23 13:21 ` Paul Walmsley
0 siblings, 1 reply; 7+ messages in thread
From: Tero.Kristo @ 2008-09-23 13:16 UTC (permalink / raw)
To: paul; +Cc: linux-omap
Hi Paul, reply below.
>-----Original Message-----
>From: ext Paul Walmsley [mailto:paul@pwsan.com]
>Sent: 23 September, 2008 16:02
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCH] PM: Fixed clockdomain state control for OMAP3
>
>Hello Tero,
>
>a few comments.
>
>On Tue, 16 Sep 2008, Tero Kristo wrote:
>
<clip>
>> - pwrdm_for_each_clkdm(pwrdm, _clkdm_deny_idle);
>> -
>> + if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
>> + omap2_clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
>
>In general, shouldn't this happen for each clkdm in the pwrdm?
> i.e., you should probably create some _clkdm_wakeup() (similar to
>_clkdm_allow_idle() ) and call that via pwrdm_for_each_clkdm().
>
>This may not have any functional effect at present with OMAP3,
>but avoids the assumption that each powerdomain will have only
>one software-controllable clockdomain (and that it will always
>appear at the 0 element in the array).
The access of element 0 only is not because we have only 1 clockdomain
for each powerdomain, but because we only want to wake up some
clockdomain
to make the powerdomain come up. It does not matter which clockdomain is
awakened, as long as one is, and I don't think it makes sense to wake
all
of them up because this would cause additional overhead.
>
>> + sleep_switch = 1;
>> + pwrdm_wait_transition(pwrdm);
>> + }
>> +
>> ret = pwrdm_set_next_pwrst(pwrdm, state);
>> if (ret) {
>> printk(KERN_ERR "Unable to set state of
>powerdomain: %s\n", @@
>> -268,7 +257,10 @@ static int set_pwrdm_state(struct
>powerdomain *pwrdm, u32 state)
>> goto err;
>> }
>>
>> - pwrdm_for_each_clkdm(pwrdm, _clkdm_allow_idle);
>> + if (sleep_switch) {
>> + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
>
>Same comment as previous - shouldn't _clkdm_allow_idle() be
>kept, and called for each clkdm in the pwrdm, instead?
Same comment.
-Tero
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-23 13:16 ` Tero.Kristo
@ 2008-09-23 13:21 ` Paul Walmsley
0 siblings, 0 replies; 7+ messages in thread
From: Paul Walmsley @ 2008-09-23 13:21 UTC (permalink / raw)
To: Tero.Kristo; +Cc: linux-omap
Hello Tero,
On Tue, 23 Sep 2008, Tero.Kristo@nokia.com wrote:
> The access of element 0 only is not because we have only 1 clockdomain
> for each powerdomain, but because we only want to wake up some
> clockdomain
> to make the powerdomain come up. It does not matter which clockdomain is
> awakened, as long as one is, and I don't think it makes sense to wake
> all
> of them up because this would cause additional overhead.
Okay, that sounds fine.
- Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-16 16:33 [PATCH] PM: Fixed clockdomain state control for OMAP3 Tero Kristo
2008-09-19 14:08 ` Högander Jouni
2008-09-23 13:02 ` Paul Walmsley
@ 2008-09-23 13:22 ` Paul Walmsley
2008-09-25 10:42 ` Tony Lindgren
2 siblings, 1 reply; 7+ messages in thread
From: Paul Walmsley @ 2008-09-23 13:22 UTC (permalink / raw)
To: Tero Kristo; +Cc: linux-omap
On Tue, 16 Sep 2008, Tero Kristo wrote:
> Hardware supervised control for clockdomain power state transitions now
> enabled in omap3_pm_init(). Also fixed set_pwrdm_state() to allow state
> changes between sleep states (i.e. RET<->OFF.)
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
Acked-by: Paul Walmsley <paul@pwsan.com>
> ---
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++++++++++++-------------------
> 1 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 01b637a..2bccdab 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -227,28 +227,13 @@ static int omap3_can_sleep(void)
> return 1;
> }
>
> -/* _clkdm_deny_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_deny_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_deny_idle(clkdm);
> - return 0;
> -}
> -
> -/* _clkdm_allow_idle - private callback function used by set_pwrdm_state() */
> -static int _clkdm_allow_idle(struct powerdomain *pwrdm,
> - struct clockdomain *clkdm)
> -{
> - omap2_clkdm_allow_idle(clkdm);
> - return 0;
> -}
> -
> /* This sets pwrdm state (other than mpu & core. Currently only ON &
> * RET are supported. Function is assuming that clkdm doesn't have
> * hw_sup mode enabled. */
> static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> {
> u32 cur_state;
> + int sleep_switch = 0;
> int ret = 0;
>
> if (pwrdm == NULL || IS_ERR(pwrdm))
> @@ -259,8 +244,12 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> if (cur_state == state)
> return ret;
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_deny_idle);
> -
> + if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
> + omap2_clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
> + sleep_switch = 1;
> + pwrdm_wait_transition(pwrdm);
> + }
> +
> ret = pwrdm_set_next_pwrst(pwrdm, state);
> if (ret) {
> printk(KERN_ERR "Unable to set state of powerdomain: %s\n",
> @@ -268,7 +257,10 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> goto err;
> }
>
> - pwrdm_for_each_clkdm(pwrdm, _clkdm_allow_idle);
> + if (sleep_switch) {
> + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
> + pwrdm_wait_transition(pwrdm);
> + }
>
> err:
> return ret;
> @@ -540,6 +532,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
> return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> }
>
> +static int __init clkdms_setup(struct clockdomain *clkdm)
> +{
> + omap2_clkdm_allow_idle(clkdm);
> + return 0;
> +}
> +
> int __init omap3_pm_init(void)
> {
> struct power_state *pwrst;
> @@ -566,6 +564,8 @@ int __init omap3_pm_init(void)
> goto err2;
> }
>
> + (void) clkdm_for_each(clkdms_setup);
> +
> mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
> if (mpu_pwrdm == NULL) {
> printk(KERN_ERR "Failed to get mpu_pwrdm\n");
> --
> 1.5.4.3
>
> --
> 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
>
- Paul
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] PM: Fixed clockdomain state control for OMAP3
2008-09-23 13:22 ` Paul Walmsley
@ 2008-09-25 10:42 ` Tony Lindgren
0 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2008-09-25 10:42 UTC (permalink / raw)
To: Paul Walmsley; +Cc: Tero Kristo, linux-omap
* Paul Walmsley <paul@booyaka.com> [080923 16:22]:
> On Tue, 16 Sep 2008, Tero Kristo wrote:
>
> > Hardware supervised control for clockdomain power state transitions now
> > enabled in omap3_pm_init(). Also fixed set_pwrdm_state() to allow state
> > changes between sleep states (i.e. RET<->OFF.)
> >
> > Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>
> Acked-by: Paul Walmsley <paul@pwsan.com>
Pushing today.
Tony
>
>
> > ---
> > arch/arm/mach-omap2/pm34xx.c | 38 +++++++++++++++++++-------------------
> > 1 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> > index 01b637a..2bccdab 100644
> > --- a/arch/arm/mach-omap2/pm34xx.c
> > +++ b/arch/arm/mach-omap2/pm34xx.c
> > @@ -227,28 +227,13 @@ static int omap3_can_sleep(void)
> > return 1;
> > }
> >
> > -/* _clkdm_deny_idle - private callback function used by set_pwrdm_state() */
> > -static int _clkdm_deny_idle(struct powerdomain *pwrdm,
> > - struct clockdomain *clkdm)
> > -{
> > - omap2_clkdm_deny_idle(clkdm);
> > - return 0;
> > -}
> > -
> > -/* _clkdm_allow_idle - private callback function used by set_pwrdm_state() */
> > -static int _clkdm_allow_idle(struct powerdomain *pwrdm,
> > - struct clockdomain *clkdm)
> > -{
> > - omap2_clkdm_allow_idle(clkdm);
> > - return 0;
> > -}
> > -
> > /* This sets pwrdm state (other than mpu & core. Currently only ON &
> > * RET are supported. Function is assuming that clkdm doesn't have
> > * hw_sup mode enabled. */
> > static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> > {
> > u32 cur_state;
> > + int sleep_switch = 0;
> > int ret = 0;
> >
> > if (pwrdm == NULL || IS_ERR(pwrdm))
> > @@ -259,8 +244,12 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> > if (cur_state == state)
> > return ret;
> >
> > - pwrdm_for_each_clkdm(pwrdm, _clkdm_deny_idle);
> > -
> > + if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
> > + omap2_clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
> > + sleep_switch = 1;
> > + pwrdm_wait_transition(pwrdm);
> > + }
> > +
> > ret = pwrdm_set_next_pwrst(pwrdm, state);
> > if (ret) {
> > printk(KERN_ERR "Unable to set state of powerdomain: %s\n",
> > @@ -268,7 +257,10 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> > goto err;
> > }
> >
> > - pwrdm_for_each_clkdm(pwrdm, _clkdm_allow_idle);
> > + if (sleep_switch) {
> > + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
> > + pwrdm_wait_transition(pwrdm);
> > + }
> >
> > err:
> > return ret;
> > @@ -540,6 +532,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
> > return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> > }
> >
> > +static int __init clkdms_setup(struct clockdomain *clkdm)
> > +{
> > + omap2_clkdm_allow_idle(clkdm);
> > + return 0;
> > +}
> > +
> > int __init omap3_pm_init(void)
> > {
> > struct power_state *pwrst;
> > @@ -566,6 +564,8 @@ int __init omap3_pm_init(void)
> > goto err2;
> > }
> >
> > + (void) clkdm_for_each(clkdms_setup);
> > +
> > mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
> > if (mpu_pwrdm == NULL) {
> > printk(KERN_ERR "Failed to get mpu_pwrdm\n");
> > --
> > 1.5.4.3
> >
> > --
> > 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
> >
>
>
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread