Re: [PATCH] i2c-at91: fix data-loss issue
Am 16. April 2012 09:30 schrieb Voss, Nikolaus n.v...@weinmann.de:
Hubert Feurstein wrote on 2012-04-13:
In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
pending at the same time. In this case TXCOMP is handled but NOT RXRDY
which causes a data-loss on the current transfer
Right, this is definitely a bug and must be corrected. Part of my
motivation for exclusively or-ing the irq bits was not reading/
writing beyond the buffer because of (still) pending bits despite
of an already finished transfer, so I gave TXCOMP the highest prio.
Because of other reasons, write_next_byte() already checks this and
does nothing if all data already has been written. My suggestion is
to have read_next_byte() do this check too, as I don't trust the
hardware to reset RXRDY _immediately_ after reading.
Adding a check in read_next_byte() would be good just for safety.
@@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
*dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
- const unsigned irqstatus = status at91_twi_read(dev, AT91_TWI_IMR);
+ unsigned irqstatus = status at91_twi_read(dev, AT91_TWI_IMR);
+
+ irqstatus = (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
The above line should be unnecessary as no more than those interrupts
are enabled anyway. Any special reason for this?
No special reason for this.
+ if (!irqstatus)
+ return IRQ_NONE;
+
+ if (irqstatus AT91_TWI_RXRDY)
+ at91_twi_read_next_byte(dev);
+
+ if (irqstatus AT91_TWI_TXRDY)
+ at91_twi_write_next_byte(dev);
I would like to exclusively or TXRDY and RXRDY as those really should
not be active at the same time. Keeps the decision tree lean ;-).
I agree, it should be save to xor at least those two.
@@ -189,6 +193,10 @@ static int
at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev-msg-flags
I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
+ /* clear any pending data */
+ (void)at91_twi_read(dev, AT91_TWI_SR);
+ (void)at91_twi_read(dev, AT91_TWI_RHR);
I would like to modify this, as this is a partial fix for the above bug
which should already be fully fixed by the modified isr.
I fear subtle data-loss if we make (partial) tabula rasa before each
transfer. I'd rather add an assertion to check if the corresponding
irqs are active as an indication for a driver/hw-bug.
You also can add both, print an error/warning if the state in SR is
not as expected and then add the two recovery lines.
I'll repost the driver with your fix on positive feedback from you.
Thanks for tracking this down.
Ben, is there any chance to get this driver into next?
Niko
Hubert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
Hi Niko,
I'm using this driver since a while a now in my system
(soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
occasionally it happens that wrong data is read from my devices. I've
traced down the issue to the way how you do the interrupt handling. In
your code you do not consider that both status-flags (TXCOMP and
RXRDY) may be pending at the same time. So you handle the TXCOMP but
NOT the RXRDY which will cause a data-loss on the current transfer. As
a consequence also the next transfer will be corrupt, because you
start with old data in RHR. So I would suggest the following changes:
static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
unsigned irqstatus = status at91_twi_read(dev, AT91_TWI_IMR);
if (irqstatus AT91_TWI_RXRDY) {
at91_twi_read_next_byte(dev);
irqstatus = ~AT91_TWI_RXRDY;
}
if (irqstatus AT91_TWI_TXRDY) {
at91_twi_write_next_byte(dev);
irqstatus = ~AT91_TWI_TXRDY;
}
if (irqstatus AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev-transfer_status = status;
complete(dev-cmd_complete);
irqstatus = ~AT91_TWI_TXCOMP;
}
if (irqstatus) {
/* There should be no pending interrupt anymore. *)
return IRQ_NONE;
}
return IRQ_HANDLED;
}
To make sure that we do not start with old data in any case, I would
suggest to read SR and RHR before initiating a new transfer.
static int at91_do_twi_transfer(struct at91_twi_dev *dev)
{
int ret;
dev_dbg(dev-dev, transfer: %s %d bytes.\n,
(dev-msg-flags I2C_M_RD) ? read : write, dev-buf_len);
INIT_COMPLETION(dev-cmd_complete);
if (dev-msg-flags I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
/* clear any pending data */
(void)at91_twi_read(dev, AT91_TWI_SR);
(void)at91_twi_read(dev, AT91_TWI_RHR);
/* if only one byte is to be read, immediately stop transfer */
if (dev-buf_len = 1 !(dev-msg-flags I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
[snip]
}
Hubert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi, I've tested this driver on a 2.6.38 kernel with several i2c clients (temp-sensor, audio-codec, touchscreen-controller, w1-bridge, io-expanders) and works without problems. SoC: at91sam9g45 Because of the 2.6.38 kernel, I had to skip [PATCH v7 2/5] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk and used instead: at91_clock_associate(twi0_clk, at91sam9g45_twi0_device.dev, twi_clk); Best Regards Hubert On 2011-11-23 16:35, Nikolaus Voss wrote: The old driver has two main deficencies: i) No repeated start (Sr) condiction is possible, this makes it unusable e.g. for most SMBus transfers. ii) I/O was done with polling/busy waiting what caused over-/underruns even at light system loads and clock speeds. The new driver overcomes these deficencies and in addition allows for more than one TWI interface. Tested-by: Hubert Feurstein h.feurst...@gmail.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness
Hi, sorry, but your patch still does not apply. You fixed the tab issue, but it seems that there are still some bad whitespaces left which corrupt your patch. Regards Hubert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
