[U-Boot-Users] [Patch 02/17] U-Boot-V2:Common:Clock Handle case of clock rollover for get_time_ns
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 Menonx0nishan@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);
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 Menonx0nishan@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);
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; (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
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
Sascha,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Tuesday, June 03, 2008 9:15 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 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
On Tue, Jun 03, 2008 at 09:39:51AM -0500, Menon, Nishanth wrote:
Sascha,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Tuesday, June 03, 2008 9:15 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 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
Sascha,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Tuesday, June 03, 2008 10:14 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 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 Menonx0nishan@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;
participants (2)
-
Menon, Nishanth -
Sascha Hauer