[RFC PATCH 0/2] serial: imx: Switch to nbcon console

[RFC PATCH 0/2] serial: imx: Switch to nbcon console

[Esben Haabendal]

This is a first attempt at porting another serial console driver to nbcon.

The first patch is a preparatory patch, thought to be suitable for mainline
inclusion now, while the second patch is for rt only for now.

The second patch does have quite a copy-paste feeling to it.

The imx_uart_console_write_atomic() and imx_uart_console_write_thread()
includes copies of code from the old imx_uart_console_write() function.
This is similar to current status of nbcon support in 8250 driver.

The midle part of imx_uart_console_write_thread() is identical to the
middle part of serial8250_console_write_thread() in
drivers/tty/serial/8250/8250_port.c, except for the arguments to
uart_console_write() function.

There is clearly potential for writing some common code to avoid both cases
of copy-paste code.

I am unsure about what the plan is for keeping the legacy console code in
the drivers. Are we going to drop that before submitting the code to
mainline?

Esben Haabendal (2):
  serial: imx: Avoid busy polling for transmitter to become empty
  serial: imx: Switch to nbcon console

 drivers/tty/serial/imx.c | 161 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 4 deletions(-)

-- 
2.44.0

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

[Esben Haabendal]

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(-)

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)
-- 
2.44.0

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Esben Haabendal]

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Esben Haabendal]

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 2/2] serial: imx: Switch to nbcon console

[Esben Haabendal]

Implements the necessary callbacks to switch the imx console driver to
perform as an nbcon console.

Add implementations for the nbcon consoles (write_atomic, write_thread,
driver_enter, driver_exit) and add CON_NBCON to the initial flags.

The legacy code is kept in order to easily switch back to legacy mode
by defining CONFIG_SERIAL_IMX_LEGACY_CONSOLE.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/imx.c | 154 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 16661827277e..7030fa2bcaea 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -230,6 +230,8 @@ struct imx_port {
 	unsigned int            saved_reg[10];
 	bool			context_saved;
 
+	bool			console_newline_needed;
+
 	enum imx_tx_state	tx_state;
 	struct hrtimer		trigger_start_tx;
 	struct hrtimer		trigger_stop_tx;
@@ -1985,8 +1987,14 @@ static void imx_uart_console_putchar(struct uart_port *port, unsigned char ch)
 		barrier();
 
 	imx_uart_writel(sport, ch, URTX0);
+
+	if (ch == '\n')
+		sport->console_newline_needed = false;
+	else
+		sport->console_newline_needed = true;
 }
 
+#ifdef CONFIG_SERIAL_IMX_LEGACY_CONSOLE
 /*
  * Interrupts are disabled on entering
  */
@@ -2034,6 +2042,140 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 	if (locked)
 		uart_port_unlock_irqrestore(&sport->port, flags);
 }
+#else
+static void imx_uart_console_driver_enter(struct console *co, unsigned long *flags)
+{
+	struct uart_port *up = &imx_uart_ports[co->index]->port;
+
+	return __uart_port_lock_irqsave(up, flags);
+}
+
+static void imx_uart_console_driver_exit(struct console *co, unsigned long flags)
+{
+	struct uart_port *up = &imx_uart_ports[co->index]->port;
+
+	return __uart_port_unlock_irqrestore(up, flags);
+}
+
+static bool imx_uart_console_write_atomic(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct imx_port *sport = imx_uart_ports[co->index];
+	struct uart_port *port = &sport->port;
+	struct imx_port_ucrs old_ucr;
+	unsigned int ucr1, usr2;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return false;
+
+	/*
+	 *	First, save UCR1/2/3 and then disable interrupts
+	 */
+	imx_uart_ucrs_save(sport, &old_ucr);
+	ucr1 = old_ucr.ucr1;
+
+	if (imx_uart_is_imx1(sport))
+		ucr1 |= IMX1_UCR1_UARTCLKEN;
+	ucr1 |= UCR1_UARTEN;
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN);
+
+	imx_uart_writel(sport, ucr1, UCR1);
+
+	imx_uart_writel(sport, old_ucr.ucr2 | UCR2_TXEN, UCR2);
+
+	if (sport->console_newline_needed)
+		uart_console_write(port, "\n", 1, imx_uart_console_putchar);
+	uart_console_write(port, wctxt->outbuf, wctxt->len,
+			   imx_uart_console_putchar);
+
+	/*
+	 *	Finally, wait for transmitter to become empty
+	 *	and restore UCR1/2/3
+	 */
+	read_poll_timeout_atomic(imx_uart_readl, usr2, usr2 & USR2_TXDC,
+				 1, 10000, false, sport, USR2);
+	imx_uart_ucrs_restore(sport, &old_ucr);
+
+	/* Success if no handover/takeover. */
+	return nbcon_exit_unsafe(wctxt);
+}
+
+static bool imx_uart_console_write_thread(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct imx_port *sport = imx_uart_ports[co->index];
+	struct uart_port *port = &sport->port;
+	struct imx_port_ucrs old_ucr;
+	unsigned int ucr1, usr2;
+	bool done = false;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return false;
+
+	/*
+	 *	First, save UCR1/2/3 and then disable interrupts
+	 */
+	imx_uart_ucrs_save(sport, &old_ucr);
+	ucr1 = old_ucr.ucr1;
+
+	if (imx_uart_is_imx1(sport))
+		ucr1 |= IMX1_UCR1_UARTCLKEN;
+	ucr1 |= UCR1_UARTEN;
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN);
+
+	imx_uart_writel(sport, ucr1, UCR1);
+
+	imx_uart_writel(sport, old_ucr.ucr2 | UCR2_TXEN, UCR2);
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		int len = READ_ONCE(wctxt->len);
+		int i;
+
+		/*
+		 * Write out the message. Toggle unsafe for each byte in order
+		 * to give another (higher priority) context the opportunity
+		 * for a friendly takeover. If such a takeover occurs, this
+		 * context must reacquire ownership in order to perform final
+		 * actions (such as re-enabling the interrupts).
+		 *
+		 * IMPORTANT: wctxt->outbuf and wctxt->len are no longer valid
+		 *	      after a reacquire so writing the message must be
+		 *	      aborted.
+		 */
+		for (i = 0; i < len; i++) {
+			if (!nbcon_enter_unsafe(wctxt)) {
+				nbcon_reacquire(wctxt);
+				break;
+			}
+
+			uart_console_write(port, wctxt->outbuf + i, 1,
+					   imx_uart_console_putchar);
+
+			if (!nbcon_exit_unsafe(wctxt)) {
+				nbcon_reacquire(wctxt);
+				break;
+			}
+		}
+		done = (i == len);
+	} else {
+		nbcon_reacquire(wctxt);
+	}
+
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire(wctxt);
+
+	/*
+	 *	Finally, wait for transmitter to become empty
+	 *	and restore UCR1/2/3
+	 */
+	read_poll_timeout(imx_uart_readl, usr2, usr2 & USR2_TXDC,
+			  1, 10000, false, sport, USR2);
+	imx_uart_ucrs_restore(sport, &old_ucr);
+
+	/* Success if no handover/takeover and message fully printed. */
+	return (nbcon_exit_unsafe(wctxt) && done);
+}
+#endif /* CONFIG_SERIAL_IMX_LEGACY_CONSOLE */
 
 /*
  * If the port was already initialised (eg, by a boot loader),
@@ -2124,6 +2266,8 @@ imx_uart_console_setup(struct console *co, char *options)
 	if (retval)
 		goto error_console;
 
+	sport->console_newline_needed = false;
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else
@@ -2160,11 +2304,19 @@ imx_uart_console_exit(struct console *co)
 static struct uart_driver imx_uart_uart_driver;
 static struct console imx_uart_console = {
 	.name		= DEV_NAME,
+#ifdef CONFIG_SERIAL_IMX_LEGACY_CONSOLE
 	.write		= imx_uart_console_write,
+	.flags		= CON_PRINTBUFFER,
+#else
+	.write_atomic	= imx_uart_console_write_atomic,
+	.write_thread	= imx_uart_console_write_thread,
+	.driver_enter	= imx_uart_console_driver_enter,
+	.driver_exit	= imx_uart_console_driver_exit,
+	.flags		= CON_PRINTBUFFER | CON_NBCON,
+#endif
 	.device		= uart_console_device,
 	.setup		= imx_uart_console_setup,
 	.exit		= imx_uart_console_exit,
-	.flags		= CON_PRINTBUFFER,
 	.index		= -1,
 	.data		= &imx_uart_uart_driver,
 };
-- 
2.44.0