All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status
Date: Sat, 08 Sep 2012 10:11:17 -0700	[thread overview]
Message-ID: <650bfef0cdcfb37ff55332b7b70476a8@bwidawsk.net> (raw)
In-Reply-To: <1346915402-9399-1-git-send-email-daniel.vetter@ffwll.ch>

On 2012-09-06 00:09, Daniel Vetter wrote:
> The gmbus interrupt generation is rather fiddly: We can only ever
> enable one interrupt source (but we always want to check for NAK
> in addition to the real bit). And the bits in the gmbus status
> register don't map at all to the bis in the irq register.
>
> To prepare for this mess, start by extracting the hw status wait
> loop into it's own function, consolidate the NAK error handling a
> bit. To keep things flexible, pass in the status bit we care about
> (in addition to any NAK signalling).
>
> v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
> Chris Wilson gladly pointed that out for me. To keep things simple,
> ignore that case for  now (we only need to idle the gmbus controller
> at the end of an entire i2c transaction, not after every message).
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 46
> ++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index b9755f6..57decac 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 
> pin)
>  }
>
>  static int
> +gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> +		     u32 gmbus2_status)
> +{
> +	int ret;
> +	int reg_offset = dev_priv->gpio_mmio_base;
> +	u32 gmbus2;
> +
> +	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> +		       (GMBUS_SATOER | gmbus2_status),
> +		       50);
> +
> +	if (gmbus2 & GMBUS_SATOER)
> +		return -ENXIO;
> +
> +	return ret;
> +}
> +
> +static int
>  gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg 
> *msg,
>  		u32 gmbus1_index)
>  {

I know this isn't new to your patch, but the consolidation leaves a 
good opportunity for clarification of the timeout via a comment.

I started digging in to the math a bit here for the 50ms timeout and 
was left confused. I believe we're off by a factor of 10 here for a 
worst case which would be a 50KHz bus speed. Comments?

> @@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg,
>  	while (len) {
>  		int ret;
>  		u32 val, loop = 0;
> -		u32 gmbus2;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>
>  		val = I915_READ(GMBUS3 + reg_offset);
>  		do {
> @@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>  		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
>  	while (len) {
>  		int ret;
> -		u32 gmbus2;
>
>  		val = loop = 0;
>  		do {
> @@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>
>  		I915_WRITE(GMBUS3 + reg_offset, val);
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>  	}
>  	return 0;
>  }
> @@ -346,8 +354,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>
>  	for (i = 0; i < num; i++) {
> -		u32 gmbus2;
> -
>  		if (gmbus_is_index_read(msgs, i, num)) {
>  			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
>  			i += 1;  /* set i to the index of the read xfer */
> @@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  		if (ret == -ENXIO)
>  			goto clear_err;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
> +		if (ret == -ENXIO)
> +			goto clear_err;
>  		if (ret)
>  			goto timeout;
> -		if (gmbus2 & GMBUS_SATOER)
> -			goto clear_err;
>  	}
>
>  	/* Generate a STOP condition on the bus. Note that gmbus can't 
> generata

-- 
Ben Widawsky, Intel Open Source Technology Center

      parent reply	other threads:[~2012-09-08 17:10 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-06  7:09 Daniel Vetter
2012-09-06  7:10 ` [PATCH 2/4] drm/i915: wire up gmbus irq handler Daniel Vetter
2012-09-06  7:10 ` [PATCH 3/4] drm/i915: use the gmbus irq for waits Daniel Vetter
2012-09-06  7:10 ` [PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle Daniel Vetter
2012-09-06 13:06   ` Chris Wilson
2012-09-06 13:44     ` [PATCH] drm/i915: use the gmbus irq for waits Daniel Vetter
2012-09-08 17:11 ` Ben Widawsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=650bfef0cdcfb37ff55332b7b70476a8@bwidawsk.net \
    --to=ben@bwidawsk.net \
    --cc=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    --subject='Re: [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.