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@xxxxxxxxxx> 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@xxxxxxxxxxxxx
