| Message ID | 1459478866-3896-1-git-send-email-lucas.de.marchi@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Lucas, Sorry for the late reply but I had to put our test environment back together to check this patch. I'll keep it around for a while in case you have further iterations to test. On 2016-04-01 04:47, Lucas De Marchi wrote: > From: Lucas De Marchi <lucas.demarchi@intel.com> > > Disabling the adapter after each transfer is pretty bad for sensors and > other devices doing small transfers at a high rate. It slows down the > transfer rate a lot since each of them have to wait the adapter to be > enabled again. > > It was done in order to avoid the adapter to generate interrupts when > it's not being used. Instead of doing that here we just disable the > interrupt generation in the controller. With a small program test to > read/write registers in a sensor the speed doubled. Example below with > write sequences of 16 bytes: > > Before: > i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=20000 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=20000 > transfer_time_avg=470.256050us > > During the init we check the status register for no activity and TX > buffer being empty since otherwise we can't change IC_TAR dynamically. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > > Mika / Christian, > > This is a second approach to a patch sent last year: > https://patchwork.ozlabs.org/patch/481952/ > > I hope that now it's in a better form. This has been tested on a Baytrail soc > (MinnowBoard Turbot) and is working. Would be nice to know if it also works on > Christian's machine since it was the one giving problems. Christian, could you > give this a try? It has been tested on top of 4.4.6 (+ some backports) and > 4.6.0-rc1. On Linux-4.6.0-rc2 the behaviour is still the same: The kernel locks up in an irq loop at dwi2c driver probe time. If I don't apply the patch everything works fine and the system boots and talks normally on the i2c bus. One solution might be to add a device-tree (and acpi) flag to tell the driver that it does not need to expect any nastily behaved devices on the bus. If this flag is given, the driver could use the faster disable-interrupt strategy. Without the flag we need to fall back to the conservative disable-i2c-controller strategy. I think such a flag should be OK because I2C buses are generally quite static and the list of devices should be known at system integration time. In the rare cases where this is not true, users could still go with the conservative approach. I know that the "cleaner" method would be to rather mark nasty devices, either in device tree or in the driver, but I guess the required changes in the I2C framework might be overkill for this dwi2c specific problem. Greetings, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGkgQ2hyaXN0aWFuLA0KDQpPbiBUaHUsIDIwMTYtMDQtMDcgYXQgMTU6MzcgKzAyMDAsIENocmlz dGlhbiBSdXBwZXJ0IHdyb3RlOg0KPiBEZWFyIEx1Y2FzLA0KPiANCj4gU29ycnkgZm9yIHRoZSBs YXRlIHJlcGx5IGJ1dCBJIGhhZCB0byBwdXQgb3VyIHRlc3QgZW52aXJvbm1lbnQgYmFjaw0KPiB0 b2dldGhlciB0byBjaGVjayB0aGlzIHBhdGNoLiBJJ2xsIGtlZXAgaXQgYXJvdW5kIGZvciBhIHdo aWxlIGluIGNhc2UNCj4geW91IGhhdmUgZnVydGhlciBpdGVyYXRpb25zIHRvIHRlc3QuDQoNCm5w LCBJJ2xsIHRyeSB0byBpdGVyYXRlIGZhc3RlciBvbiB0aGlzIHBhdGNoIG5vdywgdG9vLg0KDQo+ IE9uIExpbnV4LTQuNi4wLXJjMiB0aGUgYmVoYXZpb3VyIGlzIHN0aWxsIHRoZSBzYW1lOiBUaGUg a2VybmVsIGxvY2tzIHVwDQo+IGluIGFuIGlycSBsb29wIGF0IGR3aTJjIGRyaXZlciBwcm9iZSB0 aW1lLiBJZiBJIGRvbid0IGFwcGx5IHRoZSBwYXRjaA0KPiBldmVyeXRoaW5nIHdvcmtzIGZpbmUg YW5kIHRoZSBzeXN0ZW0gYm9vdHMgYW5kIHRhbGtzIG5vcm1hbGx5IG9uIHRoZSBpMmMNCj4gYnVz Lg0KDQo6KA0KDQpJIHJlYWxseSBob3BlZCB0aGlzIHdvdWxkIHdvcmsgaW4geW91ciBjYXNlLg0K DQo+IE9uZSBzb2x1dGlvbiBtaWdodCBiZSB0byBhZGQgYSBkZXZpY2UtdHJlZSAoYW5kIGFjcGkp IGZsYWcgdG8gdGVsbCB0aGUNCj4gZHJpdmVyIHRoYXQgaXQgZG9lcyBub3QgbmVlZCB0byBleHBl Y3QgYW55IG5hc3RpbHkgYmVoYXZlZCBkZXZpY2VzIG9uDQo+IHRoZSBidXMuIElmIHRoaXMgZmxh ZyBpcyBnaXZlbiwgdGhlIGRyaXZlciBjb3VsZCB1c2UgdGhlIGZhc3Rlcg0KPiBkaXNhYmxlLWlu dGVycnVwdCBzdHJhdGVneS4gV2l0aG91dCB0aGUgZmxhZyB3ZSBuZWVkIHRvIGZhbGwgYmFjayB0 byB0aGUNCj4gY29uc2VydmF0aXZlIGRpc2FibGUtaTJjLWNvbnRyb2xsZXIgc3RyYXRlZ3kuDQoN CkknZCBsaWtlIHRvIHRyeSBzb21lIG90aGVyIGFwcHJvYWNoZXMgYmVmb3JlIHRoYXQuIFdoYXQg aWYgd2Ugc3RhcnQgd2l0aCBpdA0KZGlzYWJsZWQgYW5kIGVuYWJsZSBhdCBmaXJzdCB1c2U/IEFm dGVyIHRoYXQgd2Uga2VlcCB0aGUgYXBwcm9hY2ggb2YganVzdA0KZGlzYWJsaW5nIHRoZSBpbnRl cnJ1cHRzLiDCoEkgY2FuIHByZXAgYSBwYXRjaCBmb3IgdGhhdC4NCg0KDQo+IEkgdGhpbmsgc3Vj aCBhIGZsYWcgc2hvdWxkIGJlIE9LIGJlY2F1c2UgSTJDIGJ1c2VzIGFyZSBnZW5lcmFsbHkgcXVp dGUNCj4gc3RhdGljIGFuZCB0aGUgbGlzdCBvZiBkZXZpY2VzIHNob3VsZCBiZSBrbm93biBhdCBz eXN0ZW0gaW50ZWdyYXRpb24NCj4gdGltZS4gSW4gdGhlIHJhcmUgY2FzZXMgd2hlcmUgdGhpcyBp cyBub3QgdHJ1ZSwgdXNlcnMgY291bGQgc3RpbGwgZ28NCj4gd2l0aCB0aGUgY29uc2VydmF0aXZl IGFwcHJvYWNoLiBJIGtub3cgdGhhdCB0aGUgImNsZWFuZXIiIG1ldGhvZCB3b3VsZA0KPiBiZSB0 byByYXRoZXIgbWFyayBuYXN0eSBkZXZpY2VzLCBlaXRoZXIgaW4gZGV2aWNlIHRyZWUgb3IgaW4g dGhlIGRyaXZlciwNCj4gYnV0IEkgZ3Vlc3MgdGhlIHJlcXVpcmVkIGNoYW5nZXMgaW4gdGhlIEky QyBmcmFtZXdvcmsgbWlnaHQgYmUgb3ZlcmtpbGwNCj4gZm9yIHRoaXMgZHdpMmMgc3BlY2lmaWMg cHJvYmxlbS4NCg0KYWdyZWVkDQoNCnRoYW5rcw0KTHVjYXMgRGUgTWFyY2hp -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 04/01/2016 05:47 AM, Lucas De Marchi wrote: > From: Lucas De Marchi <lucas.demarchi@intel.com> > > Disabling the adapter after each transfer is pretty bad for sensors and > other devices doing small transfers at a high rate. It slows down the > transfer rate a lot since each of them have to wait the adapter to be > enabled again. > > It was done in order to avoid the adapter to generate interrupts when > it's not being used. Instead of doing that here we just disable the > interrupt generation in the controller. With a small program test to > read/write registers in a sensor the speed doubled. Example below with > write sequences of 16 bytes: > > Before: > i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=20000 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=20000 > transfer_time_avg=470.256050us > I gave a test to this patch and saw similar improvements when dumping large set of registers from I2C connected audio codec. echo Y > /sys/kernel/debug/regmap/i2c-10EC5640\:00/cache_bypass time cat /sys/kernel/debug/regmap/i2c-10EC5640\:00/registers >/dev/null I checked the runtime PM status of adapter was suspended before running above cat command in order to get comparable results. Before patch time was ~0.55 - ~0.76 s and with patch applied time was ~0.16 - ~0.25 s. Hopefully we'll find how to prevent regression on Christian's machines.
On 2016-04-07 19:28, De Marchi, Lucas wrote: > Hi Christian, > > On Thu, 2016-04-07 at 15:37 +0200, Christian Ruppert wrote: >> Dear Lucas, >> >> Sorry for the late reply but I had to put our test environment back >> together to check this patch. I'll keep it around for a while in case >> you have further iterations to test. > > np, I'll try to iterate faster on this patch now, too. > >> On Linux-4.6.0-rc2 the behaviour is still the same: The kernel locks up >> in an irq loop at dwi2c driver probe time. If I don't apply the patch >> everything works fine and the system boots and talks normally on the i2c >> bus. > > :( > > I really hoped this would work in your case. > >> One solution might be to add a device-tree (and acpi) flag to tell the >> driver that it does not need to expect any nastily behaved devices on >> the bus. If this flag is given, the driver could use the faster >> disable-interrupt strategy. Without the flag we need to fall back to the >> conservative disable-i2c-controller strategy. > > I'd like to try some other approaches before that. What if we start with it > disabled and enable at first use? I think this might work in our case and be worth a try. When thinking about it, it might even be cleaner to add a way to specify a list of reset pins (e.g. through the GPIO reset framework) to trigger before activating the bus. This would allow for more than one badly behaved devices on the bus and also render everything more independent of the probe order. I'm afraid that the general case (bad device behaviour after the first transfer) still cannot be covered by this strategy but I'm not sure if I have a way to test this. > After that we keep the approach of just > disabling the interrupts. I can prep a patch for that. OK, I'll give it a try when it's ready. Greetings, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 99b54be..f8e6f2b 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -90,6 +90,7 @@ DW_IC_INTR_STOP_DET) #define DW_IC_STATUS_ACTIVITY 0x1 +#define DW_IC_STATUS_TX_EMPTY 0x2 #define DW_IC_ERR_TX_ABRT 0x1 @@ -383,6 +384,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev) /* configure the i2c master */ dw_writel(dev, dev->master_cfg , DW_IC_CON); + __i2c_dw_enable(dev, true); + if (dev->release_lock) dev->release_lock(dev); return 0; @@ -412,9 +415,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) { struct i2c_msg *msgs = dev->msgs; u32 ic_con, ic_tar = 0; - - /* Disable the adapter */ - __i2c_dw_enable(dev, false); + bool need_reenable = false; + u32 ic_status; + + /* check ic_tar and ic_con can be dynamically updated */ + ic_status = dw_readl(dev, DW_IC_STATUS); + if (ic_status & DW_IC_STATUS_ACTIVITY + || !(ic_status & DW_IC_STATUS_TX_EMPTY)) { + need_reenable = true; + __i2c_dw_enable(dev, false); + } /* if the slave address is ten bit address, enable 10BITADDR */ ic_con = dw_readl(dev, DW_IC_CON); @@ -442,8 +452,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) /* enforce disabled interrupts (due to HW issues) */ i2c_dw_disable_int(dev); - /* Enable the adapter */ - __i2c_dw_enable(dev, true); + if (need_reenable) + __i2c_dw_enable(dev, true); /* Clear and enable interrupts */ dw_readl(dev, DW_IC_CLR_INTR); @@ -624,7 +634,8 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) } /* - * Prepare controller for a transaction and call i2c_dw_xfer_msg + * Prepare controller for a transaction and start transfer by calling + * i2c_dw_xfer_init() */ static int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) @@ -665,22 +676,11 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) /* wait for tx to complete */ if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) { dev_err(dev->dev, "controller timed out\n"); - /* i2c_dw_init implicitly disables the adapter */ i2c_dw_init(dev); ret = -ETIMEDOUT; goto done; } - /* - * We must disable the adapter before returning and signaling the end - * of the current transfer. 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); - if (dev->msg_err) { ret = dev->msg_err; goto done; @@ -818,9 +818,21 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) */ tx_aborted: - if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err) + if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) + || dev->msg_err) { + /* + * We must disable interruts before returning and signaling + * the end of the current transfer. Otherwise the hardware + * might continue generating interrupts which in turn causes a + * race condition with next transfer. Needs some more + * investigation if the additional interrupts are a hardware + * bug or this driver doesn't handle them correctly yet. + */ + i2c_dw_disable_int(dev); + dw_readl(dev, DW_IC_CLR_INTR); + complete(&dev->cmd_complete); - else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) { + } else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) { /* workaround to trigger pending interrupt */ stat = dw_readl(dev, DW_IC_INTR_MASK); i2c_dw_disable_int(dev);