[U-Boot-Users] [PATCH] Bug fix for MIPS timer

[Robert Delien]

Hello Andrew,

Thank you for reviewing. It's always good to see somebody taking the time
and effort to improve the code. Though by the level of broken-ness it seems
nobody is actually using U-Boot on the MIPS platform anymore, except for
us. The timer was broken, the whole GTH2 board support is broken and the
other boards are full of warnings...

> Why did you change the while loop and add the rollover code?  The old
code:
> -   while ((ulong)((mips_count_get() - start)) < tmo)
> -      /*NOP*/;
> seems perfectly sane to me and should work whether or not you use
> read_32bit_cp0_register(CP0_COUNT) or mips_count_get() assuming tmo
> gets set correctly.

I have to dig deep for that; It's almost a year ago since I changed that.
It may be an Obi Wan, but whatever the problem was, this implementation is
perectly sane too ;-)

> Is this something NXP specific?

No, it's MIPS specific: There's no use for a 'mips_count_get'-function. The
proper way to read the count register is to use the already available
function 'read_32bit_cp0_register', because the count register is just
another MIPS coprocessor 0 register.

> In udelay() I don't like this part that silently truncates delays.
> This could lead to some ugly hard to find timing bugs.
>
> +   /* Safeguard for too-long delays */
> +   if (delayTicks >= 0x80000000)
> +      delayTicks = 0x7FFFFFFF;

Well, it still beats the previous implementation that allowed a
wrap-around, resulting in a much shorter delay than desired. And with no
error logging in place, truncating is the only thing that could be done.

> I would calculate the max. count you can handle in the code and run
> the timer polling loop multiple times if need be.

We're talking about delays of over 35 seconds being truncated. I think it's
hardly worth the extra code to support such long delays in a function
called 'udelay'. People using it for delays that long have other problems.

> long lines

> might be a good idea to throw in a compile time warning message if
> CFG_HZ != 1000

No, I'm sorry; I can't put a warning everywhere people might make a mistake
;-). Besides that, this would have to be global warning, covering all
targets. If such a check would be desired, a mechanism should be
implemented for that, which could also check other commonly made mistakes.
But I'd rather suggest people to read the manual ;-)

With kind regards,

      Robert.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20070625/8ddba456/attachment.htm