Messages in this thread Patch in this message | | | Date | Mon, 5 Feb 2024 08:56:45 +0100 | Subject | Re: [PATCH v1 tty] 8250: microchip: pci1xxxx: Add Burst mode transmission support in uart driver for reading from FIFO | From | Jiri Slaby <> |
| |
On 25. 01. 24, 11:00, Rengarajan S wrote: > pci1xxxx_handle_irq reads the burst status and checks if the FIFO > is empty and is ready to accept the incoming data. The handling is > done in pci1xxxx_tx_burst where each transaction processes data in > block of DWORDs, while any remaining bytes are processed individually, > one byte at a time. > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > --- > drivers/tty/serial/8250/8250_pci1xxxx.c | 106 ++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c > index 558c4c7f3104..d53605bf908d 100644 > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c .. > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct uart_port *port, u32 uart_status) > } > } > > +static void pci1xxxx_process_write_data(struct uart_port *port, > + struct circ_buf *xmit, > + int *data_empty_count,
count is unsigned, right?
> + u32 *valid_byte_count) > +{ > + u32 valid_burst_count = *valid_byte_count / UART_BURST_SIZE; > + > + /* > + * Each transaction transfers data in DWORDs. If there are less than > + * four remaining valid_byte_count to transfer or if the circular > + * buffer has insufficient space for a DWORD, the data is transferred > + * one byte at a time. > + */ > + while (valid_burst_count) { > + if (*data_empty_count - UART_BURST_SIZE < 0)
Huh?
*data_empty_count < UART_BURST_SIZE
instead?
> + break; > + if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE))
Is this the same as easy to understand:
CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) < UART_BURST_SIZE
?
> + break; > + writel(*(unsigned int *)&xmit->buf[xmit->tail], > + port->membase + UART_TX_BURST_FIFO);
What about unaligned accesses?
And you really wanted to spell u32 explicitly, not uint.
> + *valid_byte_count -= UART_BURST_SIZE; > + *data_empty_count -= UART_BURST_SIZE; > + valid_burst_count -= UART_BYTE_SIZE; > + > + xmit->tail = (xmit->tail + UART_BURST_SIZE) & > + (UART_XMIT_SIZE - 1);
uart_xmit_advance()
> + } > + > + while (*valid_byte_count) { > + if (*data_empty_count - UART_BYTE_SIZE < 0) > + break; > + writeb(xmit->buf[xmit->tail], port->membase + > + UART_TX_BYTE_FIFO); > + *data_empty_count -= UART_BYTE_SIZE; > + *valid_byte_count -= UART_BYTE_SIZE; > + > + /* > + * When the tail of the circular buffer is reached, the next > + * byte is transferred to the beginning of the buffer. > + */ > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & > + (UART_XMIT_SIZE - 1);
uart_xmit_advance()
> + > + /* > + * If there are any pending burst count, data is handled by > + * transmitting DWORDs at a time. > + */ > + if (valid_burst_count && (xmit->tail < > + (UART_XMIT_SIZE - UART_BURST_SIZE))) > + break; > + } > +}
This nested double loop is _really_ hard to follow. It's actually terrible with cut & paste mistakes.
Could these all loops be simply replaced by something like this pseudo code (a single loop):
while (data_empty_count) { cnt = CIRC_CNT_TO_END(); if (!cnt) break; if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned() writeb(); cnt = 1; } else { writel() cnt = UART_BURST_SIZE; } uart_xmit_advance(cnt); data_empty_count -= cnt; }
?
> +static void pci1xxxx_tx_burst(struct uart_port *port, u32 uart_status) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + u32 valid_byte_count; > + int data_empty_count; > + struct circ_buf *xmit; > + > + xmit = &port->state->xmit; > + > + if (port->x_char) { > + writeb(port->x_char, port->membase + UART_TX); > + port->icount.tx++; > + port->x_char = 0; > + return; > + } > + > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { > + port->ops->stop_tx(port);
This looks weird standing here. You should handle this below along with RPM.
> + } else {
The condition should be IMO inverted with this block in its body:
> + data_empty_count = (pci1xxxx_read_burst_status(port) & > + UART_BST_STAT_TX_COUNT_MASK) >> 8; > + do { > + valid_byte_count = uart_circ_chars_pending(xmit); > + > + pci1xxxx_process_write_data(port, xmit, > + &data_empty_count, > + &valid_byte_count); > + > + port->icount.tx++;
Why do you increase the stats only once per burst? (Solved by uart_xmit_advance() above.)
> + if (uart_circ_empty(xmit)) > + break; > + } while (data_empty_count && valid_byte_count); > + } > + > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(port); > + > + /* > + * With RPM enabled, we have to wait until the FIFO is empty before > + * the HW can go idle. So we get here once again with empty FIFO and > + * disable the interrupt and RPM in __stop_tx() > + */ > + if (uart_circ_empty(xmit) && !(up->capabilities & UART_CAP_RPM)) > + port->ops->stop_tx(port);
I wonder why this driver needs this and others don't? Should they be fixed too?
> +} > + > static int pci1xxxx_handle_irq(struct uart_port *port) > { > unsigned long flags; > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct uart_port *port) > if (status & UART_BST_STAT_LSR_RX_MASK) > pci1xxxx_rx_burst(port, status); > > + if (status & UART_BST_STAT_LSR_THRE) > + pci1xxxx_tx_burst(port, status); > + > spin_unlock_irqrestore(&port->lock, flags); > > return 1;
-- js suse labs
| |