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) > > 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.. > >> + >> + 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 >> > > -- 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
