[U-Boot-Users] OCOTEA get_timer() bug

[U-Boot-Users] OCOTEA get_timer() bug

[Andrew Wozniak]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Wolfgang Denk]

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

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

[U-Boot-Users] OCOTEA get_timer() bug

[Stefan Roese]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Andrew Wozniak]

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
> 

[U-Boot-Users] OCOTEA get_timer() bug

[Stefan Roese]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Andrew Wozniak]

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
> 

[U-Boot-Users] OCOTEA get_timer() bug

[Travis B. Sawyer]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Travis B. Sawyer]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Travis B. Sawyer]

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

[U-Boot-Users] OCOTEA get_timer() bug

[Andrew Wozniak]

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