>>-----Original Message----- >>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>Sent: Saturday, August 28, 2010 5:23 AM >>To: Gopinath, Thara >>Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; Sripathy, Vishwanath; Sawant, Anand; Cousson, Benoit >>Subject: Re: [PATCH 01/13] OMAP: Introduce a user list for each voltage domain instance in the >>voltage driver. >> >>Thara Gopinath <thara@xxxxxx> writes: >> >>> This patch introduces a user list of devices associated with each >>> voltage domain instance. The user list is implemented using plist >>> structure with priority node populated with the voltage values. >>> This patch also adds an API which will take in a device and >>> requested voltage as parameters, adds the info to the user list >>> and returns back the maximum voltage requested by all the user >>> devices. This can be used anytime to get the voltage that the >>> voltage domain instance can be transitioned into. >>> >>> Signed-off-by: Thara Gopinath <thara@xxxxxx> >>> --- >>> arch/arm/mach-omap2/voltage.c | 94 +++++++++++++++++++++++++++++ >>> arch/arm/plat-omap/include/plat/voltage.h | 2 + >>> 2 files changed, 96 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c >>> index 6a07fe9..4624250 100644 >>> --- a/arch/arm/mach-omap2/voltage.c >>> +++ b/arch/arm/mach-omap2/voltage.c >>> @@ -24,6 +24,9 @@ >>> #include <linux/clk.h> >>> #include <linux/err.h> >>> #include <linux/debugfs.h> >>> +#include <linux/spinlock.h> >>> +#include <linux/plist.h> >>> +#include <linux/slab.h> >>> >>> #include <plat/omap-pm.h> >>> #include <plat/omap34xx.h> >>> @@ -95,6 +98,20 @@ struct vp_reg_val { >>> }; >>> >>> /** >>> + * omap_vdd_user_list - The per vdd user list >>> + * >>> + * @dev : The device asking for the vdd to be set at a particular >>> + * voltage >>> + * @node : The list head entry >>> + * @volt : The voltage requested by the device <dev> >>> + */ >>> +struct omap_vdd_user_list { >>> + struct device *dev; >>> + struct plist_node node; >>> + u32 volt; >>> +}; >>> + >>> +/** >>> * omap_vdd_info - Per Voltage Domain info >>> * >>> * @volt_data : voltage table having the distinct voltages supported >>> @@ -105,6 +122,9 @@ struct vp_reg_val { >>> * vp registers >>> * @volt_clk : the clock associated with the vdd. >>> * @opp_dev : the 'struct device' associated with this vdd. >>> + * @user_lock : the lock to be used by the plist user_list >>> + * @user_list : the list head maintaining the various users >>> + * of this vdd with the voltage requested by each user. >>> * @volt_data_count : Number of distinct voltages supported by this vdd. >>> * @nominal_volt : Nominal voltaged for this vdd. >>> * cmdval_reg : Voltage controller cmdval register. >>> @@ -117,6 +137,9 @@ struct omap_vdd_info{ >>> struct clk *volt_clk; >>> struct device *opp_dev; >>> struct voltagedomain voltdm; >>> + spinlock_t user_lock; >>> + struct plist_head user_list; >>> + struct mutex scaling_mutex; >>> int volt_data_count; >>> unsigned long nominal_volt; >>> u8 cmdval_reg; >>> @@ -785,11 +808,18 @@ static void __init vdd_data_configure(struct omap_vdd_info *vdd) >>> struct dentry *vdd_debug; >>> char name[16]; >>> #endif >>> + >>> if (cpu_is_omap34xx()) >>> omap3_vdd_data_configure(vdd); >>> else if (cpu_is_omap44xx()) >>> omap4_vdd_data_configure(vdd); >>> >>> + /* Init the plist */ >>> + spin_lock_init(&vdd->user_lock); >>> + plist_head_init(&vdd->user_list, &vdd->user_lock); >>> + /* Init the DVFS mutex */ >>> + mutex_init(&vdd->scaling_mutex); >>> + >>> #ifdef CONFIG_PM_DEBUG >>> strcpy(name, "vdd_"); >>> strcat(name, vdd->voltdm.name); >>> @@ -1142,6 +1172,70 @@ unsigned long omap_vp_get_curr_volt(struct voltagedomain *voltdm) >>> } >>> >>> /** >>> + * omap_voltage_add_userreq : API to keep track of various requests to >>> + * scale the VDD and returns the best possible >>> + * voltage the VDD can be put to. >>> + * @volt_domain: pointer to the voltage domain. >>> + * @dev : the device pointer. >>> + * @volt : the voltage which is requested by the device. >>> + * >>> + * This API is to be called before the actual voltage scaling is >>> + * done to determine what is the best possible voltage the VDD can >>> + * be put to. This API adds the device <dev> in the user list of the >>> + * vdd <volt_domain> with <volt> as the requested voltage. The user list >>> + * is a plist with the priority element absolute voltage values. >>> + * The API then finds the maximum of all the requested voltages for >>> + * the VDD and returns it back through <volt> pointer itself. >>> + * Returns error value in case of any errors. >>> + */ >>> +int omap_voltage_add_userreq(struct voltagedomain *voltdm, struct device *dev, >>> + unsigned long *volt) >> >>How about just omap_voltage_add_request() >> >>Also, do we need both voltdm and dev? With your other patches, can't we >>look up the voltm based on dev? Or, is there a need for a device to add >>a request in a voltage domain other than the one to which it belongs? >> >>Also, how does one remove a voltage request? Hello Kevin, I could rename this API to what you have suggested. Yes we do need voltdm and dev. Let us say I have three devices in a voltdm and I need to maintain a request for each of these devices. When a omap_device_set_rate API is called by the device to lower its rate the voltage request will automatically get lowered. >> >>> +{ >>> + struct omap_vdd_info *vdd; >>> + struct omap_vdd_user_list *user; >>> + struct plist_node *node; >>> + int found = 0; >>> + >>> + if (!voltdm || IS_ERR(voltdm)) { >>> + pr_warning("%s: VDD specified does not exist!\n", __func__); >>> + return -EINVAL; >>> + } >>> + >>> + vdd = container_of(voltdm, struct omap_vdd_info, voltdm); >>> + >>> + mutex_lock(&vdd->scaling_mutex); >>> + >>> + plist_for_each_entry(user, &vdd->user_list, node) { >>> + if (user->dev == dev) { >>> + found = 1; >>> + break; >>> + } >>> + } >> >>Minor: I'm not crazy about the 'found' flag. IMO, readability is >>improved if you init 'user = NULL' above, use a tmp pointer to >>walk the list, and rather than 'found = 1', do 'user = tmp_user' >>when you find the match. Ok will do. >> >>> + >>> + if (!found) { >> >>and here, do if (!user) >> >>> + user = kzalloc(sizeof(struct omap_vdd_user_list), GFP_KERNEL); >>> + if (!user) { >>> + pr_err("%s: Unable to creat a new user for vdd_%s\n", >>> + __func__, voltdm->name); >>> + mutex_unlock(&vdd->scaling_mutex); >>> + return -ENOMEM; >>> + } >>> + user->dev = dev; >>> + } else { >>> + plist_del(&user->node, &vdd->user_list); >> >>hmm... if we get here, we didn't find a match, so 'user' points to the >>last element in the list (which is the highest voltage request, I >>guess), or even NULL if the list is empty. Then, this node is deleted. >> >>-ECONFUSED I do not understand this comment. But I will surely look into this part of code. Btw this code is now more tested out with OMAP4 internally. >> >>> + } >>> + >>> + plist_node_init(&user->node, *volt); >>> + plist_add(&user->node, &vdd->user_list); >>> + node = plist_first(&vdd->user_list); >>> + *volt = node->prio; >>> + >>> + mutex_unlock(&vdd->scaling_mutex); >>> + >>> + return 0; >>> +} >> >>Can't think of a use-case now (cuz it's Friday), but would we ever want >>to add multiple requests per device? >> >>The current approch will only track one request per device. IMHO only as of now only one request per device needs to be tracked. A device the volt domain to be at only one particular voltage at any instance. Regards Thara -- 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
