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

[Maarten Brock]

Hello Paul,

> > Does filling the tx buffer clear the IER bit?
> 
> No, filling the TX FIFO does not clear IER[1]. A THR write only clears the
> pending TX interrupt condition/IRQ line, not the enable bit; for SPI this
> is Fig. 33, "SPI write THR to clear TX interrupt".

It would be weird if any enable bit in IER would be changed by hardware.

But I consider it unfortunate that a THR interrupt is not active for any vacancy level over the selected threshold. There should be no need for a threshold crossing. Still, the datasheet of the SC16IS740/50/60 does use this wording for THR interrupt in IIR (my emphasis):

"Transmit FIFO empty (FIFO disable) or TX FIFO *passes* above trigger level (FIFO enable)"

And also the following indicates it is not a level detector:

"Re-enabling IER[1] will not cause a new interrupt if the THR is below the threshold."

> During active TX, sc16is7xx_ier_set(THRI) is therefore just ensuring an
> already-enabled interrupt remains enabled. It may be worth cleaning up
> redundant updates later, but I would leave the current enable/disable policy
> unchanged for the underrun fix.

I'm not so sure this is a redundant update, because the register is not marked volatile in sc16is7xx_regmap_volatile(). So I expect the IER register not to be rewritten. But it probably does unnecessarily wake up the kworker thread.

> > You might also be able to leave the tx interrupt permanently enabled.
> 
> THRI is already kept enabled during an active TX burst and is only cleared
> by sc16is7xx_stop_tx() when the xmit FIFO becomes empty.
> 
> I think leaving it enabled while idle is a separate question. Since a THR
> write clears the pending TX interrupt condition, an empty software FIFO means
> there may be no write to clear a pending TX interrupt. With a level-low IRQ,
> that could cause repeated wakeups while the hardware TX FIFO is empty.
> So disabling IER[1] when the xmit FIFO is empty still looks like the right
> policy.

I agree.

> The cleanup I had in mind is only to avoid redundant 1->1 updates while
> TX is already active.

As stated above, I expect you can save CPU cycles, but no SPI write.

Kind regards,
Maarten