[RFC PATCH V3 00/15] CAN: Add rx-fifo support and port flexcan to it

[RFC PATCH V3 00/15] CAN: Add rx-fifo support and port flexcan to it

[David Jander]

This is the whole patch series again, adding the original rx-fifo framework
from Marc Kleine-Budde as well as the morphing into IRQ-offload mode and a
sample implementation using the flexcan driver.

The new flexcan driver supports full fifo abstraction mode for V10 and newer
peripherals, and simple irq-offloading for older parts.

Changes from V2:
 - Re-issued all patches leading up to this, because some old flexcan work
   needed re-basing.
 - Some cosmetic fixes
 - Added can_rx_fifo_reset() function
 - Added rx-fifo support for can state- and error polling
 - Added support for simple irq-offloading
 - Re-enabled flexcan support for RTR reception on older IPs.

David Jander (11):
  can: flexcan: Re-write receive path to use MB queue instead of FIFO
  can: rx-fifo: Increase MB size limit from 32 to 64
  can: rx-fifo: Change to do controller off-load in interrupt and NAPI
    poll
  can: rx-fifo: fix long lines
  can: rx-fifo: Add can_rx_fifo_reset() function
  can: rx-fifo: remove obsolete comment
  can: rx-fifo: Add support for can state tracking and error polling
  can: flexcan: Add support for RX-FIFO.
  can: rx-fifo: Add support for simple irq offloading
  can: flexcan: Add MB/Fifo specific column to comment table of IP
    versions
  can: flexcan: Re-enable RTR reception support for older flexcan IPs

Marc Kleine-Budde (4):
  can: flexcan: add documentation about mailbox organizaiton
  can: flexcan: rename crl2 -> ctrl2
  can: flexcan: replace open coded mailbox code by proper defines
  can: dev: add preliminary rx-fifo

 drivers/net/can/dev.c     | 324 +++++++++++++++++++++++++++++++++
 drivers/net/can/flexcan.c | 452 ++++++++++++++++++++++++++++++++--------------
 include/linux/can/dev.h   |  46 +++++
 3 files changed, 687 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH 01/15] can: flexcan: add documentation about mailbox organizaiton

[David Jander]

From: Marc Kleine-Budde <mkl@pengutronix.de>

This patch adds a short documentation snippet about the mailbox organization as
it's regularly not correct in freescale's datasheets.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 172065c..2e70a27 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -230,6 +230,16 @@ struct flexcan_regs {
 	u32 rxfir;		/* 0x4c */
 	u32 _reserved3[12];	/* 0x50 */
 	struct flexcan_mb cantxfg[64];	/* 0x80 */
+	/* FIFO-mode:
+	 *			MB
+	 * 0x080...0x08f	0	RX message buffer
+	 * 0x090...0x0df	1-5	reserverd
+	 * 0x0e0...0x0ff	6-7	8 entry ID table
+	 *				(mx25, mx28, mx35, mx53)
+	 * 0x0e0...0x2df	6-7..37	8..128 entry ID table
+	 *			  	size conf'ed via ctrl2::RFFN
+	 *				(mx6, vf610)
+	 */
 	u32 _reserved4[408];
 	u32 mecr;		/* 0xae0 */
 	u32 erriar;		/* 0xae4 */
-- 
1.9.1

[PATCH 02/15] can: flexcan: rename crl2 -> ctrl2

[David Jander]

From: Marc Kleine-Budde <mkl@pengutronix.de>

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 2e70a27..b287367 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -93,13 +93,13 @@
 	(FLEXCAN_CTRL_ERR_BUS | FLEXCAN_CTRL_ERR_STATE)
 
 /* FLEXCAN control register 2 (CTRL2) bits */
-#define FLEXCAN_CRL2_ECRWRE		BIT(29)
-#define FLEXCAN_CRL2_WRMFRZ		BIT(28)
-#define FLEXCAN_CRL2_RFFN(x)		(((x) & 0x0f) << 24)
-#define FLEXCAN_CRL2_TASD(x)		(((x) & 0x1f) << 19)
-#define FLEXCAN_CRL2_MRP		BIT(18)
-#define FLEXCAN_CRL2_RRS		BIT(17)
-#define FLEXCAN_CRL2_EACEN		BIT(16)
+#define FLEXCAN_CTRL2_ECRWRE		BIT(29)
+#define FLEXCAN_CTRL2_WRMFRZ		BIT(28)
+#define FLEXCAN_CTRL2_RFFN(x)		(((x) & 0x0f) << 24)
+#define FLEXCAN_CTRL2_TASD(x)		(((x) & 0x1f) << 19)
+#define FLEXCAN_CTRL2_MRP		BIT(18)
+#define FLEXCAN_CTRL2_RRS		BIT(17)
+#define FLEXCAN_CTRL2_EACEN		BIT(16)
 
 /* FLEXCAN memory error control register (MECR) bits */
 #define FLEXCAN_MECR_ECRWRDIS		BIT(31)
@@ -221,7 +221,7 @@ 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 */
@@ -893,7 +893,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, reg_crl2, reg_mecr;
+	u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr;
 	int i;
 
 	/* enable module */
@@ -996,9 +996,9 @@ static int flexcan_chip_start(struct net_device *dev)
 		 * and Correction of Memory Errors" to write to
 		 * MECR register
 		 */
-		reg_crl2 = flexcan_read(&regs->crl2);
-		reg_crl2 |= FLEXCAN_CRL2_ECRWRE;
-		flexcan_write(reg_crl2, &regs->crl2);
+		reg_ctrl2 = flexcan_read(&regs->ctrl2);
+		reg_ctrl2 |= FLEXCAN_CTRL2_ECRWRE;
+		flexcan_write(reg_ctrl2, &regs->ctrl2);
 
 		reg_mecr = flexcan_read(&regs->mecr);
 		reg_mecr &= ~FLEXCAN_MECR_ECRWRDIS;
-- 
1.9.1

[PATCH 03/15] can: flexcan: replace open coded mailbox code by proper defines

[David Jander]

From: Marc Kleine-Budde <mkl@pengutronix.de>

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index b287367..20f5c0e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -158,7 +158,6 @@
 	 FLEXCAN_IFLAG_BUF(FLEXCAN_TX_BUF_ID))
 
 /* FLEXCAN message buffers */
-#define FLEXCAN_MB_CNT_CODE(x)		(((x) & 0xf) << 24)
 #define FLEXCAN_MB_CODE_RX_INACTIVE	(0x0 << 24)
 #define FLEXCAN_MB_CODE_RX_EMPTY	(0x4 << 24)
 #define FLEXCAN_MB_CODE_RX_FULL		(0x2 << 24)
@@ -479,7 +478,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->base;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	u32 can_id;
-	u32 ctrl = FLEXCAN_MB_CNT_CODE(0xc) | (cf->can_dlc << 16);
+	u32 ctrl = FLEXCAN_MB_CODE_TX_DATA | (cf->can_dlc << 16);
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
-- 
1.9.1

[PATCH 04/15] can: flexcan: 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 | 322 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 241 insertions(+), 81 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 20f5c0e..8118f35 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>
  *
@@ -24,6 +25,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/circ_buf.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/if_arp.h>
@@ -40,8 +42,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)
@@ -63,6 +65,18 @@
 #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
 #define FLEXCAN_MCR_AEN			BIT(12)
 #define FLEXCAN_MCR_MAXMB(x)		((x) & 0x7f)
+/*
+ * MCR_MAXMB:
+ * This field is 7 bits wide on V10 and newer flexcan cores, but the
+ * documentation of older versions states this field as being only 6 bits wide.
+ * This contradicts the fact that those cores still have 64 MB's, because with
+ * a 6-bit field the MAXMB value can never be higher than 63 (0x3f) which would
+ * implicate that the last MB could never be used. Since we cannot be sure
+ * whether just the documentation was wrong or older versions of the flexcan
+ * core can indeed not use the last MB, we chose the safest action and make
+ * this driver only use 63 MB's in every case for simplicity.
+ */
+#define FLEXCAN_MCR_MAXMB_USED		(0x3f)
 #define FLEXCAN_MCR_IDAM_A		(0 << 8)
 #define FLEXCAN_MCR_IDAM_B		(1 << 8)
 #define FLEXCAN_MCR_IDAM_C		(2 << 8)
@@ -113,6 +127,9 @@
 #define FLEXCAN_MECR_ECCDIS		BIT(8)
 #define FLEXCAN_MECR_NCEFAFRZ		BIT(7)
 
+/* 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)
@@ -147,17 +164,16 @@
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
-#define FLEXCAN_TX_BUF_RESERVED		8
-#define FLEXCAN_TX_BUF_ID		9
+#define FLEXCAN_TX_BUF_RESERVED		0
+#define FLEXCAN_TX_BUF_ID		1
+#define FLEXCAN_RX_AREA_START		(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_CODE_MASK		(0xf << 24)
+#define FLEXCAN_MB_CODE_RX_BUSY_BIT	(0x1 << 24)
 #define FLEXCAN_MB_CODE_RX_INACTIVE	(0x0 << 24)
 #define FLEXCAN_MB_CODE_RX_EMPTY	(0x4 << 24)
 #define FLEXCAN_MB_CODE_RX_FULL		(0x2 << 24)
@@ -175,8 +191,6 @@
 #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)
 
 /*
@@ -239,7 +253,9 @@ struct flexcan_regs {
 	 *			  	size conf'ed via ctrl2::RFFN
 	 *				(mx6, vf610)
 	 */
-	u32 _reserved4[408];
+	u32 _reserved4[256];	/* 0x480 */
+	u32 rximr[64];		/* 0x880 */
+	u32 _reserved5[88];	/* 0x980 */
 	u32 mecr;		/* 0xae0 */
 	u32 erriar;		/* 0xae4 */
 	u32 erridpr;		/* 0xae8 */
@@ -268,6 +284,10 @@ 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];
+	bool second_first;
+	unsigned int rx_head;
+	unsigned int rx_tail;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {
@@ -697,16 +717,37 @@ 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 = ACCESS_ONCE(mb->can_ctrl);
+	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+
+	/* is this MB empty? */
+	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
+	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
+		return 0;
+
+	if (code == FLEXCAN_MB_CODE_RX_OVERRRUN) {
+		/* This MB was overrun, we lost data */
+		priv->dev->stats.rx_over_errors++;
+		priv->dev->stats.rx_errors++;
+	}
+
+	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
@@ -716,27 +757,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]));
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(mb->data[0]);
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(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;
-
-	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++;
@@ -750,10 +773,13 @@ 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 ret;
+	unsigned int head;
+	unsigned int tail;
 
 	/*
 	 * The error bits are cleared on read,
@@ -764,38 +790,173 @@ 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 */
+	head = smp_load_acquire(&priv->rx_head);
+	tail = priv->rx_tail;
+	while ((CIRC_CNT(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) &&
+			(work_done < quota)) {
+		ret = flexcan_read_frame(dev, tail);
+		work_done += ret;
+		tail = (tail + 1) & (ARRAY_SIZE(priv->cantxfg_copy) -1);
+		smp_store_release(&priv->rx_tail, tail);
 	}
 
 	/* report bus errors */
 	if (flexcan_has_and_handle_berr(priv, reg_esr) && work_done < quota)
 		work_done += flexcan_poll_bus_err(dev, reg_esr);
 
-	if (work_done < 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);
-	}
 
 	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;
+	unsigned int head;
+	unsigned int tail;
+
+	mb = &regs->cantxfg[i];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+
+	while (code & FLEXCAN_MB_CODE_RX_BUSY_BIT) {
+		/* MB busy, shouldn't take long */
+		reg_ctrl = flexcan_read(&mb->can_ctrl);
+		code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+	}
+
+	/* No need to process if neither FULL nor OVERRRUN */
+	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
+	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
+		return code;
+
+	head = priv->rx_head;
+	tail = ACCESS_ONCE(priv->rx_tail);
+	if (CIRC_SPACE(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) {
+		/* Copy contents */
+		dst = &priv->cantxfg_copy[priv->rx_head];
+		dst->can_id = flexcan_read(&mb->can_id);
+		dst->data[0] = flexcan_read(&mb->data[0]);
+		dst->data[1] = flexcan_read(&mb->data[1]);
+		dst->can_ctrl = reg_ctrl;
+
+		smp_store_release(&priv->rx_head,
+			(head + 1) & (ARRAY_SIZE(priv->cantxfg_copy) - 1));
+	} else {
+		/* Buffer space overrun, we lost data */
+		priv->dev->stats.rx_over_errors++;
+		priv->dev->stats.rx_errors++;
+	}
+
+	/* Clear the interrupt flag and lock MB */
+	if (i < 32)
+		flexcan_write(BIT(i), &regs->iflag1);
+	else
+		flexcan_write(BIT(i - 32), &regs->iflag2);
+	flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE, &mb->can_ctrl);
+
+	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;
+	if (code == FLEXCAN_MB_CODE_RX_INACTIVE)
+		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY, &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;
+	unsigned int code;
+
+	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 = FLEXCAN_RX_AREA_START; i < 64; i++) {
+		code = flexcan_copy_one_rxmb(priv, i);
+		if (code == FLEXCAN_MB_CODE_RX_EMPTY)
+			break;
+	}
+
+	if ((i >= 32) && priv->second_first)
+		netdev_warn(priv->dev, "RX order cannot be guaranteed. count=%d\n", i);
+
+	priv->second_first = false;
+
+	/* No EMPTY MB in first half? */
+	if (i >= 32) {
+		/* Re-enable all disabled MBs */
+		for(i = FLEXCAN_RX_AREA_START; i < 64; i++)
+			flexcan_unlock_if_locked(priv, i);
+
+		/* Next time we need to check the second half first */
+		priv->second_first = true;
+	}
+
+	/* 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);
@@ -806,7 +967,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)) {
 		/*
@@ -814,20 +975,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);
@@ -920,11 +1071,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(FLEXCAN_MCR_MAXMB_USED);
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -960,28 +1111,36 @@ 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 = FLEXCAN_TX_BUF_ID; i < ARRAY_SIZE(regs->cantxfg); i++) {
-		flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE,
-			      &regs->cantxfg[i].can_ctrl);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* CTRL2: Enable EACEN */
+		reg_ctrl2 = flexcan_read(&regs->ctrl2);
+		reg_ctrl2 |= FLEXCAN_CTRL2_EACEN | FLEXCAN_CTRL2_RRS;
+		flexcan_write(reg_ctrl2, &regs->ctrl2);
 	}
 
+	/* Prepare RX mailboxes */
+	for (i = FLEXCAN_RX_AREA_START; i < ARRAY_SIZE(regs->cantxfg); i++) {
+		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY,
+		      &regs->cantxfg[i].can_ctrl);
+		flexcan_write(0, &regs->rximr[i]); /* Clear filter */
+	}
+
+	/* Prepare TX mailbox */
+	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+		&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+
 	/* Errata ERR005829: mark first TX mailbox as INACTIVE */
 	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
 		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
 
-	/* mark TX mailbox as INACTIVE */
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		      &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	priv->second_first = false;
+	priv->rx_head = priv->rx_tail = 0;
 
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
 	flexcan_write(0x0, &regs->rx14mask);
 	flexcan_write(0x0, &regs->rx15mask);
 
-	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
-		flexcan_write(0x0, &regs->rxfgmask);
-
 	/*
 	 * On Vybrid, disable memory error detection interrupts
 	 * and freeze mode.
@@ -1018,8 +1177,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 05/15] can: dev: add preliminary rx-fifo

[David Jander]

From: Marc Kleine-Budde <mkl@pengutronix.de>

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c   | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h |  25 ++++++++++
 2 files changed, 145 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 02492d2..c1e53e9 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -273,6 +273,126 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	return err;
 }
 
+static bool can_rx_fifo_ge(struct can_rx_fifo *fifo, unsigned int a, unsigned int b)
+{
+	if (fifo->inc)
+		return a >= b;
+	else
+		return a <= b;
+}
+
+static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
+{
+	if (fifo->inc)
+		return (*val)++;
+	else
+		return (*val)--;
+}
+
+static u32 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
+{
+	if (fifo->inc)
+		return ~0U >> (32 + fifo->low_first - fifo->high_first) << fifo->low_first;
+	else
+		return ~0U >> (32 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
+}
+
+static u32 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
+{
+	if (fifo->inc)
+		return ~0U >> (32 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
+	else
+		return ~0U >> (32 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
+}
+
+int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
+{
+	fifo->dev = dev;
+
+	if ((fifo->low_first < fifo->high_first) &&
+	    (fifo->high_first < fifo->high_last))
+		fifo->inc = true;
+	else if ((fifo->low_first > fifo->high_first) &&
+		 (fifo->high_first > fifo->high_last))
+		fifo->inc = false;
+	else
+		return -EINVAL;
+
+	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
+	    !fifo->mailbox_disable || !fifo->mailbox_receive)
+		return -EINVAL;
+
+	/* init variables */
+	fifo->mask_low = can_rx_fifo_mask_low(fifo);
+	fifo->mask_high = can_rx_fifo_mask_high(fifo);
+	fifo->next = fifo->low_first;
+	fifo->active = fifo->mask_low | fifo->mask_high;
+	fifo->mailbox_enable_mask(fifo, fifo->active);
+
+	netdev_dbg(dev, "%s: low_first=%d, high_first=%d, high_last=%d\n", __func__,
+		   fifo->low_first, fifo->high_first, fifo->high_last);
+	netdev_dbg(dev, "%s: mask_low=0x%08x mask_high=0x%08x\n", __func__,
+		   fifo->mask_low, fifo->mask_high);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_add);
+
+int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
+{
+	int received = 0;
+	u32 pending;
+	unsigned int mb;
+
+	do {
+		pending = fifo->read_pending(fifo);
+		pending &= fifo->active;
+
+		if (!(pending & BIT(fifo->next))) {
+			/*
+			 * Wrap around only if:
+			 * - we are in the upper group and
+			 * - there is a CAN frame in the first mailbox
+			 *   of the lower group.
+			 */
+			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
+			    (pending & BIT(fifo->low_first))) {
+				fifo->next = fifo->low_first;
+
+				fifo->active |= fifo->mask_high;
+				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
+			} else {
+				break;
+			}
+		}
+
+		mb = can_rx_fifo_inc(fifo, &fifo->next);
+
+		/* disable mailbox */
+		fifo->active &= ~BIT(mb);
+		fifo->mailbox_disable(fifo, mb);
+
+		fifo->mailbox_receive(fifo, mb);
+
+		if (fifo->next == fifo->high_first) {
+			fifo->active |= fifo->mask_low;
+			fifo->mailbox_enable_mask(fifo, fifo->mask_low);
+		}
+
+		received++;
+		quota--;
+	} while (quota);
+
+	return received;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
+
+u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
+{
+	return fifo->active;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_get_active_mb_mask);
+
 /*
  * Local echo of CAN messages
  *
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6992afc..e9468d0 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -64,6 +64,27 @@ struct can_priv {
 #endif
 };
 
+struct can_rx_fifo {
+	struct net_device *dev;
+
+	unsigned int low_first;
+	unsigned int high_first;
+	unsigned int high_last;		/* not needed during runtime */
+
+	u32 (*read_pending)(struct can_rx_fifo *rx_fifo);
+	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u32 mask);
+	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+
+	u32 mask_low;
+	u32 mask_high;
+	u32 active;
+
+	unsigned int next;
+
+	bool inc;
+};
+
 /*
  * get_can_dlc(value) - helper macro to cast a given data length code (dlc)
  * to __u8 and ensure the dlc value to be max. 8 bytes.
@@ -105,6 +126,10 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
+int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
+u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
+
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
 
-- 
1.9.1

[PATCH 06/15] can: rx-fifo: Increase MB size limit from 32 to 64

[David Jander]

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 24 ++++++++++++------------
 include/linux/can/dev.h | 12 ++++++------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c1e53e9..930b9f4 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -289,20 +289,20 @@ static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
 		return (*val)--;
 }
 
-static u32 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
+static u64 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0U >> (32 + fifo->low_first - fifo->high_first) << fifo->low_first;
+		return ~0LLU >> (64 + fifo->low_first - fifo->high_first) << fifo->low_first;
 	else
-		return ~0U >> (32 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
+		return ~0LLU >> (64 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
 }
 
-static u32 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
+static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0U >> (32 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
+		return ~0LLU >> (64 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
 	else
-		return ~0U >> (32 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
+		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
 }
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
@@ -331,7 +331,7 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 
 	netdev_dbg(dev, "%s: low_first=%d, high_first=%d, high_last=%d\n", __func__,
 		   fifo->low_first, fifo->high_first, fifo->high_last);
-	netdev_dbg(dev, "%s: mask_low=0x%08x mask_high=0x%08x\n", __func__,
+	netdev_dbg(dev, "%s: mask_low=0x%016llx mask_high=0x%016llx\n", __func__,
 		   fifo->mask_low, fifo->mask_high);
 
 	return 0;
@@ -341,14 +341,14 @@ EXPORT_SYMBOL_GPL(can_rx_fifo_add);
 int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 {
 	int received = 0;
-	u32 pending;
+	u64 pending;
 	unsigned int mb;
 
 	do {
 		pending = fifo->read_pending(fifo);
 		pending &= fifo->active;
 
-		if (!(pending & BIT(fifo->next))) {
+		if (!(pending & BIT_ULL(fifo->next))) {
 			/*
 			 * Wrap around only if:
 			 * - we are in the upper group and
@@ -356,7 +356,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 			 *   of the lower group.
 			 */
 			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
-			    (pending & BIT(fifo->low_first))) {
+			    (pending & BIT_ULL(fifo->low_first))) {
 				fifo->next = fifo->low_first;
 
 				fifo->active |= fifo->mask_high;
@@ -369,7 +369,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 		mb = can_rx_fifo_inc(fifo, &fifo->next);
 
 		/* disable mailbox */
-		fifo->active &= ~BIT(mb);
+		fifo->active &= ~BIT_ULL(mb);
 		fifo->mailbox_disable(fifo, mb);
 
 		fifo->mailbox_receive(fifo, mb);
@@ -387,7 +387,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
 
-u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
+u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
 {
 	return fifo->active;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index e9468d0..ed46f7d 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -71,14 +71,14 @@ struct can_rx_fifo {
 	unsigned int high_first;
 	unsigned int high_last;		/* not needed during runtime */
 
-	u32 (*read_pending)(struct can_rx_fifo *rx_fifo);
-	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u32 mask);
+	u64 (*read_pending)(struct can_rx_fifo *rx_fifo);
+	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
 	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 
-	u32 mask_low;
-	u32 mask_high;
-	u32 active;
+	u64 mask_low;
+	u64 mask_high;
+	u64 active;
 
 	unsigned int next;
 
@@ -128,7 +128,7 @@ u8 can_len2dlc(u8 len);
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
 int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
-u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
+u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
 
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
-- 
1.9.1

Re: [PATCH 06/15] can: rx-fifo: Increase MB size limit from 32 to 64

[Marc Kleine-Budde]

On Fri, Oct 10, 2014 at 05:46:51PM +0200, David Jander wrote:
> Signed-off-by: David Jander <david@protonic.nl>

Please move to the beginning of the series, so that I can squash all fifo
related patches into one. So that we have just a single (hopefully perfect)
rx-fifo patch.

regards,
Marc

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 06/15] can: rx-fifo: Increase MB size limit from 32 to 64

[David Jander]

On Sun, 19 Oct 2014 23:25:50 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On Fri, Oct 10, 2014 at 05:46:51PM +0200, David Jander wrote:
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> Please move to the beginning of the series, so that I can squash all fifo
> related patches into one. So that we have just a single (hopefully perfect)
> rx-fifo patch.

Ok, will reorder the patches.

Best regards,

-- 
David Jander
Protonic Holland.

[PATCH 07/15] can: rx-fifo: Change to do controller off-load in interrupt and NAPI poll

[David Jander]

The idea is to use rx-fifo from interrupt context and take away the need
for NAPI polling from the driver. Currently no support for error-handling
is included.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 213 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/can/dev.h |  21 +++--
 2 files changed, 180 insertions(+), 54 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 930b9f4..22a3955 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -26,6 +26,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/netlink.h>
 #include <linux/can/led.h>
+#include <linux/circ_buf.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -281,6 +282,14 @@ static bool can_rx_fifo_ge(struct can_rx_fifo *fifo, unsigned int a, unsigned in
 		return a <= b;
 }
 
+static bool can_rx_fifo_le(struct can_rx_fifo *fifo, unsigned int a, unsigned int b)
+{
+	if (fifo->inc)
+		return a <= b;
+	else
+		return a >= b;
+}
+
 static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
 {
 	if (fifo->inc)
@@ -305,27 +314,100 @@ static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
 		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
 }
 
+static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int index)
+{
+	struct net_device *dev = fifo->dev;
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	memcpy(cf, &fifo->ring[index], sizeof(*cf));
+
+	netif_receive_skb(skb);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	can_led_event(dev, CAN_LED_EVENT_RX);
+
+	return 1;
+}
+
+static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
+{
+	struct can_rx_fifo *fifo = container_of(napi, struct can_rx_fifo, napi);
+	int work_done = 0;
+	int ret;
+	unsigned int head;
+	unsigned int tail;
+
+restart_poll:
+	/* handle mailboxes */
+	head = smp_load_acquire(&fifo->ring_head);
+	tail = fifo->ring_tail;
+	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
+			(work_done < quota)) {
+		ret = can_rx_fifo_read_napi_frame(fifo, tail);
+		work_done += ret;
+		tail = (tail + 1) & (fifo->ring_size -1);
+		smp_store_release(&fifo->ring_tail, tail);
+	}
+
+	if (work_done < quota) {
+		napi_complete(napi);
+
+		/* Check if there was another interrupt */
+		head = smp_load_acquire(&fifo->ring_head);
+		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
+		    napi_reschedule(&fifo->napi))
+			goto restart_poll;
+	}
+
+	return work_done;
+}
+
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 {
+	unsigned int weight;
 	fifo->dev = dev;
 
 	if ((fifo->low_first < fifo->high_first) &&
-	    (fifo->high_first < fifo->high_last))
+	    (fifo->high_first < fifo->high_last)) {
 		fifo->inc = true;
-	else if ((fifo->low_first > fifo->high_first) &&
-		 (fifo->high_first > fifo->high_last))
+		weight = fifo->high_last - fifo->low_first;
+	} else if ((fifo->low_first > fifo->high_first) &&
+		 (fifo->high_first > fifo->high_last)) {
 		fifo->inc = false;
-	else
+		weight = fifo->low_first - fifo->high_last;
+	} else
 		return -EINVAL;
 
-	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
-	    !fifo->mailbox_disable || !fifo->mailbox_receive)
+	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
+	    !fifo->mailbox_enable)
 		return -EINVAL;
 
+	/* Make ring-buffer a sensible size that is a power of 2 */
+	fifo->ring_size = (2 << fls(weight));
+	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
+			     GFP_KERNEL);
+	if (!fifo->ring)
+		return -ENOMEM;
+
+	fifo->ring_head = fifo->ring_tail = 0;
+
+	/* Take care of NAPI handling */
+	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
+
 	/* init variables */
 	fifo->mask_low = can_rx_fifo_mask_low(fifo);
 	fifo->mask_high = can_rx_fifo_mask_high(fifo);
-	fifo->next = fifo->low_first;
+	fifo->second_first = false;
 	fifo->active = fifo->mask_low | fifo->mask_high;
 	fifo->mailbox_enable_mask(fifo, fifo->active);
 
@@ -338,60 +420,95 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_add);
 
-int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
+static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo, unsigned int n)
+{
+	unsigned int head = fifo->ring_head;
+	unsigned int tail = ACCESS_ONCE(fifo->ring_tail);
+	unsigned int ret = 0;
+
+	if (CIRC_SPACE(head, tail, fifo->ring_size) >= 1) {
+		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->ring[head], n);
+		if (ret)
+			smp_store_release(&fifo->ring_head,
+				(head + 1) & (fifo->ring_size - 1));
+	} else {
+		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->overflow, n);
+		if (ret)
+			fifo->dev->stats.rx_dropped++;
+	}
+	return ret;
+}
+
+int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
 {
-	int received = 0;
-	u64 pending;
-	unsigned int mb;
-
-	do {
-		pending = fifo->read_pending(fifo);
-		pending &= fifo->active;
-
-		if (!(pending & BIT_ULL(fifo->next))) {
-			/*
-			 * Wrap around only if:
-			 * - we are in the upper group and
-			 * - there is a CAN frame in the first mailbox
-			 *   of the lower group.
-			 */
-			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
-			    (pending & BIT_ULL(fifo->low_first))) {
-				fifo->next = fifo->low_first;
-
-				fifo->active |= fifo->mask_high;
-				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
-			} else {
-				break;
-			}
+	unsigned int i;
+	unsigned int ret;
+	unsigned int received = 0;
+
+	if (fifo->second_first) {
+		for (i = fifo->high_first;
+		     can_rx_fifo_le(fifo, i, fifo->high_last);
+		     can_rx_fifo_inc(fifo, &i)) {
+			received += can_rx_fifo_offload_if_full(fifo, i);
+			fifo->active |= BIT_ULL(i);
+			fifo->mailbox_enable(fifo, i);
 		}
+	}
 
-		mb = can_rx_fifo_inc(fifo, &fifo->next);
+	/* Copy and disable FULL MBs */
+	for (i = fifo->low_first; can_rx_fifo_le(fifo, i, fifo->high_last);
+			can_rx_fifo_inc(fifo, &i)) {
+		if (!(fifo->active & BIT_ULL(i)))
+			continue;
+		ret = can_rx_fifo_offload_if_full(fifo, i);
+		if (!ret)
+			break;
+		received += ret;
+		fifo->active &= ~BIT_ULL(i);
+	}
 
-		/* disable mailbox */
-		fifo->active &= ~BIT_ULL(mb);
-		fifo->mailbox_disable(fifo, mb);
+	if (can_rx_fifo_ge(fifo, i, fifo->high_first) && fifo->second_first)
+		netdev_warn(fifo->dev, "%s: RX order cannot be guaranteed."
+			" (count=%d)\n", __func__, i);
 
-		fifo->mailbox_receive(fifo, mb);
+	fifo->second_first = false;
 
-		if (fifo->next == fifo->high_first) {
-			fifo->active |= fifo->mask_low;
-			fifo->mailbox_enable_mask(fifo, fifo->mask_low);
-		}
+	/* No EMPTY MB in first half? */
+	if (can_rx_fifo_ge(fifo, i, fifo->high_first)) {
+		/* Re-enable all disabled MBs */
+		fifo->active = fifo->mask_low | fifo->mask_high;
+		fifo->mailbox_enable_mask(fifo, fifo->active);
+
+		/* Next time we need to check the second half first */
+		fifo->second_first = true;
+	}
 
-		received++;
-		quota--;
-	} while (quota);
+	if (received)
+		napi_schedule(&fifo->napi);
 
 	return received;
 }
-EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
+EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
+
+void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
+{
+	napi_enable(&fifo->napi);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_napi_enable);
+
+void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo)
+{
+	napi_disable(&fifo->napi);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_napi_disable);
 
-u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
+void can_rx_fifo_del(struct can_rx_fifo *fifo)
 {
-	return fifo->active;
+	if (fifo->ring)
+		kfree(fifo->ring);
+	netif_napi_del(&fifo->napi);
 }
-EXPORT_SYMBOL_GPL(can_rx_fifo_get_active_mb_mask);
+EXPORT_SYMBOL_GPL(can_rx_fifo_del);
 
 /*
  * Local echo of CAN messages
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index ed46f7d..64a8de3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -71,18 +71,25 @@ struct can_rx_fifo {
 	unsigned int high_first;
 	unsigned int high_last;		/* not needed during runtime */
 
-	u64 (*read_pending)(struct can_rx_fifo *rx_fifo);
 	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
-	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
-	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+	unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
+		struct can_frame *frame, unsigned int mb);
 
 	u64 mask_low;
 	u64 mask_high;
 	u64 active;
 
-	unsigned int next;
+	unsigned int second_first;
 
 	bool inc;
+
+	struct can_frame *ring;
+	struct can_frame overflow;
+	size_t ring_size;
+	unsigned int ring_head;
+	unsigned int ring_tail;
+	struct napi_struct napi;
 };
 
 /*
@@ -127,8 +134,10 @@ u8 can_dlc2len(u8 can_dlc);
 u8 can_len2dlc(u8 len);
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
-int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
-u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
+int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
+void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
+void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
+void can_rx_fifo_del(struct can_rx_fifo *fifo);
 
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
-- 
1.9.1

Re: [PATCH 07/15] can: rx-fifo: Change to do controller off-load in interrupt and NAPI poll

[Marc Kleine-Budde]

On Fri, Oct 10, 2014 at 05:46:52PM +0200, David Jander wrote:
> The idea is to use rx-fifo from interrupt context and take away the need
> for NAPI polling from the driver. Currently no support for error-handling
> is included.

Not a complete review but, at least a start. See comments inline.

> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/net/can/dev.c   | 213 +++++++++++++++++++++++++++++++++++++-----------
>  include/linux/can/dev.h |  21 +++--
>  2 files changed, 180 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 930b9f4..22a3955 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -26,6 +26,7 @@
>  #include <linux/can/skb.h>
>  #include <linux/can/netlink.h>
>  #include <linux/can/led.h>
> +#include <linux/circ_buf.h>
>  #include <net/rtnetlink.h>
>  
>  #define MOD_DESC "CAN device driver interface"
> @@ -281,6 +282,14 @@ static bool can_rx_fifo_ge(struct can_rx_fifo *fifo, unsigned int a, unsigned in
>  		return a <= b;
>  }
>  
> +static bool can_rx_fifo_le(struct can_rx_fifo *fifo, unsigned int a, unsigned int b)
> +{
> +	if (fifo->inc)
> +		return a <= b;
> +	else
> +		return a >= b;
> +}
> +
>  static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
>  {
>  	if (fifo->inc)
> @@ -305,27 +314,100 @@ static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
>  		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
>  }
>  
> +static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int index)
> +{
> +	struct net_device *dev = fifo->dev;
> +	struct net_device_stats *stats = &dev->stats;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +
> +	skb = alloc_can_skb(dev, &cf);
> +	if (unlikely(!skb)) {
> +		stats->rx_dropped++;
> +		return 0;
> +	}
> +
> +	memcpy(cf, &fifo->ring[index], sizeof(*cf));
> +
> +	netif_receive_skb(skb);
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;

cf may not be valid after netif_receive_skb() anymore. Please so the stats
before calling it.

> +
> +	can_led_event(dev, CAN_LED_EVENT_RX);

Please call can_led_event only once per napi invocation.

> +
> +	return 1;
> +}
> +
> +static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
> +{
> +	struct can_rx_fifo *fifo = container_of(napi, struct can_rx_fifo, napi);
> +	int work_done = 0;
> +	int ret;
> +	unsigned int head;
> +	unsigned int tail;
> +
> +restart_poll:
> +	/* handle mailboxes */
> +	head = smp_load_acquire(&fifo->ring_head);
> +	tail = fifo->ring_tail;
> +	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> +			(work_done < quota)) {
> +		ret = can_rx_fifo_read_napi_frame(fifo, tail);
> +		work_done += ret;
> +		tail = (tail + 1) & (fifo->ring_size -1);
> +		smp_store_release(&fifo->ring_tail, tail);
> +	}
> +
> +	if (work_done < quota) {
> +		napi_complete(napi);
> +
> +		/* Check if there was another interrupt */
> +		head = smp_load_acquire(&fifo->ring_head);
> +		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> +		    napi_reschedule(&fifo->napi))
> +			goto restart_poll;

Hmmm, this looks a bit strange. If I understand the code correctly you ask that
napi should be started again, but then jump directly to the beginning.

> +	}
> +
> +	return work_done;
> +}
> +
>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  {
> +	unsigned int weight;
>  	fifo->dev = dev;
>  
>  	if ((fifo->low_first < fifo->high_first) &&
> -	    (fifo->high_first < fifo->high_last))
> +	    (fifo->high_first < fifo->high_last)) {
>  		fifo->inc = true;
> -	else if ((fifo->low_first > fifo->high_first) &&
> -		 (fifo->high_first > fifo->high_last))
> +		weight = fifo->high_last - fifo->low_first;
> +	} else if ((fifo->low_first > fifo->high_first) &&
> +		 (fifo->high_first > fifo->high_last)) {
>  		fifo->inc = false;
> -	else
> +		weight = fifo->low_first - fifo->high_last;
> +	} else
>  		return -EINVAL;

Please but { } at every branch of the if else.

>  
> -	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
> -	    !fifo->mailbox_disable || !fifo->mailbox_receive)
> +	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
> +	    !fifo->mailbox_enable)
>  		return -EINVAL;
>  
> +	/* Make ring-buffer a sensible size that is a power of 2 */
> +	fifo->ring_size = (2 << fls(weight));
> +	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
> +			     GFP_KERNEL);
> +	if (!fifo->ring)
> +		return -ENOMEM;
> +
> +	fifo->ring_head = fifo->ring_tail = 0;
> +
> +	/* Take care of NAPI handling */
> +	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);

I'm not sure, if the rx-fifo should take care of the whole NAPI, I think it's
better to provide helper functions instead.

> +
>  	/* init variables */
>  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
>  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
> -	fifo->next = fifo->low_first;
> +	fifo->second_first = false;
>  	fifo->active = fifo->mask_low | fifo->mask_high;
>  	fifo->mailbox_enable_mask(fifo, fifo->active);
>  
> @@ -338,60 +420,95 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_fifo_add);
>  
> -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
> +static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo, unsigned int n)
> +{
> +	unsigned int head = fifo->ring_head;
> +	unsigned int tail = ACCESS_ONCE(fifo->ring_tail);
> +	unsigned int ret = 0;
> +
> +	if (CIRC_SPACE(head, tail, fifo->ring_size) >= 1) {
> +		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->ring[head], n);
> +		if (ret)
> +			smp_store_release(&fifo->ring_head,
> +				(head + 1) & (fifo->ring_size - 1));
> +	} else {
> +		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->overflow, n);
> +		if (ret)
> +			fifo->dev->stats.rx_dropped++;
> +	}

That's the purpose of the overflow mailbox? fifo-> overflow seems to be write
only?

> +	return ret;
> +}
> +
> +int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
>  {
> -	int received = 0;
> -	u64 pending;
> -	unsigned int mb;
> -
> -	do {
> -		pending = fifo->read_pending(fifo);
> -		pending &= fifo->active;
> -
> -		if (!(pending & BIT_ULL(fifo->next))) {
> -			/*
> -			 * Wrap around only if:
> -			 * - we are in the upper group and
> -			 * - there is a CAN frame in the first mailbox
> -			 *   of the lower group.
> -			 */
> -			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
> -			    (pending & BIT_ULL(fifo->low_first))) {
> -				fifo->next = fifo->low_first;
> -
> -				fifo->active |= fifo->mask_high;
> -				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
> -			} else {
> -				break;
> -			}
> +	unsigned int i;
> +	unsigned int ret;
> +	unsigned int received = 0;
> +
> +	if (fifo->second_first) {
> +		for (i = fifo->high_first;
> +		     can_rx_fifo_le(fifo, i, fifo->high_last);
> +		     can_rx_fifo_inc(fifo, &i)) {
> +			received += can_rx_fifo_offload_if_full(fifo, i);
> +			fifo->active |= BIT_ULL(i);
> +			fifo->mailbox_enable(fifo, i);
>  		}
> +	}
>  
> -		mb = can_rx_fifo_inc(fifo, &fifo->next);
> +	/* Copy and disable FULL MBs */
> +	for (i = fifo->low_first; can_rx_fifo_le(fifo, i, fifo->high_last);
> +			can_rx_fifo_inc(fifo, &i)) {
> +		if (!(fifo->active & BIT_ULL(i)))
> +			continue;
> +		ret = can_rx_fifo_offload_if_full(fifo, i);
> +		if (!ret)
> +			break;
> +		received += ret;
> +		fifo->active &= ~BIT_ULL(i);
> +	}
>  
> -		/* disable mailbox */
> -		fifo->active &= ~BIT_ULL(mb);
> -		fifo->mailbox_disable(fifo, mb);
> +	if (can_rx_fifo_ge(fifo, i, fifo->high_first) && fifo->second_first)
> +		netdev_warn(fifo->dev, "%s: RX order cannot be guaranteed."
> +			" (count=%d)\n", __func__, i);
>  
> -		fifo->mailbox_receive(fifo, mb);
> +	fifo->second_first = false;
>  
> -		if (fifo->next == fifo->high_first) {
> -			fifo->active |= fifo->mask_low;
> -			fifo->mailbox_enable_mask(fifo, fifo->mask_low);
> -		}
> +	/* No EMPTY MB in first half? */
> +	if (can_rx_fifo_ge(fifo, i, fifo->high_first)) {
> +		/* Re-enable all disabled MBs */
> +		fifo->active = fifo->mask_low | fifo->mask_high;
> +		fifo->mailbox_enable_mask(fifo, fifo->active);
> +
> +		/* Next time we need to check the second half first */
> +		fifo->second_first = true;
> +	}
>  
> -		received++;
> -		quota--;
> -	} while (quota);
> +	if (received)
> +		napi_schedule(&fifo->napi);
>  
>  	return received;
>  }
> -EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
> +EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
> +
> +void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
> +{
> +	napi_enable(&fifo->napi);
> +}
> +EXPORT_SYMBOL_GPL(can_rx_fifo_napi_enable);
> +
> +void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo)
> +{
> +	napi_disable(&fifo->napi);
> +}
> +EXPORT_SYMBOL_GPL(can_rx_fifo_napi_disable);
>  
> -u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
> +void can_rx_fifo_del(struct can_rx_fifo *fifo)
>  {
> -	return fifo->active;
> +	if (fifo->ring)
> +		kfree(fifo->ring);

kfree() can be called with NULL

> +	netif_napi_del(&fifo->napi);
>  }
> -EXPORT_SYMBOL_GPL(can_rx_fifo_get_active_mb_mask);
> +EXPORT_SYMBOL_GPL(can_rx_fifo_del);
>  
>  /*
>   * Local echo of CAN messages
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index ed46f7d..64a8de3 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -71,18 +71,25 @@ struct can_rx_fifo {
>  	unsigned int high_first;
>  	unsigned int high_last;		/* not needed during runtime */
>  
> -	u64 (*read_pending)(struct can_rx_fifo *rx_fifo);
>  	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
> -	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
> -	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
> +	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
> +	unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
> +		struct can_frame *frame, unsigned int mb);
>  
>  	u64 mask_low;
>  	u64 mask_high;
>  	u64 active;
>  
> -	unsigned int next;
> +	unsigned int second_first;

The rest of the code talks about low and high, what about naming this variable,
high_first?

>  
>  	bool inc;
> +
> +	struct can_frame *ring;
> +	struct can_frame overflow;
> +	size_t ring_size;
> +	unsigned int ring_head;
> +	unsigned int ring_tail;
> +	struct napi_struct napi;
>  };
>  
>  /*
> @@ -127,8 +134,10 @@ u8 can_dlc2len(u8 can_dlc);
>  u8 can_len2dlc(u8 len);
>  
>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
> -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
> -u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
> +int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
> +void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
> +void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
> +void can_rx_fifo_del(struct can_rx_fifo *fifo);
>  
>  struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
>  void free_candev(struct net_device *dev);

Marc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 07/15] can: rx-fifo: Change to do controller off-load in interrupt and NAPI poll

[David Jander]

On Mon, 20 Oct 2014 00:09:09 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On Fri, Oct 10, 2014 at 05:46:52PM +0200, David Jander wrote:
> > The idea is to use rx-fifo from interrupt context and take away the need
> > for NAPI polling from the driver. Currently no support for error-handling
> > is included.
> 
> Not a complete review but, at least a start. See comments inline.

Thanks a lot.

> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/net/can/dev.c   | 213
> > +++++++++++++++++++++++++++++++++++++----------- include/linux/can/dev.h
> > |  21 +++-- 2 files changed, 180 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 930b9f4..22a3955 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/can/skb.h>
> >  #include <linux/can/netlink.h>
> >  #include <linux/can/led.h>
> > +#include <linux/circ_buf.h>
> >  #include <net/rtnetlink.h>
> >  
> >  #define MOD_DESC "CAN device driver interface"
> > @@ -281,6 +282,14 @@ static bool can_rx_fifo_ge(struct can_rx_fifo *fifo,
> > unsigned int a, unsigned in return a <= b;
> >  }
> >  
> > +static bool can_rx_fifo_le(struct can_rx_fifo *fifo, unsigned int a,
> > unsigned int b) +{
> > +	if (fifo->inc)
> > +		return a <= b;
> > +	else
> > +		return a >= b;
> > +}
> > +
> >  static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned
> > int *val) {
> >  	if (fifo->inc)
> > @@ -305,27 +314,100 @@ static u64 can_rx_fifo_mask_high(struct can_rx_fifo
> > *fifo) return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) <<
> > fifo->high_last; }
> >  
> > +static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int
> > index) +{
> > +	struct net_device *dev = fifo->dev;
> > +	struct net_device_stats *stats = &dev->stats;
> > +	struct sk_buff *skb;
> > +	struct can_frame *cf;
> > +
> > +	skb = alloc_can_skb(dev, &cf);
> > +	if (unlikely(!skb)) {
> > +		stats->rx_dropped++;
> > +		return 0;
> > +	}
> > +
> > +	memcpy(cf, &fifo->ring[index], sizeof(*cf));
> > +
> > +	netif_receive_skb(skb);
> > +
> > +	stats->rx_packets++;
> > +	stats->rx_bytes += cf->can_dlc;
> 
> cf may not be valid after netif_receive_skb() anymore. Please so the stats
> before calling it.

Oops, right. Will fix that.

> 
> > +
> > +	can_led_event(dev, CAN_LED_EVENT_RX);
> 
> Please call can_led_event only once per napi invocation.

That is indeed a better idea. Will move the can_led_event() call to the end of
can_rx_fifo_napi_poll().

> > +
> > +	return 1;
> > +}
> > +
> > +static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
> > +{
> > +	struct can_rx_fifo *fifo = container_of(napi, struct can_rx_fifo,
> > napi);
> > +	int work_done = 0;
> > +	int ret;
> > +	unsigned int head;
> > +	unsigned int tail;
> > +
> > +restart_poll:
> > +	/* handle mailboxes */
> > +	head = smp_load_acquire(&fifo->ring_head);
> > +	tail = fifo->ring_tail;
> > +	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> > +			(work_done < quota)) {
> > +		ret = can_rx_fifo_read_napi_frame(fifo, tail);
> > +		work_done += ret;
> > +		tail = (tail + 1) & (fifo->ring_size -1);
> > +		smp_store_release(&fifo->ring_tail, tail);
> > +	}
> > +
> > +	if (work_done < quota) {
> > +		napi_complete(napi);
> > +
> > +		/* Check if there was another interrupt */
> > +		head = smp_load_acquire(&fifo->ring_head);
> > +		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> > +		    napi_reschedule(&fifo->napi))
> > +			goto restart_poll;
> 
> Hmmm, this looks a bit strange. If I understand the code correctly you ask
> that napi should be started again, but then jump directly to the beginning.

The documentation seems to say that one should use it like that:

http://www.linuxfoundation.org/collaborate/workgroups/networking/napi

If you still think it is wrong, then tell me how to re-enable napi and
continue correctly. AFAIK it is done like this in order to avoid a race when
the interrupt is called while NAPI polling was underway. napi_schedule() just
sets a flag, and does _not_ add work to a queue...

> > +	}
> > +
> > +	return work_done;
> > +}
> > +
> >  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
> >  {
> > +	unsigned int weight;
> >  	fifo->dev = dev;
> >  
> >  	if ((fifo->low_first < fifo->high_first) &&
> > -	    (fifo->high_first < fifo->high_last))
> > +	    (fifo->high_first < fifo->high_last)) {
> >  		fifo->inc = true;
> > -	else if ((fifo->low_first > fifo->high_first) &&
> > -		 (fifo->high_first > fifo->high_last))
> > +		weight = fifo->high_last - fifo->low_first;
> > +	} else if ((fifo->low_first > fifo->high_first) &&
> > +		 (fifo->high_first > fifo->high_last)) {
> >  		fifo->inc = false;
> > -	else
> > +		weight = fifo->low_first - fifo->high_last;
> > +	} else
> >  		return -EINVAL;
> 
> Please but { } at every branch of the if else.

Ok, will do.

> >  
> > -	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
> > -	    !fifo->mailbox_disable || !fifo->mailbox_receive)
> > +	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
> > +	    !fifo->mailbox_enable)
> >  		return -EINVAL;
> >  
> > +	/* Make ring-buffer a sensible size that is a power of 2 */
> > +	fifo->ring_size = (2 << fls(weight));
> > +	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
> > +			     GFP_KERNEL);
> > +	if (!fifo->ring)
> > +		return -ENOMEM;
> > +
> > +	fifo->ring_head = fifo->ring_tail = 0;
> > +
> > +	/* Take care of NAPI handling */
> > +	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
> 
> I'm not sure, if the rx-fifo should take care of the whole NAPI, I think it's
> better to provide helper functions instead.

Why not? We are removing the messages from the CAN controller in the IRQ
already, so why would the CAN driver have to even care about NAPI which
happens _after_ all that? Can you come up with an example where this may not
be desirable?
Can you illustrate your idea with helper functions?

> > +
> >  	/* init variables */
> >  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
> >  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
> > -	fifo->next = fifo->low_first;
> > +	fifo->second_first = false;
> >  	fifo->active = fifo->mask_low | fifo->mask_high;
> >  	fifo->mailbox_enable_mask(fifo, fifo->active);
> >  
> > @@ -338,60 +420,95 @@ int can_rx_fifo_add(struct net_device *dev, struct
> > can_rx_fifo *fifo) }
> >  EXPORT_SYMBOL_GPL(can_rx_fifo_add);
> >  
> > -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
> > +static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo,
> > unsigned int n) +{
> > +	unsigned int head = fifo->ring_head;
> > +	unsigned int tail = ACCESS_ONCE(fifo->ring_tail);
> > +	unsigned int ret = 0;
> > +
> > +	if (CIRC_SPACE(head, tail, fifo->ring_size) >= 1) {
> > +		ret = fifo->mailbox_move_to_buffer(fifo,
> > &fifo->ring[head], n);
> > +		if (ret)
> > +			smp_store_release(&fifo->ring_head,
> > +				(head + 1) & (fifo->ring_size - 1));
> > +	} else {
> > +		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->overflow,
> > n);
> > +		if (ret)
> > +			fifo->dev->stats.rx_dropped++;
> > +	}
> 
> That's the purpose of the overflow mailbox? fifo-> overflow seems to be write
> only?

Yes.
The idea is to simplify the code for the user. mailbox_move_to_buffer() should
just move the corresponding can message to the provided buffer and do all
interrupt-flag clearing and stuff the driver needs to do in order to free the
MB. Its just that in the case we don't have space in the circular buffer, I
don't want to complicate things for the driver and tell him that this message
should be discarded. Just think of fifo->overflow as a sort of /dev/null.
Of course I could just pass NULL to that function, but IMHO that's dangerous
because since it happens only very seldom, a missing check for NULL in that
function may bite you when it is already too late (i.e. the driver already hit
mainline).

> > +	return ret;
> > +}
> > +
> > +int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
> >  {
> > -	int received = 0;
> > -	u64 pending;
> > -	unsigned int mb;
> > -
> > -	do {
> > -		pending = fifo->read_pending(fifo);
> > -		pending &= fifo->active;
> > -
> > -		if (!(pending & BIT_ULL(fifo->next))) {
> > -			/*
> > -			 * Wrap around only if:
> > -			 * - we are in the upper group and
> > -			 * - there is a CAN frame in the first mailbox
> > -			 *   of the lower group.
> > -			 */
> > -			if (can_rx_fifo_ge(fifo, fifo->next,
> > fifo->high_first) &&
> > -			    (pending & BIT_ULL(fifo->low_first))) {
> > -				fifo->next = fifo->low_first;
> > -
> > -				fifo->active |= fifo->mask_high;
> > -				fifo->mailbox_enable_mask(fifo,
> > fifo->mask_high);
> > -			} else {
> > -				break;
> > -			}
> > +	unsigned int i;
> > +	unsigned int ret;
> > +	unsigned int received = 0;
> > +
> > +	if (fifo->second_first) {
> > +		for (i = fifo->high_first;
> > +		     can_rx_fifo_le(fifo, i, fifo->high_last);
> > +		     can_rx_fifo_inc(fifo, &i)) {
> > +			received += can_rx_fifo_offload_if_full(fifo, i);
> > +			fifo->active |= BIT_ULL(i);
> > +			fifo->mailbox_enable(fifo, i);
> >  		}
> > +	}
> >  
> > -		mb = can_rx_fifo_inc(fifo, &fifo->next);
> > +	/* Copy and disable FULL MBs */
> > +	for (i = fifo->low_first; can_rx_fifo_le(fifo, i,
> > fifo->high_last);
> > +			can_rx_fifo_inc(fifo, &i)) {
> > +		if (!(fifo->active & BIT_ULL(i)))
> > +			continue;
> > +		ret = can_rx_fifo_offload_if_full(fifo, i);
> > +		if (!ret)
> > +			break;
> > +		received += ret;
> > +		fifo->active &= ~BIT_ULL(i);
> > +	}
> >  
> > -		/* disable mailbox */
> > -		fifo->active &= ~BIT_ULL(mb);
> > -		fifo->mailbox_disable(fifo, mb);
> > +	if (can_rx_fifo_ge(fifo, i, fifo->high_first) &&
> > fifo->second_first)
> > +		netdev_warn(fifo->dev, "%s: RX order cannot be
> > guaranteed."
> > +			" (count=%d)\n", __func__, i);
> >  
> > -		fifo->mailbox_receive(fifo, mb);
> > +	fifo->second_first = false;
> >  
> > -		if (fifo->next == fifo->high_first) {
> > -			fifo->active |= fifo->mask_low;
> > -			fifo->mailbox_enable_mask(fifo, fifo->mask_low);
> > -		}
> > +	/* No EMPTY MB in first half? */
> > +	if (can_rx_fifo_ge(fifo, i, fifo->high_first)) {
> > +		/* Re-enable all disabled MBs */
> > +		fifo->active = fifo->mask_low | fifo->mask_high;
> > +		fifo->mailbox_enable_mask(fifo, fifo->active);
> > +
> > +		/* Next time we need to check the second half first */
> > +		fifo->second_first = true;
> > +	}
> >  
> > -		received++;
> > -		quota--;
> > -	} while (quota);
> > +	if (received)
> > +		napi_schedule(&fifo->napi);
> >  
> >  	return received;
> >  }
> > -EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
> > +EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
> > +
> > +void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
> > +{
> > +	napi_enable(&fifo->napi);
> > +}
> > +EXPORT_SYMBOL_GPL(can_rx_fifo_napi_enable);
> > +
> > +void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo)
> > +{
> > +	napi_disable(&fifo->napi);
> > +}
> > +EXPORT_SYMBOL_GPL(can_rx_fifo_napi_disable);
> >  
> > -u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
> > +void can_rx_fifo_del(struct can_rx_fifo *fifo)
> >  {
> > -	return fifo->active;
> > +	if (fifo->ring)
> > +		kfree(fifo->ring);
> 
> kfree() can be called with NULL

Right. Will remove the if().

> > +	netif_napi_del(&fifo->napi);
> >  }
> > -EXPORT_SYMBOL_GPL(can_rx_fifo_get_active_mb_mask);
> > +EXPORT_SYMBOL_GPL(can_rx_fifo_del);
> >  
> >  /*
> >   * Local echo of CAN messages
> > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> > index ed46f7d..64a8de3 100644
> > --- a/include/linux/can/dev.h
> > +++ b/include/linux/can/dev.h
> > @@ -71,18 +71,25 @@ struct can_rx_fifo {
> >  	unsigned int high_first;
> >  	unsigned int high_last;		/* not needed during
> > runtime */ 
> > -	u64 (*read_pending)(struct can_rx_fifo *rx_fifo);
> >  	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64
> > mask);
> > -	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int
> > mb);
> > -	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int
> > mb);
> > +	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int
> > mb);
> > +	unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo
> > *rx_fifo,
> > +		struct can_frame *frame, unsigned int mb);
> >  
> >  	u64 mask_low;
> >  	u64 mask_high;
> >  	u64 active;
> >  
> > -	unsigned int next;
> > +	unsigned int second_first;
> 
> The rest of the code talks about low and high, what about naming this
> variable, high_first?

Good idea... it was a leftover of my original patch, and I didn't change the
name. Will do.

> 
> >  
> >  	bool inc;
> > +
> > +	struct can_frame *ring;
> > +	struct can_frame overflow;
> > +	size_t ring_size;
> > +	unsigned int ring_head;
> > +	unsigned int ring_tail;
> > +	struct napi_struct napi;
> >  };
> >  
> >  /*
> > @@ -127,8 +134,10 @@ u8 can_dlc2len(u8 can_dlc);
> >  u8 can_len2dlc(u8 len);
> >  
> >  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
> > -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
> > -u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
> > +int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
> > +void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
> > +void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
> > +void can_rx_fifo_del(struct can_rx_fifo *fifo);
> >  
> >  struct net_device *alloc_candev(int sizeof_priv, unsigned int
> > echo_skb_max); void free_candev(struct net_device *dev);
> 
> Marc

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 07/15] can: rx-fifo: Change to do controller off-load in interrupt and NAPI poll

[Marc Kleine-Budde]

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

On 10/20/2014 09:06 AM, David Jander wrote:
>>> +static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
>>> +{
>>> +	struct can_rx_fifo *fifo = container_of(napi, struct can_rx_fifo,
>>> napi);
>>> +	int work_done = 0;
>>> +	int ret;
>>> +	unsigned int head;
>>> +	unsigned int tail;
>>> +
>>> +restart_poll:
>>> +	/* handle mailboxes */
>>> +	head = smp_load_acquire(&fifo->ring_head);
>>> +	tail = fifo->ring_tail;
>>> +	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
>>> +			(work_done < quota)) {
>>> +		ret = can_rx_fifo_read_napi_frame(fifo, tail);
>>> +		work_done += ret;
>>> +		tail = (tail + 1) & (fifo->ring_size -1);
>>> +		smp_store_release(&fifo->ring_tail, tail);
>>> +	}
>>> +
>>> +	if (work_done < quota) {
>>> +		napi_complete(napi);
>>> +
>>> +		/* Check if there was another interrupt */
>>> +		head = smp_load_acquire(&fifo->ring_head);
>>> +		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
>>> +		    napi_reschedule(&fifo->napi))
>>> +			goto restart_poll;
>>
>> Hmmm, this looks a bit strange. If I understand the code correctly you ask
>> that napi should be started again, but then jump directly to the beginning.
> 
> The documentation seems to say that one should use it like that:
> 
> http://www.linuxfoundation.org/collaborate/workgroups/networking/napi

Some drivers do it this way, other don't.

> If you still think it is wrong, then tell me how to re-enable napi and
> continue correctly. AFAIK it is done like this in order to avoid a race when
> the interrupt is called while NAPI polling was underway. napi_schedule() just
> sets a flag, and does _not_ add work to a queue...

I think your code is correct. There are several aspects to it, as far as
I understand the NAPI code:
- NAPI_STATE_SCHED means a poll is scheduled
- napi_reschedule(): if NAPI_STATE_SCHED is already set it will return
  false. Otherwise NAPI_STATE_SCHED will be set and a sofirq will be
  raised, true is returned.
- I think the "goto restart_poll" is an optimisation, as the softirq
  will schedule the poll function again. But AFAICS it's fine, as
  napi_complete() checks for NAPI_STATE_SCHED set.
- The above code is needed for devices on an edge triggered interrupt
  line. As there is a race window between checking the RX buffer and
  enabling the interrupt line. As we don't have an interrupt here at
  all, we must have this code. Good work!

>>> +	}
>>> +
>>> +	return work_done;
>>> +}
>>> +
>>>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>>>  {
>>> +	unsigned int weight;
>>>  	fifo->dev = dev;
>>>  
>>>  	if ((fifo->low_first < fifo->high_first) &&
>>> -	    (fifo->high_first < fifo->high_last))
>>> +	    (fifo->high_first < fifo->high_last)) {
>>>  		fifo->inc = true;
>>> -	else if ((fifo->low_first > fifo->high_first) &&
>>> -		 (fifo->high_first > fifo->high_last))
>>> +		weight = fifo->high_last - fifo->low_first;
>>> +	} else if ((fifo->low_first > fifo->high_first) &&
>>> +		 (fifo->high_first > fifo->high_last)) {
>>>  		fifo->inc = false;
>>> -	else
>>> +		weight = fifo->low_first - fifo->high_last;
>>> +	} else
>>>  		return -EINVAL;
>>
>> Please but { } at every branch of the if else.
> 
> Ok, will do.
> 
>>>  
>>> -	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
>>> -	    !fifo->mailbox_disable || !fifo->mailbox_receive)
>>> +	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
>>> +	    !fifo->mailbox_enable)
>>>  		return -EINVAL;
>>>  
>>> +	/* Make ring-buffer a sensible size that is a power of 2 */
>>> +	fifo->ring_size = (2 << fls(weight));
>>> +	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
>>> +			     GFP_KERNEL);
>>> +	if (!fifo->ring)
>>> +		return -ENOMEM;
>>> +
>>> +	fifo->ring_head = fifo->ring_tail = 0;
>>> +
>>> +	/* Take care of NAPI handling */
>>> +	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
>>
>> I'm not sure, if the rx-fifo should take care of the whole NAPI, I think it's
>> better to provide helper functions instead.
> 
> Why not? We are removing the messages from the CAN controller in the IRQ
> already, so why would the CAN driver have to even care about NAPI which
> happens _after_ all that? Can you come up with an example where this may not
> be desirable?

If NAPI is handled by the rx-fifo exclusively, everything else i.e.
tx-complete and error handling cannot be done in NAPI. Thinking more
about this and looking at the above discussed code, maybe it's better to
have the napi in the rx-fifo code.

> Can you illustrate your idea with helper functions?

Something like can_rx_fifo_napi_poll() would be our helper function,
it's supposed to be called from the NAPI handler the driver has
registered. But the driver has to take care about the napi_complete()
and napi_reschedule(), which is probably quite fragile.... So keep it as
it is.

> 
>>> +
>>>  	/* init variables */
>>>  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
>>>  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
>>> -	fifo->next = fifo->low_first;
>>> +	fifo->second_first = false;
>>>  	fifo->active = fifo->mask_low | fifo->mask_high;
>>>  	fifo->mailbox_enable_mask(fifo, fifo->active);
>>>  
>>> @@ -338,60 +420,95 @@ int can_rx_fifo_add(struct net_device *dev, struct
>>> can_rx_fifo *fifo) }
>>>  EXPORT_SYMBOL_GPL(can_rx_fifo_add);
>>>  
>>> -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
>>> +static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo,
>>> unsigned int n) +{
>>> +	unsigned int head = fifo->ring_head;
>>> +	unsigned int tail = ACCESS_ONCE(fifo->ring_tail);
>>> +	unsigned int ret = 0;
>>> +
>>> +	if (CIRC_SPACE(head, tail, fifo->ring_size) >= 1) {
>>> +		ret = fifo->mailbox_move_to_buffer(fifo,
>>> &fifo->ring[head], n);
>>> +		if (ret)
>>> +			smp_store_release(&fifo->ring_head,
>>> +				(head + 1) & (fifo->ring_size - 1));
>>> +	} else {
>>> +		ret = fifo->mailbox_move_to_buffer(fifo, &fifo->overflow,
>>> n);
>>> +		if (ret)
>>> +			fifo->dev->stats.rx_dropped++;
>>> +	}
>>
>> That's the purpose of the overflow mailbox? fifo-> overflow seems to be write
>> only?
> 
> Yes.
> The idea is to simplify the code for the user. mailbox_move_to_buffer() should
> just move the corresponding can message to the provided buffer and do all
> interrupt-flag clearing and stuff the driver needs to do in order to free the
> MB. Its just that in the case we don't have space in the circular buffer, I
> don't want to complicate things for the driver and tell him that this message
> should be discarded. Just think of fifo->overflow as a sort of /dev/null.
> Of course I could just pass NULL to that function, but IMHO that's dangerous
> because since it happens only very seldom, a missing check for NULL in that
> function may bite you when it is already too late (i.e. the driver already hit
> mainline).

Okay, got it, nice idea. Can you put a comment in the struct rx-fifo.
Can you also document the requirements on the mailbox_move_to_buffer()
callback, I remember you've written this in some email....

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: 819 bytes --]

Re: [PATCH 07/15] can: rx-fifo: Change to do controller off-load in interrupt and NAPI poll

[David Jander]

On Mon, 03 Nov 2014 12:10:17 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/20/2014 09:06 AM, David Jander wrote:
> >>> +static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
> >>> +{
> >>> +	struct can_rx_fifo *fifo = container_of(napi, struct
> >>> can_rx_fifo, napi);
> >>> +	int work_done = 0;
> >>> +	int ret;
> >>> +	unsigned int head;
> >>> +	unsigned int tail;
> >>> +
> >>> +restart_poll:
> >>> +	/* handle mailboxes */
> >>> +	head = smp_load_acquire(&fifo->ring_head);
> >>> +	tail = fifo->ring_tail;
> >>> +	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> >>> +			(work_done < quota)) {
> >>> +		ret = can_rx_fifo_read_napi_frame(fifo, tail);
> >>> +		work_done += ret;
> >>> +		tail = (tail + 1) & (fifo->ring_size -1);
> >>> +		smp_store_release(&fifo->ring_tail, tail);
> >>> +	}
> >>> +
> >>> +	if (work_done < quota) {
> >>> +		napi_complete(napi);
> >>> +
> >>> +		/* Check if there was another interrupt */
> >>> +		head = smp_load_acquire(&fifo->ring_head);
> >>> +		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> >>> +		    napi_reschedule(&fifo->napi))
> >>> +			goto restart_poll;
> >>
> >> Hmmm, this looks a bit strange. If I understand the code correctly you ask
> >> that napi should be started again, but then jump directly to the
> >> beginning.
> > 
> > The documentation seems to say that one should use it like that:
> > 
> > http://www.linuxfoundation.org/collaborate/workgroups/networking/napi
> 
> Some drivers do it this way, other don't.
> 
> > If you still think it is wrong, then tell me how to re-enable napi and
> > continue correctly. AFAIK it is done like this in order to avoid a race
> > when the interrupt is called while NAPI polling was underway.
> > napi_schedule() just sets a flag, and does _not_ add work to a queue...
> 
> I think your code is correct. There are several aspects to it, as far as
> I understand the NAPI code:
> - NAPI_STATE_SCHED means a poll is scheduled
> - napi_reschedule(): if NAPI_STATE_SCHED is already set it will return
>   false. Otherwise NAPI_STATE_SCHED will be set and a sofirq will be
>   raised, true is returned.
> - I think the "goto restart_poll" is an optimisation, as the softirq
>   will schedule the poll function again. But AFAICS it's fine, as
>   napi_complete() checks for NAPI_STATE_SCHED set.
> - The above code is needed for devices on an edge triggered interrupt
>   line. As there is a race window between checking the RX buffer and
>   enabling the interrupt line. As we don't have an interrupt here at
>   all, we must have this code. Good work!

Exactly. Thanks!

> >>> +	}
> >>> +
> >>> +	return work_done;
> >>> +}
> >>> +
> >>>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
> >>>  {
> >>> +	unsigned int weight;
> >>>  	fifo->dev = dev;
> >>>  
> >>>  	if ((fifo->low_first < fifo->high_first) &&
> >>> -	    (fifo->high_first < fifo->high_last))
> >>> +	    (fifo->high_first < fifo->high_last)) {
> >>>  		fifo->inc = true;
> >>> -	else if ((fifo->low_first > fifo->high_first) &&
> >>> -		 (fifo->high_first > fifo->high_last))
> >>> +		weight = fifo->high_last - fifo->low_first;
> >>> +	} else if ((fifo->low_first > fifo->high_first) &&
> >>> +		 (fifo->high_first > fifo->high_last)) {
> >>>  		fifo->inc = false;
> >>> -	else
> >>> +		weight = fifo->low_first - fifo->high_last;
> >>> +	} else
> >>>  		return -EINVAL;
> >>
> >> Please but { } at every branch of the if else.
> > 
> > Ok, will do.
> > 
> >>>  
> >>> -	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
> >>> -	    !fifo->mailbox_disable || !fifo->mailbox_receive)
> >>> +	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer
> >>> ||
> >>> +	    !fifo->mailbox_enable)
> >>>  		return -EINVAL;
> >>>  
> >>> +	/* Make ring-buffer a sensible size that is a power of 2 */
> >>> +	fifo->ring_size = (2 << fls(weight));
> >>> +	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
> >>> +			     GFP_KERNEL);
> >>> +	if (!fifo->ring)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	fifo->ring_head = fifo->ring_tail = 0;
> >>> +
> >>> +	/* Take care of NAPI handling */
> >>> +	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
> >>
> >> I'm not sure, if the rx-fifo should take care of the whole NAPI, I think
> >> it's better to provide helper functions instead.
> > 
> > Why not? We are removing the messages from the CAN controller in the IRQ
> > already, so why would the CAN driver have to even care about NAPI which
> > happens _after_ all that? Can you come up with an example where this may
> > not be desirable?
> 
> If NAPI is handled by the rx-fifo exclusively, everything else i.e.
> tx-complete and error handling cannot be done in NAPI. Thinking more
> about this and looking at the above discussed code, maybe it's better to
> have the napi in the rx-fifo code.

Ok, so we keep it there...

> > Can you illustrate your idea with helper functions?
> 
> Something like can_rx_fifo_napi_poll() would be our helper function,
> it's supposed to be called from the NAPI handler the driver has
> registered. But the driver has to take care about the napi_complete()
> and napi_reschedule(), which is probably quite fragile.... So keep it as
> it is.

That's what I thought too.

> > 
> >>> +
> >>>  	/* init variables */
> >>>  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
> >>>  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
> >>> -	fifo->next = fifo->low_first;
> >>> +	fifo->second_first = false;
> >>>  	fifo->active = fifo->mask_low | fifo->mask_high;
> >>>  	fifo->mailbox_enable_mask(fifo, fifo->active);
> >>>  
> >>> @@ -338,60 +420,95 @@ int can_rx_fifo_add(struct net_device *dev, struct
> >>> can_rx_fifo *fifo) }
> >>>  EXPORT_SYMBOL_GPL(can_rx_fifo_add);
> >>>  
> >>> -int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
> >>> +static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo
> >>> *fifo, unsigned int n) +{
> >>> +	unsigned int head = fifo->ring_head;
> >>> +	unsigned int tail = ACCESS_ONCE(fifo->ring_tail);
> >>> +	unsigned int ret = 0;
> >>> +
> >>> +	if (CIRC_SPACE(head, tail, fifo->ring_size) >= 1) {
> >>> +		ret = fifo->mailbox_move_to_buffer(fifo,
> >>> &fifo->ring[head], n);
> >>> +		if (ret)
> >>> +			smp_store_release(&fifo->ring_head,
> >>> +				(head + 1) & (fifo->ring_size - 1));
> >>> +	} else {
> >>> +		ret = fifo->mailbox_move_to_buffer(fifo,
> >>> &fifo->overflow, n);
> >>> +		if (ret)
> >>> +			fifo->dev->stats.rx_dropped++;
> >>> +	}
> >>
> >> That's the purpose of the overflow mailbox? fifo-> overflow seems to be
> >> write only?
> > 
> > Yes.
> > The idea is to simplify the code for the user. mailbox_move_to_buffer()
> > should just move the corresponding can message to the provided buffer and
> > do all interrupt-flag clearing and stuff the driver needs to do in order
> > to free the MB. Its just that in the case we don't have space in the
> > circular buffer, I don't want to complicate things for the driver and tell
> > him that this message should be discarded. Just think of fifo->overflow as
> > a sort of /dev/null. Of course I could just pass NULL to that function,
> > but IMHO that's dangerous because since it happens only very seldom, a
> > missing check for NULL in that function may bite you when it is already
> > too late (i.e. the driver already hit mainline).
> 
> Okay, got it, nice idea. Can you put a comment in the struct rx-fifo.
> Can you also document the requirements on the mailbox_move_to_buffer()
> callback, I remember you've written this in some email....

Ok, will do. It might take a few days until I can post the next version
unfortunately....

Best regards,

-- 
David Jander
Protonic Holland.

[PATCH 08/15] can: rx-fifo: fix long lines

[David Jander]

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

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 22a3955..3e1160a 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -301,17 +301,21 @@ static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
 static u64 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0LLU >> (64 + fifo->low_first - fifo->high_first) << fifo->low_first;
+		return ~0LLU >> (64 + fifo->low_first - fifo->high_first)
+			     << fifo->low_first;
 	else
-		return ~0LLU >> (64 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
+		return ~0LLU >> (64 - fifo->low_first + fifo->high_first)
+			     << (fifo->high_first + 1);
 }
 
 static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0LLU >> (64 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
+		return ~0LLU >> (64 + fifo->high_first - fifo->high_last - 1)
+			     << fifo->high_first;
 	else
-		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
+		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1)
+			     << fifo->high_last;
 }
 
 static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int index)
-- 
1.9.1

Re: [PATCH 08/15] can: rx-fifo: fix long lines

[Marc Kleine-Budde]

On Fri, Oct 10, 2014 at 05:46:53PM +0200, David Jander wrote:
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/net/can/dev.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 22a3955..3e1160a 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -301,17 +301,21 @@ static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
>  static u64 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
>  {
>  	if (fifo->inc)
> -		return ~0LLU >> (64 + fifo->low_first - fifo->high_first) << fifo->low_first;
> +		return ~0LLU >> (64 + fifo->low_first - fifo->high_first)
> +			     << fifo->low_first;

In the kernel, a line usually doesn't start with an operator. Please keep it in
the original line.

>  	else
> -		return ~0LLU >> (64 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
> +		return ~0LLU >> (64 - fifo->low_first + fifo->high_first)
> +			     << (fifo->high_first + 1);
>  }
>  
>  static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
>  {
>  	if (fifo->inc)
> -		return ~0LLU >> (64 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
> +		return ~0LLU >> (64 + fifo->high_first - fifo->high_last - 1)
> +			     << fifo->high_first;
>  	else
> -		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
> +		return ~0LLU >> (64 - fifo->high_first + fifo->high_last - 1)
> +			     << fifo->high_last;
>  }
>  
>  static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int index)

Marc

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 08/15] can: rx-fifo: fix long lines

[David Jander]

On Sun, 19 Oct 2014 23:18:03 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On Fri, Oct 10, 2014 at 05:46:53PM +0200, David Jander wrote:
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/net/can/dev.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 22a3955..3e1160a 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -301,17 +301,21 @@ static unsigned int can_rx_fifo_inc(struct
> > can_rx_fifo *fifo, unsigned int *val) static u64
> > can_rx_fifo_mask_low(struct can_rx_fifo *fifo) {
> >  	if (fifo->inc)
> > -		return ~0LLU >> (64 + fifo->low_first - fifo->high_first)
> > << fifo->low_first;
> > +		return ~0LLU >> (64 + fifo->low_first - fifo->high_first)
> > +			     << fifo->low_first;
> 
> In the kernel, a line usually doesn't start with an operator. Please keep it
> in the original line.

Right. I did this on purpose because I thought it was a lot more readable
like that for this case, but if you prefer coding-style, I'll fix it.

> >  	else
> > -		return ~0LLU >> (64 - fifo->low_first + fifo->high_first)
> > << (fifo->high_first + 1);
> > +		return ~0LLU >> (64 - fifo->low_first + fifo->high_first)
> > +			     << (fifo->high_first + 1);
> >  }
> >  
> >  static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
> >  {
> >  	if (fifo->inc)
> > -		return ~0LLU >> (64 + fifo->high_first - fifo->high_last
> > - 1) << fifo->high_first;
> > +		return ~0LLU >> (64 + fifo->high_first - fifo->high_last
> > - 1)
> > +			     << fifo->high_first;
> >  	else
> > -		return ~0LLU >> (64 - fifo->high_first + fifo->high_last
> > - 1) << fifo->high_last;
> > +		return ~0LLU >> (64 - fifo->high_first + fifo->high_last
> > - 1)
> > +			     << fifo->high_last;
> >  }
> >  
> >  static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int
> > index)
> 
> Marc
> 

Best regards,

-- 
David Jander
Protonic Holland.

[PATCH 09/15] can: rx-fifo: Add can_rx_fifo_reset() function

[David Jander]

This function needs to be called every time the CAN controller is reset
and before interrupts are enabled. Otherwise the irq-offload loop gets
out of sync. Detect this and BUG() to the user if he forgot.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 20 ++++++++++++++++++--
 include/linux/can/dev.h |  1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3e1160a..fc35d28 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -487,8 +487,17 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
 		fifo->second_first = true;
 	}
 
-	if (received)
-		napi_schedule(&fifo->napi);
+	if (received) {
+		can_rx_fifo_napi_schedule(fifo);
+	} else {
+		/*
+		 * This should only happen if the CAN conroller was reset, but
+		 * can_rx_fifo_reset() was not called. BUG();
+		 */
+		netdev_warn(fifo->dev, "%s: No messages found,"
+			    " RX-FIFO out of sync?\n", __func__);
+		BUG();
+	}
 
 	return received;
 }
@@ -506,6 +515,13 @@ void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_napi_disable);
 
+void can_rx_fifo_reset(struct can_rx_fifo *fifo)
+{
+	fifo->second_first = false;
+	fifo->active = fifo->mask_low | fifo->mask_high;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_reset);
+
 void can_rx_fifo_del(struct can_rx_fifo *fifo)
 {
 	if (fifo->ring)
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 64a8de3..ada324c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -137,6 +137,7 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
 int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
+void can_rx_fifo_reset(struct can_rx_fifo *fifo);
 void can_rx_fifo_del(struct can_rx_fifo *fifo);
 
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
-- 
1.9.1

Re: [PATCH 09/15] can: rx-fifo: Add can_rx_fifo_reset() function

[Marc Kleine-Budde]

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

On 10/10/2014 05:46 PM, David Jander wrote:
> This function needs to be called every time the CAN controller is reset
> and before interrupts are enabled. Otherwise the irq-offload loop gets
> out of sync. Detect this and BUG() to the user if he forgot.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/net/can/dev.c   | 20 ++++++++++++++++++--
>  include/linux/can/dev.h |  1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 3e1160a..fc35d28 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -487,8 +487,17 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
>  		fifo->second_first = true;
>  	}
>  
> -	if (received)
> -		napi_schedule(&fifo->napi);
> +	if (received) {
> +		can_rx_fifo_napi_schedule(fifo);
> +	} else {
> +		/*
> +		 * This should only happen if the CAN conroller was reset, but
> +		 * can_rx_fifo_reset() was not called. BUG();
> +		 */
> +		netdev_warn(fifo->dev, "%s: No messages found,"
> +			    " RX-FIFO out of sync?\n", __func__);
> +		BUG();

Can you replace the BUG() (which crashes the system) by something less
fatal? E.g. by some of the WARN function [1]? Does it make sense to call
the fifo_reset() function after this? We'll loose up to a FIFO size
amount of CAN frames, but the system stays hopefully working.

Marc

[1] http://lxr.free-electrons.com/source/include/asm-generic/bug.h#L84

-- 
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: 819 bytes --]

Re: [PATCH 09/15] can: rx-fifo: Add can_rx_fifo_reset() function

[David Jander]

On Mon, 03 Nov 2014 12:16:24 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/10/2014 05:46 PM, David Jander wrote:
> > This function needs to be called every time the CAN controller is reset
> > and before interrupts are enabled. Otherwise the irq-offload loop gets
> > out of sync. Detect this and BUG() to the user if he forgot.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/net/can/dev.c   | 20 ++++++++++++++++++--
> >  include/linux/can/dev.h |  1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 3e1160a..fc35d28 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -487,8 +487,17 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
> >  		fifo->second_first = true;
> >  	}
> >  
> > -	if (received)
> > -		napi_schedule(&fifo->napi);
> > +	if (received) {
> > +		can_rx_fifo_napi_schedule(fifo);
> > +	} else {
> > +		/*
> > +		 * This should only happen if the CAN conroller was
> > reset, but
> > +		 * can_rx_fifo_reset() was not called. BUG();
> > +		 */
> > +		netdev_warn(fifo->dev, "%s: No messages found,"
> > +			    " RX-FIFO out of sync?\n", __func__);
> > +		BUG();
> 
> Can you replace the BUG() (which crashes the system) by something less
> fatal? E.g. by some of the WARN function [1]? Does it make sense to call
> the fifo_reset() function after this? We'll loose up to a FIFO size
> amount of CAN frames, but the system stays hopefully working.

Ok. We just need to make enough noise, so that the potential driver's author
sees it before release... a WARN*() should do.

> Marc
> 
> [1] http://lxr.free-electrons.com/source/include/asm-generic/bug.h#L84
> 

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 09/15] can: rx-fifo: Add can_rx_fifo_reset() function

[Marc Kleine-Budde]

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

On 11/03/2014 01:46 PM, David Jander wrote:
> On Mon, 03 Nov 2014 12:16:24 +0100
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 10/10/2014 05:46 PM, David Jander wrote:
>>> This function needs to be called every time the CAN controller is reset
>>> and before interrupts are enabled. Otherwise the irq-offload loop gets
>>> out of sync. Detect this and BUG() to the user if he forgot.
>>>
>>> Signed-off-by: David Jander <david@protonic.nl>
>>> ---
>>>  drivers/net/can/dev.c   | 20 ++++++++++++++++++--
>>>  include/linux/can/dev.h |  1 +
>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index 3e1160a..fc35d28 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -487,8 +487,17 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
>>>  		fifo->second_first = true;
>>>  	}
>>>  
>>> -	if (received)
>>> -		napi_schedule(&fifo->napi);
>>> +	if (received) {
>>> +		can_rx_fifo_napi_schedule(fifo);
>>> +	} else {
>>> +		/*
>>> +		 * This should only happen if the CAN conroller was
>>> reset, but
>>> +		 * can_rx_fifo_reset() was not called. BUG();
>>> +		 */
>>> +		netdev_warn(fifo->dev, "%s: No messages found,"
>>> +			    " RX-FIFO out of sync?\n", __func__);
>>> +		BUG();
>>
>> Can you replace the BUG() (which crashes the system) by something less
>> fatal? E.g. by some of the WARN function [1]? Does it make sense to call
>> the fifo_reset() function after this? We'll loose up to a FIFO size
>> amount of CAN frames, but the system stays hopefully working.
> 
> Ok. We just need to make enough noise, so that the potential driver's author
> sees it before release... a WARN*() should do.

ACK, and it will generate a stack-trace, too. However I don't want to
have crashing machines at $CUSTOMER, if there is a subtle bug, what's
not triggered regularly.

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: 819 bytes --]

[PATCH 10/15] can: rx-fifo: remove obsolete comment

[David Jander]

Signed-off-by: David Jander <david@protonic.nl>
---
 include/linux/can/dev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index ada324c..c49c37b 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -69,7 +69,7 @@ struct can_rx_fifo {
 
 	unsigned int low_first;
 	unsigned int high_first;
-	unsigned int high_last;		/* not needed during runtime */
+	unsigned int high_last;
 
 	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
 	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
-- 
1.9.1

[PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander]

The interrupt handler should store the error flags if needed and call
can_rx_fifo_irq_error() if there was an error flag set. This will trigger
a napi-poll that will call poll_can_state() and poll_bus_error()
callbacks. Those should handle can state machine and send error frames
as needed.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
 include/linux/can/dev.h |  9 +++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index fc35d28..dac7579 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
 	unsigned int tail;
 
 restart_poll:
+	if (work_done < quota)
+		work_done += fifo->poll_can_state(fifo);
+
 	/* handle mailboxes */
 	head = smp_load_acquire(&fifo->ring_head);
 	tail = fifo->ring_tail;
@@ -363,14 +366,19 @@ restart_poll:
 		smp_store_release(&fifo->ring_tail, tail);
 	}
 
+	if (work_done < quota)
+		work_done += fifo->poll_bus_error(fifo);
+
 	if (work_done < quota) {
 		napi_complete(napi);
 
 		/* Check if there was another interrupt */
 		head = smp_load_acquire(&fifo->ring_head);
-		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
-		    napi_reschedule(&fifo->napi))
+		if (((CIRC_CNT(head, tail, fifo->ring_size) >= 1) ||
+		    fifo->poll_errors) && napi_reschedule(&fifo->napi)) {
+			fifo->poll_errors = false;
 			goto restart_poll;
+		}
 	}
 
 	return work_done;
@@ -393,7 +401,8 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 		return -EINVAL;
 
 	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
-	    !fifo->mailbox_enable)
+	    !fifo->mailbox_enable || !fifo->poll_bus_error ||
+	    !fifo->poll_can_state)
 		return -EINVAL;
 
 	/* Make ring-buffer a sensible size that is a power of 2 */
@@ -412,6 +421,7 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 	fifo->mask_low = can_rx_fifo_mask_low(fifo);
 	fifo->mask_high = can_rx_fifo_mask_high(fifo);
 	fifo->second_first = false;
+	fifo->poll_errors = false;
 	fifo->active = fifo->mask_low | fifo->mask_high;
 	fifo->mailbox_enable_mask(fifo, fifo->active);
 
@@ -503,6 +513,13 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
 
+void can_rx_fifo_irq_error(struct can_rx_fifo *fifo)
+{
+	fifo->poll_errors = true;
+	can_rx_fifo_napi_schedule(fifo);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_irq_error);
+
 void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
 {
 	napi_enable(&fifo->napi);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index c49c37b..e1ed6d4 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -75,12 +75,15 @@ struct can_rx_fifo {
 	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 	unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
 		struct can_frame *frame, unsigned int mb);
+	unsigned int (*poll_can_state)(struct can_rx_fifo *rx_fifo);
+	unsigned int (*poll_bus_error)(struct can_rx_fifo *rx_fifo);
 
 	u64 mask_low;
 	u64 mask_high;
 	u64 active;
 
 	unsigned int second_first;
+	bool poll_errors;
 
 	bool inc;
 
@@ -92,6 +95,11 @@ struct can_rx_fifo {
 	struct napi_struct napi;
 };
 
+static inline void can_rx_fifo_napi_schedule(struct can_rx_fifo *fifo)
+{
+	napi_schedule(&fifo->napi);
+}
+
 /*
  * get_can_dlc(value) - helper macro to cast a given data length code (dlc)
  * to __u8 and ensure the dlc value to be max. 8 bytes.
@@ -135,6 +143,7 @@ u8 can_len2dlc(u8 len);
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
 int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
+void can_rx_fifo_irq_error(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
 void can_rx_fifo_reset(struct can_rx_fifo *fifo);
-- 
1.9.1

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[Marc Kleine-Budde]

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

On 10/10/2014 05:46 PM, David Jander wrote:
> The interrupt handler should store the error flags if needed and call
> can_rx_fifo_irq_error() if there was an error flag set. This will trigger
> a napi-poll that will call poll_can_state() and poll_bus_error()
> callbacks. Those should handle can state machine and send error frames
> as needed.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
>  include/linux/can/dev.h |  9 +++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index fc35d28..dac7579 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
>  	unsigned int tail;
>  
>  restart_poll:
> +	if (work_done < quota)
> +		work_done += fifo->poll_can_state(fifo);
> +

Do we need two callbacks in the poll-loop? I think one should be enough.
The driver can call as many non-rx related functions as needed then.
E.g. state change, bus error handling, tx-completion handling, etc...

What about adding a guard:

	if (fifo->poll && work_done < quota)
		work_done += fifo->poll(fifo);

...so that the poll callback can be NULL.

>  	/* handle mailboxes */
>  	head = smp_load_acquire(&fifo->ring_head);
>  	tail = fifo->ring_tail;
> @@ -363,14 +366,19 @@ restart_poll:
>  		smp_store_release(&fifo->ring_tail, tail);
>  	}
>  
> +	if (work_done < quota)
> +		work_done += fifo->poll_bus_error(fifo);
> +
>  	if (work_done < quota) {
>  		napi_complete(napi);
>  
>  		/* Check if there was another interrupt */
>  		head = smp_load_acquire(&fifo->ring_head);
> -		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> -		    napi_reschedule(&fifo->napi))
> +		if (((CIRC_CNT(head, tail, fifo->ring_size) >= 1) ||
> +		    fifo->poll_errors) && napi_reschedule(&fifo->napi)) {
> +			fifo->poll_errors = false;
>  			goto restart_poll;
> +		}
>  	}
>  
>  	return work_done;
> @@ -393,7 +401,8 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  		return -EINVAL;
>  
>  	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
> -	    !fifo->mailbox_enable)
> +	    !fifo->mailbox_enable || !fifo->poll_bus_error ||
> +	    !fifo->poll_can_state)
>  		return -EINVAL;
>  
>  	/* Make ring-buffer a sensible size that is a power of 2 */
> @@ -412,6 +421,7 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
>  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
>  	fifo->second_first = false;
> +	fifo->poll_errors = false;
>  	fifo->active = fifo->mask_low | fifo->mask_high;
>  	fifo->mailbox_enable_mask(fifo, fifo->active);
>  
> @@ -503,6 +513,13 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
>  
> +void can_rx_fifo_irq_error(struct can_rx_fifo *fifo)
> +{
> +	fifo->poll_errors = true;
> +	can_rx_fifo_napi_schedule(fifo);
> +}
> +EXPORT_SYMBOL_GPL(can_rx_fifo_irq_error);
> +
>  void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
>  {
>  	napi_enable(&fifo->napi);
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index c49c37b..e1ed6d4 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -75,12 +75,15 @@ struct can_rx_fifo {
>  	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
>  	unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
>  		struct can_frame *frame, unsigned int mb);
> +	unsigned int (*poll_can_state)(struct can_rx_fifo *rx_fifo);
> +	unsigned int (*poll_bus_error)(struct can_rx_fifo *rx_fifo);
>  
>  	u64 mask_low;
>  	u64 mask_high;
>  	u64 active;
>  
>  	unsigned int second_first;
> +	bool poll_errors;
>  
>  	bool inc;
>  
> @@ -92,6 +95,11 @@ struct can_rx_fifo {
>  	struct napi_struct napi;
>  };
>  
> +static inline void can_rx_fifo_napi_schedule(struct can_rx_fifo *fifo)
> +{
> +	napi_schedule(&fifo->napi);
> +}
> +
>  /*
>   * get_can_dlc(value) - helper macro to cast a given data length code (dlc)
>   * to __u8 and ensure the dlc value to be max. 8 bytes.
> @@ -135,6 +143,7 @@ u8 can_len2dlc(u8 len);
>  
>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
>  int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
> +void can_rx_fifo_irq_error(struct can_rx_fifo *fifo);
>  void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
>  void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
>  void can_rx_fifo_reset(struct can_rx_fifo *fifo);
> 

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: 819 bytes --]

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander]

On Mon, 03 Nov 2014 12:24:08 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/10/2014 05:46 PM, David Jander wrote:
> > The interrupt handler should store the error flags if needed and call
> > can_rx_fifo_irq_error() if there was an error flag set. This will trigger
> > a napi-poll that will call poll_can_state() and poll_bus_error()
> > callbacks. Those should handle can state machine and send error frames
> > as needed.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
> >  include/linux/can/dev.h |  9 +++++++++
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index fc35d28..dac7579 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
> > *napi, int quota) unsigned int tail;
> >  
> >  restart_poll:
> > +	if (work_done < quota)
> > +		work_done += fifo->poll_can_state(fifo);
> > +
> 
> Do we need two callbacks in the poll-loop? I think one should be enough.
> The driver can call as many non-rx related functions as needed then.
> E.g. state change, bus error handling, tx-completion handling, etc...

Ok. I split it into two handlers, because I saw most drivers doing first
state-handling then message reception and last error handling... in this
particular order. If that order is not important, we can use one poll()
function for both state- and error-handling.
In that case... should the poll() handler be called _before_ or _after_
handling the mailboxes?

> What about adding a guard:
> 
> 	if (fifo->poll && work_done < quota)
> 		work_done += fifo->poll(fifo);
> 
> ...so that the poll callback can be NULL.

Ok, good idea.

> >  	/* handle mailboxes */
> >  	head = smp_load_acquire(&fifo->ring_head);
> >  	tail = fifo->ring_tail;
> > @@ -363,14 +366,19 @@ restart_poll:
> >  		smp_store_release(&fifo->ring_tail, tail);
> >  	}
> >  
> > +	if (work_done < quota)
> > +		work_done += fifo->poll_bus_error(fifo);
> > +
> >  	if (work_done < quota) {
> >  		napi_complete(napi);
> >  
> >  		/* Check if there was another interrupt */
> >  		head = smp_load_acquire(&fifo->ring_head);
> > -		if ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
> > -		    napi_reschedule(&fifo->napi))
> > +		if (((CIRC_CNT(head, tail, fifo->ring_size) >= 1) ||
> > +		    fifo->poll_errors) && napi_reschedule(&fifo->napi)) {
> > +			fifo->poll_errors = false;
> >  			goto restart_poll;
> > +		}
> >  	}
> >  
> >  	return work_done;
> > @@ -393,7 +401,8 @@ int can_rx_fifo_add(struct net_device *dev, struct
> > can_rx_fifo *fifo) return -EINVAL;
> >  
> >  	if (!fifo->mailbox_enable_mask || !fifo->mailbox_move_to_buffer ||
> > -	    !fifo->mailbox_enable)
> > +	    !fifo->mailbox_enable || !fifo->poll_bus_error ||
> > +	    !fifo->poll_can_state)
> >  		return -EINVAL;
> >  
> >  	/* Make ring-buffer a sensible size that is a power of 2 */
> > @@ -412,6 +421,7 @@ int can_rx_fifo_add(struct net_device *dev, struct
> > can_rx_fifo *fifo) fifo->mask_low = can_rx_fifo_mask_low(fifo);
> >  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
> >  	fifo->second_first = false;
> > +	fifo->poll_errors = false;
> >  	fifo->active = fifo->mask_low | fifo->mask_high;
> >  	fifo->mailbox_enable_mask(fifo, fifo->active);
> >  
> > @@ -503,6 +513,13 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
> >  }
> >  EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
> >  
> > +void can_rx_fifo_irq_error(struct can_rx_fifo *fifo)
> > +{
> > +	fifo->poll_errors = true;
> > +	can_rx_fifo_napi_schedule(fifo);
> > +}
> > +EXPORT_SYMBOL_GPL(can_rx_fifo_irq_error);
> > +
> >  void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
> >  {
> >  	napi_enable(&fifo->napi);
> > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> > index c49c37b..e1ed6d4 100644
> > --- a/include/linux/can/dev.h
> > +++ b/include/linux/can/dev.h
> > @@ -75,12 +75,15 @@ struct can_rx_fifo {
> >  	void (*mailbox_enable)(struct can_rx_fifo *rx_fifo, unsigned int
> > mb); unsigned int (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
> >  		struct can_frame *frame, unsigned int mb);
> > +	unsigned int (*poll_can_state)(struct can_rx_fifo *rx_fifo);
> > +	unsigned int (*poll_bus_error)(struct can_rx_fifo *rx_fifo);
> >  
> >  	u64 mask_low;
> >  	u64 mask_high;
> >  	u64 active;
> >  
> >  	unsigned int second_first;
> > +	bool poll_errors;
> >  
> >  	bool inc;
> >  
> > @@ -92,6 +95,11 @@ struct can_rx_fifo {
> >  	struct napi_struct napi;
> >  };
> >  
> > +static inline void can_rx_fifo_napi_schedule(struct can_rx_fifo *fifo)
> > +{
> > +	napi_schedule(&fifo->napi);
> > +}
> > +
> >  /*
> >   * get_can_dlc(value) - helper macro to cast a given data length code
> > (dlc)
> >   * to __u8 and ensure the dlc value to be max. 8 bytes.
> > @@ -135,6 +143,7 @@ u8 can_len2dlc(u8 len);
> >  
> >  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
> >  int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
> > +void can_rx_fifo_irq_error(struct can_rx_fifo *fifo);
> >  void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
> >  void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
> >  void can_rx_fifo_reset(struct can_rx_fifo *fifo);
> > 
> 
> Marc
> 

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[Marc Kleine-Budde]

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

On 11/03/2014 01:51 PM, David Jander wrote:
>> On 10/10/2014 05:46 PM, David Jander wrote:
>>> The interrupt handler should store the error flags if needed and call
>>> can_rx_fifo_irq_error() if there was an error flag set. This will trigger
>>> a napi-poll that will call poll_can_state() and poll_bus_error()
>>> callbacks. Those should handle can state machine and send error frames
>>> as needed.
>>>
>>> Signed-off-by: David Jander <david@protonic.nl>
>>> ---
>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
>>>  include/linux/can/dev.h |  9 +++++++++
>>>  2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index fc35d28..dac7579 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
>>> *napi, int quota) unsigned int tail;
>>>  
>>>  restart_poll:
>>> +	if (work_done < quota)
>>> +		work_done += fifo->poll_can_state(fifo);
>>> +
>>
>> Do we need two callbacks in the poll-loop? I think one should be enough.
>> The driver can call as many non-rx related functions as needed then.
>> E.g. state change, bus error handling, tx-completion handling, etc...
> 
> Ok. I split it into two handlers, because I saw most drivers doing first
> state-handling then message reception and last error handling... in this
> particular order. If that order is not important, we can use one poll()
> function for both state- and error-handling.
> In that case... should the poll() handler be called _before_ or _after_
> handling the mailboxes?

Following the general rule zero-one-infinity ("Allow none of foo, one of
foo, or any number of foo" [1]) one callback should be fine. As we are
optimizing for not loosing RX CAN frames I tend to say it should come
after rx.

Marc

[1] http://c2.com/cgi/wiki?ZeroOneInfinityRule

-- 
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: 819 bytes --]

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander]

On Mon, 03 Nov 2014 13:58:43 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 11/03/2014 01:51 PM, David Jander wrote:
> >> On 10/10/2014 05:46 PM, David Jander wrote:
> >>> The interrupt handler should store the error flags if needed and call
> >>> can_rx_fifo_irq_error() if there was an error flag set. This will trigger
> >>> a napi-poll that will call poll_can_state() and poll_bus_error()
> >>> callbacks. Those should handle can state machine and send error frames
> >>> as needed.
> >>>
> >>> Signed-off-by: David Jander <david@protonic.nl>
> >>> ---
> >>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
> >>>  include/linux/can/dev.h |  9 +++++++++
> >>>  2 files changed, 29 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> >>> index fc35d28..dac7579 100644
> >>> --- a/drivers/net/can/dev.c
> >>> +++ b/drivers/net/can/dev.c
> >>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
> >>> *napi, int quota) unsigned int tail;
> >>>  
> >>>  restart_poll:
> >>> +	if (work_done < quota)
> >>> +		work_done += fifo->poll_can_state(fifo);
> >>> +
> >>
> >> Do we need two callbacks in the poll-loop? I think one should be enough.
> >> The driver can call as many non-rx related functions as needed then.
> >> E.g. state change, bus error handling, tx-completion handling, etc...
> > 
> > Ok. I split it into two handlers, because I saw most drivers doing first
> > state-handling then message reception and last error handling... in this
> > particular order. If that order is not important, we can use one poll()
> > function for both state- and error-handling.
> > In that case... should the poll() handler be called _before_ or _after_
> > handling the mailboxes?
> 
> Following the general rule zero-one-infinity ("Allow none of foo, one of
> foo, or any number of foo" [1]) one callback should be fine. As we are
> optimizing for not loosing RX CAN frames I tend to say it should come
> after rx.

Hahaha, ok, I got it :)
Any idea on how to call this function? Just "poll" seems a little bit
under-documented...

> Marc
> 
> [1] http://c2.com/cgi/wiki?ZeroOneInfinityRule

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[Marc Kleine-Budde]

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

On 11/03/2014 02:09 PM, David Jander wrote:
> On Mon, 03 Nov 2014 13:58:43 +0100
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 11/03/2014 01:51 PM, David Jander wrote:
>>>> On 10/10/2014 05:46 PM, David Jander wrote:
>>>>> The interrupt handler should store the error flags if needed and call
>>>>> can_rx_fifo_irq_error() if there was an error flag set. This will trigger
>>>>> a napi-poll that will call poll_can_state() and poll_bus_error()
>>>>> callbacks. Those should handle can state machine and send error frames
>>>>> as needed.
>>>>>
>>>>> Signed-off-by: David Jander <david@protonic.nl>
>>>>> ---
>>>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
>>>>>  include/linux/can/dev.h |  9 +++++++++
>>>>>  2 files changed, 29 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>>>> index fc35d28..dac7579 100644
>>>>> --- a/drivers/net/can/dev.c
>>>>> +++ b/drivers/net/can/dev.c
>>>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
>>>>> *napi, int quota) unsigned int tail;
>>>>>  
>>>>>  restart_poll:
>>>>> +	if (work_done < quota)
>>>>> +		work_done += fifo->poll_can_state(fifo);
>>>>> +
>>>>
>>>> Do we need two callbacks in the poll-loop? I think one should be enough.
>>>> The driver can call as many non-rx related functions as needed then.
>>>> E.g. state change, bus error handling, tx-completion handling, etc...
>>>
>>> Ok. I split it into two handlers, because I saw most drivers doing first
>>> state-handling then message reception and last error handling... in this
>>> particular order. If that order is not important, we can use one poll()
>>> function for both state- and error-handling.
>>> In that case... should the poll() handler be called _before_ or _after_
>>> handling the mailboxes?
>>
>> Following the general rule zero-one-infinity ("Allow none of foo, one of
>> foo, or any number of foo" [1]) one callback should be fine. As we are
>> optimizing for not loosing RX CAN frames I tend to say it should come
>> after rx.
> 
> Hahaha, ok, I got it :)
> Any idea on how to call this function? Just "poll" seems a little bit
> under-documented...

Yes, what about extra_poll, poll_extra?

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: 819 bytes --]

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander]

On Mon, 03 Nov 2014 14:24:59 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 11/03/2014 02:09 PM, David Jander wrote:
> > On Mon, 03 Nov 2014 13:58:43 +0100
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 11/03/2014 01:51 PM, David Jander wrote:
> >>>> On 10/10/2014 05:46 PM, David Jander wrote:
> >>>>> The interrupt handler should store the error flags if needed and call
> >>>>> can_rx_fifo_irq_error() if there was an error flag set. This will
> >>>>> trigger a napi-poll that will call poll_can_state() and
> >>>>> poll_bus_error() callbacks. Those should handle can state machine and
> >>>>> send error frames as needed.
> >>>>>
> >>>>> Signed-off-by: David Jander <david@protonic.nl>
> >>>>> ---
> >>>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
> >>>>>  include/linux/can/dev.h |  9 +++++++++
> >>>>>  2 files changed, 29 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> >>>>> index fc35d28..dac7579 100644
> >>>>> --- a/drivers/net/can/dev.c
> >>>>> +++ b/drivers/net/can/dev.c
> >>>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
> >>>>> *napi, int quota) unsigned int tail;
> >>>>>  
> >>>>>  restart_poll:
> >>>>> +	if (work_done < quota)
> >>>>> +		work_done += fifo->poll_can_state(fifo);
> >>>>> +
> >>>>
> >>>> Do we need two callbacks in the poll-loop? I think one should be enough.
> >>>> The driver can call as many non-rx related functions as needed then.
> >>>> E.g. state change, bus error handling, tx-completion handling, etc...
> >>>
> >>> Ok. I split it into two handlers, because I saw most drivers doing first
> >>> state-handling then message reception and last error handling... in this
> >>> particular order. If that order is not important, we can use one poll()
> >>> function for both state- and error-handling.
> >>> In that case... should the poll() handler be called _before_ or _after_
> >>> handling the mailboxes?
> >>
> >> Following the general rule zero-one-infinity ("Allow none of foo, one of
> >> foo, or any number of foo" [1]) one callback should be fine. As we are
> >> optimizing for not loosing RX CAN frames I tend to say it should come
> >> after rx.
> > 
> > Hahaha, ok, I got it :)
> > Any idea on how to call this function? Just "poll" seems a little bit
> > under-documented...
> 
> Yes, what about extra_poll, poll_extra?

Oops. I just realized this isn't so easy... nor is this really a
zero-one-infinity case. On top of that the solution will get quite ugly too:

poll_can_state() and poll_bus_error() have two distinct functions and _both_
can generate a (error-) CAN frame. If I consolidate this into just one
handler, that handler could potentially generate 0, 1 or 2 CAN frames, so I'd
be forced to introduce an extra parameter to keep track of NAPI quota inside
the driver callback! This seems quite ugly to me, don't you think?

Any ideas?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[Marc Kleine-Budde]

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

On 11/05/2014 06:16 PM, David Jander wrote:
> On Mon, 03 Nov 2014 14:24:59 +0100
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 11/03/2014 02:09 PM, David Jander wrote:
>>> On Mon, 03 Nov 2014 13:58:43 +0100
>>> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>
>>>> On 11/03/2014 01:51 PM, David Jander wrote:
>>>>>> On 10/10/2014 05:46 PM, David Jander wrote:
>>>>>>> The interrupt handler should store the error flags if needed and call
>>>>>>> can_rx_fifo_irq_error() if there was an error flag set. This will
>>>>>>> trigger a napi-poll that will call poll_can_state() and
>>>>>>> poll_bus_error() callbacks. Those should handle can state machine and
>>>>>>> send error frames as needed.
>>>>>>>
>>>>>>> Signed-off-by: David Jander <david@protonic.nl>
>>>>>>> ---
>>>>>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
>>>>>>>  include/linux/can/dev.h |  9 +++++++++
>>>>>>>  2 files changed, 29 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>>>>>> index fc35d28..dac7579 100644
>>>>>>> --- a/drivers/net/can/dev.c
>>>>>>> +++ b/drivers/net/can/dev.c
>>>>>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct napi_struct
>>>>>>> *napi, int quota) unsigned int tail;
>>>>>>>  
>>>>>>>  restart_poll:
>>>>>>> +	if (work_done < quota)
>>>>>>> +		work_done += fifo->poll_can_state(fifo);
>>>>>>> +
>>>>>>
>>>>>> Do we need two callbacks in the poll-loop? I think one should be enough.
>>>>>> The driver can call as many non-rx related functions as needed then.
>>>>>> E.g. state change, bus error handling, tx-completion handling, etc...
>>>>>
>>>>> Ok. I split it into two handlers, because I saw most drivers doing first
>>>>> state-handling then message reception and last error handling... in this
>>>>> particular order. If that order is not important, we can use one poll()
>>>>> function for both state- and error-handling.
>>>>> In that case... should the poll() handler be called _before_ or _after_
>>>>> handling the mailboxes?
>>>>
>>>> Following the general rule zero-one-infinity ("Allow none of foo, one of
>>>> foo, or any number of foo" [1]) one callback should be fine. As we are
>>>> optimizing for not loosing RX CAN frames I tend to say it should come
>>>> after rx.
>>>
>>> Hahaha, ok, I got it :)
>>> Any idea on how to call this function? Just "poll" seems a little bit
>>> under-documented...
>>
>> Yes, what about extra_poll, poll_extra?
> 
> Oops. I just realized this isn't so easy... nor is this really a
> zero-one-infinity case. On top of that the solution will get quite ugly too:
> 
> poll_can_state() and poll_bus_error() have two distinct functions and _both_
> can generate a (error-) CAN frame. If I consolidate this into just one
> handler, that handler could potentially generate 0, 1 or 2 CAN frames, so I'd
> be forced to introduce an extra parameter to keep track of NAPI quota inside
> the driver callback! This seems quite ugly to me, don't you think?

What about:
    work_done += fifo->poll_error(fifo, quota - work_done);

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: 819 bytes --]

Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander]

On Thu, 06 Nov 2014 11:20:23 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 11/05/2014 06:16 PM, David Jander wrote:
> > On Mon, 03 Nov 2014 14:24:59 +0100
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 11/03/2014 02:09 PM, David Jander wrote:
> >>> On Mon, 03 Nov 2014 13:58:43 +0100
> >>> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>
> >>>> On 11/03/2014 01:51 PM, David Jander wrote:
> >>>>>> On 10/10/2014 05:46 PM, David Jander wrote:
> >>>>>>> The interrupt handler should store the error flags if needed and call
> >>>>>>> can_rx_fifo_irq_error() if there was an error flag set. This will
> >>>>>>> trigger a napi-poll that will call poll_can_state() and
> >>>>>>> poll_bus_error() callbacks. Those should handle can state machine and
> >>>>>>> send error frames as needed.
> >>>>>>>
> >>>>>>> Signed-off-by: David Jander <david@protonic.nl>
> >>>>>>> ---
> >>>>>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
> >>>>>>>  include/linux/can/dev.h |  9 +++++++++
> >>>>>>>  2 files changed, 29 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> >>>>>>> index fc35d28..dac7579 100644
> >>>>>>> --- a/drivers/net/can/dev.c
> >>>>>>> +++ b/drivers/net/can/dev.c
> >>>>>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct
> >>>>>>> napi_struct *napi, int quota) unsigned int tail;
> >>>>>>>  
> >>>>>>>  restart_poll:
> >>>>>>> +	if (work_done < quota)
> >>>>>>> +		work_done += fifo->poll_can_state(fifo);
> >>>>>>> +
> >>>>>>
> >>>>>> Do we need two callbacks in the poll-loop? I think one should be
> >>>>>> enough. The driver can call as many non-rx related functions as
> >>>>>> needed then. E.g. state change, bus error handling, tx-completion
> >>>>>> handling, etc...
> >>>>>
> >>>>> Ok. I split it into two handlers, because I saw most drivers doing
> >>>>> first state-handling then message reception and last error handling...
> >>>>> in this particular order. If that order is not important, we can use
> >>>>> one poll() function for both state- and error-handling.
> >>>>> In that case... should the poll() handler be called _before_ or _after_
> >>>>> handling the mailboxes?
> >>>>
> >>>> Following the general rule zero-one-infinity ("Allow none of foo, one of
> >>>> foo, or any number of foo" [1]) one callback should be fine. As we are
> >>>> optimizing for not loosing RX CAN frames I tend to say it should come
> >>>> after rx.
> >>>
> >>> Hahaha, ok, I got it :)
> >>> Any idea on how to call this function? Just "poll" seems a little bit
> >>> under-documented...
> >>
> >> Yes, what about extra_poll, poll_extra?
> > 
> > Oops. I just realized this isn't so easy... nor is this really a
> > zero-one-infinity case. On top of that the solution will get quite ugly
> > too:
> > 
> > poll_can_state() and poll_bus_error() have two distinct functions and
> > _both_ can generate a (error-) CAN frame. If I consolidate this into just
> > one handler, that handler could potentially generate 0, 1 or 2 CAN frames,
> > so I'd be forced to introduce an extra parameter to keep track of NAPI
> > quota inside the driver callback! This seems quite ugly to me, don't you
> > think?
> 
> What about:
>     work_done += fifo->poll_error(fifo, quota - work_done);

That would produce extra code like the following in flexcan.c:

static unsigned int flexcan_poll_extra(struct can_rx_fifo *fifo, quota)
{
	unsigned int ret;

	ret = flexcan_poll_state(fifo);
	if (ret < quota)
		ret += flexcan_poll_bus_err(fifo);

	return ret;
}

... which is quite ugly IMHO.

I just posted a new version, and I did not include this change yet. If you
still prefer to do it with just one handler (although I think they have two
distinct and defined functions), I will change it in the next version.

Best regards,

-- 
David Jander
Protonic Holland.

[PATCH 12/15] can: flexcan: Add support for RX-FIFO.

[David Jander]

This adds support for RX-FIFO simulating a FIFO from a linear array of message-
boxes. This breaks RTR reception on flexcan older than V10.

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

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 8118f35..5e37ce4 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -273,10 +273,10 @@ struct flexcan_devtype_data {
 struct flexcan_priv {
 	struct can_priv can;
 	struct net_device *dev;
-	struct napi_struct napi;
 
 	void __iomem *base;
 	u32 reg_esr;
+	u32 poll_esr;
 	u32 reg_ctrl_default;
 
 	struct clk *clk_ipg;
@@ -284,10 +284,8 @@ 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];
-	bool second_first;
-	unsigned int rx_head;
-	unsigned int rx_tail;
+
+	struct can_rx_fifo fifo;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {
@@ -540,6 +538,11 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+static inline struct flexcan_priv *rx_fifo_to_priv(struct can_rx_fifo *fifo)
+{
+	return container_of(fifo, struct flexcan_priv, fifo);
+}
+
 static void do_bus_err(struct net_device *dev,
 		       struct can_frame *cf, u32 reg_esr)
 {
@@ -588,16 +591,21 @@ static void do_bus_err(struct net_device *dev,
 		dev->stats.tx_errors++;
 }
 
-static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
+static unsigned int flexcan_poll_bus_err(struct can_rx_fifo *fifo)
 {
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
+	struct net_device *dev = priv->dev;
 	struct sk_buff *skb;
 	struct can_frame *cf;
 
+	if (!flexcan_has_and_handle_berr(priv, priv->poll_esr))
+		return 0;
+
 	skb = alloc_can_err_skb(dev, &cf);
 	if (unlikely(!skb))
 		return 0;
 
-	do_bus_err(dev, cf, reg_esr);
+	do_bus_err(dev, cf, priv->poll_esr);
 	netif_receive_skb(skb);
 
 	dev->stats.rx_packets++;
@@ -679,17 +687,22 @@ static void do_state(struct net_device *dev,
 	}
 }
 
-static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
+static unsigned int flexcan_poll_state(struct can_rx_fifo *fifo)
 {
-	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
+	struct net_device *dev = priv->dev;
+	struct flexcan_regs __iomem *regs = priv->base;
 	struct sk_buff *skb;
 	struct can_frame *cf;
 	enum can_state new_state;
 	int flt;
 
-	flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
+	/* esr bits are clear-on-read, so save them for flexcan_poll_bus_err() */
+	priv->poll_esr = priv->reg_esr | flexcan_read(&regs->esr);
+
+	flt = priv->poll_esr & FLEXCAN_ESR_FLT_CONF_MASK;
 	if (likely(flt == FLEXCAN_ESR_FLT_CONF_ACTIVE)) {
-		if (likely(!(reg_esr & (FLEXCAN_ESR_TX_WRN |
+		if (likely(!(priv->poll_esr & (FLEXCAN_ESR_TX_WRN |
 					FLEXCAN_ESR_RX_WRN))))
 			new_state = CAN_STATE_ERROR_ACTIVE;
 		else
@@ -717,232 +730,88 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 	return 1;
 }
 
-static int flexcan_read_frame(struct net_device *dev, int n)
+static void flexcan_mailbox_enable(struct can_rx_fifo *fifo, unsigned int n)
 {
-	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;
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb;
+	u32 reg_ctrl;
 	u32 code;
 
-	reg_ctrl = ACCESS_ONCE(mb->can_ctrl);
+	mb = &regs->cantxfg[n];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
 	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
-
-	/* is this MB empty? */
-	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
-	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
-		return 0;
-
-	if (code == FLEXCAN_MB_CODE_RX_OVERRRUN) {
-		/* This MB was overrun, we lost data */
-		priv->dev->stats.rx_over_errors++;
-		priv->dev->stats.rx_errors++;
-	}
-
-	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
-		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);
-
-	*(__be32 *)(cf->data + 0) = cpu_to_be32(mb->data[0]);
-	*(__be32 *)(cf->data + 4) = cpu_to_be32(mb->data[1]);
-
-	netif_receive_skb(skb);
-
-	stats->rx_packets++;
-	stats->rx_bytes += cf->can_dlc;
-
-	can_led_event(dev, CAN_LED_EVENT_RX);
-
-	return 1;
+	if (code == FLEXCAN_MB_CODE_RX_INACTIVE)
+		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY, &mb->can_ctrl);
+	flexcan_read(&regs->timer);
 }
 
-static int flexcan_poll(struct napi_struct *napi, int quota)
+static void flexcan_mailbox_enable_mask(struct can_rx_fifo *fifo, u64 mask)
 {
-	struct net_device *dev = napi->dev;
-	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_esr;
-	int work_done = 0;
-	int ret;
-	unsigned int head;
-	unsigned int tail;
+	struct flexcan_mb __iomem *mb;
+	u32 reg_ctrl;
+	u32 code;
+	unsigned int n;
 
-	/*
-	 * The error bits are cleared on read,
-	 * use saved value from irq handler.
-	 */
-	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
-
-	/* handle state changes */
-	work_done += flexcan_poll_state(dev, reg_esr);
-
-	/* handle mailboxes */
-	head = smp_load_acquire(&priv->rx_head);
-	tail = priv->rx_tail;
-	while ((CIRC_CNT(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) &&
-			(work_done < quota)) {
-		ret = flexcan_read_frame(dev, tail);
-		work_done += ret;
-		tail = (tail + 1) & (ARRAY_SIZE(priv->cantxfg_copy) -1);
-		smp_store_release(&priv->rx_tail, tail);
+	for (n = FLEXCAN_RX_AREA_START; n < ARRAY_SIZE(regs->cantxfg); n ++) {
+		mb = &regs->cantxfg[n];
+		reg_ctrl = flexcan_read(&mb->can_ctrl);
+		code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+		if ((mask & BIT_ULL(n)) && (code == FLEXCAN_MB_CODE_RX_INACTIVE))
+			flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY, &mb->can_ctrl);
 	}
-
-	/* report bus errors */
-	if (flexcan_has_and_handle_berr(priv, reg_esr) && work_done < quota)
-		work_done += flexcan_poll_bus_err(dev, reg_esr);
-
-	if (work_done < quota)
-		napi_complete(napi);
-
-	return work_done;
+	flexcan_read(&regs->timer);
 }
 
-static u32 flexcan_copy_one_rxmb(struct flexcan_priv *priv, int i)
+static unsigned int flexcan_mailbox_move_to_buffer(struct can_rx_fifo *fifo,
+		struct can_frame *cf, unsigned int n)
 {
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
 	struct flexcan_regs __iomem *regs = priv->base;
-	struct flexcan_mb __iomem *mb;
-	struct flexcan_mb *dst;
-	u32 reg_ctrl;
+	struct flexcan_mb *mb = &regs->cantxfg[n];
+	u32 reg_ctrl, reg_id;
 	u32 code;
-	unsigned int head;
-	unsigned int tail;
 
-	mb = &regs->cantxfg[i];
-	reg_ctrl = flexcan_read(&mb->can_ctrl);
-	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
-
-	while (code & FLEXCAN_MB_CODE_RX_BUSY_BIT) {
-		/* MB busy, shouldn't take long */
+	do {
 		reg_ctrl = flexcan_read(&mb->can_ctrl);
 		code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
-	}
+	} while (code & FLEXCAN_MB_CODE_RX_BUSY_BIT);
 
-	/* No need to process if neither FULL nor OVERRRUN */
+	/* is this MB empty? */
 	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
 	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
-		return code;
-
-	head = priv->rx_head;
-	tail = ACCESS_ONCE(priv->rx_tail);
-	if (CIRC_SPACE(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) {
-		/* Copy contents */
-		dst = &priv->cantxfg_copy[priv->rx_head];
-		dst->can_id = flexcan_read(&mb->can_id);
-		dst->data[0] = flexcan_read(&mb->data[0]);
-		dst->data[1] = flexcan_read(&mb->data[1]);
-		dst->can_ctrl = reg_ctrl;
-
-		smp_store_release(&priv->rx_head,
-			(head + 1) & (ARRAY_SIZE(priv->cantxfg_copy) - 1));
-	} else {
-		/* Buffer space overrun, we lost data */
+		return 0;
+
+	if (code == FLEXCAN_MB_CODE_RX_OVERRRUN) {
+		/* This MB was overrun, we lost data */
 		priv->dev->stats.rx_over_errors++;
 		priv->dev->stats.rx_errors++;
 	}
 
-	/* Clear the interrupt flag and lock MB */
-	if (i < 32)
-		flexcan_write(BIT(i), &regs->iflag1);
+	reg_id = flexcan_read(&mb->can_id);
+	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
+		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
-		flexcan_write(BIT(i - 32), &regs->iflag2);
-	flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE, &mb->can_ctrl);
-
-	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;
-	if (code == FLEXCAN_MB_CODE_RX_INACTIVE)
-		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY, &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;
-	unsigned int code;
-
-	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 = FLEXCAN_RX_AREA_START; i < 64; i++) {
-		code = flexcan_copy_one_rxmb(priv, i);
-		if (code == FLEXCAN_MB_CODE_RX_EMPTY)
-			break;
-	}
+		cf->can_id = (reg_id >> 18) & CAN_SFF_MASK;
 
-	if ((i >= 32) && priv->second_first)
-		netdev_warn(priv->dev, "RX order cannot be guaranteed. count=%d\n", i);
+	if (reg_ctrl & FLEXCAN_MB_CNT_RTR)
+		cf->can_id |= CAN_RTR_FLAG;
+	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
 
-	priv->second_first = false;
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(flexcan_read(&mb->data[0]));
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
 
-	/* No EMPTY MB in first half? */
-	if (i >= 32) {
-		/* Re-enable all disabled MBs */
-		for(i = FLEXCAN_RX_AREA_START; i < 64; i++)
-			flexcan_unlock_if_locked(priv, i);
+	/* Clear IRQ and lock MB */
+	if (n < 32)
+		flexcan_write(BIT(n), &regs->iflag1);
+	else
+		flexcan_write(BIT(n - 32), &regs->iflag2);
 
-		/* Next time we need to check the second half first */
-		priv->second_first = true;
-	}
+	flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE, &mb->can_ctrl);
 
-	/* Unlock the last locked MB if any */
-	flexcan_read(&regs->timer);
+	return 1;
 }
 
 static irqreturn_t flexcan_irq(int irq, void *dev_id)
@@ -961,22 +830,20 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if (reg_esr & FLEXCAN_ESR_ALL_INT)
 		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
 
-	/*
-	 * schedule NAPI in case of:
-	 * - rx IRQ
-	 * - state change IRQ
-	 * - bus error IRQ and bus error reporting is activated
-	 */
-	if ((reg_iflag1 & ~(1 << FLEXCAN_TX_BUF_ID)) || reg_iflag2 ||
-	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
+	/* bus error IRQ and bus error reporting is activated */
+	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
 		/*
 		 * The error bits are cleared on read,
-		 * save them for later use.
+		 * save them for later use and let napi poll get them.
 		 */
 		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		flexcan_copy_rxmbs(priv, reg_iflag1, reg_iflag2);
-		napi_schedule(&priv->napi);
+		can_rx_fifo_irq_error(&priv->fifo);
+	}
+
+	/* reception interrupt */
+	if ((reg_iflag1 & ~(1 << FLEXCAN_TX_BUF_ID)) || reg_iflag2) {
+		can_rx_fifo_irq_offload(&priv->fifo);
 	}
 
 	/* transmission complete interrupt */
@@ -1133,9 +1000,6 @@ static int flexcan_chip_start(struct net_device *dev)
 	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
 		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
 
-	priv->second_first = false;
-	priv->rx_head = priv->rx_tail = 0;
-
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
 	flexcan_write(0x0, &regs->rx14mask);
@@ -1170,17 +1034,19 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (err)
 		goto out_chip_disable;
 
-	/* synchronize with the can bus */
-	err = flexcan_chip_unfreeze(priv);
-	if (err)
-		goto out_transceiver_disable;
-
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+	can_rx_fifo_reset(&priv->fifo);
+
 	/* enable all MB interrupts */
 	flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
 	flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
 
+	/* synchronize with the can bus */
+	err = flexcan_chip_unfreeze(priv);
+	if (err)
+		goto out_transceiver_disable;
+
 	/* print chip status */
 	netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
 		   flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
@@ -1248,7 +1114,7 @@ static int flexcan_open(struct net_device *dev)
 
 	can_led_event(dev, CAN_LED_EVENT_OPEN);
 
-	napi_enable(&priv->napi);
+	can_rx_fifo_napi_enable(&priv->fifo);
 	netif_start_queue(dev);
 
 	return 0;
@@ -1270,7 +1136,7 @@ static int flexcan_close(struct net_device *dev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 
 	netif_stop_queue(dev);
-	napi_disable(&priv->napi);
+	can_rx_fifo_napi_disable(&priv->fifo);
 	flexcan_chip_stop(dev);
 
 	free_irq(dev->irq, dev);
@@ -1465,7 +1331,16 @@ static int flexcan_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->reg_xceiver))
 		priv->reg_xceiver = NULL;
 
-	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
+	/* Fill in RX-FIFO */
+	priv->fifo.low_first = FLEXCAN_RX_AREA_START;
+	priv->fifo.high_first = 32; /* FIXME: #define */
+	priv->fifo.high_last = 63; /* FIXME: #define */
+	priv->fifo.mailbox_enable_mask = flexcan_mailbox_enable_mask;
+	priv->fifo.mailbox_enable = flexcan_mailbox_enable;
+	priv->fifo.mailbox_move_to_buffer = flexcan_mailbox_move_to_buffer;
+	priv->fifo.poll_can_state = flexcan_poll_state;
+	priv->fifo.poll_bus_error = flexcan_poll_bus_err;
+	can_rx_fifo_add(dev, &priv->fifo);
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
@@ -1494,7 +1369,7 @@ static int flexcan_remove(struct platform_device *pdev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 
 	unregister_flexcandev(dev);
-	netif_napi_del(&priv->napi);
+	can_rx_fifo_del(&priv->fifo);
 	free_candev(dev);
 
 	return 0;
-- 
1.9.1

Re: [PATCH 12/15] can: flexcan: Add support for RX-FIFO.

[Marc Kleine-Budde]

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

On 10/10/2014 05:46 PM, David Jander wrote:
> This adds support for RX-FIFO simulating a FIFO from a linear array of message-
> boxes. This breaks RTR reception on flexcan older than V10.

Breaking things in one patch and fixing them later is not a good idea.
Please squash this patch into 4/15, so that we don't have an
intermediate non-rx-fifo flexcan driver.

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: 819 bytes --]

Re: [PATCH 12/15] can: flexcan: Add support for RX-FIFO.

[David Jander]

On Mon, 03 Nov 2014 12:26:21 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/10/2014 05:46 PM, David Jander wrote:
> > This adds support for RX-FIFO simulating a FIFO from a linear array of
> > message- boxes. This breaks RTR reception on flexcan older than V10.
> 
> Breaking things in one patch and fixing them later is not a good idea.
> Please squash this patch into 4/15, so that we don't have an
> intermediate non-rx-fifo flexcan driver.

Ok. I just thought that this series would be heavily squashed anyway before
moving further upstream....
Will try to squash that part.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH 12/15] can: flexcan: Add support for RX-FIFO.

[Marc Kleine-Budde]

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

On 11/03/2014 01:55 PM, David Jander wrote:
> On Mon, 03 Nov 2014 12:26:21 +0100
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 10/10/2014 05:46 PM, David Jander wrote:
>>> This adds support for RX-FIFO simulating a FIFO from a linear array of
>>> message- boxes. This breaks RTR reception on flexcan older than V10.
>>
>> Breaking things in one patch and fixing them later is not a good idea.
>> Please squash this patch into 4/15, so that we don't have an
>> intermediate non-rx-fifo flexcan driver.
> 
> Ok. I just thought that this series would be heavily squashed anyway before
> moving further upstream....

Yes, I'll squash all rx-fifo and flexcan related patches. But having my
first patch and your improvements seperate is better for a/my review, as
I'm (supposed to) know my patches :)

> Will try to squash that part.

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: 819 bytes --]

[PATCH 13/15] can: rx-fifo: Add support for simple irq offloading

[David Jander]

Some CAN controllers have a usable FIFO already but can still benefit from
off-loading the CAN controller FIFO in the interrupt into an extra ring-
buffer. Add support for these simpler cases also.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 76 ++++++++++++++++++++++++++++++++++++++++---------
 include/linux/can/dev.h |  2 ++
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index dac7579..278aea3 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -384,10 +384,32 @@ restart_poll:
 	return work_done;
 }
 
+static int can_rx_fifo_init_ring(struct net_device *dev,
+		struct can_rx_fifo *fifo, unsigned int weight)
+{
+	fifo->dev = dev;
+
+	/* Make ring-buffer a sensible size that is a power of 2 */
+	fifo->ring_size = (2 << fls(weight));
+	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
+			     GFP_KERNEL);
+	if (!fifo->ring)
+		return -ENOMEM;
+
+	fifo->ring_head = fifo->ring_tail = 0;
+
+	/* Take care of NAPI handling */
+	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
+
+	fifo->poll_errors = false;
+
+	return 0;
+}
+
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 {
 	unsigned int weight;
-	fifo->dev = dev;
+	int ret;
 
 	if ((fifo->low_first < fifo->high_first) &&
 	    (fifo->high_first < fifo->high_last)) {
@@ -405,23 +427,14 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 	    !fifo->poll_can_state)
 		return -EINVAL;
 
-	/* Make ring-buffer a sensible size that is a power of 2 */
-	fifo->ring_size = (2 << fls(weight));
-	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
-			     GFP_KERNEL);
-	if (!fifo->ring)
-		return -ENOMEM;
-
-	fifo->ring_head = fifo->ring_tail = 0;
-
-	/* Take care of NAPI handling */
-	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
+	ret = can_rx_fifo_init_ring(dev, fifo, weight);
+	if (ret)
+		return ret;
 
 	/* init variables */
 	fifo->mask_low = can_rx_fifo_mask_low(fifo);
 	fifo->mask_high = can_rx_fifo_mask_high(fifo);
 	fifo->second_first = false;
-	fifo->poll_errors = false;
 	fifo->active = fifo->mask_low | fifo->mask_high;
 	fifo->mailbox_enable_mask(fifo, fifo->active);
 
@@ -434,6 +447,26 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_add);
 
+int can_rx_fifo_add_simple(struct net_device *dev, struct can_rx_fifo *fifo)
+{
+	int ret;
+
+	if (!fifo->mailbox_move_to_buffer || !fifo->poll_bus_error ||
+	    !fifo->poll_can_state)
+		return -EINVAL;
+
+	ret = can_rx_fifo_init_ring(dev, fifo, 64);
+	if (ret)
+		return ret;
+
+	/* init variables */
+	fifo->mask_low = 0;
+	fifo->mask_high = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_add_simple);
+
 static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo, unsigned int n)
 {
 	unsigned int head = fifo->ring_head;
@@ -513,6 +546,23 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
 
+int can_rx_fifo_irq_offload_simple(struct can_rx_fifo *fifo)
+{
+	unsigned int received = 0;
+	unsigned int ret;
+
+	do {
+		ret = can_rx_fifo_offload_if_full(fifo, 0);
+		received += ret;
+	} while (ret);
+
+	if (received)
+		can_rx_fifo_napi_schedule(fifo);
+
+	return received;
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload_simple);
+
 void can_rx_fifo_irq_error(struct can_rx_fifo *fifo)
 {
 	fifo->poll_errors = true;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index e1ed6d4..18feef3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -142,7 +142,9 @@ u8 can_dlc2len(u8 can_dlc);
 u8 can_len2dlc(u8 len);
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
+int can_rx_fifo_add_simple(struct net_device *dev, struct can_rx_fifo *fifo);
 int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
+int can_rx_fifo_irq_offload_simple(struct can_rx_fifo *fifo);
 void can_rx_fifo_irq_error(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
 void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
-- 
1.9.1

Re: [PATCH 13/15] can: rx-fifo: Add support for simple irq offloading

[Marc Kleine-Budde]

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

On 10/10/2014 05:46 PM, David Jander wrote:
> Some CAN controllers have a usable FIFO already but can still benefit from
> off-loading the CAN controller FIFO in the interrupt into an extra ring-
> buffer. Add support for these simpler cases also.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/net/can/dev.c   | 76 ++++++++++++++++++++++++++++++++++++++++---------
>  include/linux/can/dev.h |  2 ++
>  2 files changed, 65 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index dac7579..278aea3 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -384,10 +384,32 @@ restart_poll:
>  	return work_done;
>  }
>  
> +static int can_rx_fifo_init_ring(struct net_device *dev,
> +		struct can_rx_fifo *fifo, unsigned int weight)
> +{
> +	fifo->dev = dev;
> +
> +	/* Make ring-buffer a sensible size that is a power of 2 */
> +	fifo->ring_size = (2 << fls(weight));
> +	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
> +			     GFP_KERNEL);
> +	if (!fifo->ring)
> +		return -ENOMEM;
> +
> +	fifo->ring_head = fifo->ring_tail = 0;
> +
> +	/* Take care of NAPI handling */
> +	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
> +
> +	fifo->poll_errors = false;
> +
> +	return 0;
> +}
> +
>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  {
>  	unsigned int weight;
> -	fifo->dev = dev;
> +	int ret;
>  
>  	if ((fifo->low_first < fifo->high_first) &&
>  	    (fifo->high_first < fifo->high_last)) {
> @@ -405,23 +427,14 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  	    !fifo->poll_can_state)
>  		return -EINVAL;
>  
> -	/* Make ring-buffer a sensible size that is a power of 2 */
> -	fifo->ring_size = (2 << fls(weight));
> -	fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
> -			     GFP_KERNEL);
> -	if (!fifo->ring)
> -		return -ENOMEM;
> -
> -	fifo->ring_head = fifo->ring_tail = 0;
> -
> -	/* Take care of NAPI handling */
> -	netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
> +	ret = can_rx_fifo_init_ring(dev, fifo, weight);
> +	if (ret)
> +		return ret;
>  
>  	/* init variables */
>  	fifo->mask_low = can_rx_fifo_mask_low(fifo);
>  	fifo->mask_high = can_rx_fifo_mask_high(fifo);
>  	fifo->second_first = false;
> -	fifo->poll_errors = false;
>  	fifo->active = fifo->mask_low | fifo->mask_high;
>  	fifo->mailbox_enable_mask(fifo, fifo->active);
>  
> @@ -434,6 +447,26 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_fifo_add);
>  
> +int can_rx_fifo_add_simple(struct net_device *dev, struct can_rx_fifo *fifo)
> +{
> +	int ret;
> +
> +	if (!fifo->mailbox_move_to_buffer || !fifo->poll_bus_error ||
> +	    !fifo->poll_can_state)
> +		return -EINVAL;
> +
> +	ret = can_rx_fifo_init_ring(dev, fifo, 64);
> +	if (ret)
> +		return ret;
> +
> +	/* init variables */
> +	fifo->mask_low = 0;
> +	fifo->mask_high = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(can_rx_fifo_add_simple);

I'd rather see, that we use the same rx_fifo_add() function to create
all rx-fifo available. Can we add a flag to struct can_rx_fifo?

> +
>  static unsigned int can_rx_fifo_offload_if_full(struct can_rx_fifo *fifo, unsigned int n)
>  {
>  	unsigned int head = fifo->ring_head;
> @@ -513,6 +546,23 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
>  
> +int can_rx_fifo_irq_offload_simple(struct can_rx_fifo *fifo)
> +{
> +	unsigned int received = 0;
> +	unsigned int ret;
> +
> +	do {
> +		ret = can_rx_fifo_offload_if_full(fifo, 0);
> +		received += ret;
> +	} while (ret);
> +
> +	if (received)
> +		can_rx_fifo_napi_schedule(fifo);
> +
> +	return received;
> +}
> +EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload_simple);

I think it's better to have only one offload function, that should do
the right thing depending on the struct can_rx_fifo.

> +
>  void can_rx_fifo_irq_error(struct can_rx_fifo *fifo)
>  {
>  	fifo->poll_errors = true;
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index e1ed6d4..18feef3 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -142,7 +142,9 @@ u8 can_dlc2len(u8 can_dlc);
>  u8 can_len2dlc(u8 len);
>  
>  int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
> +int can_rx_fifo_add_simple(struct net_device *dev, struct can_rx_fifo *fifo);
>  int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo);
> +int can_rx_fifo_irq_offload_simple(struct can_rx_fifo *fifo);
>  void can_rx_fifo_irq_error(struct can_rx_fifo *fifo);
>  void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo);
>  void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo);
> 

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: 819 bytes --]

[PATCH 14/15] can: flexcan: Add MB/Fifo specific column to comment table of IP versions

[David Jander]

Flexcan V10 and newer are able to receive RTR frames in a MB. Older
versions are not. Those should use flexcan in FIFO mode.

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

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 5e37ce4..b8d8058 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -197,14 +197,14 @@
  * FLEXCAN hardware feature flags
  *
  * Below is some version info we got:
- *    SOC   Version   IP-Version  Glitch-  [TR]WRN_INT  Memory err
- *                                Filter?   connected?  detection
- *   MX25  FlexCAN2  03.00.00.00     no         no         no
- *   MX28  FlexCAN2  03.00.04.00    yes        yes         no
- *   MX35  FlexCAN2  03.00.00.00     no         no         no
- *   MX53  FlexCAN2  03.00.00.00    yes         no         no
- *   MX6s  FlexCAN3  10.00.12.00    yes        yes         no
- *   VF610 FlexCAN3  ?               no        yes        yes
+ *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT  Memory err    RTR re-
+ *                                Filter?  connected?  detection   ception in MB
+ *   MX25  FlexCAN2  03.00.00.00     no        no         no         no
+ *   MX28  FlexCAN2  03.00.04.00    yes       yes         no         no
+ *   MX35  FlexCAN2  03.00.00.00     no        no         no         no
+ *   MX53  FlexCAN2  03.00.00.00    yes        no         no         no
+ *   MX6s  FlexCAN3  10.00.12.00    yes       yes         no        yes
+ *   VF610 FlexCAN3  ?               no       yes        yes        yes?
  *
  * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
  */
-- 
1.9.1

[PATCH 15/15] can: flexcan: Re-enable RTR reception support for older flexcan IPs

[David Jander]

Flexcan IPs prior to V10 need to work in FIFO mode in order to be able
to receive RTR frames, so restore FIFO-mode for older IPs while still
using rx-fifo IRQ offloading.

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

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index b8d8058..9ac89b8 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -164,12 +164,19 @@
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
-#define FLEXCAN_TX_BUF_RESERVED		0
-#define FLEXCAN_TX_BUF_ID		1
-#define FLEXCAN_RX_AREA_START		(FLEXCAN_TX_BUF_ID + 1)
+#define FLEXCAN_V10_TX_BUF_RESERVED	0
+#define FLEXCAN_V10_TX_BUF_ID		1
+#define FLEXCAN_TX_BUF_ID		8
+#define FLEXCAN_RX_AREA_START		(FLEXCAN_V10_TX_BUF_ID + 1)
 #define FLEXCAN_IFLAG_BUF(x)		BIT(x)
 #define FLEXCAN_IFLAG1_DEFAULT 		(0xfffffffe)
 #define FLEXCAN_IFLAG2_DEFAULT 		(0xffffffff)
+#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))
 
 /* FLEXCAN message buffers */
 #define FLEXCAN_MB_CODE_MASK		(0xf << 24)
@@ -286,6 +293,8 @@ struct flexcan_priv {
 	struct regulator *reg_xceiver;
 
 	struct can_rx_fifo fifo;
+
+	struct flexcan_mb __iomem *tx_mb;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {
@@ -515,25 +524,27 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (cf->can_dlc > 0) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[0]);
-		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
+		flexcan_write(data, &priv->tx_mb->data[0]);
 	}
 	if (cf->can_dlc > 3) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
-		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
+		flexcan_write(data, &priv->tx_mb->data[1]);
 	}
 
 	can_put_echo_skb(skb, dev, 0);
 
-	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
-	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	flexcan_write(can_id, &priv->tx_mb->can_id);
+	flexcan_write(ctrl, &priv->tx_mb->can_ctrl);
 
-	/* Errata ERR005829 step8:
-	 * Write twice INACTIVE(0x8) code to first MB.
-	 */
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* Errata ERR005829 step8:
+		 * Write twice INACTIVE(0x8) code to first MB.
+		 */
+		flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+		      &regs->cantxfg[FLEXCAN_V10_TX_BUF_RESERVED].can_ctrl);
+		flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+		      &regs->cantxfg[FLEXCAN_V10_TX_BUF_RESERVED].can_ctrl);
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -814,7 +825,40 @@ static unsigned int flexcan_mailbox_move_to_buffer(struct can_rx_fifo *fifo,
 	return 1;
 }
 
-static irqreturn_t flexcan_irq(int irq, void *dev_id)
+static unsigned int flexcan_fifo_move_to_buffer(struct can_rx_fifo *fifo,
+		struct can_frame *cf, unsigned int n)
+{
+	struct flexcan_priv *priv = rx_fifo_to_priv(fifo);
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
+	u32 reg_ctrl, reg_id, reg_iflag1;
+
+	reg_iflag1 = flexcan_read(&regs->iflag1);
+	if (!(reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE))
+		return 0;
+
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	reg_id = flexcan_read(&mb->can_id);
+	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
+		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
+	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);
+
+	*(__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);
+
+	return 1;
+}
+
+static irqreturn_t flexcan_v10_irq(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct net_device_stats *stats = &dev->stats;
@@ -842,11 +886,63 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	}
 
 	/* reception interrupt */
-	if ((reg_iflag1 & ~(1 << FLEXCAN_TX_BUF_ID)) || reg_iflag2) {
+	if ((reg_iflag1 & ~(1 << FLEXCAN_V10_TX_BUF_ID)) || reg_iflag2) {
 		can_rx_fifo_irq_offload(&priv->fifo);
 	}
 
 	/* transmission complete interrupt */
+	if (reg_iflag1 & (1 << FLEXCAN_V10_TX_BUF_ID)) {
+		stats->tx_bytes += can_get_echo_skb(dev, 0);
+		stats->tx_packets++;
+		can_led_event(dev, CAN_LED_EVENT_TX);
+		/* after sending a RTR frame mailbox is in RX mode */
+		flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+			      &regs->cantxfg[FLEXCAN_V10_TX_BUF_ID].can_ctrl);
+		flexcan_write((1 << FLEXCAN_V10_TX_BUF_ID), &regs->iflag1);
+		netif_wake_queue(dev);
+	}
+
+	return IRQ_HANDLED;
+}
+
+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;
+
+	reg_iflag1 = flexcan_read(&regs->iflag1);
+	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);
+
+	/* bus error IRQ and bus error reporting is activated */
+	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
+	    flexcan_has_and_handle_berr(priv, reg_esr)) {
+		/*
+		 * The error bits are cleared on read,
+		 * save them for later use and let napi poll get them.
+		 */
+		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
+		can_rx_fifo_irq_error(&priv->fifo);
+	}
+
+	/* reception interrupt */
+	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) {
+		can_rx_fifo_irq_offload_simple(&priv->fifo);
+	}
+
+	/* 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);
 		stats->tx_packets++;
@@ -938,11 +1034,18 @@ static int flexcan_chip_start(struct net_device *dev)
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
-	reg_mcr &= ~(FLEXCAN_MCR_MAXMB(0xff) | FLEXCAN_MCR_FEN);
+	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		reg_mcr &= ~FLEXCAN_MCR_FEN;
+		reg_mcr |= FLEXCAN_MCR_BCC |
+			FLEXCAN_MCR_MAXMB(FLEXCAN_MCR_MAXMB_USED);
+	} else {
+		reg_mcr |= FLEXCAN_MCR_FEN |
+			FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
+	}
 	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_BCC | FLEXCAN_MCR_MAXMB(FLEXCAN_MCR_MAXMB_USED);
+		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -986,19 +1089,31 @@ static int flexcan_chip_start(struct net_device *dev)
 	}
 
 	/* Prepare RX mailboxes */
-	for (i = FLEXCAN_RX_AREA_START; i < ARRAY_SIZE(regs->cantxfg); i++) {
-		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY,
-		      &regs->cantxfg[i].can_ctrl);
-		flexcan_write(0, &regs->rximr[i]); /* Clear filter */
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* Prepare RX mailboxes */
+		for (i = FLEXCAN_RX_AREA_START; i < ARRAY_SIZE(regs->cantxfg);
+		     i++) {
+			flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY,
+			      &regs->cantxfg[i].can_ctrl);
+			flexcan_write(0, &regs->rximr[i]); /* Clear filter */
+		}
+	} else {
+		/* clear and invalidate all mailboxes first */
+		for (i = FLEXCAN_TX_BUF_ID; i < ARRAY_SIZE(regs->cantxfg);
+		     i++) {
+			flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE,
+				      &regs->cantxfg[i].can_ctrl);
+		}
 	}
 
 	/* Prepare TX mailbox */
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE, &priv->tx_mb->can_ctrl);
 
-	/* Errata ERR005829: mark first TX mailbox as INACTIVE */
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* Errata ERR005829: mark first TX mailbox as INACTIVE */
+		flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+		      &regs->cantxfg[FLEXCAN_V10_TX_BUF_RESERVED].can_ctrl);
+	}
 
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
@@ -1036,11 +1151,17 @@ static int flexcan_chip_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	can_rx_fifo_reset(&priv->fifo);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
+		can_rx_fifo_reset(&priv->fifo);
 
-	/* enable all MB interrupts */
-	flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
-	flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* enable all MB interrupts */
+		flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
+		flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
+	} else {
+		/* enable FIFO interrupts */
+		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+	}
 
 	/* synchronize with the can bus */
 	err = flexcan_chip_unfreeze(priv);
@@ -1090,6 +1211,7 @@ static int flexcan_open(struct net_device *dev)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
 	int err;
+	irq_handler_t handler;
 
 	err = clk_prepare_enable(priv->clk_ipg);
 	if (err)
@@ -1103,7 +1225,12 @@ static int flexcan_open(struct net_device *dev)
 	if (err)
 		goto out_disable_per;
 
-	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
+		handler = flexcan_v10_irq;
+	else
+		handler = flexcan_irq;
+
+	err = request_irq(dev->irq, handler, IRQF_SHARED, dev->name, dev);
 	if (err)
 		goto out_close;
 
@@ -1260,6 +1387,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	const struct flexcan_devtype_data *devtype_data;
 	struct net_device *dev;
 	struct flexcan_priv *priv;
+	struct flexcan_regs __iomem *regs;
 	struct resource *mem;
 	struct clk *clk_ipg = NULL, *clk_per = NULL;
 	void __iomem *base;
@@ -1320,7 +1448,11 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
 		CAN_CTRLMODE_LISTENONLY	| CAN_CTRLMODE_3_SAMPLES |
 		CAN_CTRLMODE_BERR_REPORTING;
-	priv->base = base;
+	regs = priv->base = base;
+	if (devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
+		priv->tx_mb = &regs->cantxfg[FLEXCAN_V10_TX_BUF_ID];
+	else
+		priv->tx_mb = &regs->cantxfg[FLEXCAN_TX_BUF_ID];
 	priv->dev = dev;
 	priv->clk_ipg = clk_ipg;
 	priv->clk_per = clk_per;
@@ -1332,15 +1464,21 @@ static int flexcan_probe(struct platform_device *pdev)
 		priv->reg_xceiver = NULL;
 
 	/* Fill in RX-FIFO */
-	priv->fifo.low_first = FLEXCAN_RX_AREA_START;
-	priv->fifo.high_first = 32; /* FIXME: #define */
-	priv->fifo.high_last = 63; /* FIXME: #define */
-	priv->fifo.mailbox_enable_mask = flexcan_mailbox_enable_mask;
-	priv->fifo.mailbox_enable = flexcan_mailbox_enable;
-	priv->fifo.mailbox_move_to_buffer = flexcan_mailbox_move_to_buffer;
 	priv->fifo.poll_can_state = flexcan_poll_state;
 	priv->fifo.poll_bus_error = flexcan_poll_bus_err;
-	can_rx_fifo_add(dev, &priv->fifo);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		priv->fifo.low_first = FLEXCAN_RX_AREA_START;
+		priv->fifo.high_first = 32; /* FIXME: #define */
+		priv->fifo.high_last = 63; /* FIXME: #define */
+		priv->fifo.mailbox_enable_mask = flexcan_mailbox_enable_mask;
+		priv->fifo.mailbox_enable = flexcan_mailbox_enable;
+		priv->fifo.mailbox_move_to_buffer =
+			flexcan_mailbox_move_to_buffer;
+		can_rx_fifo_add(dev, &priv->fifo);
+	} else {
+		priv->fifo.mailbox_move_to_buffer = flexcan_fifo_move_to_buffer;
+		can_rx_fifo_add_simple(dev, &priv->fifo);
+	}
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
-- 
1.9.1

Re: [PATCH 15/15] can: flexcan: Re-enable RTR reception support for older flexcan IPs

[Marc Kleine-Budde]

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

On 10/10/2014 05:47 PM, David Jander wrote:
> Flexcan IPs prior to V10 need to work in FIFO mode in order to be able
> to receive RTR frames, so restore FIFO-mode for older IPs while still
> using rx-fifo IRQ offloading.

Please squash, so that we don't have a non functional driver in the mean
time.

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: 819 bytes --]