[PATCH v4] can: Convert to runtime_pm

[PATCH v4] can: Convert to runtime_pm

[Kedareswara rao Appana]

Instead of enabling/disabling clocks at several locations in the driver,
use the runtime_pm framework. This consolidates the actions for
runtime PM in the appropriate callbacks and makes the driver more
readable and mantainable.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Chnages for v4:
 - Updated with the review comments.
Changes for v3:
  - Converted the driver to use runtime_pm.
Changes for v2:
  - Removed the struct platform_device* from suspend/resume
    as suggest by Lothar.

 drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..c71f683 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/pm_runtime.h>
 
 #define DRIVER_NAME	"xilinx_can"
 
@@ -138,7 +139,7 @@ struct xcan_priv {
 	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
 	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
 			u32 val);
-	struct net_device *dev;
+	struct device *dev;
 	void __iomem *reg_base;
 	unsigned long irq_flags;
 	struct clk *bus_clk;
@@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
+
 	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
 			ndev->name, ndev);
 	if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
 		goto err;
 	}
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable device clock\n");
-		goto err_irq;
-	}
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable bus clock\n");
-		goto err_can_clk;
-	}
-
 	/* Set chip into reset mode */
 	ret = set_reset_mode(ndev);
 	if (ret < 0) {
 		netdev_err(ndev, "mode resetting failed!\n");
-		goto err_bus_clk;
+		goto err_irq;
 	}
 
 	/* Common open */
 	ret = open_candev(ndev);
 	if (ret)
-		goto err_bus_clk;
+		goto err_irq;
 
 	ret = xcan_chip_start(ndev);
 	if (ret < 0) {
@@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
 
 err_candev:
 	close_candev(ndev);
-err_bus_clk:
-	clk_disable_unprepare(priv->bus_clk);
-err_can_clk:
-	clk_disable_unprepare(priv->can_clk);
 err_irq:
 	free_irq(ndev->irq, ndev);
 err:
+	pm_runtime_put(priv->dev);
+
 	return ret;
 }
 
@@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
 	xcan_chip_stop(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
 	free_irq(ndev->irq, ndev);
 	close_candev(ndev);
 
 	can_led_event(ndev, CAN_LED_EVENT_STOP);
+	pm_runtime_put(priv->dev);
 
 	return 0;
 }
@@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret)
-		goto err;
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret)
-		goto err_clk;
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
 
 	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
 	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
 			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
 
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+	pm_runtime_put(priv->dev);
 
 	return 0;
-
-err_clk:
-	clk_disable_unprepare(priv->can_clk);
-err:
-	return ret;
 }
 
 
@@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
 
 /**
  * xcan_suspend - Suspend method for the driver
- * @dev:	Address of the platform_device structure
+ * @dev:	Address of the device structure
  *
  * Put the driver into low power mode.
- * Return: 0 always
+ * Return: 0 on success and failure value on error
  */
 static int __maybe_unused xcan_suspend(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
+/**
+ * xcan_resume - Resume from suspend
+ * @dev:	Address of the device structure
+ *
+ * Resume operation after suspend.
+ * Return: 0 on success and failure value on error
+ */
+static int __maybe_unused xcan_resume(struct device *dev)
+{
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_resume(dev);
+
+	return 0;
+
+}
+
+/**
+ * xcan_runtime_suspend - Runtime suspend method for the driver
+ * @dev:	Address of the device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
@@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
 }
 
 /**
- * xcan_resume - Resume from suspend
- * @dev:	Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev:	Address of the device structure
  *
  * Resume operation after suspend.
  * Return: 0 on success and failure value on error
  */
-static int __maybe_unused xcan_resume(struct device *dev)
+static int __maybe_unused xcan_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
@@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
 
 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
-	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	if (netif_running(ndev)) {
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
+static const struct dev_pm_ops xcan_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
+	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
+};
 
 /**
  * xcan_probe - Platform registration call
@@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = netdev_priv(ndev);
-	priv->dev = ndev;
+	priv->dev = &pdev->dev;
 	priv->can.bittiming_const = &xcan_bittiming_const;
 	priv->can.do_set_mode = xcan_do_set_mode;
 	priv->can.do_get_berr_counter = xcan_get_berr_counter;
@@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
 
 	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
+		pm_runtime_put(priv->dev);
 		goto err_unprepare_disable_busclk;
 	}
 
 	devm_can_led_init(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+
+	pm_runtime_put(&pdev->dev);
+
 	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
 			priv->reg_base, ndev->irq, priv->can.clock.freq,
 			priv->tx_max);
-- 
1.7.4

[PATCH v4] can: Convert to runtime_pm

[Kedareswara rao Appana]

Instead of enabling/disabling clocks at several locations in the driver,
use the runtime_pm framework. This consolidates the actions for
runtime PM in the appropriate callbacks and makes the driver more
readable and mantainable.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Chnages for v4:
 - Updated with the review comments.
Changes for v3:
  - Converted the driver to use runtime_pm.
Changes for v2:
  - Removed the struct platform_device* from suspend/resume
    as suggest by Lothar.

 drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..c71f683 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/pm_runtime.h>
 
 #define DRIVER_NAME	"xilinx_can"
 
@@ -138,7 +139,7 @@ struct xcan_priv {
 	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
 	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
 			u32 val);
-	struct net_device *dev;
+	struct device *dev;
 	void __iomem *reg_base;
 	unsigned long irq_flags;
 	struct clk *bus_clk;
@@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
+
 	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
 			ndev->name, ndev);
 	if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
 		goto err;
 	}
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable device clock\n");
-		goto err_irq;
-	}
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable bus clock\n");
-		goto err_can_clk;
-	}
-
 	/* Set chip into reset mode */
 	ret = set_reset_mode(ndev);
 	if (ret < 0) {
 		netdev_err(ndev, "mode resetting failed!\n");
-		goto err_bus_clk;
+		goto err_irq;
 	}
 
 	/* Common open */
 	ret = open_candev(ndev);
 	if (ret)
-		goto err_bus_clk;
+		goto err_irq;
 
 	ret = xcan_chip_start(ndev);
 	if (ret < 0) {
@@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
 
 err_candev:
 	close_candev(ndev);
-err_bus_clk:
-	clk_disable_unprepare(priv->bus_clk);
-err_can_clk:
-	clk_disable_unprepare(priv->can_clk);
 err_irq:
 	free_irq(ndev->irq, ndev);
 err:
+	pm_runtime_put(priv->dev);
+
 	return ret;
 }
 
@@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
 	xcan_chip_stop(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
 	free_irq(ndev->irq, ndev);
 	close_candev(ndev);
 
 	can_led_event(ndev, CAN_LED_EVENT_STOP);
+	pm_runtime_put(priv->dev);
 
 	return 0;
 }
@@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret)
-		goto err;
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret)
-		goto err_clk;
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
 
 	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
 	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
 			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
 
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+	pm_runtime_put(priv->dev);
 
 	return 0;
-
-err_clk:
-	clk_disable_unprepare(priv->can_clk);
-err:
-	return ret;
 }
 
 
@@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
 
 /**
  * xcan_suspend - Suspend method for the driver
- * @dev:	Address of the platform_device structure
+ * @dev:	Address of the device structure
  *
  * Put the driver into low power mode.
- * Return: 0 always
+ * Return: 0 on success and failure value on error
  */
 static int __maybe_unused xcan_suspend(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
+/**
+ * xcan_resume - Resume from suspend
+ * @dev:	Address of the device structure
+ *
+ * Resume operation after suspend.
+ * Return: 0 on success and failure value on error
+ */
+static int __maybe_unused xcan_resume(struct device *dev)
+{
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_resume(dev);
+
+	return 0;
+
+}
+
+/**
+ * xcan_runtime_suspend - Runtime suspend method for the driver
+ * @dev:	Address of the device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
@@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
 }
 
 /**
- * xcan_resume - Resume from suspend
- * @dev:	Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev:	Address of the device structure
  *
  * Resume operation after suspend.
  * Return: 0 on success and failure value on error
  */
-static int __maybe_unused xcan_resume(struct device *dev)
+static int __maybe_unused xcan_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
@@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
 
 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
-	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	if (netif_running(ndev)) {
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
+static const struct dev_pm_ops xcan_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
+	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
+};
 
 /**
  * xcan_probe - Platform registration call
@@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = netdev_priv(ndev);
-	priv->dev = ndev;
+	priv->dev = &pdev->dev;
 	priv->can.bittiming_const = &xcan_bittiming_const;
 	priv->can.do_set_mode = xcan_do_set_mode;
 	priv->can.do_get_berr_counter = xcan_get_berr_counter;
@@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
 
 	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
+		pm_runtime_put(priv->dev);
 		goto err_unprepare_disable_busclk;
 	}
 
 	devm_can_led_init(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+
+	pm_runtime_put(&pdev->dev);
+
 	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
 			priv->reg_base, ndev->irq, priv->can.clock.freq,
 			priv->tx_max);
-- 
1.7.4

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the driver,
> use the runtime_pm framework. This consolidates the actions for
> runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
> 
>  drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
>  
> @@ -138,7 +139,7 @@ struct xcan_priv {
>  	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>  	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>  			u32 val);
> -	struct net_device *dev;
> +	struct device *dev;
>  	void __iomem *reg_base;
>  	unsigned long irq_flags;
>  	struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
	netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",

[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> -	ret = clk_prepare_enable(priv->can_clk);
> -	if (ret)
> -		goto err;
> -
> -	ret = clk_prepare_enable(priv->bus_clk);
> -	if (ret)
> -		goto err_clk;
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

ditto

	Sören

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the driver,
> use the runtime_pm framework. This consolidates the actions for
> runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
> 
>  drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
>  
> @@ -138,7 +139,7 @@ struct xcan_priv {
>  	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>  	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>  			u32 val);
> -	struct net_device *dev;
> +	struct device *dev;
>  	void __iomem *reg_base;
>  	unsigned long irq_flags;
>  	struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
	netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",

[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> -	ret = clk_prepare_enable(priv->can_clk);
> -	if (ret)
> -		goto err;
> -
> -	ret = clk_prepare_enable(priv->bus_clk);
> -	if (ret)
> -		goto err_clk;
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

ditto

	Sören

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

        Could you please  provide your feedback on this patch ?

@Soren : Will update the driver with your comments during next version of the patch.

Regards,
Kedar.

-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Wednesday, December 24, 2014 4:13 AM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v4] can: Convert to runtime_pm

On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
>
>  drivers/net/can/xilinx_can.c |  123
> +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
>  #define DRIVER_NAME  "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
>       u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>       void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>                       u32 val);
> -     struct net_device *dev;
> +     struct device *dev;
>       void __iomem *reg_base;
>       unsigned long irq_flags;
>       struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
        netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",

[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> -     ret = clk_prepare_enable(priv->can_clk);
> -     if (ret)
> -             goto err;
> -
> -     ret = clk_prepare_enable(priv->bus_clk);
> -     if (ret)
> -             goto err_clk;
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

ditto

        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3629 bytes --]

Hi Marc,

        Could you please  provide your feedback on this patch ?

@Soren : Will update the driver with your comments during next version of the patch.

Regards,
Kedar.

-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Wednesday, December 24, 2014 4:13 AM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v4] can: Convert to runtime_pm

On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
>
>  drivers/net/can/xilinx_can.c |  123
> +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
>  #define DRIVER_NAME  "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
>       u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>       void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>                       u32 val);
> -     struct net_device *dev;
> +     struct device *dev;
>       void __iomem *reg_base;
>       unsigned long irq_flags;
>       struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
        netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",

[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> -     ret = clk_prepare_enable(priv->can_clk);
> -     if (ret)
> -             goto err;
> -
> -     ret = clk_prepare_enable(priv->bus_clk);
> -     if (ret)
> -             goto err_clk;
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

ditto

        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

        Could you please  provide your feedback on this patch ?

@Soren : Will update the driver with your comments during next version of the patch.

Regards,
Kedar.

-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Wednesday, December 24, 2014 4:13 AM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v4] can: Convert to runtime_pm

On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
>
>  drivers/net/can/xilinx_can.c |  123
> +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
>  #define DRIVER_NAME  "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
>       u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>       void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>                       u32 val);
> -     struct net_device *dev;
> +     struct device *dev;
>       void __iomem *reg_base;
>       unsigned long irq_flags;
>       struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
        netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",

[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>       struct xcan_priv *priv = netdev_priv(ndev);
>       int ret;
>
> -     ret = clk_prepare_enable(priv->can_clk);
> -     if (ret)
> -             goto err;
> -
> -     ret = clk_prepare_enable(priv->bus_clk);
> -     if (ret)
> -             goto err_clk;
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",

ditto

        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/06/2015 07:23 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
> 
>         Could you please  provide your feedback on this patch ?
> 
> @Soren : Will update the driver with your comments during next version of the patch.

Sure, I'll return from my season holidays and will be in the office
tomorrow.

Marc

-- 
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: 819 bytes --]

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the driver,
> use the runtime_pm framework. This consolidates the actions for
> runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
>  - Updated with the review comments.
> Changes for v3:
>   - Converted the driver to use runtime_pm.
> Changes for v2:
>   - Removed the struct platform_device* from suspend/resume
>     as suggest by Lothar.
> 
>  drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
>  
> @@ -138,7 +139,7 @@ struct xcan_priv {
>  	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>  	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>  			u32 val);
> -	struct net_device *dev;
> +	struct device *dev;
>  	void __iomem *reg_base;
>  	unsigned long irq_flags;
>  	struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> +				__func__, ret);
> +		return ret;
> +	}
> +
>  	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
>  			ndev->name, ndev);
>  	if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
>  		goto err;
>  	}
>  
> -	ret = clk_prepare_enable(priv->can_clk);
> -	if (ret) {
> -		netdev_err(ndev, "unable to enable device clock\n");
> -		goto err_irq;
> -	}
> -
> -	ret = clk_prepare_enable(priv->bus_clk);
> -	if (ret) {
> -		netdev_err(ndev, "unable to enable bus clock\n");
> -		goto err_can_clk;
> -	}
> -
>  	/* Set chip into reset mode */
>  	ret = set_reset_mode(ndev);
>  	if (ret < 0) {
>  		netdev_err(ndev, "mode resetting failed!\n");
> -		goto err_bus_clk;
> +		goto err_irq;
>  	}
>  
>  	/* Common open */
>  	ret = open_candev(ndev);
>  	if (ret)
> -		goto err_bus_clk;
> +		goto err_irq;
>  
>  	ret = xcan_chip_start(ndev);
>  	if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>  
>  err_candev:
>  	close_candev(ndev);
> -err_bus_clk:
> -	clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> -	clk_disable_unprepare(priv->can_clk);
>  err_irq:
>  	free_irq(ndev->irq, ndev);
>  err:
> +	pm_runtime_put(priv->dev);
> +
>  	return ret;
>  }
>  
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
>  	xcan_chip_stop(ndev);
> -	clk_disable_unprepare(priv->bus_clk);
> -	clk_disable_unprepare(priv->can_clk);
>  	free_irq(ndev->irq, ndev);
>  	close_candev(ndev);
>  
>  	can_led_event(ndev, CAN_LED_EVENT_STOP);
> +	pm_runtime_put(priv->dev);
>  
>  	return 0;
>  }
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> -	ret = clk_prepare_enable(priv->can_clk);
> -	if (ret)
> -		goto err;
> -
> -	ret = clk_prepare_enable(priv->bus_clk);
> -	if (ret)
> -		goto err_clk;
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> +				__func__, ret);

Please remove the \r from the error messages.

> +		return ret;
> +	}
>  
>  	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
>  	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
>  			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
>  
> -	clk_disable_unprepare(priv->bus_clk);
> -	clk_disable_unprepare(priv->can_clk);
> +	pm_runtime_put(priv->dev);
>  
>  	return 0;
> -
> -err_clk:
> -	clk_disable_unprepare(priv->can_clk);
> -err:
> -	return ret;
>  }
>  
>  
> @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
>  
>  /**
>   * xcan_suspend - Suspend method for the driver
> - * @dev:	Address of the platform_device structure
> + * @dev:	Address of the device structure
>   *
>   * Put the driver into low power mode.
> - * Return: 0 always
> + * Return: 0 on success and failure value on error
>   */
>  static int __maybe_unused xcan_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = dev_get_drvdata(dev);
> -	struct net_device *ndev = platform_get_drvdata(pdev);
> +	if (!device_may_wakeup(dev))
> +		return pm_runtime_force_suspend(dev);
> +
> +	return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev:	Address of the device structure
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error
> + */
> +static int __maybe_unused xcan_resume(struct device *dev)
> +{
> +	if (!device_may_wakeup(dev))
> +		return pm_runtime_force_resume(dev);
> +
> +	return 0;
> +
> +}
> +
> +/**
> + * xcan_runtime_suspend - Runtime suspend method for the driver
> + * @dev:	Address of the device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int __maybe_unused xcan_runtime_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
>  }
>  
>  /**
> - * xcan_resume - Resume from suspend
> - * @dev:	Address of the platformdevice structure
> + * xcan_runtime_resume - Runtime resume from suspend
> + * @dev:	Address of the device structure
>   *
>   * Resume operation after suspend.
>   * Return: 0 on success and failure value on error
>   */
> -static int __maybe_unused xcan_resume(struct device *dev)
> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
>  {
> -	struct platform_device *pdev = dev_get_drvdata(dev);
> -	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;

Some more context:

> 	ret = clk_enable(priv->bus_clk);
> 	if (ret) {
> 		dev_err(dev, "Cannot enable clock.\n");
> 		return ret;
> 	}
> 	ret = clk_enable(priv->can_clk);
> 	if (ret) {
> 		dev_err(dev, "Cannot enable clock.\n");
> 		clk_disable_unprepare(priv->bus_clk);

This disable_unprepare looks wrong, should be a disable only.

> 		return ret;
> 	}
> 
> 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> 
> 	if (netif_running(ndev)) {
> 		priv->can.state = CAN_STATE_ERROR_ACTIVE;

What happens if the device was not in ACTIVE state prior to the
runtime_suspend?

> 		netif_device_attach(ndev);
> 		netif_start_queue(ndev);
> 	}
> 
> 	return 0;
> }


>  
> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  
>  	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>  	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> -	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
>  	if (netif_running(ndev)) {
> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  		netif_device_attach(ndev);
>  		netif_start_queue(ndev);
>  	}
> @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> +	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> +};
>  
>  /**
>   * xcan_probe - Platform registration call
> @@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	priv = netdev_priv(ndev);
> -	priv->dev = ndev;
> +	priv->dev = &pdev->dev;
>  	priv->can.bittiming_const = &xcan_bittiming_const;
>  	priv->can.do_set_mode = xcan_do_set_mode;
>  	priv->can.do_get_berr_counter = xcan_get_berr_counter;
> @@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>  
>  	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>  
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_irq_safe(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
Check error values?
> +
>  	ret = register_candev(ndev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> +		pm_runtime_put(priv->dev);

Please move the pm_runtime_put into the common error exit path.

>  		goto err_unprepare_disable_busclk;
>  	}
>  
>  	devm_can_led_init(ndev);
> -	clk_disable_unprepare(priv->bus_clk);
> -	clk_disable_unprepare(priv->can_clk);
> +
> +	pm_runtime_put(&pdev->dev);
> +
>  	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
>  			priv->reg_base, ndev->irq, priv->can.clock.freq,
>  			priv->tx_max);
> 

I think you have to convert the _remove() function, too. Have a look at
the gpio-zynq.c driver:

> static int zynq_gpio_remove(struct platform_device *pdev)
> {
> 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> 
> 	pm_runtime_get_sync(&pdev->dev);

However I don't understand why the get_sync() is here. Maybe Sören can help?
	
> 	gpiochip_remove(&gpio->chip);
> 	clk_disable_unprepare(gpio->clk);
> 	device_set_wakeup_capable(&pdev->dev, 0);
> 	return 0;
> }

regards,
Marc

-- 
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: 819 bytes --]

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Wed, 2015-01-07 at 01:28PM +0100, Marc Kleine-Budde wrote:
> On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the driver,
> > use the runtime_pm framework. This consolidates the actions for
> > runtime PM in the appropriate callbacks and makes the driver more
> > readable and mantainable.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Chnages for v4:
> >  - Updated with the review comments.
> > Changes for v3:
> >   - Converted the driver to use runtime_pm.
> > Changes for v2:
> >   - Removed the struct platform_device* from suspend/resume
> >     as suggest by Lothar.
> > 
> >  drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
> >  1 files changed, 74 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> > index 6c67643..c71f683 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> >  #include <linux/can/led.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #define DRIVER_NAME	"xilinx_can"
> >  
> > @@ -138,7 +139,7 @@ struct xcan_priv {
> >  	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> >  	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> >  			u32 val);
> > -	struct net_device *dev;
> > +	struct device *dev;
> >  	void __iomem *reg_base;
> >  	unsigned long irq_flags;
> >  	struct clk *bus_clk;
> > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> >  
> > +	ret = pm_runtime_get_sync(priv->dev);
> > +	if (ret < 0) {
> > +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +				__func__, ret);
> > +		return ret;
> > +	}
> > +
> >  	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> >  			ndev->name, ndev);
> >  	if (ret < 0) {
> > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> >  		goto err;
> >  	}
> >  
> > -	ret = clk_prepare_enable(priv->can_clk);
> > -	if (ret) {
> > -		netdev_err(ndev, "unable to enable device clock\n");
> > -		goto err_irq;
> > -	}
> > -
> > -	ret = clk_prepare_enable(priv->bus_clk);
> > -	if (ret) {
> > -		netdev_err(ndev, "unable to enable bus clock\n");
> > -		goto err_can_clk;
> > -	}
> > -
> >  	/* Set chip into reset mode */
> >  	ret = set_reset_mode(ndev);
> >  	if (ret < 0) {
> >  		netdev_err(ndev, "mode resetting failed!\n");
> > -		goto err_bus_clk;
> > +		goto err_irq;
> >  	}
> >  
> >  	/* Common open */
> >  	ret = open_candev(ndev);
> >  	if (ret)
> > -		goto err_bus_clk;
> > +		goto err_irq;
> >  
> >  	ret = xcan_chip_start(ndev);
> >  	if (ret < 0) {
> > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
> >  
> >  err_candev:
> >  	close_candev(ndev);
> > -err_bus_clk:
> > -	clk_disable_unprepare(priv->bus_clk);
> > -err_can_clk:
> > -	clk_disable_unprepare(priv->can_clk);
> >  err_irq:
> >  	free_irq(ndev->irq, ndev);
> >  err:
> > +	pm_runtime_put(priv->dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> >  	netif_stop_queue(ndev);
> >  	napi_disable(&priv->napi);
> >  	xcan_chip_stop(ndev);
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> >  	free_irq(ndev->irq, ndev);
> >  	close_candev(ndev);
> >  
> >  	can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +	pm_runtime_put(priv->dev);
> >  
> >  	return 0;
> >  }
> > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> >  
> > -	ret = clk_prepare_enable(priv->can_clk);
> > -	if (ret)
> > -		goto err;
> > -
> > -	ret = clk_prepare_enable(priv->bus_clk);
> > -	if (ret)
> > -		goto err_clk;
> > +	ret = pm_runtime_get_sync(priv->dev);
> > +	if (ret < 0) {
> > +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +				__func__, ret);
> 
> Please remove the \r from the error messages.
> 
> > +		return ret;
> > +	}
> >  
> >  	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> >  	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> >  			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> >  
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> > +	pm_runtime_put(priv->dev);
> >  
> >  	return 0;
> > -
> > -err_clk:
> > -	clk_disable_unprepare(priv->can_clk);
> > -err:
> > -	return ret;
> >  }
> >  
> >  
> > @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
> >  
> >  /**
> >   * xcan_suspend - Suspend method for the driver
> > - * @dev:	Address of the platform_device structure
> > + * @dev:	Address of the device structure
> >   *
> >   * Put the driver into low power mode.
> > - * Return: 0 always
> > + * Return: 0 on success and failure value on error
> >   */
> >  static int __maybe_unused xcan_suspend(struct device *dev)
> >  {
> > -	struct platform_device *pdev = dev_get_drvdata(dev);
> > -	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	if (!device_may_wakeup(dev))
> > +		return pm_runtime_force_suspend(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev:	Address of the device structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error
> > + */
> > +static int __maybe_unused xcan_resume(struct device *dev)
> > +{
> > +	if (!device_may_wakeup(dev))
> > +		return pm_runtime_force_resume(dev);
> > +
> > +	return 0;
> > +
> > +}
> > +
> > +/**
> > + * xcan_runtime_suspend - Runtime suspend method for the driver
> > + * @dev:	Address of the device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int __maybe_unused xcan_runtime_suspend(struct device *dev)
> > +{
> > +	struct net_device *ndev = dev_get_drvdata(dev);
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  
> >  	if (netif_running(ndev)) {
> > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
> >  }
> >  
> >  /**
> > - * xcan_resume - Resume from suspend
> > - * @dev:	Address of the platformdevice structure
> > + * xcan_runtime_resume - Runtime resume from suspend
> > + * @dev:	Address of the device structure
> >   *
> >   * Resume operation after suspend.
> >   * Return: 0 on success and failure value on error
> >   */
> > -static int __maybe_unused xcan_resume(struct device *dev)
> > +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> >  {
> > -	struct platform_device *pdev = dev_get_drvdata(dev);
> > -	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	struct net_device *ndev = dev_get_drvdata(dev);
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> 
> Some more context:
> 
> > 	ret = clk_enable(priv->bus_clk);
> > 	if (ret) {
> > 		dev_err(dev, "Cannot enable clock.\n");
> > 		return ret;
> > 	}
> > 	ret = clk_enable(priv->can_clk);
> > 	if (ret) {
> > 		dev_err(dev, "Cannot enable clock.\n");
> > 		clk_disable_unprepare(priv->bus_clk);
> 
> This disable_unprepare looks wrong, should be a disable only.
> 
> > 		return ret;
> > 	}
> > 
> > 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > 
> > 	if (netif_running(ndev)) {
> > 		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> 
> What happens if the device was not in ACTIVE state prior to the
> runtime_suspend?
> 
> > 		netif_device_attach(ndev);
> > 		netif_start_queue(ndev);
> > 	}
> > 
> > 	return 0;
> > }
> 
> 
> >  
> > @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >  
> >  	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >  	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > -	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  
> >  	if (netif_running(ndev)) {
> > +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  		netif_device_attach(ndev);
> >  		netif_start_queue(ndev);
> >  	}
> > @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> > +static const struct dev_pm_ops xcan_dev_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> > +	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> > +};
> >  
> >  /**
> >   * xcan_probe - Platform registration call
> > @@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	priv = netdev_priv(ndev);
> > -	priv->dev = ndev;
> > +	priv->dev = &pdev->dev;
> >  	priv->can.bittiming_const = &xcan_bittiming_const;
> >  	priv->can.do_set_mode = xcan_do_set_mode;
> >  	priv->can.do_get_berr_counter = xcan_get_berr_counter;
> > @@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >  
> >  	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >  
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_irq_safe(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> Check error values?
> > +
> >  	ret = register_candev(ndev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> > +		pm_runtime_put(priv->dev);
> 
> Please move the pm_runtime_put into the common error exit path.
> 
> >  		goto err_unprepare_disable_busclk;
> >  	}
> >  
> >  	devm_can_led_init(ndev);
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> > +
> > +	pm_runtime_put(&pdev->dev);
> > +
> >  	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> >  			priv->reg_base, ndev->irq, priv->can.clock.freq,
> >  			priv->tx_max);
> > 
> 
> I think you have to convert the _remove() function, too. Have a look at
> the gpio-zynq.c driver:
> 
> > static int zynq_gpio_remove(struct platform_device *pdev)
> > {
> > 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > 
> > 	pm_runtime_get_sync(&pdev->dev);
> 
> However I don't understand why the get_sync() is here. Maybe Sören can help?

IIRC, the concern was that the remove function may be called while the device is
runtime suspended. Hence the remove function needs to resume the device since the
remove function may access the HW.

	Sören

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Wed, 2015-01-07 at 01:28PM +0100, Marc Kleine-Budde wrote:
> On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the driver,
> > use the runtime_pm framework. This consolidates the actions for
> > runtime PM in the appropriate callbacks and makes the driver more
> > readable and mantainable.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Chnages for v4:
> >  - Updated with the review comments.
> > Changes for v3:
> >   - Converted the driver to use runtime_pm.
> > Changes for v2:
> >   - Removed the struct platform_device* from suspend/resume
> >     as suggest by Lothar.
> > 
> >  drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
> >  1 files changed, 74 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> > index 6c67643..c71f683 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> >  #include <linux/can/led.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #define DRIVER_NAME	"xilinx_can"
> >  
> > @@ -138,7 +139,7 @@ struct xcan_priv {
> >  	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> >  	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> >  			u32 val);
> > -	struct net_device *dev;
> > +	struct device *dev;
> >  	void __iomem *reg_base;
> >  	unsigned long irq_flags;
> >  	struct clk *bus_clk;
> > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> >  
> > +	ret = pm_runtime_get_sync(priv->dev);
> > +	if (ret < 0) {
> > +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +				__func__, ret);
> > +		return ret;
> > +	}
> > +
> >  	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> >  			ndev->name, ndev);
> >  	if (ret < 0) {
> > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> >  		goto err;
> >  	}
> >  
> > -	ret = clk_prepare_enable(priv->can_clk);
> > -	if (ret) {
> > -		netdev_err(ndev, "unable to enable device clock\n");
> > -		goto err_irq;
> > -	}
> > -
> > -	ret = clk_prepare_enable(priv->bus_clk);
> > -	if (ret) {
> > -		netdev_err(ndev, "unable to enable bus clock\n");
> > -		goto err_can_clk;
> > -	}
> > -
> >  	/* Set chip into reset mode */
> >  	ret = set_reset_mode(ndev);
> >  	if (ret < 0) {
> >  		netdev_err(ndev, "mode resetting failed!\n");
> > -		goto err_bus_clk;
> > +		goto err_irq;
> >  	}
> >  
> >  	/* Common open */
> >  	ret = open_candev(ndev);
> >  	if (ret)
> > -		goto err_bus_clk;
> > +		goto err_irq;
> >  
> >  	ret = xcan_chip_start(ndev);
> >  	if (ret < 0) {
> > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
> >  
> >  err_candev:
> >  	close_candev(ndev);
> > -err_bus_clk:
> > -	clk_disable_unprepare(priv->bus_clk);
> > -err_can_clk:
> > -	clk_disable_unprepare(priv->can_clk);
> >  err_irq:
> >  	free_irq(ndev->irq, ndev);
> >  err:
> > +	pm_runtime_put(priv->dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> >  	netif_stop_queue(ndev);
> >  	napi_disable(&priv->napi);
> >  	xcan_chip_stop(ndev);
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> >  	free_irq(ndev->irq, ndev);
> >  	close_candev(ndev);
> >  
> >  	can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +	pm_runtime_put(priv->dev);
> >  
> >  	return 0;
> >  }
> > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> >  
> > -	ret = clk_prepare_enable(priv->can_clk);
> > -	if (ret)
> > -		goto err;
> > -
> > -	ret = clk_prepare_enable(priv->bus_clk);
> > -	if (ret)
> > -		goto err_clk;
> > +	ret = pm_runtime_get_sync(priv->dev);
> > +	if (ret < 0) {
> > +		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +				__func__, ret);
> 
> Please remove the \r from the error messages.
> 
> > +		return ret;
> > +	}
> >  
> >  	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> >  	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> >  			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> >  
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> > +	pm_runtime_put(priv->dev);
> >  
> >  	return 0;
> > -
> > -err_clk:
> > -	clk_disable_unprepare(priv->can_clk);
> > -err:
> > -	return ret;
> >  }
> >  
> >  
> > @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
> >  
> >  /**
> >   * xcan_suspend - Suspend method for the driver
> > - * @dev:	Address of the platform_device structure
> > + * @dev:	Address of the device structure
> >   *
> >   * Put the driver into low power mode.
> > - * Return: 0 always
> > + * Return: 0 on success and failure value on error
> >   */
> >  static int __maybe_unused xcan_suspend(struct device *dev)
> >  {
> > -	struct platform_device *pdev = dev_get_drvdata(dev);
> > -	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	if (!device_may_wakeup(dev))
> > +		return pm_runtime_force_suspend(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev:	Address of the device structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error
> > + */
> > +static int __maybe_unused xcan_resume(struct device *dev)
> > +{
> > +	if (!device_may_wakeup(dev))
> > +		return pm_runtime_force_resume(dev);
> > +
> > +	return 0;
> > +
> > +}
> > +
> > +/**
> > + * xcan_runtime_suspend - Runtime suspend method for the driver
> > + * @dev:	Address of the device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int __maybe_unused xcan_runtime_suspend(struct device *dev)
> > +{
> > +	struct net_device *ndev = dev_get_drvdata(dev);
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  
> >  	if (netif_running(ndev)) {
> > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
> >  }
> >  
> >  /**
> > - * xcan_resume - Resume from suspend
> > - * @dev:	Address of the platformdevice structure
> > + * xcan_runtime_resume - Runtime resume from suspend
> > + * @dev:	Address of the device structure
> >   *
> >   * Resume operation after suspend.
> >   * Return: 0 on success and failure value on error
> >   */
> > -static int __maybe_unused xcan_resume(struct device *dev)
> > +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> >  {
> > -	struct platform_device *pdev = dev_get_drvdata(dev);
> > -	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	struct net_device *ndev = dev_get_drvdata(dev);
> >  	struct xcan_priv *priv = netdev_priv(ndev);
> >  	int ret;
> 
> Some more context:
> 
> > 	ret = clk_enable(priv->bus_clk);
> > 	if (ret) {
> > 		dev_err(dev, "Cannot enable clock.\n");
> > 		return ret;
> > 	}
> > 	ret = clk_enable(priv->can_clk);
> > 	if (ret) {
> > 		dev_err(dev, "Cannot enable clock.\n");
> > 		clk_disable_unprepare(priv->bus_clk);
> 
> This disable_unprepare looks wrong, should be a disable only.
> 
> > 		return ret;
> > 	}
> > 
> > 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > 
> > 	if (netif_running(ndev)) {
> > 		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> 
> What happens if the device was not in ACTIVE state prior to the
> runtime_suspend?
> 
> > 		netif_device_attach(ndev);
> > 		netif_start_queue(ndev);
> > 	}
> > 
> > 	return 0;
> > }
> 
> 
> >  
> > @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >  
> >  	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >  	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > -	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  
> >  	if (netif_running(ndev)) {
> > +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  		netif_device_attach(ndev);
> >  		netif_start_queue(ndev);
> >  	}
> > @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> > +static const struct dev_pm_ops xcan_dev_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> > +	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> > +};
> >  
> >  /**
> >   * xcan_probe - Platform registration call
> > @@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	priv = netdev_priv(ndev);
> > -	priv->dev = ndev;
> > +	priv->dev = &pdev->dev;
> >  	priv->can.bittiming_const = &xcan_bittiming_const;
> >  	priv->can.do_set_mode = xcan_do_set_mode;
> >  	priv->can.do_get_berr_counter = xcan_get_berr_counter;
> > @@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >  
> >  	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >  
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_irq_safe(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> Check error values?
> > +
> >  	ret = register_candev(ndev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> > +		pm_runtime_put(priv->dev);
> 
> Please move the pm_runtime_put into the common error exit path.
> 
> >  		goto err_unprepare_disable_busclk;
> >  	}
> >  
> >  	devm_can_led_init(ndev);
> > -	clk_disable_unprepare(priv->bus_clk);
> > -	clk_disable_unprepare(priv->can_clk);
> > +
> > +	pm_runtime_put(&pdev->dev);
> > +
> >  	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> >  			priv->reg_base, ndev->irq, priv->can.clock.freq,
> >  			priv->tx_max);
> > 
> 
> I think you have to convert the _remove() function, too. Have a look at
> the gpio-zynq.c driver:
> 
> > static int zynq_gpio_remove(struct platform_device *pdev)
> > {
> > 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > 
> > 	pm_runtime_get_sync(&pdev->dev);
> 
> However I don't understand why the get_sync() is here. Maybe Sören can help?

IIRC, the concern was that the remove function may be called while the device is
runtime suspended. Hence the remove function needs to resume the device since the
remove function may access the HW.

	Sören

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/07/2015 04:58 PM, Sören Brinkmann wrote:
>> I think you have to convert the _remove() function, too. Have a look at
>> the gpio-zynq.c driver:
>>
>>> static int zynq_gpio_remove(struct platform_device *pdev)
>>> {
>>> 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>
>>> 	pm_runtime_get_sync(&pdev->dev);
>>
>> However I don't understand why the get_sync() is here. Maybe Sören can help?
> 
> IIRC, the concern was that the remove function may be called while the device is
> runtime suspended. Hence the remove function needs to resume the device since the
> remove function may access the HW.

What about the corresponding runtime_put()? Would some counter be
unbalanced upon device removal?

Without having tested it, unloading and loading, i.e. reloading a CAN
driver is easier than reloading the gpio driver on an average embedded
system. Kedareswara please test your driver with something like:
modprobe; ifconfig up; cansend; ifconfig down; rmmod in a loop.

Marc

-- 
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: 819 bytes --]

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Wed, 2015-01-07 at 05:30PM +0100, Marc Kleine-Budde wrote:
> On 01/07/2015 04:58 PM, Sören Brinkmann wrote:
> >> I think you have to convert the _remove() function, too. Have a look at
> >> the gpio-zynq.c driver:
> >>
> >>> static int zynq_gpio_remove(struct platform_device *pdev)
> >>> {
> >>> 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>
> >>> 	pm_runtime_get_sync(&pdev->dev);
> >>
> >> However I don't understand why the get_sync() is here. Maybe Sören can help?
> > 
> > IIRC, the concern was that the remove function may be called while the device is
> > runtime suspended. Hence the remove function needs to resume the device since the
> > remove function may access the HW.
> 
> What about the corresponding runtime_put()? Would some counter be
> unbalanced upon device removal?

Aren't those counters destroyed with module unloading?

	Sören

Re: [PATCH v4] can: Convert to runtime_pm

[Sören Brinkmann]

On Wed, 2015-01-07 at 05:30PM +0100, Marc Kleine-Budde wrote:
> On 01/07/2015 04:58 PM, Sören Brinkmann wrote:
> >> I think you have to convert the _remove() function, too. Have a look at
> >> the gpio-zynq.c driver:
> >>
> >>> static int zynq_gpio_remove(struct platform_device *pdev)
> >>> {
> >>> 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>
> >>> 	pm_runtime_get_sync(&pdev->dev);
> >>
> >> However I don't understand why the get_sync() is here. Maybe Sören can help?
> > 
> > IIRC, the concern was that the remove function may be called while the device is
> > runtime suspended. Hence the remove function needs to resume the device since the
> > remove function may access the HW.
> 
> What about the corresponding runtime_put()? Would some counter be
> unbalanced upon device removal?

Aren't those counters destroyed with module unloading?

	Sören

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/07/2015 05:32 PM, Sören Brinkmann wrote:
> On Wed, 2015-01-07 at 05:30PM +0100, Marc Kleine-Budde wrote:
>> On 01/07/2015 04:58 PM, Sören Brinkmann wrote:
>>>> I think you have to convert the _remove() function, too. Have a look at
>>>> the gpio-zynq.c driver:
>>>>
>>>>> static int zynq_gpio_remove(struct platform_device *pdev)
>>>>> {
>>>>> 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>
>>>>> 	pm_runtime_get_sync(&pdev->dev);
>>>>
>>>> However I don't understand why the get_sync() is here. Maybe Sören can help?
>>>
>>> IIRC, the concern was that the remove function may be called while the device is
>>> runtime suspended. Hence the remove function needs to resume the device since the
>>> remove function may access the HW.
>>
>> What about the corresponding runtime_put()? Would some counter be
>> unbalanced upon device removal?
> 
> Aren't those counters destroyed with module unloading?

I don't know.

Marc

-- 
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: 819 bytes --]

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, January 07, 2015 5:59 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> grant.likely@linaro.org
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Appana Durga Kedareswara Rao; Soren Brinkmann
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the
> > driver, use the runtime_pm framework. This consolidates the actions
> > for runtime PM in the appropriate callbacks and makes the driver more
> > readable and mantainable.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Chnages for v4:
> >  - Updated with the review comments.
> > Changes for v3:
> >   - Converted the driver to use runtime_pm.
> > Changes for v2:
> >   - Removed the struct platform_device* from suspend/resume
> >     as suggest by Lothar.
> >
> >  drivers/net/can/xilinx_can.c |  123
> > +++++++++++++++++++++++++-----------------
> >  1 files changed, 74 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/can/xilinx_can.c
> > b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> >  #include <linux/can/led.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #define DRIVER_NAME        "xilinx_can"
> >
> > @@ -138,7 +139,7 @@ struct xcan_priv {
> >     u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> >     void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> >                     u32 val);
> > -   struct net_device *dev;
> > +   struct device *dev;
> >     void __iomem *reg_base;
> >     unsigned long irq_flags;
> >     struct clk *bus_clk;
> > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
> >
> > +   ret = pm_runtime_get_sync(priv->dev);
> > +   if (ret < 0) {
> > +           netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +                           __func__, ret);
> > +           return ret;
> > +   }
> > +
> >     ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> >                     ndev->name, ndev);
> >     if (ret < 0) {
> > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> >             goto err;
> >     }
> >
> > -   ret = clk_prepare_enable(priv->can_clk);
> > -   if (ret) {
> > -           netdev_err(ndev, "unable to enable device clock\n");
> > -           goto err_irq;
> > -   }
> > -
> > -   ret = clk_prepare_enable(priv->bus_clk);
> > -   if (ret) {
> > -           netdev_err(ndev, "unable to enable bus clock\n");
> > -           goto err_can_clk;
> > -   }
> > -
> >     /* Set chip into reset mode */
> >     ret = set_reset_mode(ndev);
> >     if (ret < 0) {
> >             netdev_err(ndev, "mode resetting failed!\n");
> > -           goto err_bus_clk;
> > +           goto err_irq;
> >     }
> >
> >     /* Common open */
> >     ret = open_candev(ndev);
> >     if (ret)
> > -           goto err_bus_clk;
> > +           goto err_irq;
> >
> >     ret = xcan_chip_start(ndev);
> >     if (ret < 0) {
> > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
> >
> >  err_candev:
> >     close_candev(ndev);
> > -err_bus_clk:
> > -   clk_disable_unprepare(priv->bus_clk);
> > -err_can_clk:
> > -   clk_disable_unprepare(priv->can_clk);
> >  err_irq:
> >     free_irq(ndev->irq, ndev);
> >  err:
> > +   pm_runtime_put(priv->dev);
> > +
> >     return ret;
> >  }
> >
> > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> >     netif_stop_queue(ndev);
> >     napi_disable(&priv->napi);
> >     xcan_chip_stop(ndev);
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> >     free_irq(ndev->irq, ndev);
> >     close_candev(ndev);
> >
> >     can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +   pm_runtime_put(priv->dev);
> >
> >     return 0;
> >  }
> > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct
> net_device *ndev,
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
> >
> > -   ret = clk_prepare_enable(priv->can_clk);
> > -   if (ret)
> > -           goto err;
> > -
> > -   ret = clk_prepare_enable(priv->bus_clk);
> > -   if (ret)
> > -           goto err_clk;
> > +   ret = pm_runtime_get_sync(priv->dev);
> > +   if (ret < 0) {
> > +           netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +                           __func__, ret);
>
> Please remove the \r from the error messages.
>

Ok

> > +           return ret;
> > +   }
> >
> >     bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_TEC_MASK;
> >     bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> >                     XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> >
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> > +   pm_runtime_put(priv->dev);
> >
> >     return 0;
> > -
> > -err_clk:
> > -   clk_disable_unprepare(priv->can_clk);
> > -err:
> > -   return ret;
> >  }
> >
> >
> > @@ -967,15 +953,45 @@ static const struct net_device_ops
> > xcan_netdev_ops = {
> >
> >  /**
> >   * xcan_suspend - Suspend method for the driver
> > - * @dev:   Address of the platform_device structure
> > + * @dev:   Address of the device structure
> >   *
> >   * Put the driver into low power mode.
> > - * Return: 0 always
> > + * Return: 0 on success and failure value on error
> >   */
> >  static int __maybe_unused xcan_suspend(struct device *dev)  {
> > -   struct platform_device *pdev = dev_get_drvdata(dev);
> > -   struct net_device *ndev = platform_get_drvdata(pdev);
> > +   if (!device_may_wakeup(dev))
> > +           return pm_runtime_force_suspend(dev);
> > +
> > +   return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev:   Address of the device structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error  */ static int
> > +__maybe_unused xcan_resume(struct device *dev) {
> > +   if (!device_may_wakeup(dev))
> > +           return pm_runtime_force_resume(dev);
> > +
> > +   return 0;
> > +
> > +}
> > +
> > +/**
> > + * xcan_runtime_suspend - Runtime suspend method for the driver
> > + * @dev:   Address of the device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) {
> > +   struct net_device *ndev = dev_get_drvdata(dev);
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >
> >     if (netif_running(ndev)) {
> > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct
> > device *dev)  }
> >
> >  /**
> > - * xcan_resume - Resume from suspend
> > - * @dev:   Address of the platformdevice structure
> > + * xcan_runtime_resume - Runtime resume from suspend
> > + * @dev:   Address of the device structure
> >   *
> >   * Resume operation after suspend.
> >   * Return: 0 on success and failure value on error
> >   */
> > -static int __maybe_unused xcan_resume(struct device *dev)
> > +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> >  {
> > -   struct platform_device *pdev = dev_get_drvdata(dev);
> > -   struct net_device *ndev = platform_get_drvdata(pdev);
> > +   struct net_device *ndev = dev_get_drvdata(dev);
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
>
> Some more context:
>
> >     ret = clk_enable(priv->bus_clk);
> >     if (ret) {
> >             dev_err(dev, "Cannot enable clock.\n");
> >             return ret;
> >     }
> >     ret = clk_enable(priv->can_clk);
> >     if (ret) {
> >             dev_err(dev, "Cannot enable clock.\n");
> >             clk_disable_unprepare(priv->bus_clk);
>
> This disable_unprepare looks wrong, should be a disable only.
>

Ok

> >             return ret;
> >     }
> >
> >     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >
> >     if (netif_running(ndev)) {
> >             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>
> What happens if the device was not in ACTIVE state prior to the
> runtime_suspend?
>

I am not sure about the state of CAN at this point of time.
I just followed what other drivers are following for run time  suspend :).

> >             netif_device_attach(ndev);
> >             netif_start_queue(ndev);
> >     }
> >
> >     return 0;
> > }
>
>
> >
> > @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
> > device *dev)
> >
> >     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >
> >     if (netif_running(ndev)) {
> > +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >             netif_device_attach(ndev);
> >             netif_start_queue(ndev);
> >     }
> > @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct
> device *dev)
> >     return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> xcan_resume);
> > +static const struct dev_pm_ops xcan_dev_pm_ops = {
> > +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> > +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> xcan_runtime_resume,
> > +NULL) };
> >
> >  /**
> >   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> > static int xcan_probe(struct platform_device *pdev)
> >             return -ENOMEM;
> >
> >     priv = netdev_priv(ndev);
> > -   priv->dev = ndev;
> > +   priv->dev = &pdev->dev;
> >     priv->can.bittiming_const = &xcan_bittiming_const;
> >     priv->can.do_set_mode = xcan_do_set_mode;
> >     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> 1137,15
> > +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >
> >     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >
> > +   pm_runtime_set_active(&pdev->dev);
> > +   pm_runtime_irq_safe(&pdev->dev);
> > +   pm_runtime_enable(&pdev->dev);
> > +   pm_runtime_get_sync(&pdev->dev);
> Check error values?

Ok
> > +
> >     ret = register_candev(ndev);
> >     if (ret) {
> >             dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
> ret);
> > +           pm_runtime_put(priv->dev);
>
> Please move the pm_runtime_put into the common error exit path.
>

Ok

> >             goto err_unprepare_disable_busclk;
> >     }
> >
> >     devm_can_led_init(ndev);
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> > +
> > +   pm_runtime_put(&pdev->dev);
> > +
> >     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> depth:%d\n",
> >                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >                     priv->tx_max);
> >
>
> I think you have to convert the _remove() function, too. Have a look at the
> gpio-zynq.c driver:
>
> > static int zynq_gpio_remove(struct platform_device *pdev) {
> >     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >
> >     pm_runtime_get_sync(&pdev->dev);
>
> However I don't understand why the get_sync() is here. Maybe Sören can
> help?

I converted the remove function to use the run-time PM and .
Below is the remove code snippet.

       ret = pm_runtime_get_sync(&pdev->dev);
        if (ret < 0) {
                netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
                                __func__, ret);
                return ret;
        }

        if (set_reset_mode(ndev) < 0)
                netdev_err(ndev, "mode resetting failed!\n");

        unregister_candev(ndev);
        netif_napi_del(&priv->napi);
        free_candev(ndev);
        pm_runtime_disable(&pdev->dev);

Observed the below things in the /sys/kernel/debug/clk/clk_summary.

                                Modprobe        ifconfig up    ifconfig down   rmmod  Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up   ifconfig down  rmmod

Clk_prepare count:                         1                            1               1               1       2        2              2               2       3       3               3               3

Clk_enable count:                 0             1               0               1       2       2               2               2       3       3               3               3

Regards,
Kedar.

>
> >     gpiochip_remove(&gpio->chip);
> >     clk_disable_unprepare(gpio->clk);
> >     device_set_wakeup_capable(&pdev->dev, 0);
> >     return 0;
> > }
>
> regards,
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 13694 bytes --]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, January 07, 2015 5:59 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> grant.likely@linaro.org
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Appana Durga Kedareswara Rao; Soren Brinkmann
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the
> > driver, use the runtime_pm framework. This consolidates the actions
> > for runtime PM in the appropriate callbacks and makes the driver more
> > readable and mantainable.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Chnages for v4:
> >  - Updated with the review comments.
> > Changes for v3:
> >   - Converted the driver to use runtime_pm.
> > Changes for v2:
> >   - Removed the struct platform_device* from suspend/resume
> >     as suggest by Lothar.
> >
> >  drivers/net/can/xilinx_can.c |  123
> > +++++++++++++++++++++++++-----------------
> >  1 files changed, 74 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/can/xilinx_can.c
> > b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> >  #include <linux/can/led.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #define DRIVER_NAME        "xilinx_can"
> >
> > @@ -138,7 +139,7 @@ struct xcan_priv {
> >     u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> >     void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> >                     u32 val);
> > -   struct net_device *dev;
> > +   struct device *dev;
> >     void __iomem *reg_base;
> >     unsigned long irq_flags;
> >     struct clk *bus_clk;
> > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
> >
> > +   ret = pm_runtime_get_sync(priv->dev);
> > +   if (ret < 0) {
> > +           netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +                           __func__, ret);
> > +           return ret;
> > +   }
> > +
> >     ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> >                     ndev->name, ndev);
> >     if (ret < 0) {
> > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> >             goto err;
> >     }
> >
> > -   ret = clk_prepare_enable(priv->can_clk);
> > -   if (ret) {
> > -           netdev_err(ndev, "unable to enable device clock\n");
> > -           goto err_irq;
> > -   }
> > -
> > -   ret = clk_prepare_enable(priv->bus_clk);
> > -   if (ret) {
> > -           netdev_err(ndev, "unable to enable bus clock\n");
> > -           goto err_can_clk;
> > -   }
> > -
> >     /* Set chip into reset mode */
> >     ret = set_reset_mode(ndev);
> >     if (ret < 0) {
> >             netdev_err(ndev, "mode resetting failed!\n");
> > -           goto err_bus_clk;
> > +           goto err_irq;
> >     }
> >
> >     /* Common open */
> >     ret = open_candev(ndev);
> >     if (ret)
> > -           goto err_bus_clk;
> > +           goto err_irq;
> >
> >     ret = xcan_chip_start(ndev);
> >     if (ret < 0) {
> > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
> >
> >  err_candev:
> >     close_candev(ndev);
> > -err_bus_clk:
> > -   clk_disable_unprepare(priv->bus_clk);
> > -err_can_clk:
> > -   clk_disable_unprepare(priv->can_clk);
> >  err_irq:
> >     free_irq(ndev->irq, ndev);
> >  err:
> > +   pm_runtime_put(priv->dev);
> > +
> >     return ret;
> >  }
> >
> > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> >     netif_stop_queue(ndev);
> >     napi_disable(&priv->napi);
> >     xcan_chip_stop(ndev);
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> >     free_irq(ndev->irq, ndev);
> >     close_candev(ndev);
> >
> >     can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +   pm_runtime_put(priv->dev);
> >
> >     return 0;
> >  }
> > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct
> net_device *ndev,
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
> >
> > -   ret = clk_prepare_enable(priv->can_clk);
> > -   if (ret)
> > -           goto err;
> > -
> > -   ret = clk_prepare_enable(priv->bus_clk);
> > -   if (ret)
> > -           goto err_clk;
> > +   ret = pm_runtime_get_sync(priv->dev);
> > +   if (ret < 0) {
> > +           netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
> > +                           __func__, ret);
>
> Please remove the \r from the error messages.
>

Ok

> > +           return ret;
> > +   }
> >
> >     bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_TEC_MASK;
> >     bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> >                     XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> >
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> > +   pm_runtime_put(priv->dev);
> >
> >     return 0;
> > -
> > -err_clk:
> > -   clk_disable_unprepare(priv->can_clk);
> > -err:
> > -   return ret;
> >  }
> >
> >
> > @@ -967,15 +953,45 @@ static const struct net_device_ops
> > xcan_netdev_ops = {
> >
> >  /**
> >   * xcan_suspend - Suspend method for the driver
> > - * @dev:   Address of the platform_device structure
> > + * @dev:   Address of the device structure
> >   *
> >   * Put the driver into low power mode.
> > - * Return: 0 always
> > + * Return: 0 on success and failure value on error
> >   */
> >  static int __maybe_unused xcan_suspend(struct device *dev)  {
> > -   struct platform_device *pdev = dev_get_drvdata(dev);
> > -   struct net_device *ndev = platform_get_drvdata(pdev);
> > +   if (!device_may_wakeup(dev))
> > +           return pm_runtime_force_suspend(dev);
> > +
> > +   return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev:   Address of the device structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error  */ static int
> > +__maybe_unused xcan_resume(struct device *dev) {
> > +   if (!device_may_wakeup(dev))
> > +           return pm_runtime_force_resume(dev);
> > +
> > +   return 0;
> > +
> > +}
> > +
> > +/**
> > + * xcan_runtime_suspend - Runtime suspend method for the driver
> > + * @dev:   Address of the device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) {
> > +   struct net_device *ndev = dev_get_drvdata(dev);
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >
> >     if (netif_running(ndev)) {
> > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct
> > device *dev)  }
> >
> >  /**
> > - * xcan_resume - Resume from suspend
> > - * @dev:   Address of the platformdevice structure
> > + * xcan_runtime_resume - Runtime resume from suspend
> > + * @dev:   Address of the device structure
> >   *
> >   * Resume operation after suspend.
> >   * Return: 0 on success and failure value on error
> >   */
> > -static int __maybe_unused xcan_resume(struct device *dev)
> > +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> >  {
> > -   struct platform_device *pdev = dev_get_drvdata(dev);
> > -   struct net_device *ndev = platform_get_drvdata(pdev);
> > +   struct net_device *ndev = dev_get_drvdata(dev);
> >     struct xcan_priv *priv = netdev_priv(ndev);
> >     int ret;
>
> Some more context:
>
> >     ret = clk_enable(priv->bus_clk);
> >     if (ret) {
> >             dev_err(dev, "Cannot enable clock.\n");
> >             return ret;
> >     }
> >     ret = clk_enable(priv->can_clk);
> >     if (ret) {
> >             dev_err(dev, "Cannot enable clock.\n");
> >             clk_disable_unprepare(priv->bus_clk);
>
> This disable_unprepare looks wrong, should be a disable only.
>

Ok

> >             return ret;
> >     }
> >
> >     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >
> >     if (netif_running(ndev)) {
> >             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>
> What happens if the device was not in ACTIVE state prior to the
> runtime_suspend?
>

I am not sure about the state of CAN at this point of time.
I just followed what other drivers are following for run time  suspend :).

> >             netif_device_attach(ndev);
> >             netif_start_queue(ndev);
> >     }
> >
> >     return 0;
> > }
>
>
> >
> > @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
> > device *dev)
> >
> >     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >
> >     if (netif_running(ndev)) {
> > +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >             netif_device_attach(ndev);
> >             netif_start_queue(ndev);
> >     }
> > @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct
> device *dev)
> >     return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> xcan_resume);
> > +static const struct dev_pm_ops xcan_dev_pm_ops = {
> > +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> > +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> xcan_runtime_resume,
> > +NULL) };
> >
> >  /**
> >   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> > static int xcan_probe(struct platform_device *pdev)
> >             return -ENOMEM;
> >
> >     priv = netdev_priv(ndev);
> > -   priv->dev = ndev;
> > +   priv->dev = &pdev->dev;
> >     priv->can.bittiming_const = &xcan_bittiming_const;
> >     priv->can.do_set_mode = xcan_do_set_mode;
> >     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> 1137,15
> > +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >
> >     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >
> > +   pm_runtime_set_active(&pdev->dev);
> > +   pm_runtime_irq_safe(&pdev->dev);
> > +   pm_runtime_enable(&pdev->dev);
> > +   pm_runtime_get_sync(&pdev->dev);
> Check error values?

Ok
> > +
> >     ret = register_candev(ndev);
> >     if (ret) {
> >             dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
> ret);
> > +           pm_runtime_put(priv->dev);
>
> Please move the pm_runtime_put into the common error exit path.
>

Ok

> >             goto err_unprepare_disable_busclk;
> >     }
> >
> >     devm_can_led_init(ndev);
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> > +
> > +   pm_runtime_put(&pdev->dev);
> > +
> >     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> depth:%d\n",
> >                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >                     priv->tx_max);
> >
>
> I think you have to convert the _remove() function, too. Have a look at the
> gpio-zynq.c driver:
>
> > static int zynq_gpio_remove(struct platform_device *pdev) {
> >     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >
> >     pm_runtime_get_sync(&pdev->dev);
>
> However I don't understand why the get_sync() is here. Maybe Sören can
> help?

I converted the remove function to use the run-time PM and .
Below is the remove code snippet.

       ret = pm_runtime_get_sync(&pdev->dev);
        if (ret < 0) {
                netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
                                __func__, ret);
                return ret;
        }

        if (set_reset_mode(ndev) < 0)
                netdev_err(ndev, "mode resetting failed!\n");

        unregister_candev(ndev);
        netif_napi_del(&priv->napi);
        free_candev(ndev);
        pm_runtime_disable(&pdev->dev);

Observed the below things in the /sys/kernel/debug/clk/clk_summary.

                                Modprobe        ifconfig up    ifconfig down   rmmod  Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up   ifconfig down  rmmod

Clk_prepare count:                         1                            1               1               1       2        2              2               2       3       3               3               3

Clk_enable count:                 0             1               0               1       2       2               2               2       3       3               3               3

Regards,
Kedar.

>
> >     gpiochip_remove(&gpio->chip);
> >     clk_disable_unprepare(gpio->clk);
> >     device_set_wakeup_capable(&pdev->dev, 0);
> >     return 0;
> > }
>
> regards,
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
[...]
>>>             return ret;
>>>     }
>>>
>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>
>>>     if (netif_running(ndev)) {
>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>> What happens if the device was not in ACTIVE state prior to the
>> runtime_suspend?
>>
> 
> I am not sure about the state of CAN at this point of time.
> I just followed what other drivers are following for run time  suspend :).

Please check the state of the hardware if you go with bus off into
suspend and then resume.

>>>             netif_device_attach(ndev);
>>>             netif_start_queue(ndev);
>>>     }
>>>
>>>     return 0;
>>> }
>>
>>
>>>
>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
>>> device *dev)
>>>
>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>
>>>     if (netif_running(ndev)) {
>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>             netif_device_attach(ndev);
>>>             netif_start_queue(ndev);
>>>     }
>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct
>> device *dev)
>>>     return 0;
>>>  }
>>>
>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
>> xcan_resume);
>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
>> xcan_runtime_resume,
>>> +NULL) };
>>>
>>>  /**
>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
>>> static int xcan_probe(struct platform_device *pdev)
>>>             return -ENOMEM;
>>>
>>>     priv = netdev_priv(ndev);
>>> -   priv->dev = ndev;
>>> +   priv->dev = &pdev->dev;
>>>     priv->can.bittiming_const = &xcan_bittiming_const;
>>>     priv->can.do_set_mode = xcan_do_set_mode;
>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
>> 1137,15
>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>>>
>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>
>>> +   pm_runtime_set_active(&pdev->dev);
>>> +   pm_runtime_irq_safe(&pdev->dev);
>>> +   pm_runtime_enable(&pdev->dev);
>>> +   pm_runtime_get_sync(&pdev->dev);
>> Check error values?
> 
> Ok
>>> +
>>>     ret = register_candev(ndev);
>>>     if (ret) {
>>>             dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
>> ret);
>>> +           pm_runtime_put(priv->dev);
>>
>> Please move the pm_runtime_put into the common error exit path.
>>
> 
> Ok
> 
>>>             goto err_unprepare_disable_busclk;
>>>     }
>>>
>>>     devm_can_led_init(ndev);
>>> -   clk_disable_unprepare(priv->bus_clk);
>>> -   clk_disable_unprepare(priv->can_clk);
>>> +
>>> +   pm_runtime_put(&pdev->dev);
>>> +
>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
>> depth:%d\n",
>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
>>>                     priv->tx_max);
>>>
>>
>> I think you have to convert the _remove() function, too. Have a look at the
>> gpio-zynq.c driver:
>>
>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>
>>>     pm_runtime_get_sync(&pdev->dev);
>>
>> However I don't understand why the get_sync() is here. Maybe Sören can
>> help?
> 
> I converted the remove function to use the run-time PM and .
> Below is the remove code snippet.
> 
>        ret = pm_runtime_get_sync(&pdev->dev);
>         if (ret < 0) {
>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>                                 __func__, ret);
>                 return ret;
>         }
> 
>         if (set_reset_mode(ndev) < 0)
>                 netdev_err(ndev, "mode resetting failed!\n");
> 
>         unregister_candev(ndev);
>         netif_napi_del(&priv->napi);
>         free_candev(ndev);

>         pm_runtime_disable(&pdev->dev);

Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
been unregistered and already free()ed. Better move this directly after
the set_reset_mode(). This way you are symmetric to the probe() function.

> Observed the below things in the /sys/kernel/debug/clk/clk_summary.
> 
>                                 Modprobe        ifconfig up    ifconfig down   rmmod  Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up   ifconfig down  rmmod
> Clk_prepare count:                1             1               1               1       2       2               2               2       3       3               3               3
> Clk_enable count:                 0             1               0               1       2       2               2               2       3       3               3               3

As the numbers are increasing you have a clk_prepare_enable() leak. Your
remove() function is missing the clk_disable_unprepare() for the can and
bus clock (as you have clk_prepare_enable in probe()).

Marc

-- 
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: 819 bytes --]

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Sunday, January 11, 2015 9:11 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> [...]
> >>>             return ret;
> >>>     }
> >>>
> >>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>
> >>>     if (netif_running(ndev)) {
> >>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>
> >> What happens if the device was not in ACTIVE state prior to the
> >> runtime_suspend?
> >>
> >
> > I am not sure about the state of CAN at this point of time.
> > I just followed what other drivers are following for run time  suspend :).
>
> Please check the state of the hardware if you go with bus off into suspend
> and then resume.
>

        if (netif_running(ndev)) {
                        if (isr & XCAN_IXR_BSOFF_MASK) {
                                priv->can.state = CAN_STATE_BUS_OFF;
                           priv->write_reg(priv, XCAN_SRR_OFFSET,
                                        XCAN_SRR_RESET_MASK);
                } else if ((status & XCAN_SR_ESTAT_MASK) ==
                                        XCAN_SR_ESTAT_MASK) {
                        priv->can.state = CAN_STATE_ERROR_PASSIVE;
                } else if (status & XCAN_SR_ERRWRN_MASK) {
                        priv->can.state = CAN_STATE_ERROR_WARNING;
                } else {
                        priv->can.state = CAN_STATE_ERROR_ACTIVE;
                }
        }

Is the above code snippet ok for you?

> >>>             netif_device_attach(ndev);
> >>>             netif_start_queue(ndev);
> >>>     }
> >>>
> >>>     return 0;
> >>> }
> >>
> >>
> >>>
> >>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
> >>> device *dev)
> >>>
> >>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>
> >>>     if (netif_running(ndev)) {
> >>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>             netif_device_attach(ndev);
> >>>             netif_start_queue(ndev);
> >>>     }
> >>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> xcan_resume(struct
> >> device *dev)
> >>>     return 0;
> >>>  }
> >>>
> >>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >> xcan_resume);
> >>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >> xcan_runtime_resume,
> >>> +NULL) };
> >>>
> >>>  /**
> >>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> >>> static int xcan_probe(struct platform_device *pdev)
> >>>             return -ENOMEM;
> >>>
> >>>     priv = netdev_priv(ndev);
> >>> -   priv->dev = ndev;
> >>> +   priv->dev = &pdev->dev;
> >>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >> 1137,15
> >>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>
> >>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>
> >>> +   pm_runtime_set_active(&pdev->dev);
> >>> +   pm_runtime_irq_safe(&pdev->dev);
> >>> +   pm_runtime_enable(&pdev->dev);
> >>> +   pm_runtime_get_sync(&pdev->dev);
> >> Check error values?
> >
> > Ok
> >>> +
> >>>     ret = register_candev(ndev);
> >>>     if (ret) {
> >>>             dev_err(&pdev->dev, "fail to register failed
> >>> (err=%d)\n",
> >> ret);
> >>> +           pm_runtime_put(priv->dev);
> >>
> >> Please move the pm_runtime_put into the common error exit path.
> >>
> >
> > Ok
> >
> >>>             goto err_unprepare_disable_busclk;
> >>>     }
> >>>
> >>>     devm_can_led_init(ndev);
> >>> -   clk_disable_unprepare(priv->bus_clk);
> >>> -   clk_disable_unprepare(priv->can_clk);
> >>> +
> >>> +   pm_runtime_put(&pdev->dev);
> >>> +
> >>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >> depth:%d\n",
> >>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>                     priv->tx_max);
> >>>
> >>
> >> I think you have to convert the _remove() function, too. Have a look
> >> at the gpio-zynq.c driver:
> >>
> >>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>
> >>>     pm_runtime_get_sync(&pdev->dev);
> >>
> >> However I don't understand why the get_sync() is here. Maybe Sören
> >> can help?
> >
> > I converted the remove function to use the run-time PM and .
> > Below is the remove code snippet.
> >
> >        ret = pm_runtime_get_sync(&pdev->dev);
> >         if (ret < 0) {
> >                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >                                 __func__, ret);
> >                 return ret;
> >         }
> >
> >         if (set_reset_mode(ndev) < 0)
> >                 netdev_err(ndev, "mode resetting failed!\n");
> >
> >         unregister_candev(ndev);
> >         netif_napi_del(&priv->napi);
> >         free_candev(ndev);
>
> >         pm_runtime_disable(&pdev->dev);
>
> Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
> been unregistered and already free()ed. Better move this directly after the
> set_reset_mode(). This way you are symmetric to the probe() function.

If I move the  pm_runtime_disable after the set_reset_mode
I am getting the below error.
ERROR:
xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter: pm_runtime_get fail

If I move the pm_runtime_disable after unregister_candev everything is working fine.

Regards,
Kedar.

>
> > Observed the below things in the /sys/kernel/debug/clk/clk_summary.
> >
> >                                 Modprobe        ifconfig up    ifconfig down   rmmod
> Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up
> ifconfig down  rmmod
> > Clk_prepare count:                1             1               1               1       2       2               2
> 2       3       3               3               3
> > Clk_enable count:                 0             1               0               1       2       2               2
> 2       3       3               3               3
>
> As the numbers are increasing you have a clk_prepare_enable() leak. Your
> remove() function is missing the clk_disable_unprepare() for the can and
> bus clock (as you have clk_prepare_enable in probe()).
>
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7643 bytes --]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Sunday, January 11, 2015 9:11 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> [...]
> >>>             return ret;
> >>>     }
> >>>
> >>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>
> >>>     if (netif_running(ndev)) {
> >>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>
> >> What happens if the device was not in ACTIVE state prior to the
> >> runtime_suspend?
> >>
> >
> > I am not sure about the state of CAN at this point of time.
> > I just followed what other drivers are following for run time  suspend :).
>
> Please check the state of the hardware if you go with bus off into suspend
> and then resume.
>

        if (netif_running(ndev)) {
                        if (isr & XCAN_IXR_BSOFF_MASK) {
                                priv->can.state = CAN_STATE_BUS_OFF;
                           priv->write_reg(priv, XCAN_SRR_OFFSET,
                                        XCAN_SRR_RESET_MASK);
                } else if ((status & XCAN_SR_ESTAT_MASK) ==
                                        XCAN_SR_ESTAT_MASK) {
                        priv->can.state = CAN_STATE_ERROR_PASSIVE;
                } else if (status & XCAN_SR_ERRWRN_MASK) {
                        priv->can.state = CAN_STATE_ERROR_WARNING;
                } else {
                        priv->can.state = CAN_STATE_ERROR_ACTIVE;
                }
        }

Is the above code snippet ok for you?

> >>>             netif_device_attach(ndev);
> >>>             netif_start_queue(ndev);
> >>>     }
> >>>
> >>>     return 0;
> >>> }
> >>
> >>
> >>>
> >>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
> >>> device *dev)
> >>>
> >>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>
> >>>     if (netif_running(ndev)) {
> >>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>             netif_device_attach(ndev);
> >>>             netif_start_queue(ndev);
> >>>     }
> >>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> xcan_resume(struct
> >> device *dev)
> >>>     return 0;
> >>>  }
> >>>
> >>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >> xcan_resume);
> >>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >> xcan_runtime_resume,
> >>> +NULL) };
> >>>
> >>>  /**
> >>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> >>> static int xcan_probe(struct platform_device *pdev)
> >>>             return -ENOMEM;
> >>>
> >>>     priv = netdev_priv(ndev);
> >>> -   priv->dev = ndev;
> >>> +   priv->dev = &pdev->dev;
> >>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >> 1137,15
> >>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>
> >>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>
> >>> +   pm_runtime_set_active(&pdev->dev);
> >>> +   pm_runtime_irq_safe(&pdev->dev);
> >>> +   pm_runtime_enable(&pdev->dev);
> >>> +   pm_runtime_get_sync(&pdev->dev);
> >> Check error values?
> >
> > Ok
> >>> +
> >>>     ret = register_candev(ndev);
> >>>     if (ret) {
> >>>             dev_err(&pdev->dev, "fail to register failed
> >>> (err=%d)\n",
> >> ret);
> >>> +           pm_runtime_put(priv->dev);
> >>
> >> Please move the pm_runtime_put into the common error exit path.
> >>
> >
> > Ok
> >
> >>>             goto err_unprepare_disable_busclk;
> >>>     }
> >>>
> >>>     devm_can_led_init(ndev);
> >>> -   clk_disable_unprepare(priv->bus_clk);
> >>> -   clk_disable_unprepare(priv->can_clk);
> >>> +
> >>> +   pm_runtime_put(&pdev->dev);
> >>> +
> >>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >> depth:%d\n",
> >>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>                     priv->tx_max);
> >>>
> >>
> >> I think you have to convert the _remove() function, too. Have a look
> >> at the gpio-zynq.c driver:
> >>
> >>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>
> >>>     pm_runtime_get_sync(&pdev->dev);
> >>
> >> However I don't understand why the get_sync() is here. Maybe Sören
> >> can help?
> >
> > I converted the remove function to use the run-time PM and .
> > Below is the remove code snippet.
> >
> >        ret = pm_runtime_get_sync(&pdev->dev);
> >         if (ret < 0) {
> >                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >                                 __func__, ret);
> >                 return ret;
> >         }
> >
> >         if (set_reset_mode(ndev) < 0)
> >                 netdev_err(ndev, "mode resetting failed!\n");
> >
> >         unregister_candev(ndev);
> >         netif_napi_del(&priv->napi);
> >         free_candev(ndev);
>
> >         pm_runtime_disable(&pdev->dev);
>
> Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
> been unregistered and already free()ed. Better move this directly after the
> set_reset_mode(). This way you are symmetric to the probe() function.

If I move the  pm_runtime_disable after the set_reset_mode
I am getting the below error.
ERROR:
xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter: pm_runtime_get fail

If I move the pm_runtime_disable after unregister_candev everything is working fine.

Regards,
Kedar.

>
> > Observed the below things in the /sys/kernel/debug/clk/clk_summary.
> >
> >                                 Modprobe        ifconfig up    ifconfig down   rmmod
> Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up
> ifconfig down  rmmod
> > Clk_prepare count:                1             1               1               1       2       2               2
> 2       3       3               3               3
> > Clk_enable count:                 0             1               0               1       2       2               2
> 2       3       3               3               3
>
> As the numbers are increasing you have a clk_prepare_enable() leak. Your
> remove() function is missing the clk_disable_unprepare() for the can and
> bus clock (as you have clk_prepare_enable in probe()).
>
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Sunday, January 11, 2015 9:11 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
>> [...]
>>>>>             return ret;
>>>>>     }
>>>>>
>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>
>>>>>     if (netif_running(ndev)) {
>>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>> What happens if the device was not in ACTIVE state prior to the
>>>> runtime_suspend?
>>>>
>>>
>>> I am not sure about the state of CAN at this point of time.
>>> I just followed what other drivers are following for run time  suspend :).
>>
>> Please check the state of the hardware if you go with bus off into suspend
>> and then resume.
>>
> 
>         if (netif_running(ndev)) {
>                         if (isr & XCAN_IXR_BSOFF_MASK) {
>                                 priv->can.state = CAN_STATE_BUS_OFF;
>                            priv->write_reg(priv, XCAN_SRR_OFFSET,
>                                         XCAN_SRR_RESET_MASK);
>                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
>                                         XCAN_SR_ESTAT_MASK) {
>                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
>                 } else if (status & XCAN_SR_ERRWRN_MASK) {
>                         priv->can.state = CAN_STATE_ERROR_WARNING;
>                 } else {
>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
>                 }
>         }
> 
> Is the above code snippet ok for you?

Yes, but what's the state of the hardware when it wakes up again?

> 
>>>>>             netif_device_attach(ndev);
>>>>>             netif_start_queue(ndev);
>>>>>     }
>>>>>
>>>>>     return 0;
>>>>> }
>>>>
>>>>
>>>>>
>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
>>>>> device *dev)
>>>>>
>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>
>>>>>     if (netif_running(ndev)) {
>>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>             netif_device_attach(ndev);
>>>>>             netif_start_queue(ndev);
>>>>>     }
>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
>> xcan_resume(struct
>>>> device *dev)
>>>>>     return 0;
>>>>>  }
>>>>>
>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
>>>> xcan_resume);
>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
>>>> xcan_runtime_resume,
>>>>> +NULL) };
>>>>>
>>>>>  /**
>>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
>>>>> static int xcan_probe(struct platform_device *pdev)
>>>>>             return -ENOMEM;
>>>>>
>>>>>     priv = netdev_priv(ndev);
>>>>> -   priv->dev = ndev;
>>>>> +   priv->dev = &pdev->dev;
>>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
>>>>>     priv->can.do_set_mode = xcan_do_set_mode;
>>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
>>>> 1137,15
>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>>>>>
>>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>>>
>>>>> +   pm_runtime_set_active(&pdev->dev);
>>>>> +   pm_runtime_irq_safe(&pdev->dev);
>>>>> +   pm_runtime_enable(&pdev->dev);
>>>>> +   pm_runtime_get_sync(&pdev->dev);
>>>> Check error values?
>>>
>>> Ok
>>>>> +
>>>>>     ret = register_candev(ndev);
>>>>>     if (ret) {
>>>>>             dev_err(&pdev->dev, "fail to register failed
>>>>> (err=%d)\n",
>>>> ret);
>>>>> +           pm_runtime_put(priv->dev);
>>>>
>>>> Please move the pm_runtime_put into the common error exit path.
>>>>
>>>
>>> Ok
>>>
>>>>>             goto err_unprepare_disable_busclk;
>>>>>     }
>>>>>
>>>>>     devm_can_led_init(ndev);
>>>>> -   clk_disable_unprepare(priv->bus_clk);
>>>>> -   clk_disable_unprepare(priv->can_clk);
>>>>> +
>>>>> +   pm_runtime_put(&pdev->dev);
>>>>> +
>>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
>>>> depth:%d\n",
>>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
>>>>>                     priv->tx_max);
>>>>>
>>>>
>>>> I think you have to convert the _remove() function, too. Have a look
>>>> at the gpio-zynq.c driver:
>>>>
>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>
>>>>>     pm_runtime_get_sync(&pdev->dev);
>>>>
>>>> However I don't understand why the get_sync() is here. Maybe Sören
>>>> can help?
>>>
>>> I converted the remove function to use the run-time PM and .
>>> Below is the remove code snippet.
>>>
>>>        ret = pm_runtime_get_sync(&pdev->dev);
>>>         if (ret < 0) {
>>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>                                 __func__, ret);
>>>                 return ret;
>>>         }
>>>
>>>         if (set_reset_mode(ndev) < 0)
>>>                 netdev_err(ndev, "mode resetting failed!\n");
>>>
>>>         unregister_candev(ndev);
>>>         netif_napi_del(&priv->napi);
>>>         free_candev(ndev);
>>
>>>         pm_runtime_disable(&pdev->dev);
>>
>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
>> been unregistered and already free()ed. Better move this directly after the
>> set_reset_mode(). This way you are symmetric to the probe() function.
> 
> If I move the  pm_runtime_disable after the set_reset_mode
> I am getting the below error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter: pm_runtime_get fail
> 
> If I move the pm_runtime_disable after unregister_candev everything is working fine.

Fine - but who calls xcan_get_berr_counter here? Can you add a
dump_stack() here?

Marc
-- 
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: 819 bytes --]

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 6:56 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Sunday, January 11, 2015 9:11 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >> [...]
> >>>>>             return ret;
> >>>>>     }
> >>>>>
> >>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>
> >>>>>     if (netif_running(ndev)) {
> >>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>
> >>>> What happens if the device was not in ACTIVE state prior to the
> >>>> runtime_suspend?
> >>>>
> >>>
> >>> I am not sure about the state of CAN at this point of time.
> >>> I just followed what other drivers are following for run time  suspend :).
> >>
> >> Please check the state of the hardware if you go with bus off into
> >> suspend and then resume.
> >>
> >
> >         if (netif_running(ndev)) {
> >                         if (isr & XCAN_IXR_BSOFF_MASK) {
> >                                 priv->can.state = CAN_STATE_BUS_OFF;
> >                            priv->write_reg(priv, XCAN_SRR_OFFSET,
> >                                         XCAN_SRR_RESET_MASK);
> >                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >                                         XCAN_SR_ESTAT_MASK) {
> >                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >                 } else if (status & XCAN_SR_ERRWRN_MASK) {
> >                         priv->can.state = CAN_STATE_ERROR_WARNING;
> >                 } else {
> >                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >                 }
> >         }
> >
> > Is the above code snippet ok for you?
>
> Yes, but what's the state of the hardware when it wakes up again?

It depends on the previous state of the CAN.
I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
when it wakes up that's why  checking for the status of the CAN in the status register here to put the device in appropriate mode.

>
> >
> >>>>>             netif_device_attach(ndev);
> >>>>>             netif_start_queue(ndev);
> >>>>>     }
> >>>>>
> >>>>>     return 0;
> >>>>> }
> >>>>
> >>>>
> >>>>>
> >>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> xcan_resume(struct
> >>>>> device *dev)
> >>>>>
> >>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>
> >>>>>     if (netif_running(ndev)) {
> >>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>             netif_device_attach(ndev);
> >>>>>             netif_start_queue(ndev);
> >>>>>     }
> >>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>> device *dev)
> >>>>>     return 0;
> >>>>>  }
> >>>>>
> >>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >>>> xcan_resume);
> >>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >>>> xcan_runtime_resume,
> >>>>> +NULL) };
> >>>>>
> >>>>>  /**
> >>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> >>>>> static int xcan_probe(struct platform_device *pdev)
> >>>>>             return -ENOMEM;
> >>>>>
> >>>>>     priv = netdev_priv(ndev);
> >>>>> -   priv->dev = ndev;
> >>>>> +   priv->dev = &pdev->dev;
> >>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >>>> 1137,15
> >>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>
> >>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>>>
> >>>>> +   pm_runtime_set_active(&pdev->dev);
> >>>>> +   pm_runtime_irq_safe(&pdev->dev);
> >>>>> +   pm_runtime_enable(&pdev->dev);
> >>>>> +   pm_runtime_get_sync(&pdev->dev);
> >>>> Check error values?
> >>>
> >>> Ok
> >>>>> +
> >>>>>     ret = register_candev(ndev);
> >>>>>     if (ret) {
> >>>>>             dev_err(&pdev->dev, "fail to register failed
> >>>>> (err=%d)\n",
> >>>> ret);
> >>>>> +           pm_runtime_put(priv->dev);
> >>>>
> >>>> Please move the pm_runtime_put into the common error exit path.
> >>>>
> >>>
> >>> Ok
> >>>
> >>>>>             goto err_unprepare_disable_busclk;
> >>>>>     }
> >>>>>
> >>>>>     devm_can_led_init(ndev);
> >>>>> -   clk_disable_unprepare(priv->bus_clk);
> >>>>> -   clk_disable_unprepare(priv->can_clk);
> >>>>> +
> >>>>> +   pm_runtime_put(&pdev->dev);
> >>>>> +
> >>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >>>> depth:%d\n",
> >>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>>>                     priv->tx_max);
> >>>>>
> >>>>
> >>>> I think you have to convert the _remove() function, too. Have a
> >>>> look at the gpio-zynq.c driver:
> >>>>
> >>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>
> >>>>>     pm_runtime_get_sync(&pdev->dev);
> >>>>
> >>>> However I don't understand why the get_sync() is here. Maybe Sören
> >>>> can help?
> >>>
> >>> I converted the remove function to use the run-time PM and .
> >>> Below is the remove code snippet.
> >>>
> >>>        ret = pm_runtime_get_sync(&pdev->dev);
> >>>         if (ret < 0) {
> >>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>                                 __func__, ret);
> >>>                 return ret;
> >>>         }
> >>>
> >>>         if (set_reset_mode(ndev) < 0)
> >>>                 netdev_err(ndev, "mode resetting failed!\n");
> >>>
> >>>         unregister_candev(ndev);
> >>>         netif_napi_del(&priv->napi);
> >>>         free_candev(ndev);
> >>
> >>>         pm_runtime_disable(&pdev->dev);
> >>
> >> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >> has been unregistered and already free()ed. Better move this directly
> >> after the set_reset_mode(). This way you are symmetric to the probe()
> function.
> >
> > If I move the  pm_runtime_disable after the set_reset_mode I am
> > getting the below error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> > pm_runtime_get fail
> >
> > If I move the pm_runtime_disable after unregister_candev everything is
> working fine.
>
> Fine - but who calls xcan_get_berr_counter here? Can you add a
> dump_stack() here?
>

I think it is getting called from the atomic context.
When I  am trying to do a rmmod I am getting the above error.
ERROR:
xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
 pm_runtime_get fail.

I am getting only the above error in the console when I do rmmod.

Regards,
Kedar.

> Marc
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 8583 bytes --]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 6:56 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Sunday, January 11, 2015 9:11 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >> [...]
> >>>>>             return ret;
> >>>>>     }
> >>>>>
> >>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>
> >>>>>     if (netif_running(ndev)) {
> >>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>
> >>>> What happens if the device was not in ACTIVE state prior to the
> >>>> runtime_suspend?
> >>>>
> >>>
> >>> I am not sure about the state of CAN at this point of time.
> >>> I just followed what other drivers are following for run time  suspend :).
> >>
> >> Please check the state of the hardware if you go with bus off into
> >> suspend and then resume.
> >>
> >
> >         if (netif_running(ndev)) {
> >                         if (isr & XCAN_IXR_BSOFF_MASK) {
> >                                 priv->can.state = CAN_STATE_BUS_OFF;
> >                            priv->write_reg(priv, XCAN_SRR_OFFSET,
> >                                         XCAN_SRR_RESET_MASK);
> >                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >                                         XCAN_SR_ESTAT_MASK) {
> >                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >                 } else if (status & XCAN_SR_ERRWRN_MASK) {
> >                         priv->can.state = CAN_STATE_ERROR_WARNING;
> >                 } else {
> >                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >                 }
> >         }
> >
> > Is the above code snippet ok for you?
>
> Yes, but what's the state of the hardware when it wakes up again?

It depends on the previous state of the CAN.
I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
when it wakes up that's why  checking for the status of the CAN in the status register here to put the device in appropriate mode.

>
> >
> >>>>>             netif_device_attach(ndev);
> >>>>>             netif_start_queue(ndev);
> >>>>>     }
> >>>>>
> >>>>>     return 0;
> >>>>> }
> >>>>
> >>>>
> >>>>>
> >>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> xcan_resume(struct
> >>>>> device *dev)
> >>>>>
> >>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>
> >>>>>     if (netif_running(ndev)) {
> >>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>             netif_device_attach(ndev);
> >>>>>             netif_start_queue(ndev);
> >>>>>     }
> >>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>> device *dev)
> >>>>>     return 0;
> >>>>>  }
> >>>>>
> >>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >>>> xcan_resume);
> >>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >>>> xcan_runtime_resume,
> >>>>> +NULL) };
> >>>>>
> >>>>>  /**
> >>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
> >>>>> static int xcan_probe(struct platform_device *pdev)
> >>>>>             return -ENOMEM;
> >>>>>
> >>>>>     priv = netdev_priv(ndev);
> >>>>> -   priv->dev = ndev;
> >>>>> +   priv->dev = &pdev->dev;
> >>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >>>> 1137,15
> >>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>
> >>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>>>
> >>>>> +   pm_runtime_set_active(&pdev->dev);
> >>>>> +   pm_runtime_irq_safe(&pdev->dev);
> >>>>> +   pm_runtime_enable(&pdev->dev);
> >>>>> +   pm_runtime_get_sync(&pdev->dev);
> >>>> Check error values?
> >>>
> >>> Ok
> >>>>> +
> >>>>>     ret = register_candev(ndev);
> >>>>>     if (ret) {
> >>>>>             dev_err(&pdev->dev, "fail to register failed
> >>>>> (err=%d)\n",
> >>>> ret);
> >>>>> +           pm_runtime_put(priv->dev);
> >>>>
> >>>> Please move the pm_runtime_put into the common error exit path.
> >>>>
> >>>
> >>> Ok
> >>>
> >>>>>             goto err_unprepare_disable_busclk;
> >>>>>     }
> >>>>>
> >>>>>     devm_can_led_init(ndev);
> >>>>> -   clk_disable_unprepare(priv->bus_clk);
> >>>>> -   clk_disable_unprepare(priv->can_clk);
> >>>>> +
> >>>>> +   pm_runtime_put(&pdev->dev);
> >>>>> +
> >>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >>>> depth:%d\n",
> >>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>>>                     priv->tx_max);
> >>>>>
> >>>>
> >>>> I think you have to convert the _remove() function, too. Have a
> >>>> look at the gpio-zynq.c driver:
> >>>>
> >>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>
> >>>>>     pm_runtime_get_sync(&pdev->dev);
> >>>>
> >>>> However I don't understand why the get_sync() is here. Maybe Sören
> >>>> can help?
> >>>
> >>> I converted the remove function to use the run-time PM and .
> >>> Below is the remove code snippet.
> >>>
> >>>        ret = pm_runtime_get_sync(&pdev->dev);
> >>>         if (ret < 0) {
> >>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>                                 __func__, ret);
> >>>                 return ret;
> >>>         }
> >>>
> >>>         if (set_reset_mode(ndev) < 0)
> >>>                 netdev_err(ndev, "mode resetting failed!\n");
> >>>
> >>>         unregister_candev(ndev);
> >>>         netif_napi_del(&priv->napi);
> >>>         free_candev(ndev);
> >>
> >>>         pm_runtime_disable(&pdev->dev);
> >>
> >> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >> has been unregistered and already free()ed. Better move this directly
> >> after the set_reset_mode(). This way you are symmetric to the probe()
> function.
> >
> > If I move the  pm_runtime_disable after the set_reset_mode I am
> > getting the below error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> > pm_runtime_get fail
> >
> > If I move the pm_runtime_disable after unregister_candev everything is
> working fine.
>
> Fine - but who calls xcan_get_berr_counter here? Can you add a
> dump_stack() here?
>

I think it is getting called from the atomic context.
When I  am trying to do a rmmod I am getting the above error.
ERROR:
xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
 pm_runtime_get fail.

I am getting only the above error in the console when I do rmmod.

Regards,
Kedar.

> Marc
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Monday, January 12, 2015 6:56 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com; Michal Simek
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>>> Sent: Sunday, January 11, 2015 9:11 PM
>>>> To: Appana Durga Kedareswara Rao
>>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>>>> wg@grandegger.com
>>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>>>
>>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
>>>> [...]
>>>>>>>             return ret;
>>>>>>>     }
>>>>>>>
>>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>>>
>>>>>>>     if (netif_running(ndev)) {
>>>>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>
>>>>>> What happens if the device was not in ACTIVE state prior to the
>>>>>> runtime_suspend?
>>>>>>
>>>>>
>>>>> I am not sure about the state of CAN at this point of time.
>>>>> I just followed what other drivers are following for run time  suspend :).
>>>>
>>>> Please check the state of the hardware if you go with bus off into
>>>> suspend and then resume.
>>>>
>>>
>>>         if (netif_running(ndev)) {
>>>                         if (isr & XCAN_IXR_BSOFF_MASK) {
>>>                                 priv->can.state = CAN_STATE_BUS_OFF;
>>>                            priv->write_reg(priv, XCAN_SRR_OFFSET,
>>>                                         XCAN_SRR_RESET_MASK);
>>>                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
>>>                                         XCAN_SR_ESTAT_MASK) {
>>>                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>>                 } else if (status & XCAN_SR_ERRWRN_MASK) {
>>>                         priv->can.state = CAN_STATE_ERROR_WARNING;
>>>                 } else {
>>>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>                 }
>>>         }
>>>
>>> Is the above code snippet ok for you?
>>
>> Yes, but what's the state of the hardware when it wakes up again?
> 
> It depends on the previous state of the CAN.
> I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
> Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
> when it wakes up that's why  checking for the status of the CAN in the status register here to put the device in appropriate mode.

I understand the software side, but I don't know how your hardware
behaves. This is why I'm asking.

> 
>>
>>>
>>>>>>>             netif_device_attach(ndev);
>>>>>>>             netif_start_queue(ndev);
>>>>>>>     }
>>>>>>>
>>>>>>>     return 0;
>>>>>>> }
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
>> xcan_resume(struct
>>>>>>> device *dev)
>>>>>>>
>>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>>
>>>>>>>     if (netif_running(ndev)) {
>>>>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>>             netif_device_attach(ndev);
>>>>>>>             netif_start_queue(ndev);
>>>>>>>     }
>>>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
>>>> xcan_resume(struct
>>>>>> device *dev)
>>>>>>>     return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
>>>>>> xcan_resume);
>>>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>>>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>>>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
>>>>>> xcan_runtime_resume,
>>>>>>> +NULL) };
>>>>>>>
>>>>>>>  /**
>>>>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
>>>>>>> static int xcan_probe(struct platform_device *pdev)
>>>>>>>             return -ENOMEM;
>>>>>>>
>>>>>>>     priv = netdev_priv(ndev);
>>>>>>> -   priv->dev = ndev;
>>>>>>> +   priv->dev = &pdev->dev;
>>>>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
>>>>>>>     priv->can.do_set_mode = xcan_do_set_mode;
>>>>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
>>>>>> 1137,15
>>>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>>>>>
>>>>>>> +   pm_runtime_set_active(&pdev->dev);
>>>>>>> +   pm_runtime_irq_safe(&pdev->dev);
>>>>>>> +   pm_runtime_enable(&pdev->dev);
>>>>>>> +   pm_runtime_get_sync(&pdev->dev);
>>>>>> Check error values?
>>>>>
>>>>> Ok
>>>>>>> +
>>>>>>>     ret = register_candev(ndev);
>>>>>>>     if (ret) {
>>>>>>>             dev_err(&pdev->dev, "fail to register failed
>>>>>>> (err=%d)\n",
>>>>>> ret);
>>>>>>> +           pm_runtime_put(priv->dev);
>>>>>>
>>>>>> Please move the pm_runtime_put into the common error exit path.
>>>>>>
>>>>>
>>>>> Ok
>>>>>
>>>>>>>             goto err_unprepare_disable_busclk;
>>>>>>>     }
>>>>>>>
>>>>>>>     devm_can_led_init(ndev);
>>>>>>> -   clk_disable_unprepare(priv->bus_clk);
>>>>>>> -   clk_disable_unprepare(priv->can_clk);
>>>>>>> +
>>>>>>> +   pm_runtime_put(&pdev->dev);
>>>>>>> +
>>>>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
>>>>>> depth:%d\n",
>>>>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
>>>>>>>                     priv->tx_max);
>>>>>>>
>>>>>>
>>>>>> I think you have to convert the _remove() function, too. Have a
>>>>>> look at the gpio-zynq.c driver:
>>>>>>
>>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>>>
>>>>>>>     pm_runtime_get_sync(&pdev->dev);
>>>>>>
>>>>>> However I don't understand why the get_sync() is here. Maybe Sören
>>>>>> can help?
>>>>>
>>>>> I converted the remove function to use the run-time PM and .
>>>>> Below is the remove code snippet.
>>>>>
>>>>>        ret = pm_runtime_get_sync(&pdev->dev);
>>>>>         if (ret < 0) {
>>>>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>>>                                 __func__, ret);
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>>         if (set_reset_mode(ndev) < 0)
>>>>>                 netdev_err(ndev, "mode resetting failed!\n");
>>>>>
>>>>>         unregister_candev(ndev);
>>>>>         netif_napi_del(&priv->napi);
>>>>>         free_candev(ndev);
>>>>
>>>>>         pm_runtime_disable(&pdev->dev);
>>>>
>>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
>>>> has been unregistered and already free()ed. Better move this directly
>>>> after the set_reset_mode(). This way you are symmetric to the probe()
>> function.
>>>
>>> If I move the  pm_runtime_disable after the set_reset_mode I am
>>> getting the below error.
>>> ERROR:
>>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
>>> pm_runtime_get fail
>>>
>>> If I move the pm_runtime_disable after unregister_candev everything is
>> working fine.
>>
>> Fine - but who calls xcan_get_berr_counter here? Can you add a
>> dump_stack() here?
>>
> 
> I think it is getting called from the atomic context.
> When I  am trying to do a rmmod I am getting the above error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
>  pm_runtime_get fail.
> 
> I am getting only the above error in the console when I do rmmod.

Put a dump_stack into xcan_get_berr_counter(), then you'll see where
it's called from. However calling from atomic context should be fine.

Marc

-- 
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: 819 bytes --]

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 7:23 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Monday, January 12, 2015 6:56 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com; Michal Simek
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> >>> Hi Marc,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>> Sent: Sunday, January 11, 2015 9:11 PM
> >>>> To: Appana Durga Kedareswara Rao
> >>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >>>> wg@grandegger.com
> >>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>>>
> >>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >>>> [...]
> >>>>>>>             return ret;
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>>
> >>>>>>>     if (netif_running(ndev)) {
> >>>>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>
> >>>>>> What happens if the device was not in ACTIVE state prior to the
> >>>>>> runtime_suspend?
> >>>>>>
> >>>>>
> >>>>> I am not sure about the state of CAN at this point of time.
> >>>>> I just followed what other drivers are following for run time  suspend
> :).
> >>>>
> >>>> Please check the state of the hardware if you go with bus off into
> >>>> suspend and then resume.
> >>>>
> >>>
> >>>         if (netif_running(ndev)) {
> >>>                         if (isr & XCAN_IXR_BSOFF_MASK) {
> >>>                                 priv->can.state = CAN_STATE_BUS_OFF;
> >>>                            priv->write_reg(priv, XCAN_SRR_OFFSET,
> >>>                                         XCAN_SRR_RESET_MASK);
> >>>                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >>>                                         XCAN_SR_ESTAT_MASK) {
> >>>                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >>>                 } else if (status & XCAN_SR_ERRWRN_MASK) {
> >>>                         priv->can.state = CAN_STATE_ERROR_WARNING;
> >>>                 } else {
> >>>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>                 }
> >>>         }
> >>>
> >>> Is the above code snippet ok for you?
> >>
> >> Yes, but what's the state of the hardware when it wakes up again?
> >
> > It depends on the previous state of the CAN.
> > I mean In Suspend we are putting the device in sleep mode and in
> > resume we are waking up by putting the device into the Configuration
> > mode. We are not doing any reset of the core in the suspend/resume so it
> depends on the previous state of the CAN when it wakes up that's why
> checking for the status of the CAN in the status register here to put the
> device in appropriate mode.
>
> I understand the software side, but I don't know how your hardware
> behaves. This is why I'm asking.

As far as I know the above is  the our can controller behavior. Anyway I will check with  the h/w team
And will get back to you regarding this. Meanwhile I am sending the next version of the patch
Please review it.

Regards,
Kedar.
>
> >
> >>
> >>>
> >>>>>>>             netif_device_attach(ndev);
> >>>>>>>             netif_start_queue(ndev);
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     return 0;
> >>>>>>> }
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>>>>> device *dev)
> >>>>>>>
> >>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>>
> >>>>>>>     if (netif_running(ndev)) {
> >>>>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>>             netif_device_attach(ndev);
> >>>>>>>             netif_start_queue(ndev);
> >>>>>>>     }
> >>>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> >>>> xcan_resume(struct
> >>>>>> device *dev)
> >>>>>>>     return 0;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >>>>>> xcan_resume);
> >>>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>>>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>>>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >>>>>> xcan_runtime_resume,
> >>>>>>> +NULL) };
> >>>>>>>
> >>>>>>>  /**
> >>>>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7
> >>>>>>> @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>>             return -ENOMEM;
> >>>>>>>
> >>>>>>>     priv = netdev_priv(ndev);
> >>>>>>> -   priv->dev = ndev;
> >>>>>>> +   priv->dev = &pdev->dev;
> >>>>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>>>>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >>>>>> 1137,15
> >>>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>>
> >>>>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>>>>>
> >>>>>>> +   pm_runtime_set_active(&pdev->dev);
> >>>>>>> +   pm_runtime_irq_safe(&pdev->dev);
> >>>>>>> +   pm_runtime_enable(&pdev->dev);
> >>>>>>> +   pm_runtime_get_sync(&pdev->dev);
> >>>>>> Check error values?
> >>>>>
> >>>>> Ok
> >>>>>>> +
> >>>>>>>     ret = register_candev(ndev);
> >>>>>>>     if (ret) {
> >>>>>>>             dev_err(&pdev->dev, "fail to register failed
> >>>>>>> (err=%d)\n",
> >>>>>> ret);
> >>>>>>> +           pm_runtime_put(priv->dev);
> >>>>>>
> >>>>>> Please move the pm_runtime_put into the common error exit path.
> >>>>>>
> >>>>>
> >>>>> Ok
> >>>>>
> >>>>>>>             goto err_unprepare_disable_busclk;
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     devm_can_led_init(ndev);
> >>>>>>> -   clk_disable_unprepare(priv->bus_clk);
> >>>>>>> -   clk_disable_unprepare(priv->can_clk);
> >>>>>>> +
> >>>>>>> +   pm_runtime_put(&pdev->dev);
> >>>>>>> +
> >>>>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >>>>>> depth:%d\n",
> >>>>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>>>>>                     priv->tx_max);
> >>>>>>>
> >>>>>>
> >>>>>> I think you have to convert the _remove() function, too. Have a
> >>>>>> look at the gpio-zynq.c driver:
> >>>>>>
> >>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>>>
> >>>>>>>     pm_runtime_get_sync(&pdev->dev);
> >>>>>>
> >>>>>> However I don't understand why the get_sync() is here. Maybe
> >>>>>> Sören can help?
> >>>>>
> >>>>> I converted the remove function to use the run-time PM and .
> >>>>> Below is the remove code snippet.
> >>>>>
> >>>>>        ret = pm_runtime_get_sync(&pdev->dev);
> >>>>>         if (ret < 0) {
> >>>>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>>>                                 __func__, ret);
> >>>>>                 return ret;
> >>>>>         }
> >>>>>
> >>>>>         if (set_reset_mode(ndev) < 0)
> >>>>>                 netdev_err(ndev, "mode resetting failed!\n");
> >>>>>
> >>>>>         unregister_candev(ndev);
> >>>>>         netif_napi_del(&priv->napi);
> >>>>>         free_candev(ndev);
> >>>>
> >>>>>         pm_runtime_disable(&pdev->dev);
> >>>>
> >>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >>>> has been unregistered and already free()ed. Better move this
> >>>> directly after the set_reset_mode(). This way you are symmetric to
> >>>> the probe()
> >> function.
> >>>
> >>> If I move the  pm_runtime_disable after the set_reset_mode I am
> >>> getting the below error.
> >>> ERROR:
> >>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >>> pm_runtime_get fail
> >>>
> >>> If I move the pm_runtime_disable after unregister_candev everything
> >>> is
> >> working fine.
> >>
> >> Fine - but who calls xcan_get_berr_counter here? Can you add a
> >> dump_stack() here?
> >>
> >
> > I think it is getting called from the atomic context.
> > When I  am trying to do a rmmod I am getting the above error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >  pm_runtime_get fail.
> >
> > I am getting only the above error in the console when I do rmmod.
>
> Put a dump_stack into xcan_get_berr_counter(), then you'll see where it's
> called from. However calling from atomic context should be fine.
>
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 10000 bytes --]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 7:23 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Monday, January 12, 2015 6:56 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com; Michal Simek
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> >>> Hi Marc,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>> Sent: Sunday, January 11, 2015 9:11 PM
> >>>> To: Appana Durga Kedareswara Rao
> >>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >>>> wg@grandegger.com
> >>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>>>
> >>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >>>> [...]
> >>>>>>>             return ret;
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>>
> >>>>>>>     if (netif_running(ndev)) {
> >>>>>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>
> >>>>>> What happens if the device was not in ACTIVE state prior to the
> >>>>>> runtime_suspend?
> >>>>>>
> >>>>>
> >>>>> I am not sure about the state of CAN at this point of time.
> >>>>> I just followed what other drivers are following for run time  suspend
> :).
> >>>>
> >>>> Please check the state of the hardware if you go with bus off into
> >>>> suspend and then resume.
> >>>>
> >>>
> >>>         if (netif_running(ndev)) {
> >>>                         if (isr & XCAN_IXR_BSOFF_MASK) {
> >>>                                 priv->can.state = CAN_STATE_BUS_OFF;
> >>>                            priv->write_reg(priv, XCAN_SRR_OFFSET,
> >>>                                         XCAN_SRR_RESET_MASK);
> >>>                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >>>                                         XCAN_SR_ESTAT_MASK) {
> >>>                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >>>                 } else if (status & XCAN_SR_ERRWRN_MASK) {
> >>>                         priv->can.state = CAN_STATE_ERROR_WARNING;
> >>>                 } else {
> >>>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>                 }
> >>>         }
> >>>
> >>> Is the above code snippet ok for you?
> >>
> >> Yes, but what's the state of the hardware when it wakes up again?
> >
> > It depends on the previous state of the CAN.
> > I mean In Suspend we are putting the device in sleep mode and in
> > resume we are waking up by putting the device into the Configuration
> > mode. We are not doing any reset of the core in the suspend/resume so it
> depends on the previous state of the CAN when it wakes up that's why
> checking for the status of the CAN in the status register here to put the
> device in appropriate mode.
>
> I understand the software side, but I don't know how your hardware
> behaves. This is why I'm asking.

As far as I know the above is  the our can controller behavior. Anyway I will check with  the h/w team
And will get back to you regarding this. Meanwhile I am sending the next version of the patch
Please review it.

Regards,
Kedar.
>
> >
> >>
> >>>
> >>>>>>>             netif_device_attach(ndev);
> >>>>>>>             netif_start_queue(ndev);
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     return 0;
> >>>>>>> }
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>>>>> device *dev)
> >>>>>>>
> >>>>>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>>
> >>>>>>>     if (netif_running(ndev)) {
> >>>>>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>>             netif_device_attach(ndev);
> >>>>>>>             netif_start_queue(ndev);
> >>>>>>>     }
> >>>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> >>>> xcan_resume(struct
> >>>>>> device *dev)
> >>>>>>>     return 0;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >>>>>> xcan_resume);
> >>>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>>>>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>>>>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >>>>>> xcan_runtime_resume,
> >>>>>>> +NULL) };
> >>>>>>>
> >>>>>>>  /**
> >>>>>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7
> >>>>>>> @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>>             return -ENOMEM;
> >>>>>>>
> >>>>>>>     priv = netdev_priv(ndev);
> >>>>>>> -   priv->dev = ndev;
> >>>>>>> +   priv->dev = &pdev->dev;
> >>>>>>>     priv->can.bittiming_const = &xcan_bittiming_const;
> >>>>>>>     priv->can.do_set_mode = xcan_do_set_mode;
> >>>>>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >>>>>> 1137,15
> >>>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>>
> >>>>>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>>>>>
> >>>>>>> +   pm_runtime_set_active(&pdev->dev);
> >>>>>>> +   pm_runtime_irq_safe(&pdev->dev);
> >>>>>>> +   pm_runtime_enable(&pdev->dev);
> >>>>>>> +   pm_runtime_get_sync(&pdev->dev);
> >>>>>> Check error values?
> >>>>>
> >>>>> Ok
> >>>>>>> +
> >>>>>>>     ret = register_candev(ndev);
> >>>>>>>     if (ret) {
> >>>>>>>             dev_err(&pdev->dev, "fail to register failed
> >>>>>>> (err=%d)\n",
> >>>>>> ret);
> >>>>>>> +           pm_runtime_put(priv->dev);
> >>>>>>
> >>>>>> Please move the pm_runtime_put into the common error exit path.
> >>>>>>
> >>>>>
> >>>>> Ok
> >>>>>
> >>>>>>>             goto err_unprepare_disable_busclk;
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     devm_can_led_init(ndev);
> >>>>>>> -   clk_disable_unprepare(priv->bus_clk);
> >>>>>>> -   clk_disable_unprepare(priv->can_clk);
> >>>>>>> +
> >>>>>>> +   pm_runtime_put(&pdev->dev);
> >>>>>>> +
> >>>>>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >>>>>> depth:%d\n",
> >>>>>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>>>>>                     priv->tx_max);
> >>>>>>>
> >>>>>>
> >>>>>> I think you have to convert the _remove() function, too. Have a
> >>>>>> look at the gpio-zynq.c driver:
> >>>>>>
> >>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>>>
> >>>>>>>     pm_runtime_get_sync(&pdev->dev);
> >>>>>>
> >>>>>> However I don't understand why the get_sync() is here. Maybe
> >>>>>> Sören can help?
> >>>>>
> >>>>> I converted the remove function to use the run-time PM and .
> >>>>> Below is the remove code snippet.
> >>>>>
> >>>>>        ret = pm_runtime_get_sync(&pdev->dev);
> >>>>>         if (ret < 0) {
> >>>>>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>>>                                 __func__, ret);
> >>>>>                 return ret;
> >>>>>         }
> >>>>>
> >>>>>         if (set_reset_mode(ndev) < 0)
> >>>>>                 netdev_err(ndev, "mode resetting failed!\n");
> >>>>>
> >>>>>         unregister_candev(ndev);
> >>>>>         netif_napi_del(&priv->napi);
> >>>>>         free_candev(ndev);
> >>>>
> >>>>>         pm_runtime_disable(&pdev->dev);
> >>>>
> >>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >>>> has been unregistered and already free()ed. Better move this
> >>>> directly after the set_reset_mode(). This way you are symmetric to
> >>>> the probe()
> >> function.
> >>>
> >>> If I move the  pm_runtime_disable after the set_reset_mode I am
> >>> getting the below error.
> >>> ERROR:
> >>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >>> pm_runtime_get fail
> >>>
> >>> If I move the pm_runtime_disable after unregister_candev everything
> >>> is
> >> working fine.
> >>
> >> Fine - but who calls xcan_get_berr_counter here? Can you add a
> >> dump_stack() here?
> >>
> >
> > I think it is getting called from the atomic context.
> > When I  am trying to do a rmmod I am getting the above error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >  pm_runtime_get fail.
> >
> > I am getting only the above error in the console when I do rmmod.
>
> Put a dump_stack into xcan_get_berr_counter(), then you'll see where it's
> called from. However calling from atomic context should be fine.
>
> Marc
>
> --
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

I tried by splitting the xcan_berr_counter to __xcan_berr_counter but still seeing
the same crash as pointed by the Soren in the previous version of the patch.
Updated the driver with the other review comments in this patch.

Regards,
Kedar.


-----Original Message-----
From: Kedareswara rao Appana [mailto:appana.durga.rao@xilinx.com]
Sent: Tuesday, December 23, 2014 5:55 PM
To: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org
Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Appana Durga Kedareswara Rao; Soren Brinkmann
Subject: [PATCH v4] can: Convert to runtime_pm

Instead of enabling/disabling clocks at several locations in the driver, use the runtime_pm framework. This consolidates the actions for runtime PM in the appropriate callbacks and makes the driver more readable and mantainable.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Chnages for v4:
 - Updated with the review comments.
Changes for v3:
  - Converted the driver to use runtime_pm.
Changes for v2:
  - Removed the struct platform_device* from suspend/resume
    as suggest by Lothar.

 drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/pm_runtime.h>

 #define DRIVER_NAME    "xilinx_can"

@@ -138,7 +139,7 @@ struct xcan_priv {
        u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
        void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
                        u32 val);
-       struct net_device *dev;
+       struct device *dev;
        void __iomem *reg_base;
        unsigned long irq_flags;
        struct clk *bus_clk;
@@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
        struct xcan_priv *priv = netdev_priv(ndev);
        int ret;

+       ret = pm_runtime_get_sync(priv->dev);
+       if (ret < 0) {
+               netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+                               __func__, ret);
+               return ret;
+       }
+
        ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
                        ndev->name, ndev);
        if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
                goto err;
        }

-       ret = clk_prepare_enable(priv->can_clk);
-       if (ret) {
-               netdev_err(ndev, "unable to enable device clock\n");
-               goto err_irq;
-       }
-
-       ret = clk_prepare_enable(priv->bus_clk);
-       if (ret) {
-               netdev_err(ndev, "unable to enable bus clock\n");
-               goto err_can_clk;
-       }
-
        /* Set chip into reset mode */
        ret = set_reset_mode(ndev);
        if (ret < 0) {
                netdev_err(ndev, "mode resetting failed!\n");
-               goto err_bus_clk;
+               goto err_irq;
        }

        /* Common open */
        ret = open_candev(ndev);
        if (ret)
-               goto err_bus_clk;
+               goto err_irq;

        ret = xcan_chip_start(ndev);
        if (ret < 0) {
@@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)

 err_candev:
        close_candev(ndev);
-err_bus_clk:
-       clk_disable_unprepare(priv->bus_clk);
-err_can_clk:
-       clk_disable_unprepare(priv->can_clk);
 err_irq:
        free_irq(ndev->irq, ndev);
 err:
+       pm_runtime_put(priv->dev);
+
        return ret;
 }

@@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
        netif_stop_queue(ndev);
        napi_disable(&priv->napi);
        xcan_chip_stop(ndev);
-       clk_disable_unprepare(priv->bus_clk);
-       clk_disable_unprepare(priv->can_clk);
        free_irq(ndev->irq, ndev);
        close_candev(ndev);

        can_led_event(ndev, CAN_LED_EVENT_STOP);
+       pm_runtime_put(priv->dev);

        return 0;
 }
@@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
        struct xcan_priv *priv = netdev_priv(ndev);
        int ret;

-       ret = clk_prepare_enable(priv->can_clk);
-       if (ret)
-               goto err;
-
-       ret = clk_prepare_enable(priv->bus_clk);
-       if (ret)
-               goto err_clk;
+       ret = pm_runtime_get_sync(priv->dev);
+       if (ret < 0) {
+               netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+                               __func__, ret);
+               return ret;
+       }

        bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
        bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
                        XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);

-       clk_disable_unprepare(priv->bus_clk);
-       clk_disable_unprepare(priv->can_clk);
+       pm_runtime_put(priv->dev);

        return 0;
-
-err_clk:
-       clk_disable_unprepare(priv->can_clk);
-err:
-       return ret;
 }


@@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {

 /**
  * xcan_suspend - Suspend method for the driver
- * @dev:       Address of the platform_device structure
+ * @dev:       Address of the device structure
  *
  * Put the driver into low power mode.
- * Return: 0 always
+ * Return: 0 on success and failure value on error
  */
 static int __maybe_unused xcan_suspend(struct device *dev)  {
-       struct platform_device *pdev = dev_get_drvdata(dev);
-       struct net_device *ndev = platform_get_drvdata(pdev);
+       if (!device_may_wakeup(dev))
+               return pm_runtime_force_suspend(dev);
+
+       return 0;
+}
+
+/**
+ * xcan_resume - Resume from suspend
+ * @dev:       Address of the device structure
+ *
+ * Resume operation after suspend.
+ * Return: 0 on success and failure value on error  */ static int
+__maybe_unused xcan_resume(struct device *dev) {
+       if (!device_may_wakeup(dev))
+               return pm_runtime_force_resume(dev);
+
+       return 0;
+
+}
+
+/**
+ * xcan_runtime_suspend - Runtime suspend method for the driver
+ * @dev:       Address of the device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev) {
+       struct net_device *ndev = dev_get_drvdata(dev);
        struct xcan_priv *priv = netdev_priv(ndev);

        if (netif_running(ndev)) {
@@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)  }

 /**
- * xcan_resume - Resume from suspend
- * @dev:       Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev:       Address of the device structure
  *
  * Resume operation after suspend.
  * Return: 0 on success and failure value on error
  */
-static int __maybe_unused xcan_resume(struct device *dev)
+static int __maybe_unused xcan_runtime_resume(struct device *dev)
 {
-       struct platform_device *pdev = dev_get_drvdata(dev);
-       struct net_device *ndev = platform_get_drvdata(pdev);
+       struct net_device *ndev = dev_get_drvdata(dev);
        struct xcan_priv *priv = netdev_priv(ndev);
        int ret;

@@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)

        priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
        priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
-       priv->can.state = CAN_STATE_ERROR_ACTIVE;

        if (netif_running(ndev)) {
+               priv->can.state = CAN_STATE_ERROR_ACTIVE;
                netif_device_attach(ndev);
                netif_start_queue(ndev);
        }
@@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
        return 0;
 }

-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
+static const struct dev_pm_ops xcan_dev_pm_ops = {
+       SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
+       SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
+};

 /**
  * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
                return -ENOMEM;

        priv = netdev_priv(ndev);
-       priv->dev = ndev;
+       priv->dev = &pdev->dev;
        priv->can.bittiming_const = &xcan_bittiming_const;
        priv->can.do_set_mode = xcan_do_set_mode;
        priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)

        netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);

+       pm_runtime_set_active(&pdev->dev);
+       pm_runtime_irq_safe(&pdev->dev);
+       pm_runtime_enable(&pdev->dev);
+       pm_runtime_get_sync(&pdev->dev);
+
        ret = register_candev(ndev);
        if (ret) {
                dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
+               pm_runtime_put(priv->dev);
                goto err_unprepare_disable_busclk;
        }

        devm_can_led_init(ndev);
-       clk_disable_unprepare(priv->bus_clk);
-       clk_disable_unprepare(priv->can_clk);
+
+       pm_runtime_put(&pdev->dev);
+
        netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
                        priv->reg_base, ndev->irq, priv->can.clock.freq,
                        priv->tx_max);
--
1.7.4



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[PATCH v4] can: Convert to runtime_pm

[Kedareswara rao Appana]

Instead of enabling/disabling clocks at several locations in the driver,
use the runtime_pm framework. This consolidates the actions for
runtime PM in the appropriate callbacks and makes the driver more
readable and mantainable.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Chnages for v4:
 - Updated with the review comments.
Changes for v3:
  - Converted the driver to use runtime_pm.
Changes for v2:
  - Removed the struct platform_device* from suspend/resume
    as suggest by Lothar.

 drivers/net/can/xilinx_can.c |  123 +++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..c71f683 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/pm_runtime.h>
 
 #define DRIVER_NAME	"xilinx_can"
 
@@ -138,7 +139,7 @@ struct xcan_priv {
 	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
 	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
 			u32 val);
-	struct net_device *dev;
+	struct device *dev;
 	void __iomem *reg_base;
 	unsigned long irq_flags;
 	struct clk *bus_clk;
@@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
+
 	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
 			ndev->name, ndev);
 	if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
 		goto err;
 	}
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable device clock\n");
-		goto err_irq;
-	}
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret) {
-		netdev_err(ndev, "unable to enable bus clock\n");
-		goto err_can_clk;
-	}
-
 	/* Set chip into reset mode */
 	ret = set_reset_mode(ndev);
 	if (ret < 0) {
 		netdev_err(ndev, "mode resetting failed!\n");
-		goto err_bus_clk;
+		goto err_irq;
 	}
 
 	/* Common open */
 	ret = open_candev(ndev);
 	if (ret)
-		goto err_bus_clk;
+		goto err_irq;
 
 	ret = xcan_chip_start(ndev);
 	if (ret < 0) {
@@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
 
 err_candev:
 	close_candev(ndev);
-err_bus_clk:
-	clk_disable_unprepare(priv->bus_clk);
-err_can_clk:
-	clk_disable_unprepare(priv->can_clk);
 err_irq:
 	free_irq(ndev->irq, ndev);
 err:
+	pm_runtime_put(priv->dev);
+
 	return ret;
 }
 
@@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
 	xcan_chip_stop(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
 	free_irq(ndev->irq, ndev);
 	close_candev(ndev);
 
 	can_led_event(ndev, CAN_LED_EVENT_STOP);
+	pm_runtime_put(priv->dev);
 
 	return 0;
 }
@@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
-	ret = clk_prepare_enable(priv->can_clk);
-	if (ret)
-		goto err;
-
-	ret = clk_prepare_enable(priv->bus_clk);
-	if (ret)
-		goto err_clk;
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
+				__func__, ret);
+		return ret;
+	}
 
 	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
 	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
 			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
 
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+	pm_runtime_put(priv->dev);
 
 	return 0;
-
-err_clk:
-	clk_disable_unprepare(priv->can_clk);
-err:
-	return ret;
 }
 
 
@@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
 
 /**
  * xcan_suspend - Suspend method for the driver
- * @dev:	Address of the platform_device structure
+ * @dev:	Address of the device structure
  *
  * Put the driver into low power mode.
- * Return: 0 always
+ * Return: 0 on success and failure value on error
  */
 static int __maybe_unused xcan_suspend(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
+/**
+ * xcan_resume - Resume from suspend
+ * @dev:	Address of the device structure
+ *
+ * Resume operation after suspend.
+ * Return: 0 on success and failure value on error
+ */
+static int __maybe_unused xcan_resume(struct device *dev)
+{
+	if (!device_may_wakeup(dev))
+		return pm_runtime_force_resume(dev);
+
+	return 0;
+
+}
+
+/**
+ * xcan_runtime_suspend - Runtime suspend method for the driver
+ * @dev:	Address of the device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
@@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
 }
 
 /**
- * xcan_resume - Resume from suspend
- * @dev:	Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev:	Address of the device structure
  *
  * Resume operation after suspend.
  * Return: 0 on success and failure value on error
  */
-static int __maybe_unused xcan_resume(struct device *dev)
+static int __maybe_unused xcan_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = dev_get_drvdata(dev);
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
@@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct device *dev)
 
 	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
 	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
-	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	if (netif_running(ndev)) {
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
+static const struct dev_pm_ops xcan_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
+	SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
+};
 
 /**
  * xcan_probe - Platform registration call
@@ -1071,7 +1089,7 @@ static int xcan_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = netdev_priv(ndev);
-	priv->dev = ndev;
+	priv->dev = &pdev->dev;
 	priv->can.bittiming_const = &xcan_bittiming_const;
 	priv->can.do_set_mode = xcan_do_set_mode;
 	priv->can.do_get_berr_counter = xcan_get_berr_counter;
@@ -1137,15 +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
 
 	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
+		pm_runtime_put(priv->dev);
 		goto err_unprepare_disable_busclk;
 	}
 
 	devm_can_led_init(ndev);
-	clk_disable_unprepare(priv->bus_clk);
-	clk_disable_unprepare(priv->can_clk);
+
+	pm_runtime_put(&pdev->dev);
+
 	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
 			priv->reg_base, ndev->irq, priv->can.clock.freq,
 			priv->tx_max);
-- 
1.7.4