[PATCH v2 0/2] can: m_can: Merge FIFO ops to increase throughput

[PATCH v2 0/2] can: m_can: Merge FIFO ops to increase throughput

[Matt Kline]

While debugging low throughput on a tcan4x5x chip using a logic
analyzer, I found that the SPI bus is silent for the *vast* majority of
time spent sending or receiving a CAN frame.
Each SPI transfer takes ~5 microseconds, but there's an order of
magnitude more time (50-60 microseconds) between them. This doesn't seem
to be caused by any sort of contention - it happens on a SPI bus with a
single chip select and no other drivers accessing it. Presumably there's
a (relatively) large fixed cost to request a transfer from the SPI
controller on the hardware I'm using (an NVIDIA Jetson platform).

Let's improve throughput by combining FIFO reads and writes into larger
transfers - one for ID & DLC, one for the frame data - instead of
handling single words at a time. We could reduce the number of transfers
further by batching certain control register reads, but this is an easy
place to start, since FIFO registers are contiguous.

Since TX and RX time is dominated by the fixed, per-transfer delays
mentioned above, this nets substantial performance improvements - about
20% faster for small CAN frames and nearly 5x faster for max size
(64 byte) CAN FD frames.

This is a resend of a previous patch set, updated to use FIELD_PREP()
and friends - I fear the first one got eaten by a spam filter due to
some SMTP flub on my end.

Matt Kline (2):
  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          | 80 +++++++++++++-------------
 drivers/net/can/m_can/m_can.h          |  4 +-
 drivers/net/can/m_can/m_can_pci.c      | 11 ++--
 drivers/net/can/m_can/m_can_platform.c | 11 ++--
 drivers/net/can/m_can/tcan4x5x-core.c  | 12 ++--
 5 files changed, 61 insertions(+), 57 deletions(-)

-- 
2.32.0

[PATCH v2 1/2] 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          | 27 +++++++++++++++-----------
 drivers/net/can/m_can/m_can.h          |  2 +-
 drivers/net/can/m_can/m_can_pci.c      |  6 ++++--
 drivers/net/can/m_can/m_can_platform.c |  6 ++++--
 drivers/net/can/m_can/tcan4x5x-core.c  |  8 +++-----
 5 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index bba2a449ac70..233d5da907ec 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -319,13 +319,15 @@ 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 void m_can_fifo_read(struct m_can_classdev *cdev,
+			    u32 fgi, unsigned int offset, void *val, size_t val_count)
 {
+	u32 result;
 	u32 addr_offset = cdev->mcfg[MRAM_RXF0].off + fgi * RXF0_ELEMENT_SIZE +
 		offset;
 
-	return cdev->ops->read_fifo(cdev, addr_offset);
+	result = cdev->ops->read_fifo(cdev, addr_offset, val, val_count);
+	WARN_ON(result != 0);
 }
 
 static void m_can_fifo_write(struct m_can_classdev *cdev,
@@ -345,10 +347,13 @@ static inline void m_can_fifo_write_no_off(struct m_can_classdev *cdev,
 
 static u32 m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset)
 {
+	u32 val, result;
 	u32 addr_offset = cdev->mcfg[MRAM_TXE].off + fgi * TXE_ELEMENT_SIZE +
 		offset;
 
-	return cdev->ops->read_fifo(cdev, addr_offset);
+	result = cdev->ops->read_fifo(cdev, addr_offset, &val, 1);
+	WARN_ON(result != 0);
+	return val;
 }
 
 static inline bool m_can_tx_fifo_full(struct m_can_classdev *cdev)
@@ -460,13 +465,17 @@ static void 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_and_dlc[2];
 	u32 id, fgi, dlc;
 	u32 timestamp = 0;
-	int i;
 
 	/* 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);
+
+	m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
+	id = id_and_dlc[0];
+	dlc = id_and_dlc[1];
+
 	if (dlc & RX_BUF_FDF)
 		skb = alloc_canfd_skb(dev, &cf);
 	else
@@ -481,7 +490,6 @@ 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);
 	if (id & RX_BUF_XTD)
 		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -498,10 +506,7 @@ 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));
+		m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0), cf->data, DIV_ROUND_UP(cf->len, 4));
 	}
 
 	/* acknowledge rx fifo 0 */
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index ace071c3e58c..2571ec1efec4 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -64,7 +64,7 @@ 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);
 	int (*init)(struct m_can_classdev *cdev);
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index 128808605c3f..11614a635796 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)
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 599de0e08cd7..94c82dd39076 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)
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 4147cecfbbd6..4f77b1dbd492 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)
-- 
2.32.0

[PATCH v2 2/2] 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          | 55 ++++++++++++--------------
 drivers/net/can/m_can/m_can.h          |  2 +-
 drivers/net/can/m_can/m_can_pci.c      |  5 ++-
 drivers/net/can/m_can/m_can_platform.c |  5 ++-
 drivers/net/can/m_can/tcan4x5x-core.c  |  4 +-
 5 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 233d5da907ec..7aae2d19ad65 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 */
@@ -331,18 +331,20 @@ static void m_can_fifo_read(struct m_can_classdev *cdev,
 }
 
 static void m_can_fifo_write(struct m_can_classdev *cdev,
-			     u32 fpi, unsigned int offset, u32 val)
+			     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;
+	int result;
 
-	cdev->ops->write_fifo(cdev, addr_offset, val);
+	result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
+	WARN_ON(result != 0);
 }
 
 static inline void m_can_fifo_write_no_off(struct m_can_classdev *cdev,
 					   u32 fpi, u32 val)
 {
-	cdev->ops->write_fifo(cdev, fpi, val);
+	cdev->ops->write_fifo(cdev, fpi, &val, 1);
 }
 
 static u32 m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset)
@@ -506,7 +508,7 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 		if (dlc & RX_BUF_BRS)
 			cf->flags |= CANFD_BRS;
 
-		m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0), cf->data, DIV_ROUND_UP(cf->len, 4));
+		m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
 	}
 
 	/* acknowledge rx fifo 0 */
@@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct net_device *dev = cdev->net;
 	struct sk_buff *skb = cdev->tx_skb;
 	u32 id, cccr, fdflags;
-	int i;
 	int putidx;
+	u32 id_and_dlc[2];
 
 	cdev->tx_skb = NULL;
 
@@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	if (cf->can_id & CAN_RTR_FLAG)
 		id |= TX_BUF_RTR;
 
+	id_and_dlc[0] = id;
+
 	if (cdev->version == 30) {
 		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);
+		id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
 
-		for (i = 0; i < cf->len; i += 4)
-			m_can_fifo_write(cdev, 0,
-					 M_CAN_FIFO_DATA(i / 4),
-					 *(u32 *)(cf->data + i));
+		/* Write the frame ID, DLC, and payload to the FIFO element. */
+		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
+		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
 
 		can_put_echo_skb(skb, dev, 0, 0);
 
@@ -1618,8 +1618,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 */
-		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id);
+
+		/* Construct the DLC Field, with CAN-FD configuration.
+		 * Use the put index of fifo as message the marker,
+		 * used in the TX interrupt for sending the correct echo frame.
+		 */
 
 		/* get CAN FD configuration of frame */
 		fdflags = 0;
@@ -1628,21 +1631,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 			if (cf->flags & CANFD_BRS)
 				fdflags |= TX_BUF_BRS;
 		}
+		id_and_dlc[1] = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
+				 FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
+				 fdflags | TX_BUF_EFC;
 
-		/* 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
-		 */
-		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);
-
-		for (i = 0; i < cf->len; i += 4)
-			m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
-					 *(u32 *)(cf->data + i));
+		/* Write the frame ID, DLC, and payload to the FIFO element. */
+		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
+		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
 
 		/* Push loopback echo.
 		 * Will be looped back on TX interrupt based on message marker
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 2571ec1efec4..edf2477b064f 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -66,7 +66,7 @@ struct m_can_ops {
 	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
 	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);
 };
 
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index 11614a635796..89cc3d41e952 100644
--- a/drivers/net/can/m_can/m_can_pci.c
+++ b/drivers/net/can/m_can/m_can_pci.c
@@ -57,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 94c82dd39076..40e5351c7f74 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -46,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;
 }
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 4f77b1dbd492..89d2009c895b 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -170,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)
-- 
2.32.0

Re: [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit

[Marc Kleine-Budde]

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

On 26.07.2021 18:58:55, Matt Kline wrote:
> 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          | 55 ++++++++++++--------------
>  drivers/net/can/m_can/m_can.h          |  2 +-
>  drivers/net/can/m_can/m_can_pci.c      |  5 ++-
>  drivers/net/can/m_can/m_can_platform.c |  5 ++-
>  drivers/net/can/m_can/tcan4x5x-core.c  |  4 +-
>  5 files changed, 34 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 233d5da907ec..7aae2d19ad65 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 */
> @@ -331,18 +331,20 @@ static void m_can_fifo_read(struct m_can_classdev *cdev,
>  }
>  
>  static void m_can_fifo_write(struct m_can_classdev *cdev,
> -			     u32 fpi, unsigned int offset, u32 val)
> +			     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;
> +	int result;
>  
> -	cdev->ops->write_fifo(cdev, addr_offset, val);
> +	result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
> +	WARN_ON(result != 0);

What about converting all read/write functions to return an error, and
handle the error in the caller?

>  }
>  
>  static inline void m_can_fifo_write_no_off(struct m_can_classdev *cdev,
>  					   u32 fpi, u32 val)
>  {
> -	cdev->ops->write_fifo(cdev, fpi, val);
> +	cdev->ops->write_fifo(cdev, fpi, &val, 1);
>  }
>  
>  static u32 m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset)
> @@ -506,7 +508,7 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  		if (dlc & RX_BUF_BRS)
>  			cf->flags |= CANFD_BRS;
>  
> -		m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0), cf->data, DIV_ROUND_UP(cf->len, 4));
> +		m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
>  	}
>  
>  	/* acknowledge rx fifo 0 */
> @@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  	struct net_device *dev = cdev->net;
>  	struct sk_buff *skb = cdev->tx_skb;
>  	u32 id, cccr, fdflags;
> -	int i;
>  	int putidx;
> +	u32 id_and_dlc[2];

Can you create a struct for this?

>  
>  	cdev->tx_skb = NULL;
>  
> @@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  	if (cf->can_id & CAN_RTR_FLAG)
>  		id |= TX_BUF_RTR;
>  
> +	id_and_dlc[0] = id;
> +
>  	if (cdev->version == 30) {
>  		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);
> +		id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
>  
> -		for (i = 0; i < cf->len; i += 4)
> -			m_can_fifo_write(cdev, 0,
> -					 M_CAN_FIFO_DATA(i / 4),
> -					 *(u32 *)(cf->data + i));
> +		/* Write the frame ID, DLC, and payload to the FIFO element. */
> +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
> +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));

Does it make sense to combine these, too? Same for the v3.1 variant.

>  
>  		can_put_echo_skb(skb, dev, 0, 0);
>  
> @@ -1618,8 +1618,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 */
> -		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id);
> +
> +		/* Construct the DLC Field, with CAN-FD configuration.
> +		 * Use the put index of fifo as message the marker,
> +		 * used in the TX interrupt for sending the correct echo frame.
> +		 */
>  
>  		/* get CAN FD configuration of frame */
>  		fdflags = 0;
> @@ -1628,21 +1631,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  			if (cf->flags & CANFD_BRS)
>  				fdflags |= TX_BUF_BRS;
>  		}
> +		id_and_dlc[1] = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> +				 FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
> +				 fdflags | TX_BUF_EFC;
>  
> -		/* 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
> -		 */
> -		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);
> -
> -		for (i = 0; i < cf->len; i += 4)
> -			m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA(i / 4),
> -					 *(u32 *)(cf->data + i));
> +		/* Write the frame ID, DLC, and payload to the FIFO element. */
> +		m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
> +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
>  
>  		/* Push loopback echo.
>  		 * Will be looped back on TX interrupt based on message marker
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 2571ec1efec4..edf2477b064f 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -66,7 +66,7 @@ struct m_can_ops {
>  	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
>  	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);
>  };
>  
> diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> index 11614a635796..89cc3d41e952 100644
> --- a/drivers/net/can/m_can/m_can_pci.c
> +++ b/drivers/net/can/m_can/m_can_pci.c
> @@ -57,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 94c82dd39076..40e5351c7f74 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -46,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;
>  }
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 4f77b1dbd492..89d2009c895b 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -170,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)
> -- 
> 2.32.0

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 v2 2/2] can: m_can: Batch FIFO writes during CAN transmit

[Matt Kline]

On Wed, Aug 04, 2021 at 11:18:58AM +0200, Marc Kleine-Budde wrote:
> >  
> > -	cdev->ops->write_fifo(cdev, addr_offset, val);
> > +	result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
> > +	WARN_ON(result != 0);
> 
> What about converting all read/write functions to return an error, and
> handle the error in the caller?

Yeah, that would be cleaner.

> >  	/* acknowledge rx fifo 0 */
> > @@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  	struct net_device *dev = cdev->net;
> >  	struct sk_buff *skb = cdev->tx_skb;
> >  	u32 id, cccr, fdflags;
> > -	int i;
> >  	int putidx;
> > +	u32 id_and_dlc[2];
> 
> Can you create a struct for this?

Ditto, sure!

> >  
> >  	cdev->tx_skb = NULL;
> >  
> > @@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  	if (cf->can_id & CAN_RTR_FLAG)
> >  		id |= TX_BUF_RTR;
> >  
> > +	id_and_dlc[0] = id;
> > +
> >  	if (cdev->version == 30) {
> >  		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);
> > +		id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
> >  
> > -		for (i = 0; i < cf->len; i += 4)
> > -			m_can_fifo_write(cdev, 0,
> > -					 M_CAN_FIFO_DATA(i / 4),
> > -					 *(u32 *)(cf->data + i));
> > +		/* Write the frame ID, DLC, and payload to the FIFO element. */
> > +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
> > +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
> 
> Does it make sense to combine these, too? Same for the v3.1 variant.

I think that's the eventual goal, but since the ID, DLC, and frame data would
have to be contiguous for a single m_can_fifo_write(), you'd end up copying
things around. I wanted to start with this smaller, simpler patch first.
Is that alright?

I'll try to send a v3 up shortly.

Best,
Matt

Re: [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit

[Marc Kleine-Budde]

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

On 10.08.2021 13:47:32, Matt Kline wrote:
> On Wed, Aug 04, 2021 at 11:18:58AM +0200, Marc Kleine-Budde wrote:
> > >  
> > > -	cdev->ops->write_fifo(cdev, addr_offset, val);
> > > +	result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
> > > +	WARN_ON(result != 0);
> > 
> > What about converting all read/write functions to return an error, and
> > handle the error in the caller?
> 
> Yeah, that would be cleaner.

In the mcp251xfd (another SPI-CAN controller) driver I have the same
problem. I've basically implemented error checking everywhere. If there
is an error in the interrupt handler, I shut down the driver, see:

https://elixir.bootlin.com/linux/v5.13/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L2298

> > >  	/* acknowledge rx fifo 0 */
> > > @@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  	struct net_device *dev = cdev->net;
> > >  	struct sk_buff *skb = cdev->tx_skb;
> > >  	u32 id, cccr, fdflags;
> > > -	int i;
> > >  	int putidx;
> > > +	u32 id_and_dlc[2];
> > 
> > Can you create a struct for this?
> 
> Ditto, sure!

A struct can easily extended to hold the data, too.

> > >  
> > >  	cdev->tx_skb = NULL;
> > >  
> > > @@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  	if (cf->can_id & CAN_RTR_FLAG)
> > >  		id |= TX_BUF_RTR;
> > >  
> > > +	id_and_dlc[0] = id;
> > > +
> > >  	if (cdev->version == 30) {
> > >  		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);
> > > +		id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
> > >  
> > > -		for (i = 0; i < cf->len; i += 4)
> > > -			m_can_fifo_write(cdev, 0,
> > > -					 M_CAN_FIFO_DATA(i / 4),
> > > -					 *(u32 *)(cf->data + i));
> > > +		/* Write the frame ID, DLC, and payload to the FIFO element. */
> > > +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
> > > +		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
> > 
> > Does it make sense to combine these, too? Same for the v3.1 variant.
> 
> I think that's the eventual goal, but since the ID, DLC, and frame data would
> have to be contiguous for a single m_can_fifo_write(), you'd end up copying
> things around.

Yes, but at least for the SPI this is a neglectable overhead.

> I wanted to start with this smaller, simpler patch first. Is that
> alright?

Fine with me!

> I'll try to send a v3 up shortly.

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 --]