[PATCH 0/6] can: flexcan: add stop mode support for i.MX8QM

[PATCH 0/6] can: flexcan: add stop mode support for i.MX8QM

[Joakim Zhang]

The first patch from Liu Ying aims to export SCU symbols for SoCs w/wo
SCU, so that no need to check CONFIG_IMX_SCU in the specific driver.

The following patches are for flexcan to add stop mode support for
i.MX8QM.

Joakim Zhang (5):
  dt-bindings: can: flexcan: fix fsl,clk-source property
  dt-bindings: can: flexcan: add fsl,can-index property to indicate a
    resource
  can: flexcan: rename macro FLEXCAN_QUIRK_SETUP_STOP_MODE ->
    FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR
  can: flexcan: add CAN wakeup function for i.MX8QM
  can: flexcan: fix ECC function on LS1021A/LX2160A

Liu Ying (1):
  firmware: imx: always export SCU symbols

 .../bindings/net/can/fsl-flexcan.txt          |   7 +-
 drivers/net/can/flexcan.c                     | 143 ++++++++++++++----
 include/linux/firmware/imx/ipc.h              |  15 ++
 include/linux/firmware/imx/svc/misc.h         |  23 +++
 4 files changed, 160 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH 1/6] firmware: imx: always export SCU symbols

[Joakim Zhang]

From: Liu Ying <victor.liu@nxp.com>

Always export SCU symbols for both SCU SoCs and non-SCU SoCs to avoid
build error.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 include/linux/firmware/imx/ipc.h      | 15 +++++++++++++++
 include/linux/firmware/imx/svc/misc.h | 23 +++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/firmware/imx/ipc.h b/include/linux/firmware/imx/ipc.h
index 891057434858..300fa253fc30 100644
--- a/include/linux/firmware/imx/ipc.h
+++ b/include/linux/firmware/imx/ipc.h
@@ -34,6 +34,7 @@ struct imx_sc_rpc_msg {
 	uint8_t func;
 };
 
+#if IS_ENABLED(CONFIG_IMX_SCU)
 /*
  * This is an function to send an RPC message over an IPC channel.
  * It is called by client-side SCFW API function shims.
@@ -55,4 +56,18 @@ int imx_scu_call_rpc(struct imx_sc_ipc *ipc, void *msg, bool have_resp);
  * @return Returns an error code (0 = success, failed if < 0)
  */
 int imx_scu_get_handle(struct imx_sc_ipc **ipc);
+
+#else
+static inline int
+imx_scu_call_rpc(struct imx_sc_ipc *ipc, void *msg, bool have_resp)
+{
+	return -EIO;
+}
+
+static inline int imx_scu_get_handle(struct imx_sc_ipc **ipc)
+{
+	return -EIO;
+}
+#endif
+
 #endif /* _SC_IPC_H */
diff --git a/include/linux/firmware/imx/svc/misc.h b/include/linux/firmware/imx/svc/misc.h
index 031dd4d3c766..d255048f17de 100644
--- a/include/linux/firmware/imx/svc/misc.h
+++ b/include/linux/firmware/imx/svc/misc.h
@@ -46,6 +46,7 @@ enum imx_misc_func {
  * Control Functions
  */
 
+#if IS_ENABLED(CONFIG_IMX_SCU)
 int imx_sc_misc_set_control(struct imx_sc_ipc *ipc, u32 resource,
 			    u8 ctrl, u32 val);
 
@@ -55,4 +56,26 @@ int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
 int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
 			bool enable, u64 phys_addr);
 
+#else
+static inline int
+imx_sc_misc_set_control(struct imx_sc_ipc *ipc, u32 resource,
+			u8 ctrl, u32 val)
+{
+	return -EIO;
+}
+
+static inline int
+imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
+			u8 ctrl, u32 *val)
+{
+	return -EIO;
+}
+
+static inline int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
+				      bool enable, u64 phys_addr)
+{
+	return -EIO;
+}
+#endif
+
 #endif /* _SC_MISC_API_H */
-- 
2.17.1

[PATCH 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property

[Joakim Zhang]

Correct fsl,clk-source example since flexcan driver uses "of_property_read_u8"
to get this property.

Fixes: 9d733992772d ("dt-bindings: can: flexcan: add PE clock source property to device tree")
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index e10b6eb955e1..6af67f5e581c 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -53,5 +53,5 @@ Example:
 		interrupts = <48 0x2>;
 		interrupt-parent = <&mpic>;
 		clock-frequency = <200000000>; // filled in by bootloader
-		fsl,clk-source = <0>; // select clock source 0 for PE
+		fsl,clk-source = /bits/ 8 <0>; // select clock source 0 for PE
 	};
-- 
2.17.1

Re: [PATCH 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> Correct fsl,clk-source example since flexcan driver uses "of_property_read_u8"
> to get this property.

Hopefully today Oleksij will post the next round of the yaml bindings conversion
patch. Please resping when Oleksij's patch is applied.

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 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 14:22
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: devicetree@vger.kernel.org; Peng Fan <peng.fan@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; netdev@vger.kernel.org; Pankaj Bansal
> <pankaj.bansal@nxp.com>; linux-kernel@vger.kernel.org;
> linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Oleksij Rempel <ore@pengutronix.de>
> Subject: Re: [PATCH 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property
> 
> On 10/16/20 3:43 PM, Joakim Zhang wrote:
> > Correct fsl,clk-source example since flexcan driver uses
> "of_property_read_u8"
> > to get this property.
> 
> Hopefully today Oleksij will post the next round of the yaml bindings conversion
> patch. Please resping when Oleksij's patch is applied.

Of course.

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 |

[PATCH 3/6] dt-bindings: can: flexcan: add fsl,can-index property to indicate a resource

[Joakim Zhang]

For SoCs with SCU support, need setup stop mode via SCU firmware,
so this property can help indicate a resource.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index 6af67f5e581c..839c0c0064a2 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -43,6 +43,10 @@ Optional properties:
 		  0: clock source 0 (oscillator clock)
 		  1: clock source 1 (peripheral clock)
 
+- fsl,can-index: The index of CAN instance.
+                 For SoCs with SCU support, need setup stop mode via SCU firmware,
+                 so this property can help indicate a resource.
+
 - wakeup-source: enable CAN remote wakeup
 
 Example:
@@ -54,4 +58,5 @@ Example:
 		interrupt-parent = <&mpic>;
 		clock-frequency = <200000000>; // filled in by bootloader
 		fsl,clk-source = /bits/ 8 <0>; // select clock source 0 for PE
+		fsl,can-index = /bits/ 8 <1>; // the second CAN instance
 	};
-- 
2.17.1

Re: [PATCH 3/6] dt-bindings: can: flexcan: add fsl, can-index property to indicate a resource

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> For SoCs with SCU support, need setup stop mode via SCU firmware,
> so this property can help indicate a resource.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 6af67f5e581c..839c0c0064a2 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -43,6 +43,10 @@ Optional properties:
>  		  0: clock source 0 (oscillator clock)
>  		  1: clock source 1 (peripheral clock)
>  
> +- fsl,can-index: The index of CAN instance.
> +                 For SoCs with SCU support, need setup stop mode via SCU firmware,
> +                 so this property can help indicate a resource.

This property is not CAN specific. So the name could be more general.

> +
>  - wakeup-source: enable CAN remote wakeup
>  
>  Example:
> @@ -54,4 +58,5 @@ Example:
>  		interrupt-parent = <&mpic>;
>  		clock-frequency = <200000000>; // filled in by bootloader
>  		fsl,clk-source = /bits/ 8 <0>; // select clock source 0 for PE
> +		fsl,can-index = /bits/ 8 <1>; // the second CAN instance
>  	};
> 

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 3/6] dt-bindings: can: flexcan: add fsl, can-index property to indicate a resource

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 14:20
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: devicetree@vger.kernel.org; Peng Fan <peng.fan@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; netdev@vger.kernel.org; Pankaj Bansal
> <pankaj.bansal@nxp.com>; linux-kernel@vger.kernel.org;
> linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de
> Subject: Re: [PATCH 3/6] dt-bindings: can: flexcan: add fsl, can-index property
> to indicate a resource
> 
> On 10/16/20 3:43 PM, Joakim Zhang wrote:
> > For SoCs with SCU support, need setup stop mode via SCU firmware, so
> > this property can help indicate a resource.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > index 6af67f5e581c..839c0c0064a2 100644
> > --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > @@ -43,6 +43,10 @@ Optional properties:
> >  		  0: clock source 0 (oscillator clock)
> >  		  1: clock source 1 (peripheral clock)
> >
> > +- fsl,can-index: The index of CAN instance.
> > +                 For SoCs with SCU support, need setup stop mode via
> SCU firmware,
> > +                 so this property can help indicate a resource.
> 
> This property is not CAN specific. So the name could be more general.

How about "fsl,index"?

Best Regards,
Joakim Zhang
> > +
> >  - wakeup-source: enable CAN remote wakeup
> >
> >  Example:
> > @@ -54,4 +58,5 @@ Example:
> >  		interrupt-parent = <&mpic>;
> >  		clock-frequency = <200000000>; // filled in by bootloader
> >  		fsl,clk-source = /bits/ 8 <0>; // select clock source 0 for PE
> > +		fsl,can-index = /bits/ 8 <1>; // the second CAN instance
> >  	};
> >
> 
> 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 3/6] dt-bindings: can: flexcan: add fsl, can-index property to indicate a resource

[Marc Kleine-Budde]

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

On 10/16/20 8:52 AM, Joakim Zhang wrote:
> 
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Sent: 2020年10月16日 14:20
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
>> shawnguo@kernel.org; s.hauer@pengutronix.de
>> Cc: devicetree@vger.kernel.org; Peng Fan <peng.fan@nxp.com>; Ying Liu
>> <victor.liu@nxp.com>; netdev@vger.kernel.org; Pankaj Bansal
>> <pankaj.bansal@nxp.com>; linux-kernel@vger.kernel.org;
>> linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
>> kernel@pengutronix.de
>> Subject: Re: [PATCH 3/6] dt-bindings: can: flexcan: add fsl, can-index property
>> to indicate a resource
>>
>> On 10/16/20 3:43 PM, Joakim Zhang wrote:
>>> For SoCs with SCU support, need setup stop mode via SCU firmware, so
>>> this property can help indicate a resource.
>>>
>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> index 6af67f5e581c..839c0c0064a2 100644
>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> @@ -43,6 +43,10 @@ Optional properties:
>>>  		  0: clock source 0 (oscillator clock)
>>>  		  1: clock source 1 (peripheral clock)
>>>
>>> +- fsl,can-index: The index of CAN instance.
>>> +                 For SoCs with SCU support, need setup stop mode via
>> SCU firmware,
>>> +                 so this property can help indicate a resource.
>>
>> This property is not CAN specific. So the name could be more general.
> 
> How about "fsl,index"?

Maybe something with "scu", as it's specific to the SCU firmware.

I think it's up to Rob's and the DT people.

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

[PATCH 4/6] can: flexcan: rename macro FLEXCAN_QUIRK_SETUP_STOP_MODE -> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR

[Joakim Zhang]

This patch intends to rename FLEXCAN_QUIRK_SETUP_STOP_MODE quirk
to FLEXCAN_QUIRK_SETUP_STOP_MODE_GRP for non-scu SoCs, coming patch will
add quirk for scu SoCs.

For non-scu SoCs, setup stop mode with GPR register.
For scu SoCs, setup stop mode with SCU firmware.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 4d594e977497..e708e7bf28db 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -236,8 +236,8 @@
 #define FLEXCAN_QUIRK_BROKEN_PERR_STATE BIT(6)
 /* default to BE register access */
 #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN BIT(7)
-/* Setup stop mode to support wakeup */
-#define FLEXCAN_QUIRK_SETUP_STOP_MODE BIT(8)
+/* Setup stop mode with GPR to support wakeup */
+#define FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR BIT(8)
 /* Support CAN-FD mode */
 #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
 /* support memory detection and correction */
@@ -381,7 +381,7 @@ static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
 static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
-		FLEXCAN_QUIRK_SETUP_STOP_MODE,
+		FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR,
 };
 
 static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
@@ -393,7 +393,7 @@ static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
 static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
-		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SETUP_STOP_MODE |
+		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR |
 		FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC,
 };
 
@@ -2043,7 +2043,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	of_can_transceiver(dev);
 	devm_can_led_init(dev);
 
-	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE) {
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
 		err = flexcan_setup_stop_mode(pdev);
 		if (err)
 			dev_dbg(&pdev->dev, "failed to setup stop-mode\n");
-- 
2.17.1

Re: [PATCH 4/6] can: flexcan: rename macro FLEXCAN_QUIRK_SETUP_STOP_MODE -> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> This patch intends to rename FLEXCAN_QUIRK_SETUP_STOP_MODE quirk
> to FLEXCAN_QUIRK_SETUP_STOP_MODE_GRP for non-scu SoCs, coming patch will
> add quirk for scu SoCs.
> 
> For non-scu SoCs, setup stop mode with GPR register.
> For scu SoCs, setup stop mode with SCU firmware.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Looks good.

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

[PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Joakim Zhang]

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface
between host CPU and the SCU firmware running on M4.

For i.MX8QM, stop mode request is controlled by System Controller Unit(SCU)
firmware, this patch introduces FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk
for this function.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 125 ++++++++++++++++++++++++++++++++------
 1 file changed, 107 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e708e7bf28db..a55ea8f27f7c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -9,6 +9,7 @@
 //
 // Based on code originally by Andrey Volkov <avolkov@varma-el.com>
 
+#include <dt-bindings/firmware/imx/rsrc.h>
 #include <linux/bitfield.h>
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -17,6 +18,7 @@
 #include <linux/can/rx-offload.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/firmware/imx/sci.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
@@ -242,6 +244,8 @@
 #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
 /* support memory detection and correction */
 #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
+/* Setup stop mode with SCU firmware to support wakeup */
+#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -347,6 +351,7 @@ struct flexcan_priv {
 	u8 mb_count;
 	u8 mb_size;
 	u8 clk_src;	/* clock source of CAN Protocol Engine */
+	u8 can_idx;
 
 	u64 rx_mask;
 	u64 tx_mask;
@@ -358,6 +363,9 @@ struct flexcan_priv {
 	struct regulator *reg_xceiver;
 	struct flexcan_stop_mode stm;
 
+	/* IPC handle when setup stop mode by System Controller firmware(scfw) */
+	struct imx_sc_ipc *sc_ipc_handle;
+
 	/* Read and Write APIs */
 	u32 (*read)(void __iomem *addr);
 	void (*write)(u32 val, void __iomem *addr);
@@ -387,7 +395,7 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
 static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
-		FLEXCAN_QUIRK_SUPPORT_FD,
+		FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW,
 };
 
 static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
@@ -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 	priv->write(reg_mcr, &regs->mcr);
 }
 
+static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, bool enabled)
+{
+	u8 idx = priv->can_idx;
+	u32 rsrc_id, val;
+
+	if (idx == 0)
+		rsrc_id = IMX_SC_R_CAN_0;
+	else if (idx == 1)
+		rsrc_id = IMX_SC_R_CAN_1;
+	else
+		rsrc_id = IMX_SC_R_CAN_2;
+
+	if (enabled)
+		val = 1;
+	else
+		val = 0;
+
+	/* stop mode request via scu firmware */
+	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, IMX_SC_C_IPG_STOP, val);
+}
+
 static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
 	u32 reg_mcr;
+	int ret;
 
 	reg_mcr = priv->read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
 	priv->write(reg_mcr, &regs->mcr);
 
 	/* enable stop request */
-	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
-			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
+		ret = flexcan_stop_mode_enable_scfw(priv, true);
+		if (ret < 0)
+			return ret;
+	} else {
+		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
+				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
+	}
 
 	return flexcan_low_power_enter_ack(priv);
 }
@@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
 	u32 reg_mcr;
+	int ret;
 
 	/* remove stop request */
-	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
-			   1 << priv->stm.req_bit, 0);
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
+		ret = flexcan_stop_mode_enable_scfw(priv, false);
+		if (ret < 0)
+			return ret;
+	} else {
+		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
+				   1 << priv->stm.req_bit, 0);
+	}
 
 	reg_mcr = priv->read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
@@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct net_device *dev)
 	unregister_candev(dev);
 }
 
-static int flexcan_setup_stop_mode(struct platform_device *pdev)
+static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
@@ -1883,11 +1926,6 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 		"gpr %s req_gpr=0x02%x req_bit=%u\n",
 		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit);
 
-	device_set_wakeup_capable(&pdev->dev, true);
-
-	if (of_property_read_bool(np, "wakeup-source"))
-		device_set_wakeup_enable(&pdev->dev, true);
-
 	return 0;
 
 out_put_node:
@@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 	return ret;
 }
 
+static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct flexcan_priv *priv;
+	int ret;
+
+	priv = netdev_priv(dev);
+
+	/* this function could be defer probe, return -EPROBE_DEFER */
+	ret = imx_scu_get_handle(&priv->sc_ipc_handle);
+	if (ret < 0)
+		dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n");
+
+	return ret;
+}
+
+/* flexcan_setup_stop_mode - Setup stop mode
+ *
+ * Return: 0 setup stop mode successfully or doesn't support this feature
+ *         -EPROBE_DEFER defer probe
+ *         < 0 fail to setup stop mode
+ */
+static int flexcan_setup_stop_mode(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct flexcan_priv *priv;
+	int ret;
+
+	priv = netdev_priv(dev);
+
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW)
+		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
+		/* return 0 directly if stop mode is unsupport */
+		return 0;
+
+	if (ret) {
+		dev_warn(&pdev->dev, "failed to setup stop mode\n");
+	} else {
+		device_set_wakeup_capable(&pdev->dev, true);
+
+		if (of_property_read_bool(pdev->dev.of_node, "wakeup-source"))
+			device_set_wakeup_enable(&pdev->dev, true);
+	}
+
+	return ret;
+}
+
 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, },
@@ -1927,7 +2015,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	struct clk *clk_ipg = NULL, *clk_per = NULL;
 	struct flexcan_regs __iomem *regs;
 	int err, irq;
-	u8 clk_src = 1;
+	u8 clk_src = 1, can_idx = 0;
 	u32 clock_freq = 0;
 
 	reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver");
@@ -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev)
 				     "clock-frequency", &clock_freq);
 		of_property_read_u8(pdev->dev.of_node,
 				    "fsl,clk-source", &clk_src);
+		of_property_read_u8(pdev->dev.of_node,
+				    "fsl,can-index", &can_idx);
 	}
 
 	if (!clock_freq) {
@@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->clk_src = clk_src;
 	priv->devtype_data = devtype_data;
 	priv->reg_xceiver = reg_xceiver;
+	priv->can_idx = can_idx;
 
 	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
@@ -2030,6 +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
 		priv->can.bittiming_const = &flexcan_bittiming_const;
 	}
 
+	err = flexcan_setup_stop_mode(pdev);
+	if (err == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
@@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device *pdev)
 	of_can_transceiver(dev);
 	devm_can_led_init(dev);
 
-	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
-		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

Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> The System Controller Firmware (SCFW) is a low-level system function
> which runs on a dedicated Cortex-M core to provide power, clock, and
> resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface
> between host CPU and the SCU firmware running on M4.
> 
> For i.MX8QM, stop mode request is controlled by System Controller Unit(SCU)
> firmware, this patch introduces FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk
> for this function.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 125 ++++++++++++++++++++++++++++++++------
>  1 file changed, 107 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e708e7bf28db..a55ea8f27f7c 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -9,6 +9,7 @@
>  //
>  // Based on code originally by Andrey Volkov <avolkov@varma-el.com>
>  
> +#include <dt-bindings/firmware/imx/rsrc.h>
>  #include <linux/bitfield.h>
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -17,6 +18,7 @@
>  #include <linux/can/rx-offload.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
> @@ -242,6 +244,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
>  /* support memory detection and correction */
>  #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
> +/* Setup stop mode with SCU firmware to support wakeup */
> +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
>  
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -347,6 +351,7 @@ struct flexcan_priv {
>  	u8 mb_count;
>  	u8 mb_size;
>  	u8 clk_src;	/* clock source of CAN Protocol Engine */
> +	u8 can_idx;
>  
>  	u64 rx_mask;
>  	u64 tx_mask;
> @@ -358,6 +363,9 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	/* IPC handle when setup stop mode by System Controller firmware(scfw) */
> +	struct imx_sc_ipc *sc_ipc_handle;
> +
>  	/* Read and Write APIs */
>  	u32 (*read)(void __iomem *addr);
>  	void (*write)(u32 val, void __iomem *addr);
> @@ -387,7 +395,7 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>  static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> -		FLEXCAN_QUIRK_SUPPORT_FD,
> +		FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW,
>  };
>  
>  static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
> @@ -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>  	priv->write(reg_mcr, &regs->mcr);
>  }
>  
> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, bool enabled)
> +{
> +	u8 idx = priv->can_idx;
> +	u32 rsrc_id, val;
> +
> +	if (idx == 0)
> +		rsrc_id = IMX_SC_R_CAN_0;
> +	else if (idx == 1)
> +		rsrc_id = IMX_SC_R_CAN_1;
> +	else
> +		rsrc_id = IMX_SC_R_CAN_2;
> +
> +	if (enabled)
> +		val = 1;
> +	else
> +		val = 0;
> +
> +	/* stop mode request via scu firmware */
> +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, IMX_SC_C_IPG_STOP, val);
> +}
> +
>  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 reg_mcr;
> +	int ret;
>  
>  	reg_mcr = priv->read(&regs->mcr);
>  	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
>  	priv->write(reg_mcr, &regs->mcr);
>  
>  	/* enable stop request */
> -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> -			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> +		ret = flexcan_stop_mode_enable_scfw(priv, true);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> +	}
>  
>  	return flexcan_low_power_enter_ack(priv);
>  }
> @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 reg_mcr;
> +	int ret;
>  
>  	/* remove stop request */
> -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> -			   1 << priv->stm.req_bit, 0);
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> +		ret = flexcan_stop_mode_enable_scfw(priv, false);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +				   1 << priv->stm.req_bit, 0);
> +	}
>  
>  	reg_mcr = priv->read(&regs->mcr);
>  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct net_device *dev)
>  	unregister_candev(dev);
>  }
>  
> -static int flexcan_setup_stop_mode(struct platform_device *pdev)
> +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1883,11 +1926,6 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
>  		"gpr %s req_gpr=0x02%x req_bit=%u\n",
>  		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit);
>  
> -	device_set_wakeup_capable(&pdev->dev, true);
> -
> -	if (of_property_read_bool(np, "wakeup-source"))
> -		device_set_wakeup_enable(&pdev->dev, true);
> -
>  	return 0;
>  
>  out_put_node:
> @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv;
> +	int ret;
> +
> +	priv = netdev_priv(dev);
> +
> +	/* this function could be defer probe, return -EPROBE_DEFER */
> +	ret = imx_scu_get_handle(&priv->sc_ipc_handle);
> +	if (ret < 0)
> +		dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n");
> +
> +	return ret;
> +}
> +
> +/* flexcan_setup_stop_mode - Setup stop mode
> + *
> + * Return: 0 setup stop mode successfully or doesn't support this feature
> + *         -EPROBE_DEFER defer probe
> + *         < 0 fail to setup stop mode
> + */
> +static int flexcan_setup_stop_mode(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv;
> +	int ret;
> +
> +	priv = netdev_priv(dev);
> +
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW)
> +		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
> +		/* return 0 directly if stop mode is unsupport */
> +		return 0;
> +
> +	if (ret) {
> +		dev_warn(&pdev->dev, "failed to setup stop mode\n");
> +	} else {
> +		device_set_wakeup_capable(&pdev->dev, true);
> +
> +		if (of_property_read_bool(pdev->dev.of_node, "wakeup-source"))
> +			device_set_wakeup_enable(&pdev->dev, true);
> +	}
> +
> +	return ret;
> +}
> +
>  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, },
> @@ -1927,7 +2015,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  	struct clk *clk_ipg = NULL, *clk_per = NULL;
>  	struct flexcan_regs __iomem *regs;
>  	int err, irq;
> -	u8 clk_src = 1;
> +	u8 clk_src = 1, can_idx = 0;
>  	u32 clock_freq = 0;
>  
>  	reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver");
> @@ -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev)
>  				     "clock-frequency", &clock_freq);
>  		of_property_read_u8(pdev->dev.of_node,
>  				    "fsl,clk-source", &clk_src);
> +		of_property_read_u8(pdev->dev.of_node,
> +				    "fsl,can-index", &can_idx);
>  	}
>  
>  	if (!clock_freq) {
> @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  	priv->clk_src = clk_src;
>  	priv->devtype_data = devtype_data;
>  	priv->reg_xceiver = reg_xceiver;
> +	priv->can_idx = can_idx;
>  
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
> @@ -2030,6 +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
>  		priv->can.bittiming_const = &flexcan_bittiming_const;
>  	}
>  
> +	err = flexcan_setup_stop_mode(pdev);
> +	if (err == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

You need to free "dev". What about moving this directly before allocating dev.

Do you have to undo device_set_wakeup_capable() and device_set_wakeup_enable()
in case of a failure and/or on flexcan_remove()?

> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device *pdev)
>  	of_can_transceiver(dev);
>  	devm_can_led_init(dev);
>  
> -	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
> -		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 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 14:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM
> 
> On 10/16/20 3:43 PM, Joakim Zhang wrote:
> > The System Controller Firmware (SCFW) is a low-level system function
> > which runs on a dedicated Cortex-M core to provide power, clock, and
> > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface
> > between host CPU and the SCU firmware running on M4.
> >
> > For i.MX8QM, stop mode request is controlled by System Controller
> > Unit(SCU) firmware, this patch introduces
> > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk for this function.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 125
> > ++++++++++++++++++++++++++++++++------
> >  1 file changed, 107 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index e708e7bf28db..a55ea8f27f7c 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -9,6 +9,7 @@
> >  //
> >  // Based on code originally by Andrey Volkov <avolkov@varma-el.com>
> >
> > +#include <dt-bindings/firmware/imx/rsrc.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/can.h>
> >  #include <linux/can/dev.h>
> > @@ -17,6 +18,7 @@
> >  #include <linux/can/rx-offload.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/firmware/imx/sci.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -242,6 +244,8 @@
> >  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
> >  /* support memory detection and correction */  #define
> > FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
> > +/* Setup stop mode with SCU firmware to support wakeup */ #define
> > +FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
> >
> >  /* Structure of the message buffer */  struct flexcan_mb { @@ -347,6
> > +351,7 @@ struct flexcan_priv {
> >  	u8 mb_count;
> >  	u8 mb_size;
> >  	u8 clk_src;	/* clock source of CAN Protocol Engine */
> > +	u8 can_idx;
> >
> >  	u64 rx_mask;
> >  	u64 tx_mask;
> > @@ -358,6 +363,9 @@ struct flexcan_priv {
> >  	struct regulator *reg_xceiver;
> >  	struct flexcan_stop_mode stm;
> >
> > +	/* IPC handle when setup stop mode by System Controller firmware(scfw)
> */
> > +	struct imx_sc_ipc *sc_ipc_handle;
> > +
> >  	/* Read and Write APIs */
> >  	u32 (*read)(void __iomem *addr);
> >  	void (*write)(u32 val, void __iomem *addr); @@ -387,7 +395,7 @@
> > static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> >  		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > -		FLEXCAN_QUIRK_SUPPORT_FD,
> > +		FLEXCAN_QUIRK_SUPPORT_FD |
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW,
> >  };
> >
> >  static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { @@
> > -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct
> flexcan_priv *priv, bool enable)
> >  	priv->write(reg_mcr, &regs->mcr);
> >  }
> >
> > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv,
> > +bool enabled) {
> > +	u8 idx = priv->can_idx;
> > +	u32 rsrc_id, val;
> > +
> > +	if (idx == 0)
> > +		rsrc_id = IMX_SC_R_CAN_0;
> > +	else if (idx == 1)
> > +		rsrc_id = IMX_SC_R_CAN_1;
> > +	else
> > +		rsrc_id = IMX_SC_R_CAN_2;
> > +
> > +	if (enabled)
> > +		val = 1;
> > +	else
> > +		val = 0;
> > +
> > +	/* stop mode request via scu firmware */
> > +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id,
> > +IMX_SC_C_IPG_STOP, val); }
> > +
> >  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
> > {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> >  	u32 reg_mcr;
> > +	int ret;
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> >  	/* enable stop request */
> > -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > -			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> > +		ret = flexcan_stop_mode_enable_scfw(priv, true);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> > +	}
> >
> >  	return flexcan_low_power_enter_ack(priv);
> >  }
> > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct
> > flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> >  	u32 reg_mcr;
> > +	int ret;
> >
> >  	/* remove stop request */
> > -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > -			   1 << priv->stm.req_bit, 0);
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> > +		ret = flexcan_stop_mode_enable_scfw(priv, false);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +				   1 << priv->stm.req_bit, 0);
> > +	}
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct
> net_device *dev)
> >  	unregister_candev(dev);
> >  }
> >
> > -static int flexcan_setup_stop_mode(struct platform_device *pdev)
> > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev)
> >  {
> >  	struct net_device *dev = platform_get_drvdata(pdev);
> >  	struct device_node *np = pdev->dev.of_node; @@ -1883,11 +1926,6 @@
> > static int flexcan_setup_stop_mode(struct platform_device *pdev)
> >  		"gpr %s req_gpr=0x02%x req_bit=%u\n",
> >  		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit);
> >
> > -	device_set_wakeup_capable(&pdev->dev, true);
> > -
> > -	if (of_property_read_bool(np, "wakeup-source"))
> > -		device_set_wakeup_enable(&pdev->dev, true);
> > -
> >  	return 0;
> >
> >  out_put_node:
> > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct
> platform_device *pdev)
> >  	return ret;
> >  }
> >
> > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev)
> > +{
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct flexcan_priv *priv;
> > +	int ret;
> > +
> > +	priv = netdev_priv(dev);
> > +
> > +	/* this function could be defer probe, return -EPROBE_DEFER */
> > +	ret = imx_scu_get_handle(&priv->sc_ipc_handle);
> > +	if (ret < 0)
> > +		dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n");
> > +
> > +	return ret;
> > +}
> > +
> > +/* flexcan_setup_stop_mode - Setup stop mode
> > + *
> > + * Return: 0 setup stop mode successfully or doesn't support this feature
> > + *         -EPROBE_DEFER defer probe
> > + *         < 0 fail to setup stop mode
> > + */
> > +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct flexcan_priv *priv;
> > +	int ret;
> > +
> > +	priv = netdev_priv(dev);
> > +
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW)
> > +		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
> > +		/* return 0 directly if stop mode is unsupport */
> > +		return 0;
> > +
> > +	if (ret) {
> > +		dev_warn(&pdev->dev, "failed to setup stop mode\n");
> > +	} else {
> > +		device_set_wakeup_capable(&pdev->dev, true);
> > +
> > +		if (of_property_read_bool(pdev->dev.of_node, "wakeup-source"))
> > +			device_set_wakeup_enable(&pdev->dev, true);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  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, }, @@ -1927,7 +2015,7 @@ static int
> flexcan_probe(struct platform_device *pdev)
> >  	struct clk *clk_ipg = NULL, *clk_per = NULL;
> >  	struct flexcan_regs __iomem *regs;
> >  	int err, irq;
> > -	u8 clk_src = 1;
> > +	u8 clk_src = 1, can_idx = 0;
> >  	u32 clock_freq = 0;
> >
> >  	reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); @@
> > -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev)
> >  				     "clock-frequency", &clock_freq);
> >  		of_property_read_u8(pdev->dev.of_node,
> >  				    "fsl,clk-source", &clk_src);
> > +		of_property_read_u8(pdev->dev.of_node,
> > +				    "fsl,can-index", &can_idx);
> >  	}
> >
> >  	if (!clock_freq) {
> > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  	priv->clk_src = clk_src;
> >  	priv->devtype_data = devtype_data;
> >  	priv->reg_xceiver = reg_xceiver;
> > +	priv->can_idx = can_idx;
> >
> >  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
> >  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@
> -2030,6
> > +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
> >  		priv->can.bittiming_const = &flexcan_bittiming_const;
> >  	}
> >
> > +	err = flexcan_setup_stop_mode(pdev);
> > +	if (err == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> 
> You need to free "dev". What about moving this directly before allocating dev.
Yes, need free "dev" here if defer probe. Flexcan_priv has not allocated before allocating dev, but we need initialize and check it when setup stop mode.


> Do you have to undo device_set_wakeup_capable() and
> device_set_wakeup_enable() in case of a failure and/or on flexcan_remove()?
Yes, should invoke device_wakeup_disable() in flexcan_remove.


Best Regards,
Joakim Zhang
> > +
> >  	pm_runtime_get_noresume(&pdev->dev);
> >  	pm_runtime_set_active(&pdev->dev);
> >  	pm_runtime_enable(&pdev->dev);
> > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  	of_can_transceiver(dev);
> >  	devm_can_led_init(dev);
> >
> > -	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
> > -		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 |

Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Marc Kleine-Budde]

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

On 10/16/20 8:46 AM, Joakim Zhang wrote:
>>> @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device
>> *pdev)
>>>  	priv->clk_src = clk_src;
>>>  	priv->devtype_data = devtype_data;
>>>  	priv->reg_xceiver = reg_xceiver;
>>> +	priv->can_idx = can_idx;
>>>
>>>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>>>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@
>> -2030,6
>>> +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
>>>  		priv->can.bittiming_const = &flexcan_bittiming_const;
>>>  	}
>>>
>>> +	err = flexcan_setup_stop_mode(pdev);
>>> +	if (err == -EPROBE_DEFER)
>>> +		return -EPROBE_DEFER;
>>
>> You need to free "dev". What about moving this directly before allocating dev.
>
> Yes, need free "dev" here if defer probe. Flexcan_priv has not allocated
> before allocating dev, but we need initialize and check it when setup stop
> mode.

Right, please take care of freeing all ressouces in case of defered probe.

>> Do you have to undo device_set_wakeup_capable() and
>> device_set_wakeup_enable() in case of a failure and/or on flexcan_remove()?
>
> Yes, should invoke device_wakeup_disable() in flexcan_remove.

Make it so.

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 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> The System Controller Firmware (SCFW) is a low-level system function
> which runs on a dedicated Cortex-M core to provide power, clock, and
> resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface
> between host CPU and the SCU firmware running on M4.
> 
> For i.MX8QM, stop mode request is controlled by System Controller Unit(SCU)
> firmware, this patch introduces FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk
> for this function.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 125 ++++++++++++++++++++++++++++++++------
>  1 file changed, 107 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e708e7bf28db..a55ea8f27f7c 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -9,6 +9,7 @@
>  //
>  // Based on code originally by Andrey Volkov <avolkov@varma-el.com>
>  
> +#include <dt-bindings/firmware/imx/rsrc.h>
>  #include <linux/bitfield.h>
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -17,6 +18,7 @@
>  #include <linux/can/rx-offload.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
> @@ -242,6 +244,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
>  /* support memory detection and correction */
>  #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
> +/* Setup stop mode with SCU firmware to support wakeup */
> +#define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
>  
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -347,6 +351,7 @@ struct flexcan_priv {
>  	u8 mb_count;
>  	u8 mb_size;
>  	u8 clk_src;	/* clock source of CAN Protocol Engine */
> +	u8 can_idx;
>  
>  	u64 rx_mask;
>  	u64 tx_mask;
> @@ -358,6 +363,9 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	/* IPC handle when setup stop mode by System Controller firmware(scfw) */
> +	struct imx_sc_ipc *sc_ipc_handle;
> +
>  	/* Read and Write APIs */
>  	u32 (*read)(void __iomem *addr);
>  	void (*write)(u32 val, void __iomem *addr);
> @@ -387,7 +395,7 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>  static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> -		FLEXCAN_QUIRK_SUPPORT_FD,
> +		FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW,
>  };
>  
>  static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
> @@ -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
>  	priv->write(reg_mcr, &regs->mcr);
>  }
>  
> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, bool enabled)
> +{
> +	u8 idx = priv->can_idx;
> +	u32 rsrc_id, val;
> +
> +	if (idx == 0)
> +		rsrc_id = IMX_SC_R_CAN_0;
> +	else if (idx == 1)
> +		rsrc_id = IMX_SC_R_CAN_1;
> +	else
> +		rsrc_id = IMX_SC_R_CAN_2;

Can you introduce something like and make use of it:

#define IMX_SC_R_CAN(x)			(105 + (x))

> +
> +	if (enabled)
> +		val = 1;
> +	else
> +		val = 0;
> +
> +	/* stop mode request via scu firmware */
> +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, IMX_SC_C_IPG_STOP, val);
> +}
> +
>  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 reg_mcr;
> +	int ret;
>  
>  	reg_mcr = priv->read(&regs->mcr);
>  	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
>  	priv->write(reg_mcr, &regs->mcr);
>  
>  	/* enable stop request */
> -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> -			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> +		ret = flexcan_stop_mode_enable_scfw(priv, true);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> +	}
>  
>  	return flexcan_low_power_enter_ack(priv);
>  }
> @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
>  {
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 reg_mcr;
> +	int ret;
>  
>  	/* remove stop request */
> -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> -			   1 << priv->stm.req_bit, 0);
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> +		ret = flexcan_stop_mode_enable_scfw(priv, false);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> +				   1 << priv->stm.req_bit, 0);
> +	}
>  
>  	reg_mcr = priv->read(&regs->mcr);
>  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct net_device *dev)
>  	unregister_candev(dev);
>  }
>  
> -static int flexcan_setup_stop_mode(struct platform_device *pdev)
> +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1883,11 +1926,6 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
>  		"gpr %s req_gpr=0x02%x req_bit=%u\n",
>  		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit);
>  
> -	device_set_wakeup_capable(&pdev->dev, true);
> -
> -	if (of_property_read_bool(np, "wakeup-source"))
> -		device_set_wakeup_enable(&pdev->dev, true);
> -
>  	return 0;
>  
>  out_put_node:
> @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv;
> +	int ret;
> +
> +	priv = netdev_priv(dev);
> +
> +	/* this function could be defer probe, return -EPROBE_DEFER */
> +	ret = imx_scu_get_handle(&priv->sc_ipc_handle);
> +	if (ret < 0)
> +		dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n");
> +
> +	return ret;
> +}
> +
> +/* flexcan_setup_stop_mode - Setup stop mode
> + *
> + * Return: 0 setup stop mode successfully or doesn't support this feature
> + *         -EPROBE_DEFER defer probe
> + *         < 0 fail to setup stop mode
> + */
> +static int flexcan_setup_stop_mode(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv;
> +	int ret;
> +
> +	priv = netdev_priv(dev);
> +
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW)
> +		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
> +		/* return 0 directly if stop mode is unsupport */
> +		return 0;
> +
> +	if (ret) {
> +		dev_warn(&pdev->dev, "failed to setup stop mode\n");

return here...

> +	} else {

...and remove the else

> +		device_set_wakeup_capable(&pdev->dev, true);
> +
> +		if (of_property_read_bool(pdev->dev.of_node, "wakeup-source"))
> +			device_set_wakeup_enable(&pdev->dev, true);
> +	}
> +
> +	return ret;
> +}
> +
>  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, },
> @@ -1927,7 +2015,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  	struct clk *clk_ipg = NULL, *clk_per = NULL;
>  	struct flexcan_regs __iomem *regs;
>  	int err, irq;
> -	u8 clk_src = 1;
> +	u8 clk_src = 1, can_idx = 0;
>  	u32 clock_freq = 0;
>  
>  	reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver");
> @@ -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev)
>  				     "clock-frequency", &clock_freq);
>  		of_property_read_u8(pdev->dev.of_node,
>  				    "fsl,clk-source", &clk_src);
> +		of_property_read_u8(pdev->dev.of_node,
> +				    "fsl,can-index", &can_idx);

What happens if the DT doesn't contain the can-index? Move this into the
flexcan_setup_stop_mode_scfw() and add error handling.

>  	}
>  
>  	if (!clock_freq) {
> @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  	priv->clk_src = clk_src;
>  	priv->devtype_data = devtype_data;
>  	priv->reg_xceiver = reg_xceiver;
> +	priv->can_idx = can_idx;

Assign priv->can_idx in flexcan_setup_stop_mode_scfw(), too.

>  
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
> @@ -2030,6 +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
>  		priv->can.bittiming_const = &flexcan_bittiming_const;
>  	}
>  
> +	err = flexcan_setup_stop_mode(pdev);
> +	if (err == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device *pdev)
>  	of_can_transceiver(dev);
>  	devm_can_led_init(dev);
>  
> -	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
> -		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 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 14:19
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM
> 
> On 10/16/20 3:43 PM, Joakim Zhang wrote:
> > The System Controller Firmware (SCFW) is a low-level system function
> > which runs on a dedicated Cortex-M core to provide power, clock, and
> > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface
> > between host CPU and the SCU firmware running on M4.
> >
> > For i.MX8QM, stop mode request is controlled by System Controller
> > Unit(SCU) firmware, this patch introduces
> > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk for this function.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 125
> > ++++++++++++++++++++++++++++++++------
> >  1 file changed, 107 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index e708e7bf28db..a55ea8f27f7c 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -9,6 +9,7 @@
> >  //
> >  // Based on code originally by Andrey Volkov <avolkov@varma-el.com>
> >
> > +#include <dt-bindings/firmware/imx/rsrc.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/can.h>
> >  #include <linux/can/dev.h>
> > @@ -17,6 +18,7 @@
> >  #include <linux/can/rx-offload.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/firmware/imx/sci.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -242,6 +244,8 @@
> >  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
> >  /* support memory detection and correction */  #define
> > FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
> > +/* Setup stop mode with SCU firmware to support wakeup */ #define
> > +FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
> >
> >  /* Structure of the message buffer */  struct flexcan_mb { @@ -347,6
> > +351,7 @@ struct flexcan_priv {
> >  	u8 mb_count;
> >  	u8 mb_size;
> >  	u8 clk_src;	/* clock source of CAN Protocol Engine */
> > +	u8 can_idx;
> >
> >  	u64 rx_mask;
> >  	u64 tx_mask;
> > @@ -358,6 +363,9 @@ struct flexcan_priv {
> >  	struct regulator *reg_xceiver;
> >  	struct flexcan_stop_mode stm;
> >
> > +	/* IPC handle when setup stop mode by System Controller firmware(scfw)
> */
> > +	struct imx_sc_ipc *sc_ipc_handle;
> > +
> >  	/* Read and Write APIs */
> >  	u32 (*read)(void __iomem *addr);
> >  	void (*write)(u32 val, void __iomem *addr); @@ -387,7 +395,7 @@
> > static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> >  		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > -		FLEXCAN_QUIRK_SUPPORT_FD,
> > +		FLEXCAN_QUIRK_SUPPORT_FD |
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW,
> >  };
> >
> >  static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { @@
> > -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct
> flexcan_priv *priv, bool enable)
> >  	priv->write(reg_mcr, &regs->mcr);
> >  }
> >
> > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv,
> > +bool enabled) {
> > +	u8 idx = priv->can_idx;
> > +	u32 rsrc_id, val;
> > +
> > +	if (idx == 0)
> > +		rsrc_id = IMX_SC_R_CAN_0;
> > +	else if (idx == 1)
> > +		rsrc_id = IMX_SC_R_CAN_1;
> > +	else
> > +		rsrc_id = IMX_SC_R_CAN_2;
> 
> Can you introduce something like and make use of it:
> 
> #define IMX_SC_R_CAN(x)			(105 + (x))
OK.


> > +
> > +	if (enabled)
> > +		val = 1;
> > +	else
> > +		val = 0;
> > +
> > +	/* stop mode request via scu firmware */
> > +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id,
> > +IMX_SC_C_IPG_STOP, val); }
> > +
> >  static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
> > {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> >  	u32 reg_mcr;
> > +	int ret;
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr |= FLEXCAN_MCR_SLF_WAK;
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> >  	/* enable stop request */
> > -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > -			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> > +		ret = flexcan_stop_mode_enable_scfw(priv, true);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +				   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> > +	}
> >
> >  	return flexcan_low_power_enter_ack(priv);
> >  }
> > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct
> > flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> >  	u32 reg_mcr;
> > +	int ret;
> >
> >  	/* remove stop request */
> > -	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > -			   1 << priv->stm.req_bit, 0);
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) {
> > +		ret = flexcan_stop_mode_enable_scfw(priv, false);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> > +				   1 << priv->stm.req_bit, 0);
> > +	}
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct
> net_device *dev)
> >  	unregister_candev(dev);
> >  }
> >
> > -static int flexcan_setup_stop_mode(struct platform_device *pdev)
> > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev)
> >  {
> >  	struct net_device *dev = platform_get_drvdata(pdev);
> >  	struct device_node *np = pdev->dev.of_node; @@ -1883,11 +1926,6 @@
> > static int flexcan_setup_stop_mode(struct platform_device *pdev)
> >  		"gpr %s req_gpr=0x02%x req_bit=%u\n",
> >  		gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit);
> >
> > -	device_set_wakeup_capable(&pdev->dev, true);
> > -
> > -	if (of_property_read_bool(np, "wakeup-source"))
> > -		device_set_wakeup_enable(&pdev->dev, true);
> > -
> >  	return 0;
> >
> >  out_put_node:
> > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct
> platform_device *pdev)
> >  	return ret;
> >  }
> >
> > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev)
> > +{
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct flexcan_priv *priv;
> > +	int ret;
> > +
> > +	priv = netdev_priv(dev);
> > +
> > +	/* this function could be defer probe, return -EPROBE_DEFER */
> > +	ret = imx_scu_get_handle(&priv->sc_ipc_handle);
> > +	if (ret < 0)
> > +		dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n");
> > +
> > +	return ret;
> > +}
> > +
> > +/* flexcan_setup_stop_mode - Setup stop mode
> > + *
> > + * Return: 0 setup stop mode successfully or doesn't support this feature
> > + *         -EPROBE_DEFER defer probe
> > + *         < 0 fail to setup stop mode
> > + */
> > +static int flexcan_setup_stop_mode(struct platform_device *pdev) {
> > +	struct net_device *dev = platform_get_drvdata(pdev);
> > +	struct flexcan_priv *priv;
> > +	int ret;
> > +
> > +	priv = netdev_priv(dev);
> > +
> > +	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW)
> > +		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
> > +		/* return 0 directly if stop mode is unsupport */
> > +		return 0;
> > +
> > +	if (ret) {
> > +		dev_warn(&pdev->dev, "failed to setup stop mode\n");
> 
> return here...
My thought was that, CAN still work even failed to setup stop mode. If you think it is better, I will change the way of setup stop mode, let probe failed if can't setup it successfully.

> > +	} else {
> 
> ...and remove the else
OK.


> > +		device_set_wakeup_capable(&pdev->dev, true);
> > +
> > +		if (of_property_read_bool(pdev->dev.of_node, "wakeup-source"))
> > +			device_set_wakeup_enable(&pdev->dev, true);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  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, }, @@ -1927,7 +2015,7 @@ static int
> flexcan_probe(struct platform_device *pdev)
> >  	struct clk *clk_ipg = NULL, *clk_per = NULL;
> >  	struct flexcan_regs __iomem *regs;
> >  	int err, irq;
> > -	u8 clk_src = 1;
> > +	u8 clk_src = 1, can_idx = 0;
> >  	u32 clock_freq = 0;
> >
> >  	reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); @@
> > -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev)
> >  				     "clock-frequency", &clock_freq);
> >  		of_property_read_u8(pdev->dev.of_node,
> >  				    "fsl,clk-source", &clk_src);
> > +		of_property_read_u8(pdev->dev.of_node,
> > +				    "fsl,can-index", &can_idx);
> 
> What happens if the DT doesn't contain the can-index? Move this into the
> flexcan_setup_stop_mode_scfw() and add error handling.

Make sense.

> >  	}
> >
> >  	if (!clock_freq) {
> > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  	priv->clk_src = clk_src;
> >  	priv->devtype_data = devtype_data;
> >  	priv->reg_xceiver = reg_xceiver;
> > +	priv->can_idx = can_idx;
> 
> Assign priv->can_idx in flexcan_setup_stop_mode_scfw(), too.
OK.

Best Regards,
Joakim Zhang
> >
> >  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
> >  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@
> -2030,6
> > +2121,10 @@ static int flexcan_probe(struct platform_device *pdev)
> >  		priv->can.bittiming_const = &flexcan_bittiming_const;
> >  	}
> >
> > +	err = flexcan_setup_stop_mode(pdev);
> > +	if (err == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> > +
> >  	pm_runtime_get_noresume(&pdev->dev);
> >  	pm_runtime_set_active(&pdev->dev);
> >  	pm_runtime_enable(&pdev->dev);
> > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  	of_can_transceiver(dev);
> >  	devm_can_led_init(dev);
> >
> > -	if (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) {
> > -		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 |

RE: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Joakim Zhang]

Hi Marc,

[...]
> > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv,
> > > +bool enabled) {
> > > +	u8 idx = priv->can_idx;
> > > +	u32 rsrc_id, val;
> > > +
> > > +	if (idx == 0)
> > > +		rsrc_id = IMX_SC_R_CAN_0;
> > > +	else if (idx == 1)
> > > +		rsrc_id = IMX_SC_R_CAN_1;
> > > +	else
> > > +		rsrc_id = IMX_SC_R_CAN_2;
> >
> > Can you introduce something like and make use of it:
> >
> > #define IMX_SC_R_CAN(x)			(105 + (x))
> OK.

I thought it over again, from my point of view, use macro here directly could be more intuitive, and can achieve a direct jump.
If change to above wrapper, on the contrary make confusion, and generate the magic number 105. ☹

> > > +
> > > +	if (enabled)
> > > +		val = 1;
> > > +	else
> > > +		val = 0;
> > > +
> > > +	/* stop mode request via scu firmware */
> > > +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id,
> > > +IMX_SC_C_IPG_STOP, val); }

We still need use IMX_SC_C_IPG_STOP, why not be consistent?

Best Regards,
Joakim Zhang

Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Marc Kleine-Budde]

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

On 10/16/20 12:00 PM, Joakim Zhang wrote:
>>>> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv,
>>>> +bool enabled) {
>>>> +	u8 idx = priv->can_idx;
>>>> +	u32 rsrc_id, val;
>>>> +
>>>> +	if (idx == 0)
>>>> +		rsrc_id = IMX_SC_R_CAN_0;
>>>> +	else if (idx == 1)
>>>> +		rsrc_id = IMX_SC_R_CAN_1;
>>>> +	else
>>>> +		rsrc_id = IMX_SC_R_CAN_2;
>>>
>>> Can you introduce something like and make use of it:
>>>
>>> #define IMX_SC_R_CAN(x)			(105 + (x))
>> OK.
> 
> I thought it over again, from my point of view, use macro here directly could
> be more intuitive, and can achieve a direct jump.
> If change to above wrapper, on the contrary make confusion, and generate the
> magic number 105. ☹

The define should go into the rsrc.h, and probably be:

#define IMX_SC_R_CAN(x)		(IMX_SC_R_CAN_0 + (x))	

and if you change the firmware interface, you probably have more problems :)

>>>> +
>>>> +	if (enabled)
>>>> +		val = 1;
>>>> +	else
>>>> +		val = 0;
>>>> +
>>>> +	/* stop mode request via scu firmware */
>>>> +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id,
>>>> +IMX_SC_C_IPG_STOP, val); }
> 
> We still need use IMX_SC_C_IPG_STOP, why not be consistent?

Sorry I don't get what you want to tell me 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 5/6] can: flexcan: add CAN wakeup function for i.MX8QM

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 18:40
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM
> 
> On 10/16/20 12:00 PM, Joakim Zhang wrote:
> >>>> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv
> >>>> +*priv, bool enabled) {
> >>>> +	u8 idx = priv->can_idx;
> >>>> +	u32 rsrc_id, val;
> >>>> +
> >>>> +	if (idx == 0)
> >>>> +		rsrc_id = IMX_SC_R_CAN_0;
> >>>> +	else if (idx == 1)
> >>>> +		rsrc_id = IMX_SC_R_CAN_1;
> >>>> +	else
> >>>> +		rsrc_id = IMX_SC_R_CAN_2;
> >>>
> >>> Can you introduce something like and make use of it:
> >>>
> >>> #define IMX_SC_R_CAN(x)			(105 + (x))
> >> OK.
> >
> > I thought it over again, from my point of view, use macro here
> > directly could be more intuitive, and can achieve a direct jump.
> > If change to above wrapper, on the contrary make confusion, and
> > generate the magic number 105. ☹
> 
> The define should go into the rsrc.h, and probably be:
> 
> #define IMX_SC_R_CAN(x)		(IMX_SC_R_CAN_0 + (x))
> 
> and if you change the firmware interface, you probably have more problems :)

rsrc.h:
 /*
  * These defines are used to indicate a resource. Resources include peripherals
  * and bus masters (but not memory regions). Note items from list should
  * never be changed or removed (only added to at the end of the list).
  */
Hmm, it just list all resource id, and never be changed. Anyway, if you think above way is better, I will turn to it.

> >>>> +
> >>>> +	if (enabled)
> >>>> +		val = 1;
> >>>> +	else
> >>>> +		val = 0;
> >>>> +
> >>>> +	/* stop mode request via scu firmware */
> >>>> +	return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id,
> >>>> +IMX_SC_C_IPG_STOP, val); }
> >
> > We still need use IMX_SC_C_IPG_STOP, why not be consistent?
> 
> Sorry I don't get what you want to tell me here.

Need me add IMX_SC_C_IPG_STOP macro in the driver directly?

Such as, #define IMX_SC_C_IPG_STOP 52, so that no need to include rsrc.h file in the driver.

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 |

[PATCH 6/6] can: flexcan: fix ECC function on LS1021A/LX2160A

[Joakim Zhang]

After double check with Layerscape CAN owner (Pankaj Bansal), confirm
that LS1021A doesn't support ECC, and LX2160A indeed supports ECC.

For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR quirk to
disable non-correctable errors interrupt and freeze mode, had better use
FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index a55ea8f27f7c..7b0eb608fc9d 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -219,7 +219,7 @@
  *   MX8MP FlexCAN3  03.00.17.01    yes       yes        no      yes       yes          yes
  *   VF610 FlexCAN3  ?               no       yes        no      yes       yes?          no
  * LS1021A FlexCAN2  03.00.04.00     no       yes        no       no       yes           no
- * LX2160A FlexCAN3  03.00.23.00     no       yes        no       no       yes          yes
+ * LX2160A FlexCAN3  03.00.23.00     no       yes        no      yes       yes          yes
  *
  * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
  */
@@ -408,19 +408,19 @@ static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
 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_OFF_TIMESTAMP |
-		FLEXCAN_QUIRK_BROKEN_PERR_STATE,
+		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SUPPORT_ECC,
 };
 
 static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
-		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
-		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
+		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
 };
 
 static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
-		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD,
+		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD |
+		FLEXCAN_QUIRK_SUPPORT_ECC,
 };
 
 static const struct can_bittiming_const flexcan_bittiming_const = {
-- 
2.17.1

Re: [PATCH 6/6] can: flexcan: fix ECC function on LS1021A/LX2160A

[Marc Kleine-Budde]

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

On 10/16/20 3:43 PM, Joakim Zhang wrote:
> After double check with Layerscape CAN owner (Pankaj Bansal), confirm
> that LS1021A doesn't support ECC, and LX2160A indeed supports ECC.
> 
> For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR quirk to
> disable non-correctable errors interrupt and freeze mode, had better use
> FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index a55ea8f27f7c..7b0eb608fc9d 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -219,7 +219,7 @@
>   *   MX8MP FlexCAN3  03.00.17.01    yes       yes        no      yes       yes          yes
>   *   VF610 FlexCAN3  ?               no       yes        no      yes       yes?          no
>   * LS1021A FlexCAN2  03.00.04.00     no       yes        no       no       yes           no
> - * LX2160A FlexCAN3  03.00.23.00     no       yes        no       no       yes          yes
> + * LX2160A FlexCAN3  03.00.23.00     no       yes        no      yes       yes          yes
>   *
>   * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
>   */
> @@ -408,19 +408,19 @@ static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
>  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_OFF_TIMESTAMP |
> -		FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SUPPORT_ECC,

You add the missing ECC init for vf610, but don't mention it in the patch
subject nor description. Please make this a seperate patch and add a Fixes: line.

>  };
>  
>  static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> -		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
>  };

Please make this a seperate patch, too, along with the Fixes line.

>  static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD,
> +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD |
> +		FLEXCAN_QUIRK_SUPPORT_ECC,
>  };
>  
>  static const struct can_bittiming_const flexcan_bittiming_const = {
> 


-- 
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 6/6] can: flexcan: fix ECC function on LS1021A/LX2160A

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年10月16日 14:05
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu
> <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 6/6] can: flexcan: fix ECC function on LS1021A/LX2160A
> 
> On 10/16/20 3:43 PM, Joakim Zhang wrote:
> > After double check with Layerscape CAN owner (Pankaj Bansal), confirm
> > that LS1021A doesn't support ECC, and LX2160A indeed supports ECC.
> >
> > For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR
> quirk
> > to disable non-correctable errors interrupt and freeze mode, had
> > better use FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index a55ea8f27f7c..7b0eb608fc9d 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -219,7 +219,7 @@
> >   *   MX8MP FlexCAN3  03.00.17.01    yes       yes        no
> yes       yes          yes
> >   *   VF610 FlexCAN3  ?               no       yes        no
> yes       yes?          no
> >   * LS1021A FlexCAN2  03.00.04.00     no       yes        no
> no       yes           no
> > - * LX2160A FlexCAN3  03.00.23.00     no       yes        no
> no       yes          yes
> > + * LX2160A FlexCAN3  03.00.23.00     no       yes        no
> yes       yes          yes
> >   *
> >   * Some SOCs do not have the RX_WARN & TX_WARN interrupt line
> connected.
> >   */
> > @@ -408,19 +408,19 @@ static struct flexcan_devtype_data
> > fsl_imx8mp_devtype_data = {  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_OFF_TIMESTAMP |
> > -		FLEXCAN_QUIRK_BROKEN_PERR_STATE,
> > +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> FLEXCAN_QUIRK_SUPPORT_ECC,
> 
> You add the missing ECC init for vf610, but don't mention it in the patch subject
> nor description. Please make this a seperate patch and add a Fixes: line.
OK.

> >  };
> >
> >  static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> > -		FLEXCAN_QUIRK_DISABLE_MECR |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
> > +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
> >  };
> 
> Please make this a seperate patch, too, along with the Fixes line.
OK.

Best Regards,
Joakim Zhang
> >  static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
> >  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG |
> FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> >  		FLEXCAN_QUIRK_DISABLE_MECR |
> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> > -		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_SUPPORT_FD,
> > +		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
> FLEXCAN_QUIRK_SUPPORT_FD |
> > +		FLEXCAN_QUIRK_SUPPORT_ECC,
> >  };
> >
> >  static const struct can_bittiming_const flexcan_bittiming_const = {
> >
> 
> 
> --
> 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 |