Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable

[Westerberg, Mika]

On Sun, Apr 06, 2014 at 06:58:18PM +0100, One Thousand Gnomes wrote:
> On Sat, 5 Apr 2014 09:13:16 +0300
> "Westerberg, Mika" <mika.westerberg@xxxxxxxxx> 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 ?

I had to check BYT specs about that and I couldn't find if it does
posted-writes. For the "hidden" PCI config space it does but that's not
used in the driver anyway.

The thing here is that after suspend/resume cycle, the I2C host controller
gets reset. Once that happens the interrupt mask register is initialized to
0x8ff meaning that for example TX_EMPTY interrupts is unmasked. Nothing
happens though, as the controller is still disabled.

Now when we start the first I2C transaction after resume we call
i2c_dw_xfer_init() that then enables the controller and an interrupt is
immediately triggered even though we didn't finish the initialization. This
makes the controller/driver confused resulting timeout that Wenkai sees.

Actually the following patch should fix the problem as well. Just move the
HW enable to happen last. That way we can make sure that there is a valid
interrupt mask programmed before the controller is enabled.

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 14c4b30d4ccc..35e3371f840c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -417,12 +417,12 @@ 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);
 
-	/* Enable the adapter */
-	__i2c_dw_enable(dev, true);
-
-	/* Clear and enable interrupts */
+	/* Clear and unmask interrupts */
 	i2c_dw_clear_int(dev);
 	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
+
+	/* Enable the adapter (and interrupts) */
+	__i2c_dw_enable(dev, true);
 }
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/