[PATCH 00/15] can: m_can: Optimizations for tcan and peripheral chips

[PATCH 00/15] can: m_can: Optimizations for tcan and peripheral chips

[Markus Schneider-Pargmann]

Hi,

this series is aimed at optimizing the driver code for tcan chips and
more generally for peripheral m_can chips.

I did different things to improve the performance:
- Reduce the number of SPI transfers.
- Reduce the number of interrupts.
- Enable use of FIFOs.

I am working with a tcan4550 in loopback mode attached to a beaglebone
black. I am currently working on optimizing the receive path as well
which will be submitted in another series once it is done.

Best,
Markus

Markus Schneider-Pargmann (15):
  can: m_can: Eliminate double read of TXFQS in tx_handler
  can: m_can: Wakeup net queue once tx was issued
  can: m_can: Cache tx putidx and transmits in flight
  can: m_can: Use transmit event FIFO watermark level interrupt
  can: m_can: Disable unused interrupts
  can: m_can: Avoid reading irqstatus twice
  can: m_can: Read register PSR only on error
  can: m_can: Count TXE FIFO getidx in the driver
  can: m_can: Count read getindex in the driver
  can: m_can: Batch acknowledge rx fifo
  can: m_can: Batch acknowledge transmit events
  can: tcan4x5x: Remove invalid write in clear_interrupts
  can: tcan4x5x: Fix use of register error status mask
  can: tcan4x5x: Fix register range of first block
  can: tcan4x5x: Specify separate read/write ranges

 drivers/net/can/m_can/m_can.c           | 140 +++++++++++++++---------
 drivers/net/can/m_can/m_can.h           |   5 +
 drivers/net/can/m_can/tcan4x5x-core.c   |  19 ++--
 drivers/net/can/m_can/tcan4x5x-regmap.c |  45 ++++++--
 4 files changed, 141 insertions(+), 68 deletions(-)


base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
prerequisite-patch-id: e9df6751d43bb0d1e3b8938d7e93bc1cfa22cef2
prerequisite-patch-id: dad9ec37af766bcafe54cb156f896267a0f47fe1
prerequisite-patch-id: f4e6f1a213a31df2741a5fa3baa87aa45ef6707a
-- 
2.38.1

[PATCH 01/15] can: m_can: Eliminate double read of TXFQS in tx_handler

[Markus Schneider-Pargmann]

The TXFQS register is read first to check if the fifo is full and then
immediately again to get the putidx. This is unnecessary and adds
significant overhead if read requests are done over a slow bus, for
example SPI with tcan4x5x.

Add a variable to store the value of the register. Split the
m_can_tx_fifo_full function into two to avoid the hidden m_can_read call
if not needed.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 00d11e95fd98..2c01e3f7b23f 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -368,9 +368,14 @@ 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_read(cdev, M_CAN_TXFQS) & TXFQS_TFQF);
+	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)
@@ -1585,6 +1590,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct sk_buff *skb = cdev->tx_skb;
 	struct id_and_dlc fifo_header;
 	u32 cccr, fdflags;
+	u32 txfqs;
 	int err;
 	int putidx;
 
@@ -1641,8 +1647,10 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	} else {
 		/* 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(cdev)) {
+		if (_m_can_tx_fifo_full(txfqs)) {
 			/* This shouldn't happen */
 			netif_stop_queue(dev);
 			netdev_warn(dev,
@@ -1658,8 +1666,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,
-				   m_can_read(cdev, M_CAN_TXFQS));
+		putidx = FIELD_GET(TXFQS_TFQPI_MASK, txfqs);
 
 		/* Construct DLC Field, with CAN-FD configuration.
 		 * Use the put index of the fifo as the message marker,
-- 
2.38.1

[PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Currently the driver waits to wakeup the queue until the interrupt for
the transmit event is received and acknowledged. If we want to use the
hardware FIFO, this is too late.

Instead release the queue as soon as the transmit was transferred into
the hardware FIFO. We are then ready for the next transmit to be
transferred.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2c01e3f7b23f..4adf03111782 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1097,10 +1097,9 @@ 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))
+			if (!cdev->tx_skb && netif_queue_stopped(dev))
 				netif_wake_queue(dev);
+
 		}
 	}
 
@@ -1705,6 +1704,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		if (m_can_tx_fifo_full(cdev) ||
 		    m_can_next_echo_skb_occupied(dev, putidx))
 			netif_stop_queue(dev);
+		else if (cdev->is_peripheral && !cdev->tx_skb && netif_queue_stopped(dev))
+			netif_wake_queue(dev);
 	}
 
 	return NETDEV_TX_OK;
-- 
2.38.1

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> Currently the driver waits to wakeup the queue until the interrupt for
> the transmit event is received and acknowledged. If we want to use the
> hardware FIFO, this is too late.
> 
> Instead release the queue as soon as the transmit was transferred into
> the hardware FIFO. We are then ready for the next transmit to be
> transferred.

If you want to really speed up the TX path, remove the worker and use
the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().

Extra bonus if you implement xmit_more() and transfer more than 1 skb
per SPI transfer.

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 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Hi Marc,

On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > Currently the driver waits to wakeup the queue until the interrupt for
> > the transmit event is received and acknowledged. If we want to use the
> > hardware FIFO, this is too late.
> > 
> > Instead release the queue as soon as the transmit was transferred into
> > the hardware FIFO. We are then ready for the next transmit to be
> > transferred.
> 
> If you want to really speed up the TX path, remove the worker and use
> the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().

Good idea. I will check how regmap's async_write works and if it is
suitable to do the job. I don't want to drop the regmap usage for this
right now.

> 
> Extra bonus if you implement xmit_more() and transfer more than 1 skb
> per SPI transfer.

That's on my todo list, but I am not sure I will get to it soonish.

Thank you Marc!.

Best,
Markus

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 01.12.2022 09:43:02, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > Currently the driver waits to wakeup the queue until the interrupt for
> > > the transmit event is received and acknowledged. If we want to use the
> > > hardware FIFO, this is too late.
> > > 
> > > Instead release the queue as soon as the transmit was transferred into
> > > the hardware FIFO. We are then ready for the next transmit to be
> > > transferred.
> > 
> > If you want to really speed up the TX path, remove the worker and use
> > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> 
> Good idea. I will check how regmap's async_write works and if it is
> suitable to do the job. I don't want to drop the regmap usage for this
> right now.

IIRC regmap async write still uses mutexes, but sleeping is not allowed
in the xmit handler. The mcp251xfd driver does the endianness conversion
(and the optional CRC) manually for the TX path.

Sending directly from the xmit handler basically eliminates the queuing
between the network stack and the worker. Getting rid of the worker
makes life easier and it's faster anyways.

> > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > per SPI transfer.
> 
> That's on my todo list, but I am not sure I will get to it soonish.

I haven't implemented this for the mcp251xfd, yet, but I have some
proof-of-concept code somewhere. However, the mcp251xfd driver already
implemented byte queue limits: 0084e298acfe ("can: mcp251xfd: add BQL
support").

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 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Hi Marc,

On Thu, Dec 01, 2022 at 10:16:05AM +0100, Marc Kleine-Budde wrote:
> On 01.12.2022 09:43:02, Markus Schneider-Pargmann wrote:
> > Hi Marc,
> > 
> > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > the transmit event is received and acknowledged. If we want to use the
> > > > hardware FIFO, this is too late.
> > > > 
> > > > Instead release the queue as soon as the transmit was transferred into
> > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > transferred.
> > > 
> > > If you want to really speed up the TX path, remove the worker and use
> > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > 
> > Good idea. I will check how regmap's async_write works and if it is
> > suitable to do the job. I don't want to drop the regmap usage for this
> > right now.
> 
> IIRC regmap async write still uses mutexes, but sleeping is not allowed
> in the xmit handler. The mcp251xfd driver does the endianness conversion
> (and the optional CRC) manually for the TX path.

I just saw, you can force regmap to use spinlocks as well. But it uses
the same operation for sync operations as well.

> 
> Sending directly from the xmit handler basically eliminates the queuing
> between the network stack and the worker. Getting rid of the worker
> makes life easier and it's faster anyways.

The current implementation of the driver doesn't really queue anything
between the network stack and the worker. It is a queue of size 1 ;).

To be honest I would rather focus on the other things than on getting
rid of the worker completely as this can be done in a separate patch
later as well. Yes I agree it would be nice to get rid of the worker but
it is also probably not a major bottleneck for the performance and in
its current state it works. If I have time left at the end I will be
more than happy to do that. But for the moment I would just keep the
worker as it is. Is that OK for you?

Thanks,
Markus

> 
> > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > per SPI transfer.
> > 
> > That's on my todo list, but I am not sure I will get to it soonish.
> 
> I haven't implemented this for the mcp251xfd, yet, but I have some
> proof-of-concept code somewhere. However, the mcp251xfd driver already
> implemented byte queue limits: 0084e298acfe ("can: mcp251xfd: add BQL
> support").
> 
> 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 |

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 01.12.2022 17:49:02, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Thu, Dec 01, 2022 at 10:16:05AM +0100, Marc Kleine-Budde wrote:
> > On 01.12.2022 09:43:02, Markus Schneider-Pargmann wrote:
> > > Hi Marc,
> > > 
> > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > hardware FIFO, this is too late.
> > > > > 
> > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > transferred.
> > > > 
> > > > If you want to really speed up the TX path, remove the worker and use
> > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > 
> > > Good idea. I will check how regmap's async_write works and if it is
> > > suitable to do the job. I don't want to drop the regmap usage for this
> > > right now.
> > 
> > IIRC regmap async write still uses mutexes, but sleeping is not allowed
> > in the xmit handler. The mcp251xfd driver does the endianness conversion
> > (and the optional CRC) manually for the TX path.
> 
> I just saw, you can force regmap to use spinlocks as well. But it uses
> the same operation for sync operations as well.

But you cannot use sync SPI api under a spinlock.

> > Sending directly from the xmit handler basically eliminates the queuing
> > between the network stack and the worker. Getting rid of the worker
> > makes life easier and it's faster anyways.
> 
> The current implementation of the driver doesn't really queue anything
> between the network stack and the worker. It is a queue of size 1 ;).

Ok

> To be honest I would rather focus on the other things than on getting
> rid of the worker completely as this can be done in a separate patch
> later as well. Yes I agree it would be nice to get rid of the worker but
> it is also probably not a major bottleneck for the performance and in
> its current state it works. If I have time left at the end I will be
> more than happy to do that. But for the moment I would just keep the
> worker as it is. Is that OK for you?

Sure.

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 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Hi Marc,

On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > Currently the driver waits to wakeup the queue until the interrupt for
> > the transmit event is received and acknowledged. If we want to use the
> > hardware FIFO, this is too late.
> > 
> > Instead release the queue as soon as the transmit was transferred into
> > the hardware FIFO. We are then ready for the next transmit to be
> > transferred.
> 
> If you want to really speed up the TX path, remove the worker and use
> the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> 
> Extra bonus if you implement xmit_more() and transfer more than 1 skb
> per SPI transfer.

Just a quick question here, I mplemented a xmit_more() call and I am
testing it right now, but it always returns false even under high
pressure. The device has a txqueuelen set to 1000. Do I need to turn
some other knob for this to work?

Thanks,
Markus

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > Currently the driver waits to wakeup the queue until the interrupt for
> > > the transmit event is received and acknowledged. If we want to use the
> > > hardware FIFO, this is too late.
> > > 
> > > Instead release the queue as soon as the transmit was transferred into
> > > the hardware FIFO. We are then ready for the next transmit to be
> > > transferred.
> > 
> > If you want to really speed up the TX path, remove the worker and use
> > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > 
> > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > per SPI transfer.
> 
> Just a quick question here, I mplemented a xmit_more() call and I am
> testing it right now, but it always returns false even under high
> pressure. The device has a txqueuelen set to 1000. Do I need to turn
> some other knob for this to work?

AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").

The etas_es58x driver implements xmit_more(), I added the Author Vincent
on Cc.

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 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > Hi Marc,
> > 
> > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > the transmit event is received and acknowledged. If we want to use the
> > > > hardware FIFO, this is too late.
> > > > 
> > > > Instead release the queue as soon as the transmit was transferred into
> > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > transferred.
> > > 
> > > If you want to really speed up the TX path, remove the worker and use
> > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > 
> > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > per SPI transfer.
> > 
> > Just a quick question here, I mplemented a xmit_more() call and I am
> > testing it right now, but it always returns false even under high
> > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > some other knob for this to work?
> 
> AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> 
> The etas_es58x driver implements xmit_more(), I added the Author Vincent
> on Cc.

Have a look at netdev_queue_set_dql_min_limit() in the etas driver.

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 02/15] can: m_can: Wakeup net queue once tx was issued

[Vincent MAILHOL]

On Wed. 14 Dec. 2022 at 18:28, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> > On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > > Hi Marc,
> > >
> > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > hardware FIFO, this is too late.
> > > > >
> > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > transferred.
> > > >
> > > > If you want to really speed up the TX path, remove the worker and use
> > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > >
> > > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > > per SPI transfer.
> > >
> > > Just a quick question here, I mplemented a xmit_more() call and I am
> > > testing it right now, but it always returns false even under high
> > > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > > some other knob for this to work?

I was the first to use BQL in a CAN driver. It also took me time to
first figure out the existence of xmit_more() and even more to
understand how to make it so that it would return true.

> > AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> >
> > The etas_es58x driver implements xmit_more(), I added the Author Vincent
> > on Cc.
>
> Have a look at netdev_queue_set_dql_min_limit() in the etas driver.

The functions you need are the netdev_send_queue() and the
netdev_complete_queue():

  https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3424

For CAN, you probably want to have a look to can_skb_get_frame_len().

  https://elixir.bootlin.com/linux/latest/source/include/linux/can/length.h#L166

The netdev_queue_set_dql_min_limit() gives hints by setting a minimum
value for BQL. It is optional (and as of today I am the only user of
it).


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Hi Vincent,

On Wed, Dec 14, 2022 at 07:15:25PM +0900, Vincent MAILHOL wrote:
> On Wed. 14 Dec. 2022 at 18:28, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> > > On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > > > Hi Marc,
> > > >
> > > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > > hardware FIFO, this is too late.
> > > > > >
> > > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > > transferred.
> > > > >
> > > > > If you want to really speed up the TX path, remove the worker and use
> > > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > > >
> > > > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > > > per SPI transfer.
> > > >
> > > > Just a quick question here, I mplemented a xmit_more() call and I am
> > > > testing it right now, but it always returns false even under high
> > > > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > > > some other knob for this to work?
> 
> I was the first to use BQL in a CAN driver. It also took me time to
> first figure out the existence of xmit_more() and even more to
> understand how to make it so that it would return true.
> 
> > > AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> > >
> > > The etas_es58x driver implements xmit_more(), I added the Author Vincent
> > > on Cc.
> >
> > Have a look at netdev_queue_set_dql_min_limit() in the etas driver.
> 
> The functions you need are the netdev_send_queue() and the
> netdev_complete_queue():
> 
>   https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3424
> 
> For CAN, you probably want to have a look to can_skb_get_frame_len().
> 
>   https://elixir.bootlin.com/linux/latest/source/include/linux/can/length.h#L166
> 
> The netdev_queue_set_dql_min_limit() gives hints by setting a minimum
> value for BQL. It is optional (and as of today I am the only user of
> it).

Thank you for this summary, great that you already invested the time to
make it work with a CAN driver. I will give it a try in the m_can
driver.

Best,
Markus

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

Hi,

On Wed, Dec 14, 2022 at 11:35:43AM +0100, Markus Schneider-Pargmann wrote:
> Hi Vincent,
> 
> On Wed, Dec 14, 2022 at 07:15:25PM +0900, Vincent MAILHOL wrote:
> > On Wed. 14 Dec. 2022 at 18:28, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> > > > On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > > > > Hi Marc,
> > > > >
> > > > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > > > hardware FIFO, this is too late.
> > > > > > >
> > > > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > > > transferred.
> > > > > >
> > > > > > If you want to really speed up the TX path, remove the worker and use
> > > > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > > > >
> > > > > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > > > > per SPI transfer.
> > > > >
> > > > > Just a quick question here, I mplemented a xmit_more() call and I am
> > > > > testing it right now, but it always returns false even under high
> > > > > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > > > > some other knob for this to work?
> > 
> > I was the first to use BQL in a CAN driver. It also took me time to
> > first figure out the existence of xmit_more() and even more to
> > understand how to make it so that it would return true.
> > 
> > > > AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> > > >
> > > > The etas_es58x driver implements xmit_more(), I added the Author Vincent
> > > > on Cc.
> > >
> > > Have a look at netdev_queue_set_dql_min_limit() in the etas driver.
> > 
> > The functions you need are the netdev_send_queue() and the
> > netdev_complete_queue():
> > 
> >   https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3424
> > 
> > For CAN, you probably want to have a look to can_skb_get_frame_len().
> > 
> >   https://elixir.bootlin.com/linux/latest/source/include/linux/can/length.h#L166
> > 
> > The netdev_queue_set_dql_min_limit() gives hints by setting a minimum
> > value for BQL. It is optional (and as of today I am the only user of
> > it).
> 
> Thank you for this summary, great that you already invested the time to
> make it work with a CAN driver. I will give it a try in the m_can
> driver.

Thanks again, it looks like it is working after adding netdev_sent_queue
and netdev_complete_queue.

Best,
Markus

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Vincent MAILHOL]

On Thu. 15 Dec. 2022 at 18:44, Markus Schneider-Pargmann
<msp@baylibre.com> wrote:
> Hi,
>
> On Wed, Dec 14, 2022 at 11:35:43AM +0100, Markus Schneider-Pargmann wrote:
> > Hi Vincent,
> >
> > On Wed, Dec 14, 2022 at 07:15:25PM +0900, Vincent MAILHOL wrote:
> > > On Wed. 14 Dec. 2022 at 18:28, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > > On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> > > > > On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > > > > > Hi Marc,
> > > > > >
> > > > > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > > > > hardware FIFO, this is too late.
> > > > > > > >
> > > > > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > > > > transferred.
> > > > > > >
> > > > > > > If you want to really speed up the TX path, remove the worker and use
> > > > > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > > > > >
> > > > > > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > > > > > per SPI transfer.
> > > > > >
> > > > > > Just a quick question here, I mplemented a xmit_more() call and I am
> > > > > > testing it right now, but it always returns false even under high
> > > > > > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > > > > > some other knob for this to work?
> > >
> > > I was the first to use BQL in a CAN driver. It also took me time to
> > > first figure out the existence of xmit_more() and even more to
> > > understand how to make it so that it would return true.
> > >
> > > > > AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> > > > >
> > > > > The etas_es58x driver implements xmit_more(), I added the Author Vincent
> > > > > on Cc.
> > > >
> > > > Have a look at netdev_queue_set_dql_min_limit() in the etas driver.
> > >
> > > The functions you need are the netdev_send_queue() and the
> > > netdev_complete_queue():
> > >
> > >   https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3424
> > >
> > > For CAN, you probably want to have a look to can_skb_get_frame_len().
> > >
> > >   https://elixir.bootlin.com/linux/latest/source/include/linux/can/length.h#L166
> > >
> > > The netdev_queue_set_dql_min_limit() gives hints by setting a minimum
> > > value for BQL. It is optional (and as of today I am the only user of
> > > it).
> >
> > Thank you for this summary, great that you already invested the time to
> > make it work with a CAN driver. I will give it a try in the m_can
> > driver.
>
> Thanks again, it looks like it is working after adding netdev_sent_queue
> and netdev_complete_queue.

Happy to hear that :)

A quick advice just in case: make sure to test the different error
branches (failed TX, can_restart() after bus off…).


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Markus Schneider-Pargmann]

On Wed, Dec 14, 2022 at 10:22:01AM +0100, Marc Kleine-Budde wrote:
> On 14.12.2022 10:18:20, Marc Kleine-Budde wrote:
> > On 14.12.2022 10:14:06, Markus Schneider-Pargmann wrote:
> > > Hi Marc,
> > > 
> > > On Wed, Nov 30, 2022 at 06:21:00PM +0100, Marc Kleine-Budde wrote:
> > > > On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> > > > > Currently the driver waits to wakeup the queue until the interrupt for
> > > > > the transmit event is received and acknowledged. If we want to use the
> > > > > hardware FIFO, this is too late.
> > > > > 
> > > > > Instead release the queue as soon as the transmit was transferred into
> > > > > the hardware FIFO. We are then ready for the next transmit to be
> > > > > transferred.
> > > > 
> > > > If you want to really speed up the TX path, remove the worker and use
> > > > the spi_async() API from the xmit callback, see mcp251xfd_start_xmit().
> > > > 
> > > > Extra bonus if you implement xmit_more() and transfer more than 1 skb
> > > > per SPI transfer.
> > > 
> > > Just a quick question here, I mplemented a xmit_more() call and I am
> > > testing it right now, but it always returns false even under high
> > > pressure. The device has a txqueuelen set to 1000. Do I need to turn
> > > some other knob for this to work?
> > 
> > AFAIK you need BQL support: see 0084e298acfe ("can: mcp251xfd: add BQL support").
> > 
> > The etas_es58x driver implements xmit_more(), I added the Author Vincent
> > on Cc.
> 
> Have a look at netdev_queue_set_dql_min_limit() in the etas driver.

Thank you, I need to implement BQL, I wasn't aware of that. I may need
to delay this a bit just to get some feedback on the current state of my
patches, so I know what I need to work on first.

But it seems to be just accounting... Anyways, thank you :)

Best,
Markus

Re: [PATCH 02/15] can: m_can: Wakeup net queue once tx was issued

[Marc Kleine-Budde]

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

On 16.11.2022 21:52:55, Markus Schneider-Pargmann wrote:
> Currently the driver waits to wakeup the queue until the interrupt for
> the transmit event is received and acknowledged. If we want to use the
> hardware FIFO, this is too late.
> 
> Instead release the queue as soon as the transmit was transferred into
> the hardware FIFO. We are then ready for the next transmit to be
> transferred.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/net/can/m_can/m_can.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 2c01e3f7b23f..4adf03111782 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1097,10 +1097,9 @@ 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))
> +			if (!cdev->tx_skb && netif_queue_stopped(dev))
>  				netif_wake_queue(dev);

Please don't start the queue if the FIFO is still full. Is this a
gamble, that it will take long enough until the work queue runs that the
FIFO is not full anymore?

> +

Nitpick: Please don't introduce an extra newline here.

>  		}
>  	}
>  
> @@ -1705,6 +1704,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		if (m_can_tx_fifo_full(cdev) ||
>  		    m_can_next_echo_skb_occupied(dev, putidx))
>  			netif_stop_queue(dev);
> +		else if (cdev->is_peripheral && !cdev->tx_skb && netif_queue_stopped(dev))
> +			netif_wake_queue(dev);

Same question as above, what happens if the FIFO is full? e.g. in case
of a slow bus or the first CAN frame in the FIFO has a low prio...

>  	}
>  
>  	return NETDEV_TX_OK;
> -- 
> 2.38.1
> 
>

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

[PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

[Markus Schneider-Pargmann]

On peripheral chips every read/write can be costly. Avoid reading easily
trackable information and cache them internally. This saves multiple
reads.

Transmit FIFO put index is cached, this is increased for every time we
enqueue a transmit request.

The transmits in flight is cached as well. With each transmit request it
is increased when reading the finished transmit event it is decreased.

A submit limit is cached to avoid submitting too many transmits at once,
either because the TX FIFO or the TXE FIFO is limited. This is currently
done very conservatively as the minimum of the fifo sizes. This means we
can reach FIFO full events but won't drop anything.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 4adf03111782..f5bba848bd56 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1041,6 +1041,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 		/* ack txe element */
 		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
 							  fgi));
+		--cdev->tx_fifo_in_flight;
 
 		/* update stats */
 		m_can_tx_update_stats(cdev, msg_mark, timestamp);
@@ -1376,6 +1377,14 @@ static void m_can_start(struct net_device *dev)
 	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	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));
+		cdev->tx_fifo_in_flight = 0;
+		cdev->tx_fifo_submit_limit = min(cdev->mcfg[MRAM_TXE].num,
+						 cdev->mcfg[MRAM_TXB].num);
+	}
 }
 
 static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
@@ -1589,7 +1598,6 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct sk_buff *skb = cdev->tx_skb;
 	struct id_and_dlc fifo_header;
 	u32 cccr, fdflags;
-	u32 txfqs;
 	int err;
 	int putidx;
 
@@ -1646,10 +1654,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	} else {
 		/* 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)) {
+		if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_submit_limit) {
 			/* This shouldn't happen */
 			netif_stop_queue(dev);
 			netdev_warn(dev,
@@ -1665,7 +1671,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,
@@ -1699,9 +1705,12 @@ 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_in_flight;
+		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) ||
+		if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_submit_limit ||
 		    m_can_next_echo_skb_occupied(dev, putidx))
 			netif_stop_queue(dev);
 		else if (cdev->is_peripheral && !cdev->tx_skb && netif_queue_stopped(dev))
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 4c0267f9f297..7464ce56753a 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -92,6 +92,11 @@ struct m_can_classdev {
 	int pm_clock_support;
 	int is_peripheral;
 
+	// Store this internally to avoid fetch delays on peripheral chips
+	int tx_fifo_putidx;
+	int tx_fifo_in_flight;
+	int tx_fifo_submit_limit;
+
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.38.1

Re: [PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

[Marc Kleine-Budde]

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

On 16.11.2022 21:52:56, Markus Schneider-Pargmann wrote:
> On peripheral chips every read/write can be costly. Avoid reading easily
> trackable information and cache them internally. This saves multiple
> reads.
> 
> Transmit FIFO put index is cached, this is increased for every time we
> enqueue a transmit request.
> 
> The transmits in flight is cached as well. With each transmit request it
> is increased when reading the finished transmit event it is decreased.
> 
> A submit limit is cached to avoid submitting too many transmits at once,
> either because the TX FIFO or the TXE FIFO is limited. This is currently
> done very conservatively as the minimum of the fifo sizes. This means we
> can reach FIFO full events but won't drop anything.

You have a dedicated in_flight variable, which is read-modify-write in 2
different code path, i.e. this looks racy.

If you allow only power-of-two FIFO size, you can use 2 unsigned
variables, i.e. a head and a tail pointer. You can apply a mask to get
the index to the FIFO. The difference between head and tail is the fill
level of the FIFO. See mcp251xfd driver for this.

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 03/15] can: m_can: Cache tx putidx and transmits in flight

[Markus Schneider-Pargmann]

Hi Marc,

On Thu, Dec 01, 2022 at 12:14:50PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:52:56, Markus Schneider-Pargmann wrote:
> > On peripheral chips every read/write can be costly. Avoid reading easily
> > trackable information and cache them internally. This saves multiple
> > reads.
> > 
> > Transmit FIFO put index is cached, this is increased for every time we
> > enqueue a transmit request.
> > 
> > The transmits in flight is cached as well. With each transmit request it
> > is increased when reading the finished transmit event it is decreased.
> > 
> > A submit limit is cached to avoid submitting too many transmits at once,
> > either because the TX FIFO or the TXE FIFO is limited. This is currently
> > done very conservatively as the minimum of the fifo sizes. This means we
> > can reach FIFO full events but won't drop anything.
> 
> You have a dedicated in_flight variable, which is read-modify-write in 2
> different code path, i.e. this looks racy.

True, of course, thank you. Yes I have to redesign this a bit for
concurrency.

> If you allow only power-of-two FIFO size, you can use 2 unsigned
> variables, i.e. a head and a tail pointer. You can apply a mask to get
> the index to the FIFO. The difference between head and tail is the fill
> level of the FIFO. See mcp251xfd driver for this.

Maybe that is a trivial question but what's wrong with using atomics
instead?

The tcan mram size is limited to 2048 so I would like to avoid limiting
the possible sizes of the tx fifos.

Best,
Markus

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

Re: [PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

[Marc Kleine-Budde]

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

On 02.12.2022 09:37:40, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Thu, Dec 01, 2022 at 12:14:50PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:52:56, Markus Schneider-Pargmann wrote:
> > > On peripheral chips every read/write can be costly. Avoid reading easily
> > > trackable information and cache them internally. This saves multiple
> > > reads.
> > > 
> > > Transmit FIFO put index is cached, this is increased for every time we
> > > enqueue a transmit request.
> > > 
> > > The transmits in flight is cached as well. With each transmit request it
> > > is increased when reading the finished transmit event it is decreased.
> > > 
> > > A submit limit is cached to avoid submitting too many transmits at once,
> > > either because the TX FIFO or the TXE FIFO is limited. This is currently
> > > done very conservatively as the minimum of the fifo sizes. This means we
> > > can reach FIFO full events but won't drop anything.
> > 
> > You have a dedicated in_flight variable, which is read-modify-write in 2
> > different code path, i.e. this looks racy.
> 
> True, of course, thank you. Yes I have to redesign this a bit for
> concurrency.
> 
> > If you allow only power-of-two FIFO size, you can use 2 unsigned
> > variables, i.e. a head and a tail pointer. You can apply a mask to get
> > the index to the FIFO. The difference between head and tail is the fill
> > level of the FIFO. See mcp251xfd driver for this.
> 
> Maybe that is a trivial question but what's wrong with using atomics
> instead?

I think it's Ok to use an atomic for the fill level. The put index
doesn't need to be. No need to cache the get index, as it's in the same
register as the fill level.

As the mcp251xfd benefits from caching both indexes, a head and tail
pointer felt like the right choice. As both are only written in 1
location, no need for atomics or locking.

> The tcan mram size is limited to 2048 so I would like to avoid limiting
> the possible sizes of the tx fifos.

What FIFO sizes are you using currently?

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 03/15] can: m_can: Cache tx putidx and transmits in flight

[Markus Schneider-Pargmann]

On Fri, Dec 02, 2022 at 03:46:30PM +0100, Marc Kleine-Budde wrote:
> On 02.12.2022 09:37:40, Markus Schneider-Pargmann wrote:
> > Hi Marc,
> > 
> > On Thu, Dec 01, 2022 at 12:14:50PM +0100, Marc Kleine-Budde wrote:
> > > On 16.11.2022 21:52:56, Markus Schneider-Pargmann wrote:
> > > > On peripheral chips every read/write can be costly. Avoid reading easily
> > > > trackable information and cache them internally. This saves multiple
> > > > reads.
> > > > 
> > > > Transmit FIFO put index is cached, this is increased for every time we
> > > > enqueue a transmit request.
> > > > 
> > > > The transmits in flight is cached as well. With each transmit request it
> > > > is increased when reading the finished transmit event it is decreased.
> > > > 
> > > > A submit limit is cached to avoid submitting too many transmits at once,
> > > > either because the TX FIFO or the TXE FIFO is limited. This is currently
> > > > done very conservatively as the minimum of the fifo sizes. This means we
> > > > can reach FIFO full events but won't drop anything.
> > > 
> > > You have a dedicated in_flight variable, which is read-modify-write in 2
> > > different code path, i.e. this looks racy.
> > 
> > True, of course, thank you. Yes I have to redesign this a bit for
> > concurrency.
> > 
> > > If you allow only power-of-two FIFO size, you can use 2 unsigned
> > > variables, i.e. a head and a tail pointer. You can apply a mask to get
> > > the index to the FIFO. The difference between head and tail is the fill
> > > level of the FIFO. See mcp251xfd driver for this.
> > 
> > Maybe that is a trivial question but what's wrong with using atomics
> > instead?
> 
> I think it's Ok to use an atomic for the fill level. The put index
> doesn't need to be. No need to cache the get index, as it's in the same
> register as the fill level.
> 
> As the mcp251xfd benefits from caching both indexes, a head and tail
> pointer felt like the right choice. As both are only written in 1
> location, no need for atomics or locking.
> 
> > The tcan mram size is limited to 2048 so I would like to avoid limiting
> > the possible sizes of the tx fifos.
> 
> What FIFO sizes are you using currently?

I am currently using 13 for TXB, TXE and RXF0.

Best,
Markus

Re: [PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

[Marc Kleine-Budde]

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

On 13.12.2022 18:13:09, Markus Schneider-Pargmann wrote:
> > > The tcan mram size is limited to 2048 so I would like to avoid limiting
> > > the possible sizes of the tx fifos.
> > 
> > What FIFO sizes are you using currently?
> 
> I am currently using 13 for TXB, TXE and RXF0.

Have you CAN-FD enabled?

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 03/15] can: m_can: Cache tx putidx and transmits in flight

[Markus Schneider-Pargmann]

Hi Marc,

On Tue, Dec 13, 2022 at 08:17:17PM +0100, Marc Kleine-Budde wrote:
> On 13.12.2022 18:13:09, Markus Schneider-Pargmann wrote:
> > > > The tcan mram size is limited to 2048 so I would like to avoid limiting
> > > > the possible sizes of the tx fifos.
> > > 
> > > What FIFO sizes are you using currently?
> > 
> > I am currently using 13 for TXB, TXE and RXF0.
> 
> Have you CAN-FD enabled?

yes, it is enabled.

Best,
Markus

[PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

Currently the only mode of operation is an interrupt for every transmit
event. This is inefficient for peripheral chips. Use the transmit FIFO
event watermark interrupt instead if the FIFO size is more than 2. Use
FIFOsize - 1 for the watermark so the interrupt is triggered early
enough to not stop transmitting.

Note that if the number of transmits is less than the watermark level,
the transmit events will not be processed until there is any other
interrupt. This will only affect statistic counters. Also there is an
interrupt every time the timestamp wraps around.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f5bba848bd56..4a6972c8bacd 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -254,6 +254,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) */
@@ -1094,8 +1095,8 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 			netif_wake_queue(dev);
 		}
 	} else  {
-		if (ir & IR_TEFN) {
-			/* New TX FIFO Element arrived */
+		if (ir & (IR_TEFN | IR_TEFW)) {
+			/* New TX FIFO Element arrived or watermark reached */
 			if (m_can_echo_tx_event(dev) != 0)
 				goto out_fail;
 			if (!cdev->tx_skb && netif_queue_stopped(dev))
@@ -1242,6 +1243,7 @@ static void m_can_chip_config(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 cccr, test;
+	u32 interrupts = IR_ALL_INT;
 
 	m_can_config_endisable(cdev, true);
 
@@ -1276,11 +1278,20 @@ static void m_can_chip_config(struct net_device *dev)
 			    FIELD_PREP(TXEFC_EFS_MASK, 1) |
 			    cdev->mcfg[MRAM_TXE].off);
 	} else {
+		u32 txe_watermark;
+
+		txe_watermark = cdev->mcfg[MRAM_TXE].num - 1;
 		/* Full TX Event FIFO is used */
 		m_can_write(cdev, M_CAN_TXEFC,
+			    FIELD_PREP(TXEFC_EFWM_MASK,
+				       txe_watermark) |
 			    FIELD_PREP(TXEFC_EFS_MASK,
 				       cdev->mcfg[MRAM_TXE].num) |
 			    cdev->mcfg[MRAM_TXE].off);
+
+		/* Watermark interrupt mode */
+		if (txe_watermark)
+			interrupts &= ~IR_TEFN;
 	}
 
 	/* rx fifo configuration, blocking mode, fifo size 1 */
@@ -1338,15 +1349,13 @@ static void m_can_chip_config(struct net_device *dev)
 
 	/* Enable interrupts */
 	m_can_write(cdev, M_CAN_IR, IR_ALL_INT);
-	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.38.1

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 16.11.2022 21:52:57, Markus Schneider-Pargmann wrote:
> Currently the only mode of operation is an interrupt for every transmit
> event. This is inefficient for peripheral chips. Use the transmit FIFO
> event watermark interrupt instead if the FIFO size is more than 2. Use
> FIFOsize - 1 for the watermark so the interrupt is triggered early
> enough to not stop transmitting.
> 
> Note that if the number of transmits is less than the watermark level,
> the transmit events will not be processed until there is any other
> interrupt. This will only affect statistic counters. Also there is an
> interrupt every time the timestamp wraps around.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

Please make this configurable with the ethtool TX IRQ coalescing
parameter. Please setup an hwtimer to enable the regular interrupt after
some configurable time to avoid starving of the TX complete events.

I've implemented this for the mcp251xfd driver, see:

656fc12ddaf8 ("can: mcp251xfd: add TX IRQ coalescing ethtool support")
169d00a25658 ("can: mcp251xfd: add TX IRQ coalescing support")
846990e0ed82 ("can: mcp251xfd: add RX IRQ coalescing ethtool support")
60a848c50d2d ("can: mcp251xfd: add RX IRQ coalescing support")
9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")

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 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

Hi Marc,

Thanks for reviewing.

On Wed, Nov 30, 2022 at 06:17:15PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:52:57, Markus Schneider-Pargmann wrote:
> > Currently the only mode of operation is an interrupt for every transmit
> > event. This is inefficient for peripheral chips. Use the transmit FIFO
> > event watermark interrupt instead if the FIFO size is more than 2. Use
> > FIFOsize - 1 for the watermark so the interrupt is triggered early
> > enough to not stop transmitting.
> > 
> > Note that if the number of transmits is less than the watermark level,
> > the transmit events will not be processed until there is any other
> > interrupt. This will only affect statistic counters. Also there is an
> > interrupt every time the timestamp wraps around.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> 
> Please make this configurable with the ethtool TX IRQ coalescing
> parameter. Please setup an hwtimer to enable the regular interrupt after
> some configurable time to avoid starving of the TX complete events.

I guess hwtimer==hrtimer?

I thought about setting up a timer but decided against it as the TX
completion events are only used to update statistics of the interface,
as far as I can tell. I can implement a timer as well.

For the upcoming receive side patch I already added a hrtimer. I may try
to use the same timer for both directions as it is going to do the exact
same thing in both cases (call the interrupt routine). Of course that
depends on the details of the coalescing support. Any objections on
that?

> I've implemented this for the mcp251xfd driver, see:
> 
> 656fc12ddaf8 ("can: mcp251xfd: add TX IRQ coalescing ethtool support")
> 169d00a25658 ("can: mcp251xfd: add TX IRQ coalescing support")
> 846990e0ed82 ("can: mcp251xfd: add RX IRQ coalescing ethtool support")
> 60a848c50d2d ("can: mcp251xfd: add RX IRQ coalescing support")
> 9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")

Thanks for the pointers. I will have a look and try to implement it
similarly.

Best,
Markus

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 01.12.2022 09:25:21, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> Thanks for reviewing.
> 
> On Wed, Nov 30, 2022 at 06:17:15PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:52:57, Markus Schneider-Pargmann wrote:
> > > Currently the only mode of operation is an interrupt for every transmit
> > > event. This is inefficient for peripheral chips. Use the transmit FIFO
> > > event watermark interrupt instead if the FIFO size is more than 2. Use
> > > FIFOsize - 1 for the watermark so the interrupt is triggered early
> > > enough to not stop transmitting.
> > > 
> > > Note that if the number of transmits is less than the watermark level,
> > > the transmit events will not be processed until there is any other
> > > interrupt. This will only affect statistic counters. Also there is an
> > > interrupt every time the timestamp wraps around.
> > > 
> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > 
> > Please make this configurable with the ethtool TX IRQ coalescing
> > parameter. Please setup an hwtimer to enable the regular interrupt after
> > some configurable time to avoid starving of the TX complete events.
> 
> I guess hwtimer==hrtimer?

Sorry, yes!

> I thought about setting up a timer but decided against it as the TX
> completion events are only used to update statistics of the interface,
> as far as I can tell. I can implement a timer as well.

It's not only statistics, the sending socket can opt in to receive the
sent CAN frame on successful transmission. Other sockets will (by
default) receive successful sent CAN frames. The idea is that the other
sockets see the same CAN bus, doesn't matter if they are on a different
system receiving the CAN frame via the bus or on the same system
receiving the CAN frame as soon it has been sent to the bus.

> For the upcoming receive side patch I already added a hrtimer. I may try
> to use the same timer for both directions as it is going to do the exact
> same thing in both cases (call the interrupt routine). Of course that
> depends on the details of the coalescing support. Any objections on
> that?

For the mcp251xfd I implemented the RX and TX coalescing independent of
each other and made it configurable via ethtool's IRQ coalescing
options.

The hardware doesn't support any timeouts and only FIFO not empty, FIFO
half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
limited. I think the mcan core has the same limitations.

The configuration for the mcp251xfd looks like this:

- First decide for classical CAN or CAN-FD mode
- configure RX and TX ring size
  9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
  For TX only a single FIFO is used.
  For RX up to 3 FIFOs (up to a depth of 32 each).
  FIFO depth is limited to power of 2.
  On the mcan cores this is currently done with a DT property.
  Runtime configurable ring size is optional but gives more flexibility
  for our use-cases due to limited RAM size.
- configure RX and TX coalescing via ethtools
  Set a timeout and the max CAN frames to coalesce.
  The max frames are limited to half or full FIFO.

How does coalescing work?

If coalescing is activated during reading of the RX'ed frames the FIFO
not empty IRQ is disabled (the half or full IRQ stays enabled). After
handling the RX'ed frames a hrtimer is started. In the hrtimer's
functions the FIFO not empty IRQ is enabled again.

I decided not to call the IRQ handler from the hrtimer to avoid
concurrency, but enable the FIFO not empty IRQ.

> > I've implemented this for the mcp251xfd driver, see:
> > 
> > 656fc12ddaf8 ("can: mcp251xfd: add TX IRQ coalescing ethtool support")
> > 169d00a25658 ("can: mcp251xfd: add TX IRQ coalescing support")
> > 846990e0ed82 ("can: mcp251xfd: add RX IRQ coalescing ethtool support")
> > 60a848c50d2d ("can: mcp251xfd: add RX IRQ coalescing support")
> > 9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> 
> Thanks for the pointers. I will have a look and try to implement it
> similarly.

If you want to implement runtime configurable ring size, I created a
function to help in the calculation of the ring sizes:

a1439a5add62 ("can: mcp251xfd: ram: add helper function for runtime ring size calculation")

The code is part of the mcp251xfd driver, but is prepared to become a
generic helper function. The HW parameters are described with struct
can_ram_config and use you can_ram_get_layout() to get a valid RAM
layout based on CAN/CAN-FD ring size and coalescing parameters.

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 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

HI Marc,

On Thu, Dec 01, 2022 at 10:05:08AM +0100, Marc Kleine-Budde wrote:
> On 01.12.2022 09:25:21, Markus Schneider-Pargmann wrote:
> > Hi Marc,
> > 
> > Thanks for reviewing.
> > 
> > On Wed, Nov 30, 2022 at 06:17:15PM +0100, Marc Kleine-Budde wrote:
> > > On 16.11.2022 21:52:57, Markus Schneider-Pargmann wrote:
> > > > Currently the only mode of operation is an interrupt for every transmit
> > > > event. This is inefficient for peripheral chips. Use the transmit FIFO
> > > > event watermark interrupt instead if the FIFO size is more than 2. Use
> > > > FIFOsize - 1 for the watermark so the interrupt is triggered early
> > > > enough to not stop transmitting.
> > > > 
> > > > Note that if the number of transmits is less than the watermark level,
> > > > the transmit events will not be processed until there is any other
> > > > interrupt. This will only affect statistic counters. Also there is an
> > > > interrupt every time the timestamp wraps around.
> > > > 
> > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > 
> > > Please make this configurable with the ethtool TX IRQ coalescing
> > > parameter. Please setup an hwtimer to enable the regular interrupt after
> > > some configurable time to avoid starving of the TX complete events.
> > 
> > I guess hwtimer==hrtimer?
> 
> Sorry, yes!
> 
> > I thought about setting up a timer but decided against it as the TX
> > completion events are only used to update statistics of the interface,
> > as far as I can tell. I can implement a timer as well.
> 
> It's not only statistics, the sending socket can opt in to receive the
> sent CAN frame on successful transmission. Other sockets will (by
> default) receive successful sent CAN frames. The idea is that the other
> sockets see the same CAN bus, doesn't matter if they are on a different
> system receiving the CAN frame via the bus or on the same system
> receiving the CAN frame as soon it has been sent to the bus.

Thanks for explaining. I wasn't aware of that. I agree on the timer
then.

> 
> > For the upcoming receive side patch I already added a hrtimer. I may try
> > to use the same timer for both directions as it is going to do the exact
> > same thing in both cases (call the interrupt routine). Of course that
> > depends on the details of the coalescing support. Any objections on
> > that?
> 
> For the mcp251xfd I implemented the RX and TX coalescing independent of
> each other and made it configurable via ethtool's IRQ coalescing
> options.
> 
> The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> limited. I think the mcan core has the same limitations.

Yes and no, the mcan core provides watermark levels so it has more
options, but there is no hardware timer as well (at least I didn't see
anything usable).

> 
> The configuration for the mcp251xfd looks like this:
> 
> - First decide for classical CAN or CAN-FD mode
> - configure RX and TX ring size
>   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
>   For TX only a single FIFO is used.
>   For RX up to 3 FIFOs (up to a depth of 32 each).
>   FIFO depth is limited to power of 2.
>   On the mcan cores this is currently done with a DT property.
>   Runtime configurable ring size is optional but gives more flexibility
>   for our use-cases due to limited RAM size.
> - configure RX and TX coalescing via ethtools
>   Set a timeout and the max CAN frames to coalesce.
>   The max frames are limited to half or full FIFO.

mcan can offer more options for the max frames limit fortunately.

> 
> How does coalescing work?
> 
> If coalescing is activated during reading of the RX'ed frames the FIFO
> not empty IRQ is disabled (the half or full IRQ stays enabled). After
> handling the RX'ed frames a hrtimer is started. In the hrtimer's
> functions the FIFO not empty IRQ is enabled again.

My rx path patches are working similarly though not 100% the same. I
will adopt everything and add it to the next version of this series.

> 
> I decided not to call the IRQ handler from the hrtimer to avoid
> concurrency, but enable the FIFO not empty IRQ.

mcan uses a threaded irq and I found this nice helper function I am
currently using for the receive path.
	irq_wake_thread()

It is not widely used so I hope this is fine. But this hopefully avoids
the concurrency issue. Also I don't need to artificially create an IRQ
as you do.

> 
> > > I've implemented this for the mcp251xfd driver, see:
> > > 
> > > 656fc12ddaf8 ("can: mcp251xfd: add TX IRQ coalescing ethtool support")
> > > 169d00a25658 ("can: mcp251xfd: add TX IRQ coalescing support")
> > > 846990e0ed82 ("can: mcp251xfd: add RX IRQ coalescing ethtool support")
> > > 60a848c50d2d ("can: mcp251xfd: add RX IRQ coalescing support")
> > > 9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> > 
> > Thanks for the pointers. I will have a look and try to implement it
> > similarly.
> 
> If you want to implement runtime configurable ring size, I created a
> function to help in the calculation of the ring sizes:
> 
> a1439a5add62 ("can: mcp251xfd: ram: add helper function for runtime ring size calculation")
> 
> The code is part of the mcp251xfd driver, but is prepared to become a
> generic helper function. The HW parameters are described with struct
> can_ram_config and use you can_ram_get_layout() to get a valid RAM
> layout based on CAN/CAN-FD ring size and coalescing parameters.

Thank you. I think configurable ring sizes are currently out of scope
for me as I only have limited time for this.

Thank you Marc!

Best,
Markus

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:
> > > For the upcoming receive side patch I already added a hrtimer. I may try
> > > to use the same timer for both directions as it is going to do the exact
> > > same thing in both cases (call the interrupt routine). Of course that
> > > depends on the details of the coalescing support. Any objections on
> > > that?
> > 
> > For the mcp251xfd I implemented the RX and TX coalescing independent of
> > each other and made it configurable via ethtool's IRQ coalescing
> > options.
> > 
> > The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> > half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> > limited. I think the mcan core has the same limitations.
> 
> Yes and no, the mcan core provides watermark levels so it has more
> options, but there is no hardware timer as well (at least I didn't see
> anything usable).

Are there any limitations to the water mark level?

> > The configuration for the mcp251xfd looks like this:
> > 
> > - First decide for classical CAN or CAN-FD mode
> > - configure RX and TX ring size
> >   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> >   For TX only a single FIFO is used.
> >   For RX up to 3 FIFOs (up to a depth of 32 each).
> >   FIFO depth is limited to power of 2.
> >   On the mcan cores this is currently done with a DT property.
> >   Runtime configurable ring size is optional but gives more flexibility
> >   for our use-cases due to limited RAM size.
> > - configure RX and TX coalescing via ethtools
> >   Set a timeout and the max CAN frames to coalesce.
> >   The max frames are limited to half or full FIFO.
> 
> mcan can offer more options for the max frames limit fortunately.
> 
> > 
> > How does coalescing work?
> > 
> > If coalescing is activated during reading of the RX'ed frames the FIFO
> > not empty IRQ is disabled (the half or full IRQ stays enabled). After
> > handling the RX'ed frames a hrtimer is started. In the hrtimer's
> > functions the FIFO not empty IRQ is enabled again.
> 
> My rx path patches are working similarly though not 100% the same. I
> will adopt everything and add it to the next version of this series.
> 
> > 
> > I decided not to call the IRQ handler from the hrtimer to avoid
> > concurrency, but enable the FIFO not empty IRQ.
> 
> mcan uses a threaded irq and I found this nice helper function I am
> currently using for the receive path.
> 	irq_wake_thread()
> 
> It is not widely used so I hope this is fine. But this hopefully avoids
> the concurrency issue. Also I don't need to artificially create an IRQ
> as you do.

I think it's Ok to use the function. Which IRQs are enabled after you
leave the RX handler? The mcp251xfd driver enables only a high watermark
IRQ and sets up the hrtimer. Then we have 3 scenarios:
- high watermark IRQ triggers -> IRQ is handled,
- FIFO level between 0 and high water mark -> no IRQ triggered, but
  hrtimer will run, irq_wake_thread() is called, IRQ is handled
- FIFO level 0 -> no IRQ triggered, hrtimer will run. What do you do in
  the IRQ handler? Check if FIFO is empty and enable the FIFO not empty
  IRQ?

The mcp251xfd unconditionally enables the FIFO not empty IRQ in the
hrtimer. This avoids reading of the FIFO fill level.

[...]

> > If you want to implement runtime configurable ring size, I created a
> > function to help in the calculation of the ring sizes:
> > 
> > a1439a5add62 ("can: mcp251xfd: ram: add helper function for runtime ring size calculation")
> > 
> > The code is part of the mcp251xfd driver, but is prepared to become a
> > generic helper function. The HW parameters are described with struct
> > can_ram_config and use you can_ram_get_layout() to get a valid RAM
> > layout based on CAN/CAN-FD ring size and coalescing parameters.
> 
> Thank you. I think configurable ring sizes are currently out of scope
> for me as I only have limited time for this.

Ok.

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 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

On Thu, Dec 01, 2022 at 12:00:33PM +0100, Marc Kleine-Budde wrote:
> On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:
> > > > For the upcoming receive side patch I already added a hrtimer. I may try
> > > > to use the same timer for both directions as it is going to do the exact
> > > > same thing in both cases (call the interrupt routine). Of course that
> > > > depends on the details of the coalescing support. Any objections on
> > > > that?
> > > 
> > > For the mcp251xfd I implemented the RX and TX coalescing independent of
> > > each other and made it configurable via ethtool's IRQ coalescing
> > > options.
> > > 
> > > The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> > > half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> > > limited. I think the mcan core has the same limitations.
> > 
> > Yes and no, the mcan core provides watermark levels so it has more
> > options, but there is no hardware timer as well (at least I didn't see
> > anything usable).
> 
> Are there any limitations to the water mark level?

Anything specific? I can't really see any limitation. You can set the
watermark between 1 and 32. I guess we could also always use it instead
of the new-element interrupt, but I haven't tried that yet. That may
simplify the code.

> 
> > > The configuration for the mcp251xfd looks like this:
> > > 
> > > - First decide for classical CAN or CAN-FD mode
> > > - configure RX and TX ring size
> > >   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> > >   For TX only a single FIFO is used.
> > >   For RX up to 3 FIFOs (up to a depth of 32 each).
> > >   FIFO depth is limited to power of 2.
> > >   On the mcan cores this is currently done with a DT property.
> > >   Runtime configurable ring size is optional but gives more flexibility
> > >   for our use-cases due to limited RAM size.
> > > - configure RX and TX coalescing via ethtools
> > >   Set a timeout and the max CAN frames to coalesce.
> > >   The max frames are limited to half or full FIFO.
> > 
> > mcan can offer more options for the max frames limit fortunately.
> > 
> > > 
> > > How does coalescing work?
> > > 
> > > If coalescing is activated during reading of the RX'ed frames the FIFO
> > > not empty IRQ is disabled (the half or full IRQ stays enabled). After
> > > handling the RX'ed frames a hrtimer is started. In the hrtimer's
> > > functions the FIFO not empty IRQ is enabled again.
> > 
> > My rx path patches are working similarly though not 100% the same. I
> > will adopt everything and add it to the next version of this series.
> > 
> > > 
> > > I decided not to call the IRQ handler from the hrtimer to avoid
> > > concurrency, but enable the FIFO not empty IRQ.
> > 
> > mcan uses a threaded irq and I found this nice helper function I am
> > currently using for the receive path.
> > 	irq_wake_thread()
> > 
> > It is not widely used so I hope this is fine. But this hopefully avoids
> > the concurrency issue. Also I don't need to artificially create an IRQ
> > as you do.
> 
> I think it's Ok to use the function. Which IRQs are enabled after you
> leave the RX handler? The mcp251xfd driver enables only a high watermark
> IRQ and sets up the hrtimer. Then we have 3 scenarios:
> - high watermark IRQ triggers -> IRQ is handled,
> - FIFO level between 0 and high water mark -> no IRQ triggered, but
>   hrtimer will run, irq_wake_thread() is called, IRQ is handled
> - FIFO level 0 -> no IRQ triggered, hrtimer will run. What do you do in
>   the IRQ handler? Check if FIFO is empty and enable the FIFO not empty
>   IRQ?

I am currently doing the normal IRQ handler run. It checks the
"Interrupt Register" at the beginning. This register does not show the
interrupts that fired, it shows the status. So even though the watermark
interrupt didn't trigger when called by a timer, RF0N 'new message'
status bit is still set if there is something new in the FIFO. Of course
it is the same for the transmit status bits.
So there is no need to read the FIFO fill levels directly, just the
general status register.

Best,
Markus

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 01.12.2022 17:59:51, Markus Schneider-Pargmann wrote:
> On Thu, Dec 01, 2022 at 12:00:33PM +0100, Marc Kleine-Budde wrote:
> > On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:
> > > > > For the upcoming receive side patch I already added a hrtimer. I may try
> > > > > to use the same timer for both directions as it is going to do the exact
> > > > > same thing in both cases (call the interrupt routine). Of course that
> > > > > depends on the details of the coalescing support. Any objections on
> > > > > that?
> > > > 
> > > > For the mcp251xfd I implemented the RX and TX coalescing independent of
> > > > each other and made it configurable via ethtool's IRQ coalescing
> > > > options.
> > > > 
> > > > The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> > > > half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> > > > limited. I think the mcan core has the same limitations.
> > > 
> > > Yes and no, the mcan core provides watermark levels so it has more
> > > options, but there is no hardware timer as well (at least I didn't see
> > > anything usable).
> > 
> > Are there any limitations to the water mark level?
> 
> Anything specific? I can't really see any limitation. You can set the
> watermark between 1 and 32. I guess we could also always use it instead
> of the new-element interrupt, but I haven't tried that yet. That may
> simplify the code.

Makes sense.

> > > > The configuration for the mcp251xfd looks like this:
> > > > 
> > > > - First decide for classical CAN or CAN-FD mode
> > > > - configure RX and TX ring size
> > > >   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> > > >   For TX only a single FIFO is used.
> > > >   For RX up to 3 FIFOs (up to a depth of 32 each).
> > > >   FIFO depth is limited to power of 2.
> > > >   On the mcan cores this is currently done with a DT property.
> > > >   Runtime configurable ring size is optional but gives more flexibility
> > > >   for our use-cases due to limited RAM size.
> > > > - configure RX and TX coalescing via ethtools
> > > >   Set a timeout and the max CAN frames to coalesce.
> > > >   The max frames are limited to half or full FIFO.
> > > 
> > > mcan can offer more options for the max frames limit fortunately.
> > > 
> > > > 
> > > > How does coalescing work?
> > > > 
> > > > If coalescing is activated during reading of the RX'ed frames the FIFO
> > > > not empty IRQ is disabled (the half or full IRQ stays enabled). After
> > > > handling the RX'ed frames a hrtimer is started. In the hrtimer's
> > > > functions the FIFO not empty IRQ is enabled again.
> > > 
> > > My rx path patches are working similarly though not 100% the same. I
> > > will adopt everything and add it to the next version of this series.
> > > 
> > > > 
> > > > I decided not to call the IRQ handler from the hrtimer to avoid
> > > > concurrency, but enable the FIFO not empty IRQ.
> > > 
> > > mcan uses a threaded irq and I found this nice helper function I am
> > > currently using for the receive path.
> > > 	irq_wake_thread()
> > > 
> > > It is not widely used so I hope this is fine. But this hopefully avoids
> > > the concurrency issue. Also I don't need to artificially create an IRQ
> > > as you do.
> > 
> > I think it's Ok to use the function. Which IRQs are enabled after you
> > leave the RX handler? The mcp251xfd driver enables only a high watermark
> > IRQ and sets up the hrtimer. Then we have 3 scenarios:
> > - high watermark IRQ triggers -> IRQ is handled,
> > - FIFO level between 0 and high water mark -> no IRQ triggered, but
> >   hrtimer will run, irq_wake_thread() is called, IRQ is handled
> > - FIFO level 0 -> no IRQ triggered, hrtimer will run. What do you do in
> >   the IRQ handler? Check if FIFO is empty and enable the FIFO not empty
> >   IRQ?
> 
> I am currently doing the normal IRQ handler run. It checks the
> "Interrupt Register" at the beginning. This register does not show the
> interrupts that fired, it shows the status. So even though the watermark
> interrupt didn't trigger when called by a timer, RF0N 'new message'
> status bit is still set if there is something new in the FIFO.

That covers scenario 2 from above.

> Of course it is the same for the transmit status bits.

ACK - The TX complete event handling is a 95% copy/paste of the RX
handling.

> So there is no need to read the FIFO fill levels directly, just the
> general status register.

What do you do if the hrtimer fires and there's no CAN frame waiting in
the FIFO?

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 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

On Fri, Dec 02, 2022 at 10:23:06AM +0100, Marc Kleine-Budde wrote:
...
> > > > > The configuration for the mcp251xfd looks like this:
> > > > > 
> > > > > - First decide for classical CAN or CAN-FD mode
> > > > > - configure RX and TX ring size
> > > > >   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> > > > >   For TX only a single FIFO is used.
> > > > >   For RX up to 3 FIFOs (up to a depth of 32 each).
> > > > >   FIFO depth is limited to power of 2.
> > > > >   On the mcan cores this is currently done with a DT property.
> > > > >   Runtime configurable ring size is optional but gives more flexibility
> > > > >   for our use-cases due to limited RAM size.
> > > > > - configure RX and TX coalescing via ethtools
> > > > >   Set a timeout and the max CAN frames to coalesce.
> > > > >   The max frames are limited to half or full FIFO.
> > > > 
> > > > mcan can offer more options for the max frames limit fortunately.
> > > > 
> > > > > 
> > > > > How does coalescing work?
> > > > > 
> > > > > If coalescing is activated during reading of the RX'ed frames the FIFO
> > > > > not empty IRQ is disabled (the half or full IRQ stays enabled). After
> > > > > handling the RX'ed frames a hrtimer is started. In the hrtimer's
> > > > > functions the FIFO not empty IRQ is enabled again.
> > > > 
> > > > My rx path patches are working similarly though not 100% the same. I
> > > > will adopt everything and add it to the next version of this series.
> > > > 
> > > > > 
> > > > > I decided not to call the IRQ handler from the hrtimer to avoid
> > > > > concurrency, but enable the FIFO not empty IRQ.
> > > > 
> > > > mcan uses a threaded irq and I found this nice helper function I am
> > > > currently using for the receive path.
> > > > 	irq_wake_thread()
> > > > 
> > > > It is not widely used so I hope this is fine. But this hopefully avoids
> > > > the concurrency issue. Also I don't need to artificially create an IRQ
> > > > as you do.
> > > 
> > > I think it's Ok to use the function. Which IRQs are enabled after you
> > > leave the RX handler? The mcp251xfd driver enables only a high watermark
> > > IRQ and sets up the hrtimer. Then we have 3 scenarios:
> > > - high watermark IRQ triggers -> IRQ is handled,
> > > - FIFO level between 0 and high water mark -> no IRQ triggered, but
> > >   hrtimer will run, irq_wake_thread() is called, IRQ is handled
> > > - FIFO level 0 -> no IRQ triggered, hrtimer will run. What do you do in
> > >   the IRQ handler? Check if FIFO is empty and enable the FIFO not empty
> > >   IRQ?
> > 
> > I am currently doing the normal IRQ handler run. It checks the
> > "Interrupt Register" at the beginning. This register does not show the
> > interrupts that fired, it shows the status. So even though the watermark
> > interrupt didn't trigger when called by a timer, RF0N 'new message'
> > status bit is still set if there is something new in the FIFO.
> 
> That covers scenario 2 from above.
> 
> > Of course it is the same for the transmit status bits.
> 
> ACK - The TX complete event handling is a 95% copy/paste of the RX
> handling.
> 
> > So there is no need to read the FIFO fill levels directly, just the
> > general status register.
> 
> What do you do if the hrtimer fires and there's no CAN frame waiting in
> the FIFO?

Just enabling the 'new item' interrupt again and keep the hrtimer
disabled.

Best,
Markus

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 02.12.2022 10:43:46, Markus Schneider-Pargmann wrote:
> On Fri, Dec 02, 2022 at 10:23:06AM +0100, Marc Kleine-Budde wrote:
> ...
> > > > > > The configuration for the mcp251xfd looks like this:
> > > > > > 
> > > > > > - First decide for classical CAN or CAN-FD mode
> > > > > > - configure RX and TX ring size
> > > > > >   9263c2e92be9 ("can: mcp251xfd: ring: add support for runtime configurable RX/TX ring parameters")
> > > > > >   For TX only a single FIFO is used.
> > > > > >   For RX up to 3 FIFOs (up to a depth of 32 each).
> > > > > >   FIFO depth is limited to power of 2.
> > > > > >   On the mcan cores this is currently done with a DT property.
> > > > > >   Runtime configurable ring size is optional but gives more flexibility
> > > > > >   for our use-cases due to limited RAM size.
> > > > > > - configure RX and TX coalescing via ethtools
> > > > > >   Set a timeout and the max CAN frames to coalesce.
> > > > > >   The max frames are limited to half or full FIFO.
> > > > > 
> > > > > mcan can offer more options for the max frames limit fortunately.
> > > > > 
> > > > > > 
> > > > > > How does coalescing work?
> > > > > > 
> > > > > > If coalescing is activated during reading of the RX'ed frames the FIFO
> > > > > > not empty IRQ is disabled (the half or full IRQ stays enabled). After
> > > > > > handling the RX'ed frames a hrtimer is started. In the hrtimer's
> > > > > > functions the FIFO not empty IRQ is enabled again.
> > > > > 
> > > > > My rx path patches are working similarly though not 100% the same. I
> > > > > will adopt everything and add it to the next version of this series.
> > > > > 
> > > > > > 
> > > > > > I decided not to call the IRQ handler from the hrtimer to avoid
> > > > > > concurrency, but enable the FIFO not empty IRQ.
> > > > > 
> > > > > mcan uses a threaded irq and I found this nice helper function I am
> > > > > currently using for the receive path.
> > > > > 	irq_wake_thread()
> > > > > 
> > > > > It is not widely used so I hope this is fine. But this hopefully avoids
> > > > > the concurrency issue. Also I don't need to artificially create an IRQ
> > > > > as you do.
> > > > 
> > > > I think it's Ok to use the function. Which IRQs are enabled after you
> > > > leave the RX handler? The mcp251xfd driver enables only a high watermark
> > > > IRQ and sets up the hrtimer. Then we have 3 scenarios:
> > > > - high watermark IRQ triggers -> IRQ is handled,
> > > > - FIFO level between 0 and high water mark -> no IRQ triggered, but
> > > >   hrtimer will run, irq_wake_thread() is called, IRQ is handled
> > > > - FIFO level 0 -> no IRQ triggered, hrtimer will run. What do you do in
> > > >   the IRQ handler? Check if FIFO is empty and enable the FIFO not empty
> > > >   IRQ?
> > > 
> > > I am currently doing the normal IRQ handler run. It checks the
> > > "Interrupt Register" at the beginning. This register does not show the
> > > interrupts that fired, it shows the status. So even though the watermark
> > > interrupt didn't trigger when called by a timer, RF0N 'new message'
> > > status bit is still set if there is something new in the FIFO.
> > 
> > That covers scenario 2 from above.
> > 
> > > Of course it is the same for the transmit status bits.
> > 
> > ACK - The TX complete event handling is a 95% copy/paste of the RX
> > handling.
> > 
> > > So there is no need to read the FIFO fill levels directly, just the
> > > general status register.
> > 
> > What do you do if the hrtimer fires and there's no CAN frame waiting in
> > the FIFO?
> 
> Just enabling the 'new item' interrupt again and keep the hrtimer
> disabled.

Sounds good!

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 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Markus Schneider-Pargmann]

Hi Marc,

On Thu, Dec 01, 2022 at 05:59:53PM +0100, Markus Schneider-Pargmann wrote:
> On Thu, Dec 01, 2022 at 12:00:33PM +0100, Marc Kleine-Budde wrote:
> > On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:
> > > > > For the upcoming receive side patch I already added a hrtimer. I may try
> > > > > to use the same timer for both directions as it is going to do the exact
> > > > > same thing in both cases (call the interrupt routine). Of course that
> > > > > depends on the details of the coalescing support. Any objections on
> > > > > that?
> > > > 
> > > > For the mcp251xfd I implemented the RX and TX coalescing independent of
> > > > each other and made it configurable via ethtool's IRQ coalescing
> > > > options.
> > > > 
> > > > The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> > > > half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> > > > limited. I think the mcan core has the same limitations.
> > > 
> > > Yes and no, the mcan core provides watermark levels so it has more
> > > options, but there is no hardware timer as well (at least I didn't see
> > > anything usable).
> > 
> > Are there any limitations to the water mark level?
> 
> Anything specific? I can't really see any limitation. You can set the
> watermark between 1 and 32. I guess we could also always use it instead
> of the new-element interrupt, but I haven't tried that yet. That may
> simplify the code.

Just a quick comment here after trying this, I decided against it.
- I can't modify the watermark levels once the chip is active.
- Using interrupt (un)masking I can change the behavior for tx and rx
  with a single register write instead of two to the two fifo
  configuration registers.

You will see this in the second part of the series then.

Best,
Markus

Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt

[Marc Kleine-Budde]

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

On 13.12.2022 18:19:46, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Thu, Dec 01, 2022 at 05:59:53PM +0100, Markus Schneider-Pargmann wrote:
> > On Thu, Dec 01, 2022 at 12:00:33PM +0100, Marc Kleine-Budde wrote:
> > > On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:
> > > > > > For the upcoming receive side patch I already added a hrtimer. I may try
> > > > > > to use the same timer for both directions as it is going to do the exact
> > > > > > same thing in both cases (call the interrupt routine). Of course that
> > > > > > depends on the details of the coalescing support. Any objections on
> > > > > > that?
> > > > > 
> > > > > For the mcp251xfd I implemented the RX and TX coalescing independent of
> > > > > each other and made it configurable via ethtool's IRQ coalescing
> > > > > options.
> > > > > 
> > > > > The hardware doesn't support any timeouts and only FIFO not empty, FIFO
> > > > > half full and FIFO full IRQs and the on chip RAM for mailboxes is rather
> > > > > limited. I think the mcan core has the same limitations.
> > > > 
> > > > Yes and no, the mcan core provides watermark levels so it has more
> > > > options, but there is no hardware timer as well (at least I didn't see
> > > > anything usable).
> > > 
> > > Are there any limitations to the water mark level?
> > 
> > Anything specific? I can't really see any limitation. You can set the
> > watermark between 1 and 32. I guess we could also always use it instead
> > of the new-element interrupt, but I haven't tried that yet. That may
> > simplify the code.
> 
> Just a quick comment here after trying this, I decided against it.
> - I can't modify the watermark levels once the chip is active.
> - Using interrupt (un)masking I can change the behavior for tx and rx
>   with a single register write instead of two to the two fifo
>   configuration registers.

Makes sense.

> You will see this in the second part of the series then.

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

[PATCH 05/15] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 4a6972c8bacd..5c00c6162058 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1245,6 +1245,11 @@ static void m_can_chip_config(struct net_device *dev)
 	u32 cccr, test;
 	u32 interrupts = IR_ALL_INT;
 
+	/* Disable unused interrupts */
+	interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | 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 */
-- 
2.38.1

[PATCH 06/15] can: m_can: Avoid reading irqstatus twice

[Markus Schneider-Pargmann]

For peripheral devices the m_can_rx_handler is called directly after
setting cdev->irqstatus. This means we don't have to read the irqstatus
again in m_can_rx_handler. Avoid this by adding a parameter that is
false for direct calls.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 5c00c6162058..0efa6dee0617 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -896,14 +896,13 @@ static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
 	return work_done;
 }
 
-static int m_can_rx_handler(struct net_device *dev, int quota)
+static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int rx_work_or_err;
 	int work_done = 0;
-	u32 irqstatus, psr;
+	u32 psr;
 
-	irqstatus = cdev->irqstatus | m_can_read(cdev, M_CAN_IR);
 	if (!irqstatus)
 		goto end;
 
@@ -947,12 +946,12 @@ static int m_can_rx_handler(struct net_device *dev, int quota)
 	return work_done;
 }
 
-static int m_can_rx_peripheral(struct net_device *dev)
+static int m_can_rx_peripheral(struct net_device *dev, u32 irqstatus)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int work_done;
 
-	work_done = m_can_rx_handler(dev, NAPI_POLL_WEIGHT);
+	work_done = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, irqstatus);
 
 	/* Don't re-enable interrupts if the driver had a fatal error
 	 * (e.g., FIFO read failure).
@@ -968,8 +967,11 @@ static int m_can_poll(struct napi_struct *napi, int quota)
 	struct net_device *dev = napi->dev;
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int work_done;
+	u32 irqstatus;
+
+	irqstatus = cdev->irqstatus | m_can_read(cdev, M_CAN_IR);
 
-	work_done = m_can_rx_handler(dev, quota);
+	work_done = m_can_rx_handler(dev, quota, irqstatus);
 
 	/* Don't re-enable interrupts if the driver had a fatal error
 	 * (e.g., FIFO read failure).
@@ -1080,7 +1082,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 		m_can_disable_all_interrupts(cdev);
 		if (!cdev->is_peripheral)
 			napi_schedule(&cdev->napi);
-		else if (m_can_rx_peripheral(dev) < 0)
+		else if (m_can_rx_peripheral(dev, ir) < 0)
 			goto out_fail;
 	}
 
-- 
2.38.1

[PATCH 07/15] can: m_can: Read register PSR only on error

[Markus Schneider-Pargmann]

Only read register PSR if there is an error indicated in irqstatus.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 0efa6dee0617..1d15beaea920 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -901,7 +901,6 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int rx_work_or_err;
 	int work_done = 0;
-	u32 psr;
 
 	if (!irqstatus)
 		goto end;
@@ -927,13 +926,13 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
 		}
 	}
 
-	psr = m_can_read(cdev, M_CAN_PSR);
-
 	if (irqstatus & IR_ERR_STATE)
-		work_done += m_can_handle_state_errors(dev, psr);
+		work_done += m_can_handle_state_errors(dev,
+						       m_can_read(cdev, M_CAN_PSR));
 
 	if (irqstatus & IR_ERR_BUS_30X)
-		work_done += m_can_handle_bus_errors(dev, irqstatus, psr);
+		work_done += m_can_handle_bus_errors(dev, irqstatus,
+						     m_can_read(cdev, M_CAN_PSR));
 
 	if (irqstatus & IR_RF0N) {
 		rx_work_or_err = m_can_do_rx_poll(dev, (quota - work_done));
-- 
2.38.1

[PATCH 08/15] can: m_can: Count TXE FIFO getidx in the driver

[Markus Schneider-Pargmann]

The getindex simply increases by one for every iteration. There is no
need to get the current getidx every time from a register. Instead we
can just count and wrap if necessary.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 1d15beaea920..27095a7254dd 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1021,15 +1021,13 @@ static int m_can_echo_tx_event(struct net_device *dev)
 
 	/* Get Tx Event fifo element count */
 	txe_count = FIELD_GET(TXEFS_EFFL_MASK, m_can_txefs);
+	fgi = FIELD_GET(TXEFS_EFGI_MASK, m_can_txefs);
 
 	/* Get and process all sent elements */
 	for (i = 0; i < txe_count; i++) {
 		u32 txe, timestamp = 0;
 		int err;
 
-		/* retrieve get index */
-		fgi = FIELD_GET(TXEFS_EFGI_MASK, m_can_read(cdev, M_CAN_TXEFS));
-
 		/* get message marker, timestamp */
 		err = m_can_txe_fifo_read(cdev, fgi, 4, &txe);
 		if (err) {
@@ -1043,6 +1041,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 		/* ack txe element */
 		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
 							  fgi));
+		fgi = (++fgi >= cdev->mcfg[MRAM_TXE].num ? 0 : fgi);
 		--cdev->tx_fifo_in_flight;
 
 		/* update stats */
-- 
2.38.1

[PATCH 09/15] can: m_can: Count read getindex in the driver

[Markus Schneider-Pargmann]

The getindex gets increased by one every time. We can calculate the
correct getindex in the driver and avoid the additional reads of rxfs.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 27095a7254dd..02fd7fe4e9f8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -477,19 +477,16 @@ static void m_can_receive_skb(struct m_can_classdev *cdev,
 	}
 }
 
-static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
+static int m_can_read_fifo(struct net_device *dev, u32 fgi)
 {
 	struct net_device_stats *stats = &dev->stats;
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	struct canfd_frame *cf;
 	struct sk_buff *skb;
 	struct id_and_dlc fifo_header;
-	u32 fgi;
 	u32 timestamp = 0;
 	int err;
 
-	/* calculate the fifo get index for where to read data */
-	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
 	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &fifo_header, 2);
 	if (err)
 		goto out_fail;
@@ -554,6 +551,9 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 pkts = 0;
 	u32 rxfs;
+	u32 rx_count;
+	u32 fgi;
+	int i;
 	int err;
 
 	rxfs = m_can_read(cdev, M_CAN_RXF0S);
@@ -562,14 +562,17 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 		return 0;
 	}
 
-	while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
-		err = m_can_read_fifo(dev, rxfs);
+	rx_count = FIELD_GET(RXFS_FFL_MASK, rxfs);
+	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
+
+	for (i = 0; i < rx_count && quota > 0; ++i) {
+		err = m_can_read_fifo(dev, fgi);
 		if (err)
 			return err;
 
 		quota--;
 		pkts++;
-		rxfs = m_can_read(cdev, M_CAN_RXF0S);
+		fgi = (++fgi >= cdev->mcfg[MRAM_RXF0].num ? 0 : fgi);
 	}
 
 	return pkts;
-- 
2.38.1

[PATCH 10/15] can: m_can: Batch acknowledge rx fifo

[Markus Schneider-Pargmann]

Instead of acknowledging every item of the fifo, only acknowledge the
last item read. This behavior is documented in the datasheet. The new
getindex will be the acknowledged item + 1.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 02fd7fe4e9f8..6179b9e815ed 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -530,9 +530,6 @@ static int m_can_read_fifo(struct net_device *dev, u32 fgi)
 	}
 	stats->rx_packets++;
 
-	/* acknowledge rx fifo 0 */
-	m_can_write(cdev, M_CAN_RXF0A, fgi);
-
 	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc) << 16;
 
 	m_can_receive_skb(cdev, skb, timestamp);
@@ -553,8 +550,9 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 	u32 rxfs;
 	u32 rx_count;
 	u32 fgi;
+	int ack_fgi = -1;
 	int i;
-	int err;
+	int err = 0;
 
 	rxfs = m_can_read(cdev, M_CAN_RXF0S);
 	if (!(rxfs & RXFS_FFL_MASK)) {
@@ -568,13 +566,20 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 	for (i = 0; i < rx_count && quota > 0; ++i) {
 		err = m_can_read_fifo(dev, fgi);
 		if (err)
-			return err;
+			break;
 
 		quota--;
 		pkts++;
+		ack_fgi = fgi;
 		fgi = (++fgi >= cdev->mcfg[MRAM_RXF0].num ? 0 : fgi);
 	}
 
+	if (ack_fgi != -1)
+		m_can_write(cdev, M_CAN_RXF0A, ack_fgi);
+
+	if (err)
+		return err;
+
 	return pkts;
 }
 
-- 
2.38.1

[PATCH 11/15] can: m_can: Batch acknowledge transmit events

[Markus Schneider-Pargmann]

Transmit events from the txe fifo can be batch acknowledged by
acknowledging the last read txe fifo item. This will save txe_count
writes which is important for peripheral chips.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6179b9e815ed..347ba8e7d1b3 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1019,7 +1019,9 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	u32 txe_count = 0;
 	u32 m_can_txefs;
 	u32 fgi = 0;
+	int ack_fgi = -1;
 	int i = 0;
+	int err = 0;
 	unsigned int msg_mark;
 
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -1034,21 +1036,18 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	/* Get and process all sent elements */
 	for (i = 0; i < txe_count; i++) {
 		u32 txe, timestamp = 0;
-		int err;
 
 		/* get message marker, timestamp */
 		err = m_can_txe_fifo_read(cdev, fgi, 4, &txe);
 		if (err) {
 			netdev_err(dev, "TXE FIFO read returned %d\n", err);
-			return err;
+			break;
 		}
 
 		msg_mark = FIELD_GET(TX_EVENT_MM_MASK, txe);
 		timestamp = FIELD_GET(TX_EVENT_TXTS_MASK, txe) << 16;
 
-		/* ack txe element */
-		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
-							  fgi));
+		ack_fgi = fgi;
 		fgi = (++fgi >= cdev->mcfg[MRAM_TXE].num ? 0 : fgi);
 		--cdev->tx_fifo_in_flight;
 
@@ -1056,7 +1055,11 @@ static int m_can_echo_tx_event(struct net_device *dev)
 		m_can_tx_update_stats(cdev, msg_mark, timestamp);
 	}
 
-	return 0;
+	if (ack_fgi != -1)
+		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
+							  ack_fgi));
+
+	return err;
 }
 
 static irqreturn_t m_can_isr(int irq, void *dev_id)
-- 
2.38.1

[PATCH 12/15] can: tcan4x5x: Remove invalid write in clear_interrupts

[Markus Schneider-Pargmann]

Register 0x824 TCAN4X5X_MCAN_INT_REG is a read-only register. Any writes
to this register do not have any effect.

Remove this write. The m_can driver aldready clears the interrupts in
m_can_isr() by writing to M_CAN_IR which is translated to register
0x1050 which is a writable version of this register.

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

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 41645a24384c..1fec394b3517 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -204,11 +204,6 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
-	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_MCAN_INT_REG,
-				      TCAN4X5X_ENABLE_MCAN_INT);
-	if (ret)
-		return ret;
-
 	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
 				      TCAN4X5X_CLEAR_ALL_INT);
 	if (ret)
-- 
2.38.1

Re: [PATCH 12/15] can: tcan4x5x: Remove invalid write in clear_interrupts

[Marc Kleine-Budde]

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

On 16.11.2022 21:53:05, Markus Schneider-Pargmann wrote:
> Register 0x824 TCAN4X5X_MCAN_INT_REG is a read-only register. Any writes
> to this register do not have any effect.
> 
> Remove this write. The m_can driver aldready clears the interrupts in
> m_can_isr() by writing to M_CAN_IR which is translated to register
> 0x1050 which is a writable version of this register.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

Please add a fixes tag.

Marc

> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 41645a24384c..1fec394b3517 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -204,11 +204,6 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_MCAN_INT_REG,
> -				      TCAN4X5X_ENABLE_MCAN_INT);
> -	if (ret)
> -		return ret;
> -
>  	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
>  				      TCAN4X5X_CLEAR_ALL_INT);
>  	if (ret)
> -- 
> 2.38.1
> 
> 

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

[PATCH 13/15] can: tcan4x5x: Fix use of register error status mask

[Markus Schneider-Pargmann]

TCAN4X5X_ERROR_STATUS is not a status register that needs clearing
during interrupt handling. Instead this is a masking register that masks
error interrupts. Writing TCAN4X5X_CLEAR_ALL_INT to this register
effectively masks everything.

Rename the register and mask all error interrupts only once by writing
to the register in tcan4x5x_init.

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

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 1fec394b3517..efa2381bf85b 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -10,7 +10,7 @@
 #define TCAN4X5X_DEV_ID1 0x04
 #define TCAN4X5X_REV 0x08
 #define TCAN4X5X_STATUS 0x0C
-#define TCAN4X5X_ERROR_STATUS 0x10
+#define TCAN4X5X_ERROR_STATUS_MASK 0x10
 #define TCAN4X5X_CONTROL 0x14
 
 #define TCAN4X5X_CONFIG 0x800
@@ -204,12 +204,7 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
-	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
-				      TCAN4X5X_CLEAR_ALL_INT);
-	if (ret)
-		return ret;
-
-	return tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_ERROR_STATUS,
+	return tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
 				       TCAN4X5X_CLEAR_ALL_INT);
 }
 
@@ -229,6 +224,11 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
+	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_ERROR_STATUS_MASK,
+				      TCAN4X5X_CLEAR_ALL_INT);
+	if (ret)
+		return ret;
+
 	/* Zero out the MCAN buffers */
 	ret = m_can_init_ram(cdev);
 	if (ret)
-- 
2.38.1

Re: [PATCH 13/15] can: tcan4x5x: Fix use of register error status mask

[Marc Kleine-Budde]

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

On 16.11.2022 21:53:06, Markus Schneider-Pargmann wrote:
> TCAN4X5X_ERROR_STATUS is not a status register that needs clearing
> during interrupt handling. Instead this is a masking register that masks
> error interrupts. Writing TCAN4X5X_CLEAR_ALL_INT to this register
> effectively masks everything.
> 
> Rename the register and mask all error interrupts only once by writing
> to the register in tcan4x5x_init.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

please add a fixes tag.

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

[PATCH 14/15] can: tcan4x5x: Fix register range of first block

[Markus Schneider-Pargmann]

According to the datasheet 0x1c is the last register in the first block,
not register 0x2c.

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

diff --git a/drivers/net/can/m_can/tcan4x5x-regmap.c b/drivers/net/can/m_can/tcan4x5x-regmap.c
index 26e212b8ca7a..d4b79d2d4598 100644
--- a/drivers/net/can/m_can/tcan4x5x-regmap.c
+++ b/drivers/net/can/m_can/tcan4x5x-regmap.c
@@ -91,7 +91,7 @@ static int tcan4x5x_regmap_read(void *context,
 }
 
 static const struct regmap_range tcan4x5x_reg_table_yes_range[] = {
-	regmap_reg_range(0x0000, 0x002c),	/* Device ID and SPI Registers */
+	regmap_reg_range(0x0000, 0x001c),	/* Device ID and SPI Registers */
 	regmap_reg_range(0x0800, 0x083c),	/* Device configuration registers and Interrupt Flags*/
 	regmap_reg_range(0x1000, 0x10fc),	/* M_CAN */
 	regmap_reg_range(0x8000, 0x87fc),	/* MRAM */
-- 
2.38.1

Re: [PATCH 14/15] can: tcan4x5x: Fix register range of first block

[Marc Kleine-Budde]

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

On 16.11.2022 21:53:07, Markus Schneider-Pargmann wrote:
> According to the datasheet 0x1c is the last register in the first block,
> not register 0x2c.

The datasheet "SLLSF91A – DECEMBER 2018 – REVISED JANUARY 2020" says:

| 8.6.1 Device ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to
| 16'h002F

While the last described register is at 0xc.

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 14/15] can: tcan4x5x: Fix register range of first block

[Markus Schneider-Pargmann]

Hi Marc,

On Fri, Dec 02, 2022 at 03:28:10PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:53:07, Markus Schneider-Pargmann wrote:
> > According to the datasheet 0x1c is the last register in the first block,
> > not register 0x2c.
> 
> The datasheet "SLLSF91A – DECEMBER 2018 – REVISED JANUARY 2020" says:
> 
> | 8.6.1 Device ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to
> | 16'h002F
> 
> While the last described register is at 0xc.

Sorry, not sure what I looked up here. The last described register is
0x10 SPI Error status mask in my datasheet:
'SLLSEZ5D – JANUARY 2018 – REVISED JUNE 2022'

I would prefer using the actual registers if that is ok with you, so
0x10 here because I assume the remaining registers have internal use or
maybe don't exist at all?! If there is an undocumented register that
needs to be used at some point we can still modify the ranges.

Also it seems the existing ranges are following the same logic and don't
list the whole range, just the documented registers.

The second range is wrong as well. The last register is 0x830, will fix.

Best,
Markus

Re: [PATCH 14/15] can: tcan4x5x: Fix register range of first block

[Marc Kleine-Budde]

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

On 05.12.2022 10:30:13, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Fri, Dec 02, 2022 at 03:28:10PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:53:07, Markus Schneider-Pargmann wrote:
> > > According to the datasheet 0x1c is the last register in the first block,
> > > not register 0x2c.
> > 
> > The datasheet "SLLSF91A – DECEMBER 2018 – REVISED JANUARY 2020" says:
> > 
> > | 8.6.1 Device ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to
> > | 16'h002F
> > 
> > While the last described register is at 0xc.
> 
> Sorry, not sure what I looked up here. The last described register is
> 0x10 SPI Error status mask in my datasheet:
> 'SLLSEZ5D – JANUARY 2018 – REVISED JUNE 2022'

The TCAN4550-Q1 variant has the 0x10 register documented, while the
TCAN4550 (w/o -Q1) doesn't have.

> I would prefer using the actual registers if that is ok with you, so
> 0x10 here because I assume the remaining registers have internal use or
> maybe don't exist at all?! If there is an undocumented register that
> needs to be used at some point we can still modify the ranges.

I'm fine with using 0x10 as the last register.

> Also it seems the existing ranges are following the same logic and don't
> list the whole range, just the documented registers.
> 
> The second range is wrong as well. The last register is 0x830, will
> fix.

IIRC I used the register ranges from the section titles ("8.6.1 Device
ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to 16'h002F") when
I added the {wr,rd}_table.

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 14/15] can: tcan4x5x: Fix register range of first block

[Markus Schneider-Pargmann]

On Mon, Dec 05, 2022 at 10:44:58AM +0100, Marc Kleine-Budde wrote:
> On 05.12.2022 10:30:13, Markus Schneider-Pargmann wrote:
> > Hi Marc,
> > 
> > On Fri, Dec 02, 2022 at 03:28:10PM +0100, Marc Kleine-Budde wrote:
> > > On 16.11.2022 21:53:07, Markus Schneider-Pargmann wrote:
> > > > According to the datasheet 0x1c is the last register in the first block,
> > > > not register 0x2c.
> > > 
> > > The datasheet "SLLSF91A – DECEMBER 2018 – REVISED JANUARY 2020" says:
> > > 
> > > | 8.6.1 Device ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to
> > > | 16'h002F
> > > 
> > > While the last described register is at 0xc.
> > 
> > Sorry, not sure what I looked up here. The last described register is
> > 0x10 SPI Error status mask in my datasheet:
> > 'SLLSEZ5D – JANUARY 2018 – REVISED JUNE 2022'
> 
> The TCAN4550-Q1 variant has the 0x10 register documented, while the
> TCAN4550 (w/o -Q1) doesn't have.

Ah haven't noticed, thank you.

> 
> > I would prefer using the actual registers if that is ok with you, so
> > 0x10 here because I assume the remaining registers have internal use or
> > maybe don't exist at all?! If there is an undocumented register that
> > needs to be used at some point we can still modify the ranges.
> 
> I'm fine with using 0x10 as the last register.
> 
> > Also it seems the existing ranges are following the same logic and don't
> > list the whole range, just the documented registers.
> > 
> > The second range is wrong as well. The last register is 0x830, will
> > fix.
> 
> IIRC I used the register ranges from the section titles ("8.6.1 Device
> ID and Interrupt/Diagnostic Flag Registers: 16'h0000 to 16'h002F") when
> I added the {wr,rd}_table.

The second range in the driver was specified as 0x800-0x83c in the
driver. The last documented register is 0x830 in both normal and Q1
versions while the range in the title is 0x800-0x8ff. That's why I
thought it was using the last register, just because it is closer.

Anyways not really important.

I can put in whatever you feel comfortable with or keep as it is.

Thanks,
Markus

[PATCH 15/15] can: tcan4x5x: Specify separate read/write ranges

[Markus Schneider-Pargmann]

Specify exactly which registers are read/writeable in the chip. This
is supposed to help detect any violations in the future.

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

diff --git a/drivers/net/can/m_can/tcan4x5x-regmap.c b/drivers/net/can/m_can/tcan4x5x-regmap.c
index d4b79d2d4598..19215c39cd5b 100644
--- a/drivers/net/can/m_can/tcan4x5x-regmap.c
+++ b/drivers/net/can/m_can/tcan4x5x-regmap.c
@@ -90,16 +90,47 @@ static int tcan4x5x_regmap_read(void *context,
 	return 0;
 }
 
-static const struct regmap_range tcan4x5x_reg_table_yes_range[] = {
+static const struct regmap_range tcan4x5x_reg_table_wr_range[] = {
+	/* Device ID and SPI Registers */
+	regmap_reg_range(0x000c, 0x001c),
+	/* 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 */
+	regmap_reg_range(0x100c, 0x102c),
+	regmap_reg_range(0x1048, 0x1048),
+	regmap_reg_range(0x1050, 0x105c),
+	regmap_reg_range(0x1080, 0x1088),
+	regmap_reg_range(0x1090, 0x1090),
+	regmap_reg_range(0x1098, 0x10a0),
+	regmap_reg_range(0x10a8, 0x10b0),
+	regmap_reg_range(0x10b8, 0x10c0),
+	regmap_reg_range(0x10c8, 0x10c8),
+	regmap_reg_range(0x10d0, 0x10d4),
+	regmap_reg_range(0x10e0, 0x10e4),
+	regmap_reg_range(0x10f0, 0x10f0),
+	regmap_reg_range(0x10f8, 0x10f8),
+	/* MRAM */
+	regmap_reg_range(0x8000, 0x87fc),
+};
+
+static const struct regmap_range tcan4x5x_reg_table_rd_range[] = {
 	regmap_reg_range(0x0000, 0x001c),	/* Device ID and SPI Registers */
 	regmap_reg_range(0x0800, 0x083c),	/* Device configuration registers and Interrupt Flags*/
 	regmap_reg_range(0x1000, 0x10fc),	/* M_CAN */
 	regmap_reg_range(0x8000, 0x87fc),	/* MRAM */
 };
 
-static const struct regmap_access_table tcan4x5x_reg_table = {
-	.yes_ranges = tcan4x5x_reg_table_yes_range,
-	.n_yes_ranges = ARRAY_SIZE(tcan4x5x_reg_table_yes_range),
+static const struct regmap_access_table tcan4x5x_reg_table_wr = {
+	.yes_ranges = tcan4x5x_reg_table_wr_range,
+	.n_yes_ranges = ARRAY_SIZE(tcan4x5x_reg_table_wr_range),
+};
+
+static const struct regmap_access_table tcan4x5x_reg_table_rd = {
+	.yes_ranges = tcan4x5x_reg_table_rd_range,
+	.n_yes_ranges = ARRAY_SIZE(tcan4x5x_reg_table_rd_range),
 };
 
 static const struct regmap_config tcan4x5x_regmap = {
@@ -107,8 +138,8 @@ static const struct regmap_config tcan4x5x_regmap = {
 	.reg_stride = 4,
 	.pad_bits = 8,
 	.val_bits = 32,
-	.wr_table = &tcan4x5x_reg_table,
-	.rd_table = &tcan4x5x_reg_table,
+	.wr_table = &tcan4x5x_reg_table_wr,
+	.rd_table = &tcan4x5x_reg_table_rd,
 	.max_register = TCAN4X5X_MAX_REGISTER,
 	.cache_type = REGCACHE_NONE,
 	.read_flag_mask = (__force unsigned long)
-- 
2.38.1

Re: [PATCH 00/15] can: m_can: Optimizations for tcan and peripheral chips

[Marc Kleine-Budde]

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

On 16.11.2022 21:52:53, Markus Schneider-Pargmann wrote:
> Hi,
> 
> this series is aimed at optimizing the driver code for tcan chips and
> more generally for peripheral m_can chips.
> 
> I did different things to improve the performance:
> - Reduce the number of SPI transfers.
> - Reduce the number of interrupts.
> - Enable use of FIFOs.
> 
> I am working with a tcan4550 in loopback mode attached to a beaglebone
> black. I am currently working on optimizing the receive path as well
> which will be submitted in another series once it is done.

The patches I've not commented on look fine. If you re-spin the series
only containing those, I'll include them in my next pull request, which
I'll send out soonish.

regards,
Marc

> Best,
> Markus
> 
> Markus Schneider-Pargmann (15):
>   can: m_can: Eliminate double read of TXFQS in tx_handler
>   can: m_can: Wakeup net queue once tx was issued
>   can: m_can: Cache tx putidx and transmits in flight
>   can: m_can: Use transmit event FIFO watermark level interrupt
>   can: m_can: Disable unused interrupts
>   can: m_can: Avoid reading irqstatus twice
>   can: m_can: Read register PSR only on error
>   can: m_can: Count TXE FIFO getidx in the driver
>   can: m_can: Count read getindex in the driver
>   can: m_can: Batch acknowledge rx fifo
>   can: m_can: Batch acknowledge transmit events
>   can: tcan4x5x: Remove invalid write in clear_interrupts
>   can: tcan4x5x: Fix use of register error status mask
>   can: tcan4x5x: Fix register range of first block
>   can: tcan4x5x: Specify separate read/write ranges
> 
>  drivers/net/can/m_can/m_can.c           | 140 +++++++++++++++---------
>  drivers/net/can/m_can/m_can.h           |   5 +
>  drivers/net/can/m_can/tcan4x5x-core.c   |  19 ++--
>  drivers/net/can/m_can/tcan4x5x-regmap.c |  45 ++++++--
>  4 files changed, 141 insertions(+), 68 deletions(-)
> 
> 
> base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
> prerequisite-patch-id: e9df6751d43bb0d1e3b8938d7e93bc1cfa22cef2
> prerequisite-patch-id: dad9ec37af766bcafe54cb156f896267a0f47fe1
> prerequisite-patch-id: f4e6f1a213a31df2741a5fa3baa87aa45ef6707a

BTW: I don't have access to these prerequisite-patch-id.

-- 
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 00/15] can: m_can: Optimizations for tcan and peripheral chips

[Markus Schneider-Pargmann]

Good Morning Marc,

On Fri, Dec 02, 2022 at 03:03:06PM +0100, Marc Kleine-Budde wrote:
> On 16.11.2022 21:52:53, Markus Schneider-Pargmann wrote:
> > Hi,
> > 
> > this series is aimed at optimizing the driver code for tcan chips and
> > more generally for peripheral m_can chips.
> > 
> > I did different things to improve the performance:
> > - Reduce the number of SPI transfers.
> > - Reduce the number of interrupts.
> > - Enable use of FIFOs.
> > 
> > I am working with a tcan4550 in loopback mode attached to a beaglebone
> > black. I am currently working on optimizing the receive path as well
> > which will be submitted in another series once it is done.
> 
> The patches I've not commented on look fine. If you re-spin the series
> only containing those, I'll include them in my next pull request, which
> I'll send out soonish.

Ok, thank you, I will send a subset of the patches today.

> 
> regards,
> Marc
> 
> > Best,
> > Markus
> > 
> > Markus Schneider-Pargmann (15):
> >   can: m_can: Eliminate double read of TXFQS in tx_handler
> >   can: m_can: Wakeup net queue once tx was issued
> >   can: m_can: Cache tx putidx and transmits in flight
> >   can: m_can: Use transmit event FIFO watermark level interrupt
> >   can: m_can: Disable unused interrupts
> >   can: m_can: Avoid reading irqstatus twice
> >   can: m_can: Read register PSR only on error
> >   can: m_can: Count TXE FIFO getidx in the driver
> >   can: m_can: Count read getindex in the driver
> >   can: m_can: Batch acknowledge rx fifo
> >   can: m_can: Batch acknowledge transmit events
> >   can: tcan4x5x: Remove invalid write in clear_interrupts
> >   can: tcan4x5x: Fix use of register error status mask
> >   can: tcan4x5x: Fix register range of first block
> >   can: tcan4x5x: Specify separate read/write ranges
> > 
> >  drivers/net/can/m_can/m_can.c           | 140 +++++++++++++++---------
> >  drivers/net/can/m_can/m_can.h           |   5 +
> >  drivers/net/can/m_can/tcan4x5x-core.c   |  19 ++--
> >  drivers/net/can/m_can/tcan4x5x-regmap.c |  45 ++++++--
> >  4 files changed, 141 insertions(+), 68 deletions(-)
> > 
> > 
> > base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
> > prerequisite-patch-id: e9df6751d43bb0d1e3b8938d7e93bc1cfa22cef2
> > prerequisite-patch-id: dad9ec37af766bcafe54cb156f896267a0f47fe1
> > prerequisite-patch-id: f4e6f1a213a31df2741a5fa3baa87aa45ef6707a
> 
> BTW: I don't have access to these prerequisite-patch-id.

I think I messed up here. I have three patches, SPI fixes and devicetree
snippet that this series is based on. I guess I shouldn't have used
--base then or rebase on something without these patches first.

Thanks,
Markus