Eduardo Valentin said the following on 12/08/2009 02:23 AM: > Hello Nishanth and Kevin, > > On Thu, Nov 26, 2009 at 01:22:49AM +0100, ext Nishanth Menon wrote: > >> Kevin Hilman had written, on 11/25/2009 05:46 PM, the following: >> [...] >> >>>>> something like this instead: >>>>> >>>>> /** >>>>> * opp_find_freq() >>>>> * @oppl: OPP list >>>>> * @freq: Frequency to look for in OPP table >>>>> * >>>>> * Look for an enabled OPP with a frequency value matching @freq. >>>>> * >>>>> * Returns pointer to OPP entry if match is found, or NULL if no match >>>>> * found. >>>>> */ >>>>> struct omap_opp *opp_find_freq(const struct omap_opp *oppl, u32 freq); >>>>> >>>> I did think about it(single function doing multiple things), the >>>> intention is as follows in reality: >>>> opp_is_valid : Search only for enabled frequencies >>>> opp_has_freq : Searches for enabled/disabled frequencies >>>> >>>> This is useful for some logic which wants to enable a frequency which >>>> may have been disabled in the table. now, to retain that >>>> functionality, >>>> A) >>>> /** >>>> * opp_find_freq() - Find a opp corresponding to frequency >>>> * @oppl: opp list to search >>>> * @freq: frequency to loopup in OPP table >>>> * @enabled: search only enabled frequencies >>>> * >>>> * return opp handle corresponding to the frequency found, else >>>> * return NULL. Search for enabled frequencies if enabled flag >>>> * is true, else search for disabled frequencies also >>>> */ >>>> struct omap_opp *opp_find_freq(const struct omap_opp *oppl, >>>> unsigned long freq, bool enabled); >>>> >>>> This will handle the functionalities that are supported in rev 3. >>>> >>>> B) I rename existing functions: >>>> opp_has_freq ==> opp_is_disabled() >>>> opp_is_valid ==> opp_is_enabled() >>>> >>>> I would still prefer to go with explicit function search APIs.. >>>> >>> I like (A) here. >>> >> Ack. I can see it useful if we go to a list iterator at a later point of time. >> [...] >> >> >>>>> /** >>>>> * opp_find_freq_rounded() >>>>> * @oppl: OPP list >>>>> * @freq: Frequency to look for in OPP table >>>>> * @rounding: Rounding option: NONE, UP, DOWN >>>>> * >>>>> * Look for an OPP with a frequency value matching @freq. If >>>>> * @rounding != ROUND_NONE, find closest match using rounding. >>>>> * >>>>> * Can be used to find exact OPP, or match using given rounding: >>>>> >>>>> * @rounding == UP: find next highest frequency >>>>> * @rounding == DOWN: find next lowest frequency >>>>> * @rounding == CLOSEST: find closest frequency >>>>> * >>>>> * Returns pointer to OPP entry if match is found, or NULL if no match >>>>> * found (only possible when no rounding is used) >>>>> */ >>>>> struct omap_opp *opp_find_freq_rounded(struct omap_opp *oppl, u32 freq, u32 rounding); >>>>> >>>>> Looking at the users of the 'higher' and 'lower' OPPs in the current >>>>> code, I see that SRF tries to use all three one after the other. >>>>> First it checks for exact match, then for higher, then for lower. >>>>> This could be replaced simply by doing a 'closest' match. >>>>> >>>> hmmm.. I think we are going a full circle here. >>>> >>>> /* Search exact frequency */ >>>> #define OPP_ROUND_NONE (0 << 0) >>>> /* Search approx frequency */ >>>> #define OPP_ROUND_CLOSEST (1 << 0) >>>> /* Search up */ >>>> #define OPP_ROUND_HIGH (0 << 1) >>>> /* Search down */ >>>> #define OPP_ROUND_LOW (1 << 1) >>>> >>>> struct omap_opp *opp_find_freq_rounded(struct omap_opp *oppl, >>>> unsigned long freq, u8 rounding_flag); >>>> >>>> Note: the only difference b/w this and opp_find_freq is that >>>> opp_find_freq will also search for enabled/disabled. >>>> If I add that here, this is exactly the #1 implementation I did in >>>> http://marc.info/?l=linux-omap&m=125800489123496&w=2 >>>> ok, I used bool instead of a #define and added the complexity of using >>>> enabled flag also: >>>> >>>> bool search_higher, bool search_enabled_only, bool exact_search >>>> >>>> what you are proposing is that I go back to my implementation #1 and >>>> remove my bools instead by adding #defines: >>>> /* Search up */ >>>> #define OPP_ROUND_ENABLED (0 << 2) >>>> /* Search down */ >>>> #define OPP_ROUND_ANY (1 << 2) >>>> >>>> would even make the find_freq redundant.. >>>> >>>> Now, in your comment in >>>> http://marc.info/?l=linux-omap&m=125816031406704&w=2 quote:"I think >>>> all the options to this function make it not terribly >>>> readable." >>>> >>>> Does it become any different now? >>>> >>>> >>> Yeah, I still think multiple bools to a function is a readability >>> problem. Every time you look at a call, you have to look at the >>> prototype to remember what all the options are. A single bool or flag >>> is better IMHO. >>> >>> >>>> without taking this in, exact searches can be done by specific >>>> functions and approximate searches by another function.. we go >>>> generic functions or we go specific functions.. generic comments I >>>> have been getting is to go specific, hence the v2 and v3 series. >>>> >>> OK, you're right. I am causing us to go around in circles now, but >>> we're going around in increasingly smaller circles and hopefully >>> converging on the target. ;) >>> >> Yep :) >> >> >>> So what I propose is having two functions. One for exact matches >>> (your proposal A above) and one for approximate matches which is >>> something like my find_freq_rounded(), but should maybe be renamed >>> something like opp_find_freq_approx() or something. >>> >> OK - Signed-Off. prototypes again :) >> >> Dead functions: opp_has_freq, opp_is_valid, opp_find_freq and other searches. >> >> Only two of them remain: (git diff follows) >> opp_find_freq_approx >> >> opp_find_freq_exact >> >> >> [..] >> >>>> int __init opp_add(struct omap_opp **oppl, const struct omap_opp_def >>>> *opp_defs); >>>> >>> Mostly, but I was thinking of an API for adding a *single* OPP. The >>> init code would iterate over its opp_def table adding OPPs one at a >>> time resultingin multiple calls to opp_add(). >>> >>> OK, now I'm going around in circles again, but thinking more about his >>> we probably need your original opp_init() which will create an empty >>> master list, then repeated calls to opp_add() to add each OPP to the >>> master list. >>> >> Opp_init will be a subcase of opp_add. Giving opp_add capabiity to take >> More than one def is useful for us in the long run: >> a) You can add bunch of them without list iterator repeating in each and every adding code. >> b) You can still add a single one if you choose to. >> c) add to a NULL list == init the list. That does sound implicit to me, but if we want to create a singular function for it alone, then I recommend: >> >> lets just have opp_init, then see if we need to add opp_add -> none of the systems are mature enough for us to use it.. >> >> I am open either way. >> >> >>> Also does add need to be __init? Not sure why we would need to add >>> OPPs after boot time if we have the ability to enable/disable them at >>> runtime, but just a thought. >>> >> Removed.. if in case we would like to do an opp_add at some point. >> >> >> [...] >> >> >> diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h >> new file mode 100644 >> index 0000000..4a00685 >> --- /dev/null >> +++ b/arch/arm/plat-omap/include/plat/opp.h >> @@ -0,0 +1,184 @@ >> +/* >> + * OMAP OPP Interface >> + * >> + * Copyright (C) 2009 Texas Instruments Incorporated. >> + * Nishanth Menon >> + * Copyright (C) 2009 Deep Root Systems, LLC. >> + * Kevin Hilman >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#ifndef __ASM_ARM_OMAP_OPP_H >> +#define __ASM_ARM_OMAP_OPP_H >> + >> +/** >> + * struct omap_opp - OMAP OPP description structure >> + * @enabled: true/false - marking this OPP as enabled/disabled >> + * @rate: Frequency in hertz >> + * @opp_id: (DEPRECATED) opp identifier >> + * @vsel: Voltage in volt processor level(this usage is >> + * DEPRECATED to use Voltage in microvolts in future) >> + * uV = ((vsel * 12.5) + 600) * 1000 >> + * >> + * This structure stores the OPP information for a given domain. >> + * Due to legacy reasons, this structure is currently exposed and >> + * will soon be removed elsewhere and will only be used as a handle >> + * from the OPP internal referencing mechanism >> + */ >> +struct omap_opp { >> + bool enabled; >> + unsigned long rate; >> + u8 opp_id __deprecated; >> + u16 vsel; >> +}; >> + >> +/** >> + * opp_get_voltage() - Gets the voltage corresponding to an opp >> + * @opp: opp for which voltage has to be returned for >> + * >> + * Return voltage in micro volt corresponding to the opp, else >> + * return 0 >> + */ >> +unsigned long opp_get_voltage(const struct omap_opp *opp); >> + >> +/** >> + * opp_get_freq() - Gets the frequency corresponding to an opp >> + * @opp: opp for which frequency has to be returned for >> + * >> + * Return frequency in hertz corresponding to the opp, else >> + * return 0 >> + */ >> +unsigned long opp_get_freq(const struct omap_opp *opp); >> + >> +/** >> + * opp_get_opp_count - Get number of opps enabled in the opp list >> + * @num: returns the number of opps >> + * @oppl: opp list >> + * >> + * This functions returns the number of opps if there are any OPPs enabled, >> + * else returns corresponding error value. >> + */ >> +int opp_get_opp_count(const struct omap_opp *oppl); >> + >> +#define OPP_SEARCH_HIGH (0 << 1) >> +#define OPP_SEARCH_LOW (1 << 1) >> +/** >> + * opp_find_freq_approx() - Search for an approximate freq >> + * @oppl: starting list >> + * @freq: start frequency >> + * @dir_flags: search direction >> + * OPP_SEARCH_HIGH - search for next highest freq >> + * OPP_SEARCH_LOW - search for next lowest freq >> + * >> + * Search for the higher/lower *enabled* OPP than a starting freq >> + * from a start opp list. >> + >> + * Returns *opp and *freq is populated with the next match, >> + * else returns NULL >> + * >> + * Example usages: >> + * * print all frequencies in descending order * >> + * unsigned long freq; >> + * struct omap_opp *opp; >> + * for(opp = oppl, freq = ULONG_MAX; opp; >> + * opp = opp_find_freq_approx(opp, &freq, OPP_SEARCH_LOW)) >> + * pr_info("%ld ", freq); >> + * >> + * * print all frequencies in ascending order * >> + * unsigned long freq = 0; >> + * struct omap_opp *opp; >> + * for(opp = oppl, freq = 0; opp; >> + * opp = opp_find_freq_approx(opp, &freq, OPP_SEARCH_HIGH)) >> + * pr_info("%ld ", freq); >> + * NOTE: if we set freq as ULONG_MAX, we get the highest enabled frequency >> + * >> + */ >> +struct omap_opp *opp_find_freq_approx(struct omap_opp *oppl, >> + unsigned long *freq, u8 dir_flags); >> + >> +/** >> + * opp_find_freq_exact() - search for an exact frequency >> + * @oppl: OPP list >> + * @freq: frequency to search for >> + * @enabled: enabled/disabled OPP to search for >> + * >> + * searches for the match in the opp list and returns handle to the matching >> + * opp if found, else returns NULL. >> + * >> + * Note enabled is a modifier for the search. if enabled=true, then the match is >> + * for exact matching frequency and is enabled. if true, the match is for exact >> + * frequency which is disabled. >> + */ >> +struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl, >> + unsigned long freq, bool enabled); >> + >> + >> +/** >> + * 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 >> + * @u_volt: Nominal voltage in microvolts corresponding to this OPP >> + * >> + * OMAP SOCs have a standard set of tuples consisting of frequency and voltage >> + * pairs that the device will support per voltage domain. This is called >> + * Operating Points or OPP. The actual definitions of OMAP Operating Points >> + * 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 is >> + * available, a set of these are activated based on the precise nature of >> + * device the kernel boots up on. It is interesting to remember that each IP >> + * which belongs to a voltage domain may define their own set of OPPs on top >> + * of this - but this is handled by the appropriate driver. >> + */ >> +struct omap_opp_def { >> + bool enabled; >> + unsigned long freq; >> + u32 u_volt; >> +}; >> + >> +/** >> + * opp_add - Add/initialize an OPP table from a table definitions >> + * @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 adds the opp definition to an internal opp list and returns >> + * a handle representing the OPP list. 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. >> + * >> + * NOTE: caller should assume that on success, oppl is probably populated with >> + * a new handle and the new handle should be used for further referencing >> + */ >> +int opp_add(struct omap_opp **oppl, const struct omap_opp_def *opp_defs); >> > > > I'd prefer to see what has been proposed by Kevin previously. If I understood correctly, > you would to split this one into two functions: > > opp_add - To add a single opp. At least that is what one would expect from its name. > opp_init - iterate over all opps definition that need to be added and call opp_add. > I'd name it 'opp_init_table' though, as opp_init sounds like initializing a single opp. > > > okay. let me try and send out a updated proposal later today.. >> + >> +/** >> + * opp_enable - Enable a specific OPP >> + * @opp: pointer to opp >> + * >> + * Enables a provided opp. If the operation is valid, this returns 0, else the >> + * corresponding error value. >> + * >> + * OPP used here is from the the opp_is_valid/opp_has_freq or other search >> + * functions >> + */ >> +int opp_enable(struct omap_opp *opp); >> + >> +/** >> + * opp_disable - Disable a specific OPP >> + * @opp: pointer to opp >> + * >> + * Disables a provided opp. If the operation is valid, this returns 0, else the >> + * corresponding error value. >> + * >> + * OPP used here is from the the opp_is_valid/opp_has_freq or other search >> + * functions >> + */ >> +int opp_disable(struct omap_opp *opp); >> + >> +#endif /* __ASM_ARM_OMAP_OPP_H * >> >> -- >> 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 >> > > -- 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
