Hi Shubhrajyoti,
Thanks for the patch. It looks good, but I am not able to test this just now,
it will have to wait a few days.
I can provide feedback though... see below.
> On May 16, 2019, at 01:42, Shubhrajyoti Datta wrote:
>
> From 92129ad79d3863b37936b2b383f4827926d15934 Mon Sep 17 00:00:00 2001
> From: Shubhrajyoti Datta
> Date: Thu, 16 May 2019 10:55:13 +0530
> Subject: [PATCH] serial: uartps: Add timeout for the wait for tx empty
>
> Make the wait for the TX empty have a timeout.
> In case there is no cable connected then the loop
> runs forever.
>
> Signed-off-by: Shubhrajyoti Datta
> ---
> drivers/tty/serial/xilinx_uartps.c | 24 +---
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c
> b/drivers/tty/serial/xilinx_uartps.c
> index 75e1027..5752f12 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -689,16 +689,26 @@ static void cdns_uart_set_termios(struct uart_port
> *port,
> unsigned int baud, minbaud, maxbaud;
> unsigned long flags;
> unsigned int ctrl_reg, mode_reg;
> + ulong timeout;
>
> - spin_lock_irqsave(&port->lock, flags);
> + timeout = jiffies + msecs_to_jiffies(1000);
1 second seems like a very long time. Zynqmp integrators are most likely making
products targeting very fast boot time.
I had an idea... Instead of testing TX FIFO empty for the timeout, could we
instead detect if the TX FIFO depth has changed at all? So make the timeout
much smaller, say, how much time it should take for a byte to go out at the
current baud rate. Then each time the fifo byte count changes, the "timer"
(jiffies + msecs_to_jiffies(byte_xfer_timeout);) is reset. Hopefully, the value
for byte_xfer_timeout would be small enough to be almost inconsequential.
>
> - /* Wait for the transmit FIFO to empty before making changes */
> - if (!(readl(port->membase + CDNS_UART_CR) &
> - CDNS_UART_CR_TX_DIS)) {
> - while (!(readl(port->membase + CDNS_UART_SR) &
> - CDNS_UART_SR_TXEMPTY)) {
> - cpu_relax();
> + spin_lock_irqsave(&port->lock, flags);
> + while (1) {
> + if (time_after_eq(jiffies, timeout)) {
> + spin_unlock_irqrestore(&port->lock, flags);
> + dev_dbg(port->dev, "timed out waiting for tx empty");
Hmm... I was thinking this may be more than a dev_dbg, especially if the
timeout remains 1 second. Integrators will most likely be glad to get this as a
warning, explaining why their board takes a second longer to boot.
Is there a way to prevent systemd from changing the termios parameters? (the
legacy SystemV "init" does that also, and blocks here indefinitely btw.)
> + return;
> }
> + /* Wait for the transmit FIFO to empty before making changes */
> + if ((readl(port->membase + CDNS_UART_CR) &
> + CDNS_UART_CR_TX_DIS))
> + break;
> +
> + if ((readl(port->membase + CDNS_UART_SR) &
> + CDNS_UART_SR_TXEMPTY))
> + break;
> + cpu_relax();
> }
Another idea could be to check if the new termios settings do in fact require a
flush of the TX Fifo before going on... I would bet it doesn't and the wait
might not even be necessary in almost all cases (i.e. baud rate and bit
configuration, set by the FSBL are the same as what linux is trying to
configure.)
>
> /* Disable the TX and RX to set baud rate */
> --
> 2.1.1
>
Thanks again for the help!
Cheers!
--
___
meta-xilinx mailing list
meta-xilinx@yoctoproject.org
https://lists.yoctoproject.org/listinfo/meta-xilinx
