Hi Maciej,
Thanks for jumping in with some ref-manual quotes. Some more comments
from me below...
On 2024-10-31, "Maciej W. Rozycki" <macro@xxxxxxxxxxx> wrote:
> On Wed, 30 Oct 2024, Jiri Slaby wrote:
>> > @@ -3306,13 +3310,18 @@ static void serial8250_console_restore(struct
>> > uart_8250_port *up)
>> > static void serial8250_console_fifo_write(struct uart_8250_port *up,
>> > const char *s, unsigned int count)
>> > {
>> > - int i;
>> > const char *end = s + count;
>> > unsigned int fifosize = up->tx_loadsz;
>> > + unsigned int tx_count = 0;
>> > bool cr_sent = false;
>> > + unsigned int i;
>> > while (s != end) {
>> > - wait_for_lsr(up, UART_LSR_THRE);
>> > + /* Allow timeout for each byte of a possibly full FIFO. */
>> > + for (i = 0; i < fifosize; i++) {
>> > + if (wait_for_lsr(up, UART_LSR_THRE))
>> > + break;
>> > + }
>>
>> THRE only signals there is a space for one character.
>
> Nope[1]:
>
> "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is
> cleared when at least one byte is written to the transmit FIFO."
>
> It seems common enough a misconception that once I actually had to fix the
> bad interpretation of THRE in an unpublished platform driver to get decent
> performance out of it at higher rates such as 230400bps, as it only pushed
> one byte at a time to the FIFO while it had it all available once THRE has
> been set.
I do not know if this is true for all 8250-variants. If there is some
variant where it functions as Jiri expected, then it would mean
significant text loss during longer messages. But that would already be
a problem in the current mainline driver.
>> > + /* Allow timeout for each byte written. */
>> > + for (i = 0; i < tx_count; i++) {
>> > + if (wait_for_lsr(up, UART_LSR_THRE))
>>
>> This ensures you sent one character from the FIFO. The FIFO still can contain
>> plenty of them. Did you want UART_LSR_TEMT?
>
> The difference between THRE and TEMT is the state of the shift register
> only[2]:
>
> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift
> register are both empty."
If we wait for TEMT, we lose significant advantages of having the FIFO.
>> But what's the purpose of spinning _here_? The kernel can run and FIFO
>> too. Without the kernel waiting for the FIFO.
When serial8250_console_fifo_write() exits, the caller just does a
single wait_for_xmitr() ... with a 10ms timeout. In the FIFO case, for
<=56k baudrates, it can easily hit the timeout and thus continue before
the FIFO has been emptied.
By waiting on UART_LSR_THRE after filling the FIFO,
serial8250_console_fifo_write() waits until the hardware has had a
chance to shift out all the data. Then the final wait_for_xmitr() in the
caller only waits for the final byte to go out on the line.
Please keep in mind that none of these timeouts should trigger during
normal operation.
For v4 I am doing some refactoring (as suggested by Andy) so that the
wait-code looks a bit cleaner.
John
> References:
>
> [1] "TL16C550C, TL16C550CI Asynchronous Communications Element with
> Autoflow Control", Texas Instruments, SLLS177F -- March 1994 --
> Revised March 2001, p. 30
>
> [2] same
