[PATCH] serial: amba-pl011: Implement nbcon console

[PATCH] serial: amba-pl011: Implement nbcon console

[Toshiyuki Sato]

Implement the callbacks required for an NBCON console [0] on the 
amba-pl011 console driver.
The codes for legacy console are retained, and the module
parameter (use_nbcon) allows switching between legacy and NBCON.
The default is off (use legacy console) for now.

Referred to the NBCON implementation work for 8250 [1] and imx [2].

The normal-priority write_thread checks for console ownership
each time a character is printed.
write_atomic holds the console ownership until the entire string
is printed.

UART register operations are protected from other contexts by 
uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe) 
on panic.

The patch has been verified to correctly handle the output and
competition of messages with different priorities and flushing
panic message to console after nmi panic using ARM64 QEMU and
a physical machine(A64FX).

[0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
[1] https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.de/T/
[2] https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c627302335b@geanix.com/T/

Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
---
 drivers/tty/serial/amba-pl011.c | 113 ++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 69b7a3e1e..52fab3170 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -41,6 +41,7 @@
 #include <linux/sizes.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
+#include <linux/moduleparam.h>
 
 #define UART_NR			14
 
@@ -263,6 +264,7 @@ struct uart_amba_port {
 	char			type[12];
 	bool			rs485_tx_started;
 	unsigned int		rs485_tx_drain_interval; /* usecs */
+	bool			console_line_ended;
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
 	unsigned int		dmacr;		/* dma control reg */
@@ -274,6 +276,10 @@ struct uart_amba_port {
 #endif
 };
 
+/* if non-zero, the console is nbcon. */
+static int use_nbcon;
+module_param(use_nbcon, int, 0444);
+
 static unsigned int pl011_tx_empty(struct uart_port *port);
 
 static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
@@ -2305,6 +2311,7 @@ static void pl011_console_putchar(struct uart_port *port, unsigned char ch)
 	while (pl011_read(uap, REG_FR) & UART01x_FR_TXFF)
 		cpu_relax();
 	pl011_write(ch, uap, REG_DR);
+	uap->console_line_ended = (ch == '\n');
 }
 
 static void
@@ -2411,6 +2418,8 @@ static int pl011_console_setup(struct console *co, char *options)
 	if (ret)
 		return ret;
 
+	uap->console_line_ended = true;
+
 	if (dev_get_platdata(uap->port.dev)) {
 		struct amba_pl011_data *plat;
 
@@ -2494,6 +2503,106 @@ static int pl011_console_match(struct console *co, char *name, int idx,
 	return -ENODEV;
 }
 
+static void
+pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt)
+{
+	struct uart_amba_port *uap = amba_ports[co->index];
+	unsigned int old_cr = 0;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	clk_enable(uap->clk);
+
+	if (!uap->vendor->always_enabled) {
+		old_cr = pl011_read(uap, REG_CR);
+		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
+				uap, REG_CR);
+	}
+
+	if (!uap->console_line_ended)
+		uart_console_write(&uap->port, "\n", 1, pl011_console_putchar);
+	uart_console_write(&uap->port, wctxt->outbuf, wctxt->len, pl011_console_putchar);
+
+	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
+		cpu_relax();
+
+	if (!uap->vendor->always_enabled)
+		pl011_write(old_cr, uap, REG_CR);
+
+	clk_disable(uap->clk);
+
+	nbcon_exit_unsafe(wctxt);
+}
+
+static void
+pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt)
+{
+	struct uart_amba_port *uap = amba_ports[co->index];
+	unsigned int old_cr = 0;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	clk_enable(uap->clk);
+
+	if (!uap->vendor->always_enabled) {
+		old_cr = pl011_read(uap, REG_CR);
+		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
+				uap, REG_CR);
+	}
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		int i;
+		unsigned int len = READ_ONCE(wctxt->len);
+
+		for (i = 0; i < len; i++) {
+			if (!nbcon_enter_unsafe(wctxt))
+				break;
+			uart_console_write(&uap->port, wctxt->outbuf + i, 1, pl011_console_putchar);
+			if (!nbcon_exit_unsafe(wctxt))
+				break;
+		}
+	}
+
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
+
+	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
+		cpu_relax();
+
+	if (!uap->vendor->always_enabled)
+		pl011_write(old_cr, uap, REG_CR);
+
+	clk_disable(uap->clk);
+
+	nbcon_exit_unsafe(wctxt);
+}
+
+static void
+pl011_console_device_lock(struct console *co, unsigned long *flags)
+{
+	__uart_port_lock_irqsave(&amba_ports[co->index]->port, flags);
+}
+
+static void
+pl011_console_device_unlock(struct console *co, unsigned long flags)
+{
+	__uart_port_unlock_irqrestore(&amba_ports[co->index]->port, flags);
+}
+
+static void
+pl011_console_switch_to_nbcon(struct console *co)
+{
+	co->write = NULL;
+	co->write_atomic = pl011_console_write_atomic;
+	co->write_thread = pl011_console_write_thread;
+	co->device_lock = pl011_console_device_lock;
+	co->device_unlock = pl011_console_device_unlock;
+	co->flags = CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON;
+	pr_info("Serial: switched to nbcon\n");
+}
+
 static struct uart_driver amba_reg;
 static struct console amba_console = {
 	.name		= "ttyAMA",
@@ -2737,6 +2846,10 @@ static int pl011_register_port(struct uart_amba_port *uap)
 	pl011_write(0xffff, uap, REG_ICR);
 
 	if (!amba_reg.state) {
+		/* Replaces the console descriptor if NBCON is selected. */
+		if (amba_reg.cons && use_nbcon)
+			pl011_console_switch_to_nbcon(amba_reg.cons);
+
 		ret = uart_register_driver(&amba_reg);
 		if (ret < 0) {
 			dev_err(uap->port.dev,
-- 
2.34.1

Re: [PATCH] serial: amba-pl011: Implement nbcon console

[Greg Kroah-Hartman]

On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> Implement the callbacks required for an NBCON console [0] on the 
> amba-pl011 console driver.
> The codes for legacy console are retained, and the module
> parameter (use_nbcon) allows switching between legacy and NBCON.
> The default is off (use legacy console) for now.
> 
> Referred to the NBCON implementation work for 8250 [1] and imx [2].
> 
> The normal-priority write_thread checks for console ownership
> each time a character is printed.
> write_atomic holds the console ownership until the entire string
> is printed.
> 
> UART register operations are protected from other contexts by 
> uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe) 
> on panic.
> 
> The patch has been verified to correctly handle the output and
> competition of messages with different priorities and flushing
> panic message to console after nmi panic using ARM64 QEMU and
> a physical machine(A64FX).
> 
> [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> [1] https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.de/T/
> [2] https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c627302335b@geanix.com/T/
> 
> Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 113 ++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 69b7a3e1e..52fab3170 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -41,6 +41,7 @@
>  #include <linux/sizes.h>
>  #include <linux/io.h>
>  #include <linux/acpi.h>
> +#include <linux/moduleparam.h>
>  
>  #define UART_NR			14
>  
> @@ -263,6 +264,7 @@ struct uart_amba_port {
>  	char			type[12];
>  	bool			rs485_tx_started;
>  	unsigned int		rs485_tx_drain_interval; /* usecs */
> +	bool			console_line_ended;
>  #ifdef CONFIG_DMA_ENGINE
>  	/* DMA stuff */
>  	unsigned int		dmacr;		/* dma control reg */
> @@ -274,6 +276,10 @@ struct uart_amba_port {
>  #endif
>  };
>  
> +/* if non-zero, the console is nbcon. */
> +static int use_nbcon;
> +module_param(use_nbcon, int, 0444);

Why is a module parameter needed here?  That feels wrong and not
scalable at all.  What happens if you have multiple devices, which one
is nbcon and which isn't?


> +
>  static unsigned int pl011_tx_empty(struct uart_port *port);
>  
>  static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
> @@ -2305,6 +2311,7 @@ static void pl011_console_putchar(struct uart_port *port, unsigned char ch)
>  	while (pl011_read(uap, REG_FR) & UART01x_FR_TXFF)
>  		cpu_relax();
>  	pl011_write(ch, uap, REG_DR);
> +	uap->console_line_ended = (ch == '\n');
>  }
>  
>  static void
> @@ -2411,6 +2418,8 @@ static int pl011_console_setup(struct console *co, char *options)
>  	if (ret)
>  		return ret;
>  
> +	uap->console_line_ended = true;
> +
>  	if (dev_get_platdata(uap->port.dev)) {
>  		struct amba_pl011_data *plat;
>  
> @@ -2494,6 +2503,106 @@ static int pl011_console_match(struct console *co, char *name, int idx,
>  	return -ENODEV;
>  }
>  
> +static void
> +pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt)
> +{
> +	struct uart_amba_port *uap = amba_ports[co->index];
> +	unsigned int old_cr = 0;
> +
> +	if (!nbcon_enter_unsafe(wctxt))
> +		return;
> +
> +	clk_enable(uap->clk);
> +
> +	if (!uap->vendor->always_enabled) {
> +		old_cr = pl011_read(uap, REG_CR);
> +		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
> +				uap, REG_CR);
> +	}
> +
> +	if (!uap->console_line_ended)
> +		uart_console_write(&uap->port, "\n", 1, pl011_console_putchar);
> +	uart_console_write(&uap->port, wctxt->outbuf, wctxt->len, pl011_console_putchar);
> +
> +	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
> +		cpu_relax();
> +
> +	if (!uap->vendor->always_enabled)
> +		pl011_write(old_cr, uap, REG_CR);
> +
> +	clk_disable(uap->clk);
> +
> +	nbcon_exit_unsafe(wctxt);
> +}
> +
> +static void
> +pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt)
> +{
> +	struct uart_amba_port *uap = amba_ports[co->index];
> +	unsigned int old_cr = 0;
> +
> +	if (!nbcon_enter_unsafe(wctxt))
> +		return;
> +
> +	clk_enable(uap->clk);
> +
> +	if (!uap->vendor->always_enabled) {
> +		old_cr = pl011_read(uap, REG_CR);
> +		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
> +				uap, REG_CR);
> +	}
> +
> +	if (nbcon_exit_unsafe(wctxt)) {
> +		int i;
> +		unsigned int len = READ_ONCE(wctxt->len);
> +
> +		for (i = 0; i < len; i++) {
> +			if (!nbcon_enter_unsafe(wctxt))
> +				break;
> +			uart_console_write(&uap->port, wctxt->outbuf + i, 1, pl011_console_putchar);
> +			if (!nbcon_exit_unsafe(wctxt))
> +				break;
> +		}
> +	}
> +
> +	while (!nbcon_enter_unsafe(wctxt))
> +		nbcon_reacquire_nobuf(wctxt);
> +
> +	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
> +		cpu_relax();
> +
> +	if (!uap->vendor->always_enabled)
> +		pl011_write(old_cr, uap, REG_CR);
> +
> +	clk_disable(uap->clk);
> +
> +	nbcon_exit_unsafe(wctxt);
> +}
> +
> +static void
> +pl011_console_device_lock(struct console *co, unsigned long *flags)
> +{
> +	__uart_port_lock_irqsave(&amba_ports[co->index]->port, flags);
> +}
> +
> +static void
> +pl011_console_device_unlock(struct console *co, unsigned long flags)
> +{
> +	__uart_port_unlock_irqrestore(&amba_ports[co->index]->port, flags);
> +}
> +
> +static void
> +pl011_console_switch_to_nbcon(struct console *co)
> +{
> +	co->write = NULL;
> +	co->write_atomic = pl011_console_write_atomic;
> +	co->write_thread = pl011_console_write_thread;
> +	co->device_lock = pl011_console_device_lock;
> +	co->device_unlock = pl011_console_device_unlock;
> +	co->flags = CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON;
> +	pr_info("Serial: switched to nbcon\n");

dev_info()?

thanks,

greg k-h

RE: [PATCH] serial: amba-pl011: Implement nbcon console

[Toshiyuki Sato (Fujitsu)]

Hi, Greg. Thanks for the comment.

> On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> > Implement the callbacks required for an NBCON console [0] on the
> > amba-pl011 console driver.
> > The codes for legacy console are retained, and the module
> > parameter (use_nbcon) allows switching between legacy and NBCON.
> > The default is off (use legacy console) for now.
> >
> > Referred to the NBCON implementation work for 8250 [1] and imx [2].
> >
> > The normal-priority write_thread checks for console ownership
> > each time a character is printed.
> > write_atomic holds the console ownership until the entire string
> > is printed.
> >
> > UART register operations are protected from other contexts by
> > uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe)
> > on panic.
> >
> > The patch has been verified to correctly handle the output and
> > competition of messages with different priorities and flushing
> > panic message to console after nmi panic using ARM64 QEMU and
> > a physical machine(A64FX).
> >
> > [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> > [1]
> https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.d
> e/T/
> > [2]
> https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c62
> 7302335b@geanix.com/T/
> >
> > Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
> > ---
> >  drivers/tty/serial/amba-pl011.c | 113
> ++++++++++++++++++++++++++++++++
> >  1 file changed, 113 insertions(+)
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 69b7a3e1e..52fab3170 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -41,6 +41,7 @@
> >  #include <linux/sizes.h>
> >  #include <linux/io.h>
> >  #include <linux/acpi.h>
> > +#include <linux/moduleparam.h>
> >
> >  #define UART_NR			14
> >
> > @@ -263,6 +264,7 @@ struct uart_amba_port {
> >  	char			type[12];
> >  	bool			rs485_tx_started;
> >  	unsigned int		rs485_tx_drain_interval; /* usecs */
> > +	bool			console_line_ended;
> >  #ifdef CONFIG_DMA_ENGINE
> >  	/* DMA stuff */
> >  	unsigned int		dmacr;		/* dma control reg */
> > @@ -274,6 +276,10 @@ struct uart_amba_port {
> >  #endif
> >  };
> >
> > +/* if non-zero, the console is nbcon. */
> > +static int use_nbcon;
> > +module_param(use_nbcon, int, 0444);
> 
> Why is a module parameter needed here?  That feels wrong and not
> scalable at all.  What happens if you have multiple devices, which one
> is nbcon and which isn't?

This module parameter is for pl011 driver.
With this patch implemented, only one type will be usable, depending
on the value of use_nbcon.
I thought it would be more user-friendly if legacy operation could be
selected via boot parameter.
Would it be better to make the patch purely nbcon-compatible, without
retaining the legacy functionality?

> 
> 
> > +
> >  static unsigned int pl011_tx_empty(struct uart_port *port);
> >
> >  static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
> > @@ -2305,6 +2311,7 @@ static void pl011_console_putchar(struct uart_port
> *port, unsigned char ch)
> >  	while (pl011_read(uap, REG_FR) & UART01x_FR_TXFF)
> >  		cpu_relax();
> >  	pl011_write(ch, uap, REG_DR);
> > +	uap->console_line_ended = (ch == '\n');
> >  }
> >
> >  static void
> > @@ -2411,6 +2418,8 @@ static int pl011_console_setup(struct console *co,
> char *options)
> >  	if (ret)
> >  		return ret;
> >
> > +	uap->console_line_ended = true;
> > +
> >  	if (dev_get_platdata(uap->port.dev)) {
> >  		struct amba_pl011_data *plat;
> >
> > @@ -2494,6 +2503,106 @@ static int pl011_console_match(struct console *co,
> char *name, int idx,
> >  	return -ENODEV;
> >  }
> >
> > +static void
> > +pl011_console_write_atomic(struct console *co, struct nbcon_write_context
> *wctxt)
> > +{
> > +	struct uart_amba_port *uap = amba_ports[co->index];
> > +	unsigned int old_cr = 0;
> > +
> > +	if (!nbcon_enter_unsafe(wctxt))
> > +		return;
> > +
> > +	clk_enable(uap->clk);
> > +
> > +	if (!uap->vendor->always_enabled) {
> > +		old_cr = pl011_read(uap, REG_CR);
> > +		pl011_write((old_cr & ~UART011_CR_CTSEN) |
> (UART01x_CR_UARTEN | UART011_CR_TXE),
> > +				uap, REG_CR);
> > +	}
> > +
> > +	if (!uap->console_line_ended)
> > +		uart_console_write(&uap->port, "\n", 1,
> pl011_console_putchar);
> > +	uart_console_write(&uap->port, wctxt->outbuf, wctxt->len,
> pl011_console_putchar);
> > +
> > +	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) &
> uap->vendor->fr_busy)
> > +		cpu_relax();
> > +
> > +	if (!uap->vendor->always_enabled)
> > +		pl011_write(old_cr, uap, REG_CR);
> > +
> > +	clk_disable(uap->clk);
> > +
> > +	nbcon_exit_unsafe(wctxt);
> > +}
> > +
> > +static void
> > +pl011_console_write_thread(struct console *co, struct nbcon_write_context
> *wctxt)
> > +{
> > +	struct uart_amba_port *uap = amba_ports[co->index];
> > +	unsigned int old_cr = 0;
> > +
> > +	if (!nbcon_enter_unsafe(wctxt))
> > +		return;
> > +
> > +	clk_enable(uap->clk);
> > +
> > +	if (!uap->vendor->always_enabled) {
> > +		old_cr = pl011_read(uap, REG_CR);
> > +		pl011_write((old_cr & ~UART011_CR_CTSEN) |
> (UART01x_CR_UARTEN | UART011_CR_TXE),
> > +				uap, REG_CR);
> > +	}
> > +
> > +	if (nbcon_exit_unsafe(wctxt)) {
> > +		int i;
> > +		unsigned int len = READ_ONCE(wctxt->len);
> > +
> > +		for (i = 0; i < len; i++) {
> > +			if (!nbcon_enter_unsafe(wctxt))
> > +				break;
> > +			uart_console_write(&uap->port, wctxt->outbuf + i, 1,
> pl011_console_putchar);
> > +			if (!nbcon_exit_unsafe(wctxt))
> > +				break;
> > +		}
> > +	}
> > +
> > +	while (!nbcon_enter_unsafe(wctxt))
> > +		nbcon_reacquire_nobuf(wctxt);
> > +
> > +	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) &
> uap->vendor->fr_busy)
> > +		cpu_relax();
> > +
> > +	if (!uap->vendor->always_enabled)
> > +		pl011_write(old_cr, uap, REG_CR);
> > +
> > +	clk_disable(uap->clk);
> > +
> > +	nbcon_exit_unsafe(wctxt);
> > +}
> > +
> > +static void
> > +pl011_console_device_lock(struct console *co, unsigned long *flags)
> > +{
> > +	__uart_port_lock_irqsave(&amba_ports[co->index]->port, flags);
> > +}
> > +
> > +static void
> > +pl011_console_device_unlock(struct console *co, unsigned long flags)
> > +{
> > +	__uart_port_unlock_irqrestore(&amba_ports[co->index]->port, flags);
> > +}
> > +
> > +static void
> > +pl011_console_switch_to_nbcon(struct console *co)
> > +{
> > +	co->write = NULL;
> > +	co->write_atomic = pl011_console_write_atomic;
> > +	co->write_thread = pl011_console_write_thread;
> > +	co->device_lock = pl011_console_device_lock;
> > +	co->device_unlock = pl011_console_device_unlock;
> > +	co->flags = CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON;
> > +	pr_info("Serial: switched to nbcon\n");
> 
> dev_info()?

If the "use_nbcon" switching function is to be retained, I would like
to change pr_info to dev_info.

> 
> thanks,
> 
> greg k-h

Regards,
Toshiyuki Sato

Re: [PATCH] serial: amba-pl011: Implement nbcon console

[kernel test robot]

Hi Toshiyuki,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-linus]
[also build test ERROR on usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.13-rc6]
[cannot apply to tty/tty-testing tty/tty-next next-20250109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Toshiyuki-Sato/serial-amba-pl011-Implement-nbcon-console/20250108-085122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-linus
patch link:    https://lore.kernel.org/r/20250108004730.2302996-1-fj6611ie%40aa.jp.fujitsu.com
patch subject: [PATCH] serial: amba-pl011: Implement nbcon console
config: arm64-randconfig-003-20250109 (https://download.01.org/0day-ci/archive/20250109/202501092346.Q71aj5kq-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092346.Q71aj5kq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501092346.Q71aj5kq-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/tty/serial/amba-pl011.c: In function 'pl011_register_port':
>> drivers/tty/serial/amba-pl011.c:2851:25: error: implicit declaration of function 'pl011_console_switch_to_nbcon' [-Wimplicit-function-declaration]
    2851 |                         pl011_console_switch_to_nbcon(amba_reg.cons);
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/pl011_console_switch_to_nbcon +2851 drivers/tty/serial/amba-pl011.c

  2839	
  2840	static int pl011_register_port(struct uart_amba_port *uap)
  2841	{
  2842		int ret, i;
  2843	
  2844		/* Ensure interrupts from this UART are masked and cleared */
  2845		pl011_write(0, uap, REG_IMSC);
  2846		pl011_write(0xffff, uap, REG_ICR);
  2847	
  2848		if (!amba_reg.state) {
  2849			/* Replaces the console descriptor if NBCON is selected. */
  2850			if (amba_reg.cons && use_nbcon)
> 2851				pl011_console_switch_to_nbcon(amba_reg.cons);
  2852	
  2853			ret = uart_register_driver(&amba_reg);
  2854			if (ret < 0) {
  2855				dev_err(uap->port.dev,
  2856					"Failed to register AMBA-PL011 driver\n");
  2857				for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
  2858					if (amba_ports[i] == uap)
  2859						amba_ports[i] = NULL;
  2860				return ret;
  2861			}
  2862		}
  2863	
  2864		ret = uart_add_one_port(&amba_reg, &uap->port);
  2865		if (ret)
  2866			pl011_unregister_port(uap);
  2867	
  2868		return ret;
  2869	}
  2870	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] serial: amba-pl011: Implement nbcon console

[kernel test robot]

Hi Toshiyuki,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-linus]
[also build test ERROR on usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.13-rc6]
[cannot apply to tty/tty-testing tty/tty-next next-20250109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Toshiyuki-Sato/serial-amba-pl011-Implement-nbcon-console/20250108-085122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-linus
patch link:    https://lore.kernel.org/r/20250108004730.2302996-1-fj6611ie%40aa.jp.fujitsu.com
patch subject: [PATCH] serial: amba-pl011: Implement nbcon console
config: arm64-randconfig-002-20250109 (https://download.01.org/0day-ci/archive/20250109/202501092316.bZPuAcFC-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project f5cd181ffbb7cb61d582fe130d46580d5969d47a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092316.bZPuAcFC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501092316.bZPuAcFC-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/tty/serial/amba-pl011.c:30:
   In file included from include/linux/amba/bus.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/tty/serial/amba-pl011.c:2851:4: error: call to undeclared function 'pl011_console_switch_to_nbcon'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    2851 |                         pl011_console_switch_to_nbcon(amba_reg.cons);
         |                         ^
   3 warnings and 1 error generated.


vim +/pl011_console_switch_to_nbcon +2851 drivers/tty/serial/amba-pl011.c

  2839	
  2840	static int pl011_register_port(struct uart_amba_port *uap)
  2841	{
  2842		int ret, i;
  2843	
  2844		/* Ensure interrupts from this UART are masked and cleared */
  2845		pl011_write(0, uap, REG_IMSC);
  2846		pl011_write(0xffff, uap, REG_ICR);
  2847	
  2848		if (!amba_reg.state) {
  2849			/* Replaces the console descriptor if NBCON is selected. */
  2850			if (amba_reg.cons && use_nbcon)
> 2851				pl011_console_switch_to_nbcon(amba_reg.cons);
  2852	
  2853			ret = uart_register_driver(&amba_reg);
  2854			if (ret < 0) {
  2855				dev_err(uap->port.dev,
  2856					"Failed to register AMBA-PL011 driver\n");
  2857				for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
  2858					if (amba_ports[i] == uap)
  2859						amba_ports[i] = NULL;
  2860				return ret;
  2861			}
  2862		}
  2863	
  2864		ret = uart_add_one_port(&amba_reg, &uap->port);
  2865		if (ret)
  2866			pl011_unregister_port(uap);
  2867	
  2868		return ret;
  2869	}
  2870	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] serial: amba-pl011: Implement nbcon console

['Greg Kroah-Hartman']

On Thu, Jan 09, 2025 at 01:04:36AM +0000, Toshiyuki Sato (Fujitsu) wrote:
> Hi, Greg. Thanks for the comment.
> 
> > On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> > > Implement the callbacks required for an NBCON console [0] on the
> > > amba-pl011 console driver.
> > > The codes for legacy console are retained, and the module
> > > parameter (use_nbcon) allows switching between legacy and NBCON.
> > > The default is off (use legacy console) for now.
> > >
> > > Referred to the NBCON implementation work for 8250 [1] and imx [2].
> > >
> > > The normal-priority write_thread checks for console ownership
> > > each time a character is printed.
> > > write_atomic holds the console ownership until the entire string
> > > is printed.
> > >
> > > UART register operations are protected from other contexts by
> > > uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe)
> > > on panic.
> > >
> > > The patch has been verified to correctly handle the output and
> > > competition of messages with different priorities and flushing
> > > panic message to console after nmi panic using ARM64 QEMU and
> > > a physical machine(A64FX).
> > >
> > > [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> > > [1]
> > https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.d
> > e/T/
> > > [2]
> > https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c62
> > 7302335b@geanix.com/T/
> > >
> > > Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
> > > ---
> > >  drivers/tty/serial/amba-pl011.c | 113
> > ++++++++++++++++++++++++++++++++
> > >  1 file changed, 113 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > > index 69b7a3e1e..52fab3170 100644
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -41,6 +41,7 @@
> > >  #include <linux/sizes.h>
> > >  #include <linux/io.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/moduleparam.h>
> > >
> > >  #define UART_NR			14
> > >
> > > @@ -263,6 +264,7 @@ struct uart_amba_port {
> > >  	char			type[12];
> > >  	bool			rs485_tx_started;
> > >  	unsigned int		rs485_tx_drain_interval; /* usecs */
> > > +	bool			console_line_ended;
> > >  #ifdef CONFIG_DMA_ENGINE
> > >  	/* DMA stuff */
> > >  	unsigned int		dmacr;		/* dma control reg */
> > > @@ -274,6 +276,10 @@ struct uart_amba_port {
> > >  #endif
> > >  };
> > >
> > > +/* if non-zero, the console is nbcon. */
> > > +static int use_nbcon;
> > > +module_param(use_nbcon, int, 0444);
> > 
> > Why is a module parameter needed here?  That feels wrong and not
> > scalable at all.  What happens if you have multiple devices, which one
> > is nbcon and which isn't?
> 
> This module parameter is for pl011 driver.

Yes, and module parameters are from the 1990's, let's not add new ones
please.

> With this patch implemented, only one type will be usable, depending
> on the value of use_nbcon.
> I thought it would be more user-friendly if legacy operation could be
> selected via boot parameter.
> Would it be better to make the patch purely nbcon-compatible, without
> retaining the legacy functionality?

Why would you want the "legacy" functionality?  Aren't we converting all
consoles over to use the new stuff?

thanks,

greg k-h

RE: [PATCH] serial: amba-pl011: Implement nbcon console

[Toshiyuki Sato (Fujitsu)]

Thanks for your comment.

> On Thu, Jan 09, 2025 at 01:04:36AM +0000, Toshiyuki Sato (Fujitsu) wrote:
> > Hi, Greg. Thanks for the comment.
> >
> > > On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> > > > Implement the callbacks required for an NBCON console [0] on the
> > > > amba-pl011 console driver.
> > > > The codes for legacy console are retained, and the module
> > > > parameter (use_nbcon) allows switching between legacy and NBCON.
> > > > The default is off (use legacy console) for now.
> > > >
> > > > Referred to the NBCON implementation work for 8250 [1] and imx [2].
> > > >
> > > > The normal-priority write_thread checks for console ownership
> > > > each time a character is printed.
> > > > write_atomic holds the console ownership until the entire string
> > > > is printed.
> > > >
> > > > UART register operations are protected from other contexts by
> > > > uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe)
> > > > on panic.
> > > >
> > > > The patch has been verified to correctly handle the output and
> > > > competition of messages with different priorities and flushing
> > > > panic message to console after nmi panic using ARM64 QEMU and
> > > > a physical machine(A64FX).
> > > >
> > > > [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> > > > [1]
> > >
> https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.d
> > > e/T/
> > > > [2]
> > >
> https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c62
> > > 7302335b@geanix.com/T/
> > > >
> > > > Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
> > > > ---
> > > >  drivers/tty/serial/amba-pl011.c | 113
> > > ++++++++++++++++++++++++++++++++
> > > >  1 file changed, 113 insertions(+)
> > > >
> > > > diff --git a/drivers/tty/serial/amba-pl011.c
> b/drivers/tty/serial/amba-pl011.c
> > > > index 69b7a3e1e..52fab3170 100644
> > > > --- a/drivers/tty/serial/amba-pl011.c
> > > > +++ b/drivers/tty/serial/amba-pl011.c
> > > > @@ -41,6 +41,7 @@
> > > >  #include <linux/sizes.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/moduleparam.h>
> > > >
> > > >  #define UART_NR			14
> > > >
> > > > @@ -263,6 +264,7 @@ struct uart_amba_port {
> > > >  	char			type[12];
> > > >  	bool			rs485_tx_started;
> > > >  	unsigned int		rs485_tx_drain_interval; /* usecs */
> > > > +	bool			console_line_ended;
> > > >  #ifdef CONFIG_DMA_ENGINE
> > > >  	/* DMA stuff */
> > > >  	unsigned int		dmacr;		/* dma control reg */
> > > > @@ -274,6 +276,10 @@ struct uart_amba_port {
> > > >  #endif
> > > >  };
> > > >
> > > > +/* if non-zero, the console is nbcon. */
> > > > +static int use_nbcon;
> > > > +module_param(use_nbcon, int, 0444);
> > >
> > > Why is a module parameter needed here?  That feels wrong and not
> > > scalable at all.  What happens if you have multiple devices, which one
> > > is nbcon and which isn't?
> >
> > This module parameter is for pl011 driver.
> 
> Yes, and module parameters are from the 1990's, let's not add new ones
> please.
> 
> > With this patch implemented, only one type will be usable, depending
> > on the value of use_nbcon.
> > I thought it would be more user-friendly if legacy operation could be
> > selected via boot parameter.
> > Would it be better to make the patch purely nbcon-compatible, without
> > retaining the legacy functionality?
> 
> Why would you want the "legacy" functionality?  Aren't we converting all
> consoles over to use the new stuff?

As you say, it should be replaced with nbcon.
We will reconsider a v2 patch that supports only nbcon.

Regards,

Toshiyuki Sato