diff mbox

[09/16] drm/i915/gmbus: Reset the controller on initialisation

Message ID 1305235044-9159-10-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson May 12, 2011, 9:17 p.m. UTC
Toggle the Software Clear Interrupt bit which resets the controller to
clear any prior BUS_ERROR condition before we begin to use the
controller in earnest.

Suggested-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

Comments

Keith Packard May 13, 2011, 12:41 a.m. UTC | #1
On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.

Looks reasonable, except for the bad register offsets (see reply to
previous patch). I fear this code wasn't tested at all; it shouldn't
have worked.
Chris Wilson May 13, 2011, 9:32 a.m. UTC | #2
On Thu, 12 May 2011 17:41:36 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Toggle the Software Clear Interrupt bit which resets the controller to
> > clear any prior BUS_ERROR condition before we begin to use the
> > controller in earnest.
> 
> Looks reasonable, except for the bad register offsets (see reply to
> previous patch). I fear this code wasn't tested at all; it shouldn't
> have worked.

It has been booting daily on several machines for a month. I agree it
wouldn't have worked, but the since we automatically fallback to GPIO
should it go south, the failures didn't stop the external monitors from
being lit up.
-Chris
Keith Packard May 13, 2011, 3:01 p.m. UTC | #3
On Fri, 13 May 2011 10:32:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> It has been booting daily on several machines for a month. I agree it
> wouldn't have worked, but the since we automatically fallback to GPIO
> should it go south, the failures didn't stop the external monitors from
> being lit up.

Yeah, nice that we fall back. Do we want a warning in the kernel log
when that happens (once, not multiple times)? At least some way to
verify that the new code is doing what we expect it to do.
Chris Wilson May 13, 2011, 3:53 p.m. UTC | #4
On Fri, 13 May 2011 08:01:51 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 13 May 2011 10:32:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > It has been booting daily on several machines for a month. I agree it
> > wouldn't have worked, but the since we automatically fallback to GPIO
> > should it go south, the failures didn't stop the external monitors from
> > being lit up.
> 
> Yeah, nice that we fall back. Do we want a warning in the kernel log
> when that happens (once, not multiple times)? At least some way to
> verify that the new code is doing what we expect it to do.

We give a warning when we fail a GMBUS sequence and switch (permanently)
to using GPIO. I've been desensitised to those warnings, because there is
sometimes, depending upon the hardware and probes, one channel which fails.
The fact that I'd so utterly broken the code didn't occur to me.
-Chris
Keith Packard June 3, 2011, 8:56 p.m. UTC | #5
On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.

I don't have a new patch with corrected register referencing yet...
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index ed11523..bf1c72c 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -60,10 +60,14 @@  void
 intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(PCH_GMBUS0, 0);
-	else
-		I915_WRITE(GMBUS0, 0);
+	int reg = intel_gmbus_reg0(dev);
+
+	/* First reset the controller by toggling the Sw Clear Interrupt. */
+	I915_WRITE(reg + 1, GMBUS_SW_CLR_INT);
+	I915_WRITE(reg + 1, 0);
+
+	/* Then mark the controller as disabled. */
+	I915_WRITE(reg + 0, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)