From: David Laight <david.laight.linux@gmail.com>
To: Paul Mbewe <paultyson.mbewe@ziehl-abegg.de>
Cc: <Maarten.Brock@sttls.nl>, <linux-serial@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <gregkh@linuxfoundation.org>,
<jirislaby@kernel.org>, <hvilleneuve@dimonoff.com>,
<tobias.gannert@ziehl-abegg.de>, <joachim.knorr@ziehl-abegg.de>
Subject: Re: [PATCH 2/2] serial: sc16is7xx: set TX FIFO trigger level to half FIFO to prevent underruns
Date: Wed, 1 Jul 2026 18:41:44 +0100 [thread overview]
Message-ID: <20260701184144.62bac61e@pumpkin> (raw)
In-Reply-To: <20260701164056.951230-1-paultyson.mbewe@ziehl-abegg.de>
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.
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.
>
> 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
>
> _______________________________________
>
> ZIEHL-ABEGG
>
> Executive Board: Joachim Ley (Chairman), Marco Altherr, Wolfgang Mayer
> Supervisory Board: Dennis Ziehl (Chairman)
>
> Court of Registry: District Court Stuttgart HRB 746188
> Company Seat: Künzelsau, Germany
>
> Der Inhalt dieser E-Mail und/oder jegliche Anhänge können vertrauliche Mitteilungen enthalten und sind ausschließlich für den bezeichneten Adressaten bestimmt.
> Wenn Sie nicht der vorgesehene Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, dass jede Form der Kenntnisnahme, Veröffentlichung,
> Vervielfältigung oder Weitergabe des Inhalts dieser E-Mail einschließlich Anhängen unzulässig ist.
> Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.
>
> The content of this e-mail and/or attachments may contain confidential information and is intended solely for the named recipient.
> If you are not the intended recipient of this e-mail or on its distribution list, please note that any type of disclosure, publication,
> copying or distribution of the content of this e-mail including attachments is strictly forbidden.
> In this case, we would kindly ask you to notify the sender of the e-mail.
next prev parent reply other threads:[~2026-07-01 17:41 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-23 11:22 [PATCH 0/2] serial: sc16is7xx: fix TX inter-frame gaps on SPI UARTs Paul Mbewe
2026-06-23 11:22 ` [PATCH 1/2] serial: sc16is7xx: fix TX gap caused by kfifo circular buffer wrap-around Paul Mbewe
2026-06-23 11:22 ` [PATCH 2/2] serial: sc16is7xx: set TX FIFO trigger level to half FIFO to prevent underruns Paul Mbewe
2026-06-23 12:45 ` David Laight
2026-06-23 14:01 ` Paul Mbewe
2026-06-23 15:07 ` David Laight
2026-06-23 17:13 ` Paul Mbewe
2026-06-23 18:42 ` David Laight
2026-06-25 12:59 ` Maarten Brock
2026-07-01 16:40 ` Paul Mbewe
2026-07-01 17:41 ` David Laight [this message]
2026-07-02 12:17 ` Maarten Brock
2026-07-02 21:24 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260701184144.62bac61e@pumpkin \
--to=david.laight.linux@gmail.com \
--cc=Maarten.Brock@sttls.nl \
--cc=gregkh@linuxfoundation.org \
--cc=hvilleneuve@dimonoff.com \
--cc=jirislaby@kernel.org \
--cc=joachim.knorr@ziehl-abegg.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=paultyson.mbewe@ziehl-abegg.de \
--cc=tobias.gannert@ziehl-abegg.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox