[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

Timur Tabi
2009-09-04T14:09:25Z

On Fri, Sep 4, 2009 at 4:25 AM, Wolfgang Denk<wd@denx.de> wrote:

> There are actually two parts in Timur's mail:
>
> 1) 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?

> 2) 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?

--
Timur Tabi
Linux kernel developer at Freescale