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

[Rengarajan.S]

Hi Jiri Slaby,

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.

Will avoid nested loops in the above statement in the next patch
revision.

> 
> 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;
> }
> 
> ?
> 

In order to differentiate Burst mode handling with byte mode handling
had seperate loops for both. Since, having single while loop also does
not align with rx implementation(where we have seperate handling of
burst and byte) will it be ok to retain the current implementation?

> 
> > +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
>