Message-ID: <20140602121538.GL22111@sirena.org.uk>
Date:	Mon, 2 Jun 2014 13:15:38 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Lucas Stach <l.stach@...gutronix.de>
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()

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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists