Message-ID: <c2299dd3-e818-0192-eae4-02c045b83a30@pengutronix.de>
Date:   Sun, 27 Sep 2020 21:51:48 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Joakim Zhang <qiangqing.zhang@....com>, linux-can@...r.kernel.org
Cc:     linux-imx@....com, netdev@...r.kernel.org
Subject: Re: [PATCH V2 1/3] can: flexcan: initialize all flexcan memory for
 ECC function

On 9/27/20 6:07 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. 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.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> ---
> ChangeLogs:
> V1->V2:
> 	* update commit messages, add a datasheet reference.
> 	* initialize block memory instead of trivial memory.
> 	* inilialize reserved memory.
> ---
>  drivers/net/can/flexcan.c | 67 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e86925134009..aca0fc40ae9b 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -309,6 +309,40 @@ struct flexcan_regs {
>  
>  static_assert(sizeof(struct flexcan_regs) == 0x4 + 0xc08);
>  
> +/* Structure of memory need be initialized for ECC feature */
> +static const struct flexcan_ram_int {
> +	u32 offset;
> +	u16 len;
> +} ram_init[] = {
> +	/* 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
> +	 */
> +	{
> +		.offset = 0x80,
> +		.len = (0xadf - 0x80) / sizeof(u32) + 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
> +	 */
> +	{
> +		.offset = 0xf28,
> +		.len = (0xfff - 0xf28) / sizeof(u32) + 1,
> +	},
> +};

As it's only two ranges, I think there's no need for this struct. Directly move
code that into the for loops.

> +
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
>  };
> @@ -1292,6 +1326,36 @@ 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;
> +	int i;
> +
> +	/* 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);
> +
> +	for (i = 0; i < ram_init[0].len; i++)
> +		priv->write(0, (void __iomem *)regs + ram_init[0].offset + sizeof(u32) * i);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> +		for (i = 0; i < ram_init[1].len; i++)
> +			priv->write(0, (void __iomem *)regs + ram_init[1].offset + sizeof(u32) * i);
> +
> +	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +}
> +
>  /* flexcan_chip_start
>   *
>   * this functions is entered with clocks enabled
> @@ -1316,6 +1380,9 @@ static int flexcan_chip_start(struct net_device *dev)
>  	if (err)
>  		goto out_chip_disable;
>  
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_DISABLE_MECR)
> +		flexcan_init_ram(dev);

Can you test this on both layerscape SoCs (fsl,ls1021ar2-flexcan and
fsl,lx2160ar1-flexcan)

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



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists