RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx]
> Sent: Sunday, January 11, 2015 9:11 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Soren Brinkmann; grant.likely@xxxxxxxxxx;
> wg@xxxxxxxxxxxxxx
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> [...]
> >>>             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?

> >>>             netif_device_attach(ndev);
> >>>             netif_start_queue(ndev);
> >>>     }
> >>>
> >>>     return 0;
> >>> }
> >>
> >>
> >>>
> >>> @@ -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
> >> 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 +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
> >>> +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
> >>> +
> >>>     ret = register_candev(ndev);
> >>>     if (ret) {
> >>>             dev_err(&pdev->dev, "fail to register failed
> >>> (err=%d)\n",
> >> ret);
> >>> +           pm_runtime_put(priv->dev);
> >>
> >> Please move the pm_runtime_put into the common error exit path.
> >>
> >
> > Ok
> >
> >>>             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",
> >>>                     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:
> >>
> >>> 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);
>
> >         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.

Regards,
Kedar.

>
> > Observed the below things in the /sys/kernel/debug/clk/clk_summary.
> >
> >                                 Modprobe        ifconfig up    ifconfig down   rmmod
> Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up
> ifconfig down  rmmod
> > Clk_prepare count:                1             1               1               1       2       2               2
> 2       3       3               3               3
> > Clk_enable count:                 0             1               0               1       2       2               2
> 2       3       3               3               3
>
> As the numbers are increasing you have a clk_prepare_enable() leak. Your
> remove() function is missing the clk_disable_unprepare() for the can and
> bus clock (as you have clk_prepare_enable in probe()).
>
> 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.