Hi Marc,

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

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

quoted

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?

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

quoted

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

quoted

quoted

quoted

quoted

quoted

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.

quoted

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

If I move the pm_runtime_disable after unregister_candev everything is

working fine.

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

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

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

Regards,
Kedar.

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

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