[Intel-gfx] [PATCH 1/2] drm/i915: Take forcewake once for the entire GMBUS transaction
David Weinehall
david.weinehall at linux.intel.com
Mon Aug 22 16:42:16 UTC 2016
On Fri, Aug 19, 2016 at 05:45:02PM +0100, Chris Wilson wrote:
> As we do many register reads within a very short period of time, hold
> the GMBUS powerwell from start to finish.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: David Weinehall <david.weinehall at linux.intel.com>
Looks good, works well.
Reviewed-by: David Weinehall <david.weinehall at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_i2c.c | 131 +++++++++++++++++++--------------------
> 1 file changed, 63 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 1f266d7df2ec..6841c41281a3 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -255,67 +255,59 @@ intel_gpio_setup(struct intel_gmbus *bus, unsigned int pin)
> algo->data = bus;
> }
>
> -static int
> -gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> - u32 gmbus2_status,
> - u32 gmbus4_irq_en)
> +static int gmbus_wait(struct drm_i915_private *dev_priv, u32 status, u32 irq_en)
> {
> - int i;
> - u32 gmbus2 = 0;
> DEFINE_WAIT(wait);
> -
> - if (!HAS_GMBUS_IRQ(dev_priv))
> - gmbus4_irq_en = 0;
> + u32 gmbus2;
> + int ret;
>
> /* 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. */
> - I915_WRITE(GMBUS4, gmbus4_irq_en);
> -
> - for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
> - prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
> - TASK_UNINTERRUPTIBLE);
> + * need to wake up periodically and check that ourselves.
> + */
> + if (!HAS_GMBUS_IRQ(dev_priv))
> + irq_en = 0;
>
> - gmbus2 = I915_READ_NOTRACE(GMBUS2);
> - if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
> - break;
> + add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> + I915_WRITE_FW(GMBUS4, irq_en);
>
> - schedule_timeout(1);
> - }
> - finish_wait(&dev_priv->gmbus_wait_queue, &wait);
> + status |= GMBUS_SATOER;
> + ret = wait_for_us((gmbus2 = I915_READ_FW(GMBUS2)) & status, 2);
> + if (ret)
> + ret = wait_for((gmbus2 = I915_READ_FW(GMBUS2)) & status, 50);
>
> - I915_WRITE(GMBUS4, 0);
> + I915_WRITE_FW(GMBUS4, 0);
> + remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
>
> if (gmbus2 & GMBUS_SATOER)
> return -ENXIO;
> - if (gmbus2 & gmbus2_status)
> - return 0;
> - return -ETIMEDOUT;
> +
> + return ret;
> }
>
> static int
> gmbus_wait_idle(struct drm_i915_private *dev_priv)
> {
> + DEFINE_WAIT(wait);
> + u32 irq_enable;
> int ret;
>
> - if (!HAS_GMBUS_IRQ(dev_priv))
> - return intel_wait_for_register(dev_priv,
> - GMBUS2, GMBUS_ACTIVE, 0,
> - 10);
> -
> /* Important: The hw handles only the first bit, so set only one! */
> - I915_WRITE(GMBUS4, GMBUS_IDLE_EN);
> + irq_enable = 0;
> + if (HAS_GMBUS_IRQ(dev_priv))
> + irq_enable = GMBUS_IDLE_EN;
>
> - ret = wait_event_timeout(dev_priv->gmbus_wait_queue,
> - (I915_READ_NOTRACE(GMBUS2) & GMBUS_ACTIVE) == 0,
> - msecs_to_jiffies_timeout(10));
> + add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> + I915_WRITE_FW(GMBUS4, irq_enable);
>
> - I915_WRITE(GMBUS4, 0);
> + ret = intel_wait_for_register_fw(dev_priv,
> + GMBUS2, GMBUS_ACTIVE, 0,
> + 10);
>
> - if (ret)
> - return 0;
> - else
> - return -ETIMEDOUT;
> + I915_WRITE_FW(GMBUS4, 0);
> + remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> +
> + return ret;
> }
>
> static int
> @@ -323,22 +315,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
> unsigned short addr, u8 *buf, unsigned int len,
> u32 gmbus1_index)
> {
> - I915_WRITE(GMBUS1,
> - gmbus1_index |
> - GMBUS_CYCLE_WAIT |
> - (len << GMBUS_BYTE_COUNT_SHIFT) |
> - (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> - GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> + I915_WRITE_FW(GMBUS1,
> + gmbus1_index |
> + GMBUS_CYCLE_WAIT |
> + (len << GMBUS_BYTE_COUNT_SHIFT) |
> + (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> + GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> while (len) {
> int ret;
> u32 val, loop = 0;
>
> - ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
> - GMBUS_HW_RDY_EN);
> + ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
> if (ret)
> return ret;
>
> - val = I915_READ(GMBUS3);
> + val = I915_READ_FW(GMBUS3);
> do {
> *buf++ = val & 0xff;
> val >>= 8;
> @@ -385,12 +376,12 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
> len -= 1;
> }
>
> - I915_WRITE(GMBUS3, val);
> - I915_WRITE(GMBUS1,
> - GMBUS_CYCLE_WAIT |
> - (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
> - (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> - GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> + I915_WRITE_FW(GMBUS3, val);
> + I915_WRITE_FW(GMBUS1,
> + GMBUS_CYCLE_WAIT |
> + (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
> + (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> + GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> while (len) {
> int ret;
>
> @@ -399,10 +390,9 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
> val |= *buf++ << (8 * loop);
> } while (--len && ++loop < 4);
>
> - I915_WRITE(GMBUS3, val);
> + I915_WRITE_FW(GMBUS3, val);
>
> - ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
> - GMBUS_HW_RDY_EN);
> + ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
> if (ret)
> return ret;
> }
> @@ -460,13 +450,13 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>
> /* GMBUS5 holds 16-bit index */
> if (gmbus5)
> - I915_WRITE(GMBUS5, gmbus5);
> + I915_WRITE_FW(GMBUS5, gmbus5);
>
> ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
>
> /* Clear GMBUS5 after each index transfer */
> if (gmbus5)
> - I915_WRITE(GMBUS5, 0);
> + I915_WRITE_FW(GMBUS5, 0);
>
> return ret;
> }
> @@ -478,11 +468,15 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> struct intel_gmbus,
> adapter);
> struct drm_i915_private *dev_priv = bus->dev_priv;
> + const unsigned int fw =
> + intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> + FW_REG_READ | FW_REG_WRITE);
> int i = 0, inc, try = 0;
> int ret = 0;
>
> + intel_uncore_forcewake_get(dev_priv, fw);
> retry:
> - I915_WRITE(GMBUS0, bus->reg0);
> + I915_WRITE_FW(GMBUS0, bus->reg0);
>
> for (; i < num; i += inc) {
> inc = 1;
> @@ -496,8 +490,8 @@ retry:
> }
>
> if (!ret)
> - ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
> - GMBUS_HW_WAIT_EN);
> + ret = gmbus_wait(dev_priv,
> + GMBUS_HW_WAIT_PHASE, GMBUS_HW_WAIT_EN);
> if (ret == -ETIMEDOUT)
> goto timeout;
> else if (ret)
> @@ -508,7 +502,7 @@ retry:
> * a STOP on the very first cycle. To simplify the code we
> * unconditionally generate the STOP condition with an additional gmbus
> * cycle. */
> - I915_WRITE(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
> + I915_WRITE_FW(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
>
> /* Mark the GMBUS interface as disabled after waiting for idle.
> * We will re-enable it at the start of the next xfer,
> @@ -519,7 +513,7 @@ retry:
> adapter->name);
> ret = -ETIMEDOUT;
> }
> - I915_WRITE(GMBUS0, 0);
> + I915_WRITE_FW(GMBUS0, 0);
> ret = ret ?: i;
> goto out;
>
> @@ -548,9 +542,9 @@ clear_err:
> * of resetting the GMBUS controller and so clearing the
> * BUS_ERROR raised by the slave's NAK.
> */
> - I915_WRITE(GMBUS1, GMBUS_SW_CLR_INT);
> - I915_WRITE(GMBUS1, 0);
> - I915_WRITE(GMBUS0, 0);
> + I915_WRITE_FW(GMBUS1, GMBUS_SW_CLR_INT);
> + I915_WRITE_FW(GMBUS1, 0);
> + I915_WRITE_FW(GMBUS0, 0);
>
> DRM_DEBUG_KMS("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
> adapter->name, msgs[i].addr,
> @@ -573,7 +567,7 @@ clear_err:
> timeout:
> DRM_DEBUG_KMS("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
> bus->adapter.name, bus->reg0 & 0xff);
> - I915_WRITE(GMBUS0, 0);
> + I915_WRITE_FW(GMBUS0, 0);
>
> /*
> * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> @@ -582,6 +576,7 @@ timeout:
> ret = -EAGAIN;
>
> out:
> + intel_uncore_forcewake_put(dev_priv, fw);
> return ret;
> }
>
> --
> 2.9.3
>
More information about the Intel-gfx
mailing list