Message ID | alpine.LNX.2.00.1303190953010.9529@pobox.suse.cz (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Mar 19, 2013 at 09:56:57AM +0100, Jiri Kosina wrote: > On Mon, 18 Mar 2013, Chris Wilson wrote: > > > > +#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5) > > > void > > > 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); > > > + if (HAS_GMBUS_IRQ(dev)) > > > + I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0); > > > > There should not be any harm in always clearing GMBUS4, it exists on all > > platforms. > > > > > } > > > > > > static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable) > > > @@ -203,7 +205,6 @@ 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, > > > @@ -214,6 +215,13 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv, > > > u32 gmbus2 = 0; > > > DEFINE_WAIT(wait); > > > > > > + if (!HAS_GMBUS_IRQ(dev_priv->dev)) { > > > + int ret; > > > + ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & > > > + (GMBUS_SATOER | gmbus2_status), > > > + 50); > > > > This should couple up to the normal return code that chooses the > > appropriate return value based on gmbus2. > > > > How about just using: > > if (!HAS_GMBUS_IRQ(dev_priv->dev)) gmbus4_irq_en = 0; > > and the existing wait loop? > > I explicitly wanted to avoid touching GMBUS4 register, as the real cause > of the failure is not clear. > > But, as Yinghai Lu points out, the problem is most likely caused by > interrupt disabling not working properly (see his very good point > regarding DisINTx+ and INTx+ discrepancy), so zeroing the register out > should work .... and it indeed does in my case, hence the (tested) patch > below. > > I think it's a 3.9-rc material, and I am all open to debug this further > for 3.10 so that the race is closed and gmbus irqs can be used on Gen4 > platform properly. Agreed. Using the IRQ for GMBUS is just a performance feature that can be deferred until after we determine the root cause - and hope that the failure is somehow peculiar to GMBUS. Acked-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index acf8aec..9934724 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -203,7 +203,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) algo->data = bus; } -#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4) +#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5) static int gmbus_wait_hw_status(struct drm_i915_private *dev_priv, u32 gmbus2_status, @@ -214,6 +214,8 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv, u32 gmbus2 = 0; DEFINE_WAIT(wait); + if (!HAS_GMBUS_IRQ(dev_priv->dev)) + gmbus4_irq_en = 0; /* 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. */