On Tue, Jun 03, 2008 at 07:47:25AM -0500, Menon, Nishanth wrote:
Sascha,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Tuesday, June 03, 2008 3:08 AM To: Menon, Nishanth Cc: u-boot-users@lists.sourceforge.net; Laurent Desnogues; dirk.behme@googlemail.com; philip.balister@gmail.com; Gopinath, Thara; Kamat, Nishant; Syed Mohammed, Khasim Subject: Re: [Patch 02/17] U-Boot-V2:Common:Clock Handle case of clockrollover for get_time_ns
cycle_delta = (cycle_now - cs->cycle_last) & cs->mask;Look closer, the rollover case is handled implicitly by the unsigned arithmetics.
Agreed for logic when mask is the max value attainable. IMHO, The concept of cs->mask is messy. We do need a min, max and a mask. When: cycle_now = cs->read(); cs->cycle_last = cycle_now;
Now cycle_now == cs->cycle_last...
(cycle_now - cs->cycle_last) & cs->mask is wrong.
...so 0 & cs->mask = 0
This makes no sense, but this is not the order of execution in current code.
Assumptions made: A) The bits masked out by cs->mask will remain constant. This may not be true.
Eh? That's why they are masked out.
B) Roll over assume the min is 0 and max is cs->mask. This need not be the case. It would be good to be explicit.
Do you know any counter that does not start counting from zero? If you have, noone prevents you from substracting the value in your clocksource read function.
You are right, we do not have a possibility to detect a double rollover without interrupts. Normally this is not an issue as this code is used in timeout polling loops where the current time is polled often enough. Anyway, maybe some comment should mention that this function is not useful to measure long periods of time where 'long' is highly architecture specific.
Yes, there are indenting issue + no doxygen documentation.. It helps as clock_source is critical implementation required in the system.
Regards, Nishanth Menon