Skip to contentSkip to conversation

AT91: replace broken TWI driver i2c-at91.c

PATCH Superseded Nikolaus Voss 3 participants 7 messages Nov 8, 2011 – Nov 9, 2011

Subsystems (5)

v4 of 11 Superseded Latest is v11 →

lore
Share & tools
lore
#0 lore raw

v4 cover 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.

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 drivers/i2c/busses/i2c-at91.c: add new driver
G45 TWI: remove open drain setting for twi function gpios Add lookup entries for twi_clk for devices with more than one TWI
port

 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              |  476 +++++++++++++++++-----------
 drivers/i2c/busses/i2c-at91.h              |   80 +++++
 7 files changed, 371 insertions(+), 274 deletions(-)
 delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
 create mode 100644 drivers/i2c/busses/i2c-at91.h
Signature · 1 line
1.7.5.4
#1 lore raw

PATCH v4 1/4 drivers/i2c/busses/i2c-at91.c: remove broken driver

---
 arch/arm/mach-at91/include/mach/at91_twi.h |   68 ------
 drivers/i2c/busses/Makefile                |    1 -
 drivers/i2c/busses/i2c-at91.c              |  327 ----------------------------
 3 files changed, 0 insertions(+), 396 deletions(-)
 delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
 delete mode 100644 drivers/i2c/busses/i2c-at91.c
arch/arm/mach-at91/include/mach/at91_twi.h deleted +0 -68
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
-
drivers/i2c/busses/Makefile +0 -1
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
drivers/i2c/busses/i2c-at91.c deleted +0 -327

327 lines not on this page · raw message

Signature · 1 line
1.7.5.4
#2 lore raw

PATCH v4 2/4 drivers/i2c/busses/i2c-at91.c: add new driver

In reply to Nikolaus Voss

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

Tested on Atmel G45 with BQ20Z80 battery SMBus client.

---
 drivers/i2c/busses/Kconfig    |   11 +-
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-at91.c |  417 +++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h |   80 ++++++++
 4 files changed, 502 insertions(+), 7 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-at91.c
 create mode 100644 drivers/i2c/busses/i2c-at91.h
drivers/i2c/busses/Kconfig +4 -7
index a3afac4..2ef618d 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 SMBus interface"
drivers/i2c/busses/Makefile +1 -0
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
drivers/i2c/busses/i2c-at91.c new file +417 -0

417 lines not on this page · raw message

drivers/i2c/busses/i2c-at91.h new file +80 -0

80 lines not on this page · raw message

Signature · 1 line
1.7.5.4
#3 lore raw

Re: [PATCH v4 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

Quoted 25 lines · Nikolaus Voss
drivers/i2c/busses/Makefile +1 -0
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
drivers/i2c/busses/i2c-at91.c new file +7 -0
new file mode 100644
index 0000000..5f4be34
@@ -0,0 +1,417 @@
+/*
+    i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
+
+    Copyright (C) 2011 Nikolaus Voss <[email protected]>
+
+    Evolved from original work by:
+    Copyright (C) 2004 Rick Bronson

drivers/i2c/busses/i2c-at91.c line 17

Quoted 10 lines
drivers/i2c/busses/i2c-at91.c new file +10 -0
+    Converted to 2.6 by Andrew Victor <[email protected]>
+
+    Borrowed heavily from original work by:
+    Copyright (C) 2000 Philip Edelbrock <[email protected]>
+
+    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.
+*/

wrong multi-line comment style.

drivers/i2c/busses/i2c-at91.c line 72

Quoted 1 line
drivers/i2c/busses/i2c-at91.c new file +1 -0
+static void at91_set_twi_clock(struct at91_twi_dev *dev)

this should be __devinit as it's only used by at91_twi_hwinit()

drivers/i2c/busses/i2c-at91.c line 76

Quoted 4 lines
drivers/i2c/busses/i2c-at91.c new file +4 -0
+{
+	unsigned long cdiv, ckdiv;
+
+	/* Calcuate clock dividers and round up */

typo: calculate.

drivers/i2c/busses/i2c-at91.c line 77

Quoted 1 line
drivers/i2c/busses/i2c-at91.c new file +1 -0
+	cdiv = (clk_get_rate(dev->clk) / (2 * TWI_CLOCK_HZ)) - 3 + 1;

DIV_ROUND_UP() ??

drivers/i2c/busses/i2c-at91.c line 85

Quoted 8 lines
drivers/i2c/busses/i2c-at91.c new file +8 -0
+	ckdiv = 0;
+	while (cdiv > 255) {
+		ckdiv++;
+		cdiv = cdiv >> 1;
+	}
+
+	if (cpu_is_at91rm9200() && (ckdiv > 5)) {
+		dev_err(dev->dev, "AT91RM9200 Erratum #22: using ckdiv = 5.\n");

is it really an error ? Or would it be enough as dev_dbg() ?

Quoted 19 lines
drivers/i2c/busses/i2c-at91.c new file +19 -0
+static int at91_do_twi_transfer(struct at91_twi_dev *dev, bool is_read)
+{
+	int ret;
+
+	INIT_COMPLETION(dev->cmd_complete);
+	if (is_read) {
+		if (!dev->buf_len)
+			at91_twi_write(dev, AT91_TWI_CR,
+				       AT91_TWI_START | AT91_TWI_STOP);
+		else
+			at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
+		at91_twi_write(dev, AT91_TWI_IER,
+			       AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+	} else {
+		at91_twi_write_next_byte(dev);
+		at91_twi_write(dev, AT91_TWI_IER,
+			       AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+	}
+

drivers/i2c/busses/i2c-at91.c line 176

Quoted 10 lines
drivers/i2c/busses/i2c-at91.c new file +10 -0
+	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+	if (ret == 0) {
+		dev_err(dev->dev, "controller timed out\n");
+		at91_init_twi_bus(dev);
+		return -ETIMEDOUT;
+	}
+	if (dev->transfer_status & AT91_TWI_NACK) {
+		dev_dbg(dev->dev, "received nack\n");
+		return -ENODEV;

not sure error code matches here. If the HW replies with NACK you tell your users there's no I2C adapter ? Sounds a bit weird to me...

drivers/i2c/busses/i2c-at91.c line 259

Quoted 11 lines
drivers/i2c/busses/i2c-at91.c new file +11 -0
+static int __devinit at91_twi_probe(struct platform_device *pdev)
+{
+	struct at91_twi_dev *dev;
+	struct resource *mem, *irq, *ioarea;
+	int rc;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		return -ENODEV;
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

int irq = platform_get_irq(pdev, 0), would work better for you. In case of error it will return -ENXIO

drivers/i2c/busses/i2c-at91.c line 267

Quoted 8 lines
drivers/i2c/busses/i2c-at91.c new file +8 -0
+	if (!irq)
+		return -ENODEV;
+
+	ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
+	if (!ioarea)
+		return -EBUSY;
+
+	dev = kzalloc(sizeof(struct at91_twi_dev), GFP_KERNEL);

sizeof(*dev) would allow you to change the type of dev without having to patch this line. Quite unlikely, I know, but still...

Signature · 8 lines
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/20111109/89190d73/attachment-0001.sig>
#4 lore raw

Re: [PATCH v4 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

Quoted 6 lines · Felipe Balbi

Hi,

Quoted 2 lines · Nikolaus Voss
drivers/i2c/busses/Makefile +0 -0
index e8a1852..fba6da6 100644
Quoted 10 lines

drivers/i2c/busses/Makefile

Quoted 8 lines
drivers/i2c/busses/Makefile +8 -0
+	ckdiv = 0;
+	while (cdiv > 255) {
+		ckdiv++;
+		cdiv = cdiv >> 1;
+	}
+
+	if (cpu_is_at91rm9200() && (ckdiv > 5)) {
+		dev_err(dev->dev, "AT91RM9200 Erratum #22: using ckdiv = 5.\n");

is it really an error ? Or would it be enough as dev_dbg() ?

dev_warn is probably appropriate.

Quoted 25 lines
Quoted 25 lines
drivers/i2c/busses/Makefile +24 -0
+static int at91_do_twi_transfer(struct at91_twi_dev *dev, bool is_read)
+{
+	int ret;
+
+	INIT_COMPLETION(dev->cmd_complete);
+	if (is_read) {
+		if (!dev->buf_len)
+			at91_twi_write(dev, AT91_TWI_CR,
+				       AT91_TWI_START | AT91_TWI_STOP);
+		else
+			at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
+		at91_twi_write(dev, AT91_TWI_IER,
+			       AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+	} else {
+		at91_twi_write_next_byte(dev);
+		at91_twi_write(dev, AT91_TWI_IER,
+			       AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+	}
+
+	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+	if (ret == 0) {
+		dev_err(dev->dev, "controller timed out\n");
+		at91_init_twi_bus(dev);
Quoted 8 lines

drivers/i2c/busses/Makefile

Quoted 5 lines
drivers/i2c/busses/Makefile +5 -0
+		return -ETIMEDOUT;
+	}
+	if (dev->transfer_status & AT91_TWI_NACK) {
+		dev_dbg(dev->dev, "received nack\n");
+		return -ENODEV;

not sure error code matches here. If the HW replies with NACK you tell your users there's no I2C adapter ? Sounds a bit weird to me...

I think -ENODEV was used because a NACK can mean that there is no device at the address you are trying to talk to. Other drivers appear to use -EIO or -EREMOTEIO. The latter is possibly more correct.

~Ryan

#5 lore raw

PATCH v4 3/4 G45 TWI: remove open drain setting for twi function gpios

In reply to Nikolaus Voss

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(-)
arch/arm/mach-at91/at91sam9g45_devices.c +0 -6
index 000b5e1..e0f67e7 100644
@@ -684,18 +684,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);
 	}
Signature · 1 line
1.7.5.4
#6 lore raw

PATCH v4 4/4 Add lookup entries for twi_clk for devices with more than one TWI port

In reply to Nikolaus Voss
---
 arch/arm/mach-at91/at91sam9g45.c |    2 ++
 arch/arm/mach-at91/at91sam9rl.c  |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)
arch/arm/mach-at91/at91sam9g45.c +2 -0
index 318b040..a374899 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("twi_clk", "at91_i2c.0", &twi0_clk),
+	CLKDEV_CON_DEV_ID("twi_clk", "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),
arch/arm/mach-at91/at91sam9rl.c +2 -0
index a238105..ab81223 100644
@@ -184,6 +184,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("twi_clk", "at91_i2c.0", &twi0_clk),
+	CLKDEV_CON_DEV_ID("twi_clk", "at91_i2c.1", &twi1_clk),
 };
 
 static struct clk_lookup usart_clocks_lookups[] = {
Signature · 1 line
1.7.5.4