[U-Boot] [PATCH v2 0/4] ARMV7: OMAP: I2C driver: Restructure code to eliminate udelay calls and improve performance

[U-Boot] [PATCH v2 0/4] ARMV7: OMAP: I2C driver: Restructure code to eliminate udelay calls and improve performance

[Steve Sakoman]

I've been preparing a patch series for Beagle and Overo that detects expansion
board configuration information by reading a 128 byte I2C EEPROM on each
expansion board.

Using the OMAP I2C driver in its current form takes about 5-6 seconds to read
this small number of bytes!  Executing the i2c probe command takes close
to 10 seconds.

Examining the code I see that there are a large number of fairly long udelay calls
throughout the driver (10 - 50 milliseconds). I looked through the linux driver
and did not see equivalent delays in that code.  In fact the longest delay in the
linux code was one millisecond.

In looking at the TRM I2C section for OMAP3 and OMAP4 I don't see any requirement
for delays in the programming model description.

This patch restructures the i2c driver to eliminate most of the udelay calls
by monitoring state changes in the status register.

The EEPROM reads and the i2c probe execution are now instantaneous.

This patch series was tested on OMAP3 (Overo) and OMAP4 (Panda).  I do not
have access to OMAP2 hardware for testing.

Steve Sakoman (4):
  ARMV7: OMAP: I2C driver: Use same timeout value as linux kernel
    driver
  ARMV7: OMAP: I2C driver: Restructure i2c_read_byte function
  ARMV7: OMAP: I2C driver: Restructure i2c_write_byte function
  ARMV7: OMAP: I2C driver: Restructure i2c_probe function

 drivers/i2c/omap24xx_i2c.c |  204 ++++++++++++++++++++++++-------------------
 1 files changed, 114 insertions(+), 90 deletions(-)

[U-Boot] [PATCH v2 1/4] ARMV7: OMAP: I2C driver: Use same timeout value as linux kernel driver

[Steve Sakoman]

This patch matches the poll interval (1 millisecond) and timeout (1 second)
used in the linux driver. It also adds a return value of 0 in the event of
a timeout error and cleans up some formatting errors in that section of the
code.

Signed-off-by: Steve Sakoman <steve.sakoman@linaro.org>
Tested-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/omap24xx_i2c.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index 3febd1f..b69d051 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -27,7 +27,7 @@
 
 #include "omap24xx_i2c.h"
 
-#define I2C_TIMEOUT	10
+#define I2C_TIMEOUT	1000
 
 static void wait_for_bb (void);
 static u16 wait_for_pin (void);
@@ -392,13 +392,13 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
 
 static void wait_for_bb (void)
 {
-	int timeout = 10;
+	int timeout = I2C_TIMEOUT;
 	u16 stat;
 
 	writew(0xFFFF, &i2c_base->stat);	 /* clear current interruts...*/
 	while ((stat = readw (&i2c_base->stat) & I2C_STAT_BB) && timeout--) {
 		writew (stat, &i2c_base->stat);
-		udelay (50000);
+		udelay(1000);
 	}
 
 	if (timeout <= 0) {
@@ -411,7 +411,7 @@ static void wait_for_bb (void)
 static u16 wait_for_pin (void)
 {
 	u16 status;
-	int timeout = 10;
+	int timeout = I2C_TIMEOUT;
 
 	do {
 		udelay (1000);
@@ -424,8 +424,10 @@ static u16 wait_for_pin (void)
 	if (timeout <= 0) {
 		printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
 			readw (&i2c_base->stat));
-			writew(0xFFFF, &i2c_base->stat);
-}
+		writew(0xFFFF, &i2c_base->stat);
+		status = 0;
+	}
+
 	return status;
 }
 
-- 
1.7.0.4

[U-Boot] [PATCH v2 2/4] ARMV7: OMAP: I2C driver: Restructure i2c_read_byte function

[Steve Sakoman]

This patch removes the "magic number" delays and instead
monitors state changes in the status register bits.

Signed-off-by: Steve Sakoman <steve.sakomanlinaro.org>
Tested-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/omap24xx_i2c.c |   76 +++++++++++++++++++++----------------------
 1 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index b69d051..d176b5d 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -159,58 +159,56 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 	/* no stop bit needed here */
 	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX, &i2c_base->con);
 
-	status = wait_for_pin ();
-
-	if (status & I2C_STAT_XRDY) {
-		/* Important: have to use byte access */
-		writeb (regoffset, &i2c_base->data);
-		udelay (20000);
-		if (readw (&i2c_base->stat) & I2C_STAT_NACK) {
+	/* send register offset */
+	while (1) {
+		status = wait_for_pin();
+		if (status == 0 || status & I2C_STAT_NACK) {
 			i2c_error = 1;
+			goto read_exit;
+		}
+		if (status & I2C_STAT_XRDY) {
+			/* Important: have to use byte access */
+			writeb(regoffset, &i2c_base->data);
+			writew(I2C_STAT_XRDY, &i2c_base->stat);
+		}
+		if (status & I2C_STAT_ARDY) {
+			writew(I2C_STAT_ARDY, &i2c_base->stat);
+			break;
 		}
-	} else {
-		i2c_error = 1;
 	}
 
-	if (!i2c_error) {
-		writew (I2C_CON_EN, &i2c_base->con);
-		while (readw(&i2c_base->stat) &
-			(I2C_STAT_XRDY | I2C_STAT_ARDY)) {
-			udelay (10000);
-			/* Have to clear pending interrupt to clear I2C_STAT */
-			writew (0xFFFF, &i2c_base->stat);
+	/* set slave address */
+	writew(devaddr, &i2c_base->sa);
+	/* read one byte from slave */
+	writew(1, &i2c_base->cnt);
+	/* need stop bit here */
+	writew(I2C_CON_EN | I2C_CON_MST |
+		I2C_CON_STT | I2C_CON_STP,
+		&i2c_base->con);
+
+	/* receive data */
+	while (1) {
+		status = wait_for_pin();
+		if (status == 0 || status & I2C_STAT_NACK) {
+			i2c_error = 1;
+			goto read_exit;
 		}
-
-		/* set slave address */
-		writew (devaddr, &i2c_base->sa);
-		/* read one byte from slave */
-		writew (1, &i2c_base->cnt);
-		/* need stop bit here */
-		writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
-			&i2c_base->con);
-
-		status = wait_for_pin ();
 		if (status & I2C_STAT_RRDY) {
 #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
     defined(CONFIG_OMAP44XX)
-			*value = readb (&i2c_base->data);
+			*value = readb(&i2c_base->data);
 #else
-			*value = readw (&i2c_base->data);
+			*value = readw(&i2c_base->data);
 #endif
-			udelay (20000);
-		} else {
-			i2c_error = 1;
+			writew(I2C_STAT_RRDY, &i2c_base->stat);
 		}
-
-		if (!i2c_error) {
-			writew (I2C_CON_EN, &i2c_base->con);
-			while (readw (&i2c_base->stat) &
-				(I2C_STAT_RRDY | I2C_STAT_ARDY)) {
-				udelay (10000);
-				writew (0xFFFF, &i2c_base->stat);
-			}
+		if (status & I2C_STAT_ARDY) {
+			writew(I2C_STAT_ARDY, &i2c_base->stat);
+			break;
 		}
 	}
+
+read_exit:
 	flush_fifo();
 	writew (0xFFFF, &i2c_base->stat);
 	writew (0, &i2c_base->cnt);
-- 
1.7.0.4

[U-Boot] [PATCH v2 3/4] ARMV7: OMAP: I2C driver: Restructure i2c_write_byte function

[Steve Sakoman]

This patch removes the "magic number" delays and instead
monitors state changes in the status register bits.

Signed-off-by: Steve Sakoman <steve.sakoman@linaro.org>
Tested-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/omap24xx_i2c.c |   78 +++++++++++++++++++++++--------------------
 1 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index d176b5d..35201c3 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -218,7 +218,7 @@ read_exit:
 static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 {
 	int i2c_error = 0;
-	u16 status, stat;
+	u16 status;
 
 	/* wait until bus not busy */
 	wait_for_bb ();
@@ -231,49 +231,55 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
 		I2C_CON_STP, &i2c_base->con);
 
-	/* wait until state change */
-	status = wait_for_pin ();
-
-	if (status & I2C_STAT_XRDY) {
-#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-    defined(CONFIG_OMAP44XX)
-		/* send out 1 byte */
-		writeb (regoffset, &i2c_base->data);
-		writew (I2C_STAT_XRDY, &i2c_base->stat);
-
-		status = wait_for_pin ();
-		if ((status & I2C_STAT_XRDY)) {
-			/* send out next 1 byte */
-			writeb (value, &i2c_base->data);
-			writew (I2C_STAT_XRDY, &i2c_base->stat);
-		} else {
+	while (1) {
+		status = wait_for_pin();
+		if (status == 0 || status & I2C_STAT_NACK) {
 			i2c_error = 1;
+			goto write_exit;
 		}
+		if (status & I2C_STAT_XRDY) {
+#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
+    defined(CONFIG_OMAP44XX)
+			/* send register offset */
+			writeb(regoffset, &i2c_base->data);
+			writew(I2C_STAT_XRDY, &i2c_base->stat);
+
+			while (1) {
+				status = wait_for_pin();
+				if (status == 0 || status & I2C_STAT_NACK) {
+					i2c_error = 1;
+					goto write_exit;
+				}
+				if (status & I2C_STAT_XRDY) {
+					/* send data */
+					writeb(value, &i2c_base->data);
+					writew(I2C_STAT_XRDY, &i2c_base->stat);
+				}
+				if (status & I2C_STAT_ARDY) {
+					writew(I2C_STAT_ARDY, &i2c_base->stat);
+					break;
+				}
+			}
+			break;
 #else
-		/* send out two bytes */
-		writew ((value << 8) + regoffset, &i2c_base->data);
+			/* send out two bytes */
+			writew((value << 8) + regoffset, &i2c_base->data);
+			writew(I2C_STAT_XRDY, &i2c_base->stat);
 #endif
-		/* must have enough delay to allow BB bit to go low */
-		udelay (50000);
-		if (readw (&i2c_base->stat) & I2C_STAT_NACK) {
-			i2c_error = 1;
 		}
-	} else {
-		i2c_error = 1;
+		if (status & I2C_STAT_ARDY) {
+			writew(I2C_STAT_ARDY, &i2c_base->stat);
+			break;
+		}
 	}
 
-	if (!i2c_error) {
-		int eout = 200;
+	wait_for_bb();
 
-		writew (I2C_CON_EN, &i2c_base->con);
-		while ((stat = readw (&i2c_base->stat)) || (readw (&i2c_base->con) & I2C_CON_MST)) {
-			udelay (1000);
-			/* have to read to clear intrrupt */
-			writew (0xFFFF, &i2c_base->stat);
-			if(--eout == 0) /* better leave with error than hang */
-				break;
-		}
-	}
+	status = readw(&i2c_base->stat);
+	if (status & I2C_STAT_NACK)
+		i2c_error = 1;
+
+write_exit:
 	flush_fifo();
 	writew (0xFFFF, &i2c_base->stat);
 	writew (0, &i2c_base->cnt);
-- 
1.7.0.4

[U-Boot] [PATCH v2 4/4] ARMV7: OMAP: I2C driver: Restructure i2c_probe function

[Steve Sakoman]

This patch removes the "magic number" delays and instead
monitors state changes in the status register bits.

Signed-off-by: Steve Sakoman <steve.sakoman@linaro.org>
Tested-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/omap24xx_i2c.c |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index 35201c3..a72d1a1 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -310,6 +310,7 @@ static void flush_fifo(void)
 
 int i2c_probe (uchar chip)
 {
+	u16 status;
 	int res = 1; /* default = fail */
 
 	if (chip == readw (&i2c_base->oa)) {
@@ -325,19 +326,37 @@ int i2c_probe (uchar chip)
 	writew (chip, &i2c_base->sa);
 	/* stop bit needed here */
 	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c_base->con);
-	/* enough delay for the NACK bit set */
-	udelay (50000);
 
-	if (!(readw (&i2c_base->stat) & I2C_STAT_NACK)) {
-		res = 0;      /* success case */
-		flush_fifo();
-		writew(0xFFFF, &i2c_base->stat);
-	} else {
-		writew(0xFFFF, &i2c_base->stat);	 /* failue, clear sources*/
-		writew (readw (&i2c_base->con) | I2C_CON_STP, &i2c_base->con); /* finish up xfer */
-		udelay(20000);
-		wait_for_bb ();
+	while (1) {
+		status = wait_for_pin();
+		if (status == 0) {
+			res = 1;
+			goto probe_exit;
+		}
+		if (status & I2C_STAT_NACK) {
+			res = 1;
+			writew(0xff, &i2c_base->stat);
+			writew (readw (&i2c_base->con) | I2C_CON_STP, &i2c_base->con);
+			wait_for_bb ();
+			break;
+		}
+		if (status & I2C_STAT_ARDY) {
+			writew(I2C_STAT_ARDY, &i2c_base->stat);
+			break;
+		}
+		if (status & I2C_STAT_RRDY) {
+			res = 0;
+#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
+    defined(CONFIG_OMAP44XX)
+			readb(&i2c_base->data);
+#else
+			readw(&i2c_base->data);
+#endif
+			writew(I2C_STAT_RRDY, &i2c_base->stat);
+		}
 	}
+
+probe_exit:
 	flush_fifo();
 	writew (0, &i2c_base->cnt); /* don't allow any more data in...we don't want it.*/
 	writew(0xFFFF, &i2c_base->stat);
-- 
1.7.0.4

[U-Boot] [PATCH v2 0/4] ARMV7: OMAP: I2C driver: Restructure code to eliminate udelay calls and improve performance

[Heiko Schocher]

Hello Steve,

Steve Sakoman wrote:
> I've been preparing a patch series for Beagle and Overo that detects expansion
> board configuration information by reading a 128 byte I2C EEPROM on each
> expansion board.
> 
> Using the OMAP I2C driver in its current form takes about 5-6 seconds to read
> this small number of bytes!  Executing the i2c probe command takes close
> to 10 seconds.
> 
> Examining the code I see that there are a large number of fairly long udelay calls
> throughout the driver (10 - 50 milliseconds). I looked through the linux driver
> and did not see equivalent delays in that code.  In fact the longest delay in the
> linux code was one millisecond.
> 
> In looking at the TRM I2C section for OMAP3 and OMAP4 I don't see any requirement
> for delays in the programming model description.
> 
> This patch restructures the i2c driver to eliminate most of the udelay calls
> by monitoring state changes in the status register.
> 
> The EEPROM reads and the i2c probe execution are now instantaneous.
> 
> This patch series was tested on OMAP3 (Overo) and OMAP4 (Panda).  I do not
> have access to OMAP2 hardware for testing.
> 
> Steve Sakoman (4):
>   ARMV7: OMAP: I2C driver: Use same timeout value as linux kernel
>     driver
>   ARMV7: OMAP: I2C driver: Restructure i2c_read_byte function
>   ARMV7: OMAP: I2C driver: Restructure i2c_write_byte function
>   ARMV7: OMAP: I2C driver: Restructure i2c_probe function
> 
>  drivers/i2c/omap24xx_i2c.c |  204 ++++++++++++++++++++++++-------------------
>  1 files changed, 114 insertions(+), 90 deletions(-)

Applied v2 to u-boot-i2c.git master

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany