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

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

[Du, Wenkai]

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(+)

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);
+
 	/* Enable the adapter */
 	__i2c_dw_enable(dev, true);
 
-- 
1.7.9.5

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

[Westerberg, Mika]

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

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

[Westerberg, Mika]

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

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

[Du, Wenkai]

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

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

[Du, Wenkai]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Wolfram Sang; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

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

[Westerberg, Mika]

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.

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

[Westerberg, Mika]

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.

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

[Du, Wenkai]

>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. 

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

[Du, Wenkai]

>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. 

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

[Westerberg, Mika]

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.

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

[Westerberg, Mika]

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.

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

[One Thousand Gnomes]

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

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

[One Thousand Gnomes]

On Sat, 5 Apr 2014 09:13:16 +0300
"Westerberg, Mika" <mika.westerberg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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

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@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 ?

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);
 }
 
 /*

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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);
 }
 
 /*

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

[One Thousand Gnomes]

> I had to check BYT specs about that and I couldn't find if it does
> posted-writes. 

Then I would assume it does unless you can find a hardware engineer to
sign a statement in blood to that effect 8)

> 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.

This fixes the init case, it doesn't fix the question about returning
from the IRQ before the mask write takes effect and thus taking another
interrupt.

Alan

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

[One Thousand Gnomes]

> I had to check BYT specs about that and I couldn't find if it does
> posted-writes. 

Then I would assume it does unless you can find a hardware engineer to
sign a statement in blood to that effect 8)

> 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.

This fixes the init case, it doesn't fix the question about returning
from the IRQ before the mask write takes effect and thus taking another
interrupt.

Alan

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

[Westerberg, Mika]

On Mon, Apr 07, 2014 at 03:42:52PM +0100, One Thousand Gnomes wrote:
> > I had to check BYT specs about that and I couldn't find if it does
> > posted-writes. 
> 
> Then I would assume it does unless you can find a hardware engineer to
> sign a statement in blood to that effect 8)

Fair enough.

> > 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.
> 
> This fixes the init case, it doesn't fix the question about returning
> from the IRQ before the mask write takes effect and thus taking another
> interrupt.

Do you think we can fix it with adding a dummy read right after write to
the mask register, like the snippet below?

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 14c4b30d4ccc..ff9090381d8b 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -535,6 +535,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		intr_mask = 0;
 
 	dw_writel(dev, intr_mask,  DW_IC_INTR_MASK);
+	dw_readl(dev, DW_IC_INTR_MASK);
 }
 
 static void

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

[Du, Wenkai]

> -----Original Message-----
> From: Westerberg, Mika
> Sent: Monday, April 07, 2014 8:11 AM
> To: One Thousand Gnomes
> Cc: Du, Wenkai; linux-i2c@vger.kernel.org; Wolfram Sang; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable
> 
> 
> Do you think we can fix it with adding a dummy read right after write to the mask register, like
> the snippet below?
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-
> core.c
> index 14c4b30d4ccc..ff9090381d8b 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -535,6 +535,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  		intr_mask = 0;
> 
>  	dw_writel(dev, intr_mask,  DW_IC_INTR_MASK);
> +	dw_readl(dev, DW_IC_INTR_MASK);
>  }
> 

I have tried this read back earlier during debugging, it didn't help. You can confirm yourself as well.

On the ISR code execution path, here is the list to my understanding:

1. TX abort: this path masks all interrupt by writing 0 to interrupt mask register;
2. RX full: this path doesn't do anything on masking interrupts;
3. TX empty: this path only mask TX empty under certain conditions; it doesn't mask other interrupt sources;
4. i2c STOP condition: this path call complete() to finish the transfer; it doesn't do anything on masking interrupts;

The above analysis agrees with the debug logs: most of interrupts are left unmasked in original code when i2c core is being enabled.

I think we want all interrupts masked when enabling HW. This will ensure the required 25us core turn on delay being respected. Also there could be spurious interrupts when HW is being turned on that should be ignored.

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

[Du, Wenkai]

> -----Original Message-----
> From: Westerberg, Mika
> Sent: Monday, April 07, 2014 8:11 AM
> To: One Thousand Gnomes
> Cc: Du, Wenkai; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Wolfram Sang; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable
> 
> 
> Do you think we can fix it with adding a dummy read right after write to the mask register, like
> the snippet below?
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-
> core.c
> index 14c4b30d4ccc..ff9090381d8b 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -535,6 +535,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  		intr_mask = 0;
> 
>  	dw_writel(dev, intr_mask,  DW_IC_INTR_MASK);
> +	dw_readl(dev, DW_IC_INTR_MASK);
>  }
> 

I have tried this read back earlier during debugging, it didn't help. You can confirm yourself as well.

On the ISR code execution path, here is the list to my understanding:

1. TX abort: this path masks all interrupt by writing 0 to interrupt mask register;
2. RX full: this path doesn't do anything on masking interrupts;
3. TX empty: this path only mask TX empty under certain conditions; it doesn't mask other interrupt sources;
4. i2c STOP condition: this path call complete() to finish the transfer; it doesn't do anything on masking interrupts;

The above analysis agrees with the debug logs: most of interrupts are left unmasked in original code when i2c core is being enabled.

I think we want all interrupts masked when enabling HW. This will ensure the required 25us core turn on delay being respected. Also there could be spurious interrupts when HW is being turned on that should be ignored.

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

[Westerberg, Mika]

On Mon, Apr 07, 2014 at 07:48:02PM +0300, Du, Wenkai wrote:
> > -----Original Message-----
> > From: Westerberg, Mika
> > Sent: Monday, April 07, 2014 8:11 AM
> > To: One Thousand Gnomes
> > Cc: Du, Wenkai; linux-i2c@vger.kernel.org; Wolfram Sang; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable
> > 
> > 
> > Do you think we can fix it with adding a dummy read right after write to the mask register, like
> > the snippet below?
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-
> > core.c
> > index 14c4b30d4ccc..ff9090381d8b 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -535,6 +535,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> >  		intr_mask = 0;
> > 
> >  	dw_writel(dev, intr_mask,  DW_IC_INTR_MASK);
> > +	dw_readl(dev, DW_IC_INTR_MASK);
> >  }
> > 
> 
> I have tried this read back earlier during debugging, it didn't help. You
> can confirm yourself as well.

I mean that it helps for the posted writes to the mask register issue, Alan
pointed out.

> On the ISR code execution path, here is the list to my understanding:
> 
> 1. TX abort: this path masks all interrupt by writing 0 to interrupt mask register;
> 2. RX full: this path doesn't do anything on masking interrupts;
> 3. TX empty: this path only mask TX empty under certain conditions; it
> doesn't mask other interrupt sources;
> 4. i2c STOP condition: this path call complete() to finish the transfer;
> it doesn't do anything on masking interrupts;

You are right.

The driver enables the controller and then unmasks following interrupt
sources:

	DW_IC_INTR_RX_FULL
	DW_IC_INTR_TX_EMPTY
	DW_IC_INTR_TX_ABRT
	DW_IC_INTR_STOP_DET

This will immediately trigger TX_EMPTY interrupt and the transfer begins.
If it finishes successfully, the driver only clears DW_IC_INTR_TX_EMPTY and
disables the controller. In Subsequent transfers the same happens and it
seems to work fine (as we have the controller disabled whenever we are done
with the transfer).

I'm not sure I agree that this is the best approach but like I said, it
seems to work just fine.

Except when we resume after suspend when the controller is reset. This will
mess up the "expected" mask and trigger interrupt immediately when the
controller is enabled in i2c_dw_xfer_init().

> The above analysis agrees with the debug logs: most of interrupts are
> left unmasked in original code when i2c core is being enabled.

Yes, but only after resume. In normal cases the driver re-programs
DW_IC_INTR_TX_EMPTY to the mask (the other 3 flags are already set but
they should be harmless as long as the TX fifo is still empty).

> I think we want all interrupts masked when enabling HW. This will ensure
> the required 25us core turn on delay being respected. Also there could be
> spurious interrupts when HW is being turned on that should be ignored.

Where did you get this 25us required delay? My spec doesn't say anything
about that. It does however say that if the state bit is not yet changed,
wait 25us (@400kHz) and re-read the bit. So it can be that we don't wait at
all once we toggle that bit.

All in all, I think we can solve this resume path timeout issue, by moving
__i2c_dw_enable() to be last in i2c_dw_xfer_init().

In addition we should fix the potential posted-write problem Alan pointed
out (as a separate patch). That could actually explain the issue you have
seen where timeout occurs during normal operation. Maybe fix here is to
mask all interrupts at the end of the ISR and make sure that possible
posted writes gets flushed to the hardware before returning?

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

[Du, Wenkai]

> -----Original Message-----
> From: Westerberg, Mika
> Sent: Tuesday, April 08, 2014 3:30 AM
> To: Du, Wenkai
> Cc: One Thousand Gnomes; linux-i2c@vger.kernel.org; Wolfram Sang; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable

> Yes, but only after resume. In normal cases the driver re-programs DW_IC_INTR_TX_EMPTY to
> the mask (the other 3 flags are already set but they should be harmless as long as the TX fifo is
> still empty).
I have sent you failure traces in normal path. None of 4 interrupt sources was masked.  

> Where did you get this 25us required delay? My spec doesn't say anything about that. It does
> however say that if the state bit is not yet changed, wait 25us (@400kHz) and re-read the bit.
> So it can be that we don't wait at all once we toggle that bit.
I believe the delay patch ("i2c-designware: enable/disable the controller properly") was from you? I would be happy to see the patch reverted if these delay are really not needed. 25us-250us delay 3 times (2 disable, 1 enable) at every transfer slow down the bus too much.
 
> All in all, I think we can solve this resume path timeout issue, by moving
> __i2c_dw_enable() to be last in i2c_dw_xfer_init().
This will invalidate 2 previous patches from you: " i2c-designware: enable/disable the controller properly" , and "i2c-designware: always clear interrupts before enabling them". Are you sure they can be all reverted?

> In addition we should fix the potential posted-write problem Alan pointed out (as a separate
> patch). That could actually explain the issue you have seen where timeout occurs during normal
> operation. Maybe fix here is to mask all interrupts at the end of the ISR and make sure that
> possible posted writes gets flushed to the hardware before returning?
But these patches attempt to fix "leaking" as we know of today. In my view, masking interrupts right before enable HW is still the best, which is common practice to most drivers. It is the spirit of the original driver as well: the original driver enables interrupts _after_ core enable. I think the author must have thought the interrupt have been disabled, otherwise why enable them again? Masking interrupts fixes all the known and unknown leaks and is future proof.

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

[Westerberg, Mika]

On Thu, Apr 10, 2014 at 02:45:54AM +0300, Du, Wenkai wrote:
> > -----Original Message-----
> > From: Westerberg, Mika
> > Sent: Tuesday, April 08, 2014 3:30 AM
> > To: Du, Wenkai
> > Cc: One Thousand Gnomes; linux-i2c@vger.kernel.org; Wolfram Sang; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable
> 
> > Yes, but only after resume. In normal cases the driver re-programs DW_IC_INTR_TX_EMPTY to
> > the mask (the other 3 flags are already set but they should be harmless as long as the TX fifo is
> > still empty).
> I have sent you failure traces in normal path. None of 4 interrupt
> sources was masked.  

OK, I went through it and can't explain why those are not masked :-( And I
can't reproduce this myself.

> > Where did you get this 25us required delay? My spec doesn't say anything about that. It does
> > however say that if the state bit is not yet changed, wait 25us (@400kHz) and re-read the bit.
> > So it can be that we don't wait at all once we toggle that bit.
> I believe the delay patch ("i2c-designware: enable/disable the controller
> properly") was from you? I would be happy to see the patch reverted if
> these delay are really not needed. 25us-250us delay 3 times (2 disable, 1
> enable) at every transfer slow down the bus too much.

It is enough to have IC_ENABLE_STATUS reporting the desired state. We want
to be sure that the hardware is properly enabled/disabled before doing
anything else that might need it to be in certain state. For example writes
to some registers succeed only when the controller is disabled.

The procedure is taken directly from the I2C DW databook IIRC.

> > All in all, I think we can solve this resume path timeout issue, by moving
> > __i2c_dw_enable() to be last in i2c_dw_xfer_init().
> This will invalidate 2 previous patches from you: " i2c-designware:
> enable/disable the controller properly" , and "i2c-designware: always
> clear interrupts before enabling them". Are you sure they can be all
> reverted?

No.

> > In addition we should fix the potential posted-write problem Alan pointed out (as a separate
> > patch). That could actually explain the issue you have seen where timeout occurs during normal
> > operation. Maybe fix here is to mask all interrupts at the end of the ISR and make sure that
> > possible posted writes gets flushed to the hardware before returning?
>
> But these patches attempt to fix "leaking" as we know of today. In my
> view, masking interrupts right before enable HW is still the best, which
> is common practice to most drivers. It is the spirit of the original
> driver as well: the original driver enables interrupts _after_ core
> enable. I think the author must have thought the interrupt have been
> disabled, otherwise why enable them again? Masking interrupts fixes all
> the known and unknown leaks and is future proof.

Yes, let's do that for now.

Unfortunately we still don't know why the issue you observed happens (the
suspend and normal operation failures). At least we can make sure that
whatever "leaks" doesn't prevent further transactions happening.

Can you resend this patch with a changelog that explains the resume issue?
Along the lines of:

 The default value after reset for DW_IC_INTR_MASK (interrupt mask
 register) is 0x8ff. When we start the first transaction after resuming
 from system sleep TX_EMPTY interrupt is already unmasked (because of the
 hardware default). This causes call to __i2c_dw_enable() to immediatelly
 start the transfer which leads to timeout...

or something similar. So that it is understood that it's the default value
that makes the driver to go astray.

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

[Du, Wenkai]

> -----Original Message-----
> From: Westerberg, Mika
> Sent: Thursday, April 10, 2014 2:08 AM
> To: Du, Wenkai
> Cc: One Thousand Gnomes; linux-i2c@vger.kernel.org; Wolfram Sang; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable

> Can you resend this patch with a changelog that explains the resume issue?
Updated patch descriptions in new thread: 
[PATCH V2] i2c-designware: Mask all interrupts during i2c controller enable

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

[Du, Wenkai]

> -----Original Message-----
> From: Westerberg, Mika
> Sent: Thursday, April 10, 2014 2:08 AM
> To: Du, Wenkai
> Cc: One Thousand Gnomes; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Wolfram Sang; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] i2c-designware: Mask interrupts during i2c controller enable

> Can you resend this patch with a changelog that explains the resume issue?
Updated patch descriptions in new thread: 
[PATCH V2] i2c-designware: Mask all interrupts during i2c controller enable