Hi Jiri Slaby, The patch has been accepted and added in the tty-git tree and will be merged during the next merged window. Should the changes be given as a seperate incremental patch or should we re-submit this patch again. On Thu, 2024-02-08 at 02:49 +0000, Rengarajan S - I69107 wrote: > Hi Jiri Slaby, > > Thanks for reviewing the patch. Will address the comments in the next > patch revision. > > On Mon, 2024-02-05 at 08:56 +0100, Jiri Slaby wrote: > > [Some people who received this message don't often get email from > > jirislaby@xxxxxxxxxx. Learn why this is important at > > https://aka.ms/LearnAboutSenderIdentification ;] > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > 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@xxxxxxxxxxxxx> > > > --- > > > 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 > > >
