[U-Boot-Users] OCOTEA get_timer() bug
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
In message 42485A32.6090501@mc.com 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.
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
On Monday 28 March 2005 21:58, Wolfgang Denk wrote:
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?
No. The 440 port was done by Scott McNutt smcnutt@artsyncp.com, 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
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
Stefan Roese wrote:
On Monday 28 March 2005 21:58, Wolfgang Denk wrote:
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?
No. The 440 port was done by Scott McNutt smcnutt@artsyncp.com, 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
Hello,
On Wednesday 30 March 2005 01:17, Andrew Wozniak wrote:
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.
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
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
Stefan Roese wrote:
Hello,
On Wednesday 30 March 2005 01:17, Andrew Wozniak wrote:
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.
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 wrote:
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.
Stefan Roese wrote:
Hello,
On Wednesday 30 March 2005 01:17, Andrew Wozniak wrote:
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
Stefan Roese wrote:
On Monday 28 March 2005 21:58, Wolfgang Denk wrote:
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?
No. The 440 port was done by Scott McNutt smcnutt@artsyncp.com, 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
Andrew Wozniak wrote:
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 */
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
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
Travis B. Sawyer wrote:
Andrew Wozniak wrote:
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 */
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
participants (4)
-
Andrew Wozniak -
Stefan Roese -
Travis B. Sawyer -
Wolfgang Denk