[PATCH 0/4] PM / OPP: Minor cleanups

[PATCH 0/4] PM / OPP: Minor cleanups

[Viresh Kumar]

Hi,

Here are few cleanup patches for the OPP core. The first two simplify
the code that was written specifically due to the limitations that we
had because of RCUs. We don't RCUs anymore and this can be simplified.

The last two take care of specific corner cases.

Rebased over pm/linux-next and tested on Exynos dual core board.

Viresh Kumar (4):
  PM / OPP: Reorganize _generic_set_opp_regulator()
  PM / OPP: Don't create copy of regulators unnecessarily
  PM / OPP: opp-microvolt is not optional if regulators are set
  PM / OPP: Don't create debugfs "supply-0" directory unnecessarily

 drivers/base/power/opp/core.c    | 87 +++++++++++++++++-----------------------
 drivers/base/power/opp/debugfs.c |  6 +--
 drivers/base/power/opp/of.c      | 11 ++++-
 3 files changed, 49 insertions(+), 55 deletions(-)

-- 
2.13.0.303.g4ebf3021692d

[PATCH 1/4] PM / OPP: Reorganize _generic_set_opp_regulator()

[Viresh Kumar]

The code was overly complicated here because of the limitations that we
had with RCUs (Couldn't use opp-table and OPPs outside RCU protected
section and can't call sleep-able routines from within that). But that
is long gone now.

Reorganize _generic_set_opp_regulator() in order to avoid using "struct
dev_pm_set_opp_data" and copying data into it for the case where
opp_table->set_opp is not set.

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

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index dae61720b314..402b9e86d77c 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -543,17 +543,18 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 	return ret;
 }
 
-static int _generic_set_opp(struct dev_pm_set_opp_data *data)
+static int _generic_set_opp_regulator(struct opp_table *opp_table,
+				      struct device *dev,
+				      unsigned long old_freq,
+				      unsigned long freq,
+				      struct dev_pm_opp_supply *old_supply,
+				      struct dev_pm_opp_supply *new_supply)
 {
-	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];
-	struct device *dev= data->dev;
+	struct regulator *reg = opp_table->regulators[0];
 	int ret;
 
 	/* This function only supports single regulator per device */
-	if (WARN_ON(data->regulator_count > 1)) {
+	if (WARN_ON(opp_table->regulator_count > 1)) {
 		dev_err(dev, "multiple regulators are not supported\n");
 		return -EINVAL;
 	}
@@ -566,7 +567,7 @@ static int _generic_set_opp(struct dev_pm_set_opp_data *data)
 	}
 
 	/* Change frequency */
-	ret = _generic_set_opp_clk_only(dev, data->clk, old_freq, freq);
+	ret = _generic_set_opp_clk_only(dev, opp_table->clk, old_freq, freq);
 	if (ret)
 		goto restore_voltage;
 
@@ -580,12 +581,12 @@ static int _generic_set_opp(struct dev_pm_set_opp_data *data)
 	return 0;
 
 restore_freq:
-	if (_generic_set_opp_clk_only(dev, data->clk, freq, old_freq))
+	if (_generic_set_opp_clk_only(dev, opp_table->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)
+	if (old_supply)
 		_set_opp_voltage(dev, reg, old_supply);
 
 	return ret;
@@ -603,10 +604,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_opp)(struct dev_pm_set_opp_data *data);
 	struct dev_pm_opp *old_opp, *opp;
-	struct regulator **regulators;
-	struct dev_pm_set_opp_data *data;
 	struct clk *clk;
 	int ret, size;
 
@@ -661,38 +659,35 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
 		old_freq, freq);
 
-	regulators = opp_table->regulators;
-
 	/* Only frequency scaling */
-	if (!regulators) {
+	if (!opp_table->regulators) {
 		ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
-		goto put_opps;
-	}
+	} else if (!opp_table->set_opp) {
+		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
+						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
+						 opp->supplies);
+	} else {
+		struct dev_pm_set_opp_data *data;
 
-	if (opp_table->set_opp)
-		set_opp = opp_table->set_opp;
-	else
-		set_opp = _generic_set_opp;
-
-	data = opp_table->set_opp_data;
-	data->regulators = regulators;
-	data->regulator_count = opp_table->regulator_count;
-	data->clk = clk;
-	data->dev = dev;
-
-	data->old_opp.rate = old_freq;
-	size = sizeof(*opp->supplies) * opp_table->regulator_count;
-	if (IS_ERR(old_opp))
-		memset(data->old_opp.supplies, 0, size);
-	else
-		memcpy(data->old_opp.supplies, old_opp->supplies, size);
+		data = opp_table->set_opp_data;
+		data->regulators = opp_table->regulators;
+		data->regulator_count = opp_table->regulator_count;
+		data->clk = clk;
+		data->dev = dev;
 
-	data->new_opp.rate = freq;
-	memcpy(data->new_opp.supplies, opp->supplies, size);
+		data->old_opp.rate = old_freq;
+		size = sizeof(*opp->supplies) * opp_table->regulator_count;
+		if (IS_ERR(old_opp))
+			memset(data->old_opp.supplies, 0, size);
+		else
+			memcpy(data->old_opp.supplies, old_opp->supplies, size);
 
-	ret = set_opp(data);
+		data->new_opp.rate = freq;
+		memcpy(data->new_opp.supplies, opp->supplies, size);
+
+		ret = opp_table->set_opp(data);
+	}
 
-put_opps:
 	dev_pm_opp_put(opp);
 put_old_opp:
 	if (!IS_ERR(old_opp))
-- 
2.13.0.303.g4ebf3021692d

Re: [PATCH 1/4] PM / OPP: Reorganize _generic_set_opp_regulator()

[Stephen Boyd]

On 05/17, Viresh Kumar wrote:
> The code was overly complicated here because of the limitations that we
> had with RCUs (Couldn't use opp-table and OPPs outside RCU protected
> section and can't call sleep-able routines from within that). But that
> is long gone now.
> 
> Reorganize _generic_set_opp_regulator() in order to avoid using "struct
> dev_pm_set_opp_data" and copying data into it for the case where
> opp_table->set_opp is not set.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp/core.c | 73 ++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index dae61720b314..402b9e86d77c 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -543,17 +543,18 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
>  	return ret;
>  }
>  
> -static int _generic_set_opp(struct dev_pm_set_opp_data *data)
> +static int _generic_set_opp_regulator(struct opp_table *opp_table,

Can opp_table be const?

> +				      struct device *dev,

Maybe dev too, but that looks like a ripple effect so maybe
in another patch.

> +				      unsigned long old_freq,
> +				      unsigned long freq,
> +				      struct dev_pm_opp_supply *old_supply,
> +				      struct dev_pm_opp_supply *new_supply)

Otherwise

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

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

[PATCH 2/4] PM / OPP: Don't create copy of regulators unnecessarily

[Viresh Kumar]

This code was required while the OPP core was managed with help of RCUs,
but not anymore. Get rid of unnecessary alloc/memcpy operations.

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

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 402b9e86d77c..898f19ea0f60 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -180,7 +180,7 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *opp;
-	struct regulator *reg, **regulators;
+	struct regulator *reg;
 	unsigned long latency_ns = 0;
 	int ret, i, count;
 	struct {
@@ -198,15 +198,9 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 	if (!count)
 		goto put_opp_table;
 
-	regulators = kmalloc_array(count, sizeof(*regulators), GFP_KERNEL);
-	if (!regulators)
-		goto put_opp_table;
-
 	uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL);
 	if (!uV)
-		goto free_regulators;
-
-	memcpy(regulators, opp_table->regulators, count * sizeof(*regulators));
+		goto put_opp_table;
 
 	mutex_lock(&opp_table->lock);
 
@@ -232,15 +226,13 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 	 * isn't freed, while we are executing this routine.
 	 */
 	for (i = 0; i < count; i++) {
-		reg = regulators[i];
+		reg = opp_table->regulators[i];
 		ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max);
 		if (ret > 0)
 			latency_ns += ret * 1000;
 	}
 
 	kfree(uV);
-free_regulators:
-	kfree(regulators);
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 
-- 
2.13.0.303.g4ebf3021692d

Re: [PATCH 2/4] PM / OPP: Don't create copy of regulators unnecessarily

[Stephen Boyd]

On 05/17, Viresh Kumar wrote:
> This code was required while the OPP core was managed with help of RCUs,
> but not anymore. Get rid of unnecessary alloc/memcpy operations.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

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

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

[PATCH 3/4] PM / OPP: opp-microvolt is not optional if regulators are set

[Viresh Kumar]

If dev_pm_opp_set_regulators() is called for a device and its regulators
are set in the OPP core, the OPP nodes for the device must contain the
"opp-microvolt" property, otherwise there is something wrong and we
better error out.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/of.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 779428676f63..c6fc8cbad10d 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -131,8 +131,15 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		prop = of_find_property(opp->np, name, NULL);
 
 		/* Missing property isn't a problem, but an invalid entry is */
-		if (!prop)
-			return 0;
+		if (!prop) {
+			/* The regulator-count must be zero here */
+			if (!opp_table->regulator_count)
+				return 0;
+
+			dev_err(dev, "%s: opp-microvolt missing even if regulators are available\n",
+				__func__);
+			return -EINVAL;
+		}
 	}
 
 	vcount = of_property_count_u32_elems(opp->np, name);
-- 
2.13.0.303.g4ebf3021692d

Re: [PATCH 3/4] PM / OPP: opp-microvolt is not optional if regulators are set

[Stephen Boyd]

On 05/17, Viresh Kumar wrote:
> If dev_pm_opp_set_regulators() is called for a device and its regulators
> are set in the OPP core, the OPP nodes for the device must contain the
> "opp-microvolt" property, otherwise there is something wrong and we
> better error out.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp/of.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index 779428676f63..c6fc8cbad10d 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -131,8 +131,15 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
>  		prop = of_find_property(opp->np, name, NULL);
>  
>  		/* Missing property isn't a problem, but an invalid entry is */
> -		if (!prop)
> -			return 0;
> +		if (!prop) {
> +			/* The regulator-count must be zero here */

Comment is restating code with no insight into what's so special,
how about:

			/*
			 * But it better not be missing if we're
			 * expecting OPP core to manage regulators
			 */

Although I suppose the dev_err message is pretty good.

> +			if (!opp_table->regulator_count)
> +				return 0;
> +
> +			dev_err(dev, "%s: opp-microvolt missing even if regulators are available\n",

"missing although OPP managing regulators"?

> +				__func__);

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

[PATCH 4/4] PM / OPP: Don't create debugfs "supply-0" directory unnecessarily

[Viresh Kumar]

We create "supply-0" debugfs directory even if the device doesn't do
voltage scaling. That looks confusing, as if the regulator is found but
we never managed to get voltage levels for it.

Avoid creating such a directory unnecessarily.

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

diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index 95f433db4ac7..ce6f4832946b 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -40,11 +40,11 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
 				      struct dentry *pdentry)
 {
 	struct dentry *d;
-	int i = 0;
+	int i = -1;
 	char *name;
 
 	/* Always create at least supply-0 directory */
-	do {
+	while (++i < opp_table->regulator_count) {
 		name = kasprintf(GFP_KERNEL, "supply-%d", i);
 
 		/* Create per-opp directory */
@@ -70,7 +70,7 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
 		if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
 					  &opp->supplies[i].u_amp))
 			return false;
-	} while (++i < opp_table->regulator_count);
+	}
 
 	return true;
 }
-- 
2.13.0.303.g4ebf3021692d

Re: [PATCH 4/4] PM / OPP: Don't create debugfs "supply-0" directory unnecessarily

[Stephen Boyd]

On 05/17, Viresh Kumar wrote:
> We create "supply-0" debugfs directory even if the device doesn't do
> voltage scaling. That looks confusing, as if the regulator is found but
> we never managed to get voltage levels for it.
> 
> Avoid creating such a directory unnecessarily.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp/debugfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> index 95f433db4ac7..ce6f4832946b 100644
> --- a/drivers/base/power/opp/debugfs.c
> +++ b/drivers/base/power/opp/debugfs.c
> @@ -40,11 +40,11 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
>  				      struct dentry *pdentry)
>  {
>  	struct dentry *d;
> -	int i = 0;
> +	int i = -1;
>  	char *name;
>  
>  	/* Always create at least supply-0 directory */

Bad comment.

> -	do {
> +	while (++i < opp_table->regulator_count) {

This is a brain-melting while loop for what should be a for loop.
Please just use a for loop instead of pre-incrementing -1 and
comparing that to the unsigned type of regulator_count. i should
probably be unsigned as well.

I can only assume it was written as a do-while before because we
assumed there was always some voltage to control.

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