| Message ID | 1301995458-2699-6-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 05, 2011 at 10:24:18AM +0100, Chris Wilson 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. > > Suggested-by: Ben Widawsky <ben@bwidawsk.net> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_i2c.c | 14 +++++++++++--- > 1 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > index d3b903b..f9f0c42 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -55,10 +55,18 @@ void > intel_i2c_reset(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + int reg_offset; > + > + reg_offset = 0; > if (HAS_PCH_SPLIT(dev)) > - I915_WRITE(PCH_GMBUS0, 0); > - else > - I915_WRITE(GMBUS0, 0); > + reg_offset = PCH_GMBUS0 - GMBUS0; > + > + /* First reset the controller by toggling the Sw Clear Interrupt. */ > + I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); > + I915_WRITE(GMBUS1 + reg_offset, 0); > + > + /* Then mark the controller as disabled. */ > + I915_WRITE(GMBUS0 + reg_offset, 0); > } > I think in addition to this we should try to send a STOP condition. That way any devices in an unknown state from a failed reset (or something similar), should go back to their default state after seeing a stop. As long as the host is always the master, and the devices are fairly compliant, it should work... Alas, I'm just looking at the GMBUS interface for the first time really, and it appears there is no straight forward way to do what I want outside of bitbanging (you want to avoid toggling SCL until you've issues the STOP). Outside of being able to do that directly, I would add a call to intel_i2c_reset() from intel_teardown_gmbus(). Ben
On Tue, Apr 05, 2011 at 09:18:37AM -0700, Ben Widawsky wrote: > Outside of being able to do that directly, I would add a call to > intel_i2c_reset() from intel_teardown_gmbus(). Hmm, forget that, I think this is silly. How about just a comment in the code stating that intel_i2c_reset() should always be the first thing that happens when you bring up the gmbus. Ben
On Tue, 5 Apr 2011 10:24:18 +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. We do this in two places now, do we want to share the code? > + int reg_offset; > + > + reg_offset = 0; > if (HAS_PCH_SPLIT(dev)) > - I915_WRITE(PCH_GMBUS0, 0); > - else > - I915_WRITE(GMBUS0, 0); > + reg_offset = PCH_GMBUS0 - GMBUS0; > + > + /* First reset the controller by toggling the Sw Clear Interrupt. */ > + I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); > + I915_WRITE(GMBUS1 + reg_offset, 0); > + > + /* Then mark the controller as disabled. */ > + I915_WRITE(GMBUS0 + reg_offset, 0); That's really ugly register addressing, but it looks like a common idiom in this file...
On Tue, 05 Apr 2011 13:59:58 -0700, Keith Packard <keithp@keithp.com> wrote: > On Tue, 5 Apr 2011 10:24:18 +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. > > We do this in two places now, do we want to share the code? > > > + int reg_offset; > > + > > + reg_offset = 0; > > if (HAS_PCH_SPLIT(dev)) > > - I915_WRITE(PCH_GMBUS0, 0); > > - else > > - I915_WRITE(GMBUS0, 0); > > + reg_offset = PCH_GMBUS0 - GMBUS0; > > + > > + /* First reset the controller by toggling the Sw Clear Interrupt. */ > > + I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); > > + I915_WRITE(GMBUS1 + reg_offset, 0); > > + > > + /* Then mark the controller as disabled. */ > > + I915_WRITE(GMBUS0 + reg_offset, 0); > > That's really ugly register addressing, but it looks like a common idiom > in this file... I'd change the lot for a cleaner method, the best I thought of was a change of names for the constants/variables. IMO, static void i915_gmbus_write(struct drm_device *dev, int reg, int value) { struct drm_i915_private *dev_priv = dev->dev_private; I915_WRITE(reg + (HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0), value); } expands to something dreadful. But with a #define GMBUS_WRITE(reg, value) i915_gmbus_write(dev, reg, value) we go from I915_WRITE(GMBUS0 + reg_offset, 0); to GMBUS_WRITE(0, 0); I would still prefer GMBUS_WRITE(GMBUS0, 0); though. As the patch only addresses a theoretical bug, we can punt the meat of the patch till later and attack the stylistic points first. (Obviously through -next.) -Chris
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index d3b903b..f9f0c42 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -55,10 +55,18 @@ void intel_i2c_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + int reg_offset; + + reg_offset = 0; if (HAS_PCH_SPLIT(dev)) - I915_WRITE(PCH_GMBUS0, 0); - else - I915_WRITE(GMBUS0, 0); + reg_offset = PCH_GMBUS0 - GMBUS0; + + /* First reset the controller by toggling the Sw Clear Interrupt. */ + I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); + I915_WRITE(GMBUS1 + reg_offset, 0); + + /* Then mark the controller as disabled. */ + I915_WRITE(GMBUS0 + reg_offset, 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 | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)