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

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

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

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