linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] can: flexcan: implement can Runtime PM
@ 2018-11-29  8:08 Joakim Zhang
  2018-11-29  9:44 ` Marc Kleine-Budde
  0 siblings, 1 reply; 3+ messages in thread
From: Joakim Zhang @ 2018-11-29  8:08 UTC (permalink / raw)
  To: mkl, linux-can
  Cc: wg, netdev, linux-kernel, dl-linux-imx, Aisheng DONG, Joakim Zhang

From: Aisheng Dong <aisheng.dong@nxp.com>

Flexcan will be disabled during suspend if no wakeup function required and
enabled after resume accordingly. During this period, we could explicitly
disable clocks.
Since PM is optional, the clock is enabled at probe to guarante the
clock is running when PM is not enabled in the kernel.

Implement Runtime PM which will:
1) Without CONFIG_PM, clock is running whether Flexcan opened or closed.
2) With CONFIG_PM, clock enabled while Flexcan opened and disabled when
3) Regardless of CONFIG_PM enabled or not, Flexcan runtime status is
   SUSPENDED during Flexcan closed and ACTIVE when Flexcan opened.
4) Make Power Domain framework be able to shutdown the corresponding power
   domain of this device.

Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLog:
V1->V2:
	*rebased on patch "can: flexcan: add self wakeup support".
V2->V3:
	*fix device fails to probe without CONFIG_PM.
---
 drivers/net/can/flexcan.c | 107 +++++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 37 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0f36eafe3ac1..cc62a97237db 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regmap.h>
 
@@ -277,6 +278,7 @@ struct flexcan_priv {
 	u32 reg_imask1_default;
 	u32 reg_imask2_default;
 
+	struct device *dev;
 	struct clk *clk_ipg;
 	struct clk *clk_per;
 	const struct flexcan_devtype_data *devtype_data;
@@ -444,6 +446,27 @@ static inline void flexcan_error_irq_disable(const struct flexcan_priv *priv)
 	priv->write(reg_ctrl, &regs->ctrl);
 }
 
+static int flexcan_clks_enable(const struct flexcan_priv *priv)
+{
+	int err;
+
+	err = clk_prepare_enable(priv->clk_ipg);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(priv->clk_per);
+	if (err)
+		clk_disable_unprepare(priv->clk_ipg);
+
+	return err;
+}
+
+static void flexcan_clks_disable(const struct flexcan_priv *priv)
+{
+	clk_disable_unprepare(priv->clk_ipg);
+	clk_disable_unprepare(priv->clk_per);
+}
+
 static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
 {
 	if (!priv->reg_xceiver)
@@ -570,19 +593,13 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 	const struct flexcan_priv *priv = netdev_priv(dev);
 	int err;
 
-	err = clk_prepare_enable(priv->clk_ipg);
-	if (err)
+	err = pm_runtime_get_sync(priv->dev);
+	if (err < 0)
 		return err;
 
-	err = clk_prepare_enable(priv->clk_per);
-	if (err)
-		goto out_disable_ipg;
-
 	err = __flexcan_get_berr_counter(dev, bec);
 
-	clk_disable_unprepare(priv->clk_per);
- out_disable_ipg:
-	clk_disable_unprepare(priv->clk_ipg);
+	pm_runtime_put(priv->dev);
 
 	return err;
 }
@@ -1215,17 +1232,13 @@ static int flexcan_open(struct net_device *dev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	int err;
 
-	err = clk_prepare_enable(priv->clk_ipg);
-	if (err)
+	err = pm_runtime_get_sync(priv->dev);
+	if (err < 0)
 		return err;
 
-	err = clk_prepare_enable(priv->clk_per);
-	if (err)
-		goto out_disable_ipg;
-
 	err = open_candev(dev);
 	if (err)
-		goto out_disable_per;
+		goto out_disable_clks;
 
 	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
 	if (err)
@@ -1288,10 +1301,8 @@ static int flexcan_open(struct net_device *dev)
 	free_irq(dev->irq, dev);
  out_close:
 	close_candev(dev);
- out_disable_per:
-	clk_disable_unprepare(priv->clk_per);
- out_disable_ipg:
-	clk_disable_unprepare(priv->clk_ipg);
+ out_disable_clks:
+	pm_runtime_put(priv->dev);
 
 	return err;
 }
@@ -1306,10 +1317,9 @@ static int flexcan_close(struct net_device *dev)
 
 	can_rx_offload_del(&priv->offload);
 	free_irq(dev->irq, dev);
-	clk_disable_unprepare(priv->clk_per);
-	clk_disable_unprepare(priv->clk_ipg);
 
 	close_candev(dev);
+	pm_runtime_put(priv->dev);
 
 	can_led_event(dev, CAN_LED_EVENT_STOP);
 
@@ -1349,18 +1359,14 @@ static int register_flexcandev(struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->regs;
 	u32 reg, err;
 
-	err = clk_prepare_enable(priv->clk_ipg);
+	err = flexcan_clks_enable(priv);
 	if (err)
 		return err;
 
-	err = clk_prepare_enable(priv->clk_per);
-	if (err)
-		goto out_disable_ipg;
-
 	/* select "bus clock", chip must be disabled */
 	err = flexcan_chip_disable(priv);
 	if (err)
-		goto out_disable_per;
+		goto out_disable_clks;
 	reg = priv->read(&regs->ctrl);
 	reg |= FLEXCAN_CTRL_CLK_SRC;
 	priv->write(reg, &regs->ctrl);
@@ -1389,13 +1395,12 @@ static int register_flexcandev(struct net_device *dev)
 
 	err = register_candev(dev);
 
-	/* disable core and turn off clocks */
+	return 0;
+
  out_chip_disable:
 	flexcan_chip_disable(priv);
- out_disable_per:
-	clk_disable_unprepare(priv->clk_per);
- out_disable_ipg:
-	clk_disable_unprepare(priv->clk_ipg);
+ out_disable_clks:
+	flexcan_clks_disable(priv);
 
 	return err;
 }
@@ -1556,6 +1561,7 @@ static int flexcan_probe(struct platform_device *pdev)
 		priv->write = flexcan_write_le;
 	}
 
+	priv->dev = &pdev->dev;
 	priv->can.clock.freq = clock_freq;
 	priv->can.bittiming_const = &flexcan_bittiming_const;
 	priv->can.do_set_mode = flexcan_set_mode;
@@ -1586,6 +1592,9 @@ static int flexcan_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->regs, dev->irq);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_put(&pdev->dev);
+
 	return 0;
 
  failed_register:
@@ -1598,6 +1607,7 @@ static int flexcan_remove(struct platform_device *pdev)
 	struct net_device *dev = platform_get_drvdata(pdev);
 
 	unregister_flexcandev(dev);
+	pm_runtime_disable(&pdev->dev);
 	free_candev(dev);
 
 	return 0;
@@ -1607,7 +1617,7 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
-	int err;
+	int err = 0;
 
 	if (netif_running(dev)) {
 		/* if wakeup is enabled, enter stop mode
@@ -1620,20 +1630,22 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 			err = flexcan_chip_disable(priv);
 			if (err)
 				return err;
+
+			err = pm_runtime_force_suspend(device);
 		}
 		netif_stop_queue(dev);
 		netif_device_detach(dev);
 	}
 	priv->can.state = CAN_STATE_SLEEPING;
 
-	return 0;
+	return err;
 }
 
 static int __maybe_unused flexcan_resume(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
-	int err;
+	int err = 0;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 	if (netif_running(dev)) {
@@ -1642,14 +1654,34 @@ static int __maybe_unused flexcan_resume(struct device *device)
 		if (device_may_wakeup(device)) {
 			disable_irq_wake(dev->irq);
 		} else {
-			err = flexcan_chip_enable(priv);
+			err = pm_runtime_force_resume(device);
 			if (err)
 				return err;
+
+			err = flexcan_chip_enable(priv);
 		}
 	}
+	return err;
+}
+
+static int __maybe_unused flexcan_runtime_suspend(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct flexcan_priv *priv = netdev_priv(dev);
+
+	flexcan_clks_disable(priv);
+
 	return 0;
 }
 
+static int __maybe_unused flexcan_runtime_resume(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct flexcan_priv *priv = netdev_priv(dev);
+
+	return flexcan_clks_enable(priv);
+}
+
 static int __maybe_unused flexcan_noirq_suspend(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
@@ -1676,6 +1708,7 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
 
 static const struct dev_pm_ops flexcan_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
+	SET_RUNTIME_PM_OPS(flexcan_runtime_suspend, flexcan_runtime_resume, NULL)
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend, flexcan_noirq_resume)
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH V3] can: flexcan: implement can Runtime PM
  2018-11-29  8:08 [PATCH V3] can: flexcan: implement can Runtime PM Joakim Zhang
@ 2018-11-29  9:44 ` Marc Kleine-Budde
  2018-11-29 10:17   ` Joakim Zhang
  0 siblings, 1 reply; 3+ messages in thread
From: Marc Kleine-Budde @ 2018-11-29  9:44 UTC (permalink / raw)
  To: Joakim Zhang, linux-can
  Cc: wg, netdev, linux-kernel, dl-linux-imx, Aisheng DONG


[-- Attachment #1.1: Type: text/plain, Size: 10438 bytes --]

On 11/29/18 9:08 AM, Joakim Zhang wrote:
> From: Aisheng Dong <aisheng.dong@nxp.com>
> 
> Flexcan will be disabled during suspend if no wakeup function required and
> enabled after resume accordingly. During this period, we could explicitly
> disable clocks.
> Since PM is optional, the clock is enabled at probe to guarante the
> clock is running when PM is not enabled in the kernel.
> 
> Implement Runtime PM which will:
> 1) Without CONFIG_PM, clock is running whether Flexcan opened or closed.

ACK

> 2) With CONFIG_PM, clock enabled while Flexcan opened and disabled when

...when closed?

I think the sentence misses something at the end.

And it doesn't work:

> root@DistroKit:~ ifconfig can0
> can0      Link encap:UNSPEC  HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  
>           NOARP  MTU:16  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:10 
>           RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
>           Interrupt:34 
> 
> root@DistroKit:~ grep can /sys/kernel/debug/clk/clk_summary 
>                 can_root              1        1        0    30000000          0     0  50000
>                    can2_serial        0        0        0    30000000          0     0  50000
>                    can1_serial        1        1        0    30000000          0     0  50000
>                             can2_ipg       0        0        0    66000000          0     0  50000
>                             can1_ipg       1        1        0    66000000          0     0  50000

can0 is down, while the can1_{serial,ipg} are enabled.

> 3) Regardless of CONFIG_PM enabled or not, Flexcan runtime status is
>    SUSPENDED during Flexcan closed and ACTIVE when Flexcan opened.
> 4) Make Power Domain framework be able to shutdown the corresponding power
>    domain of this device.
> 
> Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> ChangeLog:
> V1->V2:
> 	*rebased on patch "can: flexcan: add self wakeup support".
> V2->V3:
> 	*fix device fails to probe without CONFIG_PM.
> ---
>  drivers/net/can/flexcan.c | 107 +++++++++++++++++++++++++-------------
>  1 file changed, 70 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 0f36eafe3ac1..cc62a97237db 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -24,6 +24,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regmap.h>
>  
> @@ -277,6 +278,7 @@ struct flexcan_priv {
>  	u32 reg_imask1_default;
>  	u32 reg_imask2_default;
>  
> +	struct device *dev;
>  	struct clk *clk_ipg;
>  	struct clk *clk_per;
>  	const struct flexcan_devtype_data *devtype_data;
> @@ -444,6 +446,27 @@ static inline void flexcan_error_irq_disable(const struct flexcan_priv *priv)
>  	priv->write(reg_ctrl, &regs->ctrl);
>  }
>  
> +static int flexcan_clks_enable(const struct flexcan_priv *priv)
> +{
> +	int err;
> +
> +	err = clk_prepare_enable(priv->clk_ipg);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(priv->clk_per);
> +	if (err)
> +		clk_disable_unprepare(priv->clk_ipg);
> +
> +	return err;
> +}
> +
> +static void flexcan_clks_disable(const struct flexcan_priv *priv)
> +{
> +	clk_disable_unprepare(priv->clk_ipg);
> +	clk_disable_unprepare(priv->clk_per);
> +}
> +
>  static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
>  {
>  	if (!priv->reg_xceiver)
> @@ -570,19 +593,13 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
>  	const struct flexcan_priv *priv = netdev_priv(dev);
>  	int err;
>  
> -	err = clk_prepare_enable(priv->clk_ipg);
> -	if (err)
> +	err = pm_runtime_get_sync(priv->dev);
> +	if (err < 0)
>  		return err;
>  
> -	err = clk_prepare_enable(priv->clk_per);
> -	if (err)
> -		goto out_disable_ipg;
> -
>  	err = __flexcan_get_berr_counter(dev, bec);
>  
> -	clk_disable_unprepare(priv->clk_per);
> - out_disable_ipg:
> -	clk_disable_unprepare(priv->clk_ipg);
> +	pm_runtime_put(priv->dev);
>  
>  	return err;
>  }
> @@ -1215,17 +1232,13 @@ static int flexcan_open(struct net_device *dev)
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	int err;
>  
> -	err = clk_prepare_enable(priv->clk_ipg);
> -	if (err)
> +	err = pm_runtime_get_sync(priv->dev);
> +	if (err < 0)
>  		return err;
>  
> -	err = clk_prepare_enable(priv->clk_per);
> -	if (err)
> -		goto out_disable_ipg;
> -
>  	err = open_candev(dev);
>  	if (err)
> -		goto out_disable_per;
> +		goto out_disable_clks;
>  
>  	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
>  	if (err)
> @@ -1288,10 +1301,8 @@ static int flexcan_open(struct net_device *dev)
>  	free_irq(dev->irq, dev);
>   out_close:
>  	close_candev(dev);
> - out_disable_per:
> -	clk_disable_unprepare(priv->clk_per);
> - out_disable_ipg:
> -	clk_disable_unprepare(priv->clk_ipg);
> + out_disable_clks:
> +	pm_runtime_put(priv->dev);
>  
>  	return err;
>  }
> @@ -1306,10 +1317,9 @@ static int flexcan_close(struct net_device *dev)
>  
>  	can_rx_offload_del(&priv->offload);
>  	free_irq(dev->irq, dev);
> -	clk_disable_unprepare(priv->clk_per);
> -	clk_disable_unprepare(priv->clk_ipg);
>  
>  	close_candev(dev);
> +	pm_runtime_put(priv->dev);
>  
>  	can_led_event(dev, CAN_LED_EVENT_STOP);
>  
> @@ -1349,18 +1359,14 @@ static int register_flexcandev(struct net_device *dev)
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 reg, err;
>  
> -	err = clk_prepare_enable(priv->clk_ipg);
> +	err = flexcan_clks_enable(priv);
>  	if (err)
>  		return err;
>  
> -	err = clk_prepare_enable(priv->clk_per);
> -	if (err)
> -		goto out_disable_ipg;
> -
>  	/* select "bus clock", chip must be disabled */
>  	err = flexcan_chip_disable(priv);
>  	if (err)
> -		goto out_disable_per;
> +		goto out_disable_clks;
>  	reg = priv->read(&regs->ctrl);
>  	reg |= FLEXCAN_CTRL_CLK_SRC;
>  	priv->write(reg, &regs->ctrl);
> @@ -1389,13 +1395,12 @@ static int register_flexcandev(struct net_device *dev)
>  
>  	err = register_candev(dev);
>  
> -	/* disable core and turn off clocks */

Please disable the device, even if keeping the clocks on.

> +	return 0;
> +
>   out_chip_disable:
>  	flexcan_chip_disable(priv);
> - out_disable_per:
> -	clk_disable_unprepare(priv->clk_per);
> - out_disable_ipg:
> -	clk_disable_unprepare(priv->clk_ipg);
> + out_disable_clks:
> +	flexcan_clks_disable(priv);
>  
>  	return err;
>  }
> @@ -1556,6 +1561,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  		priv->write = flexcan_write_le;
>  	}
>  
> +	priv->dev = &pdev->dev;
>  	priv->can.clock.freq = clock_freq;
>  	priv->can.bittiming_const = &flexcan_bittiming_const;
>  	priv->can.do_set_mode = flexcan_set_mode;
> @@ -1586,6 +1592,9 @@ static int flexcan_probe(struct platform_device *pdev)
>  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
>  		 priv->regs, dev->irq);
>  
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_put(&pdev->dev);
> +
>  	return 0;
>  
>   failed_register:
> @@ -1598,6 +1607,7 @@ static int flexcan_remove(struct platform_device *pdev)
>  	struct net_device *dev = platform_get_drvdata(pdev);
>  
>  	unregister_flexcandev(dev);
> +	pm_runtime_disable(&pdev->dev);
>  	free_candev(dev);
>  
>  	return 0;
> @@ -1607,7 +1617,7 @@ static int __maybe_unused flexcan_suspend(struct device *device)
>  {
>  	struct net_device *dev = dev_get_drvdata(device);
>  	struct flexcan_priv *priv = netdev_priv(dev);
> -	int err;
> +	int err = 0;
>  
>  	if (netif_running(dev)) {
>  		/* if wakeup is enabled, enter stop mode
> @@ -1620,20 +1630,22 @@ static int __maybe_unused flexcan_suspend(struct device *device)
>  			err = flexcan_chip_disable(priv);
>  			if (err)
>  				return err;
> +
> +			err = pm_runtime_force_suspend(device);
>  		}
>  		netif_stop_queue(dev);
>  		netif_device_detach(dev);
>  	}
>  	priv->can.state = CAN_STATE_SLEEPING;
>  
> -	return 0;
> +	return err;
>  }
>  
>  static int __maybe_unused flexcan_resume(struct device *device)
>  {
>  	struct net_device *dev = dev_get_drvdata(device);
>  	struct flexcan_priv *priv = netdev_priv(dev);
> -	int err;
> +	int err = 0;
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  	if (netif_running(dev)) {
> @@ -1642,14 +1654,34 @@ static int __maybe_unused flexcan_resume(struct device *device)
>  		if (device_may_wakeup(device)) {
>  			disable_irq_wake(dev->irq);
>  		} else {
> -			err = flexcan_chip_enable(priv);
> +			err = pm_runtime_force_resume(device);
>  			if (err)
>  				return err;
> +
> +			err = flexcan_chip_enable(priv);
>  		}
>  	}
> +	return err;
> +}
> +
> +static int __maybe_unused flexcan_runtime_suspend(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	flexcan_clks_disable(priv);
> +
>  	return 0;
>  }
>  
> +static int __maybe_unused flexcan_runtime_resume(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	return flexcan_clks_enable(priv);
> +}
> +
>  static int __maybe_unused flexcan_noirq_suspend(struct device *device)
>  {
>  	struct net_device *dev = dev_get_drvdata(device);
> @@ -1676,6 +1708,7 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
>  
>  static const struct dev_pm_ops flexcan_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> +	SET_RUNTIME_PM_OPS(flexcan_runtime_suspend, flexcan_runtime_resume, NULL)
>  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend, flexcan_noirq_resume)
>  };
>  
> 

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
* RE: [PATCH V3] can: flexcan: implement can Runtime PM
  2018-11-29  9:44 ` Marc Kleine-Budde
@ 2018-11-29 10:17   ` Joakim Zhang
  0 siblings, 0 replies; 3+ messages in thread
From: Joakim Zhang @ 2018-11-29 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, linux-can
  Cc: wg, netdev, linux-kernel, dl-linux-imx, Aisheng DONG


> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2018年11月29日 17:44
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Aisheng
> DONG <aisheng.dong@nxp.com>
> Subject: Re: [PATCH V3] can: flexcan: implement can Runtime PM
> 
> On 11/29/18 9:08 AM, Joakim Zhang wrote:
> > From: Aisheng Dong <aisheng.dong@nxp.com>
> >
> > Flexcan will be disabled during suspend if no wakeup function required
> > and enabled after resume accordingly. During this period, we could
> > explicitly disable clocks.
> > Since PM is optional, the clock is enabled at probe to guarante the
> > clock is running when PM is not enabled in the kernel.
> >
> > Implement Runtime PM which will:
> > 1) Without CONFIG_PM, clock is running whether Flexcan opened or closed.
> 
> ACK
> 
> > 2) With CONFIG_PM, clock enabled while Flexcan opened and disabled
> > when
> 
> ...when closed?
> 
> I think the sentence misses something at the end.
> 
> And it doesn't work:
> 
> > root@DistroKit:~ ifconfig can0
> > can0      Link encap:UNSPEC  HWaddr
> 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >           NOARP  MTU:16  Metric:1
> >           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
> >           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
> >           collisions:0 txqueuelen:10
> >           RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
> >           Interrupt:34
> >
> > root@DistroKit:~ grep can /sys/kernel/debug/clk/clk_summary
> >                 can_root              1        1        0
> 30000000          0     0  50000
> >                    can2_serial        0        0        0
> 30000000          0     0  50000
> >                    can1_serial        1        1        0
> 30000000          0     0  50000
> >                             can2_ipg       0        0        0
> 66000000          0     0  50000
> >                             can1_ipg       1        1        0
> 66000000          0     0  50000
> 
> can0 is down, while the can1_{serial,ipg} are enabled.

Hi Marc,

Thanks for reviewing the patch, and then I will test and fix the issue.

Best Regards,
Joakim Zhang

> > 3) Regardless of CONFIG_PM enabled or not, Flexcan runtime status is
> >    SUSPENDED during Flexcan closed and ACTIVE when Flexcan opened.
> > 4) Make Power Domain framework be able to shutdown the corresponding
> power
> >    domain of this device.
> >
> > Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > ChangeLog:
> > V1->V2:
> > 	*rebased on patch "can: flexcan: add self wakeup support".
> > V2->V3:
> > 	*fix device fails to probe without CONFIG_PM.
> > ---
> >  drivers/net/can/flexcan.c | 107
> > +++++++++++++++++++++++++-------------
> >  1 file changed, 70 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 0f36eafe3ac1..cc62a97237db 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>  #include <linux/regmap.h>
> >
> > @@ -277,6 +278,7 @@ struct flexcan_priv {
> >  	u32 reg_imask1_default;
> >  	u32 reg_imask2_default;
> >
> > +	struct device *dev;
> >  	struct clk *clk_ipg;
> >  	struct clk *clk_per;
> >  	const struct flexcan_devtype_data *devtype_data; @@ -444,6 +446,27
> > @@ static inline void flexcan_error_irq_disable(const struct flexcan_priv
> *priv)
> >  	priv->write(reg_ctrl, &regs->ctrl);
> >  }
> >
> > +static int flexcan_clks_enable(const struct flexcan_priv *priv) {
> > +	int err;
> > +
> > +	err = clk_prepare_enable(priv->clk_ipg);
> > +	if (err)
> > +		return err;
> > +
> > +	err = clk_prepare_enable(priv->clk_per);
> > +	if (err)
> > +		clk_disable_unprepare(priv->clk_ipg);
> > +
> > +	return err;
> > +}
> > +
> > +static void flexcan_clks_disable(const struct flexcan_priv *priv) {
> > +	clk_disable_unprepare(priv->clk_ipg);
> > +	clk_disable_unprepare(priv->clk_per);
> > +}
> > +
> >  static inline int flexcan_transceiver_enable(const struct
> > flexcan_priv *priv)  {
> >  	if (!priv->reg_xceiver)
> > @@ -570,19 +593,13 @@ static int flexcan_get_berr_counter(const struct
> net_device *dev,
> >  	const struct flexcan_priv *priv = netdev_priv(dev);
> >  	int err;
> >
> > -	err = clk_prepare_enable(priv->clk_ipg);
> > -	if (err)
> > +	err = pm_runtime_get_sync(priv->dev);
> > +	if (err < 0)
> >  		return err;
> >
> > -	err = clk_prepare_enable(priv->clk_per);
> > -	if (err)
> > -		goto out_disable_ipg;
> > -
> >  	err = __flexcan_get_berr_counter(dev, bec);
> >
> > -	clk_disable_unprepare(priv->clk_per);
> > - out_disable_ipg:
> > -	clk_disable_unprepare(priv->clk_ipg);
> > +	pm_runtime_put(priv->dev);
> >
> >  	return err;
> >  }
> > @@ -1215,17 +1232,13 @@ static int flexcan_open(struct net_device *dev)
> >  	struct flexcan_priv *priv = netdev_priv(dev);
> >  	int err;
> >
> > -	err = clk_prepare_enable(priv->clk_ipg);
> > -	if (err)
> > +	err = pm_runtime_get_sync(priv->dev);
> > +	if (err < 0)
> >  		return err;
> >
> > -	err = clk_prepare_enable(priv->clk_per);
> > -	if (err)
> > -		goto out_disable_ipg;
> > -
> >  	err = open_candev(dev);
> >  	if (err)
> > -		goto out_disable_per;
> > +		goto out_disable_clks;
> >
> >  	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
> >  	if (err)
> > @@ -1288,10 +1301,8 @@ static int flexcan_open(struct net_device *dev)
> >  	free_irq(dev->irq, dev);
> >   out_close:
> >  	close_candev(dev);
> > - out_disable_per:
> > -	clk_disable_unprepare(priv->clk_per);
> > - out_disable_ipg:
> > -	clk_disable_unprepare(priv->clk_ipg);
> > + out_disable_clks:
> > +	pm_runtime_put(priv->dev);
> >
> >  	return err;
> >  }
> > @@ -1306,10 +1317,9 @@ static int flexcan_close(struct net_device
> > *dev)
> >
> >  	can_rx_offload_del(&priv->offload);
> >  	free_irq(dev->irq, dev);
> > -	clk_disable_unprepare(priv->clk_per);
> > -	clk_disable_unprepare(priv->clk_ipg);
> >
> >  	close_candev(dev);
> > +	pm_runtime_put(priv->dev);
> >
> >  	can_led_event(dev, CAN_LED_EVENT_STOP);
> >
> > @@ -1349,18 +1359,14 @@ static int register_flexcandev(struct net_device
> *dev)
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> >  	u32 reg, err;
> >
> > -	err = clk_prepare_enable(priv->clk_ipg);
> > +	err = flexcan_clks_enable(priv);
> >  	if (err)
> >  		return err;
> >
> > -	err = clk_prepare_enable(priv->clk_per);
> > -	if (err)
> > -		goto out_disable_ipg;
> > -
> >  	/* select "bus clock", chip must be disabled */
> >  	err = flexcan_chip_disable(priv);
> >  	if (err)
> > -		goto out_disable_per;
> > +		goto out_disable_clks;
> >  	reg = priv->read(&regs->ctrl);
> >  	reg |= FLEXCAN_CTRL_CLK_SRC;
> >  	priv->write(reg, &regs->ctrl);
> > @@ -1389,13 +1395,12 @@ static int register_flexcandev(struct
> > net_device *dev)
> >
> >  	err = register_candev(dev);
> >
> > -	/* disable core and turn off clocks */
> 
> Please disable the device, even if keeping the clocks on.
> 
> > +	return 0;
> > +
> >   out_chip_disable:
> >  	flexcan_chip_disable(priv);
> > - out_disable_per:
> > -	clk_disable_unprepare(priv->clk_per);
> > - out_disable_ipg:
> > -	clk_disable_unprepare(priv->clk_ipg);
> > + out_disable_clks:
> > +	flexcan_clks_disable(priv);
> >
> >  	return err;
> >  }
> > @@ -1556,6 +1561,7 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  		priv->write = flexcan_write_le;
> >  	}
> >
> > +	priv->dev = &pdev->dev;
> >  	priv->can.clock.freq = clock_freq;
> >  	priv->can.bittiming_const = &flexcan_bittiming_const;
> >  	priv->can.do_set_mode = flexcan_set_mode; @@ -1586,6 +1592,9 @@
> > static int flexcan_probe(struct platform_device *pdev)
> >  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> >  		 priv->regs, dev->irq);
> >
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_put(&pdev->dev);
> > +
> >  	return 0;
> >
> >   failed_register:
> > @@ -1598,6 +1607,7 @@ static int flexcan_remove(struct platform_device
> *pdev)
> >  	struct net_device *dev = platform_get_drvdata(pdev);
> >
> >  	unregister_flexcandev(dev);
> > +	pm_runtime_disable(&pdev->dev);
> >  	free_candev(dev);
> >
> >  	return 0;
> > @@ -1607,7 +1617,7 @@ static int __maybe_unused
> flexcan_suspend(struct
> > device *device)  {
> >  	struct net_device *dev = dev_get_drvdata(device);
> >  	struct flexcan_priv *priv = netdev_priv(dev);
> > -	int err;
> > +	int err = 0;
> >
> >  	if (netif_running(dev)) {
> >  		/* if wakeup is enabled, enter stop mode @@ -1620,20 +1630,22
> @@
> > static int __maybe_unused flexcan_suspend(struct device *device)
> >  			err = flexcan_chip_disable(priv);
> >  			if (err)
> >  				return err;
> > +
> > +			err = pm_runtime_force_suspend(device);
> >  		}
> >  		netif_stop_queue(dev);
> >  		netif_device_detach(dev);
> >  	}
> >  	priv->can.state = CAN_STATE_SLEEPING;
> >
> > -	return 0;
> > +	return err;
> >  }
> >
> >  static int __maybe_unused flexcan_resume(struct device *device)  {
> >  	struct net_device *dev = dev_get_drvdata(device);
> >  	struct flexcan_priv *priv = netdev_priv(dev);
> > -	int err;
> > +	int err = 0;
> >
> >  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  	if (netif_running(dev)) {
> > @@ -1642,14 +1654,34 @@ static int __maybe_unused
> flexcan_resume(struct device *device)
> >  		if (device_may_wakeup(device)) {
> >  			disable_irq_wake(dev->irq);
> >  		} else {
> > -			err = flexcan_chip_enable(priv);
> > +			err = pm_runtime_force_resume(device);
> >  			if (err)
> >  				return err;
> > +
> > +			err = flexcan_chip_enable(priv);
> >  		}
> >  	}
> > +	return err;
> > +}
> > +
> > +static int __maybe_unused flexcan_runtime_suspend(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	flexcan_clks_disable(priv);
> > +
> >  	return 0;
> >  }
> >
> > +static int __maybe_unused flexcan_runtime_resume(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	return flexcan_clks_enable(priv);
> > +}
> > +
> >  static int __maybe_unused flexcan_noirq_suspend(struct device
> > *device)  {
> >  	struct net_device *dev = dev_get_drvdata(device); @@ -1676,6 +1708,7
> > @@ static int __maybe_unused flexcan_noirq_resume(struct device
> > *device)
> >
> >  static const struct dev_pm_ops flexcan_pm_ops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > +	SET_RUNTIME_PM_OPS(flexcan_runtime_suspend,
> flexcan_runtime_resume,
> > +NULL)
> >  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > flexcan_noirq_resume)  };
> >
> >
> 
> 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   |


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-29 10:17 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29  8:08 [PATCH V3] can: flexcan: implement can Runtime PM Joakim Zhang
2018-11-29  9:44 ` Marc Kleine-Budde
2018-11-29 10:17   ` Joakim Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).