Cousson, Benoit said the following on 11/21/2009 04:22 PM: >> From: Nishanth Menon [mailto:menon.nishanth@xxxxxxxxx] >> >> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
