| Message ID | 9895c8f9553523d158f47a9718bd24f22dbe0455.1712156846.git.esben@geanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: imx: Switch to nbcon console | expand |
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? > 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... > Signed-off-by: Esben Haabendal <esben@geanix.com> regards, Marc
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. >> 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. 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. 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. /Esben
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 > >> 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. > 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 :) > 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 Marc
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
On 04.04.2024 13:54:22, Esben Haabendal wrote: > >> 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... You mean the read_poll_timeout() in fsl_lpuart.c in lpuart_global_reset()? This looks like a totally different codepath to me. https://elixir.bootlin.com/linux/v6.8/source/drivers/tty/serial/fsl_lpuart.c#L2793 In the write callback the fsl_lpuart.c does a readb(); cpu_relax(); while loop. https://elixir.bootlin.com/linux/v6.8/source/drivers/tty/serial/fsl_lpuart.c#L629 https://elixir.bootlin.com/linux/v6.8/source/drivers/tty/serial/fsl_lpuart.c#L636 The other driver is ma35d1_serial.c and uses a timeout of 10ms and ignored the return value :/ > >> 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. I think it's about the length of the TX FIFO, not the full console message. And yes, while 115200 bps is typical, the corner cases should work too. > 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. makes sense regards, Marc
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 54b760d845c0..16661827277e 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -26,6 +26,7 @@ #include <linux/slab.h> #include <linux/of.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/dma-mapping.h> #include <asm/irq.h> @@ -1995,7 +1996,7 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count) struct imx_port *sport = imx_uart_ports[co->index]; struct imx_port_ucrs old_ucr; unsigned long flags; - unsigned int ucr1; + unsigned int ucr1, usr2; int locked = 1; if (sport->port.sysrq) @@ -2026,8 +2027,8 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count) * Finally, wait for transmitter to become empty * and restore UCR1/2/3 */ - while (!(imx_uart_readl(sport, USR2) & USR2_TXDC)); - + read_poll_timeout_atomic(imx_uart_readl, usr2, usr2 & USR2_TXDC, + 1, 10000, false, sport, USR2); imx_uart_ucrs_restore(sport, &old_ucr); if (locked)
Busy polling with readl() is a rather harsh way to wait for a potentially long time. While there, introduce a 10 ms timeout on this waiting, similar to what many other serial drivers do. Signed-off-by: Esben Haabendal <esben@geanix.com> --- drivers/tty/serial/imx.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)