[PATCH 1/4] drm/i915: extract gmbus_wait_hw_status

[PATCH 1/4] drm/i915: extract gmbus_wait_hw_status

[Daniel Vetter]

The gmbus interrupt generation is rather fiddly: We can only ever
enable one interrupt source (but we always want to check for NAK
in addition to the real bit). And the bits in the gmbus status
register don't map at all to the bis in the irq register.

To prepare for this mess, start by extracting the hw status wait
loop into it's own function, consolidate the NAK error handling a
bit. To keep things flexible, pass in the status bit we care about
(in addition to any NAK signalling).

v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
Chris Wilson gladly pointed that out for me. To keep things simple,
ignore that case for  now (we only need to idle the gmbus controller
at the end of an entire i2c transaction, not after every message).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c | 46 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b9755f6..57decac 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
+gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
+		     u32 gmbus2_status)
+{
+	int ret;
+	int reg_offset = dev_priv->gpio_mmio_base;
+	u32 gmbus2;
+
+	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+		       (GMBUS_SATOER | gmbus2_status),
+		       50);
+
+	if (gmbus2 & GMBUS_SATOER)
+		return -ENXIO;
+
+	return ret;
+}
+
+static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		u32 gmbus1_index)
 {
@@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	while (len) {
 		int ret;
 		u32 val, loop = 0;
-		u32 gmbus2;
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_RDY),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
 		if (ret)
-			return -ETIMEDOUT;
-		if (gmbus2 & GMBUS_SATOER)
-			return -ENXIO;
+			return ret;
 
 		val = I915_READ(GMBUS3 + reg_offset);
 		do {
@@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
-		u32 gmbus2;
 
 		val = loop = 0;
 		do {
@@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_RDY),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
 		if (ret)
-			return -ETIMEDOUT;
-		if (gmbus2 & GMBUS_SATOER)
-			return -ENXIO;
+			return ret;
 	}
 	return 0;
 }
@@ -346,8 +354,6 @@ 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 */
@@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
+		if (ret == -ENXIO)
+			goto clear_err;
 		if (ret)
 			goto timeout;
-		if (gmbus2 & GMBUS_SATOER)
-			goto clear_err;
 	}
 
 	/* Generate a STOP condition on the bus. Note that gmbus can't generata
-- 
1.7.11.2

[PATCH 2/4] drm/i915: wire up gmbus irq handler

[Daniel Vetter]

Only enables the interrupt and puts a irq handler into place, doesn't
do anything yet.

Unfortunately there's no gmbus interrupt support for gen2/3 (safe for
pnv, but there the irq is marked as "Test mode").

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a61b41a..8415fa6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -596,6 +596,11 @@ out:
 	return ret;
 }
 
+static void gmbus_irq_handler(struct drm_device *dev)
+{
+	DRM_DEBUG_DRIVER("GMBUS interrupt\n");
+}
+
 static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -607,7 +612,7 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 				 SDE_AUDIO_POWER_SHIFT);
 
 	if (pch_iir & SDE_GMBUS)
-		DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n");
+		gmbus_irq_handler(dev);
 
 	if (pch_iir & SDE_AUDIO_HDCP_MASK)
 		DRM_DEBUG_DRIVER("PCH HDCP audio interrupt\n");
@@ -650,7 +655,7 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 		DRM_DEBUG_DRIVER("AUX channel interrupt\n");
 
 	if (pch_iir & SDE_GMBUS_CPT)
-		DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n");
+		gmbus_irq_handler(dev);
 
 	if (pch_iir & SDE_AUDIO_CP_REQ_CPT)
 		DRM_DEBUG_DRIVER("Audio CP request interrupt\n");
@@ -1841,12 +1846,14 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		hotplug_mask = (SDE_CRT_HOTPLUG_CPT |
 				SDE_PORTB_HOTPLUG_CPT |
 				SDE_PORTC_HOTPLUG_CPT |
-				SDE_PORTD_HOTPLUG_CPT);
+				SDE_PORTD_HOTPLUG_CPT |
+				SDE_GMBUS_CPT);
 	} else {
 		hotplug_mask = (SDE_CRT_HOTPLUG |
 				SDE_PORTB_HOTPLUG |
 				SDE_PORTC_HOTPLUG |
 				SDE_PORTD_HOTPLUG |
+				SDE_GMBUS |
 				SDE_AUX_MASK);
 	}
 
@@ -1906,7 +1913,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	hotplug_mask = (SDE_CRT_HOTPLUG_CPT |
 			SDE_PORTB_HOTPLUG_CPT |
 			SDE_PORTC_HOTPLUG_CPT |
-			SDE_PORTD_HOTPLUG_CPT);
+			SDE_PORTD_HOTPLUG_CPT |
+			SDE_GMBUS_CPT);
 	dev_priv->pch_irq_mask = ~hotplug_mask;
 
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
@@ -1959,6 +1967,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	POSTING_READ(VLV_IER);
 
 	i915_enable_pipestat(dev_priv, 0, pipestat_enable);
+	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS);
 	i915_enable_pipestat(dev_priv, 1, pipestat_enable);
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
@@ -2454,6 +2463,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->pipestat[0] = 0;
 	dev_priv->pipestat[1] = 0;
+	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS);
 
 	/*
 	 * Enable some error detection, note the instruction error mask
-- 
1.7.11.2

[PATCH 3/4] drm/i915: use the gmbus irq for waits

[Daniel Vetter]

We need two special things to properly wire this up:
- Add another argument to gmbus_wait_hw_status to pass in the
  correct interrupt bit in gmbus4.
- Since we can only get an irq for one of the two events we want,
  hand-roll the wait_event_timeout code so that we wake up every
  jiffie and can check for NAKs. This way we also subsume gmbus
  support for platforms without interrupts (or where those are not
  yet enabled).

The important bit really is to only enable one gmbus interrupt source
at the same time - with that piece of lore figured out, this seems to
work flawlessly.

Ben Widawsky rightfully complained the lack of measurements for the
claimed benefits (especially since the first version was actually
broken and fell back to bit-banging). Previously reading the 256 byte
hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms.
Given that transfering the 256 bytes over i2c at wire speed takes
20.5ms alone, the reduction in additional overhead is rather nice.

v2: Chris Wilson wondered whether GMBUS4 might contain some set bits
when booting up an hence result in some spurious interrupts. Since we
clear GMBUS4 after every wait and we do gmbus transfer really early in
the setup sequence to detect displays the window is small, but still
be paranoid and clear it properly.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c  |  4 ++++
 drivers/gpu/drm/i915/intel_i2c.c | 43 ++++++++++++++++++++++++++++++----------
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fce782..d93ce14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -418,6 +418,8 @@ typedef struct drm_i915_private {
 	 */
 	uint32_t gpio_mmio_base;
 
+	wait_queue_head_t gmbus_wait_queue;
+
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
 	uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8415fa6..dadc86b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -598,7 +598,11 @@ out:
 
 static void gmbus_irq_handler(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
 	DRM_DEBUG_DRIVER("GMBUS interrupt\n");
+
+	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
 static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 57decac..86f2b8c 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
+	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
@@ -205,20 +206,36 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
-		     u32 gmbus2_status)
+		     u32 gmbus2_status,
+		     u32 gmbus4_irq_en)
 {
-	int ret;
+	int i;
 	int reg_offset = dev_priv->gpio_mmio_base;
-	u32 gmbus2;
+	u32 gmbus2 = 0;
+	DEFINE_WAIT(wait);
+
+	/* Important: The hw handles only the first bit, so set only one! */
+	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-		       (GMBUS_SATOER | gmbus2_status),
-		       50);
+	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
+				TASK_UNINTERRUPTIBLE);
+
+		gmbus2 = I915_READ(GMBUS2 + reg_offset);
+		if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
+			break;
+
+		schedule_timeout(1);
+	}
+	finish_wait(&dev_priv->gmbus_wait_queue, &wait);
+
+	I915_WRITE(GMBUS4 + reg_offset, 0);
 
 	if (gmbus2 & GMBUS_SATOER)
 		return -ENXIO;
-
-	return ret;
+	if (gmbus2 & gmbus2_status)
+		return 0;
+	return -ETIMEDOUT;
 }
 
 static int
@@ -239,7 +256,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		int ret;
 		u32 val, loop = 0;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 
@@ -283,7 +301,8 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 	}
@@ -368,7 +387,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
+					   GMBUS_HW_WAIT_EN);
 		if (ret == -ENXIO)
 			goto clear_err;
 		if (ret)
@@ -474,6 +494,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 		dev_priv->gpio_mmio_base = 0;
 
 	mutex_init(&dev_priv->gmbus_mutex);
+	init_waitqueue_head(&dev_priv->gmbus_wait_queue);
 
 	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
-- 
1.7.11.2

[PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle

[Daniel Vetter]

GMBUS_ACTIVE has inverted sense and so doesn't fit into the
wait_hw_status helper, hence create a new gmbus_wait_idle functions.
Also, we only care about the idle irq event and nothing else, which
allows us to use the wait_event_timeout helper directly without
jumping through hoops to catch NAKs.

Since gen2/3 don't have gmbus interrupts, handle them separately with
the old wait_for macro.

This shaves another few ms off reading EDID from a hdmi screen on my
testbox here. EDID reading with interrupt driven gmbus is now as fast
as with busy-looping gmbus at 28 ms here (with negligible cpu
overhead).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 86f2b8c..faf85b3 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,6 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 	algo->data = bus;
 }
 
+#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4)
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 		     u32 gmbus2_status,
@@ -239,6 +240,31 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 }
 
 static int
+gmbus_wait_idle(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	int reg_offset = dev_priv->gpio_mmio_base;
+
+#define C ((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0)
+
+	if (!HAS_GMBUS_IRQ(dev_priv->dev))
+		return wait_for(C, 10);
+
+	/* Important: The hw handles only the first bit, so set only one! */
+	I915_WRITE(GMBUS4 + reg_offset, GMBUS_IDLE_EN);
+
+	ret = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
+
+	I915_WRITE(GMBUS4 + reg_offset, 0);
+
+	if (ret)
+		return 0;
+	else
+		return -ETIMEDOUT;
+#undef C
+}
+
+static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		u32 gmbus1_index)
 {
@@ -405,8 +431,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	 * 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 (gmbus_wait_idle(dev_priv)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
 		ret = -ETIMEDOUT;
@@ -430,8 +455,7 @@ clear_err:
 	 * it's slow responding and only answers on the 2nd retry.
 	 */
 	ret = -ENXIO;
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10)) {
+	if (gmbus_wait_idle(dev_priv)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
 		ret = -ETIMEDOUT;
-- 
1.7.11.2

Re: [PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle

[Chris Wilson]

On Thu,  6 Sep 2012 09:10:02 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> GMBUS_ACTIVE has inverted sense and so doesn't fit into the
> wait_hw_status helper, hence create a new gmbus_wait_idle functions.
> Also, we only care about the idle irq event and nothing else, which
> allows us to use the wait_event_timeout helper directly without
> jumping through hoops to catch NAKs.
> 
> Since gen2/3 don't have gmbus interrupts, handle them separately with
> the old wait_for macro.
> 
> This shaves another few ms off reading EDID from a hdmi screen on my
> testbox here. EDID reading with interrupt driven gmbus is now as fast
> as with busy-looping gmbus at 28 ms here (with negligible cpu
> overhead).

I'll put my neck on the line and say I can't spot any other mistakes:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

A couple of triffling things, you could use whitespace more uniformly
and the comment for only enabling one interrupt source could do with
the explanation that then means we also have to poll for NAK.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: use the gmbus irq for waits

[Daniel Vetter]

We need two special things to properly wire this up:
- Add another argument to gmbus_wait_hw_status to pass in the
  correct interrupt bit in gmbus4.
- Since we can only get an irq for one of the two events we want,
  hand-roll the wait_event_timeout code so that we wake up every
  jiffie and can check for NAKs. This way we also subsume gmbus
  support for platforms without interrupts (or where those are not
  yet enabled).

The important bit really is to only enable one gmbus interrupt source
at the same time - with that piece of lore figured out, this seems to
work flawlessly.

Ben Widawsky rightfully complained the lack of measurements for the
claimed benefits (especially since the first version was actually
broken and fell back to bit-banging). Previously reading the 256 byte
hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms.
Given that transfering the 256 bytes over i2c at wire speed takes
20.5ms alone, the reduction in additional overhead is rather nice.

v2: Chris Wilson wondered whether GMBUS4 might contain some set bits
when booting up an hence result in some spurious interrupts. Since we
clear GMBUS4 after every wait and we do gmbus transfer really early in
the setup sequence to detect displays the window is small, but still
be paranoid and clear it properly.

v3: Clarify the comment that gmbus irq generation can only support one
kind of event, why it bothers us and how we work around that limit.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

fixup
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c  |  4 ++++
 drivers/gpu/drm/i915/intel_i2c.c | 45 ++++++++++++++++++++++++++++++----------
 3 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5c8d5e3..72db70d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -418,6 +418,8 @@ typedef struct drm_i915_private {
 	 */
 	uint32_t gpio_mmio_base;
 
+	wait_queue_head_t gmbus_wait_queue;
+
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
 	uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8415fa6..dadc86b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -598,7 +598,11 @@ out:
 
 static void gmbus_irq_handler(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
 	DRM_DEBUG_DRIVER("GMBUS interrupt\n");
+
+	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
 static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 57decac..7413595 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
+	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
@@ -205,20 +206,38 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
-		     u32 gmbus2_status)
+		     u32 gmbus2_status,
+		     u32 gmbus4_irq_en)
 {
-	int ret;
+	int i;
 	int reg_offset = dev_priv->gpio_mmio_base;
-	u32 gmbus2;
+	u32 gmbus2 = 0;
+	DEFINE_WAIT(wait);
+
+	/* Important: The hw handles only the first bit, so set only one! Since
+	 * we also need to check for NAKs besides the hw ready/idle signal, we
+	 * need to wake up periodically and check that ourselves. */
+	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-		       (GMBUS_SATOER | gmbus2_status),
-		       50);
+	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
+				TASK_UNINTERRUPTIBLE);
+
+		gmbus2 = I915_READ(GMBUS2 + reg_offset);
+		if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
+			break;
+
+		schedule_timeout(1);
+	}
+	finish_wait(&dev_priv->gmbus_wait_queue, &wait);
+
+	I915_WRITE(GMBUS4 + reg_offset, 0);
 
 	if (gmbus2 & GMBUS_SATOER)
 		return -ENXIO;
-
-	return ret;
+	if (gmbus2 & gmbus2_status)
+		return 0;
+	return -ETIMEDOUT;
 }
 
 static int
@@ -239,7 +258,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		int ret;
 		u32 val, loop = 0;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 
@@ -283,7 +303,8 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 	}
@@ -368,7 +389,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
+					   GMBUS_HW_WAIT_EN);
 		if (ret == -ENXIO)
 			goto clear_err;
 		if (ret)
@@ -474,6 +496,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 		dev_priv->gpio_mmio_base = 0;
 
 	mutex_init(&dev_priv->gmbus_mutex);
+	init_waitqueue_head(&dev_priv->gmbus_wait_queue);
 
 	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
-- 
1.7.11.2

Re: [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status

[Ben Widawsky]

On 2012-09-06 00:09, Daniel Vetter wrote:
> The gmbus interrupt generation is rather fiddly: We can only ever
> enable one interrupt source (but we always want to check for NAK
> in addition to the real bit). And the bits in the gmbus status
> register don't map at all to the bis in the irq register.
>
> To prepare for this mess, start by extracting the hw status wait
> loop into it's own function, consolidate the NAK error handling a
> bit. To keep things flexible, pass in the status bit we care about
> (in addition to any NAK signalling).
>
> v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
> Chris Wilson gladly pointed that out for me. To keep things simple,
> ignore that case for  now (we only need to idle the gmbus controller
> at the end of an entire i2c transaction, not after every message).
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 46
> ++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index b9755f6..57decac 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 
> pin)
>  }
>
>  static int
> +gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> +		     u32 gmbus2_status)
> +{
> +	int ret;
> +	int reg_offset = dev_priv->gpio_mmio_base;
> +	u32 gmbus2;
> +
> +	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> +		       (GMBUS_SATOER | gmbus2_status),
> +		       50);
> +
> +	if (gmbus2 & GMBUS_SATOER)
> +		return -ENXIO;
> +
> +	return ret;
> +}
> +
> +static int
>  gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg 
> *msg,
>  		u32 gmbus1_index)
>  {

I know this isn't new to your patch, but the consolidation leaves a 
good opportunity for clarification of the timeout via a comment.

I started digging in to the math a bit here for the 50ms timeout and 
was left confused. I believe we're off by a factor of 10 here for a 
worst case which would be a 50KHz bus speed. Comments?

> @@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg,
>  	while (len) {
>  		int ret;
>  		u32 val, loop = 0;
> -		u32 gmbus2;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>
>  		val = I915_READ(GMBUS3 + reg_offset);
>  		do {
> @@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>  		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
>  	while (len) {
>  		int ret;
> -		u32 gmbus2;
>
>  		val = loop = 0;
>  		do {
> @@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>
>  		I915_WRITE(GMBUS3 + reg_offset, val);
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>  	}
>  	return 0;
>  }
> @@ -346,8 +354,6 @@ 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 */
> @@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  		if (ret == -ENXIO)
>  			goto clear_err;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
> +		if (ret == -ENXIO)
> +			goto clear_err;
>  		if (ret)
>  			goto timeout;
> -		if (gmbus2 & GMBUS_SATOER)
> -			goto clear_err;
>  	}
>
>  	/* Generate a STOP condition on the bus. Note that gmbus can't 
> generata

-- 
Ben Widawsky, Intel Open Source Technology Center