Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order
@ 2020-07-30  8:01 Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps() Stephan Gerhold
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Stephan Gerhold @ 2020-07-30  8:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephan Gerhold, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
	Nishanth Menon, Stephen Boyd, linux-pm, linux-kernel,
	Niklas Cassel

I'm trying to get CPR (Core Power Reduction, AVS) working for MSM8916 on mainline.
Shortly said there are two power domains that must be scaled with the CPU OPP table:

  - (VDD)MX
  - CPR

My idea for this was to add both as "required-opps" to the CPR OPP table
and let the OPP core take care of all the scaling.

There are two remaining problems that need to be addressed for that to work:

  1. The power domains should be scaled down in reverse order
     (MX, CPR when scaling up, CPR, MX when scaling down).
  2. Something has to enable the virtual genpd devices to make the rpmpd driver
     actually respect the performance states we vote for.

Both issues were briefly discussed before (see links in the patches),
but I think we did not agree on an exact solution yet. After some consideration,
I thought it would be best to address these directly in the OPP core.

However, note that this patch is RFC because it is just supposed to initiate
discussion if alternative solutions would be better. :)

Stephan Gerhold (3):
  opp: Reduce code duplication in _set_required_opps()
  opp: Set required OPPs in reverse order when scaling down
  opp: Power on (virtual) power domains managed by the OPP core

 drivers/opp/core.c | 115 ++++++++++++++++++++++++++++++++++++---------
 drivers/opp/opp.h  |   1 +
 2 files changed, 93 insertions(+), 23 deletions(-)

--
2.27.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()
  2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
@ 2020-07-30  8:01 ` Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down Stephan Gerhold
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Stephan Gerhold @ 2020-07-30  8:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephan Gerhold, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
	Nishanth Menon, Stephen Boyd, linux-pm, linux-kernel,
	Niklas Cassel

Move call to dev_pm_genpd_set_performance_state() to a separate
function so we can avoid duplicating the code for the single and
multiple genpd case.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/opp/core.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9d7fb45b1786..f7a476b55069 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -781,6 +781,21 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	return opp_table->set_opp(data);
 }
 
+static int _set_required_opp(struct device *dev, struct device *pd_dev,
+			     struct dev_pm_opp *opp, int i)
+{
+	unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+	int ret;
+
+	ret = dev_pm_genpd_set_performance_state(pd_dev, pstate);
+	if (ret) {
+		dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
+			dev_name(pd_dev), pstate, ret);
+	}
+
+	return ret;
+}
+
 /* This is only called for PM domain for now */
 static int _set_required_opps(struct device *dev,
 			      struct opp_table *opp_table,
@@ -788,22 +803,15 @@ static int _set_required_opps(struct device *dev,
 {
 	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
 	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
-	unsigned int pstate;
+	struct device *pd_dev;
 	int i, ret = 0;
 
 	if (!required_opp_tables)
 		return 0;
 
 	/* Single genpd case */
-	if (!genpd_virt_devs) {
-		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
-		ret = dev_pm_genpd_set_performance_state(dev, pstate);
-		if (ret) {
-			dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
-				dev_name(dev), pstate, ret);
-		}
-		return ret;
-	}
+	if (!genpd_virt_devs)
+		return _set_required_opp(dev, dev, opp, 0);
 
 	/* Multiple genpd case */
 
@@ -814,17 +822,11 @@ static int _set_required_opps(struct device *dev,
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
-		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
-
-		if (!genpd_virt_devs[i])
-			continue;
+		pd_dev = genpd_virt_devs[i];
 
-		ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], pstate);
-		if (ret) {
-			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
-				dev_name(genpd_virt_devs[i]), pstate, ret);
+		ret = _set_required_opp(dev, pd_dev, opp, i);
+		if (ret)
 			break;
-		}
 	}
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down
  2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps() Stephan Gerhold
@ 2020-07-30  8:01 ` Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core Stephan Gerhold
  2020-08-12  8:53 ` [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Ulf Hansson
  3 siblings, 0 replies; 5+ messages in thread
From: Stephan Gerhold @ 2020-07-30  8:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephan Gerhold, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
	Nishanth Menon, Stephen Boyd, linux-pm, linux-kernel,
	Niklas Cassel

The OPP core already has well-defined semantics to ensure required
OPPs/regulators are set before/after the frequency change, depending
on if we scale up or down.

Similar requirements might exist for the order of required OPPs
when multiple power domains need to be scaled for a frequency change.

For example, on Qualcomm platforms using CPR (Core Power Reduction),
we need to scale the VDDMX and CPR power domain. When scaling up,
MX should be scaled up before CPR. When scaling down, CPR should be
scaled down before MX.

In general, if there are multiple "required-opps" in the device tree
I would expect that the order is either irrelevant, or there is some
dependency between the power domains. In that case, the power domains
should be scaled down in reverse order.

This commit updates _set_required_opps() to set required OPPs in
reverse order when scaling down.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Related discussion: https://lore.kernel.org/linux-arm-msm/20200525194443.GA11851@flawful.org/

The advantage of this approach is that the CPR driver does not need
to bother with the VDDMX power domain at all - the requirements
can be fully described within the device tree, see e.g. [1].
An alternative option would be to modify the CPR driver to make these votes.

[1]: https://lore.kernel.org/linux-arm-msm/20200507104603.GA581328@gerhold.net/2-msm8916-vdd-mx.patch
---
 drivers/opp/core.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index f7a476b55069..f93f551c911e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -799,7 +799,7 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev,
 /* This is only called for PM domain for now */
 static int _set_required_opps(struct device *dev,
 			      struct opp_table *opp_table,
-			      struct dev_pm_opp *opp)
+			      struct dev_pm_opp *opp, bool up)
 {
 	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
 	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
@@ -821,12 +821,24 @@ static int _set_required_opps(struct device *dev,
 	 */
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
-	for (i = 0; i < opp_table->required_opp_count; i++) {
-		pd_dev = genpd_virt_devs[i];
+	if (up) {
+		/* Scaling up? Set required OPPs in normal order */
+		for (i = 0; i < opp_table->required_opp_count; i++) {
+			pd_dev = genpd_virt_devs[i];
 
-		ret = _set_required_opp(dev, pd_dev, opp, i);
-		if (ret)
-			break;
+			ret = _set_required_opp(dev, pd_dev, opp, i);
+			if (ret)
+				break;
+		}
+	} else {
+		/* Scaling down? Set required OPPs in reverse order */
+		for (i = opp_table->required_opp_count - 1; i >= 0; i--) {
+			pd_dev = genpd_virt_devs[i];
+
+			ret = _set_required_opp(dev, pd_dev, opp, i);
+			if (ret)
+				break;
+		}
 	}
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
 
@@ -914,7 +926,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			opp_table->regulator_enabled = false;
 		}
 
-		ret = _set_required_opps(dev, opp_table, NULL);
+		ret = _set_required_opps(dev, opp_table, NULL, false);
 		goto put_opp_table;
 	}
 
@@ -973,7 +985,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling up? Configure required OPPs before frequency */
 	if (freq >= old_freq) {
-		ret = _set_required_opps(dev, opp_table, opp);
+		ret = _set_required_opps(dev, opp_table, opp, true);
 		if (ret)
 			goto put_opp;
 	}
@@ -993,7 +1005,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling down? Configure required OPPs after frequency */
 	if (!ret && freq < old_freq) {
-		ret = _set_required_opps(dev, opp_table, opp);
+		ret = _set_required_opps(dev, opp_table, opp, false);
 		if (ret)
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
 	}
-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core
  2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps() Stephan Gerhold
  2020-07-30  8:01 ` [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down Stephan Gerhold
@ 2020-07-30  8:01 ` Stephan Gerhold
  2020-08-12  8:53 ` [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Ulf Hansson
  3 siblings, 0 replies; 5+ messages in thread
From: Stephan Gerhold @ 2020-07-30  8:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephan Gerhold, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
	Nishanth Menon, Stephen Boyd, linux-pm, linux-kernel,
	Niklas Cassel

dev_pm_opp_attach_genpd() allows attaching an arbitrary number of
power domains to an OPP table. In that case, the genpd core will
create a virtual device for each of the power domains.

At the moment, the OPP core only calls
dev_pm_genpd_set_performance_state() on these virtual devices.
It does not attempt to power on the power domains. Therefore
the required power domain might never get turned on.

So far, dev_pm_opp_attach_genpd() is only used in qcom-cpufreq-nvmem.c
to attach the CPR power domain to the CPU OPP table. The CPR driver
does not check if it was actually powered on so this did not cause
any problems. However, other drivers (e.g. rpmpd) might ignore the
performance state until the power domain is actually powered on.

Since these virtual devices are managed exclusively by the OPP core,
I would say that it should also be responsible to ensure they are
enabled. A similar approach is already used for regulators, see
commit 8d45719caaf5 ("opp: core: add regulators enable and disable").

This commit implements similar functionality for the virtual genpd
devices managed by the OPP core. The power domains are turned on
the first time dev_pm_opp_set_rate() is called. They are turned off
again when dev_pm_opp_set_rate(dev, 0) is called.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Related discussion: https://lore.kernel.org/linux-arm-msm/20200426123140.GA190483@gerhold.net/

There would be also other ways to implement this, e.g. device links,
assuming that the device using the OPP table also makes use of runtime PM.
My first thought was that it would be most consistent to handle this like
regulators, bandwidth votes etc. RFC :)
---
 drivers/opp/core.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h  |  1 +
 2 files changed, 56 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index f93f551c911e..66ecffe12f01 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include "opp.h"
@@ -796,6 +797,26 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev,
 	return ret;
 }
 
+static int _enable_required_opp(struct device *dev, struct device *pd_dev,
+				bool on)
+{
+	int ret;
+
+	if (on) {
+		ret = pm_runtime_get_sync(pd_dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(pd_dev);
+			dev_err(dev, "Failed to enable %s: %d\n",
+				dev_name(pd_dev), ret);
+			return ret;
+		}
+	} else {
+		pm_runtime_put(pd_dev);
+	}
+
+	return 0;
+}
+
 /* This is only called for PM domain for now */
 static int _set_required_opps(struct device *dev,
 			      struct opp_table *opp_table,
@@ -803,6 +824,8 @@ static int _set_required_opps(struct device *dev,
 {
 	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
 	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
+	bool power_on = opp != NULL;
+	bool already_enabled = power_on == opp_table->genpd_virt_enabled;
 	struct device *pd_dev;
 	int i, ret = 0;
 
@@ -829,6 +852,20 @@ static int _set_required_opps(struct device *dev,
 			ret = _set_required_opp(dev, pd_dev, opp, i);
 			if (ret)
 				break;
+
+			if (likely(already_enabled))
+				continue;
+
+			ret = _enable_required_opp(dev, pd_dev, power_on);
+			if (ret)
+				break;
+		}
+
+		if (ret && !already_enabled) {
+			/* Rollback (skip current since it failed) */
+			for (i--; i >= 0; i--)
+				_enable_required_opp(dev, genpd_virt_devs[i],
+						     !power_on);
 		}
 	} else {
 		/* Scaling down? Set required OPPs in reverse order */
@@ -838,8 +875,26 @@ static int _set_required_opps(struct device *dev,
 			ret = _set_required_opp(dev, pd_dev, opp, i);
 			if (ret)
 				break;
+
+			if (likely(already_enabled))
+				continue;
+
+			ret = _enable_required_opp(dev, pd_dev, power_on);
+			if (ret)
+				break;
+		}
+
+		if (ret && !already_enabled) {
+			/* Rollback (skip current since it failed) */
+			for (i++; i < opp_table->required_opp_count; i++)
+				_enable_required_opp(dev, genpd_virt_devs[i],
+						     !power_on);
 		}
 	}
+
+	if (ret == 0 && !already_enabled)
+		opp_table->genpd_virt_enabled = power_on;
+
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
 
 	return ret;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e51646ff279e..01ad9e136cc8 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -188,6 +188,7 @@ struct opp_table {
 	struct device **genpd_virt_devs;
 	struct opp_table **required_opp_tables;
 	unsigned int required_opp_count;
+	bool genpd_virt_enabled;
 
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order
  2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
                   ` (2 preceding siblings ...)
  2020-07-30  8:01 ` [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core Stephan Gerhold
@ 2020-08-12  8:53 ` Ulf Hansson
  3 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2020-08-12  8:53 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Viresh Kumar, Rafael J. Wysocki, Kevin Hilman, Nishanth Menon,
	Stephen Boyd, Linux PM, Linux Kernel Mailing List, Niklas Cassel

On Thu, 30 Jul 2020 at 10:02, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> I'm trying to get CPR (Core Power Reduction, AVS) working for MSM8916 on mainline.
> Shortly said there are two power domains that must be scaled with the CPU OPP table:
>
>   - (VDD)MX
>   - CPR
>
> My idea for this was to add both as "required-opps" to the CPR OPP table
> and let the OPP core take care of all the scaling.
>
> There are two remaining problems that need to be addressed for that to work:
>
>   1. The power domains should be scaled down in reverse order
>      (MX, CPR when scaling up, CPR, MX when scaling down).
>   2. Something has to enable the virtual genpd devices to make the rpmpd driver
>      actually respect the performance states we vote for.
>
> Both issues were briefly discussed before (see links in the patches),
> but I think we did not agree on an exact solution yet. After some consideration,
> I thought it would be best to address these directly in the OPP core.
>
> However, note that this patch is RFC because it is just supposed to initiate
> discussion if alternative solutions would be better. :)

Ramping up since the holidays, so I might overlook something - but I
think your suggestion solution makes perfect sense to me.

>
> Stephan Gerhold (3):
>   opp: Reduce code duplication in _set_required_opps()
>   opp: Set required OPPs in reverse order when scaling down
>   opp: Power on (virtual) power domains managed by the OPP core
>
>  drivers/opp/core.c | 115 ++++++++++++++++++++++++++++++++++++---------
>  drivers/opp/opp.h  |   1 +
>  2 files changed, 93 insertions(+), 23 deletions(-)
>
> --
> 2.27.0

So, for the series:

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

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps() Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core Stephan Gerhold
2020-08-12  8:53 ` [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Ulf Hansson

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git