Hi Guys,
This adds code to support operating-points-v2 bindings. Not everything is supported yet, but most of the basic stuff is.
Tested with: Exynos 5250, dual cortex A15 board. With both old and new bindings.
Bindings are already frozen: http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org
Pushed here as well for reference: ssh://git@git.linaro.org/people/viresh.kumar/linux.git opp/v2
Viresh Kumar (10): opp: Relocate few routines OPP: Create _remove_device_opp() for freeing dev_opp OPP: Allocate dev_opp from _add_device_opp() OPP: Break _opp_add_dynamic() into smaller functions opp: Add support to parse "operating-points-v2" bindings OPP: Add clock-latency-ns support opp: Add OPP sharing information to OPP library OPP: Add support for opp-suspend opp: Add helpers for initializing CPU OPPs cpufreq-dt: Add support for operating-points-v2 bindings
drivers/base/power/opp.c | 1063 ++++++++++++++++++++++++++++++++---------- drivers/cpufreq/cpufreq-dt.c | 58 ++- include/linux/pm_opp.h | 29 ++ 3 files changed, 902 insertions(+), 248 deletions(-)
In order to prepare for the later commits, this relocates few routines towards the top as they will be used earlier in the code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 277 ++++++++++++++++++++++++----------------------- 1 file changed, 139 insertions(+), 138 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 677fb2843553..8c3fd57975fb 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -438,6 +438,102 @@ static struct device_opp *_add_device_opp(struct device *dev) }
/** + * _kfree_device_rcu() - Free device_opp RCU handler + * @head: RCU head + */ +static void _kfree_device_rcu(struct rcu_head *head) +{ + struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head); + + kfree_rcu(device_opp, rcu_head); +} + +/** + * _kfree_opp_rcu() - Free OPP RCU handler + * @head: RCU head + */ +static void _kfree_opp_rcu(struct rcu_head *head) +{ + struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head); + + kfree_rcu(opp, rcu_head); +} + +/** + * _opp_remove() - Remove an OPP from a table definition + * @dev_opp: points back to the device_opp struct this opp belongs to + * @opp: pointer to the OPP to remove + * + * This function removes an opp definition from the opp list. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * It is assumed that the caller holds required mutex for an RCU updater + * strategy. + */ +static void _opp_remove(struct device_opp *dev_opp, + struct dev_pm_opp *opp) +{ + /* + * Notify the changes in the availability of the operable + * frequency/voltage list. + */ + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); + list_del_rcu(&opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu); + + if (list_empty(&dev_opp->opp_list)) { + list_del_rcu(&dev_opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, + _kfree_device_rcu); + } +} + +/** + * dev_pm_opp_remove() - Remove an OPP from OPP list + * @dev: device for which we do this operation + * @freq: OPP to remove with matching 'freq' + * + * This function removes an opp from the opp list. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ + struct dev_pm_opp *opp; + struct device_opp *dev_opp; + bool found = false; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + dev_opp = _find_device_opp(dev); + if (IS_ERR(dev_opp)) + goto unlock; + + list_for_each_entry(opp, &dev_opp->opp_list, node) { + if (opp->rate == freq) { + found = true; + break; + } + } + + if (!found) { + dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n", + __func__, freq); + goto unlock; + } + + _opp_remove(dev_opp, opp); +unlock: + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove); + +/** * _opp_add_dynamic() - Allocate a dynamic OPP. * @dev: device for which we do this operation * @freq: Frequency in Hz for this OPP @@ -570,102 +666,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) EXPORT_SYMBOL_GPL(dev_pm_opp_add);
/** - * _kfree_opp_rcu() - Free OPP RCU handler - * @head: RCU head - */ -static void _kfree_opp_rcu(struct rcu_head *head) -{ - struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head); - - kfree_rcu(opp, rcu_head); -} - -/** - * _kfree_device_rcu() - Free device_opp RCU handler - * @head: RCU head - */ -static void _kfree_device_rcu(struct rcu_head *head) -{ - struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head); - - kfree_rcu(device_opp, rcu_head); -} - -/** - * _opp_remove() - Remove an OPP from a table definition - * @dev_opp: points back to the device_opp struct this opp belongs to - * @opp: pointer to the OPP to remove - * - * This function removes an opp definition from the opp list. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * It is assumed that the caller holds required mutex for an RCU updater - * strategy. - */ -static void _opp_remove(struct device_opp *dev_opp, - struct dev_pm_opp *opp) -{ - /* - * Notify the changes in the availability of the operable - * frequency/voltage list. - */ - srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); - list_del_rcu(&opp->node); - call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu); - - if (list_empty(&dev_opp->opp_list)) { - list_del_rcu(&dev_opp->node); - call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, - _kfree_device_rcu); - } -} - -/** - * dev_pm_opp_remove() - Remove an OPP from OPP list - * @dev: device for which we do this operation - * @freq: OPP to remove with matching 'freq' - * - * This function removes an opp from the opp list. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * Hence this function internally uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - */ -void dev_pm_opp_remove(struct device *dev, unsigned long freq) -{ - struct dev_pm_opp *opp; - struct device_opp *dev_opp; - bool found = false; - - /* Hold our list modification lock here */ - mutex_lock(&dev_opp_list_lock); - - dev_opp = _find_device_opp(dev); - if (IS_ERR(dev_opp)) - goto unlock; - - list_for_each_entry(opp, &dev_opp->opp_list, node) { - if (opp->rate == freq) { - found = true; - break; - } - } - - if (!found) { - dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n", - __func__, freq); - goto unlock; - } - - _opp_remove(dev_opp, opp); -unlock: - mutex_unlock(&dev_opp_list_lock); -} -EXPORT_SYMBOL_GPL(dev_pm_opp_remove); - -/** * _opp_set_availability() - helper to set the availability of an opp * @dev: device for which we do this operation * @freq: OPP frequency to modify availability @@ -825,6 +825,49 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
#ifdef CONFIG_OF /** + * of_free_opp_table() - Free OPP table entries created from static DT entries + * @dev: device pointer used to lookup device OPPs. + * + * Free OPPs created using static entries present in DT. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function indirectly uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +void of_free_opp_table(struct device *dev) +{ + struct device_opp *dev_opp; + struct dev_pm_opp *opp, *tmp; + + /* Check for existing list for 'dev' */ + dev_opp = _find_device_opp(dev); + if (IS_ERR(dev_opp)) { + int error = PTR_ERR(dev_opp); + + if (error != -ENODEV) + WARN(1, "%s: dev_opp: %d\n", + IS_ERR_OR_NULL(dev) ? + "Invalid device" : dev_name(dev), + error); + return; + } + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { + if (!opp->dynamic) + _opp_remove(dev_opp, opp); + } + + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(of_free_opp_table); + +/** * of_init_opp_table() - Initialize opp table from device tree * @dev: device pointer used to lookup device OPPs. * @@ -882,46 +925,4 @@ int of_init_opp_table(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(of_init_opp_table); - -/** - * of_free_opp_table() - Free OPP table entries created from static DT entries - * @dev: device pointer used to lookup device OPPs. - * - * Free OPPs created using static entries present in DT. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * Hence this function indirectly uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - */ -void of_free_opp_table(struct device *dev) -{ - struct device_opp *dev_opp; - struct dev_pm_opp *opp, *tmp; - - /* Check for existing list for 'dev' */ - dev_opp = _find_device_opp(dev); - if (IS_ERR(dev_opp)) { - int error = PTR_ERR(dev_opp); - if (error != -ENODEV) - WARN(1, "%s: dev_opp: %d\n", - IS_ERR_OR_NULL(dev) ? - "Invalid device" : dev_name(dev), - error); - return; - } - - /* Hold our list modification lock here */ - mutex_lock(&dev_opp_list_lock); - - /* Free static OPPs */ - list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { - if (!opp->dynamic) - _opp_remove(dev_opp, opp); - } - - mutex_unlock(&dev_opp_list_lock); -} -EXPORT_SYMBOL_GPL(of_free_opp_table); #endif
On 06/15, Viresh Kumar wrote:
In order to prepare for the later commits, this relocates few routines towards the top as they will be used earlier in the code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Hi,
On Monday, June 15, 2015 05:27:27 PM Viresh Kumar wrote:
In order to prepare for the later commits, this relocates few routines towards the top as they will be used earlier in the code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
This will be used from multiple places later. Lets create a separate routine for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 8c3fd57975fb..7895fdd64192 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -449,6 +449,22 @@ static void _kfree_device_rcu(struct rcu_head *head) }
/** + * _remove_device_opp() - Removes a device OPP table + * @dev_opp: device OPP table to be removed. + * + * Removes/frees device OPP table it it doesn't contain any OPPs. + */ +static void _remove_device_opp(struct device_opp *dev_opp) +{ + if (!list_empty(&dev_opp->opp_list)) + return; + + list_del_rcu(&dev_opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, + _kfree_device_rcu); +} + +/** * _kfree_opp_rcu() - Free OPP RCU handler * @head: RCU head */ @@ -481,11 +497,7 @@ static void _opp_remove(struct device_opp *dev_opp, list_del_rcu(&opp->node); call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
- if (list_empty(&dev_opp->opp_list)) { - list_del_rcu(&dev_opp->node); - call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, - _kfree_device_rcu); - } + _remove_device_opp(dev_opp); }
/**
On 06/15, Viresh Kumar wrote:
This will be used from multiple places later. Lets create a separate routine for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Hi,
On Monday, June 15, 2015 05:27:28 PM Viresh Kumar wrote:
This will be used from multiple places later. Lets create a separate routine for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 7895fdd64192..4b646f36f252 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
/** - * _add_device_opp() - Allocate a new device OPP table + * _add_device_opp() - Returns device OPP table * @dev: device for which we do this operation * - * New device node which uses OPPs - used when multiple devices with OPP tables - * are maintained. + * It tries to find an existing table first, if it couldn't find one, it + * allocates a new OPP table and returns that. * * Return: valid device_opp pointer if success, else NULL. */ @@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp;
+ /* Check for existing list for 'dev' first */ + dev_opp = _find_device_opp(dev); + if (!IS_ERR(dev_opp)) + return dev_opp; + /* * Allocate a new device OPP table. In the infrequent case where a new * device is needed to be added, we pay this penalty. @@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove); static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) { - struct device_opp *dev_opp = NULL; - struct dev_pm_opp *opp, *new_opp; + struct device_opp *dev_opp; + struct dev_pm_opp *opp = NULL, *new_opp; struct list_head *head; int ret;
@@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true; - new_opp->dynamic = dynamic;
- /* Check for existing list for 'dev' */ - dev_opp = _find_device_opp(dev); - if (IS_ERR(dev_opp)) { - dev_opp = _add_device_opp(dev); - if (!dev_opp) { - ret = -ENOMEM; - goto free_opp; - } - - head = &dev_opp->opp_list; - goto list_add; + dev_opp = _add_device_opp(dev); + if (!dev_opp) { + ret = -ENOMEM; + goto free_opp; }
/* @@ -620,17 +617,17 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, }
/* Duplicate OPPs ? */ - if (new_opp->rate == opp->rate) { + if (opp && new_opp->rate == opp->rate) { ret = opp->available && new_opp->u_volt == opp->u_volt ? 0 : -EEXIST;
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, new_opp->available); - goto free_opp; + goto remove_dev_opp; }
-list_add: + new_opp->dynamic = dynamic; new_opp->dev_opp = dev_opp; list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock); @@ -642,6 +639,8 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0;
+remove_dev_opp: + _remove_device_opp(dev_opp); free_opp: mutex_unlock(&dev_opp_list_lock); kfree(new_opp);
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/base/power/opp.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 7895fdd64192..4b646f36f252 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); /**
- _add_device_opp() - Allocate a new device OPP table
- _add_device_opp() - Returns device OPP table
Find device OPP table, or allocate a new one?
- @dev: device for which we do this operation
- New device node which uses OPPs - used when multiple devices with OPP tables
- are maintained.
- It tries to find an existing table first, if it couldn't find one, it
*/
- allocates a new OPP table and returns that.
- Return: valid device_opp pointer if success, else NULL.
@@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp;
- /* Check for existing list for 'dev' first */
- dev_opp = _find_device_opp(dev);
- if (!IS_ERR(dev_opp))
return dev_opp;
- /*
- Allocate a new device OPP table. In the infrequent case where a new
- device is needed to be added, we pay this penalty.
@@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove); static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) {
- struct device_opp *dev_opp = NULL;
- struct dev_pm_opp *opp, *new_opp;
- struct device_opp *dev_opp;
- struct dev_pm_opp *opp = NULL, *new_opp;
Hm...
struct list_head *head; int ret; @@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true;
- new_opp->dynamic = dynamic;
- /* Check for existing list for 'dev' */
- dev_opp = _find_device_opp(dev);
- if (IS_ERR(dev_opp)) {
dev_opp = _add_device_opp(dev);
if (!dev_opp) {
ret = -ENOMEM;
goto free_opp;
}
head = &dev_opp->opp_list;
goto list_add;
- dev_opp = _add_device_opp(dev);
- if (!dev_opp) {
ret = -ENOMEM;
}goto free_opp;
/* @@ -620,17 +617,17 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, } /* Duplicate OPPs ? */
- if (new_opp->rate == opp->rate) {
- if (opp && new_opp->rate == opp->rate) {
Isn't opp always non-NULL at this point? Maybe this if statement should be moved into the list_for_each_entry_rcu() loop.
ret = opp->available && new_opp->u_volt == opp->u_volt ? 0 : -EEXIST;
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, new_opp->available);
goto free_opp;
}goto remove_dev_opp;
-list_add:
- new_opp->dynamic = dynamic; new_opp->dev_opp = dev_opp; list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock);
@@ -642,6 +639,8 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0; +remove_dev_opp:
- _remove_device_opp(dev_opp);
Doesn't this just return early because dev_opp has something in it?
On 01-07-15, 18:02, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
- _add_device_opp() - Returns device OPP table
Find device OPP table, or allocate a new one?
Sure.
static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) {
- struct device_opp *dev_opp = NULL;
- struct dev_pm_opp *opp, *new_opp;
- struct device_opp *dev_opp;
- struct dev_pm_opp *opp = NULL, *new_opp;
Hm...
/* Duplicate OPPs ? */
- if (new_opp->rate == opp->rate) {
- if (opp && new_opp->rate == opp->rate) {
Isn't opp always non-NULL at this point? Maybe this if statement should be moved into the list_for_each_entry_rcu() loop.
when the dev_opp was getting created for the first time, before this patch, we used to do a 'goto list_add'. And so control never reached here, but now we might find a empty list of OPPs and so 'opp' can be NULL here.
Not sure about moving this to the loop, as we are already taking 'dev_opp_list_lock' in this routine.
+remove_dev_opp:
- _remove_device_opp(dev_opp);
Doesn't this just return early because dev_opp has something in it?
Hmm, it isn't required probably.
---------------------8<-----------------------
Message-Id: b13057f196330d0e08f3c517676cc3116b1be4ae.1435818265.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 12 Jun 2015 12:43:14 +0530 Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 7895fdd64192..f8df0ad3e822 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
/** - * _add_device_opp() - Allocate a new device OPP table + * _add_device_opp() - Find device OPP table or allocate a new one * @dev: device for which we do this operation * - * New device node which uses OPPs - used when multiple devices with OPP tables - * are maintained. + * It tries to find an existing table first, if it couldn't find one, it + * allocates a new OPP table and returns that. * * Return: valid device_opp pointer if success, else NULL. */ @@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp;
+ /* Check for existing list for 'dev' first */ + dev_opp = _find_device_opp(dev); + if (!IS_ERR(dev_opp)) + return dev_opp; + /* * Allocate a new device OPP table. In the infrequent case where a new * device is needed to be added, we pay this penalty. @@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove); static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) { - struct device_opp *dev_opp = NULL; - struct dev_pm_opp *opp, *new_opp; + struct device_opp *dev_opp; + struct dev_pm_opp *opp = NULL, *new_opp; struct list_head *head; int ret;
@@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true; - new_opp->dynamic = dynamic;
- /* Check for existing list for 'dev' */ - dev_opp = _find_device_opp(dev); - if (IS_ERR(dev_opp)) { - dev_opp = _add_device_opp(dev); - if (!dev_opp) { - ret = -ENOMEM; - goto free_opp; - } - - head = &dev_opp->opp_list; - goto list_add; + dev_opp = _add_device_opp(dev); + if (!dev_opp) { + ret = -ENOMEM; + goto free_opp; }
/* @@ -620,7 +617,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, }
/* Duplicate OPPs ? */ - if (new_opp->rate == opp->rate) { + if (opp && new_opp->rate == opp->rate) { ret = opp->available && new_opp->u_volt == opp->u_volt ? 0 : -EEXIST;
@@ -630,7 +627,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, goto free_opp; }
-list_add: + new_opp->dynamic = dynamic; new_opp->dev_opp = dev_opp; list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock);
On 07/01/2015 11:24 PM, Viresh Kumar wrote:
On 01-07-15, 18:02, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
/* Duplicate OPPs ? */
- if (new_opp->rate == opp->rate) {
- if (opp && new_opp->rate == opp->rate) {
Isn't opp always non-NULL at this point? Maybe this if statement should be moved into the list_for_each_entry_rcu() loop.
when the dev_opp was getting created for the first time, before this patch, we used to do a 'goto list_add'. And so control never reached here, but now we might find a empty list of OPPs and so 'opp' can be NULL here.
Yes, but doesn't list_for_each_entry_rcu() always assign opp to be at least &dev_opp->opp_list->next which will be pointing at &dev_opp->opp_list if the list is empty (modulo ->member)?
Not sure about moving this to the loop, as we are already taking 'dev_opp_list_lock' in this routine.
Sorry, I lost you here. How does that matter? I was suggesting moving the duplicate OPP check into the list_for_each_entry_rcu() loop so that this NULL check doesn't matter.
+remove_dev_opp:
- _remove_device_opp(dev_opp);
Doesn't this just return early because dev_opp has something in it?
Hmm, it isn't required probably.
---------------------8<-----------------------
Message-Id: b13057f196330d0e08f3c517676cc3116b1be4ae.1435818265.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 12 Jun 2015 12:43:14 +0530 Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Looking better.
On 02-07-15, 16:46, Stephen Boyd wrote:
Looking better.
And does this look perfect ? :)
------------------8<----------------
Message-Id: 7c23b93b6b328e0b14b5c5096c3c3322e3d8ee4b.1435905782.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 12 Jun 2015 12:43:14 +0530 Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 60 +++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 7895fdd64192..5869a8524244 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
/** - * _add_device_opp() - Allocate a new device OPP table + * _add_device_opp() - Find device OPP table or allocate a new one * @dev: device for which we do this operation * - * New device node which uses OPPs - used when multiple devices with OPP tables - * are maintained. + * It tries to find an existing table first, if it couldn't find one, it + * allocates a new OPP table and returns that. * * Return: valid device_opp pointer if success, else NULL. */ @@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp;
+ /* Check for existing list for 'dev' first */ + dev_opp = _find_device_opp(dev); + if (!IS_ERR(dev_opp)) + return dev_opp; + /* * Allocate a new device OPP table. In the infrequent case where a new * device is needed to be added, we pay this penalty. @@ -575,7 +580,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove); static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) { - struct device_opp *dev_opp = NULL; + struct device_opp *dev_opp; struct dev_pm_opp *opp, *new_opp; struct list_head *head; int ret; @@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true; - new_opp->dynamic = dynamic; - - /* Check for existing list for 'dev' */ - dev_opp = _find_device_opp(dev); - if (IS_ERR(dev_opp)) { - dev_opp = _add_device_opp(dev); - if (!dev_opp) { - ret = -ENOMEM; - goto free_opp; - }
- head = &dev_opp->opp_list; - goto list_add; + dev_opp = _add_device_opp(dev); + if (!dev_opp) { + ret = -ENOMEM; + goto free_opp; }
/* @@ -613,24 +610,25 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, */ head = &dev_opp->opp_list; list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) { - if (new_opp->rate <= opp->rate) - break; - else + if (new_opp->rate > opp->rate) { head = &opp->node; + } else if (new_opp->rate < opp->rate) { + break; + } else { + /* Duplicate OPPs */ + ret = opp->available && new_opp->u_volt == opp->u_volt ? + 0 : -EEXIST; + + dev_warn(dev, + "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", + __func__, opp->rate, opp->u_volt, + opp->available, new_opp->rate, new_opp->u_volt, + new_opp->available); + goto free_opp; + } }
- /* Duplicate OPPs ? */ - if (new_opp->rate == opp->rate) { - ret = opp->available && new_opp->u_volt == opp->u_volt ? - 0 : -EEXIST; - - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", - __func__, opp->rate, opp->u_volt, opp->available, - new_opp->rate, new_opp->u_volt, new_opp->available); - goto free_opp; - } - -list_add: + new_opp->dynamic = dynamic; new_opp->dev_opp = dev_opp; list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock);
On 07/02/2015 11:45 PM, Viresh Kumar wrote:
On 02-07-15, 16:46, Stephen Boyd wrote:
Looking better.
And does this look perfect ? :)
Yep.
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Hi,
On Friday, July 03, 2015 12:15:54 PM Viresh Kumar wrote:
On 02-07-15, 16:46, Stephen Boyd wrote:
Looking better.
And does this look perfect ? :)
------------------8<----------------
Message-Id: 7c23b93b6b328e0b14b5c5096c3c3322e3d8ee4b.1435905782.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 12 Jun 2015 12:43:14 +0530 Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
There is no need to complicate _opp_add_dynamic() with allocation of dev_opp as well. Allocate it from _add_device_opp() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Later commits would add support for new OPP bindings and this would be required then. So, lets do it in a separate patch to make it easily reviewable.
Another change worth noticing is INIT_LIST_HEAD(&opp->node). We weren't doing it earlier as we never tried to delete a list node before it is added to list. But this wouldn't be the case anymore. We might try to delete a node (just to reuse the same code paths), without it being getting added to the list.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 115 +++++++++++++++++++++++++++++------------------ 1 file changed, 71 insertions(+), 44 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 4b646f36f252..2ac48ff9c1ef 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -484,6 +484,7 @@ static void _kfree_opp_rcu(struct rcu_head *head) * _opp_remove() - Remove an OPP from a table definition * @dev_opp: points back to the device_opp struct this opp belongs to * @opp: pointer to the OPP to remove + * @notify: OPP_EVENT_REMOVE notification should be sent or not * * This function removes an opp definition from the opp list. * @@ -492,13 +493,14 @@ static void _kfree_opp_rcu(struct rcu_head *head) * strategy. */ static void _opp_remove(struct device_opp *dev_opp, - struct dev_pm_opp *opp) + struct dev_pm_opp *opp, bool notify) { /* * Notify the changes in the availability of the operable * frequency/voltage list. */ - srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); + if (notify) + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); list_del_rcu(&opp->node); call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
@@ -544,12 +546,65 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) goto unlock; }
- _opp_remove(dev_opp, opp); + _opp_remove(dev_opp, opp, true); unlock: mutex_unlock(&dev_opp_list_lock); } EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+static struct dev_pm_opp *_allocate_opp(struct device *dev, + struct device_opp **dev_opp) +{ + struct dev_pm_opp *opp; + + /* allocate new OPP node */ + opp = kzalloc(sizeof(*opp), GFP_KERNEL); + if (!opp) + return NULL; + + INIT_LIST_HEAD(&opp->node); + + *dev_opp = _add_device_opp(dev); + if (!*dev_opp) { + kfree(opp); + return NULL; + } + + return opp; +} + +static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp) +{ + struct dev_pm_opp *opp = NULL; + struct list_head *head = &dev_opp->opp_list; + + /* + * Insert new OPP in order of increasing frequency + * and discard if already present + */ + list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) { + if (new_opp->rate <= opp->rate) + break; + + head = &opp->node; + } + + /* Duplicate OPPs ? */ + if (opp && new_opp->rate == opp->rate) { + dev_warn(dev_opp->dev, + "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", + __func__, opp->rate, opp->u_volt, opp->available, + new_opp->rate, new_opp->u_volt, new_opp->available); + return opp->available && new_opp->u_volt == opp->u_volt ? + 0 : -EEXIST; + } + + new_opp->dev_opp = dev_opp; + list_add_rcu(&new_opp->node, head); + + return 0; +} + /** * _opp_add_dynamic() - Allocate a dynamic OPP. * @dev: device for which we do this operation @@ -581,55 +636,28 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, long u_volt, bool dynamic) { struct device_opp *dev_opp; - struct dev_pm_opp *opp = NULL, *new_opp; - struct list_head *head; + struct dev_pm_opp *new_opp; int ret;
- /* allocate new OPP node */ - new_opp = kzalloc(sizeof(*new_opp), GFP_KERNEL); - if (!new_opp) - return -ENOMEM; - /* Hold our list modification lock here */ mutex_lock(&dev_opp_list_lock);
+ new_opp = _allocate_opp(dev, &dev_opp); + if (!new_opp) { + ret = -ENOMEM; + goto unlock; + } + /* populate the opp table */ new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true; + new_opp->dynamic = dynamic;
- dev_opp = _add_device_opp(dev); - if (!dev_opp) { - ret = -ENOMEM; + ret = _opp_add(new_opp, dev_opp); + if (ret) goto free_opp; - } - - /* - * Insert new OPP in order of increasing frequency - * and discard if already present - */ - head = &dev_opp->opp_list; - list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) { - if (new_opp->rate <= opp->rate) - break; - else - head = &opp->node; - } - - /* Duplicate OPPs ? */ - if (opp && new_opp->rate == opp->rate) { - ret = opp->available && new_opp->u_volt == opp->u_volt ? - 0 : -EEXIST; - - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", - __func__, opp->rate, opp->u_volt, opp->available, - new_opp->rate, new_opp->u_volt, new_opp->available); - goto remove_dev_opp; - }
- new_opp->dynamic = dynamic; - new_opp->dev_opp = dev_opp; - list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock);
/* @@ -639,11 +667,10 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0;
-remove_dev_opp: - _remove_device_opp(dev_opp); free_opp: + _opp_remove(dev_opp, new_opp, false); +unlock: mutex_unlock(&dev_opp_list_lock); - kfree(new_opp); return ret; }
@@ -871,7 +898,7 @@ void of_free_opp_table(struct device *dev) /* Free static OPPs */ list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { if (!opp->dynamic) - _opp_remove(dev_opp, opp); + _opp_remove(dev_opp, opp, true); }
mutex_unlock(&dev_opp_list_lock);
Hi,
On Monday, June 15, 2015 05:27:30 PM Viresh Kumar wrote:
Later commits would add support for new OPP bindings and this would be required then. So, lets do it in a separate patch to make it easily reviewable.
Another change worth noticing is INIT_LIST_HEAD(&opp->node). We weren't doing it earlier as we never tried to delete a list node before it is added to list. But this wouldn't be the case anymore. We might try to delete a node (just to reuse the same code paths), without it being getting added to the list.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
This adds support in OPP library to parse and create list of OPPs from operating-points-v2 bindings. It takes care of most of the properties of new bindings (except shared-opp, which will be handled separately).
For backward compatibility, we keep supporting earlier bindings. We try to search for the new bindings first, in case they aren't present we look for the old deprecated ones.
There are few things marked as TODO: - Support for multiple OPP tables - Support for multiple regulators
They should be fixed separately.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 213 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 2ac48ff9c1ef..3198c3e77224 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,12 +49,17 @@ * are protected by the dev_opp_list_lock for integrity. * IMPORTANT: the opp nodes should be maintained in increasing * order. - * @dynamic: not-created from static DT entries. * @available: true/false - marks if this OPP as available or not + * @dynamic: not-created from static DT entries. + * @turbo: true if turbo (boost) OPP * @rate: Frequency in hertz - * @u_volt: Nominal voltage in microvolts corresponding to this OPP + * @u_volt: Target voltage in microvolts corresponding to this OPP + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP + * @u_amp: Maximum current drawn by the device in microamperes * @dev_opp: points back to the device_opp struct this opp belongs to * @rcu_head: RCU callback head used for deferred freeing + * @np: OPP's device node. * * This structure stores the OPP information for a given device. */ @@ -63,11 +68,22 @@ struct dev_pm_opp {
bool available; bool dynamic; + bool turbo; unsigned long rate; + + /* + * Order in which u_volt{_min|_max} are present in this structure + * shouldn't be changed. + */ unsigned long u_volt; + unsigned long u_volt_min; + unsigned long u_volt_max; + unsigned long u_amp;
struct device_opp *dev_opp; struct rcu_head rcu_head; + + struct device_node *np; };
/** @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp, */ if (notify) srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); + list_del_rcu(&opp->node); call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
@@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, }
/** + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings) + * @dev: device for which we do this operation + * @np: device node + * + * This function adds an opp definition to the opp list and returns status. The + * opp can be controlled using dev_pm_opp_enable/disable functions and may be + * removed by dev_pm_opp_remove. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + * + * Return: + * 0 On success OR + * Duplicate OPPs (both freq and volt are same) and opp->available + * -EEXIST Freq are same and volt are different OR + * Duplicate OPPs (both freq and volt are same) and !opp->available + * -ENOMEM Memory allocation failure + * -EINVAL Failed parsing the OPP node + */ +static int _opp_add_static_v2(struct device *dev, struct device_node *np) +{ + struct device_opp *dev_opp; + struct dev_pm_opp *new_opp; + int ret; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + new_opp = _allocate_opp(dev, &dev_opp); + if (!new_opp) { + ret = -ENOMEM; + goto unlock; + } + + ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate); + if (ret < 0) { + dev_err(dev, "%s: opp-hz not found\n", __func__); + goto free_opp; + } + + if (of_get_property(np, "turbo-mode", NULL)) + new_opp->turbo = true; + + new_opp->np = np; + new_opp->dynamic = false; + new_opp->available = true; + + /* + * TODO: Support multiple regulators + * + * read opp-microvolt array + */ + ret = of_property_count_u32_elems(np, "opp-microvolt"); + if (ret == 1 || ret == 3) { + /* There can be one or three elements here */ + ret = of_property_read_u32_array(np, "opp-microvolt", + (u32 *)&new_opp->u_volt, ret); + if (ret) { + dev_err(dev, "%s: error parsing opp-microvolt: %d\n", + __func__, ret); + goto free_opp; + } + } + + of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp); + + ret = _opp_add(new_opp, dev_opp); + if (ret) + goto free_opp; + + pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n", + __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, + new_opp->u_volt_min, new_opp->u_volt_max); + + mutex_unlock(&dev_opp_list_lock); + + /* + * Notify the changes in the availability of the operable + * frequency/voltage list. + */ + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); + return 0; + +free_opp: + _opp_remove(dev_opp, new_opp, false); +unlock: + mutex_unlock(&dev_opp_list_lock); + return ret; +} + +/** * dev_pm_opp_add() - Add an OPP table from a table definitions * @dev: device for which we do this operation * @freq: Frequency in Hz for this OPP @@ -905,29 +1016,64 @@ void of_free_opp_table(struct device *dev) } EXPORT_SYMBOL_GPL(of_free_opp_table);
-/** - * of_init_opp_table() - Initialize opp table from device tree - * @dev: device pointer used to lookup device OPPs. - * - * Register the initial OPP table with the OPP library for given device. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * Hence this function indirectly uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - * - * Return: - * 0 On success OR - * Duplicate OPPs (both freq and volt are same) and opp->available - * -EEXIST Freq are same and volt are different OR - * Duplicate OPPs (both freq and volt are same) and !opp->available - * -ENOMEM Memory allocation failure - * -ENODEV when 'operating-points' property is not found or is invalid data - * in device node. - * -ENODATA when empty 'operating-points' property is found - */ -int of_init_opp_table(struct device *dev) +/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */ +static struct device_node * +_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop) +{ + struct device_node *opp_np; + + opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value)); + if (!opp_np) { + dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n", + __func__, prop->name); + return ERR_PTR(-EINVAL); + } + + return opp_np; +} + +/* Initializes OPP tables based on new bindings */ +static int _of_init_opp_table_v2(struct device *dev, + const struct property *prop) +{ + struct device_node *opp_np, *np; + int ret = 0, count = 0; + + if (!prop->value) + return -ENODATA; + + /* Get opp node */ + opp_np = _of_get_opp_desc_node_from_prop(dev, prop); + if (IS_ERR(opp_np)) + return PTR_ERR(opp_np); + + /* We have opp-list node now, iterate over it and add OPPs */ + for_each_available_child_of_node(opp_np, np) { + count++; + + ret = _opp_add_static_v2(dev, np); + if (ret) { + dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, + ret); + break; + } + } + + /* There should be one of more OPP defined */ + if (WARN_ON(!count)) + goto put_opp_np; + + if (ret) + of_free_opp_table(dev); + +put_opp_np: + of_node_put(opp_np); + + return ret; +} + +/* Initializes OPP tables based on old-deprecated bindings */ +static int _of_init_opp_table_v1(struct device *dev) { const struct property *prop; const __be32 *val; @@ -962,5 +1108,47 @@ int of_init_opp_table(struct device *dev)
return 0; } + +/** + * of_init_opp_table() - Initialize opp table from device tree + * @dev: device pointer used to lookup device OPPs. + * + * Register the initial OPP table with the OPP library for given device. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function indirectly uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + * + * Return: + * 0 On success OR + * Duplicate OPPs (both freq and volt are same) and opp->available + * -EEXIST Freq are same and volt are different OR + * Duplicate OPPs (both freq and volt are same) and !opp->available + * -ENOMEM Memory allocation failure + * -ENODEV when 'operating-points' property is not found or is invalid data + * in device node. + * -ENODATA when empty 'operating-points' property is found + */ +int of_init_opp_table(struct device *dev) +{ + const struct property *prop; + + /* + * OPPs have two version of bindings now. The older one is deprecated, + * try for the new binding first. + */ + prop = of_find_property(dev->of_node, "operating-points-v2", NULL); + if (!prop) { + /* + * Try old-deprecated bindings for backward compatibility with + * older dtbs. + */ + return _of_init_opp_table_v1(dev); + } + + return _of_init_opp_table_v2(dev, prop); +} EXPORT_SYMBOL_GPL(of_init_opp_table); #endif
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 213 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 2ac48ff9c1ef..3198c3e77224 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,12 +49,17 @@
are protected by the dev_opp_list_lock for integrity.
IMPORTANT: the opp nodes should be maintained in increasing
order.
- @dynamic: not-created from static DT entries.
- @available: true/false - marks if this OPP as available or not
- @dynamic: not-created from static DT entries.
Why move dynamic?
- @turbo: true if turbo (boost) OPP
- @rate: Frequency in hertz
- @u_volt: Nominal voltage in microvolts corresponding to this OPP
- @u_volt: Target voltage in microvolts corresponding to this OPP
- @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
- @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
- @u_amp: Maximum current drawn by the device in microamperes
- @dev_opp: points back to the device_opp struct this opp belongs to
- @rcu_head: RCU callback head used for deferred freeing
*/
- @np: OPP's device node.
- This structure stores the OPP information for a given device.
@@ -63,11 +68,22 @@ struct dev_pm_opp { bool available; bool dynamic;
- bool turbo; unsigned long rate;
- /*
* Order in which u_volt{_min|_max} are present in this structure
* shouldn't be changed.
unsigned long u_volt;*/
- unsigned long u_volt_min;
- unsigned long u_volt_max;
- unsigned long u_amp;
struct device_opp *dev_opp; struct rcu_head rcu_head;
- struct device_node *np;
}; /** @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp, */ if (notify) srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
- list_del_rcu(&opp->node); call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
Please remove this hunk of noise.
@@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, } /**
- _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
- @dev: device for which we do this operation
- @np: device node
- This function adds an opp definition to the opp list and returns status. The
- opp can be controlled using dev_pm_opp_enable/disable functions and may be
- removed by dev_pm_opp_remove.
- Locking: The internal device_opp and opp structures are RCU protected.
- Hence this function internally uses RCU updater strategy with mutex locks
- to keep the integrity of the internal data structures. Callers should ensure
- that this function is *NOT* called under RCU protection or in contexts where
- mutex cannot be locked.
- Return:
- 0 On success OR
Duplicate OPPs (both freq and volt are same) and opp->available
- -EEXIST Freq are same and volt are different OR
Duplicate OPPs (both freq and volt are same) and !opp->available
- -ENOMEM Memory allocation failure
- -EINVAL Failed parsing the OPP node
- */
+static int _opp_add_static_v2(struct device *dev, struct device_node *np) +{
- struct device_opp *dev_opp;
- struct dev_pm_opp *new_opp;
- int ret;
- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- new_opp = _allocate_opp(dev, &dev_opp);
- if (!new_opp) {
ret = -ENOMEM;
goto unlock;
- }
- ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
- if (ret < 0) {
dev_err(dev, "%s: opp-hz not found\n", __func__);
goto free_opp;
- }
- if (of_get_property(np, "turbo-mode", NULL))
new_opp->turbo = true;
new_opp->turbo = of_property_read_bool(np, "turbo-mode");
- new_opp->np = np;
- new_opp->dynamic = false;
- new_opp->available = true;
- /*
* TODO: Support multiple regulators
*
* read opp-microvolt array
*/
- ret = of_property_count_u32_elems(np, "opp-microvolt");
- if (ret == 1 || ret == 3) {
/* There can be one or three elements here */
ret = of_property_read_u32_array(np, "opp-microvolt",
(u32 *)&new_opp->u_volt, ret);
It seems fragile to rely on the struct packing here. Maybe the same comment in the struct should be copied here, and possibly some better way of doing this so the code can't be subtly broken?
if (ret) {
dev_err(dev, "%s: error parsing opp-microvolt: %d\n",
__func__, ret);
goto free_opp;
}
- }
- of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
- ret = _opp_add(new_opp, dev_opp);
- if (ret)
goto free_opp;
- pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
__func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
new_opp->u_volt_min, new_opp->u_volt_max);
- mutex_unlock(&dev_opp_list_lock);
We can pr_debug after the unlock?
On 01-07-15, 18:13, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 213 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 2ac48ff9c1ef..3198c3e77224 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,12 +49,17 @@
are protected by the dev_opp_list_lock for integrity.
IMPORTANT: the opp nodes should be maintained in increasing
order.
- @dynamic: not-created from static DT entries.
- @available: true/false - marks if this OPP as available or not
- @dynamic: not-created from static DT entries.
Why move dynamic?
To match its position, as it is present in the struct below. Yes it could have been done in a separate patch, but its also fine to fix such silly mistakes in another patch :)
- @turbo: true if turbo (boost) OPP
- @rate: Frequency in hertz
- @u_volt: Nominal voltage in microvolts corresponding to this OPP
- @u_volt: Target voltage in microvolts corresponding to this OPP
- @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
- @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
- @u_amp: Maximum current drawn by the device in microamperes
- @dev_opp: points back to the device_opp struct this opp belongs to
- @rcu_head: RCU callback head used for deferred freeing
*/
- @np: OPP's device node.
- This structure stores the OPP information for a given device.
@@ -63,11 +68,22 @@ struct dev_pm_opp { bool available; bool dynamic;
- bool turbo; unsigned long rate;
- /*
* Order in which u_volt{_min|_max} are present in this structure
* shouldn't be changed.
unsigned long u_volt;*/
- unsigned long u_volt_min;
- unsigned long u_volt_max;
- unsigned long u_amp;
struct device_opp *dev_opp; struct rcu_head rcu_head;
- struct device_node *np;
}; /** @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp, */ if (notify) srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
- list_del_rcu(&opp->node); call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
Please remove this hunk of noise.
Sigh
@@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, } /**
- _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
- @dev: device for which we do this operation
- @np: device node
- This function adds an opp definition to the opp list and returns status. The
- opp can be controlled using dev_pm_opp_enable/disable functions and may be
- removed by dev_pm_opp_remove.
- Locking: The internal device_opp and opp structures are RCU protected.
- Hence this function internally uses RCU updater strategy with mutex locks
- to keep the integrity of the internal data structures. Callers should ensure
- that this function is *NOT* called under RCU protection or in contexts where
- mutex cannot be locked.
- Return:
- 0 On success OR
Duplicate OPPs (both freq and volt are same) and opp->available
- -EEXIST Freq are same and volt are different OR
Duplicate OPPs (both freq and volt are same) and !opp->available
- -ENOMEM Memory allocation failure
- -EINVAL Failed parsing the OPP node
- */
+static int _opp_add_static_v2(struct device *dev, struct device_node *np) +{
- struct device_opp *dev_opp;
- struct dev_pm_opp *new_opp;
- int ret;
- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- new_opp = _allocate_opp(dev, &dev_opp);
- if (!new_opp) {
ret = -ENOMEM;
goto unlock;
- }
- ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
- if (ret < 0) {
dev_err(dev, "%s: opp-hz not found\n", __func__);
goto free_opp;
- }
- if (of_get_property(np, "turbo-mode", NULL))
new_opp->turbo = true;
new_opp->turbo = of_property_read_bool(np, "turbo-mode");
Sure.
- new_opp->np = np;
- new_opp->dynamic = false;
- new_opp->available = true;
- /*
* TODO: Support multiple regulators
*
* read opp-microvolt array
*/
- ret = of_property_count_u32_elems(np, "opp-microvolt");
- if (ret == 1 || ret == 3) {
/* There can be one or three elements here */
ret = of_property_read_u32_array(np, "opp-microvolt",
(u32 *)&new_opp->u_volt, ret);
It seems fragile to rely on the struct packing here. Maybe the same comment in the struct should be copied here, and possibly some better way of doing this so the code can't be subtly broken?
Any example of how things will break? Aren't these guaranteed to be present at 3 consecutive 32 bit positions ?
- pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
__func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
new_opp->u_volt_min, new_opp->u_volt_max);
- mutex_unlock(&dev_opp_list_lock);
We can pr_debug after the unlock?
Okay
On 07/01/2015 11:38 PM, Viresh Kumar wrote:
On 01-07-15, 18:13, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 213 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 2ac48ff9c1ef..3198c3e77224 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,12 +49,17 @@
are protected by the dev_opp_list_lock for integrity.
IMPORTANT: the opp nodes should be maintained in increasing
order.
- @dynamic: not-created from static DT entries.
- @available: true/false - marks if this OPP as available or not
- @dynamic: not-created from static DT entries.
Why move dynamic?
To match its position, as it is present in the struct below. Yes it could have been done in a separate patch, but its also fine to fix such silly mistakes in another patch :)
Oh I thought kernel-doc didn't care what order things were documented in, so moving it around doesn't really help unless someone cares that they match the struct ordering.
- new_opp->np = np;
- new_opp->dynamic = false;
- new_opp->available = true;
- /*
* TODO: Support multiple regulators
*
* read opp-microvolt array
*/
- ret = of_property_count_u32_elems(np, "opp-microvolt");
- if (ret == 1 || ret == 3) {
/* There can be one or three elements here */
ret = of_property_read_u32_array(np, "opp-microvolt",
(u32 *)&new_opp->u_volt, ret);
It seems fragile to rely on the struct packing here. Maybe the same comment in the struct should be copied here, and possibly some better way of doing this so the code can't be subtly broken?
Any example of how things will break? Aren't these guaranteed to be present at 3 consecutive 32 bit positions ?
I'm mostly worried about someone jumbling fields in the struct. Maybe I'm too paranoid... Maybe we can have some sort of BUILD_BUG_ON() check here?
BUILD_BUG_ON(offsetof(struct dev_pm_opp, u_volt_max) - offsetof(struct dev_pm_opp, u_volt) != sizeof(u32) * 3);
Have you tried compiling that on 64-bit? sizeof(unsigned long) != sizeof(u32).
On 02-07-15, 09:07, Stephen Boyd wrote:
Oh I thought kernel-doc didn't care what order things were documented in, so moving it around doesn't really help unless someone cares that they match the struct ordering.
That was just for better readability, nothing more. And I really prefer that. Consider a case where we have 15 fields in a struct and the doc comment is all mixed up. Would be very difficult to read explanations of individual fields. Of course you can do a simple search in vim to match fields with comments, but its really good if we follow the same order in comments as well.
Anyway, will drop it from this patch at least :)
- new_opp->np = np;
- new_opp->dynamic = false;
- new_opp->available = true;
- /*
* TODO: Support multiple regulators
*
* read opp-microvolt array
*/
- ret = of_property_count_u32_elems(np, "opp-microvolt");
- if (ret == 1 || ret == 3) {
/* There can be one or three elements here */
ret = of_property_read_u32_array(np, "opp-microvolt",
(u32 *)&new_opp->u_volt, ret);
It seems fragile to rely on the struct packing here. Maybe the same comment in the struct should be copied here, and possibly some better way of doing this so the code can't be subtly broken?
Any example of how things will break? Aren't these guaranteed to be present at 3 consecutive 32 bit positions ?
I'm mostly worried about someone jumbling fields in the struct. Maybe I'm too paranoid... Maybe we can have some sort of BUILD_BUG_ON() check here?
BUILD_BUG_ON(offsetof(struct dev_pm_opp, u_volt_max) - offsetof(struct dev_pm_opp, u_volt) != sizeof(u32) * 3);
Have you tried compiling that on 64-bit? sizeof(unsigned long) != sizeof(u32).
I see how it will break for 64 bit systems now, fixed it now.
-------------------8<---------------
Message-Id: dc35329bc328e0ecb8534a3e1709a0d12cadb311.1435903622.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Mon, 15 Jun 2015 16:27:21 +0530 Subject: [PATCH] opp: Add support to parse "operating-points-v2" bindings
This adds support in OPP library to parse and create list of OPPs from operating-points-v2 bindings. It takes care of most of the properties of new bindings (except shared-opp, which will be handled separately).
For backward compatibility, we keep supporting earlier bindings. We try to search for the new bindings first, in case they aren't present we look for the old deprecated ones.
There are few things marked as TODO: - Support for multiple OPP tables - Support for multiple regulators
They should be fixed separately.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 249 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 225 insertions(+), 24 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index df5e5f14c368..c5829c86aa2a 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -51,10 +51,15 @@ * order. * @dynamic: not-created from static DT entries. * @available: true/false - marks if this OPP as available or not + * @turbo: true if turbo (boost) OPP * @rate: Frequency in hertz - * @u_volt: Nominal voltage in microvolts corresponding to this OPP + * @u_volt: Target voltage in microvolts corresponding to this OPP + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP + * @u_amp: Maximum current drawn by the device in microamperes * @dev_opp: points back to the device_opp struct this opp belongs to * @rcu_head: RCU callback head used for deferred freeing + * @np: OPP's device node. * * This structure stores the OPP information for a given device. */ @@ -63,11 +68,18 @@ struct dev_pm_opp {
bool available; bool dynamic; + bool turbo; unsigned long rate; + unsigned long u_volt; + unsigned long u_volt_min; + unsigned long u_volt_max; + unsigned long u_amp;
struct device_opp *dev_opp; struct rcu_head rcu_head; + + struct device_node *np; };
/** @@ -674,6 +686,118 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, return ret; }
+/* TODO: Support multiple regulators */ +static int opp_get_microvolt(struct dev_pm_opp *opp, struct device *dev) +{ + u32 microvolt[3] = {0}; + int count, ret; + + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); + if (!count) + return 0; + + /* There can be one or three elements here */ + if (count != 1 && count != 3) { + dev_err(dev, "%s: Invalid number of elements in opp-microvolt property (%d)\n", + __func__, count); + return -EINVAL; + } + + ret = of_property_read_u32_array(opp->np, "opp-microvolt", microvolt, + count); + if (ret) { + dev_err(dev, "%s: error parsing opp-microvolt: %d\n", __func__, + ret); + return -EINVAL; + } + + opp->u_volt = microvolt[0]; + opp->u_volt_min = microvolt[1]; + opp->u_volt_max = microvolt[2]; + + return 0; +} + +/** + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings) + * @dev: device for which we do this operation + * @np: device node + * + * This function adds an opp definition to the opp list and returns status. The + * opp can be controlled using dev_pm_opp_enable/disable functions and may be + * removed by dev_pm_opp_remove. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + * + * Return: + * 0 On success OR + * Duplicate OPPs (both freq and volt are same) and opp->available + * -EEXIST Freq are same and volt are different OR + * Duplicate OPPs (both freq and volt are same) and !opp->available + * -ENOMEM Memory allocation failure + * -EINVAL Failed parsing the OPP node + */ +static int _opp_add_static_v2(struct device *dev, struct device_node *np) +{ + struct device_opp *dev_opp; + struct dev_pm_opp *new_opp; + int ret; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + new_opp = _allocate_opp(dev, &dev_opp); + if (!new_opp) { + ret = -ENOMEM; + goto unlock; + } + + ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate); + if (ret < 0) { + dev_err(dev, "%s: opp-hz not found\n", __func__); + goto free_opp; + } + + new_opp->turbo = of_property_read_bool(np, "turbo-mode"); + + new_opp->np = np; + new_opp->dynamic = false; + new_opp->available = true; + + ret = opp_get_microvolt(new_opp, dev); + if (ret) + goto free_opp; + + of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp); + + ret = _opp_add(new_opp, dev_opp); + if (ret) + goto free_opp; + + mutex_unlock(&dev_opp_list_lock); + + pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n", + __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, + new_opp->u_volt_min, new_opp->u_volt_max); + + /* + * Notify the changes in the availability of the operable + * frequency/voltage list. + */ + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); + return 0; + +free_opp: + _opp_remove(dev_opp, new_opp, false); +unlock: + mutex_unlock(&dev_opp_list_lock); + return ret; +} + /** * dev_pm_opp_add() - Add an OPP table from a table definitions * @dev: device for which we do this operation @@ -905,29 +1029,64 @@ void of_free_opp_table(struct device *dev) } EXPORT_SYMBOL_GPL(of_free_opp_table);
-/** - * of_init_opp_table() - Initialize opp table from device tree - * @dev: device pointer used to lookup device OPPs. - * - * Register the initial OPP table with the OPP library for given device. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * Hence this function indirectly uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - * - * Return: - * 0 On success OR - * Duplicate OPPs (both freq and volt are same) and opp->available - * -EEXIST Freq are same and volt are different OR - * Duplicate OPPs (both freq and volt are same) and !opp->available - * -ENOMEM Memory allocation failure - * -ENODEV when 'operating-points' property is not found or is invalid data - * in device node. - * -ENODATA when empty 'operating-points' property is found - */ -int of_init_opp_table(struct device *dev) +/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */ +static struct device_node * +_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop) +{ + struct device_node *opp_np; + + opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value)); + if (!opp_np) { + dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n", + __func__, prop->name); + return ERR_PTR(-EINVAL); + } + + return opp_np; +} + +/* Initializes OPP tables based on new bindings */ +static int _of_init_opp_table_v2(struct device *dev, + const struct property *prop) +{ + struct device_node *opp_np, *np; + int ret = 0, count = 0; + + if (!prop->value) + return -ENODATA; + + /* Get opp node */ + opp_np = _of_get_opp_desc_node_from_prop(dev, prop); + if (IS_ERR(opp_np)) + return PTR_ERR(opp_np); + + /* We have opp-list node now, iterate over it and add OPPs */ + for_each_available_child_of_node(opp_np, np) { + count++; + + ret = _opp_add_static_v2(dev, np); + if (ret) { + dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, + ret); + break; + } + } + + /* There should be one of more OPP defined */ + if (WARN_ON(!count)) + goto put_opp_np; + + if (ret) + of_free_opp_table(dev); + +put_opp_np: + of_node_put(opp_np); + + return ret; +} + +/* Initializes OPP tables based on old-deprecated bindings */ +static int _of_init_opp_table_v1(struct device *dev) { const struct property *prop; const __be32 *val; @@ -962,5 +1121,47 @@ int of_init_opp_table(struct device *dev)
return 0; } + +/** + * of_init_opp_table() - Initialize opp table from device tree + * @dev: device pointer used to lookup device OPPs. + * + * Register the initial OPP table with the OPP library for given device. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function indirectly uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + * + * Return: + * 0 On success OR + * Duplicate OPPs (both freq and volt are same) and opp->available + * -EEXIST Freq are same and volt are different OR + * Duplicate OPPs (both freq and volt are same) and !opp->available + * -ENOMEM Memory allocation failure + * -ENODEV when 'operating-points' property is not found or is invalid data + * in device node. + * -ENODATA when empty 'operating-points' property is found + */ +int of_init_opp_table(struct device *dev) +{ + const struct property *prop; + + /* + * OPPs have two version of bindings now. The older one is deprecated, + * try for the new binding first. + */ + prop = of_find_property(dev->of_node, "operating-points-v2", NULL); + if (!prop) { + /* + * Try old-deprecated bindings for backward compatibility with + * older dtbs. + */ + return _of_init_opp_table_v1(dev); + } + + return _of_init_opp_table_v2(dev, prop); +} EXPORT_SYMBOL_GPL(of_init_opp_table); #endif
Hi,
On Monday, June 15, 2015 05:27:31 PM Viresh Kumar wrote:
This adds support in OPP library to parse and create list of OPPs from operating-points-v2 bindings. It takes care of most of the properties of new bindings (except shared-opp, which will be handled separately).
For backward compatibility, we keep supporting earlier bindings. We try to search for the new bindings first, in case they aren't present we look for the old deprecated ones.
There are few things marked as TODO:
- Support for multiple OPP tables
- Support for multiple regulators
They should be fixed separately.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 213 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 2ac48ff9c1ef..3198c3e77224 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c
[...]
@@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, } /**
- _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
- @dev: device for which we do this operation
- @np: device node
- This function adds an opp definition to the opp list and returns status. The
- opp can be controlled using dev_pm_opp_enable/disable functions and may be
- removed by dev_pm_opp_remove.
- Locking: The internal device_opp and opp structures are RCU protected.
- Hence this function internally uses RCU updater strategy with mutex locks
- to keep the integrity of the internal data structures. Callers should ensure
- that this function is *NOT* called under RCU protection or in contexts where
- mutex cannot be locked.
- Return:
- 0 On success OR
Duplicate OPPs (both freq and volt are same) and opp->available
- -EEXIST Freq are same and volt are different OR
Duplicate OPPs (both freq and volt are same) and !opp->available
- -ENOMEM Memory allocation failure
- -EINVAL Failed parsing the OPP node
- */
+static int _opp_add_static_v2(struct device *dev, struct device_node *np) +{
- struct device_opp *dev_opp;
- struct dev_pm_opp *new_opp;
- int ret;
- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- new_opp = _allocate_opp(dev, &dev_opp);
- if (!new_opp) {
ret = -ENOMEM;
goto unlock;
- }
- ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
- if (ret < 0) {
dev_err(dev, "%s: opp-hz not found\n", __func__);
goto free_opp;
- }
Isn't using u32 for storing frequency (in Hz) too small by today's standards?
[ Please note that the old v1 binding uses kHz not Hz. ]
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
Isn't using u32 for storing frequency (in Hz) too small by today's standards?
[ Please note that the old v1 binding uses kHz not Hz. ]
Hmm, probably yes we need it to be u64. Thanks for pointing out.
Hi,
On Thursday, July 09, 2015 10:48:46 AM Viresh Kumar wrote:
On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
Isn't using u32 for storing frequency (in Hz) too small by today's standards?
[ Please note that the old v1 binding uses kHz not Hz. ]
Hmm, probably yes we need it to be u64. Thanks for pointing out.
There is also a minor issue with of_init_opp_table() documentation (the function can now return -EINVAL in some cases). Except these two things the patch looks fine and once it is fixed you can add:
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 24-07-15, 20:02, Bartlomiej Zolnierkiewicz wrote:
There is also a minor issue with of_init_opp_table() documentation (the function can now return -EINVAL in some cases). Except these two things the patch looks fine and once it is fixed you can add:
Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com
Fixed the documentation issue as:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index c4ca176061b8..3ad93a7e76e5 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -1144,6 +1144,7 @@ static int _of_init_opp_table_v1(struct device *dev) * -ENODEV when 'operating-points' property is not found or is invalid data * in device node. * -ENODATA when empty 'operating-points' property is found + * -EINVAL when invalid entries are found in opp-v2 table */ int of_init_opp_table(struct device *dev) {
Please give your reviewed-by again as I wouldn't be fixing the u32 type issue for now and wasn't sure if you will give your RBY without that :)
On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
Isn't using u32 for storing frequency (in Hz) too small by today's standards?
[ Please note that the old v1 binding uses kHz not Hz. ]
I have thought about this a bit more and I am somewhat confused. Yes I agree that u32 isn't big enough for frequencies in Hz, i.e. Max value of 4294967295 ~ 4.29 GHz.
But the bigger problem lies with the clk API that we have today. It declares clk-rate as a unsigned-long which is 32 bits on a 32 bit machine and 64 bits on a 64 bit machine. And every single piece of code reading "clock-frequency" DT property, reads it as a 32 bit value as we reserve only a single cell for it.
Now, if we wanna change that, we need to start changing from the clk-API itself and that's not gonna be a small task and I would leave it to Mike/Stephen for obvious reasons :)
So, I will keep this code in sync with rest of the kernel and lets see what Mike has to say.
On 07/26/2015 08:02 PM, Viresh Kumar wrote:
On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
Isn't using u32 for storing frequency (in Hz) too small by today's standards?
[ Please note that the old v1 binding uses kHz not Hz. ]
I have thought about this a bit more and I am somewhat confused. Yes I agree that u32 isn't big enough for frequencies in Hz, i.e. Max value of 4294967295 ~ 4.29 GHz.
But the bigger problem lies with the clk API that we have today. It declares clk-rate as a unsigned-long which is 32 bits on a 32 bit machine and 64 bits on a 64 bit machine. And every single piece of code reading "clock-frequency" DT property, reads it as a 32 bit value as we reserve only a single cell for it.
Now, if we wanna change that, we need to start changing from the clk-API itself and that's not gonna be a small task and I would leave it to Mike/Stephen for obvious reasons :)
So, I will keep this code in sync with rest of the kernel and lets see what Mike has to say.
There is talk about moving to 64 bits for the frequency in the clk API. It's not actively being developed though and I'm not sure when we'll get there. From a DT perspective though, I don't see why it would be bad to have two cells instead of one cell for the frequency here. It would allow up to 64 bits of frequency so that when we change the clk API we won't need to do anything in the OPP bindings to handle it. Just because we have problems in the kernel doesn't mean we should put the same problems into DT.
On 28-07-15, 16:03, Stephen Boyd wrote:
There is talk about moving to 64 bits for the frequency in the clk API. It's not actively being developed though and I'm not sure when we'll get there. From a DT perspective though, I don't see why it would be bad to have two cells instead of one cell for the frequency here. It would allow up to 64 bits of frequency so that when we change the clk API we won't need to do anything in the OPP bindings to handle it. Just because we have problems in the kernel doesn't mean we should put the same problems into DT.
Okay, I am going to send a patch for that as well and will respin the series again.
On 28-07-15, 16:03, Stephen Boyd wrote:
There is talk about moving to 64 bits for the frequency in the clk API. It's not actively being developed though and I'm not sure when we'll get there. From a DT perspective though, I don't see why it would be bad to have two cells instead of one cell for the frequency here. It would allow up to 64 bits of frequency so that when we change the clk API we won't need to do anything in the OPP bindings to handle it. Just because we have problems in the kernel doesn't mean we should put the same problems into DT.
Updated the code as:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index d9ebbd739b23..73e4e1ce60a1 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -743,6 +743,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) { struct device_opp *dev_opp; struct dev_pm_opp *new_opp; + u64 rate; int ret;
/* Hold our list modification lock here */ @@ -754,12 +755,18 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) goto unlock; }
- ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate); + ret = of_property_read_u64(np, "opp-hz", &rate); if (ret < 0) { dev_err(dev, "%s: opp-hz not found\n", __func__); goto free_opp; }
+ /* + * Rate is defined as an unsigned long in clk API, and so casting + * explicitly to its type. Must be fixed once rate is 64 bit + * guaranteed in clk API. + */ + new_opp->rate = (unsigned long)rate; new_opp->turbo = of_property_read_bool(np, "turbo-mode");
new_opp->np = np;
With "operating-points-v2" bindings, clock-latency is defined per OPP. Users of this value expect a single value which defines the latency to switch to any clock rate. Find maximum clock-latency-ns from the OPP table to service requests from such users.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++-- include/linux/pm_opp.h | 6 ++++++ 2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 3198c3e77224..fe79e5146a29 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -57,6 +57,8 @@ * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP * @u_amp: Maximum current drawn by the device in microamperes + * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's + * frequency from any other OPP's frequency. * @dev_opp: points back to the device_opp struct this opp belongs to * @rcu_head: RCU callback head used for deferred freeing * @np: OPP's device node. @@ -79,6 +81,7 @@ struct dev_pm_opp { unsigned long u_volt_min; unsigned long u_volt_max; unsigned long u_amp; + unsigned long clock_latency_ns;
struct device_opp *dev_opp; struct rcu_head rcu_head; @@ -113,6 +116,8 @@ struct device_opp { struct srcu_notifier_head srcu_head; struct rcu_head rcu_head; struct list_head opp_list; + + unsigned long clock_latency_ns_max; };
/* @@ -230,6 +235,32 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp) EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
/** + * dev_pm_opp_get_max_clock_latency() - Get max clock latency in nanoseconds + * @dev: device for which we do this operation + * + * Return: This function returns the max clock latency in nanoseconds. + * + * Locking: This function takes rcu_read_lock(). + */ +unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev) +{ + struct device_opp *dev_opp; + unsigned long clock_latency_ns; + + rcu_read_lock(); + + dev_opp = _find_device_opp(dev); + if (IS_ERR(dev_opp)) + clock_latency_ns = 0; + else + clock_latency_ns = dev_opp->clock_latency_ns_max; + + rcu_read_unlock(); + return clock_latency_ns; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency); + +/** * dev_pm_opp_get_opp_count() - Get number of opps available in the opp list * @dev: device for which we do this operation * @@ -741,6 +772,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) new_opp->np = np; new_opp->dynamic = false; new_opp->available = true; + of_property_read_u32(np, "clock-latency-ns", + (u32 *)&new_opp->clock_latency_ns);
/* * TODO: Support multiple regulators @@ -765,9 +798,13 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) if (ret) goto free_opp;
- pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n", + if (new_opp->clock_latency_ns > dev_opp->clock_latency_ns_max) + dev_opp->clock_latency_ns_max = new_opp->clock_latency_ns; + + pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, - new_opp->u_volt_min, new_opp->u_volt_max); + new_opp->u_volt_min, new_opp->u_volt_max, + new_opp->clock_latency_ns);
mutex_unlock(&dev_opp_list_lock);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index cec2d4540914..20324b579adc 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -31,6 +31,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp); unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
int dev_pm_opp_get_opp_count(struct device *dev); +unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev, unsigned long freq, @@ -67,6 +68,11 @@ static inline int dev_pm_opp_get_opp_count(struct device *dev) return 0; }
+static inline unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev) +{ + return 0; +} + static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev, unsigned long freq, bool available) {
On 06/15, Viresh Kumar wrote:
With "operating-points-v2" bindings, clock-latency is defined per OPP. Users of this value expect a single value which defines the latency to switch to any clock rate. Find maximum clock-latency-ns from the OPP table to service requests from such users.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
An opp can be shared by multiple devices, for example its very common for CPUs to share the OPPs, i.e. when they share clock/voltage rails.
This patch adds support of shared OPPs to the OPP library.
Instead of a single device, dev_opp will not contain a list of devices that use it. It also senses if the device (we are trying to initialize OPPs for) shares OPPs with a device added earlier and in that case we update the list of devices managed by OPPs instead of duplicating OPPs again.
The same infrastructure will be used for the old OPP bindings, with later patches.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 176 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 152 insertions(+), 24 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index fe79e5146a29..6b554e417b1f 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -90,16 +90,33 @@ struct dev_pm_opp { };
/** + * struct device_list_opp - devices managed by 'struct device_opp' + * @node: list node + * @dev: device to which the struct object belongs + * @rcu_head: RCU callback head used for deferred freeing + * + * This is an internal data structure maintaining the list of devices that are + * managed by 'struct device_opp'. + */ +struct device_list_opp { + struct list_head node; + struct device *dev; + struct rcu_head rcu_head; +}; + +/** * struct device_opp - Device opp structure * @node: list node - contains the devices with OPPs that * have been registered. Nodes once added are not modified in this * list. * RCU usage: nodes are not modified in the list of device_opp, * however addition is possible and is secured by dev_opp_list_lock - * @dev: device pointer * @srcu_head: notifier head to notify the OPP availability changes. * @rcu_head: RCU callback head used for deferred freeing + * @dev_list: list of devices that share these OPPs * @opp_list: list of opps + * @np: struct device_node pointer for opp's DT node. + * @shared_opp: OPP is shared between multiple devices. * * This is an internal data structure maintaining the link to opps attached to * a device. This structure is not meant to be shared to users as it is @@ -112,12 +129,14 @@ struct dev_pm_opp { struct device_opp { struct list_head node;
- struct device *dev; struct srcu_notifier_head srcu_head; struct rcu_head rcu_head; + struct list_head dev_list; struct list_head opp_list;
+ struct device_node *np; unsigned long clock_latency_ns_max; + bool shared_opp; };
/* @@ -137,6 +156,40 @@ do { \ "dev_opp_list_lock protection"); \ } while (0)
+static struct device_list_opp *_find_list_dev(struct device *dev, + struct device_opp *dev_opp) +{ + struct device_list_opp *list_dev; + + list_for_each_entry(list_dev, &dev_opp->dev_list, node) + if (list_dev->dev == dev) + return list_dev; + + return NULL; +} + +static struct device_opp *_managed_opp(struct device_node *np) +{ + struct device_opp *dev_opp; + + list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) + if (dev_opp->np == np) { + /* + * Multiple devices can point to the same OPP table and + * so will have same node-pointer, np. + * + * But the OPPs will be considered as shared only if the + * OPP table contains a "opp-shared" property. + */ + if (dev_opp->shared_opp) + return dev_opp; + else + return NULL; + } + + return NULL; +} + /** * _find_device_opp() - find device_opp struct using device pointer * @dev: device pointer used to lookup device OPPs @@ -153,21 +206,18 @@ do { \ */ static struct device_opp *_find_device_opp(struct device *dev) { - struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV); + struct device_opp *dev_opp;
if (unlikely(IS_ERR_OR_NULL(dev))) { pr_err("%s: Invalid parameters\n", __func__); return ERR_PTR(-EINVAL); }
- list_for_each_entry_rcu(tmp_dev_opp, &dev_opp_list, node) { - if (tmp_dev_opp->dev == dev) { - dev_opp = tmp_dev_opp; - break; - } - } + list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) + if (_find_list_dev(dev, dev_opp)) + return dev_opp;
- return dev_opp; + return ERR_PTR(-ENODEV); }
/** @@ -454,6 +504,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
+/* List-dev Helpers */ +static void _kfree_list_dev_rcu(struct rcu_head *head) +{ + struct device_list_opp *list_dev; + + list_dev = container_of(head, struct device_list_opp, rcu_head); + kfree_rcu(list_dev, rcu_head); +} + +static void _remove_list_dev(struct device_list_opp *list_dev, + struct device_opp *dev_opp) +{ + list_del(&list_dev->node); + call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head, + _kfree_list_dev_rcu); +} + +static struct device_list_opp *_add_list_dev(struct device *dev, + struct device_opp *dev_opp) +{ + struct device_list_opp *list_dev; + + list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL); + if (!list_dev) + return NULL; + + /* Initialize list-dev */ + list_add_rcu(&list_dev->node, &dev_opp->dev_list); + list_dev->dev = dev; + + return list_dev; +} + /** * _add_device_opp() - Returns device OPP table * @dev: device for which we do this operation @@ -466,6 +549,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp; + struct device_list_opp *list_dev;
/* Check for existing list for 'dev' first */ dev_opp = _find_device_opp(dev); @@ -480,7 +564,14 @@ static struct device_opp *_add_device_opp(struct device *dev) if (!dev_opp) return NULL;
- dev_opp->dev = dev; + INIT_LIST_HEAD(&dev_opp->dev_list); + + list_dev = _add_list_dev(dev, dev_opp); + if (!list_dev) { + kfree(dev_opp); + return NULL; + } + srcu_init_notifier_head(&dev_opp->srcu_head); INIT_LIST_HEAD(&dev_opp->opp_list);
@@ -508,9 +599,19 @@ static void _kfree_device_rcu(struct rcu_head *head) */ static void _remove_device_opp(struct device_opp *dev_opp) { + struct device_list_opp *list_dev; + if (!list_empty(&dev_opp->opp_list)) return;
+ list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp, + node); + + _remove_list_dev(list_dev, dev_opp); + + /* dev_list must be empty now */ + WARN_ON(!list_empty(&dev_opp->dev_list)); + list_del_rcu(&dev_opp->node); call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, _kfree_device_rcu); @@ -621,7 +722,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev, return opp; }
-static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp) +static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, + struct device_opp *dev_opp) { struct dev_pm_opp *opp = NULL; struct list_head *head = &dev_opp->opp_list; @@ -639,7 +741,7 @@ static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
/* Duplicate OPPs ? */ if (opp && new_opp->rate == opp->rate) { - dev_warn(dev_opp->dev, + dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, new_opp->available); @@ -702,7 +804,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->available = true; new_opp->dynamic = dynamic;
- ret = _opp_add(new_opp, dev_opp); + ret = _opp_add(dev, new_opp, dev_opp); if (ret) goto free_opp;
@@ -794,7 +896,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
- ret = _opp_add(new_opp, dev_opp); + ret = _opp_add(dev, new_opp, dev_opp); if (ret) goto free_opp;
@@ -1027,6 +1129,9 @@ void of_free_opp_table(struct device *dev) struct device_opp *dev_opp; struct dev_pm_opp *opp, *tmp;
+ /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + /* Check for existing list for 'dev' */ dev_opp = _find_device_opp(dev); if (IS_ERR(dev_opp)) { @@ -1037,18 +1142,21 @@ void of_free_opp_table(struct device *dev) IS_ERR_OR_NULL(dev) ? "Invalid device" : dev_name(dev), error); - return; + goto unlock; }
- /* Hold our list modification lock here */ - mutex_lock(&dev_opp_list_lock); - - /* Free static OPPs */ - list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { - if (!opp->dynamic) - _opp_remove(dev_opp, opp, true); + /* Find if dev_opp manages a single device */ + if (list_is_singular(&dev_opp->dev_list)) { + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { + if (!opp->dynamic) + _opp_remove(dev_opp, opp, true); + } + } else { + _remove_list_dev(_find_list_dev(dev, dev_opp), dev_opp); }
+unlock: mutex_unlock(&dev_opp_list_lock); } EXPORT_SYMBOL_GPL(of_free_opp_table); @@ -1074,6 +1182,7 @@ static int _of_init_opp_table_v2(struct device *dev, const struct property *prop) { struct device_node *opp_np, *np; + struct device_opp *dev_opp; int ret = 0, count = 0;
if (!prop->value) @@ -1084,6 +1193,14 @@ static int _of_init_opp_table_v2(struct device *dev, if (IS_ERR(opp_np)) return PTR_ERR(opp_np);
+ dev_opp = _managed_opp(opp_np); + if (dev_opp) { + /* OPPs are already managed */ + if (!_add_list_dev(dev, dev_opp)) + ret = -ENOMEM; + goto put_opp_np; + } + /* We have opp-list node now, iterate over it and add OPPs */ for_each_available_child_of_node(opp_np, np) { count++; @@ -1100,8 +1217,19 @@ static int _of_init_opp_table_v2(struct device *dev, if (WARN_ON(!count)) goto put_opp_np;
- if (ret) + if (!ret) { + if (!dev_opp) { + dev_opp = _find_device_opp(dev); + if (WARN_ON(!dev_opp)) + goto put_opp_np; + } + + dev_opp->np = opp_np; + if (of_get_property(opp_np, "opp-shared", NULL)) + dev_opp->shared_opp = true; + } else { of_free_opp_table(dev); + }
put_opp_np: of_node_put(opp_np);
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
@@ -112,12 +129,14 @@ struct dev_pm_opp { struct device_opp { struct list_head node;
- struct device *dev; struct srcu_notifier_head srcu_head; struct rcu_head rcu_head;
- struct list_head dev_list; struct list_head opp_list;
- struct device_node *np; unsigned long clock_latency_ns_max;
- bool shared_opp; };
/* @@ -137,6 +156,40 @@ do { \ "dev_opp_list_lock protection"); \ } while (0) +static struct device_list_opp *_find_list_dev(struct device *dev,
const device?
struct device_opp *dev_opp)
+{
- struct device_list_opp *list_dev;
- list_for_each_entry(list_dev, &dev_opp->dev_list, node)
if (list_dev->dev == dev)
return list_dev;
- return NULL;
+}
+static struct device_opp *_managed_opp(struct device_node *np)
const device_node?
+{
- struct device_opp *dev_opp;
- list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
if (dev_opp->np == np) {
/*
* Multiple devices can point to the same OPP table and
* so will have same node-pointer, np.
*
* But the OPPs will be considered as shared only if the
* OPP table contains a "opp-shared" property.
*/
if (dev_opp->shared_opp)
return dev_opp;
else
return NULL;
}
- return NULL;
+}
- /**
- _find_device_opp() - find device_opp struct using device pointer
- @dev: device pointer used to lookup device OPPs
@@ -454,6 +504,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); +/* List-dev Helpers */ +static void _kfree_list_dev_rcu(struct rcu_head *head) +{
- struct device_list_opp *list_dev;
- list_dev = container_of(head, struct device_list_opp, rcu_head);
- kfree_rcu(list_dev, rcu_head);
+}
+static void _remove_list_dev(struct device_list_opp *list_dev,
struct device_opp *dev_opp)
+{
- list_del(&list_dev->node);
- call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
_kfree_list_dev_rcu);
+}
+static struct device_list_opp *_add_list_dev(struct device *dev,
Can dev be const here too?
struct device_opp *dev_opp)
+{
- struct device_list_opp *list_dev;
- list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
- if (!list_dev)
return NULL;
- /* Initialize list-dev */
- list_add_rcu(&list_dev->node, &dev_opp->dev_list);
- list_dev->dev = dev;
- return list_dev;
+}
- /**
- _add_device_opp() - Returns device OPP table
- @dev: device for which we do this operation
@@ -466,6 +549,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp;
- struct device_list_opp *list_dev;
/* Check for existing list for 'dev' first */ dev_opp = _find_device_opp(dev); @@ -621,7 +722,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev, return opp; } -static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp) +static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
{ struct dev_pm_opp *opp = NULL; struct list_head *head = &dev_opp->opp_list;struct device_opp *dev_opp)
@@ -1100,8 +1217,19 @@ static int _of_init_opp_table_v2(struct device *dev, if (WARN_ON(!count)) goto put_opp_np;
- if (ret)
- if (!ret) {
if (!dev_opp) {
dev_opp = _find_device_opp(dev);
if (WARN_ON(!dev_opp))
goto put_opp_np;
}
dev_opp->np = opp_np;
if (of_get_property(opp_np, "opp-shared", NULL))
dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared")?
dev_opp->shared_opp = true;
- } else { of_free_opp_table(dev);
- }
put_opp_np: of_node_put(opp_np);
On 17-07-15, 15:51, Stephen Boyd wrote:
+static struct device_list_opp *_find_list_dev(struct device *dev,
const device?
Diff:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 1af7ceee5433..ff4a3b267ca7 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -96,7 +96,7 @@ struct dev_pm_opp { */ struct device_list_opp { struct list_head node; - struct device *dev; + const struct device *dev; struct rcu_head rcu_head; };
@@ -152,7 +152,7 @@ do { \ "dev_opp_list_lock protection"); \ } while (0)
-static struct device_list_opp *_find_list_dev(struct device *dev, +static struct device_list_opp *_find_list_dev(const struct device *dev, struct device_opp *dev_opp) { struct device_list_opp *list_dev; @@ -164,7 +164,7 @@ static struct device_list_opp *_find_list_dev(struct device *dev, return NULL; }
-static struct device_opp *_managed_opp(struct device_node *np) +static struct device_opp *_managed_opp(const struct device_node *np) { struct device_opp *dev_opp;
@@ -517,7 +517,7 @@ static void _remove_list_dev(struct device_list_opp *list_dev, _kfree_list_dev_rcu); }
-static struct device_list_opp *_add_list_dev(struct device *dev, +static struct device_list_opp *_add_list_dev(const struct device *dev, struct device_opp *dev_opp) { struct device_list_opp *list_dev; @@ -1239,8 +1239,8 @@ static int _of_init_opp_table_v2(struct device *dev, }
dev_opp->np = opp_np; - if (of_get_property(opp_np, "opp-shared", NULL)) - dev_opp->shared_opp = true; + dev_opp->shared_opp = of_property_read_bool(opp_np, + "opp-shared"); } else { of_free_opp_table(dev); }
-----------------------------8<----------------------------
Message-Id: 26828cb0b4b22e5a466c066814bbe39a3c985ca9.1437200372.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Wed, 11 Feb 2015 16:16:28 +0800 Subject: [PATCH] opp: Add OPP sharing information to OPP library
An opp can be shared by multiple devices, for example its very common for CPUs to share the OPPs, i.e. when they share clock/voltage rails.
This patch adds support of shared OPPs to the OPP library.
Instead of a single device, dev_opp will not contain a list of devices that use it. It also senses if the device (we are trying to initialize OPPs for) shares OPPs with a device added earlier and in that case we update the list of devices managed by OPPs instead of duplicating OPPs again.
The same infrastructure will be used for the old OPP bindings, with later patches.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 176 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 152 insertions(+), 24 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 940c49152b68..ff4a3b267ca7 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -86,16 +86,33 @@ struct dev_pm_opp { };
/** + * struct device_list_opp - devices managed by 'struct device_opp' + * @node: list node + * @dev: device to which the struct object belongs + * @rcu_head: RCU callback head used for deferred freeing + * + * This is an internal data structure maintaining the list of devices that are + * managed by 'struct device_opp'. + */ +struct device_list_opp { + struct list_head node; + const struct device *dev; + struct rcu_head rcu_head; +}; + +/** * struct device_opp - Device opp structure * @node: list node - contains the devices with OPPs that * have been registered. Nodes once added are not modified in this * list. * RCU usage: nodes are not modified in the list of device_opp, * however addition is possible and is secured by dev_opp_list_lock - * @dev: device pointer * @srcu_head: notifier head to notify the OPP availability changes. * @rcu_head: RCU callback head used for deferred freeing + * @dev_list: list of devices that share these OPPs * @opp_list: list of opps + * @np: struct device_node pointer for opp's DT node. + * @shared_opp: OPP is shared between multiple devices. * * This is an internal data structure maintaining the link to opps attached to * a device. This structure is not meant to be shared to users as it is @@ -108,12 +125,14 @@ struct dev_pm_opp { struct device_opp { struct list_head node;
- struct device *dev; struct srcu_notifier_head srcu_head; struct rcu_head rcu_head; + struct list_head dev_list; struct list_head opp_list;
+ struct device_node *np; unsigned long clock_latency_ns_max; + bool shared_opp; };
/* @@ -133,6 +152,40 @@ do { \ "dev_opp_list_lock protection"); \ } while (0)
+static struct device_list_opp *_find_list_dev(const struct device *dev, + struct device_opp *dev_opp) +{ + struct device_list_opp *list_dev; + + list_for_each_entry(list_dev, &dev_opp->dev_list, node) + if (list_dev->dev == dev) + return list_dev; + + return NULL; +} + +static struct device_opp *_managed_opp(const struct device_node *np) +{ + struct device_opp *dev_opp; + + list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) + if (dev_opp->np == np) { + /* + * Multiple devices can point to the same OPP table and + * so will have same node-pointer, np. + * + * But the OPPs will be considered as shared only if the + * OPP table contains a "opp-shared" property. + */ + if (dev_opp->shared_opp) + return dev_opp; + else + return NULL; + } + + return NULL; +} + /** * _find_device_opp() - find device_opp struct using device pointer * @dev: device pointer used to lookup device OPPs @@ -149,21 +202,18 @@ do { \ */ static struct device_opp *_find_device_opp(struct device *dev) { - struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV); + struct device_opp *dev_opp;
if (unlikely(IS_ERR_OR_NULL(dev))) { pr_err("%s: Invalid parameters\n", __func__); return ERR_PTR(-EINVAL); }
- list_for_each_entry_rcu(tmp_dev_opp, &dev_opp_list, node) { - if (tmp_dev_opp->dev == dev) { - dev_opp = tmp_dev_opp; - break; - } - } + list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) + if (_find_list_dev(dev, dev_opp)) + return dev_opp;
- return dev_opp; + return ERR_PTR(-ENODEV); }
/** @@ -450,6 +500,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
+/* List-dev Helpers */ +static void _kfree_list_dev_rcu(struct rcu_head *head) +{ + struct device_list_opp *list_dev; + + list_dev = container_of(head, struct device_list_opp, rcu_head); + kfree_rcu(list_dev, rcu_head); +} + +static void _remove_list_dev(struct device_list_opp *list_dev, + struct device_opp *dev_opp) +{ + list_del(&list_dev->node); + call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head, + _kfree_list_dev_rcu); +} + +static struct device_list_opp *_add_list_dev(const struct device *dev, + struct device_opp *dev_opp) +{ + struct device_list_opp *list_dev; + + list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL); + if (!list_dev) + return NULL; + + /* Initialize list-dev */ + list_add_rcu(&list_dev->node, &dev_opp->dev_list); + list_dev->dev = dev; + + return list_dev; +} + /** * _add_device_opp() - Find device OPP table or allocate a new one * @dev: device for which we do this operation @@ -462,6 +545,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); static struct device_opp *_add_device_opp(struct device *dev) { struct device_opp *dev_opp; + struct device_list_opp *list_dev;
/* Check for existing list for 'dev' first */ dev_opp = _find_device_opp(dev); @@ -476,7 +560,14 @@ static struct device_opp *_add_device_opp(struct device *dev) if (!dev_opp) return NULL;
- dev_opp->dev = dev; + INIT_LIST_HEAD(&dev_opp->dev_list); + + list_dev = _add_list_dev(dev, dev_opp); + if (!list_dev) { + kfree(dev_opp); + return NULL; + } + srcu_init_notifier_head(&dev_opp->srcu_head); INIT_LIST_HEAD(&dev_opp->opp_list);
@@ -504,9 +595,19 @@ static void _kfree_device_rcu(struct rcu_head *head) */ static void _remove_device_opp(struct device_opp *dev_opp) { + struct device_list_opp *list_dev; + if (!list_empty(&dev_opp->opp_list)) return;
+ list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp, + node); + + _remove_list_dev(list_dev, dev_opp); + + /* dev_list must be empty now */ + WARN_ON(!list_empty(&dev_opp->dev_list)); + list_del_rcu(&dev_opp->node); call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, _kfree_device_rcu); @@ -616,7 +717,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev, return opp; }
-static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp) +static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, + struct device_opp *dev_opp) { struct dev_pm_opp *opp; struct list_head *head = &dev_opp->opp_list; @@ -632,7 +734,7 @@ static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp) break; } else { /* Duplicate OPPs */ - dev_warn(dev_opp->dev, + dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, @@ -698,7 +800,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, new_opp->available = true; new_opp->dynamic = dynamic;
- ret = _opp_add(new_opp, dev_opp); + ret = _opp_add(dev, new_opp, dev_opp); if (ret) goto free_opp;
@@ -808,7 +910,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
- ret = _opp_add(new_opp, dev_opp); + ret = _opp_add(dev, new_opp, dev_opp); if (ret) goto free_opp;
@@ -1041,6 +1143,9 @@ void of_free_opp_table(struct device *dev) struct device_opp *dev_opp; struct dev_pm_opp *opp, *tmp;
+ /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + /* Check for existing list for 'dev' */ dev_opp = _find_device_opp(dev); if (IS_ERR(dev_opp)) { @@ -1051,18 +1156,21 @@ void of_free_opp_table(struct device *dev) IS_ERR_OR_NULL(dev) ? "Invalid device" : dev_name(dev), error); - return; + goto unlock; }
- /* Hold our list modification lock here */ - mutex_lock(&dev_opp_list_lock); - - /* Free static OPPs */ - list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { - if (!opp->dynamic) - _opp_remove(dev_opp, opp, true); + /* Find if dev_opp manages a single device */ + if (list_is_singular(&dev_opp->dev_list)) { + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { + if (!opp->dynamic) + _opp_remove(dev_opp, opp, true); + } + } else { + _remove_list_dev(_find_list_dev(dev, dev_opp), dev_opp); }
+unlock: mutex_unlock(&dev_opp_list_lock); } EXPORT_SYMBOL_GPL(of_free_opp_table); @@ -1088,6 +1196,7 @@ static int _of_init_opp_table_v2(struct device *dev, const struct property *prop) { struct device_node *opp_np, *np; + struct device_opp *dev_opp; int ret = 0, count = 0;
if (!prop->value) @@ -1098,6 +1207,14 @@ static int _of_init_opp_table_v2(struct device *dev, if (IS_ERR(opp_np)) return PTR_ERR(opp_np);
+ dev_opp = _managed_opp(opp_np); + if (dev_opp) { + /* OPPs are already managed */ + if (!_add_list_dev(dev, dev_opp)) + ret = -ENOMEM; + goto put_opp_np; + } + /* We have opp-list node now, iterate over it and add OPPs */ for_each_available_child_of_node(opp_np, np) { count++; @@ -1114,8 +1231,19 @@ static int _of_init_opp_table_v2(struct device *dev, if (WARN_ON(!count)) goto put_opp_np;
- if (ret) + if (!ret) { + if (!dev_opp) { + dev_opp = _find_device_opp(dev); + if (WARN_ON(!dev_opp)) + goto put_opp_np; + } + + dev_opp->np = opp_np; + dev_opp->shared_opp = of_property_read_bool(opp_np, + "opp-shared"); + } else { of_free_opp_table(dev); + }
put_opp_np: of_node_put(opp_np);
On 07/17/2015 11:33 PM, Viresh Kumar wrote:
From: Viresh Kumar viresh.kumar@linaro.org Date: Wed, 11 Feb 2015 16:16:28 +0800 Subject: [PATCH] opp: Add OPP sharing information to OPP library
An opp can be shared by multiple devices, for example its very common for CPUs to share the OPPs, i.e. when they share clock/voltage rails.
This patch adds support of shared OPPs to the OPP library.
Instead of a single device, dev_opp will not contain a list of devices that use it. It also senses if the device (we are trying to initialize OPPs for) shares OPPs with a device added earlier and in that case we update the list of devices managed by OPPs instead of duplicating OPPs again.
The same infrastructure will be used for the old OPP bindings, with later patches.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
+static struct device_opp *_managed_opp(const struct device_node *np) +{
- struct device_opp *dev_opp;
- list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
if (dev_opp->np == np) {
/*
* Multiple devices can point to the same OPP table and
* so will have same node-pointer, np.
*
* But the OPPs will be considered as shared only if the
* OPP table contains a "opp-shared" property.
*/
if (dev_opp->shared_opp)
return dev_opp;
else
return NULL;
The janitors will probably find this and say that it could be simplified to an if () and a return without the else.
On 20-07-15, 10:46, Stephen Boyd wrote:
+static struct device_opp *_managed_opp(const struct device_node *np) +{
- struct device_opp *dev_opp;
- list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
if (dev_opp->np == np) {
/*
* Multiple devices can point to the same OPP table and
* so will have same node-pointer, np.
*
* But the OPPs will be considered as shared only if the
* OPP table contains a "opp-shared" property.
*/
if (dev_opp->shared_opp)
return dev_opp;
else
return NULL;
The janitors will probably find this and say that it could be simplified to an if () and a return without the else.
And I am trying to make the janitors happy with this:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index eb920e3f115b..8c81784fe473 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -168,7 +168,7 @@ static struct device_opp *_managed_opp(const struct device_node *np) { struct device_opp *dev_opp;
- list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) + list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) { if (dev_opp->np == np) { /* * Multiple devices can point to the same OPP table and @@ -177,11 +177,9 @@ static struct device_opp *_managed_opp(const struct device_node *np) * But the OPPs will be considered as shared only if the * OPP table contains a "opp-shared" property. */ - if (dev_opp->shared_opp) - return dev_opp; - else - return NULL; + return dev_opp->shared_opp ? dev_opp : NULL; } + }
return NULL; }
With "operating-points-v2" bindings, its possible to specify the OPP to which the device must be switched, before suspending.
This patch adds support for getting that information.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 6b554e417b1f..0022453e4b60 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -137,6 +137,7 @@ struct device_opp { struct device_node *np; unsigned long clock_latency_ns_max; bool shared_opp; + struct dev_pm_opp *suspend_opp; };
/* @@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev, goto put_opp_np;
if (!ret) { + const phandle *handle; + if (!dev_opp) { dev_opp = _find_device_opp(dev); if (WARN_ON(!dev_opp)) @@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev, dev_opp->np = opp_np; if (of_get_property(opp_np, "opp-shared", NULL)) dev_opp->shared_opp = true; + + /* OPP to select on device suspend */ + handle = of_get_property(opp_np, "opp-suspend", NULL); + if (handle) { + struct device_node *suspend_opp_np; + struct dev_pm_opp *opp; + + suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle)); + + list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) + if (opp->np == suspend_opp_np) { + dev_opp->suspend_opp = opp; + break; + } + + if (!dev_opp->suspend_opp) + dev_err(dev, "%s: Invalid opp-suspend\n", + __func__); + } } else { of_free_opp_table(dev); }
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 6b554e417b1f..0022453e4b60 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev, goto put_opp_np; if (!ret) {
const phandle *handle;
- if (!dev_opp) { dev_opp = _find_device_opp(dev); if (WARN_ON(!dev_opp))
@@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev, dev_opp->np = opp_np; if (of_get_property(opp_np, "opp-shared", NULL)) dev_opp->shared_opp = true;
/* OPP to select on device suspend */
handle = of_get_property(opp_np, "opp-suspend", NULL);
if (handle) {
struct device_node *suspend_opp_np;
struct dev_pm_opp *opp;
suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle));
Couldn't this be done with of_parse_phandle() instead? Otherwise the patch looks ok.
list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
if (opp->np == suspend_opp_np) {
dev_opp->suspend_opp = opp;
break;
}
if (!dev_opp->suspend_opp)
dev_err(dev, "%s: Invalid opp-suspend\n",
__func__);
} else { of_free_opp_table(dev); }}
On 17-07-15, 12:22, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 6b554e417b1f..0022453e4b60 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev, goto put_opp_np; if (!ret) {
const phandle *handle;
- if (!dev_opp) { dev_opp = _find_device_opp(dev); if (WARN_ON(!dev_opp))
@@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev, dev_opp->np = opp_np; if (of_get_property(opp_np, "opp-shared", NULL)) dev_opp->shared_opp = true;
/* OPP to select on device suspend */
handle = of_get_property(opp_np, "opp-suspend", NULL);
if (handle) {
struct device_node *suspend_opp_np;
struct dev_pm_opp *opp;
suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle));
Couldn't this be done with of_parse_phandle() instead? Otherwise the patch looks ok.
Yeah, it could have been. Over that, this patch requires certain modifications as the bindings changed this property before merging. Its now part of individual OPPs and so no phandles.
I will reiterate the patches soon with updates.
With "operating-points-v2" its possible to tell which devices share OPPs. We already have infrastructure to decode that information.
This patch adds following APIs: - of_get_cpus_sharing_opps: Returns cpumask of CPUs sharing OPPs (only valid with v2 bindings). - of_cpumask_init_opp_table: Initializes OPPs for all CPUs present in cpumask. - of_cpumask_free_opp_table: Frees OPPs for all CPUs present in cpumask.
- set_cpus_sharing_opps: Sets which CPUs share OPPs (only valid with old OPP bindings, as this information isn't present in DT).
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 23 +++++++ 2 files changed, 198 insertions(+)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 0022453e4b60..e24502a2692f 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -11,6 +11,7 @@ * published by the Free Software Foundation. */
+#include <linux/cpu.h> #include <linux/kernel.h> #include <linux/errno.h> #include <linux/err.h> @@ -1162,6 +1163,26 @@ void of_free_opp_table(struct device *dev) } EXPORT_SYMBOL_GPL(of_free_opp_table);
+void of_cpumask_free_opp_table(cpumask_var_t cpumask) +{ + struct device *cpu_dev; + int cpu; + + WARN_ON(cpumask_empty(cpumask)); + + for_each_cpu(cpu, cpumask) { + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + pr_err("%s: failed to get cpu%d device\n", __func__, + cpu); + continue; + } + + of_free_opp_table(cpu_dev); + } +} +EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table); + /* Returns opp descriptor node from its phandle. Caller must do of_node_put() */ static struct device_node * _of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop) @@ -1178,6 +1199,31 @@ _of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop) return opp_np; }
+/* Returns opp descriptor node for a device. Caller must do of_node_put() */ +static struct device_node *_of_get_opp_desc_node(struct device *dev) +{ + const struct property *prop; + + prop = of_find_property(dev->of_node, "operating-points-v2", NULL); + if (!prop) + return ERR_PTR(-ENODEV); + if (!prop->value) + return ERR_PTR(-ENODATA); + + /* + * TODO: Support for multiple OPP tables. + * + * There should be only ONE phandle present in "operating-points-v2" + * property. + */ + if (prop->length != sizeof(__be32)) { + dev_err(dev, "%s: Invalid opp desc phandle\n", __func__); + return ERR_PTR(-EINVAL); + } + + return _of_get_opp_desc_node_from_prop(dev, prop); +} + /* Initializes OPP tables based on new bindings */ static int _of_init_opp_table_v2(struct device *dev, const struct property *prop) @@ -1338,4 +1384,133 @@ int of_init_opp_table(struct device *dev) return _of_init_opp_table_v2(dev, prop); } EXPORT_SYMBOL_GPL(of_init_opp_table); + +int of_cpumask_init_opp_table(cpumask_var_t cpumask) +{ + struct device *cpu_dev; + int cpu, ret = 0; + + WARN_ON(cpumask_empty(cpumask)); + + for_each_cpu(cpu, cpumask) { + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + pr_err("%s: failed to get cpu%d device\n", __func__, + cpu); + continue; + } + + ret = of_init_opp_table(cpu_dev); + if (ret) { + pr_err("%s: couldn't find opp table for cpu:%d, %d\n", + __func__, cpu, ret); + + /* Free all other OPPs */ + of_cpumask_free_opp_table(cpumask); + break; + } + } + + return ret; +} +EXPORT_SYMBOL_GPL(of_cpumask_init_opp_table); + +/* Required only for V1 bindings, as v2 can manage it from DT itself */ +int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask) +{ + struct device_list_opp *list_dev; + struct device_opp *dev_opp; + struct device *dev; + int cpu, ret = 0; + + rcu_read_lock(); + + dev_opp = _find_device_opp(cpu_dev); + if (IS_ERR(dev_opp)) { + ret = -EINVAL; + goto out_rcu_read_unlock; + } + + for_each_cpu(cpu, cpumask) { + if (cpu == cpu_dev->id) + continue; + + dev = get_cpu_device(cpu); + if (!dev) { + dev_err(cpu_dev, "%s: failed to get cpu%d device\n", + __func__, cpu); + continue; + } + + list_dev = _add_list_dev(dev, dev_opp); + if (!list_dev) { + dev_err(dev, "%s: failed to add list-dev for cpu%d device\n", + __func__, cpu); + continue; + } + } +out_rcu_read_unlock: + rcu_read_unlock(); + + return 0; +} +EXPORT_SYMBOL_GPL(set_cpus_sharing_opps); + +/* + * Works only for OPP v2 bindings. + * + * cpumask should be already set to mask of cpu_dev->id. + * Returns -ENOENT if operating-points-v2 bindings aren't supported. + */ +int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask) +{ + struct device_node *np, *tmp_np; + struct device *tcpu_dev; + int cpu, ret = 0; + + /* Get OPP descriptor node */ + np = _of_get_opp_desc_node(cpu_dev); + if (IS_ERR(np)) { + dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__, + PTR_ERR(np)); + return -ENOENT; + } + + /* OPPs are shared ? */ + if (!of_get_property(np, "opp-shared", NULL)) + goto put_cpu_node; + + for_each_possible_cpu(cpu) { + if (cpu == cpu_dev->id) + continue; + + tcpu_dev = get_cpu_device(cpu); + if (!tcpu_dev) { + dev_err(cpu_dev, "%s: failed to get cpu%d device\n", + __func__, cpu); + ret = -ENODEV; + goto put_cpu_node; + } + + /* Get OPP descriptor node */ + tmp_np = _of_get_opp_desc_node(tcpu_dev); + if (IS_ERR(tmp_np)) { + dev_info(tcpu_dev, "%s: Couldn't find opp node: %ld\n", + __func__, PTR_ERR(tmp_np)); + ret = -EINVAL; + goto put_cpu_node; + } + + /* CPUs are sharing opp node */ + if (np == tmp_np) + cpumask_set_cpu(cpu, cpumask); + + of_node_put(tmp_np); + } + +put_cpu_node: + of_node_put(np); + return ret; +} +EXPORT_SYMBOL_GPL(of_get_cpus_sharing_opps); #endif diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 20324b579adc..bb52fae5b921 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -121,6 +121,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier( #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) int of_init_opp_table(struct device *dev); void of_free_opp_table(struct device *dev); +int of_cpumask_init_opp_table(cpumask_var_t cpumask); +void of_cpumask_free_opp_table(cpumask_var_t cpumask); +int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask); +int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask); #else static inline int of_init_opp_table(struct device *dev) { @@ -130,6 +134,25 @@ static inline int of_init_opp_table(struct device *dev) static inline void of_free_opp_table(struct device *dev) { } + +static inline int of_cpumask_init_opp_table(cpumask_var_t cpumask) +{ + return -ENOSYS; +} + +static inline void of_cpumask_free_opp_table(cpumask_var_t cpumask) +{ +} + +static inline int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask) +{ + return -ENOSYS; +} + +static inline int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask) +{ + return -ENOSYS; +} #endif
#endif /* __LINUX_OPP_H__ */
Support for parsing operating-points-v2 bindings is in place now, lets modify cpufreq-dt driver to use them.
For backward compatibility we will continue to support earlier bindings. Special handling for that is required, to make sure OPPs are initialized for all the CPUs.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 58 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index bab67db54b7e..f7bb2976059a 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -184,7 +184,6 @@ static int allocate_resources(int cpu, struct device **cdev,
static int cpufreq_init(struct cpufreq_policy *policy) { - struct cpufreq_dt_platform_data *pd; struct cpufreq_frequency_table *freq_table; struct device_node *np; struct private_data *priv; @@ -192,7 +191,8 @@ static int cpufreq_init(struct cpufreq_policy *policy) struct regulator *cpu_reg; struct clk *cpu_clk; unsigned long min_uV = ~0, max_uV = 0; - unsigned int transition_latency; + unsigned int transition_latency = 0; + bool need_update = false; int ret;
ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk); @@ -208,8 +208,47 @@ static int cpufreq_init(struct cpufreq_policy *policy) goto out_put_reg_clk; }
- /* OPPs might be populated at runtime, don't check for error here */ - of_init_opp_table(cpu_dev); + /* Get OPP-sharing information from "operating-points-v2" bindings */ + ret = of_get_cpus_sharing_opps(cpu_dev, policy->cpus); + if (ret) { + /* + * operating-points-v2 not supported, fallback to old method of + * finding shared-OPPs for backward compatibility. + */ + if (ret == -ENOENT) + need_update = true; + else + goto out_node_put; + } + + /* + * Initialize OPP tables for all policy->cpus. They will be shared by + * all CPUs which have marked their CPUs shared with OPP bindings. + * + * For platforms not using operating-points-v2 bindings, we do this + * before updating policy->cpus. Otherwise, we will end up creating + * duplicate OPPs for policy->cpus. + * + * OPPs might be populated at runtime, don't check for error here + */ + of_cpumask_init_opp_table(policy->cpus); + + if (need_update) { + struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data(); + + if (!pd || !pd->independent_clocks) + cpumask_setall(policy->cpus); + + /* + * OPP tables are initialized only for policy->cpu, do it for + * others as well. + */ + set_cpus_sharing_opps(cpu_dev, policy->cpus); + + of_property_read_u32(np, "clock-latency", &transition_latency); + } else { + transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev); + }
/* * But we need OPP table to function so if it is not there let's @@ -230,7 +269,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
- if (of_property_read_u32(np, "clock-latency", &transition_latency)) + if (!transition_latency) transition_latency = CPUFREQ_ETERNAL;
if (!IS_ERR(cpu_reg)) { @@ -293,10 +332,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
policy->cpuinfo.transition_latency = transition_latency;
- pd = cpufreq_get_driver_data(); - if (!pd || !pd->independent_clocks) - cpumask_setall(policy->cpus); - of_node_put(np);
return 0; @@ -306,7 +341,8 @@ static int cpufreq_init(struct cpufreq_policy *policy) out_free_priv: kfree(priv); out_free_opp: - of_free_opp_table(cpu_dev); + of_cpumask_free_opp_table(policy->cpus); +out_node_put: of_node_put(np); out_put_reg_clk: clk_put(cpu_clk); @@ -322,7 +358,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); - of_free_opp_table(priv->cpu_dev); + of_cpumask_free_opp_table(policy->related_cpus); clk_put(policy->clk); if (!IS_ERR(priv->cpu_reg)) regulator_put(priv->cpu_reg);
Hi,
On Monday, June 15, 2015 05:27:36 PM Viresh Kumar wrote:
Support for parsing operating-points-v2 bindings is in place now, lets modify cpufreq-dt driver to use them.
I believe that following patches:
* [PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper (http://lkml.org/lkml/2015/7/9/420)
* [PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes (http://lkml.org/lkml/2015/7/9/421)
should be integrated into this patch series before patch #10 (the current one) can be applied.
[ Please see http://lkml.org/lkml/2015/7/9/419 for more details. ]
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Thursday, July 09, 2015 06:13:54 PM Bartlomiej Zolnierkiewicz wrote:
Hi,
On Monday, June 15, 2015 05:27:36 PM Viresh Kumar wrote:
Support for parsing operating-points-v2 bindings is in place now, lets modify cpufreq-dt driver to use them.
I believe that following patches:
[PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper (http://lkml.org/lkml/2015/7/9/420)
[PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes (http://lkml.org/lkml/2015/7/9/421)
should be integrated into this patch series before patch #10 (the current one) can be applied.
[ Please see http://lkml.org/lkml/2015/7/9/419 for more details. ]
On the 2nd thought here is the issue description, so people don't have to search for it:
With the current code the turbo-mode opp-s are not distinguished in any way in the cpufreq subsystem and once somebody defines them in their DTS file they are treated as normal modes. I.e. if you define opp-s using opp-v2 bindings in your DTS file (for use by cpufreq-dt driver) and some are marked as turbo modes then the freq_table (build by cpufreq core) will contain turbo mode frequencies and cpufreq-dt driver will use them as normal frequencies. This is certainly not a desired behavior.
To fix it I added opp core helper to check whether opp is a turbo mode and during build of freq_table turbo mode frequencies are marked with special flag (CPUFREQ_BOOST_FREQ). Such frequencies are ignored by cpufreq core unless 'boost' mode is enabled. 'boost' mode support needs to be explicitly supported & enabled in a specific cpufreq driver. For the moment it is only available for Exynos4x12 cpufreq driver but my patchset (converting Exynos4x12 platforms to use cpufreq-dt driver) makes it available for use in cpufreq-dt (to be enabled from your platform support when needed).
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 09-07-15, 18:13, Bartlomiej Zolnierkiewicz wrote:
I believe that following patches:
[PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper (http://lkml.org/lkml/2015/7/9/420)
[PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes (http://lkml.org/lkml/2015/7/9/421)
should be integrated into this patch series before patch #10 (the current one) can be applied.
Hi Bartlomiej,
I haven't tried for the complete support yet, I will take care of boost frequencies as well, but not in this series.
On 15 June 2015 at 17:27, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Guys,
This adds code to support operating-points-v2 bindings. Not everything is supported yet, but most of the basic stuff is.
Tested with: Exynos 5250, dual cortex A15 board. With both old and new bindings.
Bindings are already frozen: http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org
Pushed here as well for reference: ssh://git@git.linaro.org/people/viresh.kumar/linux.git opp/v2
I am really hoping to get all basic stuff pushed in for 4.3, can we have some review of the code please?
-- viresh
On 30-06-15, 22:14, Viresh Kumar wrote:
On 15 June 2015 at 17:27, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Guys,
This adds code to support operating-points-v2 bindings. Not everything is supported yet, but most of the basic stuff is.
Tested with: Exynos 5250, dual cortex A15 board. With both old and new bindings.
Bindings are already frozen: http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org
Pushed here as well for reference: ssh://git@git.linaro.org/people/viresh.kumar/linux.git opp/v2
I am really hoping to get all basic stuff pushed in for 4.3, can we have some review of the code please?
@Stephen: Can you please review rest of the stuff?
@Nishanth: Can you please review the series, now that you are back :)
@Rafael: Can you please apply the patches Acked by Stephen for 4.3, so that I don't have to resend them again ?
linaro-kernel@lists.linaro.org