* [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table
@ 2018-09-19 22:20 Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 01/12] OPP: Free OPP table properly on performance state irregularities Viresh Kumar
` (11 more replies)
0 siblings, 12 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Jason Cooper, Nishanth Menon,
Rafael J. Wysocki, Sebastian Hesselbarth, Stephen Boyd,
Viresh Kumar
Cc: Viresh Kumar, linux-pm, Vincent Guittot, 4.18, linux-arm-kernel,
linux-kernel, Niklas Cassel
Hello,
Niklas Cassle recently reported some regressions with his Qcom cpufreq
driver where he was getting some errors while creating the OPPs tables.
After looking into it I realized that the OPP core incorrectly creates
multiple OPP tables for the devices even if they are sharing the OPP
table in DT. This happens when the request comes using different CPU
devices. For example, dev_pm_opp_set_supported_hw() getting called using
CPU0 and dev_pm_opp_of_add_table() getting called using CPU1.
This series redesigns the internals of the OPP core to fix that. The
redesign has simplified the core itself though.
The first three patches are fixes really for the current code and the rest
of them are making necessary changes to fix the issue defined in
$subject here.
Nikklas already tested this series and his Tested-by is already applied
to series here. I would like to get this merged during the 4.20 merge
window and will push the series to linux-next soon to get more test
coverage. Please provide comments as soon as possible, else will send it
as part of the pull request to Rafael for 4.20.
--
viresh
V1->V2:
- Nikklas reported another regressions which is fixed by the 2nd commit
in this series.
Viresh Kumar (12):
OPP: Free OPP table properly on performance state irregularities
OPP: Don't try to remove all OPP tables on failure
OPP: Protect dev_list with opp_table lock
OPP: Pass index to _of_init_opp_table()
OPP: Parse OPP table's DT properties from _of_init_opp_table()
OPP: Don't take OPP table's kref for static OPPs
OPP: Create separate kref for static OPPs list
cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove()
OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()
OPP: Use a single mechanism to free the OPP table
OPP: Prevent creating multiple OPP tables for devices sharing OPP
nodes
OPP: Pass OPP table to _of_add_opp_table_v{1|2}()
drivers/cpufreq/mvebu-cpufreq.c | 9 +-
drivers/opp/core.c | 147 ++++++++++++++++++++-----------
drivers/opp/cpu.c | 15 ++--
drivers/opp/of.c | 188 +++++++++++++++++++++-------------------
drivers/opp/opp.h | 19 ++--
include/linux/pm_opp.h | 6 ++
6 files changed, 226 insertions(+), 158 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 01/12] OPP: Free OPP table properly on performance state irregularities
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 02/12] OPP: Don't try to remove all OPP tables on failure Viresh Kumar
` (10 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot, 4 . 18,
Niklas Cassel, linux-kernel
The OPP table was freed, but not the individual OPPs which is done from
_dev_pm_opp_remove_table(). Fix it by calling _dev_pm_opp_remove_table()
as well.
Cc: 4.18 <stable@vger.kernel.org> # v4.18
Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/of.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7af0ddec936b..20988c426650 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -425,6 +425,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
count, pstate_count);
ret = -ENOENT;
+ _dev_pm_opp_remove_table(opp_table, dev, false);
goto put_opp_table;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 02/12] OPP: Don't try to remove all OPP tables on failure
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 01/12] OPP: Free OPP table properly on performance state irregularities Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 03/12] OPP: Protect dev_list with opp_table lock Viresh Kumar
` (9 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
dev_pm_opp_of_cpumask_add_table() creates the OPP table for all CPUs
present in the cpumask and on errors it should revert all changes it has
done.
It actually is doing a bit more than that. On errors, it tries to free
all the OPP tables, even the one it hasn't created yet. This may also
end up freeing the OPP tables which were created from separate path,
like dev_pm_opp_set_supported_hw().
Reported-and-tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/cpu.c | 8 ++++++--
drivers/opp/of.c | 4 ++--
drivers/opp/opp.h | 2 +-
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/opp/cpu.c b/drivers/opp/cpu.c
index 0c0910709435..2eb5e2e7ff66 100644
--- a/drivers/opp/cpu.c
+++ b/drivers/opp/cpu.c
@@ -108,7 +108,8 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
#endif /* CONFIG_CPU_FREQ */
-void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of)
+void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of,
+ int last_cpu)
{
struct device *cpu_dev;
int cpu;
@@ -116,6 +117,9 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of)
WARN_ON(cpumask_empty(cpumask));
for_each_cpu(cpu, cpumask) {
+ if (cpu == last_cpu)
+ break;
+
cpu_dev = get_cpu_device(cpu);
if (!cpu_dev) {
pr_err("%s: failed to get cpu%d device\n", __func__,
@@ -140,7 +144,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of)
*/
void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask)
{
- _dev_pm_opp_cpumask_remove_table(cpumask, false);
+ _dev_pm_opp_cpumask_remove_table(cpumask, false, -1);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_remove_table);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 20988c426650..86222586f27b 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -592,7 +592,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
*/
void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask)
{
- _dev_pm_opp_cpumask_remove_table(cpumask, true);
+ _dev_pm_opp_cpumask_remove_table(cpumask, true, -1);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_remove_table);
@@ -627,7 +627,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask)
__func__, cpu, ret);
/* Free all other OPPs */
- dev_pm_opp_of_cpumask_remove_table(cpumask);
+ _dev_pm_opp_cpumask_remove_table(cpumask, true, cpu);
break;
}
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 7c540fd063b2..a9d22aa534c3 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -196,7 +196,7 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
void _opp_free(struct dev_pm_opp *opp);
int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
-void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of);
+void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of, int last_cpu);
struct opp_table *_add_opp_table(struct device *dev);
#ifdef CONFIG_OF
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 03/12] OPP: Protect dev_list with opp_table lock
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 01/12] OPP: Free OPP table properly on performance state irregularities Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 02/12] OPP: Don't try to remove all OPP tables on failure Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 04/12] OPP: Pass index to _of_init_opp_table() Viresh Kumar
` (8 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
The dev_list needs to be protected with a lock, else we may have
simultaneous access (addition/removal) to it and that would be racy.
Extend scope of the opp_table lock to protect dev_list as well.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 21 +++++++++++++++++++--
drivers/opp/cpu.c | 2 ++
drivers/opp/opp.h | 2 +-
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 31ff03dbeb83..9f8aa31265fe 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -48,9 +48,14 @@ static struct opp_device *_find_opp_dev(const struct device *dev,
static struct opp_table *_find_opp_table_unlocked(struct device *dev)
{
struct opp_table *opp_table;
+ bool found;
list_for_each_entry(opp_table, &opp_tables, node) {
- if (_find_opp_dev(dev, opp_table)) {
+ mutex_lock(&opp_table->lock);
+ found = !!_find_opp_dev(dev, opp_table);
+ mutex_unlock(&opp_table->lock);
+
+ if (found) {
_get_opp_table_kref(opp_table);
return opp_table;
@@ -766,6 +771,8 @@ struct opp_device *_add_opp_dev(const struct device *dev,
/* Initialize opp-dev */
opp_dev->dev = dev;
+
+ mutex_lock(&opp_table->lock);
list_add(&opp_dev->node, &opp_table->dev_list);
/* Create debugfs entries for the opp_table */
@@ -773,6 +780,7 @@ struct opp_device *_add_opp_dev(const struct device *dev,
if (ret)
dev_err(dev, "%s: Failed to register opp debugfs (%d)\n",
__func__, ret);
+ mutex_unlock(&opp_table->lock);
return opp_dev;
}
@@ -791,6 +799,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev)
if (!opp_table)
return NULL;
+ mutex_init(&opp_table->lock);
INIT_LIST_HEAD(&opp_table->dev_list);
opp_dev = _add_opp_dev(dev, opp_table);
@@ -812,7 +821,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev)
BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
INIT_LIST_HEAD(&opp_table->opp_list);
- mutex_init(&opp_table->lock);
kref_init(&opp_table->kref);
/* Secure the device table modification */
@@ -854,6 +862,10 @@ static void _opp_table_kref_release(struct kref *kref)
if (!IS_ERR(opp_table->clk))
clk_put(opp_table->clk);
+ /*
+ * No need to take opp_table->lock here as we are guaranteed that no
+ * references to the OPP table are taken at this point.
+ */
opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device,
node);
@@ -1716,6 +1728,9 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
{
struct dev_pm_opp *opp, *tmp;
+ /* Protect dev_list */
+ mutex_lock(&opp_table->lock);
+
/* Find if opp_table manages a single device */
if (list_is_singular(&opp_table->dev_list)) {
/* Free static OPPs */
@@ -1733,6 +1748,8 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
} else {
_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
}
+
+ mutex_unlock(&opp_table->lock);
}
void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all)
diff --git a/drivers/opp/cpu.c b/drivers/opp/cpu.c
index 2eb5e2e7ff66..36586f66cd83 100644
--- a/drivers/opp/cpu.c
+++ b/drivers/opp/cpu.c
@@ -226,8 +226,10 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
cpumask_clear(cpumask);
if (opp_table->shared_opp == OPP_TABLE_ACCESS_SHARED) {
+ mutex_lock(&opp_table->lock);
list_for_each_entry(opp_dev, &opp_table->dev_list, node)
cpumask_set_cpu(opp_dev->dev->id, cpumask);
+ mutex_unlock(&opp_table->lock);
} else {
cpumask_set_cpu(cpu_dev->id, cpumask);
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index a9d22aa534c3..88e9f47aadf1 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -126,7 +126,7 @@ enum opp_table_access {
* @dev_list: list of devices that share these OPPs
* @opp_list: table of opps
* @kref: for reference count of the table.
- * @lock: mutex protecting the opp_list.
+ * @lock: mutex protecting the opp_list and dev_list.
* @np: struct device_node pointer for opp's DT node.
* @clock_latency_ns_max: Max clock latency in nanoseconds.
* @shared_opp: OPP is shared between multiple devices.
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 04/12] OPP: Pass index to _of_init_opp_table()
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (2 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 03/12] OPP: Protect dev_list with opp_table lock Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 05/12] OPP: Parse OPP table's DT properties from _of_init_opp_table() Viresh Kumar
` (7 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Niklas Cassel, linux-kernel
This is a preparatory patch required for the next commit which will
start using OPP table's node pointer in _of_init_opp_table(), which
requires the index in order to read the OPP table's phandle.
This commit adds the index argument in the call chains in order to get
it delivered to _of_init_opp_table().
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 19 +++++++++++++++----
drivers/opp/of.c | 12 +++++++-----
drivers/opp/opp.h | 4 ++--
include/linux/pm_opp.h | 6 ++++++
4 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9f8aa31265fe..332748adc262 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -785,7 +785,7 @@ struct opp_device *_add_opp_dev(const struct device *dev,
return opp_dev;
}
-static struct opp_table *_allocate_opp_table(struct device *dev)
+static struct opp_table *_allocate_opp_table(struct device *dev, int index)
{
struct opp_table *opp_table;
struct opp_device *opp_dev;
@@ -808,7 +808,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev)
return NULL;
}
- _of_init_opp_table(opp_table, dev);
+ _of_init_opp_table(opp_table, dev, index);
/* Find clk for the device */
opp_table->clk = clk_get(dev, NULL);
@@ -833,7 +833,7 @@ void _get_opp_table_kref(struct opp_table *opp_table)
kref_get(&opp_table->kref);
}
-struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
+static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
{
struct opp_table *opp_table;
@@ -844,15 +844,26 @@ struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
if (!IS_ERR(opp_table))
goto unlock;
- opp_table = _allocate_opp_table(dev);
+ opp_table = _allocate_opp_table(dev, index);
unlock:
mutex_unlock(&opp_table_lock);
return opp_table;
}
+
+struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
+{
+ return _opp_get_opp_table(dev, 0);
+}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table);
+struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev,
+ int index)
+{
+ return _opp_get_opp_table(dev, index);
+}
+
static void _opp_table_kref_release(struct kref *kref)
{
struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 86222586f27b..1a9e1242a2a7 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -52,7 +52,8 @@ static struct opp_table *_managed_opp(const struct device_node *np)
return managed_table;
}
-void _of_init_opp_table(struct opp_table *opp_table, struct device *dev)
+void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
+ int index)
{
struct device_node *np;
@@ -378,7 +379,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
}
/* Initializes OPP tables based on new bindings */
-static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
+static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
+ int index)
{
struct device_node *np;
struct opp_table *opp_table;
@@ -393,7 +395,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
goto put_opp_table;
}
- opp_table = dev_pm_opp_get_opp_table(dev);
+ opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
if (!opp_table)
return -ENOMEM;
@@ -526,7 +528,7 @@ int dev_pm_opp_of_add_table(struct device *dev)
return _of_add_opp_table_v1(dev);
}
- ret = _of_add_opp_table_v2(dev, opp_np);
+ ret = _of_add_opp_table_v2(dev, opp_np, 0);
of_node_put(opp_np);
return ret;
@@ -574,7 +576,7 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
return -ENODEV;
}
- ret = _of_add_opp_table_v2(dev, opp_np);
+ ret = _of_add_opp_table_v2(dev, opp_np, index);
of_node_put(opp_np);
return ret;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 88e9f47aadf1..b235e76fc8cc 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -200,9 +200,9 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of, in
struct opp_table *_add_opp_table(struct device *dev);
#ifdef CONFIG_OF
-void _of_init_opp_table(struct opp_table *opp_table, struct device *dev);
+void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
#else
-static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev) {}
+static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
#endif
#ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 099b31960dec..5d399eeef172 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -79,6 +79,7 @@ struct dev_pm_set_opp_data {
#if defined(CONFIG_PM_OPP)
struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
+struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev, int index);
void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
@@ -136,6 +137,11 @@ static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
return ERR_PTR(-ENOTSUPP);
}
+static inline struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev, int index)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
static inline void dev_pm_opp_put_opp_table(struct opp_table *opp_table) {}
static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 05/12] OPP: Parse OPP table's DT properties from _of_init_opp_table()
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (3 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 04/12] OPP: Pass index to _of_init_opp_table() Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 06/12] OPP: Don't take OPP table's kref for static OPPs Viresh Kumar
` (6 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
Parse the DT properties present in the OPP table from
_of_init_opp_table(), which is a dedicated routine for DT parsing.
Minor relocation of helpers is required for this.
It is possible now for _managed_opp() to return a partially initialized
OPP table if the OPP table is created via the helpers like
dev_pm_opp_set_supported_hw() and we need another flag to indicate if
the static OPP are already parsed or not to make sure we don't
incorrectly skip initializing the static OPPs.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/of.c | 79 +++++++++++++++++++++++++++++++++----------------------
drivers/opp/opp.h | 2 ++
2 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1a9e1242a2a7..4a19f76880d3 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -23,6 +23,24 @@
#include "opp.h"
+/*
+ * Returns opp descriptor node for a device node, caller must
+ * do of_node_put().
+ */
+static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np,
+ int index)
+{
+ /* "operating-points-v2" can be an array for power domain providers */
+ return of_parse_phandle(np, "operating-points-v2", index);
+}
+
+/* Returns opp descriptor node for a device, caller must do of_node_put() */
+struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev)
+{
+ return _opp_of_get_opp_desc_node(dev->of_node, 0);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
+
static struct opp_table *_managed_opp(const struct device_node *np)
{
struct opp_table *opp_table, *managed_table = NULL;
@@ -55,22 +73,37 @@ static struct opp_table *_managed_opp(const struct device_node *np)
void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
int index)
{
- struct device_node *np;
+ struct device_node *np, *opp_np;
+ u32 val;
/*
* Only required for backward compatibility with v1 bindings, but isn't
* harmful for other cases. And so we do it unconditionally.
*/
np = of_node_get(dev->of_node);
- if (np) {
- u32 val;
-
- if (!of_property_read_u32(np, "clock-latency", &val))
- opp_table->clock_latency_ns_max = val;
- of_property_read_u32(np, "voltage-tolerance",
- &opp_table->voltage_tolerance_v1);
- of_node_put(np);
- }
+ if (!np)
+ return;
+
+ if (!of_property_read_u32(np, "clock-latency", &val))
+ opp_table->clock_latency_ns_max = val;
+ of_property_read_u32(np, "voltage-tolerance",
+ &opp_table->voltage_tolerance_v1);
+
+ /* Get OPP table node */
+ opp_np = _opp_of_get_opp_desc_node(np, index);
+ of_node_put(np);
+
+ if (!opp_np)
+ return;
+
+ if (of_property_read_bool(opp_np, "opp-shared"))
+ opp_table->shared_opp = OPP_TABLE_ACCESS_SHARED;
+ else
+ opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
+
+ opp_table->np = opp_np;
+
+ of_node_put(opp_np);
}
static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
@@ -250,22 +283,6 @@ void dev_pm_opp_of_remove_table(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
-/* Returns opp descriptor node for a device node, caller must
- * do of_node_put() */
-static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np,
- int index)
-{
- /* "operating-points-v2" can be an array for power domain providers */
- return of_parse_phandle(np, "operating-points-v2", index);
-}
-
-/* Returns opp descriptor node for a device, caller must do of_node_put() */
-struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev)
-{
- return _opp_of_get_opp_desc_node(dev->of_node, 0);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
-
/**
* _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
* @opp_table: OPP table
@@ -392,6 +409,9 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
/* OPPs are already managed */
if (!_add_opp_dev(dev, opp_table))
ret = -ENOMEM;
+ else if (!opp_table->parsed_static_opps)
+ goto initialize_static_opps;
+
goto put_opp_table;
}
@@ -399,6 +419,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
if (!opp_table)
return -ENOMEM;
+initialize_static_opps:
/* We have opp-table node now, iterate over it and add OPPs */
for_each_available_child_of_node(opp_np, np) {
count++;
@@ -434,11 +455,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
if (pstate_count)
opp_table->genpd_performance_state = true;
- opp_table->np = opp_np;
- if (of_property_read_bool(opp_np, "opp-shared"))
- opp_table->shared_opp = OPP_TABLE_ACCESS_SHARED;
- else
- opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
+ opp_table->parsed_static_opps = true;
put_opp_table:
dev_pm_opp_put_opp_table(opp_table);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index b235e76fc8cc..b04c2b511c4d 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -129,6 +129,7 @@ enum opp_table_access {
* @lock: mutex protecting the opp_list and dev_list.
* @np: struct device_node pointer for opp's DT node.
* @clock_latency_ns_max: Max clock latency in nanoseconds.
+ * @parsed_static_opps: True if OPPs are initialized from DT.
* @shared_opp: OPP is shared between multiple devices.
* @suspend_opp: Pointer to OPP to be used during device suspend.
* @supported_hw: Array of version number to support.
@@ -164,6 +165,7 @@ struct opp_table {
/* For backward compatibility with v1 bindings */
unsigned int voltage_tolerance_v1;
+ bool parsed_static_opps;
enum opp_table_access shared_opp;
struct dev_pm_opp *suspend_opp;
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 06/12] OPP: Don't take OPP table's kref for static OPPs
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (4 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 05/12] OPP: Parse OPP table's DT properties from _of_init_opp_table() Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 07/12] OPP: Create separate kref for static OPPs list Viresh Kumar
` (5 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
The reference count is only required to be incremented for every call
that may lead to adding the OPP table. For static OPPs the same should
be done from the parent routine which adds all static OPPs together and
so only one refcount for all static OPPs.
Update code to reflect that.
The refcount is incremented every time a dynamic OPP is created (as that
can lead to creating the OPP table) and the same is dropped when the OPP
is removed.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 332748adc262..2a6976265580 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -919,7 +919,6 @@ static void _opp_kref_release(struct kref *kref)
kfree(opp);
mutex_unlock(&opp_table->lock);
- dev_pm_opp_put_opp_table(opp_table);
}
void dev_pm_opp_get(struct dev_pm_opp *opp)
@@ -963,11 +962,15 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
if (found) {
dev_pm_opp_put(opp);
+
+ /* Drop the reference taken by dev_pm_opp_add() */
+ dev_pm_opp_put_opp_table(opp_table);
} else {
dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",
__func__, freq);
}
+ /* Drop the reference taken by _find_opp_table() */
dev_pm_opp_put_opp_table(opp_table);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
@@ -1085,9 +1088,6 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
new_opp->opp_table = opp_table;
kref_init(&new_opp->kref);
- /* Get a reference to the OPP table */
- _get_opp_table_kref(opp_table);
-
ret = opp_debug_create_one(new_opp, opp_table);
if (ret)
dev_err(dev, "%s: Failed to register opp to debugfs (%d)\n",
@@ -1566,8 +1566,9 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
return -ENOMEM;
ret = _opp_add_v1(opp_table, dev, freq, u_volt, true);
+ if (ret)
+ dev_pm_opp_put_opp_table(opp_table);
- dev_pm_opp_put_opp_table(opp_table);
return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_opp_add);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 07/12] OPP: Create separate kref for static OPPs list
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (5 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 06/12] OPP: Don't take OPP table's kref for static OPPs Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove() Viresh Kumar
` (4 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
The static OPPs don't always get freed with the OPP table, it can happen
before that as well. For example, if the OPP table is first created
using helpers like dev_pm_opp_set_supported_hw() and the OPPs are
created at a later point. Now when the OPPs are removed, the OPP table
stays until the time dev_pm_opp_put_supported_hw() is called.
Later patches will streamline the freeing of OPP table and that requires
the static OPPs to get freed with help of a separate kernel reference.
This patch prepares for that by creating a separate kref for static OPPs
list.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 33 ++++++++++++++++++++++++++++++++-
drivers/opp/of.c | 7 +++++++
drivers/opp/opp.h | 3 +++
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2a6976265580..b555121b878b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -892,6 +892,33 @@ static void _opp_table_kref_release(struct kref *kref)
mutex_unlock(&opp_table_lock);
}
+void _opp_remove_all_static(struct opp_table *opp_table)
+{
+ struct dev_pm_opp *opp, *tmp;
+
+ list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
+ if (!opp->dynamic)
+ dev_pm_opp_put(opp);
+ }
+
+ opp_table->parsed_static_opps = false;
+}
+
+static void _opp_table_list_kref_release(struct kref *kref)
+{
+ struct opp_table *opp_table = container_of(kref, struct opp_table,
+ list_kref);
+
+ _opp_remove_all_static(opp_table);
+ mutex_unlock(&opp_table_lock);
+}
+
+void _put_opp_list_kref(struct opp_table *opp_table)
+{
+ kref_put_mutex(&opp_table->list_kref, _opp_table_list_kref_release,
+ &opp_table_lock);
+}
+
void dev_pm_opp_put_opp_table(struct opp_table *opp_table)
{
kref_put_mutex(&opp_table->kref, _opp_table_kref_release,
@@ -1746,8 +1773,11 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
/* Find if opp_table manages a single device */
if (list_is_singular(&opp_table->dev_list)) {
/* Free static OPPs */
+ _put_opp_list_kref(opp_table);
+
+ /* Free dynamic OPPs */
list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
- if (remove_all || !opp->dynamic)
+ if (remove_all)
dev_pm_opp_put(opp);
}
@@ -1758,6 +1788,7 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
if (opp_table->genpd_performance_state)
dev_pm_genpd_set_performance_state(dev, 0);
} else {
+ _put_opp_list_kref(opp_table);
_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
}
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 4a19f76880d3..aaa4bab69846 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -411,6 +411,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
ret = -ENOMEM;
else if (!opp_table->parsed_static_opps)
goto initialize_static_opps;
+ else
+ kref_get(&opp_table->list_kref);
goto put_opp_table;
}
@@ -420,6 +422,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
return -ENOMEM;
initialize_static_opps:
+ kref_init(&opp_table->list_kref);
+
/* We have opp-table node now, iterate over it and add OPPs */
for_each_available_child_of_node(opp_np, np) {
count++;
@@ -437,6 +441,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
/* There should be one of more OPP defined */
if (WARN_ON(!count)) {
ret = -ENOENT;
+ _put_opp_list_kref(opp_table);
goto put_opp_table;
}
@@ -491,6 +496,8 @@ static int _of_add_opp_table_v1(struct device *dev)
if (!opp_table)
return -ENOMEM;
+ kref_init(&opp_table->list_kref);
+
val = prop->value;
while (nr) {
unsigned long freq = be32_to_cpup(val++) * 1000;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index b04c2b511c4d..9274116c90e4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -126,6 +126,7 @@ enum opp_table_access {
* @dev_list: list of devices that share these OPPs
* @opp_list: table of opps
* @kref: for reference count of the table.
+ * @list_kref: for reference count of the OPP list.
* @lock: mutex protecting the opp_list and dev_list.
* @np: struct device_node pointer for opp's DT node.
* @clock_latency_ns_max: Max clock latency in nanoseconds.
@@ -157,6 +158,7 @@ struct opp_table {
struct list_head dev_list;
struct list_head opp_list;
struct kref kref;
+ struct kref list_kref;
struct mutex lock;
struct device_node *np;
@@ -200,6 +202,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *o
int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of, int last_cpu);
struct opp_table *_add_opp_table(struct device *dev);
+void _put_opp_list_kref(struct opp_table *opp_table);
#ifdef CONFIG_OF
void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove()
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (6 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 07/12] OPP: Create separate kref for static OPPs list Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-20 7:59 ` Rafael J. Wysocki
2018-09-19 22:20 ` [PATCH V2 09/12] OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table() Viresh Kumar
` (3 subsequent siblings)
11 siblings, 1 reply; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Rafael J. Wysocki
Cc: Viresh Kumar, linux-pm, Stephen Boyd, Nishanth Menon,
Vincent Guittot, linux-arm-kernel, linux-kernel
dev_pm_opp_cpumask_remove_table() is going to change in the next commit
and will not remove dynamic OPPs automatically. They must be removed
with a call to dev_pm_opp_remove().
Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/mvebu-cpufreq.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/cpufreq/mvebu-cpufreq.c b/drivers/cpufreq/mvebu-cpufreq.c
index 31513bd42705..6d33a639f902 100644
--- a/drivers/cpufreq/mvebu-cpufreq.c
+++ b/drivers/cpufreq/mvebu-cpufreq.c
@@ -84,9 +84,10 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk) / 2, 0);
if (ret) {
+ dev_pm_opp_remove(cpu_dev, clk_get_rate(clk));
clk_put(clk);
dev_err(cpu_dev, "Failed to register OPPs\n");
- goto opp_register_failed;
+ return ret;
}
ret = dev_pm_opp_set_sharing_cpus(cpu_dev,
@@ -99,11 +100,5 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
return 0;
-
-opp_register_failed:
- /* As registering has failed remove all the opp for all cpus */
- dev_pm_opp_cpumask_remove_table(cpu_possible_mask);
-
- return ret;
}
device_initcall(armada_xp_pmsu_cpufreq_init);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 09/12] OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (7 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove() Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 10/12] OPP: Use a single mechanism to free the OPP table Viresh Kumar
` (2 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
Only one platform was depending on this feature and it is already
updated now. Stop removing dynamic OPPs from _dev_pm_opp_remove_table().
This simplifies lot of paths and removes unnecessary parameters.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 20 +++++---------------
drivers/opp/cpu.c | 9 +++------
drivers/opp/of.c | 12 ++++++------
drivers/opp/opp.h | 6 +++---
4 files changed, 17 insertions(+), 30 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index b555121b878b..2319ad4a0177 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1759,14 +1759,10 @@ int dev_pm_opp_unregister_notifier(struct device *dev,
EXPORT_SYMBOL(dev_pm_opp_unregister_notifier);
/*
- * Free OPPs either created using static entries present in DT or even the
- * dynamically added entries based on remove_all param.
+ * Free OPPs either created using static entries present in DT.
*/
-void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
- bool remove_all)
+void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev)
{
- struct dev_pm_opp *opp, *tmp;
-
/* Protect dev_list */
mutex_lock(&opp_table->lock);
@@ -1775,12 +1771,6 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
/* Free static OPPs */
_put_opp_list_kref(opp_table);
- /* Free dynamic OPPs */
- list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
- if (remove_all)
- dev_pm_opp_put(opp);
- }
-
/*
* The OPP table is getting removed, drop the performance state
* constraints.
@@ -1795,7 +1785,7 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
mutex_unlock(&opp_table->lock);
}
-void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all)
+void _dev_pm_opp_find_and_remove_table(struct device *dev)
{
struct opp_table *opp_table;
@@ -1812,7 +1802,7 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all)
return;
}
- _dev_pm_opp_remove_table(opp_table, dev, remove_all);
+ _dev_pm_opp_remove_table(opp_table, dev);
dev_pm_opp_put_opp_table(opp_table);
}
@@ -1826,6 +1816,6 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all)
*/
void dev_pm_opp_remove_table(struct device *dev)
{
- _dev_pm_opp_find_and_remove_table(dev, true);
+ _dev_pm_opp_find_and_remove_table(dev);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
diff --git a/drivers/opp/cpu.c b/drivers/opp/cpu.c
index 36586f66cd83..ab6d07e78945 100644
--- a/drivers/opp/cpu.c
+++ b/drivers/opp/cpu.c
@@ -108,7 +108,7 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
#endif /* CONFIG_CPU_FREQ */
-void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of,
+void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask,
int last_cpu)
{
struct device *cpu_dev;
@@ -127,10 +127,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of,
continue;
}
- if (of)
- dev_pm_opp_of_remove_table(cpu_dev);
- else
- dev_pm_opp_remove_table(cpu_dev);
+ _dev_pm_opp_find_and_remove_table(cpu_dev);
}
}
@@ -144,7 +141,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of,
*/
void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask)
{
- _dev_pm_opp_cpumask_remove_table(cpumask, false, -1);
+ _dev_pm_opp_cpumask_remove_table(cpumask, -1);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_remove_table);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index aaa4bab69846..861cc75de329 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -279,7 +279,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
*/
void dev_pm_opp_of_remove_table(struct device *dev)
{
- _dev_pm_opp_find_and_remove_table(dev, false);
+ _dev_pm_opp_find_and_remove_table(dev);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
@@ -432,7 +432,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
if (ret) {
dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
ret);
- _dev_pm_opp_remove_table(opp_table, dev, false);
+ _dev_pm_opp_remove_table(opp_table, dev);
of_node_put(np);
goto put_opp_table;
}
@@ -453,7 +453,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
count, pstate_count);
ret = -ENOENT;
- _dev_pm_opp_remove_table(opp_table, dev, false);
+ _dev_pm_opp_remove_table(opp_table, dev);
goto put_opp_table;
}
@@ -507,7 +507,7 @@ static int _of_add_opp_table_v1(struct device *dev)
if (ret) {
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
__func__, freq, ret);
- _dev_pm_opp_remove_table(opp_table, dev, false);
+ _dev_pm_opp_remove_table(opp_table, dev);
break;
}
nr -= 2;
@@ -618,7 +618,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
*/
void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask)
{
- _dev_pm_opp_cpumask_remove_table(cpumask, true, -1);
+ _dev_pm_opp_cpumask_remove_table(cpumask, -1);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_remove_table);
@@ -653,7 +653,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask)
__func__, cpu, ret);
/* Free all other OPPs */
- _dev_pm_opp_cpumask_remove_table(cpumask, true, cpu);
+ _dev_pm_opp_cpumask_remove_table(cpumask, cpu);
break;
}
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9274116c90e4..98dd7d39e1ad 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -194,13 +194,13 @@ void _get_opp_table_kref(struct opp_table *opp_table);
int _get_opp_count(struct opp_table *opp_table);
struct opp_table *_find_opp_table(struct device *dev);
struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
-void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev, bool remove_all);
-void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all);
+void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev);
+void _dev_pm_opp_find_and_remove_table(struct device *dev);
struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
void _opp_free(struct dev_pm_opp *opp);
int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
-void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of, int last_cpu);
+void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
struct opp_table *_add_opp_table(struct device *dev);
void _put_opp_list_kref(struct opp_table *opp_table);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 10/12] OPP: Use a single mechanism to free the OPP table
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (8 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 09/12] OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table() Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 11/12] OPP: Prevent creating multiple OPP tables for devices sharing OPP nodes Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 12/12] OPP: Pass OPP table to _of_add_opp_table_v{1|2}() Viresh Kumar
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
Currently there are two separate ways to free the OPP table based on how
it is created in the first place.
We call _dev_pm_opp_remove_table() to free the static and/or dynamic
OPP, OPP list devices, etc. This is done for the case where the OPP
table is added while initializing the OPPs, like via the path
dev_pm_opp_of_add_table().
We also call dev_pm_opp_put_opp_table() in some cases which eventually
frees the OPP table structure once the reference count reaches 0. This
is used by the first case as well as other cases like
dev_pm_opp_set_regulators() where the OPPs aren't necessarily
initialized at this point.
This whole thing is a bit unclear and messy and obstruct any further
cleanup/fixup of OPP core.
This patch tries to streamline this by keeping a single path for OPP
table destruction, i.e. dev_pm_opp_put_opp_table().
All the cleanup happens in _opp_table_kref_release() now after the
reference count reaches 0. _dev_pm_opp_remove_table() is removed as it
isn't required anymore.
We don't drop the reference to the OPP table after creating it from
_of_add_opp_table_v{1|2}() anymore and the same is dropped only when we
try to remove them.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 54 ++++++++++++++++--------------------------------------
drivers/opp/of.c | 32 ++++++++++++++++++--------------
drivers/opp/opp.h | 2 +-
3 files changed, 35 insertions(+), 53 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2319ad4a0177..d3e33fd32694 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -867,23 +867,24 @@ struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev,
static void _opp_table_kref_release(struct kref *kref)
{
struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
- struct opp_device *opp_dev;
+ struct opp_device *opp_dev, *temp;
/* Release clk */
if (!IS_ERR(opp_table->clk))
clk_put(opp_table->clk);
- /*
- * No need to take opp_table->lock here as we are guaranteed that no
- * references to the OPP table are taken at this point.
- */
- opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device,
- node);
+ WARN_ON(!list_empty(&opp_table->opp_list));
- _remove_opp_dev(opp_dev, opp_table);
+ list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
+ /*
+ * The OPP table is getting removed, drop the performance state
+ * constraints.
+ */
+ if (opp_table->genpd_performance_state)
+ dev_pm_genpd_set_performance_state((struct device *)(opp_dev->dev), 0);
- /* dev_list must be empty now */
- WARN_ON(!list_empty(&opp_table->dev_list));
+ _remove_opp_dev(opp_dev, opp_table);
+ }
mutex_destroy(&opp_table->lock);
list_del(&opp_table->node);
@@ -1758,33 +1759,6 @@ int dev_pm_opp_unregister_notifier(struct device *dev,
}
EXPORT_SYMBOL(dev_pm_opp_unregister_notifier);
-/*
- * Free OPPs either created using static entries present in DT.
- */
-void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev)
-{
- /* Protect dev_list */
- mutex_lock(&opp_table->lock);
-
- /* Find if opp_table manages a single device */
- if (list_is_singular(&opp_table->dev_list)) {
- /* Free static OPPs */
- _put_opp_list_kref(opp_table);
-
- /*
- * The OPP table is getting removed, drop the performance state
- * constraints.
- */
- if (opp_table->genpd_performance_state)
- dev_pm_genpd_set_performance_state(dev, 0);
- } else {
- _put_opp_list_kref(opp_table);
- _remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
- }
-
- mutex_unlock(&opp_table->lock);
-}
-
void _dev_pm_opp_find_and_remove_table(struct device *dev)
{
struct opp_table *opp_table;
@@ -1802,8 +1776,12 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev)
return;
}
- _dev_pm_opp_remove_table(opp_table, dev);
+ _put_opp_list_kref(opp_table);
+
+ /* Drop reference taken by _find_opp_table() */
+ dev_pm_opp_put_opp_table(opp_table);
+ /* Drop reference taken while the OPP table was added */
dev_pm_opp_put_opp_table(opp_table);
}
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 861cc75de329..ae0436eaa911 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -407,14 +407,17 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
opp_table = _managed_opp(opp_np);
if (opp_table) {
/* OPPs are already managed */
- if (!_add_opp_dev(dev, opp_table))
+ if (!_add_opp_dev(dev, opp_table)) {
ret = -ENOMEM;
- else if (!opp_table->parsed_static_opps)
- goto initialize_static_opps;
- else
+ goto put_opp_table;
+ }
+
+ if (opp_table->parsed_static_opps) {
kref_get(&opp_table->list_kref);
+ return 0;
+ }
- goto put_opp_table;
+ goto initialize_static_opps;
}
opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
@@ -432,17 +435,15 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
if (ret) {
dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
ret);
- _dev_pm_opp_remove_table(opp_table, dev);
of_node_put(np);
- goto put_opp_table;
+ goto put_list_kref;
}
}
/* There should be one of more OPP defined */
if (WARN_ON(!count)) {
ret = -ENOENT;
- _put_opp_list_kref(opp_table);
- goto put_opp_table;
+ goto put_list_kref;
}
list_for_each_entry(opp, &opp_table->opp_list, node)
@@ -453,8 +454,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
count, pstate_count);
ret = -ENOENT;
- _dev_pm_opp_remove_table(opp_table, dev);
- goto put_opp_table;
+ goto put_list_kref;
}
if (pstate_count)
@@ -462,6 +462,10 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
opp_table->parsed_static_opps = true;
+ return 0;
+
+put_list_kref:
+ _put_opp_list_kref(opp_table);
put_opp_table:
dev_pm_opp_put_opp_table(opp_table);
@@ -507,13 +511,13 @@ static int _of_add_opp_table_v1(struct device *dev)
if (ret) {
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
__func__, freq, ret);
- _dev_pm_opp_remove_table(opp_table, dev);
- break;
+ _put_opp_list_kref(opp_table);
+ dev_pm_opp_put_opp_table(opp_table);
+ return ret;
}
nr -= 2;
}
- dev_pm_opp_put_opp_table(opp_table);
return ret;
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 98dd7d39e1ad..f9fbb7553fc4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -190,11 +190,11 @@ struct opp_table {
/* Routines internal to opp core */
void dev_pm_opp_get(struct dev_pm_opp *opp);
+void _opp_remove_all_static(struct opp_table *opp_table);
void _get_opp_table_kref(struct opp_table *opp_table);
int _get_opp_count(struct opp_table *opp_table);
struct opp_table *_find_opp_table(struct device *dev);
struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
-void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev);
void _dev_pm_opp_find_and_remove_table(struct device *dev);
struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
void _opp_free(struct dev_pm_opp *opp);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 11/12] OPP: Prevent creating multiple OPP tables for devices sharing OPP nodes
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (9 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 10/12] OPP: Use a single mechanism to free the OPP table Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 12/12] OPP: Pass OPP table to _of_add_opp_table_v{1|2}() Viresh Kumar
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
When two or more devices are sharing their clock and voltage rails, they
share the same OPP table. But there are some corner cases where the OPP
core incorrectly creates separate OPP tables for them.
For example, CPU 0 and 1 share clock/voltage rails. The platform
specific code calls dev_pm_opp_set_regulators() for CPU0 and the OPP
core creates an OPP table for it (the individual OPPs aren't initialized
as of now). The same is repeated for CPU1 then. Because
_opp_get_opp_table() doesn't compare DT node pointers currently, it
fails to find the link between CPU0 and CPU1 and so creates a new OPP
table.
Fix this by calling _managed_opp() from _opp_get_opp_table().
_managed_opp() gain an additional argument (index) to get the right node
pointer. This resulted in simplifying code in _of_add_opp_table_v2() as
well.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 25 ++++++++++++++++++++++---
drivers/opp/of.c | 35 +++++++++++++----------------------
drivers/opp/opp.h | 2 ++
3 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index d3e33fd32694..aaef20cf4df2 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -759,8 +759,8 @@ static void _remove_opp_dev(struct opp_device *opp_dev,
kfree(opp_dev);
}
-struct opp_device *_add_opp_dev(const struct device *dev,
- struct opp_table *opp_table)
+struct opp_device *_add_opp_dev_unlocked(const struct device *dev,
+ struct opp_table *opp_table)
{
struct opp_device *opp_dev;
int ret;
@@ -772,7 +772,6 @@ struct opp_device *_add_opp_dev(const struct device *dev,
/* Initialize opp-dev */
opp_dev->dev = dev;
- mutex_lock(&opp_table->lock);
list_add(&opp_dev->node, &opp_table->dev_list);
/* Create debugfs entries for the opp_table */
@@ -780,6 +779,17 @@ struct opp_device *_add_opp_dev(const struct device *dev,
if (ret)
dev_err(dev, "%s: Failed to register opp debugfs (%d)\n",
__func__, ret);
+
+ return opp_dev;
+}
+
+struct opp_device *_add_opp_dev(const struct device *dev,
+ struct opp_table *opp_table)
+{
+ struct opp_device *opp_dev;
+
+ mutex_lock(&opp_table->lock);
+ opp_dev = _add_opp_dev_unlocked(dev, opp_table);
mutex_unlock(&opp_table->lock);
return opp_dev;
@@ -844,6 +854,15 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
if (!IS_ERR(opp_table))
goto unlock;
+ opp_table = _managed_opp(dev, index);
+ if (opp_table) {
+ if (!_add_opp_dev_unlocked(dev, opp_table)) {
+ dev_pm_opp_put_opp_table(opp_table);
+ opp_table = NULL;
+ }
+ goto unlock;
+ }
+
opp_table = _allocate_opp_table(dev, index);
unlock:
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ae0436eaa911..1ddef52c27fd 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -41,11 +41,14 @@ struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
-static struct opp_table *_managed_opp(const struct device_node *np)
+struct opp_table *_managed_opp(struct device *dev, int index)
{
struct opp_table *opp_table, *managed_table = NULL;
+ struct device_node *np;
- mutex_lock(&opp_table_lock);
+ np = _opp_of_get_opp_desc_node(dev->of_node, index);
+ if (!np)
+ return NULL;
list_for_each_entry(opp_table, &opp_tables, node) {
if (opp_table->np == np) {
@@ -65,7 +68,7 @@ static struct opp_table *_managed_opp(const struct device_node *np)
}
}
- mutex_unlock(&opp_table_lock);
+ of_node_put(np);
return managed_table;
}
@@ -401,30 +404,19 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
{
struct device_node *np;
struct opp_table *opp_table;
- int ret = 0, count = 0, pstate_count = 0;
+ int ret, count = 0, pstate_count = 0;
struct dev_pm_opp *opp;
- opp_table = _managed_opp(opp_np);
- if (opp_table) {
- /* OPPs are already managed */
- if (!_add_opp_dev(dev, opp_table)) {
- ret = -ENOMEM;
- goto put_opp_table;
- }
-
- if (opp_table->parsed_static_opps) {
- kref_get(&opp_table->list_kref);
- return 0;
- }
-
- goto initialize_static_opps;
- }
-
opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
if (!opp_table)
return -ENOMEM;
-initialize_static_opps:
+ /* OPP table is already initialized for the device */
+ if (opp_table->parsed_static_opps) {
+ kref_get(&opp_table->list_kref);
+ return 0;
+ }
+
kref_init(&opp_table->list_kref);
/* We have opp-table node now, iterate over it and add OPPs */
@@ -466,7 +458,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
put_list_kref:
_put_opp_list_kref(opp_table);
-put_opp_table:
dev_pm_opp_put_opp_table(opp_table);
return ret;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index f9fbb7553fc4..9c6544b4f4f9 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -206,8 +206,10 @@ void _put_opp_list_kref(struct opp_table *opp_table);
#ifdef CONFIG_OF
void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
+struct opp_table *_managed_opp(struct device *dev, int index);
#else
static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
+static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
#endif
#ifdef CONFIG_DEBUG_FS
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 12/12] OPP: Pass OPP table to _of_add_opp_table_v{1|2}()
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
` (10 preceding siblings ...)
2018-09-19 22:20 ` [PATCH V2 11/12] OPP: Prevent creating multiple OPP tables for devices sharing OPP nodes Viresh Kumar
@ 2018-09-19 22:20 ` Viresh Kumar
11 siblings, 0 replies; 14+ messages in thread
From: Viresh Kumar @ 2018-09-19 22:20 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, Rafael Wysocki, linux-pm, Vincent Guittot,
Niklas Cassel, linux-kernel
Both _of_add_opp_table_v1() and _of_add_opp_table_v2() contain similar
code to get the OPP table and their parent routine also parses the DT to
find the OPP table's node pointer. This can be simplified by getting the
OPP table in advance and then passing it as argument to these routines.
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/of.c | 68 +++++++++++++++++++++++---------------------------------
1 file changed, 28 insertions(+), 40 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1ddef52c27fd..a71ff3acca0f 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -399,18 +399,12 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
}
/* Initializes OPP tables based on new bindings */
-static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
- int index)
+static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
{
struct device_node *np;
- struct opp_table *opp_table;
int ret, count = 0, pstate_count = 0;
struct dev_pm_opp *opp;
- opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
- if (!opp_table)
- return -ENOMEM;
-
/* OPP table is already initialized for the device */
if (opp_table->parsed_static_opps) {
kref_get(&opp_table->list_kref);
@@ -420,7 +414,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
kref_init(&opp_table->list_kref);
/* We have opp-table node now, iterate over it and add OPPs */
- for_each_available_child_of_node(opp_np, np) {
+ for_each_available_child_of_node(opp_table->np, np) {
count++;
ret = _opp_add_static_v2(opp_table, dev, np);
@@ -458,15 +452,13 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np,
put_list_kref:
_put_opp_list_kref(opp_table);
- dev_pm_opp_put_opp_table(opp_table);
return ret;
}
/* Initializes OPP tables based on old-deprecated bindings */
-static int _of_add_opp_table_v1(struct device *dev)
+static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
{
- struct opp_table *opp_table;
const struct property *prop;
const __be32 *val;
int nr, ret = 0;
@@ -487,10 +479,6 @@ static int _of_add_opp_table_v1(struct device *dev)
return -EINVAL;
}
- opp_table = dev_pm_opp_get_opp_table(dev);
- if (!opp_table)
- return -ENOMEM;
-
kref_init(&opp_table->list_kref);
val = prop->value;
@@ -503,7 +491,6 @@ static int _of_add_opp_table_v1(struct device *dev)
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
__func__, freq, ret);
_put_opp_list_kref(opp_table);
- dev_pm_opp_put_opp_table(opp_table);
return ret;
}
nr -= 2;
@@ -531,24 +518,24 @@ static int _of_add_opp_table_v1(struct device *dev)
*/
int dev_pm_opp_of_add_table(struct device *dev)
{
- struct device_node *opp_np;
+ struct opp_table *opp_table;
int ret;
+ opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
+ if (!opp_table)
+ return -ENOMEM;
+
/*
- * OPPs have two version of bindings now. The older one is deprecated,
- * try for the new binding first.
+ * OPPs have two version of bindings now. Also try the old (v1)
+ * bindings for backward compatibility with older dtbs.
*/
- opp_np = dev_pm_opp_of_get_opp_desc_node(dev);
- if (!opp_np) {
- /*
- * Try old-deprecated bindings for backward compatibility with
- * older dtbs.
- */
- return _of_add_opp_table_v1(dev);
- }
+ if (opp_table->np)
+ ret = _of_add_opp_table_v2(dev, opp_table);
+ else
+ ret = _of_add_opp_table_v1(dev, opp_table);
- ret = _of_add_opp_table_v2(dev, opp_np, 0);
- of_node_put(opp_np);
+ if (ret)
+ dev_pm_opp_put_opp_table(opp_table);
return ret;
}
@@ -575,28 +562,29 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
*/
int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
{
- struct device_node *opp_np;
+ struct opp_table *opp_table;
int ret, count;
-again:
- opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
- if (!opp_np) {
+ if (index) {
/*
* If only one phandle is present, then the same OPP table
* applies for all index requests.
*/
count = of_count_phandle_with_args(dev->of_node,
"operating-points-v2", NULL);
- if (count == 1 && index) {
- index = 0;
- goto again;
- }
+ if (count != 1)
+ return -ENODEV;
- return -ENODEV;
+ index = 0;
}
- ret = _of_add_opp_table_v2(dev, opp_np, index);
- of_node_put(opp_np);
+ opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
+ if (!opp_table)
+ return -ENOMEM;
+
+ ret = _of_add_opp_table_v2(dev, opp_table);
+ if (ret)
+ dev_pm_opp_put_opp_table(opp_table);
return ret;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove()
2018-09-19 22:20 ` [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove() Viresh Kumar
@ 2018-09-20 7:59 ` Rafael J. Wysocki
0 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2018-09-20 7:59 UTC (permalink / raw)
To: Viresh Kumar
Cc: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, linux-pm, Stephen Boyd, Nishanth Menon,
Vincent Guittot, linux-arm-kernel, linux-kernel
On Thursday, September 20, 2018 12:20:27 AM CEST Viresh Kumar wrote:
> dev_pm_opp_cpumask_remove_table() is going to change in the next commit
> and will not remove dynamic OPPs automatically. They must be removed
> with a call to dev_pm_opp_remove().
>
> Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/mvebu-cpufreq.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/mvebu-cpufreq.c b/drivers/cpufreq/mvebu-cpufreq.c
> index 31513bd42705..6d33a639f902 100644
> --- a/drivers/cpufreq/mvebu-cpufreq.c
> +++ b/drivers/cpufreq/mvebu-cpufreq.c
> @@ -84,9 +84,10 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
>
> ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk) / 2, 0);
> if (ret) {
> + dev_pm_opp_remove(cpu_dev, clk_get_rate(clk));
> clk_put(clk);
> dev_err(cpu_dev, "Failed to register OPPs\n");
> - goto opp_register_failed;
> + return ret;
> }
>
> ret = dev_pm_opp_set_sharing_cpus(cpu_dev,
> @@ -99,11 +100,5 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
>
> platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> return 0;
> -
> -opp_register_failed:
> - /* As registering has failed remove all the opp for all cpus */
> - dev_pm_opp_cpumask_remove_table(cpu_possible_mask);
> -
> - return ret;
> }
> device_initcall(armada_xp_pmsu_cpufreq_init);
>
Please feel free to take this one via OPP.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-09-20 8:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19 22:20 [PATCH V2 00/12] OPP: Don't create multiple OPP tables for devices sharing OPP table Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 01/12] OPP: Free OPP table properly on performance state irregularities Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 02/12] OPP: Don't try to remove all OPP tables on failure Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 03/12] OPP: Protect dev_list with opp_table lock Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 04/12] OPP: Pass index to _of_init_opp_table() Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 05/12] OPP: Parse OPP table's DT properties from _of_init_opp_table() Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 06/12] OPP: Don't take OPP table's kref for static OPPs Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 07/12] OPP: Create separate kref for static OPPs list Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 08/12] cpufreq: mvebu: Remove OPPs using dev_pm_opp_remove() Viresh Kumar
2018-09-20 7:59 ` Rafael J. Wysocki
2018-09-19 22:20 ` [PATCH V2 09/12] OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table() Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 10/12] OPP: Use a single mechanism to free the OPP table Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 11/12] OPP: Prevent creating multiple OPP tables for devices sharing OPP nodes Viresh Kumar
2018-09-19 22:20 ` [PATCH V2 12/12] OPP: Pass OPP table to _of_add_opp_table_v{1|2}() Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).