[PATCH v3 0/3] i2c/at91: add support PM functions

[PATCH v3 0/3] i2c/at91: add support PM functions

[Wenyou Yang]

Hi Wolfram,

The patches is to add the PM functions support for the at91 i2c controller.

It is based on the i2c/for-next branch of 
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git.

Best Regards,
Wenyou Yang

------
Change log:
v2.0

According to the advice from Kevin Hilman, 
1./ Wrap the runtime suspend/resume functions in CONFIG_PM instead of CONFIG_PM_RUNTIME.
2./ Call the runtime suspend/resume functions directly in the system suspend/resume.

v3.0
   Covert the system suspend/resume to suspend_noirq/resume_noirq.

Wenyou Yang (3):
  i2c/at91: add support for runtime PM
  i2c/at91: add support for system PM
  i2c/at91: adopt pinctrl support

 drivers/i2c/busses/i2c-at91.c |   71 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/3] i2c/at91: add support for runtime PM

[Wenyou Yang]

Drivers should put the device into low power states proactively whenever the
device is not in use. Thus implement support for runtime PM and use the
autosuspend feature to make sure that we can still perform well in case we see
lots of i2c traffic within short period of time.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 drivers/i2c/busses/i2c-at91.c |   38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 917d545..a25fb5e 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -31,10 +31,12 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/platform_data/dma-atmel.h>
+#include <linux/pm_runtime.h>
 
 #define DEFAULT_TWI_CLK_HZ		100000		/* max 400 Kbits/s */
 #define AT91_I2C_TIMEOUT	msecs_to_jiffies(100)	/* transfer timeout */
 #define AT91_I2C_DMA_THRESHOLD	8			/* enable DMA if transfer size is bigger than this threshold */
+#define AUTOSUSPEND_TIMEOUT		2000
 
 /* AT91 TWI register definitions */
 #define	AT91_TWI_CR		0x0000	/* Control Register */
@@ -481,6 +483,10 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
 
 	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
 
+	ret = pm_runtime_get_sync(dev->dev);
+	if (ret < 0)
+		goto out;
+
 	/*
 	 * The hardware can handle@most two messages concatenated by a
 	 * repeated start via it's internal address feature.
@@ -488,18 +494,21 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
 	if (num > 2) {
 		dev_err(dev->dev,
 			"cannot handle more than two concatenated messages.\n");
-		return 0;
+		ret = 0;
+		goto out;
 	} else if (num == 2) {
 		int internal_address = 0;
 		int i;
 
 		if (msg->flags & I2C_M_RD) {
 			dev_err(dev->dev, "first transfer must be write.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		if (msg->len > 3) {
 			dev_err(dev->dev, "first message size must be <= 3.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 
 		/* 1st msg is put into the internal address, start with 2nd */
@@ -523,7 +532,12 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
 
 	ret = at91_do_twi_transfer(dev);
 
-	return (ret < 0) ? ret : num;
+	ret = (ret < 0) ? ret : num;
+out:
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+
+	return ret;
 }
 
 static u32 at91_twi_func(struct i2c_adapter *adapter)
@@ -795,11 +809,20 @@ static int at91_twi_probe(struct platform_device *pdev)
 	dev->adapter.timeout = AT91_I2C_TIMEOUT;
 	dev->adapter.dev.of_node = pdev->dev.of_node;
 
+	pm_runtime_set_autosuspend_delay(dev->dev, AUTOSUSPEND_TIMEOUT);
+	pm_runtime_use_autosuspend(dev->dev);
+	pm_runtime_set_active(dev->dev);
+	pm_runtime_enable(dev->dev);
+
 	rc = i2c_add_numbered_adapter(&dev->adapter);
 	if (rc) {
 		dev_err(dev->dev, "Adapter %s registration failed\n",
 			dev->adapter.name);
 		clk_disable_unprepare(dev->clk);
+
+		pm_runtime_disable(dev->dev);
+		pm_runtime_set_suspended(dev->dev);
+
 		return rc;
 	}
 
@@ -814,6 +837,9 @@ static int at91_twi_remove(struct platform_device *pdev)
 	i2c_del_adapter(&dev->adapter);
 	clk_disable_unprepare(dev->clk);
 
+	pm_runtime_disable(dev->dev);
+	pm_runtime_set_suspended(dev->dev);
+
 	return 0;
 }
 
@@ -823,7 +849,7 @@ static int at91_twi_runtime_suspend(struct device *dev)
 {
 	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
 
-	clk_disable(twi_dev->clk);
+	clk_disable_unprepare(twi_dev->clk);
 
 	return 0;
 }
@@ -832,7 +858,7 @@ static int at91_twi_runtime_resume(struct device *dev)
 {
 	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
 
-	return clk_enable(twi_dev->clk);
+	return clk_prepare_enable(twi_dev->clk);
 }
 
 static const struct dev_pm_ops at91_twi_pm = {
-- 
1.7.9.5

[PATCH v3 2/3] i2c/at91: add support for system PM

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 drivers/i2c/busses/i2c-at91.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index a25fb5e..6a84a2a 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -861,7 +861,33 @@ static int at91_twi_runtime_resume(struct device *dev)
 	return clk_prepare_enable(twi_dev->clk);
 }
 
+static int at91_twi_suspend_noirq(struct device *dev)
+{
+	if (!pm_runtime_status_suspended(dev))
+		at91_twi_runtime_suspend(dev);
+
+	return 0;
+}
+
+static int at91_twi_resume_noirq(struct device *dev)
+{
+	int ret;
+
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = at91_twi_runtime_resume(dev);
+		if (ret)
+			return ret;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_request_autosuspend(dev);
+
+	return 0;
+}
+
 static const struct dev_pm_ops at91_twi_pm = {
+	.suspend_noirq	= at91_twi_suspend_noirq,
+	.resume_noirq	= at91_twi_resume_noirq,
 	.runtime_suspend	= at91_twi_runtime_suspend,
 	.runtime_resume		= at91_twi_runtime_resume,
 };
-- 
1.7.9.5

[PATCH v3 3/3] i2c/at91: adopt pinctrl support

[Wenyou Yang]

Amend the i2c at91 pin controller to optionally take a pin control
handle and set the state of the pins to:

- "default" on boot, resume and before performing an transfer
- "sleep" on suspend()

This should make it possible to optimize energy usage for the pins
both for the suspend/resume cycle

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 drivers/i2c/busses/i2c-at91.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 6a84a2a..290caf1 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/platform_data/dma-atmel.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #define DEFAULT_TWI_CLK_HZ		100000		/* max 400 Kbits/s */
 #define AT91_I2C_TIMEOUT	msecs_to_jiffies(100)	/* transfer timeout */
@@ -747,6 +748,8 @@ static int at91_twi_probe(struct platform_device *pdev)
 	u32 phy_addr;
 	u32 bus_clk_rate;
 
+	pinctrl_pm_select_default_state(&pdev->dev);
+
 	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
@@ -851,6 +854,8 @@ static int at91_twi_runtime_suspend(struct device *dev)
 
 	clk_disable_unprepare(twi_dev->clk);
 
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }
 
@@ -858,6 +863,8 @@ static int at91_twi_runtime_resume(struct device *dev)
 {
 	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
 
+	pinctrl_pm_select_default_state(dev);
+
 	return clk_prepare_enable(twi_dev->clk);
 }
 
-- 
1.7.9.5

[PATCH v3 2/3] i2c/at91: add support for system PM

[Sergei Shtylyov]

Hello.

On 10/24/2014 10:50 AM, Wenyou Yang wrote:

> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>   drivers/i2c/busses/i2c-at91.c |   26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)

> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index a25fb5e..6a84a2a 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -861,7 +861,33 @@ static int at91_twi_runtime_resume(struct device *dev)
[...]
> +static int at91_twi_resume_noirq(struct device *dev)
> +{
> +	int ret;
> +
> +	if (!pm_runtime_status_suspended(dev)) {
> +		ret = at91_twi_runtime_resume(dev);

    Resume if *not* suspended?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_request_autosuspend(dev);
> +
> +	return 0;
> +}
> +
[...]

WBR, Sergei

[PATCH v3 2/3] i2c/at91: add support for system PM

[Yang, Wenyou]

Hi,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov at cogentembedded.com]
> Sent: Friday, October 24, 2014 9:03 PM
> To: Yang, Wenyou; wsa at the-dreams.de; Desroches, Ludovic
> Cc: khilman at kernel.org; Ferre, Nicolas; linux-kernel at vger.kernel.org; linux-
> i2c at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v3 2/3] i2c/at91: add support for system PM
> 
> Hello.
> 
> On 10/24/2014 10:50 AM, Wenyou Yang wrote:
> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >   drivers/i2c/busses/i2c-at91.c |   26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> 
> > diff --git a/drivers/i2c/busses/i2c-at91.c
> > b/drivers/i2c/busses/i2c-at91.c index a25fb5e..6a84a2a 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -861,7 +861,33 @@ static int at91_twi_runtime_resume(struct device
> > *dev)
> [...]
> > +static int at91_twi_resume_noirq(struct device *dev) {
> > +	int ret;
> > +
> > +	if (!pm_runtime_status_suspended(dev)) {
> > +		ret = at91_twi_runtime_resume(dev);
> 
>     Resume if *not* suspended?
Yes, if it is not runtime_suspended, it must be the system suspended, then resume it. I think so.

> 
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_request_autosuspend(dev);
> > +
> > +	return 0;
> > +}
> > +
> [...]
> 
> WBR, Sergei

Best Regards,
Wenyou Yang

[PATCH v3 0/3] i2c/at91: add support PM functions

[Ludovic Desroches]

For the whole serie

Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com>

On Fri, Oct 24, 2014 at 02:50:14PM +0800, Wenyou Yang wrote:
> Hi Wolfram,
> 
> The patches is to add the PM functions support for the at91 i2c controller.
> 
> It is based on the i2c/for-next branch of 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git.
> 
> Best Regards,
> Wenyou Yang
> 
> ------
> Change log:
> v2.0
> 
> According to the advice from Kevin Hilman, 
> 1./ Wrap the runtime suspend/resume functions in CONFIG_PM instead of CONFIG_PM_RUNTIME.
> 2./ Call the runtime suspend/resume functions directly in the system suspend/resume.
> 
> v3.0
>    Covert the system suspend/resume to suspend_noirq/resume_noirq.
> 
> Wenyou Yang (3):
>   i2c/at91: add support for runtime PM
>   i2c/at91: add support for system PM
>   i2c/at91: adopt pinctrl support
> 
>  drivers/i2c/busses/i2c-at91.c |   71 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> -- 
> 1.7.9.5
> 

[PATCH v3 0/3] i2c/at91: add support PM functions

[Yang, Wenyou]

Hi Wolfram,

Could you give me some advice?  Thank you in advance.

> -----Original Message-----
> From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
> Sent: Wednesday, October 29, 2014 4:24 PM
> To: Yang, Wenyou
> Cc: wsa at the-dreams.de; Desroches, Ludovic; linux-i2c at vger.kernel.org; linux-
> kernel at vger.kernel.org; Ferre, Nicolas; khilman at kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH v3 0/3] i2c/at91: add support PM functions
> 
> For the whole serie
> 
> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> 
> On Fri, Oct 24, 2014 at 02:50:14PM +0800, Wenyou Yang wrote:
> > Hi Wolfram,
> >
> > The patches is to add the PM functions support for the at91 i2c controller.
> >
> > It is based on the i2c/for-next branch of
> > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git.
> >
> > Best Regards,
> > Wenyou Yang
> >
> > ------
> > Change log:
> > v2.0
> >
> > According to the advice from Kevin Hilman, 1./ Wrap the runtime
> > suspend/resume functions in CONFIG_PM instead of CONFIG_PM_RUNTIME.
> > 2./ Call the runtime suspend/resume functions directly in the system
> suspend/resume.
> >
> > v3.0
> >    Covert the system suspend/resume to suspend_noirq/resume_noirq.
> >
> > Wenyou Yang (3):
> >   i2c/at91: add support for runtime PM
> >   i2c/at91: add support for system PM
> >   i2c/at91: adopt pinctrl support
> >
> >  drivers/i2c/busses/i2c-at91.c |   71
> +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 65 insertions(+), 6 deletions(-)
> >
> > --
> > 1.7.9.5
> >

Best Regards,
Wenyou Yang

[PATCH v3 3/3] i2c/at91: adopt pinctrl support

[Wolfram Sang]

On Fri, Oct 24, 2014 at 02:50:17PM +0800, Wenyou Yang wrote:
> Amend the i2c at91 pin controller to optionally take a pin control
> handle and set the state of the pins to:
> 
> - "default" on boot, resume and before performing an transfer
> - "sleep" on suspend()
> 
> This should make it possible to optimize energy usage for the pins
> both for the suspend/resume cycle
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  drivers/i2c/busses/i2c-at91.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 6a84a2a..290caf1 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -32,6 +32,7 @@
>  #include <linux/slab.h>
>  #include <linux/platform_data/dma-atmel.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #define DEFAULT_TWI_CLK_HZ		100000		/* max 400 Kbits/s */
>  #define AT91_I2C_TIMEOUT	msecs_to_jiffies(100)	/* transfer timeout */
> @@ -747,6 +748,8 @@ static int at91_twi_probe(struct platform_device *pdev)
>  	u32 phy_addr;
>  	u32 bus_clk_rate;
>  
> +	pinctrl_pm_select_default_state(&pdev->dev);
> +

Do we really need this in probe? Isn't that default? I'm not sure,
though...

>  	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -851,6 +854,8 @@ static int at91_twi_runtime_suspend(struct device *dev)
>  
>  	clk_disable_unprepare(twi_dev->clk);
>  
> +	pinctrl_pm_select_sleep_state(dev);
> +
>  	return 0;
>  }
>  
> @@ -858,6 +863,8 @@ static int at91_twi_runtime_resume(struct device *dev)
>  {
>  	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
>  
> +	pinctrl_pm_select_default_state(dev);
> +
>  	return clk_prepare_enable(twi_dev->clk);
>  }
>  
> -- 
> 1.7.9.5
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141108/0f6a9b4f/attachment.sig>

[PATCH v3 1/3] i2c/at91: add support for runtime PM

[Wolfram Sang]

On Fri, Oct 24, 2014 at 02:50:15PM +0800, Wenyou Yang wrote:
> Drivers should put the device into low power states proactively whenever the
> device is not in use. Thus implement support for runtime PM and use the
> autosuspend feature to make sure that we can still perform well in case we see
> lots of i2c traffic within short period of time.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>

Applied to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141108/ae51c155/attachment.sig>

[PATCH v3 2/3] i2c/at91: add support for system PM

[Wolfram Sang]

On Fri, Oct 24, 2014 at 02:50:16PM +0800, Wenyou Yang wrote:
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>

Applied to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141108/36b45769/attachment.sig>

[PATCH v3 3/3] i2c/at91: adopt pinctrl support

[Yang, Wenyou]

Hi Wolfram,

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa at the-dreams.de]
> Sent: Sunday, November 09, 2014 2:03 AM
> To: Yang, Wenyou
> Cc: Desroches, Ludovic; linux-i2c at vger.kernel.org; linux-kernel at vger.kernel.org;
> Ferre, Nicolas; khilman at kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v3 3/3] i2c/at91: adopt pinctrl support
> 
> On Fri, Oct 24, 2014 at 02:50:17PM +0800, Wenyou Yang wrote:
> > Amend the i2c at91 pin controller to optionally take a pin control
> > handle and set the state of the pins to:
> >
> > - "default" on boot, resume and before performing an transfer
> > - "sleep" on suspend()
> >
> > This should make it possible to optimize energy usage for the pins
> > both for the suspend/resume cycle
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  drivers/i2c/busses/i2c-at91.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c
> > b/drivers/i2c/busses/i2c-at91.c index 6a84a2a..290caf1 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/platform_data/dma-atmel.h>  #include
> > <linux/pm_runtime.h>
> > +#include <linux/pinctrl/consumer.h>
> >
> >  #define DEFAULT_TWI_CLK_HZ		100000		/* max 400 Kbits/s
> */
> >  #define AT91_I2C_TIMEOUT	msecs_to_jiffies(100)	/* transfer timeout */
> > @@ -747,6 +748,8 @@ static int at91_twi_probe(struct platform_device *pdev)
> >  	u32 phy_addr;
> >  	u32 bus_clk_rate;
> >
> > +	pinctrl_pm_select_default_state(&pdev->dev);
> > +
> 
> Do we really need this in probe? Isn't that default? I'm not sure, though...
No, we don't need it.
It is called by the function pinctrl_bind_pins(struct device *dev)  in drivers/base/pinctrl.c

I will remove it, and send it again.

> 
> >  	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> >  	if (!dev)
> >  		return -ENOMEM;
> > @@ -851,6 +854,8 @@ static int at91_twi_runtime_suspend(struct device
> > *dev)
> >
> >  	clk_disable_unprepare(twi_dev->clk);
> >
> > +	pinctrl_pm_select_sleep_state(dev);
> > +
> >  	return 0;
> >  }
> >
> > @@ -858,6 +863,8 @@ static int at91_twi_runtime_resume(struct device
> > *dev)  {
> >  	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> >
> > +	pinctrl_pm_select_default_state(dev);
> > +
> >  	return clk_prepare_enable(twi_dev->clk);  }
> >
> > --
> > 1.7.9.5
> >

Best Regards,
Wenyou Yang