[PM-WIP-OPP][PATCH 0/2 v2] opp layer cleanups

[PM-WIP-OPP][PATCH 0/2 v2] opp layer cleanups

[Nishanth Menon]

Opp layer cleanup series - tries the following:
a) Remove the BUG_ON being used on functional statements
	(included comments from https://patchwork.kernel.org/patch/86793/)
b) Improve the voltage conversion routine a bit (unchanged)

Nishanth Menon (1):
  omap3: pm: cpufreq: BUG_ON cleanup

Phil Carmody (1):
  omap: pm: opp: twl: use DIV_ROUND_UP

 arch/arm/mach-omap2/cpufreq34xx.c |   36 ++++++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/omap3-opp.h   |    5 +++--
 arch/arm/plat-omap/opp_twl_tps.c  |    2 +-
 3 files changed, 36 insertions(+), 7 deletions(-)

Note:
I have dropped ability to store data per opp and srf hardcoded opp
depedency from this series to help the opp layer cleanups for mainline

Patches Tested on:
	SDP3430 and SDP3630

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Regards,
Nishanth Menon

[PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

BUG_ON should not ideally contain a functional code.
Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2

To do this, we change the return of omap3_pm_init_opp from
void to int and return back error value for caller to adequately
handle further decisions. to reduce code duplication, the
registration and error handling are done in loop now.

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
Ref:
v1: https://patchwork.kernel.org/patch/86793/
v2 changes:
	removed BUG_ON entirely. instead have introduced int
	return value allowing for board files which call to
	handle the return results intelligently.

 arch/arm/mach-omap2/cpufreq34xx.c |   36 ++++++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/omap3-opp.h   |    5 +++--
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
index 189c42e..01cf98f 100644
--- a/arch/arm/mach-omap2/cpufreq34xx.c
+++ b/arch/arm/mach-omap2/cpufreq34xx.c
@@ -25,6 +25,7 @@
 
 #include <plat/opp.h>
 #include <plat/cpu.h>
+#include "omap3-opp.h"
 
 static struct omap_opp_def __initdata omap34xx_mpu_rate_table[] = {
 	/* OPP1 */
@@ -109,8 +110,9 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
 	OMAP_OPP_DEF(0, 0, 0)
 };
 
-void __init omap3_pm_init_opp_table(void)
+int __init omap3_pm_init_opp_table(void)
 {
+	int i, r;
 	struct omap_opp_def **omap3_opp_def_list;
 	struct omap_opp_def *omap34xx_opp_def_list[] = {
 		omap34xx_mpu_rate_table,
@@ -122,12 +124,38 @@ void __init omap3_pm_init_opp_table(void)
 		omap36xx_l3_rate_table,
 		omap36xx_dsp_rate_table
 	};
+	enum opp_t omap3_opps[] = {
+		OPP_MPU,
+		OPP_L3,
+		OPP_DSP
+	};
 
 	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
 				omap34xx_opp_def_list;
 
-	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
-	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
-	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
+	for (i = 0; i < ARRAY_SIZE(omap3_opps); i++) {
+		r = opp_init_list(omap3_opps[i], omap3_opp_def_list[i]);
+		if (r)
+			break;
+	}
+	if (!r)
+		return 0;
+
+	/* Cascading error handling - disable all enabled OPPs */
+	pr_err("%s: Failed to register %d OPP type\n", __func__,
+		omap3_opps[i]);
+	i--;
+	while (i != -1) {
+		struct omap_opp *opp;
+		unsigned long freq = 0;
+		while (!IS_ERR(opp = opp_find_freq_ceil(omap3_opps[i],
+							&freq))) {
+			opp_disable(opp);
+			freq++;
+		}
+		i--;
+	}
+
+	return r;
 }
 
diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index 1ba85fc..3e88d8c 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -9,10 +9,11 @@
  * table after the basic initialization
  */
 #ifdef CONFIG_CPU_FREQ
-extern void omap3_pm_init_opp_table(void);
+extern int omap3_pm_init_opp_table(void);
 #else
-static inline void omap3_pm_init_opp_table(void)
+static inline int omap3_pm_init_opp_table(void)
 {
+	return 0;
 }
 #endif
 
-- 
1.6.3.3

[PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP

[Nishanth Menon]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

kernel.h provides a function for DIV_ROUND_UP which should be used
instead of replicating it in code.

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 arch/arm/plat-omap/opp_twl_tps.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/opp_twl_tps.c b/arch/arm/plat-omap/opp_twl_tps.c
index 468fb97..112f106 100644
--- a/arch/arm/plat-omap/opp_twl_tps.c
+++ b/arch/arm/plat-omap/opp_twl_tps.c
@@ -37,5 +37,5 @@ unsigned long omap_twl_vsel_to_uv(const u8 vsel)
 u8 omap_twl_uv_to_vsel(unsigned long uv)
 {
 	/* Round up to higher voltage */
-	return (((uv + 99) / 100 - 6000) + 124) / 125;
+	return DIV_ROUND_UP(uv - 600000, 12500);
 }
-- 
1.6.3.3

Re: [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> BUG_ON should not ideally contain a functional code.
> Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2
>
> To do this, we change the return of omap3_pm_init_opp from
> void to int and return back error value for caller to adequately
> handle further decisions. to reduce code duplication, the
> registration and error handling are done in loop now.
>
> Cc: Ambresh K <ambresh@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Tero Kristo <tero.kristo@nokia.com>
> Cc: Thara Gopinath <thara@ti.com>
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Ref:
> v1: https://patchwork.kernel.org/patch/86793/
> v2 changes:
> 	removed BUG_ON entirely. instead have introduced int
> 	return value allowing for board files which call to
> 	handle the return results intelligently.

Thanks, I like this better.

>  arch/arm/mach-omap2/cpufreq34xx.c |   36 ++++++++++++++++++++++++++++++++----
>  arch/arm/mach-omap2/omap3-opp.h   |    5 +++--
>  2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
> index 189c42e..01cf98f 100644
> --- a/arch/arm/mach-omap2/cpufreq34xx.c
> +++ b/arch/arm/mach-omap2/cpufreq34xx.c
> @@ -25,6 +25,7 @@
>  
>  #include <plat/opp.h>
>  #include <plat/cpu.h>
> +#include "omap3-opp.h"
>  
>  static struct omap_opp_def __initdata omap34xx_mpu_rate_table[] = {
>  	/* OPP1 */
> @@ -109,8 +110,9 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
>  	OMAP_OPP_DEF(0, 0, 0)
>  };
>  
> -void __init omap3_pm_init_opp_table(void)
> +int __init omap3_pm_init_opp_table(void)
>  {
> +	int i, r;
>  	struct omap_opp_def **omap3_opp_def_list;
>  	struct omap_opp_def *omap34xx_opp_def_list[] = {
>  		omap34xx_mpu_rate_table,
> @@ -122,12 +124,38 @@ void __init omap3_pm_init_opp_table(void)
>  		omap36xx_l3_rate_table,
>  		omap36xx_dsp_rate_table
>  	};
> +	enum opp_t omap3_opps[] = {
> +		OPP_MPU,
> +		OPP_L3,
> +		OPP_DSP
> +	};

Aren't these already defined in <plat/opp.h> ?

>  
>  	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>  				omap34xx_opp_def_list;
>  
> -	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
> -	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
> -	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
> +	for (i = 0; i < ARRAY_SIZE(omap3_opps); i++) {
> +		r = opp_init_list(omap3_opps[i], omap3_opp_def_list[i]);
> +		if (r)
> +			break;
> +	}
> +	if (!r)
> +		return 0;
> +
> +	/* Cascading error handling - disable all enabled OPPs */
> +	pr_err("%s: Failed to register %d OPP type\n", __func__,
> +		omap3_opps[i]);
> +	i--;
> +	while (i != -1) {
> +		struct omap_opp *opp;
> +		unsigned long freq = 0;

insert blank line

> +		while (!IS_ERR(opp = opp_find_freq_ceil(omap3_opps[i],
> +							&freq))) {

for redability, would rather see the line-wrap avoided. 
Just put the IS_ERR() on a separate line.

> +			opp_disable(opp);
> +			freq++;
> +		}
> +		i--;
> +	}
> +
> +	return r;
>  }
>  
> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
> index 1ba85fc..3e88d8c 100644
> --- a/arch/arm/mach-omap2/omap3-opp.h
> +++ b/arch/arm/mach-omap2/omap3-opp.h
> @@ -9,10 +9,11 @@
>   * table after the basic initialization
>   */
>  #ifdef CONFIG_CPU_FREQ
> -extern void omap3_pm_init_opp_table(void);
> +extern int omap3_pm_init_opp_table(void);
>  #else
> -static inline void omap3_pm_init_opp_table(void)
> +static inline int omap3_pm_init_opp_table(void)
>  {
> +	return 0;
>  }
>  #endif
>  
> -- 
> 1.6.3.3

Kevin

Re: [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
>
> kernel.h provides a function for DIV_ROUND_UP which should be used
> instead of replicating it in code.
>
> Cc: Ambresh K <ambresh@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Tero Kristo <tero.kristo@nokia.com>
> Cc: Thara Gopinath <thara@ti.com>
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Nice.  Applied to pm-wip-opp.

Kevin

> ---
>  arch/arm/plat-omap/opp_twl_tps.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/opp_twl_tps.c b/arch/arm/plat-omap/opp_twl_tps.c
> index 468fb97..112f106 100644
> --- a/arch/arm/plat-omap/opp_twl_tps.c
> +++ b/arch/arm/plat-omap/opp_twl_tps.c
> @@ -37,5 +37,5 @@ unsigned long omap_twl_vsel_to_uv(const u8 vsel)
>  u8 omap_twl_uv_to_vsel(unsigned long uv)
>  {
>  	/* Round up to higher voltage */
> -	return (((uv + 99) / 100 - 6000) + 124) / 125;
> +	return DIV_ROUND_UP(uv - 600000, 12500);
>  }
> -- 
> 1.6.3.3

Re: [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

Kevin Hilman had written, on 04/20/2010 06:41 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
[..]

>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>> Ref:
>> v1: https://patchwork.kernel.org/patch/86793/
>> v2 changes:
>> 	removed BUG_ON entirely. instead have introduced int
>> 	return value allowing for board files which call to
>> 	handle the return results intelligently.
> 
> Thanks, I like this better.
> 
thx.. comments follow..

>>  arch/arm/mach-omap2/cpufreq34xx.c |   36 ++++++++++++++++++++++++++++++++----
>>  arch/arm/mach-omap2/omap3-opp.h   |    5 +++--
>>  2 files changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
>> index 189c42e..01cf98f 100644
>> --- a/arch/arm/mach-omap2/cpufreq34xx.c
>> +++ b/arch/arm/mach-omap2/cpufreq34xx.c
[..]

>>  
>> -void __init omap3_pm_init_opp_table(void)
>> +int __init omap3_pm_init_opp_table(void)
>>  {
>> +	int i, r;
>>  	struct omap_opp_def **omap3_opp_def_list;
>>  	struct omap_opp_def *omap34xx_opp_def_list[] = {
>>  		omap34xx_mpu_rate_table,
>> @@ -122,12 +124,38 @@ void __init omap3_pm_init_opp_table(void)
>>  		omap36xx_l3_rate_table,
>>  		omap36xx_dsp_rate_table
>>  	};
>> +	enum opp_t omap3_opps[] = {
>> +		OPP_MPU,
>> +		OPP_L3,
>> +		OPP_DSP
>> +	};
> 
> Aren't these already defined in <plat/opp.h> ?
They are. but without using a array, I cant have a for loop ;).. i 
prefer for loops to replicating code thrice ;)..

if your question is on:
+#include "omap3-opp.h"
the reason is the header include makes sense to keep sparse happy + 
first time i built, there was'nt a crib from build when void became int, 
realized this was needed anyways.. shrug if you would like to split it 
out as a separate patch..

if your point was something else, I missed it.. :(
> 
>>  
>>  	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>>  				omap34xx_opp_def_list;
>>  
>> -	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
>> -	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
>> -	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
>> +	for (i = 0; i < ARRAY_SIZE(omap3_opps); i++) {
>> +		r = opp_init_list(omap3_opps[i], omap3_opp_def_list[i]);
>> +		if (r)
>> +			break;
>> +	}
>> +	if (!r)
>> +		return 0;
>> +
>> +	/* Cascading error handling - disable all enabled OPPs */
>> +	pr_err("%s: Failed to register %d OPP type\n", __func__,
>> +		omap3_opps[i]);
>> +	i--;
>> +	while (i != -1) {
>> +		struct omap_opp *opp;
>> +		unsigned long freq = 0;
> 
> insert blank line
> 
thx..
>> +		while (!IS_ERR(opp = opp_find_freq_ceil(omap3_opps[i],
>> +							&freq))) {
> 
> for redability, would rather see the line-wrap avoided. 
> Just put the IS_ERR() on a separate line.
/me kicks myself for being lazy about this.. i guess i better kick it 
out and clean that loop out.. will do..

[..]

-- 
Regards,
Nishanth Menon