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

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

[Menon, Nishanth]

get_time_ns does a simplistic delta = cycle_now - cycle_last. It is possible that the h/w counter reached max and reset back to 0.
This patch addresses this issue by checking for rollover condition.

NOTE 1: This does not guarantee that you cannot confuse get_time_ns. You could possibly wait for two reset cycles and then get a messed up value.
To fix that we may need interrupt mode timer tick - something on the lines of jiffies on linux.
NOTE 2: the question of cs->mask is not clear. if the mask is for the tick, then it is better done with (cycle_now & cs->mask) - (cs->cycle_last & cs->mask). Do we need min and max for register read?

Signed-off-by: Nishanth Menon<x0nishan@ti.com>

---
 common/clock.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Index: u-boot-v2.git/common/clock.c
===================================================================
--- u-boot-v2.git.orig/common/clock.c	2008-05-20 17:19:44.000000000 -0500
+++ u-boot-v2.git/common/clock.c	2008-05-20 17:26:31.000000000 -0500
@@ -37,14 +37,24 @@
 uint64_t get_time_ns(void)
 {
 	struct clocksource *cs = current_clock;
-        uint64_t cycle_now, cycle_delta;
+	uint64_t cycle_now, cycle_delta = 0;
         uint64_t ns_offset;
 
         /* read clocksource: */
         cycle_now = cs->read();
 
         /* calculate the delta since the last call: */
-        cycle_delta = (cycle_now - cs->cycle_last) & cs->mask;
+
+	/* Handle rollover case */
+	if (cycle_now < cs->cycle_last) {
+		/* FIXME: I am not convinced about the cs->mask operation..
+		 * I am assuming cs->mask is max! Probably need to change
+		 * clocksource structure to have min and max
+		 */
+		cycle_delta = cs->mask - (cs->cycle_last & cs->mask);
+		cs->cycle_last = 0;
+	}
+	cycle_delta += (cycle_now & cs->mask) - (cs->cycle_last & cs->mask);
 
         /* convert to nanoseconds: */
         ns_offset = cyc2ns(cs, cycle_delta);

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

[Sascha Hauer]

On Wed, May 21, 2008 at 11:25:15AM -0500, Menon, Nishanth wrote:
> get_time_ns does a simplistic delta = cycle_now - cycle_last. It is possible that the h/w counter reached max and reset back to 0.
> This patch addresses this issue by checking for rollover condition.
> 
> NOTE 1: This does not guarantee that you cannot confuse get_time_ns. You could possibly wait for two reset cycles and then get a messed up value.
> To fix that we may need interrupt mode timer tick - something on the lines of jiffies on linux.
> NOTE 2: the question of cs->mask is not clear. if the mask is for the tick, then it is better done with (cycle_now & cs->mask) - (cs->cycle_last & cs->mask). Do we need min and max for register read?
> 
> Signed-off-by: Nishanth Menon<x0nishan@ti.com>
> 
> ---
>  common/clock.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> Index: u-boot-v2.git/common/clock.c
> ===================================================================
> --- u-boot-v2.git.orig/common/clock.c	2008-05-20 17:19:44.000000000 -0500
> +++ u-boot-v2.git/common/clock.c	2008-05-20 17:26:31.000000000 -0500
> @@ -37,14 +37,24 @@
>  uint64_t get_time_ns(void)
>  {
>  	struct clocksource *cs = current_clock;
> -        uint64_t cycle_now, cycle_delta;
> +	uint64_t cycle_now, cycle_delta = 0;
>          uint64_t ns_offset;
>  
>          /* read clocksource: */
>          cycle_now = cs->read();
>  
>          /* calculate the delta since the last call: */
> -        cycle_delta = (cycle_now - cs->cycle_last) & cs->mask;

Look closer, the rollover case is handled implicitly by the unsigned
arithmetics.

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.

regards,
  Sascha

> +
> +	/* Handle rollover case */
> +	if (cycle_now < cs->cycle_last) {
> +		/* FIXME: I am not convinced about the cs->mask operation..
> +		 * I am assuming cs->mask is max! Probably need to change
> +		 * clocksource structure to have min and max
> +		 */
> +		cycle_delta = cs->mask - (cs->cycle_last & cs->mask);
> +		cs->cycle_last = 0;
> +	}
> +	cycle_delta += (cycle_now & cs->mask) - (cs->cycle_last & cs->mask);
>  
>          /* convert to nanoseconds: */
>          ns_offset = cyc2ns(cs, cycle_delta);
> 

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

[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

[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/ <-

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

[Menon, Nishanth]

Sascha,
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 03, 2008 9:15 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 ofclockrollover for get_time_ns
> 
> > 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.
For the sake of discussion:
unsigned int now = 0x120;
unsigned int last = 0x128;
unsigned int mask = 0xFFF0;
unsigned int delta1 = now - last;
unsigned int delta2 = (now & mask) - (last & mask);
printf ("delta1=0x%08X maskdelta1=0x%08X delta2=0x%08X\n",delta1, delta1 & mask, delta2);

Output will be:
delta1=0xFFFFFFF8 maskdelta1=0x0000FFF0 delta2=0x00000000

What we will get now is maskdelta1, while delta2 is the right value.

> 
> > 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
I do not. I am just being a paranoid idiot ;)..

> have, noone prevents you from substracting the value in your clocksource
> read function.
Max however is assumed to be cs->mask then. We could have timers which can be configured for ticking till a configured max. probably my paranoia kicking in again?

Regards,
Nishanth Menon

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

[Sascha Hauer]

On Tue, Jun 03, 2008 at 09:39:51AM -0500, Menon, Nishanth wrote:
> Sascha,
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, June 03, 2008 9:15 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 ofclockrollover for get_time_ns
> > 
> > > 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.
> For the sake of discussion:
> unsigned int now = 0x120;
> unsigned int last = 0x128;
> unsigned int mask = 0xFFF0;
> unsigned int delta1 = now - last;
> unsigned int delta2 = (now & mask) - (last & mask);
> printf ("delta1=0x%08X maskdelta1=0x%08X delta2=0x%08X\n",delta1, delta1 & mask, delta2);
> 
> Output will be:
> delta1=0xFFFFFFF8 maskdelta1=0x0000FFF0 delta2=0x00000000
> 
> What we will get now is maskdelta1, while delta2 is the right value.

It's not that I thought of the code by myself, I just looked into the
kernel and it's exactly like this in the kernel code. So you either just
found a kernel bug or we both understand something wrong.

> 
> > 
> > > 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
> I do not. I am just being a paranoid idiot ;)..
> 
> > have, noone prevents you from substracting the value in your clocksource
> > read function.
> Max however is assumed to be cs->mask then. We could have timers which can be configured for ticking till a configured max. probably my paranoia kicking in again?

If that's the only possibility to program this timer then go out and
kill the chip designer ;)

Sascha


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

[U-Boot-Users] [Patch 02/17 Try 2] U-Boot-V2:Common:Clock Handle caseofclockrollover for get_time_ns

[Menon, Nishanth]

Sascha,
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 03, 2008 10:14 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 caseofclockrollover for get_time_ns
> 
> > Output will be:
> > delta1=0xFFFFFFF8 maskdelta1=0x0000FFF0 delta2=0x00000000
> >
> > What we will get now is maskdelta1, while delta2 is the right value.
> 
> It's not that I thought of the code by myself, I just looked into the
> kernel and it's exactly like this in the kernel code. So you either just
> found a kernel bug or we both understand something wrong.

Here is a revisit of this patch. The change will ensure cycle_now and last will be masked.

Signed-off-by: Nishanth Menon<x0nishan@ti.com>

---
 common/clock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: u-boot.v2/common/clock.c
===================================================================
--- u-boot.v2.orig/common/clock.c	2007-10-10 21:50:14.000000000 -0500
+++ u-boot.v2/common/clock.c	2008-06-03 11:16:05.000000000 -0500
@@ -41,7 +41,7 @@
         uint64_t ns_offset;
 
         /* read clocksource: */
-        cycle_now = cs->read();
+	cycle_now = cs->read() & cs->mask;
 
         /* calculate the delta since the last call: */
         cycle_delta = (cycle_now - cs->cycle_last) & cs->mask;