i2c-designware: Mask interrupts during i2c controller enable

Hi all,

There were "i2c_designware 80860F41:00: controller timed out" errors
on a number of Baytrail platforms. They typically occurred during suspend
resume where some short i2c transfers were exchanged with devices. And they
mostly occurred when using fast mode, not standard mode.

The cause of the problem is that interrupts start right away when the
controller is enabled. There's this comment in __i2c_dw_enable:

/*
 * Wait 10 times the signaling period of the highest I2C
 * transfer supported by the driver (for 400KHz this is
 * 25us) as described in the DesignWare I2C databook.
 */

The __i2c_dw_enable then does a usleep for 25us to 250us, but since interrupts
were previously enabled, this wait wasn't being respected by ISR. With
additional traces, we could see ISR was activated and finished i2c transfer
while the code was still inside __i2c_dw_enable. At combination of fast
mode, certain transfer size and device response time, i2c_dw_clear_int was executed 
and cleared STOP condition flag, which resulted in i2c time out.

static void i2c_dw_xfer_init(struct dw_i2c_dev *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);
}

The patch disables interrupts before the controller is enabled to remove this
race condition.

Signed-off-by: Wenkai Du <wenkai.du@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c |    3 +++
 1 file changed, 3 insertions(+)

On Fri, Apr 04, 2014 at 08:05:23PM +0300, Du, Wenkai wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 14c4b30..71a3fa9 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -417,6 +417,9 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	 */
>  	dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
>  
> +	/* disable interrupts */
> +	i2c_dw_disable_int(dev);
> +

Please move this to i2c_dw_init() as I previously commented. This can only
happen once the controller comes out of reset (either boot, or resume from
system sleep).

>  	/* Enable the adapter */
>  	__i2c_dw_enable(dev, true);
>  
> -- 
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mika,

In current driver implementation, I2c controller is enabled, then disabled every time inside i2c_dw_xfer. So I think the interrupt masking should be done inside i2c_dw_xfer_init, where the controller is enabled.

i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
{
...
	/* start the transfers */
	i2c_dw_xfer_init(dev);
...
	/*
	 * 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);
...
}


Thanks,
Wenkai



-----Original Message-----
From: Westerberg, Mika 
Sent: Friday, April 04, 2014 11:16 AM
To: Du, Wenkai
Cc: linux-i2c@vger.kernel.org; Wolfram Sang; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable

On Fri, Apr 04, 2014 at 08:05:23PM +0300, Du, Wenkai wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-core.c 
> b/drivers/i2c/busses/i2c-designware-core.c
> index 14c4b30..71a3fa9 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -417,6 +417,9 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	 */
>  	dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
>  
> +	/* disable interrupts */
> +	i2c_dw_disable_int(dev);
> +

Please move this to i2c_dw_init() as I previously commented. This can only happen once the controller comes out of reset (either boot, or resume from system sleep).

>  	/* Enable the adapter */
>  	__i2c_dw_enable(dev, true);
>  
> --
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Apr 04, 2014 at 09:20:39PM +0300, Du, Wenkai wrote:
> In current driver implementation, I2c controller is enabled, then
> disabled every time inside i2c_dw_xfer. So I think the interrupt masking
> should be done inside i2c_dw_xfer_init, where the controller is enabled.

Interrupt masking is done already after each transaction.

The problem here is that after reset, the interrupt mask register gets
0x8ff value (HW default), which means that most of the interrupts are left
unmasked.

That is the reason why this only happens right after we resume from
system sleep. Masking interrupts on that path fixes the problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

>Interrupt masking is done already after each transaction.

At end of transfer, the code uses __i2c_dw_enable(dev, false) to disable adapter. This function doesn't mask interrupts. There is another function i2c_dw_disable that masks and clears interrupts. This could be used, but that means we need to fix in 2 places: 

1. add interrupt masking to i2c_dw_init();
2. change call __i2c_dw_enable(dev, false) to i2c_dw_disable;

I think simply masking interrupt in i2c_dw_xfer_init is cleaner and safer.

>The problem here is that after reset, the interrupt mask register gets 0x8ff value (HW default), which means that most of the interrupts are left unmasked.
>That is the reason why this only happens right after we resume from system sleep. Masking interrupts on that path fixes the problem.

The time out also happened in case of going into suspend and during normal operations. But at much less occurrence at 1-2 per 1000 cycles. 


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Apr 05, 2014 at 12:54:33AM +0300, Du, Wenkai wrote:
> >Interrupt masking is done already after each transaction.
> 
> At end of transfer, the code uses __i2c_dw_enable(dev, false) to disable
> adapter. This function doesn't mask interrupts. There is another function
> i2c_dw_disable that masks and clears interrupts. This could be used, but
> that means we need to fix in 2 places: 

Please check i2c_dw_isr() and tell me in which code path interrupts are not
getting masked. Or am I missing something fundamental here?

In case of abort, we mask interrupts. Also whenever the transaction
completes we mask interrupts (in i2c_dw_xfer_msg()).

Only place where we didn't do that, as far as I can tell, is right after
reset because of the HW default value that unmasks most of them.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 5 Apr 2014 09:13:16 +0300
"Westerberg, Mika" <mika.westerberg@intel.com> wrote:

> On Sat, Apr 05, 2014 at 12:54:33AM +0300, Du, Wenkai wrote:
> > >Interrupt masking is done already after each transaction.
> > 
> > At end of transfer, the code uses __i2c_dw_enable(dev, false) to disable
> > adapter. This function doesn't mask interrupts. There is another function
> > i2c_dw_disable that masks and clears interrupts. This could be used, but
> > that means we need to fix in 2 places: 
> 
> Please check i2c_dw_isr() and tell me in which code path interrupts are not
> getting masked. Or am I missing something fundamental here?
> 
> In case of abort, we mask interrupts. Also whenever the transaction
> completes we mask interrupts (in i2c_dw_xfer_msg()).

Well actually you mask the IRQ at some point after the function returns
if the bus allows the write to be posted. As i2c_dw_isr can then exit the
IRQ handler before the write completes I suspect you have a race ?

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html