[PATCH V2 0/6] PM / Domains: Implement domain performance states

[PATCH V2 0/6] PM / Domains: Implement domain performance states

[Viresh Kumar]

Hi,

An earlier series[1] tried to implement bindings for PM domain
performance states. Rob Herring suggested that we can actually merge the
supporting code first instead of bindings, as that will make things
easier to understand for all. The bindings can be decided and merged
later.

The bindings [1] aren't discarded yet and this series is based on a
version of those only. The bindings are only used by the last patch,
which should not be applied and is only sent for completeness.

IOW, this series doesn't have any dependencies and can be merged
straight away without waiting for the DT bindings.

A brief summary of the problem this series is trying to solve:

Some platforms have the capability to configure the performance state of
their Power Domains. The performance levels are represented by positive
integer values, a lower value represents lower performance state.

We decided earlier that we should extend Power Domain framework to
support active state power management as well.  The power-domains until
now were only concentrating on the idle state management of the device
and this needs to change in order to reuse the infrastructure of power
domains for active state management.

The first 5 patches update the PM domain and QoS frameworks to support
that and the last one presents the front end interface to it.

The V1 series was tested by hacking the OPP core a bit but this one is
also tested by Rajendra Nayak (Qcom) on *real* Qualcomm hardware for
which this work is done. And most of his feedback is incorporated here.

V1->V2:
- Based over latest pm/linux-next
- It is mostly a resend of what is sent earlier as this series hasn't
  got any reviews so far and Rafael suggested that its better I resend
  it.
- Only the 4/6 patch got an update, which was shared earlier as reply to
  V1 as well. It has got several fixes for taking care of power domain
  hierarchy, etc.

--
viresh

[1] https://marc.info/?l=linux-kernel&m=148154020127722&w=2

Viresh Kumar (6):
  PM / QOS: Add default case to the switch
  PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  PM / QOS: Add 'performance' request
  PM / domain: Register for PM QOS performance notifier
  PM / domain: Save/restore performance state at runtime suspend/resume
  PM / OPP: Add support to parse domain-performance-state

 Documentation/power/pm_qos_interface.txt |  11 ++-
 drivers/base/power/domain.c              | 125 +++++++++++++++++++++++++++++--
 drivers/base/power/opp/core.c            |  75 +++++++++++++++++++
 drivers/base/power/opp/debugfs.c         |   4 +
 drivers/base/power/opp/of.c              |  44 +++++++++++
 drivers/base/power/opp/opp.h             |  12 +++
 drivers/base/power/qos.c                 |  74 ++++++++++++++++--
 include/linux/pm_domain.h                |   6 ++
 include/linux/pm_qos.h                   |  16 +++-
 9 files changed, 345 insertions(+), 22 deletions(-)

-- 
2.7.1.410.g6faf27b

[PATCH V2 1/6] PM / QOS: Add default case to the switch

[Viresh Kumar]

The switch block handles all the QOS request types present today, but
starts giving compilation warnings as soon as a new type is added and
not handled in this.

To prevent against that, add the default case as well and do a WARN from
it.

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

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 58fcc758334e..01f615b18055 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -621,6 +621,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
 		req = dev->power.qos->flags_req;
 		dev->power.qos->flags_req = NULL;
 		break;
+	default:
+		WARN_ON(1);
+		return;
 	}
 	__dev_pm_qos_remove_request(req);
 	kfree(req);
-- 
2.7.1.410.g6faf27b

[PATCH V2 2/6] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()

[Viresh Kumar]

In order to use the same set of routines to register notifiers for
different request types, update the existing
dev_pm_qos_{add|remove}_notifier() routines with an additional
parameter: request-type.

For now, it only supports resume-latency request type.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/power/pm_qos_interface.txt | 11 +++++++----
 drivers/base/power/domain.c              |  8 +++++---
 drivers/base/power/qos.c                 | 14 ++++++++++++--
 include/linux/pm_qos.h                   | 12 ++++++++----
 4 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index 129f7c0e1483..c7989d140428 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -166,12 +166,15 @@ under the device's power directory.
 The per-device PM QoS framework has 2 different and distinct notification trees:
 a per-device notification tree and a global notification tree.
 
-int dev_pm_qos_add_notifier(device, notifier):
-Adds a notification callback function for the device.
+int dev_pm_qos_add_notifier(device, notifier, type):
+Adds a notification callback function for the device for a particular request
+type.
+
 The callback is called when the aggregated value of the device constraints list
-is changed (for resume latency device PM QoS only).
+is changed. Currently it only supports the notifier to be registered for resume
+latency device PM QoS.
 
-int dev_pm_qos_remove_notifier(device, notifier):
+int dev_pm_qos_remove_notifier(device, notifier, type):
 Removes the notification callback function for the device.
 
 int dev_pm_qos_add_global_notifier(notifier):
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3a75fb1b4126..a73d79670a64 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1213,7 +1213,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
 	else
-		dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+		dev_pm_qos_add_notifier(dev, &gpd_data->nb,
+					DEV_PM_QOS_RESUME_LATENCY);
 
 	return ret;
 }
@@ -1248,7 +1249,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
 	pdd = dev->power.subsys_data->domain_data;
 	gpd_data = to_gpd_data(pdd);
-	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
+	dev_pm_qos_remove_notifier(dev, &gpd_data->nb,
+				   DEV_PM_QOS_RESUME_LATENCY);
 
 	genpd_lock(genpd);
 
@@ -1273,7 +1275,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
  out:
 	genpd_unlock(genpd);
-	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+	dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY);
 
 	return ret;
 }
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 01f615b18055..9adc208cf1fc 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -481,6 +481,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
  *
  * @dev: target device for the constraint
  * @notifier: notifier block managed by caller.
+ * @type: request type.
  *
  * Will register the notifier into a notification chain that gets called
  * upon changes to the target value for the device.
@@ -488,10 +489,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
  * If the device's constraints object doesn't exist when this routine is called,
  * it will be created (or error code will be returned if that fails).
  */
-int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
+int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
+			    enum dev_pm_qos_req_type type)
 {
 	int ret = 0;
 
+	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+		return -EINVAL;
+
 	mutex_lock(&dev_pm_qos_mtx);
 
 	if (IS_ERR(dev->power.qos))
@@ -514,15 +519,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
  *
  * @dev: target device for the constraint
  * @notifier: notifier block to be removed.
+ * @type: request type.
  *
  * Will remove the notifier from the notification chain that gets called
  * upon changes to the target value.
  */
 int dev_pm_qos_remove_notifier(struct device *dev,
-			       struct notifier_block *notifier)
+			       struct notifier_block *notifier,
+			       enum dev_pm_qos_req_type type)
 {
 	int retval = 0;
 
+	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+		return -EINVAL;
+
 	mutex_lock(&dev_pm_qos_mtx);
 
 	/* Silently return if the constraints object is not present. */
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index d4d34791e463..08cfaeb6c178 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -143,9 +143,11 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
 int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
 int dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
 int dev_pm_qos_add_notifier(struct device *dev,
-			    struct notifier_block *notifier);
+			    struct notifier_block *notifier,
+			    enum dev_pm_qos_req_type type);
 int dev_pm_qos_remove_notifier(struct device *dev,
-			       struct notifier_block *notifier);
+			       struct notifier_block *notifier,
+			       enum dev_pm_qos_req_type type);
 int dev_pm_qos_add_global_notifier(struct notifier_block *notifier);
 int dev_pm_qos_remove_global_notifier(struct notifier_block *notifier);
 void dev_pm_qos_constraints_init(struct device *dev);
@@ -194,10 +196,12 @@ static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
 static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 			{ return 0; }
 static inline int dev_pm_qos_add_notifier(struct device *dev,
-					  struct notifier_block *notifier)
+					  struct notifier_block *notifier,
+					  enum dev_pm_qos_req_type type)
 			{ return 0; }
 static inline int dev_pm_qos_remove_notifier(struct device *dev,
-					     struct notifier_block *notifier)
+					     struct notifier_block *notifier,
+					     enum dev_pm_qos_req_type type)
 			{ return 0; }
 static inline int dev_pm_qos_add_global_notifier(
 					struct notifier_block *notifier)
-- 
2.7.1.410.g6faf27b

[PATCH V2 3/6] PM / QOS: Add 'performance' request

[Viresh Kumar]

Add a new QOS request type: DEV_PM_QOS_PERFORMANCE.

This is required to support runtime performance constraints for the
devices. Also allow notifiers to be registered against it, which can be
used by frameworks like genpd.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/power/pm_qos_interface.txt |  2 +-
 drivers/base/power/qos.c                 | 69 +++++++++++++++++++++++++++-----
 include/linux/pm_qos.h                   |  4 ++
 3 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index c7989d140428..a0a45ecbc1a8 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -172,7 +172,7 @@ type.
 
 The callback is called when the aggregated value of the device constraints list
 is changed. Currently it only supports the notifier to be registered for resume
-latency device PM QoS.
+latency and performance device PM QoS.
 
 int dev_pm_qos_remove_notifier(device, notifier, type):
 Removes the notification callback function for the device.
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 9adc208cf1fc..cf070468a7ca 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -163,6 +163,10 @@ static int apply_constraint(struct dev_pm_qos_request *req,
 			req->dev->power.set_latency_tolerance(req->dev, value);
 		}
 		break;
+	case DEV_PM_QOS_PERFORMANCE:
+		ret = pm_qos_update_target(&qos->performance, &req->data.pnode,
+					   action, value);
+		break;
 	case DEV_PM_QOS_FLAGS:
 		ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
 					  action, value);
@@ -191,12 +195,13 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	if (!qos)
 		return -ENOMEM;
 
-	n = kzalloc(sizeof(*n), GFP_KERNEL);
+	n = kzalloc(2 * sizeof(*n), GFP_KERNEL);
 	if (!n) {
 		kfree(qos);
 		return -ENOMEM;
 	}
 	BLOCKING_INIT_NOTIFIER_HEAD(n);
+	BLOCKING_INIT_NOTIFIER_HEAD(n + 1);
 
 	c = &qos->resume_latency;
 	plist_head_init(&c->list);
@@ -213,6 +218,14 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
 
+	c = &qos->performance;
+	plist_head_init(&c->list);
+	c->target_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
+	c->default_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
+	c->no_constraint_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
+	c->type = PM_QOS_MAX;
+	c->notifiers = n + 1;
+
 	INIT_LIST_HEAD(&qos->flags.list);
 
 	spin_lock_irq(&dev->power.lock);
@@ -271,6 +284,11 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
 		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
 		memset(req, 0, sizeof(*req));
 	}
+	c = &qos->performance;
+	plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+		memset(req, 0, sizeof(*req));
+	}
 	f = &qos->flags;
 	list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) {
 		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
@@ -382,6 +400,7 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
 	switch(req->type) {
 	case DEV_PM_QOS_RESUME_LATENCY:
 	case DEV_PM_QOS_LATENCY_TOLERANCE:
+	case DEV_PM_QOS_PERFORMANCE:
 		curr_value = req->data.pnode.prio;
 		break;
 	case DEV_PM_QOS_FLAGS:
@@ -492,11 +511,9 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
 int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
 			    enum dev_pm_qos_req_type type)
 {
+	struct dev_pm_qos *qos;
 	int ret = 0;
 
-	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
-		return -EINVAL;
-
 	mutex_lock(&dev_pm_qos_mtx);
 
 	if (IS_ERR(dev->power.qos))
@@ -504,10 +521,26 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
 	else if (!dev->power.qos)
 		ret = dev_pm_qos_constraints_allocate(dev);
 
-	if (!ret)
-		ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers,
+	if (ret)
+		goto unlock;
+
+	qos = dev->power.qos;
+
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
+		ret = blocking_notifier_chain_register(qos->resume_latency.notifiers,
+						       notifier);
+		break;
+	case DEV_PM_QOS_PERFORMANCE:
+		ret = blocking_notifier_chain_register(qos->performance.notifiers,
 						       notifier);
+		break;
+	default:
+		WARN_ON(1);
+		ret = -EINVAL;
+	}
 
+unlock:
 	mutex_unlock(&dev_pm_qos_mtx);
 	return ret;
 }
@@ -528,18 +561,32 @@ int dev_pm_qos_remove_notifier(struct device *dev,
 			       struct notifier_block *notifier,
 			       enum dev_pm_qos_req_type type)
 {
+	struct dev_pm_qos *qos;
 	int retval = 0;
 
-	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
-		return -EINVAL;
-
 	mutex_lock(&dev_pm_qos_mtx);
 
+	qos = dev->power.qos;
+
 	/* Silently return if the constraints object is not present. */
-	if (!IS_ERR_OR_NULL(dev->power.qos))
-		retval = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
+	if (IS_ERR_OR_NULL(qos))
+		goto unlock;
+
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
+		retval = blocking_notifier_chain_unregister(qos->resume_latency.notifiers,
+							    notifier);
+		break;
+	case DEV_PM_QOS_PERFORMANCE:
+		retval = blocking_notifier_chain_unregister(qos->performance.notifiers,
 							    notifier);
+		break;
+	default:
+		WARN_ON(1);
+		retval = -EINVAL;
+	}
 
+unlock:
 	mutex_unlock(&dev_pm_qos_mtx);
 	return retval;
 }
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 08cfaeb6c178..0e8386adb21c 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -36,6 +36,7 @@ enum pm_qos_flags_status {
 #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE	0
 #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE	0
 #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT	(-1)
+#define PM_QOS_PERFORMANCE_DEFAULT_VALUE	0
 #define PM_QOS_LATENCY_ANY			((s32)(~(__u32)0 >> 1))
 
 #define PM_QOS_FLAG_NO_POWER_OFF	(1 << 0)
@@ -55,6 +56,7 @@ struct pm_qos_flags_request {
 enum dev_pm_qos_req_type {
 	DEV_PM_QOS_RESUME_LATENCY = 1,
 	DEV_PM_QOS_LATENCY_TOLERANCE,
+	DEV_PM_QOS_PERFORMANCE,
 	DEV_PM_QOS_FLAGS,
 };
 
@@ -96,9 +98,11 @@ struct pm_qos_flags {
 struct dev_pm_qos {
 	struct pm_qos_constraints resume_latency;
 	struct pm_qos_constraints latency_tolerance;
+	struct pm_qos_constraints performance;
 	struct pm_qos_flags flags;
 	struct dev_pm_qos_request *resume_latency_req;
 	struct dev_pm_qos_request *latency_tolerance_req;
+	struct dev_pm_qos_request *performance_req;
 	struct dev_pm_qos_request *flags_req;
 };
 
-- 
2.7.1.410.g6faf27b

[PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

[Viresh Kumar]

Some platforms have the capability to configure the performance state of
their Power Domains. The performance levels are represented by positive
integer values, a lower value represents lower performance state.

This patch registers the power domain framework for PM QOS performance
notifier in order to manage performance state of power domains.

This also allows the power domain drivers to implement a
->set_performance_state() callback, which will be called by the power
domain core from the notifier routine.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a73d79670a64..1158a07f92de 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -367,6 +367,88 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static void update_domain_performance_state(struct generic_pm_domain *genpd,
+					    int depth)
+{
+	struct generic_pm_domain_data *pd_data;
+	struct generic_pm_domain *subdomain;
+	struct pm_domain_data *pdd;
+	unsigned int state = 0;
+	struct gpd_link *link;
+
+	/* Traverse all devices within the domain */
+	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+		pd_data = to_gpd_data(pdd);
+
+		if (pd_data->performance_state > state)
+			state = pd_data->performance_state;
+	}
+
+	/* Traverse all subdomains within the domain */
+	list_for_each_entry(link, &genpd->master_links, master_node) {
+		subdomain = link->slave;
+
+		if (subdomain->performance_state > state)
+			state = subdomain->performance_state;
+	}
+
+	if (genpd->performance_state == state)
+		return;
+
+	genpd->performance_state = state;
+
+	if (genpd->set_performance_state) {
+		genpd->set_performance_state(genpd, state);
+		return;
+	}
+
+	/* Propagate only if this domain has a single parent */
+	if (list_is_singular(&genpd->slave_links)) {
+		/* Performance levels are managed by parent power domain */
+
+		struct generic_pm_domain *master;
+
+		link = list_first_entry(&genpd->slave_links, struct gpd_link, slave_node);
+		master = link->master;
+
+		genpd_lock_nested(master, depth + 1);
+		update_domain_performance_state(master, depth + 1);
+		genpd_unlock(master);
+	}
+}
+
+static int genpd_dev_pm_qos_perf_notifier(struct notifier_block *nb,
+					  unsigned long val, void *ptr)
+{
+	struct generic_pm_domain_data *gpd_data;
+	struct generic_pm_domain *genpd = ERR_PTR(-ENODATA);
+	struct pm_domain_data *pdd;
+	struct device *dev;
+
+	gpd_data = container_of(nb, struct generic_pm_domain_data, perf_nb);
+	dev = gpd_data->base.dev;
+
+	spin_lock_irq(&dev->power.lock);
+
+	pdd = dev->power.subsys_data ?
+		dev->power.subsys_data->domain_data : NULL;
+
+	if (pdd && pdd->dev)
+		genpd = dev_to_genpd(dev);
+
+	spin_unlock_irq(&dev->power.lock);
+
+	if (IS_ERR(genpd))
+		return NOTIFY_DONE;
+
+	genpd_lock(genpd);
+	gpd_data->performance_state = val;
+	update_domain_performance_state(genpd, 0);
+	genpd_unlock(genpd);
+
+	return NOTIFY_DONE;
+}
+
 /**
  * genpd_power_off - Remove power from a given PM domain.
  * @genpd: PM domain to power down.
@@ -1137,6 +1219,9 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
 	gpd_data->td.constraint_changed = true;
 	gpd_data->td.effective_constraint_ns = -1;
 	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
+	gpd_data->performance_state = 0;
+
+	gpd_data->perf_nb.notifier_call = genpd_dev_pm_qos_perf_notifier;
 
 	spin_lock_irq(&dev->power.lock);
 
@@ -1210,12 +1295,16 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
  out:
 	genpd_unlock(genpd);
 
-	if (ret)
+	if (ret) {
 		genpd_free_dev_data(dev, gpd_data);
-	else
+	} else {
 		dev_pm_qos_add_notifier(dev, &gpd_data->nb,
 					DEV_PM_QOS_RESUME_LATENCY);
 
+		dev_pm_qos_add_notifier(dev, &gpd_data->perf_nb,
+					DEV_PM_QOS_PERFORMANCE);
+	}
+
 	return ret;
 }
 
@@ -1249,6 +1338,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
 	pdd = dev->power.subsys_data->domain_data;
 	gpd_data = to_gpd_data(pdd);
+	dev_pm_qos_remove_notifier(dev, &gpd_data->perf_nb,
+				   DEV_PM_QOS_PERFORMANCE);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb,
 				   DEV_PM_QOS_RESUME_LATENCY);
 
@@ -1277,6 +1368,9 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 	genpd_unlock(genpd);
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY);
 
+	dev_pm_qos_add_notifier(dev, &gpd_data->perf_nb,
+				DEV_PM_QOS_PERFORMANCE);
+
 	return ret;
 }
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 81ece61075df..d994ad5ba1c0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -62,8 +62,11 @@ struct generic_pm_domain {
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
 	unsigned int prepared_count;	/* Suspend counter of prepared devices */
+	unsigned int performance_state;	/* Max requested performance state */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
+	int (*set_performance_state)(struct generic_pm_domain *domain,
+				     unsigned int state);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	bool max_off_time_changed;
@@ -117,6 +120,8 @@ struct generic_pm_domain_data {
 	struct pm_domain_data base;
 	struct gpd_timing_data td;
 	struct notifier_block nb;
+	struct notifier_block perf_nb;
+	unsigned int performance_state;
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS
-- 
2.7.1.410.g6faf27b

[PATCH V2 5/6] PM / domain: Save/restore performance state at runtime suspend/resume

[Viresh Kumar]

With runtime PM, the devices get suspended while the system is up and
running in order to save power. At such times, it is important to
re-evaluate the required performance state of the domain, in order to
choose a lower state if possible.

This patch updates the genpd suspend/resume callbacks to do that.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1158a07f92de..ce9ab18be243 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -603,7 +603,8 @@ static int genpd_runtime_suspend(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
 	bool (*suspend_ok)(struct device *__dev);
-	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	struct generic_pm_domain_data *pd_data = dev_gpd_data(dev);
+	struct gpd_timing_data *td = &pd_data->td;
 	bool runtime_pm = pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -660,7 +661,14 @@ static int genpd_runtime_suspend(struct device *dev)
 		return 0;
 
 	genpd_lock(genpd);
+
+	/* Re-evaluate performance state of the domain */
+	pd_data->cached_performance_state = pd_data->performance_state;
+	pd_data->performance_state = 0;
+	update_domain_performance_state(genpd, 0);
+
 	genpd_power_off(genpd, false);
+
 	genpd_unlock(genpd);
 
 	return 0;
@@ -677,7 +685,8 @@ static int genpd_runtime_suspend(struct device *dev)
 static int genpd_runtime_resume(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
-	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	struct generic_pm_domain_data *pd_data = dev_gpd_data(dev);
+	struct gpd_timing_data *td = &pd_data->td;
 	bool runtime_pm = pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -700,7 +709,13 @@ static int genpd_runtime_resume(struct device *dev)
 	}
 
 	genpd_lock(genpd);
+
 	ret = genpd_power_on(genpd, 0);
+
+	/* Re-evaluate performance state of the domain */
+	pd_data->performance_state = pd_data->cached_performance_state;
+	update_domain_performance_state(genpd, 0);
+
 	genpd_unlock(genpd);
 
 	if (ret)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index d994ad5ba1c0..69b453d44adb 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -122,6 +122,7 @@ struct generic_pm_domain_data {
 	struct notifier_block nb;
 	struct notifier_block perf_nb;
 	unsigned int performance_state;
+	unsigned int cached_performance_state;
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS
-- 
2.7.1.410.g6faf27b

[PATCH V2 6/6] PM / OPP: Add support to parse domain-performance-state

[Viresh Kumar]

PLEASE DO NOT APPLY THIS PATCH

It is only sent for completeness. It uses DT bindings which aren't
finalized yet.

Some platforms have the capability to configure the performance state of
their Power Domains. The performance levels are represented by positive
integer values, a lower value represents lower performance state.

This patch introduces code in the OPP core to parse
"domain-performance-state" property from the OPP nodes.

Either none or all OPP nodes in an OPP table shall have the property
set.

NOT-Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c    | 75 ++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/debugfs.c |  4 +++
 drivers/base/power/opp/of.c      | 44 +++++++++++++++++++++++
 drivers/base/power/opp/opp.h     | 12 +++++++
 4 files changed, 135 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 91ec3232d630..b5bb5f8775dc 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -542,6 +542,63 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 	return ret;
 }
 
+static int _update_pm_qos_request(struct device *dev,
+				  struct dev_pm_qos_request *req,
+				  unsigned int perf)
+{
+	int ret;
+
+	if (likely(dev_pm_qos_request_active(req)))
+		ret = dev_pm_qos_update_request(req, perf);
+	else
+		ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_PERFORMANCE,
+					     perf);
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int _generic_set_opp_pd(struct device *dev, struct clk *clk,
+			       struct dev_pm_qos_request *req,
+			       unsigned long old_freq, unsigned long freq,
+			       unsigned int old_perf, unsigned int new_perf)
+{
+	int ret;
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		ret = _update_pm_qos_request(dev, req, new_perf);
+		if (ret)
+			return ret;
+	}
+
+	/* Change frequency */
+	ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+	if (ret)
+		goto restore_perf;
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		ret = _update_pm_qos_request(dev, req, new_perf);
+		if (ret)
+			goto restore_freq;
+	}
+
+	return 0;
+
+restore_freq:
+	if (_generic_set_opp_clk_only(dev, clk, freq, old_freq))
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_perf:
+	if (old_perf)
+		_update_pm_qos_request(dev, req, old_perf);
+
+	return ret;
+}
+
 static int _generic_set_opp(struct dev_pm_set_opp_data *data)
 {
 	struct dev_pm_opp_supply *old_supply = data->old_opp.supplies;
@@ -662,6 +719,21 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	regulators = opp_table->regulators;
 
+	/* Has power domains performance states */
+	if (opp_table->has_pd_perf_states) {
+		unsigned int old_perf = 0, new_perf;
+		struct dev_pm_qos_request *req = &opp_table->qos_request;
+
+		new_perf = opp->pd_perf_state;
+		if (!IS_ERR(old_opp))
+			old_perf = old_opp->pd_perf_state;
+
+		rcu_read_unlock();
+
+		return _generic_set_opp_pd(dev, clk, req, old_freq, freq,
+					   old_perf, new_perf);
+	}
+
 	/* Only frequency scaling */
 	if (!regulators) {
 		ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
@@ -807,6 +879,9 @@ 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;
 
+	if (dev_pm_qos_request_active(&opp_table->qos_request))
+		dev_pm_qos_remove_request(&opp_table->qos_request);
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index 95f433db4ac7..264958ab3de9 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -104,6 +104,10 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
 		return -ENOMEM;
 
+	if (!debugfs_create_u32("power_domain_perf_state", S_IRUGO, d,
+				&opp->pd_perf_state))
+		return -ENOMEM;
+
 	if (!opp_debug_create_supplies(opp, opp_table, d))
 		return -ENOMEM;
 
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 083b79fa0c62..438a6b59baed 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -310,6 +310,45 @@ 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;
 
+	/*
+	 * Make sure that all information is present around domain power states
+	 * and nothing is left out.
+	 */
+	if (!of_property_read_u32(np, "domain-performance-state",
+				  &new_opp->pd_perf_state)) {
+		if (!opp_table->has_pd) {
+			ret = -EINVAL;
+			dev_err(dev, "%s: OPP node can't have performance state as device doesn't have power-domain\n",
+				__func__);
+			goto free_opp;
+		}
+
+		if (!new_opp->pd_perf_state) {
+			ret = -EINVAL;
+			dev_err(dev, "%s: OPP node can't have performance state as 0\n",
+				__func__);
+			goto free_opp;
+		}
+
+		if (opp_table->has_pd_perf_states == -1) {
+			opp_table->has_pd_perf_states = 1;
+		} else if (!opp_table->has_pd_perf_states) {
+			ret = -EINVAL;
+			dev_err(dev, "%s: Not all OPP nodes have performance state\n",
+				__func__);
+			goto free_opp;
+		}
+	} else {
+		if (opp_table->has_pd_perf_states == -1) {
+			opp_table->has_pd_perf_states = 0;
+		} else if (opp_table->has_pd_perf_states) {
+			ret = -EINVAL;
+			dev_err(dev, "%s: Not all OPP nodes have performance state\n",
+				__func__);
+			goto free_opp;
+		}
+	}
+
 	ret = opp_parse_supplies(new_opp, dev, opp_table);
 	if (ret)
 		goto free_opp;
@@ -374,6 +413,11 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 	if (!opp_table)
 		return -ENOMEM;
 
+	if (of_find_property(dev->of_node, "power-domains", NULL)) {
+		opp_table->has_pd = true;
+		opp_table->has_pd_perf_states = -1;
+	}
+
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
 		count++;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 166eef990599..41a2c0a67031 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -20,6 +20,7 @@
 #include <linux/list.h>
 #include <linux/limits.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 #include <linux/notifier.h>
 
 struct clk;
@@ -58,6 +59,7 @@ extern struct list_head opp_tables;
  * @dynamic:	not-created from static DT entries.
  * @turbo:	true if turbo (boost) OPP
  * @suspend:	true if suspend OPP
+ * @pd_perf_state: Performance state of power domain
  * @rate:	Frequency in hertz
  * @supplies:	Power supplies voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -76,6 +78,7 @@ struct dev_pm_opp {
 	bool dynamic;
 	bool turbo;
 	bool suspend;
+	unsigned int pd_perf_state;
 	unsigned long rate;
 
 	struct dev_pm_opp_supply *supplies;
@@ -137,6 +140,11 @@ enum opp_table_access {
  * @regulator_count: Number of power supply regulators
  * @set_opp: Platform specific set_opp callback
  * @set_opp_data: Data to be passed to set_opp callback
+ * @has_pd: True if the device node contains power-domain property
+ * @has_pd_perf_states: Can have value of 0, 1 or -1. -1 means uninitialized
+ * state, 0 means that OPP nodes don't have perf states and 1 means that OPP
+ * nodes have perf states.
+ * @qos_request: Qos request.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -174,6 +182,10 @@ struct opp_table {
 	int (*set_opp)(struct dev_pm_set_opp_data *data);
 	struct dev_pm_set_opp_data *set_opp_data;
 
+	bool has_pd;
+	int has_pd_perf_states;
+	struct dev_pm_qos_request qos_request;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
-- 
2.7.1.410.g6faf27b

Re: [PATCH V2 1/6] PM / QOS: Add default case to the switch

[Pavel Machek]

On Thu 2017-02-09 09:11:47, Viresh Kumar wrote:
> The switch block handles all the QOS request types present today, but
> starts giving compilation warnings as soon as a new type is added and
> not handled in this.
> 
> To prevent against that, add the default case as well and do a WARN from
> it.

I'd say compilation-time warning is better than hmm.... stacktrace and memory leak
at runtime?

> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -621,6 +621,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
>  		req = dev->power.qos->flags_req;
>  		dev->power.qos->flags_req = NULL;
>  		break;
> +	default:
> +		WARN_ON(1);
> +		return;
>  	}
>  	__dev_pm_qos_remove_request(req);
>  	kfree(req);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V2 1/6] PM / QOS: Add default case to the switch

[Viresh Kumar]

On 09-02-17, 15:24, Pavel Machek wrote:
> On Thu 2017-02-09 09:11:47, Viresh Kumar wrote:
> > The switch block handles all the QOS request types present today, but
> > starts giving compilation warnings as soon as a new type is added and
> > not handled in this.
> > 
> > To prevent against that, add the default case as well and do a WARN from
> > it.
> 
> I'd say compilation-time warning is better than hmm.... stacktrace and memory leak
> at runtime?

Of course we aren't going to allow a compilation warning for each and every
platform that compiles this file. How do you wish to fix the issue then ?

-- 
viresh

Re: [PATCH V2 1/6] PM / QOS: Add default case to the switch

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On Fri 2017-02-10 11:30:30, Viresh Kumar wrote:
> On 09-02-17, 15:24, Pavel Machek wrote:
> > On Thu 2017-02-09 09:11:47, Viresh Kumar wrote:
> > > The switch block handles all the QOS request types present today, but
> > > starts giving compilation warnings as soon as a new type is added and
> > > not handled in this.
> > > 
> > > To prevent against that, add the default case as well and do a WARN from
> > > it.
> > 
> > I'd say compilation-time warning is better than hmm.... stacktrace and memory leak
> > at runtime?
> 
> Of course we aren't going to allow a compilation warning for each and every
> platform that compiles this file. How do you wish to fix the issue then ?

What is tue issue?

As soon as new QoS request type is added, this switch can be
fixed. There is no issue now, and there should be no issue in future.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH V2 1/6] PM / QOS: Add default case to the switch

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Fri 2017-02-10 11:30:30, Viresh Kumar wrote:
> On 09-02-17, 15:24, Pavel Machek wrote:
> > On Thu 2017-02-09 09:11:47, Viresh Kumar wrote:
> > > The switch block handles all the QOS request types present today, but
> > > starts giving compilation warnings as soon as a new type is added and
> > > not handled in this.
> > > 
> > > To prevent against that, add the default case as well and do a WARN from
> > > it.
> > 
> > I'd say compilation-time warning is better than hmm.... stacktrace and memory leak
> > at runtime?
> 
> Of course we aren't going to allow a compilation warning for each and every
> platform that compiles this file. How do you wish to fix the issue then ?

Surely compilation warnings are better than getting bug reports from
users?

Of course, it is better to fix the switch when adding new QoS type...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH V2 1/6] PM / QOS: Add default case to the switch

[Viresh Kumar]

On 10-02-17, 13:15, Pavel Machek wrote:
> As soon as new QoS request type is added, this switch can be
> fixed. There is no issue now, and there should be no issue in future.

Sure. Will do it in V3.

-- 
viresh

Re: [PATCH V2 0/6] PM / Domains: Implement domain performance states

[Viresh Kumar]

On 09-02-17, 09:11, Viresh Kumar wrote:
> The first 5 patches update the PM domain and QoS frameworks to support
> that and the last one presents the front end interface to it.

@Kevin and Ulf,

Is there something wrong with this series? Its been 7 weeks since this
series is getting posted and I haven't received a single review from
you guys. We will get into the merge window very soon and then it
wouldn't be good for me to ask for reviews as everyone would be busy.

This stuff and the rest of the development around it is getting
delayed unnecessarily.

Please see if you guys can take some time out to get this reviewed.

Thanks.

-- 
viresh

Re: [PATCH V2 0/6] PM / Domains: Implement domain performance states

[Ulf Hansson]

On 17 February 2017 at 06:38, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 09-02-17, 09:11, Viresh Kumar wrote:
>> The first 5 patches update the PM domain and QoS frameworks to support
>> that and the last one presents the front end interface to it.
>
> @Kevin and Ulf,
>
> Is there something wrong with this series? Its been 7 weeks since this
> series is getting posted and I haven't received a single review from
> you guys. We will get into the merge window very soon and then it
> wouldn't be good for me to ask for reviews as everyone would be busy.
>
> This stuff and the rest of the development around it is getting
> delayed unnecessarily.
>
> Please see if you guys can take some time out to get this reviewed.

Apologize for the delay! I will have a look asap.

Kind regards
Uffe

Re: [PATCH V2 0/6] PM / Domains: Implement domain performance states

[Kevin Hilman]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> An earlier series[1] tried to implement bindings for PM domain
> performance states. Rob Herring suggested that we can actually merge the
> supporting code first instead of bindings, as that will make things
> easier to understand for all. The bindings can be decided and merged
> later.
>
> The bindings [1] aren't discarded yet and this series is based on a
> version of those only. The bindings are only used by the last patch,
> which should not be applied and is only sent for completeness.
>
> IOW, this series doesn't have any dependencies and can be merged
> straight away without waiting for the DT bindings.
>
> A brief summary of the problem this series is trying to solve:
>
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.

And what about domains where the performance levels are represented by
someting other than positive integer values? 

IMO, this implementation should start with a more generic approach
(e.g. OPPs) that would be useful on more SoCs that just qcom.  For SoCs
like QCOM, you could use dummy/simplfied OPPs that represent the integer
values passed to the qcom firmware.

> We decided earlier that we should extend Power Domain framework to
> support active state power management as well. The power-domains until
> now were only concentrating on the idle state management of the device
> and this needs to change in order to reuse the infrastructure of power
> domains for active state management.

Yes.  Thanks for working on it!

Kevin

Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

[Kevin Hilman]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
>
> This patch registers the power domain framework for PM QOS performance
> notifier in order to manage performance state of power domains.

It seems to me it doesm't just register, but actually keeps track of the
performance_state by always tracking the max.

> This also allows the power domain drivers to implement a
> ->set_performance_state() callback, which will be called by the power
> domain core from the notifier routine.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 98 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_domain.h   |  5 +++
>  2 files changed, 101 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a73d79670a64..1158a07f92de 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -367,6 +367,88 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static void update_domain_performance_state(struct generic_pm_domain *genpd,
> +					    int depth)
> +{
> +	struct generic_pm_domain_data *pd_data;
> +	struct generic_pm_domain *subdomain;
> +	struct pm_domain_data *pdd;
> +	unsigned int state = 0;
> +	struct gpd_link *link;
> +
> +	/* Traverse all devices within the domain */
> +	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> +		pd_data = to_gpd_data(pdd);
> +
> +		if (pd_data->performance_state > state)
> +			state = pd_data->performance_state;
> +	}

This seems to only update the state if it's bigger.  Maybe I'm missing
something here, but it seems like won't be able to lower the
performance_state after it's been raised?

> +	/* Traverse all subdomains within the domain */
> +	list_for_each_entry(link, &genpd->master_links, master_node) {
> +		subdomain = link->slave;
> +
> +		if (subdomain->performance_state > state)
> +			state = subdomain->performance_state;
> +	}

So subdomains are always assumed to influence the performance_state of
the parent domains?  Is that always the case?  I suspect this should be
probably be a reasonable default assumption, but maybe controlled with a
flag.

> +	if (genpd->performance_state == state)
> +		return;
> +
> +	genpd->performance_state = state;
> +
> +	if (genpd->set_performance_state) {
> +		genpd->set_performance_state(genpd, state);
> +		return;
> +	}

So is zero not a valid performance_state?  That doesn't seem quite right
to me, but either way, it should be documented.

> +	/* Propagate only if this domain has a single parent */

Why?  This limitation should be explained in the cover letter and
changelog.  I would also expect some sort of WARN here since this could
otherwise be a rather silent failures.

[...]

Kevin

Re: [PATCH V2 5/6] PM / domain: Save/restore performance state at runtime suspend/resume

[Kevin Hilman]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> With runtime PM, the devices get suspended while the system is up and
> running in order to save power. At such times, it is important to
> re-evaluate the required performance state of the domain, in order to
> choose a lower state if possible.
>
> This patch updates the genpd suspend/resume callbacks to do that.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Doesn't this assume that a device in the domain would need to change
performance state while runtime suspended.  How would that happen?

Rather than adding this here, I would think that drivers would instead
remove any QoS requests before going into runtime suspend, which would
trigger an update before runtime suspending.

Kevin

Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

[Viresh Kumar]

On 17-02-17, 15:54, Kevin Hilman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > Some platforms have the capability to configure the performance state of
> > their Power Domains. The performance levels are represented by positive
> > integer values, a lower value represents lower performance state.
> >
> > This patch registers the power domain framework for PM QOS performance
> > notifier in order to manage performance state of power domains.
> 
> It seems to me it doesm't just register, but actually keeps track of the
> performance_state by always tracking the max.

Yes. Will update the commit log to make sure it is clear.

> > This also allows the power domain drivers to implement a
> > ->set_performance_state() callback, which will be called by the power
> > domain core from the notifier routine.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/base/power/domain.c | 98 ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pm_domain.h   |  5 +++
> >  2 files changed, 101 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index a73d79670a64..1158a07f92de 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -367,6 +367,88 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> >  	return NOTIFY_DONE;
> >  }
> >  
> > +static void update_domain_performance_state(struct generic_pm_domain *genpd,
> > +					    int depth)
> > +{
> > +	struct generic_pm_domain_data *pd_data;
> > +	struct generic_pm_domain *subdomain;
> > +	struct pm_domain_data *pdd;
> > +	unsigned int state = 0;
> > +	struct gpd_link *link;
> > +
> > +	/* Traverse all devices within the domain */
> > +	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> > +		pd_data = to_gpd_data(pdd);
> > +
> > +		if (pd_data->performance_state > state)
> > +			state = pd_data->performance_state;
> > +	}
> 
> This seems to only update the state if it's bigger.  Maybe I'm missing
> something here, but it seems like won't be able to lower the
> performance_state after it's been raised?

'state' is initialized to 0 to begin with and then the above loop
finds the highest state requested so far. The below code (after the
below loop) then changes the state if it isn't equal to the previous
one. That is, it would update the state if the new target state is
lower than the current one. Or am I missing a bug in there ?

> > +	/* Traverse all subdomains within the domain */
> > +	list_for_each_entry(link, &genpd->master_links, master_node) {
> > +		subdomain = link->slave;
> > +
> > +		if (subdomain->performance_state > state)
> > +			state = subdomain->performance_state;
> > +	}
> 
> So subdomains are always assumed to influence the performance_state of
> the parent domains?  Is that always the case? 

It is true for the hardware we have to work on right now, but I would
assume that being the most common case.

> I suspect this should be
> probably be a reasonable default assumption, but maybe controlled with a
> flag.

Yeah, maybe we can have a flag to ignore a subdomain while doing the
calculations, but I would rather defer that to the first platform that
needs it and leave it as is for now. I am afraid that code may never
get used and maybe we should wait for a real case first.

> > +	if (genpd->performance_state == state)
> > +		return;
> > +
> > +	genpd->performance_state = state;
> > +
> > +	if (genpd->set_performance_state) {
> > +		genpd->set_performance_state(genpd, state);
> > +		return;
> > +	}
> 
> So is zero not a valid performance_state?

Zero is a *valid* performance state. Are you getting confused with the
above 'if' block which checks for a valid set_performance_state()
callback and not the performance level ?

> > +	/* Propagate only if this domain has a single parent */
> 
> Why?  This limitation should be explained in the cover letter and
> changelog.  I would also expect some sort of WARN here since this could
> otherwise be a rather silent failures.

I think this limitation can just be removed, but I am confused a bit.

I thought that support for multiple parent domains isn't yet there,
isn't it? And that few people are trying to add it in kernel and the
stuff is still under review. Like this thread:

https://lkml.org/lkml/2016/11/22/792

-- 
viresh

Re: [PATCH V2 5/6] PM / domain: Save/restore performance state at runtime suspend/resume

[Viresh Kumar]

On 17-02-17, 15:58, Kevin Hilman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > With runtime PM, the devices get suspended while the system is up and
> > running in order to save power. At such times, it is important to
> > re-evaluate the required performance state of the domain, in order to
> > choose a lower state if possible.
> >
> > This patch updates the genpd suspend/resume callbacks to do that.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Doesn't this assume that a device in the domain would need to change
> performance state while runtime suspended.  How would that happen?
> 
> Rather than adding this here, I would think that drivers would instead
> remove any QoS requests before going into runtime suspend, which would
> trigger an update before runtime suspending.

Okay, lets leave it for the drivers, at least for the time being. I
will drop this patch.

-- 
viresh

Re: [PATCH V2 0/6] PM / Domains: Implement domain performance states

[Viresh Kumar]

On 17-02-17, 15:22, Kevin Hilman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > An earlier series[1] tried to implement bindings for PM domain
> > performance states. Rob Herring suggested that we can actually merge the
> > supporting code first instead of bindings, as that will make things
> > easier to understand for all. The bindings can be decided and merged
> > later.
> >
> > The bindings [1] aren't discarded yet and this series is based on a
> > version of those only. The bindings are only used by the last patch,
> > which should not be applied and is only sent for completeness.
> >
> > IOW, this series doesn't have any dependencies and can be merged
> > straight away without waiting for the DT bindings.
> >
> > A brief summary of the problem this series is trying to solve:
> >
> > Some platforms have the capability to configure the performance state of
> > their Power Domains. The performance levels are represented by positive
> > integer values, a lower value represents lower performance state.
> 
> And what about domains where the performance levels are represented by
> someting other than positive integer values? 

The V2 bindings were modified to take that into account:

https://marc.info/?l=linux-kernel&m=148154021427727&w=2

Copying an example from it:

+               domain_perf_state1: pstate@1 {
+                       performance-level = <1>;
+                       domain-microvolt = <970000 975000 985000>;
+               };

The above node describes one performance state for the power domain.
This node can have any number of configurables depending on the
individual platforms. The performance-level is the integer number we
have been talking about in this series, which can be used in all the
calculations to differentiate between different levels.

The final code in the generic PM layer will end up calling
set_performance_state(performance-level); Now the drivers can go find
a corresponding node to find different configurables like
domain-microvolt, etc and configure the hardware. Or in simple cases,
like Qcom, the performance-level can be used directly without
referring to the node.

Consider the 'performance-level' field as the 'opp-hz' field in the
OPP tables which is used to distinguish lower/higher OPPs. Just that
we will not always have a frequency here and so an integer value.

> IMO, this implementation should start with a more generic approach
> (e.g. OPPs) that would be useful on more SoCs that just qcom.

I agree with that opinion. Perhaps things became a bit confusing as
the bindings were sent separately earlier and the code followed
separately. Also not everything was finished here to follow proper
bindings. Like the generic helpers weren't introduced yet to parse the
nodes like domain_perf_state1.

> For SoCs
> like QCOM, you could use dummy/simplfied OPPs that represent the integer
> values passed to the qcom firmware.

I am not sure if reusing OPPs here would be a good choice. I would
rather have separate nodes like what I defined earlier.

In the next version I will try to send all things together and try to
close all the open ends..

-- 
viresh

Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

[Ulf Hansson]

[...]

>
>> > +   /* Propagate only if this domain has a single parent */
>>
>> Why?  This limitation should be explained in the cover letter and
>> changelog.  I would also expect some sort of WARN here since this could
>> otherwise be a rather silent failures.
>
> I think this limitation can just be removed, but I am confused a bit.
>
> I thought that support for multiple parent domains isn't yet there,
> isn't it? And that few people are trying to add it in kernel and the
> stuff is still under review. Like this thread:

A genpd PM domain can have several parents (or to use the genpd
terminology, masters).

This is different from allowing a device to be attached to more than
one PM domains, which is what I think you are referring to. That isn't
supported.

>
> https://lkml.org/lkml/2016/11/22/792
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH V2 3/6] PM / QOS: Add 'performance' request

[Ulf Hansson]

On 9 February 2017 at 04:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Add a new QOS request type: DEV_PM_QOS_PERFORMANCE.
>
> This is required to support runtime performance constraints for the
> devices. Also allow notifiers to be registered against it, which can be
> used by frameworks like genpd.

To me, this is a too slim description of why/what/how.

Just by reading from this change log, sounds like we already have
OPPs. So I think it would be useful to explain why those isn't
suitable here.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/power/pm_qos_interface.txt |  2 +-
>  drivers/base/power/qos.c                 | 69 +++++++++++++++++++++++++++-----
>  include/linux/pm_qos.h                   |  4 ++
>  3 files changed, 63 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
> index c7989d140428..a0a45ecbc1a8 100644
> --- a/Documentation/power/pm_qos_interface.txt
> +++ b/Documentation/power/pm_qos_interface.txt
> @@ -172,7 +172,7 @@ type.
>
>  The callback is called when the aggregated value of the device constraints list
>  is changed. Currently it only supports the notifier to be registered for resume

/s /only /

> -latency device PM QoS.
> +latency and performance device PM QoS.
>
>  int dev_pm_qos_remove_notifier(device, notifier, type):
>  Removes the notification callback function for the device.
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 9adc208cf1fc..cf070468a7ca 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -163,6 +163,10 @@ static int apply_constraint(struct dev_pm_qos_request *req,
>                         req->dev->power.set_latency_tolerance(req->dev, value);
>                 }
>                 break;
> +       case DEV_PM_QOS_PERFORMANCE:
> +               ret = pm_qos_update_target(&qos->performance, &req->data.pnode,
> +                                          action, value);
> +               break;
>         case DEV_PM_QOS_FLAGS:
>                 ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
>                                           action, value);
> @@ -191,12 +195,13 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
>         if (!qos)
>                 return -ENOMEM;
>
> -       n = kzalloc(sizeof(*n), GFP_KERNEL);
> +       n = kzalloc(2 * sizeof(*n), GFP_KERNEL);
>         if (!n) {
>                 kfree(qos);
>                 return -ENOMEM;
>         }
>         BLOCKING_INIT_NOTIFIER_HEAD(n);
> +       BLOCKING_INIT_NOTIFIER_HEAD(n + 1);

Hmm, why do we need a separate notifier list type here. Shouldn't we
just propagate the type of the notification as a part the notification
itself.

In other words, dev_pm_qos_add_notifier() doesn't need to take the
extra "type" parameter, but instead the notification type will be sent
to the receiver as part of the notification callback.

That should simplify a lot, both for these changes but also for
following changes to genpd.

And that said, perhaps we should add new enum definition type which
specifies which notification messages the dev PM QoS notifier supports
- to avoid confusion. Then it's up to the receiver to act upon those
messages it supports.

>
>         c = &qos->resume_latency;
>         plist_head_init(&c->list);
> @@ -213,6 +218,14 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
>         c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
>         c->type = PM_QOS_MIN;
>
> +       c = &qos->performance;
> +       plist_head_init(&c->list);
> +       c->target_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
> +       c->default_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
> +       c->no_constraint_value = PM_QOS_PERFORMANCE_DEFAULT_VALUE;
> +       c->type = PM_QOS_MAX;
> +       c->notifiers = n + 1;
> +
>         INIT_LIST_HEAD(&qos->flags.list);
>
>         spin_lock_irq(&dev->power.lock);
> @@ -271,6 +284,11 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
>                 apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>                 memset(req, 0, sizeof(*req));
>         }
> +       c = &qos->performance;
> +       plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
> +               apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> +               memset(req, 0, sizeof(*req));
> +       }
>         f = &qos->flags;
>         list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) {
>                 apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> @@ -382,6 +400,7 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
>         switch(req->type) {
>         case DEV_PM_QOS_RESUME_LATENCY:
>         case DEV_PM_QOS_LATENCY_TOLERANCE:
> +       case DEV_PM_QOS_PERFORMANCE:
>                 curr_value = req->data.pnode.prio;
>                 break;
>         case DEV_PM_QOS_FLAGS:
> @@ -492,11 +511,9 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
>  int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
>                             enum dev_pm_qos_req_type type)
>  {
> +       struct dev_pm_qos *qos;
>         int ret = 0;
>
> -       if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
> -               return -EINVAL;
> -
>         mutex_lock(&dev_pm_qos_mtx);
>
>         if (IS_ERR(dev->power.qos))
> @@ -504,10 +521,26 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
>         else if (!dev->power.qos)
>                 ret = dev_pm_qos_constraints_allocate(dev);
>
> -       if (!ret)
> -               ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers,
> +       if (ret)
> +               goto unlock;
> +
> +       qos = dev->power.qos;
> +
> +       switch (type) {
> +       case DEV_PM_QOS_RESUME_LATENCY:
> +               ret = blocking_notifier_chain_register(qos->resume_latency.notifiers,
> +                                                      notifier);
> +               break;
> +       case DEV_PM_QOS_PERFORMANCE:
> +               ret = blocking_notifier_chain_register(qos->performance.notifiers,
>                                                        notifier);
> +               break;
> +       default:
> +               WARN_ON(1);

No WARN_ON, please.

> +               ret = -EINVAL;
> +       }
>
> +unlock:
>         mutex_unlock(&dev_pm_qos_mtx);
>         return ret;
>  }

[...]

Kind regards
Uffe

Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

[Viresh Kumar]

On 21-02-17, 16:28, Ulf Hansson wrote:
> A genpd PM domain can have several parents (or to use the genpd
> terminology, masters).
> 
> This is different from allowing a device to be attached to more than
> one PM domains, which is what I think you are referring to. That isn't
> supported.

Thanks for clarifying. Got it now.

-- 
viresh