[PATCH v3 0/3] can: m_can: Merge FIFO ops to increase throughput

[PATCH v3 0/3] can: m_can: Merge FIFO ops to increase throughput

[Matt Kline]

As requested, I've propagated FIFO errors up to the m_can driver - on
failure we now log the error and disable interrupts, similar to
https://elixir.bootlin.com/linux/v5.13/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L2298

I've also folded the ID and DLC fields into a struct (as suggested) so
that we don't need to copy them to and from arrays for the FIFO transfers.

Following-up on
https://lore.kernel.org/linux-can/20210811063520.aw6hkll2kax22ytr@pengutronix.de/T/#u
Sorry for the slight delay - last week was busy!

Matt Kline (3):
  can: m_can: Disable IRQs on FIFO bus errors
  can: m_can: Batch FIFO reads during CAN receive
  can: m_can: Batch FIFO writes during CAN transmit

 drivers/net/can/m_can/m_can.c          | 221 ++++++++++++++++---------
 drivers/net/can/m_can/m_can.h          |   6 +-
 drivers/net/can/m_can/m_can_pci.c      |  11 +-
 drivers/net/can/m_can/m_can_platform.c |  15 +-
 drivers/net/can/m_can/tcan4x5x-core.c  |  16 +-
 5 files changed, 172 insertions(+), 97 deletions(-)

-- 
2.32.0

[PATCH v3 1/3] can: m_can: Disable IRQs on FIFO bus errors

[Matt Kline]

If FIFO reads or writes fail due to the underlying regmap (e.g., SPI)
I/O, propagate that up to the m_can driver, log an error, and disable
interrupts, similar to the mcp251xfd driver.

While reworking the FIFO functions to add this error handling,
add support for bulk reads and writes of multiple registers.

Signed-off-by: Matt Kline <matt@bitbashing.io>
---
 drivers/net/can/m_can/m_can.c          | 177 +++++++++++++++++--------
 drivers/net/can/m_can/m_can.h          |   6 +-
 drivers/net/can/m_can/m_can_pci.c      |  11 +-
 drivers/net/can/m_can/m_can_platform.c |  15 ++-
 drivers/net/can/m_can/tcan4x5x-core.c  |  16 +--
 5 files changed, 152 insertions(+), 73 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 43bca315a66c..83eb5cd51de5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -319,36 +319,36 @@ static inline void m_can_write(struct m_can_classdev *cdev, enum m_can_reg reg,
 	cdev->ops->write_reg(cdev, reg, val);
 }
 
-static u32 m_can_fifo_read(struct m_can_classdev *cdev,
-			   u32 fgi, unsigned int offset)
+static int m_can_fifo_read(struct m_can_classdev *cdev,
+			   u32 fgi, unsigned int offset, void *val, size_t val_count)
 {
 	u32 addr_offset = cdev->mcfg[MRAM_RXF0].off + fgi * RXF0_ELEMENT_SIZE +
 		offset;
 
-	return cdev->ops->read_fifo(cdev, addr_offset);
+	return cdev->ops->read_fifo(cdev, addr_offset, val, val_count);
 }
 
-static void m_can_fifo_write(struct m_can_classdev *cdev,
-			     u32 fpi, unsigned int offset, u32 val)
+static int m_can_fifo_write(struct m_can_classdev *cdev,
+			    u32 fpi, unsigned int offset, const void *val, size_t val_count)
 {
 	u32 addr_offset = cdev->mcfg[MRAM_TXB].off + fpi * TXB_ELEMENT_SIZE +
 		offset;
 
-	cdev->ops->write_fifo(cdev, addr_offset, val);
+	return cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
 }
 
-static inline void m_can_fifo_write_no_off(struct m_can_classdev *cdev,
-					   u32 fpi, u32 val)
+static inline int m_can_fifo_write_no_off(struct m_can_classdev *cdev,
+					  u32 fpi, u32 val)
 {
-	cdev->ops->write_fifo(cdev, fpi, val);
+	return cdev->ops->write_fifo(cdev, fpi, &val, 1);
 }
 
-static u32 m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset)
+static int m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
 {
 	u32 addr_offset = cdev->mcfg[MRAM_TXE].off + fgi * TXE_ELEMENT_SIZE +
 		offset;
 
-	return cdev->ops->read_fifo(cdev, addr_offset);
+	return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
 }
 
 static inline bool m_can_tx_fifo_full(struct m_can_classdev *cdev)
@@ -454,7 +454,7 @@ static void m_can_receive_skb(struct m_can_classdev *cdev,
 	}
 }
 
-static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
+static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
 {
 	struct net_device_stats *stats = &dev->stats;
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -462,18 +462,21 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	struct sk_buff *skb;
 	u32 id, fgi, dlc;
 	u32 timestamp = 0;
-	int i;
+	int i, err;
 
 	/* calculate the fifo get index for where to read data */
 	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
-	dlc = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC);
+	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC, &dlc, 1);
+	if (err)
+		goto out_fail;
+
 	if (dlc & RX_BUF_FDF)
 		skb = alloc_canfd_skb(dev, &cf);
 	else
 		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
 	if (!skb) {
 		stats->rx_dropped++;
-		return;
+		return 0;
 	}
 
 	if (dlc & RX_BUF_FDF)
@@ -481,7 +484,10 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	else
 		cf->len = can_cc_dlc2len((dlc >> 16) & 0x0F);
 
-	id = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID);
+	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &id, 1);
+	if (err)
+		goto out_fail;
+
 	if (id & RX_BUF_XTD)
 		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -498,10 +504,11 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 		if (dlc & RX_BUF_BRS)
 			cf->flags |= CANFD_BRS;
 
-		for (i = 0; i < cf->len; i += 4)
-			*(u32 *)(cf->data + i) =
-				m_can_fifo_read(cdev, fgi,
-						M_CAN_FIFO_DATA(i / 4));
+		for (i = 0; i < cf->len; i += 4) {
+			err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
+			if (err)
+				goto out_fail;
+		}
 	}
 
 	/* acknowledge rx fifo 0 */
@@ -513,6 +520,12 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, dlc);
 
 	m_can_receive_skb(cdev, skb, timestamp);
+
+	return 0;
+
+out_fail:
+	netdev_err(dev, "FIFO read returned %d\n", err);
+	return err;
 }
 
 static int m_can_do_rx_poll(struct net_device *dev, int quota)
@@ -520,6 +533,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 pkts = 0;
 	u32 rxfs;
+	int err;
 
 	rxfs = m_can_read(cdev, M_CAN_RXF0S);
 	if (!(rxfs & RXFS_FFL_MASK)) {
@@ -528,7 +542,14 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 	}
 
 	while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
-		m_can_read_fifo(dev, rxfs);
+		err = m_can_read_fifo(dev, rxfs);
+		if (err) {
+			/* Trust that an error back from a FIFO read is negative
+			 * so that we can differentiate it from work done (pkts).
+			 */
+			WARN_ON(err >= 0);
+			return err;
+		}
 
 		quota--;
 		pkts++;
@@ -874,6 +895,7 @@ static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
 static int m_can_rx_handler(struct net_device *dev, int quota)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
+	int rx_work_or_err;
 	int work_done = 0;
 	u32 irqstatus, psr;
 
@@ -910,8 +932,13 @@ static int m_can_rx_handler(struct net_device *dev, int quota)
 	if (irqstatus & IR_ERR_BUS_30X)
 		work_done += m_can_handle_bus_errors(dev, irqstatus, psr);
 
-	if (irqstatus & IR_RF0N)
-		work_done += m_can_do_rx_poll(dev, (quota - work_done));
+	if (irqstatus & IR_RF0N) {
+		rx_work_or_err = m_can_do_rx_poll(dev, (quota - work_done));
+		if (rx_work_or_err < 0)
+			return rx_work_or_err;
+
+		work_done += rx_work_or_err;
+	}
 end:
 	return work_done;
 }
@@ -919,12 +946,16 @@ static int m_can_rx_handler(struct net_device *dev, int quota)
 static int m_can_rx_peripheral(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
+	int work_done;
 
-	m_can_rx_handler(dev, M_CAN_NAPI_WEIGHT);
+	work_done = m_can_rx_handler(dev, M_CAN_NAPI_WEIGHT);
 
-	m_can_enable_all_interrupts(cdev);
+	// Don't re-enable interrupts if the driver had a fatal error
+	// (e.g., FIFO read failure).
+	if (work_done >= 0)
+		m_can_enable_all_interrupts(cdev);
 
-	return 0;
+	return work_done;
 }
 
 static int m_can_poll(struct napi_struct *napi, int quota)
@@ -934,7 +965,10 @@ static int m_can_poll(struct napi_struct *napi, int quota)
 	int work_done;
 
 	work_done = m_can_rx_handler(dev, quota);
-	if (work_done < quota) {
+
+	// Don't re-enable interrupts if the driver had a fatal error
+	// (e.g., FIFO read failure).
+	if (work_done >= 0 && work_done < quota) {
 		napi_complete_done(napi, work_done);
 		m_can_enable_all_interrupts(cdev);
 	}
@@ -965,7 +999,7 @@ static void m_can_tx_update_stats(struct m_can_classdev *cdev,
 	stats->tx_packets++;
 }
 
-static void m_can_echo_tx_event(struct net_device *dev)
+static int m_can_echo_tx_event(struct net_device *dev)
 {
 	u32 txe_count = 0;
 	u32 m_can_txefs;
@@ -984,12 +1018,18 @@ static void m_can_echo_tx_event(struct net_device *dev)
 	/* Get and process all sent elements */
 	for (i = 0; i < txe_count; i++) {
 		u32 txe, timestamp = 0;
+		int err;
 
 		/* retrieve get index */
 		fgi = FIELD_GET(TXEFS_EFGI_MASK, m_can_read(cdev, M_CAN_TXEFS));
 
 		/* get message marker, timestamp */
-		txe = m_can_txe_fifo_read(cdev, fgi, 4);
+		err = m_can_txe_fifo_read(cdev, fgi, 4, &txe);
+		if (err) {
+			netdev_err(dev, "TXE FIFO read returned %d\n", err);
+			return err;
+		}
+
 		msg_mark = FIELD_GET(TX_EVENT_MM_MASK, txe);
 		timestamp = FIELD_GET(TX_EVENT_TXTS_MASK, txe);
 
@@ -1000,6 +1040,8 @@ static void m_can_echo_tx_event(struct net_device *dev)
 		/* update stats */
 		m_can_tx_update_stats(cdev, msg_mark, timestamp);
 	}
+
+	return 0;
 }
 
 static irqreturn_t m_can_isr(int irq, void *dev_id)
@@ -1031,8 +1073,8 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 		m_can_disable_all_interrupts(cdev);
 		if (!cdev->is_peripheral)
 			napi_schedule(&cdev->napi);
-		else
-			m_can_rx_peripheral(dev);
+		else if (m_can_rx_peripheral(dev) < 0)
+			goto out_fail;
 	}
 
 	if (cdev->version == 30) {
@@ -1050,7 +1092,9 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	} else  {
 		if (ir & IR_TEFN) {
 			/* New TX FIFO Element arrived */
-			m_can_echo_tx_event(dev);
+			if (m_can_echo_tx_event(dev) != 0)
+				goto out_fail;
+
 			can_led_event(dev, CAN_LED_EVENT_TX);
 			if (netif_queue_stopped(dev) &&
 			    !m_can_tx_fifo_full(cdev))
@@ -1059,6 +1103,10 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	}
 
 	return IRQ_HANDLED;
+
+out_fail:
+	m_can_disable_all_interrupts(cdev);
+	return IRQ_HANDLED;
 }
 
 static const struct can_bittiming_const m_can_bittiming_const_30X = {
@@ -1540,8 +1588,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct canfd_frame *cf = (struct canfd_frame *)cdev->tx_skb->data;
 	struct net_device *dev = cdev->net;
 	struct sk_buff *skb = cdev->tx_skb;
-	u32 id, cccr, fdflags;
-	int i;
+	u32 id, dlc, cccr, fdflags;
+	int i, err;
 	int putidx;
 
 	cdev->tx_skb = NULL;
@@ -1562,14 +1610,20 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		netif_stop_queue(dev);
 
 		/* message ram configuration */
-		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id);
-		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC,
-				 can_fd_len2dlc(cf->len) << 16);
+		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, &id, 1);
+		if (err)
+			goto out_fail;
 
-		for (i = 0; i < cf->len; i += 4)
-			m_can_fifo_write(cdev, 0,
-					 M_CAN_FIFO_DATA(i / 4),
-					 *(u32 *)(cf->data + i));
+		dlc = can_fd_len2dlc(cf->len) << 16;
+		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC, &dlc, 1);
+		if (err)
+			goto out_fail;
+
+		for (i = 0; i < cf->len; i += 4) {
+			err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
+			if (err)
+				goto out_fail;
+		}
 
 		can_put_echo_skb(skb, dev, 0, 0);
 
@@ -1614,7 +1668,9 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		putidx = FIELD_GET(TXFQS_TFQPI_MASK,
 				   m_can_read(cdev, M_CAN_TXFQS));
 		/* Write ID Field to FIFO Element */
-		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id);
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &id, 1);
+		if (err)
+			goto out_fail;
 
 		/* get CAN FD configuration of frame */
 		fdflags = 0;
@@ -1629,15 +1685,19 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		 * it is used in TX interrupt for
 		 * sending the correct echo frame
 		 */
-		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DLC,
-				 FIELD_PREP(TX_BUF_MM_MASK, putidx) |
-				 FIELD_PREP(TX_BUF_DLC_MASK,
-					    can_fd_len2dlc(cf->len)) |
-				 fdflags | TX_BUF_EFC);
+		dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
+			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
+			fdflags | TX_BUF_EFC;
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DLC, &dlc, 1);
+		if (err)
+			goto out_fail;
 
-		for (i = 0; i < cf->len; i += 4)
-			m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
-					 *(u32 *)(cf->data + i));
+		for (i = 0; i < cf->len; i += 4) {
+			err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
+					       cf->data + i, 1);
+			if (err)
+				goto out_fail;
+		}
 
 		/* Push loopback echo.
 		 * Will be looped back on TX interrupt based on message marker
@@ -1654,6 +1714,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	}
 
 	return NETDEV_TX_OK;
+
+out_fail:
+	netdev_err(dev, "FIFO write returned %d\n", err);
+	m_can_disable_all_interrupts(cdev);
+	return NETDEV_TX_BUSY;
 }
 
 static void m_can_tx_work_queue(struct work_struct *ws)
@@ -1819,9 +1884,10 @@ static void m_can_of_parse_mram(struct m_can_classdev *cdev,
 		cdev->mcfg[MRAM_TXB].off, cdev->mcfg[MRAM_TXB].num);
 }
 
-void m_can_init_ram(struct m_can_classdev *cdev)
+int m_can_init_ram(struct m_can_classdev *cdev)
 {
 	int end, i, start;
+	int err = 0;
 
 	/* initialize the entire Message RAM in use to avoid possible
 	 * ECC/parity checksum errors when reading an uninitialized buffer
@@ -1830,8 +1896,13 @@ void m_can_init_ram(struct m_can_classdev *cdev)
 	end = cdev->mcfg[MRAM_TXB].off +
 		cdev->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
 
-	for (i = start; i < end; i += 4)
-		m_can_fifo_write_no_off(cdev, i, 0x0);
+	for (i = start; i < end; i += 4) {
+		err = m_can_fifo_write_no_off(cdev, i, 0x0);
+		if (err)
+			break;
+	}
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(m_can_init_ram);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index ace071c3e58c..7a3bc878535c 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -64,9 +64,9 @@ struct m_can_ops {
 	int (*clear_interrupts)(struct m_can_classdev *cdev);
 	u32 (*read_reg)(struct m_can_classdev *cdev, int reg);
 	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
-	u32 (*read_fifo)(struct m_can_classdev *cdev, int addr_offset);
+	int (*read_fifo)(struct m_can_classdev *cdev, int addr_offset, void *val, size_t val_count);
 	int (*write_fifo)(struct m_can_classdev *cdev, int addr_offset,
-			  int val);
+			  const void *val, size_t val_count);
 	int (*init)(struct m_can_classdev *cdev);
 };
 
@@ -102,7 +102,7 @@ void m_can_class_free_dev(struct net_device *net);
 int m_can_class_register(struct m_can_classdev *cdev);
 void m_can_class_unregister(struct m_can_classdev *cdev);
 int m_can_class_get_clocks(struct m_can_classdev *cdev);
-void m_can_init_ram(struct m_can_classdev *priv);
+int m_can_init_ram(struct m_can_classdev *priv);
 
 int m_can_class_suspend(struct device *dev);
 int m_can_class_resume(struct device *dev);
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index 128808605c3f..89cc3d41e952 100644
--- a/drivers/net/can/m_can/m_can_pci.c
+++ b/drivers/net/can/m_can/m_can_pci.c
@@ -39,11 +39,13 @@ static u32 iomap_read_reg(struct m_can_classdev *cdev, int reg)
 	return readl(priv->base + reg);
 }
 
-static u32 iomap_read_fifo(struct m_can_classdev *cdev, int offset)
+static int iomap_read_fifo(struct m_can_classdev *cdev, int offset, void *val, size_t val_count)
 {
 	struct m_can_pci_priv *priv = cdev_to_priv(cdev);
 
-	return readl(priv->base + offset);
+	ioread32_rep(priv->base + offset, val, val_count);
+
+	return 0;
 }
 
 static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
@@ -55,11 +57,12 @@ static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
 	return 0;
 }
 
-static int iomap_write_fifo(struct m_can_classdev *cdev, int offset, int val)
+static int iomap_write_fifo(struct m_can_classdev *cdev, int offset,
+			    const void *val, size_t val_count)
 {
 	struct m_can_pci_priv *priv = cdev_to_priv(cdev);
 
-	writel(val, priv->base + offset);
+	iowrite32_rep(priv->base + offset, val, val_count);
 
 	return 0;
 }
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 599de0e08cd7..12dec09cbd9f 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -28,11 +28,13 @@ static u32 iomap_read_reg(struct m_can_classdev *cdev, int reg)
 	return readl(priv->base + reg);
 }
 
-static u32 iomap_read_fifo(struct m_can_classdev *cdev, int offset)
+static int iomap_read_fifo(struct m_can_classdev *cdev, int offset, void *val, size_t val_count)
 {
 	struct m_can_plat_priv *priv = cdev_to_priv(cdev);
 
-	return readl(priv->mram_base + offset);
+	ioread32_rep(priv->mram_base + offset, val, val_count);
+
+	return 0;
 }
 
 static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
@@ -44,11 +46,12 @@ static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
 	return 0;
 }
 
-static int iomap_write_fifo(struct m_can_classdev *cdev, int offset, int val)
+static int iomap_write_fifo(struct m_can_classdev *cdev, int offset,
+			    const void *val, size_t val_count)
 {
 	struct m_can_plat_priv *priv = cdev_to_priv(cdev);
 
-	writel(val, priv->mram_base + offset);
+	iowrite32_rep(priv->base + offset, val, val_count);
 
 	return 0;
 }
@@ -115,7 +118,9 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mcan_class);
 
-	m_can_init_ram(mcan_class);
+	ret = m_can_init_ram(mcan_class);
+	if (ret)
+		goto probe_fail;
 
 	pm_runtime_enable(mcan_class->dev);
 	ret = m_can_class_register(mcan_class);
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 4147cecfbbd6..11c096fe94a2 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -154,14 +154,12 @@ static u32 tcan4x5x_read_reg(struct m_can_classdev *cdev, int reg)
 	return val;
 }
 
-static u32 tcan4x5x_read_fifo(struct m_can_classdev *cdev, int addr_offset)
+static int tcan4x5x_read_fifo(struct m_can_classdev *cdev, int addr_offset,
+			      void *val, size_t val_count)
 {
 	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
-	u32 val;
 
-	regmap_read(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, &val);
-
-	return val;
+	return regmap_bulk_read(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
 }
 
 static int tcan4x5x_write_reg(struct m_can_classdev *cdev, int reg, int val)
@@ -172,11 +170,11 @@ static int tcan4x5x_write_reg(struct m_can_classdev *cdev, int reg, int val)
 }
 
 static int tcan4x5x_write_fifo(struct m_can_classdev *cdev,
-			       int addr_offset, int val)
+			       int addr_offset, const void *val, size_t val_count)
 {
 	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
 
-	return regmap_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val);
+	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
 }
 
 static int tcan4x5x_power_enable(struct regulator *reg, int enable)
@@ -238,7 +236,9 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
 		return ret;
 
 	/* Zero out the MCAN buffers */
-	m_can_init_ram(cdev);
+	ret = m_can_init_ram(cdev);
+	if (ret)
+		return ret;
 
 	ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
 				 TCAN4X5X_MODE_SEL_MASK, TCAN4X5X_MODE_NORMAL);
-- 
2.32.0

[PATCH v3 2/3] can: m_can: Batch FIFO reads during CAN receive

[Matt Kline]

On peripherals communicating over a relatively slow SPI line
(e.g. tcan4x5x), individual transfers have high fixed costs.
This causes the driver to spend most of its time waiting between
transfers and severely limits throughput.

Reduce these overheads by reading more than one word at a time.
Writing could get a similar treatment in follow-on commits.

Signed-off-by: Matt Kline <matt@bitbashing.io>
---
 drivers/net/can/m_can/m_can.c | 51 +++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 83eb5cd51de5..85d6cd03bff1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -308,6 +308,15 @@ enum m_can_reg {
 #define TX_EVENT_MM_MASK	GENMASK(31, 24)
 #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
 
+/* The ID and DLC registers are adjacent in M_CAN FIFO memory,
+ * and we can save a (potentially slow) bus round trip by combining
+ * reads and writes to them.
+ */
+struct __packed id_and_dlc {
+	u32 id;
+	u32 dlc;
+};
+
 static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
 {
 	return cdev->ops->read_reg(cdev, reg);
@@ -460,17 +469,18 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	struct canfd_frame *cf;
 	struct sk_buff *skb;
-	u32 id, fgi, dlc;
+	struct id_and_dlc fifo_header;
+	u32 fgi;
 	u32 timestamp = 0;
-	int i, err;
+	int err;
 
 	/* calculate the fifo get index for where to read data */
 	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
-	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC, &dlc, 1);
+	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &fifo_header, 2);
 	if (err)
 		goto out_fail;
 
-	if (dlc & RX_BUF_FDF)
+	if (fifo_header.dlc & RX_BUF_FDF)
 		skb = alloc_canfd_skb(dev, &cf);
 	else
 		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
@@ -479,36 +489,31 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
 		return 0;
 	}
 
-	if (dlc & RX_BUF_FDF)
-		cf->len = can_fd_dlc2len((dlc >> 16) & 0x0F);
+	if (fifo_header.dlc & RX_BUF_FDF)
+		cf->len = can_fd_dlc2len((fifo_header.dlc >> 16) & 0x0F);
 	else
-		cf->len = can_cc_dlc2len((dlc >> 16) & 0x0F);
+		cf->len = can_cc_dlc2len((fifo_header.dlc >> 16) & 0x0F);
 
-	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &id, 1);
-	if (err)
-		goto out_fail;
-
-	if (id & RX_BUF_XTD)
-		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
+	if (fifo_header.id & RX_BUF_XTD)
+		cf->can_id = (fifo_header.id & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
-		cf->can_id = (id >> 18) & CAN_SFF_MASK;
+		cf->can_id = (fifo_header.id >> 18) & CAN_SFF_MASK;
 
-	if (id & RX_BUF_ESI) {
+	if (fifo_header.id & RX_BUF_ESI) {
 		cf->flags |= CANFD_ESI;
 		netdev_dbg(dev, "ESI Error\n");
 	}
 
-	if (!(dlc & RX_BUF_FDF) && (id & RX_BUF_RTR)) {
+	if (!(fifo_header.dlc & RX_BUF_FDF) && (fifo_header.id & RX_BUF_RTR)) {
 		cf->can_id |= CAN_RTR_FLAG;
 	} else {
-		if (dlc & RX_BUF_BRS)
+		if (fifo_header.dlc & RX_BUF_BRS)
 			cf->flags |= CANFD_BRS;
 
-		for (i = 0; i < cf->len; i += 4) {
-			err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
-			if (err)
-				goto out_fail;
-		}
+		err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0),
+				      cf->data, DIV_ROUND_UP(cf->len, 4));
+		if (err)
+			goto out_fail;
 	}
 
 	/* acknowledge rx fifo 0 */
@@ -517,7 +522,7 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	stats->rx_packets++;
 	stats->rx_bytes += cf->len;
 
-	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, dlc);
+	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc);
 
 	m_can_receive_skb(cdev, skb, timestamp);
 
-- 
2.32.0

[PATCH v3 3/3] can: m_can: Batch FIFO writes during CAN transmit

[Matt Kline]

Give FIFO writes the same treatment as reads to avoid fixed costs of
individual transfers on a slow bus (e.g., tcan4x5x).

Signed-off-by: Matt Kline <matt@bitbashing.io>
---
 drivers/net/can/m_can/m_can.c | 61 +++++++++++++++--------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 85d6cd03bff1..b0e20c7d596c 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -278,7 +278,7 @@ enum m_can_reg {
 /* Message RAM Elements */
 #define M_CAN_FIFO_ID		0x0
 #define M_CAN_FIFO_DLC		0x4
-#define M_CAN_FIFO_DATA(n)	(0x8 + ((n) << 2))
+#define M_CAN_FIFO_DATA		0x8
 
 /* Rx Buffer Element */
 /* R0 */
@@ -510,7 +510,7 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
 		if (fifo_header.dlc & RX_BUF_BRS)
 			cf->flags |= CANFD_BRS;
 
-		err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0),
+		err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA,
 				      cf->data, DIV_ROUND_UP(cf->len, 4));
 		if (err)
 			goto out_fail;
@@ -1593,8 +1593,9 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct canfd_frame *cf = (struct canfd_frame *)cdev->tx_skb->data;
 	struct net_device *dev = cdev->net;
 	struct sk_buff *skb = cdev->tx_skb;
-	u32 id, dlc, cccr, fdflags;
-	int i, err;
+	struct id_and_dlc fifo_header;
+	u32 cccr, fdflags;
+	int err;
 	int putidx;
 
 	cdev->tx_skb = NULL;
@@ -1602,34 +1603,30 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	/* Generate ID field for TX buffer Element */
 	/* Common to all supported M_CAN versions */
 	if (cf->can_id & CAN_EFF_FLAG) {
-		id = cf->can_id & CAN_EFF_MASK;
-		id |= TX_BUF_XTD;
+		fifo_header.id = cf->can_id & CAN_EFF_MASK;
+		fifo_header.id |= TX_BUF_XTD;
 	} else {
-		id = ((cf->can_id & CAN_SFF_MASK) << 18);
+		fifo_header.id = ((cf->can_id & CAN_SFF_MASK) << 18);
 	}
 
 	if (cf->can_id & CAN_RTR_FLAG)
-		id |= TX_BUF_RTR;
+		fifo_header.id |= TX_BUF_RTR;
 
 	if (cdev->version == 30) {
 		netif_stop_queue(dev);
 
-		/* message ram configuration */
-		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, &id, 1);
+		fifo_header.dlc = can_fd_len2dlc(cf->len) << 16;
+
+		/* Write the frame ID, DLC, and payload to the FIFO element. */
+		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, &fifo_header, 2);
 		if (err)
 			goto out_fail;
 
-		dlc = can_fd_len2dlc(cf->len) << 16;
-		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC, &dlc, 1);
+		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA,
+				       cf->data, DIV_ROUND_UP(cf->len, 4));
 		if (err)
 			goto out_fail;
 
-		for (i = 0; i < cf->len; i += 4) {
-			err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
-			if (err)
-				goto out_fail;
-		}
-
 		can_put_echo_skb(skb, dev, 0, 0);
 
 		if (cdev->can.ctrlmode & CAN_CTRLMODE_FD) {
@@ -1672,10 +1669,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		/* get put index for frame */
 		putidx = FIELD_GET(TXFQS_TFQPI_MASK,
 				   m_can_read(cdev, M_CAN_TXFQS));
-		/* Write ID Field to FIFO Element */
-		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &id, 1);
-		if (err)
-			goto out_fail;
+
+		/* Construct DLC Field, with CAN-FD configuration.
+		 * Use the put index of the fifo as the message marker,
+		 * used in the TX interrupt for sending the correct echo frame.
+		 */
 
 		/* get CAN FD configuration of frame */
 		fdflags = 0;
@@ -1685,24 +1683,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 				fdflags |= TX_BUF_BRS;
 		}
 
-		/* Construct DLC Field. Also contains CAN-FD configuration
-		 * use put index of fifo as message marker
-		 * it is used in TX interrupt for
-		 * sending the correct echo frame
-		 */
-		dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
+		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
 			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
 			fdflags | TX_BUF_EFC;
-		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DLC, &dlc, 1);
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
 		if (err)
 			goto out_fail;
 
-		for (i = 0; i < cf->len; i += 4) {
-			err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
-					       cf->data + i, 1);
-			if (err)
-				goto out_fail;
-		}
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
+				       cf->data, DIV_ROUND_UP(cf->len, 4));
+		if (err)
+			goto out_fail;
 
 		/* Push loopback echo.
 		 * Will be looped back on TX interrupt based on message marker
-- 
2.32.0

Re: [PATCH v3 1/3] can: m_can: Disable IRQs on FIFO bus errors

[Marc Kleine-Budde]

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

On 16.08.2021 22:08:51, Matt Kline wrote:
> If FIFO reads or writes fail due to the underlying regmap (e.g., SPI)
> I/O, propagate that up to the m_can driver, log an error, and disable
> interrupts, similar to the mcp251xfd driver.
> 
> While reworking the FIFO functions to add this error handling,
> add support for bulk reads and writes of multiple registers.
> 
> Signed-off-by: Matt Kline <matt@bitbashing.io>

Applied with some minor changes, see inline.

regards,
Marc

> ---
>  drivers/net/can/m_can/m_can.c          | 177 +++++++++++++++++--------
>  drivers/net/can/m_can/m_can.h          |   6 +-
>  drivers/net/can/m_can/m_can_pci.c      |  11 +-
>  drivers/net/can/m_can/m_can_platform.c |  15 ++-
>  drivers/net/can/m_can/tcan4x5x-core.c  |  16 +--
>  5 files changed, 152 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 43bca315a66c..83eb5cd51de5 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -319,36 +319,36 @@ static inline void m_can_write(struct m_can_classdev *cdev, enum m_can_reg reg,
>  	cdev->ops->write_reg(cdev, reg, val);
>  }
>  
> -static u32 m_can_fifo_read(struct m_can_classdev *cdev,
> -			   u32 fgi, unsigned int offset)
> +static int m_can_fifo_read(struct m_can_classdev *cdev,
> +			   u32 fgi, unsigned int offset, void *val, size_t val_count)

I've reformated some long lines like these to stay below 80 chars.

>  {
>  	u32 addr_offset = cdev->mcfg[MRAM_RXF0].off + fgi * RXF0_ELEMENT_SIZE +
>  		offset;
>  
> -	return cdev->ops->read_fifo(cdev, addr_offset);
> +	return cdev->ops->read_fifo(cdev, addr_offset, val, val_count);
>  }
>  
> -static void m_can_fifo_write(struct m_can_classdev *cdev,
> -			     u32 fpi, unsigned int offset, u32 val)
> +static int m_can_fifo_write(struct m_can_classdev *cdev,
> +			    u32 fpi, unsigned int offset, const void *val, size_t val_count)
>  {
>  	u32 addr_offset = cdev->mcfg[MRAM_TXB].off + fpi * TXB_ELEMENT_SIZE +
>  		offset;
>  
> -	cdev->ops->write_fifo(cdev, addr_offset, val);
> +	return cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
>  }
>  
> -static inline void m_can_fifo_write_no_off(struct m_can_classdev *cdev,
> -					   u32 fpi, u32 val)
> +static inline int m_can_fifo_write_no_off(struct m_can_classdev *cdev,
> +					  u32 fpi, u32 val)
>  {
> -	cdev->ops->write_fifo(cdev, fpi, val);
> +	return cdev->ops->write_fifo(cdev, fpi, &val, 1);
>  }
>  
> -static u32 m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset)
> +static int m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
>  {
>  	u32 addr_offset = cdev->mcfg[MRAM_TXE].off + fgi * TXE_ELEMENT_SIZE +
>  		offset;
>  
> -	return cdev->ops->read_fifo(cdev, addr_offset);
> +	return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
>  }
>  
>  static inline bool m_can_tx_fifo_full(struct m_can_classdev *cdev)
> @@ -454,7 +454,7 @@ static void m_can_receive_skb(struct m_can_classdev *cdev,
>  	}
>  }
>  
> -static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
> +static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  {
>  	struct net_device_stats *stats = &dev->stats;
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -462,18 +462,21 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	struct sk_buff *skb;
>  	u32 id, fgi, dlc;
>  	u32 timestamp = 0;
> -	int i;
> +	int i, err;
>  
>  	/* calculate the fifo get index for where to read data */
>  	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
> -	dlc = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC);
> +	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC, &dlc, 1);
> +	if (err)
> +		goto out_fail;
> +
>  	if (dlc & RX_BUF_FDF)
>  		skb = alloc_canfd_skb(dev, &cf);
>  	else
>  		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
>  	if (!skb) {
>  		stats->rx_dropped++;
> -		return;
> +		return 0;
>  	}
>  
>  	if (dlc & RX_BUF_FDF)
> @@ -481,7 +484,10 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	else
>  		cf->len = can_cc_dlc2len((dlc >> 16) & 0x0F);
>  
> -	id = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID);
> +	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &id, 1);
> +	if (err)
> +		goto out_fail;
> +
>  	if (id & RX_BUF_XTD)
>  		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
>  	else
> @@ -498,10 +504,11 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  		if (dlc & RX_BUF_BRS)
>  			cf->flags |= CANFD_BRS;
>  
> -		for (i = 0; i < cf->len; i += 4)
> -			*(u32 *)(cf->data + i) =
> -				m_can_fifo_read(cdev, fgi,
> -						M_CAN_FIFO_DATA(i / 4));
> +		for (i = 0; i < cf->len; i += 4) {
> +			err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
> +			if (err)
> +				goto out_fail;
> +		}
>  	}
>  
>  	/* acknowledge rx fifo 0 */
> @@ -513,6 +520,12 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, dlc);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
> +
> +	return 0;
> +
> +out_fail:
> +	netdev_err(dev, "FIFO read returned %d\n", err);
> +	return err;
>  }
>  
>  static int m_can_do_rx_poll(struct net_device *dev, int quota)
> @@ -520,6 +533,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	u32 pkts = 0;
>  	u32 rxfs;
> +	int err;
>  
>  	rxfs = m_can_read(cdev, M_CAN_RXF0S);
>  	if (!(rxfs & RXFS_FFL_MASK)) {
> @@ -528,7 +542,14 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  	}
>  
>  	while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> -		m_can_read_fifo(dev, rxfs);
> +		err = m_can_read_fifo(dev, rxfs);
> +		if (err) {
> +			/* Trust that an error back from a FIFO read is negative
> +			 * so that we can differentiate it from work done (pkts).
> +			 */
> +			WARN_ON(err >= 0);

m_can_read_fifo() already throws an error message, so I've removed the
WARN_ON here.

> +			return err;
> +		}
>  
>  		quota--;
>  		pkts++;
> @@ -874,6 +895,7 @@ static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
>  static int m_can_rx_handler(struct net_device *dev, int quota)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> +	int rx_work_or_err;
>  	int work_done = 0;
>  	u32 irqstatus, psr;
>  
> @@ -910,8 +932,13 @@ static int m_can_rx_handler(struct net_device *dev, int quota)
>  	if (irqstatus & IR_ERR_BUS_30X)
>  		work_done += m_can_handle_bus_errors(dev, irqstatus, psr);
>  
> -	if (irqstatus & IR_RF0N)
> -		work_done += m_can_do_rx_poll(dev, (quota - work_done));
> +	if (irqstatus & IR_RF0N) {
> +		rx_work_or_err = m_can_do_rx_poll(dev, (quota - work_done));
> +		if (rx_work_or_err < 0)
> +			return rx_work_or_err;
> +
> +		work_done += rx_work_or_err;
> +	}
>  end:
>  	return work_done;
>  }
> @@ -919,12 +946,16 @@ static int m_can_rx_handler(struct net_device *dev, int quota)
>  static int m_can_rx_peripheral(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> +	int work_done;
>  
> -	m_can_rx_handler(dev, M_CAN_NAPI_WEIGHT);
> +	work_done = m_can_rx_handler(dev, M_CAN_NAPI_WEIGHT);
>  
> -	m_can_enable_all_interrupts(cdev);
> +	// Don't re-enable interrupts if the driver had a fatal error
> +	// (e.g., FIFO read failure).

No C++ comments please, I've converted to proper netdev block style
comments.

> +	if (work_done >= 0)
> +		m_can_enable_all_interrupts(cdev);
>  
> -	return 0;
> +	return work_done;
>  }
>  
>  static int m_can_poll(struct napi_struct *napi, int quota)
> @@ -934,7 +965,10 @@ static int m_can_poll(struct napi_struct *napi, int quota)
>  	int work_done;
>  
>  	work_done = m_can_rx_handler(dev, quota);
> -	if (work_done < quota) {
> +
> +	// Don't re-enable interrupts if the driver had a fatal error
> +	// (e.g., FIFO read failure).

Same here

> +	if (work_done >= 0 && work_done < quota) {
>  		napi_complete_done(napi, work_done);
>  		m_can_enable_all_interrupts(cdev);
>  	}
> @@ -965,7 +999,7 @@ static void m_can_tx_update_stats(struct m_can_classdev *cdev,
>  	stats->tx_packets++;
>  }
>  
> -static void m_can_echo_tx_event(struct net_device *dev)
> +static int m_can_echo_tx_event(struct net_device *dev)
>  {
>  	u32 txe_count = 0;
>  	u32 m_can_txefs;
> @@ -984,12 +1018,18 @@ static void m_can_echo_tx_event(struct net_device *dev)
>  	/* Get and process all sent elements */
>  	for (i = 0; i < txe_count; i++) {
>  		u32 txe, timestamp = 0;
> +		int err;
>  
>  		/* retrieve get index */
>  		fgi = FIELD_GET(TXEFS_EFGI_MASK, m_can_read(cdev, M_CAN_TXEFS));
>  
>  		/* get message marker, timestamp */
> -		txe = m_can_txe_fifo_read(cdev, fgi, 4);
> +		err = m_can_txe_fifo_read(cdev, fgi, 4, &txe);
> +		if (err) {
> +			netdev_err(dev, "TXE FIFO read returned %d\n", err);
> +			return err;
> +		}
> +
>  		msg_mark = FIELD_GET(TX_EVENT_MM_MASK, txe);
>  		timestamp = FIELD_GET(TX_EVENT_TXTS_MASK, txe);
>  
> @@ -1000,6 +1040,8 @@ static void m_can_echo_tx_event(struct net_device *dev)
>  		/* update stats */
>  		m_can_tx_update_stats(cdev, msg_mark, timestamp);
>  	}
> +
> +	return 0;
>  }
>  
>  static irqreturn_t m_can_isr(int irq, void *dev_id)
> @@ -1031,8 +1073,8 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
>  		m_can_disable_all_interrupts(cdev);
>  		if (!cdev->is_peripheral)
>  			napi_schedule(&cdev->napi);
> -		else
> -			m_can_rx_peripheral(dev);
> +		else if (m_can_rx_peripheral(dev) < 0)
> +			goto out_fail;
>  	}
>  
>  	if (cdev->version == 30) {
> @@ -1050,7 +1092,9 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
>  	} else  {
>  		if (ir & IR_TEFN) {
>  			/* New TX FIFO Element arrived */
> -			m_can_echo_tx_event(dev);
> +			if (m_can_echo_tx_event(dev) != 0)
> +				goto out_fail;
> +
>  			can_led_event(dev, CAN_LED_EVENT_TX);
>  			if (netif_queue_stopped(dev) &&
>  			    !m_can_tx_fifo_full(cdev))
> @@ -1059,6 +1103,10 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
>  	}
>  
>  	return IRQ_HANDLED;
> +
> +out_fail:
> +	m_can_disable_all_interrupts(cdev);
> +	return IRQ_HANDLED;
>  }
>  
>  static const struct can_bittiming_const m_can_bittiming_const_30X = {
> @@ -1540,8 +1588,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  	struct canfd_frame *cf = (struct canfd_frame *)cdev->tx_skb->data;
>  	struct net_device *dev = cdev->net;
>  	struct sk_buff *skb = cdev->tx_skb;
> -	u32 id, cccr, fdflags;
> -	int i;
> +	u32 id, dlc, cccr, fdflags;
> +	int i, err;
>  	int putidx;
>  
>  	cdev->tx_skb = NULL;
> @@ -1562,14 +1610,20 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		netif_stop_queue(dev);
>  
>  		/* message ram configuration */
> -		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id);
> -		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC,
> -				 can_fd_len2dlc(cf->len) << 16);
> +		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, &id, 1);
> +		if (err)
> +			goto out_fail;
>  
> -		for (i = 0; i < cf->len; i += 4)
> -			m_can_fifo_write(cdev, 0,
> -					 M_CAN_FIFO_DATA(i / 4),
> -					 *(u32 *)(cf->data + i));
> +		dlc = can_fd_len2dlc(cf->len) << 16;
> +		err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC, &dlc, 1);
> +		if (err)
> +			goto out_fail;
> +
> +		for (i = 0; i < cf->len; i += 4) {
> +			err = m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
> +			if (err)
> +				goto out_fail;
> +		}
>  
>  		can_put_echo_skb(skb, dev, 0, 0);
>  
> @@ -1614,7 +1668,9 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		putidx = FIELD_GET(TXFQS_TFQPI_MASK,
>  				   m_can_read(cdev, M_CAN_TXFQS));
>  		/* Write ID Field to FIFO Element */
> -		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id);
> +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &id, 1);
> +		if (err)
> +			goto out_fail;
>  
>  		/* get CAN FD configuration of frame */
>  		fdflags = 0;
> @@ -1629,15 +1685,19 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		 * it is used in TX interrupt for
>  		 * sending the correct echo frame
>  		 */
> -		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DLC,
> -				 FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> -				 FIELD_PREP(TX_BUF_DLC_MASK,
> -					    can_fd_len2dlc(cf->len)) |
> -				 fdflags | TX_BUF_EFC);
> +		dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> +			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
> +			fdflags | TX_BUF_EFC;
> +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DLC, &dlc, 1);
> +		if (err)
> +			goto out_fail;
>  
> -		for (i = 0; i < cf->len; i += 4)
> -			m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
> -					 *(u32 *)(cf->data + i));
> +		for (i = 0; i < cf->len; i += 4) {
> +			err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
> +					       cf->data + i, 1);
> +			if (err)
> +				goto out_fail;
> +		}
>  
>  		/* Push loopback echo.
>  		 * Will be looped back on TX interrupt based on message marker
> @@ -1654,6 +1714,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  	}
>  
>  	return NETDEV_TX_OK;
> +
> +out_fail:
> +	netdev_err(dev, "FIFO write returned %d\n", err);
> +	m_can_disable_all_interrupts(cdev);
> +	return NETDEV_TX_BUSY;
>  }
>  
>  static void m_can_tx_work_queue(struct work_struct *ws)
> @@ -1819,9 +1884,10 @@ static void m_can_of_parse_mram(struct m_can_classdev *cdev,
>  		cdev->mcfg[MRAM_TXB].off, cdev->mcfg[MRAM_TXB].num);
>  }
>  
> -void m_can_init_ram(struct m_can_classdev *cdev)
> +int m_can_init_ram(struct m_can_classdev *cdev)
>  {
>  	int end, i, start;
> +	int err = 0;
>  
>  	/* initialize the entire Message RAM in use to avoid possible
>  	 * ECC/parity checksum errors when reading an uninitialized buffer
> @@ -1830,8 +1896,13 @@ void m_can_init_ram(struct m_can_classdev *cdev)
>  	end = cdev->mcfg[MRAM_TXB].off +
>  		cdev->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
>  
> -	for (i = start; i < end; i += 4)
> -		m_can_fifo_write_no_off(cdev, i, 0x0);
> +	for (i = start; i < end; i += 4) {
> +		err = m_can_fifo_write_no_off(cdev, i, 0x0);
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(m_can_init_ram);
>  
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index ace071c3e58c..7a3bc878535c 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -64,9 +64,9 @@ struct m_can_ops {
>  	int (*clear_interrupts)(struct m_can_classdev *cdev);
>  	u32 (*read_reg)(struct m_can_classdev *cdev, int reg);
>  	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
> -	u32 (*read_fifo)(struct m_can_classdev *cdev, int addr_offset);
> +	int (*read_fifo)(struct m_can_classdev *cdev, int addr_offset, void *val, size_t val_count);
>  	int (*write_fifo)(struct m_can_classdev *cdev, int addr_offset,
> -			  int val);
> +			  const void *val, size_t val_count);
>  	int (*init)(struct m_can_classdev *cdev);
>  };
>  
> @@ -102,7 +102,7 @@ void m_can_class_free_dev(struct net_device *net);
>  int m_can_class_register(struct m_can_classdev *cdev);
>  void m_can_class_unregister(struct m_can_classdev *cdev);
>  int m_can_class_get_clocks(struct m_can_classdev *cdev);
> -void m_can_init_ram(struct m_can_classdev *priv);
> +int m_can_init_ram(struct m_can_classdev *priv);
>  
>  int m_can_class_suspend(struct device *dev);
>  int m_can_class_resume(struct device *dev);
> diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> index 128808605c3f..89cc3d41e952 100644
> --- a/drivers/net/can/m_can/m_can_pci.c
> +++ b/drivers/net/can/m_can/m_can_pci.c
> @@ -39,11 +39,13 @@ static u32 iomap_read_reg(struct m_can_classdev *cdev, int reg)
>  	return readl(priv->base + reg);
>  }
>  
> -static u32 iomap_read_fifo(struct m_can_classdev *cdev, int offset)
> +static int iomap_read_fifo(struct m_can_classdev *cdev, int offset, void *val, size_t val_count)
>  {
>  	struct m_can_pci_priv *priv = cdev_to_priv(cdev);
>  
> -	return readl(priv->base + offset);
> +	ioread32_rep(priv->base + offset, val, val_count);
> +
> +	return 0;
>  }
>  
>  static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
> @@ -55,11 +57,12 @@ static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
>  	return 0;
>  }
>  
> -static int iomap_write_fifo(struct m_can_classdev *cdev, int offset, int val)
> +static int iomap_write_fifo(struct m_can_classdev *cdev, int offset,
> +			    const void *val, size_t val_count)
>  {
>  	struct m_can_pci_priv *priv = cdev_to_priv(cdev);
>  
> -	writel(val, priv->base + offset);
> +	iowrite32_rep(priv->base + offset, val, val_count);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index 599de0e08cd7..12dec09cbd9f 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -28,11 +28,13 @@ static u32 iomap_read_reg(struct m_can_classdev *cdev, int reg)
>  	return readl(priv->base + reg);
>  }
>  
> -static u32 iomap_read_fifo(struct m_can_classdev *cdev, int offset)
> +static int iomap_read_fifo(struct m_can_classdev *cdev, int offset, void *val, size_t val_count)
>  {
>  	struct m_can_plat_priv *priv = cdev_to_priv(cdev);
>  
> -	return readl(priv->mram_base + offset);
> +	ioread32_rep(priv->mram_base + offset, val, val_count);
> +
> +	return 0;
>  }
>  
>  static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
> @@ -44,11 +46,12 @@ static int iomap_write_reg(struct m_can_classdev *cdev, int reg, int val)
>  	return 0;
>  }
>  
> -static int iomap_write_fifo(struct m_can_classdev *cdev, int offset, int val)
> +static int iomap_write_fifo(struct m_can_classdev *cdev, int offset,
> +			    const void *val, size_t val_count)
>  {
>  	struct m_can_plat_priv *priv = cdev_to_priv(cdev);
>  
> -	writel(val, priv->mram_base + offset);
> +	iowrite32_rep(priv->base + offset, val, val_count);
>  
>  	return 0;
>  }
> @@ -115,7 +118,9 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, mcan_class);
>  
> -	m_can_init_ram(mcan_class);
> +	ret = m_can_init_ram(mcan_class);
> +	if (ret)
> +		goto probe_fail;
>  
>  	pm_runtime_enable(mcan_class->dev);
>  	ret = m_can_class_register(mcan_class);
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 4147cecfbbd6..11c096fe94a2 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -154,14 +154,12 @@ static u32 tcan4x5x_read_reg(struct m_can_classdev *cdev, int reg)
>  	return val;
>  }
>  
> -static u32 tcan4x5x_read_fifo(struct m_can_classdev *cdev, int addr_offset)
> +static int tcan4x5x_read_fifo(struct m_can_classdev *cdev, int addr_offset,
> +			      void *val, size_t val_count)
>  {
>  	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> -	u32 val;
>  
> -	regmap_read(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, &val);
> -
> -	return val;
> +	return regmap_bulk_read(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
>  }
>  
>  static int tcan4x5x_write_reg(struct m_can_classdev *cdev, int reg, int val)
> @@ -172,11 +170,11 @@ static int tcan4x5x_write_reg(struct m_can_classdev *cdev, int reg, int val)
>  }
>  
>  static int tcan4x5x_write_fifo(struct m_can_classdev *cdev,
> -			       int addr_offset, int val)
> +			       int addr_offset, const void *val, size_t val_count)
>  {
>  	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
>  
> -	return regmap_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val);
> +	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
>  }
>  
>  static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> @@ -238,7 +236,9 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
>  		return ret;
>  
>  	/* Zero out the MCAN buffers */
> -	m_can_init_ram(cdev);
> +	ret = m_can_init_ram(cdev);
> +	if (ret)
> +		return ret;
>  
>  	ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
>  				 TCAN4X5X_MODE_SEL_MASK, TCAN4X5X_MODE_NORMAL);
> -- 
> 2.32.0
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] can: m_can: Batch FIFO reads during CAN receive

[Marc Kleine-Budde]

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

On 16.08.2021 22:08:52, Matt Kline wrote:
> On peripherals communicating over a relatively slow SPI line
> (e.g. tcan4x5x), individual transfers have high fixed costs.
> This causes the driver to spend most of its time waiting between
> transfers and severely limits throughput.
> 
> Reduce these overheads by reading more than one word at a time.
> Writing could get a similar treatment in follow-on commits.
> 
> Signed-off-by: Matt Kline <matt@bitbashing.io>
> ---
>  drivers/net/can/m_can/m_can.c | 51 +++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 83eb5cd51de5..85d6cd03bff1 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -308,6 +308,15 @@ enum m_can_reg {
>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>  
> +/* The ID and DLC registers are adjacent in M_CAN FIFO memory,
> + * and we can save a (potentially slow) bus round trip by combining
> + * reads and writes to them.
> + */
> +struct __packed id_and_dlc {
> +	u32 id;
> +	u32 dlc;
> +};

No need for __packed, removed while applying.

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] can: m_can: Batch FIFO reads during CAN receive

[Aswath Govindraju]

Hi Matt, Marc,

On 17/08/21 10:38 am, Matt Kline wrote:
> On peripherals communicating over a relatively slow SPI line
> (e.g. tcan4x5x), individual transfers have high fixed costs.
> This causes the driver to spend most of its time waiting between
> transfers and severely limits throughput.
> 
> Reduce these overheads by reading more than one word at a time.
> Writing could get a similar treatment in follow-on commits.
> 
> Signed-off-by: Matt Kline <matt@bitbashing.io>
> ---
>  drivers/net/can/m_can/m_can.c | 51 +++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 83eb5cd51de5..85d6cd03bff1 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -308,6 +308,15 @@ enum m_can_reg {
>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>  
> +/* The ID and DLC registers are adjacent in M_CAN FIFO memory,
> + * and we can save a (potentially slow) bus round trip by combining
> + * reads and writes to them.
> + */
> +struct __packed id_and_dlc {
> +	u32 id;
> +	u32 dlc;
> +};
> +
>  static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
>  {
>  	return cdev->ops->read_reg(cdev, reg);
> @@ -460,17 +469,18 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	struct canfd_frame *cf;
>  	struct sk_buff *skb;
> -	u32 id, fgi, dlc;
> +	struct id_and_dlc fifo_header;
> +	u32 fgi;
>  	u32 timestamp = 0;
> -	int i, err;
> +	int err;
>  
>  	/* calculate the fifo get index for where to read data */
>  	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
> -	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC, &dlc, 1);
> +	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &fifo_header, 2);

While reading multiple register fields and calling iomap_read_fifo() in
m_can_platform.c is causing an issue.

In iomap_read_fifo(), ioread32_rep() is being used for reading.
ioread32_rep reads() from the same source address for val_count times.
This is not the intended behavior here. The source address also needs to
be shifted along with the destination address.

Is a fix required in iomap_read_fifo() ?

Thanks,
Aswath

>  	if (err)
>  		goto out_fail;
>  
> -	if (dlc & RX_BUF_FDF)
> +	if (fifo_header.dlc & RX_BUF_FDF)
>  		skb = alloc_canfd_skb(dev, &cf);
>  	else
>  		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
> @@ -479,36 +489,31 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  		return 0;
>  	}
>  
> -	if (dlc & RX_BUF_FDF)
> -		cf->len = can_fd_dlc2len((dlc >> 16) & 0x0F);
> +	if (fifo_header.dlc & RX_BUF_FDF)
> +		cf->len = can_fd_dlc2len((fifo_header.dlc >> 16) & 0x0F);
>  	else
> -		cf->len = can_cc_dlc2len((dlc >> 16) & 0x0F);
> +		cf->len = can_cc_dlc2len((fifo_header.dlc >> 16) & 0x0F);
>  
> -	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &id, 1);
> -	if (err)
> -		goto out_fail;
> -
> -	if (id & RX_BUF_XTD)
> -		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +	if (fifo_header.id & RX_BUF_XTD)
> +		cf->can_id = (fifo_header.id & CAN_EFF_MASK) | CAN_EFF_FLAG;
>  	else
> -		cf->can_id = (id >> 18) & CAN_SFF_MASK;
> +		cf->can_id = (fifo_header.id >> 18) & CAN_SFF_MASK;
>  
> -	if (id & RX_BUF_ESI) {
> +	if (fifo_header.id & RX_BUF_ESI) {
>  		cf->flags |= CANFD_ESI;
>  		netdev_dbg(dev, "ESI Error\n");
>  	}
>  
> -	if (!(dlc & RX_BUF_FDF) && (id & RX_BUF_RTR)) {
> +	if (!(fifo_header.dlc & RX_BUF_FDF) && (fifo_header.id & RX_BUF_RTR)) {
>  		cf->can_id |= CAN_RTR_FLAG;
>  	} else {
> -		if (dlc & RX_BUF_BRS)
> +		if (fifo_header.dlc & RX_BUF_BRS)
>  			cf->flags |= CANFD_BRS;
>  
> -		for (i = 0; i < cf->len; i += 4) {
> -			err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
> -			if (err)
> -				goto out_fail;
> -		}
> +		err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0),
> +				      cf->data, DIV_ROUND_UP(cf->len, 4));
> +		if (err)
> +			goto out_fail;
>  	}
>  
>  	/* acknowledge rx fifo 0 */
> @@ -517,7 +522,7 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	stats->rx_packets++;
>  	stats->rx_bytes += cf->len;
>  
> -	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, dlc);
> +	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
> 

Re: [PATCH v3 2/3] can: m_can: Batch FIFO reads during CAN receive

[Matt Kline]

Hey Aswath,

Definitely looks like a silly mistake on my part, thanks for sending fixes!
Apologies for the slow response; I was out for the past week or so.

Best,
Matt

On Thu, Sep 16, 2021 at 05:34:45PM +0530, Aswath Govindraju wrote:
> Hi Matt, Marc,
> 
> While reading multiple register fields and calling iomap_read_fifo() in
> m_can_platform.c is causing an issue.
> 
> In iomap_read_fifo(), ioread32_rep() is being used for reading.
> ioread32_rep reads() from the same source address for val_count times.
> This is not the intended behavior here. The source address also needs to
> be shifted along with the destination address.
> 
> Is a fix required in iomap_read_fifo() ?
> 
> Thanks,
> Aswath