Cousson, Benoit said the following on 11/21/2009 04:22 PM:
>> From: Nishanth Menon [mailto:[email protected]]
>>
>> Hi Benoit,
>>
>> Thanks for your detailed review comments. few views follows
>>
>> Cousson, Benoit said the following on 11/21/2009 10:07 AM:
>> [...]
>>
>>>> /**
>>>> * struct omap_opp_def - OMAP OPP Definition
>>>> * @enabled: True/false - is this OPP enabled/disabled by default
>>>> * @freq: Frequency in hertz corresponding to this OPP
>>>> * @m_volt: Nominal voltage in millivolts corresponding to this OPP
>>>> *
>>>> * OMAP SOCs from OMAP3 onwards have a standard set of tuples
>>>>
>> consisting
>>
>>>> * of frequency and voltage pairs that the device will support. This is
>>>> * Operating Point. However, the actual definitions of OMAP Operating
>>>> Point
>>>> * varies over silicon within the same family of devices. For a
>>>>
>> specific
>>
>>>> * domain, you can have a set of {frequency, voltage} pairs and this is
>>>> denoted
>>>> * by an array of omap_opp_def. As the kernel boots and more
>>>>
>> information
>>
>>> The OPP term is not necessarily something new to OMAP3; OMAP2420/2430
>>>
>> already supported 2 OPPs (100% and 50%) and to some extend OMAP1610/1710
>> were also able to run at a lowest OPP (50%).
>>
>>> To be more accurate, you should explain that the OPP is relative to a
>>>
>> voltage domain and that all IPs in that voltage domain will have
>> potentially their own OPPs depending of the voltage.
>>
>>>
>> Thanks for explaining the history. yep, will fix up.
>>
>>>> is
>>>> * available, a set of these are activated based on the precise nature
>>>>
>> of
>>
>>>> * device the kernel boots up on.
>>>> *
>>>> * NOTE: A disabled opp from the omap_opp_def table may be enabled as
>>>> part of
>>>>
>>>>
>>> Are you sure you want to allow that? What if you re-enable none supported
>>>
>> OPPs like a speed-binning OPP on a regular device?
>>
>> Yes, we want to allow this. here is why: we provide a list of ALL opps
>> supported on that family of devices of which the common opp entries
>> (across the specific family of SOCs for which the table are enabled).
>> yes, there are multiple ways of doing this, one of which is to have
>> enable/disable opp OR to use a add function etc.. we can scale this
>> function as needed in the future as our experience with more dynamic
>> silicon decisions become solid. here are specific examples:
>> cpu_is_3630() will probably register all OPPs for 3630 including speed
>> binned one which will be by default disabled, on detecting the
>> speedbinned variants, OPP will be specifically enabled.
>>
>
> OK, I still had in mind the proposal to copy the relevant OPP from a const
> table in initdata to the actual omap_opp_def table. In that case only the
> valid OPPs will be in the table. It will prevent anybody to reactivate a
> speed-binned OPP.
> BTW, since you build the omap_opp_def at boot time, why do you want to keep
> all possible OPPs for the SoC family an not keep only the relevant ones for
> the specific SoC?
>
>
>
>> Now opp.[ch] is meant to be core functions with control of some of the
>> functions from elsewhere, in kernel space with un exported functions, I
>> do not expect to write "dumb proof code", instead I would expect to
>> write smart optimized code and allow other smart people's smart code use
>> it optimally ;).. if we screw up here (e.g. enable a wrong OPP), we need
>> code fixing and with this framwork it would now be easy to track and fix
>> as OPP access are centralized.
>>
>>
>>
>>>> * runtime logic. It is discouraged to enable/disable the OPP unless
>>>> they are
>>>> * done as part of OPP registration sequence.
>>>>
>>>>
>>> Why is it discouraged? We should have as well the capability to
>>>
>> enable/disable dynamically and temporarily an OPP (thermal management,
>> frequency avoidance, select different OPP sets...).
>>
>>> You should differentiate OPPs disabled at boot time because they are not
>>>
>> supported by the platform and the ones disabled at runtime. It will avoid
>> that you potentially re-enable a forbidden OPP.
>>
>>> Why don't you remove them during the opp_init phase?
>>>
>>>
>> Discouraged IMHO you may impact rest of the today's system such as
>> cpufreq heuristics etc.. But, I agree with you, this is a current
>> limitation which is not what I should reflect in the comment - will
>> remove that statement. btw, it is smarter for the higher layers to make
>> dynamic decisions of temp frequency avoidance without having to resort
>> to enable/disable OPP IMHO.
>>
>
> I do agree, but in that case the opp_enable API should be limited to the
> opp_init phase and thus not expose at all.
> You should then clarify the scope of that API and explain what must be done
> by the CPUFreq layer.
>
I am not intending to describe the power management architecture here.
if you have a proposal of how it should be stated, please do share, I
dont think I get why you need this api to describe usage
in-yet-to-be-used-in-cpufreq? esp considering I have intentions of
describing OPP layer not, how rest of the blocks should use these APIs.
the actual implementation is upto the OPP.c.
I think a description from you might be easier than a bunch of back and
froth on emails.
>
>
>>>> */
>>>> struct omap_opp_def {
>>>> bool enabled;
>>>> unsigned long freq;
>>>> u16 m_volt;
>>>>
>>>>
>>> It might be better to use an u32 and store the voltage in microvolt
>>>
>> considering that the T2 and Phoenix have a 12.5 mV accuracy step. Moreover,
>> this is what the regulator framework is currently using, it will avoid the
>> *1000 operation, which is not that a big deal, I agree...
>>
>> Thanks for pointing this out.. yep uV makes more sense. will switch.
>>
>>
>>>> };
>>>>
>>>> /**
>>>> * opp_init - Initialize an OPP table from the initial table definition
>>>> * @oppl: Returned back to caller as the opp list to reference the
>>>>
>> OPP
>>
>>>> * @opp_defs: Array of omap_opp_def to describe the OPP. This
>>>>
>> list
>>
>>>> should be
>>>> * 0 terminated.
>>>> *
>>>> * This function creates the internal datastructure representing the
>>>> OPP list
>>>> * from an initial definition table. this handle is then used for
>>>>
>> further
>>
>>>> * validation, search, modification operations on the OPP list.
>>>> *
>>>> * This function returns 0 and the pointer to the allocated list
>>>> through oppl if
>>>> * success, else corresponding error value. Caller should NOT free the
>>>> oppl.
>>>> * opps_defs can be freed after use.
>>>> */
>>>> int opp_init(struct omap_opp **oppl, const struct omap_opp_def
>>>>
>> *opp_defs);
>>
>>>> /**
>>>> * opp_enable - Enable or disable a specific OPP
>>>>
>>>>
>>> You'd better provide two APIs opp_enable and opp_disable or find a better
>>>
>> name for that one, like opp_state or opp_set_state.
>>
>> the kernel has other xyz_enable function which does disable also. yes,
>> this had been debated internally, and Kevin pointed this out.
>>
>> example:
>> include/linux/pci.h:int pci_enable_wake(struct pci_dev *dev, pci_power_t
>> state, bool enable);
>> others: grep -R ".*enable.*(" include/|grep -v define
>>
>
> Nobody is perfect... even the current Linux code :-)
>
>
>
>> If someone has strong opinions on this and find this function extremely
>> obscure, I don't mind creating wrappers around an internal __opp_onoff
>> and provide and opp_enable and opp_disable. it is just a one liner anyways..
>>
>
> If it is not a big deal to fix that, then it might be good to do it. It will
> avoid further debate.
>
Personally, I consider this a non-issue,but, if it makes people happy,
I will add a wrapper here..
Regards,
Nishanth Menon
--
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
