Re: [PATCH 09/10 V3] omap3: pm: introduce 3630 opps

["Menon, Nishanth" <nm@ti.com>]

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@ti.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Cc: Romit Dasgupta <romit@ti.com>
>> Cc: Sanjeev Premi <premi@ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
>> Cc: Thara Gopinath <thara@ti.com>
>> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>>  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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>     
>
>