Modify the TX path to operate in an IRQ-compatible way, while
maintaining support for polling mode via the poll timer.
Signed-off-by: Gabriel Somlo <[email protected]>
---
drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index e30adb30277f..307c27398e30 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -46,6 +46,7 @@ struct liteuart_port {
struct uart_port port;
struct timer_list timer;
u32 id;
+ bool poll_tx_started;
};
#define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
@@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
static void liteuart_stop_tx(struct uart_port *port)
{
- /* not used in LiteUART, but called unconditionally from serial_core */
+ if (port->irq) {
+ u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
+ litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
+ } else {
+ struct liteuart_port *uart = to_liteuart_port(port);
+ uart->poll_tx_started = false;
+ }
}
static void liteuart_start_tx(struct uart_port *port)
{
- struct circ_buf *xmit = &port->state->xmit;
- unsigned char ch;
-
- if (unlikely(port->x_char)) {
- litex_write8(port->membase + OFF_RXTX, port->x_char);
- port->icount.tx++;
- port->x_char = 0;
- } else if (!uart_circ_empty(xmit)) {
- while (xmit->head != xmit->tail) {
- ch = xmit->buf[xmit->tail];
- xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
- port->icount.tx++;
- liteuart_putchar(port, ch);
- }
+ if (port->irq) {
+ u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
+ litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
+ } else {
+ struct liteuart_port *uart = to_liteuart_port(port);
+ uart->poll_tx_started = true;
}
-
- if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
- uart_write_wakeup(port);
}
static void liteuart_stop_rx(struct uart_port *port)
@@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port)
tty_flip_buffer_push(&port->state->port);
}
+static void liteuart_tx_chars(struct uart_port *port)
+{
+ struct circ_buf *xmit = &port->state->xmit;
+
+ if (unlikely(port->x_char)) {
+ litex_write8(port->membase + OFF_RXTX, port->x_char);
+ port->x_char = 0;
+ port->icount.tx++;
+ return;
+ }
+
+ while (!litex_read8(port->membase + OFF_TXFULL)) {
+ if (xmit->head == xmit->tail)
+ break;
+ litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]);
+ xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+ port->icount.tx++;
+ }
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+
+ if (uart_circ_empty(xmit))
+ liteuart_stop_tx(port);
+}
+
static irqreturn_t liteuart_interrupt(int irq, void *data)
{
struct liteuart_port *uart = data;
struct uart_port *port = &uart->port;
u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
- /* for now, only rx path triggers interrupts */
- isr &= EV_RX;
+ if (!(port->irq || uart->poll_tx_started))
+ isr &= ~EV_TX; /* polling mode with tx stopped */
spin_lock(&port->lock);
if (isr & EV_RX)
liteuart_rx_chars(port);
+ if (isr & EV_TX) {
+ liteuart_tx_chars(port);
+ }
spin_unlock(&port->lock);
return IRQ_RETVAL(isr);
@@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port)
}
if (!port->irq) {
+ uart->poll_tx_started = false;
timer_setup(&uart->timer, liteuart_timer, 0);
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
@@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port)
struct liteuart_port *uart = to_liteuart_port(port);
litex_write8(port->membase + OFF_EV_ENABLE, 0);
+ uart->poll_tx_started = false;
if (port->irq)
free_irq(port->irq, port);
--
2.37.3
On Sat, Nov 12, 2022 at 04:21:24PM -0500, Gabriel Somlo wrote:
> Modify the TX path to operate in an IRQ-compatible way, while
> maintaining support for polling mode via the poll timer.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> 1 file changed, 47 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index e30adb30277f..307c27398e30 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -46,6 +46,7 @@ struct liteuart_port {
> struct uart_port port;
> struct timer_list timer;
> u32 id;
> + bool poll_tx_started;
> };
>
> #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
>
> static void liteuart_stop_tx(struct uart_port *port)
> {
> - /* not used in LiteUART, but called unconditionally from serial_core */
> + if (port->irq) {
> + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> + } else {
> + struct liteuart_port *uart = to_liteuart_port(port);
> + uart->poll_tx_started = false;
> + }
> }
>
> static void liteuart_start_tx(struct uart_port *port)
> {
> - struct circ_buf *xmit = &port->state->xmit;
> - unsigned char ch;
> -
> - if (unlikely(port->x_char)) {
> - litex_write8(port->membase + OFF_RXTX, port->x_char);
> - port->icount.tx++;
> - port->x_char = 0;
> - } else if (!uart_circ_empty(xmit)) {
> - while (xmit->head != xmit->tail) {
> - ch = xmit->buf[xmit->tail];
I just realized that this:
> - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> - port->icount.tx++;
... conflicts with another accepted patch (by Ilpo) that's currently
making its way to being released:
https://lore.kernel.org/all/[email protected]/
> - liteuart_putchar(port, ch);
> - }
> + if (port->irq) {
> + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
> + } else {
> + struct liteuart_port *uart = to_liteuart_port(port);
> + uart->poll_tx_started = true;
> }
> -
> - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> - uart_write_wakeup(port);
> }
>
> static void liteuart_stop_rx(struct uart_port *port)
> @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port)
> tty_flip_buffer_push(&port->state->port);
> }
>
> +static void liteuart_tx_chars(struct uart_port *port)
> +{
> + struct circ_buf *xmit = &port->state->xmit;
> +
> + if (unlikely(port->x_char)) {
> + litex_write8(port->membase + OFF_RXTX, port->x_char);
> + port->x_char = 0;
> + port->icount.tx++;
> + return;
> + }
> +
> + while (!litex_read8(port->membase + OFF_TXFULL)) {
> + if (xmit->head == xmit->tail)
> + break;
> + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]);
Also, this:
> + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> + port->icount.tx++;
should now be `uart_xmit_advance(port, 1);` instead.
I'm going to take that into account in v4, in addition to any extra
feedback I'll receive.
Thanks!
> + }
> +
> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> + uart_write_wakeup(port);
> +
> + if (uart_circ_empty(xmit))
> + liteuart_stop_tx(port);
> +}
> +
> static irqreturn_t liteuart_interrupt(int irq, void *data)
> {
> struct liteuart_port *uart = data;
> struct uart_port *port = &uart->port;
> u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
>
> - /* for now, only rx path triggers interrupts */
> - isr &= EV_RX;
> + if (!(port->irq || uart->poll_tx_started))
> + isr &= ~EV_TX; /* polling mode with tx stopped */
>
> spin_lock(&port->lock);
> if (isr & EV_RX)
> liteuart_rx_chars(port);
> + if (isr & EV_TX) {
> + liteuart_tx_chars(port);
> + }
> spin_unlock(&port->lock);
>
> return IRQ_RETVAL(isr);
> @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port)
> }
>
> if (!port->irq) {
> + uart->poll_tx_started = false;
> timer_setup(&uart->timer, liteuart_timer, 0);
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
> @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port)
> struct liteuart_port *uart = to_liteuart_port(port);
>
> litex_write8(port->membase + OFF_EV_ENABLE, 0);
> + uart->poll_tx_started = false;
>
> if (port->irq)
> free_irq(port->irq, port);
> --
> 2.37.3
>
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> Modify the TX path to operate in an IRQ-compatible way, while
> maintaining support for polling mode via the poll timer.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> 1 file changed, 47 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index e30adb30277f..307c27398e30 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -46,6 +46,7 @@ struct liteuart_port {
> struct uart_port port;
> struct timer_list timer;
> u32 id;
> + bool poll_tx_started;
> };
>
> #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
>
> static void liteuart_stop_tx(struct uart_port *port)
> {
> - /* not used in LiteUART, but called unconditionally from serial_core */
Drop this comment from the earlier patch too given you remove it here. It
just adds useless churn in diff for no useful reason.
> + if (port->irq) {
> + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
If you put irq_mask into liteuart_port you wouldn't need to read it
back here?
> + } else {
> + struct liteuart_port *uart = to_liteuart_port(port);
> + uart->poll_tx_started = false;
> + }
> }
>
> static void liteuart_start_tx(struct uart_port *port)
> {
> - struct circ_buf *xmit = &port->state->xmit;
> - unsigned char ch;
> -
> - if (unlikely(port->x_char)) {
> - litex_write8(port->membase + OFF_RXTX, port->x_char);
> - port->icount.tx++;
> - port->x_char = 0;
> - } else if (!uart_circ_empty(xmit)) {
> - while (xmit->head != xmit->tail) {
> - ch = xmit->buf[xmit->tail];
> - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> - port->icount.tx++;
This is not based on tty-next tree. Please rebase on top of it (you
might have noted it already, IIRC, somebody noted uart_xmit_advance
conflict in some patch, perhaps it was you :-)).
> - liteuart_putchar(port, ch);
> - }
> + if (port->irq) {
> + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
->irq_mask?
> + } else {
> + struct liteuart_port *uart = to_liteuart_port(port);
> + uart->poll_tx_started = true;
> }
> -
> - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> - uart_write_wakeup(port);
> }
>
> static void liteuart_stop_rx(struct uart_port *port)
> @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port)
> tty_flip_buffer_push(&port->state->port);
> }
>
> +static void liteuart_tx_chars(struct uart_port *port)
> +{
> + struct circ_buf *xmit = &port->state->xmit;
> +
> + if (unlikely(port->x_char)) {
> + litex_write8(port->membase + OFF_RXTX, port->x_char);
> + port->x_char = 0;
> + port->icount.tx++;
> + return;
> + }
> +
> + while (!litex_read8(port->membase + OFF_TXFULL)) {
> + if (xmit->head == xmit->tail)
There exists a helper for this condition.
> + break;
> + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]);
> + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> + port->icount.tx++;
uart_xmit_advance()
> + }
> +
> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> + uart_write_wakeup(port);
> +
> + if (uart_circ_empty(xmit))
> + liteuart_stop_tx(port);
> +}
You might want to check if you can generate this whole function with
Jiri's tx helpers (IIRC, they're only in tty-next tree currently).
> +
> static irqreturn_t liteuart_interrupt(int irq, void *data)
> {
> struct liteuart_port *uart = data;
> struct uart_port *port = &uart->port;
> u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
>
> - /* for now, only rx path triggers interrupts */
> - isr &= EV_RX;
> + if (!(port->irq || uart->poll_tx_started))
> + isr &= ~EV_TX; /* polling mode with tx stopped */
>
> spin_lock(&port->lock);
> if (isr & EV_RX)
> liteuart_rx_chars(port);
> + if (isr & EV_TX) {
> + liteuart_tx_chars(port);
> + }
Extra braces.
> spin_unlock(&port->lock);
>
> return IRQ_RETVAL(isr);
> @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port)
> }
>
> if (!port->irq) {
> + uart->poll_tx_started = false;
Can poll_tx_started ever be true here?
> timer_setup(&uart->timer, liteuart_timer, 0);
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
> @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port)
> struct liteuart_port *uart = to_liteuart_port(port);
>
> litex_write8(port->membase + OFF_EV_ENABLE, 0);
> + uart->poll_tx_started = false;
>
> if (port->irq)
> free_irq(port->irq, port);
>
--
i.
On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo J?rvinen wrote:
> On Sat, 12 Nov 2022, Gabriel Somlo wrote:
>
> > Modify the TX path to operate in an IRQ-compatible way, while
> > maintaining support for polling mode via the poll timer.
> >
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > ---
> > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> > 1 file changed, 47 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index e30adb30277f..307c27398e30 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -46,6 +46,7 @@ struct liteuart_port {
> > struct uart_port port;
> > struct timer_list timer;
> > u32 id;
> > + bool poll_tx_started;
> > };
> >
> > #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> > @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> >
> > static void liteuart_stop_tx(struct uart_port *port)
> > {
> > - /* not used in LiteUART, but called unconditionally from serial_core */
>
> Drop this comment from the earlier patch too given you remove it here. It
> just adds useless churn in diff for no useful reason.
Right -- I already had this lined up for v4 :)
> > + if (port->irq) {
> > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
>
> If you put irq_mask into liteuart_port you wouldn't need to read it
> back here?
So, instead of `bool poll_tx_started` I should just keep a copy of the
irq_mask there, and take `&port->lock` whenever I need to *both* update
the mask *and* write it out to the actual device register?
> > + } else {
> > + struct liteuart_port *uart = to_liteuart_port(port);
> > + uart->poll_tx_started = false;
> > + }
> > }
> >
> > static void liteuart_start_tx(struct uart_port *port)
> > {
> > - struct circ_buf *xmit = &port->state->xmit;
> > - unsigned char ch;
> > -
> > - if (unlikely(port->x_char)) {
> > - litex_write8(port->membase + OFF_RXTX, port->x_char);
> > - port->icount.tx++;
> > - port->x_char = 0;
> > - } else if (!uart_circ_empty(xmit)) {
> > - while (xmit->head != xmit->tail) {
> > - ch = xmit->buf[xmit->tail];
> > - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > - port->icount.tx++;
>
> This is not based on tty-next tree. Please rebase on top of it (you
> might have noted it already, IIRC, somebody noted uart_xmit_advance
> conflict in some patch, perhaps it was you :-)).
Yeah, I did notice that right after I sent out v3. I've already
rebased it on top of your patch using uart_xmit_advance.
> > - liteuart_putchar(port, ch);
> > - }
> > + if (port->irq) {
> > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
>
> ->irq_mask?
I'll switch to s/bool poll_tx_started/u8 irq_mask/g in v4, hopefully
it should make it all look cleaner.
> > + } else {
> > + struct liteuart_port *uart = to_liteuart_port(port);
> > + uart->poll_tx_started = true;
> > }
> > -
> > - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > - uart_write_wakeup(port);
> > }
> >
> > static void liteuart_stop_rx(struct uart_port *port)
> > @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port)
> > tty_flip_buffer_push(&port->state->port);
> > }
> >
> > +static void liteuart_tx_chars(struct uart_port *port)
> > +{
> > + struct circ_buf *xmit = &port->state->xmit;
> > +
> > + if (unlikely(port->x_char)) {
> > + litex_write8(port->membase + OFF_RXTX, port->x_char);
> > + port->x_char = 0;
> > + port->icount.tx++;
> > + return;
> > + }
> > +
> > + while (!litex_read8(port->membase + OFF_TXFULL)) {
> > + if (xmit->head == xmit->tail)
>
> There exists a helper for this condition.
Is that in the released linus tree, or still only in tty-next?
> > + break;
> > + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]);
> > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > + port->icount.tx++;
>
> uart_xmit_advance()
Already lined up for v4.
>
> > + }
> > +
> > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > + uart_write_wakeup(port);
> > +
> > + if (uart_circ_empty(xmit))
> > + liteuart_stop_tx(port);
> > +}
>
> You might want to check if you can generate this whole function with
> Jiri's tx helpers (IIRC, they're only in tty-next tree currently).
Looks like I should switch to tty-next for this whole series, which
makes sense, since it's a tty I'm working on :)
I'll rebase on top of that before I send out v4, hopefully later this
afternoon.
> > +
> > static irqreturn_t liteuart_interrupt(int irq, void *data)
> > {
> > struct liteuart_port *uart = data;
> > struct uart_port *port = &uart->port;
> > u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> >
> > - /* for now, only rx path triggers interrupts */
> > - isr &= EV_RX;
> > + if (!(port->irq || uart->poll_tx_started))
> > + isr &= ~EV_TX; /* polling mode with tx stopped */
> >
> > spin_lock(&port->lock);
> > if (isr & EV_RX)
> > liteuart_rx_chars(port);
> > + if (isr & EV_TX) {
> > + liteuart_tx_chars(port);
> > + }
>
> Extra braces.
Got it, thanks!
> > spin_unlock(&port->lock);
> >
> > return IRQ_RETVAL(isr);
> > @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port)
> > }
> >
> > if (!port->irq) {
> > + uart->poll_tx_started = false;
>
> Can poll_tx_started ever be true here?
Proably not, but it shouldn't matter if I switch to using `u8 irq_mask`,
instead, which should be initialized to 0 during probe().
Thanks again for the feedback!
Best,
--Gabriel
> > timer_setup(&uart->timer, liteuart_timer, 0);
> > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > }
> > @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port)
> > struct liteuart_port *uart = to_liteuart_port(port);
> >
> > litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > + uart->poll_tx_started = false;
> >
> > if (port->irq)
> > free_irq(port->irq, port);
> >
>
> --
> i.
>
On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
> On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo J?rvinen wrote:
> > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> >
> > > Modify the TX path to operate in an IRQ-compatible way, while
> > > maintaining support for polling mode via the poll timer.
> > >
> > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > ---
> > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> > > 1 file changed, 47 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > index e30adb30277f..307c27398e30 100644
> > > --- a/drivers/tty/serial/liteuart.c
> > > +++ b/drivers/tty/serial/liteuart.c
> > > + if (port->irq) {
> > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> >
> > If you put irq_mask into liteuart_port you wouldn't need to read it
> > back here?
>
> So, instead of `bool poll_tx_started` I should just keep a copy of the
> irq_mask there, and take `&port->lock` whenever I need to *both* update
> the mask *and* write it out to the actual device register?
I was mostly thinking of storing EV_RX there but then it could be derived
from port->irq that is checked by all paths already.
> > > + if (unlikely(port->x_char)) {
> > > + litex_write8(port->membase + OFF_RXTX, port->x_char);
> > > + port->x_char = 0;
> > > + port->icount.tx++;
> > > + return;
> > > + }
> > > +
> > > + while (!litex_read8(port->membase + OFF_TXFULL)) {
> > > + if (xmit->head == xmit->tail)
> >
> > There exists a helper for this condition.
>
> Is that in the released linus tree, or still only in tty-next?
uart_circ_empty() has been around for ages.
> > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > > + uart_write_wakeup(port);
> > > +
> > > + if (uart_circ_empty(xmit))
> > > + liteuart_stop_tx(port);
> > > +}
> >
> > You might want to check if you can generate this whole function with
> > Jiri's tx helpers (IIRC, they're only in tty-next tree currently).
>
> Looks like I should switch to tty-next for this whole series, which
> makes sense, since it's a tty I'm working on :)
>
> I'll rebase on top of that before I send out v4, hopefully later this
> afternoon.
Ok.
As I now looked it up, Jiri's tx helpers is
8275b48b278096edc1e3ea5aa9cf946a10022f79 and you'll find some example
conversions in the changes following it.
--
i.
On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo J?rvinen wrote:
> On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
>
> > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo J?rvinen wrote:
> > > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > >
> > > > Modify the TX path to operate in an IRQ-compatible way, while
> > > > maintaining support for polling mode via the poll timer.
> > > >
> > > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > > ---
> > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> > > > 1 file changed, 47 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > index e30adb30277f..307c27398e30 100644
> > > > --- a/drivers/tty/serial/liteuart.c
> > > > +++ b/drivers/tty/serial/liteuart.c
>
> > > > + if (port->irq) {
> > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> > >
> > > If you put irq_mask into liteuart_port you wouldn't need to read it
> > > back here?
> >
> > So, instead of `bool poll_tx_started` I should just keep a copy of the
> > irq_mask there, and take `&port->lock` whenever I need to *both* update
> > the mask *and* write it out to the actual device register?
>
> I was mostly thinking of storing EV_RX there but then it could be derived
> from port->irq that is checked by all paths already.
I'm a bit confused here -- the reason I was reading in irq_mask from
the mmio port and then flipping the EV_TX bit before writing it back
out was to preserve the current value of the EV_RX bit in that
register, whatever it may happen to be at the time. If I shadowed the
entire register value (with both EV_RX and EV_TX bits reflecting their
current value), I could get away with only ever *writing* the MMIO
register each time the shadow register value is modified (one or both
of the bits are flipped), and only when port->irq is non-zero (i.e.,
the shadow register would work for polling-mode also).
I think that's how altera_uart.c is doing it (I've been using that as
a source of inspiration).
I thought that's what you were suggesting earlie, but based on your
response above I'm no longer sure I follow.
> > > > + if (unlikely(port->x_char)) {
> > > > + litex_write8(port->membase + OFF_RXTX, port->x_char);
> > > > + port->x_char = 0;
> > > > + port->icount.tx++;
> > > > + return;
> > > > + }
> > > > +
> > > > + while (!litex_read8(port->membase + OFF_TXFULL)) {
> > > > + if (xmit->head == xmit->tail)
> > >
> > > There exists a helper for this condition.
> >
> > Is that in the released linus tree, or still only in tty-next?
>
> uart_circ_empty() has been around for ages.
Oh, I misunderstood what you meant here originally -- just use
`uart_circ_empty()` like I'm already doing elsewhere in the code, got
it!
> > > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > > > + uart_write_wakeup(port);
> > > > +
> > > > + if (uart_circ_empty(xmit))
> > > > + liteuart_stop_tx(port);
> > > > +}
> > >
> > > You might want to check if you can generate this whole function with
> > > Jiri's tx helpers (IIRC, they're only in tty-next tree currently).
> >
> > Looks like I should switch to tty-next for this whole series, which
> > makes sense, since it's a tty I'm working on :)
> >
> > I'll rebase on top of that before I send out v4, hopefully later this
> > afternoon.
>
> Ok.
>
> As I now looked it up, Jiri's tx helpers is
> 8275b48b278096edc1e3ea5aa9cf946a10022f79 and you'll find some example
> conversions in the changes following it.
I'll check that out and follow the examples.
Thanks again,
--G
On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo J?rvinen wrote:
> On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
>
> > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo J?rvinen wrote:
> > > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > >
> > > > Modify the TX path to operate in an IRQ-compatible way, while
> > > > maintaining support for polling mode via the poll timer.
> > > >
> > > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > > ---
> > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> > > > 1 file changed, 47 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > index e30adb30277f..307c27398e30 100644
> > > > --- a/drivers/tty/serial/liteuart.c
> > > > +++ b/drivers/tty/serial/liteuart.c
>
> > > > + if (port->irq) {
> > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> > >
> > > If you put irq_mask into liteuart_port you wouldn't need to read it
> > > back here?
> >
> > So, instead of `bool poll_tx_started` I should just keep a copy of the
> > irq_mask there, and take `&port->lock` whenever I need to *both* update
> > the mask *and* write it out to the actual device register?
>
> I was mostly thinking of storing EV_RX there but then it could be derived
> from port->irq that is checked by all paths already.
So, just to clear up the best course of action here (before I rebase
everything on top of tty-next: How about the patch below (currently
applied separately at the end of the entire series, but I can respin
it as part of the rx path (12/14) and tx path (13/14) as appropriate.
Let me know what you think.
Thanks,
--Gabriel
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index eb0ae8c8bd94..185db423c65f 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -47,7 +47,7 @@ struct liteuart_port {
struct uart_port port;
struct timer_list timer;
u32 id;
- bool poll_tx_started;
+ u8 irq_reg;
};
#define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
@@ -70,26 +70,27 @@ static struct uart_driver liteuart_driver = {
#endif
};
+static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 bits)
+{
+ struct liteuart_port *uart = to_liteuart_port(port);
+
+ if (set)
+ uart->irq_reg |= bits;
+ else
+ uart->irq_reg &= ~bits;
+
+ if (port->irq)
+ litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
+}
+
static void liteuart_stop_tx(struct uart_port *port)
{
- if (port->irq) {
- u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
- litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
- } else {
- struct liteuart_port *uart = to_liteuart_port(port);
- uart->poll_tx_started = false;
- }
+ liteuart_update_irq_reg(port, false, EV_TX);
}
static void liteuart_start_tx(struct uart_port *port)
{
- if (port->irq) {
- u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
- litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
- } else {
- struct liteuart_port *uart = to_liteuart_port(port);
- uart->poll_tx_started = true;
- }
+ liteuart_update_irq_reg(port, true, EV_TX);
}
static void liteuart_stop_rx(struct uart_port *port)
@@ -149,12 +150,10 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
{
struct liteuart_port *uart = data;
struct uart_port *port = &uart->port;
- u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
-
- if (!(port->irq || uart->poll_tx_started))
- isr &= ~EV_TX; /* polling mode with tx stopped */
+ u8 isr;
spin_lock(&port->lock);
+ isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
if (isr & EV_RX)
liteuart_rx_chars(port);
if (isr & EV_TX)
@@ -195,39 +194,40 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
static int liteuart_startup(struct uart_port *port)
{
struct liteuart_port *uart = to_liteuart_port(port);
+ unsigned long flags;
int ret;
- u8 irq_mask = 0;
if (port->irq) {
ret = request_irq(port->irq, liteuart_interrupt, 0,
KBUILD_MODNAME, uart);
- if (ret == 0) {
- /* only enabling rx interrupts at startup */
- irq_mask = EV_RX;
- } else {
+ if (ret) {
pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
port->line, port->irq);
port->irq = 0;
}
}
+ spin_lock_irqsave(&port->lock, flags);
+ /* only enabling rx irqs during startup */
+ liteuart_update_irq_reg(port, true, EV_RX);
+ spin_unlock_irqrestore(&port->lock, flags);
+
if (!port->irq) {
- uart->poll_tx_started = false;
timer_setup(&uart->timer, liteuart_timer, 0);
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
- litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
-
return 0;
}
static void liteuart_shutdown(struct uart_port *port)
{
struct liteuart_port *uart = to_liteuart_port(port);
+ unsigned long flags;
- litex_write8(port->membase + OFF_EV_ENABLE, 0);
- uart->poll_tx_started = false;
+ spin_lock_irqsave(&port->lock, flags);
+ liteuart_update_irq_reg(port, false, EV_RX | EV_TX);
+ spin_unlock_irqrestore(&port->lock, flags);
if (port->irq)
free_irq(port->irq, port);
--
2.37.3
On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
> On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo J?rvinen wrote:
> > On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
> >
> > > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo J?rvinen wrote:
> > > > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > > >
> > > > > Modify the TX path to operate in an IRQ-compatible way, while
> > > > > maintaining support for polling mode via the poll timer.
> > > > >
> > > > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > > > ---
> > > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
> > > > > 1 file changed, 47 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > > index e30adb30277f..307c27398e30 100644
> > > > > --- a/drivers/tty/serial/liteuart.c
> > > > > +++ b/drivers/tty/serial/liteuart.c
> >
> > > > > + if (port->irq) {
> > > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> > > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> > > >
> > > > If you put irq_mask into liteuart_port you wouldn't need to read it
> > > > back here?
> > >
> > > So, instead of `bool poll_tx_started` I should just keep a copy of the
> > > irq_mask there, and take `&port->lock` whenever I need to *both* update
> > > the mask *and* write it out to the actual device register?
> >
> > I was mostly thinking of storing EV_RX there but then it could be derived
> > from port->irq that is checked by all paths already.
>
> So, just to clear up the best course of action here (before I rebase
> everything on top of tty-next: How about the patch below (currently
> applied separately at the end of the entire series, but I can respin
> it as part of the rx path (12/14) and tx path (13/14) as appropriate.
>
> Let me know what you think.
I'm fine with that I think. I'd change to bits parameter name to something
more meaningful though, I guess the irq_mask could do ok there.
--
i.
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index eb0ae8c8bd94..185db423c65f 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -47,7 +47,7 @@ struct liteuart_port {
> struct uart_port port;
> struct timer_list timer;
> u32 id;
> - bool poll_tx_started;
> + u8 irq_reg;
> };
>
> #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> @@ -70,26 +70,27 @@ static struct uart_driver liteuart_driver = {
> #endif
> };
>
> +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 bits)
> +{
> + struct liteuart_port *uart = to_liteuart_port(port);
> +
> + if (set)
> + uart->irq_reg |= bits;
> + else
> + uart->irq_reg &= ~bits;
> +
> + if (port->irq)
> + litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> +}
> +
> static void liteuart_stop_tx(struct uart_port *port)
> {
> - if (port->irq) {
> - u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX);
> - } else {
> - struct liteuart_port *uart = to_liteuart_port(port);
> - uart->poll_tx_started = false;
> - }
> + liteuart_update_irq_reg(port, false, EV_TX);
> }
>
> static void liteuart_start_tx(struct uart_port *port)
> {
> - if (port->irq) {
> - u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE);
> - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX);
> - } else {
> - struct liteuart_port *uart = to_liteuart_port(port);
> - uart->poll_tx_started = true;
> - }
> + liteuart_update_irq_reg(port, true, EV_TX);
> }
>
> static void liteuart_stop_rx(struct uart_port *port)
> @@ -149,12 +150,10 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
> {
> struct liteuart_port *uart = data;
> struct uart_port *port = &uart->port;
> - u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> -
> - if (!(port->irq || uart->poll_tx_started))
> - isr &= ~EV_TX; /* polling mode with tx stopped */
> + u8 isr;
>
> spin_lock(&port->lock);
> + isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> if (isr & EV_RX)
> liteuart_rx_chars(port);
> if (isr & EV_TX)
> @@ -195,39 +194,40 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> static int liteuart_startup(struct uart_port *port)
> {
> struct liteuart_port *uart = to_liteuart_port(port);
> + unsigned long flags;
> int ret;
> - u8 irq_mask = 0;
>
> if (port->irq) {
> ret = request_irq(port->irq, liteuart_interrupt, 0,
> KBUILD_MODNAME, uart);
> - if (ret == 0) {
> - /* only enabling rx interrupts at startup */
> - irq_mask = EV_RX;
> - } else {
> + if (ret) {
> pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> port->line, port->irq);
> port->irq = 0;
> }
> }
>
> + spin_lock_irqsave(&port->lock, flags);
> + /* only enabling rx irqs during startup */
> + liteuart_update_irq_reg(port, true, EV_RX);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> if (!port->irq) {
> - uart->poll_tx_started = false;
> timer_setup(&uart->timer, liteuart_timer, 0);
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
>
> - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
> -
> return 0;
> }
>
> static void liteuart_shutdown(struct uart_port *port)
> {
> struct liteuart_port *uart = to_liteuart_port(port);
> + unsigned long flags;
>
> - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> - uart->poll_tx_started = false;
> + spin_lock_irqsave(&port->lock, flags);
> + liteuart_update_irq_reg(port, false, EV_RX | EV_TX);
> + spin_unlock_irqrestore(&port->lock, flags);
>
> if (port->irq)
> free_irq(port->irq, port);
>