>From d178e0636358e61503ac55d39c8612ef93c1d893 Mon Sep 17 00:00:00 2001 From: Peter Rosin Date: Mon, 5 Oct 2015 10:16:18 +0200 Subject: [PATCH] Revert "i2c: at91: fix a race condition when using the DMA controller" 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