[PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Hubert Feurstein]

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

[PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

Hi,

On Fri, Apr 13, 2012 at 12:17:59PM +0200, Hubert Feurstein wrote:
> 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;
> 	}

Dude, I remember asking the exact same thing when he first posted this
driver. That whole if...else if... else if ... else didn't look right.

Can you send this in patch format Hubert ? Take a look at
Documentation/SumittingPatches if you have never done it before.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/dabfb87a/attachment-0001.sig>

[PATCH] i2c-at91: fix data-loss issue

[Hubert Feurstein]

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. As a consequence also the
next transfer will be corrupt, because old data is pending in RHR.

To make sure that we do not start with old data in any case, SR and RHR is
read empty before initiating a new transfer.

Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
---
 This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

 drivers/i2c/busses/i2c-at91.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 40c39a2..295835f 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -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);
+	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);
 
 	if (irqstatus & AT91_TWI_TXCOMP) {
 		at91_disable_twi_interrupts(dev);
 		dev->transfer_status = status;
 		complete(&dev->cmd_complete);
-	} else if (irqstatus & AT91_TWI_RXRDY) {
-		at91_twi_read_next_byte(dev);
-	} else if (irqstatus & AT91_TWI_TXRDY) {
-		at91_twi_write_next_byte(dev);
-	} else {
-		return IRQ_NONE;
 	}
 
 	return IRQ_HANDLED;
@@ -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);
+
 		/* 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;
@@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
 			internal_address |= addr << (8 * i);
 			int_addr_flag += AT91_TWI_IADRSZ_1;
 		}
+
 		at91_twi_write(dev, AT91_TWI_IADR, internal_address);
 	}
 
-- 
1.7.8.3

[PATCH] i2c-at91: fix data-loss issue

[Ryan Mallon]

On 13/04/12 21:44, Hubert Feurstein wrote:

> 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. As a consequence also the
> next transfer will be corrupt, because old data is pending in RHR.
> 
> To make sure that we do not start with old data in any case, SR and RHR is
> read empty before initiating a new transfer.
> 
> Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>


Couple of very minor comments below.

~Ryan

> ---
>  This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
> 
>  drivers/i2c/busses/i2c-at91.c |   23 ++++++++++++++++-------
>  1 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 40c39a2..295835f 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -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);

> +	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);
>  
>  	if (irqstatus & AT91_TWI_TXCOMP) {
>  		at91_disable_twi_interrupts(dev);
>  		dev->transfer_status = status;
>  		complete(&dev->cmd_complete);
> -	} else if (irqstatus & AT91_TWI_RXRDY) {
> -		at91_twi_read_next_byte(dev);
> -	} else if (irqstatus & AT91_TWI_TXRDY) {
> -		at91_twi_write_next_byte(dev);
> -	} else {
> -		return IRQ_NONE;
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -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);


Drop the void cast.

> +
>  		/* 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;
> @@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
>  			internal_address |= addr << (8 * i);
>  			int_addr_flag += AT91_TWI_IADRSZ_1;
>  		}
> +


Please don't make unrelated whitespace changes.

>  		at91_twi_write(dev, AT91_TWI_IADR, internal_address);
>  	}
>  


With those fixed, please add:

Reviewed-by: Ryan Mallon <rmallon@gmail.com>

[PATCH] i2c-at91: fix data-loss issue

[Voss, Nikolaus]

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.

> @@ -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?
 
> +	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 ;-).

> @@ -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.

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

[PATCH] i2c-at91: fix data-loss issue

[Hubert Feurstein]

Am 16. April 2012 09:30 schrieb Voss, Nikolaus <N.Voss@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

[PATCH] i2c-at91: fix data-loss issue

[Wolfram Sang]

> 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?

I think sending a new series would be good. It depends a bit on the amount
of donated tags (Tested-by, especially) if it will make it into 3.5.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120418/133c3334/attachment.sig>

[PATCH] i2c-at91: fix data-loss issue

[Voss, Nikolaus]

Adrian Yanes wrote on 2012-04-21:
> On the other hand we found that the Underrun Error (UNRE) is not handled
> in the driver. When we send data up > 2-4 bytes (quite random to say
> when it really fails) and we add some dev_dbg() to print
> dev->transfer_status we get 141 (==UNRE). According with the datasheet,
> after the first UNRE received "this action automatically generated a
> STOP bit in Master Mode. Reset by read in TWI_SR when TXCOMP is set."
> 
> We thought that one possibility for this was that the board was too slow
> to process the requests or that other interrupts were interfering.
> Disabling the interrupts inside of the twi interrupt handler did not
> help either.
> 
> The datasheet does not mention any method to implement some mechanism to
> avoid the UNRE telling the hardware to wait a bit longer for the next
> byte. Thus, one way will be to restart the transfer with the remaining
> bytes (maybe only applicable to at91rm9200) or just to return some
> error/message to userland informing that could not send all the data.

The latter is probably the easiest and most transparent solution.
There is no UNRE on G45, it just pauses the clock on an underrun
condition.

So in case UNRE is set, EIO should be returned similar to the already
handled OVRE:

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index a6f9e73..a84e19b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
 		dev_err(dev->dev, "overrun while reading\n");
 		return -EIO;
 	}
+	if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
+		dev_err(dev->dev, "underrun while writing\n");
+		return -EIO;
+	}
+
 	dev_dbg(dev->dev, "transfer complete\n");
 
 	return 0;

[PATCH] i2c-at91: fix data-loss issue

[Adrian Yanes]

> The latter is probably the easiest and most transparent solution.
> There is no UNRE on G45, it just pauses the clock on an underrun
> condition.
> 
> So in case UNRE is set, EIO should be returned similar to the already
> handled OVRE:
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index a6f9e73..a84e19b 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
>  		dev_err(dev->dev, "overrun while reading\n");
>  		return -EIO;
>  	}
> +	if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
> +		dev_err(dev->dev, "underrun while writing\n");
> +		return -EIO;
> +	}
> +
>  	dev_dbg(dev->dev, "transfer complete\n");
>  
>  	return 0;

Indeed, this should be added in order to catch this exception for the
AT91RM9200.

However, the main issue still there: the board is not able to deliver
data up to 2 bytes size without to run in the UNRE case.

Measuring the i2c bus, we verified that the data is not even arriving to
the data bus, i.e. either the kernel driver is too slow to handle it or
just we are missing some tweak required by the hardware
(nevertheless the datasheet does not give any clue).

Anyone with a AT91RM9200 that can test the proposed driver as well? it
will discard that is our hardware/board the issue rather than the
chipset itself.

Adrian