[PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller

[PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller

[AnilKumar Ch]

Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
APIs control clocks for C_CAN/D_CAN IP and prevent access to the
register of C_CAN/D_CAN IP when clock is turned off.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
This patch has been tested on AM335X EVM. Due to lack of hardware
I am not able to test c_can functionality. I appreciate if anyone
can test c_can functionality with this patch.

This patch is based on "can-next/master" 

Changes from v4:
	- Incorporated Vaibhav H review comments on v4.
	  * Moved pm_runtime put/get_sync calls to appropriate positions.
	- This patch is from "Add DT support to C_CAN/D_CAN controller"
	  patch series. Rest of the patches in this series were applied
	  so this v5 contains only this patch.

 drivers/net/can/c_can/c_can.c          |    8 ++++++++
 drivers/net/can/c_can/c_can.h          |    1 +
 drivers/net/can/c_can/c_can_platform.c |   11 +++++++++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index eea6608..ca1151d 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,8 @@
 #include <linux/if_ether.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct net_device *dev,
 	unsigned int reg_err_counter;
 	struct c_can_priv *priv = netdev_priv(dev);
 
+	pm_runtime_get_sync(&priv->pdev->dev);
+
 	reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
 	bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
 				ERR_CNT_REC_SHIFT;
@@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
 	int err;
 	struct c_can_priv *priv = netdev_priv(dev);
 
+	pm_runtime_get_sync(&priv->pdev->dev);
+
 	/* open the can device */
 	err = open_candev(dev);
 	if (err) {
@@ -1079,6 +1085,7 @@ static int c_can_open(struct net_device *dev)
 
 exit_irq_fail:
 	close_candev(dev);
+	pm_runtime_put_sync(&priv->pdev->dev);
 	return err;
 }
 
@@ -1091,6 +1098,7 @@ static int c_can_close(struct net_device *dev)
 	c_can_stop(dev);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
+	pm_runtime_put_sync(&priv->pdev->dev);
 
 	return 0;
 }
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 4e56baa..ef84439 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -153,6 +153,7 @@ struct c_can_priv {
 	struct can_priv can;	/* must be the first member */
 	struct napi_struct napi;
 	struct net_device *dev;
+	struct platform_device *pdev;
 	int tx_object;
 	int current_status;
 	int last_status;
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index d0a66cf..cbb1184 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,6 +32,7 @@
 #include <linux/clk.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/can/dev.h>
 
@@ -177,7 +178,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		goto exit_free_device;
 	}
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	dev->irq = irq;
+	priv->pdev = pdev;
 	priv->base = addr;
 	priv->can.clock.freq = clk_get_rate(clk);
 	priv->priv = clk;
@@ -192,12 +197,16 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		goto exit_free_device;
 	}
 
+	pm_runtime_put_sync(&pdev->dev);
+
 	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
 		 KBUILD_MODNAME, priv->base, dev->irq);
 	return 0;
 
 exit_free_device:
 	platform_set_drvdata(pdev, NULL);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	free_c_can_dev(dev);
 exit_iounmap:
 	iounmap(addr);
@@ -226,6 +235,8 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(mem->start, resource_size(mem));
 
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	clk_put(priv->priv);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller

[Marc Kleine-Budde]

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

On 08/07/2012 01:20 PM, AnilKumar Ch wrote:
> Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
> APIs control clocks for C_CAN/D_CAN IP and prevent access to the
> register of C_CAN/D_CAN IP when clock is turned off.
> 
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
> This patch has been tested on AM335X EVM. Due to lack of hardware
> I am not able to test c_can functionality. I appreciate if anyone
> can test c_can functionality with this patch.

The "priv->pdev->dev" will not work for c_can/d_can which are not using
the platform driver. Options:
a) priv->dev
   add a struct device pointer to the priv, and
   1) guard pm_runtime_get_sync() for priv->dev == NULL
      (or check if pm_runtime_get_sync copes with NULL pointer)

   - or -

   2) fill in the priv->dev in all other drivers (i.e. PCI)
      and check if it makes sense/works to call it on PCI hardware

b) convert all other drivers, i.e. the PCI driver to create a
   platform device

Without thinking long, I suggest to a) with flavour 1)

Maybe there are better solutions.

Marc

More comments inline:

> 
> This patch is based on "can-next/master" 
> 
> Changes from v4:
> 	- Incorporated Vaibhav H review comments on v4.
> 	  * Moved pm_runtime put/get_sync calls to appropriate positions.
> 	- This patch is from "Add DT support to C_CAN/D_CAN controller"
> 	  patch series. Rest of the patches in this series were applied
> 	  so this v5 contains only this patch.
> 
>  drivers/net/can/c_can/c_can.c          |    8 ++++++++
>  drivers/net/can/c_can/c_can.h          |    1 +
>  drivers/net/can/c_can/c_can_platform.c |   11 +++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index eea6608..ca1151d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -34,6 +34,8 @@
>  #include <linux/if_ether.h>
>  #include <linux/list.h>
>  #include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_device.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct net_device *dev,
>  	unsigned int reg_err_counter;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> +	pm_runtime_get_sync(&priv->pdev->dev);
> +

Why do you make a get_sync() here, where's the correspondning put_sync()?

>  	reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
>  	bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
>  				ERR_CNT_REC_SHIFT;
> @@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
>  	int err;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> +	pm_runtime_get_sync(&priv->pdev->dev);
> +
>  	/* open the can device */
>  	err = open_candev(dev);
>  	if (err) {

What if open_candev() fails? Please add a jump label
exit_open_candev_fail and add thje put_sync() there.

		goto exit_open_candev_fail;

> @@ -1079,6 +1085,7 @@ static int c_can_open(struct net_device *dev)
>  
>  exit_irq_fail:
>  	close_candev(dev);
 exit_open_candev_fail:
> +	pm_runtime_put_sync(&priv->pdev->dev);
>  	return err;
>  }
>  
> @@ -1091,6 +1098,7 @@ static int c_can_close(struct net_device *dev)
>  	c_can_stop(dev);
>  	free_irq(dev->irq, dev);
>  	close_candev(dev);
> +	pm_runtime_put_sync(&priv->pdev->dev);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 4e56baa..ef84439 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,7 @@ struct c_can_priv {
>  	struct can_priv can;	/* must be the first member */
>  	struct napi_struct napi;
>  	struct net_device *dev;
> +	struct platform_device *pdev;
>  	int tx_object;
>  	int current_status;
>  	int last_status;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index d0a66cf..cbb1184 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,6 +32,7 @@
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <linux/can/dev.h>
>  
> @@ -177,7 +178,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  		goto exit_free_device;
>  	}
>  
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	dev->irq = irq;
> +	priv->pdev = pdev;
>  	priv->base = addr;
>  	priv->can.clock.freq = clk_get_rate(clk);
>  	priv->priv = clk;
> @@ -192,12 +197,16 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  		goto exit_free_device;
>  	}
>  
> +	pm_runtime_put_sync(&pdev->dev);
> +
>  	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
>  		 KBUILD_MODNAME, priv->base, dev->irq);
>  	return 0;
>  
>  exit_free_device:
>  	platform_set_drvdata(pdev, NULL);
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>  	free_c_can_dev(dev);
>  exit_iounmap:
>  	iounmap(addr);
> @@ -226,6 +235,8 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	release_mem_region(mem->start, resource_size(mem));
>  
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>  	clk_put(priv->priv);
>  
>  	return 0;
> 


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

RE: [PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller

[AnilKumar, Chimata]

Hi Marc,

Thanks for the review

On Tue, Aug 07, 2012 at 17:27:05, Marc Kleine-Budde wrote:
> On 08/07/2012 01:20 PM, AnilKumar Ch wrote:
> > Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
> > APIs control clocks for C_CAN/D_CAN IP and prevent access to the
> > register of C_CAN/D_CAN IP when clock is turned off.
> > 
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> > ---
> > This patch has been tested on AM335X EVM. Due to lack of hardware
> > I am not able to test c_can functionality. I appreciate if anyone
> > can test c_can functionality with this patch.
> 
> The "priv->pdev->dev" will not work for c_can/d_can which are not using
> the platform driver. Options:
> a) priv->dev
>    add a struct device pointer to the priv, and
>    1) guard pm_runtime_get_sync() for priv->dev == NULL
>       (or check if pm_runtime_get_sync copes with NULL pointer)
> 
>    - or -

This is a better option but to implement this current priv->dev
needs to move to priv->ndev. Then we can add dev pointer to priv
struct.

> 
>    2) fill in the priv->dev in all other drivers (i.e. PCI)
>       and check if it makes sense/works to call it on PCI hardware
> 
> b) convert all other drivers, i.e. the PCI driver to create a
>    platform device
>
> Without thinking long, I suggest to a) with flavour 1)
> 
> Maybe there are better solutions.
> 
> Marc
> 
> More comments inline:
> 
> > 
> > This patch is based on "can-next/master" 
> > 
> > Changes from v4:
> > 	- Incorporated Vaibhav H review comments on v4.
> > 	  * Moved pm_runtime put/get_sync calls to appropriate positions.
> > 	- This patch is from "Add DT support to C_CAN/D_CAN controller"
> > 	  patch series. Rest of the patches in this series were applied
> > 	  so this v5 contains only this patch.
> > 
> >  drivers/net/can/c_can/c_can.c          |    8 ++++++++
> >  drivers/net/can/c_can/c_can.h          |    1 +
> >  drivers/net/can/c_can/c_can_platform.c |   11 +++++++++++
> >  3 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index eea6608..ca1151d 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -34,6 +34,8 @@
> >  #include <linux/if_ether.h>
> >  #include <linux/list.h>
> >  #include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/platform_device.h>
> >  
> >  #include <linux/can.h>
> >  #include <linux/can/dev.h>
> > @@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct net_device *dev,
> >  	unsigned int reg_err_counter;
> >  	struct c_can_priv *priv = netdev_priv(dev);
> >  
> > +	pm_runtime_get_sync(&priv->pdev->dev);
> > +
> 
> Why do you make a get_sync() here, where's the correspondning put_sync()?

While setting the bitrate, top layers checks the status of
C_CAN/D_CAN BERR register to know the bit status. I will take
care properly.

> 
> >  	reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
> >  	bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
> >  				ERR_CNT_REC_SHIFT;
> > @@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
> >  	int err;
> >  	struct c_can_priv *priv = netdev_priv(dev);
> >  
> > +	pm_runtime_get_sync(&priv->pdev->dev);
> > +
> >  	/* open the can device */
> >  	err = open_candev(dev);
> >  	if (err) {
> 
> What if open_candev() fails? Please add a jump label
> exit_open_candev_fail and add thje put_sync() there.

Missed, I will take care in return path

Regards
AnilKumar