Hi Stefan, Thanks for your validation! Could you add your test tag if you can successfully validated? Comments in-lined about your points. Best Regards, Joakim Zhang > -----Original Message----- > From: Stefan-gabriel Mirea > Sent: 2019年4月9日 1:46 > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; linux-can@xxxxxxxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH V2 2/5] can: flexcan: add CAN FD mode support > > Hello Joakim, > > I successfully validated your CAN FD support on an S32V234 based board. > Here are some points which are not very clear to me: > > Joakim Zhang <qiangqing.zhang@xxxxxxx> writes: > > + priv->write(reg_mcr | FLEXCAN_MCR_FDEN, ®s->mcr); > > According to the S32V234 reference manual (not sure if this applies to > i.MX8QM as well), CAN_CTRL1[SMP] cannot be asserted when CAN FD is > enabled. Is it safe to configure the interface with both "fd on" and > "triple-sampling on"? If not, flexcan_open should return an error when both > CAN_CTRLMODE_FD and CAN_CTRLMODE_3_SAMPLES are set in ctrlmode. [Joakim Zhang] Yes, you are right. I checked in i.MX8QM reference manual, CAN_CTRLMODE_FD and CAN_CTRLMODE_3_SAMPLES should not set together. I will fix it in V3. > Joakim Zhang <qiangqing.zhang@xxxxxxx> writes: > > + priv->offload.is_canfd = true; > > Shouldn't is_canfd be assigned false in the "else" branch? Otherwise, is_canfd > will stay true after the following sequence: > > root@s32v234evb:~# ip link set can0 type can bitrate 500000 dbitrate > 2000000 fd on > root@s32v234evb:~# ip link set can0 up > root@s32v234evb:~# ip link set can0 down root@s32v234evb:~# ip link set > can0 type can bitrate 500000 fd off root@s32v234evb:~# ip link set can0 up [Joakim Zhang] Good catch! I will fix it in V3. > Moreover, I wonder if we even need is_canfd, because frames should be fed to > the kernel as classic or FD based only on the EDL bit in the C/S word of the Rx > MB, to keep their original format. That is, you may either read EDL from within > a new callback (but I think that mailbox_read should remain the only one > which both locks and unlocks the > mailboxes) or let mailbox_read call alloc_can(fd)_skb itself. [Joakim Zhang] We added can fd support in 4.9 kernel which let mailbox_read call alloc_can(fd)_skb in the past. Now the driver allocate skb before mailbox_read in rx_offload and also read the overflow frames. I add the "is_canfd" since I don't want to change the rx_offload framework too much. @mkl@xxxxxxxxxxxxxx, could you give some advice, which solution is better? > Regards, > Stefan