Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1161141 > unrolled thread

Re: [PATCH] i2c: designware: use enable on resume instead initialization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] i2c: designware: use enable on resume instead  initialization Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-06-09 11:00 +0200
    Re: [PATCH] i2c: designware: use enable on resume instead  initialization Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-06-11 11:40 +0200

#1161141 — Re: [PATCH] i2c: designware: use enable on resume instead initialization

On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.marchi@gmail.com wrote:
> From: Fabio Mello <fabio.mello@intel.com>
> 
> According to documentation and tests, initialization is not
> necessary on module resume, since the controller keeps its state
> between disable/enable. Change the target address is also allowed.
> 
> So, this patch replaces the initialization on module resume with a
> simple enable, and removes the (non required anymore) enables and
> disables.
> 
> Signed-off-by: Fabio Mello <fabio.mello@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> 
> These pictures explain a little more the consequence of letting the
> enable+disable in the code:
> 
> 	http://pub.politreco.com/paste/TEK0011-before.jpg
> 	http://pub.politreco.com/paste/TEK0007-after.jpg
> 
> The yellow line is a GPIO toggle in userspace to mark when we start and finish
> the i2c transactions.  The blue line is the SCL in that i2c bus. Take a look on
> the huge pauses we have between any 2 transactions.  These pauses are removed
> with this patch and we are able to read our sensor's values in 950usec rather
> than 5.24msec we had before.  We are testing this using a Minnowboard Max that
> has a designware i2c controller.

Did you test this on any other platform than Intel Baytrail?

I'm adding Christian Ruppert (keeping the context) if he has any
comments. IIRC he added the per transfer enable/disable some time ago.

> 
> There's this comment in the code suggesting the designware controller might
> have problems if we don't disable it after each transfer.  We left a stress
> code running for 3 days to check if anything bad would happen.  This is the
> test code using a PCA9685 (just because it was the easiest device we had
> available to check read+write commands):
> 
> 	http://pub.politreco.com/paste/test-i2c.c
> 
>  drivers/i2c/busses/i2c-designware-core.c    | 19 ++++---------------
>  drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
>  2 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 6e25c01..b386c10 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -375,8 +375,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	/* configure the i2c master */
>  	dw_writel(dev, dev->master_cfg , DW_IC_CON);
>  
> +	/* Enable the adapter */
> +	__i2c_dw_enable(dev, true);
> +
>  	if (dev->release_lock)
>  		dev->release_lock(dev);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_init);
> @@ -405,9 +409,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 ic_con, ic_tar = 0;
>  
> -	/* Disable the adapter */
> -	__i2c_dw_enable(dev, false);
> -
>  	/* if the slave address is ten bit address, enable 10BITADDR */
>  	ic_con = dw_readl(dev, DW_IC_CON);
>  	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
> @@ -434,9 +435,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	/* enforce disabled interrupts (due to HW issues) */
>  	i2c_dw_disable_int(dev);
>  
> -	/* Enable the adapter */
> -	__i2c_dw_enable(dev, true);
> -
>  	/* Clear and enable interrupts */
>  	i2c_dw_clear_int(dev);
>  	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
> @@ -665,15 +663,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  		goto done;
>  	}
>  
> -	/*
> -	 * We must disable the adapter before unlocking the &dev->lock mutex
> -	 * below. Otherwise the hardware might continue generating interrupts
> -	 * which in turn causes a race condition with the following transfer.
> -	 * Needs some more investigation if the additional interrupts are
> -	 * a hardware bug or this driver doesn't handle them correctly yet.
> -	 */
> -	__i2c_dw_enable(dev, false);
> -
>  	if (dev->msg_err) {
>  		ret = dev->msg_err;
>  		goto done;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c270f5f..0598695 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
>  	clk_prepare_enable(i_dev->clk);
>  
>  	if (!i_dev->pm_runtime_disabled)
> -		i2c_dw_init(i_dev);
> +		i2c_dw_enable(i_dev);
>  
>  	return 0;
>  }
> -- 
> 2.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1163098

On Wed, Jun 10, 2015 at 05:05:16PM +0200, christian.ruppert@alitech.com wrote:
>    We should understand why the controller was disabled after successful
>    transfers in the first place, however. Maybe some quirk with older
>    versions of the hardware? Mika, do you have any memories about this?

I think it was in the original driver even before I did Intel specific
changes to that. I have no idea why it is done. Probably, like you said,
because some older version of the hardware needed it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web