| Message ID | 20200929203041.29758-1-qiangqing.zhang@nxp.com |
|---|---|
| Headers | show |
| Series | patch set for flexcan | expand |
On 9/29/20 2:34 PM, Marc Kleine-Budde wrote: > On 9/29/20 10:30 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. Platform has >> ECC hardware support, but select this quirk, this issue may not come to >> light. 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. >> >> In this patch, create FLEXCAN_QUIRK_SUPPORT_ECC for platforms which has ECC >> feature. If you have a ECC platform in your hand, please select this >> qurik to initialize all flexcan memory firstly, then you can select >> FLEXCAN_QUIRK_DISABLE_MECR to only enable correctable errors. >> >> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> >> --- >> ChangeLogs: >> V1->V2: >> * update commit messages, add a datasheet reference. >> * initialize block memory instead of trivial memory. >> * inilialize reserved memory. >> V2->V3: >> * add FLEXCAN_QUIRK_SUPPORT_ECC quirk. >> * remove init_ram struct. >> V3->V4: >> * move register definition into flexcan_reg. >> --- >> drivers/net/can/flexcan.c | 51 +++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c >> index e86925134009..ede25db42e87 100644 >> --- a/drivers/net/can/flexcan.c >> +++ b/drivers/net/can/flexcan.c >> @@ -239,6 +239,8 @@ >> #define FLEXCAN_QUIRK_SETUP_STOP_MODE BIT(8) >> /* Support CAN-FD mode */ >> #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9) >> +/* support memory detection and correction */ >> +#define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10) >> >> /* Structure of the message buffer */ >> struct flexcan_mb { >> @@ -292,7 +294,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 +316,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 +1307,35 @@ 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; >> + >> + /* 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(®s->ctrl2); >> + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; >> + priv->write(reg_ctrl2, ®s->ctrl2); >> + >> + memset_io(®s->mb[0][0], 0, >> + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); > > why the cast? Doh. Of cource you need to case, but you should better cast both. :) ... or use offsetof() Marc
On 9/29/20 2:38 PM, Joakim Zhang wrote: >>> 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; >>> + >>> + /* 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(®s->ctrl2); >>> + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; >>> + priv->write(reg_ctrl2, ®s->ctrl2); >>> + >>> + memset_io(®s->mb[0][0], 0, >>> + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); >> >> why the cast? > > Due to mb is defined as a u8. And the count of memset_io is bytes. right. this is why you don't need the 2nd cast. Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年9月29日 20:46 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org > Cc: netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 1/3] can: flexcan: initialize all flexcan memory for ECC > function > > On 9/29/20 2:38 PM, Joakim Zhang wrote: > >>> 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; > >>> + > >>> + /* 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(®s->ctrl2); > >>> + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; > >>> + priv->write(reg_ctrl2, ®s->ctrl2); > >>> + > >>> + memset_io(®s->mb[0][0], 0, > >>> + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); > >> > >> why the cast? > > > > Due to mb is defined as a u8. And the count of memset_io is bytes. > > right. this is why you don't need the 2nd cast. Yes, do you need me send a patch to improve it with offsetof()? 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/29/20 2:50 PM, Joakim Zhang wrote: > >> -----Original Message----- >> From: Marc Kleine-Budde <mkl@pengutronix.de> >> Sent: 2020年9月29日 20:46 >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org >> Cc: netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> >> Subject: Re: [PATCH V4 1/3] can: flexcan: initialize all flexcan memory for ECC >> function >> >> On 9/29/20 2:38 PM, Joakim Zhang wrote: >>>>> 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; >>>>> + >>>>> + /* 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(®s->ctrl2); >>>>> + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; >>>>> + priv->write(reg_ctrl2, ®s->ctrl2); >>>>> + >>>>> + memset_io(®s->mb[0][0], 0, >>>>> + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); >>>> >>>> why the cast? >>> >>> Due to mb is defined as a u8. And the count of memset_io is bytes. >> >> right. this is why you don't need the 2nd cast. > > Yes, do you need me send a patch to improve it with offsetof()? If it looks better :), then yes :) Marc
On Wed, 30 Sep 2020 04:30:39 +0800 Joakim Zhang wrote: > @@ -1292,6 +1307,35 @@ 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; > + > + /* 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(®s->ctrl2); > + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; > + priv->write(reg_ctrl2, ®s->ctrl2); > + > + memset_io(®s->mb[0][0], 0, > + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); > + > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > + memset_io(®s->tx_smb_fd[0], 0, > + (u8 *)®s->rx_smb1_fd[17] - (u8 *)®s->tx_smb_fd[0] + 0x4); > + > + reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ; > + priv->write(reg_ctrl2, ®s->ctrl2); > +} > + > /* flexcan_chip_start > * > * this functions is entered with clocks enabled drivers/net/can/flexcan.c:1329:20: warning: cast removes address space '__iomem' of expression drivers/net/can/flexcan.c:1329:43: error: subtraction of different types can't work (different address spaces)
On 9/29/20 4:48 PM, Jakub Kicinski wrote: > On Wed, 30 Sep 2020 04:30:39 +0800 Joakim Zhang wrote: >> @@ -1292,6 +1307,35 @@ 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; >> + >> + /* 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(®s->ctrl2); >> + reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ; >> + priv->write(reg_ctrl2, ®s->ctrl2); >> + >> + memset_io(®s->mb[0][0], 0, >> + (u8 *)®s->rx_smb1[3] - ®s->mb[0][0] + 0x4); >> + >> + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) >> + memset_io(®s->tx_smb_fd[0], 0, >> + (u8 *)®s->rx_smb1_fd[17] - (u8 *)®s->tx_smb_fd[0] + 0x4); >> + >> + reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ; >> + priv->write(reg_ctrl2, ®s->ctrl2); >> +} >> + >> /* flexcan_chip_start >> * >> * this functions is entered with clocks enabled > drivers/net/can/flexcan.c:1329:20: warning: cast removes address space '__iomem' of expression > drivers/net/can/flexcan.c:1329:43: error: subtraction of different types can't work (different address spaces) Thanks, this patch is outdated, we use offset of now to calculate the range: > memset_io(®s->mb[0][0], 0, > offsetof(struct flexcan_regs, rx_smb1[3]) - > offsetof(struct flexcan_regs, mb[0][0]) + 0x4); Marc