LinuxLists.cc - [PATCH v3] can: Convert to runtime

2014-11-27 13:24:23

Subject: [PATCH v3] can: Convert to runtime_pm

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

Signed-off-by: Soren Brinkmann <[email protected]>
Signed-off-by: Kedareswara rao Appana <[email protected]>
---
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 | 119 +++++++++++++++++++++++++----------------
1 files changed, 72 insertions(+), 47 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 8a998e3..1be28ed 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: runtime CAN resume failed(%d)\n\r",
+ __func__, ret);
+ return ret;
+ }
+
ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
ndev->name, ndev);
if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
goto err;
}

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

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

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

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

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

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

return 0;
}
@@ -934,27 +927,21 @@ 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 = pm_runtime_get_sync(priv->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
+ __func__, ret);
+ return ret;
+ }

- ret = clk_prepare_enable(priv->bus_clk);
- if (ret)
- goto err_clk;

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 +954,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 net_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 net_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 net_device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev)
+{
+ struct net_device *ndev = dev_get_drvdata(dev);
struct xcan_priv *priv = netdev_priv(ndev);

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

/**
- * xcan_resume - Resume from suspend
- * @dev: Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev: Address of the net_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;

@@ -1030,7 +1046,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 +1090,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 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)

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

+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_irq_safe(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_sync(&pdev->dev);
+
ret = register_candev(ndev);
if (ret) {
dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,8 +1168,9 @@ 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);
--
1.7.4

2014-11-27 15:21:09

by Michal Simek

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

2014-11-27 18:23:54

by Soren Brinkmann

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

Hi Kedar,

On Thu, 2014-11-27 at 06:38PM +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 <[email protected]>
> Signed-off-by: Kedareswara rao Appana <[email protected]>
> ---
> 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 | 119 +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 8a998e3..1be28ed 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: runtime CAN resume failed(%d)\n\r",

There might be other issues than the resume that make this fail. It
should probably just say 'pm_runtime_get failed'.
The CAN in the string should not be needed, the netdev_err macro makes
sure the device name is printed.
Can we have a space between 'failed' and the error code?
There should not be a '\r'

> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ 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 = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);

As above.

Sören

2014-11-27 21:18:11

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

On 11/27/2014 02:08 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 <[email protected]>
> Signed-off-by: Kedareswara rao Appana <[email protected]>
> ---
> 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 | 119 +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 8a998e3..1be28ed 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: runtime CAN resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ 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 = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
>
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> - goto err_clk;
>
> 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 +954,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 net_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 net_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 net_device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int __maybe_unused xcan_runtime_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> struct xcan_priv *priv = netdev_priv(ndev);
>
> if (netif_running(ndev)) {
> @@ -993,16 +1010,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
> }
>
> /**
> - * xcan_resume - Resume from suspend
> - * @dev: Address of the platformdevice structure
> + * xcan_runtime_resume - Runtime resume from suspend
> + * @dev: Address of the net_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;

...adding the rest of the function to comment it:

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

Does it have any side effects, when writing these values when the CAN
device is not UP? (e.g. when running the berr callback?) Setting its
state to ERROR active is probably wrong. Do you have to move it into the
if(netif_running())?

> if (netif_running(ndev)) {
> netif_device_attach(ndev);
> netif_start_queue(ndev);
> }
>
> return 0;
> }

>
> @@ -1030,7 +1046,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 +1090,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 +1156,11 @@ 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);

You use just clock_enable()/disable() in the runtime functions, thus you
can say they are irq_safe. On the other the the zync grpio driver uses
"full" prepare_enable/disable_unprepare calls. What's best practice here?

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

I think in case of an error, you have to call pm_runtime_put() here

> @@ -1144,8 +1168,9 @@ 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);
>

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 |

Attachments:

signature.asc (819.00 B)
OpenPGP digital signature

2014-11-27 22:47:51

by Soren Brinkmann

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
> On 11/27/2014 02:08 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 <[email protected]>
> > Signed-off-by: Kedareswara rao Appana <[email protected]>
> > ---
> > 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 | 119 +++++++++++++++++++++++++----------------
> > 1 files changed, 72 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> > index 8a998e3..1be28ed 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
[...]
> > @@ -1030,7 +1046,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 +1090,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 +1156,11 @@ 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);
>
> You use just clock_enable()/disable() in the runtime functions, thus you
> can say they are irq_safe. On the other the the zync grpio driver uses
> "full" prepare_enable/disable_unprepare calls. What's best practice here?

IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
is called from atomic context. So, I think we have to use the
disable/enable functions without the prepare/unprepare.
In the GPIO driver the that problem does not exist.

Sören

2014-11-27 22:56:03

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

On 11/27/2014 11:47 PM, Sören Brinkmann wrote:
> On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
>> On 11/27/2014 02:08 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 <[email protected]>
>>> Signed-off-by: Kedareswara rao Appana <[email protected]>
>>> ---
>>> 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 | 119 +++++++++++++++++++++++++----------------
>>> 1 files changed, 72 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>>> index 8a998e3..1be28ed 100644
>>> --- a/drivers/net/can/xilinx_can.c
>>> +++ b/drivers/net/can/xilinx_can.c
> [...]
>>> @@ -1030,7 +1046,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 +1090,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 +1156,11 @@ 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);
>>
>> You use just clock_enable()/disable() in the runtime functions, thus you
>> can say they are irq_safe. On the other the the zync grpio driver uses
>> "full" prepare_enable/disable_unprepare calls. What's best practice here?
>
> IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
> is called from atomic context. So, I think we have to use the
> disable/enable functions without the prepare/unprepare.
> In the GPIO driver the that problem does not exist.

IC, yes, correct.

This is why we introducted in other drivers a __get_berr_counter()
function, that doesn't touch the clocks, which is used from within the
driver (from the atomic contects), while get_berr_counter() will fiddle
with the clocks. This function is used for the
priv->can.do_get_berr_counter callback.

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 |

Attachments:

signature.asc (819.00 B)
OpenPGP digital signature

2014-11-27 23:46:15

by Soren Brinkmann

[permalink] [raw]

Subject: Re: [PATCH v3] can: Convert to runtime_pm

On Thu, 2014-11-27 at 11:55PM +0100, Marc Kleine-Budde wrote:
> On 11/27/2014 11:47 PM, Sören Brinkmann wrote:
> > On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
> >> On 11/27/2014 02:08 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 <[email protected]>
> >>> Signed-off-by: Kedareswara rao Appana <[email protected]>
> >>> ---
> >>> 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 | 119 +++++++++++++++++++++++++----------------
> >>> 1 files changed, 72 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> >>> index 8a998e3..1be28ed 100644
> >>> --- a/drivers/net/can/xilinx_can.c
> >>> +++ b/drivers/net/can/xilinx_can.c
> > [...]
> >>> @@ -1030,7 +1046,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 +1090,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 +1156,11 @@ 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);
> >>
> >> You use just clock_enable()/disable() in the runtime functions, thus you
> >> can say they are irq_safe. On the other the the zync grpio driver uses
> >> "full" prepare_enable/disable_unprepare calls. What's best practice here?
> >
> > IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
> > is called from atomic context. So, I think we have to use the
> > disable/enable functions without the prepare/unprepare.
> > In the GPIO driver the that problem does not exist.
>
> IC, yes, correct.
>
> This is why we introducted in other drivers a __get_berr_counter()
> function, that doesn't touch the clocks, which is used from within the
> driver (from the atomic contects), while get_berr_counter() will fiddle
> with the clocks. This function is used for the
> priv->can.do_get_berr_counter callback.

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
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).

I think the callback is required to
- not sleep
- get the device in a power state that allows querying its registers

So, I don't see how splitting the xcan_get_berr_counter callback helps
here, especially since it is not even used within the driver.

Thanks,
Sören

2014-11-28 01:47:51

by Appana Durga Kedareswara rao

[permalink] [raw]

Subject: RE: [PATCH v3] can: Convert to runtime_pm

Hi Michal,

-----Original Message-----
From: Michal Simek [mailto:[email protected]]
Sent: Thursday, November 27, 2014 8:17 PM
To: Appana Durga Kedareswara Rao; [email protected]; [email protected]; Michal Simek; Soren Brinkmann; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v3] can: Convert to runtime_pm

On 11/27/2014 02:08 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 <[email protected]>
> Signed-off-by: Kedareswara rao Appana <[email protected]>
> ---
> 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 | 119
> +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 8a998e3..1be28ed 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: runtime CAN resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ 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 = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
>
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> - goto err_clk;
>
> 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 +954,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 net_device structure

This is just the device structure.

K will modify in the next version of the patch .

> *
> * 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 net_device structure

ditto

K will modify in the next version of the patch .

> + *
> + * 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 net_device structure

ditto.
K will modify in the next version of the patch .
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int __maybe_unused xcan_runtime_suspend(struct device *dev) {
> + struct net_device *ndev = dev_get_drvdata(dev);
> struct xcan_priv *priv = netdev_priv(ndev);
>
> if (netif_running(ndev)) {
> @@ -993,16 +1010,15 @@ static int __maybe_unused xcan_suspend(struct
> device *dev) }
>
> /**
> - * xcan_resume - Resume from suspend
> - * @dev: Address of the platformdevice structure
> + * xcan_runtime_resume - Runtime resume from suspend
> + * @dev: Address of the net_device structure

ditto.
K will modify in the next version of the patch .

Regards,
Kedar.
Thanks,
Michal

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.

2014-11-28 01:50:44

by Appana Durga Kedareswara rao

[permalink] [raw]

Subject: RE: [PATCH v3] can: Convert to runtime_pm

Hi Soren,

-----Original Message-----
From: Sören Brinkmann [mailto:[email protected]]
Sent: Thursday, November 27, 2014 11:54 PM
To: Appana Durga Kedareswara Rao
Cc: [email protected]; [email protected]; Michal Simek; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Appana Durga Kedareswara Rao; [email protected]
Subject: Re: [PATCH v3] can: Convert to runtime_pm

Hi Kedar,

On Thu, 2014-11-27 at 06:38PM +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 <[email protected]>
> Signed-off-by: Kedareswara rao Appana <[email protected]>
> ---
> 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 | 119
> +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 8a998e3..1be28ed 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: runtime CAN resume failed(%d)\n\r",

There might be other issues than the resume that make this fail. It should probably just say 'pm_runtime_get failed'.
The CAN in the string should not be needed, the netdev_err macro makes sure the device name is printed.
Can we have a space between 'failed' and the error code?
There should not be a '\r'

K sure will modify the error message as you explained above.

> + __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,21 @@ 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 = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);

As above.

K will modify this too.

Regards,
Kedar.
Sören

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

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?