Re: 0d2fd5b99c95 - main - ns8250: use LSR_THRE instead of LSR_TEMT for checking tx flush

> Additionally, ns8250_bus_transmit uses ns8250_drain(UART_DRAIN_TRANSMITTER) in broken_txfifo case.

FWIW, I just had to enable the 'hw.broken_txfifo=1' workaround on physical hardware; see [Bug 286703].

Thanks,

Ravi (rpokala@)

-----Original Message-----
From: <owner-src-committers@freebsd.org <mailto:owner-src-committers@freebsd.org>> on behalf of Andriy Gapon <avg@FreeBSD.org <mailto:avg@FreeBSD.org>>
Date: Tuesday, May 20, 2025 at 17:12
To: <mmel@FreeBSD.org <mailto:mmel@FreeBSD.org>>, <src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org>>, <dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org>>, <dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org>>
Subject: Re: git: 0d2fd5b99c95 - main - ns8250: use LSR_THRE instead of LSR_TEMT for checking tx flush


On 20/05/2025 21:28, Michal Meloun wrote:
> 
> 
> On 20.05.2025 16:57, Andriy Gapon wrote:
>> The branch main has been updated by avg:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/? <https://cgit.FreeBSD.org/src/commit/?> 
>> id=0d2fd5b99c95329085d0700a4dd38507a054a50d
>>
>> commit 0d2fd5b99c95329085d0700a4dd38507a054a50d
>> Author: Andriy Gapon <avg@FreeBSD.org <mailto:avg@FreeBSD.org>>
>> AuthorDate: 2024-11-10 11:15:30 +0000
>> Commit: Andriy Gapon <avg@FreeBSD.org <mailto:avg@FreeBSD.org>>
>> CommitDate: 2025-05-20 14:55:18 +0000
>>
>> ns8250: use LSR_THRE instead of LSR_TEMT for checking tx flush


>> LSR_TEMT bit is set if both transmit hold and shift registers are
>> empty, but the flush command flushes only the hold register.
> I don't think that's true. 


I am not sure to which part of the commit message your "that" refers to, so I'll 
try to justify everything.


T_H_R_E - transmitter holding register empty
T_EMPT - transmitter empty


All hardware documentation that I have around describes those bits like that.
We do not have direct control over the shift register, hardware clears it after 
sending.


> Imho, ns8250_flush() is used also before changing
> baud rate, so we need to ensure that all bits are flushed, including the
> transmit register.


That's an interesting point.


My intention was actually to avoid bogus "FCR is broken" message which can 
happen because of a race between the UART transmission and code execution.
I think that LSR_THRE is proper for checking that FCR works.


But to actually detect and ensure that all transmission has completed we should 
use LSR_TEMT like you say.


At the same time, this UART flush is not like stdout flush, of course, where we 
ensure that all buffered data is transmitted. For UART, we just clear the FIFO 
and the holding register. So, I am not sure if polling for empty transmitter is 
important.


Besides, I do not see the code which would flush transmitter when parameters are 
changing.
I can find only two places where UART_FLUSH_TRANSMITTER is passed:
- ns8250_bus_attach
- ns8250_bus_probe


Additionally, ns8250_bus_transmit uses ns8250_drain(UART_DRAIN_TRANSMITTER) in 
broken_txfifo case.


P.S.


Maybe I don't understand the code, but UART_FLUSH_RECEIVER in ns8250_bus_attach 
looks strange to me. It's one thing to flush data while in the loop-back mode, 
but I think that in ns8250_bus_attach the hardware is fully set up to receive 
data from the outside world. So, how can we hope to drain all of it and to 
reliably detect whether FIFO flushing works. I mean that something on the other 
end could be continuously transmitting.


-- 
Andriy Gapon