Re: [PATCH v1 tty] 8250: microchip: pci1xxxx: Add Burst mode transmission support in uart driver for reading from FIFO

[Rengarajan.S]

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
>