On Tue, Jun 23, 2015 at 1:45 PM, <christian.ruppert@xxxxxxxxxxx> wrote: > Hello, > > Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16: >> Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> wrote on 10.06. >> 2015 09:07:22: >> > On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote: >> > > Hi Mika, >> > > >> > > On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg >> > > <mika.westerberg@xxxxxxxxxxxxxxx> wrote: >> > > > On Mon, Jun 08, 2015 at 02:50:28PM -0300, >> lucas.de.marchi@xxxxxxxxx wrote: >> > > >> From: Fabio Mello <fabio.mello@xxxxxxxxx> >> > > >> >> > > >> According to documentation and tests, initialization is not >> > > >> necessary on module resume, since the controller keeps its state >> > > >> between disable/enable. Change the target address is also > allowed. >> > > >> >> > > >> So, this patch replaces the initialization on module resume with > a >> > > >> simple enable, and removes the (non required anymore) enables and >> > > >> disables. >> > > >> >> > > >> Signed-off-by: Fabio Mello <fabio.mello@xxxxxxxxx> >> > > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >> > > >> --- >> > > >> >> > > >> These pictures explain a little more the consequence of letting > the >> > > >> enable+disable in the code: >> > > >> >> > > >> http://pub.politreco.com/paste/TEK0011-before.jpg >> > > >> http://pub.politreco.com/paste/TEK0007-after.jpg >> > > >> >> > > >> The yellow line is a GPIO toggle in userspace to mark when we >> > start and finish >> > > >> the i2c transactions. The blue line is the SCL in that i2c >> > bus. Take a look on >> > > >> the huge pauses we have between any 2 transactions. These >> > pauses are removed >> > > >> with this patch and we are able to read our sensor's values in >> > 950usec rather >> > > >> than 5.24msec we had before. We are testing this using a >> > Minnowboard Max that >> > > >> has a designware i2c controller. >> > > > >> > > > Did you test this on any other platform than Intel Baytrail? >> > > >> > > No. The only soc we have here with this controller is the Baytrail. >> > >> > My concern is that this patch might break some non-Intel platform. It >> > would be nice if someone (Christian?) could try this out. >> >> Ouch, this one brings back painful memories. Take a look at patch >> 38d7fadef4973bb94e36897fcb6bb6a12fdd10c9 (https://git.kernel.org/ >> cgit/linux/kernel/git/torvalds/linux.git/commit/? >> id=38d7fadef4973bb94e36897fcb6bb6a12fdd10c9) for the context. >> >> In brief: >> - Before patch 38d7fade, the driver disabled the hardware after >> successful transfers. I do not know the reason for this and I cannot >> judge whether this is necessary or not. I thus decided not to modify >> this behaviour in patch 38d7fade. >> >> - After patch 38d7fade, the driver disabled the dw controller after >> all transfers, in particular in the case of unsuccessful transfers. >> This modification was necessary because of a race condition >> triggered by an i2c slave device which interrupted transfers in the >> middle. In this case, the dw controller (at least our version) seems >> to send spurious interrupts later if it is not disabled. The >> interrupt handler is not designed to be called on already aborted >> transfers, however, which leads to undesirable behaviour if the >> interrupt occurs at the wrong moment (system hangs in irq loop). >> >> I will try to dig out the test setup we used to validate the patch >> at the time but given the fact that this was two years ago this >> might take a little while. In the meantime, do you have any means to >> stress test the case of unexpected events on the bus (client aborts >> transfer, timeout etc.)? An alternative might be to only disable the >> controller in the case of errors and leave it active after >> successful transfers. We should understand why the controller was >> disabled after successful transfers in the first place, however. >> Maybe some quirk with older versions of the hardware? Mika, do you >> have any memories about this? > > As promised I tried to dig out the test setup we used to validate patch > 38d7fade at the time but without success. (I half expected that after such > a long time...) > > So I said to myself, let's give the patch a try nevertheless and see if it > works in our system at least in the nominal case (no i2c bus errors). > > The result is not very encouraging: Out of five (identical) designware i2c > controllers we have on my test SOC, the first one initialises properly but > the second one gets stuck in the famous irq loop right away when the > module is enabled in i2c_dw_init. The system never gets around to try Are you using the pci or platform driver? I noticed yesterday the pci version is failing here with a NULL pointer dereference. > initialising the remaining three controllers due to the irq loop. I didn't > have the time to investigate the details yet but I suspect this is > triggered by some nastily behaved device on the bus. For example, some of > our external devices are notorious for keeping i2c lines tied to zero > before being properly powered on/reset/initialised by their respective > drivers (which in turn depend on the i2c master to be initialised first, > obviously). I'll grab an oscilloscope and dump the waves to confirm this > suspicion on the occasion. Yeah, it'd be great to have it. thanks for testing it with your setup. -- Lucas De Marchi -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
