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 <[email protected]>
---
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
@@ -67,6 +67,7 @@
#define SYSLOCK_RETRY_CNT 1000
#define UART_RX_BYTE_FIFO 0x00
+#define UART_TX_BYTE_FIFO 0x00
#define UART_FIFO_CTL 0x02
#define UART_ACTV_REG 0x11
@@ -100,6 +101,7 @@
#define UART_RESET_D3_RESET_DISABLE BIT(16)
#define UART_BURST_STATUS_REG 0x9C
+#define UART_TX_BURST_FIFO 0xA0
#define UART_RX_BURST_FIFO 0xA4
#define MAX_PORTS 4
@@ -109,6 +111,7 @@
#define UART_BURST_SIZE 4
#define UART_BST_STAT_RX_COUNT_MASK 0x00FF
+#define UART_BST_STAT_TX_COUNT_MASK 0xFF00
#define UART_BST_STAT_IIR_INT_PEND 0x100000
#define UART_LSR_OVERRUN_ERR_CLR 0x43
#define UART_BST_STAT_LSR_RX_MASK 0x9F000000
@@ -116,6 +119,7 @@
#define UART_BST_STAT_LSR_OVERRUN_ERR 0x2000000
#define UART_BST_STAT_LSR_PARITY_ERR 0x4000000
#define UART_BST_STAT_LSR_FRAME_ERR 0x8000000
+#define UART_BST_STAT_LSR_THRE 0x20000000
struct pci1xxxx_8250 {
unsigned int nr;
@@ -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,
+ 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)
+ break;
+ if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE))
+ break;
+ writel(*(unsigned int *)&xmit->buf[xmit->tail],
+ port->membase + UART_TX_BURST_FIFO);
+ *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);
+ }
+
+ 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);
+
+ /*
+ * 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;
+ }
+}
+
+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);
+ } else {
+ 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++;
+ 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);
+}
+
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;
--
2.25.1
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 <[email protected]>
> ---
> 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
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
> [email protected]. 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 <[email protected]>
> > ---
> > 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
>
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
> > [email protected]. 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 <[email protected]>
> > > ---
> > > 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
> >
>
On 09. 02. 24, 5:52, [email protected] wrote:
> 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.
Hi,
as you write, you cannot change the patch as it is already queued.
PLease submit changes on top.
thanks,
--
js
suse labs
On Fri, Feb 09, 2024 at 07:38:49AM +0100, Jiri Slaby wrote:
> On 09. 02. 24, 5:52, [email protected] wrote:
> > 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.
>
> Hi,
>
> as you write, you cannot change the patch as it is already queued. PLease
> submit changes on top.
Or, if the original is so bad, we can revert them now and wait for new
ones later, just let me know.
thanks,
greg k-h
Hi Greg,
Since, Most of the comments were alignment related and no
functionalities were affected, the current patch can be merged in the
next window. Will share another patch with the suggested incremental
changes shortly. Thanks!!
On Fri, 2024-02-09 at 10:20 +0000, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Fri, Feb 09, 2024 at 07:38:49AM +0100, Jiri Slaby wrote:
> > On 09. 02. 24, 5:52, [email protected] wrote:
> > > 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.
> >
> > Hi,
> >
> > as you write, you cannot change the patch as it is already queued.
> > PLease
> > submit changes on top.
>
> Or, if the original is so bad, we can revert them now and wait for
> new
> ones later, just let me know.
>
> thanks,
>
> greg k-h
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
> [email protected]. 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 <[email protected]>
> > ---
> > 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
>