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

[David Laight <david.laight.linux@gmail.com>]

On Thu, 2 Jul 2026 12:17:01 +0000
Maarten Brock <Maarten.Brock@sttls.nl> wrote:

> > From: David Laight <david.laight.linux@gmail.com>
> > Sent: Wednesday 1 July 2026 19:42
> > 
> > On Wed, 1 Jul 2026 18:40:56 +0200
> > Paul Mbewe <paultyson.mbewe@ziehl-abegg.de> 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);

That loop also processes multiple interrupt sources.

I've been looking at the code (but don't have the data sheet).
I'd look at what:
	sc16is7xx_port_read(port, SC16IS7XX_TXLVL_REG);
returns both at the bottom of sc16is7xx_handle_tx() and much later
when the IER is re-written by the other work item.
You might find trace_printf() useful - writes into the ftrace buffer.

Did ftrace show you a hardware interrupt at the time when the fifo would
have been expected to have 8 free entries?
Also how many byte times later was the interrupt (real and threaded) that
found the fifo empty?

I also noticed that the code re-enabled the tx interrupt at the end of
the threaded isr - is this actually needed? or is it just there because
the same code is called to send the first buffer.
If this is needed then it is possible that it won't generate an interrupt
if the fifo already has enough space.

There is also a question of how the hardware interrupt itself is handled.
I think there will be a single level-sensitive output from the device
itself.
This will trigger the hardware interrupt which then schedules the threaded
isr, but the hardware interrupt has to be masked until the threaded isr
finishes.
When the hardware interrupt is re-enabled you need the hardware ISR
to run if the irq line is still (or again) asserted.
If that is edge triggered things will go wrong in the way you are seeing.

	David


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