On Tue, Dec 08, 2009 at 12:31:13PM +0100, ext Nishanth Menon wrote: > Eduardo Valentin said the following on 12/08/2009 05:18 AM: > > On Tue, Dec 08, 2009 at 11:59:49AM +0100, ext Nishanth Menon wrote: > > > >> Hi, > >> thanks for your comments. few thoughts below. > >> > >> Eduardo Valentin said the following on 12/08/2009 01:49 AM: > >> > >>> Hello Nishanth, > >>> > >>> Few comments bellow. > >>> > >>> On Wed, Nov 25, 2009 at 05:09:18AM +0100, ext Nishanth Menon wrote: > >>> > >>> > >>>> Introduce the OMAP3630 OPPs including the defined OPP tuples. > >>>> > >>>> Further information on OMAP3630 can be found here: > >>>> http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?templateId=6123&navigationId=12836&contentId=52606 > >>>> > >>>> OMAP36xx family introduces: > >>>> VDD1 with 4 OPPs, of which OPP3 & 4 are available only on devices yet > >>>> to be introduced in 36xx family. Meanwhile, VDD2 has 2 opps. > >>>> > >>>> Device Support of OPPs-> > >>>> |<-3630-600->| (default) > >>>> |<- 3630-800 ->| (device: TBD) > >>>> |<- 3630-1000 ->| (device: TBD) > >>>> H/w OPP-> OPP50 OPP100 OPP-Turbo OPP1G-SB > >>>> VDD1 OPP1 OPP2 OPP3 OPP4 > >>>> VDD2 OPP1 OPP2 OPP2 OPP2 > >>>> > >>>> Note: > >>>> a) TI h/w naming for OPPs are now standardized in terms of OPP50, 100, > >>>> Turbo and SB. This maps as shown above to the opp IDs (s/w term). > >>>> b) For boards which need custom VDD1/2 OPPs, the opp table can be > >>>> updated by the board file on a need basis after the > >>>> omap3_pm_init_opp_table call. The OPPs introduced here are the > >>>> official OPP table at this point in time. > >>>> > >>>> Cc: Benoit Cousson <b-cousson@xxxxxx> > >>>> Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> > >>>> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@xxxxxx> > >>>> Cc: Paul Walmsley <paul@xxxxxxxxx> > >>>> Cc: Romit Dasgupta <romit@xxxxxx> > >>>> Cc: Sanjeev Premi <premi@xxxxxx> > >>>> Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxx> > >>>> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@xxxxxx> > >>>> Cc: Thara Gopinath <thara@xxxxxx> > >>>> Cc: Vishwanath Sripathy <vishwanath.bs@xxxxxx> > >>>> > >>>> Signed-off-by: Nishanth Menon <nm@xxxxxx> > >>>> --- > >>>> arch/arm/mach-omap2/pm34xx.c | 49 ++++++++++++++++++++++++++++++++++++++++- > >>>> 1 files changed, 47 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c > >>>> index ad21f5f..05ecf02 100644 > >>>> --- a/arch/arm/mach-omap2/pm34xx.c > >>>> +++ b/arch/arm/mach-omap2/pm34xx.c > >>>> @@ -143,6 +143,41 @@ static struct omap_opp_def __initdata omap34xx_dsp_rate_table[] = { > >>>> {.enabled = 0, .freq = 0, .u_volt = 0} > >>>> }; > >>>> > >>>> +static struct omap_opp_def __initdata omap36xx_mpu_rate_table[] = { > >>>> + /*OPP1 - OPP50*/ > >>>> + {.enabled = true, .freq = 300000000, .u_volt = 930000}, > >>>> + /*OPP2 - OPP100*/ > >>>> + {.enabled = true, .freq = 600000000, .u_volt = 1100000}, > >>>> + /*OPP3 - OPP-Turbo*/ > >>>> + {.enabled = false, .freq = 800000000, .u_volt = 1260000}, > >>>> + /*OPP4 - OPP-SB*/ > >>>> + {.enabled = false, .freq = 1000000000, .u_volt = 1310000}, > >>>> + /* Terminator */ > >>>> + {.enabled = 0, .freq = 0, .u_volt = 0} > >>>> +}; > >>>> + > >>>> +static struct omap_opp_def __initdata omap36xx_l3_rate_table[] = { > >>>> + /*OPP1 - OPP50 */ > >>>> + {.enabled = true, .freq = 100000000, .u_volt = 930000}, > >>>> + /*OPP2 - OPP100, OPP-Turbo, OPP-SB*/ > >>>> + {.enabled = true, .freq = 200000000, .u_volt = 1137500}, > >>>> + /* Terminator */ > >>>> + {.enabled = 0, .freq = 0, .u_volt = 0} > >>>> +}; > >>>> + > >>>> +static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = { > >>>> + /*OPP1 - OPP50*/ > >>>> + {.enabled = true, .freq = 260000000, .u_volt = 930000}, > >>>> + /*OPP2 - OPP100*/ > >>>> + {.enabled = true, .freq = 520000000, .u_volt = 1100000}, > >>>> + /*OPP3 - OPP-Turbo*/ > >>>> + {.enabled = false, .freq = 660000000, .u_volt = 1260000}, > >>>> + /*OPP4 - OPP-SB*/ > >>>> + {.enabled = false, .freq = 875000000, .u_volt = 1310000}, > >>>> + /* Terminator */ > >>>> + {.enabled = 0, .freq = 0, .u_volt = 0} > >>>> +}; > >>>> + > >>>> > >>>> > >>> Although it is not mandatory, I'd say this way of initializing structure array is more common: > >>> > >>> static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = { > >>> /*OPP1 - OPP50*/ > >>> { > >>> .enabled = true, > >>> .freq = 260000000, > >>> .u_volt = 930000, > >>> }, > >>> /*OPP2 - OPP100*/ > >>> { > >>> .enabled = true, > >>> .freq = 520000000, > >>> .u_volt = 1100000, > >>> }, > >>> /*OPP3 - OPP-Turbo*/ > >>> { > >>> .enabled = false, > >>> .freq = 660000000, > >>> .u_volt = 1260000, > >>> }, > >>> /*OPP4 - OPP-SB*/ > >>> { > >>> .enabled = false, > >>> .freq = 875000000, > >>> .u_volt = 1310000, > >>> }, > >>> /* Terminator */ > >>> { > >>> .enabled = 0, > >>> .freq = 0, > >>> .u_volt = 0, > >>> } > >>> }; > >>> > >>> and if you thing it is line consuming, you can always define a "INIT" macro: > >>> #define OMAP_OPP_DEF(_enabled, _freq, _uv) \ > >>> { \ > >>> .enabled = _enabled, \ > >>> .freq = _freq, \ > >>> .u_volt = _uv, \ > >>> } > >>> > >>> and use it to initialize your array: > >>> > >>> /*OPP1 - OPP50*/ > >>> static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = { > >>> OMAP_OPP_DEF(true, 260000000, 930000), > >>> OMAP_OPP_DEF(true, 520000000, 1100000), > >>> OMAP_OPP_DEF(false, 660000000, 1260000), > >>> OMAP_OPP_DEF(false, 875000000, 1310000}, > >>> OMAP_OPP_DEF(0, 0, 0), > >>> }; > >>> > >>> > >> Thanks. yep, I agree this looks cleaner. I vote for this. probably will > >> use OMAP_OPP_TERM to denote (0,0,0) > >> > > > > Nice, > > > > > >>> Beside, although it should not hurt as they are not too big, these tables should > >>> be compiled only if omap3630 is the target right? > >>> > >>> > >>> > >>>> struct omap_opp *omap3_mpu_rate_table; > >>>> struct omap_opp *omap3_dsp_rate_table; > >>>> struct omap_opp *omap3_l3_rate_table; > >>>> @@ -1299,18 +1334,28 @@ static void __init configure_vc(void) > >>>> void __init omap3_pm_init_opp_table(void) > >>>> { > >>>> int ret, i; > >>>> + struct omap_opp_def **omap3_opp_def_list; > >>>> struct omap_opp_def *omap34xx_opp_def_list[] = { > >>>> omap34xx_mpu_rate_table, > >>>> omap34xx_l3_rate_table, > >>>> omap34xx_dsp_rate_table > >>>> }; > >>>> + struct omap_opp_def *omap36xx_opp_def_list[] = { > >>>> + omap36xx_mpu_rate_table, > >>>> + omap36xx_l3_rate_table, > >>>> + omap36xx_dsp_rate_table > >>>> + }; > >>>> struct omap_opp **omap3_rate_tables[] = { > >>>> &omap3_mpu_rate_table, > >>>> &omap3_l3_rate_table, > >>>> &omap3_dsp_rate_table > >>>> }; > >>>> - for (i = 0; i < ARRAY_SIZE(omap34xx_opp_def_list); i++) { > >>>> - ret = opp_init(omap3_rate_tables[i], omap34xx_opp_def_list[i]); > >>>> + > >>>> + omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list : > >>>> + omap34xx_opp_def_list; > >>>> > >>>> > >>> here you cared for runtime target check, but tables above could be avoid if not omap3630. > >>> > >>> > >> If your concern is memory size: the tables are __initdata, they will get > >> freed once kernel boot is complete. the only representation of omap_opp > >> will be what the opp layer's definition of the same. > >> > >> The intention was to be able to have a single uImage booting across > >> multiple boards with multiple silicon -> e.g. today same uImage with > >> omap_pm_defconfig will equally boot on sdp3430 as it does on sdp3630.. > >> which is what we all prefer.. so no ways of a compile time inclusion- it > >> has to be runtime determined.. > >> > > > > Yeah, but if you have multiple board configuration, at compile time, your check > > should add all selected tables. > > > > I just pointed that because it is the way I see in current code. You check at > > compile time if you define tables or not, then at runtime you check which table > > to use. That should still work for multi board configuration (using same image > > for sdp3430 or sdp3630). > > > > > > > Check arch/arm/mach-omap2/mux.c or arch/arm/mach-omap2/mcbsp.c. > > > > There are other examples. If you take a look on those you will see that it is > > same issue you are dealing, defining static tables for several omap versions and then > > just selecting which one to use at runtime. Besides, in mux.c you see that all are > > also __initdata_or_module. > > > I think I still dont understand your intention -> > a) pm34xx.c will not be a module -> it is meant to be built in, so > __module addition to __initdata does not make any sense to it. Well, I just commented that those have the "init" flag you mention earlier. > > b) arch/arm/mach-omap2/pm34xx.c is omap3 series only build ONLY when OMAP3 is defined, there are only two of those families rt now, 34xx (of which 35xx is one), and 36xx series.. > so I dont need an enormous array definition like mux.h http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/mach-omap2/mux.c;h=c18a94eca641d61826fbad85322bc9f3d2cb4841;hb=HEAD#l257 where it is #ifdef OMAP3.. > > I am not trying to define for various OMAP versions, I am trying to > define for various OMAP3 versions.. I hope this clarifies. I still see as "a set of omap versions" support that is compiled in then and you choose which one to use at run time. > > > > > >>> > >>> > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(omap3_rate_tables); i++) { > >>>> + ret = opp_init(omap3_rate_tables[i], omap3_opp_def_list[i]); > >>>> /* We dont want half configured system at the moment */ > >>>> BUG_ON(ret); > >>>> } > >>>> -- > >>>> 1.6.3.3 > >>>> > >>>> -- > >>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in > >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx > >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >>>> > >>>> > >>> > >>> > > > > -- Eduardo Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
