RE: [PATCH v4] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Marc,

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

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