LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v5] can: Convert to runtime_pm
@ 2015-01-12 15:04 Kedareswara rao Appana
  2015-01-12 18:45 ` Sören Brinkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Kedareswara rao Appana @ 2015-01-12 15:04 UTC (permalink / raw)
  To: wg, mkl, michal.simek, soren.brinkmann, grant.likely, robh+dt
  Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-12 15:04 [PATCH v5] can: Convert to runtime_pm Kedareswara rao Appana
@ 2015-01-12 18:45 ` Sören Brinkmann
  2015-01-13 11:08   ` Marc Kleine-Budde
  2015-01-12 19:52 ` Marc Kleine-Budde
  2015-01-28 14:46 ` Marc Kleine-Budde
  2 siblings, 1 reply; 13+ messages in thread
From: Sören Brinkmann @ 2015-01-12 18:45 UTC (permalink / raw)
  To: Kedareswara rao Appana
  Cc: wg, mkl, michal.simek, grant.likely, robh+dt, linux-can, netdev,
	linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-12 15:04 [PATCH v5] can: Convert to runtime_pm Kedareswara rao Appana
  2015-01-12 18:45 ` Sören Brinkmann
@ 2015-01-12 19:52 ` Marc Kleine-Budde
  2015-01-28 14:46 ` Marc Kleine-Budde
  2 siblings, 0 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-12 19:52 UTC (permalink / raw)
  To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
	grant.likely, robh+dt
  Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-12 18:45 ` Sören Brinkmann
@ 2015-01-13 11:08   ` Marc Kleine-Budde
  2015-01-13 17:08     ` Sören Brinkmann
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-13 11:08 UTC (permalink / raw)
  To: Sören Brinkmann, Kedareswara rao Appana
  Cc: wg, michal.simek, grant.likely, robh+dt, linux-can, netdev,
	linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 11:08   ` Marc Kleine-Budde
@ 2015-01-13 17:08     ` Sören Brinkmann
  2015-01-13 17:17       ` Marc Kleine-Budde
  0 siblings, 1 reply; 13+ messages in thread
From: Sören Brinkmann @ 2015-01-13 17:08 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 17:08     ` Sören Brinkmann
@ 2015-01-13 17:17       ` Marc Kleine-Budde
  2015-01-13 17:24         ` Sören Brinkmann
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-13 17:17 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 17:17       ` Marc Kleine-Budde
@ 2015-01-13 17:24         ` Sören Brinkmann
  2015-01-13 17:44           ` Marc Kleine-Budde
  0 siblings, 1 reply; 13+ messages in thread
From: Sören Brinkmann @ 2015-01-13 17:24 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 17:24         ` Sören Brinkmann
@ 2015-01-13 17:44           ` Marc Kleine-Budde
  2015-01-13 17:49             ` Sören Brinkmann
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-13 17:44 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 17:44           ` Marc Kleine-Budde
@ 2015-01-13 17:49             ` Sören Brinkmann
  2015-01-13 18:03               ` Marc Kleine-Budde
  0 siblings, 1 reply; 13+ messages in thread
From: Sören Brinkmann @ 2015-01-13 17:49 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 17:49             ` Sören Brinkmann
@ 2015-01-13 18:03               ` Marc Kleine-Budde
  2015-01-13 18:43                 ` Sören Brinkmann
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-13 18:03 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-13 18:03               ` Marc Kleine-Budde
@ 2015-01-13 18:43                 ` Sören Brinkmann
  0 siblings, 0 replies; 13+ messages in thread
From: Sören Brinkmann @ 2015-01-13 18:43 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
	linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] can: Convert to runtime_pm
  2015-01-12 15:04 [PATCH v5] can: Convert to runtime_pm Kedareswara rao Appana
  2015-01-12 18:45 ` Sören Brinkmann
  2015-01-12 19:52 ` Marc Kleine-Budde
@ 2015-01-28 14:46 ` Marc Kleine-Budde
  2015-01-28 15:19   ` Appana Durga Kedareswara Rao
  2 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2015-01-28 14:46 UTC (permalink / raw)
  To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
	grant.likely, robh+dt
  Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
	Kedareswara rao Appana

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v5] can: Convert to runtime_pm
  2015-01-28 14:46 ` Marc Kleine-Budde
@ 2015-01-28 15:19   ` Appana Durga Kedareswara Rao
  0 siblings, 0 replies; 13+ messages in thread
From: Appana Durga Kedareswara Rao @ 2015-01-28 15:19 UTC (permalink / raw)
  To: Marc Kleine-Budde, wg, Michal Simek, Soren Brinkmann,
	grant.likely, robh+dt
  Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree

[-- 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¥

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-01-28 20:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-12 15:04 [PATCH v5] can: Convert to runtime_pm Kedareswara rao Appana
2015-01-12 18:45 ` Sören Brinkmann
2015-01-13 11:08   ` Marc Kleine-Budde
2015-01-13 17:08     ` Sören Brinkmann
2015-01-13 17:17       ` Marc Kleine-Budde
2015-01-13 17:24         ` Sören Brinkmann
2015-01-13 17:44           ` Marc Kleine-Budde
2015-01-13 17:49             ` Sören Brinkmann
2015-01-13 18:03               ` Marc Kleine-Budde
2015-01-13 18:43                 ` Sören Brinkmann
2015-01-12 19:52 ` Marc Kleine-Budde
2015-01-28 14:46 ` Marc Kleine-Budde
2015-01-28 15:19   ` Appana Durga Kedareswara Rao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).