Fixes: 1d7afc9 i2c: omap: ack IRQ in parts

PATCH In review Alexander Kochetkov 4 participants 8 messages Nov 17, 2014 – Nov 21, 2014

Subsystems (5)

Stable branch I2C subsystem host drivers OMAP2+ support OMAP1 support +1 more subsystems

lore

Share & tools

lore

Alexander Kochetkov

author

2014-11-17 16:41 UTC #0 lore raw

PATCH 1/1 Fixes: 1d7afc9 i2c: omap: ack IRQ in parts

commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts) changed the interrupt handler to complete transfers without clearing XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupts will be fired again. As a result, ISR keep processing transfer after it was already complete (from the driver code point of view).

A didn't see real impacts of the 1d7afc9, but it is really bad idea to have ISR running on user data after transfer was complete.

It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).

According to specs (if I understood correctly), in case of NACK and AL driver must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and NACK, AL, ARDY, and XDR (Master Transmitter Mode).

All that is done down the code under the if condition:
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...

The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so no interrupts is fired after ISR informs the rest of driver what transfer complete.

Note: instead of removing break under NACK case, we could just replace 'break' with 'continue' and allow NACK transfer to finish using ARDY event. I found that NACK and ARDY bits usually set together. That case confirm TI wiki:
http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK

In order if someone interested in the event traces for NACK and AL cases, I sent them to mailing list.

Tested on Beagleboard XM C.

Signed-off-by Alexander Kochetkov <[email protected]>

Cc <[email protected]> # v3.7+

---
 drivers/i2c/busses/i2c-omap.c |    2 --
 1 file changed, 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -2

index 90dcc2e..9af7095 100644
@@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			break;
 		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-			break;
 		}
 
 		/*

Signature · 1 line

1.7.9.5

Alexander Kochetkov

author

2014-11-17 17:01 UTC #1 lore raw

That's RESEND

Previous one patch was:
i2c: omap: fix NACK and Arbitration Lost irq handling
http://www.spinics.net/lists/linux-i2c/msg17490.html

I'll made more testing of current (linux/master) i2c driver (without my patch) and here I send test results.

For traces I used custom patch attached to the e-mail.

For simulating I2C bus conditions, I used simple MCU connected to Beagleboard using I2C bus. MCU can act as slave and as master, and Beagleboard can instruct MCU to setup special conditions on the bus.

I used oscilloscope to control signals on wire.

The results indicates what:
1. current i2c driver actually doesn't cause controller hang and client code segfaults. (my previous patch incorrectly blame that)

2. it looks, that current i2c, doesn't strictly follow TI specs (AM-DM37x Multimedia Device Silicon Revision 1.x - sprugn4r, Figure 17-31. HS I2C Master Transmitter Mode ... and Figure 17-32. HS I2C Master Receiver Mode ..., note 1)

3. current i2c driver keeps processing interrupts after it call omap_i2c_complete_cmd.
4. in NACK case current driver call omap_i2c_complete_cmd two times.

The patch fixes 2, 3, 4.

Here how spotted problems looks like.

i2c controller initiate master write transfer to slave device what doesn't exist on the i2c bus. (isr thread keeps processing interrupts (ARDY) after it signaled transfer completion, and call complete second time)
=========================

[ 2481.152770] omap_i2c omap_i2c.2: addr: 0x0068, len: 4, flags: 0x0, stop: 1
[ 2481.152801] omap_i2c omap_i2c.2: Pre submit: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:584)
[ 2481.152832] omap_i2c omap_i2c.2: Hard ISR: STAT=0x1110; IE=0x601f; CON=0x8602 (omap_i2c_isr:911)
[ 2481.152862] omap_i2c omap_i2c.2: Post submit: STAT=0x1110; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:586)
[ 2481.152893] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1110; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:615)
[ 2481.152923] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1112; IE=0x601f; CON=0x8602 (omap_i2c_isr_thread:936)
[ 2481.152954] omap_i2c omap_i2c.2: IRQ (ISR = 0x0012)
[ 2481.152954] omap_i2c omap_i2c.2: Complete: STAT=0x0114; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 2481.152984] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0114; IE=0x601f; CON=0x8200 (omap_i2c_isr:911)
[ 2481.153015] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0114; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 2481.153015] omap_i2c omap_i2c.2: IRQ (ISR = 0x0014)
[ 2481.153045] omap_i2c omap_i2c.2: Complete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 2481.153106] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:618)
[ 2481.153106] omap_i2c omap_i2c.2: Got NACK - send STP: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:642)

i2c controller initiate master read transfer to slave device what doesn't exist on the i2c bus. (isr thread keeps processing interrupts (ARDY) after it signaled transfer completion, and call complete second time)
=========================

[ 3201.693603] omap_i2c omap_i2c.2: addr: 0x0068, len: 4, flags: 0x1, stop: 1
[ 3201.693603] omap_i2c omap_i2c.2: Pre submit: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:584)
[ 3201.693634] omap_i2c omap_i2c.2: Post submit: STAT=0x0100; IE=0x601f; CON=0x8403 (omap_i2c_xfer_msg:586)
[ 3201.693634] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1100; IE=0x601f; CON=0x8402 (omap_i2c_xfer_msg:615)
[ 3201.693756] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0106; IE=0x601f; CON=0x8000 (omap_i2c_isr:911)
[ 3201.693847] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0106; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:936)
[ 3201.693847] omap_i2c omap_i2c.2: IRQ (ISR = 0x0006)
[ 3201.693878] omap_i2c omap_i2c.2: Complete: STAT=0x0104; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:1055)
[ 3201.693908] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0104; IE=0x601f; CON=0x8000 (omap_i2c_isr:911)
[ 3201.693908] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0104; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:936)
[ 3201.693939] omap_i2c omap_i2c.2: IRQ (ISR = 0x0004)
[ 3201.693939] omap_i2c omap_i2c.2: Complete: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:1055)
[ 3201.693969] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:618)
[ 3201.694000] omap_i2c omap_i2c.2: Got NACK - send STP: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:642)

i2c controller initiate master write transfer to slave device and get NACK on the second data byte (address and first data bytes transmit successfully).
(isr thread keeps processing interrupts (ARDY) after it signaled transfer completion, and call complete second time)
=========================

[ 3457.679901] omap_i2c omap_i2c.2: addr: 0x0058, len: 4, flags: 0x0, stop: 1
[ 3457.679931] omap_i2c omap_i2c.2: Pre submit: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:584)
[ 3457.679962] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0110; IE=0x601f; CON=0x8602 (omap_i2c_isr:911)
[ 3457.679992] omap_i2c omap_i2c.2: Post submit: STAT=0x1110; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:586)
[ 3457.680175] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1510; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:615)
[ 3457.680236] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1510; IE=0x601f; CON=0x8602 (omap_i2c_isr_thread:936)
[ 3457.680267] omap_i2c omap_i2c.2: IRQ (ISR = 0x0010)
[ 3457.680267] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1100; IE=0x601f; CON=0x8602 (omap_i2c_isr_thread:936)
[ 3457.680480] omap_i2c omap_i2c.2: Hard ISR: STAT=0x1102; IE=0x601f; CON=0x8602 (omap_i2c_isr:911)
[ 3457.680603] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0106; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 3457.680603] omap_i2c omap_i2c.2: IRQ (ISR = 0x0006)
[ 3457.680633] omap_i2c omap_i2c.2: Complete: STAT=0x0104; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 3457.680664] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0104; IE=0x601f; CON=0x8200 (omap_i2c_isr:911)
[ 3457.680694] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0104; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 3457.680694] omap_i2c omap_i2c.2: IRQ (ISR = 0x0004)
[ 3457.680694] omap_i2c omap_i2c.2: Complete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 3457.683288] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:618)
[ 3457.683319] omap_i2c omap_i2c.2: Got NACK - send STP: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:642)

i2c controller initiate master write transfer to slave device and get AL during address phase (isr thread keeps processing interrupts (XRDY) after it signaled transfer completion)
=========================

[ 3969.863067] omap_i2c omap_i2c.2: addr: 0x0058, len: 4, flags: 0x0, stop: 1
[ 3969.863067] omap_i2c omap_i2c.2: Pre submit: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:584)
[ 3969.863098] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0110; IE=0x601f; CON=0x8602 (omap_i2c_isr:911)
[ 3969.863128] omap_i2c omap_i2c.2: Post submit: STAT=0x1111; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:586)
[ 3969.863128] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1111; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:615)
[ 3969.863159] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1111; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 3969.863189] omap_i2c omap_i2c.2: IRQ (ISR = 0x0011)
[ 3969.863189] omap_i2c omap_i2c.2: Arbitration lost
[ 3969.868103] omap_i2c omap_i2c.2: Complete: STAT=0x0110; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 3969.868316] omap_i2c omap_i2c.2: Hard ISR: STAT=0x0110; IE=0x601f; CON=0x8200 (omap_i2c_isr:911)
[ 3969.868347] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0110; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 3969.868347] omap_i2c omap_i2c.2: IRQ (ISR = 0x0010)
[ 3969.868377] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 3969.875518] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:618)
[ 3969.875549] omap_i2c omap_i2c.2: Got AL - i2c reset: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:632)

i2c controller initiate master read transfer to slave device and get AL during address phase. (that case works as expected)
=========================

[ 4136.002380] omap_i2c omap_i2c.2: addr: 0x0058, len: 4, flags: 0x1, stop: 1
[ 4136.002380] omap_i2c omap_i2c.2: Pre submit: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:584)
[ 4136.002410] omap_i2c omap_i2c.2: Post submit: STAT=0x0100; IE=0x601f; CON=0x8403 (omap_i2c_xfer_msg:586)
[ 4136.002410] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1100; IE=0x601f; CON=0x8402 (omap_i2c_xfer_msg:615)
[ 4136.002593] omap_i2c omap_i2c.2: Hard ISR: STAT=0x1101; IE=0x601f; CON=0x8000 (omap_i2c_isr:911)
[ 4136.002655] omap_i2c omap_i2c.2: Thread ISR: STAT=0x0101; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:936)
[ 4136.002655] omap_i2c omap_i2c.2: IRQ (ISR = 0x0001)
[ 4136.002655] omap_i2c omap_i2c.2: Arbitration lost
[ 4136.007598] omap_i2c omap_i2c.2: Complete: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_isr_thread:1055)
[ 4136.009674] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:618)
[ 4136.009704] omap_i2c omap_i2c.2: Got AL - i2c reset: STAT=0x0100; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:632)

i2c controller initiate master write transfer to slave device and get AL on the second data byte (address and first data bytes transmit successfully).
(that case works as expected)
=========================

[ 4303.387481] omap_i2c omap_i2c.2: addr: 0x0058, len: 4, flags: 0x0, stop: 1
[ 4303.387512] omap_i2c omap_i2c.2: Pre submit: STAT=0x0000; IE=0x601f; CON=0x8000 (omap_i2c_xfer_msg:584)
[ 4303.387542] omap_i2c omap_i2c.2: Hard ISR: STAT=0x1010; IE=0x601f; CON=0x8602 (omap_i2c_isr:911)
[ 4303.387573] omap_i2c omap_i2c.2: Post submit: STAT=0x1010; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:586)
[ 4303.387603] omap_i2c omap_i2c.2: Wait comlete: STAT=0x1010; IE=0x601f; CON=0x8602 (omap_i2c_xfer_msg:615)
[ 4303.387664] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1410; IE=0x601f; CON=0x8602 (omap_i2c_isr_thread:936)
[ 4303.387664] omap_i2c omap_i2c.2: IRQ (ISR = 0x0010)
[ 4303.387695] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1400; IE=0x601f; CON=0x8602 (omap_i2c_isr_thread:936)
[ 4303.388031] omap_i2c omap_i2c.2: Hard ISR: STAT=0x1001; IE=0x601f; CON=0x8200 (omap_i2c_isr:911)
[ 4303.388122] omap_i2c omap_i2c.2: Thread ISR: STAT=0x1001; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:936)
[ 4303.388122] omap_i2c omap_i2c.2: IRQ (ISR = 0x0001)
[ 4303.388153] omap_i2c omap_i2c.2: Arbitration lost
[ 4303.393066] omap_i2c omap_i2c.2: Complete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_isr_thread:1055)
[ 4303.395141] omap_i2c omap_i2c.2: Got comlete: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:618)
[ 4303.395172] omap_i2c omap_i2c.2: Got AL - i2c reset: STAT=0x0100; IE=0x601f; CON=0x8200 (omap_i2c_xfer_msg:632)

The patch for tracing.
=========================

Quoted 1 line

From aefaa3642eae0b4fd9c1e8dc10665c55d04bad53 Mon Sep 17 00:00:00 2001

From: Alexander Kochetkov <[email protected]>
Date: Mon, 17 Nov 2014 18:35:00 +0400
Subject: i2c: omap: enable traces for debug NACK and AL transfers

Just for testing. Not for Upstream.

Signed-off-by Alexander Kochetkov <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/i2c/busses/i2c-omap.c +29 -0

index 9cfadf3..7b62cf1 100644
@@ -28,6 +28,8 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#define DEBUG 1
+
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>

@@ -277,6 +279,22 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
 }
 
+#ifdef DEBUG
+static inline void
+omap_i2c_dump_state(const char *func, int line, struct omap_i2c_dev *dev, const char *msg)
+{
+       dev_dbg(dev->dev, "%s: STAT=0x%04x; IE=0x%04x; CON=0x%04x (%s:%d)\n",
+               msg,
+               omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG),
+               omap_i2c_read_reg(dev, OMAP_I2C_IE_REG),
+               omap_i2c_read_reg(dev, OMAP_I2C_CON_REG),
+               func, line);
+}
+#define OMAP_I2C_DUMP_STATE(dev, msg) omap_i2c_dump_state(__func__, __LINE__, (dev), (msg))
+#else
+#define OMAP_I2C_DUMP_STATE(dev, msg)
+#endif
+
 static void __omap_i2c_init(struct omap_i2c_dev *dev)
 {

@@ -563,7 +581,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	if (!dev->b_hw && stop)
 		w |= OMAP_I2C_CON_STP;
 
+	OMAP_I2C_DUMP_STATE(dev, "Pre submit");
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
+	OMAP_I2C_DUMP_STATE(dev, "Post submit");
 
 	/*
 	 * Don't write stt and stp together on some hardware.

@@ -592,8 +612,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
 	 */
+	OMAP_I2C_DUMP_STATE(dev, "Wait comlete");
 	timeout = wait_for_completion_timeout(&dev->cmd_complete,
 						OMAP_I2C_TIMEOUT);
+	OMAP_I2C_DUMP_STATE(dev, "Got comlete");
 	if (timeout == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		omap_i2c_reset(dev);

@@ -607,6 +629,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
+		OMAP_I2C_DUMP_STATE(dev, "Got AL - i2c reset");
 		omap_i2c_reset(dev);
 		__omap_i2c_init(dev);
 		return -EIO;

@@ -616,6 +639,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
 
+		OMAP_I2C_DUMP_STATE(dev, "Got NACK - send STP");
 		w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 		w |= OMAP_I2C_CON_STP;
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);

@@ -884,6 +908,8 @@ omap_i2c_isr(int irq, void *dev_id)
 	mask = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 
+	OMAP_I2C_DUMP_STATE(dev, "Hard ISR");
+
 	if (stat & mask)
 		ret = IRQ_WAKE_THREAD;

@@ -907,6 +933,8 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 		stat &= bits;
 
+		OMAP_I2C_DUMP_STATE(dev, "Thread ISR");
+
 		/* If we're in receiver mode, ignore XDR/XRDY */
 		if (dev->receiver)
 			stat &= ~(OMAP_I2C_STAT_XDR | OMAP_I2C_STAT_XRDY);

@@ -1024,6 +1052,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		}
 	} while (stat);
 
+	OMAP_I2C_DUMP_STATE(dev, "Complete");
 	omap_i2c_complete_cmd(dev, err);
 
 out:

Signature · 1 line

1.7.9.5

Alexander Kochetkov

author

2014-11-17 17:36 UTC #2 lore raw

In reply to Alexander Kochetkov

Grygorii Strashko earlier reported solution for the issue.

17 нояб. 2014 г., в 17:41, Grygorii Strashko <[email protected]> написал(а):

Quoted 2 lines

Seems you've got the same issue as I :) long time ago
https://lkml.org/lkml/2013/6/7/530

Felipe Balbi

2014-11-18 16:39 UTC #3 lore raw

In reply to Alexander Kochetkov

Quoted 28 lines · Alexander Kochetkov

commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts) changed the interrupt handler to complete transfers without clearing XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupts will be fired again. As a result, ISR keep processing transfer after it was already complete (from the driver code point of view).
A didn't see real impacts of the 1d7afc9, but it is really bad idea to have ISR running on user data after transfer was complete.
It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).
According to specs (if I understood correctly), in case of NACK and AL driver must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and NACK, AL, ARDY, and XDR (Master Transmitter Mode).
All that is done down the code under the if condition:
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...
The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so no interrupts is fired after ISR informs the rest of driver what transfer complete.
Note: instead of removing break under NACK case, we could just replace 'break' with 'continue' and allow NACK transfer to finish using ARDY event. I found that NACK and ARDY bits usually set together. That case confirm TI wiki:
http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK

Quoted 7 lines

In order if someone interested in the event traces for NACK and AL cases, I sent them to mailing list.
Tested on Beagleboard XM C.
Signed-off-by Alexander Kochetkov <[email protected]>
Cc <[email protected]> # v3.7+

This is good, but subject is wrong. That fixes line should not be the subject, it should be here after Cc :-)

Other than that, I'd just ask for Aaro to test on his legacy systems.

Quoted 26 lines

---
 drivers/i2c/busses/i2c-omap.c |    2 --
 1 file changed, 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -2

index 90dcc2e..9af7095 100644
@@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			break;
 		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-			break;
 		}
 
 		/*

Signature · 1 line

1.7.9.5

Signature · 1 line

balbi

Wolfram Sang

2014-11-21 06:46 UTC #4 lore raw

Quoted 7 lines

Quoted 4 lines
Tested on Beagleboard XM C.
Signed-off-by Alexander Kochetkov <[email protected]>
Cc <[email protected]> # v3.7+
This is good, but subject is wrong. That fixes line should not be the subject, it should be here after Cc :-)

I'll take this as an Ack unless you protest.

Alexander Kochetkov

author

2014-11-18 17:00 UTC #5 lore raw

PATCH 1/1 i2c: omap: fix NACK and Arbitration Lost irq handling

In reply to Alexander Kochetkov

A didn't see real impacts of the 1d7afc9, but it is really bad idea to have ISR running on user data after transfer was complete.

It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).

According to specs (if I understood correctly), in case of NACK and AL driver must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and NACK, AL, ARDY, and XDR (Master Transmitter Mode).

All that is done down the code under the if condition:
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...

The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so no interrupts is fired after ISR informs the rest of driver what transfer complete.

In order if someone interested in the event traces for NACK and AL cases, I sent them to mailing list.

Tested on Beagleboard XM C.

Signed-off-by Alexander Kochetkov <[email protected]>

Fixes 1d7afc9 i2c: omap: ack IRQ in parts

Cc <[email protected]> # v3.7+

---
 drivers/i2c/busses/i2c-omap.c |    2 --
 1 file changed, 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -2

index 90dcc2e..9af7095 100644
@@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			break;
 		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-			break;
 		}
 
 		/*

Signature · 1 line

1.7.9.5

Aaro Koskinen

maintainer Tested-by on v1 1/1

2014-11-20 21:10 UTC #6 lore raw

Re: [PATCH] i2c: omap: fix NACK and Arbitration Lost irq handling

Quoted 28 lines · Alexander Kochetkov

commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts) changed the interrupt handler to complete transfers without clearing XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupts will be fired again. As a result, ISR keep processing transfer after it was already complete (from the driver code point of view).
A didn't see real impacts of the 1d7afc9, but it is really bad idea to have ISR running on user data after transfer was complete.
It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).
According to specs (if I understood correctly), in case of NACK and AL driver must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and NACK, AL, ARDY, and XDR (Master Transmitter Mode).
All that is done down the code under the if condition:
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...
The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so no interrupts is fired after ISR informs the rest of driver what transfer complete.
Note: instead of removing break under NACK case, we could just replace 'break' with 'continue' and allow NACK transfer to finish using ARDY event. I found that NACK and ARDY bits usually set together. That case confirm TI wiki:
http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK

Quoted 8 lines

In order if someone interested in the event traces for NACK and AL cases, I sent them to mailing list.
Tested on Beagleboard XM C.
Signed-off-by Alexander Kochetkov <[email protected]>
Fixes 1d7afc9 i2c: omap: ack IRQ in parts
Cc <[email protected]> # v3.7+

I could not see any breakage or anything wrong on OMAP2 & OMAP3. On OMAP1 I don't have anything on the OMAP I2C bus, so cannot really test anything there.

Tested-by Aaro Koskinen <[email protected]>

Quoted 30 lines

---
 drivers/i2c/busses/i2c-omap.c |    2 --
 1 file changed, 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -2

index 90dcc2e..9af7095 100644
@@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			break;
 		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-			break;
 		}
 
 		/*

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfram Sang

Applied

2014-11-21 06:50 UTC #7 lore raw

Re: [PATCH] i2c: omap: fix NACK and Arbitration Lost irq handling

In reply to Alexander Kochetkov

Quoted 28 lines · Alexander Kochetkov

commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts) changed the interrupt handler to complete transfers without clearing XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupts will be fired again. As a result, ISR keep processing transfer after it was already complete (from the driver code point of view).
A didn't see real impacts of the 1d7afc9, but it is really bad idea to have ISR running on user data after transfer was complete.
It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).
According to specs (if I understood correctly), in case of NACK and AL driver must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and NACK, AL, ARDY, and XDR (Master Transmitter Mode).
All that is done down the code under the if condition:
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...
The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so no interrupts is fired after ISR informs the rest of driver what transfer complete.
Note: instead of removing break under NACK case, we could just replace 'break' with 'continue' and allow NACK transfer to finish using ARDY event. I found that NACK and ARDY bits usually set together. That case confirm TI wiki:
http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK

Quoted 9 lines

In order if someone interested in the event traces for NACK and AL cases, I sent them to mailing list.
Tested on Beagleboard XM C.
Signed-off-by Alexander Kochetkov <[email protected]>
Fixes 1d7afc9 i2c: omap: ack IRQ in parts
Cc <[email protected]> # v3.7+

Applied to for-current, thanks!