Re: [PATCH v3 13/14] serial: liteuart: add IRQ support for the TX path

["Gabriel L. Somlo" <gsomlo@gmail.com>]

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 <gsomlo@gmail.com>
> > > > ---
> > > >  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