| Message ID | 58f37b6fd9104ce185c413c473fe047b@BY2FFO11FD050.protection.gbl |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > use the runtime_pm framework. This consolidates the actions for > runtime PM in the appropriate callbacks and makes the driver more > readable and mantainable. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > > drivers/net/can/xilinx_can.c | 123 +++++++++++++++++++++++++----------------- > 1 files changed, 74 insertions(+), 49 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 6c67643..c71f683 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -32,6 +32,7 @@ > #include <linux/can/dev.h> > #include <linux/can/error.h> > #include <linux/can/led.h> > +#include <linux/pm_runtime.h> > > #define DRIVER_NAME "xilinx_can" > > @@ -138,7 +139,7 @@ struct xcan_priv { > u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); > void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, > u32 val); > - struct net_device *dev; > + struct device *dev; > void __iomem *reg_base; > unsigned long irq_flags; > struct clk *bus_clk; > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", Does this create the intended output? I haven't seen '\r' anywhere else. Shouldn't this simply be: netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n", [...] > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > - ret = clk_prepare_enable(priv->can_clk); > - if (ret) > - goto err; > - > - ret = clk_prepare_enable(priv->bus_clk); > - if (ret) > - goto err_clk; > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", ditto Sören -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marc,
Could you please provide your feedback on this patch ?
@Soren : Will update the driver with your comments during next version of the patch.
Regards,
Kedar.
-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Wednesday, December 24, 2014 4:13 AM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v4] can: Convert to runtime_pm
On Tue, 2014-12-23 at 05:55PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Chnages for v4:
> - Updated with the review comments.
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
>
> drivers/net/can/xilinx_can.c | 123
> +++++++++++++++++++++++++-----------------
> 1 files changed, 74 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
> #define DRIVER_NAME "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> - struct net_device *dev;
> + struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
Does this create the intended output? I haven't seen '\r' anywhere else.
Shouldn't this simply be:
netdev_err(ndev, "%s: pm_runtime_get failed (%d)\n",
[...]
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret)
> - goto err;
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> - goto err_clk;
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r",
ditto
Sören
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.
On 01/06/2015 07:23 AM, Appana Durga Kedareswara Rao wrote: > Hi Marc, > > Could you please provide your feedback on this patch ? > > @Soren : Will update the driver with your comments during next version of the patch. Sure, I'll return from my season holidays and will be in the office tomorrow. Marc
On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > use the runtime_pm framework. This consolidates the actions for > runtime PM in the appropriate callbacks and makes the driver more > readable and mantainable. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > > drivers/net/can/xilinx_can.c | 123 +++++++++++++++++++++++++----------------- > 1 files changed, 74 insertions(+), 49 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 6c67643..c71f683 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -32,6 +32,7 @@ > #include <linux/can/dev.h> > #include <linux/can/error.h> > #include <linux/can/led.h> > +#include <linux/pm_runtime.h> > > #define DRIVER_NAME "xilinx_can" > > @@ -138,7 +139,7 @@ struct xcan_priv { > u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); > void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, > u32 val); > - struct net_device *dev; > + struct device *dev; > void __iomem *reg_base; > unsigned long irq_flags; > struct clk *bus_clk; > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > + __func__, ret); > + return ret; > + } > + > ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, > ndev->name, ndev); > if (ret < 0) { > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) > goto err; > } > > - ret = clk_prepare_enable(priv->can_clk); > - if (ret) { > - netdev_err(ndev, "unable to enable device clock\n"); > - goto err_irq; > - } > - > - ret = clk_prepare_enable(priv->bus_clk); > - if (ret) { > - netdev_err(ndev, "unable to enable bus clock\n"); > - goto err_can_clk; > - } > - > /* Set chip into reset mode */ > ret = set_reset_mode(ndev); > if (ret < 0) { > netdev_err(ndev, "mode resetting failed!\n"); > - goto err_bus_clk; > + goto err_irq; > } > > /* Common open */ > ret = open_candev(ndev); > if (ret) > - goto err_bus_clk; > + goto err_irq; > > ret = xcan_chip_start(ndev); > if (ret < 0) { > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) > > err_candev: > close_candev(ndev); > -err_bus_clk: > - clk_disable_unprepare(priv->bus_clk); > -err_can_clk: > - clk_disable_unprepare(priv->can_clk); > err_irq: > free_irq(ndev->irq, ndev); > err: > + pm_runtime_put(priv->dev); > + > return ret; > } > > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) > netif_stop_queue(ndev); > napi_disable(&priv->napi); > xcan_chip_stop(ndev); > - clk_disable_unprepare(priv->bus_clk); > - clk_disable_unprepare(priv->can_clk); > free_irq(ndev->irq, ndev); > close_candev(ndev); > > can_led_event(ndev, CAN_LED_EVENT_STOP); > + pm_runtime_put(priv->dev); > > return 0; > } > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > - ret = clk_prepare_enable(priv->can_clk); > - if (ret) > - goto err; > - > - ret = clk_prepare_enable(priv->bus_clk); > - if (ret) > - goto err_clk; > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > + __func__, ret); Please remove the \r from the error messages. > + return ret; > + } > > bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK; > bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & > XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); > > - clk_disable_unprepare(priv->bus_clk); > - clk_disable_unprepare(priv->can_clk); > + pm_runtime_put(priv->dev); > > return 0; > - > -err_clk: > - clk_disable_unprepare(priv->can_clk); > -err: > - return ret; > } > > > @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = { > > /** > * xcan_suspend - Suspend method for the driver > - * @dev: Address of the platform_device structure > + * @dev: Address of the device structure > * > * Put the driver into low power mode. > - * Return: 0 always > + * Return: 0 on success and failure value on error > */ > static int __maybe_unused xcan_suspend(struct device *dev) > { > - struct platform_device *pdev = dev_get_drvdata(dev); > - struct net_device *ndev = platform_get_drvdata(pdev); > + if (!device_may_wakeup(dev)) > + return pm_runtime_force_suspend(dev); > + > + return 0; > +} > + > +/** > + * xcan_resume - Resume from suspend > + * @dev: Address of the device structure > + * > + * Resume operation after suspend. > + * Return: 0 on success and failure value on error > + */ > +static int __maybe_unused xcan_resume(struct device *dev) > +{ > + if (!device_may_wakeup(dev)) > + return pm_runtime_force_resume(dev); > + > + return 0; > + > +} > + > +/** > + * xcan_runtime_suspend - Runtime suspend method for the driver > + * @dev: Address of the device structure > + * > + * Put the driver into low power mode. > + * Return: 0 always > + */ > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) > +{ > + struct net_device *ndev = dev_get_drvdata(dev); > struct xcan_priv *priv = netdev_priv(ndev); > > if (netif_running(ndev)) { > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev) > } > > /** > - * xcan_resume - Resume from suspend > - * @dev: Address of the platformdevice structure > + * xcan_runtime_resume - Runtime resume from suspend > + * @dev: Address of the device structure > * > * Resume operation after suspend. > * Return: 0 on success and failure value on error > */ > -static int __maybe_unused xcan_resume(struct device *dev) > +static int __maybe_unused xcan_runtime_resume(struct device *dev) > { > - struct platform_device *pdev = dev_get_drvdata(dev); > - struct net_device *ndev = platform_get_drvdata(pdev); > + struct net_device *ndev = dev_get_drvdata(dev); > struct xcan_priv *priv = netdev_priv(ndev); > int ret; Some more context: > ret = clk_enable(priv->bus_clk); > if (ret) { > dev_err(dev, "Cannot enable clock.\n"); > return ret; > } > ret = clk_enable(priv->can_clk); > if (ret) { > dev_err(dev, "Cannot enable clock.\n"); > clk_disable_unprepare(priv->bus_clk); This disable_unprepare looks wrong, should be a disable only. > 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? > 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? > + > 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. > 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? > gpiochip_remove(&gpio->chip); > clk_disable_unprepare(gpio->clk); > device_set_wakeup_capable(&pdev->dev, 0); > return 0; > } regards, Marc
On Wed, 2015-01-07 at 01:28PM +0100, Marc Kleine-Budde wrote: > On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote: > > Instead of enabling/disabling clocks at several locations in the driver, > > use the runtime_pm framework. This consolidates the actions for > > runtime PM in the appropriate callbacks and makes the driver more > > readable and mantainable. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > > --- > > Chnages for v4: > > - Updated with the review comments. > > Changes for v3: > > - Converted the driver to use runtime_pm. > > Changes for v2: > > - Removed the struct platform_device* from suspend/resume > > as suggest by Lothar. > > > > drivers/net/can/xilinx_can.c | 123 +++++++++++++++++++++++++----------------- > > 1 files changed, 74 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > > index 6c67643..c71f683 100644 > > --- a/drivers/net/can/xilinx_can.c > > +++ b/drivers/net/can/xilinx_can.c > > @@ -32,6 +32,7 @@ > > #include <linux/can/dev.h> > > #include <linux/can/error.h> > > #include <linux/can/led.h> > > +#include <linux/pm_runtime.h> > > > > #define DRIVER_NAME "xilinx_can" > > > > @@ -138,7 +139,7 @@ struct xcan_priv { > > u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); > > void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, > > u32 val); > > - struct net_device *dev; > > + struct device *dev; > > void __iomem *reg_base; > > unsigned long irq_flags; > > struct clk *bus_clk; > > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > > > + ret = pm_runtime_get_sync(priv->dev); > > + if (ret < 0) { > > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > > + __func__, ret); > > + return ret; > > + } > > + > > ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, > > ndev->name, ndev); > > if (ret < 0) { > > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) > > goto err; > > } > > > > - ret = clk_prepare_enable(priv->can_clk); > > - if (ret) { > > - netdev_err(ndev, "unable to enable device clock\n"); > > - goto err_irq; > > - } > > - > > - ret = clk_prepare_enable(priv->bus_clk); > > - if (ret) { > > - netdev_err(ndev, "unable to enable bus clock\n"); > > - goto err_can_clk; > > - } > > - > > /* Set chip into reset mode */ > > ret = set_reset_mode(ndev); > > if (ret < 0) { > > netdev_err(ndev, "mode resetting failed!\n"); > > - goto err_bus_clk; > > + goto err_irq; > > } > > > > /* Common open */ > > ret = open_candev(ndev); > > if (ret) > > - goto err_bus_clk; > > + goto err_irq; > > > > ret = xcan_chip_start(ndev); > > if (ret < 0) { > > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) > > > > err_candev: > > close_candev(ndev); > > -err_bus_clk: > > - clk_disable_unprepare(priv->bus_clk); > > -err_can_clk: > > - clk_disable_unprepare(priv->can_clk); > > err_irq: > > free_irq(ndev->irq, ndev); > > err: > > + pm_runtime_put(priv->dev); > > + > > return ret; > > } > > > > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) > > netif_stop_queue(ndev); > > napi_disable(&priv->napi); > > xcan_chip_stop(ndev); > > - clk_disable_unprepare(priv->bus_clk); > > - clk_disable_unprepare(priv->can_clk); > > free_irq(ndev->irq, ndev); > > close_candev(ndev); > > > > can_led_event(ndev, CAN_LED_EVENT_STOP); > > + pm_runtime_put(priv->dev); > > > > return 0; > > } > > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > > > - ret = clk_prepare_enable(priv->can_clk); > > - if (ret) > > - goto err; > > - > > - ret = clk_prepare_enable(priv->bus_clk); > > - if (ret) > > - goto err_clk; > > + ret = pm_runtime_get_sync(priv->dev); > > + if (ret < 0) { > > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > > + __func__, ret); > > Please remove the \r from the error messages. > > > + return ret; > > + } > > > > bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK; > > bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & > > XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); > > > > - clk_disable_unprepare(priv->bus_clk); > > - clk_disable_unprepare(priv->can_clk); > > + pm_runtime_put(priv->dev); > > > > return 0; > > - > > -err_clk: > > - clk_disable_unprepare(priv->can_clk); > > -err: > > - return ret; > > } > > > > > > @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = { > > > > /** > > * xcan_suspend - Suspend method for the driver > > - * @dev: Address of the platform_device structure > > + * @dev: Address of the device structure > > * > > * Put the driver into low power mode. > > - * Return: 0 always > > + * Return: 0 on success and failure value on error > > */ > > static int __maybe_unused xcan_suspend(struct device *dev) > > { > > - struct platform_device *pdev = dev_get_drvdata(dev); > > - struct net_device *ndev = platform_get_drvdata(pdev); > > + if (!device_may_wakeup(dev)) > > + return pm_runtime_force_suspend(dev); > > + > > + return 0; > > +} > > + > > +/** > > + * xcan_resume - Resume from suspend > > + * @dev: Address of the device structure > > + * > > + * Resume operation after suspend. > > + * Return: 0 on success and failure value on error > > + */ > > +static int __maybe_unused xcan_resume(struct device *dev) > > +{ > > + if (!device_may_wakeup(dev)) > > + return pm_runtime_force_resume(dev); > > + > > + return 0; > > + > > +} > > + > > +/** > > + * xcan_runtime_suspend - Runtime suspend method for the driver > > + * @dev: Address of the device structure > > + * > > + * Put the driver into low power mode. > > + * Return: 0 always > > + */ > > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) > > +{ > > + struct net_device *ndev = dev_get_drvdata(dev); > > struct xcan_priv *priv = netdev_priv(ndev); > > > > if (netif_running(ndev)) { > > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev) > > } > > > > /** > > - * xcan_resume - Resume from suspend > > - * @dev: Address of the platformdevice structure > > + * xcan_runtime_resume - Runtime resume from suspend > > + * @dev: Address of the device structure > > * > > * Resume operation after suspend. > > * Return: 0 on success and failure value on error > > */ > > -static int __maybe_unused xcan_resume(struct device *dev) > > +static int __maybe_unused xcan_runtime_resume(struct device *dev) > > { > > - struct platform_device *pdev = dev_get_drvdata(dev); > > - struct net_device *ndev = platform_get_drvdata(pdev); > > + struct net_device *ndev = dev_get_drvdata(dev); > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > Some more context: > > > ret = clk_enable(priv->bus_clk); > > if (ret) { > > dev_err(dev, "Cannot enable clock.\n"); > > return ret; > > } > > ret = clk_enable(priv->can_clk); > > if (ret) { > > dev_err(dev, "Cannot enable clock.\n"); > > clk_disable_unprepare(priv->bus_clk); > > This disable_unprepare looks wrong, should be a disable only. > > > 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? > > > 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? > > + > > 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. > > > 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? IIRC, the concern was that the remove function may be called while the device is runtime suspended. Hence the remove function needs to resume the device since the remove function may access the HW. Sören -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/07/2015 04:58 PM, Sören Brinkmann wrote: >> 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? > > IIRC, the concern was that the remove function may be called while the device is > runtime suspended. Hence the remove function needs to resume the device since the > remove function may access the HW. What about the corresponding runtime_put()? Would some counter be unbalanced upon device removal? Without having tested it, unloading and loading, i.e. reloading a CAN driver is easier than reloading the gpio driver on an average embedded system. Kedareswara please test your driver with something like: modprobe; ifconfig up; cansend; ifconfig down; rmmod in a loop. Marc
On Wed, 2015-01-07 at 05:30PM +0100, Marc Kleine-Budde wrote: > On 01/07/2015 04:58 PM, Sören Brinkmann wrote: > >> 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? > > > > IIRC, the concern was that the remove function may be called while the device is > > runtime suspended. Hence the remove function needs to resume the device since the > > remove function may access the HW. > > What about the corresponding runtime_put()? Would some counter be > unbalanced upon device removal? Aren't those counters destroyed with module unloading? Sören -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/07/2015 05:32 PM, Sören Brinkmann wrote: > On Wed, 2015-01-07 at 05:30PM +0100, Marc Kleine-Budde wrote: >> On 01/07/2015 04:58 PM, Sören Brinkmann wrote: >>>> 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? >>> >>> IIRC, the concern was that the remove function may be called while the device is >>> runtime suspended. Hence the remove function needs to resume the device since the >>> remove function may access the HW. >> >> What about the corresponding runtime_put()? Would some counter be >> unbalanced upon device removal? > > Aren't those counters destroyed with module unloading? I don't know. Marc
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Wednesday, January 07, 2015 5:59 PM > To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek; > grant.likely@linaro.org > Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Appana Durga Kedareswara Rao; Soren Brinkmann > Subject: Re: [PATCH v4] can: Convert to runtime_pm > > On 12/23/2014 01:25 PM, Kedareswara rao Appana wrote: > > Instead of enabling/disabling clocks at several locations in the > > driver, use the runtime_pm framework. This consolidates the actions > > for runtime PM in the appropriate callbacks and makes the driver more > > readable and mantainable. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > > --- > > Chnages for v4: > > - Updated with the review comments. > > Changes for v3: > > - Converted the driver to use runtime_pm. > > Changes for v2: > > - Removed the struct platform_device* from suspend/resume > > as suggest by Lothar. > > > > drivers/net/can/xilinx_can.c | 123 > > +++++++++++++++++++++++++----------------- > > 1 files changed, 74 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/net/can/xilinx_can.c > > b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644 > > --- a/drivers/net/can/xilinx_can.c > > +++ b/drivers/net/can/xilinx_can.c > > @@ -32,6 +32,7 @@ > > #include <linux/can/dev.h> > > #include <linux/can/error.h> > > #include <linux/can/led.h> > > +#include <linux/pm_runtime.h> > > > > #define DRIVER_NAME "xilinx_can" > > > > @@ -138,7 +139,7 @@ struct xcan_priv { > > u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); > > void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, > > u32 val); > > - struct net_device *dev; > > + struct device *dev; > > void __iomem *reg_base; > > unsigned long irq_flags; > > struct clk *bus_clk; > > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > > > + ret = pm_runtime_get_sync(priv->dev); > > + if (ret < 0) { > > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > > + __func__, ret); > > + return ret; > > + } > > + > > ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, > > ndev->name, ndev); > > if (ret < 0) { > > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) > > goto err; > > } > > > > - ret = clk_prepare_enable(priv->can_clk); > > - if (ret) { > > - netdev_err(ndev, "unable to enable device clock\n"); > > - goto err_irq; > > - } > > - > > - ret = clk_prepare_enable(priv->bus_clk); > > - if (ret) { > > - netdev_err(ndev, "unable to enable bus clock\n"); > > - goto err_can_clk; > > - } > > - > > /* Set chip into reset mode */ > > ret = set_reset_mode(ndev); > > if (ret < 0) { > > netdev_err(ndev, "mode resetting failed!\n"); > > - goto err_bus_clk; > > + goto err_irq; > > } > > > > /* Common open */ > > ret = open_candev(ndev); > > if (ret) > > - goto err_bus_clk; > > + goto err_irq; > > > > ret = xcan_chip_start(ndev); > > if (ret < 0) { > > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) > > > > err_candev: > > close_candev(ndev); > > -err_bus_clk: > > - clk_disable_unprepare(priv->bus_clk); > > -err_can_clk: > > - clk_disable_unprepare(priv->can_clk); > > err_irq: > > free_irq(ndev->irq, ndev); > > err: > > + pm_runtime_put(priv->dev); > > + > > return ret; > > } > > > > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) > > netif_stop_queue(ndev); > > napi_disable(&priv->napi); > > xcan_chip_stop(ndev); > > - clk_disable_unprepare(priv->bus_clk); > > - clk_disable_unprepare(priv->can_clk); > > free_irq(ndev->irq, ndev); > > close_candev(ndev); > > > > can_led_event(ndev, CAN_LED_EVENT_STOP); > > + pm_runtime_put(priv->dev); > > > > return 0; > > } > > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct > net_device *ndev, > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > > > - ret = clk_prepare_enable(priv->can_clk); > > - if (ret) > > - goto err; > > - > > - ret = clk_prepare_enable(priv->bus_clk); > > - if (ret) > > - goto err_clk; > > + ret = pm_runtime_get_sync(priv->dev); > > + if (ret < 0) { > > + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", > > + __func__, ret); > > Please remove the \r from the error messages. > Ok > > + return ret; > > + } > > > > bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & > XCAN_ECR_TEC_MASK; > > bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & > > XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); > > > > - clk_disable_unprepare(priv->bus_clk); > > - clk_disable_unprepare(priv->can_clk); > > + pm_runtime_put(priv->dev); > > > > return 0; > > - > > -err_clk: > > - clk_disable_unprepare(priv->can_clk); > > -err: > > - return ret; > > } > > > > > > @@ -967,15 +953,45 @@ static const struct net_device_ops > > xcan_netdev_ops = { > > > > /** > > * xcan_suspend - Suspend method for the driver > > - * @dev: Address of the platform_device structure > > + * @dev: Address of the device structure > > * > > * Put the driver into low power mode. > > - * Return: 0 always > > + * Return: 0 on success and failure value on error > > */ > > static int __maybe_unused xcan_suspend(struct device *dev) { > > - struct platform_device *pdev = dev_get_drvdata(dev); > > - struct net_device *ndev = platform_get_drvdata(pdev); > > + if (!device_may_wakeup(dev)) > > + return pm_runtime_force_suspend(dev); > > + > > + return 0; > > +} > > + > > +/** > > + * xcan_resume - Resume from suspend > > + * @dev: Address of the device structure > > + * > > + * Resume operation after suspend. > > + * Return: 0 on success and failure value on error */ static int > > +__maybe_unused xcan_resume(struct device *dev) { > > + if (!device_may_wakeup(dev)) > > + return pm_runtime_force_resume(dev); > > + > > + return 0; > > + > > +} > > + > > +/** > > + * xcan_runtime_suspend - Runtime suspend method for the driver > > + * @dev: Address of the device structure > > + * > > + * Put the driver into low power mode. > > + * Return: 0 always > > + */ > > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) { > > + struct net_device *ndev = dev_get_drvdata(dev); > > struct xcan_priv *priv = netdev_priv(ndev); > > > > if (netif_running(ndev)) { > > @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct > > device *dev) } > > > > /** > > - * xcan_resume - Resume from suspend > > - * @dev: Address of the platformdevice structure > > + * xcan_runtime_resume - Runtime resume from suspend > > + * @dev: Address of the device structure > > * > > * Resume operation after suspend. > > * Return: 0 on success and failure value on error > > */ > > -static int __maybe_unused xcan_resume(struct device *dev) > > +static int __maybe_unused xcan_runtime_resume(struct device *dev) > > { > > - struct platform_device *pdev = dev_get_drvdata(dev); > > - struct net_device *ndev = platform_get_drvdata(pdev); > > + struct net_device *ndev = dev_get_drvdata(dev); > > struct xcan_priv *priv = netdev_priv(ndev); > > int ret; > > Some more context: > > > ret = clk_enable(priv->bus_clk); > > if (ret) { > > dev_err(dev, "Cannot enable clock.\n"); > > return ret; > > } > > ret = clk_enable(priv->can_clk); > > if (ret) { > > dev_err(dev, "Cannot enable clock.\n"); > > clk_disable_unprepare(priv->bus_clk); > > This disable_unprepare looks wrong, should be a disable only. > Ok > > 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 :). > > 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); 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 Regards, Kedar. > > > gpiochip_remove(&gpio->chip); > > clk_disable_unprepare(gpio->clk); > > device_set_wakeup_capable(&pdev->dev, 0); > > return 0; > > } > > regards, > 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.
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
Hi Marc, > -----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: > [...] > >>> 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.
On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote: > Hi Marc, > >> -----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: >> [...] >>>>> 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? > >>>>> 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? Marc
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: > > Hi Marc, > > > >> -----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: > >> [...] > >>>>> 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.
On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote: > 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: >>> Hi Marc, >>> >>>> -----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: >>>> [...] >>>>>>> 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. > >> >>> >>>>>>> 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
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 6c67643..c71f683 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -32,6 +32,7 @@ #include <linux/can/dev.h> #include <linux/can/error.h> #include <linux/can/led.h> +#include <linux/pm_runtime.h> #define DRIVER_NAME "xilinx_can" @@ -138,7 +139,7 @@ struct xcan_priv { u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, u32 val); - struct net_device *dev; + struct device *dev; void __iomem *reg_base; unsigned long irq_flags; struct clk *bus_clk; @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) struct xcan_priv *priv = netdev_priv(ndev); int ret; + ret = pm_runtime_get_sync(priv->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", + __func__, ret); + return ret; + } + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, ndev->name, ndev); if (ret < 0) { @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) goto err; } - ret = clk_prepare_enable(priv->can_clk); - if (ret) { - netdev_err(ndev, "unable to enable device clock\n"); - goto err_irq; - } - - ret = clk_prepare_enable(priv->bus_clk); - if (ret) { - netdev_err(ndev, "unable to enable bus clock\n"); - goto err_can_clk; - } - /* Set chip into reset mode */ ret = set_reset_mode(ndev); if (ret < 0) { netdev_err(ndev, "mode resetting failed!\n"); - goto err_bus_clk; + goto err_irq; } /* Common open */ ret = open_candev(ndev); if (ret) - goto err_bus_clk; + goto err_irq; ret = xcan_chip_start(ndev); if (ret < 0) { @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) err_candev: close_candev(ndev); -err_bus_clk: - clk_disable_unprepare(priv->bus_clk); -err_can_clk: - clk_disable_unprepare(priv->can_clk); err_irq: free_irq(ndev->irq, ndev); err: + pm_runtime_put(priv->dev); + return ret; } @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) netif_stop_queue(ndev); napi_disable(&priv->napi); xcan_chip_stop(ndev); - clk_disable_unprepare(priv->bus_clk); - clk_disable_unprepare(priv->can_clk); free_irq(ndev->irq, ndev); close_candev(ndev); can_led_event(ndev, CAN_LED_EVENT_STOP); + pm_runtime_put(priv->dev); return 0; } @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, struct xcan_priv *priv = netdev_priv(ndev); int ret; - ret = clk_prepare_enable(priv->can_clk); - if (ret) - goto err; - - ret = clk_prepare_enable(priv->bus_clk); - if (ret) - goto err_clk; + ret = pm_runtime_get_sync(priv->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed\r(%d)\n\r", + __func__, ret); + return ret; + } bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK; bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); - clk_disable_unprepare(priv->bus_clk); - clk_disable_unprepare(priv->can_clk); + pm_runtime_put(priv->dev); return 0; - -err_clk: - clk_disable_unprepare(priv->can_clk); -err: - return ret; } @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = { /** * xcan_suspend - Suspend method for the driver - * @dev: Address of the platform_device structure + * @dev: Address of the device structure * * Put the driver into low power mode. - * Return: 0 always + * Return: 0 on success and failure value on error */ static int __maybe_unused xcan_suspend(struct device *dev) { - struct platform_device *pdev = dev_get_drvdata(dev); - struct net_device *ndev = platform_get_drvdata(pdev); + if (!device_may_wakeup(dev)) + return pm_runtime_force_suspend(dev); + + return 0; +} + +/** + * xcan_resume - Resume from suspend + * @dev: Address of the device structure + * + * Resume operation after suspend. + * Return: 0 on success and failure value on error + */ +static int __maybe_unused xcan_resume(struct device *dev) +{ + if (!device_may_wakeup(dev)) + return pm_runtime_force_resume(dev); + + return 0; + +} + +/** + * xcan_runtime_suspend - Runtime suspend method for the driver + * @dev: Address of the device structure + * + * Put the driver into low power mode. + * Return: 0 always + */ +static int __maybe_unused xcan_runtime_suspend(struct device *dev) +{ + struct net_device *ndev = dev_get_drvdata(dev); struct xcan_priv *priv = netdev_priv(ndev); if (netif_running(ndev)) { @@ -993,16 +1009,15 @@ static int __maybe_unused xcan_suspend(struct device *dev) } /** - * xcan_resume - Resume from suspend - * @dev: Address of the platformdevice structure + * xcan_runtime_resume - Runtime resume from suspend + * @dev: Address of the device structure * * Resume operation after suspend. * Return: 0 on success and failure value on error */ -static int __maybe_unused xcan_resume(struct device *dev) +static int __maybe_unused xcan_runtime_resume(struct device *dev) { - struct platform_device *pdev = dev_get_drvdata(dev); - struct net_device *ndev = platform_get_drvdata(pdev); + struct net_device *ndev = dev_get_drvdata(dev); struct xcan_priv *priv = netdev_priv(ndev); int ret; @@ -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); + ret = register_candev(ndev); if (ret) { dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret); + pm_runtime_put(priv->dev); 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);