i2c-s3c2410: Remove unconditional 1ms delay on each transfer
Subsystems (2)
PATCH v2 1/1 i2c-s3c2410: Remove unconditional 1ms delay on each transfer
The S3C I2C controller indicates completion of I2C transfers before the bus has a stop condition on it. In order to ensure that we do not attempt to start a new transfer before the bus is idle the driver currently inserts a 1ms delay. This is vastly larger than is generally required and has a visible effect on performance under load, such as when bringing up audio CODECs or reading back status information with non-bulk I2C reads.
Replace the sleep with a spin on the IIC status register for up to 1ms. This will busy wait but testing on my SMDK6410 system indicates that the overwhelming majority of transactions complete on the first spin, with maximum latencies of less than 10 spins so the absolute overhead of busy waiting should be at worst comprable to msleep(), and the overall system performance is dramatically improved.
The main risk is poor interaction with multimaster systems where we may miss the bus going idle before the next transaction. Defend against this by falling back to the original 1ms delay after 20 spins.
The overall effect in my testing is an approximately 20% improvement in kernel startup time.
--- drivers/i2c/busses/i2c-s3c2410.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-s3c2410.c +17 -2
@@ -482,7 +482,8 @@ 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 timeout; +unsigned long iicstat, timeout; +int spins =20 ;int ret;if (i2c->suspended)
@@ -521,7 +522,21 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,/* ensure the stop has been through the bus */ -msleep (1 ); +dev_dbg (i2c->dev,"waiting for bus idle\n" ); + +/* first, try busy waiting briefly */ +do { + 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" ); out:return ret;
Signature · 1 line
1.7.0.3
PATCH v2 1/1 i2c-s3c2410: Remove unconditional 1ms delay on each transfer
Quoted 7 lines · Mark Brown
The S3C I2C controller indicates completion of I2C transfers before the bus has a stop condition on it. In order to ensure that we do not attempt to start a new transfer before the bus is idle the driver currently inserts a 1ms delay. This is vastly larger than is generally required and has a visible effect on performance under load, such as when bringing up audio CODECs or reading back status information with non-bulk I2C reads.
ok, so far no problems with this on an s3c2440. I'll add it to the next tree.
Signature · 3 lines
Ben (ben at fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes'
PATCH v2 1/1 i2c-s3c2410: Remove unconditional 1ms delay on each transfer
In reply to Mark BrownQuoted 15 lines · Mark Brown
The S3C I2C controller indicates completion of I2C transfers before the bus has a stop condition on it. In order to ensure that we do not attempt to start a new transfer before the bus is idle the driver currently inserts a 1ms delay. This is vastly larger than is generally required and has a visible effect on performance under load, such as when bringing up audio CODECs or reading back status information with non-bulk I2C reads.
Replace the sleep with a spin on the IIC status register for up to 1ms. This will busy wait but testing on my SMDK6410 system indicates that the overwhelming majority of transactions complete on the first spin, with maximum latencies of less than 10 spins so the absolute overhead of busy waiting should be at worst comprable to
msleep(), and the overall system performance is dramatically improved.
Quoted 7 lines
The main risk is poor interaction with multimaster systems where we may miss the bus going idle before the next transaction. Defend against this by falling back to the original 1ms delay after 20 spins.
The overall effect in my testing is an approximately 20% improvement in kernel startup time.
I tested this patch on the s5pc110. This occurs msleep(1) still by time out at the my touchscreen i2c device on the s5pc110. It takes 80usec to clear S3C2410_IICSTAT_START bit in the worst case of my touchscreen and need about 400 spins. The 1msec delay is overhead to me still now.
PATCH v2 1/1 i2c-s3c2410: Remove unconditional 1ms delay on each transfer
Quoted 9 lines · Joonyoung Shim
Quoted 6 lines · Mark Brown
Replace the sleep with a spin on the IIC status register for up to 1ms. This will busy wait but testing on my SMDK6410 system indicates that the overwhelming majority of transactions complete on the first spin, with maximum latencies of less than 10 spins so the absolute overhead of busy waiting should be at worst comprable to
msleep(), and the overall system performance is dramatically improved.
Quoted 4 lines
I tested this patch on the s5pc110. This occurs msleep(1) still by time out at the my touchscreen i2c device on the s5pc110. It takes 80usec to clear
S3C2410_IICSTAT_STARTbit in the worst case of my touchscreen and need about 400 spins. The 1msec delay is overhead to me still now.
Yeah, I did expect we might run on a bit and get into the sleep which is why I left the fallback for it in there. I was trying to make as small a change as possible so that the change would be robust, the performance win is so dramatic. I guess tuning up the number of spins would work well in your case.
One idea that did occur to me while I was looking at this is to change the code so that if the delay is long instead of waiting for the bus to go idle at the end of the transaction we do so at the start of the next transaction. It's the next one that really cares that the bus has gone idle, and in a lot of cases there's enough gap between the transactions due to other work the CPU is doing to mean that there's no need to explicitly wait at all.