From patchwork Sat Jan 24 22:42:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Douglas Gilbert X-Patchwork-Id: 5700421 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 EAD3B9F305 for ; Sat, 24 Jan 2015 22:44:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D9B47201BC for ; Sat, 24 Jan 2015 22:44:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 755872015E for ; Sat, 24 Jan 2015 22:44:45 +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 1YF9Pw-00014j-4f; Sat, 24 Jan 2015 22:43:00 +0000 Received: from smtp.infotech.no ([82.134.31.41]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YF9Ps-0000zz-4Y for linux-arm-kernel@lists.infradead.org; Sat, 24 Jan 2015 22:42:57 +0000 Received: from localhost (localhost [127.0.0.1]) by smtp.infotech.no (Postfix) with ESMTP id 1C8192041AC; Sat, 24 Jan 2015 23:42:33 +0100 (CET) X-Virus-Scanned: by amavisd-new-2.6.6 (20110518) (Debian) at infotech.no Received: from smtp.infotech.no ([127.0.0.1]) by localhost (smtp.infotech.no [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HW-YAbIp20yG; Sat, 24 Jan 2015 23:42:27 +0100 (CET) Received: from [10.0.0.61] (8.191.251.212.customer.cdi.no [212.251.191.8]) by smtp.infotech.no (Postfix) with ESMTPA id D4B9C204171; Sat, 24 Jan 2015 23:42:26 +0100 (CET) Message-ID: <54C41FD0.2010607@interlog.com> Date: Sat, 24 Jan 2015 23:42:24 +0100 From: Douglas Gilbert User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Wolfram Sang , linux-arm-kernel , Linux I2C Subject: Re: [PATCH] at91: i2c-at91: improve time-out handling References: <54A58BA5.3080003@interlog.com> <20150107103113.GA30897@ldesroches-Latitude-E6320> <20150113152752.GK7660@katana> In-Reply-To: <20150113152752.GK7660@katana> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150124_144256_560654_48CE6B06 X-CRM114-Status: GOOD ( 17.37 ) X-Spam-Score: 0.0 (/) Cc: patchwork-notifications@patchwork.ozlabs.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: dgilbert@interlog.com 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 15-01-13 04:27 PM, Wolfram Sang wrote: > On Wed, Jan 07, 2015 at 11:31:14AM +0100, Ludovic Desroches wrote: >> Hi Douglas, >> >> On Thu, Jan 01, 2015 at 01:02:13PM -0500, Douglas Gilbert wrote: >>> With lk 3.19.0-rc2 and a at91sam9g25 (9x5) based system I >>> connected a NXP SC16IS750 I2C to serial bridge. After >>> routing the 750's IRQ back to the sc16is7xx driver and some >>> simple successful test, it was time for some intense testing: >>> Tx looped back to Rx on the 750, open picocom on /dev/ttySC0 >>> at 38400, and use hexdump to blast a binary file (in hex) at >>> ttySC0. The I2C SCL speed was 200,000 Hz. >>> >>> It worked as expected for a few seconds then it wedged the >>> I2C bus. That was repeatable. In the cases that I checked SCL >>> was high, SDA was low (driven by _both_ the G25's macrocell >>> and the 750!!) and IRQ was active (low). This patch stopped >>> the G25 macrocell from driving SDA low in the above wedge >>> (and stopped copious error reports going to the log). I was >>> surprised that a NXP I2C chip got into this situation, IMO >>> SDA on a slave should have a driven low timeout. IMO all >>> I2C master drivers should have provision to drive a gpio >>> connected to a (or all the) slave's RESET line(s). >>> >>> >>> ChangeLog: >>> when handling an I2C bus time-out, first clean-up the >>> DMA transfer, then do an I2C macrocell software reset >>> and restore some registers, including the interrupt >>> mask >>> >> >> I am wondering why you need to call at91_twi_irq_save() and >> at91_twi_irq_restore(). The interrupts enabled in the driver are >> AT91_TWI_TXCOMP, AT91_TWI_RXRDY and AT91_TWI_TXRDY and they are managed >> in at91_do_twi_transfer() so they would be set correctly for the next >> transfer. > > Douglas, any more info you could provide? I reran the torture tests without the at91_twi_irq_save() and at91_twi_irq_restore() calls and got the same results. So it seems that those calls are not needed; revised patch attached. Other observations: after the torture test wedges (after several seconds at 38400 baud) grounding the RESET pin on the SC16IS750 clears the I2C bus jam; thereafter I2C transmissions can continue. That implies to me that Atmel's I2C macrocell is not wedged. When the sc16is7xx driver is built as a module, then rmmod runs into a slow path dump in the logs. That comes from the clk_disable() call in: sc16is7xx_i2c_remove+0x88/0xa4 [sc16is7xx] Tests done with lk 3.19.0-rc4 using a AT91SAM9G25 system (Acme Arietta). For the attached patch: Signed-off-by: Douglas Gilbert >> Regards >> >> Ludovic >> >>> Signed-off-by: Douglas Gilbert >> >>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c >>> index 636fd2e..4d78708 100644 >>> --- a/drivers/i2c/busses/i2c-at91.c >>> +++ b/drivers/i2c/busses/i2c-at91.c >>> @@ -382,6 +382,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >>> { >>> int ret; >>> bool has_unre_flag = dev->pdata->has_unre_flag; >>> + bool timed_out = false; >>> >>> dev_dbg(dev->dev, "transfer: %s %d bytes.\n", >>> (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len); >>> @@ -440,7 +441,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >>> dev->adapter.timeout); >>> if (ret == 0) { >>> dev_err(dev->dev, "controller timed out\n"); >>> - at91_init_twi_bus(dev); >>> + timed_out = true; >>> ret = -ETIMEDOUT; >>> goto error; >>> } >>> @@ -471,6 +472,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >>> >>> error: >>> at91_twi_dma_cleanup(dev); >>> + if (timed_out) { >>> + at91_twi_irq_save(dev); >>> + at91_init_twi_bus(dev); >>> + at91_twi_irq_restore(dev); >>> + } >>> return ret; >>> } >>> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 91bd5bd..b2a6fbe 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -41,6 +41,10 @@ dtb-$(CONFIG_ARCH_AT91) += at91sam9g25ek.dtb dtb-$(CONFIG_ARCH_AT91) += at91sam9g35ek.dtb dtb-$(CONFIG_ARCH_AT91) += at91sam9x25ek.dtb dtb-$(CONFIG_ARCH_AT91) += at91sam9x35ek.dtb +dtb-$(CONFIG_ARCH_AT91) += acme-arietta.dtb +dtb-$(CONFIG_ARCH_AT91) += acme-arietta_sc16.dtb +dtb-$(CONFIG_ARCH_AT91) += at91-aria_cb.dtb +dtb-$(CONFIG_ARCH_AT91) += at91-aria_mg25.dtb # sama5d3 dtb-$(CONFIG_ARCH_AT91) += at91-sama5d3_xplained.dtb dtb-$(CONFIG_ARCH_AT91) += sama5d31ek.dtb diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c index 636fd2e..fcaf01c 100644 --- a/drivers/i2c/busses/i2c-at91.c +++ b/drivers/i2c/busses/i2c-at91.c @@ -382,6 +382,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) { int ret; bool has_unre_flag = dev->pdata->has_unre_flag; + bool timed_out = false; dev_dbg(dev->dev, "transfer: %s %d bytes.\n", (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len); @@ -440,7 +441,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) dev->adapter.timeout); if (ret == 0) { dev_err(dev->dev, "controller timed out\n"); - at91_init_twi_bus(dev); + timed_out = true; ret = -ETIMEDOUT; goto error; } @@ -471,6 +472,8 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) error: at91_twi_dma_cleanup(dev); + if (timed_out) + at91_init_twi_bus(dev); return ret; }