[PATCH 0/8 v7] fix gmbus writes and related issues

[PATCH 0/8 v7] fix gmbus writes and related issues

[Daniel Kurtz]

This patchset addresses a couple of issues with the i915 gmbus
implementation.

v7 adds a final patch to switch to using DRM_ERROR for reporting timeouts.

Daniel Kurtz (8):
  drm/i915/intel_i2c: handle zero-length writes
  drm/i915/intel_i2c: use double-buffered writes
  drm/i915/intel_i2c: always wait for IDLE before clearing NAK
  drm/i915/intel_i2c: use WAIT cycle, not STOP
  drm/i915/intel_i2c: use INDEX cycles for i2c read transactions
  drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop
  drm/i915/intel_i2c: remove POSTING_READ() from gmbus transfers
  drm/i915/intel_i2c: use DRM_ERROR on timeouts

 drivers/gpu/drm/i915/intel_i2c.c |  163 +++++++++++++++++++++++++++----------
 1 files changed, 119 insertions(+), 44 deletions(-)

-- 
1.7.7.3

[PATCH 1/8 v7] drm/i915/intel_i2c: handle zero-length writes

[Daniel Kurtz]

A common method of probing an i2c bus is trying to do a zero-length write.
Handle this case by checking the length first before decrementing it.

This is actually important, since attempting a zero-length write is one
of the ways that i2cdetect and i2c_new_probed_device detect whether
there is device present on the bus with a given address.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index c12db72..99a04f8 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -248,9 +248,10 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	u32 val, loop;
 
 	val = loop = 0;
-	do {
-		val |= *buf++ << (8 * loop);
-	} while (--len && ++loop < 4);
+	while (len && loop < 4) {
+		val |= *buf++ << (8 * loop++);
+		len -= 1;
+	}
 
 	I915_WRITE(GMBUS3 + reg_offset, val);
 	I915_WRITE(GMBUS1 + reg_offset,
-- 
1.7.7.3

[PATCH 2/8 v7] drm/i915/intel_i2c: use double-buffered writes

[Daniel Kurtz]

The GMBUS controller GMBUS3 register is double-buffered.  Take advantage
of this  by writing two 4-byte words before the first wait for HW_RDY.
This helps keep the GMBUS controller from becoming idle during long writes.

In fact, during experiments using the GMBUS interrupts, the HW_RDY
interrupt would only trigger for transactions >4 bytes after 2 writes
to GMBUS3.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 99a04f8..f02e52a 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -262,13 +262,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	POSTING_READ(GMBUS2 + reg_offset);
 	while (len) {
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_RDY),
-			     50))
-			return -ETIMEDOUT;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-			return -ENXIO;
-
 		val = loop = 0;
 		do {
 			val |= *buf++ << (8 * loop);
@@ -276,6 +269,13 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 		POSTING_READ(GMBUS2 + reg_offset);
+
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
+			     (GMBUS_SATOER | GMBUS_HW_RDY),
+			     50))
+			return -ETIMEDOUT;
+		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+			return -ENXIO;
 	}
 	return 0;
 }
-- 
1.7.7.3

[PATCH 3/8 v7] drm/i915/intel_i2c: always wait for IDLE before clearing NAK

[Daniel Kurtz]

The GMBUS controller can report a NAK condition while a transaction is
still active. If the driver is fast enough, and the bus is slow enough,
the driver may clear the NAK condition while the controller is still
busy, resulting in a confused GMBUS controller.  This will leave the
controller in a bad state such that the next transaction may fail.

Also, return -ENXIO if a device NAKs a transaction.

Note: this patch also refactors gmbus_xfer to remove the "done" label.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   41 ++++++++++++++++++++++++++++---------
 1 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index f02e52a..9707868 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -324,26 +324,47 @@ gmbus_xfer(struct i2c_adapter *adapter,
 			goto clear_err;
 	}
 
-	goto done;
+	/* Mark the GMBUS interface as disabled after waiting for idle.
+	 * We will re-enable it at the start of the next xfer,
+	 * till then let it sleep.
+	 */
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
+			 adapter->name);
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+	ret = i;
+	goto out;
 
 clear_err:
+	/*
+	 * Wait for bus to IDLE before clearing NAK.
+	 * If we clear the NAK while bus is still active, then it will stay
+	 * active and the next transaction may fail.
+	 */
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
+		     10))
+		DRM_INFO("GMBUS [%s] timed out after NAK\n", adapter->name);
+
 	/* Toggle the Software Clear Interrupt bit. This has the effect
 	 * of resetting the GMBUS controller and so clearing the
 	 * BUS_ERROR raised by the slave's NAK.
 	 */
 	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
 	I915_WRITE(GMBUS1 + reg_offset, 0);
+	I915_WRITE(GMBUS0 + reg_offset, 0);
 
-done:
-	/* Mark the GMBUS interface as disabled after waiting for idle.
-	 * We will re-enable it at the start of the next xfer,
-	 * till then let it sleep.
+	DRM_DEBUG_DRIVER("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
+			 adapter->name, msgs[i].addr,
+			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
+
+	/*
+	 * If no ACK is received during the address phase of a transaction,
+	 * the adapter must report -ENXIO.
+	 * It is not clear what to return if no ACK is received at other times.
+	 * So, we always return -ENXIO in all NAK cases, to ensure we send
+	 * it at least during the one case that is specified.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
-		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
-			 bus->adapter.name);
-	I915_WRITE(GMBUS0 + reg_offset, 0);
-	ret = i;
+	ret = -ENXIO;
 	goto out;
 
 timeout:
-- 
1.7.7.3

[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
transaction) during a DATA or WAIT phase.  In other words, the
controller rejects a STOP requested as part of the first transaction in a
sequence.

Thus, for the first transaction we must always use a WAIT cycle, detect
when the device has finished (and is in a WAIT phase), and then either
start the next transaction, or, if there are no more transactions,
generate a STOP cycle.

Note: Theoretically, the last transaction of a multi-transaction sequence
could initiate a STOP cycle.  However, this slight optimization is left
for another patch.  We return -ETIMEDOUT if the hardware doesn't
deactivate after the STOP cycle.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 9707868..25f2c9e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		bool last)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
@@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 
 	I915_WRITE(GMBUS1 + reg_offset,
 		   GMBUS_CYCLE_WAIT |
-		   (last ? GMBUS_CYCLE_STOP : 0) |
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 }
 
 static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		bool last)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
@@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	I915_WRITE(GMBUS3 + reg_offset, val);
 	I915_WRITE(GMBUS1 + reg_offset,
 		   GMBUS_CYCLE_WAIT |
-		   (last ? GMBUS_CYCLE_STOP : 0) |
 		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					       struct intel_gmbus,
 					       adapter);
 	struct drm_i915_private *dev_priv = bus->dev_priv;
-	int i, reg_offset, ret;
+	int i, reg_offset;
+	int ret = 0;
 
 	mutex_lock(&dev_priv->gmbus_mutex);
 
@@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
-		bool last = i + 1 == num;
-
 		if (msgs[i].flags & I2C_M_RD)
-			ret = gmbus_xfer_read(dev_priv, &msgs[i], last);
+			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
 		else
-			ret = gmbus_xfer_write(dev_priv, &msgs[i], last);
+			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
 
 		if (ret == -ETIMEDOUT)
 			goto timeout;
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		if (!last &&
-		    wait_for(I915_READ(GMBUS2 + reg_offset) &
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
 			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
 			     50))
 			goto timeout;
@@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
 			goto clear_err;
 	}
 
+	/* Generate a STOP condition on the bus */
+	I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+
 	/* Mark the GMBUS interface as disabled after waiting for idle.
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
+		     10)) {
 		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
+		ret = -ETIMEDOUT;
+	}
 	I915_WRITE(GMBUS0 + reg_offset, 0);
-	ret = i;
+	ret = ret ?: i;
 	goto out;
 
 clear_err:
-- 
1.7.7.3

[PATCH 5/8 v7] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Kurtz]

It is very common for an i2c device to require a small 1 or 2 byte write
followed by a read.  For example, when reading from an i2c EEPROM it is
common to write and address, offset or index followed by a reading some
values.

The i915 gmbus controller provides a special "INDEX" cycle for performing
such a small write followed by a read.  The INDEX can be either one or two
bytes long.  The advantage of using such a cycle is that the CPU has
slightly less work to do once the read with INDEX cycle is started.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   54 +++++++++++++++++++++++++++++++++++---
 1 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 25f2c9e..9c07ff2 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,13 +204,15 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		u32 gmbus1_index)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
 	u8 *buf = msg->buf;
 
 	I915_WRITE(GMBUS1 + reg_offset,
+		   gmbus1_index |
 		   GMBUS_CYCLE_WAIT |
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
@@ -276,6 +278,46 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 	return 0;
 }
 
+/*
+ * The gmbus controller can combine a 1 or 2 byte write with a read that
+ * immediately follows it by using an "INDEX" cycle.
+ */
+static bool
+gmbus_is_index_read(struct i2c_msg *msgs, int i, int num)
+{
+	return (i + 1 < num &&
+		!(msgs[i].flags & I2C_M_RD) && msgs[i].len <= 2 &&
+		(msgs[i + 1].flags & I2C_M_RD));
+}
+
+static int
+gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
+{
+	int reg_offset = dev_priv->gpio_mmio_base;
+	u32 gmbus1_index = 0;
+	u32 gmbus5 = 0;
+	int ret;
+
+	if (msgs[0].len == 2)
+		gmbus5 = GMBUS_2BYTE_INDEX_EN |
+			 msgs[0].buf[1] | (msgs[0].buf[0] << 8);
+	if (msgs[0].len == 1)
+		gmbus1_index = GMBUS_CYCLE_INDEX |
+			       (msgs[0].buf[0] << GMBUS_SLAVE_INDEX_SHIFT);
+
+	/* GMBUS5 holds 16-bit index */
+	if (gmbus5)
+		I915_WRITE(GMBUS5 + reg_offset, gmbus5);
+
+	ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
+
+	/* Clear GMBUS5 after each index transfer */
+	if (gmbus5)
+		I915_WRITE(GMBUS5 + reg_offset, 0);
+
+	return ret;
+}
+
 static int
 gmbus_xfer(struct i2c_adapter *adapter,
 	   struct i2c_msg *msgs,
@@ -300,10 +342,14 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
-		if (msgs[i].flags & I2C_M_RD)
-			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
-		else
+		if (gmbus_is_index_read(msgs, i, num)) {
+			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
+			i += 1;  /* set i to the index of the read xfer */
+		} else if (msgs[i].flags & I2C_M_RD) {
+			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
+		} else {
 			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
+		}
 
 		if (ret == -ETIMEDOUT)
 			goto timeout;
-- 
1.7.7.3

[PATCH 6/8 v7] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Daniel Kurtz]

Save the GMBUS2 value read while polling for state changes, and then
reuse this value when determining for which reason the loops were exited.
This is a small optimization which saves a couple of bus accesses for
memory mapped IO registers.

To avoid "assigning in if clause" checkpatch errors", use a ret variable
to store the wait_for macro return value.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 9c07ff2..60f90cb 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -219,13 +219,16 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
 	POSTING_READ(GMBUS2 + reg_offset);
 	do {
+		int ret;
 		u32 val, loop = 0;
+		u32 gmbus2;
 
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_RDY),
-			     50))
+		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+			       (GMBUS_SATOER | GMBUS_HW_RDY),
+			       50);
+		if (ret)
 			return -ETIMEDOUT;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (gmbus2 & GMBUS_SATOER)
 			return -ENXIO;
 
 		val = I915_READ(GMBUS3 + reg_offset);
@@ -260,6 +263,9 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	POSTING_READ(GMBUS2 + reg_offset);
 	while (len) {
+		int ret;
+		u32 gmbus2;
+
 		val = loop = 0;
 		do {
 			val |= *buf++ << (8 * loop);
@@ -268,11 +274,12 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		I915_WRITE(GMBUS3 + reg_offset, val);
 		POSTING_READ(GMBUS2 + reg_offset);
 
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_RDY),
-			     50))
+		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+			       (GMBUS_SATOER | GMBUS_HW_RDY),
+			       50);
+		if (ret)
 			return -ETIMEDOUT;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (gmbus2 & GMBUS_SATOER)
 			return -ENXIO;
 	}
 	return 0;
@@ -342,6 +349,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
+		u32 gmbus2;
+
 		if (gmbus_is_index_read(msgs, i, num)) {
 			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
 			i += 1;  /* set i to the index of the read xfer */
@@ -356,11 +365,12 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
-			     50))
+		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
+			       50);
+		if (ret)
 			goto timeout;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (gmbus2 & GMBUS_SATOER)
 			goto clear_err;
 	}
 
-- 
1.7.7.3

[PATCH 7/8 v7] drm/i915/intel_i2c: remove POSTING_READ() from gmbus transfers

[Daniel Kurtz]

The POSTING_READ() calls were originally added to make sure the writes
were flushed before any timing delays and across loops.
Now that the code has settled a bit, let's remove them.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 60f90cb..73431ed 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -217,7 +217,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
-	POSTING_READ(GMBUS2 + reg_offset);
 	do {
 		int ret;
 		u32 val, loop = 0;
@@ -261,7 +260,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
-	POSTING_READ(GMBUS2 + reg_offset);
 	while (len) {
 		int ret;
 		u32 gmbus2;
@@ -272,7 +270,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		} while (--len && ++loop < 4);
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
-		POSTING_READ(GMBUS2 + reg_offset);
 
 		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
 			       (GMBUS_SATOER | GMBUS_HW_RDY),
-- 
1.7.7.3

[PATCH 8/8 v7] drm/i915/intel_i2c: use DRM_ERROR on timeouts

[Daniel Kurtz]

GMBUS should hopefully actually work now in most cases.
So, report any timeout conditions using DRM_ERROR(), especially for the
case where the timeout causes fallback to bit-bang mode.

>From observations, the GMBUS transfers timeout and switch to Bit-Banging
when the corresponding pins are not actually hooked up to anything.  In
these cases, there is no NAK, nor timeout, nor any other indication from
the GMBUS controller that a transaction fails.  The first gmbus
transaction timeout is caught by the "wait_for" timeout, causing the
transition to bit-banging mode for subsequent (failed) attempts.

This might be what was meant by the following note in the GPIO_CTL
description:
  "The registers that control digital display (HDMI/DVI, DisplayPort)
pins should only be utilized if the Port Detected bit in the related
control register is set to 1."
(http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf,
section 2.2.2)

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 73431ed..154fedd 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -380,8 +380,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	 */
 	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
 		     10)) {
-		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
-			 adapter->name);
+		DRM_ERROR("GMBUS [%s] timed out waiting for idle\n",
+			  adapter->name);
 		ret = -ETIMEDOUT;
 	}
 	I915_WRITE(GMBUS0 + reg_offset, 0);
@@ -396,7 +396,7 @@ clear_err:
 	 */
 	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
 		     10))
-		DRM_INFO("GMBUS [%s] timed out after NAK\n", adapter->name);
+		DRM_ERROR("GMBUS [%s] timed out after NAK\n", adapter->name);
 
 	/* Toggle the Software Clear Interrupt bit. This has the effect
 	 * of resetting the GMBUS controller and so clearing the
@@ -421,8 +421,8 @@ clear_err:
 	goto out;
 
 timeout:
-	DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
-		 bus->adapter.name, bus->reg0 & 0xff);
+	DRM_ERROR("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
+		  bus->adapter.name, bus->reg0 & 0xff);
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
 	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
-- 
1.7.7.3

Re: [PATCH 0/8 v7] fix gmbus writes and related issues

[Chris Wilson]

On Fri, 30 Mar 2012 19:46:35 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> This patchset addresses a couple of issues with the i915 gmbus
> implementation.
> 
> v7 adds a final patch to switch to using DRM_ERROR for reporting timeouts.
> 
> Daniel Kurtz (8):
>   drm/i915/intel_i2c: handle zero-length writes
>   drm/i915/intel_i2c: use double-buffered writes
>   drm/i915/intel_i2c: always wait for IDLE before clearing NAK
>   drm/i915/intel_i2c: use WAIT cycle, not STOP
>   drm/i915/intel_i2c: use INDEX cycles for i2c read transactions
>   drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop
>   drm/i915/intel_i2c: remove POSTING_READ() from gmbus transfers
>   drm/i915/intel_i2c: use DRM_ERROR on timeouts

The only two I am still dubious about is 4/8: use WAIT cycle, not STOP,
and 8/8: use DRM_ERROR on timeouts, the rest are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

The last is a little debatable, as i2c can be called from userspace (and
other modules) and we have not verified that the adapters we set up
correspond to devices conditions. So I think it is still possible under
normal conditions to hit the error path, so would prefer to keep the log
level as INFO.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/8 v7] fix gmbus writes and related issues

[Daniel Vetter]

On Fri, Mar 30, 2012 at 01:49:17PM +0100, Chris Wilson wrote:
> On Fri, 30 Mar 2012 19:46:35 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > This patchset addresses a couple of issues with the i915 gmbus
> > implementation.
> > 
> > v7 adds a final patch to switch to using DRM_ERROR for reporting timeouts.
> > 
> > Daniel Kurtz (8):
> >   drm/i915/intel_i2c: handle zero-length writes
> >   drm/i915/intel_i2c: use double-buffered writes
> >   drm/i915/intel_i2c: always wait for IDLE before clearing NAK
> >   drm/i915/intel_i2c: use WAIT cycle, not STOP
> >   drm/i915/intel_i2c: use INDEX cycles for i2c read transactions
> >   drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop
> >   drm/i915/intel_i2c: remove POSTING_READ() from gmbus transfers
> >   drm/i915/intel_i2c: use DRM_ERROR on timeouts
> 
> The only two I am still dubious about is 4/8: use WAIT cycle, not STOP,
> and 8/8: use DRM_ERROR on timeouts, the rest are
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> The last is a little debatable, as i2c can be called from userspace (and
> other modules) and we have not verified that the adapters we set up
> correspond to devices conditions. So I think it is still possible under
> normal conditions to hit the error path, so would prefer to keep the log
> level as INFO.

I have to admit that I've suggested this patch. We might need to drop it
before release, but I also want to have a few pointless bug about void
error messages and I'd like to figure out whether gmbus now really works.
Easiest way for that is to throw an annoying error message into dmesg when
it doesn't.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/8 v7] drm/i915/intel_i2c: handle zero-length writes

[Chris Wilson]

On Fri, 30 Mar 2012 19:46:36 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> A common method of probing an i2c bus is trying to do a zero-length write.
> Handle this case by checking the length first before decrementing it.
> 
> This is actually important, since attempting a zero-length write is one
> of the ways that i2cdetect and i2c_new_probed_device detect whether
> there is device present on the bus with a given address.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Having done a good job with handling zero length writes, can I tempt to
submit a similar fix for zero length reads? ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Vetter]

On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
> The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
> transaction) during a DATA or WAIT phase.  In other words, the
> controller rejects a STOP requested as part of the first transaction in a
> sequence.
> 
> Thus, for the first transaction we must always use a WAIT cycle, detect
> when the device has finished (and is in a WAIT phase), and then either
> start the next transaction, or, if there are no more transactions,
> generate a STOP cycle.
> 
> Note: Theoretically, the last transaction of a multi-transaction sequence
> could initiate a STOP cycle.  However, this slight optimization is left
> for another patch.  We return -ETIMEDOUT if the hardware doesn't
> deactivate after the STOP cycle.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I've re-read gmbus register spec and STOP seems to be allowed even in the
first cycle. Does this patch solve an issue for you? If not, I prefer we
just drop it.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   30 +++++++++++++++---------------
>  1 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 9707868..25f2c9e 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>  }
>  
>  static int
> -gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
> -		bool last)
> +gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
>  {
>  	int reg_offset = dev_priv->gpio_mmio_base;
>  	u16 len = msg->len;
> @@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  
>  	I915_WRITE(GMBUS1 + reg_offset,
>  		   GMBUS_CYCLE_WAIT |
> -		   (last ? GMBUS_CYCLE_STOP : 0) |
>  		   (len << GMBUS_BYTE_COUNT_SHIFT) |
>  		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> @@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  }
>  
>  static int
> -gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
> -		bool last)
> +gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
>  {
>  	int reg_offset = dev_priv->gpio_mmio_base;
>  	u16 len = msg->len;
> @@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	I915_WRITE(GMBUS3 + reg_offset, val);
>  	I915_WRITE(GMBUS1 + reg_offset,
>  		   GMBUS_CYCLE_WAIT |
> -		   (last ? GMBUS_CYCLE_STOP : 0) |
>  		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
>  		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> @@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  					       struct intel_gmbus,
>  					       adapter);
>  	struct drm_i915_private *dev_priv = bus->dev_priv;
> -	int i, reg_offset, ret;
> +	int i, reg_offset;
> +	int ret = 0;
>  
>  	mutex_lock(&dev_priv->gmbus_mutex);
>  
> @@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>  
>  	for (i = 0; i < num; i++) {
> -		bool last = i + 1 == num;
> -
>  		if (msgs[i].flags & I2C_M_RD)
> -			ret = gmbus_xfer_read(dev_priv, &msgs[i], last);
> +			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
>  		else
> -			ret = gmbus_xfer_write(dev_priv, &msgs[i], last);
> +			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
>  
>  		if (ret == -ETIMEDOUT)
>  			goto timeout;
>  		if (ret == -ENXIO)
>  			goto clear_err;
>  
> -		if (!last &&
> -		    wait_for(I915_READ(GMBUS2 + reg_offset) &
> +		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
>  			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
>  			     50))
>  			goto timeout;
> @@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  			goto clear_err;
>  	}
>  
> +	/* Generate a STOP condition on the bus */
> +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
> +
>  	/* Mark the GMBUS interface as disabled after waiting for idle.
>  	 * We will re-enable it at the start of the next xfer,
>  	 * till then let it sleep.
>  	 */
> -	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
> +	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
> +		     10)) {
>  		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
>  			 adapter->name);
> +		ret = -ETIMEDOUT;
> +	}
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
> -	ret = i;
> +	ret = ret ?: i;
>  	goto out;
>  
>  clear_err:
> -- 
> 1.7.7.3
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Vetter]

On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
> > transaction) during a DATA or WAIT phase.  In other words, the
> > controller rejects a STOP requested as part of the first transaction in a
> > sequence.
> > 
> > Thus, for the first transaction we must always use a WAIT cycle, detect
> > when the device has finished (and is in a WAIT phase), and then either
> > start the next transaction, or, if there are no more transactions,
> > generate a STOP cycle.
> > 
> > Note: Theoretically, the last transaction of a multi-transaction sequence
> > could initiate a STOP cycle.  However, this slight optimization is left
> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
> > deactivate after the STOP cycle.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> I've re-read gmbus register spec and STOP seems to be allowed even in the
> first cycle. Does this patch solve an issue for you? If not, I prefer we
> just drop it.

Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should
keeep that hunk. I've picked up the previous 3 patches of this series, the
once after this one here conflict (without this patch here).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
>> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
>> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
>> > transaction) during a DATA or WAIT phase.  In other words, the
>> > controller rejects a STOP requested as part of the first transaction in a
>> > sequence.
>> >
>> > Thus, for the first transaction we must always use a WAIT cycle, detect
>> > when the device has finished (and is in a WAIT phase), and then either
>> > start the next transaction, or, if there are no more transactions,
>> > generate a STOP cycle.
>> >
>> > Note: Theoretically, the last transaction of a multi-transaction sequence
>> > could initiate a STOP cycle.  However, this slight optimization is left
>> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
>> > deactivate after the STOP cycle.
>> >
>> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>
>> I've re-read gmbus register spec and STOP seems to be allowed even in the
>> first cycle. Does this patch solve an issue for you? If not, I prefer we
>> just drop it.

STOP does not work in the first cycle, hence the patch.

-Daniel

>
> Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should
> keeep that hunk. I've picked up the previous 3 patches of this series, the
> once after this one here conflict (without this patch here).
> -Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Vetter]

On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote:
> On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
> >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
> >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
> >> > transaction) during a DATA or WAIT phase.  In other words, the
> >> > controller rejects a STOP requested as part of the first transaction in a
> >> > sequence.
> >> >
> >> > Thus, for the first transaction we must always use a WAIT cycle, detect
> >> > when the device has finished (and is in a WAIT phase), and then either
> >> > start the next transaction, or, if there are no more transactions,
> >> > generate a STOP cycle.
> >> >
> >> > Note: Theoretically, the last transaction of a multi-transaction sequence
> >> > could initiate a STOP cycle.  However, this slight optimization is left
> >> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
> >> > deactivate after the STOP cycle.
> >> >
> >> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >>
> >> I've re-read gmbus register spec and STOP seems to be allowed even in the
> >> first cycle. Does this patch solve an issue for you? If not, I prefer we
> >> just drop it.
> 
> STOP does not work in the first cycle, hence the patch.

Ok, I've picked this patch up and extended the comment a bit to that
effect. Just to avoid anyone else trying to 'fix' things because bspec
sounds like it should work.

I've also picked up the other patches safe for the last one, thanks a lot
for digging through the gmbus code and fixing it all up.

Now can I volunteer you for a (hopefully) last set of gmbus patches?
Afaics there are a few small things left to fix:
- zero-length reads can blow up the kernel, like zero-length writes could.
  See: https://bugs.freedesktop.org/show_bug.cgi?id=48269
- Chris Wilson suggested on irc that we should wait for HW_READY even for
  zero-length writes (and also reads), currently we don't.
- atm the debug output is too noisy. I think we can leave the fallback to
  gpio bitbanging at info (or maybe error) level, but all the other
  messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
  when userspace tries to probe the i2c with nothing connected or if the
  driver code tries to do the same. See:
  https://bugs.freedesktop.org/show_bug.cgi?id=48248

Chris, anything you want to add to the wishlist?

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Chris Wilson]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]

On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote:
> > On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
> > >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
> > >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
> > >> > transaction) during a DATA or WAIT phase.  In other words, the
> > >> > controller rejects a STOP requested as part of the first transaction in a
> > >> > sequence.
> > >> >
> > >> > Thus, for the first transaction we must always use a WAIT cycle, detect
> > >> > when the device has finished (and is in a WAIT phase), and then either
> > >> > start the next transaction, or, if there are no more transactions,
> > >> > generate a STOP cycle.
> > >> >
> > >> > Note: Theoretically, the last transaction of a multi-transaction sequence
> > >> > could initiate a STOP cycle.  However, this slight optimization is left
> > >> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
> > >> > deactivate after the STOP cycle.
> > >> >
> > >> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > >>
> > >> I've re-read gmbus register spec and STOP seems to be allowed even in the
> > >> first cycle. Does this patch solve an issue for you? If not, I prefer we
> > >> just drop it.
> > 
> > STOP does not work in the first cycle, hence the patch.
> 
> Ok, I've picked this patch up and extended the comment a bit to that
> effect. Just to avoid anyone else trying to 'fix' things because bspec
> sounds like it should work.
> 
> I've also picked up the other patches safe for the last one, thanks a lot
> for digging through the gmbus code and fixing it all up.
> 
> Now can I volunteer you for a (hopefully) last set of gmbus patches?
> Afaics there are a few small things left to fix:
> - zero-length reads can blow up the kernel, like zero-length writes could.
>   See: https://bugs.freedesktop.org/show_bug.cgi?id=48269
> - Chris Wilson suggested on irc that we should wait for HW_READY even for
>   zero-length writes (and also reads), currently we don't.
> - atm the debug output is too noisy. I think we can leave the fallback to
>   gpio bitbanging at info (or maybe error) level, but all the other
>   messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
>   when userspace tries to probe the i2c with nothing connected or if the
>   driver code tries to do the same. See:
>   https://bugs.freedesktop.org/show_bug.cgi?id=48248
> 
> Chris, anything you want to add to the wishlist?

The last major item on the wishlist is solving how to drive the SDVO i2c
over gmbus. I think it is just a matter of massaging in the channel
switch as msg[0].
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote:
>> On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
>> >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
>> >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
>> >> > transaction) during a DATA or WAIT phase.  In other words, the
>> >> > controller rejects a STOP requested as part of the first transaction in a
>> >> > sequence.
>> >> >
>> >> > Thus, for the first transaction we must always use a WAIT cycle, detect
>> >> > when the device has finished (and is in a WAIT phase), and then either
>> >> > start the next transaction, or, if there are no more transactions,
>> >> > generate a STOP cycle.
>> >> >
>> >> > Note: Theoretically, the last transaction of a multi-transaction sequence
>> >> > could initiate a STOP cycle.  However, this slight optimization is left
>> >> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
>> >> > deactivate after the STOP cycle.
>> >> >
>> >> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> >>
>> >> I've re-read gmbus register spec and STOP seems to be allowed even in the
>> >> first cycle. Does this patch solve an issue for you? If not, I prefer we
>> >> just drop it.
>>
>> STOP does not work in the first cycle, hence the patch.
>
> Ok, I've picked this patch up and extended the comment a bit to that
> effect. Just to avoid anyone else trying to 'fix' things because bspec
> sounds like it should work.
>
> I've also picked up the other patches safe for the last one, thanks a lot
> for digging through the gmbus code and fixing it all up.
>
> Now can I volunteer you for a (hopefully) last set of gmbus patches?
> Afaics there are a few small things left to fix:
> - zero-length reads can blow up the kernel, like zero-length writes could.
>  See: https://bugs.freedesktop.org/show_bug.cgi?id=48269

Got it.  Will Fix.

> - Chris Wilson suggested on irc that we should wait for HW_READY even for
>  zero-length writes (and also reads), currently we don't.

I don't think so.  We just need to wait for (GMBUS_SATOER |
GMBUS_HW_WAIT_PHASE).
Why would we wait for HW_READY, too?

> - atm the debug output is too noisy. I think we can leave the fallback to
>  gpio bitbanging at info (or maybe error) level, but all the other
>  messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
>  when userspace tries to probe the i2c with nothing connected or if the
>  driver code tries to do the same. See:
>  https://bugs.freedesktop.org/show_bug.cgi?id=48248

OK...  we can change the logging level.
However, the log in the bug to which you link seems to indicate a more
serious issue in this case.  It says to me that something on his
system is trying to talk to the disabled dpc i2c port 5 times every 10
seconds.  Each time it fails due with a time out, and each timeout
takes 50ms.  I would argue that the INFO message here is pointing out
that the hotplug code might want to check the corresponding
PORT_ENABLED bit before attempting a read over a particular DP/HDMI
gmbus port.  Perhaps I am mistaken, but if there was really nothing on
the bus, shouldn't that be a NAK, not a timeout?

>
> Chris, anything you want to add to the wishlist?
>
> Thanks, Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote:
>> > On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
>> > >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
>> > >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
>> > >> > transaction) during a DATA or WAIT phase.  In other words, the
>> > >> > controller rejects a STOP requested as part of the first transaction in a
>> > >> > sequence.
>> > >> >
>> > >> > Thus, for the first transaction we must always use a WAIT cycle, detect
>> > >> > when the device has finished (and is in a WAIT phase), and then either
>> > >> > start the next transaction, or, if there are no more transactions,
>> > >> > generate a STOP cycle.
>> > >> >
>> > >> > Note: Theoretically, the last transaction of a multi-transaction sequence
>> > >> > could initiate a STOP cycle.  However, this slight optimization is left
>> > >> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
>> > >> > deactivate after the STOP cycle.
>> > >> >
>> > >> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> > >>
>> > >> I've re-read gmbus register spec and STOP seems to be allowed even in the
>> > >> first cycle. Does this patch solve an issue for you? If not, I prefer we
>> > >> just drop it.
>> >
>> > STOP does not work in the first cycle, hence the patch.
>>
>> Ok, I've picked this patch up and extended the comment a bit to that
>> effect. Just to avoid anyone else trying to 'fix' things because bspec
>> sounds like it should work.
>>
>> I've also picked up the other patches safe for the last one, thanks a lot
>> for digging through the gmbus code and fixing it all up.
>>
>> Now can I volunteer you for a (hopefully) last set of gmbus patches?
>> Afaics there are a few small things left to fix:
>> - zero-length reads can blow up the kernel, like zero-length writes could.
>>   See: https://bugs.freedesktop.org/show_bug.cgi?id=48269
>> - Chris Wilson suggested on irc that we should wait for HW_READY even for
>>   zero-length writes (and also reads), currently we don't.
>> - atm the debug output is too noisy. I think we can leave the fallback to
>>   gpio bitbanging at info (or maybe error) level, but all the other
>>   messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
>>   when userspace tries to probe the i2c with nothing connected or if the
>>   driver code tries to do the same. See:
>>   https://bugs.freedesktop.org/show_bug.cgi?id=48248
>>
>> Chris, anything you want to add to the wishlist?
>
> The last major item on the wishlist is solving how to drive the SDVO i2c
> over gmbus. I think it is just a matter of massaging in the channel
> switch as msg[0].

I noticed that random other i2c adapter, but haven't looked at it to
know why it is so different than the others.  What is it?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Vetter]

On Thu, Apr 12, 2012 at 02:16:45AM +0800, Daniel Kurtz wrote:
> On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > - atm the debug output is too noisy. I think we can leave the fallback to
> >  gpio bitbanging at info (or maybe error) level, but all the other
> >  messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
> >  when userspace tries to probe the i2c with nothing connected or if the
> >  driver code tries to do the same. See:
> >  https://bugs.freedesktop.org/show_bug.cgi?id=48248
> 
> OK...  we can change the logging level.
> However, the log in the bug to which you link seems to indicate a more
> serious issue in this case.  It says to me that something on his
> system is trying to talk to the disabled dpc i2c port 5 times every 10
> seconds.  Each time it fails due with a time out, and each timeout
> takes 50ms.  I would argue that the INFO message here is pointing out
> that the hotplug code might want to check the corresponding
> PORT_ENABLED bit before attempting a read over a particular DP/HDMI
> gmbus port.  Perhaps I am mistaken, but if there was really nothing on
> the bus, shouldn't that be a NAK, not a timeout?

The issue is that there's no hotplug, so we run a polling loop which
checks every 10s whether anything is connected. Part of that is trying to
read an edid. I dunno exactly why we don't get a NAK but a timeout.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Chris Wilson]

On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The last major item on the wishlist is solving how to drive the SDVO i2c
> > over gmbus. I think it is just a matter of massaging in the channel
> > switch as msg[0].
> 
> I noticed that random other i2c adapter, but haven't looked at it to
> know why it is so different than the others.  What is it?

Every command sequence over SDVO is preceded by an instruction to the
controller as to which function to send the command to. Originally I
tried to reuse the GPIO method of simply sending that switch command
immediately followed by the real message, but I never succeeded in
setting up the SDVO devices in that manner. I suspect what is actually
required is to rebuild the message to include the preceeding bus switch.

However, if you don't have any SDVO devices to experiment with we can
find somebody who does.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Chris Wilson]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Thu, 12 Apr 2012 02:16:45 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > - Chris Wilson suggested on irc that we should wait for HW_READY even for
> >  zero-length writes (and also reads), currently we don't.
> 
> I don't think so.  We just need to wait for (GMBUS_SATOER |
> GMBUS_HW_WAIT_PHASE).
> Why would we wait for HW_READY, too?

Just paranoia when looking at the read/write sequences and wondering how
safe they were with 0-length read/writes. No real reason to suspect that
the code is incorrect in any way.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

On Thu, Apr 12, 2012 at 4:26 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > The last major item on the wishlist is solving how to drive the SDVO i2c
>> > over gmbus. I think it is just a matter of massaging in the channel
>> > switch as msg[0].
>>
>> I noticed that random other i2c adapter, but haven't looked at it to
>> know why it is so different than the others.  What is it?
>
> Every command sequence over SDVO is preceded by an instruction to the
> controller as to which function to send the command to. Originally I
> tried to reuse the GPIO method of simply sending that switch command
> immediately followed by the real message, but I never succeeded in
> setting up the SDVO devices in that manner. I suspect what is actually
> required is to rebuild the message to include the preceeding bus switch.
>
> However, if you don't have any SDVO devices to experiment with we can
> find somebody who does.

Sorry, I don't have any SDVO hardware to play with.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre