[U-Boot-Users] [Patch 02/17] U-Boot-V2:Common:Clock Handle case of clockrollover for get_time_ns

[Menon, Nishanth]

Sascha,
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 03, 2008 3:08 AM
> To: Menon, Nishanth
> Cc: u-boot-users at lists.sourceforge.net; Laurent Desnogues; dirk.behme at googlemail.com;
> philip.balister at 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;
(cycle_now - cs->cycle_last) & cs->mask is wrong. Assumptions made:
A) The bits masked out by cs->mask will remain constant. This may not be true.
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.
> 
> 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