* [PM-WIP-OPP][PATCH 0/2 v2] opp layer cleanups @ 2010-04-20 19:49 Nishanth Menon 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Nishanth Menon 0 siblings, 1 reply; 6+ messages in thread From: Nishanth Menon @ 2010-04-20 19:49 UTC (permalink / raw) To: linux-omap Cc: Nishanth Menon, Ambresh K, Benoit Cousson, Eduardo Valentin, Kevin Hilman, Phil Carmody, Sanjeev Premi, Tero Kristo, Thara Gopinath 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup 2010-04-20 19:49 [PM-WIP-OPP][PATCH 0/2 v2] opp layer cleanups Nishanth Menon @ 2010-04-20 19:49 ` Nishanth Menon 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP Nishanth Menon 2010-04-20 23:41 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Kevin Hilman 0 siblings, 2 replies; 6+ messages in thread From: Nishanth Menon @ 2010-04-20 19:49 UTC (permalink / raw) To: linux-omap Cc: Nishanth Menon, Ambresh K, Benoit Cousson, Eduardo Valentin, Kevin Hilman, Phil Carmody, Sanjeev Premi, Tero Kristo, Thara Gopinath 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Nishanth Menon @ 2010-04-20 19:49 ` Nishanth Menon 2010-04-20 23:42 ` Kevin Hilman 2010-04-20 23:41 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Kevin Hilman 1 sibling, 1 reply; 6+ messages in thread From: Nishanth Menon @ 2010-04-20 19:49 UTC (permalink / raw) To: linux-omap Cc: Phil Carmody, Ambresh K, Benoit Cousson, Eduardo Valentin, Kevin Hilman, Sanjeev Premi, Tero Kristo, Thara Gopinath, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP Nishanth Menon @ 2010-04-20 23:42 ` Kevin Hilman 0 siblings, 0 replies; 6+ messages in thread From: Kevin Hilman @ 2010-04-20 23:42 UTC (permalink / raw) To: Nishanth Menon Cc: linux-omap, Phil Carmody, Ambresh K, Benoit Cousson, Eduardo Valentin, Sanjeev Premi, Tero Kristo, Thara Gopinath 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Nishanth Menon 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP Nishanth Menon @ 2010-04-20 23:41 ` Kevin Hilman 2010-04-20 23:48 ` Nishanth Menon 1 sibling, 1 reply; 6+ messages in thread From: Kevin Hilman @ 2010-04-20 23:41 UTC (permalink / raw) To: Nishanth Menon Cc: linux-omap, Ambresh K, Benoit Cousson, Eduardo Valentin, Phil Carmody, Sanjeev Premi, Tero Kristo, Thara Gopinath 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup 2010-04-20 23:41 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Kevin Hilman @ 2010-04-20 23:48 ` Nishanth Menon 0 siblings, 0 replies; 6+ messages in thread From: Nishanth Menon @ 2010-04-20 23:48 UTC (permalink / raw) To: Kevin Hilman Cc: linux-omap, K, Ambresh, Cousson, Benoit, Eduardo Valentin, Phil Carmody, Premi, Sanjeev, Tero Kristo, Gopinath, Thara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-20 23:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-04-20 19:49 [PM-WIP-OPP][PATCH 0/2 v2] opp layer cleanups Nishanth Menon 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Nishanth Menon 2010-04-20 19:49 ` [PM-WIP-OPP][PATCH 2/2] omap: pm: opp: twl: use DIV_ROUND_UP Nishanth Menon 2010-04-20 23:42 ` Kevin Hilman 2010-04-20 23:41 ` [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Kevin Hilman 2010-04-20 23:48 ` Nishanth Menon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox