[RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order

[RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order

[Stephan Gerhold]

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

[RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()

[Stephan Gerhold]

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

[RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down

[Stephan Gerhold]

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

[RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

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

Re: [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order

[Ulf Hansson]

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

Re: [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down

[Stephan Gerhold]

Hi Viresh,

On Thu, Jul 30, 2020 at 10:01:45AM +0200, Stephan Gerhold wrote:
> 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>

This patch does not apply anymore after the cleanup you pushed to
opp/linux-next. I would be happy to send a v2 with that fixed.

On my other OPP patch set you mentioned that you might apply these
directly with some of your own changes - would you also prefer to do it
yourself in this case or should I send a v2?

Still looking for your feedback on both patch sets by the way! :)

Thanks!
Stephan

> ---
> 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
> 

Re: [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()

[Viresh Kumar]

On 30-07-20, 10:01, Stephan Gerhold wrote:
> 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;

Don't we need this check anymore ?

> +		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

-- 
viresh

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 30-07-20, 10:01, Stephan Gerhold wrote:
> 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 :)

This stuff was done ages back and I am starting to forget almost
everything now :)

Ulf, why doesn't pm_runtime_get(dev) take care of enabling multiple
power domain case ? RFP (request for patience) :)

-- 
viresh

Re: [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()

[Stephan Gerhold]

On Mon, Aug 24, 2020 at 04:48:20PM +0530, Viresh Kumar wrote:
> On 30-07-20, 10:01, Stephan Gerhold wrote:
> > 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;
> 
> Don't we need this check anymore ?
> 

You're right. Not sure why I removed it.

I suspect I had it in _set_required_opp() at some point, but I moved
code around several times until I was happy with the result.

We should just add it back.
Should I send a v2 with it fixed or would you like to handle it?

Thanks,
Stephan

> > +		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

Re: [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down

[Viresh Kumar]

On 21-08-20, 18:31, Stephan Gerhold wrote:
> This patch does not apply anymore after the cleanup you pushed to
> opp/linux-next. I would be happy to send a v2 with that fixed.
> 
> On my other OPP patch set you mentioned that you might apply these
> directly with some of your own changes - would you also prefer to do it
> yourself in this case or should I send a v2?

I will pick the first 2 myself, that's fine. Lets see where we go with
the third one :)

> Still looking for your feedback on both patch sets by the way! :)

Sorry about the delay, I was on vacation for over a week in between and
this and the other patchset was a bit tricky (which you may have not
realized, not sure, as I wondered if something will not work within
the OPP core for v1 binding, but it did finally I believe) :)

-- 
viresh

Re: [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down

[Stephan Gerhold]

On Mon, Aug 24, 2020 at 05:00:27PM +0530, Viresh Kumar wrote:
> On 21-08-20, 18:31, Stephan Gerhold wrote:
> > This patch does not apply anymore after the cleanup you pushed to
> > opp/linux-next. I would be happy to send a v2 with that fixed.
> > 
> > On my other OPP patch set you mentioned that you might apply these
> > directly with some of your own changes - would you also prefer to do it
> > yourself in this case or should I send a v2?
> 
> I will pick the first 2 myself, that's fine. Lets see where we go with
> the third one :)
> 

OK, please ignore my question in my reply to PATCH 1/3 then. I replied
before I read this one. Just add back the NULL checks and it should be
fine :)

> > Still looking for your feedback on both patch sets by the way! :)
> 
> Sorry about the delay, I was on vacation for over a week in between and
> this and the other patchset was a bit tricky (which you may have not
> realized, not sure, as I wondered if something will not work within
> the OPP core for v1 binding, but it did finally I believe) :)
> 

No problem! I guess I did indeed not realize potential problems for the
v1 bindings, all this compatibility code is quite confusing. :)

Thanks!
Stephan

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Mon, Aug 24, 2020 at 04:57:44PM +0530, Viresh Kumar wrote:
> On 30-07-20, 10:01, Stephan Gerhold wrote:
> > 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 :)
> 
> This stuff was done ages back and I am starting to forget almost
> everything now :)
> 
> Ulf, why doesn't pm_runtime_get(dev) take care of enabling multiple
> power domain case ? RFP (request for patience) :)
> 

So I'm really not an expert for power domains, but here is my
understanding:

We attach the power domains in dev_pm_opp_attach_genpd(opp_dev, names),
where opp_dev is the device the OPP table belongs to.

To do that, the genpd core creates a set of virtual devices. These
virtual devices are not related to opp_dev in any way. Therefore, the
power domains stay off until we run pm_runtime_get(virt_dev) for each of
the virtual devices. (Which is what is implemented in this patch...)

If I understand correctly, what you would like to do is to have a single
pm_runtime_get(opp_dev) call also enable all the virtual devices?

As far as I understand, this can be done by adding "device links"
between opp_dev and the virtual devices, e.g.

	device_link_add(opp_dev, virt_dev, DL_FLAG_PM_RUNTIME);

for each of the virtual devices.

But the problem with that approach is that it assumes that someone
actually calls pm_runtime_get(opp_dev), i.e. we assume that opp_dev is
managed by runtime PM. As far as I know, this isn't the case for the CPU
OPP table for example.

Maybe Ulf can correct me if I'm wrong :)

Thanks!
Stephan

Re: [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()

[Viresh Kumar]

On 24-08-20, 13:30, Stephan Gerhold wrote:
> You're right. Not sure why I removed it.
> 
> I suspect I had it in _set_required_opp() at some point, but I moved
> code around several times until I was happy with the result.
> 
> We should just add it back.
> Should I send a v2 with it fixed or would you like to handle it?

I have applied the first two patches to linux-next branch in my tree,
please have a look.

-- 
viresh

Re: [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps()

[Stephan Gerhold]

On Mon, Aug 24, 2020 at 05:40:04PM +0530, Viresh Kumar wrote:
> On 24-08-20, 13:30, Stephan Gerhold wrote:
> > You're right. Not sure why I removed it.
> > 
> > I suspect I had it in _set_required_opp() at some point, but I moved
> > code around several times until I was happy with the result.
> > 
> > We should just add it back.
> > Should I send a v2 with it fixed or would you like to handle it?
> 
> I have applied the first two patches to linux-next branch in my tree,
> please have a look.
> 

Looks good to me. Thank you!

Stephan

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Ulf Hansson]

On Mon, 24 Aug 2020 at 13:56, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Aug 24, 2020 at 04:57:44PM +0530, Viresh Kumar wrote:
> > On 30-07-20, 10:01, Stephan Gerhold wrote:
> > > 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 :)
> >
> > This stuff was done ages back and I am starting to forget almost
> > everything now :)
> >
> > Ulf, why doesn't pm_runtime_get(dev) take care of enabling multiple
> > power domain case ? RFP (request for patience) :)
> >
>
> So I'm really not an expert for power domains, but here is my
> understanding:
>
> We attach the power domains in dev_pm_opp_attach_genpd(opp_dev, names),
> where opp_dev is the device the OPP table belongs to.
>
> To do that, the genpd core creates a set of virtual devices. These
> virtual devices are not related to opp_dev in any way. Therefore, the
> power domains stay off until we run pm_runtime_get(virt_dev) for each of
> the virtual devices. (Which is what is implemented in this patch...)

Just to clarify. The reason why genpd creates virtual devices isn't
because of the opp table itself.

Instead this is because we can only attach one PM domain per device.
And since a device may have multiple PM domains, we need to create a
virtual device and per PM domain and attach that instead. Then it's up
to the caller to manage the virtual devices.

In some cases where the PM domains can be managed together, a device
link makes sense - while in others it doesn't.

>
> If I understand correctly, what you would like to do is to have a single
> pm_runtime_get(opp_dev) call also enable all the virtual devices?
>
> As far as I understand, this can be done by adding "device links"
> between opp_dev and the virtual devices, e.g.
>
>         device_link_add(opp_dev, virt_dev, DL_FLAG_PM_RUNTIME);
>
> for each of the virtual devices.

Yep.

>
> But the problem with that approach is that it assumes that someone
> actually calls pm_runtime_get(opp_dev), i.e. we assume that opp_dev is
> managed by runtime PM. As far as I know, this isn't the case for the CPU
> OPP table for example.
>
> Maybe Ulf can correct me if I'm wrong :)

If I understand correctly, the opp_dev is the actual consumer device.
It could represent an I/O controller for example, or a CPU in the CPU
freq case.

That said, perhaps should rely on the consumer to deploy runtime PM
support, but let the OPP core to set up the device links for the genpd
virtual devices!?

>
> Thanks!
> Stephan

Kind regards
Uffe

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> On Mon, 24 Aug 2020 at 13:56, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Mon, Aug 24, 2020 at 04:57:44PM +0530, Viresh Kumar wrote:
> > > On 30-07-20, 10:01, Stephan Gerhold wrote:
> > > > 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 :)
> > >
> > > This stuff was done ages back and I am starting to forget almost
> > > everything now :)
> > >
> > > Ulf, why doesn't pm_runtime_get(dev) take care of enabling multiple
> > > power domain case ? RFP (request for patience) :)
> > >
> >
> > So I'm really not an expert for power domains, but here is my
> > understanding:
> >
> > We attach the power domains in dev_pm_opp_attach_genpd(opp_dev, names),
> > where opp_dev is the device the OPP table belongs to.
> >
> > To do that, the genpd core creates a set of virtual devices. These
> > virtual devices are not related to opp_dev in any way. Therefore, the
> > power domains stay off until we run pm_runtime_get(virt_dev) for each of
> > the virtual devices. (Which is what is implemented in this patch...)
> 
> Just to clarify. The reason why genpd creates virtual devices isn't
> because of the opp table itself.
> 
> Instead this is because we can only attach one PM domain per device.
> And since a device may have multiple PM domains, we need to create a
> virtual device and per PM domain and attach that instead. Then it's up
> to the caller to manage the virtual devices.
> 
> In some cases where the PM domains can be managed together, a device
> link makes sense - while in others it doesn't.
> 
> >
> > If I understand correctly, what you would like to do is to have a single
> > pm_runtime_get(opp_dev) call also enable all the virtual devices?
> >
> > As far as I understand, this can be done by adding "device links"
> > between opp_dev and the virtual devices, e.g.
> >
> >         device_link_add(opp_dev, virt_dev, DL_FLAG_PM_RUNTIME);
> >
> > for each of the virtual devices.
> 
> Yep.
> 
> >
> > But the problem with that approach is that it assumes that someone
> > actually calls pm_runtime_get(opp_dev), i.e. we assume that opp_dev is
> > managed by runtime PM. As far as I know, this isn't the case for the CPU
> > OPP table for example.
> >
> > Maybe Ulf can correct me if I'm wrong :)
> 
> If I understand correctly, the opp_dev is the actual consumer device.
> It could represent an I/O controller for example, or a CPU in the CPU
> freq case.
> 

Exactly.

> That said, perhaps should rely on the consumer to deploy runtime PM
> support, but let the OPP core to set up the device links for the genpd
> virtual devices!?
> 

Yes, that would be the alternative option.
I would be fine with it as long as it also works for the CPUfreq case.

I don't think anything manages runtime PM for the CPU device, just
like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
power domain is essentially kept always-on (except for system suspend).
At least in my case this is intended.

If device links also keep the power domains on if the consumer device
does not make use of runtime PM it should work fine for my case.

Personally, I think my original patch (without device links) fits better
into the OPP API, for the following two reasons.

With device links:

  1. Unlike regulators/interconnects, attached power domains would be
     controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).

  2. ... some driver using OPP tables might not make use of runtime PM.
     In that case, the power domains would stay on the whole time,
     even if dev_pm_opp_set_rate(opp_dev, 0) was called.

With my patch, the power domain state is directly related to the
dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
relying on the runtime PM state in my opinion.

Stephan

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 24-08-20, 17:08, Stephan Gerhold wrote:
> On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > That said, perhaps should rely on the consumer to deploy runtime PM
> > support, but let the OPP core to set up the device links for the genpd
> > virtual devices!?
> > 
> 
> Yes, that would be the alternative option.

That is the right option IMO.

> I would be fine with it as long as it also works for the CPUfreq case.
> 
> I don't think anything manages runtime PM for the CPU device, just
> like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> power domain is essentially kept always-on (except for system suspend).
> At least in my case this is intended.
> 
> If device links also keep the power domains on if the consumer device
> does not make use of runtime PM it should work fine for my case.

With device link, you only need to do rpm enable/disable on the consumer device
and it will get propagated by itself.

> Personally, I think my original patch (without device links) fits better
> into the OPP API, for the following two reasons.
> 
> With device links:
> 
>   1. Unlike regulators/interconnects, attached power domains would be
>      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> 
>   2. ... some driver using OPP tables might not make use of runtime PM.
>      In that case, the power domains would stay on the whole time,
>      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> 
> With my patch, the power domain state is directly related to the
> dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> relying on the runtime PM state in my opinion.

So opp-set-rate isn't in the best of shape TBH, some things are left for the
drivers while other are done by it. Regulator-enable/disable was moved to it
some time back as people needed something like that. While on the other hand,
clk_enable/disable doesn't happen there, nor does rpm enable/disable.

Maybe one day we may want to do that, but lets make sure someone wants to do
that first.

Anyway, even in that case both of the changes would be required. We must make
device links nevertheless first. And later on if required, we may want to do rpm
enable/disable on the consumer device itself.

-- 
viresh

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Ulf Hansson]

On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-08-20, 17:08, Stephan Gerhold wrote:
> > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > support, but let the OPP core to set up the device links for the genpd
> > > virtual devices!?
> > >
> >
> > Yes, that would be the alternative option.
>
> That is the right option IMO.
>
> > I would be fine with it as long as it also works for the CPUfreq case.
> >
> > I don't think anything manages runtime PM for the CPU device, just
> > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > power domain is essentially kept always-on (except for system suspend).
> > At least in my case this is intended.
> >
> > If device links also keep the power domains on if the consumer device
> > does not make use of runtime PM it should work fine for my case.
>
> With device link, you only need to do rpm enable/disable on the consumer device
> and it will get propagated by itself.

Note that the default state for the genpd virtual device(s) is that
runtime PM has been enabled for them. This means it's left in runtime
suspended state, which allows its PM domain to be powered off (if all
other devices and child domains for it allow that too, of course).

>
> > Personally, I think my original patch (without device links) fits better
> > into the OPP API, for the following two reasons.
> >
> > With device links:
> >
> >   1. Unlike regulators/interconnects, attached power domains would be
> >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> >
> >   2. ... some driver using OPP tables might not make use of runtime PM.
> >      In that case, the power domains would stay on the whole time,
> >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> >
> > With my patch, the power domain state is directly related to the
> > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > relying on the runtime PM state in my opinion.
>
> So opp-set-rate isn't in the best of shape TBH, some things are left for the
> drivers while other are done by it. Regulator-enable/disable was moved to it
> some time back as people needed something like that. While on the other hand,
> clk_enable/disable doesn't happen there, nor does rpm enable/disable.
>
> Maybe one day we may want to do that, but lets make sure someone wants to do
> that first.
>
> Anyway, even in that case both of the changes would be required. We must make
> device links nevertheless first. And later on if required, we may want to do rpm
> enable/disable on the consumer device itself.

This sounds like a reasonable step-by-step approach.

Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
DL_FLAG_STATELESS.

But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
runtime resume the supplier (the genpd virtual device), is harder to
know - as that kind of depends on expectations by the consumer device
driver.

Kind regards
Uffe

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 24-08-20, 17:08, Stephan Gerhold wrote:
> > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > > support, but let the OPP core to set up the device links for the genpd
> > > > virtual devices!?
> > > >
> > >
> > > Yes, that would be the alternative option.
> >
> > That is the right option IMO.
> >
> > > I would be fine with it as long as it also works for the CPUfreq case.
> > >
> > > I don't think anything manages runtime PM for the CPU device, just
> > > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > > power domain is essentially kept always-on (except for system suspend).
> > > At least in my case this is intended.
> > >
> > > If device links also keep the power domains on if the consumer device
> > > does not make use of runtime PM it should work fine for my case.
> >
> > With device link, you only need to do rpm enable/disable on the consumer device
> > and it will get propagated by itself.
> 
> Note that the default state for the genpd virtual device(s) is that
> runtime PM has been enabled for them. This means it's left in runtime
> suspended state, which allows its PM domain to be powered off (if all
> other devices and child domains for it allow that too, of course).
> 
> >
> > > Personally, I think my original patch (without device links) fits better
> > > into the OPP API, for the following two reasons.
> > >
> > > With device links:
> > >
> > >   1. Unlike regulators/interconnects, attached power domains would be
> > >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> > >
> > >   2. ... some driver using OPP tables might not make use of runtime PM.
> > >      In that case, the power domains would stay on the whole time,
> > >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> > >
> > > With my patch, the power domain state is directly related to the
> > > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > > relying on the runtime PM state in my opinion.
> >
> > So opp-set-rate isn't in the best of shape TBH, some things are left for the
> > drivers while other are done by it. Regulator-enable/disable was moved to it
> > some time back as people needed something like that. While on the other hand,
> > clk_enable/disable doesn't happen there, nor does rpm enable/disable.
> >
> > Maybe one day we may want to do that, but lets make sure someone wants to do
> > that first.
> >
> > Anyway, even in that case both of the changes would be required. We must make
> > device links nevertheless first. And later on if required, we may want to do rpm
> > enable/disable on the consumer device itself.
> 
> This sounds like a reasonable step-by-step approach.
> 
> Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
> DL_FLAG_STATELESS.
> 

OK, I will give this a try later this week.

> But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
> runtime resume the supplier (the genpd virtual device), is harder to
> know - as that kind of depends on expectations by the consumer device
> driver.
> 

I'm not sure I understand the purpose of that flag. I thought we want to
link the PM state of the virtual genpd device (supplier) to the PM state
of the device of the OPP table (consumer).

Shouldn't it just determine the initial state based on the state of the
consumer device?

Thanks!
Stephan

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Ulf Hansson]

On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:
> > On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 24-08-20, 17:08, Stephan Gerhold wrote:
> > > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > > > support, but let the OPP core to set up the device links for the genpd
> > > > > virtual devices!?
> > > > >
> > > >
> > > > Yes, that would be the alternative option.
> > >
> > > That is the right option IMO.
> > >
> > > > I would be fine with it as long as it also works for the CPUfreq case.
> > > >
> > > > I don't think anything manages runtime PM for the CPU device, just
> > > > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > > > power domain is essentially kept always-on (except for system suspend).
> > > > At least in my case this is intended.
> > > >
> > > > If device links also keep the power domains on if the consumer device
> > > > does not make use of runtime PM it should work fine for my case.
> > >
> > > With device link, you only need to do rpm enable/disable on the consumer device
> > > and it will get propagated by itself.
> >
> > Note that the default state for the genpd virtual device(s) is that
> > runtime PM has been enabled for them. This means it's left in runtime
> > suspended state, which allows its PM domain to be powered off (if all
> > other devices and child domains for it allow that too, of course).
> >
> > >
> > > > Personally, I think my original patch (without device links) fits better
> > > > into the OPP API, for the following two reasons.
> > > >
> > > > With device links:
> > > >
> > > >   1. Unlike regulators/interconnects, attached power domains would be
> > > >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> > > >
> > > >   2. ... some driver using OPP tables might not make use of runtime PM.
> > > >      In that case, the power domains would stay on the whole time,
> > > >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> > > >
> > > > With my patch, the power domain state is directly related to the
> > > > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > > > relying on the runtime PM state in my opinion.
> > >
> > > So opp-set-rate isn't in the best of shape TBH, some things are left for the
> > > drivers while other are done by it. Regulator-enable/disable was moved to it
> > > some time back as people needed something like that. While on the other hand,
> > > clk_enable/disable doesn't happen there, nor does rpm enable/disable.
> > >
> > > Maybe one day we may want to do that, but lets make sure someone wants to do
> > > that first.
> > >
> > > Anyway, even in that case both of the changes would be required. We must make
> > > device links nevertheless first. And later on if required, we may want to do rpm
> > > enable/disable on the consumer device itself.
> >
> > This sounds like a reasonable step-by-step approach.
> >
> > Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
> > DL_FLAG_STATELESS.
> >
>
> OK, I will give this a try later this week.
>
> > But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
> > runtime resume the supplier (the genpd virtual device), is harder to
> > know - as that kind of depends on expectations by the consumer device
> > driver.
> >
>
> I'm not sure I understand the purpose of that flag. I thought we want to
> link the PM state of the virtual genpd device (supplier) to the PM state
> of the device of the OPP table (consumer).

Correct, but this is about synchronizing the initial runtime PM state
of the consumer and supplier.

>
> Shouldn't it just determine the initial state based on the state of the
> consumer device?

We could do that. Then something along the lines of the below, should work:

pm_runtime_get_noresume(consumer) - to prevent runtime suspend, temporarily.

if(pm_runtime_active(consumer))
    create links with DL_FLAG_RPM_ACTIVE
else
    create links without DL_FLAG_RPM_ACTIVE

pm_runtime_put(consumer)

Kind regards
Uffe

Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Tue, Aug 25, 2020 at 02:42:54PM +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:
> > > On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 24-08-20, 17:08, Stephan Gerhold wrote:
> > > > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > > > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > > > > support, but let the OPP core to set up the device links for the genpd
> > > > > > virtual devices!?
> > > > > >
> > > > >
> > > > > Yes, that would be the alternative option.
> > > >
> > > > That is the right option IMO.
> > > >
> > > > > I would be fine with it as long as it also works for the CPUfreq case.
> > > > >
> > > > > I don't think anything manages runtime PM for the CPU device, just
> > > > > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > > > > power domain is essentially kept always-on (except for system suspend).
> > > > > At least in my case this is intended.
> > > > >
> > > > > If device links also keep the power domains on if the consumer device
> > > > > does not make use of runtime PM it should work fine for my case.
> > > >
> > > > With device link, you only need to do rpm enable/disable on the consumer device
> > > > and it will get propagated by itself.
> > >
> > > Note that the default state for the genpd virtual device(s) is that
> > > runtime PM has been enabled for them. This means it's left in runtime
> > > suspended state, which allows its PM domain to be powered off (if all
> > > other devices and child domains for it allow that too, of course).
> > >
> > > >
> > > > > Personally, I think my original patch (without device links) fits better
> > > > > into the OPP API, for the following two reasons.
> > > > >
> > > > > With device links:
> > > > >
> > > > >   1. Unlike regulators/interconnects, attached power domains would be
> > > > >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> > > > >
> > > > >   2. ... some driver using OPP tables might not make use of runtime PM.
> > > > >      In that case, the power domains would stay on the whole time,
> > > > >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> > > > >
> > > > > With my patch, the power domain state is directly related to the
> > > > > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > > > > relying on the runtime PM state in my opinion.
> > > >
> > > > So opp-set-rate isn't in the best of shape TBH, some things are left for the
> > > > drivers while other are done by it. Regulator-enable/disable was moved to it
> > > > some time back as people needed something like that. While on the other hand,
> > > > clk_enable/disable doesn't happen there, nor does rpm enable/disable.
> > > >
> > > > Maybe one day we may want to do that, but lets make sure someone wants to do
> > > > that first.
> > > >
> > > > Anyway, even in that case both of the changes would be required. We must make
> > > > device links nevertheless first. And later on if required, we may want to do rpm
> > > > enable/disable on the consumer device itself.
> > >
> > > This sounds like a reasonable step-by-step approach.
> > >
> > > Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
> > > DL_FLAG_STATELESS.
> > >
> >
> > OK, I will give this a try later this week.
> >
> > > But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
> > > runtime resume the supplier (the genpd virtual device), is harder to
> > > know - as that kind of depends on expectations by the consumer device
> > > driver.
> > >
> >
> > I'm not sure I understand the purpose of that flag. I thought we want to
> > link the PM state of the virtual genpd device (supplier) to the PM state
> > of the device of the OPP table (consumer).
> 
> Correct, but this is about synchronizing the initial runtime PM state
> of the consumer and supplier.
> 
> >
> > Shouldn't it just determine the initial state based on the state of the
> > consumer device?
> 
> We could do that. Then something along the lines of the below, should work:
> 
> pm_runtime_get_noresume(consumer) - to prevent runtime suspend, temporarily.
> 
> if(pm_runtime_active(consumer))
>     create links with DL_FLAG_RPM_ACTIVE
> else
>     create links without DL_FLAG_RPM_ACTIVE
> 
> pm_runtime_put(consumer)
> 

This seems to work as expected, thanks for the suggestion!
I will submit a v2 with that shortly.

Thanks!
Stephan