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@xxxxxxxxxxxxxxx; Wolfram Sang; linux-kernel@xxxxxxxxxxxxxxx
> > 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?
--
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/