On Fri, Sep 4, 2009 at 4:25 AM, Wolfgang Denkwd@denx.de wrote:
There are actually two parts in Timur's mail:
- First part is the question if the timeout, which is currently set
to 250 us, should be raised to 1,000 us.
I cannot answer this question. I don't even understand why the i2c_wait4bus() function is needed at all.
Can you explain? I don't know enough about the I2C protocol. Why is i2c_wait4bus unnecessary?
- The second part is if the timeout definition should be changed
from being based on CONFIG_SYS_HZ to a plain numeric constant. Assuming I2C_TIMEOUT is intended to be a period of time (as the name suggests), then "(CONFIG_SYS_HZ / 4)" is clearly wrong as it's a frequency and not a time.
In this case, a comment should be added explainign what I2C_TIMEOUT means.
It took me a while to track it down, but I finally found the commit that make the change: SHA 09d318a8bb1444ec92e31cafcdba877eb9409e58, from Kumar about a year ago:
fsl_i2c: Use timebase timer functions instead of get_timer()
The current implementation of get_timer() is only really useful after we have relocated u-boot to memory. The i2c code is used before that as part of the SPD DDR setup.
We actually have a bug when using the get_timer() code before relocation because the .bss hasn't been setup and thus we could be reading/writing a random location (probably in flash).
Signed-off-by: Kumar Gala galak@kernel.crashing.org
Specifically, it does this:
while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) { - if (get_timer(timeval) > I2C_TIMEOUT) { + if ((get_ticks() - timeval) > usec2ticks(I2C_TIMEOUT)) return -1; - } }
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?