| Message ID | 1352981613-2098-4-git-send-email-ch.naveen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 15, 2012 at 05:43:32PM +0530, Naveen Krishna Chatradhi wrote: > + iicstat = readl(i2c->regs + S3C2410_IICSTAT); > + delay = 1; > + while ((iicstat & S3C2410_IICSTAT_START) && > + ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) { > + usleep_range(delay, 2 * delay); > + if (delay < S3C2410_IDLE_TIMEOUT / 10) > + delay <<= 1; > + now = ktime_get(); > + iicstat = readl(i2c->regs + S3C2410_IICSTAT); > + } > - /* first, try busy waiting briefly */ > - do { > - cpu_relax(); > - iicstat = readl(i2c->regs + S3C2410_IICSTAT); > - } while ((iicstat & S3C2410_IICSTAT_START) && --spins); On the hardware I was using when I wrote the original code here we were hitting 1-2 spins often enough to be interesting - starting off with a direct busy wait was definitely useful when doing large batches of I/O, especially compared to sleeps which might cause us to schedule. > - /* if that timed out sleep */ > - if (!spins) { > - msleep(1); > - iicstat = readl(i2c->regs + S3C2410_IICSTAT); > - } It seems like it'd be better to do the exponential backoff bit here instead of removing the busy wait completely.
Hi Mark, On Tue, Nov 20, 2012 at 12:49 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > > On Thu, Nov 15, 2012 at 05:43:32PM +0530, Naveen Krishna Chatradhi wrote: > > > + iicstat = readl(i2c->regs + S3C2410_IICSTAT); > > + delay = 1; > > + while ((iicstat & S3C2410_IICSTAT_START) && > > + ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) { > > + usleep_range(delay, 2 * delay); > > + if (delay < S3C2410_IDLE_TIMEOUT / 10) > > + delay <<= 1; > > + now = ktime_get(); > > + iicstat = readl(i2c->regs + S3C2410_IICSTAT); > > + } > > > - /* first, try busy waiting briefly */ > > - do { > > - cpu_relax(); > > - iicstat = readl(i2c->regs + S3C2410_IICSTAT); > > - } while ((iicstat & S3C2410_IICSTAT_START) && --spins); > > On the hardware I was using when I wrote the original code here we were > hitting 1-2 spins often enough to be interesting - starting off with a > direct busy wait was definitely useful when doing large batches of I/O, > especially compared to sleeps which might cause us to schedule. We check the status first to avoid any sleep()/schedule() in the case, that the CPU is slower than I2C transaction. Remember, this loop only happens after the event_wait loop has been woken up by the i2c irq. Since you are talking about hitting a tiny window of time at some arbitrary point after an irq, the CPU time to this point & I2C finishing would have to be very precisely aligned for the 1-2 loops (at CPU clock rate) to matter. HTH, -Dan > > > - /* if that timed out sleep */ > > - if (!spins) { > > - msleep(1); > > - iicstat = readl(i2c->regs + S3C2410_IICSTAT); > > - } > > It seems like it'd be better to do the exponential backoff bit here > instead of removing the busy wait completely.
On Tue, Nov 20, 2012 at 04:57:16PM +0800, Daniel Kurtz wrote: > On Tue, Nov 20, 2012 at 12:49 PM, Mark Brown > > On the hardware I was using when I wrote the original code here we were > > hitting 1-2 spins often enough to be interesting - starting off with a > > direct busy wait was definitely useful when doing large batches of I/O, > > especially compared to sleeps which might cause us to schedule. > We check the status first to avoid any sleep()/schedule() in the case, > that the CPU is slower than I2C transaction. Right, but this only works if we hit this on the very first spin. > Remember, this loop only happens after the event_wait loop has been > woken up by the i2c irq. Duh. > Since you are talking about hitting a tiny window of time at some > arbitrary point after an irq, the CPU time to this point & I2C > finishing would have to be very precisely aligned for the 1-2 loops > (at CPU clock rate) to matter. On some systems that can happen enormously reliably, finger in the air it's your fast case on the A15s you're playing with scaled down to a much slower CPU. The 20 spins I was setting the loop to was a massive overestimate for conservativism but more than 1 was common enough, IIRC spinning 5 times would have covered essentially everything.
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index fc4bf35..362a307 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -49,6 +49,9 @@ #define QUIRK_HDMIPHY (1 << 1) #define QUIRK_NO_GPIO (1 << 2) +/* Max time to wait for bus to become idle after a xfer (in us) */ +#define S3C2410_IDLE_TIMEOUT 5000 + /* i2c controller state */ enum s3c24xx_i2c_state { STATE_IDLE, @@ -556,6 +559,48 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c) return -ETIMEDOUT; } +/* s3c24xx_i2c_wait_idle + * + * wait for the i2c bus to become idle. +*/ + +static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c) +{ + unsigned long iicstat; + ktime_t start, now; + unsigned long delay; + + /* ensure the stop has been through the bus */ + + dev_dbg(i2c->dev, "waiting for bus idle\n"); + + start = now = ktime_get(); + + /* + * Most of the time, the bus is already idle within a few usec of the + * end of a transaction. However, really slow i2c devices can stretch + * the clock, delaying STOP generation. + * + * As a compromise between idle detection latency for the normal, fast + * case, and system load in the slow device case, use an exponential + * back off in the polling loop, up to 1/10th of the total timeout, + * then continue to poll at a constant rate up to the timeout. + */ + iicstat = readl(i2c->regs + S3C2410_IICSTAT); + delay = 1; + while ((iicstat & S3C2410_IICSTAT_START) && + ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) { + usleep_range(delay, 2 * delay); + if (delay < S3C2410_IDLE_TIMEOUT / 10) + delay <<= 1; + now = ktime_get(); + iicstat = readl(i2c->regs + S3C2410_IICSTAT); + } + + if (iicstat & S3C2410_IICSTAT_START) + dev_warn(i2c->dev, "timeout waiting for bus idle\n"); +} + /* s3c24xx_i2c_doxfer * * this starts an i2c transfer @@ -564,8 +609,7 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c) static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c, struct i2c_msg *msgs, int num) { - unsigned long iicstat, timeout; - int spins = 20; + unsigned long timeout; int ret; if (i2c->suspended) @@ -603,24 +647,7 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c, if (i2c->quirks & QUIRK_HDMIPHY) goto out; - /* ensure the stop has been through the bus */ - - dev_dbg(i2c->dev, "waiting for bus idle\n"); - - /* first, try busy waiting briefly */ - do { - cpu_relax(); - iicstat = readl(i2c->regs + S3C2410_IICSTAT); - } while ((iicstat & S3C2410_IICSTAT_START) && --spins); - - /* if that timed out sleep */ - if (!spins) { - msleep(1); - iicstat = readl(i2c->regs + S3C2410_IICSTAT); - } - - if (iicstat & S3C2410_IICSTAT_START) - dev_warn(i2c->dev, "timeout waiting for bus idle\n"); + s3c24xx_i2c_wait_idle(i2c); out: return ret;