From 2b4bac48c10848ccffd484e7cd025dc085c1bd32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kundr=C3=A1t?= Date: Thu, 14 Dec 2017 16:02:54 +0100 Subject: [PATCH] serial: max310x: Use batched reads when reasonably safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hardware has a 128 byte RX FIFO buffer for each independent UART. Previously, the code was always reading that byte-by-byte via independent SPI transactions and the associated overhead. In practice, this led to up to eight bytes over SPI for just one byte in the UART's RX FIFO: - reading the global IRQ register (two bytes, one for command, the other for data) - reading one UART's ISR (again two bytes) - reading the byte count (two bytes yet again) - finally, reading one byte of the FIFO via another two-byte transaction We cannot always use a batched read. If the TTY is set to intercept break conditions or report framing or parity errors, then it is required to check the Line Status Register (LSR) for each byte which is read from the RX FIFO. The documentation does not show a way of doing that in a single SPI transaction; registers 0x00 and 0x04 are separate. In my testing, this is no silver bullet. I was feeding 2MB of random data over four daisy-chaned UARTs of MAX14830, and this is the distribution that I was getting: - R <= 1: 7437322 - R <= 2: 162093 - R <= 4: 4093 - R <= 8: 4196 - R <= 16: 645 - R <= 32: 165 - R <= 64: 58 - R <= 128: 0 For a reference, batching the write operations works much better: - W <= 1: 2664 - W <= 2: 1305 - W <= 4: 627 - W <= 8: 371 - W <= 16: 121 - W <= 32: 68 - W <= 64: 33 - W <= 128: 63139 That's probably because this HW/SW combination (Clearfog Base, Armada 388) is probably "good enough" to react to the chip's IRQ "fast enough" most of the time. Still, I was getting RX overruns every now and then. In future, I plan to improve this by letting the RX FIFO be filled a little more (the chip has support for that and also for a "stale timeout" to prevent additional starvation). Signed-off-by: Jan Kundrát Signed-off-by: Greg Kroah-Hartman --- drivers/tty/serial/max310x.c | 124 ++++++++++++++++++++++++----------- 1 file changed, 85 insertions(+), 39 deletions(-) diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c index 2a2c4ea16306..dc0e1ad351f9 100644 --- a/drivers/tty/serial/max310x.c +++ b/drivers/tty/serial/max310x.c @@ -609,57 +609,103 @@ static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer)); } +static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len) +{ + u8 header[] = { port->iobase + MAX310X_RHR_REG }; + struct spi_transfer xfer[] = { + { + .tx_buf = &header, + .len = sizeof(header), + }, { + .rx_buf = rxbuf, + .len = len, + } + }; + spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer)); +} + static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen) { - unsigned int sts, ch, flag; + unsigned int sts, ch, flag, i; + u8 buf[MAX310X_FIFO_SIZE]; - if (unlikely(rxlen >= port->fifosize)) { - dev_warn_ratelimited(port->dev, "Possible RX FIFO overrun\n"); - port->icount.buf_overrun++; - /* Ensure sanity of RX level */ - rxlen = port->fifosize; - } + if (port->read_status_mask == MAX310X_LSR_RXOVR_BIT) { + /* We are just reading, happily ignoring any error conditions. + * Break condition, parity checking, framing errors -- they + * are all ignored. That means that we can do a batch-read. + * + * There is a small opportunity for race if the RX FIFO + * overruns while we're reading the buffer; the datasheets says + * that the LSR register applies to the "current" character. + * That's also the reason why we cannot do batched reads when + * asked to check the individual statuses. + * */ - while (rxlen--) { - ch = max310x_port_read(port, MAX310X_RHR_REG); sts = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG); + max310x_batch_read(port, buf, rxlen); - sts &= MAX310X_LSR_RXPAR_BIT | MAX310X_LSR_FRERR_BIT | - MAX310X_LSR_RXOVR_BIT | MAX310X_LSR_RXBRK_BIT; - - port->icount.rx++; + port->icount.rx += rxlen; flag = TTY_NORMAL; + sts &= port->read_status_mask; - if (unlikely(sts)) { - if (sts & MAX310X_LSR_RXBRK_BIT) { - port->icount.brk++; - if (uart_handle_break(port)) - continue; - } else if (sts & MAX310X_LSR_RXPAR_BIT) - port->icount.parity++; - else if (sts & MAX310X_LSR_FRERR_BIT) - port->icount.frame++; - else if (sts & MAX310X_LSR_RXOVR_BIT) - port->icount.overrun++; - - sts &= port->read_status_mask; - if (sts & MAX310X_LSR_RXBRK_BIT) - flag = TTY_BREAK; - else if (sts & MAX310X_LSR_RXPAR_BIT) - flag = TTY_PARITY; - else if (sts & MAX310X_LSR_FRERR_BIT) - flag = TTY_FRAME; - else if (sts & MAX310X_LSR_RXOVR_BIT) - flag = TTY_OVERRUN; + if (sts & MAX310X_LSR_RXOVR_BIT) { + dev_warn_ratelimited(port->dev, "Hardware RX FIFO overrun\n"); + port->icount.overrun++; } - if (uart_handle_sysrq_char(port, ch)) - continue; + for (i = 0; i < rxlen; ++i) { + uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT, buf[i], flag); + } - if (sts & port->ignore_status_mask) - continue; + } else { + if (unlikely(rxlen >= port->fifosize)) { + dev_warn_ratelimited(port->dev, "Possible RX FIFO overrun\n"); + port->icount.buf_overrun++; + /* Ensure sanity of RX level */ + rxlen = port->fifosize; + } - uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT, ch, flag); + while (rxlen--) { + ch = max310x_port_read(port, MAX310X_RHR_REG); + sts = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG); + + sts &= MAX310X_LSR_RXPAR_BIT | MAX310X_LSR_FRERR_BIT | + MAX310X_LSR_RXOVR_BIT | MAX310X_LSR_RXBRK_BIT; + + port->icount.rx++; + flag = TTY_NORMAL; + + if (unlikely(sts)) { + if (sts & MAX310X_LSR_RXBRK_BIT) { + port->icount.brk++; + if (uart_handle_break(port)) + continue; + } else if (sts & MAX310X_LSR_RXPAR_BIT) + port->icount.parity++; + else if (sts & MAX310X_LSR_FRERR_BIT) + port->icount.frame++; + else if (sts & MAX310X_LSR_RXOVR_BIT) + port->icount.overrun++; + + sts &= port->read_status_mask; + if (sts & MAX310X_LSR_RXBRK_BIT) + flag = TTY_BREAK; + else if (sts & MAX310X_LSR_RXPAR_BIT) + flag = TTY_PARITY; + else if (sts & MAX310X_LSR_FRERR_BIT) + flag = TTY_FRAME; + else if (sts & MAX310X_LSR_RXOVR_BIT) + flag = TTY_OVERRUN; + } + + if (uart_handle_sysrq_char(port, ch)) + continue; + + if (sts & port->ignore_status_mask) + continue; + + uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT, ch, flag); + } } tty_flip_buffer_push(&port->state->port);