Re: [PATCH linux-can-next/flexcan 1/4] can: flexcan: initialize all flexcan memory for ECC function

* Re: [PATCH linux-can-next/flexcan 1/4] can: flexcan: initialize all flexcan memory for ECC function
  2020-09-25 15:10 ` [PATCH linux-can-next/flexcan 1/4] can: flexcan: initialize all flexcan memory for ECC function Joakim Zhang
@ 2020-09-25  7:29   ` Marc Kleine-Budde
  2020-09-25  7:49     ` Joakim Zhang
  2020-09-27  8:01     ` Joakim Zhang
  2020-09-25  7:33   ` Marc Kleine-Budde
  1 sibling, 2 replies; 32+ messages in thread
From: Marc Kleine-Budde @ 2020-09-25  7:29 UTC (permalink / raw)
  To: Joakim Zhang, linux-can; +Cc: linux-imx, netdev

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

On 9/25/20 5:10 PM, Joakim Zhang wrote:
> There is a NOTE at the section "Detection and correction of memory errors":

Can you add a reference to one datasheet including name, revision and section?

> 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), these memory can be initialized or not.
> 
> Initialize all FlexCAN memory before accessing them, otherwise, memory
> errors may be detected. The internal region cannot be initialized when
> the hardware does not support ECC.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 92 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 286c67196592..f02f1de2bbca 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -292,7 +292,16 @@ struct flexcan_regs {
>  	u32 rximr[64];		/* 0x880 - Not affected by Soft Reset */
>  	u32 _reserved5[24];	/* 0x980 */
>  	u32 gfwr_mx6;		/* 0x9e0 - MX6 */
> -	u32 _reserved6[63];	/* 0x9e4 */
> +	u32 _reserved6[39];	/* 0x9e4 */
> +	u32 _rxfir[6];		/* 0xa80 */
> +	u32 _reserved8[2];	/* 0xa98 */
> +	u32 _rxmgmask;		/* 0xaa0 */
> +	u32 _rxfgmask;		/* 0xaa4 */
> +	u32 _rx14mask;		/* 0xaa8 */
> +	u32 _rx15mask;		/* 0xaac */
> +	u32 tx_smb[4];		/* 0xab0 */
> +	u32 rx_smb0[4];		/* 0xac0 */
> +	u32 rx_smb1[4];		/* 0xad0 */
>  	u32 mecr;		/* 0xae0 */
>  	u32 erriar;		/* 0xae4 */
>  	u32 erridpr;		/* 0xae8 */
> @@ -305,9 +314,13 @@ struct flexcan_regs {
>  	u32 fdctrl;		/* 0xc00 - Not affected by Soft Reset */
>  	u32 fdcbt;		/* 0xc04 - Not affected by Soft Reset */
>  	u32 fdcrc;		/* 0xc08 */
> +	u32 _reserved9[199];	/* 0xc0c */
> +	u32 tx_smb_fd[18];	/* 0xf28 */
> +	u32 rx_smb0_fd[18];	/* 0xf70 */
> +	u32 rx_smb1_fd[18];	/* 0xfb8 */
>  };
>  
> -static_assert(sizeof(struct flexcan_regs) == 0x4 + 0xc08);
> +static_assert(sizeof(struct flexcan_regs) == 0x4 * 18 + 0xfb8);
>  
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
> @@ -1292,6 +1305,78 @@ 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, size;
> +
> +	/* CTRL2[WRMFRZ] grants write access to all memory positions that
> +	 * require initialization. MCR[RFEN] must not be set during FlexCAN
> +	 * memory initialization.

Please add here the reference to the datasheet aswell.

> +	 */
> +	reg_ctrl2 = priv->read(&regs->ctrl2);
> +	reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +
> +	/* initialize MBs RAM */
> +	size = sizeof(regs->mb) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->mb[0][0] + sizeof(u32) * i);

Can you create a "static const struct" holding the reg (or offset) + len and
loop over it. Something linke this?

const struct struct flexcan_ram_init ram_init[] {
	void __iomem *reg;
	u16 len;
} = {
	{
		.reg = regs->mb,	/* MB RAM */
		.len = sizeof(regs->mb), / sizeof(u32),
	}, {
		.reg = regs->rximr,	/* RXIMR RAM */
		.len = sizeof(regs->rximr),
	}, {
		...
	},
};

> +
> +	/* initialize RXIMRs RAM */
> +	size = sizeof(regs->rximr) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->rximr[i]);
> +
> +	/* initialize RXFIRs RAM */
> +	size = sizeof(regs->_rxfir) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->_rxfir[i]);
> +
> +	/* initialize RXMGMASK, RXFGMASK, RX14MASK, RX15MASK RAM */
> +	priv->write(0, &regs->_rxmgmask);
> +	priv->write(0, &regs->_rxfgmask);
> +	priv->write(0, &regs->_rx14mask);
> +	priv->write(0, &regs->_rx15mask);
> +
> +	/* initialize TX_SMB RAM */
> +	size = sizeof(regs->tx_smb) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->tx_smb[i]);
> +
> +	/* initialize RX_SMB0 RAM */
> +	size = sizeof(regs->rx_smb0) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->rx_smb0[i]);
> +
> +	/* initialize RX_SMB1 RAM */
> +	size = sizeof(regs->rx_smb1) / sizeof(u32);
> +	for (i = 0; i < size; i++)
> +		priv->write(0, &regs->rx_smb1[i]);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +		/* initialize TX_SMB_FD RAM */

and the same for the fd-mode

> +		size = sizeof(regs->tx_smb_fd) / sizeof(u32);
> +		for (i = 0; i < size; i++)
> +			priv->write(0, &regs->tx_smb_fd[i]);
> +
> +		/* initialize RX_SMB0_FD RAM */
> +		size = sizeof(regs->rx_smb0_fd) / sizeof(u32);
> +		for (i = 0; i < size; i++)
> +			priv->write(0, &regs->rx_smb0_fd[i]);
> +
> +		/* initialize RX_SMB1_FD RAM */
> +		size = sizeof(regs->rx_smb1_fd) / sizeof(u32);
> +		for (i = 0; i < size; i++)
> +			priv->write(0, &regs->rx_smb0_fd[i]);
> +	}
> +
> +	reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> +	priv->write(reg_ctrl2, &regs->ctrl2);
> +}
> +
>  /* flexcan_chip_start
>   *
>   * this functions is entered with clocks enabled
> @@ -1316,6 +1401,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);
> +
>  	flexcan_set_bittiming(dev);
>  
>  	/* MCR
> 

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

^ permalink raw reply	[flat|nested] 32+ messages in thread