[PATCH 0/3] Decrease likelyhood of RX overruns

[PATCH 0/3] Decrease likelyhood of RX overruns

[David Jander]

This patch series actually fixes 3 related issues, greatly improving
resilence under high latency conditions (e.g. non-real-time kernels).
This reduces the likelyhood of receive underruns when many back-to-back
messages are received at high bitrates.

The first patch is actually a fix to a bug that causes overruns being missed.

The second patch swaps the use of the RX FIFO for using the whole MB area
as a receive buffer and off-loading this buffer area directly in the IRQ-
handler.

The last patch is part of the implementation of errata ERR005829 for the
flexcan peripheral in i.MX6 SoCs. The first steps of this errata workaround
are implicitely included in patch 2 already.

David Jander (3):
  can: flexcan.c: Correctly initialize mailboxes
  can: flexcan.c: Re-write receive path to use MB queue instead of FIFO
  can: flexcan.c: Implement last step of workaround for errata ERR005829

[PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[David Jander]

Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/flexcan.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 944aa5d..a9700f3 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->base;
 	int err;
 	u32 reg_mcr, reg_ctrl;
+	int i;
 
 	/* enable module */
 	err = flexcan_chip_enable(priv);
@@ -867,9 +868,11 @@ static int flexcan_chip_start(struct net_device *dev)
 	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
 	flexcan_write(reg_ctrl, &regs->ctrl);
 
-	/* Abort any pending TX, mark Mailbox as INACTIVE */
-	flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
-		      &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	/* Clear and invalidate all mailboxes first */
+	for (i = 0; i < 64; i++) {
+		flexcan_write(FLEXCAN_MB_CNT_CODE(0),
+		      &regs->cantxfg[i].can_ctrl);
+	}
 
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
-- 
1.9.1

[PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[David Jander]

The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
mailbox space capable of holding up to 63 messages.
This space was largely unused, limiting the permissible latency from
interrupt to NAPI to only 6 messages. This patch uses all available MBs
for message reception and frees the MBs in the IRQ handler to greatly
decrease the likelihood of receive overruns.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/flexcan.c | 303 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 226 insertions(+), 77 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index a9700f3..bf444ff 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2005-2006 Varma Electronics Oy
  * Copyright (c) 2009 Sascha Hauer, Pengutronix
  * Copyright (c) 2010 Marc Kleine-Budde, Pengutronix
+ * Copyright (c) 2014 David Jander, Protonic Holland
  *
  * Based on code originally by Andrey Volkov <avolkov@varma-el.com>
  *
@@ -40,8 +41,8 @@
 
 #define DRV_NAME			"flexcan"
 
-/* 8 for RX fifo and 2 error handling */
-#define FLEXCAN_NAPI_WEIGHT		(8 + 2)
+/* 64 MB's */
+#define FLEXCAN_NAPI_WEIGHT		(64)
 
 /* FLEXCAN module configuration register (CANMCR) bits */
 #define FLEXCAN_MCR_MDIS		BIT(31)
@@ -62,7 +63,7 @@
 #define FLEXCAN_MCR_BCC			BIT(16)
 #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
 #define FLEXCAN_MCR_AEN			BIT(12)
-#define FLEXCAN_MCR_MAXMB(x)		((x) & 0x1f)
+#define FLEXCAN_MCR_MAXMB(x)		((x) & 0x7f)
 #define FLEXCAN_MCR_IDAM_A		(0 << 8)
 #define FLEXCAN_MCR_IDAM_B		(1 << 8)
 #define FLEXCAN_MCR_IDAM_C		(2 << 8)
@@ -92,6 +93,9 @@
 #define FLEXCAN_CTRL_ERR_ALL \
 	(FLEXCAN_CTRL_ERR_BUS | FLEXCAN_CTRL_ERR_STATE)
 
+/* FLEXCAN control register 2 (CTRL2) bits */
+#define FLEXCAN_CTRL2_EACEN		BIT(16)
+
 /* FLEXCAN error and status register (ESR) bits */
 #define FLEXCAN_ESR_TWRN_INT		BIT(17)
 #define FLEXCAN_ESR_RWRN_INT		BIT(16)
@@ -125,25 +129,22 @@
 	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
-#define FLEXCAN_TX_BUF_ID		8
+#define FLEXCAN_TX_BUF_ID		1
 #define FLEXCAN_IFLAG_BUF(x)		BIT(x)
-#define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW	BIT(7)
-#define FLEXCAN_IFLAG_RX_FIFO_WARN	BIT(6)
-#define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE	BIT(5)
-#define FLEXCAN_IFLAG_DEFAULT \
-	(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW | FLEXCAN_IFLAG_RX_FIFO_AVAILABLE | \
-	 FLEXCAN_IFLAG_BUF(FLEXCAN_TX_BUF_ID))
+#define FLEXCAN_IFLAG1_DEFAULT 		(0xfffffffe)
+#define FLEXCAN_IFLAG2_DEFAULT 		(0xffffffff)
 
 /* FLEXCAN message buffers */
-#define FLEXCAN_MB_CNT_CODE(x)		(((x) & 0xf) << 24)
+#define FLEXCAN_MB_CODE_SHIFT		24
+#define FLEXCAN_MB_CODE_MASK		(0xf << FLEXCAN_MB_CODE_SHIFT)
+#define FLEXCAN_MB_CNT_CODE(x)		(((x) << FLEXCAN_MB_CODE_SHIFT) & \
+						FLEXCAN_MB_CODE_MASK)
 #define FLEXCAN_MB_CNT_SRR		BIT(22)
 #define FLEXCAN_MB_CNT_IDE		BIT(21)
 #define FLEXCAN_MB_CNT_RTR		BIT(20)
 #define FLEXCAN_MB_CNT_LENGTH(x)	(((x) & 0xf) << 16)
 #define FLEXCAN_MB_CNT_TIMESTAMP(x)	((x) & 0xffff)
 
-#define FLEXCAN_MB_CODE_MASK		(0xf0ffffff)
-
 #define FLEXCAN_TIMEOUT_US             (50)
 
 /*
@@ -185,15 +186,17 @@ struct flexcan_regs {
 	u32 imask1;		/* 0x28 */
 	u32 iflag2;		/* 0x2c */
 	u32 iflag1;		/* 0x30 */
-	u32 crl2;		/* 0x34 */
+	u32 ctrl2;		/* 0x34 */
 	u32 esr2;		/* 0x38 */
 	u32 imeur;		/* 0x3c */
 	u32 lrfr;		/* 0x40 */
 	u32 crcr;		/* 0x44 */
 	u32 rxfgmask;		/* 0x48 */
 	u32 rxfir;		/* 0x4c */
-	u32 _reserved3[12];
-	struct flexcan_mb cantxfg[64];
+	u32 _reserved3[12];	/* 0x50 */
+	struct flexcan_mb cantxfg[64]; /* 0x80 */
+	u32 _reserved4[256];	/* 0x480 */
+	u32 rximr[64];		/* 0x880 */
 };
 
 struct flexcan_devtype_data {
@@ -214,6 +217,9 @@ struct flexcan_priv {
 	struct flexcan_platform_data *pdata;
 	const struct flexcan_devtype_data *devtype_data;
 	struct regulator *reg_xceiver;
+	struct flexcan_mb cantxfg_copy[FLEXCAN_NAPI_WEIGHT];
+	int second_first;
+	u32 rx_idx;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {
@@ -608,16 +614,30 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 	return 1;
 }
 
-static void flexcan_read_fifo(const struct net_device *dev,
-			      struct can_frame *cf)
+static int flexcan_read_frame(struct net_device *dev, int n)
 {
-	const struct flexcan_priv *priv = netdev_priv(dev);
-	struct flexcan_regs __iomem *regs = priv->base;
-	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_mb *mb = &priv->cantxfg_copy[n];
+	struct can_frame *cf;
+	struct sk_buff *skb;
 	u32 reg_ctrl, reg_id;
+	u32 code;
 
-	reg_ctrl = flexcan_read(&mb->can_ctrl);
-	reg_id = flexcan_read(&mb->can_id);
+	reg_ctrl = mb->can_ctrl;
+	code = (reg_ctrl & FLEXCAN_MB_CODE_MASK) >> FLEXCAN_MB_CODE_SHIFT;
+
+	/* is this MB empty? */
+	if ((code != 0x2) && (code != 0x6))
+		return 0;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	reg_id = mb->can_id;
 	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
 		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -627,27 +647,9 @@ static void flexcan_read_fifo(const struct net_device *dev,
 		cf->can_id |= CAN_RTR_FLAG;
 	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
 
-	*(__be32 *)(cf->data + 0) = cpu_to_be32(flexcan_read(&mb->data[0]));
-	*(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
-
-	/* mark as read */
-	flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
-	flexcan_read(&regs->timer);
-}
-
-static int flexcan_read_frame(struct net_device *dev)
-{
-	struct net_device_stats *stats = &dev->stats;
-	struct can_frame *cf;
-	struct sk_buff *skb;
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(mb->data[0]);
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(mb->data[1]);
 
-	skb = alloc_can_skb(dev, &cf);
-	if (unlikely(!skb)) {
-		stats->rx_dropped++;
-		return 0;
-	}
-
-	flexcan_read_fifo(dev, cf);
 	netif_receive_skb(skb);
 
 	stats->rx_packets++;
@@ -661,10 +663,16 @@ static int flexcan_read_frame(struct net_device *dev)
 static int flexcan_poll(struct napi_struct *napi, int quota)
 {
 	struct net_device *dev = napi->dev;
-	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_iflag1, reg_esr;
+	u32 reg_esr;
 	int work_done = 0;
+	int n;
+	int ret;
+
+	/* disable RX IRQs */
+	flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->imask1);
+	flexcan_write(0, &regs->imask2);
 
 	/*
 	 * The error bits are cleared on read,
@@ -675,12 +683,13 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	/* handle state changes */
 	work_done += flexcan_poll_state(dev, reg_esr);
 
-	/* handle RX-FIFO */
-	reg_iflag1 = flexcan_read(&regs->iflag1);
-	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
-	       work_done < quota) {
-		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = flexcan_read(&regs->iflag1);
+	/* handle mailboxes */
+	for (n = 0; (n < ARRAY_SIZE(priv->cantxfg_copy)) &&
+			(work_done < quota); n++) {
+		ret = flexcan_read_frame(dev, n);
+		if (!ret)
+			break;
+		work_done += ret;
 	}
 
 	/* report bus errors */
@@ -689,24 +698,157 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 
 	if (work_done < quota) {
 		napi_complete(napi);
-		/* enable IRQs */
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
+		priv->rx_idx = 0;
+
+		/* enable RX IRQs */
+		flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
+		flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
 	}
 
 	return work_done;
 }
 
+static u32 flexcan_copy_one_rxmb(struct flexcan_priv *priv, int i)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb;
+	struct flexcan_mb *dst;
+	u32 reg_ctrl;
+	u32 code;
+
+	mb = &regs->cantxfg[i];
+	dst = &priv->cantxfg_copy[priv->rx_idx];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	code = (reg_ctrl & FLEXCAN_MB_CODE_MASK) >> FLEXCAN_MB_CODE_SHIFT;
+
+	while (code & 1) {
+		/* MB busy, shouldn't take long */
+		reg_ctrl = flexcan_read(&mb->can_ctrl);
+		code = (reg_ctrl & FLEXCAN_MB_CODE_MASK) >> FLEXCAN_MB_CODE_SHIFT;
+	}
+
+	/* Copy contents */
+	dst->can_ctrl = reg_ctrl;
+	dst->can_id = flexcan_read(&mb->can_id);
+	dst->data[0] = flexcan_read(&mb->data[0]);
+	dst->data[1] = flexcan_read(&mb->data[1]);
+
+	/* If it's FULL or OVERRUN, clear the interrupt flag and lock MB */
+	if ((code == 0x2) || (code == 0x6)) {
+		if (i < 32)
+			flexcan_write(BIT(i), &regs->iflag1);
+		else
+			flexcan_write(BIT(i - 32), &regs->iflag2);
+		flexcan_write(FLEXCAN_MB_CNT_CODE(0x0), &mb->can_ctrl);
+		if ((code == 0x6) || (priv->rx_idx ==
+				(ARRAY_SIZE(priv->cantxfg_copy) - 1))) {
+			/* This MB was overrun, we lost data */
+			priv->dev->stats.rx_over_errors++;
+			priv->dev->stats.rx_errors++;
+		}
+		if (priv->rx_idx < (ARRAY_SIZE(priv->cantxfg_copy) - 1))
+			priv->rx_idx++;
+	}
+
+	return code;
+}
+
+static void flexcan_unlock_if_locked(struct flexcan_priv *priv, int i)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb;
+	u32 reg_ctrl;
+	u32 code;
+
+	mb = &regs->cantxfg[i];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	code = (reg_ctrl & FLEXCAN_MB_CODE_MASK) >> FLEXCAN_MB_CODE_SHIFT;
+	if (!code)
+		flexcan_write(FLEXCAN_MB_CNT_CODE(0x4), &mb->can_ctrl);
+}
+
+/*
+ * flexcan_copy_rxmbs
+ *
+ * This function is called from interrupt and needs to make a safe copy
+ * of the MB area to offload the chip immediately to avoid loosing
+ * messages due to latency.
+ * To avoid loosing messages due to race-conditions while reading the MB's,
+ * we need to make use of the locking mechanism of FlexCAN. Unfortunately,
+ * in this mode, automatic locking applies _only_ to MBs that are _not_ EMPTY.
+ * This means we could identify a MB as EMPTY while it is about to get filled.
+ * To avoid this situation from disturbing our queue ordering, we do the
+ * following:
+ * We split the MB area into two halves:
+ * The lower 32 (-2 due to one TX-MB and errata ERR005829) and the upper 32.
+ * We start with all MBs in EMPTY state and all filters disabled (don't care).
+ * FlexCAN will start filling from the lowest MB. Once this function is called,
+ * we copy and disable in an atomic operation all FULL MBs. The first EMPTY one
+ * we encounter may be about to get filled so we stop there. Next time FlexCAN
+ * will have filled more MBs. Once there are no EMPTY MBs in the lower half, we
+ * EMPTY all FULL or locked MBs again.
+ * Next time we have the following situation:
+ * If there is a FULL MB in the upper half, it is because it was about to get
+ * filled when we scanned last time, or got filled just before emptying the
+ * lowest MB, so this will be the first MB we need to copy now. If there are
+ * no EMPTY MBs in the lower half at this time, it means we cannot guarantee
+ * ordering anymore. It also means latency exceeded 30 messages.
+ */
+static void flexcan_copy_rxmbs(struct flexcan_priv *priv, u32 iflag1, u32 iflag2)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	int i;
+
+	if (priv->second_first) {
+		/* Begin with second half */
+		for(i = 32; i < 64; i++) {
+			flexcan_copy_one_rxmb(priv, i);
+			flexcan_unlock_if_locked(priv, i);
+		}
+	}
+
+	/* Copy and disable FULL MBs */
+	for(i = 1; i < 64; i++) {
+		if (i == FLEXCAN_TX_BUF_ID)
+			continue;
+		if (flexcan_copy_one_rxmb(priv, i) == 0x4)
+			break;
+	}
+
+	if ((i >= 32) && priv->second_first)
+		netdev_warn(priv->dev, "RX order cannot be guaranteed. count=%d\n", i);
+
+	priv->second_first = 0;
+
+	/* No EMPTY MB in first half? */
+	if (i >= 32) {
+		/* Re-enable all disabled MBs */
+		for(i = 1; i < 64; i++) {
+			if (i == FLEXCAN_TX_BUF_ID)
+				continue;
+			flexcan_unlock_if_locked(priv, i);
+		}
+
+		/* Next time we need to check the second half first */
+		priv->second_first = 1;
+	}
+
+	/* Unlock the last locked MB if any */
+	flexcan_read(&regs->timer);
+}
+
 static irqreturn_t flexcan_irq(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct net_device_stats *stats = &dev->stats;
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_iflag1, reg_esr;
+	u32 reg_iflag1, reg_iflag2, reg_esr;
 
 	reg_iflag1 = flexcan_read(&regs->iflag1);
+	reg_iflag2 = flexcan_read(&regs->iflag2);
 	reg_esr = flexcan_read(&regs->esr);
+
 	/* ACK all bus error and state change IRQ sources */
 	if (reg_esr & FLEXCAN_ESR_ALL_INT)
 		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
@@ -717,7 +859,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	 * - state change IRQ
 	 * - bus error IRQ and bus error reporting is activated
 	 */
-	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
+	if ((reg_iflag1 & ~(1 << FLEXCAN_TX_BUF_ID)) || reg_iflag2 ||
 	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
 		/*
@@ -725,20 +867,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		 * save them for later use.
 		 */
 		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
-			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
-		       &regs->ctrl);
+		flexcan_copy_rxmbs(priv, reg_iflag1, reg_iflag2);
 		napi_schedule(&priv->napi);
 	}
 
-	/* FIFO overflow */
-	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
-		flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
-		dev->stats.rx_over_errors++;
-		dev->stats.rx_errors++;
-	}
-
 	/* transmission complete interrupt */
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		stats->tx_bytes += can_get_echo_skb(dev, 0);
@@ -800,7 +932,7 @@ static int flexcan_chip_start(struct net_device *dev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
 	int err;
-	u32 reg_mcr, reg_ctrl;
+	u32 reg_mcr, reg_ctrl, reg_ctrl2;
 	int i;
 
 	/* enable module */
@@ -828,11 +960,11 @@ static int flexcan_chip_start(struct net_device *dev)
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
-	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
-	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
+	reg_mcr &= ~(FLEXCAN_MCR_MAXMB(0xff) | FLEXCAN_MCR_FEN);
+	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
 		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
-		FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
+		FLEXCAN_MCR_BCC | FLEXCAN_MCR_MAXMB(0x40);
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -868,12 +1000,28 @@ static int flexcan_chip_start(struct net_device *dev)
 	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
 	flexcan_write(reg_ctrl, &regs->ctrl);
 
-	/* Clear and invalidate all mailboxes first */
-	for (i = 0; i < 64; i++) {
-		flexcan_write(FLEXCAN_MB_CNT_CODE(0),
-		      &regs->cantxfg[i].can_ctrl);
+	/* CTRL2: Enable EACEN */
+	reg_ctrl2 = flexcan_read(&regs->ctrl2);
+	reg_ctrl2 |= FLEXCAN_CTRL2_EACEN;
+	flexcan_write(reg_ctrl2, &regs->ctrl2);
+
+	/* Prepare mailboxes. Skip the first, use one for TX the rest for RX */
+	for (i = 1; i < 64; i++) {
+		if (i == FLEXCAN_TX_BUF_ID)
+			flexcan_write(FLEXCAN_MB_CNT_CODE(0x8),
+				&regs->cantxfg[i].can_ctrl);
+		else
+			flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
+			      &regs->cantxfg[i].can_ctrl);
+		flexcan_write(0, &regs->rximr[i]); /* Clear filter */
 	}
 
+	/* Invalidate mailbox 0 according to errata ERR005829 */
+	flexcan_write(FLEXCAN_MB_CNT_CODE(0x0), &regs->cantxfg[0].can_ctrl);
+
+	priv->second_first = 0;
+	priv->rx_idx = 0;
+
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
 	flexcan_write(0x0, &regs->rx14mask);
@@ -893,8 +1041,9 @@ static int flexcan_chip_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* enable FIFO interrupts */
-	flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+	/* enable all MB interrupts */
+	flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
+	flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
 
 	/* print chip status */
 	netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
-- 
1.9.1

[PATCH 3/3] can: flexcan.c: Implement last step of workaround for errata ERR005829

[David Jander]

It is not clear if this errata only applies to i.MX6, but the impact of
the workaround should be minimal. Basically it comes down to reserving
mailbox 0 as a permanently inactive MB and writing twice to its C/S field
on every message transmit.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/flexcan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index bf444ff..3998d4c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -434,6 +434,10 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
 	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
+	/* Errata ERR005829 step8: Write twice INACTIVE(0b1000) code to MB 0 */
+	flexcan_write(FLEXCAN_MB_CNT_CODE(0x8), &regs->cantxfg[0].can_ctrl);
+	flexcan_write(FLEXCAN_MB_CNT_CODE(0x8), &regs->cantxfg[0].can_ctrl);
+
 	return NETDEV_TX_OK;
 }
 
-- 
1.9.1

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On 08/27/2014 11:58 AM, David Jander wrote:
> Apparently mailboxes may contain random data at startup, causing some of
> them being prepared for message reception. This causes overruns being
> missed or even confusing the IRQ check for trasmitted messages, increasing
> the transmit counter instead of the error counter.
> 
> Signed-off-by: David Jander <david@protonic.nl>

Before patch

0d1862e can: flexcan: fix flexcan_chip_start() on imx6

there was a loop clearing the whole cantxfg register space. But this
turned out to be bogus, as message buffers 1...7 are reserved by the
FIFO engine and we're not allowed to tough them. This lead to some kind
of abort on imx6.

You may need this patch once you don't make use of the FIFO engine any more.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[David Jander]

On Tue, 02 Sep 2014 12:24:28 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/27/2014 11:58 AM, David Jander wrote:
> > Apparently mailboxes may contain random data at startup, causing some of
> > them being prepared for message reception. This causes overruns being
> > missed or even confusing the IRQ check for trasmitted messages, increasing
> > the transmit counter instead of the error counter.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> Before patch
> 
> 0d1862e can: flexcan: fix flexcan_chip_start() on imx6
> 
> there was a loop clearing the whole cantxfg register space. But this
> turned out to be bogus, as message buffers 1...7 are reserved by the
> FIFO engine and we're not allowed to tough them. This lead to some kind
> of abort on imx6.
> 
> You may need this patch once you don't make use of the FIFO engine any more.

You will need this patch in either case, but indeed, if you use the FIFO, you
should skip the MB's that are shadowed by the FIFO.
If you don't clear the rest of the MB's they may still contain random data and
the problem remains.
IMHO 0d1862e is wrong, since buffers are not in reset default values. There is
no indication of that in the reference manual, and I have observed that they
are indeed not cleared after reset.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]

On 09/02/2014 12:37 PM, David Jander wrote:
> On Tue, 02 Sep 2014 12:24:28 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 08/27/2014 11:58 AM, David Jander wrote:
>>> Apparently mailboxes may contain random data at startup, causing some of
>>> them being prepared for message reception. This causes overruns being
>>> missed or even confusing the IRQ check for trasmitted messages, increasing
>>> the transmit counter instead of the error counter.
>>>
>>> Signed-off-by: David Jander <david@protonic.nl>
>>
>> Before patch
>>
>> 0d1862e can: flexcan: fix flexcan_chip_start() on imx6
>>
>> there was a loop clearing the whole cantxfg register space. But this
>> turned out to be bogus, as message buffers 1...7 are reserved by the
>> FIFO engine and we're not allowed to tough them. This lead to some kind
>> of abort on imx6.
>>
>> You may need this patch once you don't make use of the FIFO engine any more.
> 
> You will need this patch in either case, but indeed, if you use the FIFO, you
> should skip the MB's that are shadowed by the FIFO.

ACK

> If you don't clear the rest of the MB's they may still contain random data and
> the problem remains.
> IMHO 0d1862e is wrong, since buffers are not in reset default values. There is
> no indication of that in the reference manual, and I have observed that they
> are indeed not cleared after reset.

Yes, 0d1862e was not complete, the initialisation was fixes with:

d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
                       mark one MB for TX and abort pending TX

Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
and the code of the tx mailbox is set to 0x4 == tx, inactive.

This should be enough in FIFO mode, correct?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[David Jander]

On Tue, 02 Sep 2014 12:59:42 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 09/02/2014 12:37 PM, David Jander wrote:
> > On Tue, 02 Sep 2014 12:24:28 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 08/27/2014 11:58 AM, David Jander wrote:
> >>> Apparently mailboxes may contain random data at startup, causing some of
> >>> them being prepared for message reception. This causes overruns being
> >>> missed or even confusing the IRQ check for trasmitted messages,
> >>> increasing the transmit counter instead of the error counter.
> >>>
> >>> Signed-off-by: David Jander <david@protonic.nl>
> >>
> >> Before patch
> >>
> >> 0d1862e can: flexcan: fix flexcan_chip_start() on imx6
> >>
> >> there was a loop clearing the whole cantxfg register space. But this
> >> turned out to be bogus, as message buffers 1...7 are reserved by the
> >> FIFO engine and we're not allowed to tough them. This lead to some kind
> >> of abort on imx6.
> >>
> >> You may need this patch once you don't make use of the FIFO engine any
> >> more.
> > 
> > You will need this patch in either case, but indeed, if you use the FIFO,
> > you should skip the MB's that are shadowed by the FIFO.
> 
> ACK
> 
> > If you don't clear the rest of the MB's they may still contain random data
> > and the problem remains.
> > IMHO 0d1862e is wrong, since buffers are not in reset default values.
> > There is no indication of that in the reference manual, and I have
> > observed that they are indeed not cleared after reset.
> 
> Yes, 0d1862e was not complete, the initialisation was fixes with:
> 
> d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
>                        mark one MB for TX and abort pending TX
> 
> Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
> and the code of the tx mailbox is set to 0x4 == tx, inactive.
> 
> This should be enough in FIFO mode, correct?

AFAICS not. There could still be other MB's with reception or transmission
enabled (randomly) causing potential data loss, extra frames sent and/or errors
in the statistics.
If the FIFO is full for example it should overflow with the next message, but
if the next message instead hits an (randomly) empty and readied RX MB
somewhere, the overflow is undetected and one (or more) frame(s) is lost.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 3/3] can: flexcan.c: Implement last step of workaround for errata ERR005829

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On 08/27/2014 11:58 AM, David Jander wrote:
> It is not clear if this errata only applies to i.MX6, but the impact of
> the workaround should be minimal. Basically it comes down to reserving
> mailbox 0 as a permanently inactive MB and writing twice to its C/S field
> on every message transmit.
> 
> Signed-off-by: David Jander <david@protonic.nl>

Please fix this errata in one patch exactly. Please fix the errata
before introducing a new feature, i.e. rewriting of the RX path.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On 08/27/2014 11:58 AM, David Jander wrote:
> The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> mailbox space capable of holding up to 63 messages.
> This space was largely unused, limiting the permissible latency from
> interrupt to NAPI to only 6 messages. This patch uses all available MBs
> for message reception and frees the MBs in the IRQ handler to greatly
> decrease the likelihood of receive overruns.

What about the order of the incoming CAN frames? Is it still preserved?

You make use of the CTRL2 register, which is not present on some older
(but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
0x40, which is not supported on older IPs. The register rximr, is also
not present on older cores. Don't break support for the older CAN cores.

Please make this patch based on linux-can-next/master (which holds some
updates to the regs structure).

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[David Jander]

On Tue, 02 Sep 2014 12:59:42 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 09/02/2014 12:37 PM, David Jander wrote:
> > On Tue, 02 Sep 2014 12:24:28 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 08/27/2014 11:58 AM, David Jander wrote:
> >>> Apparently mailboxes may contain random data at startup, causing some of
> >>> them being prepared for message reception. This causes overruns being
> >>> missed or even confusing the IRQ check for trasmitted messages,
> >>> increasing the transmit counter instead of the error counter.
> >>>
> >>> Signed-off-by: David Jander <david@protonic.nl>
> >>
> >> Before patch
> >>
> >> 0d1862e can: flexcan: fix flexcan_chip_start() on imx6
> >>
> >> there was a loop clearing the whole cantxfg register space. But this
> >> turned out to be bogus, as message buffers 1...7 are reserved by the
> >> FIFO engine and we're not allowed to tough them. This lead to some kind
> >> of abort on imx6.
> >>
> >> You may need this patch once you don't make use of the FIFO engine any
> >> more.
> > 
> > You will need this patch in either case, but indeed, if you use the FIFO,
> > you should skip the MB's that are shadowed by the FIFO.
> 
> ACK
> 
> > If you don't clear the rest of the MB's they may still contain random data
> > and the problem remains.
> > IMHO 0d1862e is wrong, since buffers are not in reset default values.
> > There is no indication of that in the reference manual, and I have
> > observed that they are indeed not cleared after reset.
> 
> Yes, 0d1862e was not complete, the initialisation was fixes with:
> 
> d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
>                        mark one MB for TX and abort pending TX
> 
> Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
> and the code of the tx mailbox is set to 0x4 == tx, inactive.

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 3f21142..f028c5d 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -62,7 +62,7 @@
 #define FLEXCAN_MCR_BCC			BIT(16)
 #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
 #define FLEXCAN_MCR_AEN			BIT(12)
-#define FLEXCAN_MCR_MAXMB(x)		((x) & 0xf)
+#define FLEXCAN_MCR_MAXMB(x)		((x) & 0x1f)
 #define FLEXCAN_MCR_IDAM_A		(0 << 8)
 #define FLEXCAN_MCR_IDAM_B		(1 << 8)
 #define FLEXCAN_MCR_IDAM_C		(2 << 8)
@@ -735,9 +735,11 @@ static int flexcan_chip_start(struct net_device *dev)
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
+	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
-		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
+		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
+		FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
Eh! This looks wrong! The MAXMB field is 7 bits wide according to the
reference manual (bits 0-6)... but the reset default value is supposed to be
0x0f, so the mask is still enough to clear the reset default.
What I don't understand is why the CAN controller is still able to put
messages into MB's after the FIFO is full. At least that is what I observed.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 3/3] can: flexcan.c: Implement last step of workaround for errata ERR005829

[David Jander]

On Tue, 02 Sep 2014 13:28:55 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/27/2014 11:58 AM, David Jander wrote:
> > It is not clear if this errata only applies to i.MX6, but the impact of
> > the workaround should be minimal. Basically it comes down to reserving
> > mailbox 0 as a permanently inactive MB and writing twice to its C/S field
> > on every message transmit.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> Please fix this errata in one patch exactly. Please fix the errata
> before introducing a new feature, i.e. rewriting of the RX path.

Ok, I will try to split this out in the next version.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

On 09/02/2014 01:32 PM, David Jander wrote:
> On Tue, 02 Sep 2014 12:59:42 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 09/02/2014 12:37 PM, David Jander wrote:
>>> On Tue, 02 Sep 2014 12:24:28 +0200
>>> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>
>>>> On 08/27/2014 11:58 AM, David Jander wrote:
>>>>> Apparently mailboxes may contain random data at startup, causing some of
>>>>> them being prepared for message reception. This causes overruns being
>>>>> missed or even confusing the IRQ check for trasmitted messages,
>>>>> increasing the transmit counter instead of the error counter.
>>>>>
>>>>> Signed-off-by: David Jander <david@protonic.nl>
>>>>
>>>> Before patch
>>>>
>>>> 0d1862e can: flexcan: fix flexcan_chip_start() on imx6
>>>>
>>>> there was a loop clearing the whole cantxfg register space. But this
>>>> turned out to be bogus, as message buffers 1...7 are reserved by the
>>>> FIFO engine and we're not allowed to tough them. This lead to some kind
>>>> of abort on imx6.
>>>>
>>>> You may need this patch once you don't make use of the FIFO engine any
>>>> more.
>>>
>>> You will need this patch in either case, but indeed, if you use the FIFO,
>>> you should skip the MB's that are shadowed by the FIFO.
>>
>> ACK
>>
>>> If you don't clear the rest of the MB's they may still contain random data
>>> and the problem remains.
>>> IMHO 0d1862e is wrong, since buffers are not in reset default values.
>>> There is no indication of that in the reference manual, and I have
>>> observed that they are indeed not cleared after reset.
>>
>> Yes, 0d1862e was not complete, the initialisation was fixes with:
>>
>> d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
>>                        mark one MB for TX and abort pending TX
>>
>> Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
>> and the code of the tx mailbox is set to 0x4 == tx, inactive.
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 3f21142..f028c5d 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -62,7 +62,7 @@
>  #define FLEXCAN_MCR_BCC			BIT(16)
>  #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
>  #define FLEXCAN_MCR_AEN			BIT(12)
> -#define FLEXCAN_MCR_MAXMB(x)		((x) & 0xf)
> +#define FLEXCAN_MCR_MAXMB(x)		((x) & 0x1f)
>  #define FLEXCAN_MCR_IDAM_A		(0 << 8)
>  #define FLEXCAN_MCR_IDAM_B		(1 << 8)
>  #define FLEXCAN_MCR_IDAM_C		(2 << 8)
> @@ -735,9 +735,11 @@ static int flexcan_chip_start(struct net_device *dev)
>  	 *
>  	 */
>  	reg_mcr = flexcan_read(&regs->mcr);
> +	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
>  	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
>  		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
> -		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
> +		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
> +		FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	flexcan_write(reg_mcr, &regs->mcr);
>  
> Eh! This looks wrong! The MAXMB field is 7 bits wide according to the
> reference manual (bits 0-6)... but the reset default value is supposed to be

...according to the imx6 reference manual. The size of the MAXMB field
depends on the actual IP version.

> 0x0f, so the mask is still enough to clear the reset default.
> What I don't understand is why the CAN controller is still able to put
> messages into MB's after the FIFO is full. At least that is what I observed.

It says to in the imx6 manual, but not in the older ones (see other
mail, that I'm still writing).

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Tue, 02 Sep 2014 13:30:24 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/27/2014 11:58 AM, David Jander wrote:
> > The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> > mailbox space capable of holding up to 63 messages.
> > This space was largely unused, limiting the permissible latency from
> > interrupt to NAPI to only 6 messages. This patch uses all available MBs
> > for message reception and frees the MBs in the IRQ handler to greatly
> > decrease the likelihood of receive overruns.
> 
> What about the order of the incoming CAN frames? Is it still preserved?

Yes, it is preserved as long as latency doesn't exceed 30 frames, which is
way more than the original driver could take. The algorithm is not trivial,
therefor I included an explanatory comment to flexcan_copy_rxmbs().

> You make use of the CTRL2 register, which is not present on some older
> (but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
> 0x40, which is not supported on older IPs. The register rximr, is also
> not present on older cores. Don't break support for the older CAN cores.

Oops. Thanks for pointing that out. I will check the reference manual of the
i.MX53 (which should have be the oldest supported version of this IP core,
right?). Of course I do not want to break older CAN cores.
I will check correctness testing the code on an i.MX28 board which I have.

Indeed the MAXMB field in IP-version 3 seems to be 6 bits wide instead of 7,
which doesn't make sense, since there are still 64 MB's. If one would want all
MB's to take part in the arbitration process, one would need to write 0x40 to
this register, which doesn't fit. Could it be that this is just a
documentation error? I would bet so. Who knows?
We can settle for 0x3f to be safe, can't we?

> Please make this patch based on linux-can-next/master (which holds some
> updates to the regs structure).

Thanks for pointing out. I will use that as base for the next version.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

On 09/02/2014 01:15 PM, David Jander wrote:

>> Yes, 0d1862e was not complete, the initialisation was fixes with:
>>
>> d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
>>                        mark one MB for TX and abort pending TX
>>
>> Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
>> and the code of the tx mailbox is set to 0x4 == tx, inactive.
>>
>> This should be enough in FIFO mode, correct?
> 
> AFAICS not. There could still be other MB's with reception or transmission
> enabled (randomly) causing potential data loss, extra frames sent and/or errors
> in the statistics.

> If the FIFO is full for example it should overflow with the next message, but
> if the next message instead hits an (randomly) empty and readied RX MB
> somewhere, the overflow is undetected and one (or more) frame(s) is lost.

Is FIFO to normal mailbox overflow a new feature of the imx6 flexcan
core? The mx35 data sheet states:

> If the FIFO is full and more frames continue to be received, an
> OVERFLOW interrupt is issued to the ARM and subsequent frames are not
> accepted until the ARM creates space in the FIFO by reading one or
> more frames.

While the mx6 states:

> Note that the flag will not be asserted when the Rx FIFO is
> full and the message was captured by a Mailbox.

But later in the imx35:

> In the event that the FIFO is full, the matching algorithm always
> looks for a matching message buffer outside the FIFO region.

This probably means even on the mx35 the _FIFO_ does not accept any more
message if it's full, but the other mailboxes may....

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[David Jander]

On Tue, 02 Sep 2014 15:54:06 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 09/02/2014 01:15 PM, David Jander wrote:
> 
> >> Yes, 0d1862e was not complete, the initialisation was fixes with:
> >>
> >> d5a7b40 can: flexcan: flexcan_chip_start: fix regression,
> >>                        mark one MB for TX and abort pending TX
> >>
> >> Which sets FLEXCAN_MCR_MAXMB to 8, which is the only mailbox used for tx
> >> and the code of the tx mailbox is set to 0x4 == tx, inactive.
> >>
> >> This should be enough in FIFO mode, correct?
> > 
> > AFAICS not. There could still be other MB's with reception or transmission
> > enabled (randomly) causing potential data loss, extra frames sent and/or
> > errors in the statistics.
> 
> > If the FIFO is full for example it should overflow with the next message,
> > but if the next message instead hits an (randomly) empty and readied RX MB
> > somewhere, the overflow is undetected and one (or more) frame(s) is lost.
> 
> Is FIFO to normal mailbox overflow a new feature of the imx6 flexcan
> core? The mx35 data sheet states:
> 
> > If the FIFO is full and more frames continue to be received, an
> > OVERFLOW interrupt is issued to the ARM and subsequent frames are not
> > accepted until the ARM creates space in the FIFO by reading one or
> > more frames.
> 
> While the mx6 states:
> 
> > Note that the flag will not be asserted when the Rx FIFO is
> > full and the message was captured by a Mailbox.
> 
> But later in the imx35:
> 
> > In the event that the FIFO is full, the matching algorithm always
> > looks for a matching message buffer outside the FIFO region.
> 
> This probably means even on the mx35 the _FIFO_ does not accept any more
> message if it's full, but the other mailboxes may....

In the i.MX6 RM there's also this (Chapter 27.6.5 Matching Process):

> [...]
> If the FIFO is enabled, the priority of scanning can be selected between
> Mailboxes and FIFO filters. In any case, the matching starts from the lowest
> number Message Buffer toward the higher ones. If no match is found within
> the first structure then the other is scanned subsequently. In the event
> that the FIFO is full, the matching algorithm will always look for a
> matching MB outside the FIFO region.

What's not quite clear to me right now, is the function of MCR_MAXMB after
reading this. As far as I have observed, MCR_MAXMB is of no influence for
this. I needed to explicitely invalidate all remaining MB's to avoid flexcan
putting overflow messages there.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 1/3] can: flexcan.c: Correctly initialize mailboxes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

On 09/02/2014 01:38 PM, Marc Kleine-Budde wrote:

>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 3f21142..f028c5d 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -62,7 +62,7 @@
>>  #define FLEXCAN_MCR_BCC			BIT(16)
>>  #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
>>  #define FLEXCAN_MCR_AEN			BIT(12)
>> -#define FLEXCAN_MCR_MAXMB(x)		((x) & 0xf)
>> +#define FLEXCAN_MCR_MAXMB(x)		((x) & 0x1f)
>>  #define FLEXCAN_MCR_IDAM_A		(0 << 8)
>>  #define FLEXCAN_MCR_IDAM_B		(1 << 8)
>>  #define FLEXCAN_MCR_IDAM_C		(2 << 8)
>> @@ -735,9 +735,11 @@ static int flexcan_chip_start(struct net_device *dev)
>>  	 *
>>  	 */
>>  	reg_mcr = flexcan_read(&regs->mcr);
>> +	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
>>  	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
>>  		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
>> -		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
>> +		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
>> +		FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
>>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>>  	flexcan_write(reg_mcr, &regs->mcr);
>>  
>> Eh! This looks wrong! The MAXMB field is 7 bits wide according to the
>> reference manual (bits 0-6)... but the reset default value is supposed to be
> 
> ...according to the imx6 reference manual. The size of the MAXMB field
> depends on the actual IP version.

On mcf5213 (some old coldfire, not supported by this driver)
it's 4 bit wide.
On mx25, mx28, mx53 it's 6 bits wide.
On mx6{q,sdl}, vf610 it's 7 bits wide.
I'm not sure about the powerpc based cores as I don't have the data sheets.

I cannot find a data sheet, where it has a width of 5 bit.

I'll prepare a patch to fix it.

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On 09/02/2014 02:04 PM, David Jander wrote:
> On Tue, 02 Sep 2014 13:30:24 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 08/27/2014 11:58 AM, David Jander wrote:
>>> The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
>>> mailbox space capable of holding up to 63 messages.
>>> This space was largely unused, limiting the permissible latency from
>>> interrupt to NAPI to only 6 messages. This patch uses all available MBs
>>> for message reception and frees the MBs in the IRQ handler to greatly
>>> decrease the likelihood of receive overruns.
>>
>> What about the order of the incoming CAN frames? Is it still preserved?
> 
> Yes, it is preserved as long as latency doesn't exceed 30 frames, which is
> way more than the original driver could take. The algorithm is not trivial,
> therefor I included an explanatory comment to flexcan_copy_rxmbs().

To be honest, I haven't looked at the algorithm in detail. I think we
first have to fix both bugs (the errata and the mailbox initialization
issue).

>> You make use of the CTRL2 register, which is not present on some older
>> (but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
>> 0x40, which is not supported on older IPs. The register rximr, is also
>> not present on older cores. Don't break support for the older CAN cores.
> 
> Oops. Thanks for pointing that out. I will check the reference manual of the
> i.MX53 (which should have be the oldest supported version of this IP core,
> right?). Of course I do not want to break older CAN cores.
> I will check correctness testing the code on an i.MX28 board which I have.

Regarding the versions of the IP cores, we should not trust them, as
they are not public available..

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Tue, 02 Sep 2014 16:53:46 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 09/02/2014 02:04 PM, David Jander wrote:
> > On Tue, 02 Sep 2014 13:30:24 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 08/27/2014 11:58 AM, David Jander wrote:
> >>> The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> >>> mailbox space capable of holding up to 63 messages.
> >>> This space was largely unused, limiting the permissible latency from
> >>> interrupt to NAPI to only 6 messages. This patch uses all available MBs
> >>> for message reception and frees the MBs in the IRQ handler to greatly
> >>> decrease the likelihood of receive overruns.
> >>
> >> What about the order of the incoming CAN frames? Is it still preserved?
> > 
> > Yes, it is preserved as long as latency doesn't exceed 30 frames, which is
> > way more than the original driver could take. The algorithm is not trivial,
> > therefor I included an explanatory comment to flexcan_copy_rxmbs().
> 
> To be honest, I haven't looked at the algorithm in detail. I think we
> first have to fix both bugs (the errata and the mailbox initialization
> issue).

Ok, I agree.

> >> You make use of the CTRL2 register, which is not present on some older
> >> (but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
> >> 0x40, which is not supported on older IPs. The register rximr, is also
> >> not present on older cores. Don't break support for the older CAN cores.
> > 
> > Oops. Thanks for pointing that out. I will check the reference manual of
> > the i.MX53 (which should have be the oldest supported version of this IP
> > core, right?). Of course I do not want to break older CAN cores.
> > I will check correctness testing the code on an i.MX28 board which I have.
> 
> Regarding the versions of the IP cores, we should not trust them, as
> they are not public available..

Ok. I fear that for this last patch we will need some run-time check for the
version of the flexcan we have. I suppose the typical table with OF ids and
flags will do?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On 09/03/2014 09:19 AM, David Jander wrote:
>>>> You make use of the CTRL2 register, which is not present on some older
>>>> (but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
>>>> 0x40, which is not supported on older IPs. The register rximr, is also
>>>> not present on older cores. Don't break support for the older CAN cores.
>>>
>>> Oops. Thanks for pointing that out. I will check the reference manual of
>>> the i.MX53 (which should have be the oldest supported version of this IP
>>> core, right?). Of course I do not want to break older CAN cores.
>>> I will check correctness testing the code on an i.MX28 board which I have.
>>
>> Regarding the versions of the IP cores, we should not trust them, as
>> they are not public available..
> 
> Ok. I fear that for this last patch we will need some run-time check for the
> version of the flexcan we have. I suppose the typical table with OF ids and
> flags will do?

Yes, we already track this information in
> struct flexcan_devtype_data {
> 	u32 features;	/* hardware controller features */
> };

There is:
> #define FLEXCAN_HAS_V10_FEATURES	BIT(1) /* For core version >= 10 */

which is probably what you need. Feel free to add another bit, if you
need to.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] can: flexcan.c: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Tue, 02 Sep 2014 13:30:24 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/27/2014 11:58 AM, David Jander wrote:
> > The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> > mailbox space capable of holding up to 63 messages.
> > This space was largely unused, limiting the permissible latency from
> > interrupt to NAPI to only 6 messages. This patch uses all available MBs
> > for message reception and frees the MBs in the IRQ handler to greatly
> > decrease the likelihood of receive overruns.
> 
> What about the order of the incoming CAN frames? Is it still preserved?
> 
> You make use of the CTRL2 register, which is not present on some older
> (but supported) flexcan IP cores. You increase FLEXCAN_MCR_MAXMB to
> 0x40, which is not supported on older IPs. The register rximr, is also
> not present on older cores. Don't break support for the older CAN cores.

I've checked the RM of i.MX53, i.MX28 and i.MX25, and they all seem to support
the rximr registers. Which IP version doesn't support them (that is still
supported by this driver anyway)?

AFAICS, the only unsupported feature on older SoC's is the CTRL2 register, and
the fact that the documentation says the MCR_MAXMB field is only 6 bits wide,
right?

Best regards,

-- 
David Jander
Protonic Holland.