diff mbox series

[2/2] can: flexcan: remove the auto stop mode for IMX93

Message ID 20230726035032.3073951-2-haibo.chen@nxp.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [1/2] arm64: dts: imx93: add the Flex-CAN stop mode by GPR | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1342 this patch: 1342
netdev/cc_maintainers warning 1 maintainers not CCed: mailhol.vincent@wanadoo.fr
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1365 this patch: 1365
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Bough Chen July 26, 2023, 3:50 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

IMX93 A0 chip involve the internal q-channel handshake in LPCG and
CCM to automatically handle the Flex-CAN IPG STOP signal. Only after
FLEX-CAN enter stop mode then can support the self-wakeup feature.

But meet issue when do the continue system PM stress test. When config
the CAN as wakeup source, the first time after system suspend, any data
on CAN bus can wakeup the system, this is as expect. But the second time
when system suspend, data on CAN bus can't wakeup the system. If continue
this test, we find in odd time system enter suspend, CAN can wakeup the
system, but in even number system enter suspend, CAN can't wakeup the
system.

IC find a bug in the auto stop mode logic when handle the q-channel, and
can't fix it easily. So for the new imx93 A1, IC drop the auto stop mode
and involve the GPR to support stop mode (used before). IC define a bit
in GPR which can trigger the IPG STOP signal to Flex-CAN, let it go into
stop mode.

Now NXP claim to drop IMX93 A0, and only support IMX93 A1. So this patch
remove the auto stop mode, and add flag FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR
to imx93.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 37 ++++----------------------
 drivers/net/can/flexcan/flexcan.h      |  2 --
 2 files changed, 5 insertions(+), 34 deletions(-)

Comments

Marc Kleine-Budde July 26, 2023, 6:35 a.m. UTC | #1 
On 26.07.2023 11:50:32, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> IMX93 A0 chip involve the internal q-channel handshake in LPCG and
> CCM to automatically handle the Flex-CAN IPG STOP signal. Only after
> FLEX-CAN enter stop mode then can support the self-wakeup feature.
> 
> But meet issue when do the continue system PM stress test. When config
> the CAN as wakeup source, the first time after system suspend, any data
> on CAN bus can wakeup the system, this is as expect. But the second time
> when system suspend, data on CAN bus can't wakeup the system. If continue
> this test, we find in odd time system enter suspend, CAN can wakeup the
> system, but in even number system enter suspend, CAN can't wakeup the
> system.
> 
> IC find a bug in the auto stop mode logic when handle the q-channel, and
> can't fix it easily. So for the new imx93 A1, IC drop the auto stop mode
> and involve the GPR to support stop mode (used before). IC define a bit
> in GPR which can trigger the IPG STOP signal to Flex-CAN, let it go into
> stop mode.
> 
> Now NXP claim to drop IMX93 A0, and only support IMX93 A1. So this patch
> remove the auto stop mode, and add flag FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR
> to imx93.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 37 ++++----------------------
>  drivers/net/can/flexcan/flexcan.h      |  2 --
>  2 files changed, 5 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index ff0fc18baf13..a3f3a9c909be 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -348,7 +348,7 @@ static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
>  static struct flexcan_devtype_data fsl_imx93_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_RX_MAILBOX |
> -		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_AUTO_STOP_MODE |
> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR |

AFAICS this change breaks systems with old device trees (i.e. without
"fsl,stop-mode") and new kernels. The flexcan driver will not probe
anymore.

Marc
Bough Chen July 26, 2023, 8:04 a.m. UTC | #2 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2023年7月26日 14:36
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> wg@grandegger.com; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; devicetree@vger.kernel.org;
> linux-can@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH 2/2] can: flexcan: remove the auto stop mode for IMX93
> 
> On 26.07.2023 11:50:32, haibo.chen@nxp.com wrote:
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > IMX93 A0 chip involve the internal q-channel handshake in LPCG and CCM
> > to automatically handle the Flex-CAN IPG STOP signal. Only after
> > FLEX-CAN enter stop mode then can support the self-wakeup feature.
> >
> > But meet issue when do the continue system PM stress test. When config
> > the CAN as wakeup source, the first time after system suspend, any
> > data on CAN bus can wakeup the system, this is as expect. But the
> > second time when system suspend, data on CAN bus can't wakeup the
> > system. If continue this test, we find in odd time system enter
> > suspend, CAN can wakeup the system, but in even number system enter
> > suspend, CAN can't wakeup the system.
> >
> > IC find a bug in the auto stop mode logic when handle the q-channel,
> > and can't fix it easily. So for the new imx93 A1, IC drop the auto
> > stop mode and involve the GPR to support stop mode (used before). IC
> > define a bit in GPR which can trigger the IPG STOP signal to Flex-CAN,
> > let it go into stop mode.
> >
> > Now NXP claim to drop IMX93 A0, and only support IMX93 A1. So this
> > patch remove the auto stop mode, and add flag
> > FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR to imx93.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/net/can/flexcan/flexcan-core.c | 37 ++++----------------------
> >  drivers/net/can/flexcan/flexcan.h      |  2 --
> >  2 files changed, 5 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan/flexcan-core.c
> > b/drivers/net/can/flexcan/flexcan-core.c
> > index ff0fc18baf13..a3f3a9c909be 100644
> > --- a/drivers/net/can/flexcan/flexcan-core.c
> > +++ b/drivers/net/can/flexcan/flexcan-core.c
> > @@ -348,7 +348,7 @@ static struct flexcan_devtype_data
> > fsl_imx8mp_devtype_data = {  static struct flexcan_devtype_data
> fsl_imx93_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> >  		FLEXCAN_QUIRK_DISABLE_MECR |
> FLEXCAN_QUIRK_USE_RX_MAILBOX |
> > -		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> FLEXCAN_QUIRK_AUTO_STOP_MODE |
> > +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR
> > +|
> 
> AFAICS this change breaks systems with old device trees (i.e. without
> "fsl,stop-mode") and new kernels. The flexcan driver will not probe anymore.

You are right. Lack of "fsl,stop-mode" should not break the flexcan driver probe.
I will send V2 patch to fix this.

Best Regards
Haibo Chen
> 
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
diff mbox series

Patch

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index ff0fc18baf13..a3f3a9c909be 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -348,7 +348,7 @@  static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
 static struct flexcan_devtype_data fsl_imx93_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_RX_MAILBOX |
-		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_AUTO_STOP_MODE |
+		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR |
 		FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC |
 		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
 		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
@@ -544,11 +544,6 @@  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 	} else if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
 		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
 				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
-	} else if (priv->devtype_data.quirks & FLEXCAN_QUIRK_AUTO_STOP_MODE) {
-		/* For the auto stop mode, software do nothing, hardware will cover
-		 * all the operation automatically after system go into low power mode.
-		 */
-		return 0;
 	}
 
 	return flexcan_low_power_enter_ack(priv);
@@ -574,12 +569,6 @@  static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
 	priv->write(reg_mcr, &regs->mcr);
 
-	/* For the auto stop mode, hardware will exist stop mode
-	 * automatically after system go out of low power mode.
-	 */
-	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_AUTO_STOP_MODE)
-		return 0;
-
 	return flexcan_low_power_exit_ack(priv);
 }
 
@@ -1994,8 +1983,6 @@  static int flexcan_setup_stop_mode(struct platform_device *pdev)
 		ret = flexcan_setup_stop_mode_scfw(pdev);
 	else if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR)
 		ret = flexcan_setup_stop_mode_gpr(pdev);
-	else if (priv->devtype_data.quirks & FLEXCAN_QUIRK_AUTO_STOP_MODE)
-		ret = 0;
 	else
 		/* return 0 directly if doesn't support stop mode feature */
 		return 0;
@@ -2320,16 +2307,8 @@  static int __maybe_unused flexcan_noirq_suspend(struct device *device)
 	if (netif_running(dev)) {
 		int err;
 
-		if (device_may_wakeup(device)) {
+		if (device_may_wakeup(device))
 			flexcan_enable_wakeup_irq(priv, true);
-			/* For auto stop mode, need to keep the clock on before
-			 * system go into low power mode. After system go into
-			 * low power mode, hardware will config the flexcan into
-			 * stop mode, and gate off the clock automatically.
-			 */
-			if (priv->devtype_data.quirks & FLEXCAN_QUIRK_AUTO_STOP_MODE)
-				return 0;
-		}
 
 		err = pm_runtime_force_suspend(device);
 		if (err)
@@ -2347,15 +2326,9 @@  static int __maybe_unused flexcan_noirq_resume(struct device *device)
 	if (netif_running(dev)) {
 		int err;
 
-		/* For the wakeup in auto stop mode, no need to gate on the
-		 * clock here, hardware will do this automatically.
-		 */
-		if (!(device_may_wakeup(device) &&
-		      priv->devtype_data.quirks & FLEXCAN_QUIRK_AUTO_STOP_MODE)) {
-			err = pm_runtime_force_resume(device);
-			if (err)
-				return err;
-		}
+		err = pm_runtime_force_resume(device);
+		if (err)
+			return err;
 
 		if (device_may_wakeup(device))
 			flexcan_enable_wakeup_irq(priv, false);
diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
index 91402977780b..025c3417031f 100644
--- a/drivers/net/can/flexcan/flexcan.h
+++ b/drivers/net/can/flexcan/flexcan.h
@@ -68,8 +68,6 @@ 
 #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15)
 /* Device supports RX via FIFO */
 #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
-/* auto enter stop mode to support wakeup */
-#define FLEXCAN_QUIRK_AUTO_STOP_MODE BIT(17)
 
 struct flexcan_devtype_data {
 	u32 quirks;		/* quirks needed for different IP cores */