[PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[Reddy, Teerth]

>From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
From: Teerth Reddy <teerth@ti.com>
Date: Wed, 9 Sep 2009 11:01:04 +0530
Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue

This patch fixes the VDD2 OPP1 issue. The patch has change
which does not allow VDD2 OPP setting to 1.VDD2 should not be put 
at OPP1 as this is not a supported OPP for VDD2

Signed-off-by: Teerth Reddy <teerth@ti.com>
---
 arch/arm/mach-omap2/pm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index fec7d00..d0e03c4 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
 		}
 		resource_set_opp_level(VDD1_OPP, value, flags);
 	} else if (attr == &vdd2_opp_attr) {
-		if (value < 1 || value > 3) {
+		if (value < 2 || value > 3) {
 			printk(KERN_ERR "vdd_opp_store: Invalid value\n");
 			return -EINVAL;
 		}
-- 
1.5.4.7

Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[Kevin Hilman]

"Reddy, Teerth" <teerth@ti.com> writes:

> From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
> From: Teerth Reddy <teerth@ti.com>
> Date: Wed, 9 Sep 2009 11:01:04 +0530
> Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue
>
> This patch fixes the VDD2 OPP1 issue. The patch has change
> which does not allow VDD2 OPP setting to 1.VDD2 should not be put 
> at OPP1 as this is not a supported OPP for VDD2

Patch looks fine, but shortlog (subject) and changelog should be more
clear.  These should be written for people who are not as familiar
with the code.  For example, seeing this shortlog in a git history
would not be helpful as the irst thing one would ask is "what OPP1
issue?"

How about something like this:

Subject: OMAP3: PM: do not allow OPP1 allow for VDD2

Since OPP1 is not a supported OPP for VDD2, do not allow it to be
changed using the sysfs interface.

Kevin


> Signed-off-by: Teerth Reddy <teerth@ti.com>
> ---
>  arch/arm/mach-omap2/pm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index fec7d00..d0e03c4 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		}
>  		resource_set_opp_level(VDD1_OPP, value, flags);
>  	} else if (attr == &vdd2_opp_attr) {
> -		if (value < 1 || value > 3) {
> +		if (value < 2 || value > 3) {
>  			printk(KERN_ERR "vdd_opp_store: Invalid value\n");
>  			return -EINVAL;
>  		}
> -- 
> 1.5.4.7

Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[Nishanth Menon]

Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:
> From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
> From: Teerth Reddy <teerth@ti.com>
> Date: Wed, 9 Sep 2009 11:01:04 +0530
> Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue
> 
> This patch fixes the VDD2 OPP1 issue. The patch has change
> which does not allow VDD2 OPP setting to 1.VDD2 should not be put 
> at OPP1 as this is not a supported OPP for VDD2
> 
> Signed-off-by: Teerth Reddy <teerth@ti.com>
> ---
>  arch/arm/mach-omap2/pm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index fec7d00..d0e03c4 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		}
>  		resource_set_opp_level(VDD1_OPP, value, flags);
>  	} else if (attr == &vdd2_opp_attr) {
> -		if (value < 1 || value > 3) {
> +		if (value < 2 || value > 3) {
>  			printk(KERN_ERR "vdd_opp_store: Invalid value\n");
>  			return -EINVAL;
>  		}
this is not scalable. we should be able to disable OPPs for each OPP 
from the OPP array. with different silicons, we could have the same OPP 
enabled/disabled. NAK -> need to handle based on mpu_opps[vale].rate ==0

-- 
Regards,
Nishanth Menon

Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:
>> From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
>> From: Teerth Reddy <teerth@ti.com>
>> Date: Wed, 9 Sep 2009 11:01:04 +0530
>> Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue
>>
>> This patch fixes the VDD2 OPP1 issue. The patch has change
>> which does not allow VDD2 OPP setting to 1.VDD2 should not be put at
>> OPP1 as this is not a supported OPP for VDD2
>>
>> Signed-off-by: Teerth Reddy <teerth@ti.com>
>> ---
>>  arch/arm/mach-omap2/pm.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
>> index fec7d00..d0e03c4 100644
>> --- a/arch/arm/mach-omap2/pm.c
>> +++ b/arch/arm/mach-omap2/pm.c
>> @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
>>  		}
>>  		resource_set_opp_level(VDD1_OPP, value, flags);
>>  	} else if (attr == &vdd2_opp_attr) {
>> -		if (value < 1 || value > 3) {
>> +		if (value < 2 || value > 3) {
>>  			printk(KERN_ERR "vdd_opp_store: Invalid value\n");
>>  			return -EINVAL;
>>  		}
> this is not scalable. we should be able to disable OPPs for each OPP
> from the OPP array. with different silicons, we could have the same
> OPP enabled/disabled. NAK -> need to handle based on
> mpu_opps[vale].rate ==0

Agreed that the current code is not scalable, but that was a problem
before Teerth's fix.

The proposed patch is a bugfix to existing code and should be merged
after my comments are addressed.

Then, the scalability issues can be addressed separately.

Kevin

RE: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

[Menon, Nishanth]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Thursday, October 08, 2009 9:58 AM
> To: Menon, Nishanth
> Cc: Reddy, Teerth; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue
> 
> Nishanth Menon <nm@ti.com> writes:
> 
> > Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:
> >> From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
> >> From: Teerth Reddy <teerth@ti.com>
> >> Date: Wed, 9 Sep 2009 11:01:04 +0530
> >> Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue
> >>
> >> This patch fixes the VDD2 OPP1 issue. The patch has change
> >> which does not allow VDD2 OPP setting to 1.VDD2 should not be put at
> >> OPP1 as this is not a supported OPP for VDD2
> >>
> >> Signed-off-by: Teerth Reddy <teerth@ti.com>
> >> ---
> >>  arch/arm/mach-omap2/pm.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> >> index fec7d00..d0e03c4 100644
> >> --- a/arch/arm/mach-omap2/pm.c
> >> +++ b/arch/arm/mach-omap2/pm.c
> >> @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> >>  		}
> >>  		resource_set_opp_level(VDD1_OPP, value, flags);
> >>  	} else if (attr == &vdd2_opp_attr) {
> >> -		if (value < 1 || value > 3) {
> >> +		if (value < 2 || value > 3) {
> >>  			printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> >>  			return -EINVAL;
> >>  		}
> > this is not scalable. we should be able to disable OPPs for each OPP
> > from the OPP array. with different silicons, we could have the same
> > OPP enabled/disabled. NAK -> need to handle based on
> > mpu_opps[vale].rate ==0
> 
> Agreed that the current code is not scalable, but that was a problem
> before Teerth's fix.
> 
> The proposed patch is a bugfix to existing code and should be merged
> after my comments are addressed.
> 
> Then, the scalability issues can be addressed separately.
> 
Ok, reverting my objection.

Will try to do a follow up patch -> essentially let program_opp make that decision IMHO.


Regards,
Nishanth Menon