Re: [Patch V8] i2c: imx: add runtime pm support to improve the performance
clk_enable is called here. enables the clock here. > > The calling sequence is: > pm_runtime_use_autosuspend(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > pm_runtime_get_sync(&pdev->dev); are we not enabling it again? there by effectively disabling pm. Or did I missing something. > > The cleanup order is: > pm_runtime_put_noidle(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > > Is it that I miss pm_runtime_dont_use_autosuspend(&pdev->dev); > > Thank you! > > Best Reguards > Gao Pan > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [Patch V8] i2c: imx: add runtime pm support to improve the performance
From: Uwe Kleine-König <mailto:u.kleine-koe...@pengutronix.de> Sent: Tuesday, October 13, 2015 3:07 PM > To: Gao Pan-B54642 > Cc: w...@the-dreams.de; linux-i2c@vger.kernel.org; Li Frank-B20596; Duan > Fugang-B38611; ker...@pengutronix.de; hkallwe...@gmail.com > Subject: Re: [Patch V8] i2c: imx: add runtime pm support to improve the > performance > > Hello, > > On Fri, Sep 18, 2015 at 05:51:09PM +0800, Gao Pan wrote: > > 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 > > Signed-off-by: Gao Pan > > If runtime-pm is disabled the net result of this patch is that the clock > is never disabled, right? I think this is ok, but I would have pointed > that out in the commit log. Yes, you are right, I will point that out in the commit log in next version. > > @@ -1037,6 +1045,18 @@ 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); > > + > > + ret = pm_runtime_get_sync(&pdev->dev); > > + if (ret < 0) > > + goto rpm_disable; > > + > > /* Set up clock divider */ > > i2c_imx->bitrate = IMX_I2C_BIT_RATE; > > ret = of_property_read_u32(pdev->dev.of_node, > > @@ -1053,12 +1073,11 @@ 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"); > > - goto clk_disable; > > + goto rpm_disable; > > Is it right, that here the same error path is taken as when > pm_runtime_get_sync fails above? Even if it works now, not having the > error cleanup path undo all things in reverse order is a danger to get it > wrong in the next change. So if this is fixable, it would be nice to > implement. When i2c_add_numbered_adapter fails, the cleanup should include the runtime pm stuff. So it's ok to take the same error patch. What do you mean by undo all things in reverse order, I think the cleanup is in reverse order. The calling sequence is: pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); pm_runtime_get_sync(&pdev->dev); The cleanup order is: pm_runtime_put_noidle(&pdev->dev); pm_runtime_disable(&pdev->dev); pm_runtime_set_suspended(&pdev->dev); Is it that I miss pm_runtime_dont_use_autosuspend(&pdev->dev); Thank you! Best Reguards Gao Pan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V8] i2c: imx: add runtime pm support to improve the performance
Hello, On Fri, Sep 18, 2015 at 05:51:09PM +0800, Gao Pan wrote: > 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 > Signed-off-by: Gao Pan If runtime-pm is disabled the net result of this patch is that the clock is never disabled, right? I think this is ok, but I would have pointed that out in the commit log. > @@ -1037,6 +1045,18 @@ 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); > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) > + goto rpm_disable; > + > /* Set up clock divider */ > i2c_imx->bitrate = IMX_I2C_BIT_RATE; > ret = of_property_read_u32(pdev->dev.of_node, > @@ -1053,12 +1073,11 @@ 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"); > - goto clk_disable; > + goto rpm_disable; Is it right, that here the same error path is taken as when pm_runtime_get_sync fails above? Even if it works now, not having the error cleanup path undo all things in reverse order is a danger to get it wrong in the next change. So if this is fixable, it would be nice to implement. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V8] i2c: imx: add runtime pm support to improve the performance
On Fri, Sep 18, 2015 at 05:51:09PM +0800, Gao Pan wrote: > 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 > Signed-off-by: Gao Pan Uwe? Sascha? signature.asc Description: Digital signature
RE: [Patch V8] i2c: imx: add runtime pm support to improve the performance
Ping... From: Gao Pan <mailto:b54...@freescale.com> Sent: Friday, September 18, 2015 5:51 PM > To: w...@the-dreams.de > Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; Gao > Pan-B54642; u.kleine-koe...@pengutronix.de; ker...@pengutronix.de; > hkallwe...@gmail.com > Subject: [Patch V8] i2c: imx: add runtime pm support to improve the > performance > > 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 > Signed-off-by: Gao Pan > --- > 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 > > V5: > -clean up and disable runtime PM when i2c_add_numbered_adapter fails - > use pm_runtime_get and pm_runtime_put_autosuspend in probe > > V6: > -disable the clock manually and set the state to suspended explicitly > with > pm_runtime_set_suspended > > V7: > -manually disabling the clock and use pm_runtime_put_noidle in the > remove callback > > V8: > -move out label after the two runtime pm calls in i2c_imx_xfer > > drivers/i2c/busses/i2c-imx.c | 90 > ++-- > 1 file changed, 78 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 785aa67..91bdf28 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -53,6 +53,7 @@ > #include > #include > #include > +#include > > /** Defines > > > * > **/ > @@ -118,6 +119,8 @@ > #define I2CR_IEN_OPCODE_00x0 > #define I2CR_IEN_OPCODE_1I2CR_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); > > + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > + > +out: > dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, > (result < 0) ? "error" : "success msg", >
[Patch V8] i2c: imx: add runtime pm support to improve the performance
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 Signed-off-by: Gao Pan --- 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 V5: -clean up and disable runtime PM when i2c_add_numbered_adapter fails -use pm_runtime_get and pm_runtime_put_autosuspend in probe V6: -disable the clock manually and set the state to suspended explicitly with pm_runtime_set_suspended V7: -manually disabling the clock and use pm_runtime_put_noidle in the remove callback V8: -move out label after the two runtime pm calls in i2c_imx_xfer drivers/i2c/busses/i2c-imx.c | 90 ++-- 1 file changed, 78 insertions(+), 12 deletions(-) diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 785aa67..91bdf28 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include #include #include +#include /** 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); + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); + +out: 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,18 @@ 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); + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) + goto rpm_disable; +