[PATCH V2 0/5] can: flexcan: add CAN FD support on i.MX8QM

[PATCH V2 0/5] can: flexcan: add CAN FD support on i.MX8QM

[Joakim Zhang]

This patch set intends to add fd mode support on i.MX8QM platform.

Dong Aisheng (4):
  can: rx-offload: add CANFD support based on offload
  can: flexcan: add CAN FD mode support
  can: flexcan: add CANFD BRS support and improve bittiming setting
  can: flexcan: add imx8qm support

Joakim Zhang (1):
  can: flexcan: add ISO CAN FD feature support

 drivers/net/can/flexcan.c      | 222 ++++++++++++++++++++++++++++-----
 drivers/net/can/rx-offload.c   |  16 ++-
 include/linux/can/rx-offload.h |   4 +-
 3 files changed, 206 insertions(+), 36 deletions(-)

-- 
2.17.1

[PATCH V2 1/5] can: rx-offload: add CANFD support based on offload

[Joakim Zhang]

From: Dong Aisheng <aisheng.dong@nxp.com>

Using struct canfd_frame instead of can_frame to add support for CAN FD
mode in offload. FlexCAN controller will set the is_canfd variable when
it sets CAN FD mode.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

ChangeLog:
----------
V1->V2:
	*None
---
 drivers/net/can/rx-offload.c   | 16 ++++++++++------
 include/linux/can/rx-offload.h |  4 +++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/rx-offload.c b/drivers/net/can/rx-offload.c
index 2ce4fa8698c7..131fe600deb3 100644
--- a/drivers/net/can/rx-offload.c
+++ b/drivers/net/can/rx-offload.c
@@ -55,11 +55,11 @@ static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
 
 	while ((work_done < quota) &&
 	       (skb = skb_dequeue(&offload->skb_queue))) {
-		struct can_frame *cf = (struct can_frame *)skb->data;
+		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 
 		work_done++;
 		stats->rx_packets++;
-		stats->rx_bytes += cf->can_dlc;
+		stats->rx_bytes += cf->len;
 		netif_receive_skb(skb);
 	}
 
@@ -122,16 +122,20 @@ static struct sk_buff *can_rx_offload_offload_one(struct can_rx_offload *offload
 {
 	struct sk_buff *skb = NULL;
 	struct can_rx_offload_cb *cb;
-	struct can_frame *cf;
+	struct canfd_frame *cf;
 	int ret;
 
 	/* If queue is full or skb not available, read to discard mailbox */
 	if (likely(skb_queue_len(&offload->skb_queue) <=
-		   offload->skb_queue_len_max))
-		skb = alloc_can_skb(offload->dev, &cf);
+		   offload->skb_queue_len_max)) {
+		if (offload->is_canfd)
+			skb = alloc_canfd_skb(offload->dev, &cf);
+		else
+			skb = alloc_can_skb(offload->dev, (struct can_frame **)&cf);
+	}
 
 	if (!skb) {
-		struct can_frame cf_overflow;
+		struct canfd_frame cf_overflow;
 		u32 timestamp;
 
 		ret = offload->mailbox_read(offload, &cf_overflow,
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index 8268811a697e..6448e7dfc170 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -23,7 +23,7 @@
 struct can_rx_offload {
 	struct net_device *dev;
 
-	unsigned int (*mailbox_read)(struct can_rx_offload *offload, struct can_frame *cf,
+	unsigned int (*mailbox_read)(struct can_rx_offload *offload, struct canfd_frame *cf,
 				     u32 *timestamp, unsigned int mb);
 
 	struct sk_buff_head skb_queue;
@@ -35,6 +35,8 @@ struct can_rx_offload {
 	struct napi_struct napi;
 
 	bool inc;
+
+	bool is_canfd;
 };
 
 int can_rx_offload_add_timestamp(struct net_device *dev, struct can_rx_offload *offload);
-- 
2.17.1

[PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Joakim Zhang]

From: Dong Aisheng <aisheng.dong@nxp.com>

This patch intends to add CAN FD mode support in driver, it means that
payload size can extend up to 64 bytes.

NOTE: Bit rate switch (BRS) enabled by system reset when it enables CAN
FD mode (explicitly set BRS again in driver). So CAN hardware has support
BRS, but now driver has not support it due to bit timing always set in CBT
register other than CTRL1 register. It will add in next patch.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

ChangeLog:
----------
V1->V2:
	*move "priv->offload.is_canfd = true" from _probe() to _start()
---
 drivers/net/can/flexcan.c | 110 ++++++++++++++++++++++++++++++++++----
 1 file changed, 100 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e35083ff31ee..f28a4c3e8087 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -52,6 +52,7 @@
 #define FLEXCAN_MCR_IRMQ		BIT(16)
 #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
 #define FLEXCAN_MCR_AEN			BIT(12)
+#define FLEXCAN_MCR_FDEN		BIT(11)
 /* MCR_MAXMB: maximum used MBs is MAXMB + 1 */
 #define FLEXCAN_MCR_MAXMB(x)		((x) & 0x7f)
 #define FLEXCAN_MCR_IDAM_A		(0x0 << 8)
@@ -137,6 +138,20 @@
 	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
 	 FLEXCAN_ESR_WAK_INT)
 
+/* FLEXCAN FD control register (FDCTRL) bits */
+#define FLEXCAN_FDCTRL_FDRATE		BIT(31)
+#define FLEXCAN_FDCTRL_MBDSR3(x)	(((x) & 0x3) << 25)
+#define FLEXCAN_FDCTRL_MBDSR2(x)	(((x) & 0x3) << 22)
+#define FLEXCAN_FDCTRL_MBDSR1(x)	(((x) & 0x3) << 19)
+#define FLEXCAN_FDCTRL_MBDSR0(x)	(((x) & 0x3) << 16)
+
+/* FLEXCAN FD Bit Timing register (FDCBT) bits */
+#define FLEXCAN_FDCBT_FPRESDIV(x)	(((x) & 0x3ff) << 20)
+#define FLEXCAN_FDCBT_FRJW(x)		(((x) & 0x07) << 16)
+#define FLEXCAN_FDCBT_FPROPSEG(x)	(((x) & 0x1f) << 10)
+#define FLEXCAN_FDCBT_FPSEG1(x)		(((x) & 0x07) << 5)
+#define FLEXCAN_FDCBT_FPSEG2(x)		((x) & 0x07)
+
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
 #define FLEXCAN_TX_MB_RESERVED_OFF_FIFO		8
@@ -148,6 +163,10 @@
 #define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE	BIT(5)
 
 /* FLEXCAN message buffers */
+#define FLEXCAN_MB_CNT_EDL		BIT(31)
+#define FLEXCAN_MB_CNT_BRS		BIT(30)
+#define FLEXCAN_MB_CNT_ESI		BIT(29)
+
 #define FLEXCAN_MB_CODE_MASK		(0xf << 24)
 #define FLEXCAN_MB_CODE_RX_BUSY_BIT	(0x1 << 24)
 #define FLEXCAN_MB_CODE_RX_INACTIVE	(0x0 << 24)
@@ -192,6 +211,7 @@
 #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for error passive */
 #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE register access */
 #define FLEXCAN_QUIRK_SETUP_STOP_MODE		BIT(8) /* Setup stop mode to support wakeup */
+#define FLEXCAN_QUIRK_TIMESTAMP_SUPPORT_FD	BIT(9) /* Use timestamp then support can fd mode */
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -250,6 +270,9 @@ struct flexcan_regs {
 	u32 rerrdr;		/* 0xaf4 */
 	u32 rerrsynr;		/* 0xaf8 */
 	u32 errsr;		/* 0xafc */
+	u32 _reserved7[64];     /* 0xb00 */
+	u32 fdctrl;             /* 0xc00 */
+	u32 fdcbt;              /* 0xc04 */
 };
 
 struct flexcan_devtype_data {
@@ -337,6 +360,18 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
 	.brp_inc = 1,
 };
 
+static const struct can_bittiming_const flexcan_fd_data_bittiming_const = {
+	.name = DRV_NAME,
+	.tseg1_min = 1,
+	.tseg1_max = 39,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 8,
+	.brp_min = 1,
+	.brp_max = 1024,
+	.brp_inc = 1,
+};
+
 /* FlexCAN module is essentially modelled as a little-endian IP in most
  * SoCs, i.e the registers as well as the message buffer areas are
  * implemented in a little-endian fashion.
@@ -609,10 +644,10 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 static netdev_tx_t flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
-	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 	u32 can_id;
 	u32 data;
-	u32 ctrl = FLEXCAN_MB_CODE_TX_DATA | (cf->can_dlc << 16);
+	u32 ctrl = FLEXCAN_MB_CODE_TX_DATA | ((can_len2dlc(cf->len)) << 16);
 	int i;
 
 	if (can_dropped_invalid_skb(dev, skb))
@@ -630,7 +665,10 @@ static netdev_tx_t flexcan_start_xmit(struct sk_buff *skb, struct net_device *de
 	if (cf->can_id & CAN_RTR_FLAG)
 		ctrl |= FLEXCAN_MB_CNT_RTR;
 
-	for (i = 0; i < cf->can_dlc; i += sizeof(u32)) {
+	if (can_is_canfd_skb(skb))
+		ctrl |= FLEXCAN_MB_CNT_EDL;
+
+	for (i = 0; i < cf->len; i += sizeof(u32)) {
 		data = be32_to_cpup((__be32 *)&cf->data[i]);
 		priv->write(data, &priv->tx_mb->data[i / sizeof(u32)]);
 	}
@@ -790,7 +828,7 @@ static inline struct flexcan_priv *rx_offload_to_priv(struct can_rx_offload *off
 }
 
 static unsigned int flexcan_mailbox_read(struct can_rx_offload *offload,
-					 struct can_frame *cf,
+					 struct canfd_frame *cf,
 					 u32 *timestamp, unsigned int n)
 {
 	struct flexcan_priv *priv = rx_offload_to_priv(offload);
@@ -836,11 +874,21 @@ static unsigned int flexcan_mailbox_read(struct can_rx_offload *offload,
 	else
 		cf->can_id = (reg_id >> 18) & CAN_SFF_MASK;
 
-	if (reg_ctrl & FLEXCAN_MB_CNT_RTR)
-		cf->can_id |= CAN_RTR_FLAG;
-	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
+	if (reg_ctrl & FLEXCAN_MB_CNT_EDL) {
+		cf->len = can_dlc2len((reg_ctrl >> 16) & 0x0F);
+	} else {
+		cf->len = get_can_dlc((reg_ctrl >> 16) & 0x0F);
+
+		if (reg_ctrl & FLEXCAN_MB_CNT_RTR)
+			cf->can_id |= CAN_RTR_FLAG;
+	}
+
+	if (reg_ctrl & FLEXCAN_MB_CNT_ESI) {
+		cf->flags |= CANFD_ESI;
+		netdev_warn(priv->can.dev, "ESI Error\n");
+	}
 
-	for (i = 0; i < cf->can_dlc; i += sizeof(u32)) {
+	for (i = 0; i < cf->len; i += sizeof(u32)) {
 		__be32 data = cpu_to_be32(priv->read(&mb->data[i / sizeof(u32)]));
 		*(__be32 *)(cf->data + i) = data;
 	}
@@ -985,6 +1033,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
 	const struct can_bittiming *bt = &priv->can.bittiming;
+	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	struct flexcan_regs __iomem *regs = priv->regs;
 	u32 reg;
 
@@ -1014,6 +1063,15 @@ static void flexcan_set_bittiming(struct net_device *dev)
 	netdev_dbg(dev, "writing ctrl=0x%08x\n", reg);
 	priv->write(reg, &regs->ctrl);
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		reg = FLEXCAN_FDCBT_FPRESDIV(dbt->brp - 1) |
+			FLEXCAN_FDCBT_FPSEG1(dbt->phase_seg1 - 1) |
+			FLEXCAN_FDCBT_FPSEG2(dbt->phase_seg2 - 1) |
+			FLEXCAN_FDCBT_FRJW(dbt->sjw - 1) |
+			FLEXCAN_FDCBT_FPROPSEG(dbt->prop_seg);
+		priv->write(reg, &regs->fdcbt);
+	}
+
 	/* print chip status */
 	netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
 		   priv->read(&regs->mcr), priv->read(&regs->ctrl));
@@ -1028,7 +1086,7 @@ static int flexcan_chip_start(struct net_device *dev)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->regs;
-	u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr;
+	u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr, reg_fdctrl;
 	u64 reg_imask;
 	int err, i;
 	struct flexcan_mb __iomem *mb;
@@ -1125,6 +1183,24 @@ static int flexcan_chip_start(struct net_device *dev)
 	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
 	priv->write(reg_ctrl, &regs->ctrl);
 
+	/* FDCTRL
+	 *
+	 * support BRS when set CAN FD mode
+	 * 64 bytes payload per MB and 7 MBs per RAM block by default
+	 * enable CAN FD mode
+	 */
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		reg_fdctrl = priv->read(&regs->fdctrl);
+		reg_fdctrl |= FLEXCAN_FDCTRL_FDRATE;
+		reg_fdctrl |= FLEXCAN_FDCTRL_MBDSR3(3) | FLEXCAN_FDCTRL_MBDSR2(3) |
+				FLEXCAN_FDCTRL_MBDSR1(3) | FLEXCAN_FDCTRL_MBDSR0(3);
+		priv->write(reg_fdctrl, &regs->fdctrl);
+		reg_mcr = priv->read(&regs->mcr);
+		priv->write(reg_mcr | FLEXCAN_MCR_FDEN, &regs->mcr);
+
+		priv->offload.is_canfd = true;
+	}
+
 	if ((priv->devtype_data->quirks & FLEXCAN_QUIRK_ENABLE_EACEN_RRS)) {
 		reg_ctrl2 = priv->read(&regs->ctrl2);
 		reg_ctrl2 |= FLEXCAN_CTRL2_EACEN | FLEXCAN_CTRL2_RRS;
@@ -1261,7 +1337,10 @@ static int flexcan_open(struct net_device *dev)
 	if (err)
 		goto out_close;
 
-	priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
+		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
+	else
+		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
 	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
 			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
 
@@ -1600,6 +1679,17 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->clk_src = clk_src;
 	priv->devtype_data = devtype_data;
 	priv->reg_xceiver = reg_xceiver;
+	priv->offload.is_canfd = false;
+
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_TIMESTAMP_SUPPORT_FD) {
+		if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
+			priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
+			priv->can.data_bittiming_const = &flexcan_fd_data_bittiming_const;
+		} else {
+			dev_err(&pdev->dev, "canfd mode can't work on fifo mode\n");
+			err = -EINVAL;
+		}
+	}
 
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
-- 
2.17.1

Re: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Stefan-gabriel Mirea]

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@nxp.com> writes:
> +		priv->write(reg_mcr | FLEXCAN_MCR_FDEN, &regs->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 <qiangqing.zhang@nxp.com> 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

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.

Regards,
Stefan

RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Joakim Zhang]

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@nxp.com>; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@nxp.com> writes:
> > +		priv->write(reg_mcr | FLEXCAN_MCR_FDEN, &regs->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@nxp.com> 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@pengutronix.de, could you give some advice, which solution is better?
              
> Regards,
> Stefan

RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Stefan-gabriel Mirea]

> From: Joakim Zhang
> Sent: Tuesday, April 9, 2019 5:07 AM
> Hi Stefan,
> 
> Thanks for your validation! Could you add your test tag if you can 
> successfully validated?

Sure, no problem. Please note that I needed to replace "flexcan_read" and "flexcan_write" with "priv->read" and "priv->write" respectively, in PATCH V2 4/5.

>  [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@pengutronix.de, could you give 
> some advice, which solution is better?

This is more of a functionality issue. For example, candump from canutils 4.0.6 never shows CAN FD frames, but should still be able to show the CAN 2.0 ones regardless of whether "fd on" was supplied. I suggest the following separation:

can_rx_offload_offload_one:
	struct sk_buff *skb = NULL;
	...
	bool drop = unlikely(skb_queue_len(&offload->skb_queue) >
			     offload->skb_queue_len_max);

	if (offload->mailbox_read(offload, drop, &skb, &timestamp, n) && !skb)
		offload->dev->stats.rx_dropped++;

	if (skb) {
		struct can_rx_offload_cb *cb = can_rx_offload_get_cb(skb);

		cb->timestamp = timestamp;
	}

	return skb;

flexcan_mailbox_read:
	...
	if (!drop) {
		if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
			*skb = alloc_canfd_skb(offload->dev, &cf);
		else
			*skb = alloc_can_skb(offload->dev,
					     (struct can_frame **)&cf);
	}
	if (*skb) {
		/* use cf */
		...
	}
	/* mark as read */

Although not visible in the FlexCAN case, the socket buffers would be also freed from inside mailbox_read if errors were encountered after allocation.

Regards,
Stefan

RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Joakim Zhang]

> -----Original Message-----
> From: Stefan-gabriel Mirea
> Sent: 2019年4月9日 15:05
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Marc Kleine-Budde
> <mkl@pengutronix.de>
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support
> 
> > From: Joakim Zhang
> > Sent: Tuesday, April 9, 2019 5:07 AM
> > Hi Stefan,
> >
> > Thanks for your validation! Could you add your test tag if you can
> > successfully validated?
> 
> Sure, no problem. Please note that I needed to replace "flexcan_read" and
> "flexcan_write" with "priv->read" and "priv->write" respectively, in PATCH V2
> 4/5.

  Hi Stefan,

  I made a mistake here, thank you for pointing out. I will fix it in V3.

> >  [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@pengutronix.de, could you give
> > some advice, which solution is better?
> 
> This is more of a functionality issue. For example, candump from canutils 4.0.6
> never shows CAN FD frames, but should still be able to show the CAN 2.0 ones
> regardless of whether "fd on" was supplied. I suggest the following separation:
> 
> can_rx_offload_offload_one:
> 	struct sk_buff *skb = NULL;
> 	...
> 	bool drop = unlikely(skb_queue_len(&offload->skb_queue) >
> 			     offload->skb_queue_len_max);
> 
> 	if (offload->mailbox_read(offload, drop, &skb, &timestamp, n) && !skb)
> 		offload->dev->stats.rx_dropped++;
> 
> 	if (skb) {
> 		struct can_rx_offload_cb *cb = can_rx_offload_get_cb(skb);
> 
> 		cb->timestamp = timestamp;
> 	}
> 
> 	return skb;
> 
> flexcan_mailbox_read:
> 	...
> 	if (!drop) {
> 		if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
> 			*skb = alloc_canfd_skb(offload->dev, &cf);
> 		else
> 			*skb = alloc_can_skb(offload->dev,
> 					     (struct can_frame **)&cf);
> 	}
> 	if (*skb) {
> 		/* use cf */
> 		...
> 	}
> 	/* mark as read */
> 
> Although not visible in the FlexCAN case, the socket buffers would be also
> freed from inside mailbox_read if errors were encountered after allocation.
  
 This is a good solution, I will add it in V3. Thank you very much.

Best Regards,
Joakim Zhang

> Regards,
> Stefan

[PATCH V2 3/5] can: flexcan: add CANFD BRS support and improve bittiming setting

[Joakim Zhang]

From: Dong Aisheng <aisheng.dong@nxp.com>

This patch intends to add CANFD BitRate Switch(BRS) support. Bit timing
always set in CBT register other than CTRL1 register when CANFD
supports BRS, it will extend the range of all CAN bit timing variables
(PRESDIV, PROPSEG, PSEG1, PSEG2 and RJW), which will improve the bit
timing accuracy.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

ChangeLog:
----------
V1->V2:
	*both normal mode and fd mode use CBT register to set bittimg on
	i.MX8 platform.
---
 drivers/net/can/flexcan.c | 116 +++++++++++++++++++++++++++++---------
 1 file changed, 88 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f28a4c3e8087..63363f1b85d1 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -138,6 +138,14 @@
 	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
 	 FLEXCAN_ESR_WAK_INT)
 
+/* FLEXCAN Bit Timing register (CBT) bits */
+#define FLEXCAN_CBT_BTF		        BIT(31)
+#define FLEXCAN_CBT_EPRESDIV(x)	        (((x) & 0x3ff) << 21)
+#define FLEXCAN_CBT_ERJW(x)		(((x) & 0x1f) << 16)
+#define FLEXCAN_CBT_EPROPSEG(x)	        (((x) & 0x3f) << 10)
+#define FLEXCAN_CBT_EPSEG1(x)		(((x) & 0x1f) << 5)
+#define FLEXCAN_CBT_EPSEG2(x)		((x) & 0x1f)
+
 /* FLEXCAN FD control register (FDCTRL) bits */
 #define FLEXCAN_FDCTRL_FDRATE		BIT(31)
 #define FLEXCAN_FDCTRL_MBDSR3(x)	(((x) & 0x3) << 25)
@@ -245,7 +253,8 @@ struct flexcan_regs {
 	u32 crcr;		/* 0x44 */
 	u32 rxfgmask;		/* 0x48 */
 	u32 rxfir;		/* 0x4c */
-	u32 _reserved3[12];	/* 0x50 */
+	u32 cbt;                /* 0x50 */
+	u32 _reserved3[11];     /* 0x54 */
 	u8 mb[2][512];		/* 0x80 */
 	/* FIFO-mode:
 	 *			MB
@@ -360,6 +369,18 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
 	.brp_inc = 1,
 };
 
+static const struct can_bittiming_const flexcan_fd_bittiming_const = {
+	.name = DRV_NAME,
+	.tseg1_min = 2,
+	.tseg1_max = 64,
+	.tseg2_min = 1,
+	.tseg2_max = 32,
+	.sjw_max = 32,
+	.brp_min = 1,
+	.brp_max = 1024,
+	.brp_inc = 1,
+};
+
 static const struct can_bittiming_const flexcan_fd_data_bittiming_const = {
 	.name = DRV_NAME,
 	.tseg1_min = 1,
@@ -665,9 +686,13 @@ static netdev_tx_t flexcan_start_xmit(struct sk_buff *skb, struct net_device *de
 	if (cf->can_id & CAN_RTR_FLAG)
 		ctrl |= FLEXCAN_MB_CNT_RTR;
 
-	if (can_is_canfd_skb(skb))
+	if (can_is_canfd_skb(skb)) {
 		ctrl |= FLEXCAN_MB_CNT_EDL;
 
+		if (cf->flags & CANFD_BRS)
+			ctrl |= FLEXCAN_MB_CNT_BRS;
+	}
+
 	for (i = 0; i < cf->len; i += sizeof(u32)) {
 		data = be32_to_cpup((__be32 *)&cf->data[i]);
 		priv->write(data, &priv->tx_mb->data[i / sizeof(u32)]);
@@ -876,6 +901,9 @@ static unsigned int flexcan_mailbox_read(struct can_rx_offload *offload,
 
 	if (reg_ctrl & FLEXCAN_MB_CNT_EDL) {
 		cf->len = can_dlc2len((reg_ctrl >> 16) & 0x0F);
+
+		if (reg_ctrl & FLEXCAN_MB_CNT_BRS)
+			cf->flags |= CANFD_BRS;
 	} else {
 		cf->len = get_can_dlc((reg_ctrl >> 16) & 0x0F);
 
@@ -1038,21 +1066,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
 	u32 reg;
 
 	reg = priv->read(&regs->ctrl);
-	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
-		 FLEXCAN_CTRL_RJW(0x3) |
-		 FLEXCAN_CTRL_PSEG1(0x7) |
-		 FLEXCAN_CTRL_PSEG2(0x7) |
-		 FLEXCAN_CTRL_PROPSEG(0x7) |
-		 FLEXCAN_CTRL_LPB |
-		 FLEXCAN_CTRL_SMP |
-		 FLEXCAN_CTRL_LOM);
-
-	reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) |
-		FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) |
-		FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) |
-		FLEXCAN_CTRL_RJW(bt->sjw - 1) |
-		FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1);
-
+	reg &= ~(FLEXCAN_CTRL_LPB | FLEXCAN_CTRL_SMP | FLEXCAN_CTRL_LOM);
 	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
 		reg |= FLEXCAN_CTRL_LPB;
 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
@@ -1063,18 +1077,63 @@ static void flexcan_set_bittiming(struct net_device *dev)
 	netdev_dbg(dev, "writing ctrl=0x%08x\n", reg);
 	priv->write(reg, &regs->ctrl);
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
-		reg = FLEXCAN_FDCBT_FPRESDIV(dbt->brp - 1) |
-			FLEXCAN_FDCBT_FPSEG1(dbt->phase_seg1 - 1) |
-			FLEXCAN_FDCBT_FPSEG2(dbt->phase_seg2 - 1) |
-			FLEXCAN_FDCBT_FRJW(dbt->sjw - 1) |
-			FLEXCAN_FDCBT_FPROPSEG(dbt->prop_seg);
-		priv->write(reg, &regs->fdcbt);
+	if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
+		reg = FLEXCAN_CBT_EPRESDIV(bt->brp - 1) |
+			FLEXCAN_CBT_EPSEG1(bt->phase_seg1 - 1) |
+			FLEXCAN_CBT_EPSEG2(bt->phase_seg2 - 1) |
+			FLEXCAN_CBT_ERJW(bt->sjw - 1) |
+			FLEXCAN_CBT_EPROPSEG(bt->prop_seg - 1) |
+			FLEXCAN_CBT_BTF;
+		priv->write(reg, &regs->cbt);
+
+		netdev_dbg(dev, "bt: prediv %d seg1 %d seg2 %d rjw %d propseg %d\n",
+			   bt->brp - 1, bt->phase_seg1 - 1, bt->phase_seg2 - 1,
+			   bt->sjw - 1, bt->prop_seg - 1);
+
+		if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+			reg = FLEXCAN_FDCBT_FPRESDIV(dbt->brp - 1) |
+				FLEXCAN_FDCBT_FPSEG1(dbt->phase_seg1 - 1) |
+				FLEXCAN_FDCBT_FPSEG2(dbt->phase_seg2 - 1) |
+				FLEXCAN_FDCBT_FRJW(dbt->sjw - 1) |
+				FLEXCAN_FDCBT_FPROPSEG(dbt->prop_seg);
+			priv->write(reg, &regs->fdcbt);
+
+			if (bt->brp != dbt->brp)
+				netdev_warn(dev, "PRESDIV not the same, may risk transfer errors\n");
+
+			netdev_dbg(dev, "fdbt: prediv %d seg1 %d seg2 %d rjw %d propseg %d\n",
+				   dbt->brp - 1, dbt->phase_seg1 - 1, dbt->phase_seg2 - 1,
+				   dbt->sjw - 1, dbt->prop_seg);
+
+			netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x cbt=0x%08x fdcbt=0x%08x\n",
+				   __func__, priv->read(&regs->mcr),
+				   priv->read(&regs->ctrl),
+				   priv->read(&regs->cbt),
+				   priv->read(&regs->fdcbt));
+		}
+	} else {
+		reg = priv->read(&regs->ctrl);
+		reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
+			 FLEXCAN_CTRL_RJW(0x3) |
+			 FLEXCAN_CTRL_PSEG1(0x7) |
+			 FLEXCAN_CTRL_PSEG2(0x7) |
+			 FLEXCAN_CTRL_PROPSEG(0x7));
+
+		reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) |
+			FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) |
+			FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) |
+			FLEXCAN_CTRL_RJW(bt->sjw - 1) |
+			FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1);
+		priv->write(reg, &regs->ctrl);
+
+		netdev_dbg(dev, "bt: prediv %d seg1 %d seg2 %d rjw %d propseg %d\n",
+			   bt->brp - 1, bt->phase_seg1 - 1, bt->phase_seg2 - 1,
+			   bt->sjw - 1, bt->prop_seg - 1);
+
+		/* print chip status */
+		netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
+			   priv->read(&regs->mcr), priv->read(&regs->ctrl));
 	}
-
-	/* print chip status */
-	netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
-		   priv->read(&regs->mcr), priv->read(&regs->ctrl));
 }
 
 /* flexcan_chip_start
@@ -1684,6 +1743,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_TIMESTAMP_SUPPORT_FD) {
 		if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
 			priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
+			priv->can.bittiming_const = &flexcan_fd_bittiming_const;
 			priv->can.data_bittiming_const = &flexcan_fd_data_bittiming_const;
 		} else {
 			dev_err(&pdev->dev, "canfd mode can't work on fifo mode\n");
-- 
2.17.1

[PATCH V2 4/5] can: flexcan: add ISO CAN FD feature support

[Joakim Zhang]

ISO CAN FD is introduced to increase the failture detection capability
than non-ISO CAN FD. The non-ISO CAN FD is still supported by FlexCAN so
that it can be used mainly during an intermediate phase, for evaluation
and development purposes.

Therefore, it is strongly recommended to configure FlexCAN to the ISO
CAN FD protocol by setting the ISOCANFDEN field in the CTRL2 register.

NOTE: if you only set "fd on", driver will use ISO FD MODE by default.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

ChangeLog:
----------
V1->V2:
	*None
---
 drivers/net/can/flexcan.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 63363f1b85d1..bcc935ebd105 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -92,6 +92,7 @@
 #define FLEXCAN_CTRL2_MRP		BIT(18)
 #define FLEXCAN_CTRL2_RRS		BIT(17)
 #define FLEXCAN_CTRL2_EACEN		BIT(16)
+#define FLEXCAN_CTRL2_ISOCANFDEN	BIT(12)
 
 /* FLEXCAN memory error control register (MECR) bits */
 #define FLEXCAN_MECR_ECRWRDIS		BIT(31)
@@ -1257,6 +1258,12 @@ static int flexcan_chip_start(struct net_device *dev)
 		reg_mcr = priv->read(&regs->mcr);
 		priv->write(reg_mcr | FLEXCAN_MCR_FDEN, &regs->mcr);
 
+		reg_ctrl2 = flexcan_read(&regs->ctrl2);
+		if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO))
+			flexcan_write(reg_ctrl2 | FLEXCAN_CTRL2_ISOCANFDEN, &regs->ctrl2);
+		else
+			flexcan_write(reg_ctrl2 & ~FLEXCAN_CTRL2_ISOCANFDEN, &regs->ctrl2);
+
 		priv->offload.is_canfd = true;
 	}
 
@@ -1742,7 +1749,7 @@ static int flexcan_probe(struct platform_device *pdev)
 
 	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_TIMESTAMP_SUPPORT_FD) {
 		if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
-			priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
+			priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO;
 			priv->can.bittiming_const = &flexcan_fd_bittiming_const;
 			priv->can.data_bittiming_const = &flexcan_fd_data_bittiming_const;
 		} else {
-- 
2.17.1

[PATCH V2 5/5] can: flexcan: add imx8qm support

[Joakim Zhang]

From: Dong Aisheng <aisheng.dong@nxp.com>

The Flexcan on i.MX8QM supports CAN FD protocol.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

ChangeLog:
----------
V1->V2:
	*None
---
 drivers/net/can/flexcan.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index bcc935ebd105..064bdb960cb7 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -346,6 +346,12 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
 		FLEXCAN_QUIRK_SETUP_STOP_MODE,
 };
 
+static struct flexcan_devtype_data fsl_imx8qm_devtype_data = {
+	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
+		FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
+		FLEXCAN_QUIRK_TIMESTAMP_SUPPORT_FD,
+};
+
 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 |
@@ -1630,6 +1636,7 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 }
 
 static const struct of_device_id flexcan_of_match[] = {
+	{ .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, },
 	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
 	{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
 	{ .compatible = "fsl,imx53-flexcan", .data = &fsl_imx25_devtype_data, },
-- 
2.17.1

RE: [PATCH V2 0/5] can: flexcan: add CAN FD support on i.MX8QM

[Joakim Zhang]

Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年3月19日 13:17
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; wg@grandegger.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Joakim Zhang
> <qiangqing.zhang@nxp.com>
> Subject: [PATCH V2 0/5] can: flexcan: add CAN FD support on i.MX8QM
> 
> This patch set intends to add fd mode support on i.MX8QM platform.
> 
> Dong Aisheng (4):
>   can: rx-offload: add CANFD support based on offload
>   can: flexcan: add CAN FD mode support
>   can: flexcan: add CANFD BRS support and improve bittiming setting
>   can: flexcan: add imx8qm support
> 
> Joakim Zhang (1):
>   can: flexcan: add ISO CAN FD feature support
> 
>  drivers/net/can/flexcan.c      | 222 ++++++++++++++++++++++++++++-----
>  drivers/net/can/rx-offload.c   |  16 ++-
>  include/linux/can/rx-offload.h |   4 +-
>  3 files changed, 206 insertions(+), 36 deletions(-)
> 
> --
> 2.17.1