| Message ID | 20200925151028.11004-2-qiangqing.zhang@nxp.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | patch set for flexcan | expand |
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(®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), }, { ... }, }; > + > + /* 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 > + 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
On 9/25/20 5:10 PM, Joakim Zhang wrote: > There is a NOTE at the section "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), 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> Is this whole patch valid/compatible with the mx7,too? Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月25日 15:33 > 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": > > 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> > > Is this whole patch valid/compatible with the mx7,too? Hi Marc, I notice it just now, seems lack of patch for imx firmware in upstream, that will always export scu symbols. include/linux/firmware/imx/svc/misc.h 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 |
> -----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? 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.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. 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 |
On 9/25/20 9:38 AM, Joakim Zhang wrote: > I notice it just now, seems lack of patch for imx firmware in upstream, that > will always export scu symbols. include/linux/firmware/imx/svc/misc.h That will affect "can: flexcan: add CAN wakeup function for i.MX8" not this patch, right? Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月25日 16:11 > 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 9:38 AM, Joakim Zhang wrote: > > I notice it just now, seems lack of patch for imx firmware in > > upstream, that will always export scu symbols. > > include/linux/firmware/imx/svc/misc.h > > That will affect "can: flexcan: add CAN wakeup function for i.MX8" not this > patch, right? Hi Marc, Yes, only affect "can: flexcan: add CAN wakeup function for i.MX8", I will remove this patch first. Sorry, I replied in a wrong place. "can: flexcan: initialize all flexcan memory for ECC function" for this patch, I find this issue in i.MX8MP, which is the first SoC implements ECC for i.MX I think this patch should compatible with others which has ECC support, but I don't have one to have a test. 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 |
On 9/25/20 10:51 AM, Joakim Zhang wrote: >>> I notice it just now, seems lack of patch for imx firmware in >>> upstream, that will always export scu symbols. >>> include/linux/firmware/imx/svc/misc.h >> >> That will affect "can: flexcan: add CAN wakeup function for i.MX8" not this >> patch, right? > > Yes, only affect "can: flexcan: add CAN wakeup function for i.MX8", I will > remove this patch first. ok > Sorry, I replied in a wrong place. np > "can: flexcan: initialize all flexcan memory for ECC function" for this > patch, I find this issue in i.MX8MP, which is the first SoC implements ECC > for i.MX What about the mx7? > I think this patch should compatible with others which has ECC support, but I > don't have one to have a test. What about the mx7? Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月25日 17:03 > 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 10:51 AM, Joakim Zhang wrote: > >>> I notice it just now, seems lack of patch for imx firmware in > >>> upstream, that will always export scu symbols. > >>> include/linux/firmware/imx/svc/misc.h > >> > >> That will affect "can: flexcan: add CAN wakeup function for i.MX8" > >> not this patch, right? > > > > Yes, only affect "can: flexcan: add CAN wakeup function for i.MX8", I > > will remove this patch first. > > ok > > > Sorry, I replied in a wrong place. > > np > > > "can: flexcan: initialize all flexcan memory for ECC function" for > > this patch, I find this issue in i.MX8MP, which is the first SoC > > implements ECC for i.MX > > What about the mx7? > > > I think this patch should compatible with others which has ECC > > support, but I don't have one to have a test. > > What about the mx7? As I know only i.MX7D integrated in Flexcan without ECC, I am not quite understand what you meaning. Could you explain more? 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 |
On 9/25/20 11:09 AM, Joakim Zhang wrote: >>> "can: flexcan: initialize all flexcan memory for ECC function" for >>> this patch, I find this issue in i.MX8MP, which is the first SoC >>> implements ECC for i.MX >> >> What about the mx7? >>> I think this patch should compatible with others which has ECC >>> support, but I don't have one to have a test. >> >> What about the mx7? > > As I know only i.MX7D integrated in Flexcan without ECC, I am not quite understand what you meaning. > Could you explain more? Doh! Looking at the table in driver corrects me and says the VF610 has ECC (not the imx7). regards, Marc
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(®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), > }, { > ... > }, > }; 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
On 9/27/20 10:01 AM, Joakim Zhang wrote: > [...] >> 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? One option is not to make it a global variable, but to move it into the function, then you have the reg pointer available. > 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? If it's OK on all SoCs to initialize the complete RAM area, just do it. Then we can get rid of the proposed struct at all. > "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." Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月28日 3:56 > 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/27/20 10:01 AM, Joakim Zhang wrote: > > [...] > >> 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? > > One option is not to make it a global variable, but to move it into the function, > then you have the reg pointer available. Will take into account if later we also need implement this struct. > > 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? > > If it's OK on all SoCs to initialize the complete RAM area, just do it. Then we can > get rid of the proposed struct at all. Should be OK according to IP guys feedbacks. I am checking layerscape's CAN section: There is no ECC section in LS1021A https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/layerscape-multicore-processors/layerscape-1021a-dual-core-communications-processor-with-lcd-controller:LS1021A?tab=Documentation_Tab ECC section in LX2160A, also contains the same NOTE as i.MX8MP. https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/layerscape-multicore-processors/layerscape-lx2160a-multicore-communications-processor:LX2160A?tab=Documentation_Tab Hi @Pankaj Bansal, could you please also have a check? Best Regards, Joakim Zhang > > "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." > > 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 |
On 9/28/20 4:27 AM, Joakim Zhang wrote: >> If it's OK on all SoCs to initialize the complete RAM area, just do it. Then we can >> get rid of the proposed struct at all. > > Should be OK according to IP guys feedbacks. Good! > 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 | > FLEXCAN_QUIRK_BROKEN_PERR_STATE, > }; > > static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > }; > > static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD, > }; > I am checking layerscape's CAN section: > > There is no ECC section in LS1021A > https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/layerscape-multicore-processors/layerscape-1021a-dual-core-communications-processor-with-lcd-controller:LS1021A?tab=Documentation_Tab Hmmm, why does the LS1021A have "FLEXCAN_QUIRK_DISABLE_MECR"? The bits in the ctrl2 and the mecr register itself used in the quirk are marked as reserved in this datasheet.... Can @Pankaj Bansal clarify this? > ECC section in LX2160A, also contains the same NOTE as i.MX8MP. > https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/layerscape-multicore-processors/layerscape-lx2160a-multicore-communications-processor:LX2160A?tab=Documentation_Tab > Hi @Pankaj Bansal, could you please also have a check? Can someone check the vf610, too? regards, Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月28日 15:01 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org; > Pankaj Bansal <pankaj.bansal@nxp.com> > 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/28/20 4:27 AM, Joakim Zhang wrote: > >> If it's OK on all SoCs to initialize the complete RAM area, just do > >> it. Then we can get rid of the proposed struct at all. > > > > Should be OK according to IP guys feedbacks. > > Good! > > > 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 | > > FLEXCAN_QUIRK_BROKEN_PERR_STATE, > > }; > > > > static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > > }; > > > > static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_SUPPORT_FD, }; > > > I am checking layerscape's CAN section: > > > > There is no ECC section in LS1021A > > https://www.nxp.com/products/processors-and-microcontrollers/arm-proce > > ssors/layerscape-multicore-processors/layerscape-1021a-dual-core-commu > > nications-processor-with-lcd-controller:LS1021A?tab=Documentation_Tab > > Hmmm, why does the LS1021A have "FLEXCAN_QUIRK_DISABLE_MECR"? The > bits in the > ctrl2 and the mecr register itself used in the quirk are marked as reserved in > this datasheet.... > > Can @Pankaj Bansal clarify this? > > > ECC section in LX2160A, also contains the same NOTE as i.MX8MP. > > https://www.nxp.com/products/processors-and-microcontrollers/arm-proce > > ssors/layerscape-multicore-processors/layerscape-lx2160a-multicore-com > > munications-processor:LX2160A?tab=Documentation_Tab > > > Hi @Pankaj Bansal, could you please also have a check? > Can someone check the vf610, too? I check the VF610 RM just now, indeed it has ECC feature, there is also a NOTE in "12.1.4.13 Detection and Correction of Memory Errors" section: All FlexCAN memory must be initialized before starting its operation in order to have the parity bits in memory properly updated. The WRMFRZ bit in Control 2 Register (CTRL2) grants write access to all memory positions from 0x080 to 0xADF. Best Regards, Joakim Zhang > 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 |
On 9/28/20 9:58 AM, Joakim Zhang wrote: >> Can someone check the vf610, too? > > I check the VF610 RM just now, indeed it has ECC feature, there is also a NOTE in "12.1.4.13 Detection and Correction of Memory Errors" section: > > All FlexCAN memory must be initialized before starting its > operation in order to have the parity bits in memory properly > updated. The WRMFRZ bit in Control 2 Register (CTRL2) > grants write access to all memory positions from 0x080 to > 0xADF. Sounds good. Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月28日 16:21 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org; > Pankaj Bansal <pankaj.bansal@nxp.com> > 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/28/20 9:58 AM, Joakim Zhang wrote: > >> Can someone check the vf610, too? > > > > I check the VF610 RM just now, indeed it has ECC feature, there is also a > NOTE in "12.1.4.13 Detection and Correction of Memory Errors" section: > > > > All FlexCAN memory must be initialized before starting its operation > > in order to have the parity bits in memory properly updated. The > > WRMFRZ bit in Control 2 Register (CTRL2) grants write access to all > > memory positions from 0x080 to 0xADF. > > Sounds good. Could I send out a V3 to review firstly, then wait Pankaj have time to do the test? 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 |
On 9/28/20 10:36 AM, Joakim Zhang wrote: > Could I send out a V3 to review firstly, then wait Pankaj have time to do the > test? sure, go ahead. Marc
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. + */ + 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); + + /* 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 */ + 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
There is a NOTE at the section "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), 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(-)