On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:

Hi Marc,

quoted

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Sunday, January 11, 2015 9:11 PM
To: Appana Durga Kedareswara Rao
Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
wg@grandegger.com
Subject: Re: [PATCH v4] can: Convert to runtime_pm

On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
[...]

quoted

quoted

quoted

            return ret;
    }

    priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
    priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);

    if (netif_running(ndev)) {
            priv->can.state = CAN_STATE_ERROR_ACTIVE;

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

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

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

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

Is the above code snippet ok for you?

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

quoted

quoted

quoted

quoted

            netif_device_attach(ndev);
            netif_start_queue(ndev);
    }

    return 0;
}

quoted

@@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct

device *dev)

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

    if (netif_running(ndev)) {
+           priv->can.state = CAN_STATE_ERROR_ACTIVE;
            netif_device_attach(ndev);
            netif_start_queue(ndev);
    }

@@ -1030,7 +1045,10 @@ static int __maybe_unused

xcan_resume(struct

quoted

quoted

device *dev)

quoted

    return 0;
 }

-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,

xcan_resume);

quoted

+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,

quoted

+NULL) };

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

    priv = netdev_priv(ndev);
-   priv->dev = ndev;
+   priv->dev = &pdev->dev;
    priv->can.bittiming_const = &xcan_bittiming_const;
    priv->can.do_set_mode = xcan_do_set_mode;
    priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -

1137,15

quoted

+1155,22 @@ static int xcan_probe(struct platform_device *pdev)

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

+   pm_runtime_set_active(&pdev->dev);
+   pm_runtime_irq_safe(&pdev->dev);
+   pm_runtime_enable(&pdev->dev);
+   pm_runtime_get_sync(&pdev->dev);

Check error values?

Ok

quoted

quoted

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

ret);

quoted

+           pm_runtime_put(priv->dev);

Please move the pm_runtime_put into the common error exit path.

Ok

quoted

quoted

            goto err_unprepare_disable_busclk;
    }

    devm_can_led_init(ndev);
-   clk_disable_unprepare(priv->bus_clk);
-   clk_disable_unprepare(priv->can_clk);
+
+   pm_runtime_put(&pdev->dev);
+
    netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo

depth:%d\n",

quoted

                    priv->reg_base, ndev->irq, priv->can.clock.freq,
                    priv->tx_max);

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

quoted

static int zynq_gpio_remove(struct platform_device *pdev) {
    struct zynq_gpio *gpio = platform_get_drvdata(pdev);

    pm_runtime_get_sync(&pdev->dev);

However I don't understand why the get_sync() is here. Maybe Sören
can help?

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

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

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

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

quoted

        pm_runtime_disable(&pdev->dev);

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

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

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

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

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