AT91: replace broken TWI driver i2c-at91.c

PATCH Superseded Nikolaus Voss 2 participants 6 messages Nov 8, 2011 – Nov 10, 2011

Subsystems (5)

Arm (32-bit) Arm sub-architectures I2C subsystem host drivers Arm/Microchip (AT91) SoC support +1 more subsystems

lore

Share & tools

lore

Nikolaus Voss

author

2011-11-10 14:08 UTC #0 lore raw

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

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/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              |  511 ++++++++++++++++------------
 drivers/i2c/busses/i2c-at91.h              |   80 +++++
 12 files changed, 390 insertions(+), 295 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

Nikolaus Voss

author

2011-11-08 10:49 UTC #1 lore raw

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

Signed-off-by Nikolaus Voss <[email protected]>

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

Nikolaus Voss

author

2011-11-08 11:09 UTC #2 lore raw

PATCH v5 2/4 Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk

In reply to Nikolaus Voss

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

Signed-off-by Nikolaus Voss <[email protected]>

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

arch/arm/mach-at91/at91cap9.c +1 -0

index ecdd54d..e69d5e0 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),
 };

arch/arm/mach-at91/at91rm9200.c +1 -0

index 713d3bd..a52f777 100644
@@ -193,6 +193,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),
 };

arch/arm/mach-at91/at91sam9260.c +1 -0

index b84a9f6..6e98738 100644
@@ -199,6 +199,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("t4_clk", "atmel_tcb.1", &tc4_clk),
 	CLKDEV_CON_DEV_ID("t5_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),

arch/arm/mach-at91/at91sam9261.c +1 -0

index 658a518..4946fb5 100644
@@ -176,6 +176,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),
 };
 
 static struct clk_lookup usart_clocks_lookups[] = {

arch/arm/mach-at91/at91sam9263.c +1 -0

index f83fbb0..2c14e43 100644
@@ -189,6 +189,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),
 };

arch/arm/mach-at91/at91sam9g45.c +2 -0

index 318b040..357f056 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),

arch/arm/mach-at91/at91sam9rl.c +2 -0

index a238105..312beb5 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(NULL, "at91_i2c.0", &twi0_clk),
+	CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
 };
 
 static struct clk_lookup usart_clocks_lookups[] = {

Signature · 1 line

1.7.5.4

Nikolaus Voss

author

2011-11-08 10:49 UTC #3 lore raw

PATCH v5 3/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.

Signed-off-by Nikolaus Voss <[email protected]>

---
 drivers/i2c/busses/Kconfig    |   11 +-
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-at91.c |  410 +++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h |   80 ++++++++
 4 files changed, 495 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 +410 -0

410 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

Felipe Balbi

Reviewed-by on v5 3/4

2011-11-10 20:10 UTC #4 lore raw

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

Quoted 11 lines · 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.
Signed-off-by Nikolaus Voss <[email protected]>

FWIW:

Reviewed-by Felipe Balbi <[email protected]>

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/20111110/fdf1b885/attachment.sig>

Nikolaus Voss

author

2011-11-08 11:11 UTC #5 lore raw

PATCH v5 4/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?)

Signed-off-by Nikolaus Voss <[email protected]>

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