Am 27.08.2015 um 09:06 schrieb Gao Pandy: > From: Heiner Kallweit <mailto:hkallweit1@xxxxxxxxx> Sent: Thursday, August 27, 2015 1:50 PM >> To: Gao Pan-B54642; wsa@xxxxxxxxxxxxx >> Cc: linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan Fugang-B38611; >> u.kleine-koenig@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx >> Subject: Re: [Patch V5] i2c: imx: add runtime pm support to improve the >> performance >> >> Am 27.08.2015 um 06:01 schrieb Gao Pandy: >>> From: Heiner Kallweit <mailto:hkallweit1@xxxxxxxxx> Sent: Thursday, >>> August 27, 2015 2:52 AM >>>> To: Gao Pan-B54642; wsa@xxxxxxxxxxxxx >>>> Cc: linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan Fugang-B38611; >>>> u.kleine-koenig@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx >>>> Subject: Re: [Patch V5] i2c: imx: add runtime pm support to improve >>>> the performance >>>> >>>> Am 26.08.2015 um 07:23 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@xxxxxxxxxxxxx> >>>>> Signed-off-by: Gao Pan <b54642@xxxxxxxxxxxxx> >>>>> [wsa: fixed some indentation] >>>>> Signed-off-by: Wolfram Sang <wsa@xxxxxxxxxxxxx> >>>>> --- >>>>> 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 >>>>> >>>>> drivers/i2c/busses/i2c-imx.c | 91 >>>>> ++++++++++++++++++++++++++++++++++++-------- >>>>> 1 file changed, 76 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-imx.c >>>>> b/drivers/i2c/busses/i2c-imx.c index 785aa67..8940a89 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,15 +1027,17 @@ 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); >>>>> if (ret) { >>>>> dev_err(&pdev->dev, "can't claim irq %d\n", irq); >>>>> - goto clk_disable; >>>>> + clk_disable_unprepare(i2c_imx->clk); >>>>> + return ret; >>>>> } >>>>> >>>>> /* Init queue */ >>>>> @@ -1037,6 +1046,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(&pdev->dev); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> /* Set up clock divider */ >>>>> i2c_imx->bitrate = IMX_I2C_BIT_RATE; >>>>> ret = of_property_read_u32(pdev->dev.of_node, >>>>> @@ -1053,12 +1074,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; >>>>> } >>>>> >>>>> - /* 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_put_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); @@ >>>>> -1071,14 +1091,20 @@ static int i2c_imx_probe(struct platform_device >>>>> *pdev) >>>>> >>>>> return 0; /* Return OK */ >>>>> >>>>> -clk_disable: >>>>> - clk_disable_unprepare(i2c_imx->clk); >>>>> +rpm_disable: >>>>> + pm_runtime_put(&pdev->dev); >>>>> + pm_runtime_disable(&pdev->dev); >>>> The cleanup has to be in reverse order of the setup. Therefore the >>>> rpm cleanup should happen before the clock disabling. >>>> As it is now you wouldn't disable the clock when registration fails. >>> >>> Thanks. When registration fails, pm_runtime_put is used to cleanup the >> runtime status and disable clk. >>> Actually, I think pm_runtime_put_sync_suspend is better here. >> You have another error path where only the clock has to be disabled >> (getting irq fails) as rpm isn't set up yet. Therefore you can't combine >> both cleanups. >> And even despite the fact that suspending and disabling clock technically >> is the same using suspend in an error path is not intuitive (as there's >> no way to resume). >> >>> >>> >>>> And better use pm_runtime_put_noidle. You don't want to suspend here >>>> if the ref count is 0. >>>> >>>> Last but not least a call to pm_runtime_set_suspended is missing >>>> (because after disabling the clock the device effectively is in >> suspended state). >>> >>> The status is set suspended in pm_runtime_put_sync_suspend, so >> pm_runtime_set_suspended is unnecessary here. >>> What do you think about it? >>> >>> Please see the following code: >>> rpm_disable: >>> pm_runtime_put_sync_suspend(&pdev->dev); >>> pm_runtime_disable(&pdev->dev); >>> return ret; >>> >> See above. Better disable the clock manually and set the state to >> suspended explicitly. >> >>>>> return ret; >>>>> > > Thanks a lot! How about the following logic? > > /* Request IRQ */ > ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, > pdev->name, i2c_imx); > if (ret) { > dev_err(&pdev->dev, "can't claim irq %d\n", irq); > goto clk_disable; > } > > ret = pm_runtime_get(&pdev->dev); > if (ret < 0) > goto rpm_disable; > > rpm_disable: > pm_runtime_put_noidle(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > clk_disable: > clk_disable_unprepare(i2c_imx->clk); > return ret; > To be on the very safe side you could use pm_runtime_get_sync instead of pm_runtime_get. Apart from that it looks ok to me now. > >>>>> >>>>> 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 >>>>> +1119,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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
