[PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Paul Walmsley]

The SGX device on OMAP3 does not support retention, so remove RET from the 
list of possible SGX power states.  Problem debugged by Richard Woodruff 
<r-woodruff2@ti.com>.
    
Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>

diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
index 446a1ed..edfad42 100644
--- a/arch/arm/mach-omap2/powerdomains34xx.h
+++ b/arch/arm/mach-omap2/powerdomains34xx.h
@@ -255,6 +255,11 @@ static struct powerdomain dss_pwrdm = {
 	},
 };
 
+/*
+ * Although the 34XX TRM Rev K Table 4-371 notes that retention is a
+ * possible SGX powerstate, the SGX device itself does not support
+ * retention.
+ */
 static struct powerdomain sgx_pwrdm = {
 	.name		  = "sgx_pwrdm",
 	.prcm_offs	  = OMAP3430ES2_SGX_MOD,
@@ -262,7 +267,7 @@ static struct powerdomain sgx_pwrdm = {
 	.wkdep_srcs	  = gfx_sgx_wkdeps,
 	.sleepdep_srcs	  = cam_gfx_sleepdeps,
 	/* XXX This is accurate for 3430 SGX, but what about GFX? */
-	.pwrsts		  = PWRSTS_OFF_RET_ON,
+	.pwrsts		  = PWRSTS_OFF_ON,
 	.pwrsts_logic_ret = PWRDM_POWER_RET,
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [081105 11:39]:
> 
> The SGX device on OMAP3 does not support retention, so remove RET from the 
> list of possible SGX power states.  Problem debugged by Richard Woodruff 
> <r-woodruff2@ti.com>.

Pushing.

Tony

> Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> 
> diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
> index 446a1ed..edfad42 100644
> --- a/arch/arm/mach-omap2/powerdomains34xx.h
> +++ b/arch/arm/mach-omap2/powerdomains34xx.h
> @@ -255,6 +255,11 @@ static struct powerdomain dss_pwrdm = {
>  	},
>  };
>  
> +/*
> + * Although the 34XX TRM Rev K Table 4-371 notes that retention is a
> + * possible SGX powerstate, the SGX device itself does not support
> + * retention.
> + */
>  static struct powerdomain sgx_pwrdm = {
>  	.name		  = "sgx_pwrdm",
>  	.prcm_offs	  = OMAP3430ES2_SGX_MOD,
> @@ -262,7 +267,7 @@ static struct powerdomain sgx_pwrdm = {
>  	.wkdep_srcs	  = gfx_sgx_wkdeps,
>  	.sleepdep_srcs	  = cam_gfx_sleepdeps,
>  	/* XXX This is accurate for 3430 SGX, but what about GFX? */
> -	.pwrsts		  = PWRSTS_OFF_RET_ON,
> +	.pwrsts		  = PWRSTS_OFF_ON,
>  	.pwrsts_logic_ret = PWRDM_POWER_RET,
>  	.banks		  = 1,
>  	.pwrsts_mem_ret	  = {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1750 bytes --]

On Wed, 5 Nov 2008, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [081105 11:39]:
> > 
> > The SGX device on OMAP3 does not support retention, so remove RET from the 
> > list of possible SGX power states.  Problem debugged by Richard Woodruff 
> > <r-woodruff2@ti.com>.
> 
> Pushing.

Just FYI, that patch prevents the kernel from booting due to a bug in 
pm34xx.c; patch below.

Even with the follwoing patch, the initial power state setup code in 
pm34xx.c will bail out early since it still tries to set the SGX next 
power state to retention, which it does not support.


- Paul


OMAP3 PM: use list_for_each_entry_safe() when deleting list entries

From: Paul Walmsley <paul@pwsan.com>

The error path in clkdms_setup() needs to use list_for_each_entry_safe()
when deleting entries from the list, or the kernel will crash.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Jouni Högander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a11a657..da098d2 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -540,7 +540,7 @@ static int __init clkdms_setup(struct clockdomain *clkdm)
 
 int __init omap3_pm_init(void)
 {
-	struct power_state *pwrst;
+	struct power_state *pwrst, *tmp;
 	int ret;
 
 	printk(KERN_ERR "Power Management for TI OMAP3.\n");
@@ -583,7 +583,7 @@ err1:
 	return ret;
 err2:
 	free_irq(INT_34XX_PRCM_MPU_IRQ, NULL);
-	list_for_each_entry(pwrst, &pwrst_list, node) {
+	list_for_each_entry_safe(pwrst, tmp, &pwrst_list, node) {
 		list_del(&pwrst->node);
 		kfree(pwrst);
 	}

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [081106 03:09]:
> On Wed, 5 Nov 2008, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [081105 11:39]:
> > > 
> > > The SGX device on OMAP3 does not support retention, so remove RET from the 
> > > list of possible SGX power states.  Problem debugged by Richard Woodruff 
> > > <r-woodruff2@ti.com>.
> > 
> > Pushing.
> 
> Just FYI, that patch prevents the kernel from booting due to a bug in 
> pm34xx.c; patch below.
> 
> Even with the follwoing patch, the initial power state setup code in 
> pm34xx.c will bail out early since it still tries to set the SGX next 
> power state to retention, which it does not support.

Pushing.

Tony

> 
> 
> - Paul
> 
> 
> OMAP3 PM: use list_for_each_entry_safe() when deleting list entries
> 
> From: Paul Walmsley <paul@pwsan.com>
> 
> The error path in clkdms_setup() needs to use list_for_each_entry_safe()
> when deleting entries from the list, or the kernel will crash.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Jouni Högander <jouni.hogander@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index a11a657..da098d2 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -540,7 +540,7 @@ static int __init clkdms_setup(struct clockdomain *clkdm)
>  
>  int __init omap3_pm_init(void)
>  {
> -	struct power_state *pwrst;
> +	struct power_state *pwrst, *tmp;
>  	int ret;
>  
>  	printk(KERN_ERR "Power Management for TI OMAP3.\n");
> @@ -583,7 +583,7 @@ err1:
>  	return ret;
>  err2:
>  	free_irq(INT_34XX_PRCM_MPU_IRQ, NULL);
> -	list_for_each_entry(pwrst, &pwrst_list, node) {
> +	list_for_each_entry_safe(pwrst, tmp, &pwrst_list, node) {
>  		list_del(&pwrst->node);
>  		kfree(pwrst);
>  	}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Högander Jouni]

"ext Paul Walmsley" <paul@pwsan.com> writes:

> On Wed, 5 Nov 2008, Tony Lindgren wrote:
>
>> * Paul Walmsley <paul@pwsan.com> [081105 11:39]:
>> > 
>> > The SGX device on OMAP3 does not support retention, so remove RET from the 
>> > list of possible SGX power states.  Problem debugged by Richard Woodruff 
>> > <r-woodruff2@ti.com>.
>> 
>> Pushing.
>
> Just FYI, that patch prevents the kernel from booting due to a bug in 
> pm34xx.c; patch below.
>
> Even with the follwoing patch, the initial power state setup code in 
> pm34xx.c will bail out early since it still tries to set the SGX next 
> power state to retention, which it does not support.

What do you Paul think about patch below:

From: Jouni Hogander <jouni.hogander@nokia.com>
Date: Fri, 7 Nov 2008 16:50:51 +0200
Subject: [PATCH] OMAP3: PM: Check that wanted state is supported by pwrdm in pwrdms_setup

Check that wanted sleep state is supported by powerdomain. If it is
not supported, then use next highest supported state.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index da098d2..d9959a8 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -515,6 +515,7 @@ static void __init prcm_setup_regs(void)
 static int __init pwrdms_setup(struct powerdomain *pwrdm)
 {
 	struct power_state *pwrst;
+	u32 next_state = PWRDM_POWER_RET;
 
 	if (!pwrdm->pwrsts)
 		return 0;
@@ -523,12 +524,20 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
 	if (!pwrst)
 		return -ENOMEM;
 	pwrst->pwrdm = pwrdm;
-	pwrst->next_state = PWRDM_POWER_RET;
 	list_add(&pwrst->node, &pwrst_list);
 
 	if (pwrdm_has_hdwr_sar(pwrdm))
 		pwrdm_enable_hdwr_sar(pwrdm);
 
+	while (!(pwrdm->pwrsts & (1 << next_state))) {
+		if (next_state > PWRDM_POWER_ON) {
+			next_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
+			break;
+		}
+		next_state++;
+	}
+	pwrst->next_state = next_state;
+
 	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
 }
 
-- 
1.6.0.1

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2082 bytes --]

Hi Jouni,

On Fri, 7 Nov 2008, Högander Jouni wrote:

> What do you Paul think about patch below:

I'm okay with it, but one potential problem: won't this prevent the chip 
from entering retention, since SGX will be set to ON?

Anyway, if you agree this is a problem, what do you think about adding a 
powerdomain flag that indicates that the powerdomain next power state 
should be set to OFF on initialization, and then testing that in 
set_pwrdm_state() ?

- Paul


> From: Jouni Hogander <jouni.hogander@nokia.com>
> Date: Fri, 7 Nov 2008 16:50:51 +0200
> Subject: [PATCH] OMAP3: PM: Check that wanted state is supported by pwrdm in pwrdms_setup
> 
> Check that wanted sleep state is supported by powerdomain. If it is
> not supported, then use next highest supported state.
> 
> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index da098d2..d9959a8 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -515,6 +515,7 @@ static void __init prcm_setup_regs(void)
>  static int __init pwrdms_setup(struct powerdomain *pwrdm)
>  {
>  	struct power_state *pwrst;
> +	u32 next_state = PWRDM_POWER_RET;
>  
>  	if (!pwrdm->pwrsts)
>  		return 0;
> @@ -523,12 +524,20 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
>  	if (!pwrst)
>  		return -ENOMEM;
>  	pwrst->pwrdm = pwrdm;
> -	pwrst->next_state = PWRDM_POWER_RET;
>  	list_add(&pwrst->node, &pwrst_list);
>  
>  	if (pwrdm_has_hdwr_sar(pwrdm))
>  		pwrdm_enable_hdwr_sar(pwrdm);
>  
> +	while (!(pwrdm->pwrsts & (1 << next_state))) {
> +		if (next_state > PWRDM_POWER_ON) {
> +			next_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
> +			break;
> +		}
> +		next_state++;
> +	}
> +	pwrst->next_state = next_state;
> +
>  	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>  }
>  
> -- 
> 1.6.0.1
> 
> 


- Paul

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Högander Jouni]

"ext Paul Walmsley" <paul@pwsan.com> writes:

> Hi Jouni,
>
> On Fri, 7 Nov 2008, Högander Jouni wrote:
>
>> What do you Paul think about patch below:
>
> I'm okay with it, but one potential problem: won't this prevent the chip 
> from entering retention, since SGX will be set to ON?

Yes, you are right here.

>
> Anyway, if you agree this is a problem, what do you think about adding a 
> powerdomain flag that indicates that the powerdomain next power state 
> should be set to OFF on initialization, and then testing that in 
> set_pwrdm_state() ?

I wouldn't add any flags for this. The goal is finally to set all
next_states as OFF until someone has set some constraint which
prevents OFF usage. For now we need to use RET as default, because
drivers are not supporting OFF mode. Do you agree this?

Easiest way here would be to add own hook for SGX in pwrdms_setup? One
more strcmp("*_pwrdm, pwrdm->name) :)

What do you think?

>
> - Paul
>
>
>> From: Jouni Hogander <jouni.hogander@nokia.com>
>> Date: Fri, 7 Nov 2008 16:50:51 +0200
>> Subject: [PATCH] OMAP3: PM: Check that wanted state is supported by pwrdm in pwrdms_setup
>> 
>> Check that wanted sleep state is supported by powerdomain. If it is
>> not supported, then use next highest supported state.
>> 
>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>> ---
>>  arch/arm/mach-omap2/pm34xx.c |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index da098d2..d9959a8 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -515,6 +515,7 @@ static void __init prcm_setup_regs(void)
>>  static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>  {
>>  	struct power_state *pwrst;
>> +	u32 next_state = PWRDM_POWER_RET;
>>  
>>  	if (!pwrdm->pwrsts)
>>  		return 0;
>> @@ -523,12 +524,20 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>  	if (!pwrst)
>>  		return -ENOMEM;
>>  	pwrst->pwrdm = pwrdm;
>> -	pwrst->next_state = PWRDM_POWER_RET;
>>  	list_add(&pwrst->node, &pwrst_list);
>>  
>>  	if (pwrdm_has_hdwr_sar(pwrdm))
>>  		pwrdm_enable_hdwr_sar(pwrdm);
>>  
>> +	while (!(pwrdm->pwrsts & (1 << next_state))) {
>> +		if (next_state > PWRDM_POWER_ON) {
>> +			next_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
>> +			break;
>> +		}
>> +		next_state++;
>> +	}
>> +	pwrst->next_state = next_state;
>> +
>>  	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>>  }
>>  
>> -- 
>> 1.6.0.1
>> 
>> 
>
>
> - Paul

-- 
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: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Kevin Hilman]

Högander Jouni wrote:
> "ext Paul Walmsley" <paul@pwsan.com> writes:
> 
>> Hi Jouni,
>>
>> On Fri, 7 Nov 2008, Högander Jouni wrote:
>>
>>> What do you Paul think about patch below:
>> I'm okay with it, but one potential problem: won't this prevent the chip 
>> from entering retention, since SGX will be set to ON?
> 
> Yes, you are right here.
> 
>> Anyway, if you agree this is a problem, what do you think about adding a 
>> powerdomain flag that indicates that the powerdomain next power state 
>> should be set to OFF on initialization, and then testing that in 
>> set_pwrdm_state() ?
> 
> I wouldn't add any flags for this. The goal is finally to set all
> next_states as OFF until someone has set some constraint which
> prevents OFF usage. For now we need to use RET as default, because
> drivers are not supporting OFF mode. Do you agree this?
> 
> Easiest way here would be to add own hook for SGX in pwrdms_setup? One
> more strcmp("*_pwrdm, pwrdm->name) :)
> 
> What do you think?
> 

Personally, I don't like these if statements (or ifdefs) in pwrdms_setup 
or the off_mode_enable hook.  And I would like to see them all disappear.

I would rather see set_pwrdm_state() be smarter by simply not trying
to use a state that is not in its list of allowed states.

Kevin


>> - Paul
>>
>>
>>> From: Jouni Hogander <jouni.hogander@nokia.com>
>>> Date: Fri, 7 Nov 2008 16:50:51 +0200
>>> Subject: [PATCH] OMAP3: PM: Check that wanted state is supported by pwrdm in pwrdms_setup
>>>
>>> Check that wanted sleep state is supported by powerdomain. If it is
>>> not supported, then use next highest supported state.
>>>
>>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>>> ---
>>>  arch/arm/mach-omap2/pm34xx.c |   11 ++++++++++-
>>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>>> index da098d2..d9959a8 100644
>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>> @@ -515,6 +515,7 @@ static void __init prcm_setup_regs(void)
>>>  static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>>  {
>>>  	struct power_state *pwrst;
>>> +	u32 next_state = PWRDM_POWER_RET;
>>>  
>>>  	if (!pwrdm->pwrsts)
>>>  		return 0;
>>> @@ -523,12 +524,20 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>>  	if (!pwrst)
>>>  		return -ENOMEM;
>>>  	pwrst->pwrdm = pwrdm;
>>> -	pwrst->next_state = PWRDM_POWER_RET;
>>>  	list_add(&pwrst->node, &pwrst_list);
>>>  
>>>  	if (pwrdm_has_hdwr_sar(pwrdm))
>>>  		pwrdm_enable_hdwr_sar(pwrdm);
>>>  
>>> +	while (!(pwrdm->pwrsts & (1 << next_state))) {
>>> +		if (next_state > PWRDM_POWER_ON) {
>>> +			next_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
>>> +			break;
>>> +		}
>>> +		next_state++;
>>> +	}
>>> +	pwrst->next_state = next_state;
>>> +
>>>  	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>>>  }
>>>  
>>> -- 
>>> 1.6.0.1
>>>
>>>
>>
>> - 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

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Högander Jouni]

"ext Kevin Hilman" <khilman@deeprootsystems.com> writes:

> Högander Jouni wrote:
>> "ext Paul Walmsley" <paul@pwsan.com> writes:
>>
>>> Hi Jouni,
>>>
>>> On Fri, 7 Nov 2008, Högander Jouni wrote:
>>>
>>>> What do you Paul think about patch below:
>>> I'm okay with it, but one potential problem: won't this prevent the
>>> chip from entering retention, since SGX will be set to ON?
>>
>> Yes, you are right here.
>>
>>> Anyway, if you agree this is a problem, what do you think about
>>> adding a powerdomain flag that indicates that the powerdomain next
>>> power state should be set to OFF on initialization, and then
>>> testing that in set_pwrdm_state() ?
>>
>> I wouldn't add any flags for this. The goal is finally to set all
>> next_states as OFF until someone has set some constraint which
>> prevents OFF usage. For now we need to use RET as default, because
>> drivers are not supporting OFF mode. Do you agree this?
>>
>> Easiest way here would be to add own hook for SGX in pwrdms_setup? One
>> more strcmp("*_pwrdm, pwrdm->name) :)
>>
>> What do you think?
>>
>
> Personally, I don't like these if statements (or ifdefs) in
> pwrdms_setup or the off_mode_enable hook.  And I would like to see
> them all disappear.

Should we write all the states in off_mode_enable/pwrdms_setup
and then write them again in resource_refresh/CPUidle loop.

>
> I would rather see set_pwrdm_state() be smarter by simply not trying
> to use a state that is not in its list of allowed states.

Should we then just ignore possible error value from set_pwrdm_state?
Or consider unsupported PWRDM_ST as a not an error in set_pwrdm_state? Just
ignore it?

>
> Kevin
>
>
>>> - Paul
>>>
>>>
>>>> From: Jouni Hogander <jouni.hogander@nokia.com>
>>>> Date: Fri, 7 Nov 2008 16:50:51 +0200
>>>> Subject: [PATCH] OMAP3: PM: Check that wanted state is supported by pwrdm in pwrdms_setup
>>>>
>>>> Check that wanted sleep state is supported by powerdomain. If it is
>>>> not supported, then use next highest supported state.
>>>>
>>>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>>>> ---
>>>>  arch/arm/mach-omap2/pm34xx.c |   11 ++++++++++-
>>>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>>>> index da098d2..d9959a8 100644
>>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>>> @@ -515,6 +515,7 @@ static void __init prcm_setup_regs(void)
>>>>  static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>>>  {
>>>>  	struct power_state *pwrst;
>>>> +	u32 next_state = PWRDM_POWER_RET;
>>>>   	if (!pwrdm->pwrsts)
>>>>  		return 0;
>>>> @@ -523,12 +524,20 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm)
>>>>  	if (!pwrst)
>>>>  		return -ENOMEM;
>>>>  	pwrst->pwrdm = pwrdm;
>>>> -	pwrst->next_state = PWRDM_POWER_RET;
>>>>  	list_add(&pwrst->node, &pwrst_list);
>>>>   	if (pwrdm_has_hdwr_sar(pwrdm))
>>>>  		pwrdm_enable_hdwr_sar(pwrdm);
>>>>  +	while (!(pwrdm->pwrsts & (1 << next_state))) {
>>>> +		if (next_state > PWRDM_POWER_ON) {
>>>> +			next_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
>>>> +			break;
>>>> +		}
>>>> +		next_state++;
>>>> +	}
>>>> +	pwrst->next_state = next_state;
>>>> +
>>>>  	return set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>>>>  }
>>>>  -- 
>>>> 1.6.0.1
>>>>
>>>>
>>>
>>> - Paul
>>
>

-- 
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: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 787 bytes --]

Hi Jouni, Kevin,

On Tue, 11 Nov 2008, Högander Jouni wrote:

> I wouldn't add any flags for this. The goal is finally to set all
> next_states as OFF until someone has set some constraint which
> prevents OFF usage. For now we need to use RET as default, because
> drivers are not supporting OFF mode. Do you agree this?

Yes.

> Easiest way here would be to add own hook for SGX in pwrdms_setup? One 
> more strcmp("*_pwrdm, pwrdm->name) :)
> 
> What do you think?

That is okay with me.  This seems to be an unusual case - I guess it's due 
to an SGX firmware bug.  

So Kevin, if you have a strong opposition to that strcmp, maybe we should 
use a powerdomain flag for the SGX pwrdm.  Otherwise, let's go with a 
strcmp as Jouni suggested.  Thoughts?


- Paul

[PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm

[Jouni Hogander]

Check that wanted sleep state is supported by powerdomain. If it is
not supported, then use next lowest supported state.

Check also on suspend that state of pwrdm was lower or equal.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index da098d2..babead5 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -239,8 +239,13 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 	if (pwrdm == NULL || IS_ERR(pwrdm))
 		return -EINVAL;
 
-	cur_state = pwrdm_read_next_pwrst(pwrdm);
+	while (!(pwrdm->pwrsts & (1 << state))) {
+		if (state < PWRDM_POWER_OFF)
+			return ret;
+		state--;
+	}
 
+	cur_state = pwrdm_read_next_pwrst(pwrdm);
 	if (cur_state == state)
 		return ret;
 
@@ -314,7 +319,7 @@ restore:
 	list_for_each_entry(pwrst, &pwrst_list, node) {
 		set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state);
 		state = pwrdm_read_prev_pwrst(pwrst->pwrdm);
-		if (state != pwrst->next_state) {
+		if (state > pwrst->next_state) {
 			printk(KERN_INFO "Powerdomain (%s) didn't enter "
 			       "target state %d\n",
 			       pwrst->pwrdm->name, pwrst->next_state);
-- 
1.6.0.1

Re: [PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm

[Paul Walmsley]

Hi Jouni,

one quick review comment:

On Fri, 14 Nov 2008, Jouni Hogander wrote:

> Check that wanted sleep state is supported by powerdomain. If it is
> not supported, then use next lowest supported state.
> 
> Check also on suspend that state of pwrdm was lower or equal.

...

> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index da098d2..babead5 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -239,8 +239,13 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
>  	if (pwrdm == NULL || IS_ERR(pwrdm))
>  		return -EINVAL;
>  
> -	cur_state = pwrdm_read_next_pwrst(pwrdm);
> +	while (!(pwrdm->pwrsts & (1 << state))) {
> +		if (state < PWRDM_POWER_OFF)
> +			return ret;

This if-statement will never execute, since 'state' is unsigned, and 
PWRDM_POWER_OFF is 0.  Maybe the easiest way to fix it would be to make 
'state' a signed int?

Other than that, looks good to me.


> +		state--;
> +	}
>  
> +	cur_state = pwrdm_read_next_pwrst(pwrdm);
>  	if (cur_state == state)
>  		return ret;


- Paul

Re: [PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm

[Paul Walmsley]

> > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> > index da098d2..babead5 100644
> > --- a/arch/arm/mach-omap2/pm34xx.c
> > +++ b/arch/arm/mach-omap2/pm34xx.c
> > @@ -239,8 +239,13 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
> >  	if (pwrdm == NULL || IS_ERR(pwrdm))
> >  		return -EINVAL;
> >  
> > -	cur_state = pwrdm_read_next_pwrst(pwrdm);
> > +	while (!(pwrdm->pwrsts & (1 << state))) {
> > +		if (state < PWRDM_POWER_OFF)
> > +			return ret;
> 
> This if-statement will never execute, since 'state' is unsigned, and 
> PWRDM_POWER_OFF is 0.  Maybe the easiest way to fix it would be to make 
> 'state' a signed int?
 
Hmm.  That if-statement should go after the 'state--' also.



> Other than that, looks good to me.
> 
> 
> > +		state--;
> > +	}
> >  
> > +	cur_state = pwrdm_read_next_pwrst(pwrdm);
> >  	if (cur_state == state)
> >  		return ret;
> 
> 
> - 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
> 


- Paul

[PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm v2

[Jouni Hogander]

Check that wanted sleep state is supported by powerdomain. If it is
not supported, then use next lowest supported state.

Check also on suspend that state of pwrdm was lower or equal.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index da098d2..9f5a544 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -239,8 +239,13 @@ static int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 	if (pwrdm == NULL || IS_ERR(pwrdm))
 		return -EINVAL;
 
-	cur_state = pwrdm_read_next_pwrst(pwrdm);
+	while (!(pwrdm->pwrsts & (1 << state))) {
+		if (state == PWRDM_POWER_OFF)
+			return ret;
+		state--;
+	}
 
+	cur_state = pwrdm_read_next_pwrst(pwrdm);
 	if (cur_state == state)
 		return ret;
 
@@ -314,7 +319,7 @@ restore:
 	list_for_each_entry(pwrst, &pwrst_list, node) {
 		set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state);
 		state = pwrdm_read_prev_pwrst(pwrst->pwrdm);
-		if (state != pwrst->next_state) {
+		if (state > pwrst->next_state) {
 			printk(KERN_INFO "Powerdomain (%s) didn't enter "
 			       "target state %d\n",
 			       pwrst->pwrdm->name, pwrst->next_state);
-- 
1.6.0.1

Re: [PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm v2

[Paul Walmsley]

On Mon, 17 Nov 2008, Jouni Hogander wrote:

> Check that wanted sleep state is supported by powerdomain. If it is
> not supported, then use next lowest supported state.
> 
> Check also on suspend that state of pwrdm was lower or equal.
> 
> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>

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



- Paul

Re: [PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm v2

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Mon, 17 Nov 2008, Jouni Hogander wrote:
>
>> Check that wanted sleep state is supported by powerdomain. If it is
>> not supported, then use next lowest supported state.
>> 
>> Check also on suspend that state of pwrdm was lower or equal.
>> 
>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>
> Acked-by: Paul Walmsley <paul@pwsan.com>
>

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Re: [PATCH] OMAP3: PM: Check in set_pwrdm_state that target state is supported by pwrdm v2

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [081118 15:56]:
> Paul Walmsley <paul@pwsan.com> writes:
> 
> > On Mon, 17 Nov 2008, Jouni Hogander wrote:
> >
> >> Check that wanted sleep state is supported by powerdomain. If it is
> >> not supported, then use next lowest supported state.
> >> 
> >> Check also on suspend that state of pwrdm was lower or equal.
> >> 
> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> >
> > Acked-by: Paul Walmsley <paul@pwsan.com>
> >
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Pushing this today.

Tony

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Steve Sakoman]

On Wed, Nov 12, 2008 at 11:07 AM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi Jouni, Kevin,
>
> On Tue, 11 Nov 2008, Högander Jouni wrote:
>
>> I wouldn't add any flags for this. The goal is finally to set all
>> next_states as OFF until someone has set some constraint which
>> prevents OFF usage. For now we need to use RET as default, because
>> drivers are not supporting OFF mode. Do you agree this?
>
> Yes.
>
>> Easiest way here would be to add own hook for SGX in pwrdms_setup? One
>> more strcmp("*_pwrdm, pwrdm->name) :)
>>
>> What do you think?
>
> That is okay with me.  This seems to be an unusual case - I guess it's due
> to an SGX firmware bug.
>
> So Kevin, if you have a strong opposition to that strcmp, maybe we should
> use a powerdomain flag for the SGX pwrdm.  Otherwise, let's go with a
> strcmp as Jouni suggested.  Thoughts?

Has there been a final resolution on this?

I am unable to boot reliably on Overo with 2.6.28-rc5 -- almost every
boot attempt gives me:

Power Management for TI OMAP3.
Unable to set state of powerdomain: sgx_pwrdm
Failed to setup powerdomains
omap2|3_pm_init failed: -22

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3 powerdomains: remove RET from SGX power states list

[Kevin Hilman]

"Steve Sakoman" <sakoman@gmail.com> writes:

> On Wed, Nov 12, 2008 at 11:07 AM, Paul Walmsley <paul@pwsan.com> wrote:
>> Hi Jouni, Kevin,
>>
>> On Tue, 11 Nov 2008, Högander Jouni wrote:
>>
>>> I wouldn't add any flags for this. The goal is finally to set all
>>> next_states as OFF until someone has set some constraint which
>>> prevents OFF usage. For now we need to use RET as default, because
>>> drivers are not supporting OFF mode. Do you agree this?
>>
>> Yes.
>>
>>> Easiest way here would be to add own hook for SGX in pwrdms_setup? One
>>> more strcmp("*_pwrdm, pwrdm->name) :)
>>>
>>> What do you think?
>>
>> That is okay with me.  This seems to be an unusual case - I guess it's due
>> to an SGX firmware bug.
>>
>> So Kevin, if you have a strong opposition to that strcmp, maybe we should
>> use a powerdomain flag for the SGX pwrdm.  Otherwise, let's go with a
>> strcmp as Jouni suggested.  Thoughts?
>
> Has there been a final resolution on this?
>
> I am unable to boot reliably on Overo with 2.6.28-rc5 -- almost every
> boot attempt gives me:
>
> Power Management for TI OMAP3.
> Unable to set state of powerdomain: sgx_pwrdm
> Failed to setup powerdomains
> omap2|3_pm_init failed: -22
>

Hi Steve,

Tony just pushed the final version[1] of the fix for this issue.
Please let me know if your problem still persists.

Kevin


[1] see commit 7a54b0f6f39ea1e5dbff6bb47314bea228bf6e44
--
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