[PATCH V4 0/1] can: flexcan: add self wakeup support

[PATCH V4 0/1] can: flexcan: add self wakeup support

[Joakim Zhang]

This patch intends to add CAN self wakeup support. The CAN controller can
parse stop mode property from device tree to enable self wakeup feature.

ChangeLog:
V1->V2:
	*add a vendor prefix in property (stop-mode -> fsl,stop-mode).
V2->V3:
	*add FLEXCAN_QUIRK_SETUP_STOP_MODE quirk.
	*rename function.
	*fix system can't be wakeuped during suspend.
V3->V4:
	*normalize the code following Aisheng Dong's comments.

Aisheng Dong (1):
  can: flexcan: add self wakeup support

 drivers/net/can/flexcan.c | 163 +++++++++++++++++++++++++++++++++++---
 1 file changed, 154 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH V4 1/1] can: flexcan: add self wakeup support

[Joakim Zhang]

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

If wakeup is enabled, enter stop mode, else enter disabled mode. Self wake
can only work on stop mode.

Starting from IMX6, the flexcan stop mode control bits is SoC specific,
move it out of IP driver and parse it from devicetree.

Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 163 +++++++++++++++++++++++++++++++++++---
 1 file changed, 154 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 8e972ef08637..83431810316e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -19,11 +19,14 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #define DRV_NAME			"flexcan"
 
@@ -131,7 +134,8 @@
 	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)
 #define FLEXCAN_ESR_ALL_INT \
 	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
-	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
+	FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
+	FLEXCAN_ESR_WAK_INT)
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
@@ -190,6 +194,7 @@
 #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp based offloading */
 #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for error passive */
 #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE register access */
+#define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop mode to support wakeup */
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -254,6 +259,14 @@ struct flexcan_devtype_data {
 	u32 quirks;		/* quirks needed for different IP cores */
 };
 
+struct flexcan_stop_mode {
+	struct regmap *gpr;
+	u8 req_gpr;
+	u8 req_bit;
+	u8 ack_gpr;
+	u8 ack_bit;
+};
+
 struct flexcan_priv {
 	struct can_priv can;
 	struct can_rx_offload offload;
@@ -270,6 +283,7 @@ struct flexcan_priv {
 	struct clk *clk_per;
 	const struct flexcan_devtype_data *devtype_data;
 	struct regulator *reg_xceiver;
+	struct flexcan_stop_mode stm;
 
 	/* Read and Write APIs */
 	u32 (*read)(void __iomem *addr);
@@ -293,7 +307,8 @@ static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
 
 static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
-		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE,
+		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
+		FLEXCAN_QUIRK_SETUP_STOP_MODE,
 };
 
 static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
@@ -353,6 +368,35 @@ static inline void flexcan_write_le(u32 val, void __iomem *addr)
 	iowrite32(val, addr);
 }
 
+static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_mcr;
+
+	reg_mcr = priv->read(&regs->mcr);
+
+	if (enable)
+		reg_mcr |= FLEXCAN_MCR_WAK_MSK;
+	else
+		reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
+
+	priv->write(reg_mcr, &regs->mcr);
+}
+
+static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv)
+{
+	/* enable stop request */
+	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
+			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
+}
+
+static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv)
+{
+	/* remove stop request */
+	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
+			   1 << priv->stm.req_bit, 0);
+}
+
 static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
@@ -940,6 +984,10 @@ static int flexcan_chip_start(struct net_device *dev)
 		reg_mcr |= FLEXCAN_MCR_FEN |
 			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
 	}
+
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE)
+		reg_mcr |= FLEXCAN_MCR_SLF_WAK;
+
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	priv->write(reg_mcr, &regs->mcr);
 
@@ -1244,6 +1292,58 @@ static void unregister_flexcandev(struct net_device *dev)
 	unregister_candev(dev);
 }
 
+static int flexcan_setup_stop_mode(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *gpr_np;
+	struct flexcan_priv *priv;
+	phandle phandle;
+	u32 out_val[5];
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	/* stop mode property format is:
+	 * <&gpr req_gpr req_bit ack_gpr ack_bit>.
+	 */
+	ret = of_property_read_u32_array(np, "fsl,stop-mode", out_val, 5);
+	if (ret) {
+		dev_dbg(&pdev->dev, "no stop-mode property\n");
+		return ret;
+	}
+	phandle = *out_val;
+
+	gpr_np = of_find_node_by_phandle(phandle);
+	if (!gpr_np) {
+		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
+		return PTR_ERR(gpr_np);
+	}
+
+	priv = netdev_priv(dev);
+	priv->stm.gpr = syscon_node_to_regmap(gpr_np);
+	if (IS_ERR(priv->stm.gpr)) {
+		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
+		return PTR_ERR(priv->stm.gpr);
+	}
+
+	of_node_put(gpr_np);
+
+	priv->stm.req_gpr = out_val[1];
+	priv->stm.req_bit = out_val[2];
+	priv->stm.ack_gpr = out_val[3];
+	priv->stm.ack_bit = out_val[4];
+
+	dev_dbg(&pdev->dev, "gpr %s req_gpr 0x%x req_bit %u ack_gpr 0x%x ack_bit %u\n",
+		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit, priv->stm.ack_gpr,
+		priv->stm.ack_bit);
+
+	device_set_wakeup_capable(&pdev->dev, true);
+
+	return 0;
+}
+
 static const struct of_device_id flexcan_of_match[] = {
 	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
 	{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
@@ -1396,6 +1496,12 @@ static int flexcan_probe(struct platform_device *pdev)
 
 	devm_can_led_init(dev);
 
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
+		err = flexcan_setup_stop_mode(pdev);
+		if (err)
+			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
+	}
+
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->regs, dev->irq);
 
@@ -1426,9 +1532,17 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 	int err;
 
 	if (netif_running(dev)) {
-		err = flexcan_chip_disable(priv);
-		if (err)
-			return err;
+		/* if wakeup is enabled, enter stop mode
+		 * else enter disabled mode.
+		 */
+		if (device_may_wakeup(device)) {
+			enable_irq_wake(dev->irq);
+			flexcan_enter_stop_mode(priv);
+		} else {
+			err = flexcan_chip_disable(priv);
+			if (err)
+				return err;
+		}
 		netif_stop_queue(dev);
 		netif_device_detach(dev);
 	}
@@ -1447,14 +1561,45 @@ static int __maybe_unused flexcan_resume(struct device *device)
 	if (netif_running(dev)) {
 		netif_device_attach(dev);
 		netif_start_queue(dev);
-		err = flexcan_chip_enable(priv);
-		if (err)
-			return err;
+		if (device_may_wakeup(device)) {
+			flexcan_enable_wakeup_irq(priv, false);
+		} else {
+			err = flexcan_chip_enable(priv);
+			if (err)
+				return err;
+		}
+	}
+	return 0;
+}
+
+static int __maybe_unused flexcan_noirq_suspend(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct flexcan_priv *priv = netdev_priv(dev);
+
+	if (netif_running(dev) && device_may_wakeup(device))
+		flexcan_enable_wakeup_irq(priv, true);
+
+	return 0;
+}
+
+static int __maybe_unused flexcan_noirq_resume(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct flexcan_priv *priv = netdev_priv(dev);
+
+	if (netif_running(dev) && device_may_wakeup(device)) {
+		disable_irq_wake(dev->irq);
+		flexcan_exit_stop_mode(priv);
 	}
+
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend, flexcan_resume);
+static const struct dev_pm_ops flexcan_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend, flexcan_noirq_resume)
+};
 
 static struct platform_driver flexcan_driver = {
 	.driver = {
-- 
2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Aisheng DONG]

This mostly looks good to me.
A few minor comments

> -----Original Message-----
> From: Joakim Zhang
> Sent: Wednesday, November 21, 2018 8:32 PM
> To: linux-can@vger.kernel.org; mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; Aisheng DONG <aisheng.dong@nxp.com>; Joakim
> Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> From: Aisheng Dong <aisheng.dong@nxp.com>
> 
> If wakeup is enabled, enter stop mode, else enter disabled mode. Self wake can
> only work on stop mode.
> 
> Starting from IMX6, the flexcan stop mode control bits is SoC specific, move it
> out of IP driver and parse it from devicetree.
> 
> Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 163
> +++++++++++++++++++++++++++++++++++---
>  1 file changed, 154 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 8e972ef08637..83431810316e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -19,11 +19,14 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> 
>  #define DRV_NAME			"flexcan"
> 
> @@ -131,7 +134,8 @@
>  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> FLEXCAN_ESR_ALL_INT \
>  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> +	FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> +	FLEXCAN_ESR_WAK_INT)
> 
>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -190,6 +194,7 @@
>  #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp
> based offloading */
>  #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for
> error passive */
>  #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE
> register access */
> +#define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop
> mode to support wakeup */
> 
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -254,6 +259,14 @@ struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
>  };
> 
> +struct flexcan_stop_mode {
> +	struct regmap *gpr;
> +	u8 req_gpr;
> +	u8 req_bit;
> +	u8 ack_gpr;
> +	u8 ack_bit;
> +};
> +
>  struct flexcan_priv {
>  	struct can_priv can;
>  	struct can_rx_offload offload;
> @@ -270,6 +283,7 @@ struct flexcan_priv {
>  	struct clk *clk_per;
>  	const struct flexcan_devtype_data *devtype_data;
>  	struct regulator *reg_xceiver;
> +	struct flexcan_stop_mode stm;
> 
>  	/* Read and Write APIs */
>  	u32 (*read)(void __iomem *addr);
> @@ -293,7 +307,8 @@ static const struct flexcan_devtype_data
> fsl_imx28_devtype_data = {
> 
>  static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> +		FLEXCAN_QUIRK_SETUP_STOP_MODE,
>  };
> 
>  static const struct flexcan_devtype_data fsl_vf610_devtype_data = { @@
> -353,6 +368,35 @@ static inline void flexcan_write_le(u32 val, void __iomem
> *addr)
>  	iowrite32(val, addr);
>  }
> 
> +static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> +enable) {
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_mcr;
> +
> +	reg_mcr = priv->read(&regs->mcr);
> +
> +	if (enable)
> +		reg_mcr |= FLEXCAN_MCR_WAK_MSK;
> +	else
> +		reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
> +
> +	priv->write(reg_mcr, &regs->mcr);
> +}
> +
> +static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv) {
> +	/* enable stop request */
> +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit); }
> +
> +static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv) {
> +	/* remove stop request */
> +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +			   1 << priv->stm.req_bit, 0);
> +}
> +
>  static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)  {
>  	struct flexcan_regs __iomem *regs = priv->regs; @@ -940,6 +984,10 @@
> static int flexcan_chip_start(struct net_device *dev)
>  		reg_mcr |= FLEXCAN_MCR_FEN |
>  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
>  	}
> +
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE)
> +		reg_mcr |= FLEXCAN_MCR_SLF_WAK;

Please try if you can merge this into flexcan_enable_wakeup_irq().
If not, you can check with device_can_wakeup() as the stop parsing may fail.

> +
>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	priv->write(reg_mcr, &regs->mcr);
> 
> @@ -1244,6 +1292,58 @@ static void unregister_flexcandev(struct net_device
> *dev)
>  	unregister_candev(dev);
>  }
> 
> +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *gpr_np;
> +	struct flexcan_priv *priv;
> +	phandle phandle;
> +	u32 out_val[5];
> +	int ret;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	/* stop mode property format is:
> +	 * <&gpr req_gpr req_bit ack_gpr ack_bit>.
> +	 */
> +	ret = of_property_read_u32_array(np, "fsl,stop-mode", out_val, 5);
> +	if (ret) {
> +		dev_dbg(&pdev->dev, "no stop-mode property\n");
> +		return ret;
> +	}
> +	phandle = *out_val;
> +
> +	gpr_np = of_find_node_by_phandle(phandle);
> +	if (!gpr_np) {
> +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
> +		return PTR_ERR(gpr_np);
> +	}
> +
> +	priv = netdev_priv(dev);
> +	priv->stm.gpr = syscon_node_to_regmap(gpr_np);

Better to put node here to cover failure case?

Regards
Dong Aisheng

> +	if (IS_ERR(priv->stm.gpr)) {
> +		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
> +		return PTR_ERR(priv->stm.gpr);
> +	}
> +
> +	of_node_put(gpr_np);
> +
> +	priv->stm.req_gpr = out_val[1];
> +	priv->stm.req_bit = out_val[2];
> +	priv->stm.ack_gpr = out_val[3];
> +	priv->stm.ack_bit = out_val[4];
> +
> +	dev_dbg(&pdev->dev, "gpr %s req_gpr 0x%x req_bit %u ack_gpr 0x%x
> ack_bit %u\n",
> +		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit,
> priv->stm.ack_gpr,
> +		priv->stm.ack_bit);
> +
> +	device_set_wakeup_capable(&pdev->dev, true);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id flexcan_of_match[] = {
>  	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
>  	{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
> @@ -1396,6 +1496,12 @@ static int flexcan_probe(struct platform_device
> *pdev)
> 
>  	devm_can_led_init(dev);
> 
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
> +		err = flexcan_setup_stop_mode(pdev);
> +		if (err)
> +			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
> +	}
> +
>  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
>  		 priv->regs, dev->irq);
> 
> @@ -1426,9 +1532,17 @@ static int __maybe_unused flexcan_suspend(struct
> device *device)
>  	int err;
> 
>  	if (netif_running(dev)) {
> -		err = flexcan_chip_disable(priv);
> -		if (err)
> -			return err;
> +		/* if wakeup is enabled, enter stop mode
> +		 * else enter disabled mode.
> +		 */
> +		if (device_may_wakeup(device)) {
> +			enable_irq_wake(dev->irq);
> +			flexcan_enter_stop_mode(priv);
> +		} else {
> +			err = flexcan_chip_disable(priv);
> +			if (err)
> +				return err;
> +		}
>  		netif_stop_queue(dev);
>  		netif_device_detach(dev);
>  	}
> @@ -1447,14 +1561,45 @@ static int __maybe_unused
> flexcan_resume(struct device *device)
>  	if (netif_running(dev)) {
>  		netif_device_attach(dev);
>  		netif_start_queue(dev);
> -		err = flexcan_chip_enable(priv);
> -		if (err)
> -			return err;
> +		if (device_may_wakeup(device)) {
> +			flexcan_enable_wakeup_irq(priv, false);
> +		} else {
> +			err = flexcan_chip_enable(priv);
> +			if (err)
> +				return err;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int __maybe_unused flexcan_noirq_suspend(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	if (netif_running(dev) && device_may_wakeup(device))
> +		flexcan_enable_wakeup_irq(priv, true);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	if (netif_running(dev) && device_may_wakeup(device)) {
> +		disable_irq_wake(dev->irq);
> +		flexcan_exit_stop_mode(priv);
>  	}
> +
>  	return 0;
>  }
> 
> -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> flexcan_resume);
> +static const struct dev_pm_ops flexcan_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> +flexcan_noirq_resume) };
> 
>  static struct platform_driver flexcan_driver = {
>  	.driver = {
> --
> 2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Aisheng DONG]

> -----Original Message-----
> From: Aisheng DONG
> Sent: Wednesday, November 21, 2018 9:00 PM
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> This mostly looks good to me.
> A few minor comments
> 

BTW, you should re-send the series with binding doc patch if it's still not merged.

Regards
Dong Aisheng

> > -----Original Message-----
> > From: Joakim Zhang
> > Sent: Wednesday, November 21, 2018 8:32 PM
> > To: linux-can@vger.kernel.org; mkl@pengutronix.de
> > Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>; Aisheng DONG <aisheng.dong@nxp.com>; Joakim
> Zhang
> > <qiangqing.zhang@nxp.com>
> > Subject: [PATCH V4 1/1] can: flexcan: add self wakeup support
> >
> > From: Aisheng Dong <aisheng.dong@nxp.com>
> >
> > If wakeup is enabled, enter stop mode, else enter disabled mode. Self
> > wake can only work on stop mode.
> >
> > Starting from IMX6, the flexcan stop mode control bits is SoC
> > specific, move it out of IP driver and parse it from devicetree.
> >
> > Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 163
> > +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 154 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 8e972ef08637..83431810316e 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -19,11 +19,14 @@
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/regmap.h>
> >
> >  #define DRV_NAME			"flexcan"
> >
> > @@ -131,7 +134,8 @@
> >  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> > FLEXCAN_ESR_ALL_INT \
> >  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> > -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> > +	FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> > +	FLEXCAN_ESR_WAK_INT)
> >
> >  /* FLEXCAN interrupt flag register (IFLAG) bits */
> >  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -190,6 +194,7
> @@
> >  #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp
> > based offloading */
> >  #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt
> for
> > error passive */
> >  #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE
> > register access */
> > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop
> > mode to support wakeup */
> >
> >  /* Structure of the message buffer */  struct flexcan_mb { @@ -254,6
> > +259,14 @@ struct flexcan_devtype_data {
> >  	u32 quirks;		/* quirks needed for different IP cores */
> >  };
> >
> > +struct flexcan_stop_mode {
> > +	struct regmap *gpr;
> > +	u8 req_gpr;
> > +	u8 req_bit;
> > +	u8 ack_gpr;
> > +	u8 ack_bit;
> > +};
> > +
> >  struct flexcan_priv {
> >  	struct can_priv can;
> >  	struct can_rx_offload offload;
> > @@ -270,6 +283,7 @@ struct flexcan_priv {
> >  	struct clk *clk_per;
> >  	const struct flexcan_devtype_data *devtype_data;
> >  	struct regulator *reg_xceiver;
> > +	struct flexcan_stop_mode stm;
> >
> >  	/* Read and Write APIs */
> >  	u32 (*read)(void __iomem *addr);
> > @@ -293,7 +307,8 @@ static const struct flexcan_devtype_data
> > fsl_imx28_devtype_data = {
> >
> >  static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> > FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> > -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> > +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > +		FLEXCAN_QUIRK_SETUP_STOP_MODE,
> >  };
> >
> >  static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
> > @@
> > -353,6 +368,35 @@ static inline void flexcan_write_le(u32 val, void
> > __iomem
> > *addr)
> >  	iowrite32(val, addr);
> >  }
> >
> > +static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > +enable) {
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	u32 reg_mcr;
> > +
> > +	reg_mcr = priv->read(&regs->mcr);
> > +
> > +	if (enable)
> > +		reg_mcr |= FLEXCAN_MCR_WAK_MSK;
> > +	else
> > +		reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
> > +
> > +	priv->write(reg_mcr, &regs->mcr);
> > +}
> > +
> > +static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv) {
> > +	/* enable stop request */
> > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit); }
> > +
> > +static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv) {
> > +	/* remove stop request */
> > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +			   1 << priv->stm.req_bit, 0);
> > +}
> > +
> >  static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
> {
> >  	struct flexcan_regs __iomem *regs = priv->regs; @@ -940,6 +984,10 @@
> > static int flexcan_chip_start(struct net_device *dev)
> >  		reg_mcr |= FLEXCAN_MCR_FEN |
> >  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
> >  	}
> > +
> > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE)
> > +		reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> 
> Please try if you can merge this into flexcan_enable_wakeup_irq().
> If not, you can check with device_can_wakeup() as the stop parsing may fail.
> 
> > +
> >  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> > @@ -1244,6 +1292,58 @@ static void unregister_flexcandev(struct
> > net_device
> > *dev)
> >  	unregister_candev(dev);
> >  }
> >
> > +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *gpr_np;
> > +	struct flexcan_priv *priv;
> > +	phandle phandle;
> > +	u32 out_val[5];
> > +	int ret;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	/* stop mode property format is:
> > +	 * <&gpr req_gpr req_bit ack_gpr ack_bit>.
> > +	 */
> > +	ret = of_property_read_u32_array(np, "fsl,stop-mode", out_val, 5);
> > +	if (ret) {
> > +		dev_dbg(&pdev->dev, "no stop-mode property\n");
> > +		return ret;
> > +	}
> > +	phandle = *out_val;
> > +
> > +	gpr_np = of_find_node_by_phandle(phandle);
> > +	if (!gpr_np) {
> > +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
> > +		return PTR_ERR(gpr_np);
> > +	}
> > +
> > +	priv = netdev_priv(dev);
> > +	priv->stm.gpr = syscon_node_to_regmap(gpr_np);
> 
> Better to put node here to cover failure case?
> 
> Regards
> Dong Aisheng
> 
> > +	if (IS_ERR(priv->stm.gpr)) {
> > +		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
> > +		return PTR_ERR(priv->stm.gpr);
> > +	}
> > +
> > +	of_node_put(gpr_np);
> > +
> > +	priv->stm.req_gpr = out_val[1];
> > +	priv->stm.req_bit = out_val[2];
> > +	priv->stm.ack_gpr = out_val[3];
> > +	priv->stm.ack_bit = out_val[4];
> > +
> > +	dev_dbg(&pdev->dev, "gpr %s req_gpr 0x%x req_bit %u ack_gpr 0x%x
> > ack_bit %u\n",
> > +		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit,
> > priv->stm.ack_gpr,
> > +		priv->stm.ack_bit);
> > +
> > +	device_set_wakeup_capable(&pdev->dev, true);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct of_device_id flexcan_of_match[] = {
> >  	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
> >  	{ .compatible = "fsl,imx28-flexcan", .data =
> > &fsl_imx28_devtype_data, }, @@ -1396,6 +1496,12 @@ static int
> > flexcan_probe(struct platform_device
> > *pdev)
> >
> >  	devm_can_led_init(dev);
> >
> > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
> > +		err = flexcan_setup_stop_mode(pdev);
> > +		if (err)
> > +			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
> > +	}
> > +
> >  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> >  		 priv->regs, dev->irq);
> >
> > @@ -1426,9 +1532,17 @@ static int __maybe_unused
> > flexcan_suspend(struct device *device)
> >  	int err;
> >
> >  	if (netif_running(dev)) {
> > -		err = flexcan_chip_disable(priv);
> > -		if (err)
> > -			return err;
> > +		/* if wakeup is enabled, enter stop mode
> > +		 * else enter disabled mode.
> > +		 */
> > +		if (device_may_wakeup(device)) {
> > +			enable_irq_wake(dev->irq);
> > +			flexcan_enter_stop_mode(priv);
> > +		} else {
> > +			err = flexcan_chip_disable(priv);
> > +			if (err)
> > +				return err;
> > +		}
> >  		netif_stop_queue(dev);
> >  		netif_device_detach(dev);
> >  	}
> > @@ -1447,14 +1561,45 @@ static int __maybe_unused
> > flexcan_resume(struct device *device)
> >  	if (netif_running(dev)) {
> >  		netif_device_attach(dev);
> >  		netif_start_queue(dev);
> > -		err = flexcan_chip_enable(priv);
> > -		if (err)
> > -			return err;
> > +		if (device_may_wakeup(device)) {
> > +			flexcan_enable_wakeup_irq(priv, false);
> > +		} else {
> > +			err = flexcan_chip_enable(priv);
> > +			if (err)
> > +				return err;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_suspend(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device))
> > +		flexcan_enable_wakeup_irq(priv, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device)) {
> > +		disable_irq_wake(dev->irq);
> > +		flexcan_exit_stop_mode(priv);
> >  	}
> > +
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> > flexcan_resume);
> > +static const struct dev_pm_ops flexcan_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > +flexcan_noirq_resume) };
> >
> >  static struct platform_driver flexcan_driver = {
> >  	.driver = {
> > --
> > 2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Joakim Zhang]

Hi Aisheng,


> -----Original Message-----
> From: Aisheng DONG
> Sent: 2018年11月21日 21:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> This mostly looks good to me.
> A few minor comments
> 
> > -----Original Message-----
> > From: Joakim Zhang
> > Sent: Wednesday, November 21, 2018 8:32 PM
> > To: linux-can@vger.kernel.org; mkl@pengutronix.de
> > Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>; Aisheng DONG <aisheng.dong@nxp.com>; Joakim
> Zhang
> > <qiangqing.zhang@nxp.com>
> > Subject: [PATCH V4 1/1] can: flexcan: add self wakeup support
> >
> > From: Aisheng Dong <aisheng.dong@nxp.com>
> >
> > If wakeup is enabled, enter stop mode, else enter disabled mode. Self
> > wake can only work on stop mode.
> >
> > Starting from IMX6, the flexcan stop mode control bits is SoC
> > specific, move it out of IP driver and parse it from devicetree.
> >
> > Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 163
> > +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 154 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 8e972ef08637..83431810316e 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -19,11 +19,14 @@
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/regmap.h>
> >
> >  #define DRV_NAME			"flexcan"
> >
> > @@ -131,7 +134,8 @@
> >  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> > FLEXCAN_ESR_ALL_INT \
> >  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> > -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> > +	FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> > +	FLEXCAN_ESR_WAK_INT)
> >
> >  /* FLEXCAN interrupt flag register (IFLAG) bits */
> >  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -190,6 +194,7
> @@
> >  #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp
> > based offloading */
> >  #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt
> for
> > error passive */
> >  #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE
> > register access */
> > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop
> > mode to support wakeup */
> >
> >  /* Structure of the message buffer */  struct flexcan_mb { @@ -254,6
> > +259,14 @@ struct flexcan_devtype_data {
> >  	u32 quirks;		/* quirks needed for different IP cores */
> >  };
> >
> > +struct flexcan_stop_mode {
> > +	struct regmap *gpr;
> > +	u8 req_gpr;
> > +	u8 req_bit;
> > +	u8 ack_gpr;
> > +	u8 ack_bit;
> > +};
> > +
> >  struct flexcan_priv {
> >  	struct can_priv can;
> >  	struct can_rx_offload offload;
> > @@ -270,6 +283,7 @@ struct flexcan_priv {
> >  	struct clk *clk_per;
> >  	const struct flexcan_devtype_data *devtype_data;
> >  	struct regulator *reg_xceiver;
> > +	struct flexcan_stop_mode stm;
> >
> >  	/* Read and Write APIs */
> >  	u32 (*read)(void __iomem *addr);
> > @@ -293,7 +307,8 @@ static const struct flexcan_devtype_data
> > fsl_imx28_devtype_data = {
> >
> >  static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> > FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> > -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> > +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > +		FLEXCAN_QUIRK_SETUP_STOP_MODE,
> >  };
> >
> >  static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
> > @@
> > -353,6 +368,35 @@ static inline void flexcan_write_le(u32 val, void
> > __iomem
> > *addr)
> >  	iowrite32(val, addr);
> >  }
> >
> > +static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > +enable) {
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	u32 reg_mcr;
> > +
> > +	reg_mcr = priv->read(&regs->mcr);
> > +
> > +	if (enable)
> > +		reg_mcr |= FLEXCAN_MCR_WAK_MSK;
> > +	else
> > +		reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
> > +
> > +	priv->write(reg_mcr, &regs->mcr);
> > +}
> > +
> > +static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv) {
> > +	/* enable stop request */
> > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit); }
> > +
> > +static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv) {
> > +	/* remove stop request */
> > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +			   1 << priv->stm.req_bit, 0);
> > +}
> > +
> >  static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
> {
> >  	struct flexcan_regs __iomem *regs = priv->regs; @@ -940,6 +984,10 @@
> > static int flexcan_chip_start(struct net_device *dev)
> >  		reg_mcr |= FLEXCAN_MCR_FEN |
> >  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
> >  	}
> > +
> > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE)
> > +		reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> 
> Please try if you can merge this into flexcan_enable_wakeup_irq().
> If not, you can check with device_can_wakeup() as the stop parsing may fail.

I think that can't be merged into flexcan_enable_wakeup_irq() due to the FlexCAN module will not monitor the bus for wakeup event if the wakeup event arrives between flexcan_suspend() and flexcan_noirq_suspend() (between has entered stop mode and enable the wakeup irq).

I will add the check to avoid the stop mode parsing failed.

> > +
> >  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> > @@ -1244,6 +1292,58 @@ static void unregister_flexcandev(struct
> > net_device
> > *dev)
> >  	unregister_candev(dev);
> >  }
> >
> > +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *gpr_np;
> > +	struct flexcan_priv *priv;
> > +	phandle phandle;
> > +	u32 out_val[5];
> > +	int ret;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	/* stop mode property format is:
> > +	 * <&gpr req_gpr req_bit ack_gpr ack_bit>.
> > +	 */
> > +	ret = of_property_read_u32_array(np, "fsl,stop-mode", out_val, 5);
> > +	if (ret) {
> > +		dev_dbg(&pdev->dev, "no stop-mode property\n");
> > +		return ret;
> > +	}
> > +	phandle = *out_val;
> > +
> > +	gpr_np = of_find_node_by_phandle(phandle);
> > +	if (!gpr_np) {
> > +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
> > +		return PTR_ERR(gpr_np);
> > +	}
> > +
> > +	priv = netdev_priv(dev);
> > +	priv->stm.gpr = syscon_node_to_regmap(gpr_np);
> 
> Better to put node here to cover failure case?

It seems better.

Thanks for your review and then I will send V5 to fix the issue.

Best Regards,
Joakim Zhang

> Regards
> Dong Aisheng
> 
> > +	if (IS_ERR(priv->stm.gpr)) {
> > +		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
> > +		return PTR_ERR(priv->stm.gpr);
> > +	}
> > +
> > +	of_node_put(gpr_np);
> > +
> > +	priv->stm.req_gpr = out_val[1];
> > +	priv->stm.req_bit = out_val[2];
> > +	priv->stm.ack_gpr = out_val[3];
> > +	priv->stm.ack_bit = out_val[4];
> > +
> > +	dev_dbg(&pdev->dev, "gpr %s req_gpr 0x%x req_bit %u ack_gpr 0x%x
> > ack_bit %u\n",
> > +		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit,
> > priv->stm.ack_gpr,
> > +		priv->stm.ack_bit);
> > +
> > +	device_set_wakeup_capable(&pdev->dev, true);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct of_device_id flexcan_of_match[] = {
> >  	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
> >  	{ .compatible = "fsl,imx28-flexcan", .data =
> > &fsl_imx28_devtype_data, }, @@ -1396,6 +1496,12 @@ static int
> > flexcan_probe(struct platform_device
> > *pdev)
> >
> >  	devm_can_led_init(dev);
> >
> > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
> > +		err = flexcan_setup_stop_mode(pdev);
> > +		if (err)
> > +			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
> > +	}
> > +
> >  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> >  		 priv->regs, dev->irq);
> >
> > @@ -1426,9 +1532,17 @@ static int __maybe_unused
> > flexcan_suspend(struct device *device)
> >  	int err;
> >
> >  	if (netif_running(dev)) {
> > -		err = flexcan_chip_disable(priv);
> > -		if (err)
> > -			return err;
> > +		/* if wakeup is enabled, enter stop mode
> > +		 * else enter disabled mode.
> > +		 */
> > +		if (device_may_wakeup(device)) {
> > +			enable_irq_wake(dev->irq);
> > +			flexcan_enter_stop_mode(priv);
> > +		} else {
> > +			err = flexcan_chip_disable(priv);
> > +			if (err)
> > +				return err;
> > +		}
> >  		netif_stop_queue(dev);
> >  		netif_device_detach(dev);
> >  	}
> > @@ -1447,14 +1561,45 @@ static int __maybe_unused
> > flexcan_resume(struct device *device)
> >  	if (netif_running(dev)) {
> >  		netif_device_attach(dev);
> >  		netif_start_queue(dev);
> > -		err = flexcan_chip_enable(priv);
> > -		if (err)
> > -			return err;
> > +		if (device_may_wakeup(device)) {
> > +			flexcan_enable_wakeup_irq(priv, false);
> > +		} else {
> > +			err = flexcan_chip_enable(priv);
> > +			if (err)
> > +				return err;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_suspend(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device))
> > +		flexcan_enable_wakeup_irq(priv, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device)) {
> > +		disable_irq_wake(dev->irq);
> > +		flexcan_exit_stop_mode(priv);
> >  	}
> > +
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> > flexcan_resume);
> > +static const struct dev_pm_ops flexcan_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > +flexcan_noirq_resume) };
> >
> >  static struct platform_driver flexcan_driver = {
> >  	.driver = {
> > --
> > 2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Joakim Zhang]

Hi AIsheng,


> -----Original Message-----
> From: Aisheng DONG
> Sent: 2018年11月21日 21:02
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> > -----Original Message-----
> > From: Aisheng DONG
> > Sent: Wednesday, November 21, 2018 9:00 PM
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> > mkl@pengutronix.de
> > Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> >
> > This mostly looks good to me.
> > A few minor comments
> >
> 
> BTW, you should re-send the series with binding doc patch if it's still not
> merged.

It has been reviewed by: Rob Herring <robh@kernel.org>, but still has not been merged. I will re-send the series with binding doc.

Best Regards,
Joakim Zhang

> Regards
> Dong Aisheng
> 
> > > -----Original Message-----
> > > From: Joakim Zhang
> > > Sent: Wednesday, November 21, 2018 8:32 PM
> > > To: linux-can@vger.kernel.org; mkl@pengutronix.de
> > > Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> > > <linux-imx@nxp.com>; Aisheng DONG <aisheng.dong@nxp.com>; Joakim
> > Zhang
> > > <qiangqing.zhang@nxp.com>
> > > Subject: [PATCH V4 1/1] can: flexcan: add self wakeup support
> > >
> > > From: Aisheng Dong <aisheng.dong@nxp.com>
> > >
> > > If wakeup is enabled, enter stop mode, else enter disabled mode.
> > > Self wake can only work on stop mode.
> > >
> > > Starting from IMX6, the flexcan stop mode control bits is SoC
> > > specific, move it out of IP driver and parse it from devicetree.
> > >
> > > Signed-off-by: Aisheng Dong <aisheng.dong@nxp.com>
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > >  drivers/net/can/flexcan.c | 163
> > > +++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 154 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > > index 8e972ef08637..83431810316e 100644
> > > --- a/drivers/net/can/flexcan.c
> > > +++ b/drivers/net/can/flexcan.c
> > > @@ -19,11 +19,14 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/io.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regulator/consumer.h>
> > > +#include <linux/regmap.h>
> > >
> > >  #define DRV_NAME			"flexcan"
> > >
> > > @@ -131,7 +134,8 @@
> > >  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> > > FLEXCAN_ESR_ALL_INT \
> > >  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> > > -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> > > +	FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> > > +	FLEXCAN_ESR_WAK_INT)
> > >
> > >  /* FLEXCAN interrupt flag register (IFLAG) bits */
> > >  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -190,6
> > > +194,7
> > @@
> > >  #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp
> > > based offloading */
> > >  #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt
> > for
> > > error passive */
> > >  #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE
> > > register access */
> > > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop
> > > mode to support wakeup */
> > >
> > >  /* Structure of the message buffer */  struct flexcan_mb { @@
> > > -254,6
> > > +259,14 @@ struct flexcan_devtype_data {
> > >  	u32 quirks;		/* quirks needed for different IP cores */
> > >  };
> > >
> > > +struct flexcan_stop_mode {
> > > +	struct regmap *gpr;
> > > +	u8 req_gpr;
> > > +	u8 req_bit;
> > > +	u8 ack_gpr;
> > > +	u8 ack_bit;
> > > +};
> > > +
> > >  struct flexcan_priv {
> > >  	struct can_priv can;
> > >  	struct can_rx_offload offload;
> > > @@ -270,6 +283,7 @@ struct flexcan_priv {
> > >  	struct clk *clk_per;
> > >  	const struct flexcan_devtype_data *devtype_data;
> > >  	struct regulator *reg_xceiver;
> > > +	struct flexcan_stop_mode stm;
> > >
> > >  	/* Read and Write APIs */
> > >  	u32 (*read)(void __iomem *addr);
> > > @@ -293,7 +307,8 @@ static const struct flexcan_devtype_data
> > > fsl_imx28_devtype_data = {
> > >
> > >  static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> > >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> > > FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> > > -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > > FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> > > +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> > > FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > > +		FLEXCAN_QUIRK_SETUP_STOP_MODE,
> > >  };
> > >
> > >  static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
> > > @@
> > > -353,6 +368,35 @@ static inline void flexcan_write_le(u32 val, void
> > > __iomem
> > > *addr)
> > >  	iowrite32(val, addr);
> > >  }
> > >
> > > +static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv,
> > > +bool
> > > +enable) {
> > > +	struct flexcan_regs __iomem *regs = priv->regs;
> > > +	u32 reg_mcr;
> > > +
> > > +	reg_mcr = priv->read(&regs->mcr);
> > > +
> > > +	if (enable)
> > > +		reg_mcr |= FLEXCAN_MCR_WAK_MSK;
> > > +	else
> > > +		reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
> > > +
> > > +	priv->write(reg_mcr, &regs->mcr);
> > > +}
> > > +
> > > +static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv) {
> > > +	/* enable stop request */
> > > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > > +			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit); }
> > > +
> > > +static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv) {
> > > +	/* remove stop request */
> > > +	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > > +			   1 << priv->stm.req_bit, 0);
> > > +}
> > > +
> > >  static inline void flexcan_error_irq_enable(const struct
> > > flexcan_priv *priv)
> > {
> > >  	struct flexcan_regs __iomem *regs = priv->regs; @@ -940,6 +984,10
> > > @@ static int flexcan_chip_start(struct net_device *dev)
> > >  		reg_mcr |= FLEXCAN_MCR_FEN |
> > >  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
> > >  	}
> > > +
> > > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE)
> > > +		reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> >
> > Please try if you can merge this into flexcan_enable_wakeup_irq().
> > If not, you can check with device_can_wakeup() as the stop parsing may fail.
> >
> > > +
> > >  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
> > >  	priv->write(reg_mcr, &regs->mcr);
> > >
> > > @@ -1244,6 +1292,58 @@ static void unregister_flexcandev(struct
> > > net_device
> > > *dev)
> > >  	unregister_candev(dev);
> > >  }
> > >
> > > +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> > > +	struct net_device *dev = platform_get_drvdata(pdev);
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct device_node *gpr_np;
> > > +	struct flexcan_priv *priv;
> > > +	phandle phandle;
> > > +	u32 out_val[5];
> > > +	int ret;
> > > +
> > > +	if (!np)
> > > +		return -EINVAL;
> > > +
> > > +	/* stop mode property format is:
> > > +	 * <&gpr req_gpr req_bit ack_gpr ack_bit>.
> > > +	 */
> > > +	ret = of_property_read_u32_array(np, "fsl,stop-mode", out_val, 5);
> > > +	if (ret) {
> > > +		dev_dbg(&pdev->dev, "no stop-mode property\n");
> > > +		return ret;
> > > +	}
> > > +	phandle = *out_val;
> > > +
> > > +	gpr_np = of_find_node_by_phandle(phandle);
> > > +	if (!gpr_np) {
> > > +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
> > > +		return PTR_ERR(gpr_np);
> > > +	}
> > > +
> > > +	priv = netdev_priv(dev);
> > > +	priv->stm.gpr = syscon_node_to_regmap(gpr_np);
> >
> > Better to put node here to cover failure case?
> >
> > Regards
> > Dong Aisheng
> >
> > > +	if (IS_ERR(priv->stm.gpr)) {
> > > +		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
> > > +		return PTR_ERR(priv->stm.gpr);
> > > +	}
> > > +
> > > +	of_node_put(gpr_np);
> > > +
> > > +	priv->stm.req_gpr = out_val[1];
> > > +	priv->stm.req_bit = out_val[2];
> > > +	priv->stm.ack_gpr = out_val[3];
> > > +	priv->stm.ack_bit = out_val[4];
> > > +
> > > +	dev_dbg(&pdev->dev, "gpr %s req_gpr 0x%x req_bit %u ack_gpr 0x%x
> > > ack_bit %u\n",
> > > +		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit,
> > > priv->stm.ack_gpr,
> > > +		priv->stm.ack_bit);
> > > +
> > > +	device_set_wakeup_capable(&pdev->dev, true);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct of_device_id flexcan_of_match[] = {
> > >  	{ .compatible = "fsl,imx6q-flexcan", .data =
> &fsl_imx6q_devtype_data, },
> > >  	{ .compatible = "fsl,imx28-flexcan", .data =
> > > &fsl_imx28_devtype_data, }, @@ -1396,6 +1496,12 @@ static int
> > > flexcan_probe(struct platform_device
> > > *pdev)
> > >
> > >  	devm_can_led_init(dev);
> > >
> > > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
> > > +		err = flexcan_setup_stop_mode(pdev);
> > > +		if (err)
> > > +			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
> > > +	}
> > > +
> > >  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> > >  		 priv->regs, dev->irq);
> > >
> > > @@ -1426,9 +1532,17 @@ static int __maybe_unused
> > > flexcan_suspend(struct device *device)
> > >  	int err;
> > >
> > >  	if (netif_running(dev)) {
> > > -		err = flexcan_chip_disable(priv);
> > > -		if (err)
> > > -			return err;
> > > +		/* if wakeup is enabled, enter stop mode
> > > +		 * else enter disabled mode.
> > > +		 */
> > > +		if (device_may_wakeup(device)) {
> > > +			enable_irq_wake(dev->irq);
> > > +			flexcan_enter_stop_mode(priv);
> > > +		} else {
> > > +			err = flexcan_chip_disable(priv);
> > > +			if (err)
> > > +				return err;
> > > +		}
> > >  		netif_stop_queue(dev);
> > >  		netif_device_detach(dev);
> > >  	}
> > > @@ -1447,14 +1561,45 @@ static int __maybe_unused
> > > flexcan_resume(struct device *device)
> > >  	if (netif_running(dev)) {
> > >  		netif_device_attach(dev);
> > >  		netif_start_queue(dev);
> > > -		err = flexcan_chip_enable(priv);
> > > -		if (err)
> > > -			return err;
> > > +		if (device_may_wakeup(device)) {
> > > +			flexcan_enable_wakeup_irq(priv, false);
> > > +		} else {
> > > +			err = flexcan_chip_enable(priv);
> > > +			if (err)
> > > +				return err;
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused flexcan_noirq_suspend(struct device
> > > +*device) {
> > > +	struct net_device *dev = dev_get_drvdata(device);
> > > +	struct flexcan_priv *priv = netdev_priv(dev);
> > > +
> > > +	if (netif_running(dev) && device_may_wakeup(device))
> > > +		flexcan_enable_wakeup_irq(priv, true);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> > > +	struct net_device *dev = dev_get_drvdata(device);
> > > +	struct flexcan_priv *priv = netdev_priv(dev);
> > > +
> > > +	if (netif_running(dev) && device_may_wakeup(device)) {
> > > +		disable_irq_wake(dev->irq);
> > > +		flexcan_exit_stop_mode(priv);
> > >  	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> > > flexcan_resume);
> > > +static const struct dev_pm_ops flexcan_pm_ops = {
> > > +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > > +flexcan_noirq_resume) };
> > >
> > >  static struct platform_driver flexcan_driver = {
> > >  	.driver = {
> > > --
> > > 2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Aisheng DONG]

[...]

> +
> +static int __maybe_unused flexcan_noirq_suspend(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	if (netif_running(dev) && device_may_wakeup(device))
> +		flexcan_enable_wakeup_irq(priv, true);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	if (netif_running(dev) && device_may_wakeup(device)) {
> +		disable_irq_wake(dev->irq);

A bit more thinking:
Can we put flexcan_enable_wakeup_irq(priv, false) here and move disable_irq_wake
to resume function?
Then it looks better on pairs for those functions.

I'm not sure if irq will be lost or we may even not need wakeup irq.
Please help check it.

Regards
Dong Aisheng

> +		flexcan_exit_stop_mode(priv);
>  	}
> +
>  	return 0;
>  }
> 
> -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> flexcan_resume);
> +static const struct dev_pm_ops flexcan_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> +flexcan_noirq_resume) };
> 
>  static struct platform_driver flexcan_driver = {
>  	.driver = {
> --
> 2.17.1

RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Joakim Zhang]

Hi Aisheng,


> -----Original Message-----
> From: Aisheng DONG
> Sent: 2018年11月22日 10:31
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> [...]
> 
> > +
> > +static int __maybe_unused flexcan_noirq_suspend(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device))
> > +		flexcan_enable_wakeup_irq(priv, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device)) {
> > +		disable_irq_wake(dev->irq);
> 
> A bit more thinking:
> Can we put flexcan_enable_wakeup_irq(priv, false) here and move
> disable_irq_wake to resume function?
> Then it looks better on pairs for those functions.
> 
> I'm not sure if irq will be lost or we may even not need wakeup irq.
> Please help check it.

1. I have try that flexcan_enable_wakeup_irq(priv, false) can move into flexcan_noirq_resume() and disable_irq_wake() can move into flexcan_resume().
  If you think that is better, I will modify it.

2. If we don't disable wakeup irq after it exits stop mode, it will continue enter wakeup interrupt handle function when the wakeup event arriving during suspend after
  the system has done once more suspend/resume.

Best Regards,
Joakim Zhang

> Regards
> Dong Aisheng
> 
> > +		flexcan_exit_stop_mode(priv);
> >  	}
> > +
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> > flexcan_resume);
> > +static const struct dev_pm_ops flexcan_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > +flexcan_noirq_resume) };
> >
> >  static struct platform_driver flexcan_driver = {
> >  	.driver = {
> > --
> > 2.17.1