[PATCH 0/4] i2c-s3c2410: Fix a bug and optimize code

[PATCH 0/4] i2c-s3c2410: Fix a bug and optimize code

[Naveen Krishna Chatradhi]

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

[PATCH 0/4] i2c-s3c2410: Fix a bug and optimize code

[Naveen Krishna Chatradhi]

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

[PATCH 1/4] i2c-s3c2410: grab adapter lock while changing i2c clock

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Olof Johansson <olofj@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 3e0335f..155cb04 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -59,7 +59,6 @@ enum s3c24xx_i2c_state {
 };
 
 struct s3c24xx_i2c {
-	spinlock_t		lock;
 	wait_queue_head_t	wait;
 	unsigned int            quirks;
 	unsigned int		suspended:1;
@@ -540,8 +539,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 		goto out;
 	}
 
-	spin_lock_irq(&i2c->lock);
-
 	i2c->msg     = msgs;
 	i2c->msg_num = num;
 	i2c->msg_ptr = 0;
@@ -550,7 +547,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 
 	s3c24xx_i2c_enable_irq(i2c);
 	s3c24xx_i2c_message_start(i2c, msgs);
-	spin_unlock_irq(&i2c->lock);
 
 	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
 
@@ -740,7 +736,6 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
 					  unsigned long val, void *data)
 {
 	struct s3c24xx_i2c *i2c = freq_to_i2c(nb);
-	unsigned long flags;
 	unsigned int got;
 	int delta_f;
 	int ret;
@@ -754,9 +749,9 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
 
 	if ((val == CPUFREQ_POSTCHANGE && delta_f < 0) ||
 	    (val == CPUFREQ_PRECHANGE && delta_f > 0)) {
-		spin_lock_irqsave(&i2c->lock, flags);
+		i2c_lock_adapter(&i2c->adap);
 		ret = s3c24xx_i2c_clockrate(i2c, &got);
-		spin_unlock_irqrestore(&i2c->lock, flags);
+		i2c_unlock_adapter(&i2c->adap);
 
 		if (ret < 0)
 			dev_err(i2c->dev, "cannot find frequency\n");
@@ -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 */
-- 
1.7.9.5

[PATCH 1/4] i2c-s3c2410: grab adapter lock while changing i2c clock

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Olof Johansson <olofj@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 3e0335f..155cb04 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -59,7 +59,6 @@ enum s3c24xx_i2c_state {
 };
 
 struct s3c24xx_i2c {
-	spinlock_t		lock;
 	wait_queue_head_t	wait;
 	unsigned int            quirks;
 	unsigned int		suspended:1;
@@ -540,8 +539,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 		goto out;
 	}
 
-	spin_lock_irq(&i2c->lock);
-
 	i2c->msg     = msgs;
 	i2c->msg_num = num;
 	i2c->msg_ptr = 0;
@@ -550,7 +547,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 
 	s3c24xx_i2c_enable_irq(i2c);
 	s3c24xx_i2c_message_start(i2c, msgs);
-	spin_unlock_irq(&i2c->lock);
 
 	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
 
@@ -740,7 +736,6 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
 					  unsigned long val, void *data)
 {
 	struct s3c24xx_i2c *i2c = freq_to_i2c(nb);
-	unsigned long flags;
 	unsigned int got;
 	int delta_f;
 	int ret;
@@ -754,9 +749,9 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
 
 	if ((val == CPUFREQ_POSTCHANGE && delta_f < 0) ||
 	    (val == CPUFREQ_PRECHANGE && delta_f > 0)) {
-		spin_lock_irqsave(&i2c->lock, flags);
+		i2c_lock_adapter(&i2c->adap);
 		ret = s3c24xx_i2c_clockrate(i2c, &got);
-		spin_unlock_irqrestore(&i2c->lock, flags);
+		i2c_unlock_adapter(&i2c->adap);
 
 		if (ret < 0)
 			dev_err(i2c->dev, "cannot find frequency\n");
@@ -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 */
-- 
1.7.9.5

[PATCH 2/4] i2c-s3c2410: do not generate STOP for QUIRK_HDMIPHY

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 155cb04..fc4bf35 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -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;
+	/*
+	 * 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.
+	 */
+	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,
 	else if (ret != num)
 		dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
 
+	/* For QUIRK_HDMIPHY, bus is already disabled */
+	if (i2c->quirks & QUIRK_HDMIPHY)
+		goto out;
+
 	/* ensure the stop has been through the bus */
 
 	dev_dbg(i2c->dev, "waiting for bus idle\n");
-- 
1.7.9.5

[PATCH 2/4] i2c-s3c2410: do not generate STOP for QUIRK_HDMIPHY

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 155cb04..fc4bf35 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -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;
+	/*
+	 * 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.
+	 */
+	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,
 	else if (ret != num)
 		dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
 
+	/* For QUIRK_HDMIPHY, bus is already disabled */
+	if (i2c->quirks & QUIRK_HDMIPHY)
+		goto out;
+
 	/* ensure the stop has been through the bus */
 
 	dev_dbg(i2c->dev, "waiting for bus idle\n");
-- 
1.7.9.5

[PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Olof Johansson <olofj@chromium.org>
Cc: Benson Leung <bleung@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   67 ++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 20 deletions(-)

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;
-- 
1.7.9.5

[PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Olof Johansson <olofj@chromium.org>
Cc: Benson Leung <bleung@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   67 ++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 20 deletions(-)

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;
-- 
1.7.9.5

[PATCH 4/4] i2c-s3c2410: do not special case HDMIPHY stuck bus detection

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

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 <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |   16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 362a307..c76ca7c 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -531,13 +531,6 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	int timeout = 400;
 
-	/* the timeout for HDMIPHY is reduced to 10 ms because
-	 * the hangup is expected to happen, so waiting 400 ms
-	 * causes only unnecessary system hangup
-	 */
-	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);
-
-		return 0;
-	}
-
 	return -ETIMEDOUT;
 }
 
-- 
1.7.9.5

[PATCH 4/4] i2c-s3c2410: do not special case HDMIPHY stuck bus detection

[Naveen Krishna Chatradhi]

From: Daniel Kurtz <djkurtz@chromium.org>

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 <djkurtz@chromium.org>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 362a307..c76ca7c 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -531,13 +531,6 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	int timeout = 400;
 
-	/* the timeout for HDMIPHY is reduced to 10 ms because
-	 * the hangup is expected to happen, so waiting 400 ms
-	 * causes only unnecessary system hangup
-	 */
-	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);
-
-		return 0;
-	}
-
 	return -ETIMEDOUT;
 }
 
-- 
1.7.9.5

Re: [PATCH 0/4] i2c-s3c2410: Fix a bug and optimize code

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

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

Applied this series to for-next, thanks!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 0/4] i2c-s3c2410: Fix a bug and optimize code

[Wolfram Sang]

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

Applied this series to for-next, thanks!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/98493682/attachment.sig>

Re: [PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Mark Brown]

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.

[PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Mark Brown]

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.

Re: [PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Daniel Kurtz]

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.

[PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Daniel Kurtz]

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.

Re: [PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/4] i2c-s3c2410: use exponential back off while polling for bus idle

[Mark Brown]

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.
-------------- 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>