> -----Original Message-----
> From: Marc Kleine-Budde <m...@pengutronix.de>
> Sent: 2020年9月25日 15:29
> To: Joakim Zhang <qiangqing.zh...@nxp.com>; linux-...@vger.kernel.org
> Cc: dl-linux-imx <linux-...@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?
Ok.
> > 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.zh...@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.
OK.
> > + */
> > + reg_ctrl2 = priv->read(®s->ctrl2);
> > + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
> > + priv->write(reg_ctrl2, ®s->ctrl2);
> > +
> > + /* initialize MBs RAM */
> > + size = sizeof(regs->mb) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->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),
> }, {
> ...
> },
> };
Better solution, I will change into this way. Thanks.
>
> > +
> > + /* initialize RXIMRs RAM */
> > + size = sizeof(regs->rximr) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->rximr[i]);
> > +
> > + /* initialize RXFIRs RAM */
> > + size = sizeof(regs->_rxfir) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->_rxfir[i]);
> > +
> > + /* initialize RXMGMASK, RXFGMASK, RX14MASK, RX15MASK RAM */
> > + priv->write(0, ®s->_rxmgmask);
> > + priv->write(0, ®s->_rxfgmask);
> > + priv->write(0, ®s->_rx14mask);
> > + priv->write(0, ®s->_rx15mask);
> > +
> > + /* initialize TX_SMB RAM */
> > + size = sizeof(regs->tx_smb) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->tx_smb[i]);
> > +
> > + /* initialize RX_SMB0 RAM */
> > + size = sizeof(regs->rx_smb0) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->rx_smb0[i]);
> > +
> > + /* initialize RX_SMB1 RAM */
> > + size = sizeof(regs->rx_smb1) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->rx_smb1[i]);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > + /* initialize TX_SMB_FD RAM */
>
> and the same for the fd-mode
OK.
Best Regards,
Joakim Zhang
> > + size = sizeof(regs->tx_smb_fd) / sizeof(u32);
> > + for (i = 0; i < size; i++)
> > + priv->write(0, ®s->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, ®s->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, ®s->rx_smb0_fd[i]);
> > + }
> > +
> > + reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
> > + priv->write(reg_ctrl2, ®s->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 |
