Re: [PATCH v6] can: xilinx: Convert to runtime_pm

[Sören Brinkmann]

Hi Kedar,

On Thu, 2015-10-22 at 10:15AM +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: Kedareswara rao Appana 
[...]
>  /**
>   * xcan_probe - Platform registration call
> @@ -1072,7 +1103,7 @@ static int xcan_probe(struct platform_device *pdev)
>   return -ENOMEM;
>  
>   priv = netdev_priv(ndev);
> - priv->dev = ndev;
> + priv->dev = >dev;
>   priv->can.bittiming_const = _bittiming_const;
>   priv->can.do_set_mode = xcan_do_set_mode;
>   priv->can.do_get_berr_counter = xcan_get_berr_counter;
> @@ -1114,21 +1145,30 @@ static int xcan_probe(struct platform_device *pdev)
>   }
>   }
>  
> - ret = clk_prepare_enable(priv->can_clk);
> + ret = clk_prepare(priv->can_clk);
>   if (ret) {
>   dev_err(>dev, "unable to enable device clock\n");
>   goto err_free;
>   }
>  
> - ret = clk_prepare_enable(priv->bus_clk);
> + ret = clk_prepare(priv->bus_clk);

Are these clk_prepare calls needed here? The runtime PM calls do
clk_prepare_enable and clk_disable_unprepare.

>   if (ret) {
>   dev_err(>dev, "unable to enable bus clock\n");
> - goto err_unprepare_disable_dev;
> + goto err_unprepare_dev;
>   }
>  
>   priv->write_reg = xcan_write_reg_le;
>   priv->read_reg = xcan_read_reg_le;
>  
> + pm_runtime_irq_safe(>dev);
> + pm_runtime_enable(>dev);
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + goto err_unprepare_busclk;
> + }
> +
>   if (priv->read_reg(priv, XCAN_SR_OFFSET) != XCAN_SR_CONFIG_MASK) {
>   priv->write_reg = xcan_write_reg_be;
>   priv->read_reg = xcan_read_reg_be;
> @@ -1141,22 +1181,26 @@ static int xcan_probe(struct platform_device *pdev)
>   ret = register_candev(ndev);
>   if (ret) {
>   dev_err(>dev, "fail to register failed (err=%d)\n", ret);
> - goto err_unprepare_disable_busclk;
> + goto err_disableclks;
>   }
>  
>   devm_can_led_init(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> +
> + pm_runtime_put(>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);
>  
>   return 0;
>  
> -err_unprepare_disable_busclk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_unprepare_disable_dev:
> - clk_disable_unprepare(priv->can_clk);
> +err_disableclks:
> + pm_runtime_put(priv->dev);
> +err_unprepare_busclk:
> + pm_runtime_disable(>dev);
> + clk_unprepare(priv->bus_clk);
> +err_unprepare_dev:
> + clk_unprepare(priv->can_clk);
>  err_free:
>   free_candev(ndev);
>  err:
> @@ -1175,11 +1219,11 @@ static int xcan_remove(struct platform_device *pdev)
>   struct net_device *ndev = platform_get_drvdata(pdev);
>   struct xcan_priv *priv = netdev_priv(ndev);
>  
> - if (set_reset_mode(ndev) < 0)
> - netdev_err(ndev, "mode resetting failed!\n");
> -
>   unregister_candev(ndev);
> + pm_runtime_disable(>dev);
>   netif_napi_del(>napi);
> + clk_unprepare(priv->bus_clk);
> + clk_unprepare(priv->can_clk);

I think this can go away when the prepare calls in probe go away.

Thanks,
Sören
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6] can: xilinx: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Soren,

> -Original Message-
> From: Sören Brinkmann [mailto:soren.brinkm...@xilinx.com]
> Sent: Friday, October 23, 2015 3:43 AM
> To: Appana Durga Kedareswara Rao
> Cc: Anirudha Sarangi; w...@grandegger.com; m...@pengutronix.de; Michal
> Simek; linux-...@vger.kernel.org; netdev@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-ker...@vger.kernel.org; Appana Durga
> Kedareswara Rao
> Subject: Re: [PATCH v6] can: xilinx: Convert to runtime_pm
> 
> Hi Kedar,
> 
> On Thu, 2015-10-22 at 10:15AM +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: Kedareswara rao Appana <appa...@xilinx.com>
> [...]
> >  /**
> >   * xcan_probe - Platform registration call @@ -1072,7 +1103,7 @@
> > static int xcan_probe(struct platform_device *pdev)
> > return -ENOMEM;
> >
> > priv = netdev_priv(ndev);
> > -   priv->dev = ndev;
> > +   priv->dev = >dev;
> > priv->can.bittiming_const = _bittiming_const;
> > priv->can.do_set_mode = xcan_do_set_mode;
> > priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> 1114,21
> > +1145,30 @@ static int xcan_probe(struct platform_device *pdev)
> > }
> > }
> >
> > -   ret = clk_prepare_enable(priv->can_clk);
> > +   ret = clk_prepare(priv->can_clk);
> > if (ret) {
> > dev_err(>dev, "unable to enable device clock\n");
> > goto err_free;
> > }
> >
> > -   ret = clk_prepare_enable(priv->bus_clk);
> > +   ret = clk_prepare(priv->bus_clk);
> 
> Are these clk_prepare calls needed here? The runtime PM calls do
> clk_prepare_enable and clk_disable_unprepare.

Ok Sure will remove.

> 
> > if (ret) {
> > dev_err(>dev, "unable to enable bus clock\n");
> > -   goto err_unprepare_disable_dev;
> > +   goto err_unprepare_dev;
> > }
> >
> > priv->write_reg = xcan_write_reg_le;
> > priv->read_reg = xcan_read_reg_le;
> >
> > +   pm_runtime_irq_safe(>dev);
> > +   pm_runtime_enable(>dev);
> > +   ret = pm_runtime_get_sync(>dev);
> > +   if (ret < 0) {
> > +   netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> > +   __func__, ret);
> > +   goto err_unprepare_busclk;
> > +   }
> > +
> > if (priv->read_reg(priv, XCAN_SR_OFFSET) != XCAN_SR_CONFIG_MASK) {
> > priv->write_reg = xcan_write_reg_be;
> > priv->read_reg = xcan_read_reg_be;
> > @@ -1141,22 +1181,26 @@ static int xcan_probe(struct platform_device
> *pdev)
> > ret = register_candev(ndev);
> > if (ret) {
> > dev_err(>dev, "fail to register failed (err=%d)\n", ret);
> > -   goto err_unprepare_disable_busclk;
> > +   goto err_disableclks;
> > }
> >
> > devm_can_led_init(ndev);
> > -   clk_disable_unprepare(priv->bus_clk);
> > -   clk_disable_unprepare(priv->can_clk);
> > +
> > +   pm_runtime_put(>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);
> >
> > return 0;
> >
> > -err_unprepare_disable_busclk:
> > -   clk_disable_unprepare(priv->bus_clk);
> > -err_unprepare_disable_dev:
> > -   clk_disable_unprepare(priv->can_clk);
> > +err_disableclks:
> > +   pm_runtime_put(priv->dev);
> > +err_unprepare_busclk:
> > +   pm_runtime_disable(>dev);
> > +   clk_unprepare(priv->bus_clk);
> > +err_unprepare_dev:
> > +   clk_unprepare(priv->can_clk);
> >  err_free:
> > free_candev(ndev);
> >  err:
> > @@ -1175,11 +1219,11 @@ static int xcan_remove(struct platform_device
> *pdev)
> > struct net_device *ndev = platform_get_drvdata(pdev);
> > struct xcan_priv *priv = netdev_priv(ndev);
> >
> > -   if (set_reset_mode(ndev) < 0)
> > -   netdev_err(ndev, "mode resetting failed!\n");
> > -
> > unregister_candev(ndev);
> > +   pm_runtime_disable(>dev);
> > netif_napi_del(>napi);
> > +   clk_unprepare(priv->bus_clk);
> > +   clk_unprepare(priv->can_clk);
> 
> I think this can go away when the prepare calls in probe go away.

Ok will fix and will send next version of the patch.

Regards,
Kedar.

> 
>   Thanks,
>   Sören

[PATCH v6] can: xilinx: Convert to runtime_pm

[Kedareswara rao Appana]

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: Kedareswara rao Appana 
---
Sorry for the long delay for sending this next version of the patch.
somehow couldn't manage to send this next version of the patch.
 
Changes for v6:
 - Updated the driver with review comments as suggested by Marc.
Changes for v5:
 - Updated with the review comments.
   Updated the remove fuction to use runtime_pm.
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 | 176 +++
 1 file changed, 110 insertions(+), 66 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index fc55e8e..6114214 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #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;
@@ -843,6 +844,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(%d)\n",
+   __func__, ret);
+   return ret;
+   }
+
ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
ndev->name, ndev);
if (ret < 0) {
@@ -850,29 +858,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) {
@@ -888,13 +884,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;
 }
 
@@ -911,12 +905,11 @@ static int xcan_close(struct net_device *ndev)
netif_stop_queue(ndev);
napi_disable(>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;
 }
@@ -935,27 +928,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(%d)\n",
+   __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;
 }
 
 
@@ -968,15 +954,45 @@ static const struct net_device_ops xcan_netdev_ops = {
 
 /**
  * xcan_suspend - Suspend method for the driver
- * @dev:   Address of the platform_device structure
+ *