| Message ID | 20240715-flexcan-v2-4-2873014c595a@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | can: fsl,flexcan: add fsl,s32v234-flexcan and imx95 wakeup | expand |
On 15.07.2024 17:27:23, Frank Li wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > iMX95 defines a bit in GPR that sets/unsets the IPG_STOP signal to the > FlexCAN module, controlling its entry into STOP mode. Wakeup should work > even if FlexCAN is in STOP mode. > > Due to iMX95 architecture design, the A-Core cannot access GPR; only the > system manager (SM) can configure GPR. To support the wakeup feature, > follow these steps: > > - For suspend: > 1) During Linux suspend, when CAN suspends, do nothing for GPR and keep > CAN-related clocks on. > 2) In ATF, check whether CAN needs to support wakeup; if yes, send a > request to SM through the SCMI protocol. > 3) In SM, configure the GPR and unset IPG_STOP. > 4) A-Core suspends. > > - For wakeup and resume: > 1) A-Core wakeup event arrives. > 2) In SM, deassert IPG_STOP. > 3) Linux resumes. > > Add a new fsl_imx95_devtype_data and FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI to > reflect this. > > Reviewed-by: Han Xu <han.xu@nxp.com> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/net/can/flexcan/flexcan-core.c | 49 ++++++++++++++++++++++++++++++---- > drivers/net/can/flexcan/flexcan.h | 2 ++ > 2 files changed, 46 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c > index f6e609c388d55..fe972d5b8fbe0 100644 > --- a/drivers/net/can/flexcan/flexcan-core.c > +++ b/drivers/net/can/flexcan/flexcan-core.c > @@ -354,6 +354,14 @@ static struct flexcan_devtype_data fsl_imx93_devtype_data = { > FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, > }; > > +static const struct flexcan_devtype_data fsl_imx95_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_SETUP_STOP_MODE_SCMI | > + FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC | > + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX | > + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, Please keep the flags sorted by their value. > +}; Please add a newline here. > static const struct flexcan_devtype_data fsl_vf610_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_RX_MAILBOX | > @@ -548,6 +556,13 @@ 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_SETUP_STOP_MODE_SCMI) { > + /* For the SCMI mode, driver do nothing, ATF will send request to > + * SM(system manager, M33 core) through SCMI protocol after linux > + * suspend. Once SM get this request, it will send IPG_STOP signal > + * to Flex_CAN, let CAN in STOP mode. > + */ > + return 0; > } > > return flexcan_low_power_enter_ack(priv); > @@ -559,7 +574,11 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv) > u32 reg_mcr; > int ret; > > - /* remove stop request */ > + /* Remove stop request, for FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, > + * do nothing here, because ATF already send request to SM before > + * linux resume. Once SM get this request, it will deassert the > + * IPG_STOP signal to Flex_CAN. > + */ > if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > ret = flexcan_stop_mode_enable_scfw(priv, false); > if (ret < 0) > @@ -1987,6 +2006,9 @@ 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_SETUP_STOP_MODE_SCMI) > + /* ATF will handle all STOP_IPG related work */ > + ret = 0; > else > /* return 0 directly if doesn't support stop mode feature */ > return 0; > @@ -2013,6 +2035,7 @@ static const struct of_device_id flexcan_of_match[] = { > { .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, }, > { .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, }, > { .compatible = "fsl,imx93-flexcan", .data = &fsl_imx93_devtype_data, }, > + { .compatible = "fsl,imx95-flexcan", .data = &fsl_imx95_devtype_data, }, > { .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, }, > { .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, }, > { .compatible = "fsl,imx53-flexcan", .data = &fsl_imx25_devtype_data, }, > @@ -2311,9 +2334,22 @@ 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 FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, it need needs > + * ATF to send request to SM through SCMI protocol, > + * SM will assert the IPG_STOP signal. But all this > + * works need the CAN clocks keep on. > + * After the CAN module get the IPG_STOP mode, and gets > + * switch to STOP mode, whether still keep the CAN switches > + * clocks on or gate them off depend on the Hardware > + * design. > + */ > + if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI) > + return 0; > + } > + > err = pm_runtime_force_suspend(device); > if (err) > return err; > @@ -2330,9 +2366,12 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device) > if (netif_running(dev)) { > int err; > > - err = pm_runtime_force_resume(device); > - if (err) > - return err; > + if (!(device_may_wakeup(device) && ^^^^^^^^^^^^^^^^^^^^^^^^ Where does this come from? > + priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) { > + 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 025c3417031f4..4933d8c7439e6 100644 > --- a/drivers/net/can/flexcan/flexcan.h > +++ b/drivers/net/can/flexcan/flexcan.h > @@ -68,6 +68,8 @@ > #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15) > /* Device supports RX via FIFO */ > #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16) > +/* Setup stop mode with ATF SCMI protocol to support wakeup */ > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17) > > struct flexcan_devtype_data { > u32 quirks; /* quirks needed for different IP cores */ > > -- > 2.34.1 > > regards, Marc
On Tue, Jul 16, 2024 at 09:06:14AM +0200, Marc Kleine-Budde wrote: > On 15.07.2024 17:27:23, Frank Li wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > iMX95 defines a bit in GPR that sets/unsets the IPG_STOP signal to the > > FlexCAN module, controlling its entry into STOP mode. Wakeup should work > > even if FlexCAN is in STOP mode. > > > > Due to iMX95 architecture design, the A-Core cannot access GPR; only the > > system manager (SM) can configure GPR. To support the wakeup feature, > > follow these steps: > > > > - For suspend: > > 1) During Linux suspend, when CAN suspends, do nothing for GPR and keep > > CAN-related clocks on. > > 2) In ATF, check whether CAN needs to support wakeup; if yes, send a > > request to SM through the SCMI protocol. > > 3) In SM, configure the GPR and unset IPG_STOP. > > 4) A-Core suspends. > > > > - For wakeup and resume: > > 1) A-Core wakeup event arrives. > > 2) In SM, deassert IPG_STOP. > > 3) Linux resumes. > > > > Add a new fsl_imx95_devtype_data and FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI to > > reflect this. > > > > Reviewed-by: Han Xu <han.xu@nxp.com> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/net/can/flexcan/flexcan-core.c | 49 ++++++++++++++++++++++++++++++---- > > drivers/net/can/flexcan/flexcan.h | 2 ++ > > 2 files changed, 46 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c > > index f6e609c388d55..fe972d5b8fbe0 100644 > > --- a/drivers/net/can/flexcan/flexcan-core.c > > +++ b/drivers/net/can/flexcan/flexcan-core.c > > @@ -354,6 +354,14 @@ static struct flexcan_devtype_data fsl_imx93_devtype_data = { > > FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, > > }; > > > > +static const struct flexcan_devtype_data fsl_imx95_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_SETUP_STOP_MODE_SCMI | > > + FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC | > > + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX | > > + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, > > Please keep the flags sorted by their value. > > > +}; > > Please add a newline here. > > > static const struct flexcan_devtype_data fsl_vf610_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_RX_MAILBOX | > > @@ -548,6 +556,13 @@ 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_SETUP_STOP_MODE_SCMI) { > > + /* For the SCMI mode, driver do nothing, ATF will send request to > > + * SM(system manager, M33 core) through SCMI protocol after linux > > + * suspend. Once SM get this request, it will send IPG_STOP signal > > + * to Flex_CAN, let CAN in STOP mode. > > + */ > > + return 0; > > } > > > > return flexcan_low_power_enter_ack(priv); > > @@ -559,7 +574,11 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv) > > u32 reg_mcr; > > int ret; > > > > - /* remove stop request */ > > + /* Remove stop request, for FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, > > + * do nothing here, because ATF already send request to SM before > > + * linux resume. Once SM get this request, it will deassert the > > + * IPG_STOP signal to Flex_CAN. > > + */ > > if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > > ret = flexcan_stop_mode_enable_scfw(priv, false); > > if (ret < 0) > > @@ -1987,6 +2006,9 @@ 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_SETUP_STOP_MODE_SCMI) > > + /* ATF will handle all STOP_IPG related work */ > > + ret = 0; > > else > > /* return 0 directly if doesn't support stop mode feature */ > > return 0; > > @@ -2013,6 +2035,7 @@ static const struct of_device_id flexcan_of_match[] = { > > { .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, }, > > { .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, }, > > { .compatible = "fsl,imx93-flexcan", .data = &fsl_imx93_devtype_data, }, > > + { .compatible = "fsl,imx95-flexcan", .data = &fsl_imx95_devtype_data, }, > > { .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, }, > > { .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, }, > > { .compatible = "fsl,imx53-flexcan", .data = &fsl_imx25_devtype_data, }, > > @@ -2311,9 +2334,22 @@ 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 FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, it need > needs > > + * ATF to send request to SM through SCMI protocol, > > + * SM will assert the IPG_STOP signal. But all this > > + * works need the CAN clocks keep on. > > + * After the CAN module get the IPG_STOP mode, and > gets > > + * switch to STOP mode, whether still keep the CAN > switches > > + * clocks on or gate them off depend on the Hardware > > + * design. > > + */ > > + if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI) > > + return 0; > > + } > > + > > err = pm_runtime_force_suspend(device); > > if (err) > > return err; > > @@ -2330,9 +2366,12 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device) > > if (netif_running(dev)) { > > int err; > > > > - err = pm_runtime_force_resume(device); > > - if (err) > > - return err; > > + if (!(device_may_wakeup(device) && > ^^^^^^^^^^^^^^^^^^^^^^^^ > > Where does this come from? include/linux/pm_wakeup.h static inline bool device_may_wakeup(struct device *dev) { return dev->power.can_wakeup && !!dev->power.wakeup; } Frank > > > + priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) { > > + 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 025c3417031f4..4933d8c7439e6 100644 > > --- a/drivers/net/can/flexcan/flexcan.h > > +++ b/drivers/net/can/flexcan/flexcan.h > > @@ -68,6 +68,8 @@ > > #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15) > > /* Device supports RX via FIFO */ > > #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16) > > +/* Setup stop mode with ATF SCMI protocol to support wakeup */ > > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17) > > > > struct flexcan_devtype_data { > > u32 quirks; /* quirks needed for different IP cores */ > > > > -- > > 2.34.1 > > > > > > regards, > 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 |
On 16.07.2024 10:40:31, Frank Li wrote: > > > @@ -2330,9 +2366,12 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device) > > > if (netif_running(dev)) { > > > int err; > > > > > > - err = pm_runtime_force_resume(device); > > > - if (err) > > > - return err; > > > + if (!(device_may_wakeup(device) && > > ^^^^^^^^^^^^^^^^^^^^^^^^ > > > > Where does this come from? > > include/linux/pm_wakeup.h > > static inline bool device_may_wakeup(struct device *dev) > { > return dev->power.can_wakeup && !!dev->power.wakeup; > } Sorry for the confusion. I wanted to point out, that the original driver doesn't have the check to device_may_wakeup(). Why was this added? > > > > > + priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) { > > > + 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 025c3417031f4..4933d8c7439e6 100644 > > > --- a/drivers/net/can/flexcan/flexcan.h > > > +++ b/drivers/net/can/flexcan/flexcan.h > > > @@ -68,6 +68,8 @@ > > > #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15) > > > /* Device supports RX via FIFO */ > > > #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16) > > > +/* Setup stop mode with ATF SCMI protocol to support wakeup */ > > > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17) > > > > > > struct flexcan_devtype_data { > > > u32 quirks; /* quirks needed for different IP cores */ regards, Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2024年7月16日 22:46 > To: Frank Li <frank.li@nxp.com> > Cc: Vincent Mailhol <mailhol.vincent@wanadoo.fr>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; linux-can@vger.kernel.org; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Bough Chen > <haibo.chen@nxp.com>; imx@lists.linux.dev; Han Xu <han.xu@nxp.com> > Subject: Re: [PATCH v2 4/4] can: flexcan: add wakeup support for imx95 > > On 16.07.2024 10:40:31, Frank Li wrote: > > > > @@ -2330,9 +2366,12 @@ static int __maybe_unused > flexcan_noirq_resume(struct device *device) > > > > if (netif_running(dev)) { > > > > int err; > > > > > > > > - err = pm_runtime_force_resume(device); > > > > - if (err) > > > > - return err; > > > > + if (!(device_may_wakeup(device) && > > > ^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > Where does this come from? > > > > include/linux/pm_wakeup.h > > > > static inline bool device_may_wakeup(struct device *dev) > > { > > return dev->power.can_wakeup && !!dev->power.wakeup; > > } > > Sorry for the confusion. I wanted to point out, that the original driver doesn't > have the check to device_may_wakeup(). Why was this added? Hi Marc, Here add this to make sure for CAN with flag FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI and really used as wakeup source, do not need to call pm_runtime_force_resume(), keep it align with what we do in flexcan_noirq_suspend. As the comment in flexcan_noirq_suspend, CAN with flag FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, when used as wakeup source, need to keep CAN clock on when system suspend, let ATF part logic works, detail steps please refer to this patch commit log. Whether gate off the CAN clock or not depends on the Hardware design. So for this case, in flexcan_noirq_suspend, directly return0, do not call the pm_runtime_force_suspend(), then in flexcan_noirq_resume(), use the same logic to skip the pm_runtime_force_resume(). Best Regards Haibo Chen > > > > > > > > + priv->devtype_data.quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) { > > > > + 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 025c3417031f4..4933d8c7439e6 100644 > > > > --- a/drivers/net/can/flexcan/flexcan.h > > > > +++ b/drivers/net/can/flexcan/flexcan.h > > > > @@ -68,6 +68,8 @@ > > > > #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15) > > > > /* Device supports RX via FIFO */ #define > > > > FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16) > > > > +/* Setup stop mode with ATF SCMI protocol to support wakeup */ > > > > +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17) > > > > > > > > struct flexcan_devtype_data { > > > > u32 quirks; /* quirks needed for different IP cores */ > > regards, > 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 |
On 17.07.2024 02:03:35, Bough Chen wrote: > > On 16.07.2024 10:40:31, Frank Li wrote: > > > > > @@ -2330,9 +2366,12 @@ static int __maybe_unused > > flexcan_noirq_resume(struct device *device) > > > > > if (netif_running(dev)) { > > > > > int err; > > > > > > > > > > - err = pm_runtime_force_resume(device); > > > > > - if (err) > > > > > - return err; > > > > > + if (!(device_may_wakeup(device) && > > > > ^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > > > Where does this come from? > > > > > > include/linux/pm_wakeup.h > > > > > > static inline bool device_may_wakeup(struct device *dev) > > > { > > > return dev->power.can_wakeup && !!dev->power.wakeup; > > > } > > > > Sorry for the confusion. I wanted to point out, that the original driver doesn't > > have the check to device_may_wakeup(). Why was this added? > > Here add this to make sure for CAN with flag > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI and really used as wakeup source, > do not need to call pm_runtime_force_resume(), keep it align with what > we do in flexcan_noirq_suspend. > As the comment in flexcan_noirq_suspend, CAN with flag > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, when used as wakeup source, need > to keep CAN clock on when system suspend, let ATF part logic works, > detail steps please refer to this patch commit log. Whether gate off > the CAN clock or not depends on the Hardware design. So for this case, > in flexcan_noirq_suspend, directly return0, do not call the > pm_runtime_force_suspend(), then in flexcan_noirq_resume(), use the > same logic to skip the pm_runtime_force_resume(). Please change the control flow, so that flexcan_noirq_suspend() and flexcan_noirq_resume() look symmetrical. regards, Marc
diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c index f6e609c388d55..fe972d5b8fbe0 100644 --- a/drivers/net/can/flexcan/flexcan-core.c +++ b/drivers/net/can/flexcan/flexcan-core.c @@ -354,6 +354,14 @@ static struct flexcan_devtype_data fsl_imx93_devtype_data = { FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, }; +static const struct flexcan_devtype_data fsl_imx95_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_SETUP_STOP_MODE_SCMI | + FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC | + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX | + FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR, +}; static const struct flexcan_devtype_data fsl_vf610_devtype_data = { .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_RX_MAILBOX | @@ -548,6 +556,13 @@ 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_SETUP_STOP_MODE_SCMI) { + /* For the SCMI mode, driver do nothing, ATF will send request to + * SM(system manager, M33 core) through SCMI protocol after linux + * suspend. Once SM get this request, it will send IPG_STOP signal + * to Flex_CAN, let CAN in STOP mode. + */ + return 0; } return flexcan_low_power_enter_ack(priv); @@ -559,7 +574,11 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv) u32 reg_mcr; int ret; - /* remove stop request */ + /* Remove stop request, for FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, + * do nothing here, because ATF already send request to SM before + * linux resume. Once SM get this request, it will deassert the + * IPG_STOP signal to Flex_CAN. + */ if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { ret = flexcan_stop_mode_enable_scfw(priv, false); if (ret < 0) @@ -1987,6 +2006,9 @@ 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_SETUP_STOP_MODE_SCMI) + /* ATF will handle all STOP_IPG related work */ + ret = 0; else /* return 0 directly if doesn't support stop mode feature */ return 0; @@ -2013,6 +2035,7 @@ static const struct of_device_id flexcan_of_match[] = { { .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, }, { .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, }, { .compatible = "fsl,imx93-flexcan", .data = &fsl_imx93_devtype_data, }, + { .compatible = "fsl,imx95-flexcan", .data = &fsl_imx95_devtype_data, }, { .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, }, { .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, }, { .compatible = "fsl,imx53-flexcan", .data = &fsl_imx25_devtype_data, }, @@ -2311,9 +2334,22 @@ 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 FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, it need + * ATF to send request to SM through SCMI protocol, + * SM will assert the IPG_STOP signal. But all this + * works need the CAN clocks keep on. + * After the CAN module get the IPG_STOP mode, and + * switch to STOP mode, whether still keep the CAN + * clocks on or gate them off depend on the Hardware + * design. + */ + if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI) + return 0; + } + err = pm_runtime_force_suspend(device); if (err) return err; @@ -2330,9 +2366,12 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device) if (netif_running(dev)) { int err; - err = pm_runtime_force_resume(device); - if (err) - return err; + if (!(device_may_wakeup(device) && + priv->devtype_data.quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) { + 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 025c3417031f4..4933d8c7439e6 100644 --- a/drivers/net/can/flexcan/flexcan.h +++ b/drivers/net/can/flexcan/flexcan.h @@ -68,6 +68,8 @@ #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15) /* Device supports RX via FIFO */ #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16) +/* Setup stop mode with ATF SCMI protocol to support wakeup */ +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17) struct flexcan_devtype_data { u32 quirks; /* quirks needed for different IP cores */