[BUG] i2c-designware silently fails on long transfers

[BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

With reference to this commit:

commit d39f77b06a712fcba6185a20bb209e357923d980
Author: Andrew Jackson <Andrew.Jackson@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@arm.com>
    Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
    Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Wolfram Sang <wsa@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.

[BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

With reference to this commit:

commit d39f77b06a712fcba6185a20bb209e357923d980
Author: Andrew Jackson <Andrew.Jackson@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@arm.com>
    Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
    Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Wolfram Sang <wsa@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.

[PATCH 1/2] i2c: designware: report short transfers

[Russell King]

Rather than reporting success for a short transfer due to interrupt
latency, report an error both to the caller, as well as to the kernel
log.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-designware-core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 9703fe392543..c53058d6139c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -758,7 +758,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
 	/* no error */
-	if (likely(!dev->cmd_err)) {
+	if (likely(!dev->cmd_err && !dev->status)) {
 		ret = num;
 		goto done;
 	}
@@ -768,6 +768,11 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		ret = i2c_dw_handle_tx_abort(dev);
 		goto done;
 	}
+
+	if (dev->status)
+		dev_err(dev->dev,
+			"transfer terminated early - interrupt latency too high?\n");
+
 	ret = -EIO;
 
 done:
-- 
2.7.4

[PATCH 1/2] i2c: designware: report short transfers

[Russell King]

Rather than reporting success for a short transfer due to interrupt
latency, report an error both to the caller, as well as to the kernel
log.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-designware-core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 9703fe392543..c53058d6139c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -758,7 +758,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
 	/* no error */
-	if (likely(!dev->cmd_err)) {
+	if (likely(!dev->cmd_err && !dev->status)) {
 		ret = num;
 		goto done;
 	}
@@ -768,6 +768,11 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		ret = i2c_dw_handle_tx_abort(dev);
 		goto done;
 	}
+
+	if (dev->status)
+		dev_err(dev->dev,
+			"transfer terminated early - interrupt latency too high?\n");
+
 	ret = -EIO;
 
 done:
-- 
2.7.4

[PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Russell King]

When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
count the number of bytes:

	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);

	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
		if (rx_limit - dev->rx_outstanding <= 0)
			break;
		rx_limit--;
		dev->rx_outstanding++;
	}

DW_IC_RXFLR indicates how many bytes are available to be read in the
FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
the number of bytes that we've requested to be read so far, but which
have not been read.

Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
comparing them results in each byte consuming "two" bytes in this
tracking, so this is obviously wrong.

Secondly, the number of bytes that _could_ be received into the FIFO at
any time is the number of bytes we have so far requested but not yet
read from the FIFO - in other words dev->rx_outstanding.

So, in order to request enough bytes to fill the RX FIFO, we need to
request dev->rx_fifo_depth - dev->rx_outstanding bytes.

Modifying the code thusly results in us reaching the maximum number of
bytes outstanding each time we queue more "receive" operations, provided
the transfer allows that to happen.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
With this applied, I now get:

[    1.726388] i2c_designware 7ffa0000.i2c: transfer terminated early - interrupt latency too high?
[    1.733813] tda998x 0-0070: Error -5 reading from 0x900
[    1.737708] tda998x 0-0070: failed to read edid block 0: -5
[    1.743683] tda998x 0-0070: failed to read EDID

which is a more graceful failure than letting DRM detect the bad
transfer by checking the EDID checksum and hoping that the untransferred
bytes don't result in the EDID checksum succeeding.

 drivers/i2c/busses/i2c-designware-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 11e866d05368..9703fe392543 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -611,7 +611,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
 
 				/* avoid rx buffer overrun */
-				if (rx_limit - dev->rx_outstanding <= 0)
+				if (dev->rx_outstanding >= dev->rx_fifo_depth)
 					break;
 
 				dw_writel(dev, cmd | 0x100, DW_IC_DATA_CMD);
-- 
2.7.4

[PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Russell King]

When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
count the number of bytes:

	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);

	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
		if (rx_limit - dev->rx_outstanding <= 0)
			break;
		rx_limit--;
		dev->rx_outstanding++;
	}

DW_IC_RXFLR indicates how many bytes are available to be read in the
FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
the number of bytes that we've requested to be read so far, but which
have not been read.

Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
comparing them results in each byte consuming "two" bytes in this
tracking, so this is obviously wrong.

Secondly, the number of bytes that _could_ be received into the FIFO at
any time is the number of bytes we have so far requested but not yet
read from the FIFO - in other words dev->rx_outstanding.

So, in order to request enough bytes to fill the RX FIFO, we need to
request dev->rx_fifo_depth - dev->rx_outstanding bytes.

Modifying the code thusly results in us reaching the maximum number of
bytes outstanding each time we queue more "receive" operations, provided
the transfer allows that to happen.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
With this applied, I now get:

[    1.726388] i2c_designware 7ffa0000.i2c: transfer terminated early - interrupt latency too high?
[    1.733813] tda998x 0-0070: Error -5 reading from 0x900
[    1.737708] tda998x 0-0070: failed to read edid block 0: -5
[    1.743683] tda998x 0-0070: failed to read EDID

which is a more graceful failure than letting DRM detect the bad
transfer by checking the EDID checksum and hoping that the untransferred
bytes don't result in the EDID checksum succeeding.

 drivers/i2c/busses/i2c-designware-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 11e866d05368..9703fe392543 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -611,7 +611,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
 
 				/* avoid rx buffer overrun */
-				if (rx_limit - dev->rx_outstanding <= 0)
+				if (dev->rx_outstanding >= dev->rx_fifo_depth)
 					break;
 
 				dw_writel(dev, cmd | 0x100, DW_IC_DATA_CMD);
-- 
2.7.4

Re: [BUG] i2c-designware silently fails on long transfers

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> With reference to this commit:
> 
> commit d39f77b06a712fcba6185a20bb209e357923d980
> Author: Andrew Jackson <Andrew.Jackson@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@arm.com>
>     Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>     Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>     Signed-off-by: Wolfram Sang <wsa@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.

Totally agree.

> 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 wonder if we can get it work more reliably by using DMA (provided that
there are DMA channels available for I2C in Juno)? That would allow the
hardware to perform longer reads without relying on how fast the
interrupt handler is able to empty the Rx FIFO.

[BUG] i2c-designware silently fails on long transfers

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> With reference to this commit:
> 
> commit d39f77b06a712fcba6185a20bb209e357923d980
> Author: Andrew Jackson <Andrew.Jackson@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@arm.com>
>     Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>     Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>     Signed-off-by: Wolfram Sang <wsa@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.

Totally agree.

> 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 wonder if we can get it work more reliably by using DMA (provided that
there are DMA channels available for I2C in Juno)? That would allow the
hardware to perform longer reads without relying on how fast the
interrupt handler is able to empty the Rx FIFO.

Re: [PATCH 1/2] i2c: designware: report short transfers

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
> Rather than reporting success for a short transfer due to interrupt
> latency, report an error both to the caller, as well as to the kernel
> log.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH 1/2] i2c: designware: report short transfers

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
> Rather than reporting success for a short transfer due to interrupt
> latency, report an error both to the caller, as well as to the kernel
> log.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
> count the number of bytes:
> 
> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
> 
> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> 		if (rx_limit - dev->rx_outstanding <= 0)
> 			break;
> 		rx_limit--;
> 		dev->rx_outstanding++;
> 	}
> 
> DW_IC_RXFLR indicates how many bytes are available to be read in the
> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
> the number of bytes that we've requested to be read so far, but which
> have not been read.
> 
> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
> comparing them results in each byte consuming "two" bytes in this
> tracking, so this is obviously wrong.
> 
> Secondly, the number of bytes that _could_ be received into the FIFO at
> any time is the number of bytes we have so far requested but not yet
> read from the FIFO - in other words dev->rx_outstanding.
> 
> So, in order to request enough bytes to fill the RX FIFO, we need to
> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
> 
> Modifying the code thusly results in us reaching the maximum number of
> bytes outstanding each time we queue more "receive" operations, provided
> the transfer allows that to happen.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Mika Westerberg]

On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
> count the number of bytes:
> 
> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
> 
> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> 		if (rx_limit - dev->rx_outstanding <= 0)
> 			break;
> 		rx_limit--;
> 		dev->rx_outstanding++;
> 	}
> 
> DW_IC_RXFLR indicates how many bytes are available to be read in the
> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
> the number of bytes that we've requested to be read so far, but which
> have not been read.
> 
> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
> comparing them results in each byte consuming "two" bytes in this
> tracking, so this is obviously wrong.
> 
> Secondly, the number of bytes that _could_ be received into the FIFO at
> any time is the number of bytes we have so far requested but not yet
> read from the FIFO - in other words dev->rx_outstanding.
> 
> So, in order to request enough bytes to fill the RX FIFO, we need to
> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
> 
> Modifying the code thusly results in us reaching the maximum number of
> bytes outstanding each time we queue more "receive" operations, provided
> the transfer allows that to happen.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > 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 wonder if we can get it work more reliably by using DMA (provided that
> there are DMA channels available for I2C in Juno)? That would allow the
> hardware to perform longer reads without relying on how fast the
> interrupt handler is able to empty the Rx FIFO.

It would need to DMA to the Tx FIFO to keep it filled - it triggers the
stop condition when the Tx FIFO empties.  From what I can see in the
driver, the Tx FIFO not only takes the data but also a "command" to tell
the hardware what to do.

The Rx FIFO would also need DMA to avoid it overflowing due to high
interrupt latency.

I don't know what state DMA is in on the Juno, or even whether it has
DMA - it has a PL330 DMA controller, but I see nothing in the DT files
making use of it.

-- 
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.

[BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > 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 wonder if we can get it work more reliably by using DMA (provided that
> there are DMA channels available for I2C in Juno)? That would allow the
> hardware to perform longer reads without relying on how fast the
> interrupt handler is able to empty the Rx FIFO.

It would need to DMA to the Tx FIFO to keep it filled - it triggers the
stop condition when the Tx FIFO empties.  From what I can see in the
driver, the Tx FIFO not only takes the data but also a "command" to tell
the hardware what to do.

The Rx FIFO would also need DMA to avoid it overflowing due to high
interrupt latency.

I don't know what state DMA is in on the Juno, or even whether it has
DMA - it has a PL330 DMA controller, but I see nothing in the DT files
making use of it.

-- 
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.

Re: [BUG] i2c-designware silently fails on long transfers

[Mika Westerberg]

On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> > On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > > 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 wonder if we can get it work more reliably by using DMA (provided that
> > there are DMA channels available for I2C in Juno)? That would allow the
> > hardware to perform longer reads without relying on how fast the
> > interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.

Yes, the command is either "read" or "write" (meaning even for Rx a
write to the Tx FIFO is needed).

> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.

I guess for Rx we would need to supply a dummy buffer of "read" commands
for the Tx FIFO in addition to reading bytes from Rx FIFO. But still
that might help to improve reliability in case of Juno.

> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

OK

[BUG] i2c-designware silently fails on long transfers

[Mika Westerberg]

On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> > On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > > 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 wonder if we can get it work more reliably by using DMA (provided that
> > there are DMA channels available for I2C in Juno)? That would allow the
> > hardware to perform longer reads without relying on how fast the
> > interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.

Yes, the command is either "read" or "write" (meaning even for Rx a
write to the Tx FIFO is needed).

> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.

I guess for Rx we would need to supply a dummy buffer of "read" commands
for the Tx FIFO in addition to reading bytes from Rx FIFO. But still
that might help to improve reliability in case of Juno.

> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

OK

Re: [BUG] i2c-designware silently fails on long transfers

[Liviu Dudau]

On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> > On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > > 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 wonder if we can get it work more reliably by using DMA (provided that
> > there are DMA channels available for I2C in Juno)? That would allow the
> > hardware to perform longer reads without relying on how fast the
> > interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.
> 
> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.
> 
> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

The only thing we have tested PL330 with was UART and I2S. I'm not sure we
can really use it with I2C given the above hardware configuration issue.

The other thing we have tried in our private branches was to repeat the EDID
transfer a number of times until the checksum was correct. Andrew Jackson
sent a patch a year or so back to have some ridiculously high number of
retries (30) and that has been rightfully shut down in the upstream.

The unfortunate thing is that the monitors and TVs that we use inside ARM
for testing don't seem to trigger this issue on a regular basis. We had one
or two small 7" TVs that did that, and the brute force retry of EDID transfer
sorted them out for the limited use that we had for them.

Best regards,
Liviu

> 
> -- 
> 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.

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[BUG] i2c-designware silently fails on long transfers

[Liviu Dudau]

On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> > On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > > 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 wonder if we can get it work more reliably by using DMA (provided that
> > there are DMA channels available for I2C in Juno)? That would allow the
> > hardware to perform longer reads without relying on how fast the
> > interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.
> 
> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.
> 
> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

The only thing we have tested PL330 with was UART and I2S. I'm not sure we
can really use it with I2C given the above hardware configuration issue.

The other thing we have tried in our private branches was to repeat the EDID
transfer a number of times until the checksum was correct. Andrew Jackson
sent a patch a year or so back to have some ridiculously high number of
retries (30) and that has been rightfully shut down in the upstream.

The unfortunate thing is that the monitors and TVs that we use inside ARM
for testing don't seem to trigger this issue on a regular basis. We had one
or two small 7" TVs that did that, and the brute force retry of EDID transfer
sorted them out for the limited use that we had for them.

Best regards,
Liviu

> 
> -- 
> 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.

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Re: [BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 11:21:35AM +0000, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> > It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> > stop condition when the Tx FIFO empties.  From what I can see in the
> > driver, the Tx FIFO not only takes the data but also a "command" to tell
> > the hardware what to do.
> > 
> > The Rx FIFO would also need DMA to avoid it overflowing due to high
> > interrupt latency.
> > 
> > I don't know what state DMA is in on the Juno, or even whether it has
> > DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> > making use of it.
> 
> The only thing we have tested PL330 with was UART and I2S. I'm not sure we
> can really use it with I2C given the above hardware configuration issue.
> 
> The other thing we have tried in our private branches was to repeat the EDID
> transfer a number of times until the checksum was correct. Andrew Jackson
> sent a patch a year or so back to have some ridiculously high number of
> retries (30) and that has been rightfully shut down in the upstream.
> 
> The unfortunate thing is that the monitors and TVs that we use inside ARM
> for testing don't seem to trigger this issue on a regular basis. We had one
> or two small 7" TVs that did that, and the brute force retry of EDID transfer
> sorted them out for the limited use that we had for them.

Due to the way the TDA998x works, it doesn't have much to do with the TV.
The TDA998x is instructed to read up to 128 bytes of EDID data into its
own internal buffer.  It then does so, and raises an interrupt (or we
notice that the interrupt flag is set when there's no hardware interrupt
line), and we then read the EDID data out of the TDA998x.

At the point we're reading from the TDA998x, it has loaded the data from
the DDC bus and the DDC bus is idle... we don't have direct access to
the DDC bus.

The only reason I can think that the checksum would pass is if you were
somehow ending up with data in the buffer that did cause the basic
checksum to pass, even though you had an incomplete read.

-- 
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.

[BUG] i2c-designware silently fails on long transfers

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 11:21:35AM +0000, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> > It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> > stop condition when the Tx FIFO empties.  From what I can see in the
> > driver, the Tx FIFO not only takes the data but also a "command" to tell
> > the hardware what to do.
> > 
> > The Rx FIFO would also need DMA to avoid it overflowing due to high
> > interrupt latency.
> > 
> > I don't know what state DMA is in on the Juno, or even whether it has
> > DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> > making use of it.
> 
> The only thing we have tested PL330 with was UART and I2S. I'm not sure we
> can really use it with I2C given the above hardware configuration issue.
> 
> The other thing we have tried in our private branches was to repeat the EDID
> transfer a number of times until the checksum was correct. Andrew Jackson
> sent a patch a year or so back to have some ridiculously high number of
> retries (30) and that has been rightfully shut down in the upstream.
> 
> The unfortunate thing is that the monitors and TVs that we use inside ARM
> for testing don't seem to trigger this issue on a regular basis. We had one
> or two small 7" TVs that did that, and the brute force retry of EDID transfer
> sorted them out for the limited use that we had for them.

Due to the way the TDA998x works, it doesn't have much to do with the TV.
The TDA998x is instructed to read up to 128 bytes of EDID data into its
own internal buffer.  It then does so, and raises an interrupt (or we
notice that the interrupt flag is set when there's no hardware interrupt
line), and we then read the EDID data out of the TDA998x.

At the point we're reading from the TDA998x, it has loaded the data from
the DDC bus and the DDC bus is idle... we don't have direct access to
the DDC bus.

The only reason I can think that the checksum would pass is if you were
somehow ending up with data in the buffer that did cause the basic
checksum to pass, even though you had an incomplete read.

-- 
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.

Re: [BUG] i2c-designware silently fails on long transfers

[Robin Murphy]

On 21/11/16 10:43, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
>> On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
>>> 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 wonder if we can get it work more reliably by using DMA (provided that
>> there are DMA channels available for I2C in Juno)? That would allow the
>> hardware to perform longer reads without relying on how fast the
>> interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.
> 
> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.
> 
> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

There's no DMA request wired up for I2C, unfortunately - only for the
UARTs, I2S, FPFGA tile and trace unit. I recall Liviu did have a patch
to lower the I2C clock as the mitigation you mention, which I thought
had made it into mainline, but apparently not.

Robin.

[BUG] i2c-designware silently fails on long transfers

[Robin Murphy]

On 21/11/16 10:43, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
>> On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
>>> 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 wonder if we can get it work more reliably by using DMA (provided that
>> there are DMA channels available for I2C in Juno)? That would allow the
>> hardware to perform longer reads without relying on how fast the
>> interrupt handler is able to empty the Rx FIFO.
> 
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties.  From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.
> 
> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.
> 
> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.

There's no DMA request wired up for I2C, unfortunately - only for the
UARTs, I2S, FPFGA tile and trace unit. I recall Liviu did have a patch
to lower the I2C clock as the mitigation you mention, which I thought
had made it into mainline, but apparently not.

Robin.

Re: [PATCH 1/2] i2c: designware: report short transfers

[Jarkko Nikula]

On 21.11.2016 12:32, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
>> Rather than reporting success for a short transfer due to interrupt
>> latency, report an error both to the caller, as well as to the kernel
>> log.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH 1/2] i2c: designware: report short transfers

[Jarkko Nikula]

On 21.11.2016 12:32, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
>> Rather than reporting success for a short transfer due to interrupt
>> latency, report an error both to the caller, as well as to the kernel
>> log.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Jarkko Nikula]

On 21.11.2016 12:40, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
>> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
>> count the number of bytes:
>>
>> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
>>
>> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
>> 		if (rx_limit - dev->rx_outstanding <= 0)
>> 			break;
>> 		rx_limit--;
>> 		dev->rx_outstanding++;
>> 	}
>>
>> DW_IC_RXFLR indicates how many bytes are available to be read in the
>> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
>> the number of bytes that we've requested to be read so far, but which
>> have not been read.
>>
>> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
>> comparing them results in each byte consuming "two" bytes in this
>> tracking, so this is obviously wrong.
>>
>> Secondly, the number of bytes that _could_ be received into the FIFO at
>> any time is the number of bytes we have so far requested but not yet
>> read from the FIFO - in other words dev->rx_outstanding.
>>
>> So, in order to request enough bytes to fill the RX FIFO, we need to
>> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
>>
>> Modifying the code thusly results in us reaching the maximum number of
>> bytes outstanding each time we queue more "receive" operations, provided
>> the transfer allows that to happen.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Jarkko Nikula]

On 21.11.2016 12:40, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
>> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
>> count the number of bytes:
>>
>> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
>>
>> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
>> 		if (rx_limit - dev->rx_outstanding <= 0)
>> 			break;
>> 		rx_limit--;
>> 		dev->rx_outstanding++;
>> 	}
>>
>> DW_IC_RXFLR indicates how many bytes are available to be read in the
>> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
>> the number of bytes that we've requested to be read so far, but which
>> have not been read.
>>
>> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
>> comparing them results in each byte consuming "two" bytes in this
>> tracking, so this is obviously wrong.
>>
>> Secondly, the number of bytes that _could_ be received into the FIFO at
>> any time is the number of bytes we have so far requested but not yet
>> read from the FIFO - in other words dev->rx_outstanding.
>>
>> So, in order to request enough bytes to fill the RX FIFO, we need to
>> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
>>
>> Modifying the code thusly results in us reaching the maximum number of
>> bytes outstanding each time we queue more "receive" operations, provided
>> the transfer allows that to happen.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 1/2] i2c: designware: report short transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
> Rather than reporting success for a short transfer due to interrupt
> latency, report an error both to the caller, as well as to the kernel
> log.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/2] i2c: designware: report short transfers

[Wolfram Sang]

On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
> Rather than reporting success for a short transfer due to interrupt
> latency, report an error both to the caller, as well as to the kernel
> log.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-current, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161124/956359d1/attachment.sig>

Re: [PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
> count the number of bytes:
> 
> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
> 
> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> 		if (rx_limit - dev->rx_outstanding <= 0)
> 			break;
> 		rx_limit--;
> 		dev->rx_outstanding++;
> 	}
> 
> DW_IC_RXFLR indicates how many bytes are available to be read in the
> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
> the number of bytes that we've requested to be read so far, but which
> have not been read.
> 
> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
> comparing them results in each byte consuming "two" bytes in this
> tracking, so this is obviously wrong.
> 
> Secondly, the number of bytes that _could_ be received into the FIFO at
> any time is the number of bytes we have so far requested but not yet
> read from the FIFO - in other words dev->rx_outstanding.
> 
> So, in order to request enough bytes to fill the RX FIFO, we need to
> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
> 
> Modifying the code thusly results in us reaching the maximum number of
> bytes outstanding each time we queue more "receive" operations, provided
> the transfer allows that to happen.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] i2c: designware: fix rx fifo depth tracking

[Wolfram Sang]

On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
> count the number of bytes:
> 
> 	rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
> 
> 	while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> 		if (rx_limit - dev->rx_outstanding <= 0)
> 			break;
> 		rx_limit--;
> 		dev->rx_outstanding++;
> 	}
> 
> DW_IC_RXFLR indicates how many bytes are available to be read in the
> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
> the number of bytes that we've requested to be read so far, but which
> have not been read.
> 
> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
> comparing them results in each byte consuming "two" bytes in this
> tracking, so this is obviously wrong.
> 
> Secondly, the number of bytes that _could_ be received into the FIFO at
> any time is the number of bytes we have so far requested but not yet
> read from the FIFO - in other words dev->rx_outstanding.
> 
> So, in order to request enough bytes to fill the RX FIFO, we need to
> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
> 
> Modifying the code thusly results in us reaching the maximum number of
> bytes outstanding each time we queue more "receive" operations, provided
> the transfer allows that to happen.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-current, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161124/cd1f3cc5/attachment.sig>