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.