[U-Boot] [PATCH v2 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 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; }
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);
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);
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);
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
participants (2)
-
Heiko Schocher -
Steve Sakoman