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

[Sascha Hauer]

On Tue, Jun 03, 2008 at 07:47:25AM -0500, Menon, Nishanth wrote:
> 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;

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
> 

-- 
Pengutronix e.K. - Linux Solutions for Science and Industry
-----------------------------------------------------------
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite -> http://www.pengutronix.de/impressum/ <-