[PATCH V2 0/4] can: flexcan: fixes for stop mode

[PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

Hi Sean,
	Could you help check the patch set? With your suggestions, I
have cooked a patch to exit stop mode during probe stage.

	IMHO, I think this patch is unneed, now in flexcan driver,
enter stop mode when suspend, and then exit stop mode when resume.
AFAIK, as long as flexcan_suspend has been called, flexcan_resume will
be called, unless the system hang during suspend/resume. If so, only
code reset can activate OS again. Could you please tell me how does CAN
stucked in stop mode at your side?

	If indeed need, Why not check it in flexcan_open? Once we find
CAN is stuckd in stop mode, users can down->up CAN interface to get out of
stop mode. I think this could be more reasonable.

Joakim Zhang

Joakim Zhang (3):
  can: flexcan: try to exit stop mode during probe stage
  can: flexcan: change the way of stop mode acknowledgment
  can: flexcan: add LPSR mode support

Sean Nyekjaer (1):
  can: flexcan: fix deadlock when using self wakeup

 drivers/net/can/flexcan.c | 132 +++++++++++++++++++++++---------------
 1 file changed, 80 insertions(+), 52 deletions(-)

-- 
2.17.1

[PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

From: Sean Nyekjaer <sean@geanix.com>

When suspending, when there is still can traffic on the interfaces the
flexcan immediately wakes the platform again. As it should :-). But it
throws this error msg:
[ 3169.378661] PM: noirq suspend of devices failed

On the way down to suspend the interface that throws the error message does
call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
flexcan_enter_stop_mode is called, but on the way out of suspend the driver
only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
current driver it can't recover from this even with a soft reboot, it requires
a hard reboot.

This patch can fix deadlock when using self wakeup, it happenes to be
able to fix another issue that frames out-of-order in first IRQ handler
run after wakeup.

In wakeup case, after system resume, frames received out-of-order in
first IRQ handler, the problem is wakeup latency from frame reception to
IRQ handler is much bigger than the counter overflow. This means it's
impossible to sort the CAN frames by timestamp. The reason is that controller
exits stop mode during noirq resume, then it can receive the frame immediately.
If noirq reusme stage consumes much time, it will extend interrupt response
time. So exit stop mode during resume stage instead of noirq resume can
fix this issue.

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
------
ChangeLog:
	V1->V2: no change.
---
 drivers/net/can/flexcan.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 2efa06119f68..2297663cacb2 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -134,8 +134,7 @@
 	(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_WAK_INT)
+	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
@@ -960,6 +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 	reg_esr = priv->read(&regs->esr);
 
+	/* ACK wakeup interrupt */
+	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
+		handled = IRQ_HANDLED;
+		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
+	}
+
 	/* ACK all bus error and state change IRQ sources */
 	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
 		handled = IRQ_HANDLED;
@@ -1722,6 +1727,9 @@ static int __maybe_unused flexcan_resume(struct device *device)
 		netif_start_queue(dev);
 		if (device_may_wakeup(device)) {
 			disable_irq_wake(dev->irq);
+			err = flexcan_exit_stop_mode(priv);
+			if (err)
+				return err;
 		} else {
 			err = pm_runtime_force_resume(device);
 			if (err)
@@ -1767,14 +1775,9 @@ 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);
-	int err;
 
-	if (netif_running(dev) && device_may_wakeup(device)) {
+	if (netif_running(dev) && device_may_wakeup(device))
 		flexcan_enable_wakeup_irq(priv, false);
-		err = flexcan_exit_stop_mode(priv);
-		if (err)
-			return err;
-	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

CAN controller could be stucked in stop mode once it enters stop mode
when suspend, and then it fails to exit stop mode when resume. Only code
reset can get CAN out of stop mode, so add stop mode remove request
during probe stage for other methods(soft reset from chip level,
unbind/bind driver, etc) to let CAN active again. MCR[LPMACK] will be
checked when enable CAN in register_flexcandev().

Suggested-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
------
ChangeLog:
	V1->V2: new add.
---
 drivers/net/can/flexcan.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 2297663cacb2..5d5ed28d3005 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -449,6 +449,13 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 	return 0;
 }
 
+static void flexcan_try_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;
@@ -1649,6 +1656,21 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->devtype_data = devtype_data;
 	priv->reg_xceiver = reg_xceiver;
 
+	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");
+
+		/* CAN controller could be stucked in stop mode once it enters
+		 * stop mode when suspend, and then it fails to exit stop
+		 * mode when resume. Only code reset can get CAN out of stop
+		 * mode, so add stop mode remove request here for other methods
+		 * (soft reset, bind, etc) to let CAN active again. MCR[LPMACK]
+		 * will be checked when enable CAN in register_flexcandev().
+		 */
+		flexcan_try_exit_stop_mode(priv);
+	}
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
@@ -1661,12 +1683,6 @@ 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");
-	}
-
 	return 0;
 
  failed_register:
-- 
2.17.1

[PATCH V2 3/4] can: flexcan: change the way of stop mode acknowledgment

[Joakim Zhang]

Stop mode is entered when Stop mode is requested at chip level and
MCR[LPM_ACK] is asserted by the FlexCAN.

Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
acknowledgment, not the acknowledgment from chip level which is used
for glitch filter.

Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
------
ChangeLog:
	V1->V2: no change.
---
 drivers/net/can/flexcan.c | 64 ++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 5d5ed28d3005..d178146b3da5 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -388,6 +388,34 @@ static struct flexcan_mb __iomem *flexcan_get_mb(const struct flexcan_priv *priv
 		(&priv->regs->mb[bank][priv->mb_size * mb_index]);
 }
 
+static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
+
+	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		udelay(10);
+
+	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
+
+	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		udelay(10);
+
+	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
@@ -406,7 +434,6 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int ackval;
 	u32 reg_mcr;
 
 	reg_mcr = priv->read(&regs->mcr);
@@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
 
 	/* get stop acknowledgment */
-	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
-				     ackval, ackval & (1 << priv->stm.ack_bit),
-				     0, FLEXCAN_TIMEOUT_US))
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_enter_low_power_ack(priv);
 }
 
 static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int ackval;
 	u32 reg_mcr;
 
 	/* remove stop request */
 	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
 			   1 << priv->stm.req_bit, 0);
 
-	/* get stop acknowledgment */
-	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
-				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
-				     0, FLEXCAN_TIMEOUT_US))
-		return -ETIMEDOUT;
-
 	reg_mcr = priv->read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
 	priv->write(reg_mcr, &regs->mcr);
 
-	return 0;
+	/* get stop acknowledgment */
+	return flexcan_exit_low_power_ack(priv);
 }
 
 static void flexcan_try_exit_stop_mode(struct flexcan_priv *priv)
@@ -512,39 +528,25 @@ static inline int flexcan_transceiver_disable(const struct flexcan_priv *priv)
 static int flexcan_chip_enable(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
 	u32 reg;
 
 	reg = priv->read(&regs->mcr);
 	reg &= ~FLEXCAN_MCR_MDIS;
 	priv->write(reg, &regs->mcr);
 
-	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		udelay(10);
-
-	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_exit_low_power_ack(priv);
 }
 
 static int flexcan_chip_disable(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
 	u32 reg;
 
 	reg = priv->read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS;
 	priv->write(reg, &regs->mcr);
 
-	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		udelay(10);
-
-	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_enter_low_power_ack(priv);
 }
 
 static int flexcan_chip_freeze(struct flexcan_priv *priv)
-- 
2.17.1

[PATCH V2 4/4] can: flexcan: add LPSR mode support

[Joakim Zhang]

For i.MX7D LPSR mode, the controller will lost power and got the
configuration state lost after system resume back. (coming i.MX8QM/QXP
will also completely power off the domain, the controller state will be
lost and needs restore).
So we need to set pinctrl state again and re-start chip to do
re-configuration after resume.

For wakeup case, it should not set pinctrl to sleep state by
pinctrl_pm_select_sleep_state.
For interface is not up before suspend case, we don't need
re-configure as it will be configured by user later by interface up.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
------
ChangeLog:
	V1->V2: no change.
---
 drivers/net/can/flexcan.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index d178146b3da5..d1509cffdd24 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/regmap.h>
 
 #define DRV_NAME			"flexcan"
@@ -1707,7 +1708,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 = 0;
+	int err;
 
 	if (netif_running(dev)) {
 		/* if wakeup is enabled, enter stop mode
@@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 			if (err)
 				return err;
 		} else {
-			err = flexcan_chip_disable(priv);
+			flexcan_chip_stop(dev);
+
+			err = pm_runtime_force_suspend(device);
 			if (err)
 				return err;
 
-			err = pm_runtime_force_suspend(device);
+			pinctrl_pm_select_sleep_state(device);
 		}
 		netif_stop_queue(dev);
 		netif_device_detach(dev);
 	}
 	priv->can.state = CAN_STATE_SLEEPING;
 
-	return err;
+	return 0;
 }
 
 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 = 0;
+	int err;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 	if (netif_running(dev)) {
@@ -1749,15 +1752,19 @@ static int __maybe_unused flexcan_resume(struct device *device)
 			if (err)
 				return err;
 		} else {
+			pinctrl_pm_select_default_state(device);
+
 			err = pm_runtime_force_resume(device);
 			if (err)
 				return err;
 
-			err = flexcan_chip_enable(priv);
+			err = flexcan_chip_start(dev);
+			if (err)
+				return err;
 		}
 	}
 
-	return err;
+	return 0;
 }
 
 static int __maybe_unused flexcan_runtime_suspend(struct device *device)
-- 
2.17.1

Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Sean Nyekjaer]

On 27/11/2019 06.56, Joakim Zhang wrote:
> 	Could you help check the patch set? With your suggestions, I
> have cooked a patch to exit stop mode during probe stage.
> 
> 	IMHO, I think this patch is unneed, now in flexcan driver,
> enter stop mode when suspend, and then exit stop mode when resume.
> AFAIK, as long as flexcan_suspend has been called, flexcan_resume will
> be called, unless the system hang during suspend/resume. If so, only
> code reset can activate OS again. Could you please tell me how does CAN
> stucked in stop mode at your side?

Hi Joakim,

Thanks I'll test this :-)
Guess I will have do some hacking to get it stuck in stop mode.

We have a lot of devices in the field that doesn't have:
"can: flexcan: fix deadlock when using self wakeup"

And they have traffic on both CAN interfaces, that way it's quite easy 
to get them stuck in stop mode.

/Sean

Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Sean Nyekjaer]

On 27/11/2019 07.12, Sean Nyekjaer wrote:
> 
> 
> On 27/11/2019 06.56, Joakim Zhang wrote:
>>     Could you help check the patch set? With your suggestions, I
>> have cooked a patch to exit stop mode during probe stage.
>>
>>     IMHO, I think this patch is unneed, now in flexcan driver,
>> enter stop mode when suspend, and then exit stop mode when resume.
>> AFAIK, as long as flexcan_suspend has been called, flexcan_resume will
>> be called, unless the system hang during suspend/resume. If so, only
>> code reset can activate OS again. Could you please tell me how does CAN
>> stucked in stop mode at your side?
> 
> Hi Joakim,
> 
> Thanks I'll test this :-)
> Guess I will have do some hacking to get it stuck in stop mode.
> 
> We have a lot of devices in the field that doesn't have:
> "can: flexcan: fix deadlock when using self wakeup"
> 
> And they have traffic on both CAN interfaces, that way it's quite easy 
> to get them stuck in stop mode.
> 
> /Sean

Hi Joakim,

I have been testing this.
I have a hacked version of the driver that calls 
flexcan_enter_stop_mode() as the last step in the probe function.

First insert of flexcan.ko when stop mode is activated:
flexcan 2090000.flexcan: Linked as a consumer to regulator.4

flexcan 2090000.flexcan: registering netdev failed

flexcan 2090000.flexcan: Dropping the link to regulator.4

flexcan: probe of 2090000.flexcan failed with error -110

flexcan 2094000.flexcan: Linked as a consumer to regulator.4

flexcan 2094000.flexcan: registering netdev failed

flexcan 2094000.flexcan: Dropping the link to regulator.4

flexcan: probe of 2094000.flexcan failed with error -110


When I insert a flexcan.ko with the patch
"can: flexcan: try to exit stop mode during probe stage":
flexcan 2090000.flexcan: Linked as a consumer to regulator.4

flexcan 2090000.flexcan: Unbalanced pm_runtime_enable!

flexcan 2094000.flexcan: Linked as a consumer to regulator.4

flexcan 2094000.flexcan: Unbalanced pm_runtime_enable!

I works as I expected but, I think we need to do some pm_runtime cleanup 
when bailing with error -110.
Anyways it works great, thanks for your work on this.

/Sean

RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年11月27日 16:13
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> 
> 
> 
> On 27/11/2019 07.12, Sean Nyekjaer wrote:
> >
> >
> > On 27/11/2019 06.56, Joakim Zhang wrote:
> >>     Could you help check the patch set? With your suggestions, I have
> >> cooked a patch to exit stop mode during probe stage.
> >>
> >>     IMHO, I think this patch is unneed, now in flexcan driver, enter
> >> stop mode when suspend, and then exit stop mode when resume.
> >> AFAIK, as long as flexcan_suspend has been called, flexcan_resume
> >> will be called, unless the system hang during suspend/resume. If so,
> >> only code reset can activate OS again. Could you please tell me how
> >> does CAN stucked in stop mode at your side?
> >
> > Hi Joakim,
> >
> > Thanks I'll test this :-)
> > Guess I will have do some hacking to get it stuck in stop mode.
> >
> > We have a lot of devices in the field that doesn't have:
> > "can: flexcan: fix deadlock when using self wakeup"
> >
> > And they have traffic on both CAN interfaces, that way it's quite easy
> > to get them stuck in stop mode.
> >
> > /Sean
> 
> Hi Joakim,
> 
> I have been testing this.
> I have a hacked version of the driver that calls
> flexcan_enter_stop_mode() as the last step in the probe function.
> 
> First insert of flexcan.ko when stop mode is activated:
> flexcan 2090000.flexcan: Linked as a consumer to regulator.4
> 
> flexcan 2090000.flexcan: registering netdev failed
> 
> flexcan 2090000.flexcan: Dropping the link to regulator.4
> 
> flexcan: probe of 2090000.flexcan failed with error -110
> 
> flexcan 2094000.flexcan: Linked as a consumer to regulator.4
> 
> flexcan 2094000.flexcan: registering netdev failed
> 
> flexcan 2094000.flexcan: Dropping the link to regulator.4
> 
> flexcan: probe of 2094000.flexcan failed with error -110
> 
> 
> When I insert a flexcan.ko with the patch
> "can: flexcan: try to exit stop mode during probe stage":
> flexcan 2090000.flexcan: Linked as a consumer to regulator.4
> 
> flexcan 2090000.flexcan: Unbalanced pm_runtime_enable!
> 
> flexcan 2094000.flexcan: Linked as a consumer to regulator.4
> 
> flexcan 2094000.flexcan: Unbalanced pm_runtime_enable!
> 
> I works as I expected but, I think we need to do some pm_runtime cleanup
> when bailing with error -110.
> Anyways it works great, thanks for your work on this.

Hi Sean,

Thanks for your quirk test, I used unbind/bind to test, do not meet such issue.
I will build as a module to have a test.

Best Regards,
Joakim Zhang
> /Sean

RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年11月27日 17:37
> To: Sean Nyekjaer <sean@geanix.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> 
> 
> > -----Original Message-----
> > From: Sean Nyekjaer <sean@geanix.com>
> > Sent: 2019年11月27日 16:13
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> > linux-can@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> > Subject: Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> >
> >
> >
> > On 27/11/2019 07.12, Sean Nyekjaer wrote:
> > >
> > >
> > > On 27/11/2019 06.56, Joakim Zhang wrote:
> > >>     Could you help check the patch set? With your suggestions, I
> > >> have cooked a patch to exit stop mode during probe stage.
> > >>
> > >>     IMHO, I think this patch is unneed, now in flexcan driver,
> > >> enter stop mode when suspend, and then exit stop mode when resume.
> > >> AFAIK, as long as flexcan_suspend has been called, flexcan_resume
> > >> will be called, unless the system hang during suspend/resume. If
> > >> so, only code reset can activate OS again. Could you please tell me
> > >> how does CAN stucked in stop mode at your side?
> > >
> > > Hi Joakim,
> > >
> > > Thanks I'll test this :-)
> > > Guess I will have do some hacking to get it stuck in stop mode.
> > >
> > > We have a lot of devices in the field that doesn't have:
> > > "can: flexcan: fix deadlock when using self wakeup"
> > >
> > > And they have traffic on both CAN interfaces, that way it's quite
> > > easy to get them stuck in stop mode.
> > >
> > > /Sean
> >
> > Hi Joakim,
> >
> > I have been testing this.
> > I have a hacked version of the driver that calls
> > flexcan_enter_stop_mode() as the last step in the probe function.
> >
> > First insert of flexcan.ko when stop mode is activated:
> > flexcan 2090000.flexcan: Linked as a consumer to regulator.4
> >
> > flexcan 2090000.flexcan: registering netdev failed
> >
> > flexcan 2090000.flexcan: Dropping the link to regulator.4
> >
> > flexcan: probe of 2090000.flexcan failed with error -110
> >
> > flexcan 2094000.flexcan: Linked as a consumer to regulator.4
> >
> > flexcan 2094000.flexcan: registering netdev failed
> >
> > flexcan 2094000.flexcan: Dropping the link to regulator.4
> >
> > flexcan: probe of 2094000.flexcan failed with error -110
> >
> >
> > When I insert a flexcan.ko with the patch
> > "can: flexcan: try to exit stop mode during probe stage":
> > flexcan 2090000.flexcan: Linked as a consumer to regulator.4
> >
> > flexcan 2090000.flexcan: Unbalanced pm_runtime_enable!
> >
> > flexcan 2094000.flexcan: Linked as a consumer to regulator.4
> >
> > flexcan 2094000.flexcan: Unbalanced pm_runtime_enable!
> >
> > I works as I expected but, I think we need to do some pm_runtime
> > cleanup when bailing with error -110.
> > Anyways it works great, thanks for your work on this.
> 
> Hi Sean,
> 
> Thanks for your quirk test, I used unbind/bind to test, do not meet such issue.
> I will build as a module to have a test.

One more should confirm with you, you inserted a flexcan.ko after stop mode activated without fix patch firstly, and then inserted a flexcan.ko
with fix patch. If yes, this could cause unbalanced pm_runtime_enabled. The reason is that firstly inserted the flexcan.ko would enable device runtime pm,
and then you inserted flexcan.ko enable device runtime pm again.

Could you please insert flexcan.ko with fix patch directly after stop mode activated?
 
Best Regards,
Joakim Zhang
> Best Regards,
> Joakim Zhang
> > /Sean

Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Sean Nyekjaer]

On 27/11/2019 10.48, Joakim Zhang wrote:
> One more should confirm with you, you inserted a flexcan.ko after stop mode activated without fix patch firstly, and then inserted a flexcan.ko
> with fix patch. If yes, this could cause unbalanced pm_runtime_enabled. The reason is that firstly inserted the flexcan.ko would enable device runtime pm,
> and then you inserted flexcan.ko enable device runtime pm again.
> 
> Could you please insert flexcan.ko with fix patch directly after stop mode activated?
>   
Hi,

If I insert flexcan.ko with fix patch directly after stop mode 
activated, the unbalanced msg is not shown...
I guess we are good then :)

/Sean

RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年11月27日 17:52
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> 
> 
> 
> On 27/11/2019 10.48, Joakim Zhang wrote:
> > One more should confirm with you, you inserted a flexcan.ko after stop
> > mode activated without fix patch firstly, and then inserted a
> > flexcan.ko with fix patch. If yes, this could cause unbalanced
> pm_runtime_enabled. The reason is that firstly inserted the flexcan.ko would
> enable device runtime pm, and then you inserted flexcan.ko enable device
> runtime pm again.
> >
> > Could you please insert flexcan.ko with fix patch directly after stop mode
> activated?
> >
> Hi,
> 
> If I insert flexcan.ko with fix patch directly after stop mode activated, the
> unbalanced msg is not shown...
> I guess we are good then :)

Great, Thanks a lot!

Could you give your Test-by tag for this patch set? And then Marc could review this patch set.

Best Regards,
Joakim Zhang
> /Sean

Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 27/11/2019 06.56, Joakim Zhang wrote:
> From: Sean Nyekjaer <sean@geanix.com>
> 
> When suspending, when there is still can traffic on the interfaces the
> flexcan immediately wakes the platform again. As it should :-). But it
> throws this error msg:
> [ 3169.378661] PM: noirq suspend of devices failed
> 
> On the way down to suspend the interface that throws the error message does
> call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
> flexcan_enter_stop_mode is called, but on the way out of suspend the driver
> only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
> flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
> current driver it can't recover from this even with a soft reboot, it requires
> a hard reboot.
> 
> This patch can fix deadlock when using self wakeup, it happenes to be
> able to fix another issue that frames out-of-order in first IRQ handler
> run after wakeup.
> 
> In wakeup case, after system resume, frames received out-of-order in
> first IRQ handler, the problem is wakeup latency from frame reception to
> IRQ handler is much bigger than the counter overflow. This means it's
> impossible to sort the CAN frames by timestamp. The reason is that controller
> exits stop mode during noirq resume, then it can receive the frame immediately.
> If noirq reusme stage consumes much time, it will extend interrupt response
> time. So exit stop mode during resume stage instead of noirq resume can
> fix this issue.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>   drivers/net/can/flexcan.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 2efa06119f68..2297663cacb2 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -134,8 +134,7 @@
>   	(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_WAK_INT)
> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
>   
>   /* FLEXCAN interrupt flag register (IFLAG) bits */
>   /* Errata ERR005829 step7: Reserve first valid MB */
> @@ -960,6 +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>   
>   	reg_esr = priv->read(&regs->esr);
>   
> +	/* ACK wakeup interrupt */
> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> +		handled = IRQ_HANDLED;
> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> +	}
> +
>   	/* ACK all bus error and state change IRQ sources */
>   	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>   		handled = IRQ_HANDLED;
> @@ -1722,6 +1727,9 @@ static int __maybe_unused flexcan_resume(struct device *device)
>   		netif_start_queue(dev);
>   		if (device_may_wakeup(device)) {
>   			disable_irq_wake(dev->irq);
> +			err = flexcan_exit_stop_mode(priv);
> +			if (err)
> +				return err;
>   		} else {
>   			err = pm_runtime_force_resume(device);
>   			if (err)
> @@ -1767,14 +1775,9 @@ 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);
> -	int err;
>   
> -	if (netif_running(dev) && device_may_wakeup(device)) {
> +	if (netif_running(dev) && device_may_wakeup(device))
>   		flexcan_enable_wakeup_irq(priv, false);
> -		err = flexcan_exit_stop_mode(priv);
> -		if (err)
> -			return err;
> -	}
>   
>   	return 0;
>   }
> 

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Sean Nyekjaer]

On 27/11/2019 06.56, Joakim Zhang wrote:
> CAN controller could be stucked in stop mode once it enters stop mode
> when suspend, and then it fails to exit stop mode when resume. Only code
> reset can get CAN out of stop mode, so add stop mode remove request
> during probe stage for other methods(soft reset from chip level,
> unbind/bind driver, etc) to let CAN active again. MCR[LPMACK] will be
> checked when enable CAN in register_flexcandev().
> 
> Suggested-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
> ------
> ChangeLog:
> 	V1->V2: new add.
> ---
>   drivers/net/can/flexcan.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 2297663cacb2..5d5ed28d3005 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -449,6 +449,13 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>   	return 0;
>   }
>   
> +static void flexcan_try_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;
> @@ -1649,6 +1656,21 @@ static int flexcan_probe(struct platform_device *pdev)
>   	priv->devtype_data = devtype_data;
>   	priv->reg_xceiver = reg_xceiver;
>   
> +	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");
> +
> +		/* CAN controller could be stucked in stop mode once it enters
> +		 * stop mode when suspend, and then it fails to exit stop
> +		 * mode when resume. Only code reset can get CAN out of stop
> +		 * mode, so add stop mode remove request here for other methods
> +		 * (soft reset, bind, etc) to let CAN active again. MCR[LPMACK]
> +		 * will be checked when enable CAN in register_flexcandev().
> +		 */
> +		flexcan_try_exit_stop_mode(priv);
> +	}
> +
>   	pm_runtime_get_noresume(&pdev->dev);
>   	pm_runtime_set_active(&pdev->dev);
>   	pm_runtime_enable(&pdev->dev);
> @@ -1661,12 +1683,6 @@ 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");
> -	}
> -
>   	return 0;
>   
>    failed_register:
> 

Re: [PATCH V2 3/4] can: flexcan: change the way of stop mode acknowledgment

[Sean Nyekjaer]

On 27/11/2019 06.56, Joakim Zhang wrote:
> Stop mode is entered when Stop mode is requested at chip level and
> MCR[LPM_ACK] is asserted by the FlexCAN.
> 
> Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
> acknowledgment, not the acknowledgment from chip level which is used
> for glitch filter.
> 
> Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>   drivers/net/can/flexcan.c | 64 ++++++++++++++++++++-------------------
>   1 file changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 5d5ed28d3005..d178146b3da5 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -388,6 +388,34 @@ static struct flexcan_mb __iomem *flexcan_get_mb(const struct flexcan_priv *priv
>   		(&priv->regs->mb[bank][priv->mb_size * mb_index]);
>   }
>   
> +static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> +
> +	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		udelay(10);
> +
> +	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> +
> +	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		udelay(10);
> +
> +	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
>   static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>   {
>   	struct flexcan_regs __iomem *regs = priv->regs;
> @@ -406,7 +434,6 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>   static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>   {
>   	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int ackval;
>   	u32 reg_mcr;
>   
>   	reg_mcr = priv->read(&regs->mcr);
> @@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>   			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
>   
>   	/* get stop acknowledgment */
> -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> -				     ackval, ackval & (1 << priv->stm.ack_bit),
> -				     0, FLEXCAN_TIMEOUT_US))
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_enter_low_power_ack(priv);
>   }
>   
>   static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>   {
>   	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int ackval;
>   	u32 reg_mcr;
>   
>   	/* remove stop request */
>   	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
>   			   1 << priv->stm.req_bit, 0);
>   
> -	/* get stop acknowledgment */
> -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> -				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
> -				     0, FLEXCAN_TIMEOUT_US))
> -		return -ETIMEDOUT;
> -
>   	reg_mcr = priv->read(&regs->mcr);
>   	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
>   	priv->write(reg_mcr, &regs->mcr);
>   
> -	return 0;
> +	/* get stop acknowledgment */
> +	return flexcan_exit_low_power_ack(priv);
>   }
>   
>   static void flexcan_try_exit_stop_mode(struct flexcan_priv *priv)
> @@ -512,39 +528,25 @@ static inline int flexcan_transceiver_disable(const struct flexcan_priv *priv)
>   static int flexcan_chip_enable(struct flexcan_priv *priv)
>   {
>   	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
>   	u32 reg;
>   
>   	reg = priv->read(&regs->mcr);
>   	reg &= ~FLEXCAN_MCR_MDIS;
>   	priv->write(reg, &regs->mcr);
>   
> -	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		udelay(10);
> -
> -	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_exit_low_power_ack(priv);
>   }
>   
>   static int flexcan_chip_disable(struct flexcan_priv *priv)
>   {
>   	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
>   	u32 reg;
>   
>   	reg = priv->read(&regs->mcr);
>   	reg |= FLEXCAN_MCR_MDIS;
>   	priv->write(reg, &regs->mcr);
>   
> -	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		udelay(10);
> -
> -	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_enter_low_power_ack(priv);
>   }
>   
>   static int flexcan_chip_freeze(struct flexcan_priv *priv)
> 

Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Sean Nyekjaer]

On 27/11/2019 10.58, Joakim Zhang wrote:
> Could you give your Test-by tag for this patch set? And then Marc could review this patch set.

Done :)
Can't test patch 4/4...

Hope Marc can give his comments and pick these.

/Sean

RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

> -----Original Message-----
> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org>
> On Behalf Of Sean Nyekjaer
> Sent: 2019年11月27日 18:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> 
> 
> 
> On 27/11/2019 10.58, Joakim Zhang wrote:
> > Could you give your Test-by tag for this patch set? And then Marc could
> review this patch set.
> 
> Done :)

Thanks a lot😊

> Can't test patch 4/4...
> Hope Marc can give his comments and pick these.

Much hope, I would improve it with Marc's comments.

Best Regards,
Joakim Zhang
> /Sean

RE: [PATCH V2 0/4] can: flexcan: fixes for stop mode

[Joakim Zhang]

> -----Original Message-----
> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org>
> On Behalf Of Sean Nyekjaer
> Sent: 2019年11月27日 18:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 0/4] can: flexcan: fixes for stop mode
> 
> 
> 
> On 27/11/2019 10.58, Joakim Zhang wrote:
> > Could you give your Test-by tag for this patch set? And then Marc could
> review this patch set.
> 
> Done :)
> Can't test patch 4/4...
> 
> Hope Marc can give his comments and pick these.

Hi Marc,

Could you please review this patch set, or need I resend it? Sean has already tested first three patches.

Best Regards,
Joakim Zhang
> /Sean

Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Marc Kleine-Budde]

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

On 11/27/19 6:56 AM, Joakim Zhang wrote:
> From: Sean Nyekjaer <sean@geanix.com>
> 
> When suspending, when there is still can traffic on the interfaces the
> flexcan immediately wakes the platform again. As it should :-). But it
> throws this error msg:
> [ 3169.378661] PM: noirq suspend of devices failed
> 
> On the way down to suspend the interface that throws the error message does
> call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
> flexcan_enter_stop_mode is called, but on the way out of suspend the driver
> only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
> flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
> current driver it can't recover from this even with a soft reboot, it requires
> a hard reboot.
> 
> This patch can fix deadlock when using self wakeup, it happenes to be
> able to fix another issue that frames out-of-order in first IRQ handler
> run after wakeup.
> 
> In wakeup case, after system resume, frames received out-of-order in
> first IRQ handler, the problem is wakeup latency from frame reception to
> IRQ handler is much bigger than the counter overflow. This means it's
> impossible to sort the CAN frames by timestamp. The reason is that controller
> exits stop mode during noirq resume, then it can receive the frame immediately.
> If noirq reusme stage consumes much time, it will extend interrupt response
> time. So exit stop mode during resume stage instead of noirq resume can
> fix this issue.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>  drivers/net/can/flexcan.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 2efa06119f68..2297663cacb2 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -134,8 +134,7 @@
>  	(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_WAK_INT)
> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)

Why do you remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT?

>  
>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>  /* Errata ERR005829 step7: Reserve first valid MB */
> @@ -960,6 +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  
>  	reg_esr = priv->read(&regs->esr);
>  
> +	/* ACK wakeup interrupt */
> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> +		handled = IRQ_HANDLED;
> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> +	}
> +

If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need that
explicit ACK here.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Marc Kleine-Budde]

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

On 12/3/19 6:25 PM, Marc Kleine-Budde wrote:
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
>> From: Sean Nyekjaer <sean@geanix.com>
>>
>> When suspending, when there is still can traffic on the interfaces the
>> flexcan immediately wakes the platform again. As it should :-). But it
>> throws this error msg:
>> [ 3169.378661] PM: noirq suspend of devices failed
>>
>> On the way down to suspend the interface that throws the error message does
>> call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
>> flexcan_enter_stop_mode is called, but on the way out of suspend the driver
>> only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
>> flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
>> current driver it can't recover from this even with a soft reboot, it requires
>> a hard reboot.
>>
>> This patch can fix deadlock when using self wakeup, it happenes to be
>> able to fix another issue that frames out-of-order in first IRQ handler
>> run after wakeup.
>>
>> In wakeup case, after system resume, frames received out-of-order in
>> first IRQ handler, the problem is wakeup latency from frame reception to
>> IRQ handler is much bigger than the counter overflow. This means it's
>> impossible to sort the CAN frames by timestamp. The reason is that controller
>> exits stop mode during noirq resume, then it can receive the frame immediately.
>> If noirq reusme stage consumes much time, it will extend interrupt response
>> time. So exit stop mode during resume stage instead of noirq resume can
>> fix this issue.
>>
>> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> ------
>> ChangeLog:
>> 	V1->V2: no change.
>> ---
>>  drivers/net/can/flexcan.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 2efa06119f68..2297663cacb2 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -134,8 +134,7 @@
>>  	(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_WAK_INT)
>> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> 
> Why do you remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT?
> 
>>  
>>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>>  /* Errata ERR005829 step7: Reserve first valid MB */
>> @@ -960,6 +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>  
>>  	reg_esr = priv->read(&regs->esr);
>>  
>> +	/* ACK wakeup interrupt */
>> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
>> +		handled = IRQ_HANDLED;
>> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
>> +	}
>> +
> 
> If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need that
> explicit ACK here.

Otherwise this patch is OK. With this patch the flexcan_suspend() and
flexcan_resume() look finally symmetric. \o/

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Marc Kleine-Budde]

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

On 11/27/19 6:56 AM, Joakim Zhang wrote:
> CAN controller could be stucked in stop mode once it enters stop mode
                          ^^^^^^^ stuck
> when suspend, and then it fails to exit stop mode when resume.

How can this happen?

> Only code reset can get CAN out of stop mode,

What is "code reset"?

> so add stop mode remove request during probe stage for other
> methods(soft reset from chip level, unbind/bind driver, etc) to let
        ^^^ please add a space
> CAN active again.

Can you rephrase the sentence after "so add stop mode remove request
during probe stage". I'm not completely sure what you want to tell.

> MCR[LPMACK] will be checked when enable CAN in 
> register_flexcandev().
> 
> Suggested-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: new add.
> ---
>  drivers/net/can/flexcan.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 2297663cacb2..5d5ed28d3005 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -449,6 +449,13 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>  	return 0;
>  }
>  
> +static void flexcan_try_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;
> @@ -1649,6 +1656,21 @@ static int flexcan_probe(struct platform_device *pdev)
>  	priv->devtype_data = devtype_data;
>  	priv->reg_xceiver = reg_xceiver;
>  
> +	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");
> +
> +		/* CAN controller could be stucked in stop mode once it enters
> +		 * stop mode when suspend, and then it fails to exit stop
> +		 * mode when resume. Only code reset can get CAN out of stop
> +		 * mode, so add stop mode remove request here for other methods
> +		 * (soft reset, bind, etc) to let CAN active again. MCR[LPMACK]
> +		 * will be checked when enable CAN in register_flexcandev().
> +		 */
> +		flexcan_try_exit_stop_mode(priv);
> +	}
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> @@ -1661,12 +1683,6 @@ 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");
> -	}
> -
>  	return 0;
>  
>   failed_register:
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 3/4] can: flexcan: change the way of stop mode acknowledgment

[Marc Kleine-Budde]

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

On 11/27/19 6:56 AM, Joakim Zhang wrote:
> Stop mode is entered when Stop mode is requested at chip level and
> MCR[LPM_ACK] is asserted by the FlexCAN.
> 
> Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
> acknowledgment, not the acknowledgment from chip level which is used
> for glitch filter.
> 
> Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>  drivers/net/can/flexcan.c | 64 ++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 5d5ed28d3005..d178146b3da5 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -388,6 +388,34 @@ static struct flexcan_mb __iomem *flexcan_get_mb(const struct flexcan_priv *priv
>  		(&priv->regs->mb[bank][priv->mb_size * mb_index]);
>  }
>  
> +static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)

nitpick: please call this flexcan_low_power_enter_ack()
> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> +
> +	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		udelay(10);
> +
> +	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)

...and this one flexcan_low_power_exit_ack()

> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> +
> +	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> +		udelay(10);
> +
> +	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

Can you move split the creation of
flexcan_low_power_enter_ack()/flexcan_low_power_exit_ack() and use in
flexcan_chip_enable/disable() into one patch and the conversion of
flexcan_enter_stop_mode()/flexcan_exit_stop_mode() into another.

Marc

> +
>  static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
> @@ -406,7 +434,6 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int ackval;
>  	u32 reg_mcr;
>  
>  	reg_mcr = priv->read(&regs->mcr);
> @@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>  			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
>  
>  	/* get stop acknowledgment */
> -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> -				     ackval, ackval & (1 << priv->stm.ack_bit),
> -				     0, FLEXCAN_TIMEOUT_US))
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_enter_low_power_ack(priv);
>  }
>  
>  static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int ackval;
>  	u32 reg_mcr;
>  
>  	/* remove stop request */
>  	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
>  			   1 << priv->stm.req_bit, 0);
>  
> -	/* get stop acknowledgment */
> -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> -				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
> -				     0, FLEXCAN_TIMEOUT_US))
> -		return -ETIMEDOUT;
> -
>  	reg_mcr = priv->read(&regs->mcr);
>  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
>  	priv->write(reg_mcr, &regs->mcr);
>  
> -	return 0;
> +	/* get stop acknowledgment */
> +	return flexcan_exit_low_power_ack(priv);
>  }
>  
>  static void flexcan_try_exit_stop_mode(struct flexcan_priv *priv)
> @@ -512,39 +528,25 @@ static inline int flexcan_transceiver_disable(const struct flexcan_priv *priv)
>  static int flexcan_chip_enable(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
>  	u32 reg;
>  
>  	reg = priv->read(&regs->mcr);
>  	reg &= ~FLEXCAN_MCR_MDIS;
>  	priv->write(reg, &regs->mcr);
>  
> -	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		udelay(10);
> -
> -	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_exit_low_power_ack(priv);
>  }
>  
>  static int flexcan_chip_disable(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
> -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
>  	u32 reg;
>  
>  	reg = priv->read(&regs->mcr);
>  	reg |= FLEXCAN_MCR_MDIS;
>  	priv->write(reg, &regs->mcr);
>  
> -	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		udelay(10);
> -
> -	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return flexcan_enter_low_power_ack(priv);
>  }
>  
>  static int flexcan_chip_freeze(struct flexcan_priv *priv)
> 


-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

RE: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 1:25
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[...]
> >  drivers/net/can/flexcan.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 2efa06119f68..2297663cacb2 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -134,8 +134,7 @@
> >  	(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_WAK_INT)
> > +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> 
> Why do you remove the FLEXCAN_ESR_WAK_INT from the
> FLEXCAN_ESR_ALL_INT?
> 
> >
> >  /* FLEXCAN interrupt flag register (IFLAG) bits */
> >  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -960,6
> > +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >
> >  	reg_esr = priv->read(&regs->esr);
> >
> > +	/* ACK wakeup interrupt */
> > +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> > +		handled = IRQ_HANDLED;
> > +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> > +	}
> > +
> 
> If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need
> that explicit ACK here.

Hi Marc,

I remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT since FLEXCAN_ESR_ALL_INT is for
all bus error and state change IRQ sources, wakeup interrupt does not belong to these. If you think this does
not need, I can remove this change.

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 2:15
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
> stage
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > CAN controller could be stucked in stop mode once it enters stop mode
>                           ^^^^^^^ stuck
> > when suspend, and then it fails to exit stop mode when resume.
> 
> How can this happen?

I am also confused how can this happen, as I asked Sean, only CAN enter stop mode when suspend, then system hang, it could let CAN
stuck in stop mode. However, Sean said this indeed happen at his side, @sean@geanix.com, could you explain how this happen in details?

> > Only code reset can get CAN out of stop mode,
> 
> What is "code reset"?

As I know, "code reset" is to press the POWER KEY from the board. At my side, reboot command from OS also can get CAN out of stop mode.
Below is experiment I did:
	Firstly, do a hacking to let CAN stuck into stop mode, then:
	(1) press power on/off key, get CAN out of stop mode;
	(2) reboot command from console, get CAN out of stop mode;
	(3) unbind/bind driver, cannot get CAN out of stop mode;  
	(4) remod/insmod module, cannot get CAN out of stop mode;

> > so add stop mode remove request during probe stage for other
> > methods(soft reset from chip level, unbind/bind driver, etc) to let
>         ^^^ please add a space
> > CAN active again.
> 
> Can you rephrase the sentence after "so add stop mode remove request during
> probe stage". I'm not completely sure what you want to tell.

Sure.

Best Regards,
Joakim Zhang
> > MCR[LPMACK] will be checked when enable CAN in register_flexcandev().
> >
> > Suggested-by: Sean Nyekjaer <sean@geanix.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

[...]

> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 3/4] can: flexcan: change the way of stop mode acknowledgment

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 2:22
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 3/4] can: flexcan: change the way of stop mode
> acknowledgment
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > Stop mode is entered when Stop mode is requested at chip level and
> > MCR[LPM_ACK] is asserted by the FlexCAN.
> >
> > Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
> > acknowledgment, not the acknowledgment from chip level which is used
> > for glitch filter.
> >
> > Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ------
> > ChangeLog:
> > 	V1->V2: no change.
> > ---
> >  drivers/net/can/flexcan.c | 64
> > ++++++++++++++++++++-------------------
> >  1 file changed, 33 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 5d5ed28d3005..d178146b3da5 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -388,6 +388,34 @@ static struct flexcan_mb __iomem
> *flexcan_get_mb(const struct flexcan_priv *priv
> >  		(&priv->regs->mb[bank][priv->mb_size * mb_index]);  }
> >
> > +static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)
> 
> nitpick: please call this flexcan_low_power_enter_ack()
> > +{
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> > +
> > +	while (timeout-- && !(priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > +		udelay(10);
> > +
> > +	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)
> 
> ...and this one flexcan_low_power_exit_ack()
> 
> > +{
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> > +
> > +	while (timeout-- && (priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > +		udelay(10);
> > +
> > +	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> 
> Can you move split the creation of
> flexcan_low_power_enter_ack()/flexcan_low_power_exit_ack() and use in
> flexcan_chip_enable/disable() into one patch and the conversion of
> flexcan_enter_stop_mode()/flexcan_exit_stop_mode() into another.

Of course, this could be more clearer and reasonable. Thanks.

Best Regards,
Joakim Zhang
> Marc
> 
> > +
> >  static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > enable)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs; @@ -406,7 +434,6 @@
> > static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > enable)  static inline int flexcan_enter_stop_mode(struct flexcan_priv
> > *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int ackval;
> >  	u32 reg_mcr;
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> > @@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct
> flexcan_priv *priv)
> >  			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> >
> >  	/* get stop acknowledgment */
> > -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> > -				     ackval, ackval & (1 << priv->stm.ack_bit),
> > -				     0, FLEXCAN_TIMEOUT_US))
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_enter_low_power_ack(priv);
> >  }
> >
> >  static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
> > {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int ackval;
> >  	u32 reg_mcr;
> >
> >  	/* remove stop request */
> >  	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> >  			   1 << priv->stm.req_bit, 0);
> >
> > -	/* get stop acknowledgment */
> > -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> > -				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
> > -				     0, FLEXCAN_TIMEOUT_US))
> > -		return -ETIMEDOUT;
> > -
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> > -	return 0;
> > +	/* get stop acknowledgment */
> > +	return flexcan_exit_low_power_ack(priv);
> >  }
> >
> >  static void flexcan_try_exit_stop_mode(struct flexcan_priv *priv) @@
> > -512,39 +528,25 @@ static inline int flexcan_transceiver_disable(const
> > struct flexcan_priv *priv)  static int flexcan_chip_enable(struct
> > flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> >  	u32 reg;
> >
> >  	reg = priv->read(&regs->mcr);
> >  	reg &= ~FLEXCAN_MCR_MDIS;
> >  	priv->write(reg, &regs->mcr);
> >
> > -	while (timeout-- && (priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > -		udelay(10);
> > -
> > -	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_exit_low_power_ack(priv);
> >  }
> >
> >  static int flexcan_chip_disable(struct flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> >  	u32 reg;
> >
> >  	reg = priv->read(&regs->mcr);
> >  	reg |= FLEXCAN_MCR_MDIS;
> >  	priv->write(reg, &regs->mcr);
> >
> > -	while (timeout-- && !(priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > -		udelay(10);
> > -
> > -	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_enter_low_power_ack(priv);
> >  }
> >
> >  static int flexcan_chip_freeze(struct flexcan_priv *priv)
> >
> 
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Joakim Zhang]

Hi Marc,

You may have missed this patch, any comments about it?

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年11月27日 13:57
> To: mkl@pengutronix.de; sean@geanix.com; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org; Joakim Zhang
> <qiangqing.zhang@nxp.com>
> Subject: [PATCH V2 4/4] can: flexcan: add LPSR mode support
> 
> For i.MX7D LPSR mode, the controller will lost power and got the configuration
> state lost after system resume back. (coming i.MX8QM/QXP will also
> completely power off the domain, the controller state will be lost and needs
> restore).
> So we need to set pinctrl state again and re-start chip to do re-configuration
> after resume.
> 
> For wakeup case, it should not set pinctrl to sleep state by
> pinctrl_pm_select_sleep_state.
> For interface is not up before suspend case, we don't need re-configure as it
> will be configured by user later by interface up.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> d178146b3da5..d1509cffdd24 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -26,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/regmap.h>
> 
>  #define DRV_NAME			"flexcan"
> @@ -1707,7 +1708,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 = 0;
> +	int err;
> 
>  	if (netif_running(dev)) {
>  		/* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27
> @@ static int __maybe_unused flexcan_suspend(struct device *device)
>  			if (err)
>  				return err;
>  		} else {
> -			err = flexcan_chip_disable(priv);
> +			flexcan_chip_stop(dev);
> +
> +			err = pm_runtime_force_suspend(device);
>  			if (err)
>  				return err;
> 
> -			err = pm_runtime_force_suspend(device);
> +			pinctrl_pm_select_sleep_state(device);
>  		}
>  		netif_stop_queue(dev);
>  		netif_device_detach(dev);
>  	}
>  	priv->can.state = CAN_STATE_SLEEPING;
> 
> -	return err;
> +	return 0;
>  }
> 
>  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 = 0;
> +	int err;
> 
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  	if (netif_running(dev)) {
> @@ -1749,15 +1752,19 @@ static int __maybe_unused
> flexcan_resume(struct device *device)
>  			if (err)
>  				return err;
>  		} else {
> +			pinctrl_pm_select_default_state(device);
> +
>  			err = pm_runtime_force_resume(device);
>  			if (err)
>  				return err;
> 
> -			err = flexcan_chip_enable(priv);
> +			err = flexcan_chip_start(dev);
> +			if (err)
> +				return err;
>  		}
>  	}
> 
> -	return err;
> +	return 0;
>  }
> 
>  static int __maybe_unused flexcan_runtime_suspend(struct device *device)
> --
> 2.17.1

Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Marc Kleine-Budde]

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

On 12/4/19 2:58 AM, Joakim Zhang wrote:
> [...]
>>>  drivers/net/can/flexcan.c | 19 +++++++++++--------
>>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index 2efa06119f68..2297663cacb2 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -134,8 +134,7 @@
>>>  	(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_WAK_INT)
>>> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
>>
>> Why do you remove the FLEXCAN_ESR_WAK_INT from the
>> FLEXCAN_ESR_ALL_INT?
>>
>>>
>>>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>>>  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -960,6
>>> +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>>
>>>  	reg_esr = priv->read(&regs->esr);
>>>
>>> +	/* ACK wakeup interrupt */
>>> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
>>> +		handled = IRQ_HANDLED;
>>> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
>>> +	}
>>> +
>>
>> If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need
>> that explicit ACK here.
> 
> Hi Marc,
> 
> I remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT since
> FLEXCAN_ESR_ALL_INT is for all bus error and state change IRQ
> sources, wakeup interrupt does not belong to these. If you think this
> does not need, I can remove this change.

I see, makes sense.

Make this a separate patch. Move the FLEXCAN_ESR_WAK_INT from the
FLEXCAN_ESR_ALL_INT, but add it to the existing ack of the interrupts.
Like this:

> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index b6f675a5e2d9..74f622b40b61 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -960,10 +960,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  
>         reg_esr = priv->read(&regs->esr);
>  
> -       /* ACK all bus error and state change IRQ sources */
> -       if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> +       /* ACK all bus error, state change and wake IRQ sources */
> +       if (reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT)) {
>                 handled = IRQ_HANDLED;
> -               priv->write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
> +               priv->write(reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT), &regs->esr);
>         }
>  
>         /* state change interrupt or broken error state quirk fix is enabled */

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

RE: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 16:31
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup
> 
> On 12/4/19 2:58 AM, Joakim Zhang wrote:
> > [...]
> >>>  drivers/net/can/flexcan.c | 19 +++++++++++--------
> >>>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >>> index 2efa06119f68..2297663cacb2 100644
> >>> --- a/drivers/net/can/flexcan.c
> >>> +++ b/drivers/net/can/flexcan.c
> >>> @@ -134,8 +134,7 @@
> >>>  	(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_WAK_INT)
> >>> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> >>
> >> Why do you remove the FLEXCAN_ESR_WAK_INT from the
> >> FLEXCAN_ESR_ALL_INT?
> >>
> >>>
> >>>  /* FLEXCAN interrupt flag register (IFLAG) bits */
> >>>  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -960,6
> >>> +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >>>
> >>>  	reg_esr = priv->read(&regs->esr);
> >>>
> >>> +	/* ACK wakeup interrupt */
> >>> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> >>> +		handled = IRQ_HANDLED;
> >>> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> >>> +	}
> >>> +
> >>
> >> If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need
> >> that explicit ACK here.
> >
> > Hi Marc,
> >
> > I remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT since
> > FLEXCAN_ESR_ALL_INT is for all bus error and state change IRQ sources,
> > wakeup interrupt does not belong to these. If you think this does not
> > need, I can remove this change.
> 
> I see, makes sense.
> 
> Make this a separate patch. Move the FLEXCAN_ESR_WAK_INT from the
> FLEXCAN_ESR_ALL_INT, but add it to the existing ack of the interrupts.
> Like this:
> 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index b6f675a5e2d9..74f622b40b61 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -960,10 +960,10 @@ static irqreturn_t flexcan_irq(int irq, void
> > *dev_id)
> >
> >         reg_esr = priv->read(&regs->esr);
> >
> > -       /* ACK all bus error and state change IRQ sources */
> > -       if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> > +       /* ACK all bus error, state change and wake IRQ sources */
> > +       if (reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT)) {
> >                 handled = IRQ_HANDLED;
> > -               priv->write(reg_esr & FLEXCAN_ESR_ALL_INT,
> &regs->esr);
> > +               priv->write(reg_esr & (FLEXCAN_ESR_ALL_INT |
> > + FLEXCAN_ESR_WAK_INT), &regs->esr);
> >         }
> >
> >         /* state change interrupt or broken error state quirk fix is
> > enabled */

Got it! Thanks.

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Marc Kleine-Budde]

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

On 12/4/19 3:22 AM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Sent: 2019年12月4日 2:15
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
>> linux-can@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
>> stage
>>
>> On 11/27/19 6:56 AM, Joakim Zhang wrote:
>>> CAN controller could be stucked in stop mode once it enters stop mode
>>                           ^^^^^^^ stuck
>>> when suspend, and then it fails to exit stop mode when resume.
>>
>> How can this happen?
> 
> I am also confused how can this happen, as I asked Sean, only CAN
> enter stop mode when suspend, then system hang,
How do you recover the system when suspended?

> it could let CAN 
> stuck in stop mode. However, Sean said this indeed happen at his
> side, @sean@geanix.com, could you explain how this happen in
> details?
That would be good.

>>> Only code reset can get CAN out of stop mode,
>>
>> What is "code reset"?
> 
> As I know, "code reset" is to press the POWER KEY from the board. At
> my side, reboot command from OS also can get CAN out of stop mode.
Do you mean "cold reset", also known as Power-On-Reset, POR or power cycle?

What does pressing the POWER KEY do? A power cycle of the system or
toggling the reset line of the imx?

We need to describe in detail, as not everyone has the same board as
you, and these boards might not even have a power key :)

> Below is experiment I did:
> 	Firstly, do a hacking to let CAN stuck into stop mode, then:

You mean you put the CAN into stop mode without keeping track in the CAN
driver that the CAN-IP is in stop mode, e.g. by hacking the driver.

Then you try several methods to recover:

> 	(1) press power on/off key, get CAN out of stop mode;
> 	(2) reboot command from console, get CAN out of stop mode;
> 	(3) unbind/bind driver, cannot get CAN out of stop mode;  
> 	(4) remod/insmod module, cannot get CAN out of stop mode;

(2) resets the complete imx, including the CAN-IP core, (1) probably, too.

(3) and (4) fail to recover the CAN core, as the IP core is still
powered off by some upstream component. So the question why this happens
in the first place is IMHO as important as trying to wake up the core. I
think if we discover this situation (CAN Core is in stop-mode in probe)
we should print a warning message, but try to recover.

>>> so add stop mode remove request during probe stage for other
>>> methods(soft reset from chip level, unbind/bind driver, etc) to let
>>         ^^^ please add a space
>>> CAN active again.
>>
>> Can you rephrase the sentence after "so add stop mode remove request during
>> probe stage". I'm not completely sure what you want to tell.
> 
> Sure.

tnx,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Marc Kleine-Budde]

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

On 11/27/19 6:56 AM, Joakim Zhang wrote:
> For i.MX7D LPSR mode, the controller will lost power and got the
> configuration state lost after system resume back. (coming i.MX8QM/QXP
> will also completely power off the domain, the controller state will be
> lost and needs restore).
> So we need to set pinctrl state again and re-start chip to do
> re-configuration after resume.
> 
> For wakeup case, it should not set pinctrl to sleep state by
> pinctrl_pm_select_sleep_state.
> For interface is not up before suspend case, we don't need
> re-configure as it will be configured by user later by interface up.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index d178146b3da5..d1509cffdd24 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -26,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/regmap.h>
>  
>  #define DRV_NAME			"flexcan"
> @@ -1707,7 +1708,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 = 0;
> +	int err;
>  
>  	if (netif_running(dev)) {
>  		/* if wakeup is enabled, enter stop mode
> @@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device)
>  			if (err)
>  				return err;
>  		} else {
> -			err = flexcan_chip_disable(priv);
> +			flexcan_chip_stop(dev);
> +
> +			err = pm_runtime_force_suspend(device);
>  			if (err)
>  				return err;
>  
> -			err = pm_runtime_force_suspend(device);
> +			pinctrl_pm_select_sleep_state(device);

Please add error handling for pinctrl_pm_select_sleep_state().

>  		}
>  		netif_stop_queue(dev);
>  		netif_device_detach(dev);
>  	}
>  	priv->can.state = CAN_STATE_SLEEPING;
>  
> -	return err;
> +	return 0;
>  }
>  
>  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 = 0;
> +	int err;
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  	if (netif_running(dev)) {
> @@ -1749,15 +1752,19 @@ static int __maybe_unused flexcan_resume(struct device *device)
>  			if (err)
>  				return err;
>  		} else {
> +			pinctrl_pm_select_default_state(device);

same here

> +
>  			err = pm_runtime_force_resume(device);
>  			if (err)
>  				return err;
>  
> -			err = flexcan_chip_enable(priv);
> +			err = flexcan_chip_start(dev);
> +			if (err)
> +				return err;
>  		}
>  	}
>  
> -	return err;
> +	return 0;
>  }
>  
>  static int __maybe_unused flexcan_runtime_suspend(struct device *device)
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Marc Kleine-Budde]

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

On 11/27/19 6:56 AM, Joakim Zhang wrote:
> For i.MX7D LPSR mode, the controller will lost power and got the
> configuration state lost after system resume back. (coming i.MX8QM/QXP
> will also completely power off the domain, the controller state will be
> lost and needs restore).
> So we need to set pinctrl state again and re-start chip to do
> re-configuration after resume.
> 
> For wakeup case, it should not set pinctrl to sleep state by
> pinctrl_pm_select_sleep_state.
> For interface is not up before suspend case, we don't need
> re-configure as it will be configured by user later by interface up.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ------
> ChangeLog:
> 	V1->V2: no change.
> ---
>  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index d178146b3da5..d1509cffdd24 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -26,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/regmap.h>
>  
>  #define DRV_NAME			"flexcan"
> @@ -1707,7 +1708,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 = 0;
> +	int err;
>  
>  	if (netif_running(dev)) {
>  		/* if wakeup is enabled, enter stop mode
> @@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device)
>  			if (err)
>  				return err;
>  		} else {
> -			err = flexcan_chip_disable(priv);
> +			flexcan_chip_stop(dev);

chip_stop calls chip_disable, but doesn't propagate the error value.
Please create a seperate patch to propagate the error value of chip_stop().

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

RE: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 16:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
> stage
> 
> On 12/4/19 3:22 AM, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Sent: 2019年12月4日 2:15
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> >> linux-can@vger.kernel.org
> >> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> >> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode
> >> during probe stage
> >>
> >> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> >>> CAN controller could be stucked in stop mode once it enters stop
> >>> mode
> >>                           ^^^^^^^ stuck
> >>> when suspend, and then it fails to exit stop mode when resume.
> >>
> >> How can this happen?
> >
> > I am also confused how can this happen, as I asked Sean, only CAN
> > enter stop mode when suspend, then system hang,
> How do you recover the system when suspended?
RTC wakeup or TTY wakeup.

> > it could let CAN
> > stuck in stop mode. However, Sean said this indeed happen at his side,
> > @sean@geanix.com, could you explain how this happen in details?
> That would be good.
>
> >>> Only code reset can get CAN out of stop mode,
> >>
> >> What is "code reset"?
> >
> > As I know, "code reset" is to press the POWER KEY from the board. At
> > my side, reboot command from OS also can get CAN out of stop mode.
> Do you mean "cold reset", also known as Power-On-Reset, POR or power
> cycle?
Should be Power-On-Reset.

> What does pressing the POWER KEY do? A power cycle of the system or
> toggling the reset line of the imx?
I think it toggles the reset line of imx. I am so sorry that I am not familiar with system reset :(.
 
> We need to describe in detail, as not everyone has the same board as you, and
> these boards might not even have a power key :)
Yes.

> > Below is experiment I did:
> > 	Firstly, do a hacking to let CAN stuck into stop mode, then:
> 
> You mean you put the CAN into stop mode without keeping track in the CAN
> driver that the CAN-IP is in stop mode, e.g. by hacking the driver.
Yes, you can add flexcan_enter_stop_mode() at the last of driver probe. After probe, CAN has been stuck in stop mode.
Or you can enable CAN wakeup, then comment out flexcan_exit_stop_mode() in flexcan_resume(), do suspend then wakeup system, CAN has been stuck in stop mode.

> Then you try several methods to recover:
> 
> > 	(1) press power on/off key, get CAN out of stop mode;
> > 	(2) reboot command from console, get CAN out of stop mode;
> > 	(3) unbind/bind driver, cannot get CAN out of stop mode;
> > 	(4) remod/insmod module, cannot get CAN out of stop mode;
> 
> (2) resets the complete imx, including the CAN-IP core, (1) probably, too.
Yes, since stop mode enter/exit request at a chip level, need reset completely, such as a "code reset", would get CAN out stop mode.
"Soft reset" cannot get CAN out of stop mode.

> (3) and (4) fail to recover the CAN core, as the IP core is still powered off by
> some upstream component. So the question why this happens in the first place
> is IMHO as important as trying to wake up the core. I think if we discover this
> situation (CAN Core is in stop-mode in probe) we should print a warning
> message, but try to recover.
We really need figure out why CAN could be stuck in stop mode. As I know, enter stop mode in flexcan_suspend(), and then exit stop mode in flexcan_resume(), it could be impossible.
Hope Sean can explain it in details, then we can discuss how to fix it more reasonable.

Thanks Marc.

Best Regards,
Joakim Zhang
> >>> so add stop mode remove request during probe stage for other
> >>> methods(soft reset from chip level, unbind/bind driver, etc) to let
> >>         ^^^ please add a space
> >>> CAN active again.
> >>
> >> Can you rephrase the sentence after "so add stop mode remove request
> >> during probe stage". I'm not completely sure what you want to tell.
> >
> > Sure.
> 
> tnx,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 17:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > For i.MX7D LPSR mode, the controller will lost power and got the
> > configuration state lost after system resume back. (coming i.MX8QM/QXP
> > will also completely power off the domain, the controller state will
> > be lost and needs restore).
> > So we need to set pinctrl state again and re-start chip to do
> > re-configuration after resume.
> >
> > For wakeup case, it should not set pinctrl to sleep state by
> > pinctrl_pm_select_sleep_state.
> > For interface is not up before suspend case, we don't need
> > re-configure as it will be configured by user later by interface up.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ------
> > ChangeLog:
> > 	V1->V2: no change.
> > ---
> >  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index d178146b3da5..d1509cffdd24 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/regmap.h>
> >
> >  #define DRV_NAME			"flexcan"
> > @@ -1707,7 +1708,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 = 0;
> > +	int err;
> >
> >  	if (netif_running(dev)) {
> >  		/* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27
> @@
> > static int __maybe_unused flexcan_suspend(struct device *device)
> >  			if (err)
> >  				return err;
> >  		} else {
> > -			err = flexcan_chip_disable(priv);
> > +			flexcan_chip_stop(dev);
> > +
> > +			err = pm_runtime_force_suspend(device);
> >  			if (err)
> >  				return err;
> >
> > -			err = pm_runtime_force_suspend(device);
> > +			pinctrl_pm_select_sleep_state(device);
> 
> Please add error handling for pinctrl_pm_select_sleep_state().
Got it.

> >  		}
> >  		netif_stop_queue(dev);
> >  		netif_device_detach(dev);
> >  	}
> >  	priv->can.state = CAN_STATE_SLEEPING;
> >
> > -	return err;
> > +	return 0;
> >  }
> >
> >  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 = 0;
> > +	int err;
> >
> >  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >  	if (netif_running(dev)) {
> > @@ -1749,15 +1752,19 @@ static int __maybe_unused
> flexcan_resume(struct device *device)
> >  			if (err)
> >  				return err;
> >  		} else {
> > +			pinctrl_pm_select_default_state(device);
> 
> same here
Got it.

> > +
> >  			err = pm_runtime_force_resume(device);
> >  			if (err)
> >  				return err;
> >
> > -			err = flexcan_chip_enable(priv);
> > +			err = flexcan_chip_start(dev);
> > +			if (err)
> > +				return err;
> >  		}
> >  	}
> >
> > -	return err;
> > +	return 0;
> >  }
> >
> >  static int __maybe_unused flexcan_runtime_suspend(struct device
> > *device)
> >


Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 17:21
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > For i.MX7D LPSR mode, the controller will lost power and got the
> > configuration state lost after system resume back. (coming i.MX8QM/QXP
> > will also completely power off the domain, the controller state will
> > be lost and needs restore).
> > So we need to set pinctrl state again and re-start chip to do
> > re-configuration after resume.
> >
> > For wakeup case, it should not set pinctrl to sleep state by
> > pinctrl_pm_select_sleep_state.
> > For interface is not up before suspend case, we don't need
> > re-configure as it will be configured by user later by interface up.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ------
> > ChangeLog:
> > 	V1->V2: no change.
> > ---
> >  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index d178146b3da5..d1509cffdd24 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/regmap.h>
> >
> >  #define DRV_NAME			"flexcan"
> > @@ -1707,7 +1708,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 = 0;
> > +	int err;
> >
> >  	if (netif_running(dev)) {
> >  		/* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27
> @@
> > static int __maybe_unused flexcan_suspend(struct device *device)
> >  			if (err)
> >  				return err;
> >  		} else {
> > -			err = flexcan_chip_disable(priv);
> > +			flexcan_chip_stop(dev);
> 
> chip_stop calls chip_disable, but doesn't propagate the error value.
> Please create a seperate patch to propagate the error value of chip_stop().

Okay, I will do it.

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 4/4] can: flexcan: add LPSR mode support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 17:21
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > For i.MX7D LPSR mode, the controller will lost power and got the
> > configuration state lost after system resume back. (coming i.MX8QM/QXP
> > will also completely power off the domain, the controller state will
> > be lost and needs restore).
> > So we need to set pinctrl state again and re-start chip to do
> > re-configuration after resume.
> >
> > For wakeup case, it should not set pinctrl to sleep state by
> > pinctrl_pm_select_sleep_state.
> > For interface is not up before suspend case, we don't need
> > re-configure as it will be configured by user later by interface up.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ------
> > ChangeLog:
> > 	V1->V2: no change.
> > ---
> >  drivers/net/can/flexcan.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index d178146b3da5..d1509cffdd24 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/regmap.h>
> >
> >  #define DRV_NAME			"flexcan"
> > @@ -1707,7 +1708,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 = 0;
> > +	int err;
> >
> >  	if (netif_running(dev)) {
> >  		/* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27
> @@
> > static int __maybe_unused flexcan_suspend(struct device *device)
> >  			if (err)
> >  				return err;
> >  		} else {
> > -			err = flexcan_chip_disable(priv);
> > +			flexcan_chip_stop(dev);
> 
> chip_stop calls chip_disable, but doesn't propagate the error value.
> Please create a seperate patch to propagate the error value of chip_stop().

Hi Marc,

I found not only chip_disable should propagate the error value, others chip_freeze, transceiver_disable also need.'
I cook a patch below, is it fine for you?

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 19602b77907f..c5e4b6928dee 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1263,14 +1263,19 @@ static int flexcan_chip_start(struct net_device *dev)
  *
  * this functions is entered with clocks enabled
  */
-static void flexcan_chip_stop(struct net_device *dev)
+static int flexcan_chip_stop(struct net_device *dev)
 {
        struct flexcan_priv *priv = netdev_priv(dev);
        struct flexcan_regs __iomem *regs = priv->regs;
+       int err;

        /* freeze + disable module */
-       flexcan_chip_freeze(priv);
-       flexcan_chip_disable(priv);
+       err = flexcan_chip_freeze(priv);
+       if (err)
+               return err;
+       err = flexcan_chip_disable(priv);
+       if (err)
+               goto out_chip_unfreeze;

        /* Disable all interrupts */
        priv->write(0, &regs->imask2);
@@ -1278,8 +1283,19 @@ static void flexcan_chip_stop(struct net_device *dev)
        priv->write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
                    &regs->ctrl);

-       flexcan_transceiver_disable(priv);
+       err = flexcan_transceiver_disable(priv);
+       if (err)
+               goto out_chip_enable;
+
        priv->can.state = CAN_STATE_STOPPED;
+
+       return 0;
+
+out_chip_enable:
+       flexcan_chip_enable(priv);
+out_chip_unfreeze:
+       flexcan_chip_unfreeze(priv);
+       return err;
 }

 static int flexcan_open(struct net_device *dev)

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

Hi Sean,

Could you please answer Marc's concern in details when you are free? Then we can discuss how to fix it more reasonable. Thanks.
 
Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年12月4日 16:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
> stage
> 
> On 12/4/19 3:22 AM, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Sent: 2019年12月4日 2:15
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> >> linux-can@vger.kernel.org
> >> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> >> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode
> >> during probe stage
> >>
> >> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> >>> CAN controller could be stucked in stop mode once it enters stop
> >>> mode
> >>                           ^^^^^^^ stuck
> >>> when suspend, and then it fails to exit stop mode when resume.
> >>
> >> How can this happen?
> >
> > I am also confused how can this happen, as I asked Sean, only CAN
> > enter stop mode when suspend, then system hang,
> How do you recover the system when suspended?
> 
> > it could let CAN
> > stuck in stop mode. However, Sean said this indeed happen at his side,
> > @sean@geanix.com, could you explain how this happen in details?
> That would be good.
> 
> >>> Only code reset can get CAN out of stop mode,
> >>
> >> What is "code reset"?
> >
> > As I know, "code reset" is to press the POWER KEY from the board. At
> > my side, reboot command from OS also can get CAN out of stop mode.
> Do you mean "cold reset", also known as Power-On-Reset, POR or power
> cycle?
> 
> What does pressing the POWER KEY do? A power cycle of the system or
> toggling the reset line of the imx?
> 
> We need to describe in detail, as not everyone has the same board as you, and
> these boards might not even have a power key :)
> 
> > Below is experiment I did:
> > 	Firstly, do a hacking to let CAN stuck into stop mode, then:
> 
> You mean you put the CAN into stop mode without keeping track in the CAN
> driver that the CAN-IP is in stop mode, e.g. by hacking the driver.
> 
> Then you try several methods to recover:
> 
> > 	(1) press power on/off key, get CAN out of stop mode;
> > 	(2) reboot command from console, get CAN out of stop mode;
> > 	(3) unbind/bind driver, cannot get CAN out of stop mode;
> > 	(4) remod/insmod module, cannot get CAN out of stop mode;
> 
> (2) resets the complete imx, including the CAN-IP core, (1) probably, too.
> 
> (3) and (4) fail to recover the CAN core, as the IP core is still powered off by
> some upstream component. So the question why this happens in the first place
> is IMHO as important as trying to wake up the core. I think if we discover this
> situation (CAN Core is in stop-mode in probe) we should print a warning
> message, but try to recover.
> 
> >>> so add stop mode remove request during probe stage for other
> >>> methods(soft reset from chip level, unbind/bind driver, etc) to let
> >>         ^^^ please add a space
> >>> CAN active again.
> >>
> >> Can you rephrase the sentence after "so add stop mode remove request
> >> during probe stage". I'm not completely sure what you want to tell.
> >
> > Sure.
> 
> tnx,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Sean Nyekjaer]

On 04/12/2019 09.45, Marc Kleine-Budde wrote:
> On 12/4/19 3:22 AM, Joakim Zhang wrote:
>>
>>> -----Original Message-----
>>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Sent: 2019年12月4日 2:15
>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
>>> linux-can@vger.kernel.org
>>> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
>>> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
>>> stage
>>>
>>> On 11/27/19 6:56 AM, Joakim Zhang wrote:
>>>> CAN controller could be stucked in stop mode once it enters stop mode
>>>                            ^^^^^^^ stuck
>>>> when suspend, and then it fails to exit stop mode when resume.
>>>
>>> How can this happen?
>>
>> I am also confused how can this happen, as I asked Sean, only CAN
>> enter stop mode when suspend, then system hang,
> How do you recover the system when suspended?
> 
>> it could let CAN
>> stuck in stop mode. However, Sean said this indeed happen at his
>> side, @sean@geanix.com, could you explain how this happen in
>> details?
> That would be good.
> 
>>>> Only code reset can get CAN out of stop mode,
>>>
>>> What is "code reset"?
>>
>> As I know, "code reset" is to press the POWER KEY from the board. At
>> my side, reboot command from OS also can get CAN out of stop mode.
> Do you mean "cold reset", also known as Power-On-Reset, POR or power cycle?
> 
> What does pressing the POWER KEY do? A power cycle of the system or
> toggling the reset line of the imx?
> 
> We need to describe in detail, as not everyone has the same board as
> you, and these boards might not even have a power key :)
> 
>> Below is experiment I did:
>> 	Firstly, do a hacking to let CAN stuck into stop mode, then:
> 
> You mean you put the CAN into stop mode without keeping track in the CAN
> driver that the CAN-IP is in stop mode, e.g. by hacking the driver.
> 
> Then you try several methods to recover:
> 
>> 	(1) press power on/off key, get CAN out of stop mode;
>> 	(2) reboot command from console, get CAN out of stop mode;
>> 	(3) unbind/bind driver, cannot get CAN out of stop mode;
>> 	(4) remod/insmod module, cannot get CAN out of stop mode;
> 
> (2) resets the complete imx, including the CAN-IP core, (1) probably, too.
No, if the CAN-IP core is in stop-mode it will stay that way even after 
a reboot from the console.
At least it's what we are seeing in the field.

This could be because we are missing a wire from the watchdog out to the 
RESETBMCU/PWRON on the PMIC.
But i guess a check for if the CAN-Ip is in stop-mode doesn't hurt 
anything :)

/Sean

RE: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年12月5日 17:22
> To: Marc Kleine-Budde <mkl@pengutronix.de>; Joakim Zhang
> <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
> stage
> 
> 
> 
> On 04/12/2019 09.45, Marc Kleine-Budde wrote:
> > On 12/4/19 3:22 AM, Joakim Zhang wrote:
> >>
> >>> -----Original Message-----
> >>> From: Marc Kleine-Budde <mkl@pengutronix.de>
> >>> Sent: 2019年12月4日 2:15
> >>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com;
> >>> linux-can@vger.kernel.org
> >>> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> >>> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode
> >>> during probe stage
> >>>
> >>> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> >>>> CAN controller could be stucked in stop mode once it enters stop
> >>>> mode
> >>>                            ^^^^^^^ stuck
> >>>> when suspend, and then it fails to exit stop mode when resume.
> >>>
> >>> How can this happen?
> >>
> >> I am also confused how can this happen, as I asked Sean, only CAN
> >> enter stop mode when suspend, then system hang,
> > How do you recover the system when suspended?
> >
> >> it could let CAN
> >> stuck in stop mode. However, Sean said this indeed happen at his
> >> side, @sean@geanix.com, could you explain how this happen in details?
> > That would be good.
> >
> >>>> Only code reset can get CAN out of stop mode,
> >>>
> >>> What is "code reset"?
> >>
> >> As I know, "code reset" is to press the POWER KEY from the board. At
> >> my side, reboot command from OS also can get CAN out of stop mode.
> > Do you mean "cold reset", also known as Power-On-Reset, POR or power
> cycle?
> >
> > What does pressing the POWER KEY do? A power cycle of the system or
> > toggling the reset line of the imx?
> >
> > We need to describe in detail, as not everyone has the same board as
> > you, and these boards might not even have a power key :)
> >
> >> Below is experiment I did:
> >> 	Firstly, do a hacking to let CAN stuck into stop mode, then:
> >
> > You mean you put the CAN into stop mode without keeping track in the
> > CAN driver that the CAN-IP is in stop mode, e.g. by hacking the driver.
> >
> > Then you try several methods to recover:
> >
> >> 	(1) press power on/off key, get CAN out of stop mode;
> >> 	(2) reboot command from console, get CAN out of stop mode;
> >> 	(3) unbind/bind driver, cannot get CAN out of stop mode;
> >> 	(4) remod/insmod module, cannot get CAN out of stop mode;
> >
> > (2) resets the complete imx, including the CAN-IP core, (1) probably, too.
> No, if the CAN-IP core is in stop-mode it will stay that way even after a reboot
> from the console.
> At least it's what we are seeing in the field.
> 
> This could be because we are missing a wire from the watchdog out to the
> RESETBMCU/PWRON on the PMIC.
> But i guess a check for if the CAN-Ip is in stop-mode doesn't hurt anything :)
Hi Sean,

At my side, both Power-On-Reset and reboot from console can get CAN-IP out of stop mode, HW is i.MX7D-SDB/i.MX8QXP-mek.
I think HW design could make difference.

We more care about how does CAN-IP stuck in stop mode, could you please explain in details? We want figure out the root cause.

> /Sean

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Sean Nyekjaer]

On 05/12/2019 12.04, Joakim Zhang wrote:
> Hi Sean,
> 
> At my side, both Power-On-Reset and reboot from console can get CAN-IP out of stop mode, HW is i.MX7D-SDB/i.MX8QXP-mek.
> I think HW design could make difference.
> 
> We more care about how does CAN-IP stuck in stop mode, could you please explain in details? We want figure out the root cause.

When running only with the first stop mode commit:
de3578c198c6 ("can: flexcan: add self wakeup support")
And there is incoming traffic on both CAN lines.
I happens when going into suspend.
Then the CAN-IP is stuck in stop mode..

I'm on this custom i.MX6ull board.
I have another custom board also with the i.MX6ull where the watchdog 
pins are wired to the PMIC.
Will try to test on that next week

/Sean

RE: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年12月5日 19:12
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Marc Kleine-Budde
> <mkl@pengutronix.de>; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
> stage
> 
> 
> 
> On 05/12/2019 12.04, Joakim Zhang wrote:
> > Hi Sean,
> >
> > At my side, both Power-On-Reset and reboot from console can get CAN-IP out
> of stop mode, HW is i.MX7D-SDB/i.MX8QXP-mek.
> > I think HW design could make difference.
> >
> > We more care about how does CAN-IP stuck in stop mode, could you please
> explain in details? We want figure out the root cause.
> 
> When running only with the first stop mode commit:
> de3578c198c6 ("can: flexcan: add self wakeup support") And there is incoming
> traffic on both CAN lines.
> I happens when going into suspend.
> Then the CAN-IP is stuck in stop mode..

That means with below patch then CAN-IP could not been stuck in stop mode, right?
	can: flexcan: fix deadlock when using self wakeup

If yes, I think we don't need check stop mode in probe stage, since issue has disappeared automatically.

> I'm on this custom i.MX6ull board.
> I have another custom board also with the i.MX6ull where the watchdog pins
> are wired to the PMIC.
> Will try to test on that next week

Okay, thanks.

Best Regards,
Joakim Zhang
> /Sean

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Sean Nyekjaer]

On 05/12/2019 12.19, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Sean Nyekjaer <sean@geanix.com>
>> Sent: 2019年12月5日 19:12
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Marc Kleine-Budde
>> <mkl@pengutronix.de>; linux-can@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe
>> stage
>>
>>
>>
>> On 05/12/2019 12.04, Joakim Zhang wrote:
>>> Hi Sean,
>>>
>>> At my side, both Power-On-Reset and reboot from console can get CAN-IP out
>> of stop mode, HW is i.MX7D-SDB/i.MX8QXP-mek.
>>> I think HW design could make difference.
>>>
>>> We more care about how does CAN-IP stuck in stop mode, could you please
>> explain in details? We want figure out the root cause.
>>
>> When running only with the first stop mode commit:
>> de3578c198c6 ("can: flexcan: add self wakeup support") And there is incoming
>> traffic on both CAN lines.
>> I happens when going into suspend.
>> Then the CAN-IP is stuck in stop mode..
> 
> That means with below patch then CAN-IP could not been stuck in stop mode, right?
> 	can: flexcan: fix deadlock when using self wakeup
Yes :)
> 
> If yes, I think we don't need check stop mode in probe stage, since issue has disappeared automatically.

If one have devices deployed where:
"can: flexcan: fix deadlock when using self wakeup" isn't applied.
They could have devices stuck in stop-mode.

That's what i meant by this patch doesn't do any harm to have the check 
included.

/Sean

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Marc Kleine-Budde]

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

On 12/5/19 12:32 PM, Sean Nyekjaer wrote:
>> If yes, I think we don't need check stop mode in probe stage, since
>> issue has disappeared automatically.
> 
> If one have devices deployed where:
> "can: flexcan: fix deadlock when using self wakeup" isn't applied.
> They could have devices stuck in stop-mode.

Ok.

But both patches:

    can: flexcan: fix deadlock when using self wakeup
    can: flexcan: try to exit stop mode during probe stage

are not yet mainline, so if "can: flexcan: fix deadlock when using self
wakeup" fixes the problem and goes into stable we don't need "can:
flexcan: try to exit stop mode during probe stage", right?

> That's what i meant by this patch doesn't do any harm to have the check 
> included.

I don't want to have code in the driver that serves no purpose.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Sean Nyekjaer]

On 07/12/2019 21.32, Marc Kleine-Budde wrote:
> On 12/5/19 12:32 PM, Sean Nyekjaer wrote:
>>> If yes, I think we don't need check stop mode in probe stage, since
>>> issue has disappeared automatically.
>>
>> If one have devices deployed where:
>> "can: flexcan: fix deadlock when using self wakeup" isn't applied.
>> They could have devices stuck in stop-mode.
> 
> Ok.
> 
> But both patches:
> 
>      can: flexcan: fix deadlock when using self wakeup
>      can: flexcan: try to exit stop mode during probe stage
> 
> are not yet mainline, so if "can: flexcan: fix deadlock when using self
> wakeup" fixes the problem and goes into stable we don't need "can:
> flexcan: try to exit stop mode during probe stage", right?
> 
>> That's what i meant by this patch doesn't do any harm to have the check
>> included.
> 
> I don't want to have code in the driver that serves no purpose.
> 

Fine with me, I'm continuing to have the patch included in my tree until 
all devices are upgraded with:
	can: flexcan: fix deadlock when using self wakeup

/Sean

> regards,
> Marc
> 

Re: [PATCH V2 2/4] can: flexcan: try to exit stop mode during probe stage

[Marc Kleine-Budde]

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

On 12/8/19 11:47 AM, Sean Nyekjaer wrote:
>> But both patches:
>>
>>      can: flexcan: fix deadlock when using self wakeup
>>      can: flexcan: try to exit stop mode during probe stage
>>
>> are not yet mainline, so if "can: flexcan: fix deadlock when using self
>> wakeup" fixes the problem and goes into stable we don't need "can:
>> flexcan: try to exit stop mode during probe stage", right?
>>
>>> That's what i meant by this patch doesn't do any harm to have the check
>>> included.
>>
>> I don't want to have code in the driver that serves no purpose.
> 
> Fine with me, I'm continuing to have the patch included in my tree until 
> all devices are upgraded with:
> 	can: flexcan: fix deadlock when using self wakeup

That makes sende in your usecase, as you already have "can: flexcan: try
to exit stop mode during probe stage" in your tree.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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