[PATCH V3 0/1] can: flexcan: Add CAN self wakeup support

[PATCH V3 0/1] can: flexcan: Add CAN self wakeup support

[Joakim Zhang]

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

There is a issue in V2 is that the system can be wakeuped only when system
is totally suspend, V3 has fixed this issue.

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.

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

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

-- 
2.17.1

[PATCH V3 1/1] can: flexcan: Add self wakeup support

[Joakim Zhang]

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

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 3813f6708201..d21d0db8c83e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -19,12 +19,17 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.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/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #define DRV_NAME			"flexcan"
 
@@ -132,7 +137,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 */
@@ -191,6 +197,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 {
@@ -255,6 +262,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;
@@ -272,6 +287,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);
@@ -295,7 +311,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 = {
@@ -392,6 +409,40 @@ static void flexcan_clks_disable(const struct flexcan_priv *priv)
 	clk_disable_unprepare(priv->clk_per);
 }
 
+static void flexcan_wake_mask_enable(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_mcr;
+
+	reg_mcr = priv->read(&regs->mcr);
+	reg_mcr |= FLEXCAN_MCR_WAK_MSK;
+	priv->write(reg_mcr, &regs->mcr);
+}
+
+static void flexcan_wake_mask_disable(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_mcr;
+
+	reg_mcr = priv->read(&regs->mcr);
+	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 int flexcan_transceiver_enable(const struct flexcan_priv *priv)
 {
 	if (!priv->reg_xceiver)
@@ -955,6 +1006,10 @@ static int flexcan_chip_start(struct net_device *dev)
 		reg_mcr |= FLEXCAN_MCR_FEN |
 			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
 	}
+
+	/* enable self wakeup */
+	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
+
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	priv->write(reg_mcr, &regs->mcr);
 
@@ -1240,6 +1295,60 @@ 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 *node;
+	struct flexcan_priv *priv;
+	bool wakeup = true;
+	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;
+
+	node = of_find_node_by_phandle(phandle);
+	if (!node) {
+		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
+		return PTR_ERR(node);
+	}
+
+	priv = netdev_priv(dev);
+	priv->stm.gpr = syscon_node_to_regmap(node);
+	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(node);
+
+	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",
+			node->full_name, priv->stm.req_gpr,
+			priv->stm.req_bit, priv->stm.ack_gpr,
+			priv->stm.ack_bit);
+
+	device_set_wakeup_capable(&pdev->dev, wakeup);
+
+	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, },
@@ -1400,6 +1509,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");
+	}
+
 	pm_runtime_put(&pdev->dev);
 
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
@@ -1437,10 +1552,18 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 	int err = 0;
 
 	if (netif_running(dev)) {
-		err = flexcan_chip_disable(priv);
-		if (err)
-			return err;
-		err = pm_runtime_force_suspend(device);
+		/* 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;
+			err = pm_runtime_force_suspend(device);
+		}
 
 		netif_stop_queue(dev);
 		netif_device_detach(dev);
@@ -1461,10 +1584,14 @@ static int __maybe_unused flexcan_resume(struct device *device)
 		netif_device_attach(dev);
 		netif_start_queue(dev);
 
-		err = pm_runtime_force_resume(device);
-		if (err)
-			return err;
-		err = flexcan_chip_enable(priv);
+		if (device_may_wakeup(device)) {
+			flexcan_wake_mask_disable(priv);
+		} else {
+			err = pm_runtime_force_resume(device);
+			if (err)
+				return err;
+			err = flexcan_chip_enable(priv);
+		}
 	}
 	return err;
 }
@@ -1489,10 +1616,34 @@ static int __maybe_unused flexcan_runtime_resume(struct device *device)
 	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_wake_mask_enable(priv);
+
+	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 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)
+	SET_RUNTIME_PM_OPS(flexcan_runtime_suspend, flexcan_runtime_resume, NULL)
 };
 
 static struct platform_driver flexcan_driver = {
-- 
2.17.1

RE: [PATCH V3 1/1] can: flexcan: Add self wakeup support

[Aisheng DONG]

> -----Original Message-----
> From: Joakim Zhang
[...]
> 
> 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: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 175
> +++++++++++++++++++++++++++++++++++---
>  1 file changed, 163 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 3813f6708201..d21d0db8c83e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -19,12 +19,17 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>

This two lines seems not neccesary

> +#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/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> 
>  #define DRV_NAME			"flexcan"
> 
> @@ -132,7 +137,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 */ @@ -191,6 +197,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 {
> @@ -255,6 +262,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;
> @@ -272,6 +287,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);
> @@ -295,7 +311,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 = { @@
> -392,6 +409,40 @@ static void flexcan_clks_disable(const struct flexcan_priv
> *priv)
>  	clk_disable_unprepare(priv->clk_per);
>  }
> 
> +static void flexcan_wake_mask_enable(struct flexcan_priv *priv) {
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_mcr;
> +
> +	reg_mcr = priv->read(&regs->mcr);
> +	reg_mcr |= FLEXCAN_MCR_WAK_MSK;
> +	priv->write(reg_mcr, &regs->mcr);
> +}
> +
> +static void flexcan_wake_mask_disable(struct flexcan_priv *priv) {
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_mcr;
> +
> +	reg_mcr = priv->read(&regs->mcr);
> +	reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
> +	priv->write(reg_mcr, &regs->mcr);
> +}
> +

Please combine those two functions into one, also improve the name a bit.
e.g.
flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)

> +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 int flexcan_transceiver_enable(const struct flexcan_priv *priv)  {
>  	if (!priv->reg_xceiver)
> @@ -955,6 +1006,10 @@ static int flexcan_chip_start(struct net_device *dev)
>  		reg_mcr |= FLEXCAN_MCR_FEN |
>  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
>  	}
> +
> +	/* enable self wakeup */
> +	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> +

Should we check before setting as we already have a SETUP_STOP_MODE quirk?

>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	priv->write(reg_mcr, &regs->mcr);
> 
> @@ -1240,6 +1295,60 @@ 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 *node;

Probably gpr_np is better.

> +	struct flexcan_priv *priv;
> +	bool wakeup = true;
> +	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;
> +
> +	node = of_find_node_by_phandle(phandle);
> +	if (!node) {
> +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
> +		return PTR_ERR(node);
> +	}
> +
> +	priv = netdev_priv(dev);
> +	priv->stm.gpr = syscon_node_to_regmap(node);
> +	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(node);
> +
> +	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",
> +			node->full_name, priv->stm.req_gpr,
> +			priv->stm.req_bit, priv->stm.ack_gpr,
> +			priv->stm.ack_bit);
> +
> +	device_set_wakeup_capable(&pdev->dev, wakeup);

If 'wakeup' variable is not changeable, then not need use it.

> +
> +	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, },
> @@ -1400,6 +1509,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");
> +	}
> +
>  	pm_runtime_put(&pdev->dev);
> 
>  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n", @@
> -1437,10 +1552,18 @@ static int __maybe_unused flexcan_suspend(struct
> device *device)
>  	int err = 0;
> 
>  	if (netif_running(dev)) {
> -		err = flexcan_chip_disable(priv);
> -		if (err)
> -			return err;
> -		err = pm_runtime_force_suspend(device);
> +		/* 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;
> +			err = pm_runtime_force_suspend(device);

This is mixed with runtime pm.
I would suggestion you first upstream wakeup support.
Then runtime pm later separately.

Regards
Dong Aisheng

> +		}
> 
>  		netif_stop_queue(dev);
>  		netif_device_detach(dev);
> @@ -1461,10 +1584,14 @@ static int __maybe_unused
> flexcan_resume(struct device *device)
>  		netif_device_attach(dev);
>  		netif_start_queue(dev);
> 
> -		err = pm_runtime_force_resume(device);
> -		if (err)
> -			return err;
> -		err = flexcan_chip_enable(priv);
> +		if (device_may_wakeup(device)) {
> +			flexcan_wake_mask_disable(priv);
> +		} else {
> +			err = pm_runtime_force_resume(device);
> +			if (err)
> +				return err;
> +			err = flexcan_chip_enable(priv);
> +		}
>  	}
>  	return err;
>  }
> @@ -1489,10 +1616,34 @@ static int __maybe_unused
> flexcan_runtime_resume(struct device *device)
>  	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_wake_mask_enable(priv);
> +
> +	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 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)
> +	SET_RUNTIME_PM_OPS(flexcan_runtime_suspend,
> flexcan_runtime_resume,
> +NULL)
>  };
> 
>  static struct platform_driver flexcan_driver = {
> --
> 2.17.1

Re: [PATCH V3 1/1] can: flexcan: Add self wakeup support

[Marc Kleine-Budde]

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

On 11/21/18 4:34 AM, Aisheng DONG wrote:
>> -----Original Message-----
>> From: Joakim Zhang
> [...]
>>
>> 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: Dong Aisheng <aisheng.dong@nxp.com>
>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> ---
>>  drivers/net/can/flexcan.c | 175
>> +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 163 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
>> 3813f6708201..d21d0db8c83e 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -19,12 +19,17 @@
>>  #include <linux/delay.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
> 
> This two lines seems not neccesary
> 
>> +#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/pm_runtime.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <linux/regmap.h>
>>
>>  #define DRV_NAME			"flexcan"
>>
>> @@ -132,7 +137,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 */ @@ -191,6 +197,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 {
>> @@ -255,6 +262,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;
>> @@ -272,6 +287,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);
>> @@ -295,7 +311,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 = { @@
>> -392,6 +409,40 @@ static void flexcan_clks_disable(const struct flexcan_priv
>> *priv)
>>  	clk_disable_unprepare(priv->clk_per);
>>  }
>>
>> +static void flexcan_wake_mask_enable(struct flexcan_priv *priv) {
>> +	struct flexcan_regs __iomem *regs = priv->regs;
>> +	u32 reg_mcr;
>> +
>> +	reg_mcr = priv->read(&regs->mcr);
>> +	reg_mcr |= FLEXCAN_MCR_WAK_MSK;
>> +	priv->write(reg_mcr, &regs->mcr);
>> +}
>> +
>> +static void flexcan_wake_mask_disable(struct flexcan_priv *priv) {
>> +	struct flexcan_regs __iomem *regs = priv->regs;
>> +	u32 reg_mcr;
>> +
>> +	reg_mcr = priv->read(&regs->mcr);
>> +	reg_mcr &= ~FLEXCAN_MCR_WAK_MSK;
>> +	priv->write(reg_mcr, &regs->mcr);
>> +}
>> +
> 
> Please combine those two functions into one, also improve the name a bit.
> e.g.
> flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
> 
>> +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 int flexcan_transceiver_enable(const struct flexcan_priv *priv)  {
>>  	if (!priv->reg_xceiver)
>> @@ -955,6 +1006,10 @@ static int flexcan_chip_start(struct net_device *dev)
>>  		reg_mcr |= FLEXCAN_MCR_FEN |
>>  			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
>>  	}
>> +
>> +	/* enable self wakeup */
>> +	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
>> +
> 
> Should we check before setting as we already have a SETUP_STOP_MODE quirk?
> 
>>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>>  	priv->write(reg_mcr, &regs->mcr);
>>
>> @@ -1240,6 +1295,60 @@ 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 *node;
> 
> Probably gpr_np is better.
> 
>> +	struct flexcan_priv *priv;
>> +	bool wakeup = true;
>> +	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;
>> +
>> +	node = of_find_node_by_phandle(phandle);
>> +	if (!node) {
>> +		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
>> +		return PTR_ERR(node);
>> +	}
>> +
>> +	priv = netdev_priv(dev);
>> +	priv->stm.gpr = syscon_node_to_regmap(node);
>> +	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(node);
>> +
>> +	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",
>> +			node->full_name, priv->stm.req_gpr,
>> +			priv->stm.req_bit, priv->stm.ack_gpr,
>> +			priv->stm.ack_bit);
>> +
>> +	device_set_wakeup_capable(&pdev->dev, wakeup);
> 
> If 'wakeup' variable is not changeable, then not need use it.
> 
>> +
>> +	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, },
>> @@ -1400,6 +1509,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");
>> +	}
>> +
>>  	pm_runtime_put(&pdev->dev);
>>
>>  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n", @@
>> -1437,10 +1552,18 @@ static int __maybe_unused flexcan_suspend(struct
>> device *device)
>>  	int err = 0;
>>
>>  	if (netif_running(dev)) {
>> -		err = flexcan_chip_disable(priv);
>> -		if (err)
>> -			return err;
>> -		err = pm_runtime_force_suspend(device);
>> +		/* 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;
>> +			err = pm_runtime_force_suspend(device);
> 
> This is mixed with runtime pm.
> I would suggestion you first upstream wakeup support.
> Then runtime pm later separately.

In fact this patch is based on the patch "can: flexcan: Implement CAN
Runtime PM", which is still under review, too.

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 --]

RE: [PATCH V3 1/1] can: flexcan: Add self wakeup support

[Aisheng DONG]

[...]
> >> @@
> >> -1437,10 +1552,18 @@ static int __maybe_unused flexcan_suspend(struct
> >> device *device)
> >>  	int err = 0;
> >>
> >>  	if (netif_running(dev)) {
> >> -		err = flexcan_chip_disable(priv);
> >> -		if (err)
> >> -			return err;
> >> -		err = pm_runtime_force_suspend(device);
> >> +		/* 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;
> >> +			err = pm_runtime_force_suspend(device);
> >
> > This is mixed with runtime pm.
> > I would suggestion you first upstream wakeup support.
> > Then runtime pm later separately.
> 
> In fact this patch is based on the patch "can: flexcan: Implement CAN Runtime
> PM", which is still under review, too.

Yes, please ignore rpm patch first.
This patch will be re-made and not depends on rpm for easy review.
After wakeup is ready, rpm patch will rebase on it.

Regards
Dong Aisheng

> 
> 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   |

Re: [PATCH V3 1/1] can: flexcan: Add self wakeup support

[Marc Kleine-Budde]

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

On 11/22/18 12:01 PM, Aisheng DONG wrote:
> [...]
>>>> @@
>>>> -1437,10 +1552,18 @@ static int __maybe_unused flexcan_suspend(struct
>>>> device *device)
>>>>  	int err = 0;
>>>>
>>>>  	if (netif_running(dev)) {
>>>> -		err = flexcan_chip_disable(priv);
>>>> -		if (err)
>>>> -			return err;
>>>> -		err = pm_runtime_force_suspend(device);
>>>> +		/* 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;
>>>> +			err = pm_runtime_force_suspend(device);
>>>
>>> This is mixed with runtime pm.
>>> I would suggestion you first upstream wakeup support.
>>> Then runtime pm later separately.
>>
>> In fact this patch is based on the patch "can: flexcan: Implement CAN Runtime
>> PM", which is still under review, too.
> 
> Yes, please ignore rpm patch first.
> This patch will be re-made and not depends on rpm for easy review.
> After wakeup is ready, rpm patch will rebase on it.

ok.

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 --]