OCOTEA get_timer() bug

U-Boot-Users Andrew Wozniak 4 participants 10 messages Mar 28, 2005 – Apr 5, 2005

Share & tools

Andrew Wozniak

author

2005-03-28 19:25 UTC #0 lore raw

I found this bug a while ago and have finally gotten around to looking at it because I needed its service.

In previous posts to this email list, Wolfgang had mentioned that the "time unit" returned by get_timer() is 1 msec. Close examination of interrupt_init_cpu() at cpu/ppc4xx/interrupts.c suggests a timer interval setup of 10 msec!

A quick experiment via u-boot application, shows this to be true:

printf("...tic\n");
tic = get_timer(0);
udelay(2000000); /* 2sec delay so that I can "see" it */
toc = get_timer(0);
printf("...toc\n %d %d\n", tic, toc);

run it:
...tic
...toc
1278 1478

therefore:
200 = delta
2sec/200 = 10msec time unit from get_timer()

I've tested the following and propose it as a fix, replace this:
val = gd->bd->bi_intfreq/100; /* 10 msec */
with:
val = gd->bd->bi_intfreq/1000; /* 1 msec */

I've also tested some of the network related functions to insure that no timeout conditions have appeared.

I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.

Sorry for the long post, Andrew

Wolfgang Denk

2005-03-28 19:58 UTC #1 lore raw

Quoted 7 lines · In message <[email protected]> you wrote:

I found this bug a while ago and have finally gotten around to looking at it because I needed its service.
In previous posts to this email list, Wolfgang had mentioned that the "time unit" returned by get_timer() is 1 msec. Close examination of interrupt_init_cpu() at cpu/ppc4xx/interrupts.c suggests a timer interval setup of 10 msec!

I think you are right.

Quoted 10 lines

I've tested the following and propose it as a fix, replace this:
   val = gd->bd->bi_intfreq/100;	 /* 10 msec */
with:
   val = gd->bd->bi_intfreq/1000; /* 1 msec */
I've also tested some of the network related functions to insure that no timeout conditions have appeared.
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.

I don't know why the 440 is handled separately.

Stefan, do you have an idea?

Best regards,

Wolfgang Denk

Signature · 5 lines

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You know that feeling when you're leaning back  on  a  stool  and  it
starts to tip over? Well, that's how I feel all the time.
- Steven Wright

Stefan Roese

2005-03-29 06:26 UTC #2 lore raw

Quoted 8 lines · Wolfgang Denk

Quoted 8 lines
I've tested the following and propose it as a fix, replace this:
   val = gd->bd->bi_intfreq/100;	 /* 10 msec */
with:
   val = gd->bd->bi_intfreq/1000; /* 1 msec */
I've also tested some of the network related functions to insure that no timeout conditions have appeared.

Quoted 6 lines

Quoted 2 lines
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.
I don't know why the 440 is handled separately.
Stefan, do you have an idea?

No. The 440 port was done by Scott McNutt <[email protected]>, if I remember right. But it seems that the fix is correct. Scott (or any other 440 users), any comments on this?

Best regards,
Stefan

Andrew Wozniak

author

2005-03-29 23:17 UTC #3 lore raw

Hello,

My only concern with the fix is that it will break something else that assumes the timer interval to be 10msec. There are a few "files" that depend on it for timeout operations:

cpu/ppc4xx/440gx_enet.c

          if ((time_now - time_start) > 3000)   <--- 30 sec timo?

net/bootp.c

	bp->bp_secs = htons(get_timer(0) / CFG_HZ);
	...
         BootpID += get_timer(0);

net/tftp.c

	TftpOurPort = 1024 + (get_timer(0) % 3072);

net/net.c
post/rtc.c
tools/updater/flash_hw.c

-Andrew

Quoted 26 lines · Stefan Roese

Quoted 8 lines · Wolfgang Denk
Quoted 8 lines
I've tested the following and propose it as a fix, replace this:
val = gd->bd->bi_intfreq/100; /* 10 msec */
with:
val = gd->bd->bi_intfreq/1000; /* 1 msec */
I've also tested some of the network related functions to insure that no timeout conditions have appeared.
Quoted 6 lines
Quoted 2 lines
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.
I don't know why the 440 is handled separately.
Stefan, do you have an idea?
No. The 440 port was done by Scott McNutt <[email protected]>, if I remember right. But it seems that the fix is correct. Scott (or any other 440 users), any comments on this?
Best regards,
Stefan

Stefan Roese

2005-03-30 07:13 UTC #4 lore raw

Hello,

Quoted 6 lines · Andrew Wozniak

My only concern with the fix is that it will break something else that assumes the timer interval to be 10msec. There are a few "files" that depend on it for timeout operations:
cpu/ppc4xx/440gx_enet.c
          if ((time_now - time_start) > 3000)   <--- 30 sec timo?

This is 440 specific, but in the comments a few lines above 3 seconds timeout is mentioned. So even this seems to be a bug with a 10 ms timer interval.

Quoted 11 lines

net/bootp.c
	bp->bp_secs = htons(get_timer(0) / CFG_HZ);
	...
         BootpID += get_timer(0);
net/tftp.c
	TftpOurPort = 1024 + (get_timer(0) % 3072);
net/net.c
post/rtc.c
tools/updater/flash_hw.c

All this is generic code (not 440 spcific), so a 1 ms timer interval is assumed.

Give me a few days and I will test it on an OCOTEA board.

Best regards,
Stefan

Andrew Wozniak

author

2005-03-31 18:26 UTC #5 lore raw

Hello Stefan,

Testing on a real OCOTEA board will be a great help.

Our target is a custom 440gx board with an OCOTEA port of U-Boot. I will try to do some testing to determine if there are any side affects on our board.

-andrew

Quoted 38 lines · Stefan Roese

Hello,
Quoted 6 lines · Andrew Wozniak
My only concern with the fix is that it will break something else that assumes the timer interval to be 10msec. There are a few "files" that depend on it for timeout operations:
cpu/ppc4xx/440gx_enet.c
         if ((time_now - time_start) > 3000)   <--- 30 sec timo?
This is 440 specific, but in the comments a few lines above 3 seconds timeout is mentioned. So even this seems to be a bug with a 10 ms timer interval.
Quoted 11 lines
net/bootp.c
	bp->bp_secs = htons(get_timer(0) / CFG_HZ);
	...
        BootpID += get_timer(0);
net/tftp.c
	TftpOurPort = 1024 + (get_timer(0) % 3072);
net/net.c
post/rtc.c
tools/updater/flash_hw.c
All this is generic code (not 440 spcific), so a 1 ms timer interval is assumed.
Give me a few days and I will test it on an OCOTEA board.
Best regards,
Stefan

Travis B. Sawyer

2005-04-04 17:35 UTC #6 lore raw

Quoted 26 lines · Andrew Wozniak

Hello Stefan,
Our target is a custom 440gx board with an OCOTEA port of U-Boot. I will try to do some testing to determine if there are any side affects on our board.
Quoted 18 lines · Stefan Roese
Hello,
Quoted 6 lines · Andrew Wozniak
My only concern with the fix is that it will break something else that assumes the timer interval to be 10msec. There are a few "files" that depend on it for timeout operations:
cpu/ppc4xx/440gx_enet.c
         if ((time_now - time_start) > 3000)   <--- 30 sec timo?
This is 440 specific, but in the comments a few lines above 3 seconds timeout is mentioned. So even this seems to be a bug with a 10 ms timer interval.

Sorry I've been a bit remiss in responding, been away from the email due to a new addition to the family (Son #2 on 26 april 2005)

The 440gx port I did was essentially a port of the 440gp by Scott McNutt. Looks like I blindly accepted some stuff (er, missed some stuff).

I'll attempt to give it a whirl on our custom board and possibly on my ocotea also.

I'll be going in to work some this week...

-travis

Travis B. Sawyer

2005-04-05 13:26 UTC #7 lore raw

In reply to Stefan Roese

Quoted 16 lines · Stefan Roese

Quoted 13 lines · Wolfgang Denk
Quoted 10 lines
I've tested the following and propose it as a fix, replace this:
val = gd->bd->bi_intfreq/100; /* 10 msec */
with:
val = gd->bd->bi_intfreq/1000; /* 1 msec */
I've also tested some of the network related functions to insure that no timeout conditions have appeared.
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.
I don't know why the 440 is handled separately.

Quoted 9 lines

Quoted 1 line
Stefan, do you have an idea?
No. The 440 port was done by Scott McNutt <[email protected]>, if I remember right. But it seems that the fix is correct. Scott (or any other 440 users), any comments on this?
Best regards,
Stefan

Looking back at my original patch (using sourceforge cvs browse) The patch for the 440 GX did not change that portion of code, so the ebony should also exhibit this behavior. I recently rx'ed a message from one of my users that the timer was returning ~10x of what was expected. That should have been a clue to me...

I'll give the above fix a whirl on my custom board and report back.

-travis

Travis B. Sawyer

2005-04-05 14:08 UTC #8 lore raw

In reply to Andrew Wozniak

Quoted 8 lines · Andrew Wozniak

I found this bug a while ago and have finally gotten around to looking at it because I needed its service.
I've tested the following and propose it as a fix, replace this:
val = gd->bd->bi_intfreq/100; /* 10 msec */
with:
val = gd->bd->bi_intfreq/1000; /* 1 msec */

Quoted 8 lines

I've also tested some of the network related functions to insure that no timeout conditions have appeared.
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.
Sorry for the long post, Andrew

Andrew et al:

Make a patch, it gets my sign off (for what it's worth ;)

-travis

Andrew Wozniak

author

2005-04-05 22:52 UTC #9 lore raw

Travis,

Thanks for looking into this. I'll post a patch - when I figure out how to create a patch file. Haven't done anything like that yet.

-andy

Quoted 26 lines · Travis B. Sawyer

Quoted 8 lines · Andrew Wozniak
I found this bug a while ago and have finally gotten around to looking at it because I needed its service.
I've tested the following and propose it as a fix, replace this:
val = gd->bd->bi_intfreq/100; /* 10 msec */
with:
val = gd->bd->bi_intfreq/1000; /* 1 msec */
Quoted 8 lines
I've also tested some of the network related functions to insure that no timeout conditions have appeared.
I want to post this patch but would prefer a code review from those that are more familiar with get_timer() use within the OCOTEA codebase.
Sorry for the long post, Andrew
Andrew et al:
Make a patch, it gets my sign off (for what it's worth ;)
-travis