| Message ID | 1474298777-5858-1-git-send-email-noralf@tronnes.org |
|---|---|
| State | Superseded |
| Headers | show |
Noralf Trønnes <noralf@tronnes.org> writes: > Writing messages larger than the FIFO size results in a hang, rendering > the machine unusable. This is because the RXD status flag is set on the > first interrupt which results in bcm2835_drain_rxfifo() stealing bytes > from the buffer. The controller continues to trigger interrupts waiting > for the missing bytes, but bcm2835_fill_txfifo() has none to give. > In this situation wait_for_completion_timeout() apparently is unable to > stop the madness. > > The BCM2835 ARM Peripherals datasheet has this to say about the flags: > TXD: is set when the FIFO has space for at least one byte of data. > RXD: is set when the FIFO contains at least one byte of data. > TXW: is set during a write transfer and the FIFO is less than full. > RXR: is set during a read transfer and the FIFO is or more full. > > Implementing the logic from the downstream i2c-bcm2708 driver solved > the hang problem. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Patches 1, 3 are: Reviewed-by: Eric Anholt <eric@anholt.net> For patch 2 I followed some of it, but couldn't quite say it's a review. I trust your testing, and that the path has had a lot more testing in the downstream tree, so it's: Acked-by: Eric Anholt <eric@anholt.net> Thanks!
Den 19.09.2016 18:51, skrev Eric Anholt: > Noralf Trønnes <noralf@tronnes.org> writes: > >> Writing messages larger than the FIFO size results in a hang, rendering >> the machine unusable. This is because the RXD status flag is set on the >> first interrupt which results in bcm2835_drain_rxfifo() stealing bytes >> from the buffer. The controller continues to trigger interrupts waiting >> for the missing bytes, but bcm2835_fill_txfifo() has none to give. >> In this situation wait_for_completion_timeout() apparently is unable to >> stop the madness. >> >> The BCM2835 ARM Peripherals datasheet has this to say about the flags: >> TXD: is set when the FIFO has space for at least one byte of data. >> RXD: is set when the FIFO contains at least one byte of data. >> TXW: is set during a write transfer and the FIFO is less than full. >> RXR: is set during a read transfer and the FIFO is or more full. >> >> Implementing the logic from the downstream i2c-bcm2708 driver solved >> the hang problem. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Patches 1, 3 are: > > Reviewed-by: Eric Anholt <eric@anholt.net> > > For patch 2 I followed some of it, but couldn't quite say it's a review. > I trust your testing, and that the path has had a lot more testing in > the downstream tree, so it's: I don't know how much testing downstream really has had since this feature is disabled by default. So there is a possibility that this can break some devices (while at the same time enable others). I wondered about adding a module parameter so it could be disabled at runtime, but decided to see what the review would be first. It is of course possible to add a this module parameter later should it turn out to break some device. Noralf. > Acked-by: Eric Anholt <eric@anholt.net> > > Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19.09.2016 17:26, Noralf Trønnes wrote: > Writing messages larger than the FIFO size results in a hang, rendering > the machine unusable. This is because the RXD status flag is set on the > first interrupt which results in bcm2835_drain_rxfifo() stealing bytes > from the buffer. The controller continues to trigger interrupts waiting > for the missing bytes, but bcm2835_fill_txfifo() has none to give. I remember having seen similar interrupt issues with the SPI HW-block. > In this situation wait_for_completion_timeout() apparently is unable to > stop the madness. That is because it is a level irq that immediately triggers another interrupt giving no CPU no time to do other (threaded) OS activity - this might be slightly different for multi-core machines... > The BCM2835 ARM Peripherals datasheet has this to say about the flags: > TXD: is set when the FIFO has space for at least one byte of data. > RXD: is set when the FIFO contains at least one byte of data. > TXW: is set during a write transfer and the FIFO is less than full. > RXR: is set during a read transfer and the FIFO is or more full. > > Implementing the logic from the downstream i2c-bcm2708 driver solved > the hang problem. > > Signed-off-by: Noralf Trønnes<noralf@tronnes.org> Reviewed-by: Martin Sperl <kernel@martin.sperl.org> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index d4f3239..f283b71 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -64,6 +64,7 @@ struct bcm2835_i2c_dev { int irq; struct i2c_adapter adapter; struct completion completion; + struct i2c_msg *curr_msg; u32 msg_err; u8 *msg_buf; size_t msg_buf_remaining; @@ -126,14 +127,13 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) return IRQ_HANDLED; } - if (val & BCM2835_I2C_S_RXD) { - bcm2835_drain_rxfifo(i2c_dev); - if (!(val & BCM2835_I2C_S_DONE)) - return IRQ_HANDLED; - } - if (val & BCM2835_I2C_S_DONE) { - if (i2c_dev->msg_buf_remaining) + if (i2c_dev->curr_msg->flags & I2C_M_RD) { + bcm2835_drain_rxfifo(i2c_dev); + val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); + } + + if ((val & BCM2835_I2C_S_RXD) || i2c_dev->msg_buf_remaining) i2c_dev->msg_err = BCM2835_I2C_S_LEN; else i2c_dev->msg_err = 0; @@ -141,11 +141,16 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) return IRQ_HANDLED; } - if (val & BCM2835_I2C_S_TXD) { + if (val & BCM2835_I2C_S_TXW) { bcm2835_fill_txfifo(i2c_dev); return IRQ_HANDLED; } + if (val & BCM2835_I2C_S_RXR) { + bcm2835_drain_rxfifo(i2c_dev); + return IRQ_HANDLED; + } + return IRQ_NONE; } @@ -155,6 +160,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev, u32 c; unsigned long time_left; + i2c_dev->curr_msg = msg; i2c_dev->msg_buf = msg->buf; i2c_dev->msg_buf_remaining = msg->len; reinit_completion(&i2c_dev->completion);
Writing messages larger than the FIFO size results in a hang, rendering the machine unusable. This is because the RXD status flag is set on the first interrupt which results in bcm2835_drain_rxfifo() stealing bytes from the buffer. The controller continues to trigger interrupts waiting for the missing bytes, but bcm2835_fill_txfifo() has none to give. In this situation wait_for_completion_timeout() apparently is unable to stop the madness. The BCM2835 ARM Peripherals datasheet has this to say about the flags: TXD: is set when the FIFO has space for at least one byte of data. RXD: is set when the FIFO contains at least one byte of data. TXW: is set during a write transfer and the FIFO is less than full. RXR: is set during a read transfer and the FIFO is or more full. Implementing the logic from the downstream i2c-bcm2708 driver solved the hang problem. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/i2c/busses/i2c-bcm2835.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)