[PATCH] drm/i915: Reset GMBUS controller after NAK

[PATCH] drm/i915: Reset GMBUS controller after NAK

[Chris Wilson]

Once a NAK has been asserted by the slave, we need to reset the GMBUS
controller in order to continue. This is done by asserting the Software
Clear Interrupt bit and then clearing it again to restore operations.

If we don't clear the NAK, then all future GMBUS xfers will fail,
including DDC probes and EDID retrieval.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 82d04c5..abdedd8 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -259,7 +259,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
 				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					return 0;
+					goto clear_err;
 
 				val = I915_READ(GMBUS3 + reg_offset);
 				do {
@@ -287,7 +287,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
 				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					return 0;
+					goto clear_err;
 
 				val = loop = 0;
 				do {
@@ -302,14 +302,25 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
 			goto timeout;
 		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-			return 0;
+			goto clear_err;
 	}
 
-	return num;
+	goto done;
+
+clear_err:
+	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
+	POSTING_READ(GMBUS1 + reg_offset);
+	I915_WRITE(GMBUS1 + reg_offset, 0);
+
+done:
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+	return i;
 
 timeout:
 	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
 		 bus->reg0 & 0xff, bus->adapter.name);
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+
 	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
 	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
 	if (!bus->force_bit)
-- 
1.7.4.1

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 452 bytes --]

On Wed, 30 Mar 2011 17:07:11 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +clear_err:
> +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> +	POSTING_READ(GMBUS1 + reg_offset);
> +	I915_WRITE(GMBUS1 + reg_offset, 0);

Any posting read needed here?

> +
> +done:
> +	I915_WRITE(GMBUS0 + reg_offset, 0);

What's this new write doing in the non-error path? Do we need a posting
read after it?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Chris Wilson]

On Wed, 30 Mar 2011 09:48:25 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 30 Mar 2011 17:07:11 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > +clear_err:
> > +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> > +	POSTING_READ(GMBUS1 + reg_offset);
> > +	I915_WRITE(GMBUS1 + reg_offset, 0);
> 
> Any posting read needed here?

I'm not even sure we need the first posting read. Maybe it should be a
  wait_for(I915_READ(GMBUS1 + reg_offset) & GMBUS_SW_CLR_INT, 100)
to be clearer that we are simply giving the hardware the chance to assert
the bit and reset before re-enabling.
 
> > +
> > +done:
> > +	I915_WRITE(GMBUS0 + reg_offset, 0);
> 
> What's this new write doing in the non-error path? Do we need a posting
> read after it?

No, GMBUS0 is not read until the very first phase of the data cycle. And
the very first thing we do in the next xfer is a write to GMBUS0 of the
port settings. I just thought that explicitly marking the GMBUS controller
as disabled when not in use by us would lead to less confusion in future.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1001 bytes --]

On Wed, 30 Mar 2011 17:59:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I'm not even sure we need the first posting read. Maybe it should be a
>   wait_for(I915_READ(GMBUS1 + reg_offset) & GMBUS_SW_CLR_INT, 100)
> to be clearer that we are simply giving the hardware the chance to assert
> the bit and reset before re-enabling.

Doesn't look like the hardware actually delays in setting this bit, so
your original sequence should be correct. Of course, a comment
indicating that you're toggling the SW_CLR_INT bit to reset the bus
might be nice.

> No, GMBUS0 is not read until the very first phase of the data cycle. And
> the very first thing we do in the next xfer is a write to GMBUS0 of the
> port settings. I just thought that explicitly marking the GMBUS controller
> as disabled when not in use by us would lead to less confusion in
> future.

Sounds reasonable. I'd love to have seen a comment in the code and in
the patch...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Reset GMBUS controller after NAK

[Chris Wilson]

Once a NAK has been asserted by the slave, we need to reset the GMBUS
controller in order to continue. This is done by asserting the Software
Clear Interrupt bit and then clearing it again to restore operations.

If we don't clear the NAK, then all future GMBUS xfers will fail,
including DDC probes and EDID retrieval.

v2: Add some comments as suggested by Keith Packard.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 82d04c5..d3b903b 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -259,7 +259,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
 				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					return 0;
+					goto clear_err;
 
 				val = I915_READ(GMBUS3 + reg_offset);
 				do {
@@ -287,7 +287,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
 				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					return 0;
+					goto clear_err;
 
 				val = loop = 0;
 				do {
@@ -302,14 +302,31 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
 			goto timeout;
 		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-			return 0;
+			goto clear_err;
 	}
 
-	return num;
+	goto done;
+
+clear_err:
+	/* 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);
+
+done:
+	/* Mark the GMBUS interface as disabled. We will re-enable it at the
+	 * start of the next xfer, till then let it sleep.
+	 */
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+	return i;
 
 timeout:
 	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
 		 bus->reg0 & 0xff, bus->adapter.name);
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+
 	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
 	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
 	if (!bus->force_bit)
-- 
1.7.4.1

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 690 bytes --]

On Wed, 30 Mar 2011 18:42:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Once a NAK has been asserted by the slave, we need to reset the GMBUS
> controller in order to continue. This is done by asserting the Software
> Clear Interrupt bit and then clearing it again to restore operations.
> 
> If we don't clear the NAK, then all future GMBUS xfers will fail,
> including DDC probes and EDID retrieval.
> 
> v2: Add some comments as suggested by Keith Packard.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Jesse Barnes]

On Wed, 30 Mar 2011 17:07:11 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Once a NAK has been asserted by the slave, we need to reset the GMBUS
> controller in order to continue. This is done by asserting the Software
> Clear Interrupt bit and then clearing it again to restore operations.
> 
> If we don't clear the NAK, then all future GMBUS xfers will fail,
> including DDC probes and EDID retrieval.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

This one fixes the issue I was seeing on my HP test machine; LVDS DDC
probing seems to work ok once this fix is applied.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Steven Newbury]

----- Original message -----
> On Wed, 30 Mar 2011 17:07:11 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Once a NAK has been asserted by the slave, we need to reset the GMBUS
> > controller in order to continue. This is done by asserting the Software
> > Clear Interrupt bit and then clearing it again to restore operations.
> > 
> > If we don't clear the NAK, then all future GMBUS xfers will fail,
> > including DDC probes and EDID retrieval.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> 
> This one fixes the issue I was seeing on my HP test machine; LVDS DDC
> probing seems to work ok once this fix is applied.
> 
> -- 
Could this be related to my inability to successfully probe the ch7036 with the ChromeOS Chrontel driver? Is it worth me testing it again with this patch applied? (I'm currently waiting to hear back from Zotac, my board supplier)

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Chris Wilson]

On Thu, 31 Mar 2011 16:16:27 +0100, Steven Newbury <steve@snewbury.org.uk> wrote:
> ----- Original message -----
> > On Wed, 30 Mar 2011 17:07:11 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > Once a NAK has been asserted by the slave, we need to reset the GMBUS
> > > controller in order to continue. This is done by asserting the Software
> > > Clear Interrupt bit and then clearing it again to restore operations.
> > > 
> > > If we don't clear the NAK, then all future GMBUS xfers will fail,
> > > including DDC probes and EDID retrieval.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > 
> > This one fixes the issue I was seeing on my HP test machine; LVDS DDC
> > probing seems to work ok once this fix is applied.
> > 
> > -- 
> Could this be related to my inability to successfully probe the ch7036 with the ChromeOS Chrontel driver? Is it worth me testing it again with this patch applied? (I'm currently waiting to hear back from Zotac, my board supplier)

Hmm. Depends, but unlikely. I would have thought the nm10_gpio driver
you were using used it's only bitbanging on the GPIO lines rather than
GMBUS. If in doubt, disable the use of GMBUS by 

diff --git a/drivers/gpu/drm/i915/intel_i2c.c
b/drivers/gpu/drm/i915/intel_i2c.c
index d3b903b..ef9f664 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -401,7 +401,7 @@ int intel_setup_gmbus(struct drm_device *dev)
                bus->reg0 = i | GMBUS_RATE_100KHZ;
 
                /* XXX force bit banging until GMBUS is fully debugged */
-               if (IS_GEN2(dev))
+               if (1)
                        bus->force_bit = intel_gpio_create(dev_priv, i);
        }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Steven Newbury]

----- Original message -----
> On Thu, 31 Mar 2011 16:16:27 +0100, Steven Newbury
> <steve@snewbury.org.uk> wrote:
> > ----- Original message -----
> > > On Wed, 30 Mar 2011 17:07:11 +0100
> > > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > 
> > > > Once a NAK has been asserted by the slave, we need to reset the
> > > > GMBUS controller in order to continue. This is done by asserting
> > > > the Software Clear Interrupt bit and then clearing it again to
> > > > restore operations.
> > > > 
> > > > If we don't clear the NAK, then all future GMBUS xfers will fail,
> > > > including DDC probes and EDID retrieval.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > 
> > > This one fixes the issue I was seeing on my HP test machine; LVDS DDC
> > > probing seems to work ok once this fix is applied.
> > > 
> > > -- 
> > Could this be related to my inability to successfully probe the ch7036
> > with the ChromeOS Chrontel driver? Is it worth me testing it again
> > with this patch applied? (I'm currently waiting to hear back from
> > Zotac, my board supplier)
> 
> Hmm. Depends, but unlikely. I would have thought the nm10_gpio driver
> you were using used it's only bitbanging on the GPIO lines rather than
> GMBUS. If in doubt, disable the use of GMBUS by 
> 
The GPIO is only used for HDMI hotplug detection, chip communication goes via i2c, or at least would if I could get it working... :-(

Re: [PATCH] drm/i915: Reset GMBUS controller after NAK

[Steven Newbury]

----- Original message -----
> ----- Original message -----
> > On Thu, 31 Mar 2011 16:16:27 +0100, Steven Newbury
> > <steve@snewbury.org.uk> wrote:
> > > ----- Original message -----
> > > > On Wed, 30 Mar 2011 17:07:11 +0100
> > > > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > 
> > > > > Once a NAK has been asserted by the slave, we need to reset the
> > > > > GMBUS controller in order to continue. This is done by asserting
> > > > > the Software Clear Interrupt bit and then clearing it again to
> > > > > restore operations.
> > > > > 
> > > > > If we don't clear the NAK, then all future GMBUS xfers will fail,
> > > > > including DDC probes and EDID retrieval.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35781
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > ---
> > > > 
> > > > This one fixes the issue I was seeing on my HP test machine; LVDS
> > > > DDC probing seems to work ok once this fix is applied.
> > > > 
> > > > -- 
> > > Could this be related to my inability to successfully probe the
> > > ch7036 with the ChromeOS Chrontel driver? Is it worth me testing it
> > > again with this patch applied? (I'm currently waiting to hear back
> > > from Zotac, my board supplier)
> > 
> > Hmm. Depends, but unlikely. I would have thought the nm10_gpio driver
> > you were using used it's only bitbanging on the GPIO lines rather than
> > GMBUS. If in doubt, disable the use of GMBUS by 
> > 
> The GPIO is only used for HDMI hotplug detection, chip communication
> goes via i2c, or at least would if I could get it working... :-(

Since I pulled in drm-fixes I get different behaviour when probing for the ch7036, in my kernel log I now get:
[drm] GMBUS timed out, falling back to bit banging on pin 0 [i915 gmbus disabled]
[drm] GMBUS timed out, falling back to bit banging on pin 4 [i915 gmbus dpc]
[drm] GMBUS timed out, falling back to bit banging on pin 5 [i915 gmbus dpb]
[drm] GMBUS timed out, falling back to bit banging on pin 6 [i915 gmbus reserved]
[drm] GMBUS timed out, falling back to bit banging on pin 7 [i915 gmbus dpd]

The probe still fails, on some i2c buses I get as before:

XAUTHORITY=/home/mythtv/.Xauthority ./ch7036_monitor -v -p -d/dev/i2c-2
./ch7036_monitor: starts
Found device ID 0xff
./ch7036_monitor: Fatal: Device ID 0xff not the expected 0x56

whilst on others:

XAUTHORITY=/home/mythtv/.Xauthority ./ch7036_monitor -v -p -d/dev/i2c-4
./ch7036_monitor: starts
Write byte fail (3, 03, 04): No such device or addressFound device ID 0xffffffff
./ch7036_monitor: Fatal: Device ID 0xffffffff not the expected 0x56