Messages in this thread | | | Date | Sun, 11 Jan 2015 16:41:20 +0100 | From | Marc Kleine-Budde <> | 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.
>>> 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.
> 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 |
[unhandled content-type:application/pgp-signature] | |