Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1504640 > unrolled thread

[PATCH V2 0/8] PM / OPP: Multiple regulator support

Back to article view | Back to linux.kernel

Contents

  [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
    [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
      Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to  _set_opp_voltage() Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 01:20 +0200
        Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to  _set_opp_voltage() Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-25 05:50 +0200
          Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to  _set_opp_voltage() Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 22:30 +0200
            Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to  _set_opp_voltage() Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-26 05:40 +0200
    [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
      Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu  protected section Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 01:00 +0200
        Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu  protected section Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-25 05:40 +0200
    [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
      Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 21:00 +0200
        Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-26 08:10 +0200
    [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
      Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom  opp_set_rate() callbacks Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 21:10 +0200
        Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom  opp_set_rate() callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-26 08:10 +0200
    [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 11:00 +0200
      Re: [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple  regulators per device Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 00:50 +0200
    Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-10-21 15:40 +0200
      Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-21 17:50 +0200
        Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support Dave Gerlach <d-gerlach@ti.com> - 2016-10-24 03:10 +0200
          Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-24 06:30 +0200
            Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support Dave Gerlach <d-gerlach@ti.com> - 2016-10-25 23:20 +0200
              Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-26 05:30 +0200

Page 1 of 2  [1] 2  Next page →

#1504640 — [PATCH V2 0/8] PM / OPP: Multiple regulator support

Hi,

Some platforms (like TI) have complex DVFS configuration for CPU
devices, where multiple regulators are required to be configured to
change DVFS state of the device. This was explained well by Nishanth
earlier [1].

Some thoughts went into it few months back but then it all got lost. I
am trying to get that back on track with this thread.

One of the major complaints around multiple regulators case was that the
DT isn't responsible in any way to represent the ordering in which
multiple supplies need to be programmed, before or after frequency
change. It was considered in this patch and such information is left to
the platform specific OPP driver now, which can register its own
opp_set_rate() callback with the OPP core and the OPP core will then
call it during DVFS.

The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
and code to pass values for multiple regulators and verified that they
are all properly read by the kernel (using debugfs interface).

Though more testing on real (TI) platforms would be useful.

This is rebased over: linux-next branch in the PM tree.

V1->V2:
- Ack from Rob for 1st patch
- Moved the supplies structure to pm_opp.h (Dave)
- Fixed an compilation warning.

--
viresh

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

Viresh Kumar (8):
  PM / OPP: Reword binding supporting multiple regulators per device
  PM / OPP: Don't use OPP structure outside of rcu protected section
  PM / OPP: Manage supply's voltage/current in a separate structure
  PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
  PM / OPP: Add infrastructure to manage multiple regulators
  PM / OPP: Separate out _generic_opp_set_rate()
  PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()

 Documentation/devicetree/bindings/opp/opp.txt |  25 +-
 drivers/base/power/opp/core.c                 | 501 ++++++++++++++++++++------
 drivers/base/power/opp/debugfs.c              |  48 ++-
 drivers/base/power/opp/of.c                   | 104 ++++--
 drivers/base/power/opp/opp.h                  |  20 +-
 drivers/cpufreq/cpufreq-dt.c                  |   9 +-
 include/linux/pm_opp.h                        |  49 ++-
 7 files changed, 574 insertions(+), 182 deletions(-)

-- 
2.7.1.410.g6faf27b

[toc] | [next] | [standalone]

#1504644 — [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

Pass the entire supply structure instead of all of its fields.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 44 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 8d6006151c9a..37fad2eb0f47 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -542,8 +542,7 @@ static struct clk *_get_opp_clk(struct device *dev)
 }
 
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
-			    unsigned long u_volt, unsigned long u_volt_min,
-			    unsigned long u_volt_max)
+			    struct dev_pm_opp_supply *supply)
 {
 	int ret;
 
@@ -554,14 +553,15 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 		return 0;
 	}
 
-	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, u_volt_min,
-		u_volt, u_volt_max);
+	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+		supply->u_volt_min, supply->u_volt, supply->u_volt_max);
 
-	ret = regulator_set_voltage_triplet(reg, u_volt_min, u_volt,
-					    u_volt_max);
+	ret = regulator_set_voltage_triplet(reg, supply->u_volt_min,
+					    supply->u_volt, supply->u_volt_max);
 	if (ret)
 		dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
-			__func__, u_volt_min, u_volt, u_volt_max, ret);
+			__func__, supply->u_volt_min, supply->u_volt,
+			supply->u_volt_max, ret);
 
 	return ret;
 }
@@ -583,8 +583,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct regulator *reg;
 	struct clk *clk;
 	unsigned long freq, old_freq;
-	unsigned long u_volt, u_volt_min, u_volt_max;
-	unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
+	struct dev_pm_opp_supply old_supply, new_supply;
 	int ret;
 
 	if (unlikely(!target_freq)) {
@@ -634,17 +633,12 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
-	if (IS_ERR(old_opp)) {
-		old_u_volt = 0;
-	} else {
-		old_u_volt = old_opp->supply.u_volt;
-		old_u_volt_min = old_opp->supply.u_volt_min;
-		old_u_volt_max = old_opp->supply.u_volt_max;
-	}
+	if (IS_ERR(old_opp))
+		old_supply.u_volt = 0;
+	else
+		memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
 
-	u_volt = opp->supply.u_volt;
-	u_volt_min = opp->supply.u_volt_min;
-	u_volt_max = opp->supply.u_volt_max;
+	memcpy(&new_supply, &opp->supply, sizeof(new_supply));
 
 	reg = opp_table->regulator;
 
@@ -652,8 +646,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling up? Scale voltage before frequency */
 	if (freq > old_freq) {
-		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
-				       u_volt_max);
+		ret = _set_opp_voltage(dev, reg, &new_supply);
 		if (ret)
 			goto restore_voltage;
 	}
@@ -672,8 +665,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling down? Scale voltage after frequency */
 	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
-				       u_volt_max);
+		ret = _set_opp_voltage(dev, reg, &new_supply);
 		if (ret)
 			goto restore_freq;
 	}
@@ -686,10 +678,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_u_volt) {
-		_set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
-				 old_u_volt_max);
-	}
+	if (old_supply.u_volt)
+		_set_opp_voltage(dev, reg, &old_supply);
 
 	return ret;
 }
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [next] | [standalone]

#1507820 — Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

On 10/20, Viresh Kumar wrote:
> Pass the entire supply structure instead of all of its fields.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

This patch should be combined with the previous one. I'm still
not sure if it even makes sense to do this though. Do we really
have to make duplicate "OPP snapshot" structures just because of
how OPPs use RCU?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1507952 — Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

On 24-10-16, 16:14, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > Pass the entire supply structure instead of all of its fields.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> This patch should be combined with the previous one.

I think it is a fair to do this separately as this is a completely different
logical change.

> I'm still
> not sure if it even makes sense to do this though.

:)

> Do we really
> have to make duplicate "OPP snapshot" structures just because of
> how OPPs use RCU?

I agree. With RCU, yes this change is probably required. But I am not sure if
RCU fits that well to OPP core anymore. A rw-lock may be much easier to help.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1508605 — Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

On 10/25, Viresh Kumar wrote:
> On 24-10-16, 16:14, Stephen Boyd wrote:
> > On 10/20, Viresh Kumar wrote:
> > > Pass the entire supply structure instead of all of its fields.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > 
> > This patch should be combined with the previous one.
> 
> I think it is a fair to do this separately as this is a completely different
> logical change.

Let's agree to disagree.

> 
> > I'm still
> > not sure if it even makes sense to do this though.
> 
> :)
> 
> > Do we really
> > have to make duplicate "OPP snapshot" structures just because of
> > how OPPs use RCU?
> 
> I agree. With RCU, yes this change is probably required. But I am not sure if
> RCU fits that well to OPP core anymore. A rw-lock may be much easier to help.
> 

For things like AVS we'll probably want to do that, although it's
sort of funny because replacing RCU with rw-locks is the opposite
direction most people go. With AVS we would be updating the
voltage(s) in use for the current OPP, and we would want that
update to block any OPP transition until the voltage is adjusted.
I don't know how we would do that with RCU very well. Plus, RCU
is for reader heavy things, but we mostly have one or two
readers.

I guess it's ok for now to do all this copying, but it feels like
we'll need to undo a large portion of it later with things like
AVS. Or at least we'll be doing copies for almost no reason
because we'll want to hold the read lock across the whole OPP
transition. I was going to suggest we pass around information
about what we want to grab from the RCU protected data
structures, think index of regulator, etc. and then have small
RCU read-side critical sections to grab that info during the OPP
transition but I'm not sure that's any better. It might be worse
because the OPP could change during the OPP transition and we
could be using half of the old and half of the new data.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1508777 — Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

On 25-10-16, 13:26, Stephen Boyd wrote:
> For things like AVS we'll probably want to do that, although it's
> sort of funny because replacing RCU with rw-locks is the opposite
> direction most people go.

Yes, that would be very funny :)

> With AVS we would be updating the
> voltage(s) in use for the current OPP, and we would want that
> update to block any OPP transition until the voltage is adjusted.
> I don't know how we would do that with RCU very well. Plus, RCU
> is for reader heavy things, but we mostly have one or two
> readers.

Not just that, think of opp_disable() function. What guarantees currently that
an OPP being disabled isn't already used right now? Or is on the way of getting
used?

I strongly feel RCU is not the best fit for OPP core at least.

> I guess it's ok for now to do all this copying, but it feels like
> we'll need to undo a large portion of it later with things like
> AVS.

Yes.

> Or at least we'll be doing copies for almost no reason
> because we'll want to hold the read lock across the whole OPP
> transition. I was going to suggest we pass around information
> about what we want to grab from the RCU protected data
> structures, think index of regulator, etc. and then have small
> RCU read-side critical sections to grab that info during the OPP
> transition but I'm not sure that's any better. It might be worse
> because the OPP could change during the OPP transition and we
> could be using half of the old and half of the new data.

The problem is that this code is getting harder to read for everybody. If we are
finding it difficult to understand, what about newbies..

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1504647 — [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section

The OPP structure must not be used out of the rcu protected section.
Cache the values to be used in separate variables instead.

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

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..056527a3fb4e 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -584,6 +584,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct clk *clk;
 	unsigned long freq, old_freq;
 	unsigned long u_volt, u_volt_min, u_volt_max;
+	unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
 	int ret;
 
 	if (unlikely(!target_freq)) {
@@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
+	if (IS_ERR(old_opp)) {
+		old_u_volt = 0;
+	} else {
+		old_u_volt = old_opp->u_volt;
+		old_u_volt_min = old_opp->u_volt_min;
+		old_u_volt_max = old_opp->u_volt_max;
+	}
+
 	u_volt = opp->u_volt;
 	u_volt_min = opp->u_volt_min;
 	u_volt_max = opp->u_volt_max;
@@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (!IS_ERR(old_opp))
-		_set_opp_voltage(dev, reg, old_opp->u_volt,
-				 old_opp->u_volt_min, old_opp->u_volt_max);
+	if (old_u_volt) {
+		_set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
+				 old_u_volt_max);
+	}
 
 	return ret;
 }
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [next] | [standalone]

#1507815 — Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section

On 10/20, Viresh Kumar wrote:
> The OPP structure must not be used out of the rcu protected section.
> Cache the values to be used in separate variables instead.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Was this found by visual inspection or through some static
checker? Just curious.

> @@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		return ret;
>  	}
>  
> +	if (IS_ERR(old_opp)) {
> +		old_u_volt = 0;
> +	} else {
> +		old_u_volt = old_opp->u_volt;
> +		old_u_volt_min = old_opp->u_volt_min;
> +		old_u_volt_max = old_opp->u_volt_max;
> +	}
> +
>  	u_volt = opp->u_volt;
>  	u_volt_min = opp->u_volt_min;
>  	u_volt_max = opp->u_volt_max;
> @@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  			__func__, old_freq);
>  restore_voltage:
>  	/* This shouldn't harm even if the voltages weren't updated earlier */
> -	if (!IS_ERR(old_opp))
> -		_set_opp_voltage(dev, reg, old_opp->u_volt,
> -				 old_opp->u_volt_min, old_opp->u_volt_max);
> +	if (old_u_volt) {

What if old_u_volt == 0 is valid? We could have another variable
like 'valid' or something that we use to figure out if we should
set values instead. Then this isn't a potential pitfall.

> +		_set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
> +				 old_u_volt_max);
> +	}
>  
>  	return ret;
>  }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1507947 — Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section

On 24-10-16, 15:52, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > The OPP structure must not be used out of the rcu protected section.
> > Cache the values to be used in separate variables instead.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Was this found by visual inspection or through some static
> checker? Just curious.

Visual inspection :)

> > @@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  		return ret;
> >  	}
> >  
> > +	if (IS_ERR(old_opp)) {
> > +		old_u_volt = 0;
> > +	} else {
> > +		old_u_volt = old_opp->u_volt;
> > +		old_u_volt_min = old_opp->u_volt_min;
> > +		old_u_volt_max = old_opp->u_volt_max;
> > +	}
> > +
> >  	u_volt = opp->u_volt;
> >  	u_volt_min = opp->u_volt_min;
> >  	u_volt_max = opp->u_volt_max;
> > @@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  			__func__, old_freq);
> >  restore_voltage:
> >  	/* This shouldn't harm even if the voltages weren't updated earlier */
> > -	if (!IS_ERR(old_opp))
> > -		_set_opp_voltage(dev, reg, old_opp->u_volt,
> > -				 old_opp->u_volt_min, old_opp->u_volt_max);
> > +	if (old_u_volt) {
> 
> What if old_u_volt == 0 is valid?

How can that be valid ?

> We could have another variable
> like 'valid' or something that we use to figure out if we should
> set values instead. Then this isn't a potential pitfall.

I can do that but just wanted to know if we need that or not.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1504648 — [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()

Later patches would add support for custom opp_set_rate callbacks. This
patch separates out the code for generic opp_set_rate handler in order
to prepare for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 181 +++++++++++++++++++++++++++++-------------
 drivers/base/power/opp/opp.h  |  18 +----
 include/linux/pm_opp.h        |  31 ++++++++
 3 files changed, 161 insertions(+), 69 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 45c70ce07864..96f04392daef 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -596,6 +596,73 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
+static inline int
+_generic_opp_set_rate_clk_only(struct device *dev, struct clk *clk,
+			       unsigned long old_freq, unsigned long freq)
+{
+	int ret;
+
+	/* Change frequency */
+	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
+		__func__, old_freq, freq);
+
+	ret = clk_set_rate(clk, freq);
+	if (ret) {
+		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+			ret);
+	}
+
+	return ret;
+}
+
+static int _generic_opp_set_rate(struct device *dev,
+				 struct dev_pm_set_rate_data *data)
+{
+	struct dev_pm_opp_supply *old_supply = data->old_opp.supplies;
+	struct dev_pm_opp_supply *new_supply = data->new_opp.supplies;
+	unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
+	struct regulator *reg = data->regulators[0];
+	int ret;
+
+	/* This function only supports single regulator per device */
+	if (WARN_ON(data->regulator_count > 1)) {
+		dev_err(dev, "multiple regulators are not supported\n");
+		return -EINVAL;
+	}
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		ret = _set_opp_voltage(dev, reg, new_supply);
+		if (ret)
+			goto restore_voltage;
+	}
+
+	/* Change frequency */
+	ret = _generic_opp_set_rate_clk_only(dev, data->clk, old_freq, freq);
+	if (ret)
+		goto restore_voltage;
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		ret = _set_opp_voltage(dev, reg, new_supply);
+		if (ret)
+			goto restore_freq;
+	}
+
+	return 0;
+
+restore_freq:
+	if (_generic_opp_set_rate_clk_only(dev, data->clk, freq, old_freq))
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_voltage:
+	/* This shouldn't harm even if the voltages weren't updated earlier */
+	if (old_supply->u_volt)
+		_set_opp_voltage(dev, reg, old_supply);
+
+	return ret;
+}
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
@@ -609,12 +676,12 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
+	unsigned long freq, old_freq;
 	struct dev_pm_opp *old_opp, *opp;
-	struct regulator *reg = ERR_PTR(-ENXIO);
+	struct regulator **regulators;
+	struct dev_pm_set_rate_data *data;
 	struct clk *clk;
-	unsigned long freq, old_freq;
-	struct dev_pm_opp_supply old_supply, new_supply;
-	int ret;
+	int ret, size;
 
 	if (unlikely(!target_freq)) {
 		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
@@ -663,64 +730,32 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
-	if (opp_table->regulators) {
-		/* This function only supports single regulator per device */
-		if (WARN_ON(opp_table->regulator_count > 1)) {
-			dev_err(dev, "multiple regulators not supported\n");
-			rcu_read_unlock();
-			return -EINVAL;
-		}
+	regulators = opp_table->regulators;
 
-		reg = opp_table->regulators[0];
+	/* Only frequency scaling */
+	if (!regulators) {
+		rcu_read_unlock();
+		return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
 	}
 
+	data = opp_table->set_rate_data;
+	data->regulators = regulators;
+	data->regulator_count = opp_table->regulator_count;
+	data->clk = clk;
+
+	data->old_opp.rate = old_freq;
+	size = sizeof(*opp->supplies) * opp_table->regulator_count;
 	if (IS_ERR(old_opp))
-		old_supply.u_volt = 0;
+		memset(data->old_opp.supplies, 0, size);
 	else
-		memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
+		memcpy(data->old_opp.supplies, old_opp->supplies, size);
 
-	memcpy(&new_supply, opp->supplies, sizeof(new_supply));
+	data->new_opp.rate = freq;
+	memcpy(data->new_opp.supplies, opp->supplies, size);
 
 	rcu_read_unlock();
 
-	/* Scaling up? Scale voltage before frequency */
-	if (freq > old_freq) {
-		ret = _set_opp_voltage(dev, reg, &new_supply);
-		if (ret)
-			goto restore_voltage;
-	}
-
-	/* Change frequency */
-
-	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
-		__func__, old_freq, freq);
-
-	ret = clk_set_rate(clk, freq);
-	if (ret) {
-		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
-			ret);
-		goto restore_voltage;
-	}
-
-	/* Scaling down? Scale voltage after frequency */
-	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, &new_supply);
-		if (ret)
-			goto restore_freq;
-	}
-
-	return 0;
-
-restore_freq:
-	if (clk_set_rate(clk, old_freq))
-		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
-restore_voltage:
-	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_supply.u_volt)
-		_set_opp_voltage(dev, reg, &old_supply);
-
-	return ret;
+	return _generic_opp_set_rate(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -1353,6 +1388,38 @@ void dev_pm_opp_put_prop_name(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
+static int _allocate_set_rate_data(struct opp_table *opp_table)
+{
+	struct dev_pm_set_rate_data *data;
+	int len, count = opp_table->regulator_count;
+
+	if (WARN_ON(!count))
+		return -EINVAL;
+
+	/* space for set_rate_data */
+	len = sizeof(*data);
+
+	/* space for old_opp.supplies and new_opp.supplies */
+	len += 2 * sizeof(struct dev_pm_opp_supply) * count;
+
+	data = kzalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->old_opp.supplies = (void *)(data + 1);
+	data->new_opp.supplies = data->old_opp.supplies + count;
+
+	opp_table->set_rate_data = data;
+
+	return 0;
+}
+
+static void _free_set_rate_data(struct opp_table *opp_table)
+{
+	kfree(opp_table->set_rate_data);
+	opp_table->set_rate_data = NULL;
+}
+
 /**
  * dev_pm_opp_set_regulators() - Set regulator names for the device
  * @dev: Device for which regulator name is being set.
@@ -1420,6 +1487,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
 
 	opp_table->regulator_count = count;
 
+	/* Allocate block only once to pass to ->set_rate() */
+	ret = _allocate_set_rate_data(opp_table);
+	if (ret)
+		goto free_regulators;
+
 	mutex_unlock(&opp_table_lock);
 	return 0;
 
@@ -1429,6 +1501,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
 
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
+	opp_table->regulator_count = 0;
 err:
 	_remove_opp_table(opp_table);
 unlock:
@@ -1474,6 +1547,8 @@ void dev_pm_opp_put_regulators(struct device *dev)
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
 		regulator_put(opp_table->regulators[i]);
 
+	_free_set_rate_data(opp_table);
+
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
 	opp_table->regulator_count = 0;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index d3f0861f9bff..6629c53c0aa1 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -47,22 +47,6 @@ extern struct list_head opp_tables;
  */
 
 /**
- * struct dev_pm_opp_supply - Power supply voltage/current values
- * @u_volt:	Target voltage in microvolts corresponding to this OPP
- * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
- * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
- * @u_amp:	Maximum current drawn by the device in microamperes
- *
- * This structure stores the voltage/current values for a single power supply.
- */
-struct dev_pm_opp_supply {
-	unsigned long u_volt;
-	unsigned long u_volt_min;
-	unsigned long u_volt_max;
-	unsigned long u_amp;
-};
-
-/**
  * struct dev_pm_opp - Generic OPP description structure
  * @node:	opp table node. The nodes are maintained throughout the lifetime
  *		of boot. It is expected only an optimal set of OPPs are
@@ -194,6 +178,8 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	struct dev_pm_set_rate_data *set_rate_data;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0606b70a8b97..73713a8424b1 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/notifier.h>
 
+struct clk;
 struct dev_pm_opp;
 struct device;
 
@@ -24,6 +25,36 @@ enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
 };
 
+/**
+ * struct dev_pm_opp_supply - Power supply voltage/current values
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp:	Maximum current drawn by the device in microamperes
+ *
+ * This structure stores the voltage/current values for a single power supply.
+ */
+struct dev_pm_opp_supply {
+	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long u_amp;
+};
+
+struct dev_pm_opp_info {
+	unsigned long rate;
+	struct dev_pm_opp_supply *supplies;
+};
+
+struct dev_pm_set_rate_data {
+	struct dev_pm_opp_info old_opp;
+	struct dev_pm_opp_info new_opp;
+
+	struct regulator **regulators;
+	unsigned int regulator_count;
+	struct clk *clk;
+};
+
 #if defined(CONFIG_PM_OPP)
 
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [next] | [standalone]

#1508558 — Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()

On 10/20, Viresh Kumar wrote:
> Later patches would add support for custom opp_set_rate callbacks. This

I know the OPP consumer function has "rate" in the name, but it
makes more sense to call the callback set_opp instead because we
could be doing a lot more than setting the opp rate.

> patch separates out the code for generic opp_set_rate handler in order
> to prepare for that.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 45c70ce07864..96f04392daef 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -596,6 +596,73 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
>  	return ret;
>  }
>  
> +static inline int
> +_generic_opp_set_rate_clk_only(struct device *dev, struct clk *clk,
> +			       unsigned long old_freq, unsigned long freq)
> +{
> +	int ret;
> +
> +	/* Change frequency */
> +	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
> +		__func__, old_freq, freq);

Perhaps this should stay at the beginning of OPP transitions?
Otherwise it can get confusing when multiple switching OPP
messages appear on OPP transition failures.

> +
> +	ret = clk_set_rate(clk, freq);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
> +			ret);
> +	}
> +
> +	return ret;
> +}
> +
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index d3f0861f9bff..6629c53c0aa1 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -47,22 +47,6 @@ extern struct list_head opp_tables;
>   */
>  
>  /**
> - * struct dev_pm_opp_supply - Power supply voltage/current values
> - * @u_volt:	Target voltage in microvolts corresponding to this OPP
> - * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
> - * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
> - * @u_amp:	Maximum current drawn by the device in microamperes
> - *
> - * This structure stores the voltage/current values for a single power supply.
> - */
> -struct dev_pm_opp_supply {
> -	unsigned long u_volt;
> -	unsigned long u_volt_min;
> -	unsigned long u_volt_max;
> -	unsigned long u_amp;
> -};
> -
> -/**
>   * struct dev_pm_opp - Generic OPP description structure
>   * @node:	opp table node. The nodes are maintained throughout the lifetime
>   *		of boot. It is expected only an optimal set of OPPs are
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0606b70a8b97..73713a8424b1 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -17,6 +17,7 @@
>  #include <linux/err.h>
>  #include <linux/notifier.h>
>  
> +struct clk;

Is struct regulator also forward declared?

>  struct dev_pm_opp;
>  struct device;
>  
> @@ -24,6 +25,36 @@ enum dev_pm_opp_event {
>  	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
>  };
>  
> +/**
> + * struct dev_pm_opp_supply - Power supply voltage/current values
> + * @u_volt:	Target voltage in microvolts corresponding to this OPP
> + * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
> + * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
> + * @u_amp:	Maximum current drawn by the device in microamperes
> + *
> + * This structure stores the voltage/current values for a single power supply.
> + */
> +struct dev_pm_opp_supply {
> +	unsigned long u_volt;
> +	unsigned long u_volt_min;
> +	unsigned long u_volt_max;
> +	unsigned long u_amp;
> +};

This structure moved during this series. Can we avoid that and
already have it in the right place to begin with?

> +
> +struct dev_pm_opp_info {
> +	unsigned long rate;
> +	struct dev_pm_opp_supply *supplies;
> +};
> +
> +struct dev_pm_set_rate_data {

dev_pm_set_opp_data?

> +	struct dev_pm_opp_info old_opp;
> +	struct dev_pm_opp_info new_opp;
> +
> +	struct regulator **regulators;
> +	unsigned int regulator_count;
> +	struct clk *clk;
> +};

The above two structures don't get kernel doc?

> +
>  #if defined(CONFIG_PM_OPP)
>  
>  unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
> -- 
> 2.7.1.410.g6faf27b
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1508809 — Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()

On 25-10-16, 11:59, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > Later patches would add support for custom opp_set_rate callbacks. This
> 
> I know the OPP consumer function has "rate" in the name, but it
> makes more sense to call the callback set_opp instead because we
> could be doing a lot more than setting the opp rate.

Done.

> > patch separates out the code for generic opp_set_rate handler in order
> > to prepare for that.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> > index 45c70ce07864..96f04392daef 100644
> > --- a/drivers/base/power/opp/core.c
> > +++ b/drivers/base/power/opp/core.c
> > @@ -596,6 +596,73 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
> >  	return ret;
> >  }
> >  
> > +static inline int
> > +_generic_opp_set_rate_clk_only(struct device *dev, struct clk *clk,
> > +			       unsigned long old_freq, unsigned long freq)
> > +{
> > +	int ret;
> > +
> > +	/* Change frequency */
> > +	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
> > +		__func__, old_freq, freq);
> 
> Perhaps this should stay at the beginning of OPP transitions?
> Otherwise it can get confusing when multiple switching OPP
> messages appear on OPP transition failures.

Done.

> > +struct clk;
> 
> Is struct regulator also forward declared?

Done now.

> >  struct dev_pm_opp;
> >  struct device;
> >  
> > @@ -24,6 +25,36 @@ enum dev_pm_opp_event {
> >  	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
> >  };
> >  
> > +/**
> > + * struct dev_pm_opp_supply - Power supply voltage/current values
> > + * @u_volt:	Target voltage in microvolts corresponding to this OPP
> > + * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
> > + * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
> > + * @u_amp:	Maximum current drawn by the device in microamperes
> > + *
> > + * This structure stores the voltage/current values for a single power supply.
> > + */
> > +struct dev_pm_opp_supply {
> > +	unsigned long u_volt;
> > +	unsigned long u_volt_min;
> > +	unsigned long u_volt_max;
> > +	unsigned long u_amp;
> > +};
> 
> This structure moved during this series. Can we avoid that and
> already have it in the right place to begin with?

Done.

> > +
> > +struct dev_pm_opp_info {
> > +	unsigned long rate;
> > +	struct dev_pm_opp_supply *supplies;
> > +};
> > +
> > +struct dev_pm_set_rate_data {
> 
> dev_pm_set_opp_data?

Done.

> > +	struct dev_pm_opp_info old_opp;
> > +	struct dev_pm_opp_info new_opp;
> > +
> > +	struct regulator **regulators;
> > +	unsigned int regulator_count;
> > +	struct clk *clk;
> > +};
> 
> The above two structures don't get kernel doc?

Done.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1504650 — [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks

The generic opp_set_rate() handler isn't sufficient for platforms with
complex DVFS.  For example, some TI platforms have multiple regulators
for a CPU device. The order in which various supplies need to be
programmed is only known to the platform code and its best to leave it
to it.

This patch implements APIs to register platform specific opp_set_rate()
callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/opp/opp.h  |   1 +
 include/linux/pm_opp.h        |  10 ++++
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 96f04392daef..f7c4ef194aac 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -677,6 +677,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
 	unsigned long freq, old_freq;
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_opp *old_opp, *opp;
 	struct regulator **regulators;
 	struct dev_pm_set_rate_data *data;
@@ -738,6 +739,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
 	}
 
+	if (opp_table->set_rate)
+		set_rate = opp_table->set_rate;
+	else
+		set_rate = _generic_opp_set_rate;
+
 	data = opp_table->set_rate_data;
 	data->regulators = regulators;
 	data->regulator_count = opp_table->regulator_count;
@@ -755,7 +761,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	rcu_read_unlock();
 
-	return _generic_opp_set_rate(dev, data);
+	return set_rate(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -889,6 +895,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->regulators)
 		return;
 
+	if (opp_table->set_rate)
+		return;
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
@@ -1562,6 +1571,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
 
 /**
+ * dev_pm_opp_register_set_rate_helper() - Register custom OPP set rate helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_rate: Custom OPP set rate helper.
+ *
+ * This is useful to support complex platforms (like platforms with multiple
+ * regulators per device), instead of the generic OPP set rate helper.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_register_set_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	if (!set_rate)
+		return -EINVAL;
+
+	mutex_lock(&opp_table_lock);
+
+	opp_table = _add_opp_table(dev);
+	if (!opp_table) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	/* This should be called before OPPs are initialized */
+	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	/* Already have custom set_rate helper */
+	if (WARN_ON(opp_table->set_rate)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	opp_table->set_rate = set_rate;
+
+	mutex_unlock(&opp_table_lock);
+	return 0;
+
+err:
+	_remove_opp_table(opp_table);
+unlock:
+	mutex_unlock(&opp_table_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_rate_helper);
+
+/**
+ * dev_pm_opp_register_put_rate_helper() - Releases resources blocked for
+ *					   set_rate helper
+ * @dev: Device for which custom set_rate helper has to be cleared.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_register_put_rate_helper(struct device *dev)
+{
+	struct opp_table *opp_table;
+
+	mutex_lock(&opp_table_lock);
+
+	/* Check for existing table for 'dev' first */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "Failed to find opp_table: %ld\n",
+			PTR_ERR(opp_table));
+		goto unlock;
+	}
+
+	if (!opp_table->set_rate) {
+		dev_err(dev, "%s: Doesn't have custom set_rate helper set\n",
+			__func__);
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating opp_table */
+	WARN_ON(!list_empty(&opp_table->opp_list));
+
+	opp_table->set_rate = NULL;
+
+	/* Try freeing opp_table if this was the last blocking resource */
+	_remove_opp_table(opp_table);
+
+unlock:
+	mutex_unlock(&opp_table_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_rate_helper);
+
+/**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 6629c53c0aa1..86e31f06ca64 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -178,6 +178,7 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_set_rate_data *set_rate_data;
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 73713a8424b1..ce070257d6eb 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -95,6 +95,8 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
 int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count);
 void dev_pm_opp_put_regulators(struct device *dev);
+int dev_pm_opp_register_set_rate_helper(struct device *dev, int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data));
+void dev_pm_opp_register_put_rate_helper(struct device *dev);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -194,6 +196,14 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
 
 static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
 
+static inline int dev_pm_opp_register_set_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	return -ENOTSUPP;
+}
+
+static inline void dev_pm_opp_register_put_rate_helper(struct device *dev) {}
+
 static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 {
 	return -ENOTSUPP;
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [next] | [standalone]

#1508564 — Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks

On 10/20, Viresh Kumar wrote:
> The generic opp_set_rate() handler isn't sufficient for platforms with
> complex DVFS.  For example, some TI platforms have multiple regulators
> for a CPU device. The order in which various supplies need to be
> programmed is only known to the platform code and its best to leave it
> to it.
> 
> This patch implements APIs to register platform specific opp_set_rate()
> callback.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Overall it looks ok, but I'd prefer set_opp instead of set_rate.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1508812 — Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks

On 25-10-16, 12:01, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > The generic opp_set_rate() handler isn't sufficient for platforms with
> > complex DVFS.  For example, some TI platforms have multiple regulators
> > for a CPU device. The order in which various supplies need to be
> > programmed is only known to the platform code and its best to leave it
> > to it.
> > 
> > This patch implements APIs to register platform specific opp_set_rate()
> > callback.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> Overall it looks ok, but I'd prefer set_opp instead of set_rate.

Done.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1504657 — [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device

On certain platforms (like TI), DVFS for a single device (CPU) requires
configuring multiple power supplies.

The OPP bindings already contains binding and example to explain this
case, but it isn't sufficient. For example, there is no way for the code
parsing these bindings to know which voltage values belong to which
power supply. Also its not possible to know the order in which the
supplies need to be configured while switching OPPs.

This patch tries to clarify on those details and does some minor changes
as well.

Note that the bindings do not specify the order in which the regulators
need to be programmed and the order in which the entries are added for
the supplies.

The user of the bindings (like the kernel) shall know these details
already and the DT is responsible to supply only the readings for the
regulators.

Cc: Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index ee91cbdd95ee..af476df510f1 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -86,8 +86,13 @@ properties.
   Single entry is for target voltage and three entries are for <target min max>
   voltages.
 
-  Entries for multiple regulators must be present in the same order as
-  regulators are specified in device's DT node.
+  Entries for multiple regulators shall be provided in the same field separated
+  by angular brackets <>. The OPP binding doesn't provide any provisions to
+  relate the values to their power supplies or the order in which the supplies
+  need to be configured.
+
+  Entries for all regulators shall be of the same size, i.e. either all use a
+  single value or triplets.
 
 - opp-microvolt-<name>: Named opp-microvolt property. This is exactly similar to
   the above opp-microvolt property, but allows multiple voltage ranges to be
@@ -104,10 +109,12 @@ properties.
 
   Should only be set if opp-microvolt is set for the OPP.
 
-  Entries for multiple regulators must be present in the same order as
-  regulators are specified in device's DT node. If this property isn't required
-  for few regulators, then this should be marked as zero for them. If it isn't
-  required for any regulator, then this property need not be present.
+  Entries for multiple regulators shall be provided in the same field separated
+  by angular brackets <>. If current values aren't required for a regulator,
+  then it shall be filled with 0. If current values aren't required for any of
+  the regulators, then this field is not required. The OPP binding doesn't
+  provide any provisions to relate the values to their power supplies or the
+  order in which the supplies need to be configured.
 
 - opp-microamp-<name>: Named opp-microamp property. Similar to
   opp-microvolt-<name> property, but for microamp instead.
@@ -386,10 +393,12 @@ Example 4: Handling multiple regulators
 / {
 	cpus {
 		cpu@0 {
-			compatible = "arm,cortex-a7";
+			compatible = "vendor,cpu-type";
 			...
 
-			cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
+			vcc0-supply = <&cpu_supply0>;
+			vcc1-supply = <&cpu_supply1>;
+			vcc2-supply = <&cpu_supply2>;
 			operating-points-v2 = <&cpu0_opp_table>;
 		};
 	};
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [next] | [standalone]

#1507808 — Re: [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device

On 10/20, Viresh Kumar wrote:
> On certain platforms (like TI), DVFS for a single device (CPU) requires
> configuring multiple power supplies.
> 
> The OPP bindings already contains binding and example to explain this
> case, but it isn't sufficient. For example, there is no way for the code
> parsing these bindings to know which voltage values belong to which
> power supply. Also its not possible to know the order in which the
> supplies need to be configured while switching OPPs.
> 
> This patch tries to clarify on those details and does some minor changes
> as well.
> 
> Note that the bindings do not specify the order in which the regulators
> need to be programmed and the order in which the entries are added for
> the supplies.
> 
> The user of the bindings (like the kernel) shall know these details
> already and the DT is responsible to supply only the readings for the
> regulators.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1505957

On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi,
>
> Some platforms (like TI) have complex DVFS configuration for CPU
> devices, where multiple regulators are required to be configured to
> change DVFS state of the device. This was explained well by Nishanth
> earlier [1].
>
> Some thoughts went into it few months back but then it all got lost. I
> am trying to get that back on track with this thread.
>
> One of the major complaints around multiple regulators case was that the
> DT isn't responsible in any way to represent the ordering in which
> multiple supplies need to be programmed, before or after frequency
> change. It was considered in this patch and such information is left to
> the platform specific OPP driver now, which can register its own
> opp_set_rate() callback with the OPP core and the OPP core will then
> call it during DVFS.
>
> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> and code to pass values for multiple regulators and verified that they
> are all properly read by the kernel (using debugfs interface).
>
> Though more testing on real (TI) platforms would be useful.
>
> This is rebased over: linux-next branch in the PM tree.
>
> V1->V2:
> - Ack from Rob for 1st patch
> - Moved the supplies structure to pm_opp.h (Dave)
> - Fixed an compilation warning.

I need somebody from the OPP camp to review patches [2-8/8] for me.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1506102

On 21 October 2016 at 19:09, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Hi,
>>
>> Some platforms (like TI) have complex DVFS configuration for CPU
>> devices, where multiple regulators are required to be configured to
>> change DVFS state of the device. This was explained well by Nishanth
>> earlier [1].
>>
>> Some thoughts went into it few months back but then it all got lost. I
>> am trying to get that back on track with this thread.
>>
>> One of the major complaints around multiple regulators case was that the
>> DT isn't responsible in any way to represent the ordering in which
>> multiple supplies need to be programmed, before or after frequency
>> change. It was considered in this patch and such information is left to
>> the platform specific OPP driver now, which can register its own
>> opp_set_rate() callback with the OPP core and the OPP core will then
>> call it during DVFS.
>>
>> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
>> and code to pass values for multiple regulators and verified that they
>> are all properly read by the kernel (using debugfs interface).
>>
>> Though more testing on real (TI) platforms would be useful.
>>
>> This is rebased over: linux-next branch in the PM tree.
>>
>> V1->V2:
>> - Ack from Rob for 1st patch
>> - Moved the supplies structure to pm_opp.h (Dave)
>> - Fixed an compilation warning.
>
> I need somebody from the OPP camp to review patches [2-8/8] for me.

Sure, I have already asked Stephen yesterday to do that.

--
viresh

[toc] | [prev] | [next] | [standalone]

#1506816

Hi,
On 10/21/2016 10:40 AM, Viresh Kumar wrote:
> On 21 October 2016 at 19:09, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> Hi,
>>>
>>> Some platforms (like TI) have complex DVFS configuration for CPU
>>> devices, where multiple regulators are required to be configured to
>>> change DVFS state of the device. This was explained well by Nishanth
>>> earlier [1].
>>>
>>> Some thoughts went into it few months back but then it all got lost. I
>>> am trying to get that back on track with this thread.
>>>
>>> One of the major complaints around multiple regulators case was that the
>>> DT isn't responsible in any way to represent the ordering in which
>>> multiple supplies need to be programmed, before or after frequency
>>> change. It was considered in this patch and such information is left to
>>> the platform specific OPP driver now, which can register its own
>>> opp_set_rate() callback with the OPP core and the OPP core will then
>>> call it during DVFS.
>>>
>>> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
>>> and code to pass values for multiple regulators and verified that they
>>> are all properly read by the kernel (using debugfs interface).
>>>
>>> Though more testing on real (TI) platforms would be useful.
>>>
>>> This is rebased over: linux-next branch in the PM tree.
>>>
>>> V1->V2:
>>> - Ack from Rob for 1st patch
>>> - Moved the supplies structure to pm_opp.h (Dave)
>>> - Fixed an compilation warning.
>>
>> I need somebody from the OPP camp to review patches [2-8/8] for me.
>
> Sure, I have already asked Stephen yesterday to do that.

Overall this series looks good to me apart from a few small things. Most 
importantly I was able to get a working implementation using two 
regulators on ti dra7xx platform with proper sequencing built on top of 
this series. We have cpu regulator and Adaptive body bias (abb) 
regulator that must be scaled in a certain order before or after clock 
scaling and I was able to implement a rough custom set_rate to perform 
this and ran some dvfs stress tests that all worked fine.

First comment, I think the platform specific set_rate is a good place to 
hook in for adaptive voltage scaling as well. I was able to implement TI 
Class0 AVS in the same code by using the requested transition voltage as 
a reference and programming AVS voltage using that, along with scaling 
the additional regulators in sequence (the original multi regulator 
functionality). I would think some people would want to use this even 
with single regulator platforms, no? cpufreq-dt works as is for that, we 
just swap out the regulators.

This raises some concerns about dependencies/probe sequencing. Right now 
we just need to make sure the cpufreq-dt driver probes after we have 
called _set_regulators, but if our platform code fails cpufreq-dt 
currently will treat this as no regulator needed for the platform and 
operate without one, which will likely hang the system. Is there a good 
way to to guarantee this doesn't happen? My main concern is that if we 
plan to provide a platform specific set-rate function, we should have a 
way to indicate this and prevent things from progressing if it isn't yet 
ready.

Again, overall I think it solves the multi regulator problem, and it 
works well for AVS as well. For the series:

Tested-by: Dave Gerlach <d-gerlach@ti.com>

Regards,
Dave

>
> --
> viresh
>

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web