Message-ID: <1401704829.4655.13.camel@weser.hi.pengutronix.de>
Date:	Mon, 02 Jun 2014 12:27:09 +0200
From:	Lucas Stach <l.stach@...gutronix.de>
To:	Mark Brown <broonie@...nel.org>
Cc:	Liam Girdwood <lgirdwood@...il.com>, linux-kernel@...r.kernel.org,
	kernel@...gutronix.de
Subject: Re: [PATCH] regulator: core: allow non-exact matches in
 regulator_set_voltage_time()

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/  |

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists