[PATCH] Revert "i2c: at91: fix a race condition when using the DMA controller"
From: Peter Rosin
Date: Mon Oct 05 2015 - 04:16:18 EST
This reverts commit 93563a6a71bb69dd324fc7354c60fb05f84aae6b.
Conflicts:
drivers/i2c/busses/i2c-at91.c
---
drivers/i2c/busses/i2c-at91.c | 97 +++++++++--------------------------------
1 file changed, 21 insertions(+), 76 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 1c758cd1e1ba..b5a5ef26b142 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -74,9 +74,6 @@
#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
#define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
-#define AT91_TWI_INT_MASK \
- (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK)
-
#define AT91_TWI_IER 0x0024 /* Interrupt Enable Register */
#define AT91_TWI_IDR 0x0028 /* Interrupt Disable Register */
#define AT91_TWI_IMR 0x002c /* Interrupt Mask Register */
@@ -155,12 +152,13 @@ static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
static void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
{
- at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_INT_MASK);
+ at91_twi_write(dev, AT91_TWI_IDR,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
}
static void at91_twi_irq_save(struct at91_twi_dev *dev)
{
- dev->imr = at91_twi_read(dev, AT91_TWI_IMR) & AT91_TWI_INT_MASK;
+ dev->imr = at91_twi_read(dev, AT91_TWI_IMR) & 0x7;
at91_disable_twi_interrupts(dev);
}
@@ -255,16 +253,7 @@ static void at91_twi_write_data_dma_callback(void *data)
dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg[0]),
dev->buf_len, DMA_TO_DEVICE);
- /*
- * When this callback is called, THR/TX FIFO is likely not to be empty
- * yet. So we have to wait for TXCOMP or NACK bits to be set into the
- * Status Register to be sure that the STOP bit has been sent and the
- * transfer is completed. The NACK interrupt has already been enabled,
- * we just have to enable TXCOMP one.
- */
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
- if (!dev->pdata->has_alt_cmd)
- at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
}
static void at91_twi_write_data_dma(struct at91_twi_dev *dev)
@@ -391,13 +380,10 @@ static void at91_twi_read_data_dma_callback(void *data)
dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg[0]),
dev->buf_len, DMA_FROM_DEVICE);
- if (!dev->pdata->has_alt_cmd) {
- /* The last two bytes have to be read without using dma */
- dev->buf += dev->buf_len - 2;
- dev->buf_len = 2;
- ier |= AT91_TWI_RXRDY;
- }
- at91_twi_write(dev, AT91_TWI_IER, ier);
+ /* The last two bytes have to be read without using dma */
+ dev->buf += dev->buf_len - 2;
+ dev->buf_len = 2;
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY);
}
static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
@@ -473,7 +459,7 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
/* catch error flags */
dev->transfer_status |= status;
- if (irqstatus & (AT91_TWI_TXCOMP | AT91_TWI_NACK)) {
+ if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
complete(&dev->cmd_complete);
}
@@ -488,49 +474,6 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
bool has_unre_flag = dev->pdata->has_unre_flag;
bool has_alt_cmd = dev->pdata->has_alt_cmd;
- /*
- * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
- * read flag but shows the state of the transmission at the time the
- * Status Register is read. According to the programmer datasheet,
- * TXCOMP is set when both holding register and internal shifter are
- * empty and STOP condition has been sent.
- * Consequently, we should enable NACK interrupt rather than TXCOMP to
- * detect transmission failure.
- * Indeed let's take the case of an i2c write command using DMA.
- * Whenever the slave doesn't acknowledge a byte, the LOCK, NACK and
- * TXCOMP bits are set together into the Status Register.
- * LOCK is a clear on write bit, which is set to prevent the DMA
- * controller from sending new data on the i2c bus after a NACK
- * condition has happened. Once locked, this i2c peripheral stops
- * triggering the DMA controller for new data but it is more than
- * likely that a new DMA transaction is already in progress, writing
- * into the Transmit Holding Register. Since the peripheral is locked,
- * these new data won't be sent to the i2c bus but they will remain
- * into the Transmit Holding Register, so TXCOMP bit is cleared.
- * Then when the interrupt handler is called, the Status Register is
- * read: the TXCOMP bit is clear but NACK bit is still set. The driver
- * manage the error properly, without waiting for timeout.
- * This case can be reproduced easyly when writing into an at24 eeprom.
- *
- * Besides, the TXCOMP bit is already set before the i2c transaction
- * has been started. For read transactions, this bit is cleared when
- * writing the START bit into the Control Register. So the
- * corresponding interrupt can safely be enabled just after.
- * However for write transactions managed by the CPU, we first write
- * into THR, so TXCOMP is cleared. Then we can safely enable TXCOMP
- * interrupt. If TXCOMP interrupt were enabled before writing into THR,
- * the interrupt handler would be called immediately and the i2c command
- * would be reported as completed.
- * Also when a write transaction is managed by the DMA controller,
- * enabling the TXCOMP interrupt in this function may lead to a race
- * condition since we don't know whether the TXCOMP interrupt is enabled
- * before or after the DMA has started to write into THR. So the TXCOMP
- * interrupt is enabled later by at91_twi_write_data_dma_callback().
- * Immediately after in that DMA callback, if the alternative command
- * mode is not used, we still need to send the STOP condition manually
- * writing the corresponding bit into the Control Register.
- */
-
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
@@ -578,24 +521,26 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
* seems to be the best solution.
*/
if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
at91_twi_read_data_dma(dev);
- } else {
+ /*
+ * It is important to enable TXCOMP irq here because
+ * doing it only when transferring the last two bytes
+ * will mask NACK errors since TXCOMP is set when a
+ * NACK occurs.
+ */
at91_twi_write(dev, AT91_TWI_IER,
- AT91_TWI_TXCOMP |
- AT91_TWI_NACK |
- AT91_TWI_RXRDY);
- }
+ AT91_TWI_TXCOMP);
+ } else
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
} else {
if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
at91_twi_write_data_dma(dev);
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
} else {
at91_twi_write_next_byte(dev);
at91_twi_write(dev, AT91_TWI_IER,
- AT91_TWI_TXCOMP |
- AT91_TWI_NACK |
- AT91_TWI_TXRDY);
+ AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
}
}
--
1.7.10.4
--------------000908060903010009070601--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/