[BUG] i2c-designware silently fails on long transfers
Russell King - ARM Linux
linux at armlinux.org.uk
Fri Nov 18 11:35:42 PST 2016
With reference to this commit:
commit d39f77b06a712fcba6185a20bb209e357923d980
Author: Andrew Jackson <Andrew.Jackson at arm.com>
Date: Fri Nov 7 12:10:44 2014 +0000
i2c: designware: prevent early stop on TX FIFO empty
If the Designware core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN
set to zero, allowing the TX FIFO to become empty causes a STOP
condition to be generated on the I2C bus. If the transmit FIFO
threshold is set too high, an erroneous STOP condition can be
generated on long transfers - particularly where the interrupt
latency is extended.
Signed-off-by: Andrew Jackson <Andrew.Jackson at arm.com>
Signed-off-by: Liviu Dudau <Liviu.Dudau at arm.com>
Tested-by: Mika Westerberg <mika.westerberg at linux.intel.com>
Signed-off-by: Wolfram Sang <wsa at the-dreams.de>
The TDA998x driver issues long I2C transfers to read the EDID from the
device - and userspace can also issue large transfers too. However,
if a DW core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN set as
zero, the above commit doesn't seem to solve the problem. During
boot, with the patch below, I see:
[ 1.736549] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x10
[ 1.736564] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510
[ 1.736608] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
[ 1.736799] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514
[ 1.736819] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510
...
[ 1.737986] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
[ 1.738010] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514
[ 1.738034] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
[ 1.738039] random: fast init done
[ 1.740120] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x714
[ 1.740231] i2c_dw_xfer: ffffffc97657b770:1 -> ffffffc97657b770:1 (0:0) [0 0 3 0] 8 [tx:ffffffc976682380:47] [rx:ffffffc9766823c9:55]
[ 1.740249] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 93
[ 1.746979] Raw EDID:
[ 1.747934] 00 ff ff ff ff ff ff 00 34 a9 96 a2 01 01 01 01
[ 1.752342] 00 17 01 03 80 80 48 78 0a da ff a3 58 4a a2 29
[ 1.756748] 17 49 4b 21 08 00 31 40 45 40 61 40 81 80 01 01
[ 1.761153] 01 01 01 01 01 01 02 3a 80 d0 72 38 2d 40 10 2c
[ 1.765555] 45 80 ba 88 21 00 00 1e 02 00 d0 4e 30 09 12 54
[ 1.769958] 01 08 02 00 23 36 01 40 01 05 00 80 a1 4c 4b 49
[ 1.774361] 22 00 00 40 03 00 28 00 23 01 20 00 01 88 00 01
[ 1.778762] 08 00 00 40 00 02 03 04 0a 00 80 00 02 00 00 40
The significant thing is the "i2c_dw_xfer" line, where I add a print of
the current state. Here, we can see that the transfer is mid-way, but
a stop condition has been generated by the hardware, leaving 55 bytes
to be received.
Unfortunately, the i2c-designware driver ignores this, and believes that
the transfer completed both fully and successfully, but returns bogus
data to userspace or the kernel driver. That's really _bad_ behaviour
by the driver - it should at least return an error.
This problem is _soo_ bad that on my Juno, I can't run Xorg (it hits
this every time we try to read the EDID) nor can I boot with the TV
connected (it hits this every boot as well.)
I'd go as far as to say that the i2c-designware hardware, when
configured with this option set to zero, is fundamentally broken for OS
which do not provide any guarantee for interrupt latency, such as Linux.
The commit above tries to mitigate this by reducing the Tx FIFO
threshold, so the interrupt is raised sooner, but that's clearly not
enough for reliable operation.
Another mitigation would be to lower the I2C bus frequency on Juno from
400kHz to 100kHz, so that there's 4x longer IRQ latency possible.
However, even that isn't going to be reliable - even going to 100kHz
isn't going to allow the above case to be solved - the interrupt is
delayed by around 2ms, and it takes about 1.4ms to send/receive 16 bytes
at 100kHz. (9 * 16 / (100*10^3)).
So, I think all hope is lost for i2c-designware on Juno to cope with
reading the EDID from TDA998x reliably.
I have one patch which solves a problem in the accounting of bytes, and
another to ensure that we return an error for an incomplete transfer,
both will be sent threaded to this mail.
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 11e866d05368..060ae9e5a916 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -752,6 +752,15 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
goto done;
}
+ printk(KERN_DEBUG "%s: %p:%d -> %p:%d (%d:%d) [%x %x %x %x] %d [tx:%p:%d] [rx:%p:%d]\n",
+ __func__, msgs, num,
+ dev->msgs, dev->msgs_num,
+ dev->msg_write_idx, dev->msg_read_idx,
+ dev->cmd_err, dev->msg_err, dev->status, dev->abort_source,
+ dev->rx_outstanding,
+ dev->tx_buf, dev->tx_buf_len,
+ dev->rx_buf, dev->rx_buf_len);
+
if (dev->msg_err) {
ret = dev->msg_err;
goto done;
@@ -857,7 +866,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
enabled = dw_readl(dev, DW_IC_ENABLE);
stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
- dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
+ dev_printk(KERN_DEBUG, dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
return IRQ_NONE;
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
More information about the linux-arm-kernel
mailing list