[PATCH v3 00/14] serial: liteuart: add IRQ support

[PATCH v3 00/14] serial: liteuart: add IRQ support

[Gabriel Somlo]

Add IRQ support to the LiteX LiteUART serial interface

Changes from v2:
  - further split out "separate RX loop from poll timer" into
    dedicated patches highlighting additional changes explicitly:
      - factoring out tty_flip_buffer_push() (6/14)
      - ack only RX events in RX loop (7/14)
      - pass constant flag to uart_insert_char() directly (8/14)
      - fix variable types in rx loop (9/14)
      - separating RX loop from poll timer (10/14)
  - added patch (11/14) to move function definitions to a more
    convenient location, making subsequent changes easier to read
    in diff patch form.
  - fixes and clarifications for RX path IRQ support patch (now 12/14):
      - only return IRQ_HANDLED for RX events
      - use pr_fmt() to improve style of irq setup error message
      - remove unnecessary locking from around single-register access
  - modify TX path to support both IRQ-mode and polling (13/14)
  - move polling-only liteuart_putchar() behind same conditional
    (CONFIG_SERIAL_LITEUART_CONSOLE) as the rest of the code that's
    still using it.

> Changes from v1:
>   - split minor cosmetic changes out into individual patches
>     (1/3 became 1..5/7)
>   - patches 6/7 and 7/7 unchanged (used to be 2/3 and 3/3)

Gabriel Somlo (14):
  serial: liteuart: use KBUILD_MODNAME as driver name
  serial: liteuart: use bit number macros
  serial: liteuart: remove unused uart_ops stubs
  serial: liteuart: don't set unused port fields
  serial: liteuart: minor style fix in liteuart_init()
  serial: liteuart: move tty_flip_buffer_push() out of rx loop
  serial: liteuart: rx loop should only ack rx events
  serial: liteuart: simplify passing of uart_insert_char() flag
  serial: liteuart: fix rx loop variable types
  serial: liteuart: separate rx loop from poll timer
  serial: liteuart: move function definitions
  serial: liteuart: add IRQ support for the RX path
  serial: liteuart: add IRQ support for the TX path
  serial: liteuart: move polling putchar() function

 drivers/tty/serial/liteuart.c | 226 +++++++++++++++++++++-------------
 1 file changed, 142 insertions(+), 84 deletions(-)

-- 
2.37.3

[PATCH v3 01/14] serial: liteuart: use KBUILD_MODNAME as driver name

[Gabriel Somlo]

Replace hard-coded instances of "liteuart" with KBUILD_MODNAME.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 4c0604325ee9..32b81bd03d0c 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -57,7 +57,7 @@ static struct console liteuart_console;
 
 static struct uart_driver liteuart_driver = {
 	.owner = THIS_MODULE,
-	.driver_name = "liteuart",
+	.driver_name = KBUILD_MODNAME,
 	.dev_name = "ttyLXU",
 	.major = 0,
 	.minor = 0,
@@ -322,7 +322,7 @@ static struct platform_driver liteuart_platform_driver = {
 	.probe = liteuart_probe,
 	.remove = liteuart_remove,
 	.driver = {
-		.name = "liteuart",
+		.name = KBUILD_MODNAME,
 		.of_match_table = liteuart_of_match,
 	},
 };
@@ -368,7 +368,7 @@ static int liteuart_console_setup(struct console *co, char *options)
 }
 
 static struct console liteuart_console = {
-	.name = "liteuart",
+	.name = KBUILD_MODNAME,
 	.write = liteuart_console_write,
 	.device = uart_console_device,
 	.setup = liteuart_console_setup,
-- 
2.37.3

[PATCH v3 02/14] serial: liteuart: use bit number macros

[Gabriel Somlo]

Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 32b81bd03d0c..1497d4cdc221 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -38,8 +38,8 @@
 #define OFF_EV_ENABLE	0x14
 
 /* events */
-#define EV_TX		0x1
-#define EV_RX		0x2
+#define EV_TX		BIT(0)
+#define EV_RX		BIT(1)
 
 struct liteuart_port {
 	struct uart_port port;
-- 
2.37.3

[PATCH v3 03/14] serial: liteuart: remove unused uart_ops stubs

[Gabriel Somlo]

Remove stub uart_ops methods that are not called unconditionally
from serial_core. Document stubs that are expected to be present.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 1497d4cdc221..90f6280c5452 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -122,6 +122,7 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
 
 static void liteuart_stop_tx(struct uart_port *port)
 {
+	/* not used in LiteUART, but called unconditionally from serial_core */
 }
 
 static void liteuart_start_tx(struct uart_port *port)
@@ -154,11 +155,6 @@ static void liteuart_stop_rx(struct uart_port *port)
 	del_timer(&uart->timer);
 }
 
-static void liteuart_break_ctl(struct uart_port *port, int break_state)
-{
-	/* LiteUART doesn't support sending break signal */
-}
-
 static int liteuart_startup(struct uart_port *port)
 {
 	struct liteuart_port *uart = to_liteuart_port(port);
@@ -197,15 +193,6 @@ static const char *liteuart_type(struct uart_port *port)
 	return "liteuart";
 }
 
-static void liteuart_release_port(struct uart_port *port)
-{
-}
-
-static int liteuart_request_port(struct uart_port *port)
-{
-	return 0;
-}
-
 static void liteuart_config_port(struct uart_port *port, int flags)
 {
 	/*
@@ -232,13 +219,10 @@ static const struct uart_ops liteuart_ops = {
 	.stop_tx	= liteuart_stop_tx,
 	.start_tx	= liteuart_start_tx,
 	.stop_rx	= liteuart_stop_rx,
-	.break_ctl	= liteuart_break_ctl,
 	.startup	= liteuart_startup,
 	.shutdown	= liteuart_shutdown,
 	.set_termios	= liteuart_set_termios,
 	.type		= liteuart_type,
-	.release_port	= liteuart_release_port,
-	.request_port	= liteuart_request_port,
 	.config_port	= liteuart_config_port,
 	.verify_port	= liteuart_verify_port,
 };
-- 
2.37.3

[PATCH v3 04/14] serial: liteuart: don't set unused port fields

[Gabriel Somlo]

Remove regshift and iobase port fields, since they are unused
by the driver.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 90f6280c5452..5b684fd198b7 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -264,9 +264,7 @@ static int liteuart_probe(struct platform_device *pdev)
 	port->iotype = UPIO_MEM;
 	port->flags = UPF_BOOT_AUTOCONF;
 	port->ops = &liteuart_ops;
-	port->regshift = 2;
 	port->fifosize = 16;
-	port->iobase = 1;
 	port->type = PORT_UNKNOWN;
 	port->line = dev_id;
 	spin_lock_init(&port->lock);
-- 
2.37.3

[PATCH v3 05/14] serial: liteuart: minor style fix in liteuart_init()

[Gabriel Somlo]

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 5b684fd198b7..047d5ad32e13 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -398,12 +398,10 @@ static int __init liteuart_init(void)
 		return res;
 
 	res = platform_driver_register(&liteuart_platform_driver);
-	if (res) {
+	if (res)
 		uart_unregister_driver(&liteuart_driver);
-		return res;
-	}
 
-	return 0;
+	return res;
 }
 
 static void __exit liteuart_exit(void)
-- 
2.37.3

[PATCH v3 06/14] serial: liteuart: move tty_flip_buffer_push() out of rx loop

[Gabriel Somlo]

Calling tty_flip_buffer_push() for each individual received character
is overkill. Move it out of the rx loop, and only call it once per
set of characters received together.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 047d5ad32e13..ff3486860989 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -86,10 +86,10 @@ static void liteuart_timer(struct timer_list *t)
 		/* no overflow bits in status */
 		if (!(uart_handle_sysrq_char(port, ch)))
 			uart_insert_char(port, status, 0, ch, flg);
-
-		tty_flip_buffer_push(&port->state->port);
 	}
 
+	tty_flip_buffer_push(&port->state->port);
+
 	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
 }
 
-- 
2.37.3

[PATCH v3 07/14] serial: liteuart: rx loop should only ack rx events

[Gabriel Somlo]

While receiving characters, it is necessary to acknowledge each one
by writing to the EV_PENDING register's EV_RX bit. Ensure we do not
also gratuitously set the EV_TX bit in the process.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index ff3486860989..b5ab48aa35cf 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -81,7 +81,7 @@ static void liteuart_timer(struct timer_list *t)
 		port->icount.rx++;
 
 		/* necessary for RXEMPTY to refresh its value */
-		litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
+		litex_write8(membase + OFF_EV_PENDING, EV_RX);
 
 		/* no overflow bits in status */
 		if (!(uart_handle_sysrq_char(port, ch)))
-- 
2.37.3

[PATCH v3 08/14] serial: liteuart: simplify passing of uart_insert_char() flag

[Gabriel Somlo]

Simply provide the hard-coded TTY_NORMAL flag to uart_insert_char()
directly -- no need to dedicate a variable for that exclusive purpose.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index b5ab48aa35cf..e9e99d6b5fef 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -72,7 +72,6 @@ static void liteuart_timer(struct timer_list *t)
 	struct liteuart_port *uart = from_timer(uart, t, timer);
 	struct uart_port *port = &uart->port;
 	unsigned char __iomem *membase = port->membase;
-	unsigned int flg = TTY_NORMAL;
 	int ch;
 	unsigned long status;
 
@@ -85,7 +84,7 @@ static void liteuart_timer(struct timer_list *t)
 
 		/* no overflow bits in status */
 		if (!(uart_handle_sysrq_char(port, ch)))
-			uart_insert_char(port, status, 0, ch, flg);
+			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
 	}
 
 	tty_flip_buffer_push(&port->state->port);
-- 
2.37.3

[PATCH v3 09/14] serial: liteuart: fix rx loop variable types

[Gabriel Somlo]

Update variable types to match the signature of uart_insert_char()
which consumes them.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index e9e99d6b5fef..974da0f73257 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -72,8 +72,7 @@ static void liteuart_timer(struct timer_list *t)
 	struct liteuart_port *uart = from_timer(uart, t, timer);
 	struct uart_port *port = &uart->port;
 	unsigned char __iomem *membase = port->membase;
-	int ch;
-	unsigned long status;
+	unsigned int status, ch;
 
 	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
 		ch = litex_read8(membase + OFF_RXTX);
-- 
2.37.3

[PATCH v3 10/14] serial: liteuart: separate rx loop from poll timer

[Gabriel Somlo]

Convert the rx loop into its own dedicated function, and (for now)
call it from the poll timer. This is in preparation for adding irq
support to the receive path.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 974da0f73257..172ac190ba2f 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -67,10 +67,8 @@ static struct uart_driver liteuart_driver = {
 #endif
 };
 
-static void liteuart_timer(struct timer_list *t)
+static void liteuart_rx_chars(struct uart_port *port)
 {
-	struct liteuart_port *uart = from_timer(uart, t, timer);
-	struct uart_port *port = &uart->port;
 	unsigned char __iomem *membase = port->membase;
 	unsigned int status, ch;
 
@@ -87,6 +85,14 @@ static void liteuart_timer(struct timer_list *t)
 	}
 
 	tty_flip_buffer_push(&port->state->port);
+}
+
+static void liteuart_timer(struct timer_list *t)
+{
+	struct liteuart_port *uart = from_timer(uart, t, timer);
+	struct uart_port *port = &uart->port;
+
+	liteuart_rx_chars(port);
 
 	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
 }
-- 
2.37.3

[PATCH v3 11/14] serial: liteuart: move function definitions

[Gabriel Somlo]

Move definitions for liteuart_[stop|start]_tx(), liteuart_stop_rx(),
and liteuart_putchar() to a more convenient location in preparation
for adding IRQ support. This patch contains no functional changes.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 98 +++++++++++++++++------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 172ac190ba2f..cf1ce597b45e 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -67,36 +67,6 @@ static struct uart_driver liteuart_driver = {
 #endif
 };
 
-static void liteuart_rx_chars(struct uart_port *port)
-{
-	unsigned char __iomem *membase = port->membase;
-	unsigned int status, ch;
-
-	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
-		ch = litex_read8(membase + OFF_RXTX);
-		port->icount.rx++;
-
-		/* necessary for RXEMPTY to refresh its value */
-		litex_write8(membase + OFF_EV_PENDING, EV_RX);
-
-		/* no overflow bits in status */
-		if (!(uart_handle_sysrq_char(port, ch)))
-			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
-	}
-
-	tty_flip_buffer_push(&port->state->port);
-}
-
-static void liteuart_timer(struct timer_list *t)
-{
-	struct liteuart_port *uart = from_timer(uart, t, timer);
-	struct uart_port *port = &uart->port;
-
-	liteuart_rx_chars(port);
-
-	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
-}
-
 static void liteuart_putchar(struct uart_port *port, unsigned char ch)
 {
 	while (litex_read8(port->membase + OFF_TXFULL))
@@ -105,25 +75,6 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
 	litex_write8(port->membase + OFF_RXTX, ch);
 }
 
-static unsigned int liteuart_tx_empty(struct uart_port *port)
-{
-	/* not really tx empty, just checking if tx is not full */
-	if (!litex_read8(port->membase + OFF_TXFULL))
-		return TIOCSER_TEMT;
-
-	return 0;
-}
-
-static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
-{
-	/* modem control register is not present in LiteUART */
-}
-
-static unsigned int liteuart_get_mctrl(struct uart_port *port)
-{
-	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
-}
-
 static void liteuart_stop_tx(struct uart_port *port)
 {
 	/* not used in LiteUART, but called unconditionally from serial_core */
@@ -159,6 +110,55 @@ static void liteuart_stop_rx(struct uart_port *port)
 	del_timer(&uart->timer);
 }
 
+static void liteuart_rx_chars(struct uart_port *port)
+{
+	unsigned char __iomem *membase = port->membase;
+	unsigned int status, ch;
+
+	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
+		ch = litex_read8(membase + OFF_RXTX);
+		port->icount.rx++;
+
+		/* necessary for RXEMPTY to refresh its value */
+		litex_write8(membase + OFF_EV_PENDING, EV_RX);
+
+		/* no overflow bits in status */
+		if (!(uart_handle_sysrq_char(port, ch)))
+			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
+	}
+
+	tty_flip_buffer_push(&port->state->port);
+}
+
+static void liteuart_timer(struct timer_list *t)
+{
+	struct liteuart_port *uart = from_timer(uart, t, timer);
+	struct uart_port *port = &uart->port;
+
+	liteuart_rx_chars(port);
+
+	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+}
+
+static unsigned int liteuart_tx_empty(struct uart_port *port)
+{
+	/* not really tx empty, just checking if tx is not full */
+	if (!litex_read8(port->membase + OFF_TXFULL))
+		return TIOCSER_TEMT;
+
+	return 0;
+}
+
+static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+	/* modem control register is not present in LiteUART */
+}
+
+static unsigned int liteuart_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
+}
+
 static int liteuart_startup(struct uart_port *port)
 {
 	struct liteuart_port *uart = to_liteuart_port(port);
-- 
2.37.3

[PATCH v3 12/14] serial: liteuart: add IRQ support for the RX path

[Gabriel Somlo]

Add support for IRQ-driven RX. Support for the TX path will be added
in a separate commit.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index cf1ce597b45e..e30adb30277f 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/console.h>
+#include <linux/interrupt.h>
 #include <linux/litex.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
 	tty_flip_buffer_push(&port->state->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;
+
+	spin_lock(&port->lock);
+	if (isr & EV_RX)
+		liteuart_rx_chars(port);
+	spin_unlock(&port->lock);
+
+	return IRQ_RETVAL(isr);
+}
+
 static void liteuart_timer(struct timer_list *t)
 {
 	struct liteuart_port *uart = from_timer(uart, t, timer);
 	struct uart_port *port = &uart->port;
 
-	liteuart_rx_chars(port);
-
+	liteuart_interrupt(0, port);
 	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
 }
 
@@ -162,19 +179,42 @@ 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);
+	int ret;
+	u8 irq_mask = 0;
 
-	/* disable events */
-	litex_write8(port->membase + OFF_EV_ENABLE, 0);
+	if (port->irq) {
+		ret = request_irq(port->irq, liteuart_interrupt, 0,
+				  KBUILD_MODNAME, uart);
+		if (ret == 0) {
+			/* only enable rx interrupts at this time */
+			irq_mask = EV_RX;
+		} else {
+			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
+				port->line, port->irq);
+			port->irq = 0;
+		}
+	}
 
-	/* prepare timer for polling */
-	timer_setup(&uart->timer, liteuart_timer, 0);
-	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+	if (!port->irq) {
+		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);
+
+	litex_write8(port->membase + OFF_EV_ENABLE, 0);
+
+	if (port->irq)
+		free_irq(port->irq, port);
+	else
+		del_timer_sync(&uart->timer);
 }
 
 static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
@@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
 		goto err_erase_id;
 	}
 
+	/* get irq */
+	ret = platform_get_irq_optional(pdev, 0);
+	if (ret < 0 && ret != -ENXIO)
+		return ret;
+	if (ret > 0)
+		port->irq = ret;
+
 	/* values not from device tree */
 	port->dev = &pdev->dev;
 	port->iotype = UPIO_MEM;
-- 
2.37.3

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

[Gabriel Somlo]

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

[PATCH v3 14/14] serial: liteuart: move polling putchar() function

[Gabriel Somlo]

The polling liteuart_putchar() function is only called from methods
conditionally enabled by CONFIG_SERIAL_LITEUART_CONSOLE. Move its
definition closer to the console code where it is dependent on the
same config option.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 drivers/tty/serial/liteuart.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 307c27398e30..767c356e60c9 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -69,14 +69,6 @@ static struct uart_driver liteuart_driver = {
 #endif
 };
 
-static void liteuart_putchar(struct uart_port *port, unsigned char ch)
-{
-	while (litex_read8(port->membase + OFF_TXFULL))
-		cpu_relax();
-
-	litex_write8(port->membase + OFF_RXTX, ch);
-}
-
 static void liteuart_stop_tx(struct uart_port *port)
 {
 	if (port->irq) {
@@ -389,6 +381,14 @@ static struct platform_driver liteuart_platform_driver = {
 
 #ifdef CONFIG_SERIAL_LITEUART_CONSOLE
 
+static void liteuart_putchar(struct uart_port *port, unsigned char ch)
+{
+	while (litex_read8(port->membase + OFF_TXFULL))
+		cpu_relax();
+
+	litex_write8(port->membase + OFF_RXTX, ch);
+}
+
 static void liteuart_console_write(struct console *co, const char *s,
 	unsigned int count)
 {
-- 
2.37.3

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

[Gabriel L. Somlo]

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 <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
> @@ -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/20221019091151.6692-20-ilpo.jarvinen@linux.intel.com/

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

Re: [PATCH v3 02/14] serial: liteuart: use bit number macros

[Ilpo Järvinen]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 32b81bd03d0c..1497d4cdc221 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -38,8 +38,8 @@
>  #define OFF_EV_ENABLE	0x14
>  
>  /* events */
> -#define EV_TX		0x1
> -#define EV_RX		0x2
> +#define EV_TX		BIT(0)
> +#define EV_RX		BIT(1)
>  
>  struct liteuart_port {
>  	struct uart_port port;
> 

This seems to assume some random header provides BIT() for you?

-- 
 i.

Re: [PATCH v3 03/14] serial: liteuart: remove unused uart_ops stubs

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Remove stub uart_ops methods that are not called unconditionally
> from serial_core. Document stubs that are expected to be present.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 1497d4cdc221..90f6280c5452 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -122,6 +122,7 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
>  
>  static void liteuart_stop_tx(struct uart_port *port)
>  {
> +	/* not used in LiteUART, but called unconditionally from serial_core */
>  }
>  
>  static void liteuart_start_tx(struct uart_port *port)
> @@ -154,11 +155,6 @@ static void liteuart_stop_rx(struct uart_port *port)
>  	del_timer(&uart->timer);
>  }
>  
> -static void liteuart_break_ctl(struct uart_port *port, int break_state)
> -{
> -	/* LiteUART doesn't support sending break signal */
> -}
> -
>  static int liteuart_startup(struct uart_port *port)
>  {
>  	struct liteuart_port *uart = to_liteuart_port(port);
> @@ -197,15 +193,6 @@ static const char *liteuart_type(struct uart_port *port)
>  	return "liteuart";
>  }
>  
> -static void liteuart_release_port(struct uart_port *port)
> -{
> -}
> -
> -static int liteuart_request_port(struct uart_port *port)
> -{
> -	return 0;
> -}
> -
>  static void liteuart_config_port(struct uart_port *port, int flags)
>  {
>  	/*
> @@ -232,13 +219,10 @@ static const struct uart_ops liteuart_ops = {
>  	.stop_tx	= liteuart_stop_tx,
>  	.start_tx	= liteuart_start_tx,
>  	.stop_rx	= liteuart_stop_rx,
> -	.break_ctl	= liteuart_break_ctl,
>  	.startup	= liteuart_startup,
>  	.shutdown	= liteuart_shutdown,
>  	.set_termios	= liteuart_set_termios,
>  	.type		= liteuart_type,
> -	.release_port	= liteuart_release_port,
> -	.request_port	= liteuart_request_port,
>  	.config_port	= liteuart_config_port,
>  	.verify_port	= liteuart_verify_port,
>  };
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v3 06/14] serial: liteuart: move tty_flip_buffer_push() out of rx loop

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Calling tty_flip_buffer_push() for each individual received character
> is overkill. Move it out of the rx loop, and only call it once per
> set of characters received together.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 047d5ad32e13..ff3486860989 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -86,10 +86,10 @@ static void liteuart_timer(struct timer_list *t)
>  		/* no overflow bits in status */
>  		if (!(uart_handle_sysrq_char(port, ch)))
>  			uart_insert_char(port, status, 0, ch, flg);
> -
> -		tty_flip_buffer_push(&port->state->port);
>  	}
>  
> +	tty_flip_buffer_push(&port->state->port);
> +
>  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 05/14] serial: liteuart: minor style fix in liteuart_init()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 5b684fd198b7..047d5ad32e13 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -398,12 +398,10 @@ static int __init liteuart_init(void)
>  		return res;
>  
>  	res = platform_driver_register(&liteuart_platform_driver);
> -	if (res) {
> +	if (res)
>  		uart_unregister_driver(&liteuart_driver);
> -		return res;
> -	}
>  
> -	return 0;
> +	return res;
>  }
>  
>  static void __exit liteuart_exit(void)

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v3 08/14] serial: liteuart: simplify passing of uart_insert_char() flag

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Simply provide the hard-coded TTY_NORMAL flag to uart_insert_char()
> directly -- no need to dedicate a variable for that exclusive purpose.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index b5ab48aa35cf..e9e99d6b5fef 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -72,7 +72,6 @@ static void liteuart_timer(struct timer_list *t)
>  	struct liteuart_port *uart = from_timer(uart, t, timer);
>  	struct uart_port *port = &uart->port;
>  	unsigned char __iomem *membase = port->membase;
> -	unsigned int flg = TTY_NORMAL;
>  	int ch;
>  	unsigned long status;
>  
> @@ -85,7 +84,7 @@ static void liteuart_timer(struct timer_list *t)
>  
>  		/* no overflow bits in status */
>  		if (!(uart_handle_sysrq_char(port, ch)))
> -			uart_insert_char(port, status, 0, ch, flg);
> +			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
>  	}
>  
>  	tty_flip_buffer_push(&port->state->port);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 10/14] serial: liteuart: separate rx loop from poll timer

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Convert the rx loop into its own dedicated function, and (for now)
> call it from the poll timer. This is in preparation for adding irq
> support to the receive path.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 974da0f73257..172ac190ba2f 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -67,10 +67,8 @@ static struct uart_driver liteuart_driver = {
>  #endif
>  };
>  
> -static void liteuart_timer(struct timer_list *t)
> +static void liteuart_rx_chars(struct uart_port *port)
>  {
> -	struct liteuart_port *uart = from_timer(uart, t, timer);
> -	struct uart_port *port = &uart->port;
>  	unsigned char __iomem *membase = port->membase;
>  	unsigned int status, ch;
>  
> @@ -87,6 +85,14 @@ static void liteuart_timer(struct timer_list *t)
>  	}
>  
>  	tty_flip_buffer_push(&port->state->port);
> +}
> +
> +static void liteuart_timer(struct timer_list *t)
> +{
> +	struct liteuart_port *uart = from_timer(uart, t, timer);
> +	struct uart_port *port = &uart->port;
> +
> +	liteuart_rx_chars(port);
>  
>  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v3 09/14] serial: liteuart: fix rx loop variable types

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Update variable types to match the signature of uart_insert_char()
> which consumes them.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index e9e99d6b5fef..974da0f73257 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -72,8 +72,7 @@ static void liteuart_timer(struct timer_list *t)
>  	struct liteuart_port *uart = from_timer(uart, t, timer);
>  	struct uart_port *port = &uart->port;
>  	unsigned char __iomem *membase = port->membase;
> -	int ch;
> -	unsigned long status;
> +	unsigned int status, ch;
>  
>  	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
>  		ch = litex_read8(membase + OFF_RXTX);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 11/14] serial: liteuart: move function definitions

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4213 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Move definitions for liteuart_[stop|start]_tx(), liteuart_stop_rx(),
> and liteuart_putchar() to a more convenient location in preparation
> for adding IRQ support. This patch contains no functional changes.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 98 +++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 172ac190ba2f..cf1ce597b45e 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -67,36 +67,6 @@ static struct uart_driver liteuart_driver = {
>  #endif
>  };
>  
> -static void liteuart_rx_chars(struct uart_port *port)
> -{
> -	unsigned char __iomem *membase = port->membase;
> -	unsigned int status, ch;
> -
> -	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> -		ch = litex_read8(membase + OFF_RXTX);
> -		port->icount.rx++;
> -
> -		/* necessary for RXEMPTY to refresh its value */
> -		litex_write8(membase + OFF_EV_PENDING, EV_RX);
> -
> -		/* no overflow bits in status */
> -		if (!(uart_handle_sysrq_char(port, ch)))
> -			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
> -	}
> -
> -	tty_flip_buffer_push(&port->state->port);
> -}
> -
> -static void liteuart_timer(struct timer_list *t)
> -{
> -	struct liteuart_port *uart = from_timer(uart, t, timer);
> -	struct uart_port *port = &uart->port;
> -
> -	liteuart_rx_chars(port);
> -
> -	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> -}
> -
>  static void liteuart_putchar(struct uart_port *port, unsigned char ch)
>  {
>  	while (litex_read8(port->membase + OFF_TXFULL))
> @@ -105,25 +75,6 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
>  	litex_write8(port->membase + OFF_RXTX, ch);
>  }
>  
> -static unsigned int liteuart_tx_empty(struct uart_port *port)
> -{
> -	/* not really tx empty, just checking if tx is not full */
> -	if (!litex_read8(port->membase + OFF_TXFULL))
> -		return TIOCSER_TEMT;
> -
> -	return 0;
> -}
> -
> -static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> -{
> -	/* modem control register is not present in LiteUART */
> -}
> -
> -static unsigned int liteuart_get_mctrl(struct uart_port *port)
> -{
> -	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> -}
> -
>  static void liteuart_stop_tx(struct uart_port *port)
>  {
>  	/* not used in LiteUART, but called unconditionally from serial_core */
> @@ -159,6 +110,55 @@ static void liteuart_stop_rx(struct uart_port *port)
>  	del_timer(&uart->timer);
>  }
>  
> +static void liteuart_rx_chars(struct uart_port *port)
> +{
> +	unsigned char __iomem *membase = port->membase;
> +	unsigned int status, ch;
> +
> +	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> +		ch = litex_read8(membase + OFF_RXTX);
> +		port->icount.rx++;
> +
> +		/* necessary for RXEMPTY to refresh its value */
> +		litex_write8(membase + OFF_EV_PENDING, EV_RX);
> +
> +		/* no overflow bits in status */
> +		if (!(uart_handle_sysrq_char(port, ch)))
> +			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
> +	}
> +
> +	tty_flip_buffer_push(&port->state->port);
> +}
> +
> +static void liteuart_timer(struct timer_list *t)
> +{
> +	struct liteuart_port *uart = from_timer(uart, t, timer);
> +	struct uart_port *port = &uart->port;
> +
> +	liteuart_rx_chars(port);
> +
> +	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> +}
> +
> +static unsigned int liteuart_tx_empty(struct uart_port *port)
> +{
> +	/* not really tx empty, just checking if tx is not full */
> +	if (!litex_read8(port->membase + OFF_TXFULL))
> +		return TIOCSER_TEMT;
> +
> +	return 0;
> +}
> +
> +static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +	/* modem control register is not present in LiteUART */
> +}
> +
> +static unsigned int liteuart_get_mctrl(struct uart_port *port)
> +{
> +	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> +}
> +
>  static int liteuart_startup(struct uart_port *port)
>  {
>  	struct liteuart_port *uart = to_liteuart_port(port);
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 02/14] serial: liteuart: use bit number macros

[Gabriel L. Somlo]

On Tue, Nov 15, 2022 at 05:33:47PM +0200, Ilpo Järvinen wrote:
> On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> 
> > Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.
> > 
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > ---
> >  drivers/tty/serial/liteuart.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 32b81bd03d0c..1497d4cdc221 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -38,8 +38,8 @@
> >  #define OFF_EV_ENABLE	0x14
> >  
> >  /* events */
> > -#define EV_TX		0x1
> > -#define EV_RX		0x2
> > +#define EV_TX		BIT(0)
> > +#define EV_RX		BIT(1)
> >  
> >  struct liteuart_port {
> >  	struct uart_port port;
> > 
> 
> This seems to assume some random header provides BIT() for you?

OK: version 4 of the series will have this commit explicitly
#include <linux/bits.h>, which right now is most likely brought
in implicitly by one or more of the existing headers.

Thanks,
--Gabriel

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

[Ilpo Järvinen]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> Add support for IRQ-driven RX. Support for the TX path will be added
> in a separate commit.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
>  1 file changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index cf1ce597b45e..e30adb30277f 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/console.h>
> +#include <linux/interrupt.h>
>  #include <linux/litex.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
>  	tty_flip_buffer_push(&port->state->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 */

Please don't add comment like this at all when your series removes it in a 
later patch.

> +	isr &= EV_RX;
> +
> +	spin_lock(&port->lock);
> +	if (isr & EV_RX)
> +		liteuart_rx_chars(port);
> +	spin_unlock(&port->lock);
> +
> +	return IRQ_RETVAL(isr);
> +}
> +
>  static void liteuart_timer(struct timer_list *t)
>  {
>  	struct liteuart_port *uart = from_timer(uart, t, timer);
>  	struct uart_port *port = &uart->port;
>  
> -	liteuart_rx_chars(port);
> -
> +	liteuart_interrupt(0, port);
>  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
>  }
>  
> @@ -162,19 +179,42 @@ 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);
> +	int ret;
> +	u8 irq_mask = 0;
>  
> -	/* disable events */
> -	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> +	if (port->irq) {
> +		ret = request_irq(port->irq, liteuart_interrupt, 0,
> +				  KBUILD_MODNAME, uart);
> +		if (ret == 0) {
> +			/* only enable rx interrupts at this time */

This comment seems pretty useless. Your code says very much the same.

-- 
 i.

> +			irq_mask = EV_RX;
> +		} else {
> +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> +				port->line, port->irq);
> +			port->irq = 0;
> +		}
> +	}
>  
> -	/* prepare timer for polling */
> -	timer_setup(&uart->timer, liteuart_timer, 0);
> -	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> +	if (!port->irq) {
> +		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);
> +
> +	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> +
> +	if (port->irq)
> +		free_irq(port->irq, port);
> +	else
> +		del_timer_sync(&uart->timer);
>  }
>  
>  static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> @@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
>  		goto err_erase_id;
>  	}
>  
> +	/* get irq */
> +	ret = platform_get_irq_optional(pdev, 0);
> +	if (ret < 0 && ret != -ENXIO)
> +		return ret;
> +	if (ret > 0)
> +		port->irq = ret;
> +
>  	/* values not from device tree */
>  	port->dev = &pdev->dev;
>  	port->iotype = UPIO_MEM;
> 

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

[Ilpo Järvinen]

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
> @@ -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.

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

[Gabriel L. Somlo]

On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo Järvinen wrote:
> On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> 
> > Add support for IRQ-driven RX. Support for the TX path will be added
> > in a separate commit.
> > 
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > ---
> >  drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> >  1 file changed, 54 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index cf1ce597b45e..e30adb30277f 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <linux/console.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/litex.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> >  	tty_flip_buffer_push(&port->state->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 */
> 
> Please don't add comment like this at all when your series removes it in a 
> later patch.

OK, I will remove it in v4.

> > +	isr &= EV_RX;
> > +
> > +	spin_lock(&port->lock);
> > +	if (isr & EV_RX)
> > +		liteuart_rx_chars(port);
> > +	spin_unlock(&port->lock);
> > +
> > +	return IRQ_RETVAL(isr);
> > +}
> > +
> >  static void liteuart_timer(struct timer_list *t)
> >  {
> >  	struct liteuart_port *uart = from_timer(uart, t, timer);
> >  	struct uart_port *port = &uart->port;
> >  
> > -	liteuart_rx_chars(port);
> > -
> > +	liteuart_interrupt(0, port);
> >  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> >  }
> >  
> > @@ -162,19 +179,42 @@ 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);
> > +	int ret;
> > +	u8 irq_mask = 0;
> >  
> > -	/* disable events */
> > -	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > +	if (port->irq) {
> > +		ret = request_irq(port->irq, liteuart_interrupt, 0,
> > +				  KBUILD_MODNAME, uart);
> > +		if (ret == 0) {
> > +			/* only enable rx interrupts at this time */
> 
> This comment seems pretty useless. Your code says very much the same.

The comment was meant to let the reader know that the code is doing it
*intentionally* (rather than forgetting to enable tx irqs by mistake).
But I'm OK with removing this comment in v4 as well if you think
that's an overly paranoid and redundant thing to do... :)

Thanks,
--Gabriel
 
> -- 
>  i.
> 
> > +			irq_mask = EV_RX;
> > +		} else {
> > +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> > +				port->line, port->irq);
> > +			port->irq = 0;
> > +		}
> > +	}
> >  
> > -	/* prepare timer for polling */
> > -	timer_setup(&uart->timer, liteuart_timer, 0);
> > -	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > +	if (!port->irq) {
> > +		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);
> > +
> > +	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > +
> > +	if (port->irq)
> > +		free_irq(port->irq, port);
> > +	else
> > +		del_timer_sync(&uart->timer);
> >  }
> >  
> >  static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> > @@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
> >  		goto err_erase_id;
> >  	}
> >  
> > +	/* get irq */
> > +	ret = platform_get_irq_optional(pdev, 0);
> > +	if (ret < 0 && ret != -ENXIO)
> > +		return ret;
> > +	if (ret > 0)
> > +		port->irq = ret;
> > +
> >  	/* values not from device tree */
> >  	port->dev = &pdev->dev;
> >  	port->iotype = UPIO_MEM;
> > 
> 

Re: [PATCH v3 14/14] serial: liteuart: move polling putchar() function

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

On Sat, 12 Nov 2022, Gabriel Somlo wrote:

> The polling liteuart_putchar() function is only called from methods
> conditionally enabled by CONFIG_SERIAL_LITEUART_CONSOLE. Move its
> definition closer to the console code where it is dependent on the
> same config option.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 307c27398e30..767c356e60c9 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -69,14 +69,6 @@ static struct uart_driver liteuart_driver = {
>  #endif
>  };
>  
> -static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> -{
> -	while (litex_read8(port->membase + OFF_TXFULL))
> -		cpu_relax();
> -
> -	litex_write8(port->membase + OFF_RXTX, ch);
> -}
> -
>  static void liteuart_stop_tx(struct uart_port *port)
>  {
>  	if (port->irq) {
> @@ -389,6 +381,14 @@ static struct platform_driver liteuart_platform_driver = {
>  
>  #ifdef CONFIG_SERIAL_LITEUART_CONSOLE
>  
> +static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> +{
> +	while (litex_read8(port->membase + OFF_TXFULL))
> +		cpu_relax();
> +
> +	litex_write8(port->membase + OFF_RXTX, ch);
> +}
> +
>  static void liteuart_console_write(struct console *co, const char *s,
>  	unsigned int count)
>  {

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:

> On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo Järvinen wrote:
> > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > 
> > > Add support for IRQ-driven RX. Support for the TX path will be added
> > > in a separate commit.
> > > 
> > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > > ---
> > >  drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> > >  1 file changed, 54 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > index cf1ce597b45e..e30adb30277f 100644
> > > --- a/drivers/tty/serial/liteuart.c
> > > +++ b/drivers/tty/serial/liteuart.c
> > > @@ -6,6 +6,7 @@
> > >   */
> > >  
> > >  #include <linux/console.h>
> > > +#include <linux/interrupt.h>
> > >  #include <linux/litex.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> > >  	tty_flip_buffer_push(&port->state->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 */
> > 
> > Please don't add comment like this at all when your series removes it in a 
> > later patch.
> 
> OK, I will remove it in v4.
> 
> > > +	isr &= EV_RX;
> > > +
> > > +	spin_lock(&port->lock);
> > > +	if (isr & EV_RX)
> > > +		liteuart_rx_chars(port);
> > > +	spin_unlock(&port->lock);
> > > +
> > > +	return IRQ_RETVAL(isr);
> > > +}
> > > +
> > >  static void liteuart_timer(struct timer_list *t)
> > >  {
> > >  	struct liteuart_port *uart = from_timer(uart, t, timer);
> > >  	struct uart_port *port = &uart->port;
> > >  
> > > -	liteuart_rx_chars(port);
> > > -
> > > +	liteuart_interrupt(0, port);
> > >  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > >  }
> > >  
> > > @@ -162,19 +179,42 @@ 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);
> > > +	int ret;
> > > +	u8 irq_mask = 0;
> > >  
> > > -	/* disable events */
> > > -	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > > +	if (port->irq) {
> > > +		ret = request_irq(port->irq, liteuart_interrupt, 0,
> > > +				  KBUILD_MODNAME, uart);
> > > +		if (ret == 0) {
> > > +			/* only enable rx interrupts at this time */
> > 
> > This comment seems pretty useless. Your code says very much the same.
> 
> The comment was meant to let the reader know that the code is doing it
> *intentionally* (rather than forgetting to enable tx irqs by mistake).
> But I'm OK with removing this comment in v4 as well if you think
> that's an overly paranoid and redundant thing to do... :)

I see. Reading the other comment first caused me to misinterpret this one 
to mean that only RX interrupts are implemented.

Maybe if you change "at this time" to "at startup" it would make it more 
obvious.

-- 
 i.

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

[Gabriel L. Somlo]

On Tue, Nov 15, 2022 at 06:21:00PM +0200, Ilpo Järvinen wrote:
> On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
> 
> > On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo Järvinen wrote:
> > > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > > 
> > > > Add support for IRQ-driven RX. Support for the TX path will be added
> > > > in a separate commit.
> > > > 
> > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > > > ---
> > > >  drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> > > >  1 file changed, 54 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > index cf1ce597b45e..e30adb30277f 100644
> > > > --- a/drivers/tty/serial/liteuart.c
> > > > +++ b/drivers/tty/serial/liteuart.c
> > > > @@ -6,6 +6,7 @@
> > > >   */
> > > >  
> > > >  #include <linux/console.h>
> > > > +#include <linux/interrupt.h>
> > > >  #include <linux/litex.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> > > >  	tty_flip_buffer_push(&port->state->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 */
> > > 
> > > Please don't add comment like this at all when your series removes it in a 
> > > later patch.
> > 
> > OK, I will remove it in v4.
> > 
> > > > +	isr &= EV_RX;
> > > > +
> > > > +	spin_lock(&port->lock);
> > > > +	if (isr & EV_RX)
> > > > +		liteuart_rx_chars(port);
> > > > +	spin_unlock(&port->lock);
> > > > +
> > > > +	return IRQ_RETVAL(isr);
> > > > +}
> > > > +
> > > >  static void liteuart_timer(struct timer_list *t)
> > > >  {
> > > >  	struct liteuart_port *uart = from_timer(uart, t, timer);
> > > >  	struct uart_port *port = &uart->port;
> > > >  
> > > > -	liteuart_rx_chars(port);
> > > > -
> > > > +	liteuart_interrupt(0, port);
> > > >  	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > > >  }
> > > >  
> > > > @@ -162,19 +179,42 @@ 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);
> > > > +	int ret;
> > > > +	u8 irq_mask = 0;
> > > >  
> > > > -	/* disable events */
> > > > -	litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > > > +	if (port->irq) {
> > > > +		ret = request_irq(port->irq, liteuart_interrupt, 0,
> > > > +				  KBUILD_MODNAME, uart);
> > > > +		if (ret == 0) {
> > > > +			/* only enable rx interrupts at this time */
> > > 
> > > This comment seems pretty useless. Your code says very much the same.
> > 
> > The comment was meant to let the reader know that the code is doing it
> > *intentionally* (rather than forgetting to enable tx irqs by mistake).
> > But I'm OK with removing this comment in v4 as well if you think
> > that's an overly paranoid and redundant thing to do... :)
> 
> I see. Reading the other comment first caused me to misinterpret this one 
> to mean that only RX interrupts are implemented.
> 
> Maybe if you change "at this time" to "at startup" it would make it more 
> obvious.
 
OK, I'll fix the comment per your suggestion rather than get rid of it.

Thanks again,
--G

> -- 
>  i.

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

[Gabriel L. Somlo]

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
> > @@ -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.
> 

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

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.

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

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

[Gabriel L. Somlo]

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.

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

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

[Gabriel L. Somlo]

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5753 bytes --]

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

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);
>