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

[Graeme Russ]

Hi Wolfgang,

On 12/07/11 07:56, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <1309261269-4363-1-git-send-email-graeme.russ at gmail.com> you wrote:
>> The following series is a work-in-progress revamp of the timer API. The aim
>> is to create a new userland API consisting of the following functions
>> (along with a few arch level support functions):
> 
> I have to apologize for not commenting on all of this for such a long
> time.  There are a number of reeasons for this silence - lack of
> available time being one of them (and probably the most pressing one),
> but I also needed some time to lean back and think through all of
> this.

I understand - I also would rather spend more time on this and get it right
now rather than going back and fixing it again

> One thing I always wanted to do in the previous discussion was to check
> what other projects (like Linux, barebox, etc.) are doing in this area.
> I think it is worth reading the Linux Documentation/timers/highres.txt
> document, especially the sections about John Stultz's Generic Time Of
> Day (GTOD) framework, please the documents linked there (i. e. the OLS
> slides [the link in Documentation/timers/highres.txt is stale; use
> http://www.kernel.org/pub/linux/kernel/people/tglx/hrtimers/ols2006-hrtimers.pdf
> instead] and the paper "We Are Not Getting Any Younger: A New Approach
> to Time and Timers" by J. Stultz et al. in
> http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf p. 219ff).

Will do

> Having this still in mind, I took a look across the fence to what
> barebox is doing.  Guess what?  From "common/clock.c":
> 
> 	 * clock.c - generic clocksource implementation
> 	 *
> 	 * This file contains the clocksource implementation from the Linux
> 	 * kernel originally by John Stultz
> 
> OK.  Message received.
> 
> What I'm asking myself (and now you) is: Should we really re-invent
> the wheel again?
> 
> 
> Regarding the function names:
> 
>> u32 time_now_ms(void);
>> u32 time_since_ms(u32 from, u32 to);
>> u32 time_max_since_ms(u32 from, u32 to);
> 
> I don't like these.  They appear wrong to me, and they are not in sync
> with the wiki page either.

I know - there was some discussion about these names and they were in a
little bit of flux (hence WIP). I was looking to get some community clarity
on this first before updating the wiki. In hindsight, I'm glad I didn't
update the wiki as it looks like there may be a better approach anyway

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

> Second, I don't feel well with "time_now()" - what is this, what does
> it do?  Does it set or get a time?  The "get_time()" suggestion we
> discussed earlier feels still much better to me (as it clearly says
> which operation the function performs).  Or we could even follow the
> example of the Unix system call time(2) and just use "time()".

Well time_ was the namespace for the new API and now is, well, now ;)

> The signature of time_since(from, to) is broken.  For "time since" it
> seems logically to expect a single argument only: time_since(when).
> The wiki page still lists this function as time_delta() which seems
> way more logical to me [although I have to admit that I don;t
> understand what the "delta_type" argument might be.

It was a refinement of your suggestion that sometimes we want to know that
at least a specific period of time has passed (timeouts) and sometimes we
want to know what the maximum amount of time that has passed is (profiling)

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

> need any of those, nor every other project I'm aware of.  Come on,
> let's keep the code small and efficient and do without these bells and
> whistles.  Quote Antoine de Saint-Exupery: "Perfection is reached,
> not when there is no longer anything to add, but when there is no
> longer anything to take away."

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.

>> This current patch series migrates the users of the existing timer API
>> consisting of get_timer() and reset_timer() to the new API while still
>> retaining the arch specific framework in the background.
> 
> I feel bad that you already have spent so much work, and only now I
> come and call everything into question again.  Sorry.

That's OK - Thanks for the feedback.

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?

Regards,

Graeme