Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Original-Subject: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD
Author: mike.isely@cobaltdigital.com
Original-Cc: Mike Isely <mike.isely@cobaltdigital.com>,
  Mike Isely <isely@pobox.com>,
  Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>,
  Ray Jui <rjui@broadcom.com>, Scott Branden <sbranden@broadcom.com>,
  linux-rpi-kernel@lists.infradead.org,
  linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
From:   mike.isely@cobaltdigital.com
To:     Andi Shyti <andi.shyti@kernel.org>,
        Florian Fainelli <florian.fainelli@broadcom.com>
Cc:     Mike Isely <mike.isely@cobaltdigital.com>,
        Mike Isely <isely@pobox.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        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>
In-Reply-To: <20231030162114.3603829-1-mike.isely@cobaltdigital.com>
References: <20231030162114.3603829-1-mike.isely@cobaltdigital.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

From: Mike Isely <mike.isely@cobaltdigital.com>

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 <isely@pobox.com>
---
 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);
 }
-- 
2.39.2