RE: [PATCH v4] can: Convert to runtime_pm

From: Appana Durga Kedareswara Rao
Date: Mon Jan 12 2015 - 10:04:22 EST

Next message: Kedareswara rao Appana: "[PATCH v5] can: Convert to runtime_pm"
Previous message: Hans Verkuil: "Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder"
In reply to: Marc Kleine-Budde: "Re: [PATCH v4] can: Convert to runtime_pm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx]
> Sent: Monday, January 12, 2015 7:23 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 02:49 PM, Appana Durga Kedareswara Rao wrote:
> > 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.
>
> I understand the software side, but I don't know how your hardware
> behaves. This is why I'm asking.

As far as I know the above is the our can controller behavior. Anyway I will check with the h/w team
And will get back to you regarding this. Meanwhile I am sending the next version of the patch
Please review it.

Regards,
Kedar.
>
> >
> >>
> >>>
> >>>>>>> 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.
>
> Put a dump_stack into xcan_get_berr_counter(), then you'll see where it's
> called from. However calling from atomic context should be fine.
>
> 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.

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nÇ‰·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~††Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå

Next message: Kedareswara rao Appana: "[PATCH v5] can: Convert to runtime_pm"
Previous message: Hans Verkuil: "Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder"
In reply to: Marc Kleine-Budde: "Re: [PATCH v4] can: Convert to runtime_pm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]