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

[RFC PATCH v2 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?

v2:
- Switch to tight loop (no udelay()) in atomic context.
- Increase timeout to 1 second.
- Add note in commit message about (no) error handling on timeout.


Esben Haabendal (2):
  serial: imx: Introduce timeout when waiting on transmitter 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 v2 1/2] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
deadlock.

In case of the timeout, there is not much we can do, so we simply ignore
the transmitter state and optimistically try to continue.

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..f7e4f38f08f3 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,
+				 0, 1000000, false, sport, USR2);
 	imx_uart_ucrs_restore(sport, &old_ucr);
 
 	if (locked)
-- 
2.44.0

[RFC PATCH v2 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 f7e4f38f08f3..2a49880c2651 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,
+				 0, 1000000, 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, 1000000, 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

Re: [RFC PATCH v2 1/2] serial: imx: Introduce timeout when waiting on transmitter empty

[Marc Kleine-Budde]

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

On 05.04.2024 09:56:37, Esben Haabendal wrote:
> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
> deadlock.
>
> In case of the timeout, there is not much we can do, so we simply ignore
> the transmitter state and optimistically try to continue.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

For the next round you should consult "scripts/get_maintainer.pl
drivers/tty/serial/imx.c" and add these people/lists on Cc.

> ---
>  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..f7e4f38f08f3 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,
> +				 0, 1000000, false, sport, USR2);

You can make use of USEC_PER_SEC here.

After fixing this, feel free to add my:

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

>  	imx_uart_ucrs_restore(sport, &old_ucr);
>  
>  	if (locked)
> -- 
> 2.44.0
> 
> 

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 v2 1/2] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

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

> On 05.04.2024 09:56:37, Esben Haabendal wrote:
>> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
>> deadlock.
>>
>> In case of the timeout, there is not much we can do, so we simply ignore
>> the transmitter state and optimistically try to continue.
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>
> For the next round you should consult "scripts/get_maintainer.pl
> drivers/tty/serial/imx.c" and add these people/lists on Cc.

Yes. The idea was to do the RFC here on linux-rt, with focus on the 2nd
patch in the series. When that is in good shape, I will send the first
patch separately to all the proper maintainers and lists.

>> ---
>>  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..f7e4f38f08f3 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,
>> +				 0, 1000000, false, sport, USR2);
>
> You can make use of USEC_PER_SEC here.
>
> After fixing this, feel free to add my:
>
> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

Thanks.

/Esben

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

[Marc Kleine-Budde]

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

On 05.04.2024 09:56:36, Esben Haabendal wrote:
> 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?

Some additional thoughts:
- Code duplication should be avoided.
- You can remove the first "ifdef" if you mark the functions as __maybe_unused.
- Use get_maintainer.pl to address all relevant parties and discuss if
  you should keep the legacy console. You should do this next, as the
  above gets irrelevant if the legacy console can be removed.

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 v2 2/2] serial: imx: Switch to nbcon console

[John Ogness]

Hi Esben,

You are correct that there is considerable copy/paste from the 8250
driver. Hopefully having another nbcon driver will help us find a way to
cleanly consolidate that logic. Thanks for your work on this.

Comments from me below...

On 2024-04-05, Esben Haabendal <esben@geanix.com> wrote:
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index f7e4f38f08f3..2a49880c2651 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> +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,
> +				 0, 1000000, false, sport, USR2);
> +	imx_uart_ucrs_restore(sport, &old_ucr);
> +
> +	/* Success if no handover/takeover. */
> +	return nbcon_exit_unsafe(wctxt);
> +}

In your imx_uart_console_write_atomic() I see lots of register usage:

ucr1, ucr2, ucr3, usr2, uts

It is critical that _all_ usage of these registers throughout the driver
is protected, preferably protected by the port lock. Please go through
and verify that. If imx_uart_console_write_thread() is using even more
registers, you will need to check those as well.

For the 8250 I went through all uses and found several problems [0]. The
imx driver may have similar issues.

John Ogness

[0] https://lore.kernel.org/lkml/20230525093159.223817-1-john.ogness@linutronix.de