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

[Paul Mbewe]

Hi David, Maarten,

Thanks, I double-checked this with finer TXLVL instrumentation, and the
root cause is clearer now: the failures are stale-TXLVL under-fills, not a
separate THRI re-arm issue.

> You need the refill path to be much faster then the drain path.

Agreed. The issue is that sc16is7xx reads TXLVL once and sizes the FIFO
write from that value. On this SPI-backed path, hardirq/softirq activity,
RT scheduling and the synchronous SPI transfer wait can delay the refill
while the UART continues draining. So the TXLVL value can become stale
before the write completes.

One failing example:

  tx_start       txlvl=9  txlvl_read_us=580
  tx_pre_write   sent=9   pre_write_us=16
  tx_segment     sent=9   seg_us=364
  tx_post_write  txlvl_before=9 sent=9 txlvl_after=12 pending_after=29
                 post_gap_us=12 post_txlvl_us=130

Here TXLVL is free space, and the trigger is 8 free spaces. The driver
read 9 free spaces and wrote 9 bytes, but the post-write read still saw
12 free spaces with data pending. Even allowing for the post-write read
window, the FIFO was not reliably pushed below the trigger, so no new
threshold crossing was expected. All failing samples I have checked show
the same pattern.

> The block of data also isn't guaranteed to fill the hardware fifo,
> it only contains data to the end of the software ring buffer - so
> might only be 1 byte.

Yes, exactly. Patch 1 fixes that by looping over contiguous kfifo segments
instead of stopping after the first linear segment.

Patch 2 fixes the stale TXLVL part: after a FIFO write, re-read TXLVL and
top up again if data remains pending and the FIFO is still above the TX
trigger level. This follows the same principle as max310x: do not base the
whole refill on a single FIFO-level sample.

> Are you sure?
> I'd expect it to be active low and asserted throughout.

Yes, I checked with the scope. In the failing case the trigger-level interrupt
does not assert; the next INT assertion is only the late FIFO-empty event,
where ftrace reads TXLVL=64 with data still pending. So this is not a lost SoC
edge or a held-low level interrupt missed by the interrupt controller.

> I'd look up what causes the IIR to report THRI.

The FIFO has to be refilled below the trigger so that draining can cross it
again; re-enabling THRI alone is not enough. The datasheet notes that
"Re-enabling IER[1] will not cause a new interrupt if the THR is below the
threshold." So if data remains pending and post-write TXLVL is still above
the trigger, the driver has to top up again.

> I also noticed that the code seems to be doing slow RMW cycles at the end
> of every tx to enable/disable the tx interrupt.

Agreed. Mid-frame the trailing sc16is7xx_ier_set(THRI) is only a 1->1
update because THRI is already enabled. It does not generate a fresh event.
I will keep any cleanup of that path separate so the underrun fix stays
focused.

> Something subtle makes it happen less often, but whatever it is can
> still happen.

Yes. The 32 free-space trigger is still useful in practice: it makes each refill
larger and reduces the refill rate. At 115200 8N1, the same stale-refill
condition would need roughly a 2.8 ms delay instead of about 0.7 ms with
the default 8-free-space trigger. So I will keep it as a separate
load-reduction / margin patch, while the TXLVL re-read/top-up remains the
correctness fix.

I have been running the re-read/top-up change overnight and want to get it
validated on another setup as well before sending v2. I will include the
results in the v2 cover letter.

For v2 I plan to split this as:

  1. kfifo wrap-around fix;
  2. TXLVL re-read/top-up fix;
  3. 32 free-space trigger as a separate load-reduction patch.

For follow-up, as discussed with Maarten, I will coordinate with Crescent
Hsieh around the existing rx_trig_bytes work and look at a matching TX
trigger interface. I would keep that separate from v2 so the underrun fix
stays focused.

Thanks
Paul