[PATCH v5] can: Convert to runtime_pm

[PATCH v5] 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>
---
Changes for v5:
 - Updated with the review comments.
   Updated the remove fuction to use runtime_pm.
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 |  157 ++++++++++++++++++++++++++++-------------
 1 files changed, 107 insertions(+), 50 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..67aef00 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(%d)\n",
+				__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(%d)\n",
+				__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,18 +1009,18 @@ 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;
+	u32 isr, status;
 
 	ret = clk_enable(priv->bus_clk);
 	if (ret) {
@@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	ret = clk_enable(priv->can_clk);
 	if (ret) {
 		dev_err(dev, "Cannot enable clock.\n");
-		clk_disable_unprepare(priv->bus_clk);
+		clk_disable(priv->bus_clk);
 		return ret;
 	}
 
 	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;
+	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+	status = priv->read_reg(priv, XCAN_SR_OFFSET);
 
 	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;
+		}
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1059,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 +1103,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,6 +1169,16 @@ 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);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+			__func__, ret);
+		goto err_pmdisable;
+	}
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
 	}
 
 	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);
 
 	return 0;
 
+err_pmdisable:
+	pm_runtime_disable(&pdev->dev);
 err_unprepare_disable_busclk:
+	pm_runtime_put(priv->dev);
 	clk_disable_unprepare(priv->bus_clk);
 err_unprepare_disable_dev:
 	clk_disable_unprepare(priv->can_clk);
@@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct xcan_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	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);
+	pm_runtime_disable(&pdev->dev);
 	netif_napi_del(&priv->napi);
+	clk_disable_unprepare(priv->bus_clk);
+	clk_disable_unprepare(priv->can_clk);
 	free_candev(ndev);
 
 	return 0;
-- 
1.7.4

[PATCH v5] 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>
---
Changes for v5:
 - Updated with the review comments.
   Updated the remove fuction to use runtime_pm.
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 |  157 ++++++++++++++++++++++++++++-------------
 1 files changed, 107 insertions(+), 50 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..67aef00 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(%d)\n",
+				__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(%d)\n",
+				__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,18 +1009,18 @@ 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;
+	u32 isr, status;
 
 	ret = clk_enable(priv->bus_clk);
 	if (ret) {
@@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	ret = clk_enable(priv->can_clk);
 	if (ret) {
 		dev_err(dev, "Cannot enable clock.\n");
-		clk_disable_unprepare(priv->bus_clk);
+		clk_disable(priv->bus_clk);
 		return ret;
 	}
 
 	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;
+	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+	status = priv->read_reg(priv, XCAN_SR_OFFSET);
 
 	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;
+		}
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1059,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 +1103,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,6 +1169,16 @@ 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);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+			__func__, ret);
+		goto err_pmdisable;
+	}
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
 	}
 
 	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);
 
 	return 0;
 
+err_pmdisable:
+	pm_runtime_disable(&pdev->dev);
 err_unprepare_disable_busclk:
+	pm_runtime_put(priv->dev);
 	clk_disable_unprepare(priv->bus_clk);
 err_unprepare_disable_dev:
 	clk_disable_unprepare(priv->can_clk);
@@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct xcan_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	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);
+	pm_runtime_disable(&pdev->dev);
 	netif_napi_del(&priv->napi);
+	clk_disable_unprepare(priv->bus_clk);
+	clk_disable_unprepare(priv->can_clk);
 	free_candev(ndev);
 
 	return 0;
-- 
1.7.4

[PATCH v5] 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>
---
Changes for v5:
 - Updated with the review comments.
   Updated the remove fuction to use runtime_pm.
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 |  157 ++++++++++++++++++++++++++++-------------
 1 files changed, 107 insertions(+), 50 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..67aef00 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(%d)\n",
+				__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(%d)\n",
+				__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,18 +1009,18 @@ 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;
+	u32 isr, status;
 
 	ret = clk_enable(priv->bus_clk);
 	if (ret) {
@@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
 	ret = clk_enable(priv->can_clk);
 	if (ret) {
 		dev_err(dev, "Cannot enable clock.\n");
-		clk_disable_unprepare(priv->bus_clk);
+		clk_disable(priv->bus_clk);
 		return ret;
 	}
 
 	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;
+	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+	status = priv->read_reg(priv, XCAN_SR_OFFSET);
 
 	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;
+		}
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
@@ -1030,7 +1059,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 +1103,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,6 +1169,16 @@ 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);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+			__func__, ret);
+		goto err_pmdisable;
+	}
+
 	ret = register_candev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
 	}
 
 	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);
 
 	return 0;
 
+err_pmdisable:
+	pm_runtime_disable(&pdev->dev);
 err_unprepare_disable_busclk:
+	pm_runtime_put(priv->dev);
 	clk_disable_unprepare(priv->bus_clk);
 err_unprepare_disable_dev:
 	clk_disable_unprepare(priv->can_clk);
@@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct xcan_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	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);
+	pm_runtime_disable(&pdev->dev);
 	netif_napi_del(&priv->napi);
+	clk_disable_unprepare(priv->bus_clk);
+	clk_disable_unprepare(priv->can_clk);
 	free_candev(ndev);
 
 	return 0;
-- 
1.7.4

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Mon, 2015-01-12 at 08:34PM +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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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 |  157 ++++++++++++++++++++++++++++-------------
>  1 files changed, 107 insertions(+), 50 deletions(-)
[..]
> +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;
> +	u32 isr, status;
>  
>  	ret = clk_enable(priv->bus_clk);
>  	if (ret) {
> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	ret = clk_enable(priv->can_clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable clock.\n");
> -		clk_disable_unprepare(priv->bus_clk);
> +		clk_disable(priv->bus_clk);
[...]
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	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);
> +	pm_runtime_disable(&pdev->dev);
>  	netif_napi_del(&priv->napi);
> +	clk_disable_unprepare(priv->bus_clk);
> +	clk_disable_unprepare(priv->can_clk);

Shouldn't pretty much all these occurrences of clk_disable/enable
disappear? This should all be handled by the runtime_pm framework now.

	Sören

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Mon, 2015-01-12 at 08:34PM +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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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 |  157 ++++++++++++++++++++++++++++-------------
>  1 files changed, 107 insertions(+), 50 deletions(-)
[..]
> +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;
> +	u32 isr, status;
>  
>  	ret = clk_enable(priv->bus_clk);
>  	if (ret) {
> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	ret = clk_enable(priv->can_clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable clock.\n");
> -		clk_disable_unprepare(priv->bus_clk);
> +		clk_disable(priv->bus_clk);
[...]
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	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);
> +	pm_runtime_disable(&pdev->dev);
>  	netif_napi_del(&priv->napi);
> +	clk_disable_unprepare(priv->bus_clk);
> +	clk_disable_unprepare(priv->can_clk);

Shouldn't pretty much all these occurrences of clk_disable/enable
disappear? This should all be handled by the runtime_pm framework now.

	Sören

[PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Mon, 2015-01-12 at 08:34PM +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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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 |  157 ++++++++++++++++++++++++++++-------------
>  1 files changed, 107 insertions(+), 50 deletions(-)
[..]
> +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;
> +	u32 isr, status;
>  
>  	ret = clk_enable(priv->bus_clk);
>  	if (ret) {
> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	ret = clk_enable(priv->can_clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable clock.\n");
> -		clk_disable_unprepare(priv->bus_clk);
> +		clk_disable(priv->bus_clk);
[...]
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	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);
> +	pm_runtime_disable(&pdev->dev);
>  	netif_napi_del(&priv->napi);
> +	clk_disable_unprepare(priv->bus_clk);
> +	clk_disable_unprepare(priv->can_clk);

Shouldn't pretty much all these occurrences of clk_disable/enable
disappear? This should all be handled by the runtime_pm framework now.

	S?ren

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/12/2015 04:04 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>

FTBFS:

> drivers/net/can/xilinx_can.c:1064:9: error: undefined identifier 'SET_PM_RUNTIME_PM_OPS'
>   CC [M]  drivers/net/can/xilinx_can.o
> drivers/net/can/xilinx_can.c:1064:2: error: implicit declaration of function ‘SET_PM_RUNTIME_PM_OPS’ [-Werror=implicit-function-declaration]
>   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
>   ^
> drivers/net/can/xilinx_can.c:1065:1: error: initializer element is not constant
>  };
>  ^
> drivers/net/can/xilinx_can.c:1065:1: error: (near initialization for ‘xcan_dev_pm_ops.prepare’)

Have a look at commit 40bd62c6194bdee1bc6652b3b0aa28e34883f603:

More comments inline. Looks quite good now.

>     PM: Remove the SET_PM_RUNTIME_PM_OPS() macro
> 
>     There're now no users left of the SET_PM_RUNTIME_PM_OPS() macro, since
>     all have converted to use the SET_RUNTIME_PM_OPS() macro instead, so
>     let's remove it.
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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 |  157 ++++++++++++++++++++++++++++-------------
>  1 files changed, 107 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..67aef00 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(%d)\n",
> +				__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(%d)\n",
> +				__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,18 +1009,18 @@ 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;
> +	u32 isr, status;
>  
>  	ret = clk_enable(priv->bus_clk);
>  	if (ret) {
> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	ret = clk_enable(priv->can_clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable clock.\n");
> -		clk_disable_unprepare(priv->bus_clk);
> +		clk_disable(priv->bus_clk);
>  		return ret;
>  	}
>  
>  	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;
> +	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	status = priv->read_reg(priv, XCAN_SR_OFFSET);
>  
>  	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;
> +		}
>  		netif_device_attach(ndev);
>  		netif_start_queue(ndev);
>  	}
> @@ -1030,7 +1059,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 +1103,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,6 +1169,16 @@ 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);
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> +			__func__, ret);
> +		goto err_pmdisable;

after err_pmdisable is a runtime_put(), your cleanup is broken.

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

I think you have to adjust the jump label.

> 	}
> 
> 	devm_can_led_init(ndev);
> 
> 	pm_runtime_put(&pdev->dev);



> @@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
>  	}
>  
>  	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);
>  
>  	return 0;
>  
> +err_pmdisable:
> +	pm_runtime_disable(&pdev->dev);
>  err_unprepare_disable_busclk:
> +	pm_runtime_put(priv->dev);

This cleanup is not correct.

>  	clk_disable_unprepare(priv->bus_clk);
>  err_unprepare_disable_dev:
>  	clk_disable_unprepare(priv->can_clk);
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	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);
> +	pm_runtime_disable(&pdev->dev);
>  	netif_napi_del(&priv->napi);
> +	clk_disable_unprepare(priv->bus_clk);
> +	clk_disable_unprepare(priv->can_clk);
>  	free_candev(ndev);
>  
>  	return 0;
> 

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/12/2015 04:04 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>

FTBFS:

> drivers/net/can/xilinx_can.c:1064:9: error: undefined identifier 'SET_PM_RUNTIME_PM_OPS'
>   CC [M]  drivers/net/can/xilinx_can.o
> drivers/net/can/xilinx_can.c:1064:2: error: implicit declaration of function ?SET_PM_RUNTIME_PM_OPS? [-Werror=implicit-function-declaration]
>   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
>   ^
> drivers/net/can/xilinx_can.c:1065:1: error: initializer element is not constant
>  };
>  ^
> drivers/net/can/xilinx_can.c:1065:1: error: (near initialization for ?xcan_dev_pm_ops.prepare?)

Have a look at commit 40bd62c6194bdee1bc6652b3b0aa28e34883f603:

More comments inline. Looks quite good now.

>     PM: Remove the SET_PM_RUNTIME_PM_OPS() macro
> 
>     There're now no users left of the SET_PM_RUNTIME_PM_OPS() macro, since
>     all have converted to use the SET_RUNTIME_PM_OPS() macro instead, so
>     let's remove it.
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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 |  157 ++++++++++++++++++++++++++++-------------
>  1 files changed, 107 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..67aef00 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(%d)\n",
> +				__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(%d)\n",
> +				__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,18 +1009,18 @@ 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;
> +	u32 isr, status;
>  
>  	ret = clk_enable(priv->bus_clk);
>  	if (ret) {
> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>  	ret = clk_enable(priv->can_clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable clock.\n");
> -		clk_disable_unprepare(priv->bus_clk);
> +		clk_disable(priv->bus_clk);
>  		return ret;
>  	}
>  
>  	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;
> +	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	status = priv->read_reg(priv, XCAN_SR_OFFSET);
>  
>  	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;
> +		}
>  		netif_device_attach(ndev);
>  		netif_start_queue(ndev);
>  	}
> @@ -1030,7 +1059,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 +1103,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,6 +1169,16 @@ 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);
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> +			__func__, ret);
> +		goto err_pmdisable;

after err_pmdisable is a runtime_put(), your cleanup is broken.

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

I think you have to adjust the jump label.

> 	}
> 
> 	devm_can_led_init(ndev);
> 
> 	pm_runtime_put(&pdev->dev);



> @@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
>  	}
>  
>  	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);
>  
>  	return 0;
>  
> +err_pmdisable:
> +	pm_runtime_disable(&pdev->dev);
>  err_unprepare_disable_busclk:
> +	pm_runtime_put(priv->dev);

This cleanup is not correct.

>  	clk_disable_unprepare(priv->bus_clk);
>  err_unprepare_disable_dev:
>  	clk_disable_unprepare(priv->can_clk);
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	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);
> +	pm_runtime_disable(&pdev->dev);
>  	netif_napi_del(&priv->napi);
> +	clk_disable_unprepare(priv->bus_clk);
> +	clk_disable_unprepare(priv->can_clk);
>  	free_candev(ndev);
>  
>  	return 0;
> 

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   |

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

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> On Mon, 2015-01-12 at 08:34PM +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>
>> ---
>> Changes for v5:
>>  - Updated with the review comments.
>>    Updated the remove fuction to use runtime_pm.
>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>  1 files changed, 107 insertions(+), 50 deletions(-)
> [..]
>> +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;
>> +	u32 isr, status;
>>  
>>  	ret = clk_enable(priv->bus_clk);
>>  	if (ret) {
>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>  	ret = clk_enable(priv->can_clk);
>>  	if (ret) {
>>  		dev_err(dev, "Cannot enable clock.\n");
>> -		clk_disable_unprepare(priv->bus_clk);
>> +		clk_disable(priv->bus_clk);
> [...]
>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>  {
>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>  	struct xcan_priv *priv = netdev_priv(ndev);
>> +	int ret;
>> +
>> +	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);
>> +	pm_runtime_disable(&pdev->dev);
>>  	netif_napi_del(&priv->napi);
>> +	clk_disable_unprepare(priv->bus_clk);
>> +	clk_disable_unprepare(priv->can_clk);
> 
> Shouldn't pretty much all these occurrences of clk_disable/enable
> disappear? This should all be handled by the runtime_pm framework now.

We have:
- clk_prepare_enable() in probe
- clk_disable_unprepare() in remove
- clk_enable() in runtime_resume
- clk_disable() in runtime_suspend

Which is, as far as I understand the right way to do it. Maybe
Kedareswara can post the clock debug output again with this patch
iteration. Have I missed something?

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
> On Mon, 2015-01-12 at 08:34PM +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>
>> ---
>> Changes for v5:
>>  - Updated with the review comments.
>>    Updated the remove fuction to use runtime_pm.
>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>  1 files changed, 107 insertions(+), 50 deletions(-)
> [..]
>> +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;
>> +	u32 isr, status;
>>  
>>  	ret = clk_enable(priv->bus_clk);
>>  	if (ret) {
>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>  	ret = clk_enable(priv->can_clk);
>>  	if (ret) {
>>  		dev_err(dev, "Cannot enable clock.\n");
>> -		clk_disable_unprepare(priv->bus_clk);
>> +		clk_disable(priv->bus_clk);
> [...]
>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>  {
>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>  	struct xcan_priv *priv = netdev_priv(ndev);
>> +	int ret;
>> +
>> +	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);
>> +	pm_runtime_disable(&pdev->dev);
>>  	netif_napi_del(&priv->napi);
>> +	clk_disable_unprepare(priv->bus_clk);
>> +	clk_disable_unprepare(priv->can_clk);
> 
> Shouldn't pretty much all these occurrences of clk_disable/enable
> disappear? This should all be handled by the runtime_pm framework now.

We have:
- clk_prepare_enable() in probe
- clk_disable_unprepare() in remove
- clk_enable() in runtime_resume
- clk_disable() in runtime_suspend

Which is, as far as I understand the right way to do it. Maybe
Kedareswara can post the clock debug output again with this patch
iteration. Have I missed something?

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   |

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

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> > On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >> ---
> >> Changes for v5:
> >>  - Updated with the review comments.
> >>    Updated the remove fuction to use runtime_pm.
> >> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>  1 files changed, 107 insertions(+), 50 deletions(-)
> > [..]
> >> +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;
> >> +	u32 isr, status;
> >>  
> >>  	ret = clk_enable(priv->bus_clk);
> >>  	if (ret) {
> >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>  	ret = clk_enable(priv->can_clk);
> >>  	if (ret) {
> >>  		dev_err(dev, "Cannot enable clock.\n");
> >> -		clk_disable_unprepare(priv->bus_clk);
> >> +		clk_disable(priv->bus_clk);
> > [...]
> >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>  {
> >>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>  	struct xcan_priv *priv = netdev_priv(ndev);
> >> +	int ret;
> >> +
> >> +	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);
> >> +	pm_runtime_disable(&pdev->dev);
> >>  	netif_napi_del(&priv->napi);
> >> +	clk_disable_unprepare(priv->bus_clk);
> >> +	clk_disable_unprepare(priv->can_clk);
> > 
> > Shouldn't pretty much all these occurrences of clk_disable/enable
> > disappear? This should all be handled by the runtime_pm framework now.
> 
> We have:
> - clk_prepare_enable() in probe

This should become something like pm_runtime_get_sync(), shouldn't it?

> - clk_disable_unprepare() in remove

pm_runtime_put()

> - clk_enable() in runtime_resume
> - clk_disable() in runtime_suspend

These are the ones needed.

The above makes me suspect that the clocks are always on, regardless of
the runtime suspend state since they are enabled in probe and disabled
in remove, is that right? Ideally, the usage in probe and remove should
be migrated to runtime_pm and clocks should really only be running when
needed and not throughout the whole lifetime of the driver.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> > On Mon, 2015-01-12 at 08:34PM +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>
> >> ---
> >> Changes for v5:
> >>  - Updated with the review comments.
> >>    Updated the remove fuction to use runtime_pm.
> >> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>  1 files changed, 107 insertions(+), 50 deletions(-)
> > [..]
> >> +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;
> >> +	u32 isr, status;
> >>  
> >>  	ret = clk_enable(priv->bus_clk);
> >>  	if (ret) {
> >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>  	ret = clk_enable(priv->can_clk);
> >>  	if (ret) {
> >>  		dev_err(dev, "Cannot enable clock.\n");
> >> -		clk_disable_unprepare(priv->bus_clk);
> >> +		clk_disable(priv->bus_clk);
> > [...]
> >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>  {
> >>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>  	struct xcan_priv *priv = netdev_priv(ndev);
> >> +	int ret;
> >> +
> >> +	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);
> >> +	pm_runtime_disable(&pdev->dev);
> >>  	netif_napi_del(&priv->napi);
> >> +	clk_disable_unprepare(priv->bus_clk);
> >> +	clk_disable_unprepare(priv->can_clk);
> > 
> > Shouldn't pretty much all these occurrences of clk_disable/enable
> > disappear? This should all be handled by the runtime_pm framework now.
> 
> We have:
> - clk_prepare_enable() in probe

This should become something like pm_runtime_get_sync(), shouldn't it?

> - clk_disable_unprepare() in remove

pm_runtime_put()

> - clk_enable() in runtime_resume
> - clk_disable() in runtime_suspend

These are the ones needed.

The above makes me suspect that the clocks are always on, regardless of
the runtime suspend state since they are enabled in probe and disabled
in remove, is that right? Ideally, the usage in probe and remove should
be migrated to runtime_pm and clocks should really only be running when
needed and not throughout the whole lifetime of the driver.

	Sören

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> > On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >> ---
> >> Changes for v5:
> >>  - Updated with the review comments.
> >>    Updated the remove fuction to use runtime_pm.
> >> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>  1 files changed, 107 insertions(+), 50 deletions(-)
> > [..]
> >> +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;
> >> +	u32 isr, status;
> >>  
> >>  	ret = clk_enable(priv->bus_clk);
> >>  	if (ret) {
> >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>  	ret = clk_enable(priv->can_clk);
> >>  	if (ret) {
> >>  		dev_err(dev, "Cannot enable clock.\n");
> >> -		clk_disable_unprepare(priv->bus_clk);
> >> +		clk_disable(priv->bus_clk);
> > [...]
> >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>  {
> >>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>  	struct xcan_priv *priv = netdev_priv(ndev);
> >> +	int ret;
> >> +
> >> +	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);
> >> +	pm_runtime_disable(&pdev->dev);
> >>  	netif_napi_del(&priv->napi);
> >> +	clk_disable_unprepare(priv->bus_clk);
> >> +	clk_disable_unprepare(priv->can_clk);
> > 
> > Shouldn't pretty much all these occurrences of clk_disable/enable
> > disappear? This should all be handled by the runtime_pm framework now.
> 
> We have:
> - clk_prepare_enable() in probe

This should become something like pm_runtime_get_sync(), shouldn't it?

> - clk_disable_unprepare() in remove

pm_runtime_put()

> - clk_enable() in runtime_resume
> - clk_disable() in runtime_suspend

These are the ones needed.

The above makes me suspect that the clocks are always on, regardless of
the runtime suspend state since they are enabled in probe and disabled
in remove, is that right? Ideally, the usage in probe and remove should
be migrated to runtime_pm and clocks should really only be running when
needed and not throughout the whole lifetime of the driver.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
> > On Mon, 2015-01-12 at 08:34PM +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>
> >> ---
> >> Changes for v5:
> >>  - Updated with the review comments.
> >>    Updated the remove fuction to use runtime_pm.
> >> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>  1 files changed, 107 insertions(+), 50 deletions(-)
> > [..]
> >> +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;
> >> +	u32 isr, status;
> >>  
> >>  	ret = clk_enable(priv->bus_clk);
> >>  	if (ret) {
> >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>  	ret = clk_enable(priv->can_clk);
> >>  	if (ret) {
> >>  		dev_err(dev, "Cannot enable clock.\n");
> >> -		clk_disable_unprepare(priv->bus_clk);
> >> +		clk_disable(priv->bus_clk);
> > [...]
> >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>  {
> >>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>  	struct xcan_priv *priv = netdev_priv(ndev);
> >> +	int ret;
> >> +
> >> +	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);
> >> +	pm_runtime_disable(&pdev->dev);
> >>  	netif_napi_del(&priv->napi);
> >> +	clk_disable_unprepare(priv->bus_clk);
> >> +	clk_disable_unprepare(priv->can_clk);
> > 
> > Shouldn't pretty much all these occurrences of clk_disable/enable
> > disappear? This should all be handled by the runtime_pm framework now.
> 
> We have:
> - clk_prepare_enable() in probe

This should become something like pm_runtime_get_sync(), shouldn't it?

> - clk_disable_unprepare() in remove

pm_runtime_put()

> - clk_enable() in runtime_resume
> - clk_disable() in runtime_suspend

These are the ones needed.

The above makes me suspect that the clocks are always on, regardless of
the runtime suspend state since they are enabled in probe and disabled
in remove, is that right? Ideally, the usage in probe and remove should
be migrated to runtime_pm and clocks should really only be running when
needed and not throughout the whole lifetime of the driver.

	S?ren

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>> ---
>>>> Changes for v5:
>>>>  - Updated with the review comments.
>>>>    Updated the remove fuction to use runtime_pm.
>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>> [..]
>>>> +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;
>>>> +	u32 isr, status;
>>>>  
>>>>  	ret = clk_enable(priv->bus_clk);
>>>>  	if (ret) {
>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>  	ret = clk_enable(priv->can_clk);
>>>>  	if (ret) {
>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>> +		clk_disable(priv->bus_clk);
>>> [...]
>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>  {
>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>> +	int ret;
>>>> +
>>>> +	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);
>>>> +	pm_runtime_disable(&pdev->dev);
>>>>  	netif_napi_del(&priv->napi);
>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>> +	clk_disable_unprepare(priv->can_clk);
>>>
>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>> disappear? This should all be handled by the runtime_pm framework now.
>>
>> We have:
>> - clk_prepare_enable() in probe
> 
> This should become something like pm_runtime_get_sync(), shouldn't it?
> 
>> - clk_disable_unprepare() in remove
> 
> pm_runtime_put()
> 
>> - clk_enable() in runtime_resume
>> - clk_disable() in runtime_suspend
> 
> These are the ones needed.
> 
> The above makes me suspect that the clocks are always on, regardless of

Define "on" :)
The clocks are prepared after probe() exists, but not enabled. The first
pm_runtime_get_sync() will enable the clocks.

> the runtime suspend state since they are enabled in probe and disabled
> in remove, is that right? Ideally, the usage in probe and remove should
> be migrated to runtime_pm and clocks should really only be running when
> needed and not throughout the whole lifetime of the driver.

The clocks are not en/disabled via pm_runtime, because
pm_runtime_get_sync() is called from atomic contect. We can have another
look into the driver and try to change this.

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>> ---
>>>> Changes for v5:
>>>>  - Updated with the review comments.
>>>>    Updated the remove fuction to use runtime_pm.
>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>> [..]
>>>> +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;
>>>> +	u32 isr, status;
>>>>  
>>>>  	ret = clk_enable(priv->bus_clk);
>>>>  	if (ret) {
>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>  	ret = clk_enable(priv->can_clk);
>>>>  	if (ret) {
>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>> +		clk_disable(priv->bus_clk);
>>> [...]
>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>  {
>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>> +	int ret;
>>>> +
>>>> +	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);
>>>> +	pm_runtime_disable(&pdev->dev);
>>>>  	netif_napi_del(&priv->napi);
>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>> +	clk_disable_unprepare(priv->can_clk);
>>>
>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>> disappear? This should all be handled by the runtime_pm framework now.
>>
>> We have:
>> - clk_prepare_enable() in probe
> 
> This should become something like pm_runtime_get_sync(), shouldn't it?
> 
>> - clk_disable_unprepare() in remove
> 
> pm_runtime_put()
> 
>> - clk_enable() in runtime_resume
>> - clk_disable() in runtime_suspend
> 
> These are the ones needed.
> 
> The above makes me suspect that the clocks are always on, regardless of

Define "on" :)
The clocks are prepared after probe() exists, but not enabled. The first
pm_runtime_get_sync() will enable the clocks.

> the runtime suspend state since they are enabled in probe and disabled
> in remove, is that right? Ideally, the usage in probe and remove should
> be migrated to runtime_pm and clocks should really only be running when
> needed and not throughout the whole lifetime of the driver.

The clocks are not en/disabled via pm_runtime, because
pm_runtime_get_sync() is called from atomic contect. We can have another
look into the driver and try to change this.

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   |

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

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>> ---
> >>>> Changes for v5:
> >>>>  - Updated with the review comments.
> >>>>    Updated the remove fuction to use runtime_pm.
> >>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>> [..]
> >>>> +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;
> >>>> +	u32 isr, status;
> >>>>  
> >>>>  	ret = clk_enable(priv->bus_clk);
> >>>>  	if (ret) {
> >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>  	ret = clk_enable(priv->can_clk);
> >>>>  	if (ret) {
> >>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>> +		clk_disable(priv->bus_clk);
> >>> [...]
> >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>  {
> >>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>> +	int ret;
> >>>> +
> >>>> +	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);
> >>>> +	pm_runtime_disable(&pdev->dev);
> >>>>  	netif_napi_del(&priv->napi);
> >>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>> +	clk_disable_unprepare(priv->can_clk);
> >>>
> >>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>> disappear? This should all be handled by the runtime_pm framework now.
> >>
> >> We have:
> >> - clk_prepare_enable() in probe
> > 
> > This should become something like pm_runtime_get_sync(), shouldn't it?
> > 
> >> - clk_disable_unprepare() in remove
> > 
> > pm_runtime_put()
> > 
> >> - clk_enable() in runtime_resume
> >> - clk_disable() in runtime_suspend
> > 
> > These are the ones needed.
> > 
> > The above makes me suspect that the clocks are always on, regardless of
> 
> Define "on" :)
> The clocks are prepared after probe() exists, but not enabled. The first
> pm_runtime_get_sync() will enable the clocks.
> 
> > the runtime suspend state since they are enabled in probe and disabled
> > in remove, is that right? Ideally, the usage in probe and remove should
> > be migrated to runtime_pm and clocks should really only be running when
> > needed and not throughout the whole lifetime of the driver.
> 
> The clocks are not en/disabled via pm_runtime, because
> pm_runtime_get_sync() is called from atomic contect. We can have another
> look into the driver and try to change this.

Wasn't that why the call to pm_runtime_irq_safe() was added?
Also, clk_enable/disable should be okay to be run from atomic context.
And if the clock are already prepared after the exit of probe that
should be enough. Then remove() should just have to do the unprepare.
But I don't see why runtime_pm shouldn't be able to do the
enable/disable.

	Sören

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>> ---
> >>>> Changes for v5:
> >>>>  - Updated with the review comments.
> >>>>    Updated the remove fuction to use runtime_pm.
> >>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>> [..]
> >>>> +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;
> >>>> +	u32 isr, status;
> >>>>  
> >>>>  	ret = clk_enable(priv->bus_clk);
> >>>>  	if (ret) {
> >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>  	ret = clk_enable(priv->can_clk);
> >>>>  	if (ret) {
> >>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>> +		clk_disable(priv->bus_clk);
> >>> [...]
> >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>  {
> >>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>> +	int ret;
> >>>> +
> >>>> +	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);
> >>>> +	pm_runtime_disable(&pdev->dev);
> >>>>  	netif_napi_del(&priv->napi);
> >>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>> +	clk_disable_unprepare(priv->can_clk);
> >>>
> >>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>> disappear? This should all be handled by the runtime_pm framework now.
> >>
> >> We have:
> >> - clk_prepare_enable() in probe
> > 
> > This should become something like pm_runtime_get_sync(), shouldn't it?
> > 
> >> - clk_disable_unprepare() in remove
> > 
> > pm_runtime_put()
> > 
> >> - clk_enable() in runtime_resume
> >> - clk_disable() in runtime_suspend
> > 
> > These are the ones needed.
> > 
> > The above makes me suspect that the clocks are always on, regardless of
> 
> Define "on" :)
> The clocks are prepared after probe() exists, but not enabled. The first
> pm_runtime_get_sync() will enable the clocks.
> 
> > the runtime suspend state since they are enabled in probe and disabled
> > in remove, is that right? Ideally, the usage in probe and remove should
> > be migrated to runtime_pm and clocks should really only be running when
> > needed and not throughout the whole lifetime of the driver.
> 
> The clocks are not en/disabled via pm_runtime, because
> pm_runtime_get_sync() is called from atomic contect. We can have another
> look into the driver and try to change this.

Wasn't that why the call to pm_runtime_irq_safe() was added?
Also, clk_enable/disable should be okay to be run from atomic context.
And if the clock are already prepared after the exit of probe that
should be enough. Then remove() should just have to do the unprepare.
But I don't see why runtime_pm shouldn't be able to do the
enable/disable.

	Sören

[PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
> > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
> >>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>> ---
> >>>> Changes for v5:
> >>>>  - Updated with the review comments.
> >>>>    Updated the remove fuction to use runtime_pm.
> >>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>> [..]
> >>>> +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;
> >>>> +	u32 isr, status;
> >>>>  
> >>>>  	ret = clk_enable(priv->bus_clk);
> >>>>  	if (ret) {
> >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>  	ret = clk_enable(priv->can_clk);
> >>>>  	if (ret) {
> >>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>> +		clk_disable(priv->bus_clk);
> >>> [...]
> >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>  {
> >>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>> +	int ret;
> >>>> +
> >>>> +	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);
> >>>> +	pm_runtime_disable(&pdev->dev);
> >>>>  	netif_napi_del(&priv->napi);
> >>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>> +	clk_disable_unprepare(priv->can_clk);
> >>>
> >>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>> disappear? This should all be handled by the runtime_pm framework now.
> >>
> >> We have:
> >> - clk_prepare_enable() in probe
> > 
> > This should become something like pm_runtime_get_sync(), shouldn't it?
> > 
> >> - clk_disable_unprepare() in remove
> > 
> > pm_runtime_put()
> > 
> >> - clk_enable() in runtime_resume
> >> - clk_disable() in runtime_suspend
> > 
> > These are the ones needed.
> > 
> > The above makes me suspect that the clocks are always on, regardless of
> 
> Define "on" :)
> The clocks are prepared after probe() exists, but not enabled. The first
> pm_runtime_get_sync() will enable the clocks.
> 
> > the runtime suspend state since they are enabled in probe and disabled
> > in remove, is that right? Ideally, the usage in probe and remove should
> > be migrated to runtime_pm and clocks should really only be running when
> > needed and not throughout the whole lifetime of the driver.
> 
> The clocks are not en/disabled via pm_runtime, because
> pm_runtime_get_sync() is called from atomic contect. We can have another
> look into the driver and try to change this.

Wasn't that why the call to pm_runtime_irq_safe() was added?
Also, clk_enable/disable should be okay to be run from atomic context.
And if the clock are already prepared after the exit of probe that
should be enough. Then remove() should just have to do the unprepare.
But I don't see why runtime_pm shouldn't be able to do the
enable/disable.

	S?ren

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>>>> On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>> Changes for v5:
>>>>>>  - Updated with the review comments.
>>>>>>    Updated the remove fuction to use runtime_pm.
>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>>>> [..]
>>>>>> +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;
>>>>>> +	u32 isr, status;
>>>>>>  
>>>>>>  	ret = clk_enable(priv->bus_clk);
>>>>>>  	if (ret) {
>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>>>  	ret = clk_enable(priv->can_clk);
>>>>>>  	if (ret) {
>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>>>> +		clk_disable(priv->bus_clk);
>>>>> [...]
>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>>>  {
>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	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);
>>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>>>  	netif_napi_del(&priv->napi);
>>>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>>>> +	clk_disable_unprepare(priv->can_clk);
>>>>>
>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>
>>>> We have:
>>>> - clk_prepare_enable() in probe
>>>
>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>
>>>> - clk_disable_unprepare() in remove
>>>
>>> pm_runtime_put()
>>>
>>>> - clk_enable() in runtime_resume
>>>> - clk_disable() in runtime_suspend
>>>
>>> These are the ones needed.
>>>
>>> The above makes me suspect that the clocks are always on, regardless of
>>
>> Define "on" :)
>> The clocks are prepared after probe() exists, but not enabled. The first
>> pm_runtime_get_sync() will enable the clocks.
>>
>>> the runtime suspend state since they are enabled in probe and disabled
>>> in remove, is that right? Ideally, the usage in probe and remove should
>>> be migrated to runtime_pm and clocks should really only be running when
>>> needed and not throughout the whole lifetime of the driver.
>>
>> The clocks are not en/disabled via pm_runtime, because
>> pm_runtime_get_sync() is called from atomic contect. We can have another
>> look into the driver and try to change this.

> Wasn't that why the call to pm_runtime_irq_safe() was added?

Good question. That should be investigated.

> Also, clk_enable/disable should be okay to be run from atomic context.
> And if the clock are already prepared after the exit of probe that
> should be enough. Then remove() should just have to do the unprepare.
> But I don't see why runtime_pm shouldn't be able to do the
> enable/disable.

runtime_pm does call the clk_{enable,disable} function. But you mean
clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
of calling clk_prepare_enable(). Good idea! I think the
"pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Coming back whether blocking calls are allowed or not.
If you make a call to pm_runtime_irq_safe(), you state that it's okay to
call pm_runtime_get_sync() from atomic context. But it's only called in
open, probe, remove and in xcan_get_berr_counter, which is not called
from atomic either. So let's try to remove the pm_runtime_irq_safe() and
use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
runtime_suspend() functions.

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 v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>>>> ---
>>>>>> Changes for v5:
>>>>>>  - Updated with the review comments.
>>>>>>    Updated the remove fuction to use runtime_pm.
>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>>>> [..]
>>>>>> +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;
>>>>>> +	u32 isr, status;
>>>>>>  
>>>>>>  	ret = clk_enable(priv->bus_clk);
>>>>>>  	if (ret) {
>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>>>  	ret = clk_enable(priv->can_clk);
>>>>>>  	if (ret) {
>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>>>> +		clk_disable(priv->bus_clk);
>>>>> [...]
>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>>>  {
>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	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);
>>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>>>  	netif_napi_del(&priv->napi);
>>>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>>>> +	clk_disable_unprepare(priv->can_clk);
>>>>>
>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>
>>>> We have:
>>>> - clk_prepare_enable() in probe
>>>
>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>
>>>> - clk_disable_unprepare() in remove
>>>
>>> pm_runtime_put()
>>>
>>>> - clk_enable() in runtime_resume
>>>> - clk_disable() in runtime_suspend
>>>
>>> These are the ones needed.
>>>
>>> The above makes me suspect that the clocks are always on, regardless of
>>
>> Define "on" :)
>> The clocks are prepared after probe() exists, but not enabled. The first
>> pm_runtime_get_sync() will enable the clocks.
>>
>>> the runtime suspend state since they are enabled in probe and disabled
>>> in remove, is that right? Ideally, the usage in probe and remove should
>>> be migrated to runtime_pm and clocks should really only be running when
>>> needed and not throughout the whole lifetime of the driver.
>>
>> The clocks are not en/disabled via pm_runtime, because
>> pm_runtime_get_sync() is called from atomic contect. We can have another
>> look into the driver and try to change this.

> Wasn't that why the call to pm_runtime_irq_safe() was added?

Good question. That should be investigated.

> Also, clk_enable/disable should be okay to be run from atomic context.
> And if the clock are already prepared after the exit of probe that
> should be enough. Then remove() should just have to do the unprepare.
> But I don't see why runtime_pm shouldn't be able to do the
> enable/disable.

runtime_pm does call the clk_{enable,disable} function. But you mean
clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
of calling clk_prepare_enable(). Good idea! I think the
"pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Coming back whether blocking calls are allowed or not.
If you make a call to pm_runtime_irq_safe(), you state that it's okay to
call pm_runtime_get_sync() from atomic context. But it's only called in
open, probe, remove and in xcan_get_berr_counter, which is not called
from atomic either. So let's try to remove the pm_runtime_irq_safe() and
use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
runtime_suspend() functions.

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/13/2015 06:24 PM, S?ren Brinkmann wrote:
> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
>>>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>>>> ---
>>>>>> Changes for v5:
>>>>>>  - Updated with the review comments.
>>>>>>    Updated the remove fuction to use runtime_pm.
>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>>>> [..]
>>>>>> +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;
>>>>>> +	u32 isr, status;
>>>>>>  
>>>>>>  	ret = clk_enable(priv->bus_clk);
>>>>>>  	if (ret) {
>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>>>  	ret = clk_enable(priv->can_clk);
>>>>>>  	if (ret) {
>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>>>> +		clk_disable(priv->bus_clk);
>>>>> [...]
>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>>>  {
>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	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);
>>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>>>  	netif_napi_del(&priv->napi);
>>>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>>>> +	clk_disable_unprepare(priv->can_clk);
>>>>>
>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>
>>>> We have:
>>>> - clk_prepare_enable() in probe
>>>
>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>
>>>> - clk_disable_unprepare() in remove
>>>
>>> pm_runtime_put()
>>>
>>>> - clk_enable() in runtime_resume
>>>> - clk_disable() in runtime_suspend
>>>
>>> These are the ones needed.
>>>
>>> The above makes me suspect that the clocks are always on, regardless of
>>
>> Define "on" :)
>> The clocks are prepared after probe() exists, but not enabled. The first
>> pm_runtime_get_sync() will enable the clocks.
>>
>>> the runtime suspend state since they are enabled in probe and disabled
>>> in remove, is that right? Ideally, the usage in probe and remove should
>>> be migrated to runtime_pm and clocks should really only be running when
>>> needed and not throughout the whole lifetime of the driver.
>>
>> The clocks are not en/disabled via pm_runtime, because
>> pm_runtime_get_sync() is called from atomic contect. We can have another
>> look into the driver and try to change this.

> Wasn't that why the call to pm_runtime_irq_safe() was added?

Good question. That should be investigated.

> Also, clk_enable/disable should be okay to be run from atomic context.
> And if the clock are already prepared after the exit of probe that
> should be enough. Then remove() should just have to do the unprepare.
> But I don't see why runtime_pm shouldn't be able to do the
> enable/disable.

runtime_pm does call the clk_{enable,disable} function. But you mean
clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
of calling clk_prepare_enable(). Good idea! I think the
"pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Coming back whether blocking calls are allowed or not.
If you make a call to pm_runtime_irq_safe(), you state that it's okay to
call pm_runtime_get_sync() from atomic context. But it's only called in
open, probe, remove and in xcan_get_berr_counter, which is not called
from atomic either. So let's try to remove the pm_runtime_irq_safe() and
use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
runtime_suspend() functions.

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   |

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

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>> On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>> ---
> >>>>>> Changes for v5:
> >>>>>>  - Updated with the review comments.
> >>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>> [..]
> >>>>>> +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;
> >>>>>> +	u32 isr, status;
> >>>>>>  
> >>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>  	if (ret) {
> >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>  	if (ret) {
> >>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>> +		clk_disable(priv->bus_clk);
> >>>>> [...]
> >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>  {
> >>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	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);
> >>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>  	netif_napi_del(&priv->napi);
> >>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>
> >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>
> >>>> We have:
> >>>> - clk_prepare_enable() in probe
> >>>
> >>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>
> >>>> - clk_disable_unprepare() in remove
> >>>
> >>> pm_runtime_put()
> >>>
> >>>> - clk_enable() in runtime_resume
> >>>> - clk_disable() in runtime_suspend
> >>>
> >>> These are the ones needed.
> >>>
> >>> The above makes me suspect that the clocks are always on, regardless of
> >>
> >> Define "on" :)
> >> The clocks are prepared after probe() exists, but not enabled. The first
> >> pm_runtime_get_sync() will enable the clocks.
> >>
> >>> the runtime suspend state since they are enabled in probe and disabled
> >>> in remove, is that right? Ideally, the usage in probe and remove should
> >>> be migrated to runtime_pm and clocks should really only be running when
> >>> needed and not throughout the whole lifetime of the driver.
> >>
> >> The clocks are not en/disabled via pm_runtime, because
> >> pm_runtime_get_sync() is called from atomic contect. We can have another
> >> look into the driver and try to change this.
> 
> > Wasn't that why the call to pm_runtime_irq_safe() was added?
> 
> Good question. That should be investigated.
> 
> > Also, clk_enable/disable should be okay to be run from atomic context.
> > And if the clock are already prepared after the exit of probe that
> > should be enough. Then remove() should just have to do the unprepare.
> > But I don't see why runtime_pm shouldn't be able to do the
> > enable/disable.
> 
> runtime_pm does call the clk_{enable,disable} function. But you mean
> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> of calling clk_prepare_enable(). Good idea! I think the
> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Right, that's what I was thinking. The proposed changes make sense, IMHO.

> 
> Coming back whether blocking calls are allowed or not.
> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> call pm_runtime_get_sync() from atomic context. But it's only called in
> open, probe, remove and in xcan_get_berr_counter, which is not called
> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> runtime_suspend() functions.

IIRC, xcan_get_berr_counter() is called from atomic context. I think
that was how this got started.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>>>> ---
> >>>>>> Changes for v5:
> >>>>>>  - Updated with the review comments.
> >>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>> [..]
> >>>>>> +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;
> >>>>>> +	u32 isr, status;
> >>>>>>  
> >>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>  	if (ret) {
> >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>  	if (ret) {
> >>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>> +		clk_disable(priv->bus_clk);
> >>>>> [...]
> >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>  {
> >>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	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);
> >>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>  	netif_napi_del(&priv->napi);
> >>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>
> >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>
> >>>> We have:
> >>>> - clk_prepare_enable() in probe
> >>>
> >>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>
> >>>> - clk_disable_unprepare() in remove
> >>>
> >>> pm_runtime_put()
> >>>
> >>>> - clk_enable() in runtime_resume
> >>>> - clk_disable() in runtime_suspend
> >>>
> >>> These are the ones needed.
> >>>
> >>> The above makes me suspect that the clocks are always on, regardless of
> >>
> >> Define "on" :)
> >> The clocks are prepared after probe() exists, but not enabled. The first
> >> pm_runtime_get_sync() will enable the clocks.
> >>
> >>> the runtime suspend state since they are enabled in probe and disabled
> >>> in remove, is that right? Ideally, the usage in probe and remove should
> >>> be migrated to runtime_pm and clocks should really only be running when
> >>> needed and not throughout the whole lifetime of the driver.
> >>
> >> The clocks are not en/disabled via pm_runtime, because
> >> pm_runtime_get_sync() is called from atomic contect. We can have another
> >> look into the driver and try to change this.
> 
> > Wasn't that why the call to pm_runtime_irq_safe() was added?
> 
> Good question. That should be investigated.
> 
> > Also, clk_enable/disable should be okay to be run from atomic context.
> > And if the clock are already prepared after the exit of probe that
> > should be enough. Then remove() should just have to do the unprepare.
> > But I don't see why runtime_pm shouldn't be able to do the
> > enable/disable.
> 
> runtime_pm does call the clk_{enable,disable} function. But you mean
> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> of calling clk_prepare_enable(). Good idea! I think the
> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Right, that's what I was thinking. The proposed changes make sense, IMHO.

> 
> Coming back whether blocking calls are allowed or not.
> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> call pm_runtime_get_sync() from atomic context. But it's only called in
> open, probe, remove and in xcan_get_berr_counter, which is not called
> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> runtime_suspend() functions.

IIRC, xcan_get_berr_counter() is called from atomic context. I think
that was how this got started.

	Sören

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>> On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>> ---
> >>>>>> Changes for v5:
> >>>>>>  - Updated with the review comments.
> >>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>> [..]
> >>>>>> +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;
> >>>>>> +	u32 isr, status;
> >>>>>>  
> >>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>  	if (ret) {
> >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>  	if (ret) {
> >>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>> +		clk_disable(priv->bus_clk);
> >>>>> [...]
> >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>  {
> >>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	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);
> >>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>  	netif_napi_del(&priv->napi);
> >>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>
> >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>
> >>>> We have:
> >>>> - clk_prepare_enable() in probe
> >>>
> >>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>
> >>>> - clk_disable_unprepare() in remove
> >>>
> >>> pm_runtime_put()
> >>>
> >>>> - clk_enable() in runtime_resume
> >>>> - clk_disable() in runtime_suspend
> >>>
> >>> These are the ones needed.
> >>>
> >>> The above makes me suspect that the clocks are always on, regardless of
> >>
> >> Define "on" :)
> >> The clocks are prepared after probe() exists, but not enabled. The first
> >> pm_runtime_get_sync() will enable the clocks.
> >>
> >>> the runtime suspend state since they are enabled in probe and disabled
> >>> in remove, is that right? Ideally, the usage in probe and remove should
> >>> be migrated to runtime_pm and clocks should really only be running when
> >>> needed and not throughout the whole lifetime of the driver.
> >>
> >> The clocks are not en/disabled via pm_runtime, because
> >> pm_runtime_get_sync() is called from atomic contect. We can have another
> >> look into the driver and try to change this.
> 
> > Wasn't that why the call to pm_runtime_irq_safe() was added?
> 
> Good question. That should be investigated.
> 
> > Also, clk_enable/disable should be okay to be run from atomic context.
> > And if the clock are already prepared after the exit of probe that
> > should be enough. Then remove() should just have to do the unprepare.
> > But I don't see why runtime_pm shouldn't be able to do the
> > enable/disable.
> 
> runtime_pm does call the clk_{enable,disable} function. But you mean
> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> of calling clk_prepare_enable(). Good idea! I think the
> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Right, that's what I was thinking. The proposed changes make sense, IMHO.

> 
> Coming back whether blocking calls are allowed or not.
> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> call pm_runtime_get_sync() from atomic context. But it's only called in
> open, probe, remove and in xcan_get_berr_counter, which is not called
> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> runtime_suspend() functions.

IIRC, xcan_get_berr_counter() is called from atomic context. I think
that was how this got started.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:24 PM, S?ren Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
> >>>>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>>>> ---
> >>>>>> Changes for v5:
> >>>>>>  - Updated with the review comments.
> >>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>> [..]
> >>>>>> +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;
> >>>>>> +	u32 isr, status;
> >>>>>>  
> >>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>  	if (ret) {
> >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>  	if (ret) {
> >>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>> +		clk_disable(priv->bus_clk);
> >>>>> [...]
> >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>  {
> >>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	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);
> >>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>  	netif_napi_del(&priv->napi);
> >>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>
> >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>
> >>>> We have:
> >>>> - clk_prepare_enable() in probe
> >>>
> >>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>
> >>>> - clk_disable_unprepare() in remove
> >>>
> >>> pm_runtime_put()
> >>>
> >>>> - clk_enable() in runtime_resume
> >>>> - clk_disable() in runtime_suspend
> >>>
> >>> These are the ones needed.
> >>>
> >>> The above makes me suspect that the clocks are always on, regardless of
> >>
> >> Define "on" :)
> >> The clocks are prepared after probe() exists, but not enabled. The first
> >> pm_runtime_get_sync() will enable the clocks.
> >>
> >>> the runtime suspend state since they are enabled in probe and disabled
> >>> in remove, is that right? Ideally, the usage in probe and remove should
> >>> be migrated to runtime_pm and clocks should really only be running when
> >>> needed and not throughout the whole lifetime of the driver.
> >>
> >> The clocks are not en/disabled via pm_runtime, because
> >> pm_runtime_get_sync() is called from atomic contect. We can have another
> >> look into the driver and try to change this.
> 
> > Wasn't that why the call to pm_runtime_irq_safe() was added?
> 
> Good question. That should be investigated.
> 
> > Also, clk_enable/disable should be okay to be run from atomic context.
> > And if the clock are already prepared after the exit of probe that
> > should be enough. Then remove() should just have to do the unprepare.
> > But I don't see why runtime_pm shouldn't be able to do the
> > enable/disable.
> 
> runtime_pm does call the clk_{enable,disable} function. But you mean
> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> of calling clk_prepare_enable(). Good idea! I think the
> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.

Right, that's what I was thinking. The proposed changes make sense, IMHO.

> 
> Coming back whether blocking calls are allowed or not.
> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> call pm_runtime_get_sync() from atomic context. But it's only called in
> open, probe, remove and in xcan_get_berr_counter, which is not called
> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> runtime_suspend() functions.

IIRC, xcan_get_berr_counter() is called from atomic context. I think
that was how this got started.

	S?ren

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
>>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
>>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>>>>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>>>>>> ---
>>>>>>>> Changes for v5:
>>>>>>>>  - Updated with the review comments.
>>>>>>>>    Updated the remove fuction to use runtime_pm.
>>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>>>>>> [..]
>>>>>>>> +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;
>>>>>>>> +	u32 isr, status;
>>>>>>>>  
>>>>>>>>  	ret = clk_enable(priv->bus_clk);
>>>>>>>>  	if (ret) {
>>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>>>>>  	ret = clk_enable(priv->can_clk);
>>>>>>>>  	if (ret) {
>>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>>>>>> +		clk_disable(priv->bus_clk);
>>>>>>> [...]
>>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>>>>>  {
>>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	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);
>>>>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>>>>>  	netif_napi_del(&priv->napi);
>>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>>>>>> +	clk_disable_unprepare(priv->can_clk);
>>>>>>>
>>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>>>
>>>>>> We have:
>>>>>> - clk_prepare_enable() in probe
>>>>>
>>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>>>
>>>>>> - clk_disable_unprepare() in remove
>>>>>
>>>>> pm_runtime_put()
>>>>>
>>>>>> - clk_enable() in runtime_resume
>>>>>> - clk_disable() in runtime_suspend
>>>>>
>>>>> These are the ones needed.
>>>>>
>>>>> The above makes me suspect that the clocks are always on, regardless of
>>>>
>>>> Define "on" :)
>>>> The clocks are prepared after probe() exists, but not enabled. The first
>>>> pm_runtime_get_sync() will enable the clocks.
>>>>
>>>>> the runtime suspend state since they are enabled in probe and disabled
>>>>> in remove, is that right? Ideally, the usage in probe and remove should
>>>>> be migrated to runtime_pm and clocks should really only be running when
>>>>> needed and not throughout the whole lifetime of the driver.
>>>>
>>>> The clocks are not en/disabled via pm_runtime, because
>>>> pm_runtime_get_sync() is called from atomic contect. We can have another
>>>> look into the driver and try to change this.
>>
>>> Wasn't that why the call to pm_runtime_irq_safe() was added?
>>
>> Good question. That should be investigated.
>>
>>> Also, clk_enable/disable should be okay to be run from atomic context.
>>> And if the clock are already prepared after the exit of probe that
>>> should be enough. Then remove() should just have to do the unprepare.
>>> But I don't see why runtime_pm shouldn't be able to do the
>>> enable/disable.
>>
>> runtime_pm does call the clk_{enable,disable} function. But you mean
>> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
>> of calling clk_prepare_enable(). Good idea! I think the
>> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> 
> Right, that's what I was thinking. The proposed changes make sense, IMHO.
> 
>>
>> Coming back whether blocking calls are allowed or not.
>> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
>> call pm_runtime_get_sync() from atomic context. But it's only called in
>> open, probe, remove and in xcan_get_berr_counter, which is not called
>> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
>> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
>> runtime_suspend() functions.
> 
> IIRC, xcan_get_berr_counter() is called from atomic context. I think
> that was how this got started.

In some drivers the get_berr_counter() function is used in the irq
handler, but here it's only called from outside, an thus from non atomic
context.

From an older mail of yours:

> I have the feeling I'm missing something. If I remove the 'must not
> sleep' requirement from the runtime suspend/resume functions, I get
> this:
> 
> BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954

http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954

I think it's failing because of the pm_runtime_irq_safe() call.

> in_atomic(): 0, irqs_disabled(): 0, pid: 161, name: ip
> INFO: lockdep is turned off.
> CPU: 0 PID: 161 Comm: ip Not tainted 3.18.0-rc1-xilinx-00059-g21da26693b61-dirty #104
> [<c00186a8>] (unwind_backtrace) from [<c00139f4>] (show_stack+0x20/0x24)
> [<c00139f4>] (show_stack) from [<c055a41c>] (dump_stack+0x8c/0xd0)
> [<c055a41c>] (dump_stack) from [<c0054808>] (__might_sleep+0x1ac/0x1e4)
> [<c0054808>] (__might_sleep) from [<c034f8f0>] (__pm_runtime_resume+0x40/0x9c)
> [<c034f8f0>] (__pm_runtime_resume) from [<c03b48d8>] (xcan_get_berr_counter+0x2c/0x9c)
> [<c03b48d8>] (xcan_get_berr_counter) from [<c03b2ecc>] (can_fill_info+0x160/0x1f4)
> [<c03b2ecc>] (can_fill_info) from [<c049f3b0>] (rtnl_fill_ifinfo+0x794/0x970)
> [<c049f3b0>] (rtnl_fill_ifinfo) from [<c04a0048>] (rtnl_dump_ifinfo+0x1b4/0x2fc)
> [<c04a0048>] (rtnl_dump_ifinfo) from [<c04af9c8>] (netlink_dump+0xe4/0x270)
> [<c04af9c8>] (netlink_dump) from [<c04b0764>] (__netlink_dump_start+0xdc/0x170)
> [<c04b0764>] (__netlink_dump_start) from [<c04a1fc4>] (rtnetlink_rcv_msg+0x154/0x1e0)
> [<c04a1fc4>] (rtnetlink_rcv_msg) from [<c04b1e88>] (netlink_rcv_skb+0x68/0xc4)
> [<c04b1e88>] (netlink_rcv_skb) from [<c04a045c>] (rtnetlink_rcv+0x28/0x34)
> [<c04a045c>] (rtnetlink_rcv) from [<c04b1770>] (netlink_unicast+0x144/0x210)
> [<c04b1770>] (netlink_unicast) from [<c04b1c9c>] (netlink_sendmsg+0x394/0x414)
> [<c04b1c9c>] (netlink_sendmsg) from [<c046ffcc>] (sock_sendmsg+0x8c/0xc0)
> [<c046ffcc>] (sock_sendmsg) from [<c04726bc>] (SyS_sendto+0xd8/0x114)
> [<c04726bc>] (SyS_sendto) from [<c000f3e0>] (ret_fast_syscall+0x0/0x48)
> 
> I.e. the core calls this function from atomic context. And in an earlier
> thread you said the core can also call this before/after calling the
> open/close callbacks (which applies here too, I think).

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/13/2015 06:49 PM, S?ren Brinkmann wrote:
> On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:24 PM, S?ren Brinkmann wrote:
>>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
>>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>>>> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
>>>>>>> On Mon, 2015-01-12 at 08:34PM +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>
>>>>>>>> ---
>>>>>>>> Changes for v5:
>>>>>>>>  - Updated with the review comments.
>>>>>>>>    Updated the remove fuction to use runtime_pm.
>>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
>>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
>>>>>>> [..]
>>>>>>>> +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;
>>>>>>>> +	u32 isr, status;
>>>>>>>>  
>>>>>>>>  	ret = clk_enable(priv->bus_clk);
>>>>>>>>  	if (ret) {
>>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>>>>>>>  	ret = clk_enable(priv->can_clk);
>>>>>>>>  	if (ret) {
>>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
>>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
>>>>>>>> +		clk_disable(priv->bus_clk);
>>>>>>> [...]
>>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>>>>>>>>  {
>>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	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);
>>>>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>>>>>  	netif_napi_del(&priv->napi);
>>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
>>>>>>>> +	clk_disable_unprepare(priv->can_clk);
>>>>>>>
>>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>>>
>>>>>> We have:
>>>>>> - clk_prepare_enable() in probe
>>>>>
>>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>>>
>>>>>> - clk_disable_unprepare() in remove
>>>>>
>>>>> pm_runtime_put()
>>>>>
>>>>>> - clk_enable() in runtime_resume
>>>>>> - clk_disable() in runtime_suspend
>>>>>
>>>>> These are the ones needed.
>>>>>
>>>>> The above makes me suspect that the clocks are always on, regardless of
>>>>
>>>> Define "on" :)
>>>> The clocks are prepared after probe() exists, but not enabled. The first
>>>> pm_runtime_get_sync() will enable the clocks.
>>>>
>>>>> the runtime suspend state since they are enabled in probe and disabled
>>>>> in remove, is that right? Ideally, the usage in probe and remove should
>>>>> be migrated to runtime_pm and clocks should really only be running when
>>>>> needed and not throughout the whole lifetime of the driver.
>>>>
>>>> The clocks are not en/disabled via pm_runtime, because
>>>> pm_runtime_get_sync() is called from atomic contect. We can have another
>>>> look into the driver and try to change this.
>>
>>> Wasn't that why the call to pm_runtime_irq_safe() was added?
>>
>> Good question. That should be investigated.
>>
>>> Also, clk_enable/disable should be okay to be run from atomic context.
>>> And if the clock are already prepared after the exit of probe that
>>> should be enough. Then remove() should just have to do the unprepare.
>>> But I don't see why runtime_pm shouldn't be able to do the
>>> enable/disable.
>>
>> runtime_pm does call the clk_{enable,disable} function. But you mean
>> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
>> of calling clk_prepare_enable(). Good idea! I think the
>> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> 
> Right, that's what I was thinking. The proposed changes make sense, IMHO.
> 
>>
>> Coming back whether blocking calls are allowed or not.
>> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
>> call pm_runtime_get_sync() from atomic context. But it's only called in
>> open, probe, remove and in xcan_get_berr_counter, which is not called
>> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
>> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
>> runtime_suspend() functions.
> 
> IIRC, xcan_get_berr_counter() is called from atomic context. I think
> that was how this got started.

In some drivers the get_berr_counter() function is used in the irq
handler, but here it's only called from outside, an thus from non atomic
context.

>From an older mail of yours:

> I have the feeling I'm missing something. If I remove the 'must not
> sleep' requirement from the runtime suspend/resume functions, I get
> this:
> 
> BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954

http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954

I think it's failing because of the pm_runtime_irq_safe() call.

> in_atomic(): 0, irqs_disabled(): 0, pid: 161, name: ip
> INFO: lockdep is turned off.
> CPU: 0 PID: 161 Comm: ip Not tainted 3.18.0-rc1-xilinx-00059-g21da26693b61-dirty #104
> [<c00186a8>] (unwind_backtrace) from [<c00139f4>] (show_stack+0x20/0x24)
> [<c00139f4>] (show_stack) from [<c055a41c>] (dump_stack+0x8c/0xd0)
> [<c055a41c>] (dump_stack) from [<c0054808>] (__might_sleep+0x1ac/0x1e4)
> [<c0054808>] (__might_sleep) from [<c034f8f0>] (__pm_runtime_resume+0x40/0x9c)
> [<c034f8f0>] (__pm_runtime_resume) from [<c03b48d8>] (xcan_get_berr_counter+0x2c/0x9c)
> [<c03b48d8>] (xcan_get_berr_counter) from [<c03b2ecc>] (can_fill_info+0x160/0x1f4)
> [<c03b2ecc>] (can_fill_info) from [<c049f3b0>] (rtnl_fill_ifinfo+0x794/0x970)
> [<c049f3b0>] (rtnl_fill_ifinfo) from [<c04a0048>] (rtnl_dump_ifinfo+0x1b4/0x2fc)
> [<c04a0048>] (rtnl_dump_ifinfo) from [<c04af9c8>] (netlink_dump+0xe4/0x270)
> [<c04af9c8>] (netlink_dump) from [<c04b0764>] (__netlink_dump_start+0xdc/0x170)
> [<c04b0764>] (__netlink_dump_start) from [<c04a1fc4>] (rtnetlink_rcv_msg+0x154/0x1e0)
> [<c04a1fc4>] (rtnetlink_rcv_msg) from [<c04b1e88>] (netlink_rcv_skb+0x68/0xc4)
> [<c04b1e88>] (netlink_rcv_skb) from [<c04a045c>] (rtnetlink_rcv+0x28/0x34)
> [<c04a045c>] (rtnetlink_rcv) from [<c04b1770>] (netlink_unicast+0x144/0x210)
> [<c04b1770>] (netlink_unicast) from [<c04b1c9c>] (netlink_sendmsg+0x394/0x414)
> [<c04b1c9c>] (netlink_sendmsg) from [<c046ffcc>] (sock_sendmsg+0x8c/0xc0)
> [<c046ffcc>] (sock_sendmsg) from [<c04726bc>] (SyS_sendto+0xd8/0x114)
> [<c04726bc>] (SyS_sendto) from [<c000f3e0>] (ret_fast_syscall+0x0/0x48)
> 
> I.e. the core calls this function from atomic context. And in an earlier
> thread you said the core can also call this before/after calling the
> open/close callbacks (which applies here too, I think).

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   |

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

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>>>> On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>>> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>>> ---
> >>>>>>>> Changes for v5:
> >>>>>>>>  - Updated with the review comments.
> >>>>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>>>> [..]
> >>>>>>>> +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;
> >>>>>>>> +	u32 isr, status;
> >>>>>>>>  
> >>>>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +		clk_disable(priv->bus_clk);
> >>>>>>> [...]
> >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>>>  {
> >>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	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);
> >>>>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>>>  	netif_napi_del(&priv->napi);
> >>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>>>
> >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>>>
> >>>>>> We have:
> >>>>>> - clk_prepare_enable() in probe
> >>>>>
> >>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>>>
> >>>>>> - clk_disable_unprepare() in remove
> >>>>>
> >>>>> pm_runtime_put()
> >>>>>
> >>>>>> - clk_enable() in runtime_resume
> >>>>>> - clk_disable() in runtime_suspend
> >>>>>
> >>>>> These are the ones needed.
> >>>>>
> >>>>> The above makes me suspect that the clocks are always on, regardless of
> >>>>
> >>>> Define "on" :)
> >>>> The clocks are prepared after probe() exists, but not enabled. The first
> >>>> pm_runtime_get_sync() will enable the clocks.
> >>>>
> >>>>> the runtime suspend state since they are enabled in probe and disabled
> >>>>> in remove, is that right? Ideally, the usage in probe and remove should
> >>>>> be migrated to runtime_pm and clocks should really only be running when
> >>>>> needed and not throughout the whole lifetime of the driver.
> >>>>
> >>>> The clocks are not en/disabled via pm_runtime, because
> >>>> pm_runtime_get_sync() is called from atomic contect. We can have another
> >>>> look into the driver and try to change this.
> >>
> >>> Wasn't that why the call to pm_runtime_irq_safe() was added?
> >>
> >> Good question. That should be investigated.
> >>
> >>> Also, clk_enable/disable should be okay to be run from atomic context.
> >>> And if the clock are already prepared after the exit of probe that
> >>> should be enough. Then remove() should just have to do the unprepare.
> >>> But I don't see why runtime_pm shouldn't be able to do the
> >>> enable/disable.
> >>
> >> runtime_pm does call the clk_{enable,disable} function. But you mean
> >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> >> of calling clk_prepare_enable(). Good idea! I think the
> >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> > 
> > Right, that's what I was thinking. The proposed changes make sense, IMHO.
> > 
> >>
> >> Coming back whether blocking calls are allowed or not.
> >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> >> call pm_runtime_get_sync() from atomic context. But it's only called in
> >> open, probe, remove and in xcan_get_berr_counter, which is not called
> >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> >> runtime_suspend() functions.
> > 
> > IIRC, xcan_get_berr_counter() is called from atomic context. I think
> > that was how this got started.
> 
> In some drivers the get_berr_counter() function is used in the irq
> handler, but here it's only called from outside, an thus from non atomic
> context.
> 
> From an older mail of yours:
> 
> > I have the feeling I'm missing something. If I remove the 'must not
> > sleep' requirement from the runtime suspend/resume functions, I get
> > this:
> > 
> > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954
> 
> http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954
> 
> I think it's failing because of the pm_runtime_irq_safe() call.

Adding that call fixed this issue.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>>>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>>>>>> ---
> >>>>>>>> Changes for v5:
> >>>>>>>>  - Updated with the review comments.
> >>>>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>>>> [..]
> >>>>>>>> +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;
> >>>>>>>> +	u32 isr, status;
> >>>>>>>>  
> >>>>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +		clk_disable(priv->bus_clk);
> >>>>>>> [...]
> >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>>>  {
> >>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	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);
> >>>>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>>>  	netif_napi_del(&priv->napi);
> >>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>>>
> >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>>>
> >>>>>> We have:
> >>>>>> - clk_prepare_enable() in probe
> >>>>>
> >>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>>>
> >>>>>> - clk_disable_unprepare() in remove
> >>>>>
> >>>>> pm_runtime_put()
> >>>>>
> >>>>>> - clk_enable() in runtime_resume
> >>>>>> - clk_disable() in runtime_suspend
> >>>>>
> >>>>> These are the ones needed.
> >>>>>
> >>>>> The above makes me suspect that the clocks are always on, regardless of
> >>>>
> >>>> Define "on" :)
> >>>> The clocks are prepared after probe() exists, but not enabled. The first
> >>>> pm_runtime_get_sync() will enable the clocks.
> >>>>
> >>>>> the runtime suspend state since they are enabled in probe and disabled
> >>>>> in remove, is that right? Ideally, the usage in probe and remove should
> >>>>> be migrated to runtime_pm and clocks should really only be running when
> >>>>> needed and not throughout the whole lifetime of the driver.
> >>>>
> >>>> The clocks are not en/disabled via pm_runtime, because
> >>>> pm_runtime_get_sync() is called from atomic contect. We can have another
> >>>> look into the driver and try to change this.
> >>
> >>> Wasn't that why the call to pm_runtime_irq_safe() was added?
> >>
> >> Good question. That should be investigated.
> >>
> >>> Also, clk_enable/disable should be okay to be run from atomic context.
> >>> And if the clock are already prepared after the exit of probe that
> >>> should be enough. Then remove() should just have to do the unprepare.
> >>> But I don't see why runtime_pm shouldn't be able to do the
> >>> enable/disable.
> >>
> >> runtime_pm does call the clk_{enable,disable} function. But you mean
> >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> >> of calling clk_prepare_enable(). Good idea! I think the
> >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> > 
> > Right, that's what I was thinking. The proposed changes make sense, IMHO.
> > 
> >>
> >> Coming back whether blocking calls are allowed or not.
> >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> >> call pm_runtime_get_sync() from atomic context. But it's only called in
> >> open, probe, remove and in xcan_get_berr_counter, which is not called
> >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> >> runtime_suspend() functions.
> > 
> > IIRC, xcan_get_berr_counter() is called from atomic context. I think
> > that was how this got started.
> 
> In some drivers the get_berr_counter() function is used in the irq
> handler, but here it's only called from outside, an thus from non atomic
> context.
> 
> From an older mail of yours:
> 
> > I have the feeling I'm missing something. If I remove the 'must not
> > sleep' requirement from the runtime suspend/resume functions, I get
> > this:
> > 
> > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954
> 
> http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954
> 
> I think it's failing because of the pm_runtime_irq_safe() call.

Adding that call fixed this issue.

	Sören

Re: [PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> >>>>>>> On Mon, 2015-01-12 at 08:34PM +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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>>> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>>> ---
> >>>>>>>> Changes for v5:
> >>>>>>>>  - Updated with the review comments.
> >>>>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>>>> [..]
> >>>>>>>> +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;
> >>>>>>>> +	u32 isr, status;
> >>>>>>>>  
> >>>>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +		clk_disable(priv->bus_clk);
> >>>>>>> [...]
> >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>>>  {
> >>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	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);
> >>>>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>>>  	netif_napi_del(&priv->napi);
> >>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>>>
> >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>>>
> >>>>>> We have:
> >>>>>> - clk_prepare_enable() in probe
> >>>>>
> >>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>>>
> >>>>>> - clk_disable_unprepare() in remove
> >>>>>
> >>>>> pm_runtime_put()
> >>>>>
> >>>>>> - clk_enable() in runtime_resume
> >>>>>> - clk_disable() in runtime_suspend
> >>>>>
> >>>>> These are the ones needed.
> >>>>>
> >>>>> The above makes me suspect that the clocks are always on, regardless of
> >>>>
> >>>> Define "on" :)
> >>>> The clocks are prepared after probe() exists, but not enabled. The first
> >>>> pm_runtime_get_sync() will enable the clocks.
> >>>>
> >>>>> the runtime suspend state since they are enabled in probe and disabled
> >>>>> in remove, is that right? Ideally, the usage in probe and remove should
> >>>>> be migrated to runtime_pm and clocks should really only be running when
> >>>>> needed and not throughout the whole lifetime of the driver.
> >>>>
> >>>> The clocks are not en/disabled via pm_runtime, because
> >>>> pm_runtime_get_sync() is called from atomic contect. We can have another
> >>>> look into the driver and try to change this.
> >>
> >>> Wasn't that why the call to pm_runtime_irq_safe() was added?
> >>
> >> Good question. That should be investigated.
> >>
> >>> Also, clk_enable/disable should be okay to be run from atomic context.
> >>> And if the clock are already prepared after the exit of probe that
> >>> should be enough. Then remove() should just have to do the unprepare.
> >>> But I don't see why runtime_pm shouldn't be able to do the
> >>> enable/disable.
> >>
> >> runtime_pm does call the clk_{enable,disable} function. But you mean
> >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> >> of calling clk_prepare_enable(). Good idea! I think the
> >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> > 
> > Right, that's what I was thinking. The proposed changes make sense, IMHO.
> > 
> >>
> >> Coming back whether blocking calls are allowed or not.
> >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> >> call pm_runtime_get_sync() from atomic context. But it's only called in
> >> open, probe, remove and in xcan_get_berr_counter, which is not called
> >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> >> runtime_suspend() functions.
> > 
> > IIRC, xcan_get_berr_counter() is called from atomic context. I think
> > that was how this got started.
> 
> In some drivers the get_berr_counter() function is used in the irq
> handler, but here it's only called from outside, an thus from non atomic
> context.
> 
> From an older mail of yours:
> 
> > I have the feeling I'm missing something. If I remove the 'must not
> > sleep' requirement from the runtime suspend/resume functions, I get
> > this:
> > 
> > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954
> 
> http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954
> 
> I think it's failing because of the pm_runtime_irq_safe() call.

Adding that call fixed this issue.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] can: Convert to runtime_pm

[Sören Brinkmann]

On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:49 PM, S?ren Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:24 PM, S?ren Brinkmann wrote:
> >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/13/2015 06:08 PM, S?ren Brinkmann wrote:
> >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >>>>>> On 01/12/2015 07:45 PM, S?ren Brinkmann wrote:
> >>>>>>> On Mon, 2015-01-12 at 08:34PM +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>
> >>>>>>>> ---
> >>>>>>>> Changes for v5:
> >>>>>>>>  - Updated with the review comments.
> >>>>>>>>    Updated the remove fuction to use runtime_pm.
> >>>>>>>> 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 |  157 ++++++++++++++++++++++++++++-------------
> >>>>>>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>>>>>> [..]
> >>>>>>>> +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;
> >>>>>>>> +	u32 isr, status;
> >>>>>>>>  
> >>>>>>>>  	ret = clk_enable(priv->bus_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>>>>>  	ret = clk_enable(priv->can_clk);
> >>>>>>>>  	if (ret) {
> >>>>>>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>>>>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +		clk_disable(priv->bus_clk);
> >>>>>>> [...]
> >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>>>>>  {
> >>>>>>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>>>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	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);
> >>>>>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>>>>>  	netif_napi_del(&priv->napi);
> >>>>>>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>>>>>> +	clk_disable_unprepare(priv->can_clk);
> >>>>>>>
> >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>>>>>> disappear? This should all be handled by the runtime_pm framework now.
> >>>>>>
> >>>>>> We have:
> >>>>>> - clk_prepare_enable() in probe
> >>>>>
> >>>>> This should become something like pm_runtime_get_sync(), shouldn't it?
> >>>>>
> >>>>>> - clk_disable_unprepare() in remove
> >>>>>
> >>>>> pm_runtime_put()
> >>>>>
> >>>>>> - clk_enable() in runtime_resume
> >>>>>> - clk_disable() in runtime_suspend
> >>>>>
> >>>>> These are the ones needed.
> >>>>>
> >>>>> The above makes me suspect that the clocks are always on, regardless of
> >>>>
> >>>> Define "on" :)
> >>>> The clocks are prepared after probe() exists, but not enabled. The first
> >>>> pm_runtime_get_sync() will enable the clocks.
> >>>>
> >>>>> the runtime suspend state since they are enabled in probe and disabled
> >>>>> in remove, is that right? Ideally, the usage in probe and remove should
> >>>>> be migrated to runtime_pm and clocks should really only be running when
> >>>>> needed and not throughout the whole lifetime of the driver.
> >>>>
> >>>> The clocks are not en/disabled via pm_runtime, because
> >>>> pm_runtime_get_sync() is called from atomic contect. We can have another
> >>>> look into the driver and try to change this.
> >>
> >>> Wasn't that why the call to pm_runtime_irq_safe() was added?
> >>
> >> Good question. That should be investigated.
> >>
> >>> Also, clk_enable/disable should be okay to be run from atomic context.
> >>> And if the clock are already prepared after the exit of probe that
> >>> should be enough. Then remove() should just have to do the unprepare.
> >>> But I don't see why runtime_pm shouldn't be able to do the
> >>> enable/disable.
> >>
> >> runtime_pm does call the clk_{enable,disable} function. But you mean
> >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
> >> of calling clk_prepare_enable(). Good idea! I think the
> >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
> > 
> > Right, that's what I was thinking. The proposed changes make sense, IMHO.
> > 
> >>
> >> Coming back whether blocking calls are allowed or not.
> >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to
> >> call pm_runtime_get_sync() from atomic context. But it's only called in
> >> open, probe, remove and in xcan_get_berr_counter, which is not called
> >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and
> >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume()
> >> runtime_suspend() functions.
> > 
> > IIRC, xcan_get_berr_counter() is called from atomic context. I think
> > that was how this got started.
> 
> In some drivers the get_berr_counter() function is used in the irq
> handler, but here it's only called from outside, an thus from non atomic
> context.
> 
> From an older mail of yours:
> 
> > I have the feeling I'm missing something. If I remove the 'must not
> > sleep' requirement from the runtime suspend/resume functions, I get
> > this:
> > 
> > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954
> 
> http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954
> 
> I think it's failing because of the pm_runtime_irq_safe() call.

Adding that call fixed this issue.

	S?ren

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1088 bytes --]

On 01/12/2015 04:04 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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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.

Any plans to submit a v6?

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

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

On 01/12/2015 04:04 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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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.

Any plans to submit a v6?

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

[PATCH v5] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/12/2015 04:04 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>
> ---
> Changes for v5:
>  - Updated with the review comments.
>    Updated the remove fuction to use runtime_pm.
> 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.

Any plans to submit a v6?

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   |

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

RE: [PATCH v5] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, January 28, 2015 8:16 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> Soren Brinkmann; grant.likely@linaro.org; robh+dt@kernel.org
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Appana Durga Kedareswara Rao
> Subject: Re: [PATCH v5] can: Convert to runtime_pm
>
> On 01/12/2015 04:04 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>
> > ---
> > Changes for v5:
> >  - Updated with the review comments.
> >    Updated the remove fuction to use runtime_pm.
> > 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.
>
> Any plans to submit a v6?

I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest.

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 v5] 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: 2289 bytes --]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, January 28, 2015 8:16 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> Soren Brinkmann; grant.likely@linaro.org; robh+dt@kernel.org
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Appana Durga Kedareswara Rao
> Subject: Re: [PATCH v5] can: Convert to runtime_pm
>
> On 01/12/2015 04:04 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>
> > ---
> > Changes for v5:
> >  - Updated with the review comments.
> >    Updated the remove fuction to use runtime_pm.
> > 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.
>
> Any plans to submit a v6?

I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest.

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¥

[PATCH v5] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
> Sent: Wednesday, January 28, 2015 8:16 PM
> To: Appana Durga Kedareswara Rao; wg at grandegger.com; Michal Simek;
> Soren Brinkmann; grant.likely at linaro.org; robh+dt at kernel.org
> Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; Appana Durga Kedareswara Rao
> Subject: Re: [PATCH v5] can: Convert to runtime_pm
>
> On 01/12/2015 04:04 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>
> > ---
> > Changes for v5:
> >  - Updated with the review comments.
> >    Updated the remove fuction to use runtime_pm.
> > 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.
>
> Any plans to submit a v6?

I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest.

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.