AT91: replace broken TWI driver i2c-at91.c

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.

A remaining limitation is the fact, that only one repeated start is possible (two concatenated messages). This limitation is imposed by the hardware. However, this should not be a problem as all common i2c-client communication does not rely on more than one repeated start.

v9: i) merge i2c-at91.c patches to single patch
ii) rewrite cwgr reg after timeout to reenable the right twi clock
v8: i) remove local include
ii) remove cpu_is_rm9200() and put twi hw bug info in platform_device iii) use readl/writel_relaxed instead of __raw_readl/writel
v7: i) fix bug if internal address > 1 byte

    ii) send stop when len == 1 (both reported by Carsten Behling)

v6: support for I2C_SMBUS_BLOCK_DATA transfers.
Better use of clk_(un)prepare().
More sensible transfer timeout.
v5: Another round of review comments from Ryan Mallon, Felipe Balbi
and Russell King: convert twi clk to use .dev_id, cleanups
v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
Moved register include file to local include, code cleanups
v3: Integrated review comments from Ryan Mallon and Felipe Balbi
v2: Fixed whitespace issue

Nikolaus Voss (4):
drivers/i2c/busses/i2c-at91.c: remove broken driver Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk drivers/i2c/busses/i2c-at91.c: add new driver
G45 TWI: remove open drain setting for twi function gpios

 arch/arm/mach-at91/at91cap9.c              |    1 +
 arch/arm/mach-at91/at91rm9200.c            |    1 +
 arch/arm/mach-at91/at91rm9200_devices.c    |    9 +
 arch/arm/mach-at91/at91sam9260.c           |    1 +
 arch/arm/mach-at91/at91sam9261.c           |    1 +
 arch/arm/mach-at91/at91sam9263.c           |    1 +
 arch/arm/mach-at91/at91sam9g45.c           |    2 +
 arch/arm/mach-at91/at91sam9g45_devices.c   |    6 -
 arch/arm/mach-at91/at91sam9rl.c            |    2 +
 arch/arm/mach-at91/include/mach/at91_twi.h |   68 ----
 drivers/i2c/busses/Kconfig                 |   11 +-
 drivers/i2c/busses/i2c-at91.c              |  561 +++++++++++++++++-----------
 12 files changed, 373 insertions(+), 291 deletions(-)
 delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h

1.7.5.4

---
 arch/arm/mach-at91/include/mach/at91_twi.h |   68 ------
 drivers/i2c/busses/Makefile                |    1 -
 drivers/i2c/busses/i2c-at91.c              |  314 ----------------------------
 3 files changed, 0 insertions(+), 383 deletions(-)
 delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
 delete mode 100644 drivers/i2c/busses/i2c-at91.c

deleted file mode 100644
index bb2880f..0000000
@@ -1,68 +0,0 @@
-/*
- * arch/arm/mach-at91/include/mach/at91_twi.h
- *
- * Copyright (C) 2005 Ivan Kokshaysky
- * Copyright (C) SAN People
- *
- * Two-wire Interface (TWI) registers.
- * Based on AT91RM9200 datasheet revision E.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#ifndef AT91_TWI_H
-#define AT91_TWI_H
-
-#define	AT91_TWI_CR		0x00		/* Control Register */
-#define		AT91_TWI_START		(1 <<  0)	/* Send a Start Condition */
-#define		AT91_TWI_STOP		(1 <<  1)	/* Send a Stop Condition */
-#define		AT91_TWI_MSEN		(1 <<  2)	/* Master Transfer Enable */
-#define		AT91_TWI_MSDIS		(1 <<  3)	/* Master Transfer Disable */
-#define		AT91_TWI_SVEN		(1 <<  4)	/* Slave Transfer Enable [SAM9260 only] */
-#define		AT91_TWI_SVDIS		(1 <<  5)	/* Slave Transfer Disable [SAM9260 only] */
-#define		AT91_TWI_SWRST		(1 <<  7)	/* Software Reset */
-
-#define	AT91_TWI_MMR		0x04		/* Master Mode Register */
-#define		AT91_TWI_IADRSZ		(3    <<  8)	/* Internal Device Address Size */
-#define			AT91_TWI_IADRSZ_NO		(0 << 8)
-#define			AT91_TWI_IADRSZ_1		(1 << 8)
-#define			AT91_TWI_IADRSZ_2		(2 << 8)
-#define			AT91_TWI_IADRSZ_3		(3 << 8)
-#define		AT91_TWI_MREAD		(1    << 12)	/* Master Read Direction */
-#define		AT91_TWI_DADR		(0x7f << 16)	/* Device Address */
-
-#define	AT91_TWI_SMR		0x08		/* Slave Mode Register [SAM9260 only] */
-#define		AT91_TWI_SADR		(0x7f << 16)	/* Slave Address */
-
-#define	AT91_TWI_IADR		0x0c		/* Internal Address Register */
-
-#define	AT91_TWI_CWGR		0x10		/* Clock Waveform Generator Register */
-#define		AT91_TWI_CLDIV		(0xff <<  0)	/* Clock Low Divisor */
-#define		AT91_TWI_CHDIV		(0xff <<  8)	/* Clock High Divisor */
-#define		AT91_TWI_CKDIV		(7    << 16)	/* Clock Divider */
-
-#define	AT91_TWI_SR		0x20		/* Status Register */
-#define		AT91_TWI_TXCOMP		(1 <<  0)	/* Transmission Complete */
-#define		AT91_TWI_RXRDY		(1 <<  1)	/* Receive Holding Register Ready */
-#define		AT91_TWI_TXRDY		(1 <<  2)	/* Transmit Holding Register Ready */
-#define		AT91_TWI_SVREAD		(1 <<  3)	/* Slave Read [SAM9260 only] */
-#define		AT91_TWI_SVACC		(1 <<  4)	/* Slave Access [SAM9260 only] */
-#define		AT91_TWI_GACC		(1 <<  5)	/* General Call Access [SAM9260 only] */
-#define		AT91_TWI_OVRE		(1 <<  6)	/* Overrun Error [AT91RM9200 only] */
-#define		AT91_TWI_UNRE		(1 <<  7)	/* Underrun Error [AT91RM9200 only] */
-#define		AT91_TWI_NACK		(1 <<  8)	/* Not Acknowledged */
-#define		AT91_TWI_ARBLST		(1 <<  9)	/* Arbitration Lost [SAM9260 only] */
-#define		AT91_TWI_SCLWS		(1 << 10)	/* Clock Wait State [SAM9260 only] */
-#define		AT91_TWI_EOSACC		(1 << 11)	/* End of Slave Address [SAM9260 only] */
-
-#define	AT91_TWI_IER		0x24		/* Interrupt Enable Register */
-#define	AT91_TWI_IDR		0x28		/* Interrupt Disable Register */
-#define	AT91_TWI_IMR		0x2c		/* Interrupt Mask Register */
-#define	AT91_TWI_RHR		0x30		/* Receive Holding Register */
-#define	AT91_TWI_THR		0x34		/* Transmit Holding Register */
-
-#endif
-

index fba6da6..e8a1852 100644
@@ -28,7 +28,6 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
-obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o

1.7.5.4

The old driver used con_id clock entries. Convert to use dev_id for clock lookup via standard method.

---
 arch/arm/mach-at91/at91cap9.c    |    1 +
 arch/arm/mach-at91/at91rm9200.c  |    1 +
 arch/arm/mach-at91/at91sam9260.c |    1 +
 arch/arm/mach-at91/at91sam9261.c |    1 +
 arch/arm/mach-at91/at91sam9263.c |    1 +
 arch/arm/mach-at91/at91sam9g45.c |    2 ++
 arch/arm/mach-at91/at91sam9rl.c  |    2 ++
 7 files changed, 9 insertions(+), 0 deletions(-)

index a42edc2..676d6cf 100644
@@ -219,6 +219,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
 	/* fake hclk clock */
 	CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
 	CLKDEV_CON_ID("pioA", &pioABCD_clk),

index 99c3174..7c597e5 100644
@@ -194,6 +194,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
 	/* fake hclk clock */
 	CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
 	CLKDEV_CON_ID("pioA", &pioA_clk),

index d4036ba..e21a3dc 100644
@@ -200,6 +200,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("t1_clk", "atmel_tcb.1", &tc4_clk),
 	CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.1", &tc5_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
 	/* more usart lookup table for DT entries */
 	CLKDEV_CON_DEV_ID("usart", "fffff200.serial", &mck),
 	CLKDEV_CON_DEV_ID("usart", "fffb0000.serial", &usart0_clk),

index 023c2ff..b84d882 100644
@@ -175,6 +175,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
 	CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &hck0),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
 	CLKDEV_CON_ID("pioA", &pioA_clk),
 	CLKDEV_CON_ID("pioB", &pioB_clk),
 	CLKDEV_CON_ID("pioC", &pioC_clk),

index 75e876c..e6e6a80 100644
@@ -190,6 +190,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.0", &spi0_clk),
 	CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
 	CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
 	/* fake hclk clock */
 	CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
 	CLKDEV_CON_ID("pioA", &pioA_clk),

index 1cb6a96..9e7ca32 100644
@@ -220,6 +220,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
 	CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb0_clk),
 	CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
 	CLKDEV_CON_DEV_ID(NULL, "atmel-trng", &trng_clk),

index d2c91a8..2c88b94 100644
@@ -183,6 +183,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.0", &tc2_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
 	CLKDEV_CON_ID("pioA", &pioA_clk),
 	CLKDEV_CON_ID("pioB", &pioB_clk),
 	CLKDEV_CON_ID("pioC", &pioC_clk),

1.7.5.4

This driver has the following properties compared to the old driver:
1. Support for multiple interfaces.
2. Interrupt driven I/O as opposed to polling/busy waiting.
3. Support for _one_ repeated start (Sr) condition, which is enough for most real-world applications including all SMBus transfer types. (The hardware does not support issuing arbitrary Sr conditions on the

    bus.)

testing: SoC: at91sam9g45
- BQ20Z80 battery SMBus client.
- on a 2.6.38 kernel with several i2c clients (temp-sensor, audio-codec, touchscreen-controller, w1-bridge, io-expanders)

---
 arch/arm/mach-at91/at91rm9200_devices.c |    9 +
 drivers/i2c/busses/Kconfig              |   11 +-
 drivers/i2c/busses/Makefile             |    1 +
 drivers/i2c/busses/i2c-at91.c           |  455 +++++++++++++++++++++++++++++++
 4 files changed, 469 insertions(+), 7 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-at91.c

index 97676bd..0912f29 100644
@@ -493,9 +493,18 @@ static struct resource twi_resources[] = {
 	},
 };
 
+static const struct platform_device_id twi_id = {
+	/*
+	 * driver_data is "1" for hardware with ckdiv upper limit == 5
+	 * (AT91RM9200 erratum 22), "0" for twi modules without this bug
+	 */
+	.driver_data	= 1,
+};
+
 static struct platform_device at91rm9200_twi_device = {
 	.name		= "at91_i2c",
 	.id		= -1,
+	.id_entry	= &twi_id,
 	.resource	= twi_resources,
 	.num_resources	= ARRAY_SIZE(twi_resources),
 };

index 3101dd5..92158df 100644
@@ -285,18 +285,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
 
 config I2C_AT91
 	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
-	depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+	depends on ARCH_AT91 && EXPERIMENTAL
 	help
 	  This supports the use of the I2C interface on Atmel AT91
 	  processors.
 
-	  This driver is BROKEN because the controller which it uses
-	  will easily trigger RX overrun and TX underrun errors.  Using
-	  low I2C clock rates may partially work around those issues
-	  on some systems.  Another serious problem is that there is no
-	  documented way to issue repeated START conditions, as needed
+	  A serious problem is that there is no documented way to issue
+	  repeated START conditions for more than two messages, as needed
 	  to support combined I2C messages.  Use the i2c-gpio driver
-	  unless your system can cope with those limitations.
+	  unless your system can cope with this limitation.
 
 config I2C_AU1550
 	tristate "Au1550/Au1200/Au1300 SMBus interface"

index e8a1852..fba6da6 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o

1.7.5.4

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

Hi,

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>

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.

---
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(-)

index 40c39a2..295835f 100644
@@ -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

Couple of very minor comments below.

~Ryan

Hubert Feurstein wrote on 2012-04-13:

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.

Am 16. April 2012 09:30 schrieb Voss, Nikolaus <[email protected]>:

Adding a check in read_next_byte() would be good just for safety.

No special reason for this.

I agree, it should be save to xor at least those two.

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.

Hubert

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>

Hello,

we performed some measurements with the new version of the patch (v10), in order to verify that the twi clock was performing better with the new ternary operator introduced for the calculation of CLDIV & CHDIV.

However, we noticed we were using the wrongs resistors in our setup, changing them to the correct ones, the driver gives us a frequency rate of ~396kHz when we tweak the TWI_CLK_HZ to 400000. Using the previous version the driver (without the specific offset for the AT91RM9200) it gives a frequency rate of ~392kHz. Therefore, both rates are inside of the error margin that the oscilloscope can give, so we suggest to keep the latest code introduced for it, mostly because it behaves as the datasheet claims.

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.

Adrian

Adrian Yanes wrote on 2012-04-21:

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:

index a6f9e73..a84e19b 100644
@@ -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;

Apparently the issue with the AT91RM9200 is the chipset itself due to a factory design defect. Some people claim that the root cause of it is:

"Since the TWI doesn't have a DMA channel there is no way to deliver timely data to the Tx buffer under heavy CPU load."[1]

Other threads[2] highlights same issues with the AT91RM9200 and the data loss. Even in 2007, Ronny Nilsson tried to do some patch[3] blocking the interrupts in order to win more feasibility, some testing was performed[4], and up to 100kHz (exactly the same value as we started getting errors), the driver just failed. At the end of the discussion the conclusion was that the TWI controller of the AT91RM9200 is just broken and the GPIO driver is the only alternative.

Therefore, your driver will work for all of those chipsets using PDC with the TWI. However, it won't work with the AT91RM9200 (and probably old hardware variants that keep same hardware design).

So I recommend to add a note/comment indicating such incompatibility for the AT91RM9200, either in the source code or in the kernel config.

Thanks.

Adrian

1 - http://lists.atrpms.net/pipermail/i2c/2007-October/002026.html
2 - http://lists.lm-sensors.org/pipermail/i2c/2007-February/000773.html
3 - http://lists.lm-sensors.org/pipermail/i2c/2007-February/000833.html
4 - http://lists.atrpms.net/pipermail/i2c/2007-October/002034.html

The G45 datasheets explicitly states that setting the open drain property on peripheral function gpios is not allowed. (How about other A91 chips?)

---
 arch/arm/mach-at91/at91sam9g45_devices.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

index 96e2adc..050e6e2 100644
@@ -685,18 +685,12 @@ void __init at91_add_device_i2c(short i2c_id, struct i2c_board_info *devices, in
 	/* pins used for TWI interface */
 	if (i2c_id == 0) {
 		at91_set_A_periph(AT91_PIN_PA20, 0);		/* TWD */
-		at91_set_multi_drive(AT91_PIN_PA20, 1);
-
 		at91_set_A_periph(AT91_PIN_PA21, 0);		/* TWCK */
-		at91_set_multi_drive(AT91_PIN_PA21, 1);
 
 		platform_device_register(&at91sam9g45_twi0_device);
 	} else {
 		at91_set_A_periph(AT91_PIN_PB10, 0);		/* TWD */
-		at91_set_multi_drive(AT91_PIN_PB10, 1);
-
 		at91_set_A_periph(AT91_PIN_PB11, 0);		/* TWCK */
-		at91_set_multi_drive(AT91_PIN_PB11, 1);
 
 		platform_device_register(&at91sam9g45_twi1_device);
 	}

1.7.5.4