[Patch v1] i2c: imx: add runtime pm support to improve the performance

[Patch v1] i2c: imx: add runtime pm support to improve the performance

[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 consuption and i2c bus performance, runtime
pm is the good solution for it. The clk is enabled when a i2c
transfer starts, and disabled afer 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

>From the test result, the patch get better performance.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 785aa67..cc4b5d6 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)
@@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 	struct imx_i2c_struct *i2c_imx = dev_id;
 	unsigned int temp;
 
+	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
+		return IRQ_NONE;
+
 	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 	if (temp & I2SR_IIF) {
 		/* save status register */
@@ -894,6 +896,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 (IS_ERR_VALUE(result))
+		goto out;
+
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx);
 	if (result)
@@ -950,6 +956,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);
@@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		return PTR_ERR(i2c_imx->clk);
 	}
 
-	ret = clk_prepare_enable(i2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "can't enable I2C clock\n");
-		return ret;
-	}
 	/* Request IRQ */
 	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
 				pdev->name, i2c_imx);
@@ -1037,6 +1042,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_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	/* Set up clock divider */
 	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
 	ret = of_property_read_u32(pdev->dev.of_node,
@@ -1056,9 +1069,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		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_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);
@@ -1079,6 +1091,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 (IS_ERR_VALUE(ret))
+		return ret;
 
 	/* remove adapter */
 	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
@@ -1093,14 +1110,51 @@ 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\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+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,
+		.pm     = I2C_IMX_PM_OPS,
 		.of_match_table = i2c_imx_dt_ids,
 	},
 	.id_table	= imx_i2c_devtype,
-- 
1.9.1

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Shubhrajyoti Datta]

On Thu, Jun 11, 2015 at 7:20 AM, Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org> 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 consuption and i2c bus performance, runtime
> pm is the good solution for it. The clk is enabled when a i2c
> transfer starts, and disabled afer 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
>
> From the test result, the patch get better performance.
>
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 785aa67..cc4b5d6 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)
> @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>         struct imx_i2c_struct *i2c_imx = dev_id;
>         unsigned int temp;
>
> +       if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> +               return IRQ_NONE;
> +

Didn't quite get this one.

RE: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Duan Andy]

From: Shubhrajyoti Datta <omaplinuxkernel@gmail.com> Sent: Thursday, June 11, 2015 5:29 PM
> To: Gao Pan-B54642
> Cc: Wolfram Sang; Linux-I2C; Li Frank-B20596; Duan Fugang-B38611
> Subject: Re: [Patch v1] i2c: imx: add runtime pm support to improve the
> performance
> 
> On Thu, Jun 11, 2015 at 7:20 AM, Gao Pan <b54642@freescale.com> 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 consuption and i2c bus performance, runtime pm is
> > the good solution for it. The clk is enabled when a i2c transfer
> > starts, and disabled afer 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
> >
> > From the test result, the patch get better performance.
> >
> > Signed-off-by: Fugang Duan <B38611@freescale.com>
> > Signed-off-by: Gao Pan <b54642@freescale.com>
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 78
> > +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c index 785aa67..cc4b5d6 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) @@ -583,6
> > +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> >         struct imx_i2c_struct *i2c_imx = dev_id;
> >         unsigned int temp;
> >
> > +       if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> > +               return IRQ_NONE;
> > +
> 
> Didn't quite get this one.

Yes, there don't need to add pm_runtime_suspended() check in isr handler. But in some worse worse case, like system is very busy and irq is blocked by others that irq response coming is very late while i2c clock is gated off, the check can avoid system hang.

So I think it can be reasonable. How do you think ?

Regards,
Andy

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Shubhrajyoti Datta]

<snip>
>> >  static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -583,6
>> > +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>> >         struct imx_i2c_struct *i2c_imx = dev_id;
>> >         unsigned int temp;
>> >
>> > +       if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
>> > +               return IRQ_NONE;
>> > +
>>
>> Didn't quite get this one.
>
> Yes, there don't need to add pm_runtime_suspended() check in isr handler. But in some worse worse case,  like system is very
> busy and irq is blocked by others

you mean other irqs?

> that irq response coming is very late while i2c clock is gated off, the check can avoid system hang.
>
> So I think it can be reasonable. How do you think ?
>
> Regards,
> Andy

RE: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Duan Andy]

From: Shubhrajyoti Datta <omaplinuxkernel@gmail.com> Sent: Thursday, June 11, 2015 6:11 PM
> To: Duan Fugang-B38611
> Cc: Gao Pan-B54642; Wolfram Sang; Linux-I2C; Li Frank-B20596
> Subject: Re: [Patch v1] i2c: imx: add runtime pm support to improve the
> performance
> 
> <snip>
> >> >  static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -583,6
> >> > +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> >> >         struct imx_i2c_struct *i2c_imx = dev_id;
> >> >         unsigned int temp;
> >> >
> >> > +       if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> >> > +               return IRQ_NONE;
> >> > +
> >>
> >> Didn't quite get this one.
> >
> > Yes, there don't need to add pm_runtime_suspended() check in isr
> > handler. But in some worse worse case,  like system is very busy and
> > irq is blocked by others
> 
> you mean other irqs?

Maybe.  
Or spin_lock_irq_save() is called for some bad routine for long time ....

> 
> > that irq response coming is very late while i2c clock is gated off, the
> check can avoid system hang.
> >
> > So I think it can be reasonable. How do you think ?
> >
> > Regards,
> > Andy

RE: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

Ping...

> From: Gao Pan [mailto:b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org]
> Sent: Thursday, June 11, 2015 9:50 AM
> To: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li Frank-B20596; Duan Fugang-B38611
> Subject: [Patch v1] 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 consuption and i2c bus performance, runtime pm is 
> the good solution for it. The clk is enabled when a i2c transfer 
> starts, and disabled afer 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
> 
> From the test result, the patch get better performance.
> 
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-imx.c | 78 
> +++++++++++++++++++++++++++++++++++++-
> ------
>  1 file changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c 
> b/drivers/i2c/busses/i2c-imx.c index 785aa67..cc4b5d6 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) @@ -583,6 
> +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int temp;
> 
> +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> +		return IRQ_NONE;
> +
>  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	if (temp & I2SR_IIF) {
>  		/* save status register */
> @@ -894,6 +896,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 (IS_ERR_VALUE(result))
> +		goto out;
> +
>  	/* Start I2C transfer */
>  	result = i2c_imx_start(i2c_imx);
>  	if (result)
> @@ -950,6 +956,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);
> @@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
>  		return PTR_ERR(i2c_imx->clk);
>  	}
> 
> -	ret = clk_prepare_enable(i2c_imx->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "can't enable I2C clock\n");
> -		return ret;
> -	}
>  	/* Request IRQ */
>  	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
>  				pdev->name, i2c_imx);
> @@ -1037,6 +1042,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_enable(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	/* Set up clock divider */
>  	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
>  	ret = of_property_read_u32(pdev->dev.of_node,
> @@ -1056,9 +1069,8 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
>  		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_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); @@
> -1079,6 +1091,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 (IS_ERR_VALUE(ret))
> +		return ret;
> 
>  	/* remove adapter */
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); @@ -1093,14
> +1110,51 @@ 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\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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,
> +		.pm     = I2C_IMX_PM_OPS,
>  		.of_match_table = i2c_imx_dt_ids,
>  	},
>  	.id_table	= imx_i2c_devtype,
> --
> 1.9.1

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Uwe Kleine-König]

Hello,

On Thu, Jun 11, 2015 at 09:50:04AM +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.
Which platform are you referring here? Looking at i.MX21 I cannot find a
delay for the i2c clock.

> To weigh the power consuption and i2c bus performance, runtime
consumption

> pm is the good solution for it. The clk is enabled when a i2c
> transfer starts, and disabled afer a specifically defined delay.
after

> [...]
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 785aa67..cc4b5d6 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> [...]
> @@ -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;
you remove clk_prepare_enable enable and instead rely on
clk_prepare_enable in i2c_imx_runtime_resume, right?

If I understand correctly this results in never enabling the clock if
CONFIG_PM is disabled?!

> [...]
> @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int temp;
>  
> +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> +		return IRQ_NONE;
> +
I don't claim to understand the runtime pm stuff, but I agree to the
previous reviewer that this smells fishy. If this is required it needs a
comment why.

>  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	if (temp & I2SR_IIF) {
>  		/* save status register */
> @@ -894,6 +896,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 (IS_ERR_VALUE(result))
Is this better than
+	if (result < 0)
?

> +		goto out;
> [...]
> @@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		return PTR_ERR(i2c_imx->clk);
>  	}
>  
> -	ret = clk_prepare_enable(i2c_imx->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "can't enable I2C clock\n");
> -		return ret;
> -	}
Is there a reason the clock was enabled *here* before. With the way you
rearranged the code the irq handler might be entered before the clock is
on. But if I'm not mistaken (which might very well be the case) there is
a problem also without the patch if the bootloader jumps to linux with a
pending i2c transfer that fires when the clk_disable is done below. IMHO
"Set up chip registers to defaults" should be done before "Request
IRQ"?!

> [...]
> @@ -1093,14 +1110,51 @@ static int i2c_imx_remove(struct platform_device *pdev)
> [...]
> +#ifdef CONFIG_PM
> +static int i2c_imx_runtime_suspend(struct device *dev)
> +{
> +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
s/  / /
> [...]
> +static int i2c_imx_runtime_resume(struct device *dev)
> +{
> +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
s/  / /
> +	int ret;
> +
> +	ret = clk_prepare_enable(i2c_imx->clk);
> +	if (ret) {
> +		dev_err(dev, "can't enable I2C clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
this is equivalent to:

	if (ret)
		dev_err(dev, "can't enable I2C clock\n");

	return ret;

Probably a matter of taste. Also I'd suggest to add the value of ret to
the message to make the report more useful.

> +}
> +
> +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,
> +		.pm     = I2C_IMX_PM_OPS,
>  		.of_match_table = i2c_imx_dt_ids,
This increases the number of styles to place the = to three. .name uses
a tab, .of_match_table a space and you add .pm with >1 spaces.

>  	},
>  	.id_table	= imx_i2c_devtype,
unrelated to this patch: The = in the line above is not aligned
consistently compared to the other members at the same level like
.probe.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sent: Thursday, June 18, 2015 5:49 AM
> To: Gao Pan-B54642
> Cc: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li Frank-B20596; Duan
> Fugang-B38611
> Subject: Re: [Patch v1] i2c: imx: add runtime pm support to improve the
> performance
> 
> Hello,
> 
> On Thu, Jun 11, 2015 at 09:50:04AM +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.
> Which platform are you referring here? Looking at i.MX21 I cannot find a
> delay for the i2c clock.
I tested platforms are i.MX6q/i.MX7d that both use i.MX21 i2c IP.

There just need one delay before disable i2c controller:

static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
{
        ...

        if (is_imx1_i2c(i2c_imx)) {
                /*
                 * This delay caused by an i.MXL hardware bug.
                 * If no (or too short) delay, no "STOP" bit will be generated.
                 */
                udelay(i2c_imx->disable_delay);
        }

	  ...

        /* Disable I2C controller */
        temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
        imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
} 

> 
> > To weigh the power consuption and i2c bus performance, runtime
> consumption

Thanks for your detail review, will change it in next version.

> > pm is the good solution for it. The clk is enabled when a i2c transfer
> > starts, and disabled afer a specifically defined delay.
> after

Thanks.

> > [...]
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c index 785aa67..cc4b5d6 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > [...]
> > @@ -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;
> you remove clk_prepare_enable enable and instead rely on
> clk_prepare_enable in i2c_imx_runtime_resume, right?
> 
Yes, you are right.

> If I understand correctly this results in never enabling the clock if
> CONFIG_PM is disabled?!
> 
Yes, you are right, I will change it in next version.


> > [...]
> > @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void
> *dev_id)
> >  	struct imx_i2c_struct *i2c_imx = dev_id;
> >  	unsigned int temp;
> >
> > +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> > +		return IRQ_NONE;
> > +
> I don't claim to understand the runtime pm stuff, but I agree to the
> previous reviewer that this smells fishy. If this is required it needs a
> comment why.
> 
In fact, I agree you two reviewers' consideration. As I said, this is just to avoid system hang in worse case.
Of course, I still don't catch the worse case. So I will remove this pm stuff.

> >  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> >  	if (temp & I2SR_IIF) {
> >  		/* save status register */
> > @@ -894,6 +896,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 (IS_ERR_VALUE(result))
> Is this better than
> +	if (result < 0)
> ?

Yes, thanks. 
 

> 
> > +		goto out;
> > [...]
> > @@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >  		return PTR_ERR(i2c_imx->clk);
> >  	}
> >
> > -	ret = clk_prepare_enable(i2c_imx->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "can't enable I2C clock\n");
> > -		return ret;
> > -	}
> Is there a reason the clock was enabled *here* before. With the way you
> rearranged the code the irq handler might be entered before the clock is
> on. But if I'm not mistaken (which might very well be the case) there is
> a problem also without the patch if the bootloader jumps to linux with a
> pending i2c transfer that fires when the clk_disable is done below. IMHO
> "Set up chip registers to defaults" should be done before "Request IRQ"?!
> 
Agree with your opinion.  I will fix it in the next version.

> > [...]
> > @@ -1093,14 +1110,51 @@ static int i2c_imx_remove(struct
> > platform_device *pdev) [...]
> > +#ifdef CONFIG_PM
> > +static int i2c_imx_runtime_suspend(struct device *dev) {
> > +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
> s/  / /
> > [...]
> > +static int i2c_imx_runtime_resume(struct device *dev) {
> > +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
> s/  / /
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(i2c_imx->clk);
> > +	if (ret) {
> > +		dev_err(dev, "can't enable I2C clock\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> this is equivalent to:
> 
> 	if (ret)
> 		dev_err(dev, "can't enable I2C clock\n");
> 
> 	return ret;
> 
> Probably a matter of taste. Also I'd suggest to add the value of ret to
> the message to make the report more useful.
> 
Yes, thanks.

> > +}
> > +
> > +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,
> > +		.pm     = I2C_IMX_PM_OPS,
> >  		.of_match_table = i2c_imx_dt_ids,
> This increases the number of styles to place the = to three. .name uses a
> tab, .of_match_table a space and you add .pm with >1 spaces.
> 
Yes, thanks.

> >  	},
> >  	.id_table	= imx_i2c_devtype,
> unrelated to this patch: The = in the line above is not aligned
> consistently compared to the other members at the same level like .probe.
> 
Yes, thanks.

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
 |

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

> > > +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> > > +		return IRQ_NONE;
> > > +
> > I don't claim to understand the runtime pm stuff, but I agree to the
> > previous reviewer that this smells fishy. If this is required it needs a
> > comment why.
> > 
> In fact, I agree you two reviewers' consideration. As I said, this is just to avoid system hang in worse case.
> Of course, I still don't catch the worse case. So I will remove this pm stuff.

So, there is a worst case where the system can hang now which is not
present in the current version of this driver (i.e. before your patch)?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

From: Wolfram Sang <mailto:wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> sent: Thursday, June 18, 2015 12:24 PM
> To: Gao Pan-B54642
> Cc: Uwe Kleine-König; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li Frank-B20596; Duan
> Fugang-B38611
> Subject: Re: [Patch v1] i2c: imx: add runtime pm support to improve the
> performance
> 
> > > > +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> > > > +		return IRQ_NONE;
> > > > +
> > > I don't claim to understand the runtime pm stuff, but I agree to the
> > > previous reviewer that this smells fishy. If this is required it
> > > needs a comment why.
> > >
> > In fact, I agree you two reviewers' consideration. As I said, this is
> just to avoid system hang in worse case.
> > Of course, I still don't catch the worse case. So I will remove this pm
> stuff.
> 
> So, there is a worst case where the system can hang now which is not
> present in the current version of this driver (i.e. before your patch)?

Hi, Wolfram,

Before the patch, I don't catch the issue on Fsl engineer platforms.
But I remembered I supported one automotive customer that in their user case the issue can be reproduced in long term stress test.

Anyway, I prefer to remove the pm staff in here.

Thanks for your review. I will send the V2 patch.