diff mbox series

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

Message ID 20200730080146.25185-3-stephan@gerhold.net (mailing list archive)
State RFC, archived
Headers show
Series opp: required_opps: Power on genpd, scale down in reverse order | expand

Commit Message

Stephan Gerhold July 30, 2020, 8:01 a.m. UTC
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(-)

Comments

Stephan Gerhold Aug. 21, 2020, 4:31 p.m. UTC | #1
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
>
Viresh Kumar Aug. 24, 2020, 11:30 a.m. UTC | #2
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) :)
Stephan Gerhold Aug. 24, 2020, 11:42 a.m. UTC | #3
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
diff mbox series

Patch

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);
 	}