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:

quoted

On 01.12.2022 11:12:20, Markus Schneider-Pargmann wrote:

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

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.

quoted

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 |