Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state
writes:
>
>
>>-Original Message-
>>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
>>Sent: 08 March, 2010 19:16
>>To: Kristo Tero (Nokia-D/Tampere)
>>Cc: linux-omap@vger.kernel.org
>>Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for
>>suspending to INACTIVE state
>>
>> writes:
>>
>>[...]
>>
>>> True, ancient info there. OFF for example has been supported
>>for ages already.
>>>
>>>>
>>>>
>>>>> + if (state != PWRDM_POWER_INACTIVE)
>>>>> + while (!(pwrdm->pwrsts & (1 << state))) {
>>>>> + if (state == PWRDM_POWER_OFF)
>>>>> + return ret;
>>>>> + state--;
>>>>> + }
>>>>
>>>>I think all powerdomains can be inactive right?
>>>
>>> Yes.
>>>
>>>>I think it would be cleaner to just have all the pwrdm->pwrsts fields
>>>>include intactive as a valid option.
>>>>
>>>>Something like the patch below. IIRC, you did something like this in
>>>>one of the earlier versions of the patch.
>>>
>>> Yeah, something like this was done previously, however Paul did not
>>> like the idea of changing the generic powerdomain code too much so I
>>> dropped it completely. It is now done only via the support functions
>>> in patch #1, and only done for the powerdomains that actually need
>>> it for the cpuidle (mpu/core/neon.) It would be possible to add
>>> support for the rest of the powerdomains also, but I decided to drop
>>> this in favor of getting the patch set in.
>>
>>I'm not proposing changing any of the other powerdomain code. Just
>>changing the PWRSTS_* defines, essentially so that INACTIVE is
>>a valid state.
>>
>>That will eliminate the need for a special check for inactive in this
>>patch.
>
> This is a chicken-egg problem. If you alter the PWRSTS_* defines,
> you need to change implementation of pwrdm_set_next_pwrst() as it
> would accept INACTIVE also, which is not supported by the code right
> now.
OK, I see the chicken-egg problem now.
You're original version is ok with me.
Thanks,
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state
>-Original Message-
>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
>Sent: 08 March, 2010 19:16
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for
>suspending to INACTIVE state
>
> writes:
>
>[...]
>
>> True, ancient info there. OFF for example has been supported
>for ages already.
>>
>>>
>>>
>>>> + if (state != PWRDM_POWER_INACTIVE)
>>>> + while (!(pwrdm->pwrsts & (1 << state))) {
>>>> + if (state == PWRDM_POWER_OFF)
>>>> + return ret;
>>>> + state--;
>>>> + }
>>>
>>>I think all powerdomains can be inactive right?
>>
>> Yes.
>>
>>>I think it would be cleaner to just have all the pwrdm->pwrsts fields
>>>include intactive as a valid option.
>>>
>>>Something like the patch below. IIRC, you did something like this in
>>>one of the earlier versions of the patch.
>>
>> Yeah, something like this was done previously, however Paul did not
>> like the idea of changing the generic powerdomain code too much so I
>> dropped it completely. It is now done only via the support functions
>> in patch #1, and only done for the powerdomains that actually need
>> it for the cpuidle (mpu/core/neon.) It would be possible to add
>> support for the rest of the powerdomains also, but I decided to drop
>> this in favor of getting the patch set in.
>
>I'm not proposing changing any of the other powerdomain code. Just
>changing the PWRSTS_* defines, essentially so that INACTIVE is
>a valid state.
>
>That will eliminate the need for a special check for inactive in this
>patch.
This is a chicken-egg problem. If you alter the PWRSTS_* defines, you need to
change implementation of pwrdm_set_next_pwrst() as it would accept INACTIVE
also, which is not supported by the code right now.
>
>Kevin
>
>>>
>>>diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h
>>>b/arch/arm/plat-omap/include/plat/powerdomain.h
>>>index a1ecd47..c692472 100644
>>>--- a/arch/arm/plat-omap/include/plat/powerdomain.h
>>>+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>>>@@ -31,17 +31,17 @@
>>> #define PWRDM_MAX_PWRSTS4
>>>
>>> /* Powerdomain allowable state bitfields */
>>>-#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF) | \
>>>+#define PWRSTS_ON ((1 << PWRDM_POWER_INACTIVE) | \
>>> (1 << PWRDM_POWER_ON))
>>>
>>>+#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF)
>| PWRSTS_ON)
>>>+
>>> #define PWRSTS_OFF_RET ((1 << PWRDM_POWER_OFF) | \
>>> (1 << PWRDM_POWER_RET))
>>>
>>>-#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET) | \
>>>- (1 << PWRDM_POWER_ON))
>>>-
>>>-#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>>>+#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET)
>| PWRSTS_ON)
>>>
>>>+#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON)
>>>
>>> /* Powerdomain flags */
>>> #define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware
>>>save-and-restore support */
>>>
>--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state
writes:
[...]
> True, ancient info there. OFF for example has been supported for ages already.
>
>>
>>
>>> + if (state != PWRDM_POWER_INACTIVE)
>>> + while (!(pwrdm->pwrsts & (1 << state))) {
>>> + if (state == PWRDM_POWER_OFF)
>>> + return ret;
>>> + state--;
>>> + }
>>
>>I think all powerdomains can be inactive right?
>
> Yes.
>
>>I think it would be cleaner to just have all the pwrdm->pwrsts fields
>>include intactive as a valid option.
>>
>>Something like the patch below. IIRC, you did something like this in
>>one of the earlier versions of the patch.
>
> Yeah, something like this was done previously, however Paul did not
> like the idea of changing the generic powerdomain code too much so I
> dropped it completely. It is now done only via the support functions
> in patch #1, and only done for the powerdomains that actually need
> it for the cpuidle (mpu/core/neon.) It would be possible to add
> support for the rest of the powerdomains also, but I decided to drop
> this in favor of getting the patch set in.
I'm not proposing changing any of the other powerdomain code. Just
changing the PWRSTS_* defines, essentially so that INACTIVE is
a valid state.
That will eliminate the need for a special check for inactive in this
patch.
Kevin
>>
>>diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h
>>b/arch/arm/plat-omap/include/plat/powerdomain.h
>>index a1ecd47..c692472 100644
>>--- a/arch/arm/plat-omap/include/plat/powerdomain.h
>>+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>>@@ -31,17 +31,17 @@
>> #define PWRDM_MAX_PWRSTS 4
>>
>> /* Powerdomain allowable state bitfields */
>>-#define PWRSTS_OFF_ON((1 << PWRDM_POWER_OFF) | \
>>+#define PWRSTS_ON((1 << PWRDM_POWER_INACTIVE) | \
>> (1 << PWRDM_POWER_ON))
>>
>>+#define PWRSTS_OFF_ON((1 << PWRDM_POWER_OFF) | PWRSTS_ON)
>>+
>> #define PWRSTS_OFF_RET ((1 << PWRDM_POWER_OFF) | \
>> (1 << PWRDM_POWER_RET))
>>
>>-#define PWRSTS_RET_ON((1 << PWRDM_POWER_RET) | \
>>- (1 << PWRDM_POWER_ON))
>>-
>>-#define PWRSTS_OFF_RET_ON(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>>+#define PWRSTS_RET_ON((1 << PWRDM_POWER_RET) | PWRSTS_ON)
>>
>>+#define PWRSTS_OFF_RET_ON(PWRSTS_OFF_RET | PWRSTS_ON)
>>
>> /* Powerdomain flags */
>> #define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware
>>save-and-restore support */
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state
>-Original Message-
>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
>Sent: 02 March, 2010 01:43
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv6 9/9] OMAP3: PM: Added support for
>suspending to INACTIVE state
>
>Tero Kristo writes:
>
>> From: Tero Kristo
>>
>> With the new support functions this is now possible.
>Suspending to INACTIVE
>> is useful for testing purposes.
>>
>> Signed-off-by: Tero Kristo
>> ---
>> arch/arm/mach-omap2/pm34xx.c | 11 ++-
>> 1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c
>b/arch/arm/mach-omap2/pm34xx.c
>> index cdbedcf..41d6cc3 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -633,11 +633,12 @@ int set_pwrdm_state(struct powerdomain
>*pwrdm, u32 state)
>> if (pwrdm == NULL || IS_ERR(pwrdm))
>> return -EINVAL;
>>
>> -while (!(pwrdm->pwrsts & (1 << state))) {
>> -if (state == PWRDM_POWER_OFF)
>> -return ret;
>> -state--;
>> -}
>
>The comment above set_pwrdm_state() says only ON & RET are supported.
>Please update that comment as well.
True, ancient info there. OFF for example has been supported for ages already.
>
>
>> +if (state != PWRDM_POWER_INACTIVE)
>> +while (!(pwrdm->pwrsts & (1 << state))) {
>> +if (state == PWRDM_POWER_OFF)
>> +return ret;
>> +state--;
>> +}
>
>I think all powerdomains can be inactive right?
Yes.
>I think it would be cleaner to just have all the pwrdm->pwrsts fields
>include intactive as a valid option.
>
>Something like the patch below. IIRC, you did something like this in
>one of the earlier versions of the patch.
Yeah, something like this was done previously, however Paul did not like the
idea of changing the generic powerdomain code too much so I dropped it
completely. It is now done only via the support functions in patch #1, and only
done for the powerdomains that actually need it for the cpuidle
(mpu/core/neon.) It would be possible to add support for the rest of the
powerdomains also, but I decided to drop this in favor of getting the patch set
in.
>
>Kevin
>
>diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h
>b/arch/arm/plat-omap/include/plat/powerdomain.h
>index a1ecd47..c692472 100644
>--- a/arch/arm/plat-omap/include/plat/powerdomain.h
>+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>@@ -31,17 +31,17 @@
> #define PWRDM_MAX_PWRSTS 4
>
> /* Powerdomain allowable state bitfields */
>-#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF) | \
>+#define PWRSTS_ON ((1 << PWRDM_POWER_INACTIVE) | \
>(1 << PWRDM_POWER_ON))
>
>+#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF) | PWRSTS_ON)
>+
> #define PWRSTS_OFF_RET((1 << PWRDM_POWER_OFF) | \
>(1 << PWRDM_POWER_RET))
>
>-#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET) | \
>- (1 << PWRDM_POWER_ON))
>-
>-#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>+#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET) | PWRSTS_ON)
>
>+#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON)
>
> /* Powerdomain flags */
> #define PWRDM_HAS_HDWR_SAR(1 << 0) /* hardware
>save-and-restore support */
>--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state
Tero Kristo writes:
> From: Tero Kristo
>
> With the new support functions this is now possible. Suspending to INACTIVE
> is useful for testing purposes.
>
> Signed-off-by: Tero Kristo
> ---
> arch/arm/mach-omap2/pm34xx.c | 11 ++-
> 1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index cdbedcf..41d6cc3 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -633,11 +633,12 @@ int set_pwrdm_state(struct powerdomain *pwrdm, u32
> state)
> if (pwrdm == NULL || IS_ERR(pwrdm))
> return -EINVAL;
>
> - while (!(pwrdm->pwrsts & (1 << state))) {
> - if (state == PWRDM_POWER_OFF)
> - return ret;
> - state--;
> - }
The comment above set_pwrdm_state() says only ON & RET are supported.
Please update that comment as well.
> + if (state != PWRDM_POWER_INACTIVE)
> + while (!(pwrdm->pwrsts & (1 << state))) {
> + if (state == PWRDM_POWER_OFF)
> + return ret;
> + state--;
> + }
I think all powerdomains can be inactive right?
I think it would be cleaner to just have all the pwrdm->pwrsts fields
include intactive as a valid option.
Something like the patch below. IIRC, you did something like this in
one of the earlier versions of the patch.
Kevin
diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h
b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
#define PWRDM_MAX_PWRSTS 4
/* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF) | \
+#define PWRSTS_ON ((1 << PWRDM_POWER_INACTIVE) | \
(1 << PWRDM_POWER_ON))
+#define PWRSTS_OFF_ON ((1 << PWRDM_POWER_OFF) | PWRSTS_ON)
+
#define PWRSTS_OFF_RET ((1 << PWRDM_POWER_OFF) | \
(1 << PWRDM_POWER_RET))
-#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET) | \
-(1 << PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
+#define PWRSTS_RET_ON ((1 << PWRDM_POWER_RET) | PWRSTS_ON)
+#define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON)
/* Powerdomain flags */
#define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware save-and-restore support */
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
