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