Message ID | 1305235044-9159-10-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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
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.
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
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 --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)
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(-)