Hi Jean,
Thanks for the review.
> -----Original Message-----
> From: Jean-Francois Dagenais <[email protected]>
> Sent: Thursday, May 16, 2019 6:15 PM
> To: Shubhrajyoti Datta <[email protected]>
> Cc: Michal Simek <[email protected]>; [email protected];
> Nathan Rossi <[email protected]>
> Subject: Re: xilinx_uartps infinite wait loop
>
> 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 <[email protected]>
> wrote:
> >
> > From 92129ad79d3863b37936b2b383f4827926d15934 Mon Sep 17
> 00:00:00 2001
> > From: Shubhrajyoti Datta <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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 will update the timeout .
> 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.
>
Or maybe plan for the slowest baudrate.
Does that sound reasonable ?
> >
> > - /* 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.
Will update in the next version.
>
> 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
It is not the new termios that needs the flush I think it is to ensure that
the
Earlier data that is going on is not flushed.
> 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.)
This can be an optimization.
>
> >
> > /* Disable the TX and RX to set baud rate */
> > --
> > 2.1.1
> >
>
> Thanks again for the help!
> Cheers!
--
_______________________________________________
meta-xilinx mailing list
[email protected]
https://lists.yoctoproject.org/listinfo/meta-xilinx
