[PATCH v2] i2c: cadence: Move to sensible power management

[PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

Currently the clocks are enabled at probe and disabled at remove.
Which keeps the clocks enabled even if no transaction is going on.
This patch enables the clocks at the start of transfer and disables
after it.

Also adapts to runtime pm.
Remove xi2c->suspended and use pm runtime status instead.

converts dev pm to const to silence a checkpatch warning.

Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
---
changes since v2
update the cc list

 drivers/i2c/busses/i2c-cadence.c |   73 ++++++++++++++++++++++++--------------
 1 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 84deed6..6b08d16 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 
 /* Register offsets for the I2C device. */
 #define CDNS_I2C_CR_OFFSET		0x00 /* Control Register, RW */
@@ -96,6 +97,8 @@
 					 CDNS_I2C_IXR_COMP)
 
 #define CDNS_I2C_TIMEOUT		msecs_to_jiffies(1000)
+/* timeout for pm runtime autosuspend */
+#define CNDS_I2C_PM_TIMEOUT		1000	/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH		16
 /* FIFO depth at which the DATA interrupt occurs */
@@ -128,7 +131,6 @@
  * @xfer_done:		Transfer complete status
  * @p_send_buf:		Pointer to transmit buffer
  * @p_recv_buf:		Pointer to receive buffer
- * @suspended:		Flag holding the device's PM status
  * @send_count:		Number of bytes still expected to send
  * @recv_count:		Number of bytes still expected to receive
  * @curr_recv_count:	Number of bytes to be received in current transfer
@@ -141,6 +143,7 @@
  * @quirks:		flag for broken hold bit usage in r1p10
  */
 struct cdns_i2c {
+	struct device		*dev;
 	void __iomem *membase;
 	struct i2c_adapter adap;
 	struct i2c_msg *p_msg;
@@ -148,7 +151,6 @@ struct cdns_i2c {
 	struct completion xfer_done;
 	unsigned char *p_send_buf;
 	unsigned char *p_recv_buf;
-	u8 suspended;
 	unsigned int send_count;
 	unsigned int recv_count;
 	unsigned int curr_recv_count;
@@ -569,9 +571,14 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	struct cdns_i2c *id = adap->algo_data;
 	bool hold_quirk;
 
+	ret = pm_runtime_get_sync(id->dev);
+	if (ret < 0)
+		return ret;
 	/* Check if the bus is free */
-	if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
-		return -EAGAIN;
+	if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+		ret = -EAGAIN;
+		goto out;
+	}
 
 	hold_quirk = !!(id->quirks & CDNS_I2C_BROKEN_HOLD_BIT);
 	/*
@@ -590,7 +597,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			if (msgs[count].flags & I2C_M_RD) {
 				dev_warn(adap->dev.parent,
 					 "Can't do repeated start after a receive message\n");
-				return -EOPNOTSUPP;
+				ret = -EOPNOTSUPP;
+				goto out;
 			}
 		}
 		id->bus_hold_flag = 1;
@@ -608,20 +616,26 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 		ret = cdns_i2c_process_msg(id, msgs, adap);
 		if (ret)
-			return ret;
+			goto out;
 
 		/* Report the other error interrupts to application */
 		if (id->err_status) {
 			cdns_i2c_master_reset(adap);
 
-			if (id->err_status & CDNS_I2C_IXR_NACK)
-				return -ENXIO;
-
-			return -EIO;
+			if (id->err_status & CDNS_I2C_IXR_NACK) {
+				ret = -ENXIO;
+				goto out;
+			}
+			ret = -EIO;
+			goto out;
 		}
 	}
 
-	return num;
+	ret = num;
+out:
+	pm_runtime_mark_last_busy(id->dev);
+	pm_runtime_put_autosuspend(id->dev);
+	return ret;
 }
 
 /**
@@ -760,7 +774,7 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 	struct clk_notifier_data *ndata = data;
 	struct cdns_i2c *id = to_cdns_i2c(nb);
 
-	if (id->suspended)
+	if (pm_runtime_suspended(id->dev))
 		return NOTIFY_OK;
 
 	switch (event) {
@@ -808,14 +822,12 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
  *
  * Return: 0 always
  */
-static int __maybe_unused cdns_i2c_suspend(struct device *_dev)
+static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev)
 {
-	struct platform_device *pdev = container_of(_dev,
-			struct platform_device, dev);
+	struct platform_device *pdev = to_platform_device(dev);
 	struct cdns_i2c *xi2c = platform_get_drvdata(pdev);
 
 	clk_disable(xi2c->clk);
-	xi2c->suspended = 1;
 
 	return 0;
 }
@@ -828,26 +840,25 @@ static int __maybe_unused cdns_i2c_suspend(struct device *_dev)
  *
  * Return: 0 on success and error value on error
  */
-static int __maybe_unused cdns_i2c_resume(struct device *_dev)
+static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = container_of(_dev,
-			struct platform_device, dev);
+	struct platform_device *pdev = to_platform_device(dev);
 	struct cdns_i2c *xi2c = platform_get_drvdata(pdev);
 	int ret;
 
 	ret = clk_enable(xi2c->clk);
 	if (ret) {
-		dev_err(_dev, "Cannot enable clock.\n");
+		dev_err(dev, "Cannot enable clock.\n");
 		return ret;
 	}
 
-	xi2c->suspended = 0;
-
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(cdns_i2c_dev_pm_ops, cdns_i2c_suspend,
-			 cdns_i2c_resume);
+static const struct dev_pm_ops cdns_i2c_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend,
+			   cdns_i2c_runtime_resume, NULL)
+};
 
 static const struct cdns_platform_data r1p10_i2c_def = {
 	.quirks = CDNS_I2C_BROKEN_HOLD_BIT,
@@ -881,6 +892,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	if (!id)
 		return -ENOMEM;
 
+	id->dev = &pdev->dev;
 	platform_set_drvdata(pdev, id);
 
 	match = of_match_node(cdns_i2c_of_match, pdev->dev.of_node);
@@ -913,10 +925,14 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(id->clk);
 	}
 	ret = clk_prepare_enable(id->clk);
-	if (ret) {
+	if (ret)
 		dev_err(&pdev->dev, "Unable to enable clock.\n");
-		return ret;
-	}
+
+	pm_runtime_enable(id->dev);
+	pm_runtime_set_autosuspend_delay(id->dev, CNDS_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(id->dev);
+	pm_runtime_set_active(id->dev);
+
 	id->clk_rate_change_nb.notifier_call = cdns_i2c_clk_notifier_cb;
 	if (clk_notifier_register(id->clk, &id->clk_rate_change_nb))
 		dev_warn(&pdev->dev, "Unable to register clock notifier.\n");
@@ -966,6 +982,8 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 
 err_clk_dis:
 	clk_disable_unprepare(id->clk);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	return ret;
 }
 
@@ -984,6 +1002,7 @@ static int cdns_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&id->adap);
 	clk_notifier_unregister(id->clk, &id->clk_rate_change_nb);
 	clk_disable_unprepare(id->clk);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
1.7.1

[PATCH v2] i2c: cadence: Move to sensible power management

[Sören Brinkmann]

Hi Shubhrajyoti,


On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
> Currently the clocks are enabled at probe and disabled at remove.
> Which keeps the clocks enabled even if no transaction is going on.
> This patch enables the clocks at the start of transfer and disables
> after it.
> 
> Also adapts to runtime pm.
> Remove xi2c->suspended and use pm runtime status instead.
> 
> converts dev pm to const to silence a checkpatch warning.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>

To me, this looks all good. Just one small concern below.

> ---
> changes since v2
> update the cc list
> 
>  drivers/i2c/busses/i2c-cadence.c |   73 ++++++++++++++++++++++++--------------
>  1 files changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 84deed6..6b08d16 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  
>  /* Register offsets for the I2C device. */
>  #define CDNS_I2C_CR_OFFSET		0x00 /* Control Register, RW */
> @@ -96,6 +97,8 @@
>  					 CDNS_I2C_IXR_COMP)
>  
>  #define CDNS_I2C_TIMEOUT		msecs_to_jiffies(1000)
> +/* timeout for pm runtime autosuspend */
> +#define CNDS_I2C_PM_TIMEOUT		1000	/* ms */
>  
>  #define CDNS_I2C_FIFO_DEPTH		16
>  /* FIFO depth at which the DATA interrupt occurs */
> @@ -128,7 +131,6 @@
>   * @xfer_done:		Transfer complete status
>   * @p_send_buf:		Pointer to transmit buffer
>   * @p_recv_buf:		Pointer to receive buffer
> - * @suspended:		Flag holding the device's PM status
>   * @send_count:		Number of bytes still expected to send
>   * @recv_count:		Number of bytes still expected to receive
>   * @curr_recv_count:	Number of bytes to be received in current transfer
> @@ -141,6 +143,7 @@
>   * @quirks:		flag for broken hold bit usage in r1p10
>   */
>  struct cdns_i2c {
> +	struct device		*dev;
>  	void __iomem *membase;
>  	struct i2c_adapter adap;
>  	struct i2c_msg *p_msg;
> @@ -148,7 +151,6 @@ struct cdns_i2c {
>  	struct completion xfer_done;
>  	unsigned char *p_send_buf;
>  	unsigned char *p_recv_buf;
> -	u8 suspended;

There might have been a reason to store this flag here. Did you test
this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
nothing that can sleep is called from atomic context.

	S?ren

[PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Wed, Oct 28, 2015 at 9:48 PM, S?ren Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> Hi Shubhrajyoti,
>
>
> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>> Currently the clocks are enabled at probe and disabled at remove.
>> Which keeps the clocks enabled even if no transaction is going on.
>> This patch enables the clocks at the start of transfer and disables
>> after it.
>>
>> Also adapts to runtime pm.
>> Remove xi2c->suspended and use pm runtime status instead.
>>
>> converts dev pm to const to silence a checkpatch warning.
>>
>> Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
>
> To me, this looks all good. Just one small concern below.

Thanks for the review.

>
>> ---
>> changes since v2
>> update the cc list
>>
>>  drivers/i2c/busses/i2c-cadence.c |   73 ++++++++++++++++++++++++--------------
>>  1 files changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
>> index 84deed6..6b08d16 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/of.h>
>> +#include <linux/pm_runtime.h>
>>
>>  /* Register offsets for the I2C device. */
>>  #define CDNS_I2C_CR_OFFSET           0x00 /* Control Register, RW */
>> @@ -96,6 +97,8 @@
>>                                        CDNS_I2C_IXR_COMP)
>>
>>  #define CDNS_I2C_TIMEOUT             msecs_to_jiffies(1000)
>> +/* timeout for pm runtime autosuspend */
>> +#define CNDS_I2C_PM_TIMEOUT          1000    /* ms */
>>
>>  #define CDNS_I2C_FIFO_DEPTH          16
>>  /* FIFO depth at which the DATA interrupt occurs */
>> @@ -128,7 +131,6 @@
>>   * @xfer_done:               Transfer complete status
>>   * @p_send_buf:              Pointer to transmit buffer
>>   * @p_recv_buf:              Pointer to receive buffer
>> - * @suspended:               Flag holding the device's PM status
>>   * @send_count:              Number of bytes still expected to send
>>   * @recv_count:              Number of bytes still expected to receive
>>   * @curr_recv_count: Number of bytes to be received in current transfer
>> @@ -141,6 +143,7 @@
>>   * @quirks:          flag for broken hold bit usage in r1p10
>>   */
>>  struct cdns_i2c {
>> +     struct device           *dev;
>>       void __iomem *membase;
>>       struct i2c_adapter adap;
>>       struct i2c_msg *p_msg;
>> @@ -148,7 +151,6 @@ struct cdns_i2c {
>>       struct completion xfer_done;
>>       unsigned char *p_send_buf;
>>       unsigned char *p_recv_buf;
>> -     u8 suspended;
>
> There might have been a reason to store this flag here. Did you test
> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
> nothing that can sleep is called from atomic context.
Done now.


Essentially this is a flag is set in suspend routine. and checked in
the isr I use
pm_runtime_suspended(id->dev) instead.

>
>         S?ren
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta
<shubhrajyoti.datta@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 9:48 PM, S?ren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
>> Hi Shubhrajyoti,
>>
>>
>> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>>> Currently the clocks are enabled at probe and disabled at remove.
>>> Which keeps the clocks enabled even if no transaction is going on.
>>> This patch enables the clocks at the start of transfer and disables
>>> after it.
>>>
>>> Also adapts to runtime pm.
>>> Remove xi2c->suspended and use pm runtime status instead.
>>>
>>> converts dev pm to const to silence a checkpatch warning.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
>>
>> To me, this looks all good. Just one small concern below.
>
> Thanks for the review.
Soren ,
Do are you ok with the change or do you want me to resend without the
suspended flag change.

<>
>>
>> There might have been a reason to store this flag here. Did you test
>> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
>> nothing that can sleep is called from atomic context.
> Done now.
>
>
> Essentially this is a flag is set in suspend routine. and checked in
> the isr I use
> pm_runtime_suspended(id->dev) instead.
>
>>
>>         S?ren
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] i2c: cadence: Move to sensible power management

[Sören Brinkmann]

On Sat, 2015-11-21 at 07:00PM +0530, Shubhrajyoti Datta wrote:
> On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta
> <shubhrajyoti.datta@gmail.com> wrote:
> > On Wed, Oct 28, 2015 at 9:48 PM, S?ren Brinkmann
> > <soren.brinkmann@xilinx.com> wrote:
> >> Hi Shubhrajyoti,
> >>
> >>
> >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
> >>> Currently the clocks are enabled at probe and disabled at remove.
> >>> Which keeps the clocks enabled even if no transaction is going on.
> >>> This patch enables the clocks at the start of transfer and disables
> >>> after it.
> >>>
> >>> Also adapts to runtime pm.
> >>> Remove xi2c->suspended and use pm runtime status instead.
> >>>
> >>> converts dev pm to const to silence a checkpatch warning.
> >>>
> >>> Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
> >>
> >> To me, this looks all good. Just one small concern below.
> >
> > Thanks for the review.
> Soren ,
> Do are you ok with the change or do you want me to resend without the
> suspended flag change.

I'm always for removing code that is not needed. If things are tested
and well and work without throwing any warnings I'm OK with it.

	S?ren

[PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Tue, Nov 24, 2015 at 12:17 AM, S?ren Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Sat, 2015-11-21 at 07:00PM +0530, Shubhrajyoti Datta wrote:
>> On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta
>> <shubhrajyoti.datta@gmail.com> wrote:
>> > On Wed, Oct 28, 2015 at 9:48 PM, S?ren Brinkmann
>> > <soren.brinkmann@xilinx.com> wrote:
>> >> Hi Shubhrajyoti,
>> >>
>> >>
>> >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>> >>> Currently the clocks are enabled at probe and disabled at remove.
>> >>> Which keeps the clocks enabled even if no transaction is going on.
>> >>> This patch enables the clocks at the start of transfer and disables
>> >>> after it.
>> >>>
>> >>> Also adapts to runtime pm.
>> >>> Remove xi2c->suspended and use pm runtime status instead.
>> >>>
>> >>> converts dev pm to const to silence a checkpatch warning.
>> >>>
>> >>> Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
>> >>
>> >> To me, this looks all good. Just one small concern below.
>> >
>> > Thanks for the review.
>> Soren ,
>> Do are you ok with the change or do you want me to resend without the
>> suspended flag change.
>
> I'm always for removing code that is not needed. If things are tested
> and well and work without throwing any warnings I'm OK with it.

It should be also having a suspended book-keeping in the driver is not
needed the pm does that for us.

I will spilt the patch and resend.

Thanks,

>
>         S?ren