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

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Markus Schneider-Pargmann <msp@baylibre.com>
Cc: Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
	Wolfgang Grandegger <wg@grandegger.com>,
	linux-can@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/15] can: m_can: Use transmit event FIFO watermark level interrupt
Date: Thu, 1 Dec 2022 10:05:08 +0100	[thread overview]
Message-ID: <20221201090508.jh5iymwmhs3orb2v@pengutronix.de> (raw)
In-Reply-To: <20221201082521.3tqevaygz4nhw52u@blmsp>

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