[i2c] [PATCH] AT91 bus driver loses data

[Ronny Nilsson]

Hi
Sorry for the delay. Below are some answers to you comments.


> > diff -rup linux-2.6.21-rc4-orig/drivers/i2c/busses/i2c-at91.c

> > +#include <asm/semaphore.h>
> There is a mutex primitive now (for some time actually).
> Use <linux/mutex.h> instead.

Ok, thanks.



> > +static struct twi_transfer_t {
> > +	unsigned char *buf;
> > +	int length;
> > +	u32 status;				/* Saved flags from HW interface */
> > +	volatile char isFinished;
>
> Also, volatile is not sufficient here.  You must use atomic
> operations on this variable both in interrupt and task context. 
> Actually, you don't need this variable at all.  You can test the
> status value from the hardware as part of the wait_ condition.  This
> is what I recommend.

My initial idea was to read the status directly from the HW register 
both in wait_event() and the ISR. However that ain't possible since 
merely by doing a read one implicitly makes an ACK of the error flags 
too. Your suggestion of using xchg() inside wait_event() might be worth 
a try though.



> It's not part of your patch, but I have a question about two lines
> here...
>
>  68 #define at91_twi_read(reg)              __raw_readl(twi_base
>  69 #define at91_twi_write(reg, val)        __raw_writel((val),
>
> I understand that the __raw variants are used here in order to avoid
> the byte swapping.  However, this also loses the IO ordering
> guarantees of a straight readl/writel.  I don't know if this could be
> a source of problems for this driver (given that it will only be used
> on one specific platform), but I guess it could be.  Reading through
> asm-ppc/io.h, it looks like we should use in_be32() and out_be32()
> here instead.  Anyone else comment?

I have no satisfactory answer on this... Using outl() would probably 
break big endian ARM systems.



> > +static irqreturn_t xfer_read_irq(int irq, void *dev_id)
> > +{
> > +	register u32 status = at91_twi_read(AT91_TWI_SR);
> > +	int rc = IRQ_NONE;
> >
> > -	/* Read data */
> > -	while (length--) {
> > -		if (!length)	/* need to send Stop  */
> > +	if (likely(status & AT91_TWI_RXRDY && twi_transfer.length)) {
> > +		if (--twi_transfer.length <= 1) {
> > +			/* When notified of the second to last
> > +			 * byte the last one is already on the
> > +			 * way too. Time for sending Stop. */
> >  			at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> > -		if (!at91_poll_status(AT91_TWI_RXRDY)) {
> > -			dev_dbg(&adap->dev, "RXRDY timeout\n");
> > -			return -ETIMEDOUT;
> > +			at91_twi_write(AT91_TWI_IER, AT91_TWI_TXCOMP);
> >  		}
> > -		*buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
> > +		*twi_transfer.buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
> > +		rc = IRQ_HANDLED;
> >  	}
>
> That doesn't make sense.  After you read the second-to-last byte, you
> send the STOP condition and set the IRQ mask to allow the TXCOMP
> event only.  Where are you reading the last byte?
>
> Wait, I see... you *will* pass through there one more time with RXRDY
> set and pick up the last byte... but that means you're going to do
> TWI_CR <= STOP twice.  The datasheet doesn't show anything like that.

You're quite right. I found out the hard way the datasheet doesn't tell 
us the whole truth. The problem seems to be that even though you issue 
a STOP condition one further byte will be clocked in from the slave 
(which the datasheet says nothing about). Asserting STOP twice is an 
attempt to be on the safe side. A single second-to-last STOP works too 
though.



> > -	return 0;
> > +	if (unlikely(status & AT91_TWI_TXCOMP)) {
> > +		twi_transfer.status |= status;
>
> If I understand you... the OR here is to propogate a possible error
> status from an earlier run through the ISR.  IMHO it would be cleaner
> to clear IER (mask all interrupts) and just poll the SR to completion
> in case of an error. Then you could do something like this...
>
> > +		at91_twi_write(AT91_TWI_IDR, 0xffffffff);
> > +		twi_transfer.isFinished = 1;
> > +		wake_up(&twi_transfer.sleepQ);
>
> 		xchg(&twi_transfer.status, status);
> 		wake_up_interruptible(&twi_transfer.sleepq);
>
> ... getting rid of isFinished altogether.

OK, I'll try it out.



> > +static irqreturn_t xfer_write_irq(int irq, void *dev_id)
> > +{
> > +	register u32 status = at91_twi_read(AT91_TWI_SR);
> > +	int rc = IRQ_NONE;
> > +
> > +	if (unlikely(status & AT91_TWI_TXCOMP)) {
> > +		twi_transfer.status |= status;
> > +		at91_twi_write(AT91_TWI_IDR, 0xffffffff);
> > +		twi_transfer.isFinished = 1;
> > +		wake_up(&twi_transfer.sleepQ);
> > +		rc = IRQ_HANDLED;
> > +	} else if (unlikely(status & (AT91_TWI_UNRE | AT91_TWI_NACK))) {
> > +		at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> > +		at91_twi_write(AT91_TWI_IDR, ~AT91_TWI_TXCOMP);
> > +		at91_twi_write(AT91_TWI_IER, AT91_TWI_TXCOMP);
> > +		twi_transfer.status |= status;
> > +		rc = IRQ_HANDLED;
> > +	} else if (likely(status & AT91_TWI_TXRDY)) {
> > +		if (twi_transfer.length) {
> > +			at91_twi_write(AT91_TWI_THR, *twi_transfer.buf++);
> > +			twi_transfer.length--;
> > +		} else {
> > +			at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> > +			at91_twi_write(AT91_TWI_IER, AT91_TWI_TXCOMP);
> > +			at91_twi_write(AT91_TWI_IDR, AT91_TWI_TXRDY);
> > +		}
> > +		rc = IRQ_HANDLED;
> > +	}
> > +
> > +	return rc;
> > +}
> > +
>
> Almost all the same comments for the read handler apply here.
>
> Also, I assume the IDR and IER are combined to produce the actual
> state of the interrupt source.  I.e. TWI_IMR == TWI_IER & ~TWI_IDR. 
> If that's the case, then please just init TWI_IDR to 0 and leave it
> alone; use TWI_IER as needed. What a bizarre register interface, btw.

Tell me about it... Unfortunately, using TWI_IER alone doesn't work 
(writing zeros to the bit fields has no effect and they are write 
only). There is one advantage to this scheme though, when modifying 
TWI_IMR a single write is enough. No read-modify-write operation is 
required as is usually the case.



> > +static int xfer_write(struct i2c_adapter *adap, unsigned char
> > *buf, int length) +{
> > +	int err = 0;
> >
> > -	/* Send Stop */
> > -	at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> > +	twi_transfer.buf = buf;
> > +	twi_transfer.length = length - 1;
> > +	twi_transfer.status = 0;
> > +	twi_transfer.isFinished = 0;
> > +	twi_transfer.direction = TWI_WRITE;
> > +	barrier();
> > +
> > +	/* Start transmission. The block of
> > +	 * disabled IRQs is a short but
> > +	 * critical section. */
> > +	local_irq_disable();
> > +	at91_twi_write(AT91_TWI_THR, *twi_transfer.buf++);
> > +	at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
> > +	at91_twi_write(AT91_TWI_IER, AT91_TWI_NACK |
> > +		AT91_TWI_UNRE | AT91_TWI_TXRDY);
> > +	local_irq_enable();
> > +
> > +	if (wait_event_interruptible_timeout(
> > +			twi_transfer.sleepQ, twi_transfer.isFinished,
> > +			(TWI_BITS_PER_BYTE * HZ * length) / clockrate +
> > +			TWI_TIMEOUT) < 0) {
> > +		xfer_abort();
> > +	}
> >
> > -	return 0;
> > +	/* Transfer finished. Any errors? A hw
> > +	 * issue somtimes flag underruns falsely,
> > +	 * hence need to check xfered length. */
> > +	if (twi_transfer.status & (AT91_TWI_NACK | AT91_TWI_UNRE) &&
> > +			twi_transfer.length) {
> > +		xfer_abort();
> > +		err = -EIO;
>
> Is this a published errata?  If so, please provide a link, thanks.

It's a leftover from something I discovered my self when doing trial 
and error testing in 100 kHz. However I haven't seen the problem since 
we lowered the clockrate. It can probably be removed by now.



> > @@ -143,6 +350,9 @@ static int at91_xfer(struct i2c_adapter
> >  {
> >  	int i, ret;
> >
> > +	if (down_interruptible(&twi_transfer.mutex))
> > +		return -ERESTARTSYS;
> > +
> >  	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> >
> >  	for (i = 0; i < num; i++) {
> > @@ -151,6 +361,7 @@ static int at91_xfer(struct i2c_adapter
> >  			pmsg->len, pmsg->len > 1 ? "s" : "",
> >  			pmsg->flags & I2C_M_RD ? "from" : "to",	pmsg->addr);
> >
> > +		at91_twi_hwinit();		/* Reset every transfer */
>
> This seems wasteful.  Whatever part of that function that needs to
> happen here, factor it out into a separate function.  E.g. I can't
> see any need to reinit the clock every transfer.

Extensive tests made my TWI controller to sometimes hang without this 
patch. (The datasheet doesn't tell you that either...) OK, I'll split 
the function up.


Regards
/Ronny