From patchwork Mon Oct 5 08:45:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Rosin X-Patchwork-Id: 7325421 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6792A9F1D5 for ; Mon, 5 Oct 2015 08:48:44 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 16752206C0 for ; Mon, 5 Oct 2015 08:48:43 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C99F5205B1 for ; Mon, 5 Oct 2015 08:48:41 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Zj1PG-0003yO-Si; Mon, 05 Oct 2015 08:46:02 +0000 Received: from mail.lysator.liu.se ([130.236.254.3]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Zj1PC-0003qY-27 for linux-arm-kernel@lists.infradead.org; Mon, 05 Oct 2015 08:45:59 +0000 Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 549454000F; Mon, 5 Oct 2015 10:45:33 +0200 (CEST) Received: from [192.168.0.68] (217-210-101-82-no95.business.telia.com [217.210.101.82]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 5753E40007; Mon, 5 Oct 2015 10:45:31 +0200 (CEST) Subject: Re: Regression: at24 eeprom writing To: Linux Kernel Mailing List References: <560F0DB1.2020101@lysator.liu.se> From: Peter Rosin Message-ID: <561238A9.1060406@lysator.liu.se> Date: Mon, 5 Oct 2015 10:45:29 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <560F0DB1.2020101@lysator.liu.se> X-Virus-Scanned: ClamAV using ClamSMTP X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151005_014558_494213_EA8313B6 X-CRM114-Status: GOOD ( 38.76 ) X-Spam-Score: -4.2 (----) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Christian Gmainer , Ludovic Desroches , Cyrille Pitchen , linux-arm-kernel@lists.infradead.org, Wolfram Sang Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2015-10-03 01:05, Peter Rosin wrote: > Hi! > > I recently upgraded from the atmel linux-3.18-at91 kernel to vanilla 4.2 > and everything seemed fine. Until I tried to write to the little eeprom > chip. I then tried the linux-4.1-at91 kernel and that suffers too. > > The symptoms are that it seems like writes get interrupted, and restarted > again without properly initializing everything again. Inspecting the i2c > bus during these fails gets me something like this (int hex) when I > > echo abcdefghijklmnopqr > /sys/bus/i2c/devices/0-0050/eeprom > > S a0 00 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 P > S a0 10 (clk and data low for a "long" time) 10 71 72 0a P > > Notice how the address byte in the second chunk (10) is repeated after > the strange event on the i2c bus. > > I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3 > "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data" > eeprom writing starts working again. > > AFAICT, the i2c-at91 bus driver makes the eeprom driver use the > i2c_transfer code path both with that patch and with it reverted, > so I sadly don't see why the patch makes a difference. > > I'm on a board that is based on the sama5d31 evaluation kit, with a > NXP SE97BTP,547 chip and this in the devicetree: > > i2c0: i2c@f0014000 { > status = "okay"; > > jc42@18 { > compatible = "jc42"; > reg = <0x18>; > }; > > eeprom@50 { > compatible = "24c02"; > reg = <0x50>; > pagesize = <16>; > }; > }; Ok, I found the culprit, and I double and triple checked it this time... If I move to the very latest on the linux-3.18-at91 branch, the bug is there too. Which made it vastly more palatable to bisect the bug. The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b "i2c: at91: fix a race condition when using the DMA controller" which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list. If I add that patch on top of my previously working tree, it behaves just as newer kernels, i.e. equally bad. The patch doesn't revert cleanly, but reverting the patch and quick-n-dirty-fixing the conflict on vanilla 4.2 makes the problem go away. I have attached what I actually reverted. Cheers, Peter 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