[PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[Lucas Stach]

Currently this function only provides a valid output if both
old_uV and new_uV are exact voltages that can be provided by the
regulator.
This is almost impossible to achive as the consumer has
no way to know the exact voltages provided by the regulator.
Also it creates an unexpected twist in the API, as a user
could possibly not use the same voltage parameters for
set_voltage_time() as for set_voltage_time().

This breaks the current cpufreq users of this function, as they
stick in the raw voltages retrieved from their operating points,
which may or may not match one of the regulator voltages.

To make this function behave as expected employ the same logic
as used when calling set_voltage() and round the voltages to
the closest matching voltage supported by the regulator.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/regulator/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9a09f3cdbabb..29ab83f49da9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2477,7 +2477,7 @@ int regulator_set_voltage_time(struct regulator *regulator,
 	struct regulator_ops	*ops = rdev->desc->ops;
 	int old_sel = -1;
 	int new_sel = -1;
-	int voltage;
+	int voltage, best_match_old_uV = INT_MAX, best_match_new_uV = INT_MAX;
 	int i;
 
 	/* Currently requires operations to do this */
@@ -2486,16 +2486,19 @@ int regulator_set_voltage_time(struct regulator *regulator,
 		return -EINVAL;
 
 	for (i = 0; i < rdev->desc->n_voltages; i++) {
-		/* We only look for exact voltage matches here */
 		voltage = regulator_list_voltage(regulator, i);
 		if (voltage < 0)
 			return -EINVAL;
 		if (voltage == 0)
 			continue;
-		if (voltage == old_uV)
+		if (voltage >= old_uV && voltage < best_match_old_uV) {
 			old_sel = i;
-		if (voltage == new_uV)
+			best_match_old_uV = voltage;
+		}
+		if (voltage >= new_uV && voltage < best_match_new_uV) {
 			new_sel = i;
+			best_match_new_uV = voltage;
+		}
 	}
 
 	if (old_sel < 0 || new_sel < 0)
-- 
2.0.0.rc0

Re: [PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[Lucas Stach]

Am Mittwoch, den 21.05.2014, 18:53 +0200 schrieb Lucas Stach:
> Currently this function only provides a valid output if both
> old_uV and new_uV are exact voltages that can be provided by the
> regulator.
> This is almost impossible to achive as the consumer has
> no way to know the exact voltages provided by the regulator.
> Also it creates an unexpected twist in the API, as a user
> could possibly not use the same voltage parameters for
> set_voltage_time() as for set_voltage_time().
And one of the above set_voltage_time() should obviously just have been
a set_voltage().

> 
> This breaks the current cpufreq users of this function, as they
> stick in the raw voltages retrieved from their operating points,
> which may or may not match one of the regulator voltages.
> 
> To make this function behave as expected employ the same logic
> as used when calling set_voltage() and round the voltages to
> the closest matching voltage supported by the regulator.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/regulator/core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9a09f3cdbabb..29ab83f49da9 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2477,7 +2477,7 @@ int regulator_set_voltage_time(struct regulator *regulator,
>  	struct regulator_ops	*ops = rdev->desc->ops;
>  	int old_sel = -1;
>  	int new_sel = -1;
> -	int voltage;
> +	int voltage, best_match_old_uV = INT_MAX, best_match_new_uV = INT_MAX;
>  	int i;
>  
>  	/* Currently requires operations to do this */
> @@ -2486,16 +2486,19 @@ int regulator_set_voltage_time(struct regulator *regulator,
>  		return -EINVAL;
>  
>  	for (i = 0; i < rdev->desc->n_voltages; i++) {
> -		/* We only look for exact voltage matches here */
>  		voltage = regulator_list_voltage(regulator, i);
>  		if (voltage < 0)
>  			return -EINVAL;
>  		if (voltage == 0)
>  			continue;
> -		if (voltage == old_uV)
> +		if (voltage >= old_uV && voltage < best_match_old_uV) {
>  			old_sel = i;
> -		if (voltage == new_uV)
> +			best_match_old_uV = voltage;
> +		}
> +		if (voltage >= new_uV && voltage < best_match_new_uV) {
>  			new_sel = i;
> +			best_match_new_uV = voltage;
> +		}
>  	}
>  
>  	if (old_sel < 0 || new_sel < 0)

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

On Wed, May 21, 2014 at 06:53:59PM +0200, Lucas Stach wrote:
> Currently this function only provides a valid output if both
> old_uV and new_uV are exact voltages that can be provided by the
> regulator.
> This is almost impossible to achive as the consumer has
> no way to know the exact voltages provided by the regulator.

Yes it does - this is what regulator_list_voltage() is there for.
Drivers can enumerate all the voltages supported by a regulator.

> This breaks the current cpufreq users of this function, as they
> stick in the raw voltages retrieved from their operating points,
> which may or may not match one of the regulator voltages.

At least the code in cpufreq-cpu0 looks a bit confused here.  The use of
min_uV and max_uV is a bit unclear but probably correct however for some
reason it appears that what it's doing is stepping through each single
step transition between two adjacent frequencies, getting the transition
latency for that and then summing those.  Given that it needs a single
number I'd expect it to instead be getting the minimum and maximum
voltages and then working out the highest latency for transitioning
between those, what it's doing at the minute will be overestimating any
fixed component of transition latency (from the time taken to issue
commands to the device for example).

Incidentally the clock API ought to have a similar thing - at the minute
the driver just has a fixed number stuffed into it from DT but it really
ought to be able to ask the clock API in the same way as it asks the
regulator API.

> To make this function behave as expected employ the same logic
> as used when calling set_voltage() and round the voltages to
> the closest matching voltage supported by the regulator.

That's not what the set_voltage() code does - what it does is find the
lowest voltage in the requested range.  

Your code won't actually do quite what cpufreq-cpu0 is doing since it
uses set_voltage_tol() which will ask for a range around the voltage
it's trying to set so the query in cpufreq-cpu0 will come out as
something different to what the driver actually ends up doing when it
does transitions.  We should probably add functions to query what the
actual voltage selected for a given set_voltage() and set_voltage_tol()
will be then let that be fed into requesting the transition times.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[Lucas Stach]

Am Sonntag, den 01.06.2014, 12:38 +0100 schrieb Mark Brown:
> On Wed, May 21, 2014 at 06:53:59PM +0200, Lucas Stach wrote:
> > Currently this function only provides a valid output if both
> > old_uV and new_uV are exact voltages that can be provided by the
> > regulator.
> > This is almost impossible to achive as the consumer has
> > no way to know the exact voltages provided by the regulator.
> 
> Yes it does - this is what regulator_list_voltage() is there for.
> Drivers can enumerate all the voltages supported by a regulator.
> 
> > This breaks the current cpufreq users of this function, as they
> > stick in the raw voltages retrieved from their operating points,
> > which may or may not match one of the regulator voltages.
> 
> At least the code in cpufreq-cpu0 looks a bit confused here.  The use of
> min_uV and max_uV is a bit unclear but probably correct however for some
> reason it appears that what it's doing is stepping through each single
> step transition between two adjacent frequencies, getting the transition
> latency for that and then summing those.  Given that it needs a single
> number I'd expect it to instead be getting the minimum and maximum
> voltages and then working out the highest latency for transitioning
> between those, what it's doing at the minute will be overestimating any
> fixed component of transition latency (from the time taken to issue
> commands to the device for example).
> 
Note the add is not within any loop, so what cpufreq-cpu0 currently does
is getting the lowest and highest voltage and using the transition time
between those two. It's just adding this to a fixed delay used to
represent other delays like PLL relock.

> Incidentally the clock API ought to have a similar thing - at the minute
> the driver just has a fixed number stuffed into it from DT but it really
> ought to be able to ask the clock API in the same way as it asks the
> regulator API.
> 
Right, this should be easily fixable by calling clk_round_rate() on te
OPP defined frequencies.

> > To make this function behave as expected employ the same logic
> > as used when calling set_voltage() and round the voltages to
> > the closest matching voltage supported by the regulator.
> 
> That's not what the set_voltage() code does - what it does is find the
> lowest voltage in the requested range.  
> 
> Your code won't actually do quite what cpufreq-cpu0 is doing since it
> uses set_voltage_tol() which will ask for a range around the voltage
> it's trying to set so the query in cpufreq-cpu0 will come out as
> something different to what the driver actually ends up doing when it
> does transitions.  We should probably add functions to query what the
> actual voltage selected for a given set_voltage() and set_voltage_tol()
> will be then let that be fed into requesting the transition times.

Hm, this sounds a lot like clk_round_rate() for the regulator API which
sounds like a sensible addition. One problem I see here is that the
result is not really a static value, but rather depends on the
consumers. If we call into the regulator API early to ask about the
voltage we will get when asking for a range, the result may well be
different than the real value after other consumers have registered
themselves with a different min_uV.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH] regulator: core: allow non-exact matches in regulator_set_voltage_time()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]

On Mon, Jun 02, 2014 at 12:27:09PM +0200, Lucas Stach wrote:
> Am Sonntag, den 01.06.2014, 12:38 +0100 schrieb Mark Brown:
> > On Wed, May 21, 2014 at 06:53:59PM +0200, Lucas Stach wrote:

> > At least the code in cpufreq-cpu0 looks a bit confused here.  The use of
> > min_uV and max_uV is a bit unclear but probably correct however for some
> > reason it appears that what it's doing is stepping through each single
> > step transition between two adjacent frequencies, getting the transition
> > latency for that and then summing those.  Given that it needs a single
> > number I'd expect it to instead be getting the minimum and maximum
> > voltages and then working out the highest latency for transitioning
> > between those, what it's doing at the minute will be overestimating any
> > fixed component of transition latency (from the time taken to issue
> > commands to the device for example).

> Note the add is not within any loop, so what cpufreq-cpu0 currently does
> is getting the lowest and highest voltage and using the transition time
> between those two. It's just adding this to a fixed delay used to
> represent other delays like PLL relock.

Ugh, that IS_ERR() and for loop really aren't helping legibility here.
It'd be better for it to go through and check the voltage on every
frequency, probably it won't make much difference but it'd be more
robust.

> > Incidentally the clock API ought to have a similar thing - at the minute
> > the driver just has a fixed number stuffed into it from DT but it really
> > ought to be able to ask the clock API in the same way as it asks the
> > regulator API.

> Right, this should be easily fixable by calling clk_round_rate() on te
> OPP defined frequencies.

No, you're missing the point.  The clock API ought to be providing a way
of querying the latency.

> > Your code won't actually do quite what cpufreq-cpu0 is doing since it
> > uses set_voltage_tol() which will ask for a range around the voltage
> > it's trying to set so the query in cpufreq-cpu0 will come out as
> > something different to what the driver actually ends up doing when it
> > does transitions.  We should probably add functions to query what the
> > actual voltage selected for a given set_voltage() and set_voltage_tol()
> > will be then let that be fed into requesting the transition times.

> Hm, this sounds a lot like clk_round_rate() for the regulator API which
> sounds like a sensible addition. One problem I see here is that the
> result is not really a static value, but rather depends on the
> consumers. If we call into the regulator API early to ask about the
> voltage we will get when asking for a range, the result may well be
> different than the real value after other consumers have registered
> themselves with a different min_uV.

Sure, but this stuff is never going to work absolutely reliably in the
presence of multiple consumers anyway and you're most likely going to be
getting a worst case number which is what you want.  If you're doing an
absolute delay you'd need to query what the actual transition was and
delay at that point.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]