Re: [PATCH 2/2] regulator: Optimise out noop voltage changes

Re: [PATCH 2/2] regulator: Optimise out noop voltage changes

[Saravana Kannan]

> Reported-by: Saravana Kannan <skannan@codeaurora.org>
Thanks

>  int regulator_set_voltage(struct regulator *regulator, int min_uV, int
> max_uV)
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
> -	int ret;
> +	int ret = 0;
>
>  	mutex_lock(&rdev->mutex);
>
> +	/* If we're setting the same range as last time the change
> +	 * should be a noop (some cpufreq implementations use the same +	 *
voltage for multiple frequencies, for example).
> +	 */
> +	if (regulator->min_uV == min_uV && regulator->max_uV == max_uV)
+		goto out;
> +

I have only web email access now. Sorry for the vague references to code
that follow.

When you reported a merge/rebase issue the other day, I didn't respond
with a similar patch for the following reasons:
1. With support for multiple consumers, we can further optimize and call
the producer's set voltage only if the final voltage range changes. So if
consumer A asks for (2.0 - 5.0) and consumer B keeps changing between (1.0
- 5.0) and (1.5 - 5.0), then we can completely no-op all of consumer B's
calls.

2. When I was trying to do the above this Sunday, I also noticed what
looks like a bug or at least an unpleasant behavior. A consumer's min_uV
and max_uV were being updated (for-next around Dec 12th) before calling
the producer's set voltage. So, in the above example, if consumer C comes
in and asks for (10 - 15), it will prevent the producer voltage from ever
changing again. All of consumer A and B's future requests will result in a
failure since min_uV > max_uV when you do the consumer aggregation.

Hope my vague references to code is good enough to point you the code I'm
talking about. May be you already fixed it too!

Thanks,
Saravana
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] regulator: Optimise out noop voltage changes

[Mark Brown]

On Thu, Dec 16, 2010 at 05:46:07PM -0800, Saravana Kannan wrote:
> 
> 1. With support for multiple consumers, we can further optimize and call
> the producer's set voltage only if the final voltage range changes. So if
> consumer A asks for (2.0 - 5.0) and consumer B keeps changing between (1.0
> - 5.0) and (1.5 - 5.0), then we can completely no-op all of consumer B's
> calls.

Yes, that could be done as a further optimisation - we'd be able to bale
out after the 

> 2. When I was trying to do the above this Sunday, I also noticed what
> looks like a bug or at least an unpleasant behavior. A consumer's min_uV
> and max_uV were being updated (for-next around Dec 12th) before calling
> the producer's set voltage. So, in the above example, if consumer C comes
> in and asks for (10 - 15), it will prevent the producer voltage from ever
> changing again. All of consumer A and B's future requests will result in a
> failure since min_uV > max_uV when you do the consumer aggregation.

I'm sorry I can't parse this at all.  What is a producer?

If the consumers are all asking for incompatible things there's a system
integration issue and the consumers need to be sorted out; it's
difficult for the regulator API to do anything safely while everything
wants incompatible configurations.

Re: [PATCH 2/2] regulator: Optimise out noop voltage changes

[Saravana Kannan]

Mark Brown wrote:
>> 2. When I was trying to do the above this Sunday, I also noticed what
>> looks like a bug or at least an unpleasant behavior. A consumer's min_uV
>> and max_uV were being updated (for-next around Dec 12th) before calling
>> the producer's set voltage. So, in the above example, if consumer C
>> comes
>> in and asks for (10 - 15), it will prevent the producer voltage from
>> ever
>> changing again. All of consumer A and B's future requests will result in
>> a
>> failure since min_uV > max_uV when you do the consumer aggregation.
>
> I'm sorry I can't parse this at all.  What is a producer?
>
> If the consumers are all asking for incompatible things there's a system
> integration issue and the consumers need to be sorted out; it's
> difficult for the regulator API to do anything safely while everything
> wants incompatible configurations.

Sorry for the confusing explanation. Like I said, I didn't have access to
the code and I was using terms I had in my head. Anyway, wrote a quick
patch and sent it your way. Think of it more as an RFC to explain my point
-- I'm more than willing to implement it differently if you don't like it.

Thanks,
Saravana
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/2] regulator: Add API to re-apply voltage to hardware

[Mark Brown]

When cooperating with an external control source the regulator setup
may be changed underneath the API. Currently consumers can just redo
the regulator_set_voltage() to restore a previously set configuration
but provide an explicit API for doing this as optimsations in the
regulator_set_voltage() implementation will shortly prevent that.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c           |   47 ++++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |    1 +
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3d72cc8..a12cba3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1727,6 +1727,53 @@ out:
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
 
+/**
+ * regulator_sync_voltage - re-apply last regulator output voltage
+ * @regulator: regulator source
+ *
+ * Re-apply the last configured voltage.  This is intended to be used
+ * where some external control source the consumer is cooperating with
+ * has caused the configured voltage to change.
+ */
+int regulator_sync_voltage(struct regulator *regulator)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	int ret, min_uV, max_uV;
+
+	mutex_lock(&rdev->mutex);
+
+	if (!rdev->desc->ops->set_voltage &&
+	    !rdev->desc->ops->set_voltage_sel) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* This is only going to work if we've had a voltage configured. */
+	if (!regulator->min_uV && !regulator->max_uV) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	min_uV = regulator->min_uV;
+	max_uV = regulator->max_uV;
+
+	/* This should be a paranoia check... */
+	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		goto out;
+
+	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		goto out;
+
+	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+
+out:
+	mutex_unlock(&rdev->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_sync_voltage);
+
 static int _regulator_get_voltage(struct regulator_dev *rdev)
 {
 	int sel;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ebd7472..7954f6b 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -154,6 +154,7 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 				   int min_uV, int max_uV);
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV);
 int regulator_get_voltage(struct regulator *regulator);
+int regulator_sync_voltage(struct regulator *regulator);
 int regulator_set_current_limit(struct regulator *regulator,
 			       int min_uA, int max_uA);
 int regulator_get_current_limit(struct regulator *regulator);
-- 
1.7.1

[PATCH 2/2] regulator: Optimise out noop voltage changes

[Mark Brown]

If a consumer sets the same voltage range as is currently configured
for that consumer there's no need to run through setting the voltage
again. This pattern may occur with some CPUfreq implementations where
the same voltage range is used for multiple frequencies.

Reported-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a12cba3..ab419f8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1697,10 +1697,17 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 {
 	struct regulator_dev *rdev = regulator->rdev;
-	int ret;
+	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
 
+	/* If we're setting the same range as last time the change
+	 * should be a noop (some cpufreq implementations use the same
+	 * voltage for multiple frequencies, for example).
+	 */
+	if (regulator->min_uV == min_uV && regulator->max_uV == max_uV)
+		goto out;
+
 	/* sanity check */
 	if (!rdev->desc->ops->set_voltage &&
 	    !rdev->desc->ops->set_voltage_sel) {
-- 
1.7.1