| Message ID | 1440469235-2328-1-git-send-email-b54642@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
Am 25.08.2015 um 04:20 schrieb Gao Pan: > In our former i2c driver, i2c clk is enabled and disabled in > xfer function, which contributes to power saving. However, > the clk enable process brings a busy wait delay until the core > is stable. As a result, the performance is sacrificed. > > To weigh the power consumption and i2c bus performance, runtime > pm is the good solution for it. The clk is enabled when a i2c > transfer starts, and disabled after a specifically defined delay. > > Without the patch the test case (many eeprom reads) executes with approx: > real 1m7.735s > user 0m0.488s > sys 0m20.040s > > With the patch the same test case (many eeprom reads) executes with approx: > real 0m54.241s > user 0m0.440s > sys 0m5.920s > > Signed-off-by: Fugang Duan <B38611@freescale.com> > Signed-off-by: Gao Pan <b54642@freescale.com> > [wsa: fixed some indentation] > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > --- > V2: > As Uwe Kleine-König's suggestion, the version do below changes: > -call clk_prepare_enable in probe to avoid never enabling clock > if CONFIG_PM is disabled > -enable clock before request IRQ in probe > -remove the pm staff in i2c_imx_isr > > V3: > -pm_runtime_get_sync returns < 0 as error > > V4: > -add pm_runtime_set_active before pm_runtime_enable > -replace pm_runtime_put_autosuspend with pm_runtime_autosuspend > in probe > -add error disposal when i2c_add_numbered_adapter fails > > drivers/i2c/busses/i2c-imx.c | 79 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 68 insertions(+), 11 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 785aa67..7c63047 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -53,6 +53,7 @@ > #include <linux/platform_device.h> > #include <linux/sched.h> > #include <linux/slab.h> > +#include <linux/pm_runtime.h> > > /** Defines ******************************************************************** > *******************************************************************************/ > @@ -118,6 +119,8 @@ > #define I2CR_IEN_OPCODE_0 0x0 > #define I2CR_IEN_OPCODE_1 I2CR_IEN > > +#define I2C_PM_TIMEOUT 10 /* ms */ > + > /** Variables ****************************************************************** > *******************************************************************************/ > > @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) > > i2c_imx_set_clk(i2c_imx); > > - result = clk_prepare_enable(i2c_imx->clk); > - if (result) > - return result; > imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); > /* Enable I2C controller */ > imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR); > @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) > /* Disable I2C controller */ > temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - clk_disable_unprepare(i2c_imx->clk); > } > > static irqreturn_t i2c_imx_isr(int irq, void *dev_id) > @@ -894,6 +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); > > + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > + if (result < 0) > + goto out; > + > /* Start I2C transfer */ > result = i2c_imx_start(i2c_imx); > if (result) > @@ -950,6 +953,10 @@ fail0: > /* Stop I2C transfer */ > i2c_imx_stop(i2c_imx); > > +out: > + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > + > dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, > (result < 0) ? "error" : "success msg", > (result < 0) ? result : num); > @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct platform_device *pdev) > > ret = clk_prepare_enable(i2c_imx->clk); > if (ret) { > - dev_err(&pdev->dev, "can't enable I2C clock\n"); > + dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret); > return ret; > } > + > /* Request IRQ */ > ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, > pdev->name, i2c_imx); > @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct platform_device *pdev) > /* Set up adapter data */ > i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); > > + /* Set up platform driver data */ > + platform_set_drvdata(pdev, i2c_imx); > + > + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + > /* Set up clock divider */ > i2c_imx->bitrate = IMX_I2C_BIT_RATE; > ret = of_property_read_u32(pdev->dev.of_node, > @@ -1053,12 +1069,13 @@ static int i2c_imx_probe(struct platform_device *pdev) > ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > if (ret < 0) { > dev_err(&pdev->dev, "registration failed\n"); > + pm_runtime_mark_last_busy(&pdev->dev); > + pm_runtime_autosuspend(&pdev->dev); It would be more consistent with the general practice not to clean up here but to define a new error label, goto there and do the clean-up there. And if adding the adapter fails most likely you don't want to autosuspend but clean up and disable runtime PM. > goto clk_disable; > } > > - /* Set up platform driver data */ > - platform_set_drvdata(pdev, i2c_imx); > - clk_disable_unprepare(i2c_imx->clk); > + pm_runtime_mark_last_busy(&pdev->dev); > + pm_runtime_autosuspend(&pdev->dev); >From enabling runtime PM to here you don't hold a reference so theoretically it might happen that the device gets suspended in-between. I'm not able to say whether this is an actual potential issue here. However to be on the safe side it wouldn't hurt to do a pm_runtime_get_xx when enabling PM and use pm_runtime_put_autosuspend here. Regards, Heiner > > dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq); > dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res); > @@ -1079,6 +1096,11 @@ clk_disable: > static int i2c_imx_remove(struct platform_device *pdev) > { > struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); > + int ret; > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) > + return ret; > > /* remove adapter */ > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); > @@ -1093,17 +1115,52 @@ static int i2c_imx_remove(struct platform_device *pdev) > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR); > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); > > + pm_runtime_put_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + > return 0; > } > > +#ifdef CONFIG_PM > +static int i2c_imx_runtime_suspend(struct device *dev) > +{ > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > + > + clk_disable_unprepare(i2c_imx->clk); > + > + return 0; > +} > + > +static int i2c_imx_runtime_resume(struct device *dev) > +{ > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > + int ret; > + > + ret = clk_prepare_enable(i2c_imx->clk); > + if (ret) > + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > + > + return ret; > +} > + > +static const struct dev_pm_ops i2c_imx_pm_ops = { > + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, > + i2c_imx_runtime_resume, NULL) > +}; > +#define I2C_IMX_PM_OPS (&i2c_imx_pm_ops) > +#else > +#define I2C_IMX_PM_OPS NULL > +#endif /* CONFIG_PM */ > + > static struct platform_driver i2c_imx_driver = { > .probe = i2c_imx_probe, > .remove = i2c_imx_remove, > - .driver = { > - .name = DRIVER_NAME, > + .driver = { > + .name = DRIVER_NAME, > + .pm = I2C_IMX_PM_OPS, > .of_match_table = i2c_imx_dt_ids, > }, > - .id_table = imx_i2c_devtype, > + .id_table = imx_i2c_devtype, > }; > > static int __init i2c_adap_imx_init(void) > -- 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
RnJvbTogSGVpbmVyIEthbGx3ZWl0IDxtYWlsdG86aGthbGx3ZWl0MUBnbWFpbC5jb20+IFNlbnQ6 IFR1ZXNkYXksIEF1Z3VzdCAyNSwgMjAxNSAyOjM3IFBNDQo+IFRvOiBHYW8gUGFuLUI1NDY0Mjsg d3NhQHRoZS1kcmVhbXMuZGUNCj4gQ2M6IGxpbnV4LWkyY0B2Z2VyLmtlcm5lbC5vcmc7IExpIEZy YW5rLUIyMDU5NjsgRHVhbiBGdWdhbmctQjM4NjExOw0KPiB1LmtsZWluZS1rb2VuaWdAcGVuZ3V0 cm9uaXguZGU7IGtlcm5lbEBwZW5ndXRyb25peC5kZQ0KPiBTdWJqZWN0OiBSZTogW1BhdGNoIFY0 XSBpMmM6IGlteDogYWRkIHJ1bnRpbWUgcG0gc3VwcG9ydCB0byBpbXByb3ZlIHRoZQ0KPiBwZXJm b3JtYW5jZQ0KPiANCj4gQW0gMjUuMDguMjAxNSB1bSAwNDoyMCBzY2hyaWViIEdhbyBQYW46DQo+ ID4gSW4gb3VyIGZvcm1lciBpMmMgZHJpdmVyLCBpMmMgY2xrIGlzIGVuYWJsZWQgYW5kIGRpc2Fi bGVkIGluIHhmZXINCj4gPiBmdW5jdGlvbiwgd2hpY2ggY29udHJpYnV0ZXMgdG8gcG93ZXIgc2F2 aW5nLiBIb3dldmVyLCB0aGUgY2xrIGVuYWJsZQ0KPiA+IHByb2Nlc3MgYnJpbmdzIGEgYnVzeSB3 YWl0IGRlbGF5IHVudGlsIHRoZSBjb3JlIGlzIHN0YWJsZS4gQXMgYQ0KPiA+IHJlc3VsdCwgdGhl IHBlcmZvcm1hbmNlIGlzIHNhY3JpZmljZWQuDQo+ID4NCj4gPiBUbyB3ZWlnaCB0aGUgcG93ZXIg Y29uc3VtcHRpb24gYW5kIGkyYyBidXMgcGVyZm9ybWFuY2UsIHJ1bnRpbWUgcG0gaXMNCj4gPiB0 aGUgZ29vZCBzb2x1dGlvbiBmb3IgaXQuIFRoZSBjbGsgaXMgZW5hYmxlZCB3aGVuIGEgaTJjIHRy YW5zZmVyDQo+ID4gc3RhcnRzLCBhbmQgZGlzYWJsZWQgYWZ0ZXIgYSBzcGVjaWZpY2FsbHkgZGVm aW5lZCBkZWxheS4NCj4gPg0KPiA+IFdpdGhvdXQgdGhlIHBhdGNoIHRoZSB0ZXN0IGNhc2UgKG1h bnkgZWVwcm9tIHJlYWRzKSBleGVjdXRlcyB3aXRoDQo+IGFwcHJveDoNCj4gPiByZWFsIDFtNy43 MzVzDQo+ID4gdXNlciAwbTAuNDg4cw0KPiA+IHN5cyAwbTIwLjA0MHMNCj4gPg0KPiA+IFdpdGgg dGhlIHBhdGNoIHRoZSBzYW1lIHRlc3QgY2FzZSAobWFueSBlZXByb20gcmVhZHMpIGV4ZWN1dGVz IHdpdGgNCj4gYXBwcm94Og0KPiA+IHJlYWwgMG01NC4yNDFzDQo+ID4gdXNlciAwbTAuNDQwcw0K PiA+IHN5cyAwbTUuOTIwcw0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogRnVnYW5nIER1YW4gPEIz ODYxMUBmcmVlc2NhbGUuY29tPg0KPiA+IFNpZ25lZC1vZmYtYnk6IEdhbyBQYW4gPGI1NDY0MkBm cmVlc2NhbGUuY29tPg0KPiA+IFt3c2E6IGZpeGVkIHNvbWUgaW5kZW50YXRpb25dDQo+ID4gU2ln bmVkLW9mZi1ieTogV29sZnJhbSBTYW5nIDx3c2FAdGhlLWRyZWFtcy5kZT4NCj4gPiAtLS0NCj4g PiBWMjoNCj4gPiBBcyBVd2UgS2xlaW5lLUvDtm5pZydzIHN1Z2dlc3Rpb24sIHRoZSB2ZXJzaW9u IGRvIGJlbG93IGNoYW5nZXM6DQo+ID4gIC1jYWxsIGNsa19wcmVwYXJlX2VuYWJsZSBpbiBwcm9i ZSB0byBhdm9pZCBuZXZlciBlbmFibGluZyBjbG9jaw0KPiA+ICAgaWYgQ09ORklHX1BNIGlzIGRp c2FibGVkDQo+ID4gIC1lbmFibGUgY2xvY2sgYmVmb3JlIHJlcXVlc3QgSVJRIGluIHByb2JlICAt cmVtb3ZlIHRoZSBwbSBzdGFmZiBpbg0KPiA+IGkyY19pbXhfaXNyDQo+ID4NCj4gPiBWMzoNCj4g PiAgLXBtX3J1bnRpbWVfZ2V0X3N5bmMgcmV0dXJucyA8IDAgYXMgZXJyb3INCj4gPg0KPiA+IFY0 Og0KPiA+ICAtYWRkIHBtX3J1bnRpbWVfc2V0X2FjdGl2ZSBiZWZvcmUgcG1fcnVudGltZV9lbmFi bGUgIC1yZXBsYWNlDQo+ID4gcG1fcnVudGltZV9wdXRfYXV0b3N1c3BlbmQgd2l0aCBwbV9ydW50 aW1lX2F1dG9zdXNwZW5kDQo+ID4gICBpbiBwcm9iZQ0KPiA+ICAtYWRkIGVycm9yIGRpc3Bvc2Fs IHdoZW4gaTJjX2FkZF9udW1iZXJlZF9hZGFwdGVyIGZhaWxzDQo+ID4NCj4gPiAgZHJpdmVycy9p MmMvYnVzc2VzL2kyYy1pbXguYyB8IDc5DQo+ID4gKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKysrKystLS0tLS0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDY4IGluc2VydGlvbnMoKyks IDExIGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaTJjL2J1c3Nl cy9pMmMtaW14LmMNCj4gPiBiL2RyaXZlcnMvaTJjL2J1c3Nlcy9pMmMtaW14LmMgaW5kZXggNzg1 YWE2Ny4uN2M2MzA0NyAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2kyYy9idXNzZXMvaTJjLWlt eC5jDQo+ID4gKysrIGIvZHJpdmVycy9pMmMvYnVzc2VzL2kyYy1pbXguYw0KPiA+IEBAIC01Myw2 ICs1Myw3IEBADQo+ID4gICNpbmNsdWRlIDxsaW51eC9wbGF0Zm9ybV9kZXZpY2UuaD4NCj4gPiAg I2luY2x1ZGUgPGxpbnV4L3NjaGVkLmg+DQo+ID4gICNpbmNsdWRlIDxsaW51eC9zbGFiLmg+DQo+ ID4gKyNpbmNsdWRlIDxsaW51eC9wbV9ydW50aW1lLmg+DQo+ID4NCj4gPiAgLyoqIERlZmluZXMN Cj4gPiAqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioq KioqKioqKioqKioqKioqKg0KPiA+DQo+ID4gKioqKioqKioqKioqKioqKioqKioqKioqKioqKioq KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKg0KPiA+ICoqKioqKioqKi8N Cj4gPiBAQCAtMTE4LDYgKzExOSw4IEBADQo+ID4gICNkZWZpbmUgSTJDUl9JRU5fT1BDT0RFXzAJ MHgwDQo+ID4gICNkZWZpbmUgSTJDUl9JRU5fT1BDT0RFXzEJSTJDUl9JRU4NCj4gPg0KPiA+ICsj ZGVmaW5lIEkyQ19QTV9USU1FT1VUCQkxMCAvKiBtcyAqLw0KPiA+ICsNCj4gPiAgLyoqIFZhcmlh Ymxlcw0KPiA+ICoqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioq KioqKioqKioqKioqKioqKioqKg0KPiA+DQo+ID4gKioqKioqKioqKioqKioqKioqKioqKioqKioq KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKg0KPiA+ICoqKioqKioq Ki8NCj4gPg0KPiA+IEBAIC01MjAsOSArNTIzLDYgQEAgc3RhdGljIGludCBpMmNfaW14X3N0YXJ0 KHN0cnVjdCBpbXhfaTJjX3N0cnVjdA0KPiA+ICppMmNfaW14KQ0KPiA+DQo+ID4gIAlpMmNfaW14 X3NldF9jbGsoaTJjX2lteCk7DQo+ID4NCj4gPiAtCXJlc3VsdCA9IGNsa19wcmVwYXJlX2VuYWJs ZShpMmNfaW14LT5jbGspOw0KPiA+IC0JaWYgKHJlc3VsdCkNCj4gPiAtCQlyZXR1cm4gcmVzdWx0 Ow0KPiA+ICAJaW14X2kyY193cml0ZV9yZWcoaTJjX2lteC0+aWZkciwgaTJjX2lteCwgSU1YX0ky Q19JRkRSKTsNCj4gPiAgCS8qIEVuYWJsZSBJMkMgY29udHJvbGxlciAqLw0KPiA+ICAJaW14X2ky Y193cml0ZV9yZWcoaTJjX2lteC0+aHdkYXRhLT5pMnNyX2Nscl9vcGNvZGUsIGkyY19pbXgsDQo+ ID4gSU1YX0kyQ19JMlNSKTsgQEAgLTU3NSw3ICs1NzUsNiBAQCBzdGF0aWMgdm9pZCBpMmNfaW14 X3N0b3Aoc3RydWN0DQo+IGlteF9pMmNfc3RydWN0ICppMmNfaW14KQ0KPiA+ICAJLyogRGlzYWJs ZSBJMkMgY29udHJvbGxlciAqLw0KPiA+ICAJdGVtcCA9IGkyY19pbXgtPmh3ZGF0YS0+aTJjcl9p ZW5fb3Bjb2RlIF4gSTJDUl9JRU4sDQo+ID4gIAlpbXhfaTJjX3dyaXRlX3JlZyh0ZW1wLCBpMmNf aW14LCBJTVhfSTJDX0kyQ1IpOw0KPiA+IC0JY2xrX2Rpc2FibGVfdW5wcmVwYXJlKGkyY19pbXgt PmNsayk7DQo+ID4gIH0NCj4gPg0KPiA+ICBzdGF0aWMgaXJxcmV0dXJuX3QgaTJjX2lteF9pc3Io aW50IGlycSwgdm9pZCAqZGV2X2lkKSBAQCAtODk0LDYNCj4gPiArODkzLDEwIEBAIHN0YXRpYyBp bnQgaTJjX2lteF94ZmVyKHN0cnVjdCBpMmNfYWRhcHRlciAqYWRhcHRlciwNCj4gPg0KPiA+ICAJ ZGV2X2RiZygmaTJjX2lteC0+YWRhcHRlci5kZXYsICI8JXM+XG4iLCBfX2Z1bmNfXyk7DQo+ID4N Cj4gPiArCXJlc3VsdCA9IHBtX3J1bnRpbWVfZ2V0X3N5bmMoaTJjX2lteC0+YWRhcHRlci5kZXYu cGFyZW50KTsNCj4gPiArCWlmIChyZXN1bHQgPCAwKQ0KPiA+ICsJCWdvdG8gb3V0Ow0KPiA+ICsN Cj4gPiAgCS8qIFN0YXJ0IEkyQyB0cmFuc2ZlciAqLw0KPiA+ICAJcmVzdWx0ID0gaTJjX2lteF9z dGFydChpMmNfaW14KTsNCj4gPiAgCWlmIChyZXN1bHQpDQo+ID4gQEAgLTk1MCw2ICs5NTMsMTAg QEAgZmFpbDA6DQo+ID4gIAkvKiBTdG9wIEkyQyB0cmFuc2ZlciAqLw0KPiA+ICAJaTJjX2lteF9z dG9wKGkyY19pbXgpOw0KPiA+DQo+ID4gK291dDoNCj4gPiArCXBtX3J1bnRpbWVfbWFya19sYXN0 X2J1c3koaTJjX2lteC0+YWRhcHRlci5kZXYucGFyZW50KTsNCj4gPiArCXBtX3J1bnRpbWVfcHV0 X2F1dG9zdXNwZW5kKGkyY19pbXgtPmFkYXB0ZXIuZGV2LnBhcmVudCk7DQo+ID4gKw0KPiA+ICAJ ZGV2X2RiZygmaTJjX2lteC0+YWRhcHRlci5kZXYsICI8JXM+IGV4aXQgd2l0aDogJXM6ICVkXG4i LCBfX2Z1bmNfXywNCj4gPiAgCQkocmVzdWx0IDwgMCkgPyAiZXJyb3IiIDogInN1Y2Nlc3MgbXNn IiwNCj4gPiAgCQkJKHJlc3VsdCA8IDApID8gcmVzdWx0IDogbnVtKTsNCj4gPiBAQCAtMTAyMCw5 ICsxMDI3LDEwIEBAIHN0YXRpYyBpbnQgaTJjX2lteF9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2 aWNlDQo+ID4gKnBkZXYpDQo+ID4NCj4gPiAgCXJldCA9IGNsa19wcmVwYXJlX2VuYWJsZShpMmNf aW14LT5jbGspOw0KPiA+ICAJaWYgKHJldCkgew0KPiA+IC0JCWRldl9lcnIoJnBkZXYtPmRldiwg ImNhbid0IGVuYWJsZSBJMkMgY2xvY2tcbiIpOw0KPiA+ICsJCWRldl9lcnIoJnBkZXYtPmRldiwg ImNhbid0IGVuYWJsZSBJMkMgY2xvY2ssIHJldD0lZFxuIiwgcmV0KTsNCj4gPiAgCQlyZXR1cm4g cmV0Ow0KPiA+ICAJfQ0KPiA+ICsNCj4gPiAgCS8qIFJlcXVlc3QgSVJRICovDQo+ID4gIAlyZXQg PSBkZXZtX3JlcXVlc3RfaXJxKCZwZGV2LT5kZXYsIGlycSwgaTJjX2lteF9pc3IsIDAsDQo+ID4g IAkJCQlwZGV2LT5uYW1lLCBpMmNfaW14KTsNCj4gPiBAQCAtMTAzNyw2ICsxMDQ1LDE0IEBAIHN0 YXRpYyBpbnQgaTJjX2lteF9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlDQo+ICpwZGV2KQ0K PiA+ICAJLyogU2V0IHVwIGFkYXB0ZXIgZGF0YSAqLw0KPiA+ICAJaTJjX3NldF9hZGFwZGF0YSgm aTJjX2lteC0+YWRhcHRlciwgaTJjX2lteCk7DQo+ID4NCj4gPiArCS8qIFNldCB1cCBwbGF0Zm9y bSBkcml2ZXIgZGF0YSAqLw0KPiA+ICsJcGxhdGZvcm1fc2V0X2RydmRhdGEocGRldiwgaTJjX2lt eCk7DQo+ID4gKw0KPiA+ICsJcG1fcnVudGltZV9zZXRfYXV0b3N1c3BlbmRfZGVsYXkoJnBkZXYt PmRldiwgSTJDX1BNX1RJTUVPVVQpOw0KPiA+ICsJcG1fcnVudGltZV91c2VfYXV0b3N1c3BlbmQo JnBkZXYtPmRldik7DQo+ID4gKwlwbV9ydW50aW1lX3NldF9hY3RpdmUoJnBkZXYtPmRldik7DQo+ ID4gKwlwbV9ydW50aW1lX2VuYWJsZSgmcGRldi0+ZGV2KTsNCj4gPiArDQo+ID4gIAkvKiBTZXQg dXAgY2xvY2sgZGl2aWRlciAqLw0KPiA+ICAJaTJjX2lteC0+Yml0cmF0ZSA9IElNWF9JMkNfQklU X1JBVEU7DQo+ID4gIAlyZXQgPSBvZl9wcm9wZXJ0eV9yZWFkX3UzMihwZGV2LT5kZXYub2Zfbm9k ZSwNCj4gPiBAQCAtMTA1MywxMiArMTA2OSwxMyBAQCBzdGF0aWMgaW50IGkyY19pbXhfcHJvYmUo c3RydWN0IHBsYXRmb3JtX2RldmljZQ0KPiAqcGRldikNCj4gPiAgCXJldCA9IGkyY19hZGRfbnVt YmVyZWRfYWRhcHRlcigmaTJjX2lteC0+YWRhcHRlcik7DQo+ID4gIAlpZiAocmV0IDwgMCkgew0K PiA+ICAJCWRldl9lcnIoJnBkZXYtPmRldiwgInJlZ2lzdHJhdGlvbiBmYWlsZWRcbiIpOw0KPiA+ ICsJCXBtX3J1bnRpbWVfbWFya19sYXN0X2J1c3koJnBkZXYtPmRldik7DQo+ID4gKwkJcG1fcnVu dGltZV9hdXRvc3VzcGVuZCgmcGRldi0+ZGV2KTsNCj4gSXQgd291bGQgYmUgbW9yZSBjb25zaXN0 ZW50IHdpdGggdGhlIGdlbmVyYWwgcHJhY3RpY2Ugbm90IHRvIGNsZWFuIHVwDQo+IGhlcmUgYnV0 IHRvIGRlZmluZSBhIG5ldyBlcnJvciBsYWJlbCwgZ290byB0aGVyZSBhbmQgZG8gdGhlIGNsZWFu LXVwDQo+IHRoZXJlLg0KPiBBbmQgaWYgYWRkaW5nIHRoZSBhZGFwdGVyIGZhaWxzIG1vc3QgbGlr ZWx5IHlvdSBkb24ndCB3YW50IHRvIGF1dG9zdXNwZW5kDQo+IGJ1dCBjbGVhbiB1cCBhbmQgZGlz YWJsZSBydW50aW1lIFBNLg0KIA0KVGhhbmsgeW91LCB3aWxsIGNoYW5nZSBpdCBpbiBuZXh0IHZl cnNpb24uDQoNCj4gPiAgCQlnb3RvIGNsa19kaXNhYmxlOw0KPiA+ICAJfQ0KPiA+DQo+ID4gLQkv KiBTZXQgdXAgcGxhdGZvcm0gZHJpdmVyIGRhdGEgKi8NCj4gPiAtCXBsYXRmb3JtX3NldF9kcnZk YXRhKHBkZXYsIGkyY19pbXgpOw0KPiA+IC0JY2xrX2Rpc2FibGVfdW5wcmVwYXJlKGkyY19pbXgt PmNsayk7DQo+ID4gKwlwbV9ydW50aW1lX21hcmtfbGFzdF9idXN5KCZwZGV2LT5kZXYpOw0KPiA+ ICsJcG1fcnVudGltZV9hdXRvc3VzcGVuZCgmcGRldi0+ZGV2KTsNCj4gPkZyb20gZW5hYmxpbmcg cnVudGltZSBQTSB0byBoZXJlIHlvdSBkb24ndCBob2xkIGEgcmVmZXJlbmNlIHNvDQo+ID50aGVv cmV0aWNhbGx5DQo+IGl0IG1pZ2h0IGhhcHBlbiB0aGF0IHRoZSBkZXZpY2UgZ2V0cyBzdXNwZW5k ZWQgaW4tYmV0d2Vlbi4NCj4gSSdtIG5vdCBhYmxlIHRvIHNheSB3aGV0aGVyIHRoaXMgaXMgYW4g YWN0dWFsIHBvdGVudGlhbCBpc3N1ZSBoZXJlLg0KPiBIb3dldmVyIHRvIGJlIG9uIHRoZSBzYWZl IHNpZGUgaXQgd291bGRuJ3QgaHVydCB0byBkbyBhIHBtX3J1bnRpbWVfZ2V0X3h4DQo+IHdoZW4g ZW5hYmxpbmcgUE0gYW5kIHVzZSBwbV9ydW50aW1lX3B1dF9hdXRvc3VzcGVuZCBoZXJlLg0KDQpU aGFua3MuIElmIEkgdXNlIHBtX3J1bnRpbWVfZ2V0X3h4IHdoZW4gZW5hYmxpbmcgUE0sIGNsa19w cmVwYXJlX2VuYWJsZSB3aWxsIGJlIGNhbGxlZCBhZ2Fpbi4gDQpIb3dldmVyLCBpdCBoYXMgYmVl biBjYWxsZWQgYmVmb3JlLiBJdCdzIGtpbmQgb2YgY29kZSByZWR1bmRhbmN5IHRvIHVzZSBwbV9y dW50aW1lX2dldF94eC4NCg0KPiBSZWdhcmRzLCBIZWluZXINCj4gPg0KPiA+ICAJZGV2X2RiZygm aTJjX2lteC0+YWRhcHRlci5kZXYsICJjbGFpbWVkIGlycSAlZFxuIiwgaXJxKTsNCj4gPiAgCWRl dl9kYmcoJmkyY19pbXgtPmFkYXB0ZXIuZGV2LCAiZGV2aWNlIHJlc291cmNlczogJXBSXG4iLCBy ZXMpOyBAQA0KPiA+IC0xMDc5LDYgKzEwOTYsMTEgQEAgY2xrX2Rpc2FibGU6DQo+ID4gIHN0YXRp YyBpbnQgaTJjX2lteF9yZW1vdmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikgIHsNCj4g PiAgCXN0cnVjdCBpbXhfaTJjX3N0cnVjdCAqaTJjX2lteCA9IHBsYXRmb3JtX2dldF9kcnZkYXRh KHBkZXYpOw0KPiA+ICsJaW50IHJldDsNCj4gPiArDQo+ID4gKwlyZXQgPSBwbV9ydW50aW1lX2dl dF9zeW5jKCZwZGV2LT5kZXYpOw0KPiA+ICsJaWYgKHJldCA8IDApDQo+ID4gKwkJcmV0dXJuIHJl dDsNCj4gPg0KPiA+ICAJLyogcmVtb3ZlIGFkYXB0ZXIgKi8NCj4gPiAgCWRldl9kYmcoJmkyY19p bXgtPmFkYXB0ZXIuZGV2LCAiYWRhcHRlciByZW1vdmVkXG4iKTsgQEAgLTEwOTMsMTcNCj4gPiAr MTExNSw1MiBAQCBzdGF0aWMgaW50IGkyY19pbXhfcmVtb3ZlKHN0cnVjdCBwbGF0Zm9ybV9kZXZp Y2UgKnBkZXYpDQo+ID4gIAlpbXhfaTJjX3dyaXRlX3JlZygwLCBpMmNfaW14LCBJTVhfSTJDX0ky Q1IpOw0KPiA+ICAJaW14X2kyY193cml0ZV9yZWcoMCwgaTJjX2lteCwgSU1YX0kyQ19JMlNSKTsN Cj4gPg0KPiA+ICsJcG1fcnVudGltZV9wdXRfc3luYygmcGRldi0+ZGV2KTsNCj4gPiArCXBtX3J1 bnRpbWVfZGlzYWJsZSgmcGRldi0+ZGV2KTsNCj4gPiArDQo+ID4gIAlyZXR1cm4gMDsNCj4gPiAg fQ0KPiA+DQo+ID4gKyNpZmRlZiBDT05GSUdfUE0NCj4gPiArc3RhdGljIGludCBpMmNfaW14X3J1 bnRpbWVfc3VzcGVuZChzdHJ1Y3QgZGV2aWNlICpkZXYpIHsNCj4gPiArCXN0cnVjdCBpbXhfaTJj X3N0cnVjdCAqaTJjX2lteCAgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsNCj4gPiArDQo+ID4gKwlj bGtfZGlzYWJsZV91bnByZXBhcmUoaTJjX2lteC0+Y2xrKTsNCj4gPiArDQo+ID4gKwlyZXR1cm4g MDsNCj4gPiArfQ0KPiA+ICsNCj4gPiArc3RhdGljIGludCBpMmNfaW14X3J1bnRpbWVfcmVzdW1l KHN0cnVjdCBkZXZpY2UgKmRldikgew0KPiA+ICsJc3RydWN0IGlteF9pMmNfc3RydWN0ICppMmNf aW14ICA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOw0KPiA+ICsJaW50IHJldDsNCj4gPiArDQo+ID4g KwlyZXQgPSBjbGtfcHJlcGFyZV9lbmFibGUoaTJjX2lteC0+Y2xrKTsNCj4gPiArCWlmIChyZXQp DQo+ID4gKwkJZGV2X2VycihkZXYsICJjYW4ndCBlbmFibGUgSTJDIGNsb2NrLCByZXQ9JWRcbiIs IHJldCk7DQo+ID4gKw0KPiA+ICsJcmV0dXJuIHJldDsNCj4gPiArfQ0KPiA+ICsNCj4gPiArc3Rh dGljIGNvbnN0IHN0cnVjdCBkZXZfcG1fb3BzIGkyY19pbXhfcG1fb3BzID0gew0KPiA+ICsJU0VU X1JVTlRJTUVfUE1fT1BTKGkyY19pbXhfcnVudGltZV9zdXNwZW5kLA0KPiA+ICsJCQkgICBpMmNf aW14X3J1bnRpbWVfcmVzdW1lLCBOVUxMKQ0KPiA+ICt9Ow0KPiA+ICsjZGVmaW5lIEkyQ19JTVhf UE1fT1BTICgmaTJjX2lteF9wbV9vcHMpICNlbHNlICNkZWZpbmUgSTJDX0lNWF9QTV9PUFMNCj4g PiArTlVMTCAjZW5kaWYgLyogQ09ORklHX1BNICovDQo+ID4gKw0KPiA+ICBzdGF0aWMgc3RydWN0 IHBsYXRmb3JtX2RyaXZlciBpMmNfaW14X2RyaXZlciA9IHsNCj4gPiAgCS5wcm9iZSA9IGkyY19p bXhfcHJvYmUsDQo+ID4gIAkucmVtb3ZlID0gaTJjX2lteF9yZW1vdmUsDQo+ID4gLQkuZHJpdmVy CT0gew0KPiA+IC0JCS5uYW1lCT0gRFJJVkVSX05BTUUsDQo+ID4gKwkuZHJpdmVyID0gew0KPiA+ ICsJCS5uYW1lID0gRFJJVkVSX05BTUUsDQo+ID4gKwkJLnBtID0gSTJDX0lNWF9QTV9PUFMsDQo+ ID4gIAkJLm9mX21hdGNoX3RhYmxlID0gaTJjX2lteF9kdF9pZHMsDQo+ID4gIAl9LA0KPiA+IC0J LmlkX3RhYmxlCT0gaW14X2kyY19kZXZ0eXBlLA0KPiA+ICsJLmlkX3RhYmxlID0gaW14X2kyY19k ZXZ0eXBlLA0KPiA+ICB9Ow0KPiA+DQo+ID4gIHN0YXRpYyBpbnQgX19pbml0IGkyY19hZGFwX2lt eF9pbml0KHZvaWQpDQo+ID4NCg0K -- 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
> From: linux-i2c-owner@vger.kernel.org [mailto:linux-i2c- > owner@vger.kernel.org] On Behalf Of Gao Pandy > Sent: Tuesday, August 25, 2015 7:09 PM > To: Heiner Kallweit; wsa@the-dreams.de > Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; > u.kleine-koenig@pengutronix.de; kernel@pengutronix.de > Subject: RE: [Patch V4] i2c: imx: add runtime pm support to improve the > performance > > From: Heiner Kallweit <mailto:hkallweit1@gmail.com> Sent: Tuesday, August > 25, 2015 2:37 PM > > To: Gao Pan-B54642; wsa@the-dreams.de > > Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; > > u.kleine-koenig@pengutronix.de; kernel@pengutronix.de > > Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve > > the performance > > > > Am 25.08.2015 um 04:20 schrieb Gao Pan: > > > In our former i2c driver, i2c clk is enabled and disabled in xfer > > > function, which contributes to power saving. However, the clk enable > > > process brings a busy wait delay until the core is stable. As a > > > result, the performance is sacrificed. > > > > > > To weigh the power consumption and i2c bus performance, runtime pm > > > is the good solution for it. The clk is enabled when a i2c transfer > > > starts, and disabled after a specifically defined delay. > > > > > > Without the patch the test case (many eeprom reads) executes with > > approx: > > > real 1m7.735s > > > user 0m0.488s > > > sys 0m20.040s > > > > > > With the patch the same test case (many eeprom reads) executes with > > approx: > > > real 0m54.241s > > > user 0m0.440s > > > sys 0m5.920s > > > > > > Signed-off-by: Fugang Duan <B38611@freescale.com> > > > Signed-off-by: Gao Pan <b54642@freescale.com> > > > [wsa: fixed some indentation] > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > > --- > > > V2: > > > As Uwe Kleine-König's suggestion, the version do below changes: > > > -call clk_prepare_enable in probe to avoid never enabling clock > > > if CONFIG_PM is disabled > > > -enable clock before request IRQ in probe -remove the pm staff in > > > i2c_imx_isr > > > > > > V3: > > > -pm_runtime_get_sync returns < 0 as error > > > > > > V4: > > > -add pm_runtime_set_active before pm_runtime_enable -replace > > > pm_runtime_put_autosuspend with pm_runtime_autosuspend > > > in probe > > > -add error disposal when i2c_add_numbered_adapter fails > > > > > > drivers/i2c/busses/i2c-imx.c | 79 > > > ++++++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 68 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > > b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644 > > > --- a/drivers/i2c/busses/i2c-imx.c > > > +++ b/drivers/i2c/busses/i2c-imx.c > > > @@ -53,6 +53,7 @@ > > > #include <linux/platform_device.h> > > > #include <linux/sched.h> > > > #include <linux/slab.h> > > > +#include <linux/pm_runtime.h> > > > > > > /** Defines > > > ******************************************************************** > > > > > > ******************************************************************** > > > ** > > > *********/ > > > @@ -118,6 +119,8 @@ > > > #define I2CR_IEN_OPCODE_0 0x0 > > > #define I2CR_IEN_OPCODE_1 I2CR_IEN > > > > > > +#define I2C_PM_TIMEOUT 10 /* ms */ > > > + > > > /** Variables > > > ****************************************************************** > > > > > > ******************************************************************** > > > ** > > > *********/ > > > > > > @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct > > > *i2c_imx) > > > > > > i2c_imx_set_clk(i2c_imx); > > > > > > - result = clk_prepare_enable(i2c_imx->clk); > > > - if (result) > > > - return result; > > > imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); > > > /* Enable I2C controller */ > > > imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, > > > IMX_I2C_I2SR); @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct > > imx_i2c_struct *i2c_imx) > > > /* Disable I2C controller */ > > > temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, > > > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > > > - clk_disable_unprepare(i2c_imx->clk); > > > } > > > > > > static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -894,6 > > > +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > > > > > > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); > > > > > > + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > > > + if (result < 0) > > > + goto out; > > > + > > > /* Start I2C transfer */ > > > result = i2c_imx_start(i2c_imx); > > > if (result) > > > @@ -950,6 +953,10 @@ fail0: > > > /* Stop I2C transfer */ > > > i2c_imx_stop(i2c_imx); > > > > > > +out: > > > + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > > > + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > > > + > > > dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, > > > (result < 0) ? "error" : "success msg", > > > (result < 0) ? result : num); > > > @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct > > > platform_device > > > *pdev) > > > > > > ret = clk_prepare_enable(i2c_imx->clk); > > > if (ret) { > > > - dev_err(&pdev->dev, "can't enable I2C clock\n"); > > > + dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret); > > > return ret; > > > } > > > + > > > /* Request IRQ */ > > > ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, > > > pdev->name, i2c_imx); > > > @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct > > > platform_device > > *pdev) > > > /* Set up adapter data */ > > > i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); > > > > > > + /* Set up platform driver data */ > > > + platform_set_drvdata(pdev, i2c_imx); > > > + > > > + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > > > + pm_runtime_use_autosuspend(&pdev->dev); > > > + pm_runtime_set_active(&pdev->dev); > > > + pm_runtime_enable(&pdev->dev); > > > + > > > /* Set up clock divider */ > > > i2c_imx->bitrate = IMX_I2C_BIT_RATE; > > > ret = of_property_read_u32(pdev->dev.of_node, > > > @@ -1053,12 +1069,13 @@ static int i2c_imx_probe(struct > > > platform_device > > *pdev) > > > ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > > > if (ret < 0) { > > > dev_err(&pdev->dev, "registration failed\n"); > > > + pm_runtime_mark_last_busy(&pdev->dev); > > > + pm_runtime_autosuspend(&pdev->dev); > > It would be more consistent with the general practice not to clean up > > here but to define a new error label, goto there and do the clean-up > > there. > > And if adding the adapter fails most likely you don't want to > > autosuspend but clean up and disable runtime PM. > > Thank you, will change it in next version. > > > > goto clk_disable; > > > } > > > > > > - /* Set up platform driver data */ > > > - platform_set_drvdata(pdev, i2c_imx); > > > - clk_disable_unprepare(i2c_imx->clk); > > > + pm_runtime_mark_last_busy(&pdev->dev); > > > + pm_runtime_autosuspend(&pdev->dev); > > >From enabling runtime PM to here you don't hold a reference so > > >theoretically > > it might happen that the device gets suspended in-between. > > I'm not able to say whether this is an actual potential issue here. > > However to be on the safe side it wouldn't hurt to do a > > pm_runtime_get_xx when enabling PM and use pm_runtime_put_autosuspend > here. > > Thanks. If I use pm_runtime_get_xx when enabling PM, clk_prepare_enable > will be called again. > However, it has been called before. It's kind of code redundancy to use > pm_runtime_get_xx. > > > Regards, Heiner > > > > > > dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq); > > > dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res); @@ > > > -1079,6 +1096,11 @@ clk_disable: > > > static int i2c_imx_remove(struct platform_device *pdev) { > > > struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = pm_runtime_get_sync(&pdev->dev); > > > + if (ret < 0) > > > + return ret; > > > > > > /* remove adapter */ > > > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); @@ -1093,17 > > > +1115,52 @@ static int i2c_imx_remove(struct platform_device *pdev) > > > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR); > > > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); > > > > > > + pm_runtime_put_sync(&pdev->dev); > > > + pm_runtime_disable(&pdev->dev); > > > + > > > return 0; > > > } > > > > > > +#ifdef CONFIG_PM > > > +static int i2c_imx_runtime_suspend(struct device *dev) { > > > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > > > + > > > + clk_disable_unprepare(i2c_imx->clk); > > > + > > > + return 0; > > > +} > > > + > > > +static int i2c_imx_runtime_resume(struct device *dev) { > > > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + ret = clk_prepare_enable(i2c_imx->clk); > > > + if (ret) > > > + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > > > + > > > + return ret; > > > +} > > > + > > > +static const struct dev_pm_ops i2c_imx_pm_ops = { > > > + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, > > > + i2c_imx_runtime_resume, NULL) }; #define > I2C_IMX_PM_OPS > > > +(&i2c_imx_pm_ops) #else #define I2C_IMX_PM_OPS NULL #endif /* > > > +CONFIG_PM */ > > > + > > > static struct platform_driver i2c_imx_driver = { > > > .probe = i2c_imx_probe, > > > .remove = i2c_imx_remove, > > > - .driver = { > > > - .name = DRIVER_NAME, > > > + .driver = { > > > + .name = DRIVER_NAME, > > > + .pm = I2C_IMX_PM_OPS, > > > .of_match_table = i2c_imx_dt_ids, > > > }, > > > - .id_table = imx_i2c_devtype, > > > + .id_table = imx_i2c_devtype, > > > }; > > > > > > static int __init i2c_adap_imx_init(void)
Am 25.08.2015 um 13:09 schrieb Gao Pandy: > From: Heiner Kallweit <mailto:hkallweit1@gmail.com> Sent: Tuesday, August 25, 2015 2:37 PM >> To: Gao Pan-B54642; wsa@the-dreams.de >> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; >> u.kleine-koenig@pengutronix.de; kernel@pengutronix.de >> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve the >> performance >> >> Am 25.08.2015 um 04:20 schrieb Gao Pan: >>> In our former i2c driver, i2c clk is enabled and disabled in xfer >>> function, which contributes to power saving. However, the clk enable >>> process brings a busy wait delay until the core is stable. As a >>> result, the performance is sacrificed. >>> >>> To weigh the power consumption and i2c bus performance, runtime pm is >>> the good solution for it. The clk is enabled when a i2c transfer >>> starts, and disabled after a specifically defined delay. >>> >>> Without the patch the test case (many eeprom reads) executes with >> approx: >>> real 1m7.735s >>> user 0m0.488s >>> sys 0m20.040s >>> >>> With the patch the same test case (many eeprom reads) executes with >> approx: >>> real 0m54.241s >>> user 0m0.440s >>> sys 0m5.920s >>> >>> Signed-off-by: Fugang Duan <B38611@freescale.com> >>> Signed-off-by: Gao Pan <b54642@freescale.com> >>> [wsa: fixed some indentation] >>> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> >>> --- >>> V2: >>> As Uwe Kleine-König's suggestion, the version do below changes: >>> -call clk_prepare_enable in probe to avoid never enabling clock >>> if CONFIG_PM is disabled >>> -enable clock before request IRQ in probe -remove the pm staff in >>> i2c_imx_isr >>> >>> V3: >>> -pm_runtime_get_sync returns < 0 as error >>> >>> V4: >>> -add pm_runtime_set_active before pm_runtime_enable -replace >>> pm_runtime_put_autosuspend with pm_runtime_autosuspend >>> in probe >>> -add error disposal when i2c_add_numbered_adapter fails >>> >>> drivers/i2c/busses/i2c-imx.c | 79 >>> ++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 68 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-imx.c >>> b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644 >>> --- a/drivers/i2c/busses/i2c-imx.c >>> +++ b/drivers/i2c/busses/i2c-imx.c >>> @@ -53,6 +53,7 @@ >>> #include <linux/platform_device.h> >>> #include <linux/sched.h> >>> #include <linux/slab.h> >>> +#include <linux/pm_runtime.h> >>> >>> /** Defines >>> ******************************************************************** >>> >>> ********************************************************************** >>> *********/ >>> @@ -118,6 +119,8 @@ >>> #define I2CR_IEN_OPCODE_0 0x0 >>> #define I2CR_IEN_OPCODE_1 I2CR_IEN >>> >>> +#define I2C_PM_TIMEOUT 10 /* ms */ >>> + >>> /** Variables >>> ****************************************************************** >>> >>> ********************************************************************** >>> *********/ >>> >>> @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct >>> *i2c_imx) >>> >>> i2c_imx_set_clk(i2c_imx); >>> >>> - result = clk_prepare_enable(i2c_imx->clk); >>> - if (result) >>> - return result; >>> imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); >>> /* Enable I2C controller */ >>> imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, >>> IMX_I2C_I2SR); @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct >> imx_i2c_struct *i2c_imx) >>> /* Disable I2C controller */ >>> temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, >>> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); >>> - clk_disable_unprepare(i2c_imx->clk); >>> } >>> >>> static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -894,6 >>> +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, >>> >>> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); >>> >>> + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); >>> + if (result < 0) >>> + goto out; >>> + >>> /* Start I2C transfer */ >>> result = i2c_imx_start(i2c_imx); >>> if (result) >>> @@ -950,6 +953,10 @@ fail0: >>> /* Stop I2C transfer */ >>> i2c_imx_stop(i2c_imx); >>> >>> +out: >>> + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); >>> + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); >>> + >>> dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, >>> (result < 0) ? "error" : "success msg", >>> (result < 0) ? result : num); >>> @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct platform_device >>> *pdev) >>> >>> ret = clk_prepare_enable(i2c_imx->clk); >>> if (ret) { >>> - dev_err(&pdev->dev, "can't enable I2C clock\n"); >>> + dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret); >>> return ret; >>> } >>> + >>> /* Request IRQ */ >>> ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, >>> pdev->name, i2c_imx); >>> @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct platform_device >> *pdev) >>> /* Set up adapter data */ >>> i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); >>> >>> + /* Set up platform driver data */ >>> + platform_set_drvdata(pdev, i2c_imx); >>> + >>> + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); >>> + pm_runtime_use_autosuspend(&pdev->dev); >>> + pm_runtime_set_active(&pdev->dev); >>> + pm_runtime_enable(&pdev->dev); >>> + >>> /* Set up clock divider */ >>> i2c_imx->bitrate = IMX_I2C_BIT_RATE; >>> ret = of_property_read_u32(pdev->dev.of_node, >>> @@ -1053,12 +1069,13 @@ static int i2c_imx_probe(struct platform_device >> *pdev) >>> ret = i2c_add_numbered_adapter(&i2c_imx->adapter); >>> if (ret < 0) { >>> dev_err(&pdev->dev, "registration failed\n"); >>> + pm_runtime_mark_last_busy(&pdev->dev); >>> + pm_runtime_autosuspend(&pdev->dev); >> It would be more consistent with the general practice not to clean up >> here but to define a new error label, goto there and do the clean-up >> there. >> And if adding the adapter fails most likely you don't want to autosuspend >> but clean up and disable runtime PM. > > Thank you, will change it in next version. > >>> goto clk_disable; >>> } >>> >>> - /* Set up platform driver data */ >>> - platform_set_drvdata(pdev, i2c_imx); >>> - clk_disable_unprepare(i2c_imx->clk); >>> + pm_runtime_mark_last_busy(&pdev->dev); >>> + pm_runtime_autosuspend(&pdev->dev); >> >From enabling runtime PM to here you don't hold a reference so >>> theoretically >> it might happen that the device gets suspended in-between. >> I'm not able to say whether this is an actual potential issue here. >> However to be on the safe side it wouldn't hurt to do a pm_runtime_get_xx >> when enabling PM and use pm_runtime_put_autosuspend here. > > Thanks. If I use pm_runtime_get_xx when enabling PM, clk_prepare_enable will be called again. > However, it has been called before. It's kind of code redundancy to use pm_runtime_get_xx. No, it won't. This function checks whether the device is in state "active" and in this case it doesn't call the resume callback and returns 1. See also the source code of runtime PM in drivers/base/power/runtime.c Rgds, Heiner > >> Regards, Heiner >>> >>> dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq); >>> dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res); @@ >>> -1079,6 +1096,11 @@ clk_disable: >>> static int i2c_imx_remove(struct platform_device *pdev) { >>> struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); >>> + int ret; >>> + >>> + ret = pm_runtime_get_sync(&pdev->dev); >>> + if (ret < 0) >>> + return ret; >>> >>> /* remove adapter */ >>> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); @@ -1093,17 >>> +1115,52 @@ static int i2c_imx_remove(struct platform_device *pdev) >>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR); >>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); >>> >>> + pm_runtime_put_sync(&pdev->dev); >>> + pm_runtime_disable(&pdev->dev); >>> + >>> return 0; >>> } >>> >>> +#ifdef CONFIG_PM >>> +static int i2c_imx_runtime_suspend(struct device *dev) { >>> + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); >>> + >>> + clk_disable_unprepare(i2c_imx->clk); >>> + >>> + return 0; >>> +} >>> + >>> +static int i2c_imx_runtime_resume(struct device *dev) { >>> + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + ret = clk_prepare_enable(i2c_imx->clk); >>> + if (ret) >>> + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); >>> + >>> + return ret; >>> +} >>> + >>> +static const struct dev_pm_ops i2c_imx_pm_ops = { >>> + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, >>> + i2c_imx_runtime_resume, NULL) >>> +}; >>> +#define I2C_IMX_PM_OPS (&i2c_imx_pm_ops) #else #define I2C_IMX_PM_OPS >>> +NULL #endif /* CONFIG_PM */ >>> + >>> static struct platform_driver i2c_imx_driver = { >>> .probe = i2c_imx_probe, >>> .remove = i2c_imx_remove, >>> - .driver = { >>> - .name = DRIVER_NAME, >>> + .driver = { >>> + .name = DRIVER_NAME, >>> + .pm = I2C_IMX_PM_OPS, >>> .of_match_table = i2c_imx_dt_ids, >>> }, >>> - .id_table = imx_i2c_devtype, >>> + .id_table = imx_i2c_devtype, >>> }; >>> >>> static int __init i2c_adap_imx_init(void) >>> > -- 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
From: Heiner Kallweit <mailto:hkallweit1@gmail.com> Sent: Wednesday, August 26, 2015 3:53 AM > To: Gao Pan-B54642; wsa@the-dreams.de > Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; > u.kleine-koenig@pengutronix.de; kernel@pengutronix.de > Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve the > performance > > Am 25.08.2015 um 13:09 schrieb Gao Pandy: > > From: Heiner Kallweit <mailto:hkallweit1@gmail.com> Sent: Tuesday, > > August 25, 2015 2:37 PM > >> To: Gao Pan-B54642; wsa@the-dreams.de > >> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; > >> u.kleine-koenig@pengutronix.de; kernel@pengutronix.de > >> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve > >> the performance > >> > >> Am 25.08.2015 um 04:20 schrieb Gao Pan: > >>> In our former i2c driver, i2c clk is enabled and disabled in xfer > >>> function, which contributes to power saving. However, the clk enable > >>> process brings a busy wait delay until the core is stable. As a > >>> result, the performance is sacrificed. > >>> > >>> To weigh the power consumption and i2c bus performance, runtime pm > >>> is the good solution for it. The clk is enabled when a i2c transfer > >>> starts, and disabled after a specifically defined delay. > >>> > >>> Without the patch the test case (many eeprom reads) executes with > >> approx: > >>> real 1m7.735s > >>> user 0m0.488s > >>> sys 0m20.040s > >>> > >>> With the patch the same test case (many eeprom reads) executes with > >> approx: > >>> real 0m54.241s > >>> user 0m0.440s > >>> sys 0m5.920s > >>> > >>> Signed-off-by: Fugang Duan <B38611@freescale.com> > >>> Signed-off-by: Gao Pan <b54642@freescale.com> > >>> [wsa: fixed some indentation] > >>> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > >>> --- > >>> V2: > >>> As Uwe Kleine-König's suggestion, the version do below changes: > >>> -call clk_prepare_enable in probe to avoid never enabling clock > >>> if CONFIG_PM is disabled > >>> -enable clock before request IRQ in probe -remove the pm staff in > >>> i2c_imx_isr > >>> > >>> V3: > >>> -pm_runtime_get_sync returns < 0 as error > >>> > >>> V4: > >>> -add pm_runtime_set_active before pm_runtime_enable -replace > >>> pm_runtime_put_autosuspend with pm_runtime_autosuspend > >>> in probe > >>> -add error disposal when i2c_add_numbered_adapter fails > >>> > >>> drivers/i2c/busses/i2c-imx.c | 79 > >>> ++++++++++++++++++++++++++++++++++++++------ > >>> 1 file changed, 68 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/i2c/busses/i2c-imx.c > >>> b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644 > >>> --- a/drivers/i2c/busses/i2c-imx.c > >>> +++ b/drivers/i2c/busses/i2c-imx.c > >>> @@ -53,6 +53,7 @@ > >>> #include <linux/platform_device.h> > >>> #include <linux/sched.h> > >>> #include <linux/slab.h> > >>> +#include <linux/pm_runtime.h> > >>> > >>> /** Defines > >>> ******************************************************************** > >>> > >>> ******************************************************************** > >>> ** > >>> *********/ > >>> @@ -118,6 +119,8 @@ > >>> #define I2CR_IEN_OPCODE_0 0x0 > >>> #define I2CR_IEN_OPCODE_1 I2CR_IEN > >>> > >>> +#define I2C_PM_TIMEOUT 10 /* ms */ > >>> + > >>> /** Variables > >>> ****************************************************************** > >>> > >>> ******************************************************************** > >>> ** > >>> *********/ > >>> > >>> @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct > >>> *i2c_imx) > >>> > >>> i2c_imx_set_clk(i2c_imx); > >>> > >>> - result = clk_prepare_enable(i2c_imx->clk); > >>> - if (result) > >>> - return result; > >>> imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); > >>> /* Enable I2C controller */ > >>> imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, > >>> IMX_I2C_I2SR); @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct > >> imx_i2c_struct *i2c_imx) > >>> /* Disable I2C controller */ > >>> temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, > >>> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > >>> - clk_disable_unprepare(i2c_imx->clk); > >>> } > >>> > >>> static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -894,6 > >>> +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > >>> > >>> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); > >>> > >>> + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > >>> + if (result < 0) > >>> + goto out; > >>> + > >>> /* Start I2C transfer */ > >>> result = i2c_imx_start(i2c_imx); > >>> if (result) > >>> @@ -950,6 +953,10 @@ fail0: > >>> /* Stop I2C transfer */ > >>> i2c_imx_stop(i2c_imx); > >>> > >>> +out: > >>> + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > >>> + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > >>> + > >>> dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, > >>> (result < 0) ? "error" : "success msg", > >>> (result < 0) ? result : num); > >>> @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct > >>> platform_device > >>> *pdev) > >>> > >>> ret = clk_prepare_enable(i2c_imx->clk); > >>> if (ret) { > >>> - dev_err(&pdev->dev, "can't enable I2C clock\n"); > >>> + dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret); > >>> return ret; > >>> } > >>> + > >>> /* Request IRQ */ > >>> ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, > >>> pdev->name, i2c_imx); > >>> @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct > >>> platform_device > >> *pdev) > >>> /* Set up adapter data */ > >>> i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); > >>> > >>> + /* Set up platform driver data */ > >>> + platform_set_drvdata(pdev, i2c_imx); > >>> + > >>> + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > >>> + pm_runtime_use_autosuspend(&pdev->dev); > >>> + pm_runtime_set_active(&pdev->dev); > >>> + pm_runtime_enable(&pdev->dev); > >>> + > >>> /* Set up clock divider */ > >>> i2c_imx->bitrate = IMX_I2C_BIT_RATE; > >>> ret = of_property_read_u32(pdev->dev.of_node, > >>> @@ -1053,12 +1069,13 @@ static int i2c_imx_probe(struct > >>> platform_device > >> *pdev) > >>> ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > >>> if (ret < 0) { > >>> dev_err(&pdev->dev, "registration failed\n"); > >>> + pm_runtime_mark_last_busy(&pdev->dev); > >>> + pm_runtime_autosuspend(&pdev->dev); > >> It would be more consistent with the general practice not to clean up > >> here but to define a new error label, goto there and do the clean-up > >> there. > >> And if adding the adapter fails most likely you don't want to > >> autosuspend but clean up and disable runtime PM. > > > > Thank you, will change it in next version. > > > >>> goto clk_disable; > >>> } > >>> > >>> - /* Set up platform driver data */ > >>> - platform_set_drvdata(pdev, i2c_imx); > >>> - clk_disable_unprepare(i2c_imx->clk); > >>> + pm_runtime_mark_last_busy(&pdev->dev); > >>> + pm_runtime_autosuspend(&pdev->dev); > >> >From enabling runtime PM to here you don't hold a reference so > >>> theoretically > >> it might happen that the device gets suspended in-between. > >> I'm not able to say whether this is an actual potential issue here. > >> However to be on the safe side it wouldn't hurt to do a > >> pm_runtime_get_xx when enabling PM and use pm_runtime_put_autosuspend > here. > > > > Thanks. If I use pm_runtime_get_xx when enabling PM, clk_prepare_enable > will be called again. > > However, it has been called before. It's kind of code redundancy to use > pm_runtime_get_xx. > No, it won't. This function checks whether the device is in state > "active" and in this case it doesn't call the resume callback and returns > 1. See also the source code of runtime PM in drivers/base/power/runtime.c Thank you, will change it in next version. > Rgds, Heiner > > > >> Regards, Heiner > >>> > >>> dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq); > >>> dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res); @@ > >>> -1079,6 +1096,11 @@ clk_disable: > >>> static int i2c_imx_remove(struct platform_device *pdev) { > >>> struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); > >>> + int ret; > >>> + > >>> + ret = pm_runtime_get_sync(&pdev->dev); > >>> + if (ret < 0) > >>> + return ret; > >>> > >>> /* remove adapter */ > >>> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); @@ -1093,17 > >>> +1115,52 @@ static int i2c_imx_remove(struct platform_device *pdev) > >>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR); > >>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); > >>> > >>> + pm_runtime_put_sync(&pdev->dev); > >>> + pm_runtime_disable(&pdev->dev); > >>> + > >>> return 0; > >>> } > >>> > >>> +#ifdef CONFIG_PM > >>> +static int i2c_imx_runtime_suspend(struct device *dev) { > >>> + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > >>> + > >>> + clk_disable_unprepare(i2c_imx->clk); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int i2c_imx_runtime_resume(struct device *dev) { > >>> + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > >>> + int ret; > >>> + > >>> + ret = clk_prepare_enable(i2c_imx->clk); > >>> + if (ret) > >>> + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static const struct dev_pm_ops i2c_imx_pm_ops = { > >>> + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, > >>> + i2c_imx_runtime_resume, NULL) }; #define > I2C_IMX_PM_OPS > >>> +(&i2c_imx_pm_ops) #else #define I2C_IMX_PM_OPS NULL #endif /* > >>> +CONFIG_PM */ > >>> + > >>> static struct platform_driver i2c_imx_driver = { > >>> .probe = i2c_imx_probe, > >>> .remove = i2c_imx_remove, > >>> - .driver = { > >>> - .name = DRIVER_NAME, > >>> + .driver = { > >>> + .name = DRIVER_NAME, > >>> + .pm = I2C_IMX_PM_OPS, > >>> .of_match_table = i2c_imx_dt_ids, > >>> }, > >>> - .id_table = imx_i2c_devtype, > >>> + .id_table = imx_i2c_devtype, > >>> }; > >>> > >>> static int __init i2c_adap_imx_init(void)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include <linux/platform_device.h> #include <linux/sched.h> #include <linux/slab.h> +#include <linux/pm_runtime.h> /** Defines ******************************************************************** *******************************************************************************/ @@ -118,6 +119,8 @@ #define I2CR_IEN_OPCODE_0 0x0 #define I2CR_IEN_OPCODE_1 I2CR_IEN +#define I2C_PM_TIMEOUT 10 /* ms */ + /** Variables ****************************************************************** *******************************************************************************/ @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) i2c_imx_set_clk(i2c_imx); - result = clk_prepare_enable(i2c_imx->clk); - if (result) - return result; imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); /* Enable I2C controller */ imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR); @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Disable I2C controller */ temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); - clk_disable_unprepare(i2c_imx->clk); } static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -894,6 +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); + if (result < 0) + goto out; + /* Start I2C transfer */ result = i2c_imx_start(i2c_imx); if (result) @@ -950,6 +953,10 @@ fail0: /* Stop I2C transfer */ i2c_imx_stop(i2c_imx); +out: + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); + dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, (result < 0) ? "error" : "success msg", (result < 0) ? result : num); @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct platform_device *pdev) ret = clk_prepare_enable(i2c_imx->clk); if (ret) { - dev_err(&pdev->dev, "can't enable I2C clock\n"); + dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret); return ret; } + /* Request IRQ */ ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, pdev->name, i2c_imx); @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct platform_device *pdev) /* Set up adapter data */ i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); + /* Set up platform driver data */ + platform_set_drvdata(pdev, i2c_imx); + + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + /* Set up clock divider */ i2c_imx->bitrate = IMX_I2C_BIT_RATE; ret = of_property_read_u32(pdev->dev.of_node, @@ -1053,12 +1069,13 @@ static int i2c_imx_probe(struct platform_device *pdev) ret = i2c_add_numbered_adapter(&i2c_imx->adapter); if (ret < 0) { dev_err(&pdev->dev, "registration failed\n"); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_autosuspend(&pdev->dev); goto clk_disable; } - /* Set up platform driver data */ - platform_set_drvdata(pdev, i2c_imx); - clk_disable_unprepare(i2c_imx->clk); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_autosuspend(&pdev->dev); dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq); dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res); @@ -1079,6 +1096,11 @@ clk_disable: static int i2c_imx_remove(struct platform_device *pdev) { struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); + int ret; + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) + return ret; /* remove adapter */ dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); @@ -1093,17 +1115,52 @@ static int i2c_imx_remove(struct platform_device *pdev) imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR); imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); + pm_runtime_put_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); + return 0; } +#ifdef CONFIG_PM +static int i2c_imx_runtime_suspend(struct device *dev) +{ + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); + + clk_disable_unprepare(i2c_imx->clk); + + return 0; +} + +static int i2c_imx_runtime_resume(struct device *dev) +{ + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); + int ret; + + ret = clk_prepare_enable(i2c_imx->clk); + if (ret) + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); + + return ret; +} + +static const struct dev_pm_ops i2c_imx_pm_ops = { + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, + i2c_imx_runtime_resume, NULL) +}; +#define I2C_IMX_PM_OPS (&i2c_imx_pm_ops) +#else +#define I2C_IMX_PM_OPS NULL +#endif /* CONFIG_PM */ + static struct platform_driver i2c_imx_driver = { .probe = i2c_imx_probe, .remove = i2c_imx_remove, - .driver = { - .name = DRIVER_NAME, + .driver = { + .name = DRIVER_NAME, + .pm = I2C_IMX_PM_OPS, .of_match_table = i2c_imx_dt_ids, }, - .id_table = imx_i2c_devtype, + .id_table = imx_i2c_devtype, }; static int __init i2c_adap_imx_init(void)