[PATCH 0/7 v6] fix gmbus writes and related issues

[PATCH 0/7 v6] fix gmbus writes and related issues

[Daniel Kurtz]

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

v6 is a rebased and trimmed version of the patchset,
since the first few patches have already been merged onto drm-intel-next-queued.
 * Cleans up INDEX cycle implementation
 * Only remove POSTING_READ() when it immediately precedes another I915_READ().

Daniel Kurtz (7):
  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

 drivers/gpu/drm/i915/intel_i2c.c |  159 ++++++++++++++++++++++++++++----------
 1 files changed, 117 insertions(+), 42 deletions(-)

-- 
1.7.7.3

[PATCH 1/7 v6] 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/7 v6] 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.

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/7 v6] 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/7 v6] 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/7 v6] 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/7 v6] 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/7 v6] 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

Re: [PATCH 1/7 v6] drm/i915/intel_i2c: handle zero-length writes

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:33 +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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/7 v6] drm/i915/intel_i2c: handle zero-length writes

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:33 +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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/7 v6] drm/i915/intel_i2c: use double-buffered writes

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:34 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

"For byte counts that are greater than four bytes, this register will be
written with subsequent data only after the HW_RDY status bit is set"

Hmm, I had interpretted that as should only be. But if you take into
account that the register is indeed double-buffered, it does make sense
that the hardware itself is only updated after the HW_RDY signal.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/7 v6] drm/i915/intel_i2c: use double-buffered writes

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:34 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

"For byte counts that are greater than four bytes, this register will be
written with subsequent data only after the HW_RDY status bit is set"

Hmm, I had interpretted that as should only be. But if you take into
account that the register is indeed double-buffered, it does make sense
that the hardware itself is only updated after the HW_RDY signal.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:36 +0800, Daniel Kurtz <djkurtz@chromium.org> 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.

The original docs have "this can only cause a STOP to be generated if a
GMBUS cycle is generated, the GMBUS is currently in a data phase, or it
is in a WAIT phase."

So from that it seems STOP | INDEX? | WAIT is always a valid
combination and is explicitly listed in the register set.

I defer to actual testing though ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:36 +0800, Daniel Kurtz <djkurtz@chromium.org> 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.

The original docs have "this can only cause a STOP to be generated if a
GMBUS cycle is generated, the GMBUS is currently in a data phase, or it
is in a WAIT phase."

So from that it seems STOP | INDEX? | WAIT is always a valid
combination and is explicitly listed in the register set.

I defer to actual testing though ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/7 v6] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:37 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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.

Hmm, I have

"gmbus1, bits 8-15: 8-bit GMBUS slave register
This field is redundant and should not be used."

Scary. :)

Otherwise, the code itself looks correct and quite neatly done now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/7 v6] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:37 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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.

Hmm, I have

"gmbus1, bits 8-15: 8-bit GMBUS slave register
This field is redundant and should not be used."

Scary. :)

Otherwise, the code itself looks correct and quite neatly done now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7 v6] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:38 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7 v6] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:38 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:39 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, 29 Mar 2012 02:26:39 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/7 v6] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Kurtz]

On Thu, Mar 29, 2012 at 2:52 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 29 Mar 2012 02:26:37 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> 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.
>
> Hmm, I have
>
> "gmbus1, bits 8-15: 8-bit GMBUS slave register
> This field is redundant and should not be used."
>
> Scary. :)

Perhaps INDEX ops are only available on some chipsets?
Is this something you can double check?

>
> Otherwise, the code itself looks correct and quite neatly done now.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Kurtz]

On Thu, Mar 29, 2012 at 2:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 29 Mar 2012 02:26:36 +0800, Daniel Kurtz <djkurtz@chromium.org> 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.
>
> The original docs have "this can only cause a STOP to be generated if a
> GMBUS cycle is generated, the GMBUS is currently in a data phase, or it
> is in a WAIT phase."
>
> So from that it seems STOP | INDEX? | WAIT is always a valid
> combination and is explicitly listed in the register set.
>
> I defer to actual testing though ;)

Unfortunately, STOP on first transaction didn't work for me :(.
Perhaps the docs are missing a "when":

"this can only cause a STOP to be generated if a GMBUS cycle is
generated *when* the GMBUS is currently in a data phase, or it is in a
WAIT phase."



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

Re: [PATCH 2/7 v6] drm/i915/intel_i2c: use double-buffered writes

[Daniel Kurtz]

On Thu, Mar 29, 2012 at 2:41 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 29 Mar 2012 02:26:34 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> 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.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>
> "For byte counts that are greater than four bytes, this register will be
> written with subsequent data only after the HW_RDY status bit is set"
>
> Hmm, I had interpretted that as should only be. But if you take into
> account that the register is indeed double-buffered, it does make sense
> that the hardware itself is only updated after the HW_RDY signal.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

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


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

Re: [PATCH 2/7 v6] drm/i915/intel_i2c: use double-buffered writes

[Daniel Vetter]

On Thu, Mar 29, 2012 at 04:46:39PM +0800, Daniel Kurtz wrote:
> On Thu, Mar 29, 2012 at 2:41 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, 29 Mar 2012 02:26:34 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> >> 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.
> >>
> >> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >
> > "For byte counts that are greater than four bytes, this register will be
> > written with subsequent data only after the HW_RDY status bit is set"
> >
> > Hmm, I had interpretted that as should only be. But if you take into
> > account that the register is indeed double-buffered, it does make sense
> > that the hardware itself is only updated after the HW_RDY signal.
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> In fact, during my experiments using the GMBUS interrupts, the HW_RDY
> interrupt would only trigger for transactions > 4 bytes after 2 writes
> to GMBUS3.

I think that's rather important information. Can you add this to your
commit message?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/7 v6] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Vetter]

On Thu, Mar 29, 2012 at 04:37:18PM +0800, Daniel Kurtz wrote:
> On Thu, Mar 29, 2012 at 2:52 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, 29 Mar 2012 02:26:37 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> >> 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.
> >
> > Hmm, I have
> >
> > "gmbus1, bits 8-15: 8-bit GMBUS slave register
> > This field is redundant and should not be used."
> >
> > Scary. :)
> 
> Perhaps INDEX ops are only available on some chipsets?
> Is this something you can double check?

I think index ops are available, the text sounds more like a hint to the
programmer that for VCH devices you should use word reg index access (i.e.
gmbus 5). We'll see when it blows up.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48