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: Fri, 2 Dec 2022 15:03:52 +0100	[thread overview]
Message-ID: <20221202140352.ncbgecrmkoa73mmz@pengutronix.de> (raw)
In-Reply-To: <20221202094346.u2nasxlcwh7llwe5@blmsp>

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