[PATCH v2 00/18] can: m_can: Optimizations for m_can/tcan part 2

[PATCH v2 00/18] can: m_can: Optimizations for m_can/tcan part 2

[Markus Schneider-Pargmann]

Hi Marc and everyone,

second version part 2, I fixed the bug I noticed for integrated m_can
devices. The accounting was wrong or missing for these. I don't have the
integrated hardware myself so any testing is appreciated (I only have
the tcan device to test the mcan driver). Also v2 rebases on top of
v6.2-rc5.

The series implements many small and bigger throughput improvements and
adds rx/tx coalescing at the end.

Best,
Markus

Changes in v2:
- Rebased on v6.2-rc5
- Fixed missing/broken accounting for non peripheral m_can devices.

part 1:
v1 - https://lore.kernel.org/lkml/20221116205308.2996556-1-msp@baylibre.com
v2 - https://lore.kernel.org/lkml/20221206115728.1056014-1-msp@baylibre.com

part 2:
v1 - https://lore.kernel.org/lkml/20221221152537.751564-1-msp@baylibre.com

Markus Schneider-Pargmann (18):
  can: tcan4x5x: Remove reserved register 0x814 from writable table
  can: tcan4x5x: Check size of mram configuration
  can: m_can: Remove repeated check for is_peripheral
  can: m_can: Always acknowledge all interrupts
  can: m_can: Remove double interrupt enable
  can: m_can: Disable unused interrupts
  can: m_can: Keep interrupts enabled during peripheral read
  can: m_can: Write transmit header and data in one transaction
  can: m_can: Implement receive coalescing
  can: m_can: Implement transmit coalescing
  can: m_can: Add rx coalescing ethtool support
  can: m_can: Add tx coalescing ethtool support
  can: m_can: Cache tx putidx
  can: m_can: Use the workqueue as queue
  can: m_can: Introduce a tx_fifo_in_flight counter
  can: m_can: Use tx_fifo_in_flight for netif_queue control
  can: m_can: Implement BQL
  can: m_can: Implement transmit submission coalescing

 drivers/net/can/m_can/m_can.c           | 514 ++++++++++++++++++------
 drivers/net/can/m_can/m_can.h           |  36 +-
 drivers/net/can/m_can/tcan4x5x-core.c   |   5 +
 drivers/net/can/m_can/tcan4x5x-regmap.c |   1 -
 4 files changed, 432 insertions(+), 124 deletions(-)

-- 
2.39.0

[PATCH v2 01/18] can: tcan4x5x: Remove reserved register 0x814 from writable table

[Markus Schneider-Pargmann]

The mentioned register is not writable. It is reserved and should not be
written.

Fixes: 39dbb21b6a29 ("can: tcan4x5x: Specify separate read/write ranges")
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/tcan4x5x-regmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-regmap.c b/drivers/net/can/m_can/tcan4x5x-regmap.c
index 2b218ce04e9f..fafa6daa67e6 100644
--- a/drivers/net/can/m_can/tcan4x5x-regmap.c
+++ b/drivers/net/can/m_can/tcan4x5x-regmap.c
@@ -95,7 +95,6 @@ static const struct regmap_range tcan4x5x_reg_table_wr_range[] = {
 	regmap_reg_range(0x000c, 0x0010),
 	/* Device configuration registers and Interrupt Flags*/
 	regmap_reg_range(0x0800, 0x080c),
-	regmap_reg_range(0x0814, 0x0814),
 	regmap_reg_range(0x0820, 0x0820),
 	regmap_reg_range(0x0830, 0x0830),
 	/* M_CAN */
-- 
2.39.0

[PATCH v2 02/18] can: tcan4x5x: Check size of mram configuration

[Markus Schneider-Pargmann]

To reduce debugging effort in case the mram is misconfigured, add this
size check of the DT configuration. Currently if the mram configuration
doesn't fit into the available MRAM it just overwrites other areas of
the MRAM.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c         | 16 ++++++++++++++++
 drivers/net/can/m_can/m_can.h         |  1 +
 drivers/net/can/m_can/tcan4x5x-core.c |  5 +++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 8e83d6963d85..8ccf20f093f8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1886,6 +1886,22 @@ static int register_m_can_dev(struct net_device *dev)
 	return register_candev(dev);
 }
 
+int m_can_check_mram_cfg(struct m_can_classdev *cdev, u32 mram_max_size)
+{
+	u32 total_size;
+
+	total_size = cdev->mcfg[MRAM_TXB].off - cdev->mcfg[MRAM_SIDF].off +
+			cdev->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
+	if (total_size > mram_max_size) {
+		dev_err(cdev->dev, "Total size of mram config(%u) exceeds mram(%u)\n",
+			total_size, mram_max_size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(m_can_check_mram_cfg);
+
 static void m_can_of_parse_mram(struct m_can_classdev *cdev,
 				const u32 *mram_config_vals)
 {
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index a839dc71dc9b..d8150d8128e7 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -101,6 +101,7 @@ 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);
 int m_can_init_ram(struct m_can_classdev *priv);
+int m_can_check_mram_cfg(struct m_can_classdev *cdev, u32 mram_max_size);
 
 int m_can_class_suspend(struct device *dev);
 int m_can_class_resume(struct device *dev);
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 2342aa011647..e706518176e4 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -80,6 +80,7 @@
 	 TCAN4X5X_MCAN_IR_RF1F)
 
 #define TCAN4X5X_MRAM_START 0x8000
+#define TCAN4X5X_MRAM_SIZE 0x800
 #define TCAN4X5X_MCAN_OFFSET 0x1000
 
 #define TCAN4X5X_CLEAR_ALL_INT 0xffffffff
@@ -307,6 +308,10 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 	if (!mcan_class)
 		return -ENOMEM;
 
+	ret = m_can_check_mram_cfg(mcan_class, TCAN4X5X_MRAM_SIZE);
+	if (ret)
+		goto out_m_can_class_free_dev;
+
 	priv = cdev_to_priv(mcan_class);
 
 	priv->power = devm_regulator_get_optional(&spi->dev, "vsup");
-- 
2.39.0

[PATCH v2 03/18] can: m_can: Remove repeated check for is_peripheral

[Markus Schneider-Pargmann]

Merge both if-blocks to fix this.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 8ccf20f093f8..33abe938c30b 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1592,10 +1592,8 @@ static int m_can_close(struct net_device *dev)
 		cdev->tx_skb = NULL;
 		destroy_workqueue(cdev->tx_wq);
 		cdev->tx_wq = NULL;
-	}
-
-	if (cdev->is_peripheral)
 		can_rx_offload_disable(&cdev->offload);
+	}
 
 	close_candev(dev);
 
-- 
2.39.0

[PATCH v2 04/18] can: m_can: Always acknowledge all interrupts

[Markus Schneider-Pargmann]

The code already exits the function on !ir before this condition. No
need to check again if anything is set as IR_ALL_INT is 0xffffffff.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 33abe938c30b..2df39dfa309b 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1083,8 +1083,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	/* ACK all irqs */
-	if (ir & IR_ALL_INT)
-		m_can_write(cdev, M_CAN_IR, ir);
+	m_can_write(cdev, M_CAN_IR, ir);
 
 	if (cdev->ops->clear_interrupts)
 		cdev->ops->clear_interrupts(cdev);
-- 
2.39.0

[PATCH v2 05/18] can: m_can: Remove double interrupt enable

[Markus Schneider-Pargmann]

Interrupts are enabled a few lines further down as well. Remove this
second call to enable all interrupts.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2df39dfa309b..a668a5836cf8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1364,7 +1364,6 @@ static int m_can_chip_config(struct net_device *dev)
 	m_can_write(cdev, M_CAN_TEST, test);
 
 	/* Enable interrupts */
-	m_can_write(cdev, M_CAN_IR, IR_ALL_INT);
 	if (!(cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
 		if (cdev->version == 30)
 			m_can_write(cdev, M_CAN_IE, IR_ALL_INT &
-- 
2.39.0

[PATCH v2 06/18] can: m_can: Disable unused interrupts

[Markus Schneider-Pargmann]

There are a number of interrupts that are not used by the driver at the
moment. Disable all of these.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a668a5836cf8..ef5ca5d37b0d 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1262,6 +1262,7 @@ static int m_can_chip_config(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 cccr, test;
+	u32 interrupts = IR_ALL_INT;
 	int err;
 
 	err = m_can_init_ram(cdev);
@@ -1270,6 +1271,11 @@ static int m_can_chip_config(struct net_device *dev)
 		return err;
 	}
 
+	/* Disable unused interrupts */
+	interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TEFW | IR_TFE |
+			IR_TCF | IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N |
+			IR_RF0F | IR_RF0W);
+
 	m_can_config_endisable(cdev, true);
 
 	/* RX Buffer/FIFO Element Size 64 bytes data field */
@@ -1364,15 +1370,13 @@ static int m_can_chip_config(struct net_device *dev)
 	m_can_write(cdev, M_CAN_TEST, test);
 
 	/* Enable interrupts */
-	if (!(cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
+	if (!(cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
 		if (cdev->version == 30)
-			m_can_write(cdev, M_CAN_IE, IR_ALL_INT &
-				    ~(IR_ERR_LEC_30X));
+			interrupts &= ~(IR_ERR_LEC_30X);
 		else
-			m_can_write(cdev, M_CAN_IE, IR_ALL_INT &
-				    ~(IR_ERR_LEC_31X));
-	else
-		m_can_write(cdev, M_CAN_IE, IR_ALL_INT);
+			interrupts &= ~(IR_ERR_LEC_31X);
+	}
+	m_can_write(cdev, M_CAN_IE, interrupts);
 
 	/* route all interrupts to INT0 */
 	m_can_write(cdev, M_CAN_ILS, ILS_ALL_INT0);
-- 
2.39.0

Re: [PATCH v2 06/18] can: m_can: Disable unused interrupts

[Simon Horman]

On Wed, Jan 25, 2023 at 08:50:47PM +0100, Markus Schneider-Pargmann wrote:
> There are a number of interrupts that are not used by the driver at the
> moment. Disable all of these.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/net/can/m_can/m_can.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a668a5836cf8..ef5ca5d37b0d 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1262,6 +1262,7 @@ static int m_can_chip_config(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	u32 cccr, test;
> +	u32 interrupts = IR_ALL_INT;
>  	int err;

nit: reverse xmas tree (longest line to shortest line) for local variable
declarations.

...

[PATCH v2 07/18] can: m_can: Keep interrupts enabled during peripheral read

[Markus Schneider-Pargmann]

Interrupts currently get disabled if the interrupt status shows new
received data. Non-peripheral chips handle receiving in a worker thread,
but peripheral chips are handling the receive process in the threaded
interrupt routine itself without scheduling it for a different worker.
So there is no need to disable interrupts for peripheral chips.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index ef5ca5d37b0d..78f6ed744c36 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -972,8 +972,8 @@ static int m_can_rx_peripheral(struct net_device *dev, u32 irqstatus)
 	/* 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);
+	if (work_done < 0)
+		m_can_disable_all_interrupts(cdev);
 
 	return work_done;
 }
@@ -1095,11 +1095,12 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	 */
 	if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X)) {
 		cdev->irqstatus = ir;
-		m_can_disable_all_interrupts(cdev);
-		if (!cdev->is_peripheral)
+		if (!cdev->is_peripheral) {
+			m_can_disable_all_interrupts(cdev);
 			napi_schedule(&cdev->napi);
-		else if (m_can_rx_peripheral(dev, ir) < 0)
+		} else if (m_can_rx_peripheral(dev, ir) < 0) {
 			goto out_fail;
+		}
 	}
 
 	if (cdev->version == 30) {
-- 
2.39.0

[PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction

[Markus Schneider-Pargmann]

Combine header and data before writing to the transmit fifo to reduce
the overhead for peripheral chips.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 78f6ed744c36..440bc0536951 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		m_can_write(cdev, M_CAN_TXBAR, 0x1);
 		/* End of xmit function for version 3.0.x */
 	} else {
+		char buf[TXB_ELEMENT_SIZE];
 		/* Transmit routine for version >= v3.1.x */
 
 		txfqs = m_can_read(cdev, M_CAN_TXFQS);
@@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		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_ID, &fifo_header, 2);
-		if (err)
-			goto out_fail;
+		memcpy(buf, &fifo_header, 8);
+		memcpy(&buf[8], &cf->data, cf->len);
 
-		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
-				       cf->data, DIV_ROUND_UP(cf->len, 4));
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
+				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
 		if (err)
 			goto out_fail;
 
-- 
2.39.0

Re: [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction

[Simon Horman]

On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> Combine header and data before writing to the transmit fifo to reduce
> the overhead for peripheral chips.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/net/can/m_can/m_can.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 78f6ed744c36..440bc0536951 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
>  		/* End of xmit function for version 3.0.x */
>  	} else {
> +		char buf[TXB_ELEMENT_SIZE];
>  		/* Transmit routine for version >= v3.1.x */
>  
>  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		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_ID, &fifo_header, 2);
> -		if (err)
> -			goto out_fail;
> +		memcpy(buf, &fifo_header, 8);
> +		memcpy(&buf[8], &cf->data, cf->len);
>  
> -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));

Perhaps I am missing something here, but my reading is that:

- 8 is a length in bytes
- the 5th argument to m_can_fifo_write is the val_count parameter,
  whose unit is 4-byte long values.

  By this logic, perhaps the correct value for this argument is:

  DIV_ROUND_UP(8 + cf->len, 4)

Also:

- If cf->len is not a multiple of 4, is there a possibility
  that uninitialised trailing data in buf will be used
  indirectly by m_can_fifo_write()?

>  		if (err)
>  			goto out_fail;
>  
> -- 
> 2.39.0
> 

Re: [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction

[Markus Schneider-Pargmann]

Hi Simon,

On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > Combine header and data before writing to the transmit fifo to reduce
> > the overhead for peripheral chips.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 78f6ed744c36..440bc0536951 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> >  		/* End of xmit function for version 3.0.x */
> >  	} else {
> > +		char buf[TXB_ELEMENT_SIZE];
> >  		/* Transmit routine for version >= v3.1.x */
> >  
> >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  		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_ID, &fifo_header, 2);
> > -		if (err)
> > -			goto out_fail;
> > +		memcpy(buf, &fifo_header, 8);
> > +		memcpy(&buf[8], &cf->data, cf->len);
> >  
> > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> 
> Perhaps I am missing something here, but my reading is that:
> 
> - 8 is a length in bytes
> - the 5th argument to m_can_fifo_write is the val_count parameter,
>   whose unit is 4-byte long values.
> 
>   By this logic, perhaps the correct value for this argument is:
> 
>   DIV_ROUND_UP(8 + cf->len, 4)

Thank you for spotting this. You are totally right, I will fix it for
the next version.

> 
> Also:
> 
> - If cf->len is not a multiple of 4, is there a possibility
>   that uninitialised trailing data in buf will be used
>   indirectly by m_can_fifo_write()?

Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
values above have to be multiple of 4 because of the CAN-FD
specification.

With 'buf' it should read garbage from the buffer which I think is not a
problem as the chip knows how much of the data to use. Also the tx
elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
the next element with that. The chip minimum size is 8 bytes for the
data field anyways. So I think this is fine.

Thanks,
Markus

> 
> >  		if (err)
> >  			goto out_fail;
> >  
> > -- 
> > 2.39.0
> > 

Re: [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction

[Simon Horman]

On Mon, Jan 30, 2023 at 09:04:19AM +0100, Markus Schneider-Pargmann wrote:
> Hi Simon,
> 
> On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> > On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > > Combine header and data before writing to the transmit fifo to reduce
> > > the overhead for peripheral chips.
> > > 
> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > ---
> > >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > > index 78f6ed744c36..440bc0536951 100644
> > > --- a/drivers/net/can/m_can/m_can.c
> > > +++ b/drivers/net/can/m_can/m_can.c
> > > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> > >  		/* End of xmit function for version 3.0.x */
> > >  	} else {
> > > +		char buf[TXB_ELEMENT_SIZE];
> > >  		/* Transmit routine for version >= v3.1.x */
> > >  
> > >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  		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_ID, &fifo_header, 2);
> > > -		if (err)
> > > -			goto out_fail;
> > > +		memcpy(buf, &fifo_header, 8);
> > > +		memcpy(&buf[8], &cf->data, cf->len);
> > >  
> > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> > 
> > Perhaps I am missing something here, but my reading is that:
> > 
> > - 8 is a length in bytes
> > - the 5th argument to m_can_fifo_write is the val_count parameter,
> >   whose unit is 4-byte long values.
> > 
> >   By this logic, perhaps the correct value for this argument is:
> > 
> >   DIV_ROUND_UP(8 + cf->len, 4)
> 
> Thank you for spotting this. You are totally right, I will fix it for
> the next version.

Thanks.

> > Also:
> > 
> > - If cf->len is not a multiple of 4, is there a possibility
> >   that uninitialised trailing data in buf will be used
> >   indirectly by m_can_fifo_write()?
> 
> Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
> values above have to be multiple of 4 because of the CAN-FD
> specification.
> 
> With 'buf' it should read garbage from the buffer which I think is not a
> problem as the chip knows how much of the data to use. Also the tx
> elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
> the next element with that. The chip minimum size is 8 bytes for the
> data field anyways. So I think this is fine.

I'm not the expert on the hw in question here, but intuitively
I do feel that it may be unwise to send uninitialised data.
While I'm happy to defer to you on this, I do wonder if it would be somehow
better to use memcpy_and_pad() in place of memcpy().

Re: [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction

[Markus Schneider-Pargmann]

Hi Simon,

On Sat, Feb 04, 2023 at 02:05:57PM +0100, Simon Horman wrote:
> On Mon, Jan 30, 2023 at 09:04:19AM +0100, Markus Schneider-Pargmann wrote:
> > Hi Simon,
> > 
> > On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> > > On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > > > Combine header and data before writing to the transmit fifo to reduce
> > > > the overhead for peripheral chips.
> > > > 
> > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > > ---
> > > >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > > > index 78f6ed744c36..440bc0536951 100644
> > > > --- a/drivers/net/can/m_can/m_can.c
> > > > +++ b/drivers/net/can/m_can/m_can.c
> > > > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > > >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> > > >  		/* End of xmit function for version 3.0.x */
> > > >  	} else {
> > > > +		char buf[TXB_ELEMENT_SIZE];
> > > >  		/* Transmit routine for version >= v3.1.x */
> > > >  
> > > >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > > > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > > >  		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_ID, &fifo_header, 2);
> > > > -		if (err)
> > > > -			goto out_fail;
> > > > +		memcpy(buf, &fifo_header, 8);
> > > > +		memcpy(&buf[8], &cf->data, cf->len);
> > > >  
> > > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > > > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > > > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > > > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> > > 
> > > Perhaps I am missing something here, but my reading is that:
> > > 
> > > - 8 is a length in bytes
> > > - the 5th argument to m_can_fifo_write is the val_count parameter,
> > >   whose unit is 4-byte long values.
> > > 
> > >   By this logic, perhaps the correct value for this argument is:
> > > 
> > >   DIV_ROUND_UP(8 + cf->len, 4)
> > 
> > Thank you for spotting this. You are totally right, I will fix it for
> > the next version.
> 
> Thanks.
> 
> > > Also:
> > > 
> > > - If cf->len is not a multiple of 4, is there a possibility
> > >   that uninitialised trailing data in buf will be used
> > >   indirectly by m_can_fifo_write()?
> > 
> > Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
> > values above have to be multiple of 4 because of the CAN-FD
> > specification.
> > 
> > With 'buf' it should read garbage from the buffer which I think is not a
> > problem as the chip knows how much of the data to use. Also the tx
> > elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
> > the next element with that. The chip minimum size is 8 bytes for the
> > data field anyways. So I think this is fine.
> 
> I'm not the expert on the hw in question here, but intuitively
> I do feel that it may be unwise to send uninitialised data.
> While I'm happy to defer to you on this, I do wonder if it would be somehow
> better to use memcpy_and_pad() in place of memcpy().

Thank you, I think it is safe, but memcpy_and_pad seems like a good
solution here to make it even safer.

Best,
Markus

[PATCH v2 09/18] can: m_can: Implement receive coalescing

[Markus Schneider-Pargmann]

m_can offers the possibility to set an interrupt on reaching a watermark
level in the receive FIFO. This can be used to implement coalescing.
Unfortunately there is no hardware timeout available to trigger an
interrupt if only a few messages were received within a given time. To
solve this I am using a hrtimer to wake up the irq thread after x
microseconds.

The timer is always started if receive coalescing is enabled and new
received frames were available during an interrupt. The timer is stopped
if during a interrupt handling no new data was available.

If the timer is started the new item interrupt is disabled and the
watermark interrupt takes over. If the timer is not started again, the
new item interrupt is enabled again, notifying the handler about every
new item received.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 69 ++++++++++++++++++++++++++++++++---
 drivers/net/can/m_can/m_can.h |  7 ++++
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 440bc0536951..a9d028ebc1f4 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1070,15 +1070,55 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	return err;
 }
 
+static void m_can_interrupt_enable(struct m_can_classdev *cdev, u32 interrupts)
+{
+	if (cdev->active_interrupts == interrupts)
+		return;
+	cdev->ops->write_reg(cdev, M_CAN_IE, interrupts);
+	cdev->active_interrupts = interrupts;
+}
+
+static void m_can_coalescing_disable(struct m_can_classdev *cdev)
+{
+	u32 new_interrupts = cdev->active_interrupts | IR_RF0N;
+
+	hrtimer_cancel(&cdev->irq_timer);
+	m_can_interrupt_enable(cdev, new_interrupts);
+}
+
+static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
+{
+	u32 new_interrupts = cdev->active_interrupts;
+	bool enable_timer = false;
+
+	if (cdev->rx_coalesce_usecs_irq > 0 && (ir & (IR_RF0N | IR_RF0W))) {
+		enable_timer = true;
+		new_interrupts &= ~IR_RF0N;
+	} else if (!hrtimer_active(&cdev->irq_timer)) {
+		new_interrupts |= IR_RF0N;
+	}
+
+	m_can_interrupt_enable(cdev, new_interrupts);
+	if (enable_timer) {
+		hrtimer_start(&cdev->irq_timer,
+			      ns_to_ktime(cdev->rx_coalesce_usecs_irq * NSEC_PER_USEC),
+			      HRTIMER_MODE_REL);
+	}
+}
+
 static irqreturn_t m_can_isr(int irq, void *dev_id)
 {
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 ir;
 
-	if (pm_runtime_suspended(cdev->dev))
+	if (pm_runtime_suspended(cdev->dev)) {
+		m_can_coalescing_disable(cdev);
 		return IRQ_NONE;
+	}
+
 	ir = m_can_read(cdev, M_CAN_IR);
+	m_can_coalescing_update(cdev, ir);
 	if (!ir)
 		return IRQ_NONE;
 
@@ -1093,13 +1133,17 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	 * - state change IRQ
 	 * - bus error IRQ and bus error reporting
 	 */
-	if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X)) {
+	if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
 		cdev->irqstatus = ir;
 		if (!cdev->is_peripheral) {
 			m_can_disable_all_interrupts(cdev);
 			napi_schedule(&cdev->napi);
-		} else if (m_can_rx_peripheral(dev, ir) < 0) {
-			goto out_fail;
+		} else {
+			int pkts;
+
+			pkts = m_can_rx_peripheral(dev, ir);
+			if (pkts < 0)
+				goto out_fail;
 		}
 	}
 
@@ -1135,6 +1179,15 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static enum hrtimer_restart m_can_irq_timer(struct hrtimer *timer)
+{
+	struct m_can_classdev *cdev = container_of(timer, struct m_can_classdev, irq_timer);
+
+	irq_wake_thread(cdev->net->irq, cdev->net);
+
+	return HRTIMER_NORESTART;
+}
+
 static const struct can_bittiming_const m_can_bittiming_const_30X = {
 	.name = KBUILD_MODNAME,
 	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
@@ -1275,7 +1328,7 @@ static int m_can_chip_config(struct net_device *dev)
 	/* Disable unused interrupts */
 	interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TEFW | IR_TFE |
 			IR_TCF | IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N |
-			IR_RF0F | IR_RF0W);
+			IR_RF0F);
 
 	m_can_config_endisable(cdev, true);
 
@@ -1319,6 +1372,7 @@ static int m_can_chip_config(struct net_device *dev)
 
 	/* rx fifo configuration, blocking mode, fifo size 1 */
 	m_can_write(cdev, M_CAN_RXF0C,
+		    FIELD_PREP(RXFC_FWM_MASK, cdev->rx_max_coalesced_frames_irq) |
 		    FIELD_PREP(RXFC_FS_MASK, cdev->mcfg[MRAM_RXF0].num) |
 		    cdev->mcfg[MRAM_RXF0].off);
 
@@ -1377,7 +1431,7 @@ static int m_can_chip_config(struct net_device *dev)
 		else
 			interrupts &= ~(IR_ERR_LEC_31X);
 	}
-	m_can_write(cdev, M_CAN_IE, interrupts);
+	m_can_interrupt_enable(cdev, interrupts);
 
 	/* route all interrupts to INT0 */
 	m_can_write(cdev, M_CAN_ILS, ILS_ALL_INT0);
@@ -2056,6 +2110,9 @@ int m_can_class_register(struct m_can_classdev *cdev)
 
 	of_can_transceiver(cdev->net);
 
+	hrtimer_init(&cdev->irq_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	cdev->irq_timer.function = m_can_irq_timer;
+
 	dev_info(cdev->dev, "%s device registered (irq=%d, version=%d)\n",
 		 KBUILD_MODNAME, cdev->net->irq, cdev->version);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index d8150d8128e7..35aa3c70b0a3 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -84,6 +84,8 @@ struct m_can_classdev {
 	struct sk_buff *tx_skb;
 	struct phy *transceiver;
 
+	struct hrtimer irq_timer;
+
 	struct m_can_ops *ops;
 
 	int version;
@@ -92,6 +94,11 @@ struct m_can_classdev {
 	int pm_clock_support;
 	int is_peripheral;
 
+	// Cached M_CAN_IE register content
+	u32 active_interrupts;
+	u32 rx_max_coalesced_frames_irq;
+	u32 rx_coalesce_usecs_irq;
+
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.39.0

[PATCH v2 10/18] can: m_can: Implement transmit coalescing

[Markus Schneider-Pargmann]

Extend the coalescing implementation for transmits.

In normal mode the chip raises an interrupt for every finished transmit.
This implementation switches to coalescing mode as soon as an interrupt
handled a transmit. For coalescing the watermark level interrupt is used
to interrupt exactly after x frames were sent. It switches back into
normal mode once there was an interrupt with no finished transmit and
the timer being inactive.

The timer is shared with receive coalescing. The time for receive and
transmit coalescing timers have to be the same for that to work. The
benefit is to have only a single running timer.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 33 ++++++++++++++++++++-------------
 drivers/net/can/m_can/m_can.h |  3 +++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a9d028ebc1f4..31a5e551ebd9 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -255,6 +255,7 @@ enum m_can_reg {
 #define TXESC_TBDS_64B		0x7
 
 /* Tx Event FIFO Configuration (TXEFC) */
+#define TXEFC_EFWM_MASK		GENMASK(29, 24)
 #define TXEFC_EFS_MASK		GENMASK(21, 16)
 
 /* Tx Event FIFO Status (TXEFS) */
@@ -1080,7 +1081,7 @@ static void m_can_interrupt_enable(struct m_can_classdev *cdev, u32 interrupts)
 
 static void m_can_coalescing_disable(struct m_can_classdev *cdev)
 {
-	u32 new_interrupts = cdev->active_interrupts | IR_RF0N;
+	u32 new_interrupts = cdev->active_interrupts | IR_RF0N | IR_TEFN;
 
 	hrtimer_cancel(&cdev->irq_timer);
 	m_can_interrupt_enable(cdev, new_interrupts);
@@ -1089,21 +1090,26 @@ static void m_can_coalescing_disable(struct m_can_classdev *cdev)
 static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
 {
 	u32 new_interrupts = cdev->active_interrupts;
-	bool enable_timer = false;
+	bool enable_rx_timer = false;
+	bool enable_tx_timer = false;
 
 	if (cdev->rx_coalesce_usecs_irq > 0 && (ir & (IR_RF0N | IR_RF0W))) {
-		enable_timer = true;
+		enable_rx_timer = true;
 		new_interrupts &= ~IR_RF0N;
-	} else if (!hrtimer_active(&cdev->irq_timer)) {
-		new_interrupts |= IR_RF0N;
 	}
+	if (cdev->tx_coalesce_usecs_irq > 0 && (ir & (IR_TEFN | IR_TEFW))) {
+		enable_tx_timer = true;
+		new_interrupts &= ~IR_TEFN;
+	}
+	if (!enable_rx_timer && !hrtimer_active(&cdev->irq_timer))
+		new_interrupts |= IR_RF0N;
+	if (!enable_tx_timer && !hrtimer_active(&cdev->irq_timer))
+		new_interrupts |= IR_TEFN;
 
 	m_can_interrupt_enable(cdev, new_interrupts);
-	if (enable_timer) {
-		hrtimer_start(&cdev->irq_timer,
-			      ns_to_ktime(cdev->rx_coalesce_usecs_irq * NSEC_PER_USEC),
+	if (enable_rx_timer | enable_tx_timer)
+		hrtimer_start(&cdev->irq_timer, cdev->irq_timer_wait,
 			      HRTIMER_MODE_REL);
-	}
 }
 
 static irqreturn_t m_can_isr(int irq, void *dev_id)
@@ -1158,7 +1164,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 			netif_wake_queue(dev);
 		}
 	} else  {
-		if (ir & IR_TEFN) {
+		if (ir & (IR_TEFN | IR_TEFW)) {
 			/* New TX FIFO Element arrived */
 			if (m_can_echo_tx_event(dev) != 0)
 				goto out_fail;
@@ -1326,9 +1332,8 @@ static int m_can_chip_config(struct net_device *dev)
 	}
 
 	/* Disable unused interrupts */
-	interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TEFW | IR_TFE |
-			IR_TCF | IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N |
-			IR_RF0F);
+	interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TFE | IR_TCF |
+			IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F);
 
 	m_can_config_endisable(cdev, true);
 
@@ -1365,6 +1370,8 @@ static int m_can_chip_config(struct net_device *dev)
 	} else {
 		/* Full TX Event FIFO is used */
 		m_can_write(cdev, M_CAN_TXEFC,
+			    FIELD_PREP(TXEFC_EFWM_MASK,
+				       cdev->tx_max_coalesced_frames_irq) |
 			    FIELD_PREP(TXEFC_EFS_MASK,
 				       cdev->mcfg[MRAM_TXE].num) |
 			    cdev->mcfg[MRAM_TXE].off);
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 35aa3c70b0a3..0721faae7886 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -85,6 +85,7 @@ struct m_can_classdev {
 	struct phy *transceiver;
 
 	struct hrtimer irq_timer;
+	ktime_t irq_timer_wait;
 
 	struct m_can_ops *ops;
 
@@ -98,6 +99,8 @@ struct m_can_classdev {
 	u32 active_interrupts;
 	u32 rx_max_coalesced_frames_irq;
 	u32 rx_coalesce_usecs_irq;
+	u32 tx_max_coalesced_frames_irq;
+	u32 tx_coalesce_usecs_irq;
 
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
-- 
2.39.0

[PATCH v2 11/18] can: m_can: Add rx coalescing ethtool support

[Markus Schneider-Pargmann]

Add the possibility to set coalescing parameters with ethtool.

rx-frames-irq and rx-usecs-irq can only be set and unset together as the
implemented mechanism would not work otherwise. rx-frames-irq can't be
greater than the RX FIFO size.

Also all values can only be changed if the chip is not active.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 46 +++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 31a5e551ebd9..35c38c230a00 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1935,8 +1935,54 @@ static const struct net_device_ops m_can_netdev_ops = {
 	.ndo_change_mtu = can_change_mtu,
 };
 
+static int m_can_get_coalesce(struct net_device *dev,
+			      struct ethtool_coalesce *ec,
+			      struct kernel_ethtool_coalesce *kec,
+			      struct netlink_ext_ack *ext_ack)
+{
+	struct m_can_classdev *cdev = netdev_priv(dev);
+
+	ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
+	ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
+
+	return 0;
+}
+
+static int m_can_set_coalesce(struct net_device *dev,
+			      struct ethtool_coalesce *ec,
+			      struct kernel_ethtool_coalesce *kec,
+			      struct netlink_ext_ack *ext_ack)
+{
+	struct m_can_classdev *cdev = netdev_priv(dev);
+
+	if (cdev->can.state != CAN_STATE_STOPPED) {
+		netdev_err(dev, "Device is in use, please shut it down first\n");
+		return -EBUSY;
+	}
+
+	if (ec->rx_max_coalesced_frames_irq > cdev->mcfg[MRAM_RXF0].num) {
+		netdev_err(dev, "rx-frames-irq (%u) greater than the RX FIFO (%u)\n",
+			   ec->rx_max_coalesced_frames_irq,
+			   cdev->mcfg[MRAM_RXF0].num);
+		return -EINVAL;
+	}
+	if ((ec->rx_max_coalesced_frames_irq == 0) != (ec->rx_coalesce_usecs_irq == 0)) {
+		netdev_err(dev, "rx-frames-irq and rx-usecs-irq can only be set together\n");
+		return -EINVAL;
+	}
+
+	cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
+	cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
+
+	return 0;
+}
+
 static const struct ethtool_ops m_can_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
+		ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ,
 	.get_ts_info = ethtool_op_get_ts_info,
+	.get_coalesce = m_can_get_coalesce,
+	.set_coalesce = m_can_set_coalesce,
 };
 
 static int register_m_can_dev(struct net_device *dev)
-- 
2.39.0

[PATCH v2 12/18] can: m_can: Add tx coalescing ethtool support

[Markus Schneider-Pargmann]

Add get/set functions for ethtool coalescing. tx-frames-irq and
tx-usecs-irq can only be set/unset together. tx-frames-irq needs to be
less than TXE and TXB.

As rx and tx share the same timer, rx-usecs-irq and tx-usecs-irq can be
enabled/disabled individually but they need to have the same value if
enabled.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 35c38c230a00..9d2345f52ddc 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1944,6 +1944,8 @@ static int m_can_get_coalesce(struct net_device *dev,
 
 	ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
 	ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
+	ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
+	ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
 
 	return 0;
 }
@@ -1970,16 +1972,50 @@ static int m_can_set_coalesce(struct net_device *dev,
 		netdev_err(dev, "rx-frames-irq and rx-usecs-irq can only be set together\n");
 		return -EINVAL;
 	}
+	if (ec->tx_max_coalesced_frames_irq > cdev->mcfg[MRAM_TXE].num) {
+		netdev_err(dev, "tx-frames-irq (%u) greater than the TX event FIFO (%u)\n",
+			   ec->tx_max_coalesced_frames_irq,
+			   cdev->mcfg[MRAM_TXE].num);
+		return -EINVAL;
+	}
+	if (ec->tx_max_coalesced_frames_irq > cdev->mcfg[MRAM_TXB].num) {
+		netdev_err(dev, "tx-frames-irq (%u) greater than the TX FIFO (%u)\n",
+			   ec->tx_max_coalesced_frames_irq,
+			   cdev->mcfg[MRAM_TXB].num);
+		return -EINVAL;
+	}
+	if ((ec->tx_max_coalesced_frames_irq == 0) != (ec->tx_coalesce_usecs_irq == 0)) {
+		netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
+		return -EINVAL;
+	}
+	if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
+	    ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
+		netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
+			   ec->rx_coalesce_usecs_irq,
+			   ec->tx_coalesce_usecs_irq);
+		return -EINVAL;
+	}
 
 	cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
 	cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
+	cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
+	cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
+
+	if (cdev->rx_coalesce_usecs_irq)
+		cdev->irq_timer_wait =
+			ns_to_ktime(cdev->rx_coalesce_usecs_irq * NSEC_PER_USEC);
+	else
+		cdev->irq_timer_wait =
+			ns_to_ktime(cdev->tx_coalesce_usecs_irq * NSEC_PER_USEC);
 
 	return 0;
 }
 
 static const struct ethtool_ops m_can_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
-		ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ,
+		ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
+		ETHTOOL_COALESCE_TX_USECS_IRQ |
+		ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
 	.get_ts_info = ethtool_op_get_ts_info,
 	.get_coalesce = m_can_get_coalesce,
 	.set_coalesce = m_can_set_coalesce,
-- 
2.39.0

[PATCH v2 13/18] can: m_can: Cache tx putidx

[Markus Schneider-Pargmann]

m_can_tx_handler is the only place where data is written to the tx fifo.
We can calculate the putidx in the driver code here to avoid the
dependency on the txfqs register.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 8 +++++++-
 drivers/net/can/m_can/m_can.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 9d2345f52ddc..83ffb4c0eb4c 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1475,6 +1475,10 @@ static int m_can_start(struct net_device *dev)
 
 	m_can_enable_all_interrupts(cdev);
 
+	if (cdev->version > 30)
+		cdev->tx_fifo_putidx = FIELD_GET(TXFQS_TFQPI_MASK,
+						 m_can_read(cdev, M_CAN_TXFQS));
+
 	return 0;
 }
 
@@ -1764,7 +1768,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		}
 
 		/* get put index for frame */
-		putidx = FIELD_GET(TXFQS_TFQPI_MASK, txfqs);
+		putidx = cdev->tx_fifo_putidx;
 
 		/* Construct DLC Field, with CAN-FD configuration.
 		 * Use the put index of the fifo as the message marker,
@@ -1797,6 +1801,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 
 		/* Enable TX FIFO element to start transfer  */
 		m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
+		cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
+					0 : cdev->tx_fifo_putidx);
 
 		/* stop network queue if fifo full */
 		if (m_can_tx_fifo_full(cdev) ||
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 0721faae7886..4437085f63b8 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -102,6 +102,9 @@ struct m_can_classdev {
 	u32 tx_max_coalesced_frames_irq;
 	u32 tx_coalesce_usecs_irq;
 
+	// Store this internally to avoid fetch delays on peripheral chips
+	int tx_fifo_putidx;
+
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.39.0

[PATCH v2 14/18] can: m_can: Use the workqueue as queue

[Markus Schneider-Pargmann]

The current implementation uses the workqueue for peripheral chips to
submit work. Only a single work item is queued and used at any time.

To be able to keep more than one transmit in flight at a time, prepare
the workqueue to support multiple transmits at the same time.

Each work item now has a separate storage for a skb and a pointer to
cdev. This assures that each workitem can be processed individually.

The workqueue is replaced by an ordered workqueue which makes sure that
only a single worker processes the items queued on the workqueue. Also
items are ordered by the order they were enqueued. This removes most of
the concurrency the workqueue normally offers. It is not necessary for
this driver.

The cleanup functions have to be adopted a bit to handle this new
mechanism.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 109 ++++++++++++++++++++--------------
 drivers/net/can/m_can/m_can.h |  12 +++-
 2 files changed, 74 insertions(+), 47 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 83ffb4c0eb4c..03e6466f9b32 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -443,17 +443,16 @@ static void m_can_clean(struct net_device *net)
 {
 	struct m_can_classdev *cdev = netdev_priv(net);
 
-	if (cdev->tx_skb) {
-		int putidx = 0;
+	for (int i = 0; i != cdev->tx_fifo_size; ++i) {
+		if (!cdev->tx_ops[i].skb)
+			continue;
 
 		net->stats.tx_errors++;
-		if (cdev->version > 30)
-			putidx = FIELD_GET(TXFQS_TFQPI_MASK,
-					   m_can_read(cdev, M_CAN_TXFQS));
-
-		can_free_echo_skb(cdev->net, putidx, NULL);
-		cdev->tx_skb = NULL;
+		cdev->tx_ops[i].skb = NULL;
 	}
+
+	for (int i = 0; i != cdev->can.echo_skb_max; ++i)
+		can_free_echo_skb(cdev->net, i, NULL);
 }
 
 /* For peripherals, pass skb to rx-offload, which will push skb from
@@ -1656,8 +1655,9 @@ static int m_can_close(struct net_device *dev)
 	m_can_clk_stop(cdev);
 	free_irq(dev->irq, dev);
 
+	m_can_clean(dev);
+
 	if (cdev->is_peripheral) {
-		cdev->tx_skb = NULL;
 		destroy_workqueue(cdev->tx_wq);
 		cdev->tx_wq = NULL;
 		can_rx_offload_disable(&cdev->offload);
@@ -1684,19 +1684,17 @@ static int m_can_next_echo_skb_occupied(struct net_device *dev, int putidx)
 	return !!cdev->can.echo_skb[next_idx];
 }
 
-static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
+static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
+				    struct sk_buff *skb)
 {
-	struct canfd_frame *cf = (struct canfd_frame *)cdev->tx_skb->data;
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 	struct net_device *dev = cdev->net;
-	struct sk_buff *skb = cdev->tx_skb;
 	struct id_and_dlc fifo_header;
 	u32 cccr, fdflags;
 	u32 txfqs;
 	int err;
 	int putidx;
 
-	cdev->tx_skb = NULL;
-
 	/* Generate ID field for TX buffer Element */
 	/* Common to all supported M_CAN versions */
 	if (cf->can_id & CAN_EFF_FLAG) {
@@ -1820,10 +1818,36 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 
 static void m_can_tx_work_queue(struct work_struct *ws)
 {
-	struct m_can_classdev *cdev = container_of(ws, struct m_can_classdev,
-						   tx_work);
+	struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
+	struct m_can_classdev *cdev = op->cdev;
+	struct sk_buff *skb = op->skb;
 
-	m_can_tx_handler(cdev);
+	op->skb = NULL;
+	m_can_tx_handler(cdev, skb);
+}
+
+static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
+{
+	cdev->tx_ops[cdev->next_tx_op].skb = skb;
+	queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
+
+	++cdev->next_tx_op;
+	if (cdev->next_tx_op >= cdev->tx_fifo_size)
+		cdev->next_tx_op = 0;
+}
+
+static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
+					       struct sk_buff *skb)
+{
+	if (cdev->can.state == CAN_STATE_BUS_OFF) {
+		m_can_clean(cdev->net);
+		return NETDEV_TX_OK;
+	}
+
+	netif_stop_queue(cdev->net);
+	m_can_tx_queue_skb(cdev, skb);
+
+	return NETDEV_TX_OK;
 }
 
 static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
@@ -1834,30 +1858,10 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	if (can_dev_dropped_skb(dev, skb))
 		return NETDEV_TX_OK;
 
-	if (cdev->is_peripheral) {
-		if (cdev->tx_skb) {
-			netdev_err(dev, "hard_xmit called while tx busy\n");
-			return NETDEV_TX_BUSY;
-		}
-
-		if (cdev->can.state == CAN_STATE_BUS_OFF) {
-			m_can_clean(dev);
-		} else {
-			/* Need to stop the queue to avoid numerous requests
-			 * from being sent.  Suggested improvement is to create
-			 * a queueing mechanism that will queue the skbs and
-			 * process them in order.
-			 */
-			cdev->tx_skb = skb;
-			netif_stop_queue(cdev->net);
-			queue_work(cdev->tx_wq, &cdev->tx_work);
-		}
-	} else {
-		cdev->tx_skb = skb;
-		return m_can_tx_handler(cdev);
-	}
-
-	return NETDEV_TX_OK;
+	if (cdev->is_peripheral)
+		return m_can_start_peripheral_xmit(cdev, skb);
+	else
+		return m_can_tx_handler(cdev, skb);
 }
 
 static int m_can_open(struct net_device *dev)
@@ -1885,15 +1889,17 @@ static int m_can_open(struct net_device *dev)
 
 	/* register interrupt handler */
 	if (cdev->is_peripheral) {
-		cdev->tx_skb = NULL;
-		cdev->tx_wq = alloc_workqueue("mcan_wq",
-					      WQ_FREEZABLE | WQ_MEM_RECLAIM, 0);
+		cdev->tx_wq = alloc_ordered_workqueue("mcan_wq",
+						      WQ_FREEZABLE | WQ_MEM_RECLAIM);
 		if (!cdev->tx_wq) {
 			err = -ENOMEM;
 			goto out_wq_fail;
 		}
 
-		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
+		for (int i = 0; i != cdev->tx_fifo_size; ++i) {
+			cdev->tx_ops[i].cdev = cdev;
+			INIT_WORK(&cdev->tx_ops[i].work, m_can_tx_work_queue);
+		}
 
 		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
 					   IRQF_ONESHOT,
@@ -2179,6 +2185,19 @@ int m_can_class_register(struct m_can_classdev *cdev)
 {
 	int ret;
 
+	cdev->tx_fifo_size = max(1, min(cdev->mcfg[MRAM_TXB].num,
+					cdev->mcfg[MRAM_TXE].num));
+	if (cdev->is_peripheral) {
+		cdev->tx_ops =
+			devm_kzalloc(cdev->dev,
+				     cdev->tx_fifo_size * sizeof(*cdev->tx_ops),
+				     GFP_KERNEL);
+		if (!cdev->tx_ops) {
+			dev_err(cdev->dev, "Failed to allocate tx_ops for workqueue\n");
+			return -ENOMEM;
+		}
+	}
+
 	if (cdev->pm_clock_support) {
 		ret = m_can_clk_start(cdev);
 		if (ret)
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 4437085f63b8..774672fe7d2e 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -70,6 +70,12 @@ struct m_can_ops {
 	int (*init)(struct m_can_classdev *cdev);
 };
 
+struct m_can_tx_op {
+	struct m_can_classdev *cdev;
+	struct work_struct work;
+	struct sk_buff *skb;
+};
+
 struct m_can_classdev {
 	struct can_priv can;
 	struct can_rx_offload offload;
@@ -80,8 +86,6 @@ struct m_can_classdev {
 	struct clk *cclk;
 
 	struct workqueue_struct *tx_wq;
-	struct work_struct tx_work;
-	struct sk_buff *tx_skb;
 	struct phy *transceiver;
 
 	struct hrtimer irq_timer;
@@ -105,6 +109,10 @@ struct m_can_classdev {
 	// Store this internally to avoid fetch delays on peripheral chips
 	int tx_fifo_putidx;
 
+	struct m_can_tx_op *tx_ops;
+	int tx_fifo_size;
+	int next_tx_op;
+
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.39.0

[PATCH v2 15/18] can: m_can: Introduce a tx_fifo_in_flight counter

[Markus Schneider-Pargmann]

Keep track of the number of transmits in flight.

This patch prepares the driver to control the network interface queue
based on this counter. By itself this counter be
implemented with an atomic, but as we need to do other things in the
critical sections later I am using a spinlock instead.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 36 ++++++++++++++++++++++++++++++++++-
 drivers/net/can/m_can/m_can.h |  4 ++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 03e6466f9b32..f46d411bc796 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -453,6 +453,10 @@ static void m_can_clean(struct net_device *net)
 
 	for (int i = 0; i != cdev->can.echo_skb_max; ++i)
 		can_free_echo_skb(cdev->net, i, NULL);
+
+	spin_lock(&cdev->tx_handling_spinlock);
+	cdev->tx_fifo_in_flight = 0;
+	spin_unlock(&cdev->tx_handling_spinlock);
 }
 
 /* For peripherals, pass skb to rx-offload, which will push skb from
@@ -1023,6 +1027,20 @@ static void m_can_tx_update_stats(struct m_can_classdev *cdev,
 	stats->tx_packets++;
 }
 
+static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
+{
+	spin_lock(&cdev->tx_handling_spinlock);
+	cdev->tx_fifo_in_flight -= transmitted;
+	spin_unlock(&cdev->tx_handling_spinlock);
+}
+
+static void m_can_start_tx(struct m_can_classdev *cdev)
+{
+	spin_lock(&cdev->tx_handling_spinlock);
+	++cdev->tx_fifo_in_flight;
+	spin_unlock(&cdev->tx_handling_spinlock);
+}
+
 static int m_can_echo_tx_event(struct net_device *dev)
 {
 	u32 txe_count = 0;
@@ -1032,6 +1050,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	int i = 0;
 	int err = 0;
 	unsigned int msg_mark;
+	int processed = 0;
 
 	struct m_can_classdev *cdev = netdev_priv(dev);
 
@@ -1061,12 +1080,15 @@ static int m_can_echo_tx_event(struct net_device *dev)
 
 		/* update stats */
 		m_can_tx_update_stats(cdev, msg_mark, timestamp);
+		++processed;
 	}
 
 	if (ack_fgi != -1)
 		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
 							  ack_fgi));
 
+	m_can_finish_tx(cdev, processed);
+
 	return err;
 }
 
@@ -1161,6 +1183,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 				timestamp = m_can_get_timestamp(cdev);
 			m_can_tx_update_stats(cdev, 0, timestamp);
 			netif_wake_queue(dev);
+			m_can_finish_tx(cdev, 1);
 		}
 	} else  {
 		if (ir & (IR_TEFN | IR_TEFW)) {
@@ -1845,11 +1868,22 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
 	}
 
 	netif_stop_queue(cdev->net);
+
+	m_can_start_tx(cdev);
+
 	m_can_tx_queue_skb(cdev, skb);
 
 	return NETDEV_TX_OK;
 }
 
+static netdev_tx_t m_can_start_fast_xmit(struct m_can_classdev *cdev,
+					 struct sk_buff *skb)
+{
+	m_can_start_tx(cdev);
+
+	return m_can_tx_handler(cdev, skb);
+}
+
 static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
@@ -1861,7 +1895,7 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	if (cdev->is_peripheral)
 		return m_can_start_peripheral_xmit(cdev, skb);
 	else
-		return m_can_tx_handler(cdev, skb);
+		return m_can_start_fast_xmit(cdev, skb);
 }
 
 static int m_can_open(struct net_device *dev)
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 774672fe7d2e..bfef2c89e239 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -109,6 +109,10 @@ struct m_can_classdev {
 	// Store this internally to avoid fetch delays on peripheral chips
 	int tx_fifo_putidx;
 
+	/* Protects shared state between start_xmit and m_can_isr */
+	spinlock_t tx_handling_spinlock;
+	int tx_fifo_in_flight;
+
 	struct m_can_tx_op *tx_ops;
 	int tx_fifo_size;
 	int next_tx_op;
-- 
2.39.0

[PATCH v2 16/18] can: m_can: Use tx_fifo_in_flight for netif_queue control

[Markus Schneider-Pargmann]

The network queue is currently always stopped in start_xmit and
continued in the interrupt handler. This is not possible anymore if we
want to keep multiple transmits in flight in parallel.

Use the previously introduced tx_fifo_in_flight counter to control the
network queue instead. This has the benefit of not needing to ask the
hardware about fifo status.

This patch stops the network queue in start_xmit if the number of
transmits in flight reaches the size of the fifo and wakes up the queue
from the interrupt handler once the transmits in flight drops below the
fifo size. This means any skbs over the limit will be rejected
immediately in start_xmit (it shouldn't be possible at all to reach that
state anyways).

The maximum number of transmits in flight is the size of the fifo.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 72 +++++++++++++----------------------
 1 file changed, 27 insertions(+), 45 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f46d411bc796..b24ca7a1ecfc 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -370,16 +370,6 @@ m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
 	return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
 }
 
-static inline bool _m_can_tx_fifo_full(u32 txfqs)
-{
-	return !!(txfqs & TXFQS_TFQF);
-}
-
-static inline bool m_can_tx_fifo_full(struct m_can_classdev *cdev)
-{
-	return _m_can_tx_fifo_full(m_can_read(cdev, M_CAN_TXFQS));
-}
-
 static void m_can_config_endisable(struct m_can_classdev *cdev, bool enable)
 {
 	u32 cccr = m_can_read(cdev, M_CAN_CCCR);
@@ -1030,15 +1020,30 @@ static void m_can_tx_update_stats(struct m_can_classdev *cdev,
 static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
 {
 	spin_lock(&cdev->tx_handling_spinlock);
+	if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_size && transmitted > 0)
+		netif_wake_queue(cdev->net);
 	cdev->tx_fifo_in_flight -= transmitted;
 	spin_unlock(&cdev->tx_handling_spinlock);
 }
 
-static void m_can_start_tx(struct m_can_classdev *cdev)
+static netdev_tx_t m_can_start_tx(struct m_can_classdev *cdev)
 {
+	int tx_fifo_in_flight;
+
 	spin_lock(&cdev->tx_handling_spinlock);
-	++cdev->tx_fifo_in_flight;
+	tx_fifo_in_flight = cdev->tx_fifo_in_flight + 1;
+	if (tx_fifo_in_flight >= cdev->tx_fifo_size) {
+		netif_stop_queue(cdev->net);
+		if (tx_fifo_in_flight > cdev->tx_fifo_size) {
+			netdev_err(cdev->net, "hard_xmit called while TX FIFO full\n");
+			spin_unlock(&cdev->tx_handling_spinlock);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	cdev->tx_fifo_in_flight = tx_fifo_in_flight;
 	spin_unlock(&cdev->tx_handling_spinlock);
+
+	return NETDEV_TX_OK;
 }
 
 static int m_can_echo_tx_event(struct net_device *dev)
@@ -1182,7 +1187,6 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 			if (cdev->is_peripheral)
 				timestamp = m_can_get_timestamp(cdev);
 			m_can_tx_update_stats(cdev, 0, timestamp);
-			netif_wake_queue(dev);
 			m_can_finish_tx(cdev, 1);
 		}
 	} else  {
@@ -1190,10 +1194,6 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 			/* New TX FIFO Element arrived */
 			if (m_can_echo_tx_event(dev) != 0)
 				goto out_fail;
-
-			if (netif_queue_stopped(dev) &&
-			    !m_can_tx_fifo_full(cdev))
-				netif_wake_queue(dev);
 		}
 	}
 
@@ -1714,7 +1714,6 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 	struct net_device *dev = cdev->net;
 	struct id_and_dlc fifo_header;
 	u32 cccr, fdflags;
-	u32 txfqs;
 	int err;
 	int putidx;
 
@@ -1770,24 +1769,6 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 		char buf[TXB_ELEMENT_SIZE];
 		/* Transmit routine for version >= v3.1.x */
 
-		txfqs = m_can_read(cdev, M_CAN_TXFQS);
-
-		/* Check if FIFO full */
-		if (_m_can_tx_fifo_full(txfqs)) {
-			/* This shouldn't happen */
-			netif_stop_queue(dev);
-			netdev_warn(dev,
-				    "TX queue active although FIFO is full.");
-
-			if (cdev->is_peripheral) {
-				kfree_skb(skb);
-				dev->stats.tx_dropped++;
-				return NETDEV_TX_OK;
-			} else {
-				return NETDEV_TX_BUSY;
-			}
-		}
-
 		/* get put index for frame */
 		putidx = cdev->tx_fifo_putidx;
 
@@ -1824,11 +1805,6 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 		m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
 		cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
 					0 : cdev->tx_fifo_putidx);
-
-		/* stop network queue if fifo full */
-		if (m_can_tx_fifo_full(cdev) ||
-		    m_can_next_echo_skb_occupied(dev, putidx))
-			netif_stop_queue(dev);
 	}
 
 	return NETDEV_TX_OK;
@@ -1862,14 +1838,16 @@ static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
 static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
 					       struct sk_buff *skb)
 {
+	netdev_tx_t err;
+
 	if (cdev->can.state == CAN_STATE_BUS_OFF) {
 		m_can_clean(cdev->net);
 		return NETDEV_TX_OK;
 	}
 
-	netif_stop_queue(cdev->net);
-
-	m_can_start_tx(cdev);
+	err = m_can_start_tx(cdev);
+	if (err != NETDEV_TX_OK)
+		return err;
 
 	m_can_tx_queue_skb(cdev, skb);
 
@@ -1879,7 +1857,11 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
 static netdev_tx_t m_can_start_fast_xmit(struct m_can_classdev *cdev,
 					 struct sk_buff *skb)
 {
-	m_can_start_tx(cdev);
+	netdev_tx_t err;
+
+	err = m_can_start_tx(cdev);
+	if (err != NETDEV_TX_OK)
+		return err;
 
 	return m_can_tx_handler(cdev, skb);
 }
-- 
2.39.0

[PATCH v2 17/18] can: m_can: Implement BQL

[Markus Schneider-Pargmann]

Implement byte queue limiting in preparation for the use of xmit_more().

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 49 +++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index b24ca7a1ecfc..c6a09369d1aa 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -444,6 +444,8 @@ static void m_can_clean(struct net_device *net)
 	for (int i = 0; i != cdev->can.echo_skb_max; ++i)
 		can_free_echo_skb(cdev->net, i, NULL);
 
+	netdev_reset_queue(cdev->net);
+
 	spin_lock(&cdev->tx_handling_spinlock);
 	cdev->tx_fifo_in_flight = 0;
 	spin_unlock(&cdev->tx_handling_spinlock);
@@ -998,27 +1000,32 @@ static int m_can_poll(struct napi_struct *napi, int quota)
  * echo. timestamp is used for peripherals to ensure correct ordering
  * by rx-offload, and is ignored for non-peripherals.
  */
-static void m_can_tx_update_stats(struct m_can_classdev *cdev,
-				  unsigned int msg_mark,
-				  u32 timestamp)
+static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
+					  unsigned int msg_mark, u32 timestamp)
 {
 	struct net_device *dev = cdev->net;
 	struct net_device_stats *stats = &dev->stats;
+	unsigned int frame_len;
 
 	if (cdev->is_peripheral)
 		stats->tx_bytes +=
 			can_rx_offload_get_echo_skb(&cdev->offload,
 						    msg_mark,
 						    timestamp,
-						    NULL);
+						    &frame_len);
 	else
-		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, NULL);
+		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, &frame_len);
 
 	stats->tx_packets++;
+
+	return frame_len;
 }
 
-static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
+static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted,
+			    int transmitted_frame_len)
 {
+	netdev_completed_queue(cdev->net, transmitted, transmitted_frame_len);
+
 	spin_lock(&cdev->tx_handling_spinlock);
 	if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_size && transmitted > 0)
 		netif_wake_queue(cdev->net);
@@ -1056,6 +1063,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	int err = 0;
 	unsigned int msg_mark;
 	int processed = 0;
+	int processed_frame_len = 0;
 
 	struct m_can_classdev *cdev = netdev_priv(dev);
 
@@ -1084,7 +1092,9 @@ static int m_can_echo_tx_event(struct net_device *dev)
 		fgi = (++fgi >= cdev->mcfg[MRAM_TXE].num ? 0 : fgi);
 
 		/* update stats */
-		m_can_tx_update_stats(cdev, msg_mark, timestamp);
+		processed_frame_len += m_can_tx_update_stats(cdev, msg_mark,
+							     timestamp);
+
 		++processed;
 	}
 
@@ -1092,7 +1102,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
 							  ack_fgi));
 
-	m_can_finish_tx(cdev, processed);
+	m_can_finish_tx(cdev, processed, processed_frame_len);
 
 	return err;
 }
@@ -1183,11 +1193,12 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 		if (ir & IR_TC) {
 			/* Transmission Complete Interrupt*/
 			u32 timestamp = 0;
+			unsigned int frame_len;
 
 			if (cdev->is_peripheral)
 				timestamp = m_can_get_timestamp(cdev);
-			m_can_tx_update_stats(cdev, 0, timestamp);
-			m_can_finish_tx(cdev, 1);
+			frame_len = m_can_tx_update_stats(cdev, 0, timestamp);
+			m_can_finish_tx(cdev, 1, frame_len);
 		}
 	} else  {
 		if (ir & (IR_TEFN | IR_TEFW)) {
@@ -1716,6 +1727,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 	u32 cccr, fdflags;
 	int err;
 	int putidx;
+	unsigned int frame_len = can_skb_get_frame_len(skb);
 
 	/* Generate ID field for TX buffer Element */
 	/* Common to all supported M_CAN versions */
@@ -1761,7 +1773,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 		}
 		m_can_write(cdev, M_CAN_TXBTIE, 0x1);
 
-		can_put_echo_skb(skb, dev, 0, 0);
+		can_put_echo_skb(skb, dev, 0, frame_len);
 
 		m_can_write(cdev, M_CAN_TXBAR, 0x1);
 		/* End of xmit function for version 3.0.x */
@@ -1799,7 +1811,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 		/* Push loopback echo.
 		 * Will be looped back on TX interrupt based on message marker
 		 */
-		can_put_echo_skb(skb, dev, putidx, 0);
+		can_put_echo_skb(skb, dev, putidx, frame_len);
 
 		/* Enable TX FIFO element to start transfer  */
 		m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
@@ -1870,14 +1882,23 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
+	netdev_tx_t ret;
+	unsigned int frame_len;
 
 	if (can_dev_dropped_skb(dev, skb))
 		return NETDEV_TX_OK;
 
+	frame_len = can_skb_get_frame_len(skb);
+
 	if (cdev->is_peripheral)
-		return m_can_start_peripheral_xmit(cdev, skb);
+		ret = m_can_start_peripheral_xmit(cdev, skb);
 	else
-		return m_can_start_fast_xmit(cdev, skb);
+		ret = m_can_start_fast_xmit(cdev, skb);
+
+	if (ret == NETDEV_TX_OK)
+		netdev_sent_queue(dev, frame_len);
+
+	return ret;
 }
 
 static int m_can_open(struct net_device *dev)
-- 
2.39.0

[PATCH v2 18/18] can: m_can: Implement transmit submission coalescing

[Markus Schneider-Pargmann]

m_can supports submitting mulitple transmits with one register write.
This is an interesting option to reduce the number of SPI transfers for
peripheral chips.

The m_can_tx_op is extended with a bool that signals if it is the last
transmission and the submit should be executed immediately.

The worker then writes the skb to the FIFO and submits it only if the
submit bool is set. If it isn't set, the worker will write the next skb
which is waiting in the workqueue to the FIFO, etc.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---

Notes:
    Notes:
    - I ran into lost messages in the receive FIFO when using this
      implementation. I guess this only shows up with my test setup in
      loopback mode and maybe not enough CPU power.
    - I put this behind the tx-frames ethtool coalescing option as we do
      wait before submitting packages but it is something different than the
      tx-frames-irq option. I am not sure if this is the correct option,
      please let me know.

 drivers/net/can/m_can/m_can.c | 55 ++++++++++++++++++++++++++++++++---
 drivers/net/can/m_can/m_can.h |  6 ++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index c6a09369d1aa..99bfcfec3775 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1504,6 +1504,9 @@ static int m_can_start(struct net_device *dev)
 	if (ret)
 		return ret;
 
+	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
+				       cdev->tx_max_coalesced_frames);
+
 	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	m_can_enable_all_interrupts(cdev);
@@ -1813,8 +1816,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 		 */
 		can_put_echo_skb(skb, dev, putidx, frame_len);
 
-		/* Enable TX FIFO element to start transfer  */
-		m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
+		if (cdev->is_peripheral) {
+			/* Delay enabling TX FIFO element */
+			cdev->tx_peripheral_submit |= BIT(putidx);
+		} else {
+			/* Enable TX FIFO element to start transfer  */
+			m_can_write(cdev, M_CAN_TXBAR, BIT(putidx));
+		}
 		cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
 					0 : cdev->tx_fifo_putidx);
 	}
@@ -1827,6 +1835,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
 	return NETDEV_TX_BUSY;
 }
 
+static void m_can_tx_submit(struct m_can_classdev *cdev)
+{
+	if (cdev->version == 30)
+		return;
+	if (!cdev->is_peripheral)
+		return;
+
+	m_can_write(cdev, M_CAN_TXBAR, cdev->tx_peripheral_submit);
+	cdev->tx_peripheral_submit = 0;
+}
+
 static void m_can_tx_work_queue(struct work_struct *ws)
 {
 	struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
@@ -1835,11 +1854,15 @@ static void m_can_tx_work_queue(struct work_struct *ws)
 
 	op->skb = NULL;
 	m_can_tx_handler(cdev, skb);
+	if (op->submit)
+		m_can_tx_submit(cdev);
 }
 
-static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
+static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb,
+			       bool submit)
 {
 	cdev->tx_ops[cdev->next_tx_op].skb = skb;
+	cdev->tx_ops[cdev->next_tx_op].submit = submit;
 	queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
 
 	++cdev->next_tx_op;
@@ -1851,6 +1874,7 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
 					       struct sk_buff *skb)
 {
 	netdev_tx_t err;
+	bool submit;
 
 	if (cdev->can.state == CAN_STATE_BUS_OFF) {
 		m_can_clean(cdev->net);
@@ -1861,7 +1885,15 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
 	if (err != NETDEV_TX_OK)
 		return err;
 
-	m_can_tx_queue_skb(cdev, skb);
+	++cdev->nr_txs_without_submit;
+	if (cdev->nr_txs_without_submit >= cdev->tx_max_coalesced_frames ||
+	    !netdev_xmit_more()) {
+		cdev->nr_txs_without_submit = 0;
+		submit = true;
+	} else {
+		submit = false;
+	}
+	m_can_tx_queue_skb(cdev, skb, submit);
 
 	return NETDEV_TX_OK;
 }
@@ -1993,6 +2025,7 @@ static int m_can_get_coalesce(struct net_device *dev,
 
 	ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
 	ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
+	ec->tx_max_coalesced_frames = cdev->tx_max_coalesced_frames;
 	ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
 	ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
 
@@ -2037,6 +2070,18 @@ static int m_can_set_coalesce(struct net_device *dev,
 		netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
 		return -EINVAL;
 	}
+	if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXE].num) {
+		netdev_err(dev, "tx-frames (%u) greater than the TX event FIFO (%u)\n",
+			   ec->tx_max_coalesced_frames,
+			   cdev->mcfg[MRAM_TXE].num);
+		return -EINVAL;
+	}
+	if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXB].num) {
+		netdev_err(dev, "tx-frames (%u) greater than the TX FIFO (%u)\n",
+			   ec->tx_max_coalesced_frames,
+			   cdev->mcfg[MRAM_TXB].num);
+		return -EINVAL;
+	}
 	if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
 	    ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
 		netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
@@ -2047,6 +2092,7 @@ static int m_can_set_coalesce(struct net_device *dev,
 
 	cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
 	cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
+	cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
 	cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
 	cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
 
@@ -2064,6 +2110,7 @@ static const struct ethtool_ops m_can_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
 		ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
 		ETHTOOL_COALESCE_TX_USECS_IRQ |
+		ETHTOOL_COALESCE_TX_MAX_FRAMES |
 		ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
 	.get_ts_info = ethtool_op_get_ts_info,
 	.get_coalesce = m_can_get_coalesce,
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index bfef2c89e239..e209de81b5a4 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -74,6 +74,7 @@ struct m_can_tx_op {
 	struct m_can_classdev *cdev;
 	struct work_struct work;
 	struct sk_buff *skb;
+	bool submit;
 };
 
 struct m_can_classdev {
@@ -103,6 +104,7 @@ struct m_can_classdev {
 	u32 active_interrupts;
 	u32 rx_max_coalesced_frames_irq;
 	u32 rx_coalesce_usecs_irq;
+	u32 tx_max_coalesced_frames;
 	u32 tx_max_coalesced_frames_irq;
 	u32 tx_coalesce_usecs_irq;
 
@@ -117,6 +119,10 @@ struct m_can_classdev {
 	int tx_fifo_size;
 	int next_tx_op;
 
+	int nr_txs_without_submit;
+	/* bitfield of fifo elements that will be submitted together */
+	u32 tx_peripheral_submit;
+
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.39.0

Re: [PATCH v2 18/18] can: m_can: Implement transmit submission coalescing

[Vincent MAILHOL]

Hi Markus,

I did not have time for a deep review yet. But so far, I didn't find
anything odd aside from two nitpicks.

On Thu. 26 Jan 2023 à 04:53, Markus Schneider-Pargmann
<msp@baylibre.com> a écrit :
>
> m_can supports submitting mulitple transmits with one register write.
                            ^^^^^^^^
multiple

> This is an interesting option to reduce the number of SPI transfers for
> peripheral chips.
>
> The m_can_tx_op is extended with a bool that signals if it is the last
> transmission and the submit should be executed immediately.
>
> The worker then writes the skb to the FIFO and submits it only if the
> submit bool is set. If it isn't set, the worker will write the next skb
> which is waiting in the workqueue to the FIFO, etc.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>
> Notes:
>     Notes:
>     - I ran into lost messages in the receive FIFO when using this
>       implementation. I guess this only shows up with my test setup in
>       loopback mode and maybe not enough CPU power.
>     - I put this behind the tx-frames ethtool coalescing option as we do
>       wait before submitting packages but it is something different than the
>       tx-frames-irq option. I am not sure if this is the correct option,
>       please let me know.
>
>  drivers/net/can/m_can/m_can.c | 55 ++++++++++++++++++++++++++++++++---
>  drivers/net/can/m_can/m_can.h |  6 ++++
>  2 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index c6a09369d1aa..99bfcfec3775 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1504,6 +1504,9 @@ static int m_can_start(struct net_device *dev)
>         if (ret)
>                 return ret;
>
> +       netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
> +                                      cdev->tx_max_coalesced_frames);
> +
>         cdev->can.state = CAN_STATE_ERROR_ACTIVE;
>
>         m_can_enable_all_interrupts(cdev);
> @@ -1813,8 +1816,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
>                  */
>                 can_put_echo_skb(skb, dev, putidx, frame_len);
>
> -               /* Enable TX FIFO element to start transfer  */
> -               m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
> +               if (cdev->is_peripheral) {
> +                       /* Delay enabling TX FIFO element */
> +                       cdev->tx_peripheral_submit |= BIT(putidx);
> +               } else {
> +                       /* Enable TX FIFO element to start transfer  */
> +                       m_can_write(cdev, M_CAN_TXBAR, BIT(putidx));
> +               }
>                 cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
>                                         0 : cdev->tx_fifo_putidx);
>         }
> @@ -1827,6 +1835,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
>         return NETDEV_TX_BUSY;
>  }
>
> +static void m_can_tx_submit(struct m_can_classdev *cdev)
> +{
> +       if (cdev->version == 30)
> +               return;
> +       if (!cdev->is_peripheral)
> +               return;
> +
> +       m_can_write(cdev, M_CAN_TXBAR, cdev->tx_peripheral_submit);
> +       cdev->tx_peripheral_submit = 0;
> +}
> +
>  static void m_can_tx_work_queue(struct work_struct *ws)
>  {
>         struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
> @@ -1835,11 +1854,15 @@ static void m_can_tx_work_queue(struct work_struct *ws)
>
>         op->skb = NULL;
>         m_can_tx_handler(cdev, skb);
> +       if (op->submit)
> +               m_can_tx_submit(cdev);
>  }
>
> -static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
> +static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb,
> +                              bool submit)
>  {
>         cdev->tx_ops[cdev->next_tx_op].skb = skb;
> +       cdev->tx_ops[cdev->next_tx_op].submit = submit;
>         queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
>
>         ++cdev->next_tx_op;
> @@ -1851,6 +1874,7 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
>                                                struct sk_buff *skb)
>  {
>         netdev_tx_t err;
> +       bool submit;
>
>         if (cdev->can.state == CAN_STATE_BUS_OFF) {
>                 m_can_clean(cdev->net);
> @@ -1861,7 +1885,15 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
>         if (err != NETDEV_TX_OK)
>                 return err;
>
> -       m_can_tx_queue_skb(cdev, skb);
> +       ++cdev->nr_txs_without_submit;
> +       if (cdev->nr_txs_without_submit >= cdev->tx_max_coalesced_frames ||
> +           !netdev_xmit_more()) {
> +               cdev->nr_txs_without_submit = 0;
> +               submit = true;
> +       } else {
> +               submit = false;
> +       }
> +       m_can_tx_queue_skb(cdev, skb, submit);
>
>         return NETDEV_TX_OK;
>  }
> @@ -1993,6 +2025,7 @@ static int m_can_get_coalesce(struct net_device *dev,
>
>         ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
>         ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
> +       ec->tx_max_coalesced_frames = cdev->tx_max_coalesced_frames;
>         ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
>         ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
>
> @@ -2037,6 +2070,18 @@ static int m_can_set_coalesce(struct net_device *dev,
>                 netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
>                 return -EINVAL;
>         }
> +       if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXE].num) {
> +               netdev_err(dev, "tx-frames (%u) greater than the TX event FIFO (%u)\n",
                                             ^^^^
          ^^^^
Avoid parenthesis.
Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages:

  Printing numbers in parentheses (%d) adds no value and should be avoided.

This comment applies to other patches in the series as well.

> +                          ec->tx_max_coalesced_frames,
> +                          cdev->mcfg[MRAM_TXE].num);
> +               return -EINVAL;
> +       }
> +       if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXB].num) {
> +               netdev_err(dev, "tx-frames (%u) greater than the TX FIFO (%u)\n",
> +                          ec->tx_max_coalesced_frames,
> +                          cdev->mcfg[MRAM_TXB].num);
> +               return -EINVAL;
> +       }
>         if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
>             ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
>                 netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
> @@ -2047,6 +2092,7 @@ static int m_can_set_coalesce(struct net_device *dev,
>
>         cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
>         cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
> +       cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
>         cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
>         cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
>
> @@ -2064,6 +2110,7 @@ static const struct ethtool_ops m_can_ethtool_ops = {
>         .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
>                 ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
>                 ETHTOOL_COALESCE_TX_USECS_IRQ |
> +               ETHTOOL_COALESCE_TX_MAX_FRAMES |
>                 ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
>         .get_ts_info = ethtool_op_get_ts_info,
>         .get_coalesce = m_can_get_coalesce,
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index bfef2c89e239..e209de81b5a4 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -74,6 +74,7 @@ struct m_can_tx_op {
>         struct m_can_classdev *cdev;
>         struct work_struct work;
>         struct sk_buff *skb;
> +       bool submit;
>  };
>
>  struct m_can_classdev {
> @@ -103,6 +104,7 @@ struct m_can_classdev {
>         u32 active_interrupts;
>         u32 rx_max_coalesced_frames_irq;
>         u32 rx_coalesce_usecs_irq;
> +       u32 tx_max_coalesced_frames;
>         u32 tx_max_coalesced_frames_irq;
>         u32 tx_coalesce_usecs_irq;
>
> @@ -117,6 +119,10 @@ struct m_can_classdev {
>         int tx_fifo_size;
>         int next_tx_op;
>
> +       int nr_txs_without_submit;
> +       /* bitfield of fifo elements that will be submitted together */
> +       u32 tx_peripheral_submit;
> +
>         struct mram_cfg mcfg[MRAM_CFG_NUM];
>  };

Re: [PATCH v2 18/18] can: m_can: Implement transmit submission coalescing

[Markus Schneider-Pargmann]

Hi Vincent,

On Sun, Jan 29, 2023 at 12:00:48AM +0900, Vincent MAILHOL wrote:
> Hi Markus,
> 
> I did not have time for a deep review yet. But so far, I didn't find
> anything odd aside from two nitpicks.
> 
> On Thu. 26 Jan 2023 à 04:53, Markus Schneider-Pargmann
> <msp@baylibre.com> a écrit :
> >
> > m_can supports submitting mulitple transmits with one register write.
>                             ^^^^^^^^
> multiple
> 
> > This is an interesting option to reduce the number of SPI transfers for
> > peripheral chips.
> >
> > The m_can_tx_op is extended with a bool that signals if it is the last
> > transmission and the submit should be executed immediately.
> >
> > The worker then writes the skb to the FIFO and submits it only if the
> > submit bool is set. If it isn't set, the worker will write the next skb
> > which is waiting in the workqueue to the FIFO, etc.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >
> > Notes:
> >     Notes:
> >     - I ran into lost messages in the receive FIFO when using this
> >       implementation. I guess this only shows up with my test setup in
> >       loopback mode and maybe not enough CPU power.
> >     - I put this behind the tx-frames ethtool coalescing option as we do
> >       wait before submitting packages but it is something different than the
> >       tx-frames-irq option. I am not sure if this is the correct option,
> >       please let me know.
> >
> >  drivers/net/can/m_can/m_can.c | 55 ++++++++++++++++++++++++++++++++---
> >  drivers/net/can/m_can/m_can.h |  6 ++++
> >  2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index c6a09369d1aa..99bfcfec3775 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1504,6 +1504,9 @@ static int m_can_start(struct net_device *dev)
> >         if (ret)
> >                 return ret;
> >
> > +       netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
> > +                                      cdev->tx_max_coalesced_frames);
> > +
> >         cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> >
> >         m_can_enable_all_interrupts(cdev);
> > @@ -1813,8 +1816,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> >                  */
> >                 can_put_echo_skb(skb, dev, putidx, frame_len);
> >
> > -               /* Enable TX FIFO element to start transfer  */
> > -               m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
> > +               if (cdev->is_peripheral) {
> > +                       /* Delay enabling TX FIFO element */
> > +                       cdev->tx_peripheral_submit |= BIT(putidx);
> > +               } else {
> > +                       /* Enable TX FIFO element to start transfer  */
> > +                       m_can_write(cdev, M_CAN_TXBAR, BIT(putidx));
> > +               }
> >                 cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
> >                                         0 : cdev->tx_fifo_putidx);
> >         }
> > @@ -1827,6 +1835,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> >         return NETDEV_TX_BUSY;
> >  }
> >
> > +static void m_can_tx_submit(struct m_can_classdev *cdev)
> > +{
> > +       if (cdev->version == 30)
> > +               return;
> > +       if (!cdev->is_peripheral)
> > +               return;
> > +
> > +       m_can_write(cdev, M_CAN_TXBAR, cdev->tx_peripheral_submit);
> > +       cdev->tx_peripheral_submit = 0;
> > +}
> > +
> >  static void m_can_tx_work_queue(struct work_struct *ws)
> >  {
> >         struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
> > @@ -1835,11 +1854,15 @@ static void m_can_tx_work_queue(struct work_struct *ws)
> >
> >         op->skb = NULL;
> >         m_can_tx_handler(cdev, skb);
> > +       if (op->submit)
> > +               m_can_tx_submit(cdev);
> >  }
> >
> > -static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
> > +static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb,
> > +                              bool submit)
> >  {
> >         cdev->tx_ops[cdev->next_tx_op].skb = skb;
> > +       cdev->tx_ops[cdev->next_tx_op].submit = submit;
> >         queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
> >
> >         ++cdev->next_tx_op;
> > @@ -1851,6 +1874,7 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> >                                                struct sk_buff *skb)
> >  {
> >         netdev_tx_t err;
> > +       bool submit;
> >
> >         if (cdev->can.state == CAN_STATE_BUS_OFF) {
> >                 m_can_clean(cdev->net);
> > @@ -1861,7 +1885,15 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> >         if (err != NETDEV_TX_OK)
> >                 return err;
> >
> > -       m_can_tx_queue_skb(cdev, skb);
> > +       ++cdev->nr_txs_without_submit;
> > +       if (cdev->nr_txs_without_submit >= cdev->tx_max_coalesced_frames ||
> > +           !netdev_xmit_more()) {
> > +               cdev->nr_txs_without_submit = 0;
> > +               submit = true;
> > +       } else {
> > +               submit = false;
> > +       }
> > +       m_can_tx_queue_skb(cdev, skb, submit);
> >
> >         return NETDEV_TX_OK;
> >  }
> > @@ -1993,6 +2025,7 @@ static int m_can_get_coalesce(struct net_device *dev,
> >
> >         ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
> >         ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
> > +       ec->tx_max_coalesced_frames = cdev->tx_max_coalesced_frames;
> >         ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
> >         ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
> >
> > @@ -2037,6 +2070,18 @@ static int m_can_set_coalesce(struct net_device *dev,
> >                 netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
> >                 return -EINVAL;
> >         }
> > +       if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXE].num) {
> > +               netdev_err(dev, "tx-frames (%u) greater than the TX event FIFO (%u)\n",
>                                              ^^^^
>           ^^^^
> Avoid parenthesis.
> Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages:
> 
>   Printing numbers in parentheses (%d) adds no value and should be avoided.
> 
> This comment applies to other patches in the series as well.

Thank you for having a look and pointing this out, I wasn't aware of
that. I will fix these for the next version.

Best,
Markus

> 
> > +                          ec->tx_max_coalesced_frames,
> > +                          cdev->mcfg[MRAM_TXE].num);
> > +               return -EINVAL;
> > +       }
> > +       if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXB].num) {
> > +               netdev_err(dev, "tx-frames (%u) greater than the TX FIFO (%u)\n",
> > +                          ec->tx_max_coalesced_frames,
> > +                          cdev->mcfg[MRAM_TXB].num);
> > +               return -EINVAL;
> > +       }
> >         if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
> >             ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
> >                 netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
> > @@ -2047,6 +2092,7 @@ static int m_can_set_coalesce(struct net_device *dev,
> >
> >         cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
> >         cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
> > +       cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
> >         cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
> >         cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
> >
> > @@ -2064,6 +2110,7 @@ static const struct ethtool_ops m_can_ethtool_ops = {
> >         .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
> >                 ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
> >                 ETHTOOL_COALESCE_TX_USECS_IRQ |
> > +               ETHTOOL_COALESCE_TX_MAX_FRAMES |
> >                 ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
> >         .get_ts_info = ethtool_op_get_ts_info,
> >         .get_coalesce = m_can_get_coalesce,
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index bfef2c89e239..e209de81b5a4 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -74,6 +74,7 @@ struct m_can_tx_op {
> >         struct m_can_classdev *cdev;
> >         struct work_struct work;
> >         struct sk_buff *skb;
> > +       bool submit;
> >  };
> >
> >  struct m_can_classdev {
> > @@ -103,6 +104,7 @@ struct m_can_classdev {
> >         u32 active_interrupts;
> >         u32 rx_max_coalesced_frames_irq;
> >         u32 rx_coalesce_usecs_irq;
> > +       u32 tx_max_coalesced_frames;
> >         u32 tx_max_coalesced_frames_irq;
> >         u32 tx_coalesce_usecs_irq;
> >
> > @@ -117,6 +119,10 @@ struct m_can_classdev {
> >         int tx_fifo_size;
> >         int next_tx_op;
> >
> > +       int nr_txs_without_submit;
> > +       /* bitfield of fifo elements that will be submitted together */
> > +       u32 tx_peripheral_submit;
> > +
> >         struct mram_cfg mcfg[MRAM_CFG_NUM];
> >  };