Re: [RFC PATCH 1/2] serial: imx: Avoid busy polling for transmitter to become empty

[Esben Haabendal <esben@geanix.com>]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 04.04.2024 13:04:27, Esben Haabendal wrote:
>> Marc Kleine-Budde <mkl@pengutronix.de> writes:
>> 
>> > On 03.04.2024 17:22:52, Esben Haabendal wrote:
>> >> Busy polling with readl() is a rather harsh way to wait for a potentially
>> >> long time.
>> >
>> > This read_poll_timeout_atomic() is compiled to an
>> > imx_uart_readl()/udelay()/cpu_relax() loop. Does the introduction of
>> > udelay() bring any advantages?
>> 
>> Good point. Probably not. I can set sleep_us 0 to go back to a tight
>> loop.
>
> Sounds good

I will do that for v2 then.

>> >> While there, introduce a 10 ms timeout on this waiting, similar to what
>> >> many other serial drivers do.
>> >
>> > But you don't handle the return value...
>> 
>> True. But this is similar to all the different wait_for_xmitr()
>> functions, which does basically the same. They are all void, so the
>> timeout is handled in same happy-go-lucky style.
>
> IMHO the patch description should mention that the driver now ignores
> the state of the transmitter after the timeout.

Ok. Will do.

>> I think the best we could do would be to show an error message. But
>> maybe that is not the most sane thing to do to report a problem with
>> writing error messages. I don't know, but maybe that is why most the
>> other serial drivers are handling it like this.
>
> Writing an error message within the console driver could lead to a
> positive feedback loop :)

Yes, probably best to just silently ignore it.

>> In fsl_lpuart.c and uartlite.c a warning message is printed if/when this
>> timeout occurs. I am fine with doing that here as well...
>> 
>> On a related note. I am unsure if 10 ms is a good choice for timeout. I
>> picked it because it seems like a common value used in many/most
>> drivers. But at least some drivers use something like 1 s, which to me
>> sounds more sane given that we cannot do any meaningful error handling
>> on timeout.
>
> Not having any experience with console drivers, I think the time to
> empty the FIFO depends on the size of the TX FIFO and the speed of the
> UART.
>
> With some numbers (FIFO size and UART speed) pulled out of thin air (and
> neglecting start/stop/parity bits):
>
>     32 bytes * 8 bit/byte / 9600 bit/s = 26.7ms

I assume that typical console usage will have messages much larger than
32 bytes. But on the other hand, most use cases will be 115200 bit/s.

But in general, I would be more comofortable with a 1 second timeout. It
should be more than large enough to handle all realistic cases. But
will avoid spinning forever if uart for some reason does never clear the
TXD bit.

/Esben