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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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