[PATCH V3 0/3] patch set for flexcan

[PATCH V3 0/3] patch set for flexcan

[Joakim Zhang]

Hi Marc,

ECC is enabled by default if SoCs support this feature, so I think the
common solution is to initialize all flexcan memory for these SoCs. For
that, I create FLEXCAN_QUIRK_SUPPORT_ECC quirk to add this feature, then
users can decide to whether select FLEXCAN_QUIRK_DISABLE_MECR qurik or not.

If you agree with me, later I will add a patch into patchset to modify
devtype_data for vf610, ls1021a and lx2160a, after Pankaj's check.

Joakim Zhang (3):
  can: flexcan: initialize all flexcan memory for ECC function
  can: flexcan: add flexcan driver for i.MX8MP
  can: flexcan: disable runtime PM if register flexcandev failed

 drivers/net/can/flexcan.c | 61 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

-- 
2.17.1

[PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Joakim Zhang]

One issue was reported at a baremetal environment, which is used for
FPGA verification. "The first transfer will fail for extended ID
format(for both 2.0B and FD format), following frames can be transmitted
and received successfully for extended format, and standard format don't
have this issue. This issue occurred randomly with high possiblity, when
it occurs, the transmitter will detect a BIT1 error, the receiver a CRC
error. According to the spec, a non-correctable error may cause this
transfer failure."

With FLEXCAN_QUIRK_DISABLE_MECR quirk, it supports correctable errors,
disable non-correctable errors interrupt and freeze mode. Platform has
ECC hardware support, but select this quirk, this issue may not come to
light. Initialize all FlexCAN memory before accessing them, at least it
can avoid non-correctable errors detected due to memory uninitialized.
The internal region can't be initialized when the hardware doesn't support
ECC.

According to IMX8MPRM, Rev.C, 04/2020. There is a NOTE at the section
11.8.3.13 Detection and correction of memory errors:
"All FlexCAN memory must be initialized before starting its operation in
order to have the parity bits in memory properly updated. CTRL2[WRMFRZ]
grants write access to all memory positions that require initialization,
ranging from 0x080 to 0xADF and from 0xF28 to 0xFFF when the CAN FD feature
is enabled. The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers need to
be initialized as well. MCR[RFEN] must not be set during memory initialization."

Memory range from 0x080 to 0xADF, there are reserved memory (unimplemented
by hardware, e.g. only configure 64 MBs), these memory can be initialized or not.
In this patch, initialize all flexcan memory which includes reserved memory.

In this patch, create FLEXCAN_QUIRK_SUPPORT_ECC for platforms which has ECC
feature. If you have a ECC platform in your hand, please select this
qurik to initialize all flexcan memory firstly, then you can select
FLEXCAN_QUIRK_DISABLE_MECR to only enable correctable errors.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLogs:
V1->V2:
	* update commit messages, add a datasheet reference.
	* initialize block memory instead of trivial memory.
	* inilialize reserved memory.
V2->V3:
	* add FLEXCAN_QUIRK_SUPPORT_ECC quirk.
	* remove init_ram struct.
---
 drivers/net/can/flexcan.c | 50 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e86925134009..0ae7436ee6ef 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -239,6 +239,8 @@
 #define FLEXCAN_QUIRK_SETUP_STOP_MODE BIT(8)
 /* Support CAN-FD mode */
 #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
+/* support memory detection and correction */
+#define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -1292,6 +1294,51 @@ static void flexcan_set_bittiming(struct net_device *dev)
 		return flexcan_set_bittiming_ctrl(dev);
 }
 
+static void flexcan_init_ram(struct net_device *dev)
+{
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_ctrl2;
+
+	/* 11.8.3.13 Detection and correction of memory errors:
+	 * CTRL2[WRMFRZ] grants write access to all memory positions that
+	 * require initialization, ranging from 0x080 to 0xADF and
+	 * from 0xF28 to 0xFFF when the CAN FD feature is enabled.
+	 * The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers need to
+	 * be initialized as well. MCR[RFEN] must not be set during memory
+	 * initialization.
+	 */
+	reg_ctrl2 = priv->read(&regs->ctrl2);
+	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
+	priv->write(reg_ctrl2, &regs->ctrl2);
+
+	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
+	 * 0x0080--0x087F:	128 MBs
+	 * 0x0880--0x0A7F:	128 RXIMRs
+	 * 0x0A80--0x0A97:	6 RXFIRs
+	 * 0x0A98--0x0A9F:	Reserved
+	 * 0x0AA0--0x0AA3:	RXMGMASK
+	 * 0x0AA4--0x0AA7:	RXFGMASK
+	 * 0x0AA8--0x0AAB:	RX14MASK
+	 * 0x0AAC--0x0AAF:	RX15MASK
+	 * 0x0AB0--0x0ABF:	TX_SMB
+	 * 0x0AC0--0x0ACF:	RX_SMB0
+	 * 0x0AD0--0x0ADF:	RX_SMB1
+	 */
+	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);
+
+	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
+	 * ram details as below list:
+	 * 0x0F28--0x0F6F:	TX_SMB_FD
+	 * 0x0F70--0x0FB7:	RX_SMB0_FD
+	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
+	 */
+	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);
+
+	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
+	priv->write(reg_ctrl2, &regs->ctrl2);
+}
+
 /* flexcan_chip_start
  *
  * this functions is entered with clocks enabled
@@ -1316,6 +1363,9 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (err)
 		goto out_chip_disable;
 
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_ECC)
+		flexcan_init_ram(dev);
+
 	flexcan_set_bittiming(dev);
 
 	/* MCR
-- 
2.17.1

[PATCH V3 2/3] can: flexcan: add flexcan driver for i.MX8MP

[Joakim Zhang]

Add flexcan driver for i.MX8MP, which supports CAN FD and ECC.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLogs:
V1->V2:
	* sort the order of the quirks by their value.
V2->V3:
	* add FLEXCAN_QUIRK_SUPPORT_ECC for i.MX8MP.
---
 drivers/net/can/flexcan.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0ae7436ee6ef..925efc986b6b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -214,6 +214,7 @@
  *   MX53  FlexCAN2  03.00.00.00    yes        no        no       no        no           no
  *   MX6s  FlexCAN3  10.00.12.00    yes       yes        no       no       yes           no
  *   MX8QM FlexCAN3  03.00.23.00    yes       yes        no       no       yes          yes
+ *   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
@@ -378,6 +379,13 @@ static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
 		FLEXCAN_QUIRK_SUPPORT_FD,
 };
 
+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_SUPPORT_FD | FLEXCAN_QUIRK_SUPPORT_ECC,
+};
+
 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 |
@@ -1895,6 +1903,7 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 
 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,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, },
-- 
2.17.1

[PATCH V3 3/3] can: flexcan: disable runtime PM if register flexcandev failed

[Joakim Zhang]

Disable runtime PM if register flexcandev failed, and balance reference
of usage_count.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLogs:
V1->V3:
	* no changes.
---
 drivers/net/can/flexcan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 925efc986b6b..fbbc90454df1 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -2058,6 +2058,8 @@ static int flexcan_probe(struct platform_device *pdev)
 	return 0;
 
  failed_register:
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	free_candev(dev);
 	return err;
 }
-- 
2.17.1

RE: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2020年9月29日 2:03
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC
> function
> 
> One issue was reported at a baremetal environment, which is used for FPGA
> verification. "The first transfer will fail for extended ID format(for both 2.0B and
> FD format), following frames can be transmitted and received successfully for
> extended format, and standard format don't have this issue. This issue
> occurred randomly with high possiblity, when it occurs, the transmitter will
> detect a BIT1 error, the receiver a CRC error. According to the spec, a
> non-correctable error may cause this transfer failure."
> 
> With FLEXCAN_QUIRK_DISABLE_MECR quirk, it supports correctable errors,
> disable non-correctable errors interrupt and freeze mode. Platform has ECC
> hardware support, but select this quirk, this issue may not come to light.
> Initialize all FlexCAN memory before accessing them, at least it can avoid
> non-correctable errors detected due to memory uninitialized.
> The internal region can't be initialized when the hardware doesn't support ECC.
> 
> According to IMX8MPRM, Rev.C, 04/2020. There is a NOTE at the section
> 11.8.3.13 Detection and correction of memory errors:
> "All FlexCAN memory must be initialized before starting its operation in order
> to have the parity bits in memory properly updated. CTRL2[WRMFRZ] grants
> write access to all memory positions that require initialization, ranging from
> 0x080 to 0xADF and from 0xF28 to 0xFFF when the CAN FD feature is enabled.
> The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers need to be
> initialized as well. MCR[RFEN] must not be set during memory initialization."
> 
> Memory range from 0x080 to 0xADF, there are reserved memory
> (unimplemented by hardware, e.g. only configure 64 MBs), these memory can
> be initialized or not.
> In this patch, initialize all flexcan memory which includes reserved memory.
> 
> In this patch, create FLEXCAN_QUIRK_SUPPORT_ECC for platforms which has
> ECC feature. If you have a ECC platform in your hand, please select this qurik to
> initialize all flexcan memory firstly, then you can select
> FLEXCAN_QUIRK_DISABLE_MECR to only enable correctable errors.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> ChangeLogs:
> V1->V2:
> 	* update commit messages, add a datasheet reference.
> 	* initialize block memory instead of trivial memory.
> 	* inilialize reserved memory.
> V2->V3:
> 	* add FLEXCAN_QUIRK_SUPPORT_ECC quirk.
> 	* remove init_ram struct.
> ---
>  drivers/net/can/flexcan.c | 50
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> e86925134009..0ae7436ee6ef 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -239,6 +239,8 @@
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE BIT(8)
>  /* Support CAN-FD mode */
>  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
> +/* support memory detection and correction */ #define
> +FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
> 
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -1292,6 +1294,51 @@ static void flexcan_set_bittiming(struct net_device
> *dev)
>  		return flexcan_set_bittiming_ctrl(dev);  }
> 
> +static void flexcan_init_ram(struct net_device *dev) {
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_ctrl2;
> +
> +	/* 11.8.3.13 Detection and correction of memory errors:
> +	 * CTRL2[WRMFRZ] grants write access to all memory positions that
> +	 * require initialization, ranging from 0x080 to 0xADF and
> +	 * from 0xF28 to 0xFFF when the CAN FD feature is enabled.
> +	 * The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers
> need to
> +	 * be initialized as well. MCR[RFEN] must not be set during memory
> +	 * initialization.
> +	 */
> +	reg_ctrl2 = priv->read(&regs->ctrl2);
> +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +
> +	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
> +	 * 0x0080--0x087F:	128 MBs
> +	 * 0x0880--0x0A7F:	128 RXIMRs
> +	 * 0x0A80--0x0A97:	6 RXFIRs
> +	 * 0x0A98--0x0A9F:	Reserved
> +	 * 0x0AA0--0x0AA3:	RXMGMASK
> +	 * 0x0AA4--0x0AA7:	RXFGMASK
> +	 * 0x0AA8--0x0AAB:	RX14MASK
> +	 * 0x0AAC--0x0AAF:	RX15MASK
> +	 * 0x0AB0--0x0ABF:	TX_SMB
> +	 * 0x0AC0--0x0ACF:	RX_SMB0
> +	 * 0x0AD0--0x0ADF:	RX_SMB1
> +	 */
> +	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);
> +
> +	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
> +	 * ram details as below list:
> +	 * 0x0F28--0x0F6F:	TX_SMB_FD
> +	 * 0x0F70--0x0FB7:	RX_SMB0_FD
> +	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
> +	 */
> +	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);

Sorry, should be this, please ignore it first.

if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);

Best Regards,
Joakim Zhang
> +	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +}
> +
>  /* flexcan_chip_start
>   *
>   * this functions is entered with clocks enabled @@ -1316,6 +1363,9 @@
> static int flexcan_chip_start(struct net_device *dev)
>  	if (err)
>  		goto out_chip_disable;
> 
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_ECC)
> +		flexcan_init_ram(dev);
> +
>  	flexcan_set_bittiming(dev);
> 
>  	/* MCR
> --
> 2.17.1

Re: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Marc Kleine-Budde]

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

On 9/28/20 8:02 PM, Joakim Zhang wrote:
> One issue was reported at a baremetal environment, which is used for
> FPGA verification. "The first transfer will fail for extended ID
> format(for both 2.0B and FD format), following frames can be transmitted
> and received successfully for extended format, and standard format don't
> have this issue. This issue occurred randomly with high possiblity, when
> it occurs, the transmitter will detect a BIT1 error, the receiver a CRC
> error. According to the spec, a non-correctable error may cause this
> transfer failure."
> 
> With FLEXCAN_QUIRK_DISABLE_MECR quirk, it supports correctable errors,
> disable non-correctable errors interrupt and freeze mode. Platform has
> ECC hardware support, but select this quirk, this issue may not come to
> light. Initialize all FlexCAN memory before accessing them, at least it
> can avoid non-correctable errors detected due to memory uninitialized.
> The internal region can't be initialized when the hardware doesn't support
> ECC.
> 
> According to IMX8MPRM, Rev.C, 04/2020. There is a NOTE at the section
> 11.8.3.13 Detection and correction of memory errors:
> "All FlexCAN memory must be initialized before starting its operation in
> order to have the parity bits in memory properly updated. CTRL2[WRMFRZ]
> grants write access to all memory positions that require initialization,
> ranging from 0x080 to 0xADF and from 0xF28 to 0xFFF when the CAN FD feature
> is enabled. The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers need to
> be initialized as well. MCR[RFEN] must not be set during memory initialization."
> 
> Memory range from 0x080 to 0xADF, there are reserved memory (unimplemented
> by hardware, e.g. only configure 64 MBs), these memory can be initialized or not.
> In this patch, initialize all flexcan memory which includes reserved memory.
> 
> In this patch, create FLEXCAN_QUIRK_SUPPORT_ECC for platforms which has ECC
> feature. If you have a ECC platform in your hand, please select this
> qurik to initialize all flexcan memory firstly, then you can select
> FLEXCAN_QUIRK_DISABLE_MECR to only enable correctable errors.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> ChangeLogs:
> V1->V2:
> 	* update commit messages, add a datasheet reference.
> 	* initialize block memory instead of trivial memory.
> 	* inilialize reserved memory.
> V2->V3:
> 	* add FLEXCAN_QUIRK_SUPPORT_ECC quirk.
> 	* remove init_ram struct.
> ---
>  drivers/net/can/flexcan.c | 50 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e86925134009..0ae7436ee6ef 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -239,6 +239,8 @@
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE BIT(8)
>  /* Support CAN-FD mode */
>  #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9)
> +/* support memory detection and correction */
> +#define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>  
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -1292,6 +1294,51 @@ static void flexcan_set_bittiming(struct net_device *dev)
>  		return flexcan_set_bittiming_ctrl(dev);
>  }
>  
> +static void flexcan_init_ram(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_ctrl2;
> +
> +	/* 11.8.3.13 Detection and correction of memory errors:
> +	 * CTRL2[WRMFRZ] grants write access to all memory positions that
> +	 * require initialization, ranging from 0x080 to 0xADF and
> +	 * from 0xF28 to 0xFFF when the CAN FD feature is enabled.
> +	 * The RXMGMASK, RX14MASK, RX15MASK, and RXFGMASK registers need to
> +	 * be initialized as well. MCR[RFEN] must not be set during memory
> +	 * initialization.
> +	 */
> +	reg_ctrl2 = priv->read(&regs->ctrl2);
> +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +
> +	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
> +	 * 0x0080--0x087F:	128 MBs
> +	 * 0x0880--0x0A7F:	128 RXIMRs
> +	 * 0x0A80--0x0A97:	6 RXFIRs
> +	 * 0x0A98--0x0A9F:	Reserved
> +	 * 0x0AA0--0x0AA3:	RXMGMASK
> +	 * 0x0AA4--0x0AA7:	RXFGMASK
> +	 * 0x0AA8--0x0AAB:	RX14MASK
> +	 * 0x0AAC--0x0AAF:	RX15MASK
> +	 * 0x0AB0--0x0ABF:	TX_SMB
> +	 * 0x0AC0--0x0ACF:	RX_SMB0
> +	 * 0x0AD0--0x0ADF:	RX_SMB1

I don't like to have the register definition here *again), we have struct
flexcan_regs for this.

> +	 */
> +	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);

why the cast?

Can you use the "&regs->foo - &regs->bar + x" to get the length for the memset?

> +
> +	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
> +	 * ram details as below list:
> +	 * 0x0F28--0x0F6F:	TX_SMB_FD
> +	 * 0x0F70--0x0FB7:	RX_SMB0_FD
> +	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
> +	 */
> +	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);

same here

> +
> +	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +}
> +
>  /* flexcan_chip_start
>   *
>   * this functions is entered with clocks enabled
> @@ -1316,6 +1363,9 @@ static int flexcan_chip_start(struct net_device *dev)
>  	if (err)
>  		goto out_chip_disable;
>  
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_ECC)
> +		flexcan_init_ram(dev);
> +
>  	flexcan_set_bittiming(dev);
>  
>  	/* MCR
> 

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 V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年9月29日 18:54
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC
> function

[...]
> > +	reg_ctrl2 = priv->read(&regs->ctrl2);
> > +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> > +	priv->write(reg_ctrl2, &regs->ctrl2);
> > +
> > +	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
> > +	 * 0x0080--0x087F:	128 MBs
> > +	 * 0x0880--0x0A7F:	128 RXIMRs
> > +	 * 0x0A80--0x0A97:	6 RXFIRs
> > +	 * 0x0A98--0x0A9F:	Reserved
> > +	 * 0x0AA0--0x0AA3:	RXMGMASK
> > +	 * 0x0AA4--0x0AA7:	RXFGMASK
> > +	 * 0x0AA8--0x0AAB:	RX14MASK
> > +	 * 0x0AAC--0x0AAF:	RX15MASK
> > +	 * 0x0AB0--0x0ABF:	TX_SMB
> > +	 * 0x0AC0--0x0ACF:	RX_SMB0
> > +	 * 0x0AD0--0x0ADF:	RX_SMB1
> 
> I don't like to have the register definition here *again), we have struct
> flexcan_regs for this.

Do you mean still move these register definitions into flexcan_regs, right?

> > +	 */
> > +	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);
> 
> why the cast?

Yes, no need, will remove it.

> Can you use the "&regs->foo - &regs->bar + x" to get the length for the
> memset?

After move above register definition into flexcan_regs, I can change to use this way to get the length for the memset_io.


> > +
> > +	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
> > +	 * ram details as below list:
> > +	 * 0x0F28--0x0F6F:	TX_SMB_FD
> > +	 * 0x0F70--0x0FB7:	RX_SMB0_FD
> > +	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
> > +	 */
> > +	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);
> 
> same here

Will change.

Hi Marc, I'm going on holiday from tomorrow, so I would delay to send out a V4 to review until I come back, sorry for this.
Thanks for your comments of "can: flexcan: add CAN wakeup function for i.MX8", will rework the patch later.
 
Best Regards,
Joakim Zhang
> > +
> > +	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> > +	priv->write(reg_ctrl2, &regs->ctrl2); }
> > +
> >  /* flexcan_chip_start
> >   *
> >   * this functions is entered with clocks enabled @@ -1316,6 +1363,9
> > @@ static int flexcan_chip_start(struct net_device *dev)
> >  	if (err)
> >  		goto out_chip_disable;
> >
> > +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_ECC)
> > +		flexcan_init_ram(dev);
> > +
> >  	flexcan_set_bittiming(dev);
> >
> >  	/* MCR
> >
> 
> 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 V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Marc Kleine-Budde]

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

On 9/29/20 1:27 PM, Joakim Zhang wrote:
> [...]
>>> +	reg_ctrl2 = priv->read(&regs->ctrl2);
>>> +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
>>> +	priv->write(reg_ctrl2, &regs->ctrl2);
>>> +
>>> +	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
>>> +	 * 0x0080--0x087F:	128 MBs
>>> +	 * 0x0880--0x0A7F:	128 RXIMRs
>>> +	 * 0x0A80--0x0A97:	6 RXFIRs
>>> +	 * 0x0A98--0x0A9F:	Reserved
>>> +	 * 0x0AA0--0x0AA3:	RXMGMASK
>>> +	 * 0x0AA4--0x0AA7:	RXFGMASK
>>> +	 * 0x0AA8--0x0AAB:	RX14MASK
>>> +	 * 0x0AAC--0x0AAF:	RX15MASK
>>> +	 * 0x0AB0--0x0ABF:	TX_SMB
>>> +	 * 0x0AC0--0x0ACF:	RX_SMB0
>>> +	 * 0x0AD0--0x0ADF:	RX_SMB1
>>
>> I don't like to have the register definition here *again), we have struct
>> flexcan_regs for this.
> 
> Do you mean still move these register definitions into flexcan_regs, right?

ack

>>> +	 */
>>> +	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);
>>
>> why the cast?
> 
> Yes, no need, will remove it.
> 
>> Can you use the "&regs->foo - &regs->bar + x" to get the length for the
>> memset?
> 
> After move above register definition into flexcan_regs, I can change to use
> this way to get the length for the memset_io.

ACK

>>> +
>>> +	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
>>> +	 * ram details as below list:
>>> +	 * 0x0F28--0x0F6F:	TX_SMB_FD
>>> +	 * 0x0F70--0x0FB7:	RX_SMB0_FD
>>> +	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
>>> +	 */
>>> +	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);
>>
>> same here
> 
> Will change.

tnx

> Hi Marc, I'm going on holiday from tomorrow, so I would delay to send out a
> V4 to review until I come back, sorry for this. Thanks for your comments of
> "can: flexcan: add CAN wakeup function for i.MX8", will rework the patch
> later.

Fine with me, I think we can push these changes via net to v5.10 after the v5.9
release is out.

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 V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年9月29日 19:31
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC
> function
> 
> On 9/29/20 1:27 PM, Joakim Zhang wrote:
> > [...]
> >>> +	reg_ctrl2 = priv->read(&regs->ctrl2);
> >>> +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> >>> +	priv->write(reg_ctrl2, &regs->ctrl2);
> >>> +
> >>> +	/* ranging from 0x0080 to 0x0ADF, ram details as below list:
> >>> +	 * 0x0080--0x087F:	128 MBs
> >>> +	 * 0x0880--0x0A7F:	128 RXIMRs
> >>> +	 * 0x0A80--0x0A97:	6 RXFIRs
> >>> +	 * 0x0A98--0x0A9F:	Reserved
> >>> +	 * 0x0AA0--0x0AA3:	RXMGMASK
> >>> +	 * 0x0AA4--0x0AA7:	RXFGMASK
> >>> +	 * 0x0AA8--0x0AAB:	RX14MASK
> >>> +	 * 0x0AAC--0x0AAF:	RX15MASK
> >>> +	 * 0x0AB0--0x0ABF:	TX_SMB
> >>> +	 * 0x0AC0--0x0ACF:	RX_SMB0
> >>> +	 * 0x0AD0--0x0ADF:	RX_SMB1
> >>
> >> I don't like to have the register definition here *again), we have
> >> struct flexcan_regs for this.
> >
> > Do you mean still move these register definitions into flexcan_regs, right?
> 
> ack
> 
> >>> +	 */
> >>> +	memset_io((void __iomem *)regs + 0x80, 0, 0xadf - 0x80 + 1);
> >>
> >> why the cast?
> >
> > Yes, no need, will remove it.
> >
> >> Can you use the "&regs->foo - &regs->bar + x" to get the length for
> >> the memset?
> >
> > After move above register definition into flexcan_regs, I can change
> > to use this way to get the length for the memset_io.
> 
> ACK
> 
> >>> +
> >>> +	/* ranging from 0x0F28 to 0x0FFF when CAN FD feature is enabled,
> >>> +	 * ram details as below list:
> >>> +	 * 0x0F28--0x0F6F:	TX_SMB_FD
> >>> +	 * 0x0F70--0x0FB7:	RX_SMB0_FD
> >>> +	 * 0x0FB8--0x0FFF:	RX_SMB0_FD
> >>> +	 */
> >>> +	memset_io((void __iomem *)regs + 0xf28, 0, 0xfff - 0xf28 + 1);
> >>
> >> same here
> >
> > Will change.
> 
> tnx
> 
> > Hi Marc, I'm going on holiday from tomorrow, so I would delay to send
> > out a
> > V4 to review until I come back, sorry for this. Thanks for your
> > comments of
> > "can: flexcan: add CAN wakeup function for i.MX8", will rework the
> > patch later.
> 
> Fine with me, I think we can push these changes via net to v5.10 after the v5.9
> release is out.

I don't know whether to rush into V5.10, since I will back on Oct 12th. If you have such plan, I will rework the patch right now.
After I test, I will send out the V4.

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

Re: [PATCH V3 1/3] can: flexcan: initialize all flexcan memory for ECC function

[Marc Kleine-Budde]

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

On 9/29/20 1:36 PM, Joakim Zhang wrote:
>> Fine with me, I think we can push these changes via net to v5.10 after the v5.9
>> release is out.
> 
> I don't know whether to rush into V5.10, since I will back on Oct 12th. If
> you have such plan, I will rework the patch right now.
>
> After I test, I will send out the V4.
Ok, ho hurries :) And enjoy your holidays.

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