From patchwork Fri Nov 18 19:35:42 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 9437261 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 178F46047D for ; Fri, 18 Nov 2016 19:37:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0C31C299D3 for ; Fri, 18 Nov 2016 19:37:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F2F1B299E5; Fri, 18 Nov 2016 19:37:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 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.wl.linuxfoundation.org (Postfix) with ESMTPS id 47938299D3 for ; Fri, 18 Nov 2016 19:37:50 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1c7oxS-00052H-Ok; Fri, 18 Nov 2016 19:36:22 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1c7oxO-0004yU-NG for linux-arm-kernel@lists.infradead.org; Fri, 18 Nov 2016 19:36:20 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:Content-Type:MIME-Version:Message-ID:Subject:Cc:To:From:Date; bh=X52kGwR7grZKXJMlq1nWdHuEm0p/N74PlLoMMqu7+V8=; b=W6RlbmugSDFdo3qgKhPqSorsFVNsAHpe+W2Ma+aY01u/e8TL+FtYQzlj67WdKqTBt1nVIWxxERkznj8v9b4GefwPvDl9jS2sI2p6Ziso68PjozCInR3CA0/0swUzS0jE0YhwnG/Uk1CdWNOmaw8/vs1VLjjkcztIS+CdsQLWqCs=; Received: from n2100.armlinux.org.uk ([fd8f:7570:feb6:1:214:fdff:fe10:4f86]:35426) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1c7owr-0006mv-RK; Fri, 18 Nov 2016 19:35:46 +0000 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1c7owo-0004Tu-MS; Fri, 18 Nov 2016 19:35:42 +0000 Date: Fri, 18 Nov 2016 19:35:42 +0000 From: Russell King - ARM Linux To: Andrew Jackson , Liviu Dudau , Mika Westerberg , Wolfram Sang , Jarkko Nikula , Andy Shevchenko Subject: [BUG] i2c-designware silently fails on long transfers Message-ID: <20161118193542.GO1041@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20161118_113619_174364_D986C179 X-CRM114-Status: GOOD ( 19.06 ) 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: linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP With reference to this commit: commit d39f77b06a712fcba6185a20bb209e357923d980 Author: Andrew Jackson Date: Fri Nov 7 12:10:44 2014 +0000 i2c: designware: prevent early stop on TX FIFO empty If the Designware core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN set to zero, allowing the TX FIFO to become empty causes a STOP condition to be generated on the I2C bus. If the transmit FIFO threshold is set too high, an erroneous STOP condition can be generated on long transfers - particularly where the interrupt latency is extended. Signed-off-by: Andrew Jackson Signed-off-by: Liviu Dudau Tested-by: Mika Westerberg Signed-off-by: Wolfram Sang The TDA998x driver issues long I2C transfers to read the EDID from the device - and userspace can also issue large transfers too. However, if a DW core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN set as zero, the above commit doesn't seem to solve the problem. During boot, with the patch below, I see: [ 1.736549] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x10 [ 1.736564] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510 [ 1.736608] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504 [ 1.736799] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514 [ 1.736819] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510 ... [ 1.737986] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504 [ 1.738010] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514 [ 1.738034] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504 [ 1.738039] random: fast init done [ 1.740120] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x714 [ 1.740231] i2c_dw_xfer: ffffffc97657b770:1 -> ffffffc97657b770:1 (0:0) [0 0 3 0] 8 [tx:ffffffc976682380:47] [rx:ffffffc9766823c9:55] [ 1.740249] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 93 [ 1.746979] Raw EDID: [ 1.747934] 00 ff ff ff ff ff ff 00 34 a9 96 a2 01 01 01 01 [ 1.752342] 00 17 01 03 80 80 48 78 0a da ff a3 58 4a a2 29 [ 1.756748] 17 49 4b 21 08 00 31 40 45 40 61 40 81 80 01 01 [ 1.761153] 01 01 01 01 01 01 02 3a 80 d0 72 38 2d 40 10 2c [ 1.765555] 45 80 ba 88 21 00 00 1e 02 00 d0 4e 30 09 12 54 [ 1.769958] 01 08 02 00 23 36 01 40 01 05 00 80 a1 4c 4b 49 [ 1.774361] 22 00 00 40 03 00 28 00 23 01 20 00 01 88 00 01 [ 1.778762] 08 00 00 40 00 02 03 04 0a 00 80 00 02 00 00 40 The significant thing is the "i2c_dw_xfer" line, where I add a print of the current state. Here, we can see that the transfer is mid-way, but a stop condition has been generated by the hardware, leaving 55 bytes to be received. Unfortunately, the i2c-designware driver ignores this, and believes that the transfer completed both fully and successfully, but returns bogus data to userspace or the kernel driver. That's really _bad_ behaviour by the driver - it should at least return an error. This problem is _soo_ bad that on my Juno, I can't run Xorg (it hits this every time we try to read the EDID) nor can I boot with the TV connected (it hits this every boot as well.) I'd go as far as to say that the i2c-designware hardware, when configured with this option set to zero, is fundamentally broken for OS which do not provide any guarantee for interrupt latency, such as Linux. The commit above tries to mitigate this by reducing the Tx FIFO threshold, so the interrupt is raised sooner, but that's clearly not enough for reliable operation. Another mitigation would be to lower the I2C bus frequency on Juno from 400kHz to 100kHz, so that there's 4x longer IRQ latency possible. However, even that isn't going to be reliable - even going to 100kHz isn't going to allow the above case to be solved - the interrupt is delayed by around 2ms, and it takes about 1.4ms to send/receive 16 bytes at 100kHz. (9 * 16 / (100*10^3)). So, I think all hope is lost for i2c-designware on Juno to cope with reading the EDID from TDA998x reliably. I have one patch which solves a problem in the accounting of bytes, and another to ensure that we return an error for an incomplete transfer, both will be sent threaded to this mail. diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 11e866d05368..060ae9e5a916 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -752,6 +752,15 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) goto done; } + printk(KERN_DEBUG "%s: %p:%d -> %p:%d (%d:%d) [%x %x %x %x] %d [tx:%p:%d] [rx:%p:%d]\n", + __func__, msgs, num, + dev->msgs, dev->msgs_num, + dev->msg_write_idx, dev->msg_read_idx, + dev->cmd_err, dev->msg_err, dev->status, dev->abort_source, + dev->rx_outstanding, + dev->tx_buf, dev->tx_buf_len, + dev->rx_buf, dev->rx_buf_len); + if (dev->msg_err) { ret = dev->msg_err; goto done; @@ -857,7 +866,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) enabled = dw_readl(dev, DW_IC_ENABLE); stat = dw_readl(dev, DW_IC_RAW_INTR_STAT); - dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat); + dev_printk(KERN_DEBUG, dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat); if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY)) return IRQ_NONE;