I2C hard coded timeout limiting block size to 1.5 KB

[mic006]

There is an hard coded constant timeout of 150 ms in the I2C kernel driver (I2C_TIMEOUT_MS in i2c-bcm2708.c).
At 100 KHz, it means that you cannot send (and receive ?) a data block longer than 1.5 KB.

I was trying to send a 2KB block, getting the timeout issue.

Is there any good reason (I2C spec ?, HW constraint ?) for this hardcoded value ?
Userspace app can set an I2C timeout via I2C_TIMEOUT ioctl(), so maybe the constant should be replaced by the adap->timeout value.

[cbeytas]

I think this is a very good idea. I've modified the driver in my kernel and it is working great with the I2C_TIMEOUT ioctl. It defaults to a generous 100 jiffies (1 second).

In i2c-bcm2708.c change:
ret = wait_for_completion_timeout(&bi->done,
msecs_to_jiffies(I2C_TIMEOUT_MS));
to:
ret = wait_for_completion_timeout(&bi->done, adap->timeout);

Then get rid of the I2C_TIMEOUT_MS define.

[pgroe]

+1 for that issue to be patched in the master branch.

This issue is especially nasty, because the driver doesn't recover properly from that software timeout, so that the next transfer fails as well (at least it recovers correctly at the next transfer after the timeout).

Some pictures from the scope: (image 1 to 3 from the out timing transfer, the last image from the next transfer, or what's left of it)

Should there be opened another issue for the problem with not recovering correctly?

[popcornmix]

ping @pelwell @notro thoughts on the timeout patch?

[notro]

I think it's a good idea and it seems to be a common pattern:

$ grep -Ir -A3 wait_for_completion_timeout drivers/i2c/busses/
drivers/i2c/busses/i2c-cadence.c:       time_left = wait_for_completion_timeout(&id->xfer_done, adap->timeout);
--
drivers/i2c/busses/i2c-riic.c:          time_left = wait_for_completion_timeout(&riic->msg_done, riic->adapter.timeout);
--
drivers/i2c/busses/i2c-davinci.c:       time_left = wait_for_completion_timeout(&dev->cmd_complete,
drivers/i2c/busses/i2c-davinci.c-                                               dev->adapter.timeout);
--
drivers/i2c/busses/i2c-hix5hd2.c:       timeout = wait_for_completion_timeout(&priv->msg_complete,
drivers/i2c/busses/i2c-hix5hd2.c-                                             priv->adap.timeout);
--
drivers/i2c/busses/i2c-st.c:    timeout = wait_for_completion_timeout(&i2c_dev->complete,
drivers/i2c/busses/i2c-st.c-                    i2c_dev->adap.timeout);
--
drivers/i2c/busses/i2c-bfin-twi.c:              if (!wait_for_completion_timeout(&iface->complete,
drivers/i2c/busses/i2c-bfin-twi.c-                      adap->timeout)) {
--
drivers/i2c/busses/i2c-bfin-twi.c:              if (!wait_for_completion_timeout(&iface->complete,
drivers/i2c/busses/i2c-bfin-twi.c-                      adap->timeout)) {
--
drivers/i2c/busses/i2c-nomadik.c:       timeout = wait_for_completion_timeout(
drivers/i2c/busses/i2c-nomadik.c-               &dev->xfer_complete, dev->adap.timeout);
--
drivers/i2c/busses/i2c-at91.c:  time_left = wait_for_completion_timeout(&dev->cmd_complete,
drivers/i2c/busses/i2c-at91.c-                                        dev->adapter.timeout);

http://lxr.free-electrons.com/ident?i=i2c_register_adapter

static int i2c_register_adapter(struct i2c_adapter *adap)
{
...
        /* Set default timeout to 1 second if not already set */
        if (adap->timeout == 0)
                adap->timeout = HZ;

http://lxr.free-electrons.com/ident?i=i2cdev_ioctl

static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
...
        case I2C_TIMEOUT:
                /* For historical reasons, user-space sets the timeout
                 * value in units of 10 ms.
                 */
                client->adapter->timeout = msecs_to_jiffies(arg * 10);
                break;

[pelwell]

As notro says, it's the way other I2C drivers derive the timeout, and since it is an error condition I see no harm in making the timeout larger (within reason).

[popcornmix]

Same for SPI_TIMEOUT_MS in drivers/spi/spi-bcm2708.c?

[pelwell]

Yes, why not - 1 second seems to be the standard for SPI (I couldn't see an SPI equivalent to the I2C adapter timeout).

[pgroe]

Regarding the patch I can confirm, that it solves the limitation to 1.5KB. I successfully transferred up to 5KB at once (the maximum data length my target device supports).

There's another point I would like to patch:

My target device uses 16 bit registers. If I trigger a read command (using I2C_RDWR in combined mode with writing the register address and reading from that register), I sometimes run in timeout in another way:
Poll for transfer start bit (i2c-bcm2708.c, line 194) times out with running out of wait_loops.

I therefore increased
#define I2C_WAIT_LOOP_COUNT 40
to
#define I2C_WAIT_LOOP_COUNT 200

Another way would be to also use the adap->timeout value.

But using my patch I reduced the timeouts in the combined transfers from about 20% to 0% for me.

[pelwell]

Please rpi-update, test, and close if appropriate.

[pelwell]

ping @mic006 @pgroe

[pgroe]

Sry, didn't have much time. Just tried the new master build. Works like a charm :)

All tests succeeded as they did with my self built patched kernel. Successfully read up to 1024 bytes (my target does not allow more read bytes at once) and wrote 5120 bytes at once (whole memory).