RE: [PATCH 2/2] serial: sc16is7xx: set TX FIFO trigger level to half FIFO to prevent underruns

[Maarten Brock]

> From: David Laight <david.laight.linux@xxxxxxxxx>
> Sent: Wednesday 1 July 2026 19:42
> 
> On Wed, 1 Jul 2026 18:40:56 +0200
> Paul Mbewe <paultyson.mbewe@xxxxxxxxxxxxxx> wrote:
> 
> > Hi David, Maarten,
> >
> > I rechecked this with ftrace, and I think the right explanation is
> > narrower than my v1 commit message.
> >
> > The underrun trace shows the SC16IS7xx refill path running with the
> > hardware TX FIFO already empty while the tty xmit FIFO still had data
> > queued, for example:
> >
> >   sc16is7xx delayed refill: line=0 txlvl=64 pending=172
> >
> > So the conclusions for me are:
> >
> >   - The driver can catch up if the FIFO is only partially drained when
> >     the THRI path runs. TXLVL will report more than the nominal trigger
> >     amount and the driver can write more bytes.
> >
> >   - Once TXLVL is 64, the FIFO has already emptied and the TX gap has
> >     already occurred.
> 
> Right, but there must be something happening after the previous time the
> fifo was filled that makes this one happen too late.
> Either the hardware ISR was very late or it didn't schedule the thread.
> 
> Thinks.....
> Maybe you are getting a fifo empty interrupt (rather than the fifo threshold)
> interrupt, with the threshold one getting lost.
> So what happens if the threshold is reached while the threaded ISR is still
> running?
> When that happens the ISR needs to loop, either in software or by re-enabling
> a level-sensitive interrupt so that another hardware interrupt happens
> immediately.

The loop is already present in sc16is7xx_irq() with 
do { ... } while (keep_polling);

> I'm not at all sure how that works with threaded ISR on the SPI interface.
> Especially since you really don't want to read any more SPI registers
> than absolutely necessary (you need to avoid PCIe reads as well!).
> 
> For a 'normal' ethernet driver rx path it is important to check the next
> rx ring entry a second time after enabling the hardware interrupt.
> Otherwise you miss a wakeup.
> I think you are getting (effectively) the same problem.
> 
> You might need to use (say) ktime_get_ns() to work out how many bytes
> were transmitted between the fifo space being read and the end of the
> fifo writes to see if you need to do a second refill.
> (Or, rather, to detect that you definitely don't need to do one.)
> 
> With the threshold set to 32 it is much less likely that it will be hit
> during the fifo writes, so you are much less likely to lose an interrupt.
> But scheduling delays could still make it happen.
> 
> 	David
> 
> >
> >   - The v1 explanation was wrong to describe a 32-free-space trigger as
> >     increasing the per-interrupt refill window. It does not. With the
> >     default trigger, the FIFO has about 56 character times left when THRI
> >     fires; with a 32-free-space trigger, it has about 32 character times
> >     left.
> >
> >   - As Maarten pointed out, the benefit is the other side of the
> >     trade-off: the refill event rate is reduced by 4x, from roughly
> >     1 refill per 8 transmitted bytes to 1 refill per 32 transmitted bytes.
> >
> >   - On the tested SPI-backed system, that lower refill rate reduces the
> >     IRQ/SPI load enough to avoid the observed underruns.
> >
> > The change is limited to SPI-attached parts so the I2C case is
> > unaffected.
> >
> > For v2 I will:
> >
> >   - rework the commit message around Maarten's trade-off calculation:
> >     4x fewer TX refill events, at the cost of reducing the time-to-empty
> >     margin from 56 to 32 character times;
> >   - limit the TX trigger change to SPI-backed SC16IS7xx devices;
> >   - leave the RX trigger level unchanged.

I see no reason to limit this to SPI or to the transmit case only.

> > If maintainers would prefer this to be board-configurable instead, I can
> > look at a follow-up DT binding/property, but I would prefer to keep v2
> > focused on the TX/SPI case.
> >
> > Thanks,
> > Paul

Further, I think this should be combined with this recent patch proposal from Crescent Hsieh:
[PATCH v2 14/15] serial: 8250: allow UART drivers to override rx_trig_bytes handling

which introduces a way to set/get the rxtrig count per port. A similar thing can be added for txtrig.

Kind regards,
Maarten Brock