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 <[email protected]>
>> Cc: Kevin Hilman <[email protected]>
>> Cc: Madhusudhan Chikkature Rajashekar <[email protected]>
>> Cc: Paul Walmsley <[email protected]>
>> Cc: Romit Dasgupta <[email protected]>
>> Cc: Sanjeev Premi <[email protected]>
>> Cc: Santosh Shilimkar <[email protected]>
>> Cc: Sergio Alberto Aguirre Rodriguez <[email protected]>
>> Cc: Thara Gopinath <[email protected]>
>> Cc: Vishwanath Sripathy <[email protected]>
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> 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 [email protected]
>> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
