Re: Question on OPP table handling

[Nishanth Menon <nm@ti.com>]

Premi, Sanjeev had written, on 10/06/2009 03:52 AM, the following:
>>>>>>> +	{0, 0, 0},
>>>>>>> +	/*OPP1*/
>>>>>>> +	{S125M, VDD1_OPP1, 0x1E},
>>>>>>> +	/*OPP2*/
>>>>>>> +	{S250M, VDD1_OPP2, 0x26},
>>>>>>> +	/*OPP3*/
>>>>>>> +	{S500M, VDD1_OPP3, 0x30},
>>>>>>> +	/*OPP4*/
>>>>>>> +	{S550M, VDD1_OPP4, 0x36},
>>>>>>> +	/*OPP5*/
>>>>>>> +	{S600M, VDD1_OPP5, 0x3C},
>>>>>>> +};
>>>>>>>
>>>>>> For those involved,
>>>>>> if we wanted to convert omap3_mpu_table[] into
>>>>>> *omap3_mpu_table so that
>>>>>> we dynamically initialize it based on cpu type - what 
>> would be the
>>>>>> recommendations?
>>>>> Nishanth,
>>>>>
>>>>> Good idea!
>>>>>
>>>>> As a table, previous patch enables it (not as intent, but due to
>>> syntax):
>>>>>   >  +/* struct omap_opp_table - View OPP table as an object
>>>>>   > + * @min: Minimum OPP id
>>>>>   > + * @max: Maximim OPP id
>>>>>   > + * @opps: Pointer to array defining the OPPs.
>>>>>   > + *
>>>>>   > + * An OPP table has varied length. Knowing minimum 
>> and maximum
>>>>>   > + * OPP ids allow easy traversal.
>>>>>   > + */
>>>>>   > +struct omap_opp_table {
>>>>>   > +       u8      min;
>>>>>   > +       u8      max;
>>>>>   > +       struct omap_opp* opps;
>>>>>   > +};
>>>>>
>>>>> But now, I think it would be good to have an API that can fill an
>>> opp_table:
>>>>> int add_opp_definition(u8 id, u32 freq, u16 vsel);
>>>>>
>>>>> ...and, if an array is preferred, length can be set as:
>>>>> int set_opp_table_length (u8 max);
>>>> I'm all for dynamic OPP setting, but not as an array.  A 
>> list should
>>>> probably be used.
>>> Won't a list implementation cause more than necessary 
>> overhead? I agree
>>> that something like set_opp_table_length probably might be 
>> redundant in
>>> that case.
>> I'm aligned with Nishanth. I think a static table with the 
>> possibility to disable some entry is good enough to deal with 
>> most of the OPPs we have on OMAP3 and we will have to handle on OMAP4.
>>
>> OPPs are defined during silicon characterization, and should 
>> not be changed dynamically (in theory).
> 
> [sp] The intent of 'dynamic' is not with respect to changing the
>      OPPs but manitaining OPPs in an array or a list.
> 
>      This is to take care of possibility that an OPP is not
>      applicable for specific devices. E.g. OPP5 was earlier
>      considered 'overdrive'; and the code had a small 'hack'
>      to prevent this OPP being used during cpufreq.
> 
>      Marking the OPP 'disabled/invalid' in the table would have
>      been a better solution.
> 
>      In a 'list' implementation, the node corresponding to such
>      OPPs can be removed from the 'active' list.

Couple of opinions on why a list with disabled/invalid marker might not 
make sense as a grand unified OPP table:
a) it is no better than a list implementation
b) it is a waste of memory.
c) search Algo overheads

Recommendation a.k.a hybrid approach:
* Each silicon has it's own static OPP table
* Each table has a invalid marker which is disabled for silicon variants 
which dont need specific OPPs
* OPPs table be stored in a hash table a.k.a how do we optimize search 
for OPP params?

-- 
Regards,
Nishanth Menon