The following set of patches fixes a bug in i2c-s3c2410 driver
with respect to the functioning of dedicated HDMIPHY channel.
1. Removing unwanted spinlock
2. Correcting the Stop sequence
3. Optimizing the wait loop for bus idle.
4. Removing unnecessary HDMI special cases
Respectively.
Daniel Kurtz (4):
i2c-s3c2410: grab adapter lock while changing i2c clock
i2c-s3c2410: do not generate STOP for QUIRK_HDMIPHY
i2c-s3c2410: use exponential back off while polling for bus idle
i2c-s3c2410: do not special case HDMIPHY stuck bus detection
drivers/i2c/busses/i2c-s3c2410.c | 134 ++++++++++++++++++++++++++------------
1 file changed, 91 insertions(+), 43 deletions(-)
--
1.7.9.5
From: Daniel Kurtz <redacted>
We probably don't want to change I2C frequency while a transfer is in
progress. The current implementation grabs a spinlock, but that only
protected the writes to IICCON when starting a message, it didn't protect
against clock changes in the middle of a transaction.
Note: The i2c-core already grabs the adapter lock before calling
s3c24xx_i2c_doxfer(), which ensures that only one caller is issuing a
xfer at a time. This means it is not necessary to disable interrupts
(spin_lock_irqsave) when changing frequencies, since there won't be
any i2c interrupts if there is no on-going xfer.
Lastly, i2c_lock_adapter() may cause the cpufreq_transition to sleep if
if a xfer is in progress, but this is ok since cpufreq notifiers are
called in a kernel thread, and there are already cases where it could
sleep, such as when using i2c to update the output of a voltage
regulator.
Note: the cpufreq part of this change has no functional affect on
exynos, where the i2c clock is independent of the cpufreq.
But, there is a slight perfomance boost since we no longer need to
lock/unlock an additional spinlock.
Signed-off-by: Daniel Kurtz <redacted>
Cc: Olof Johansson <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <redacted>
Signed-off-by: Naveen Krishna Chatradhi <redacted>
---
drivers/i2c/busses/i2c-s3c2410.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
@@ -962,7 +957,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)i2c->adap.class=I2C_CLASS_HWMON|I2C_CLASS_SPD;i2c->tx_setup=50;-spin_lock_init(&i2c->lock);init_waitqueue_head(&i2c->wait);/* find the clock and enable it */
From: Daniel Kurtz <redacted>
buses
The datasheet says that the STOP sequence should be:
1) I2CSTAT.5 = 0 - Clear BUSY (or 'generate STOP')
2) I2CCON.4 = 0 - Clear IRQPEND
3) Wait until the stop condition takes effect.
4*) I2CSTAT.4 = 0 - Clear TXRXEN
Where, step "4*" is only for buses with the "HDMIPHY" quirk.
However, after much experimentation, it appears that:
a) normal buses automatically clear BUSY and transition from
Master->Slave when they complete generating a STOP condition.
Therefore, step (3) can be done in doxfer() by polling I2CCON.4
after starting the STOP generation here.
b) HDMIPHY bus does neither, so there is no way to do step 3.
There is no indication when this bus has finished generating STOP.
In fact, we have found that as soon as the IRQPEND bit is cleared in
step 2, the HDMIPHY bus generates the STOP condition, and then immediately
starts transferring another data byte, even though the bus is supposedly
stopped. This is presumably because the bus is still in "Master" mode,
and its BUSY bit is still set.
To avoid these extra post-STOP transactions on HDMI phy devices, we just
disable Serial Output on the bus (I2CSTAT.4 = 0) directly, instead of
first generating a proper STOP condition. This should float SDA & SCK
terminating the transfer. Subsequent transfers start with a proper START
condition, and proceed normally.
The HDMIPHY bus is an internal bus that always has exactly two devices,
the host as Master and the HDMIPHY device as the slave. Skipping the STOP
condition has been tested on this bus and works.
Also, since we disable the bus directly from the isr, we can skip the bus
idle polling loop at the end of doxfer().
Signed-off-by: Daniel Kurtz <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <redacted>
Signed-off-by: Naveen Krishna Chatradhi <redacted>
---
drivers/i2c/busses/i2c-s3c2410.c | 47 ++++++++++++++++++++++++++++++++++++--
1 file changed, 45 insertions(+), 2 deletions(-)
@@ -234,8 +234,47 @@ static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c *i2c, int ret)dev_dbg(i2c->dev,"STOP\n");-/* stop the transfer */-iicstat&=~S3C2410_IICSTAT_START;+/*+*ThedatasheetsaysthattheSTOPsequenceshouldbe:+*1)I2CSTAT.5=0-ClearBUSY(or'generateSTOP')+*2)I2CCON.4=0-ClearIRQPEND+*3)Waituntilthestopconditiontakeseffect.+*4*)I2CSTAT.4=0-ClearTXRXEN+*+*Where,step"4*"isonlyforbuseswiththe"HDMIPHY"quirk.+*+*However,aftermuchexperimentation,itappearsthat:+*a)normalbusesautomaticallyclearBUSYandtransitionfrom+*Master->SlavewhentheycompletegeneratingaSTOPcondition.+*Therefore,step(3)canbedoneindoxfer()bypollingI2CCON.4+*afterstartingtheSTOPgenerationhere.+*b)HDMIPHYbusdoesneither,sothereisnowaytodostep3.+*Thereisnoindicationwhenthisbushasfinishedgenerating+*STOP.+*+*Infact,wehavefoundthatassoonastheIRQPENDbitisclearedin+*step2,theHDMIPHYbusgeneratestheSTOPcondition,andthen+*immediatelystartstransferringanotherdatabyte,eventhoughthe+*busissupposedlystopped.Thisispresumablybecausethebusis+*stillin"Master"mode,anditsBUSYbitisstillset.+*+*Toavoidtheseextrapost-STOPtransactionsonHDMIphydevices,we+*justdisableSerialOutputonthebus(I2CSTAT.4=0)directly,+*insteadoffirstgeneratingaproperSTOPcondition.Thisshould+*floatSDA&SCKterminatingthetransfer.Subsequenttransfers+*startwithaproperSTARTcondition,andproceednormally.+*+*TheHDMIPHYbusisaninternalbusthatalwayshasexactlytwo+*devices,thehostasMasterandtheHDMIPHYdeviceastheslave.+*SkippingtheSTOPconditionhasbeentestedonthisbusandworks.+*/+if(i2c->quirks&QUIRK_HDMIPHY){+/* Stop driving the I2C pins */+iicstat&=~S3C2410_IICSTAT_TXRXEN;+}else{+/* stop the transfer */+iicstat&=~S3C2410_IICSTAT_START;+}writel(iicstat,i2c->regs+S3C2410_IICSTAT);i2c->state=STATE_STOP;
@@ -560,6 +599,10 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,elseif(ret!=num)dev_dbg(i2c->dev,"incomplete xfer (%d)\n",ret);+/* For QUIRK_HDMIPHY, bus is already disabled */+if(i2c->quirks&QUIRK_HDMIPHY)+gotoout;+/* ensure the stop has been through the bus */dev_dbg(i2c->dev,"waiting for bus idle\n");
From: Daniel Kurtz <redacted>
Usually, the i2c controller has finished emitting the i2c STOP before the
driver reaches the bus idle polling loop. Optimize for this most common
case by reading IICSTAT first and potentially skipping the loop.
If the cpu is faster than the hardware, we wait for bus idle in a polling
loop. However, since the duration of one iteration of the loop is
dependent on cpu freq, and this i2c IP is used on many different systems,
use a time based loop timeout (5 ms).
We would like very low latencies to detect bus idle for the normal
'fast' case. However, if a device is slow to release the bus for some
reason, it could hold off the STOP generation for up to several
milliseconds. Rapidly polling for bus idle would seriously load the CPU
while waiting for it to release the bus. So, use a partial exponential
backoff as a compromise between idle detection latency and cpu load.
Signed-off-by: Daniel Kurtz <redacted>
Cc: Olof Johansson <redacted>
Cc: Benson Leung <bleung@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <redacted>
Signed-off-by: Naveen Krishna Chatradhi <redacted>
---
drivers/i2c/busses/i2c-s3c2410.c | 67 ++++++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 20 deletions(-)
@@ -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 */enums3c24xx_i2c_state{STATE_IDLE,
@@ -556,6 +559,48 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)return-ETIMEDOUT;}+/* s3c24xx_i2c_wait_idle+*+*waitforthei2cbustobecomeidle.+*/++staticvoids3c24xx_i2c_wait_idle(structs3c24xx_i2c*i2c)+{+unsignedlongiicstat;+ktime_tstart,now;+unsignedlongdelay;++/* ensure the stop has been through the bus */++dev_dbg(i2c->dev,"waiting for bus idle\n");++start=now=ktime_get();++/*+*Mostofthetime,thebusisalreadyidlewithinafewusecofthe+*endofatransaction.However,reallyslowi2cdevicescanstretch+*theclock,delayingSTOPgeneration.+*+*Asacompromisebetweenidledetectionlatencyforthenormal,fast+*case,andsystemloadintheslowdevicecase,useanexponential+*backoffinthepollingloop,upto1/10thofthetotaltimeout,+*thencontinuetopollataconstantrateuptothetimeout.+*/+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**thisstartsani2ctransfer
@@ -564,8 +609,7 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)staticints3c24xx_i2c_doxfer(structs3c24xx_i2c*i2c,structi2c_msg*msgs,intnum){-unsignedlongiicstat,timeout;-intspins=20;+unsignedlongtimeout;intret;if(i2c->suspended)
@@ -603,24 +647,7 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,if(i2c->quirks&QUIRK_HDMIPHY)gotoout;-/* 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:returnret;
From: Daniel Kurtz <redacted>
Commit "i2c-s3c2410: Add HDMIPHY quirk for S3C2440" added support for
HDMIPHY with some special handling in s3c24xx_i2c_set_master:
"due to unknown reason (probably HW bug in HDMIPHY and/or the controller)
a transfer fails to finish. The controller hangs after sending the last
byte, the workaround for this bug is resetting the controller after each
transfer"
The "unknown reason" was that the proper sequence for generating a STOP
condition wasn't being followed as per the datasheet. Since this is fixed
by "PATCH: i2c-s3c2410: do not generate STOP for QUIRK_HDMIPHY buses",
remove the special handling.
Signed-off-by: Daniel Kurtz <redacted>
Cc: Daniel Kurtz <redacted>
Signed-off-by: Naveen Krishna Chatradhi <redacted>
---
drivers/i2c/busses/i2c-s3c2410.c | 16 ----------------
1 file changed, 16 deletions(-)
@@ -531,13 +531,6 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)unsignedlongiicstat;inttimeout=400;-/* the timeout for HDMIPHY is reduced to 10 ms because-*thehangupisexpectedtohappen,sowaiting400ms-*causesonlyunnecessarysystemhangup-*/-if(i2c->quirks&QUIRK_HDMIPHY)-timeout=10;-while(timeout-->0){iicstat=readl(i2c->regs+S3C2410_IICSTAT);
@@ -547,15 +540,6 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)msleep(1);}-/* hang-up of bus dedicated for HDMIPHY occurred, resetting */-if(i2c->quirks&QUIRK_HDMIPHY){-writel(0,i2c->regs+S3C2410_IICCON);-writel(0,i2c->regs+S3C2410_IICSTAT);-writel(0,i2c->regs+S3C2410_IICDS);--return0;-}-return-ETIMEDOUT;}
From: Wolfram Sang <hidden> Date: 2012-11-16 12:05:35
On Thu, Nov 15, 2012 at 05:43:29PM +0530, Naveen Krishna Chatradhi wrote:
The following set of patches fixes a bug in i2c-s3c2410 driver
with respect to the functioning of dedicated HDMIPHY channel.
1. Removing unwanted spinlock
2. Correcting the Stop sequence
3. Optimizing the wait loop for bus idle.
4. Removing unnecessary HDMI special cases
Respectively.
Daniel Kurtz (4):
i2c-s3c2410: grab adapter lock while changing i2c clock
i2c-s3c2410: do not generate STOP for QUIRK_HDMIPHY
i2c-s3c2410: use exponential back off while polling for bus idle
i2c-s3c2410: do not special case HDMIPHY stuck bus detection
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.
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
quoted
- /* 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.
From: Mark Brown <hidden> Date: 2012-11-20 09:10:59
On Tue, Nov 20, 2012 at 04:57:16PM +0800, Daniel Kurtz wrote:
On Tue, Nov 20, 2012 at 12:49 PM, Mark Brown
quoted
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121120/8c42a44d/attachment-0001.sig>