[PATCH 0/7] PM /Domain/OPP: Add support to get performance state from DT

[PATCH 0/7] PM /Domain/OPP: Add support to get performance state from DT

[Viresh Kumar]

Hi,

Now that the DT bindings [1] are already Reviewed/Acked by respective
maintainers, here is the code to start using them.

The first two patches provide helpers in the OPP core, [3-5]/7 update
the PM domain core to start supporting domain OPP tables, etc, 6/7
updates the OPP core to use the new callback provided by the PM domains
to get performance state and the last one removes the unused helpers
now.

This is tested on Hikey620 and works just fine.

--
viresh

[1] https://lkml.kernel.org/r/cover.1513591822.git.viresh.kumar@linaro.org

Viresh Kumar (7):
  PM / OPP: Implement dev_pm_opp_of_add_table_indexed()
  PM / OPP: Implement of_dev_pm_opp_find_required_opp()
  PM / Domain: Add struct device to genpd
  PM / Domain: Add support to parse domain's OPP table
  PM / Domain: Implement of_dev_pm_genpd_get_performance_state()
  PM / OPP: Get performance state using genpd helper
  PM / OPP: Remove dev_pm_opp_{un}register_get_pstate_helper()

 drivers/base/power/domain.c | 159 ++++++++++++++++++++++++++++++++++++++++----
 drivers/opp/core.c          |  82 +----------------------
 drivers/opp/of.c            | 123 +++++++++++++++++++++++++++++++---
 drivers/opp/opp.h           |   3 +-
 include/linux/pm_domain.h   |  12 ++++
 include/linux/pm_opp.h      |  22 +++---
 6 files changed, 284 insertions(+), 117 deletions(-)

-- 
2.15.0.194.g9af6a3dea062

[PATCH 1/7] PM / OPP: Implement dev_pm_opp_of_add_table_indexed()

[Viresh Kumar]

The "operating-points-v2" property can contain a list of phandles now,
specifically for the power domain providers that provide multiple
domains.

Add support to parse that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/of.c       | 50 +++++++++++++++++++++++++++++++++++++++++---------
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index cb716aa2f44b..22c9bd191f62 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -250,20 +250,17 @@ 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)
+static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np,
+						     int index)
 {
-	/*
-	 * There should be only ONE phandle present in "operating-points-v2"
-	 * property.
-	 */
-
-	return of_parse_phandle(np, "operating-points-v2", 0);
+	/* "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);
+	return _opp_of_get_opp_desc_node(dev->of_node, 0);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
 
@@ -509,6 +506,41 @@ int dev_pm_opp_of_add_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
 
+/**
+ * dev_pm_opp_of_add_table_indexed() - Initialize indexed opp table from device tree
+ * @dev:	device pointer used to lookup OPP table.
+ * @index:	Index number.
+ *
+ * Register the initial OPP table with the OPP library for given device only
+ * using the "operating-points-v2" property.
+ *
+ * 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
+ * -EINVAL	when invalid entries are found in opp-v2 table
+ */
+int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
+{
+	struct device_node *opp_np;
+	int ret;
+
+	opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
+	if (!opp_np)
+		return -ENODEV;
+
+	ret = _of_add_opp_table_v2(dev, opp_np);
+	of_node_put(opp_np);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
+
 /* CPU device specific helpers */
 
 /**
@@ -613,7 +645,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
 		}
 
 		/* Get OPP descriptor node */
-		tmp_np = _opp_of_get_opp_desc_node(cpu_np);
+		tmp_np = _opp_of_get_opp_desc_node(cpu_np, 0);
 		of_node_put(cpu_np);
 		if (!tmp_np) {
 			pr_err("%pOF: Couldn't find opp node\n", cpu_np);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6c2d2e88f066..f042fdeaaa3c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -303,6 +303,7 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
 int dev_pm_opp_of_add_table(struct device *dev);
+int dev_pm_opp_of_add_table_indexed(struct device *dev, int index);
 void dev_pm_opp_of_remove_table(struct device *dev);
 int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
 void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
@@ -314,6 +315,11 @@ static inline int dev_pm_opp_of_add_table(struct device *dev)
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
+{
+	return -ENOTSUPP;
+}
+
 static inline void dev_pm_opp_of_remove_table(struct device *dev)
 {
 }
-- 
2.15.0.194.g9af6a3dea062

[PATCH 2/7] PM / OPP: Implement of_dev_pm_opp_find_required_opp()

[Viresh Kumar]

A device's DT node or its OPP nodes can contain a phandle to other
device's OPP node, in the "required-opp" property.

This patch implements a routine to find that required OPP from the node
that contains the "required-opp" property.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     |  4 +---
 drivers/opp/of.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h      |  1 +
 include/linux/pm_opp.h |  6 ++++++
 4 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 92fa94a6dcc1..bfcad8983fd1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -33,8 +33,6 @@ LIST_HEAD(opp_tables);
 /* Lock to allow exclusive modification to the device and opp lists */
 DEFINE_MUTEX(opp_table_lock);
 
-static void dev_pm_opp_get(struct dev_pm_opp *opp);
-
 static struct opp_device *_find_opp_dev(const struct device *dev,
 					struct opp_table *opp_table)
 {
@@ -892,7 +890,7 @@ static void _opp_kref_release(struct kref *kref)
 	dev_pm_opp_put_opp_table(opp_table);
 }
 
-static void dev_pm_opp_get(struct dev_pm_opp *opp)
+void dev_pm_opp_get(struct dev_pm_opp *opp)
 {
 	kref_get(&opp->kref);
 }
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 22c9bd191f62..2d1bb348ae73 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -665,3 +665,56 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_sharing_cpus);
+
+/**
+ * of_dev_pm_opp_find_required_opp() - Search for required OPP.
+ * @dev: The device whose OPP node is referenced by the 'np' DT node.
+ * @np: Node that contains the "required-opp" property.
+ *
+ * Returns the OPP of the device 'dev', whose phandle is present in the "np"
+ * node.
+ *
+ * Return: Matching opp, else returns ERR_PTR in case of error and should be
+ * handled using IS_ERR.
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev,
+						   struct device_node *np)
+{
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
+	struct device_node *required_np;
+	struct opp_table *opp_table;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	required_np = of_parse_phandle(np, "required-opp", 0);
+	if (unlikely(!required_np)) {
+		dev_err(dev, "Unable to parse required-opp\n");
+		goto put_opp_table;
+	}
+
+	mutex_lock(&opp_table->lock);
+
+	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
+		if (temp_opp->available && temp_opp->np == required_np) {
+			opp = temp_opp;
+
+			/* Increment the reference count of OPP */
+			dev_pm_opp_get(opp);
+			break;
+		}
+	}
+
+	mutex_unlock(&opp_table->lock);
+
+	of_node_put(required_np);
+put_opp_table:
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(of_dev_pm_opp_find_required_opp);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4d00061648a3..b181032e6938 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -187,6 +187,7 @@ struct opp_table {
 };
 
 /* Routines internal to opp core */
+void dev_pm_opp_get(struct dev_pm_opp *opp);
 void _get_opp_table_kref(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);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f042fdeaaa3c..70686f434c13 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -309,6 +309,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
 void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
+struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np);
 #else
 static inline int dev_pm_opp_of_add_table(struct device *dev)
 {
@@ -342,6 +343,11 @@ static inline struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device
 {
 	return NULL;
 }
+
+static inline struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np)
+{
+	return NULL;
+}
 #endif
 
 #endif		/* __LINUX_OPP_H__ */
-- 
2.15.0.194.g9af6a3dea062

[PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

The power-domain core would be using the OPP core going forward and the
OPP core has the basic requirement of a device structure for its working.

Add a struct device to the genpd structure and also add a genpd bus type
for the devices.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0c80bea05bcb..013c1e206167 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
 	}
 }
 
+static struct bus_type genpd_bus_type = {
+	.name =		"genpd",
+};
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 			return ret;
 	}
 
+	genpd->dev.bus = &genpd_bus_type;
+	device_initialize(&genpd->dev);
+	dev_set_name(&genpd->dev, "%s", genpd->name);
+
+	ret = device_add(&genpd->dev);
+	if (ret) {
+		dev_err(&genpd->dev, "failed to add device: %d\n", ret);
+		put_device(&genpd->dev);
+		kfree(genpd->free);
+		return ret;
+	}
+
 	mutex_lock(&gpd_list_lock);
 	list_add(&genpd->gpd_list_node, &gpd_list);
 	mutex_unlock(&gpd_list_lock);
@@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 
 	list_del(&genpd->gpd_list_node);
 	genpd_unlock(genpd);
+	device_del(&genpd->dev);
 	cancel_work_sync(&genpd->power_off_work);
 	kfree(genpd->free);
 	pr_debug("%s: removed %s\n", __func__, genpd->name);
@@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 		if (!ret) {
 			genpd->provider = &np->fwnode;
 			genpd->has_provider = true;
+			genpd->dev.of_node = np;
 		}
 	}
 
@@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 		data->domains[i]->provider = &np->fwnode;
 		data->domains[i]->has_provider = true;
+		data->domains[i]->dev.of_node = np;
 	}
 
 	ret = genpd_add_provider(np, data->xlate, data);
@@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void)
 }
 __exitcall(genpd_debug_exit);
 #endif /* CONFIG_DEBUG_FS */
+
+static int __init pm_genpd_core_init(void)
+{
+	int ret;
+
+	ret = bus_register(&genpd_bus_type);
+	if (ret)
+		pr_err("bus_register failed (%d)\n", ret);
+
+	return ret;
+}
+pure_initcall(pm_genpd_core_init);
+
+static void __exit pm_genpd_core_exit(void)
+{
+	bus_unregister(&genpd_bus_type);
+}
+__exitcall(pm_genpd_core_exit);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 04dbef9847d3..aaacaa35005d 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -49,6 +49,7 @@ struct genpd_power_state {
 struct genpd_lock_ops;
 
 struct generic_pm_domain {
+	struct device dev;
 	struct dev_pm_domain domain;	/* PM domain operations */
 	struct list_head gpd_list_node;	/* Node in the global PM domains list */
 	struct list_head master_links;	/* Links with PM domain as a master */
-- 
2.15.0.194.g9af6a3dea062

[PATCH 4/7] PM / Domain: Add support to parse domain's OPP table

[Viresh Kumar]

Parse the OPP table for power domains if they have their
set_performance_state() callback set.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 78 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 013c1e206167..1ad4ad0b0de0 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_qos.h>
@@ -1900,15 +1901,33 @@ int of_genpd_add_provider_simple(struct device_node *np,
 
 	mutex_lock(&gpd_list_lock);
 
-	if (genpd_present(genpd)) {
-		ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
-		if (!ret) {
-			genpd->provider = &np->fwnode;
-			genpd->has_provider = true;
-			genpd->dev.of_node = np;
+	if (!genpd_present(genpd))
+		goto unlock;
+
+	genpd->dev.of_node = np;
+
+	/* Parse genpd OPP table */
+	if (genpd->set_performance_state) {
+		ret = dev_pm_opp_of_add_table(&genpd->dev);
+		if (ret) {
+			dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
+				ret);
+			goto unlock;
 		}
 	}
 
+	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
+	if (ret) {
+		if (genpd->set_performance_state)
+			dev_pm_opp_of_remove_table(&genpd->dev);
+
+		goto unlock;
+	}
+
+	genpd->provider = &np->fwnode;
+	genpd->has_provider = true;
+
+unlock:
 	mutex_unlock(&gpd_list_lock);
 
 	return ret;
@@ -1923,6 +1942,7 @@ EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
 int of_genpd_add_provider_onecell(struct device_node *np,
 				  struct genpd_onecell_data *data)
 {
+	struct generic_pm_domain *genpd;
 	unsigned int i;
 	int ret = -EINVAL;
 
@@ -1935,14 +1955,27 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		data->xlate = genpd_xlate_onecell;
 
 	for (i = 0; i < data->num_domains; i++) {
-		if (!data->domains[i])
+		genpd = data->domains[i];
+
+		if (!genpd)
 			continue;
-		if (!genpd_present(data->domains[i]))
+		if (!genpd_present(genpd))
 			goto error;
 
-		data->domains[i]->provider = &np->fwnode;
-		data->domains[i]->has_provider = true;
-		data->domains[i]->dev.of_node = np;
+		genpd->dev.of_node = np;
+
+		/* Parse genpd OPP table */
+		if (genpd->set_performance_state) {
+			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
+			if (ret) {
+				dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
+					i, ret);
+				goto error;
+			}
+		}
+
+		genpd->provider = &np->fwnode;
+		genpd->has_provider = true;
 	}
 
 	ret = genpd_add_provider(np, data->xlate, data);
@@ -1955,10 +1988,16 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 error:
 	while (i--) {
-		if (!data->domains[i])
+		genpd = data->domains[i];
+
+		if (!genpd)
 			continue;
-		data->domains[i]->provider = NULL;
-		data->domains[i]->has_provider = false;
+
+		genpd->provider = NULL;
+		genpd->has_provider = false;
+
+		if (genpd->set_performance_state)
+			dev_pm_opp_of_remove_table(&genpd->dev);
 	}
 
 	mutex_unlock(&gpd_list_lock);
@@ -1985,10 +2024,17 @@ void of_genpd_del_provider(struct device_node *np)
 			 * provider, set the 'has_provider' to false
 			 * so that the PM domain can be safely removed.
 			 */
-			list_for_each_entry(gpd, &gpd_list, gpd_list_node)
-				if (gpd->provider == &np->fwnode)
+			list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+				if (gpd->provider == &np->fwnode) {
 					gpd->has_provider = false;
 
+					if (!gpd->set_performance_state)
+						continue;
+
+					dev_pm_opp_of_remove_table(&gpd->dev);
+				}
+			}
+
 			list_del(&cp->link);
 			of_node_put(cp->node);
 			kfree(cp);
-- 
2.15.0.194.g9af6a3dea062

[PATCH 5/7] PM / Domain: Implement of_dev_pm_genpd_get_performance_state()

[Viresh Kumar]

This implements of_dev_pm_genpd_get_performance_state() which can be
used from the device drivers or the OPP core to find the performance
state encoded in the "required-opp" property of a node. Different
platforms may encode the performance state differently using the OPP
table (they may simply return value of opp-hz or opp-microvolt, or apply
some algorithm on top of those values) and so a new callback is
implemented to allow platform specific drivers to convert the power
domain OPP to a performance state.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   | 11 +++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1ad4ad0b0de0..ef43b75982fa 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2415,6 +2415,54 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
 
+/**
+ * of_dev_pm_genpd_get_performance_state- Gets performance state of device's
+ * power domain corresponding to a DT node's "required-opp" property.
+ *
+ * @dev: Device for which the performance-state needs to be found.
+ * @np: DT node where the "required-opp" property is present. This can be
+ *	the device node itself (if it doesn't have an OPP table) or a node
+ *	within the OPP table of a device (if device has an OPP table).
+ * @state: Pointer to return performance state.
+ *
+ * Returns performance state corresponding to the "required-opp" property of
+ * a DT node. This calls platform specific genpd->get_performance_state()
+ * callback to translate power domain OPP to performance state.
+ *
+ * Returns performance state on success and 0 on failure.
+ */
+unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
+						   struct device_node *np)
+{
+	struct generic_pm_domain *genpd;
+	struct dev_pm_opp *opp;
+	int state = 0;
+
+	genpd = dev_to_genpd(dev);
+	if (IS_ERR(genpd))
+		return 0;
+
+	if (unlikely(!genpd->set_performance_state))
+		return 0;
+
+	genpd_lock(genpd);
+
+	opp = of_dev_pm_opp_find_required_opp(&genpd->dev, np);
+	if (IS_ERR(opp)) {
+		state = PTR_ERR(opp);
+		goto unlock;
+	}
+
+	state = genpd->get_performance_state(genpd, opp);
+	dev_pm_opp_put(opp);
+
+unlock:
+	genpd_unlock(genpd);
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(of_dev_pm_genpd_get_performance_state);
+
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index aaacaa35005d..4edbdaa54cec 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -47,6 +47,7 @@ struct genpd_power_state {
 };
 
 struct genpd_lock_ops;
+struct dev_pm_opp;
 
 struct generic_pm_domain {
 	struct device dev;
@@ -68,6 +69,8 @@ struct generic_pm_domain {
 	unsigned int performance_state;	/* Aggregated max performance state */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
+	unsigned int (*get_performance_state)(struct generic_pm_domain *genpd,
+					      struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
 	struct gpd_dev_ops dev_ops;
@@ -244,6 +247,8 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
 extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
 extern int of_genpd_parse_idle_states(struct device_node *dn,
 			struct genpd_power_state **states, int *n);
+extern unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
+				struct device_node *np);
 
 int genpd_dev_pm_attach(struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
@@ -279,6 +284,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
 	return -ENODEV;
 }
 
+static inline unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
+					struct device_node *np)
+{
+	return -ENODEV;
+}
+
 static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return -ENODEV;
-- 
2.15.0.194.g9af6a3dea062

[PATCH 6/7] PM / OPP: Get performance state using genpd helper

[Viresh Kumar]

The genpd core provides an API now to retrieve the performance state
from DT, use that instead of the ->get_pstate() callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  3 ---
 drivers/opp/of.c   | 20 +++++++++++++++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bfcad8983fd1..6194219fb95f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1034,9 +1034,6 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 		return ret;
 	}
 
-	if (opp_table->get_pstate)
-		new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
-
 	list_add(&new_opp->node, head);
 	mutex_unlock(&opp_table->lock);
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 2d1bb348ae73..21265af55117 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -17,6 +17,7 @@
 #include <linux/errno.h>
 #include <linux/device.h>
 #include <linux/of_device.h>
+#include <linux/pm_domain.h>
 #include <linux/slab.h>
 #include <linux/export.h>
 
@@ -321,6 +322,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
 	if (!of_property_read_u32(np, "clock-latency-ns", &val))
 		new_opp->clock_latency_ns = val;
 
+	new_opp->pstate = of_dev_pm_genpd_get_performance_state(dev, np);
+
 	ret = opp_parse_supplies(new_opp, dev, opp_table);
 	if (ret)
 		goto free_opp;
@@ -371,7 +374,8 @@ 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;
+	int ret = 0, count = 0, pstate_count = 0;
+	struct dev_pm_opp *opp;
 
 	opp_table = _managed_opp(opp_np);
 	if (opp_table) {
@@ -405,6 +409,20 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 		goto put_opp_table;
 	}
 
+	list_for_each_entry(opp, &opp_table->opp_list, node)
+		pstate_count += !!opp->pstate;
+
+	/* Either all or none of the nodes shall have performance state set */
+	if (pstate_count && pstate_count != count) {
+		dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
+			count, pstate_count);
+		ret = -ENOENT;
+		goto put_opp_table;
+	}
+
+	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;
-- 
2.15.0.194.g9af6a3dea062

[PATCH 7/7] PM / OPP: Remove dev_pm_opp_{un}register_get_pstate_helper()

[Viresh Kumar]

These helpers aren't used anymore, remove them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 75 --------------------------------------------------
 drivers/opp/opp.h      |  2 --
 include/linux/pm_opp.h | 10 -------
 3 files changed, 87 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6194219fb95f..9e3c40437991 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1545,81 +1545,6 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
 
-/**
- * dev_pm_opp_register_get_pstate_helper() - Register get_pstate() helper.
- * @dev: Device for which the helper is getting registered.
- * @get_pstate: Helper.
- *
- * TODO: Remove this callback after the same information is available via Device
- * Tree.
- *
- * This allows a platform to initialize the performance states of individual
- * OPPs for its devices, until we get similar information directly from DT.
- *
- * This must be called before the OPPs are initialized for the device.
- */
-struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
-		int (*get_pstate)(struct device *dev, unsigned long rate))
-{
-	struct opp_table *opp_table;
-	int ret;
-
-	if (!get_pstate)
-		return ERR_PTR(-EINVAL);
-
-	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
-
-	/* This should be called before OPPs are initialized */
-	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
-		ret = -EBUSY;
-		goto err;
-	}
-
-	/* Already have genpd_performance_state set */
-	if (WARN_ON(opp_table->genpd_performance_state)) {
-		ret = -EBUSY;
-		goto err;
-	}
-
-	opp_table->genpd_performance_state = true;
-	opp_table->get_pstate = get_pstate;
-
-	return opp_table;
-
-err:
-	dev_pm_opp_put_opp_table(opp_table);
-
-	return ERR_PTR(ret);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_register_get_pstate_helper);
-
-/**
- * dev_pm_opp_unregister_get_pstate_helper() - Releases resources blocked for
- *					   get_pstate() helper
- * @opp_table: OPP table returned from dev_pm_opp_register_get_pstate_helper().
- *
- * Release resources blocked for platform specific get_pstate() helper.
- */
-void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table)
-{
-	if (!opp_table->genpd_performance_state) {
-		pr_err("%s: Doesn't have performance states set\n",
-		       __func__);
-		return;
-	}
-
-	/* Make sure there are no concurrent readers while updating opp_table */
-	WARN_ON(!list_empty(&opp_table->opp_list));
-
-	opp_table->genpd_performance_state = false;
-	opp_table->get_pstate = NULL;
-
-	dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_get_pstate_helper);
-
 /**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index b181032e6938..ae99295b5382 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -140,7 +140,6 @@ enum opp_table_access {
  * @genpd_performance_state: Device's power domain support performance state.
  * @set_opp: Platform specific set_opp callback
  * @set_opp_data: Data to be passed to set_opp callback
- * @get_pstate: Platform specific get_pstate callback
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -178,7 +177,6 @@ struct opp_table {
 
 	int (*set_opp)(struct dev_pm_set_opp_data *data);
 	struct dev_pm_set_opp_data *set_opp_data;
-	int (*get_pstate)(struct device *dev, unsigned long rate);
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 70686f434c13..528a7d9cf2ef 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -125,8 +125,6 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
 void dev_pm_opp_put_clkname(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
-struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev, int (*get_pstate)(struct device *dev, unsigned long rate));
-void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -247,14 +245,6 @@ static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device
 
 static inline void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) {}
 
-static inline struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
-		int (*get_pstate)(struct device *dev, unsigned long rate))
-{
-	return ERR_PTR(-ENOTSUPP);
-}
-
-static inline void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table) {}
-
 static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 {
 	return ERR_PTR(-ENOTSUPP);
-- 
2.15.0.194.g9af6a3dea062

Re: [PATCH 0/7] PM /Domain/OPP: Add support to get performance state from DT

[Viresh Kumar]

On 22-12-17, 12:56, Viresh Kumar wrote:
> Hi,
> 
> Now that the DT bindings [1] are already Reviewed/Acked by respective
> maintainers, here is the code to start using them.
> 
> The first two patches provide helpers in the OPP core, [3-5]/7 update
> the PM domain core to start supporting domain OPP tables, etc, 6/7
> updates the OPP core to use the new callback provided by the PM domains
> to get performance state and the last one removes the unused helpers
> now.
> 
> This is tested on Hikey620 and works just fine.

Ping !

-- 
viresh

Re: [PATCH 0/7] PM /Domain/OPP: Add support to get performance state from DT

[Rafael J. Wysocki]

On Thursday, January 18, 2018 7:34:04 AM CET Viresh Kumar wrote:
> On 22-12-17, 12:56, Viresh Kumar wrote:
> > Hi,
> > 
> > Now that the DT bindings [1] are already Reviewed/Acked by respective
> > maintainers, here is the code to start using them.
> > 
> > The first two patches provide helpers in the OPP core, [3-5]/7 update
> > the PM domain core to start supporting domain OPP tables, etc, 6/7
> > updates the OPP core to use the new callback provided by the PM domains
> > to get performance state and the last one removes the unused helpers
> > now.
> > 
> > This is tested on Hikey620 and works just fine.
> 
> Ping !

Well, whom are you pinging exactly and why?

Thanks,
Rafael

Re: [PATCH 0/7] PM /Domain/OPP: Add support to get performance state from DT

[Viresh Kumar]

On 18-01-18, 20:24, Rafael J. Wysocki wrote:
> On Thursday, January 18, 2018 7:34:04 AM CET Viresh Kumar wrote:
> > On 22-12-17, 12:56, Viresh Kumar wrote:
> > > Hi,
> > > 
> > > Now that the DT bindings [1] are already Reviewed/Acked by respective
> > > maintainers, here is the code to start using them.
> > > 
> > > The first two patches provide helpers in the OPP core, [3-5]/7 update
> > > the PM domain core to start supporting domain OPP tables, etc, 6/7
> > > updates the OPP core to use the new callback provided by the PM domains
> > > to get performance state and the last one removes the unused helpers
> > > now.
> > > 
> > > This is tested on Hikey620 and works just fine.
> > 
> > Ping !
> 
> Well, whom are you pinging exactly and why?

Ulf and Kevin as its been almost a month since this series is posted
and has received no comments at all.

-- 
viresh

Re: [PATCH 1/7] PM / OPP: Implement dev_pm_opp_of_add_table_indexed()

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The "operating-points-v2" property can contain a list of phandles now,
> specifically for the power domain providers that provide multiple
> domains.
>
> Add support to parse that.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/opp/of.c       | 50 +++++++++++++++++++++++++++++++++++++++++---------
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index cb716aa2f44b..22c9bd191f62 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -250,20 +250,17 @@ 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)
> +static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np,
> +                                                    int index)
>  {
> -       /*
> -        * There should be only ONE phandle present in "operating-points-v2"
> -        * property.
> -        */
> -
> -       return of_parse_phandle(np, "operating-points-v2", 0);
> +       /* "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);
> +       return _opp_of_get_opp_desc_node(dev->of_node, 0);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
>
> @@ -509,6 +506,41 @@ int dev_pm_opp_of_add_table(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
>
> +/**
> + * dev_pm_opp_of_add_table_indexed() - Initialize indexed opp table from device tree
> + * @dev:       device pointer used to lookup OPP table.
> + * @index:     Index number.
> + *
> + * Register the initial OPP table with the OPP library for given device only
> + * using the "operating-points-v2" property.
> + *
> + * 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
> + * -EINVAL     when invalid entries are found in opp-v2 table
> + */
> +int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
> +{
> +       struct device_node *opp_np;
> +       int ret;
> +
> +       opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
> +       if (!opp_np)
> +               return -ENODEV;
> +
> +       ret = _of_add_opp_table_v2(dev, opp_np);
> +       of_node_put(opp_np);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
> +
>  /* CPU device specific helpers */
>
>  /**
> @@ -613,7 +645,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>                 }
>
>                 /* Get OPP descriptor node */
> -               tmp_np = _opp_of_get_opp_desc_node(cpu_np);
> +               tmp_np = _opp_of_get_opp_desc_node(cpu_np, 0);
>                 of_node_put(cpu_np);
>                 if (!tmp_np) {
>                         pr_err("%pOF: Couldn't find opp node\n", cpu_np);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 6c2d2e88f066..f042fdeaaa3c 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -303,6 +303,7 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
>
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
>  int dev_pm_opp_of_add_table(struct device *dev);
> +int dev_pm_opp_of_add_table_indexed(struct device *dev, int index);
>  void dev_pm_opp_of_remove_table(struct device *dev);
>  int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
>  void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
> @@ -314,6 +315,11 @@ static inline int dev_pm_opp_of_add_table(struct device *dev)
>         return -ENOTSUPP;
>  }
>
> +static inline int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
> +{
> +       return -ENOTSUPP;
> +}
> +
>  static inline void dev_pm_opp_of_remove_table(struct device *dev)
>  {
>  }
> --
> 2.15.0.194.g9af6a3dea062
>

Re: [PATCH 2/7] PM / OPP: Implement of_dev_pm_opp_find_required_opp()

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> A device's DT node or its OPP nodes can contain a phandle to other
> device's OPP node, in the "required-opp" property.
>
> This patch implements a routine to find that required OPP from the node
> that contains the "required-opp" property.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

[...]

> +++ b/include/linux/pm_opp.h
> @@ -309,6 +309,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
>  void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
>  int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
>  struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
> +struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np);

I find the name of the exported function a bit "too" self-explainable,
opp...opp. :-)

However it's consistent with the existing APIs.

[...]

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The power-domain core would be using the OPP core going forward and the
> OPP core has the basic requirement of a device structure for its working.

According to the OPP core also seems to require the ->dev.of_node to
be set. Actually, it seems like that part belongs in patch4 instead,
while starting to make use of the opp OF APIs. Could you please move
it?
>
> Add a struct device to the genpd structure and also add a genpd bus type
> for the devices.

This seems reasonable, however I am wondering if we could simplify
this as bit. See more comments below.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0c80bea05bcb..013c1e206167 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>         }
>  }
>
> +static struct bus_type genpd_bus_type = {
> +       .name =         "genpd",
> +};

This seems silly. Can't we just avoid having a bus type altogether,
thus no need to call bus_register() as well?

> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                         return ret;
>         }
>
> +       genpd->dev.bus = &genpd_bus_type;
> +       device_initialize(&genpd->dev);
> +       dev_set_name(&genpd->dev, "%s", genpd->name);
> +
> +       ret = device_add(&genpd->dev);

What's the point of adding the device? Can we skip this step, as I
guess the opp core is only using the device as cookie rather actual
using it? No?

> +       if (ret) {
> +               dev_err(&genpd->dev, "failed to add device: %d\n", ret);
> +               put_device(&genpd->dev);
> +               kfree(genpd->free);
> +               return ret;
> +       }
> +
>         mutex_lock(&gpd_list_lock);
>         list_add(&genpd->gpd_list_node, &gpd_list);
>         mutex_unlock(&gpd_list_lock);
> @@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>
>         list_del(&genpd->gpd_list_node);
>         genpd_unlock(genpd);
> +       device_del(&genpd->dev);
>         cancel_work_sync(&genpd->power_off_work);
>         kfree(genpd->free);
>         pr_debug("%s: removed %s\n", __func__, genpd->name);
> @@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                 if (!ret) {
>                         genpd->provider = &np->fwnode;
>                         genpd->has_provider = true;
> +                       genpd->dev.of_node = np;
>                 }
>         }
>
> @@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>
>                 data->domains[i]->provider = &np->fwnode;
>                 data->domains[i]->has_provider = true;
> +               data->domains[i]->dev.of_node = np;
>         }
>
>         ret = genpd_add_provider(np, data->xlate, data);
> @@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void)
>  }
>  __exitcall(genpd_debug_exit);
>  #endif /* CONFIG_DEBUG_FS */
> +
> +static int __init pm_genpd_core_init(void)
> +{
> +       int ret;
> +
> +       ret = bus_register(&genpd_bus_type);
> +       if (ret)
> +               pr_err("bus_register failed (%d)\n", ret);
> +
> +       return ret;
> +}
> +pure_initcall(pm_genpd_core_init);
> +
> +static void __exit pm_genpd_core_exit(void)
> +{
> +       bus_unregister(&genpd_bus_type);
> +}
> +__exitcall(pm_genpd_core_exit);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 04dbef9847d3..aaacaa35005d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -49,6 +49,7 @@ struct genpd_power_state {
>  struct genpd_lock_ops;
>
>  struct generic_pm_domain {
> +       struct device dev;
>         struct dev_pm_domain domain;    /* PM domain operations */
>         struct list_head gpd_list_node; /* Node in the global PM domains list */
>         struct list_head master_links;  /* Links with PM domain as a master */
> --
> 2.15.0.194.g9af6a3dea062
>

Kind regards
Uffe

Re: [PATCH 4/7] PM / Domain: Add support to parse domain's OPP table

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Parse the OPP table for power domains if they have their
> set_performance_state() callback set.

Nitpick:
This is a bit too short, please elaborate a bit on the why and what
this change does.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 78 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 62 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 013c1e206167..1ad4ad0b0de0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_qos.h>
> @@ -1900,15 +1901,33 @@ int of_genpd_add_provider_simple(struct device_node *np,
>
>         mutex_lock(&gpd_list_lock);
>
> -       if (genpd_present(genpd)) {
> -               ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> -               if (!ret) {
> -                       genpd->provider = &np->fwnode;
> -                       genpd->has_provider = true;
> -                       genpd->dev.of_node = np;
> +       if (!genpd_present(genpd))
> +               goto unlock;
> +
> +       genpd->dev.of_node = np;
> +
> +       /* Parse genpd OPP table */
> +       if (genpd->set_performance_state) {
> +               ret = dev_pm_opp_of_add_table(&genpd->dev);
> +               if (ret) {
> +                       dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> +                               ret);
> +                       goto unlock;
>                 }
>         }
>
> +       ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> +       if (ret) {
> +               if (genpd->set_performance_state)
> +                       dev_pm_opp_of_remove_table(&genpd->dev);
> +
> +               goto unlock;
> +       }
> +
> +       genpd->provider = &np->fwnode;
> +       genpd->has_provider = true;
> +
> +unlock:
>         mutex_unlock(&gpd_list_lock);
>
>         return ret;
> @@ -1923,6 +1942,7 @@ EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
>  int of_genpd_add_provider_onecell(struct device_node *np,
>                                   struct genpd_onecell_data *data)
>  {
> +       struct generic_pm_domain *genpd;
>         unsigned int i;
>         int ret = -EINVAL;
>
> @@ -1935,14 +1955,27 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 data->xlate = genpd_xlate_onecell;
>
>         for (i = 0; i < data->num_domains; i++) {
> -               if (!data->domains[i])
> +               genpd = data->domains[i];
> +
> +               if (!genpd)
>                         continue;
> -               if (!genpd_present(data->domains[i]))
> +               if (!genpd_present(genpd))
>                         goto error;
>
> -               data->domains[i]->provider = &np->fwnode;
> -               data->domains[i]->has_provider = true;
> -               data->domains[i]->dev.of_node = np;
> +               genpd->dev.of_node = np;
> +
> +               /* Parse genpd OPP table */
> +               if (genpd->set_performance_state) {
> +                       ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
> +                       if (ret) {
> +                               dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> +                                       i, ret);
> +                               goto error;
> +                       }
> +               }
> +
> +               genpd->provider = &np->fwnode;
> +               genpd->has_provider = true;
>         }
>
>         ret = genpd_add_provider(np, data->xlate, data);
> @@ -1955,10 +1988,16 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>
>  error:
>         while (i--) {
> -               if (!data->domains[i])
> +               genpd = data->domains[i];
> +
> +               if (!genpd)
>                         continue;
> -               data->domains[i]->provider = NULL;
> -               data->domains[i]->has_provider = false;
> +
> +               genpd->provider = NULL;
> +               genpd->has_provider = false;
> +
> +               if (genpd->set_performance_state)
> +                       dev_pm_opp_of_remove_table(&genpd->dev);
>         }
>
>         mutex_unlock(&gpd_list_lock);
> @@ -1985,10 +2024,17 @@ void of_genpd_del_provider(struct device_node *np)
>                          * provider, set the 'has_provider' to false
>                          * so that the PM domain can be safely removed.
>                          */
> -                       list_for_each_entry(gpd, &gpd_list, gpd_list_node)
> -                               if (gpd->provider == &np->fwnode)
> +                       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +                               if (gpd->provider == &np->fwnode) {
>                                         gpd->has_provider = false;
>
> +                                       if (!gpd->set_performance_state)
> +                                               continue;

Nitpick, perhaps change this to:

if (gpd->set_performance_state)
       dev_pm_opp_of_remove_table(&gpd->dev);

> +
> +                                       dev_pm_opp_of_remove_table(&gpd->dev);
> +                               }
> +                       }
> +
>                         list_del(&cp->link);
>                         of_node_put(cp->node);
>                         kfree(cp);
> --
> 2.15.0.194.g9af6a3dea062
>

When fixed the nitpicks, feel free to add:

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH 5/7] PM / Domain: Implement of_dev_pm_genpd_get_performance_state()

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> This implements of_dev_pm_genpd_get_performance_state() which can be
> used from the device drivers or the OPP core to find the performance
> state encoded in the "required-opp" property of a node. Different

Please add a two newlines after "node".

> platforms may encode the performance state differently using the OPP
> table (they may simply return value of opp-hz or opp-microvolt, or apply
> some algorithm on top of those values) and so a new callback is
> implemented to allow platform specific drivers to convert the power
> domain OPP to a performance state.

Could you perhaps clarify at what typical point(s) the drivers or the
OPP core should call this new API? That is useful information in the
changelog.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 11 +++++++++++
>  2 files changed, 59 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 1ad4ad0b0de0..ef43b75982fa 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2415,6 +2415,54 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +/**
> + * of_dev_pm_genpd_get_performance_state- Gets performance state of device's
> + * power domain corresponding to a DT node's "required-opp" property.
> + *
> + * @dev: Device for which the performance-state needs to be found.
> + * @np: DT node where the "required-opp" property is present. This can be
> + *     the device node itself (if it doesn't have an OPP table) or a node
> + *     within the OPP table of a device (if device has an OPP table).
> + * @state: Pointer to return performance state.
> + *
> + * Returns performance state corresponding to the "required-opp" property of
> + * a DT node. This calls platform specific genpd->get_performance_state()
> + * callback to translate power domain OPP to performance state.
> + *
> + * Returns performance state on success and 0 on failure.
> + */
> +unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> +                                                  struct device_node *np)

First: To clarify the API, perhaps change to "...struct device_node *opp_node)"

Second, the function name is confusing. To me, it sounds like an API
that will get the dynamically requested performance state for the
device and that isn't the case.

So, I would rather name it something along the lines of,
"of_genpd_opp_to_performance_state()". Can you please consider
renaming it?

> +{
> +       struct generic_pm_domain *genpd;
> +       struct dev_pm_opp *opp;
> +       int state = 0;
> +
> +       genpd = dev_to_genpd(dev);
> +       if (IS_ERR(genpd))
> +               return 0;
> +
> +       if (unlikely(!genpd->set_performance_state))
> +               return 0;
> +
> +       genpd_lock(genpd);
> +
> +       opp = of_dev_pm_opp_find_required_opp(&genpd->dev, np);
> +       if (IS_ERR(opp)) {
> +               state = PTR_ERR(opp);
> +               goto unlock;
> +       }
> +
> +       state = genpd->get_performance_state(genpd, opp);
> +       dev_pm_opp_put(opp);
> +
> +unlock:
> +       genpd_unlock(genpd);
> +
> +       return state;
> +}
> +EXPORT_SYMBOL_GPL(of_dev_pm_genpd_get_performance_state);
> +
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index aaacaa35005d..4edbdaa54cec 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -47,6 +47,7 @@ struct genpd_power_state {
>  };
>
>  struct genpd_lock_ops;
> +struct dev_pm_opp;
>
>  struct generic_pm_domain {
>         struct device dev;
> @@ -68,6 +69,8 @@ struct generic_pm_domain {
>         unsigned int performance_state; /* Aggregated max performance state */
>         int (*power_off)(struct generic_pm_domain *domain);
>         int (*power_on)(struct generic_pm_domain *domain);
> +       unsigned int (*get_performance_state)(struct generic_pm_domain *genpd,
> +                                             struct dev_pm_opp *opp);

According to the comments above, please rename the callback to:
"opp_to_performance_state", or whatever better name you can come up with. :-)

>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>                                      unsigned int state);
>         struct gpd_dev_ops dev_ops;
> @@ -244,6 +247,8 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>  extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>  extern int of_genpd_parse_idle_states(struct device_node *dn,
>                         struct genpd_power_state **states, int *n);
> +extern unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> +                               struct device_node *np);
>
>  int genpd_dev_pm_attach(struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -279,6 +284,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
>         return -ENODEV;
>  }
>
> +static inline unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> +                                       struct device_node *np)
> +{
> +       return -ENODEV;
> +}
> +
>  static inline int genpd_dev_pm_attach(struct device *dev)
>  {
>         return -ENODEV;
> --
> 2.15.0.194.g9af6a3dea062
>

Mostly minor things, so overall this looks good to me!

Kind regards
Uffe

Re: [PATCH 6/7] PM / OPP: Get performance state using genpd helper

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The genpd core provides an API now to retrieve the performance state
> from DT, use that instead of the ->get_pstate() callback.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/opp/core.c |  3 ---
>  drivers/opp/of.c   | 20 +++++++++++++++++++-
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index bfcad8983fd1..6194219fb95f 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1034,9 +1034,6 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>                 return ret;
>         }
>
> -       if (opp_table->get_pstate)
> -               new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
> -
>         list_add(&new_opp->node, head);
>         mutex_unlock(&opp_table->lock);
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 2d1bb348ae73..21265af55117 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -17,6 +17,7 @@
>  #include <linux/errno.h>
>  #include <linux/device.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
>
> @@ -321,6 +322,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
>         if (!of_property_read_u32(np, "clock-latency-ns", &val))
>                 new_opp->clock_latency_ns = val;
>
> +       new_opp->pstate = of_dev_pm_genpd_get_performance_state(dev, np);
> +
>         ret = opp_parse_supplies(new_opp, dev, opp_table);
>         if (ret)
>                 goto free_opp;
> @@ -371,7 +374,8 @@ 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;
> +       int ret = 0, count = 0, pstate_count = 0;
> +       struct dev_pm_opp *opp;
>
>         opp_table = _managed_opp(opp_np);
>         if (opp_table) {
> @@ -405,6 +409,20 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>                 goto put_opp_table;
>         }
>
> +       list_for_each_entry(opp, &opp_table->opp_list, node)
> +               pstate_count += !!opp->pstate;
> +
> +       /* Either all or none of the nodes shall have performance state set */
> +       if (pstate_count && pstate_count != count) {
> +               dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
> +                       count, pstate_count);
> +               ret = -ENOENT;
> +               goto put_opp_table;
> +       }
> +
> +       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;
> --
> 2.15.0.194.g9af6a3dea062
>

Re: [PATCH 7/7] PM / OPP: Remove dev_pm_opp_{un}register_get_pstate_helper()

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> These helpers aren't used anymore, remove them.

I guess they weren't used even before? However, as we now have the new
OF based APIs, we can use them instead.

Anyway, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 75 --------------------------------------------------
>  drivers/opp/opp.h      |  2 --
>  include/linux/pm_opp.h | 10 -------
>  3 files changed, 87 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6194219fb95f..9e3c40437991 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1545,81 +1545,6 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
>
> -/**
> - * dev_pm_opp_register_get_pstate_helper() - Register get_pstate() helper.
> - * @dev: Device for which the helper is getting registered.
> - * @get_pstate: Helper.
> - *
> - * TODO: Remove this callback after the same information is available via Device
> - * Tree.
> - *
> - * This allows a platform to initialize the performance states of individual
> - * OPPs for its devices, until we get similar information directly from DT.
> - *
> - * This must be called before the OPPs are initialized for the device.
> - */
> -struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
> -               int (*get_pstate)(struct device *dev, unsigned long rate))
> -{
> -       struct opp_table *opp_table;
> -       int ret;
> -
> -       if (!get_pstate)
> -               return ERR_PTR(-EINVAL);
> -
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> -
> -       /* This should be called before OPPs are initialized */
> -       if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> -               ret = -EBUSY;
> -               goto err;
> -       }
> -
> -       /* Already have genpd_performance_state set */
> -       if (WARN_ON(opp_table->genpd_performance_state)) {
> -               ret = -EBUSY;
> -               goto err;
> -       }
> -
> -       opp_table->genpd_performance_state = true;
> -       opp_table->get_pstate = get_pstate;
> -
> -       return opp_table;
> -
> -err:
> -       dev_pm_opp_put_opp_table(opp_table);
> -
> -       return ERR_PTR(ret);
> -}
> -EXPORT_SYMBOL_GPL(dev_pm_opp_register_get_pstate_helper);
> -
> -/**
> - * dev_pm_opp_unregister_get_pstate_helper() - Releases resources blocked for
> - *                                        get_pstate() helper
> - * @opp_table: OPP table returned from dev_pm_opp_register_get_pstate_helper().
> - *
> - * Release resources blocked for platform specific get_pstate() helper.
> - */
> -void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table)
> -{
> -       if (!opp_table->genpd_performance_state) {
> -               pr_err("%s: Doesn't have performance states set\n",
> -                      __func__);
> -               return;
> -       }
> -
> -       /* Make sure there are no concurrent readers while updating opp_table */
> -       WARN_ON(!list_empty(&opp_table->opp_list));
> -
> -       opp_table->genpd_performance_state = false;
> -       opp_table->get_pstate = NULL;
> -
> -       dev_pm_opp_put_opp_table(opp_table);
> -}
> -EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_get_pstate_helper);
> -
>  /**
>   * dev_pm_opp_add()  - Add an OPP table from a table definitions
>   * @dev:       device for which we do this operation
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index b181032e6938..ae99295b5382 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -140,7 +140,6 @@ enum opp_table_access {
>   * @genpd_performance_state: Device's power domain support performance state.
>   * @set_opp: Platform specific set_opp callback
>   * @set_opp_data: Data to be passed to set_opp callback
> - * @get_pstate: Platform specific get_pstate callback
>   * @dentry:    debugfs dentry pointer of the real device directory (not links).
>   * @dentry_name: Name of the real dentry.
>   *
> @@ -178,7 +177,6 @@ struct opp_table {
>
>         int (*set_opp)(struct dev_pm_set_opp_data *data);
>         struct dev_pm_set_opp_data *set_opp_data;
> -       int (*get_pstate)(struct device *dev, unsigned long rate);
>
>  #ifdef CONFIG_DEBUG_FS
>         struct dentry *dentry;
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 70686f434c13..528a7d9cf2ef 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -125,8 +125,6 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
>  void dev_pm_opp_put_clkname(struct opp_table *opp_table);
>  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
>  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> -struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev, int (*get_pstate)(struct device *dev, unsigned long rate));
> -void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table);
>  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
>  int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
>  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> @@ -247,14 +245,6 @@ static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device
>
>  static inline void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) {}
>
> -static inline struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
> -               int (*get_pstate)(struct device *dev, unsigned long rate))
> -{
> -       return ERR_PTR(-ENOTSUPP);
> -}
> -
> -static inline void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table) {}
> -
>  static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
>  {
>         return ERR_PTR(-ENOTSUPP);
> --
> 2.15.0.194.g9af6a3dea062
>

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 10:30, Ulf Hansson wrote:
> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > The power-domain core would be using the OPP core going forward and the
> > OPP core has the basic requirement of a device structure for its working.
> 
> According to the OPP core also seems to require the ->dev.of_node to
> be set. Actually, it seems like that part belongs in patch4 instead,
> while starting to make use of the opp OF APIs. Could you please move
> it?

You meaning setting of the of_node to the next patch? I can do that if that;s
what you want.

> > +static struct bus_type genpd_bus_type = {
> > +       .name =         "genpd",
> > +};
> 
> This seems silly. Can't we just avoid having a bus type altogether,
> thus no need to call bus_register() as well?

I thought we need a bus where we want to add the device, haven't tried with that
pointer being empty. Will try that and let you know.

> > +
> >  /**
> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
> >   * @genpd: PM domain object to initialize.
> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> >                         return ret;
> >         }
> >
> > +       genpd->dev.bus = &genpd_bus_type;
> > +       device_initialize(&genpd->dev);
> > +       dev_set_name(&genpd->dev, "%s", genpd->name);
> > +
> > +       ret = device_add(&genpd->dev);
> 
> What's the point of adding the device? Can we skip this step, as I
> guess the opp core is only using the device as cookie rather actual
> using it? No?

We also use it for the OPP debugfs stuff, so that would be required I believe.
We also use it for playing with clk/regulator stuff as well, though we may not
use it in genpd case for now.

-- 
viresh

Re: [PATCH 4/7] PM / Domain: Add support to parse domain's OPP table

[Viresh Kumar]

On 22-03-18, 10:31, Ulf Hansson wrote:
> Nitpick, perhaps change this to:
> 
> if (gpd->set_performance_state)
>        dev_pm_opp_of_remove_table(&gpd->dev);

I probably did that to save the 80 column thing. So crossing 80 columns in this
case is fine ?

-- 
viresh

Re: [PATCH 4/7] PM / Domain: Add support to parse domain's OPP table

[Ulf Hansson]

On 22 March 2018 at 11:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-03-18, 10:31, Ulf Hansson wrote:
>> Nitpick, perhaps change this to:
>>
>> if (gpd->set_performance_state)
>>        dev_pm_opp_of_remove_table(&gpd->dev);
>
> I probably did that to save the 80 column thing. So crossing 80 columns in this
> case is fine ?

If that is needed, then okay!

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 March 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-03-18, 10:30, Ulf Hansson wrote:
>> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > The power-domain core would be using the OPP core going forward and the
>> > OPP core has the basic requirement of a device structure for its working.
>>
>> According to the OPP core also seems to require the ->dev.of_node to
>> be set. Actually, it seems like that part belongs in patch4 instead,
>> while starting to make use of the opp OF APIs. Could you please move
>> it?
>
> You meaning setting of the of_node to the next patch? I can do that if that;s
> what you want.

Yes, please.

>
>> > +static struct bus_type genpd_bus_type = {
>> > +       .name =         "genpd",
>> > +};
>>
>> This seems silly. Can't we just avoid having a bus type altogether,
>> thus no need to call bus_register() as well?
>
> I thought we need a bus where we want to add the device, haven't tried with that
> pointer being empty. Will try that and let you know.

Great!

>
>> > +
>> >  /**
>> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
>> >   * @genpd: PM domain object to initialize.
>> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>> >                         return ret;
>> >         }
>> >
>> > +       genpd->dev.bus = &genpd_bus_type;
>> > +       device_initialize(&genpd->dev);
>> > +       dev_set_name(&genpd->dev, "%s", genpd->name);
>> > +
>> > +       ret = device_add(&genpd->dev);
>>
>> What's the point of adding the device? Can we skip this step, as I
>> guess the opp core is only using the device as cookie rather actual
>> using it? No?
>
> We also use it for the OPP debugfs stuff, so that would be required I believe.

Right, however, isn't that only using the dev_name(dev), which you
don't need to add the device to make use of.

Or maybe I missing something around this...

> We also use it for playing with clk/regulator stuff as well, though we may not
> use it in genpd case for now.
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 11:18, Ulf Hansson wrote:
> On 22 March 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 22-03-18, 10:30, Ulf Hansson wrote:
> >> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >> > +       ret = device_add(&genpd->dev);
> >>
> >> What's the point of adding the device? Can we skip this step, as I
> >> guess the opp core is only using the device as cookie rather actual
> >> using it? No?
> >
> > We also use it for the OPP debugfs stuff, so that would be required I believe.
> 
> Right, however, isn't that only using the dev_name(dev), which you
> don't need to add the device to make use of.
> 
> Or maybe I missing something around this...

So I tested this bit. The code works fine even if the device isn't added
(registered), but this looks a bit sloppy to attempt.

Please let me know what would you prefer in this case, add the device or not.

-- 
viresh

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 9 April 2018 at 09:53, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-03-18, 11:18, Ulf Hansson wrote:
>> On 22 March 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 22-03-18, 10:30, Ulf Hansson wrote:
>> >> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> >> > +       ret = device_add(&genpd->dev);
>> >>
>> >> What's the point of adding the device? Can we skip this step, as I
>> >> guess the opp core is only using the device as cookie rather actual
>> >> using it? No?
>> >
>> > We also use it for the OPP debugfs stuff, so that would be required I believe.
>>
>> Right, however, isn't that only using the dev_name(dev), which you
>> don't need to add the device to make use of.
>>
>> Or maybe I missing something around this...
>
> So I tested this bit. The code works fine even if the device isn't added
> (registered), but this looks a bit sloppy to attempt.
>
> Please let me know what would you prefer in this case, add the device or not.

Well, I don't know if there is policy of how to do this in general. Or
perhaps this can be considered as a special case as the device is only
going to be used as cookie (at least so far).

I suggest we keep it as simple as possible and *not* add (register) the device.

Kind regards
Uffe