Date: Thu, 22 May 2025 08:37:49 +0200 From: mmel@FreeBSD.org To: Ravi Pokala <rpokala@freebsd.org>, Andriy Gapon <avg@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: 0d2fd5b99c95 - main - ns8250: use LSR_THRE instead of LSR_TEMT for checking tx flush Message-ID: <13b1bbfe-4a60-4c4a-aa4e-ed168cb1286a@FreeBSD.org> In-Reply-To: <37BF51C0-C631-493F-B3AF-3AA9FC32551B@panasas.com> References: <202505201457.54KEvD1r053951@gitrepo.freebsd.org> <1a11f640-be62-4f4e-b537-70806ac54831@FreeBSD.org> <bc1afdab-72d6-43c1-9603-1eed07cb25e6@FreeBSD.org> <37BF51C0-C631-493F-B3AF-3AA9FC32551B@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 22.05.2025 4:42, Ravi Pokala wrote: >> 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. > > Oups, I take it back and apologize for the noise. The short story is that I was confused by my local uart driver changes and mistook ns8250_flush() for ns8250_drain(). The long story is that I have several control and measurement devices connected via serial port. These need a dynamic baud rate change because they communicate at a low baud rate (for robustness), but perform bulk transfers of measurement data at a higher baud rate. Because of this, I modified the uart ioctl to handle TIOCSETA/W/F/ ioctls which are, of course, implemented by ns8250_drain() and ns8250_flush(). This allows the control application to send a baud rate change message, wait for all data (including the transmit register) to be sent, and then safely change the baud rate. Again,sorry for the noise. > 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. > > I'm not sure if I understand you exactly, but in loop-back mode the RX input is internally connected to the TX output (i.e. disconnected from the outside world). So from that perspective, the FIFO detection and flush seems fine to me. Michal
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?13b1bbfe-4a60-4c4a-aa4e-ed168cb1286a>