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

From: Markus Schneider-Pargmann <msp@baylibre.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
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 09:25:21 +0100	[thread overview]
Message-ID: <20221201082521.3tqevaygz4nhw52u@blmsp> (raw)
In-Reply-To: <20221130171715.nujptzwnut7silbm@pengutronix.de>

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