| Message ID | 2bc0b5d3f99184d8ddbf860666513f9fa7760af8.1712303358.git.esben@geanix.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: imx: Switch to nbcon console | expand |
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
John Ogness <john.ogness@linutronix.de> writes: > 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. The _write_thread() uses only the same registers. > For the 8250 I went through all uses and found several problems [0]. The > imx driver may have similar issues. I have now gone through all usage of above mentioned registers in the driver. I found a single missing lock/unlock, but other than that the driver seems to be in good shape in this regards. In the _probe() function, the following registers are accessed unprotected: ucr1, ucr2, ucr3, uts. But I assume this should be safe, as the uart port is not registred yet, and interrupt handlers have not been registered. I will send out a v3 short, hoping we can get this in a shape where we can get it merged shortly after the 8250 driver nbcon patches. /Esben
On 2024-09-13, Esben Haabendal <esben@geanix.com> wrote: > I have now gone through all usage of above mentioned registers in the > driver. I found a single missing lock/unlock, but other than that the > driver seems to be in good shape in this regards. Great. Please send a separate patch for the missing lock/unlock to mainline. It should be a mainline problem as well. > In the _probe() function, the following registers are accessed > unprotected: ucr1, ucr2, ucr3, uts. But I assume this should be safe, > as the uart port is not registred yet, and interrupt handlers have not > been registered. Sure. > I will send out a v3 short, hoping we can get this in a shape where we > can get it merged shortly after the 8250 driver nbcon patches. Great! Note that I am currently pushing the 8250 nbcon to mainline. It is in the review process and there have already been significant changes. It is all 8250-specific changes, so it probably is not related to your work. Just letting you know. I will CC you on the next series submission. John Ogness
John Ogness <john.ogness@linutronix.de> writes: > On 2024-09-13, Esben Haabendal <esben@geanix.com> wrote: >> I have now gone through all usage of above mentioned registers in the >> driver. I found a single missing lock/unlock, but other than that the >> driver seems to be in good shape in this regards. > > Great. Please send a separate patch for the missing lock/unlock to > mainline. It should be a mainline problem as well. Ok. I will send that separate to this series then. >> In the _probe() function, the following registers are accessed >> unprotected: ucr1, ucr2, ucr3, uts. But I assume this should be safe, >> as the uart port is not registred yet, and interrupt handlers have not >> been registered. > > Sure. > >> I will send out a v3 short, hoping we can get this in a shape where we >> can get it merged shortly after the 8250 driver nbcon patches. > > Great! > > Note that I am currently pushing the 8250 nbcon to mainline. It is in > the review process and there have already been significant changes. It > is all 8250-specific changes, so it probably is not related to your > work. Just letting you know. I will CC you on the next series > submission. Thanks. I will follow the work there, and try to make any adjustments that might be needed. /Esben
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, };
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(-)