| Message ID | 1447963344-16266-4-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/19/2015 03:02 PM, Soren Brinkmann wrote: > start_tx must start transmitting characters. Regardless of the state of > the circular buffer, always enable the transmitter hardware. Why? Does cdns_uart_stop_tx() actually stop the transmitter so that data is remains in the transmitter? > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > drivers/tty/serial/xilinx_uartps.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c > index 2c98c357d9a0..df6778d17949 100644 > --- a/drivers/tty/serial/xilinx_uartps.c > +++ b/drivers/tty/serial/xilinx_uartps.c > @@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port) > { > unsigned int status, numbytes = port->fifosize; > > - if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) > - return; The test for tx stopped needs to remain; otherwise, transmission will restart even if the tty has been stopped (for example by userspace) or IXON software flow control. Regards, Peter Hurley > - > /* > * Set the TX enable bit and clear the TX disable bit to enable the > * transmitter. > @@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port) > status |= CDNS_UART_CR_TX_EN; > writel(status, port->membase + CDNS_UART_CR_OFFSET); > > + if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) > + return; > + > while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) & > CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) { > /* Break if no more data available in the UART buffer */ >
On 11/19/2015 03:02 PM, Soren Brinkmann wrote: > start_tx must start transmitting characters. Regardless of the state of > the circular buffer, always enable the transmitter hardware. Why? Does cdns_uart_stop_tx() actually stop the transmitter so that data remains in the transmitter? > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > drivers/tty/serial/xilinx_uartps.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c > index 2c98c357d9a0..df6778d17949 100644 > --- a/drivers/tty/serial/xilinx_uartps.c > +++ b/drivers/tty/serial/xilinx_uartps.c > @@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port) > { > unsigned int status, numbytes = port->fifosize; > > - if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) > - return; The test for tx stopped needs to remain; otherwise, transmission will restart even if the tty has been stopped (for example by userspace or IXON software flow control). Regards, Peter Hurley > - > /* > * Set the TX enable bit and clear the TX disable bit to enable the > * transmitter. > @@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port) > status |= CDNS_UART_CR_TX_EN; > writel(status, port->membase + CDNS_UART_CR_OFFSET); > > + if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) > + return; > + > while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) & > CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) { > /* Break if no more data available in the UART buffer */ >
Hi Peter, On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote: > > > On 11/19/2015 03:02 PM, Soren Brinkmann wrote: > > start_tx must start transmitting characters. Regardless of the state of > > the circular buffer, always enable the transmitter hardware. > > Why? > > Does cdns_uart_stop_tx() actually stop the transmitter so that > data remains in the transmitter? Well, I saw my system freezing and the cause seemed to be that the UART receiver and/or transmitters were disabled while the system was trying to print. Hence, I started questioning all locations touching the transmitter/receiver enable. I read the docs in https://www.kernel.org/doc/Documentation/serial/driver, which simply says "Start transmitting characters." for start_tx(). Hence, I thought, this function is probably supposed to just do that and start the transmitter. I'll test whether this patch can be dropped. Thanks, Sören
On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote: > > > On 11/19/2015 03:02 PM, Soren Brinkmann wrote: > > start_tx must start transmitting characters. Regardless of the state of > > the circular buffer, always enable the transmitter hardware. > > Why? > > Does cdns_uart_stop_tx() actually stop the transmitter so that > data remains in the transmitter? Fixing up the patch, I looked at this one. It might actually do that. Without having changed anything. The doc says: "The driver should stop transmitting characters as soon as possible.". And the implementation is really not draining any FIFO, but just disabling the transmitter. I take your question as that this might not be this way? Should stop_tx drain the FIFO first? Thanks, Sören
On 11/20/2015 12:05 PM, Sören Brinkmann wrote: > On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote: >> >> >> On 11/19/2015 03:02 PM, Soren Brinkmann wrote: >>> start_tx must start transmitting characters. Regardless of the state of >>> the circular buffer, always enable the transmitter hardware. >> >> Why? >> >> Does cdns_uart_stop_tx() actually stop the transmitter so that >> data remains in the transmitter? > > Fixing up the patch, I looked at this one. It might actually do that. Ok. > Without having changed anything. The doc says: "The driver should > stop transmitting characters as soon as possible.". And the > implementation is really not draining any FIFO, but just disabling the > transmitter. I take your question as that this might not be this way? > Should stop_tx drain the FIFO first? No. Most h/w can't actually stop the transmitter (or not without losing data), so that's why the expectation is only for "as soon as possible". Stopping sooner is better. Regards, Peter Hurley
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c index 2c98c357d9a0..df6778d17949 100644 --- a/drivers/tty/serial/xilinx_uartps.c +++ b/drivers/tty/serial/xilinx_uartps.c @@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port) { unsigned int status, numbytes = port->fifosize; - if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) - return; - /* * Set the TX enable bit and clear the TX disable bit to enable the * transmitter. @@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port) status |= CDNS_UART_CR_TX_EN; writel(status, port->membase + CDNS_UART_CR_OFFSET); + if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port)) + return; + while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) & CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) { /* Break if no more data available in the UART buffer */
start_tx must start transmitting characters. Regardless of the state of the circular buffer, always enable the transmitter hardware. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/tty/serial/xilinx_uartps.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)