From patchwork Mon Oct 30 16:21:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: mike.isely@cobaltdigital.com X-Patchwork-Id: 13440756 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8FAD6C4332F for ; Mon, 30 Oct 2023 16:27:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=K9aVxLh218etmVGMT2W3auorJXCDgAmSvL62Wq8GBIs=; b=FTmV5Yn/I0Qq2M CjmBDvTNOt5mLOvaAlkUI+pdxGnUJkJP7oqoLDuf3/iEEp7o/mc1mIVREl+5dOeMmivmOo1tvVBmm A5k8NinpnJvEXFdhwLnKAK55763qfYRx5ZP9ymaiNZcx+h5yH1P3jq1vTQK2XrGSxBrI66efJxHj4 43pdJMO2QFF0CY9mlOB91OonvN5OecBk9yUJ+tl71ts7cvofTV6DC63C1T3sCEJs7+A9cnSMLlz7P btSPlZCcCCoWZJZDTCSmfBGi+VdIi2rQ00kYZhJXhzyGn2pXnsPKF7Gr4pLxeOfa85C9bqxpyU/WB CO5j0WhLyqCTFNUXlvAA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qxV6D-003dgk-01; Mon, 30 Oct 2023 16:26:45 +0000 Received: from cnc.isely.net ([192.69.181.175]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qxV68-003dfI-1Q; Mon, 30 Oct 2023 16:26:41 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=isely.net; s=deb; t=1698682891; bh=RJdQVRnJ416Wgcvb+WG6qIsjnzbvVyX8BWqzPyjn0Lw=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=B3htted8H4OPvFO0ZKlY6OyIk66ctUL+MsAlwryE2lPloLVr5B5eHDasV4oHbwKR8 6pED6Dp6FaQRoCIoTKhA8xonLSXFyaUzqlPEwCA4One02DRulNggogOjce3FFFc9N5 bXWeaPBB20PITelVzhY4T22o2ilSs1oawHM7KPWfYkEMw9M5IMeD1oK99CySy Original-Subject: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD Author: mike.isely@cobaltdigital.com Original-Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Received: from cobalt1.eng.cobalt.local (ts3-dock1.isely.net [::ffff:192.168.23.13]) (AUTH: PLAIN isely, TLS: TLS1.3,256bits,ECDHE_RSA_AES_256_GCM_SHA384) by cnc.isely.net with ESMTPSA id 000000000008096B.00000000653FD80B.00007A07; Mon, 30 Oct 2023 11:21:31 -0500 From: mike.isely@cobaltdigital.com To: Andi Shyti , Florian Fainelli Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD Date: Mon, 30 Oct 2023 11:21:14 -0500 Message-Id: <20231030162114.3603829-3-mike.isely@cobaltdigital.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> References: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> Mime-Version: 1.0 X-Mime-Autoconverted: from 8bit to 7bit by courier 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231030_092640_563393_EB4EA246 X-CRM114-Status: GOOD ( 18.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org From: Mike Isely There is a race in the bcm2835 i2c hardware: When one starts a write transaction, two things apparently take place at the same time: (1) an interrupt is posted to cause the FIFO to be filled with TX data, and (2) an I2C transaction is started on the wire with the slave select byte. The race happens if there's no slave, as this causes a slave selection timeout, raising the ERR flag in the hardware and setting DONE. The setting of that DONE flag races against TXW. If TXW gets set first, then an interrupt is raised if INTT was set. If ERR gets set first, then an interrupt is raised if INTD was set. It's one or the other, not both - probably because DONE being set disables the hardware INTT interrupt path. MOST of the time, TXW gets set first, the ISR runs, sees ERR is set and cleanly fails the transaction. However some of the time DONE gets set first - but since the driver doesn't enable INTD until it's on the last message - there's no interrupt at all. Thus the ISR never fires and the driver detects a timeout instead. At best, the "wrong" error code is delivered to the owner of the transaction. At worst, if the timeout doesn't propertly clean up the hardware (see prior commit fixing that), the next - likely unrelated - transaction will get fouled, leading to bizarre behavior in logic otherwise unrelated to the source of the original error. The fix here is to set INTD on for all messages not just the last one. In that way, unexpected failures which might set DONE earlier than expected will always trigger an interrupt and be handled correctly. The datasheet for this hardware doesn't describe any scenario where the hardware can realistically hang - even a stretched clock will be noticed if it takes too long. So in theory a timeout should really NEVER happen, and with this fix I was completely unable to trigger any further timeouts at all. Signed-off-by: Mike Isely --- drivers/i2c/busses/i2c-bcm2835.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index 96de875394e1..70005c037ff9 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -235,26 +235,22 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev) static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev) { - u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN; + u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN | BCM2835_I2C_C_INTD; struct i2c_msg *msg = i2c_dev->curr_msg; - bool last_msg = (i2c_dev->num_msgs == 1); if (!i2c_dev->num_msgs) return; i2c_dev->num_msgs--; i2c_dev->msg_buf = msg->buf; i2c_dev->msg_buf_remaining = msg->len; if (msg->flags & I2C_M_RD) c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR; else c |= BCM2835_I2C_C_INTT; - if (last_msg) - c |= BCM2835_I2C_C_INTD; - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr); bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len); bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c); }