Messages in this thread | | | Date | Tue, 31 Oct 2023 13:37:00 +0100 | Subject | Re: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD | From | Stefan Wahren <> |
| |
[Forware to Dave and Phil]
Am 30.10.23 um 17:21 schrieb mike.isely@cobaltdigital.com: > 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); > }
| |