[U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite

[Graeme Russ]

Hi Wolfgang,

Thanks for the renewed feedback

On 12/07/11 18:49, Wolfgang Denk wrote:
> Dear Graeme,
> 
> I'm trying to summarize your last 3 postings here.
> 
> In message <4E1B7E0C.8000900 at gmail.com> you wrote:
>>
>>> First, I would very much like to get rid of this "_ms" thing.  We
>>> should rather make very clear in the documentation which unit the time
>>> services are based on, and use this consequently.  Only functions
>>> using a different unit should make this clean in their names.
>>
>> Ideally, I think this should be microseconds (a lot happens in a
>> microsecond on modern platforms, and a millisecond can be an eternity)
> 
> Then we might as well follow the example of the Linux generic TOD
> framework and use nanoseconds instead - we will need u64 data types 
> anyway when going for microseconds, so we can just follow their
> example.

Yes, I like the idea of using nanoseconds as a raw timebase

>>> Third: all this time_*_max(), ..._min() and ...raw() stuff.  Aren't we
>>> over-engineering here?  We have been successfully writing U-Boot code
>>> for 11 years now, and never needed these before.  Neither does Linux
>>
>> Part of the problem came from the realisation that some architectures have
>> really bad timer resolution (Nios2 in particular). In an ideal world, I
>> think a uniform microsecond timer (millisecond is to coarse to cater for
>> all needs such as udelay and profiling) would be sufficient to not need
>> these functions but while ever we have platforms with 'poor' timers, I
>> think you may find these functions have a place.
> 
> No, I disagree here.  We should not clutter up generic code with
> things that are only needed for a single (unusually restricted)
> architecture.  Instead, we should try to come up with a design that
> hides such details in architecture specific code.

So how do we deal with Nios2? It is what caused such a deep investigation
into the timer API. We have three choices I can think of off the top of my
head:

 1. Move the whole timer API up to the architecture level and replicate
code everywhere
 2. Make the whole timer API weak
 3. Fundamentally allow the timer API to handle arbitrary timer resolutions

1. Way ugly. We already have this, and that is why we are here today
2. Well, you know what will happen - Someone will be unhappy with the
generic API and rewrite a completely different one (timer_masked anyone!)
3. Why is this so evil?

I'm open to other options if you have any

>> As stated - This started from a suggestion made by yourself (albeit one you
>> did not think was apparently necessary). The idea of the separate functions
>> (rather than a parameter to a single function) was that if the functions
>> were not used, they would be optimised out.
> 
> You know how things go: if you offer the users a set of 20 functions
> so they can chose the one or two that fit their purposes best, they
> will end up with using all 20 of them, especially when diffferent
> parts of the code get maintained by different people with different
> preferences.  Approaches like that always get out of hand quickly.
> 
> We don't have any such stuff now, and U-Boot is working fine.  Let's
> keep it with that, or make it even more simple.  But not the other
> way.

Realistically we are looking at the following functionality:

1) Get the current time
2) Report the minimum time elapsed since an arbitrary epoch
3) Report the maximum time elapsed since an arbitrary epoch
4) Delay for an arbitrary period of time

4 is a derivative of 2 - Just loop until at least the required time has
elapsed.

And you then suggest bringing in no less than 6 functions from Linux

>> What would you like me to do with the clean-up patches that you have
>> already ack'd which do not relate directly to the new API? Should I mark
>> the current series as Rejected in patchwork and create a brand new smaller
>> series which only has those specific patches?
> 
> I suggest you create a new patch series from the independent clean-up
> patches and submit this as normal patches (i. e. without the WIP / RFC
> part).  This would already be a great improvement, I think.  Thanks a
> lot for the efforts!

Done - I will reject the current series and rebase/repost the patches you
have already ack'd and assign them to you in patchwork - I'll leave it up
to you to pull them in

> In message <4E1B88EE.9040104 at gmail.com> you wrote:
> 
>> - Looking at the low-level framework described in ols2006-hrtimers.pdf
>> (Linux API), we are looking at implementing the same thing - An
>> architecture specific free running, high speed, high resolution, high
>> accuracy hardware counter/timer and a low speed interrupt which translates
>> the hardware counter/timer to a common API. In this respect, we are not
>> re-inventing that wheel at all
> 
> Indeed.  We should also avoid re-inventing the algorithms, and
> eventually re-implementing the code.  In any case, I think it would be
> great to use a compatible API so we don;t have to change too many
> things when adapting code from Linux etc.

OK, I'll spend some time digging into the Linux framework a bit more - I
have only had a brief look so far

>> - The rest of the Linux API is like hitting a thumb-tack with a
>> sledgehammer - Timer Queues, NTP, Callbacks, Scheduling etc, etc, etc. We
>> only want to do two things:
> 
> Right.  Linux is a full-blown OS with a lot of needs we don't have in
> U-Boot.
> 
>>   1. Monitor timeouts
>>   2. Calculate code execution time (and even then, not everyone all the
>>      time)
> 
> Actrually we need just timestamps.
> 
> All the rest (like delays, timeouts, executin times etc.) can be
> derived from that.

Agree, but we need to implement the derivative functions in lib/ otherwise
we doom ourselves to the current mess all over again

>> - The Linux API is nanosecond resolution. We are hard pressed to get the
>> average hardware to support microsecond resolution
> 
> On the other hand, it makes little difference to the code wether we
> use microseconds or nanoseconds.  It's just slightly different values
> in the u64 variables.

Agreed, nanoseconds it is

>> - The Linux API includes usage of 'clock events' - These are timer
>> interrupts driven by programmable hardware and relieve the stress on the
>> timer scheduler keeping track of every single timer - Overkill for a boot
>> loader
> 
> Agreed. Well, partially.  We should still follow the example of
> keeping the generic code clean of architecture / timer specific code.
> This should be implemented in the respective arechitecture specific
> code.

And that is exactly what the proposed framework was working towards. The
only feature the architecture (be it the CPU, SOC, Chipset, Board,
whatever) must provide is a method of accessing a free-running counter and
an indication of how fast that counter increments. All the rest is done in lib/

Provided you have access to an incrementing value which increments at a
fixed rate and you know the rate, the rest is architecture independent

>> - The Linux API includes 'Time of Day' (Wall Time) - Again, I don't think
>> we really need this in a boot loader (we have an RTC API we can use if we
>> want to get the current Date and Time).
> 
> We could also say this is all we need. If we have a working high
> precision TOD timestamp, we can derive all other things we need from
> that.

So you want to look at bringing in the Linux TOD API as well? That means we
have to synchronise the timer to an RTC which is something not all boards
have. I think we should stick (at least for the time being) to an arbitrary
timer whose value means absolutely nothing other than the difference
between two values during any given U-Boot execution represent the number
of nanoseconds elapsed.

>> So, all we need is a fixed resolution free running timer we can use to do
>> simple (one at a time) timing operations. No callbacks, no scheduling, to
>> 'Wall Time', no 'Clock Events', no NTP etc. The Linux API is 'Too Big'
> 
> I never proposed to adapt all of it.

Phew ;)

> My suggestion was to pick the parts that fit, and use them.
> especially, use the same API (like going for nanoseconds as unit of
> time), and use their algorithms (and ideally also their code) to solve
> the problems we have to deal with.
> 
>> I don't think we are re-inventing the wheel with our underlying concept -
> 
> Not with the concept, but with the code.
> 
> 
> 
> In message <4E1BAED7.3070009 at gmail.com> you wrote:
>>
>> I think I can safely say that:
>>
>> a) We do not want to inherit the code from Linux. It's good code clean
>> code, but it is just way too much for what we need. It handles registering
>> and unregistering clock sources, change clock source 'rating' (what ever
>> that is) and relies heavily on quite heavy (for U-Boot) struct's (which may
>> need to be carted around in Global Data)
> 
> See above.  You are right, simply copying code unreflected makes no
> sense.  But we should use it as example for API, algorithms, and code.
> 
>> b) Barebox doesn't really inherit that much from Linux anyway
> 
> Right, they did what I proposed, and copied only a part of it, i. e.
> the generic TOD framework.

Actually, when I look at it I see so little in common I am surprised they
even bothered with the attribution at all. Maybe they started with more and
progressively stripped it bare - I'll have a better look

> Even if we assume that they made a good choice for this selection for
> the purposes of barebox, this still does not mean that the same
> selection should be made for U-Boot.
> 
> I'm the last to complein if we come up with a leaner implementation
> that still solves the problems we're trying to solve.
> 
>> I think we have the basics right (and that is the same as Linux). We just
>> need to settle on a few finer points such as:
>>
>>  - Raw time interval. Linux uses nanoseconds. A 64-bit nanosecond clock
>>    source goes for >580 years so even if the highest resolution clock
>>    available to us right now is microsecond, it will not hurt to go with a
>>    nanosecond time-base as that will provide us with the greatest
>>    flexibility going forward. However, this will lead to a lot of integer
>>    divides and/or multiplies to scale to millisecond and microsecond
>>    intervals
> 
> You argumented above against milliseconds (which still works
> reasonably well with 32 bit data types) and suggested to use
> microseconds.  With a 32 bit data type and using microseconds as unit,
> you can get some 4,300 seconds for unsigned and some 2,100 seconds for
> signed data types - in other words, in the order of a little over one
> hour resp. half an hour.  This is too tight - so we would have to use
> 64 bit data types here, too.

Well for starters, that was before I read the Linux framework and I never
suggested a 32-bit timer either.

>>  - Function naming
> 
> See my previous comments. And the longer I think about it, the more I
> think we should just use
> 
> 	u64 time(void)
> 
> as core of this new code.

Agreed - As said before (for the time being) the return value of any
arbitrary call to time() means nothing. It _may_ mean the number of
nanoseconds since power-up, but this is by no means guaranteed.

But what about delays (no-brainer - ndelay, udelay, mdelay) and 'elapsed time'

>>  - Performing time comparisons for timeouts - Barebox for example has a
>>    is_timeout function which takes a start time and a delay (in
>>    nanoseconds) an implements ndealy, udealy, and mdelay using is_timeout
> 
> Well, here I think we should have a look at Linux again here. In
> include/linux/jiffies.h they provide a nice set of inlines, which in
> our case would reuse directly:
> 
> 	time_after()
> 	time_after_eq()
> 	time_before()
> 	time_before_eq()
> 	time_in_range()
> 	time_in_range_open()
> 	

When would we use time_in_range and time_in_range_open? I don't think we
can build an argument based on portability of code from Linux as we will
need to do a lot of code touch-ups because we will not be bringing in the
complete Linux timer API anyway.

In some respects, I think it less confusing to clearly define our own API
rather than generating a illusion of compatibility.

> The classic timeout code then becomes:
> 
> 	u64 then = time() + TIMEOUT;
> 	...
> 	if (time_after(time(), then)) {
> 		/* handle timeout */
> 	}

Arrrgh - Sorry, but we have been around and around and around this. There
are sooooo many way to do this - The two big contenders have been:

1)
	u64 start = time();

	do {
		...stuff...
	} while (time_since(start) < TIMEOUT);


2)
	u64 now = time();
	u64 end = future_time(now, TIMEOUT);

	do {
		...stuff...
	} while(time() < end);

And now we throw a third into the mix.

time_since and future_time are designed to take into account the underlying
resolution of the hardware timer/counter. The big thing to remember is that
we _must_ handle the case where the underlying timer is too coarse

>> I think what has been proposed here recently and documented (slightly
>> out-of-date) on the wiki is the way to go. Taking the Linux or Barebox
>> source will be more effort than it's worth for the complexity it brings in.
> 
> I don't want to take all of it.  But there is alarge amount of good
> things, and we should pick up bits and pieces we can use to save us
> efforts and pain.

As I said, I will have a closer look at the Linux API...

Regards,

Graeme