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

[Joakim Zhang <qiangqing.zhang@nxp.com>]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年9月25日 15:29
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH linux-can-next/flexcan 1/4] can: flexcan: initialize all flexcan
> memory for ECC function
> 
> 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>
> > +	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),
> 	}, {
> 		...
> 	},
> };

In this version, I only initialize the implemented memory, so that it's a several trivial memory slice, reserved memory not initialized. Follow your point, I need create a global pointer for struct flexcan_reg,
i.e. static struct flexcan_regs *reg, so that we can use .reg = regs->mb in ram_init[], IMHO, I don't quite want to add this, or is there any better solution to get the reg/len value?

According to below notes and discussed with IP owner before, reserved memory also can be initialized. So I want to add two memory regions, and initialize them together,
this could be more clean. I will send out a V2, please let me know which one do you think is better?

"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."

Best Regards,
Joakim Zhang