[U-Boot] [PATCH RFC 0/4] ARMV7: OMAP: I2C driver: Restructure code to eliminate udelay calls and improve performance
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(-)
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 --- 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; }
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
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> --- 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);
Hello Steve,
Steve Sakoman wrote:
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>
drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++---------------------- 1 files changed, 37 insertions(+), 39 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
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 --- drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++++-------------------- 1 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index d176b5d..828264e 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -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);
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++++-------------------- 1 files changed, 41 insertions(+), 35 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++++-------------------- 1 files changed, 41 insertions(+), 35 deletions(-)
After trying this for the omap3_beagle board, I get an compiler warning:
[hs@pollux u-boot]$ ./MAKEALL omap3_beagle Configuring for omap3_beagle board... omap24xx_i2c.c: In function 'i2c_write_byte': omap24xx_i2c.c:221: warning: unused variable 'stat' text data bss dec hex filename 218103 11412 202384 431899 6971b ./u-boot
--------------------- SUMMARY ---------------------------- Boards compiled: 1 Boards with warnings or errors: 1 ( omap3_beagle ) ---------------------------------------------------------- [hs@pollux u-boot]$
following patch fixes it.
BTW: Just for the record, your patchset works fine and faster on the beagle board, for example:
before your after your patchset patchset i2c probe 9s 0,4s i2c md 48 0 100 17s 1s
would you post a v2 of this patch, and I add my "Tested-by" to it, or is it OK, if I add my fix patch to u-boot-i2c master?
From 01c6c59014c4174ad4d13944d740d3491d9cf137 Mon Sep 17 00:00:00 2001
From: Heiko Schocher hs@denx.de Date: Wed, 20 Oct 2010 07:57:05 +0200 Subject: [PATCH] ARMV7: OMAP: I2C driver: fix compiler warning
Signed-off-by: Heiko Schocher hs@denx.de --- drivers/i2c/omap24xx_i2c.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index eb153fb..a72d1a1 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 ();
bye, Heiko
On Wed, 2010-10-20 at 08:08 +0200, Heiko Schocher wrote:
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++++-------------------- 1 files changed, 41 insertions(+), 35 deletions(-)
After trying this for the omap3_beagle board, I get an compiler warning:
[hs@pollux u-boot]$ ./MAKEALL omap3_beagle Configuring for omap3_beagle board... omap24xx_i2c.c: In function 'i2c_write_byte': omap24xx_i2c.c:221: warning: unused variable 'stat'
Hmm . . . I can swear I fixed that! Must be getting old :-)
text data bss dec hex filename 218103 11412 202384 431899 6971b ./u-boot
--------------------- SUMMARY ---------------------------- Boards compiled: 1 Boards with warnings or errors: 1 ( omap3_beagle )
[hs@pollux u-boot]$
following patch fixes it.
BTW: Just for the record, your patchset works fine and faster on the beagle board, for example:
before your after your patchset patchseti2c probe 9s 0,4s i2c md 48 0 100 17s 1s
I'm glad that you see the same speedups!
What tool do you use to measure the speedups?
would you post a v2 of this patch, and I add my "Tested-by" to it, or is it OK, if I add my fix patch to u-boot-i2c master?
I've posted v2 with your "Tested-by" and the warning fix.
If it helps, the patches are in my omap4-next-upstream branch:
http://www.sakoman.com/cgi-bin/gitweb.cgi?p=u-boot.git;a=shortlog;h=refs/hea...
Thanks for testing and for the comments!
Steve
From 01c6c59014c4174ad4d13944d740d3491d9cf137 Mon Sep 17 00:00:00 2001 From: Heiko Schocher hs@denx.de Date: Wed, 20 Oct 2010 07:57:05 +0200 Subject: [PATCH] ARMV7: OMAP: I2C driver: fix compiler warning
Signed-off-by: Heiko Schocher hs@denx.de
drivers/i2c/omap24xx_i2c.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index eb153fb..a72d1a1 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 ();
bye, Heiko
Hello Steve,
Steve Sakoman wrote:
On Wed, 2010-10-20 at 08:08 +0200, Heiko Schocher wrote:
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 76 +++++++++++++++++++++++-------------------- 1 files changed, 41 insertions(+), 35 deletions(-)
After trying this for the omap3_beagle board, I get an compiler warning:
[hs@pollux u-boot]$ ./MAKEALL omap3_beagle Configuring for omap3_beagle board... omap24xx_i2c.c: In function 'i2c_write_byte': omap24xx_i2c.c:221: warning: unused variable 'stat'
Hmm . . . I can swear I fixed that! Must be getting old :-)
;-)
text data bss dec hex filename 218103 11412 202384 431899 6971b ./u-boot
--------------------- SUMMARY ---------------------------- Boards compiled: 1 Boards with warnings or errors: 1 ( omap3_beagle )
[hs@pollux u-boot]$
following patch fixes it.
BTW: Just for the record, your patchset works fine and faster on the beagle board, for example:
before your after your patchset patchseti2c probe 9s 0,4s i2c md 48 0 100 17s 1s
I'm glad that you see the same speedups!
What tool do you use to measure the speedups?
You find it here: ftp://ftp.denx.de/pub/tools/time_log
You can start this script for example with:
kermit -c 2>&1 | ./time_log "start"
and then measure with for example:
echo start;i2c md 48 0 100;echo stop
and you get the time the command(s) between start and stop needed ...
would you post a v2 of this patch, and I add my "Tested-by" to it, or is it OK, if I add my fix patch to u-boot-i2c master?
I've posted v2 with your "Tested-by" and the warning fix.
Ok, thanks!
bye, Heiko
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 --- 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 828264e..eb153fb 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);
Hello Steve,
Steve Sakoman wrote:
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
drivers/i2c/omap24xx_i2c.c | 41 ++++++++++++++++++++++++++++++----------- 1 files changed, 30 insertions(+), 11 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
participants (2)
-
Heiko Schocher -
Steve Sakoman