[U-Boot] [RFC] ARM timing code refactoring

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4D3B3B5C.2060205 at free.fr> you wrote:
> 
> > When attempting to come up with true generic code, we should probably
> > _always_ use a (virtual) unsigned 64 bit counter.
> 
> That's fine with me. We'll just have to keep in mind that with a 32-bit 
> HW counter, the upper 32 bits of the 64-bit virtual counter must be 
> managed by SW -- which should not prove to be too much of a problem, but 
> will force us to handle HW-to-SW rollovers in get_timer() on each 
> virtual timer reads.

Yes.

> >> The only thing I slightly dislike about the overall idea is the signed
> >> rather than unsigned comparison in the timeout function (I prefer using
> >> the full 32-bit range, even if only as an academic point) and the fact
> >> that the value of the timeout is encoded in advance in the loop control
> >> variable 'timeout'.
> >
> > Please don't.  Use an unsigned counter and allow it to roll over.
> > Anything else causes additional (and unnecessary) restrictions and
> > makes the code more complicated.
> 
> Hmm... don't we actually agree here? I was also expressing my preference 
> for unsigned.

I'm happy if we agree. I just wan't sure.

> > I'm not sure what you have in mind with "substract".  I strongly
> > recommend to avoid problems with wrap-around etc. right from the
> > beginning, and let unsigned arithmentcs handle this for us.
> 
> That's what I meant with my comment about subtracting: the difference of 
> two unsigned integers is always correct.

Good.

> >> e.g. 'then = time(0)' would set 'then' to the number of ticks elapsed
> >> from boot, while 'now = time(then)' would set 'now' the ticks elapsed
> >> from 'then'; and a 'ms_to_ticks(x)' (again, or 'milliseconds(x)') :
> >
> > Do we really need such a function?  As far as I can tell we don't
> > really have any time (in the sense of wallclock time) related
> > requirements, we only need delta times, and nobody really cares about
> > the starting point.
> 
> Hmm... My idea with providing time() with an argument was that precisely 
> since we are interested only in elapsed time, not absolute time, our 
> basic time function should be able to tell us relative times.

The disadvantage of this approach is that such calls cannot be nested,
i. e. you must always make sure that the code run within the timeout
loop does not attempt to set up a timeout on it's own.  From my point
of view, this is a killing point.

> Functionally this is the same as what I suggested, minus the absolute 
> get_timer() vs relative time(x) call. Either way works, I'm not going to 
> try and make a point that we use "my" time(x).

See above for the fundamental difference - not in the implementation
for a single timeout, but from a system-wide point of view.

> >> 	then = time(0);
> >> 	do {...} while ( time(then)<  ms_to_ticks(100) );
> >
> > We should NOT do this.  It is bound to break as soon as the code
> > in the loop (the "..." part) needs to implement a timeout, too.
> 
> I'm not sure to understand why. Can you develop how you think this would 
> break with an inner timeout?

Sure:

	/* implement something which needs a 100 ms timeout */
	then = time(0);

	do {
		int then_nested;
		... do something...
		... do more...
		/* now do something which needs a 5 ms timeout */
		then_nested = time(0);
		do {
			...
		} while (time(then_nested) < ms_to_ticks(5));

	} while (time(then) < ms_to_ticks(100));

You see the problem?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Remember, there's a big difference between kneeling down and  bending
over.                                                   - Frank Zappa